stoplightio / spectral

A flexible JSON/YAML linter for creating automated style guides, with baked in support for OpenAPI (v3.1, v3.0, and v2.0), Arazzo v1.0, as well as AsyncAPI v2.x.
https://stoplight.io/spectral
Apache License 2.0
2.53k stars 241 forks source link

Default Resolvers Not Resolving Relative to Given Document #1314

Closed dillonredding closed 1 year ago

dillonredding commented 4 years ago

Describe the bug The default resolvers don't function as documented.

By default, http(s) and file protocols are resolved, relatively to the document Spectral lints against. [Emphasis mine]

However, when linting with a new Spectral() object, the file $refs are resolved relative to where the script runs, not the file being linted.

To Reproduce

C:/api/openapi.yaml:

openapi: 3.0.3
info:
  title: Test API
  version: 1.0.0
paths:
  /test:
    get:
      responses:
        '200':
          $ref: components.yaml#/Ok

C:/api/components.yaml:

Ok:
  description: Success

C:/dev/script.ts:

import fs from 'fs';
import YAML from 'js-yaml';
import { isOpenApiv3, Spectral } from '@stoplight/spectral';
import { DiagnosticSeverity } from '@stoplight/types';

const openApi = fs.readFileSync('C:/api/openapi.yaml', 'utf-8');

const spectral = new Spectral();
spectral.registerFormat('oas3', isOpenApiv3);
spectral.run(YAML.safeLoad(openApi) as string)
    .then(results => {
        results.filter(result => result.severity === DiagnosticSeverity.Error)
            .forEach(result => console.log(result.message))
    });
$ ts-node C:\dev\script.ts
ENOENT: no such file or directory, open 'C:\dev\components.yaml'

Expected behavior Given the OpenAPI document and script above, there should be no errors and therefore no output. Spectral's default resolvers should be looking for C:/api/components.yaml, not C:/dev/components.yaml.

Environment:

jftanner commented 4 years ago

I ran unto a similar problem and found a solution. It seems you can specify the filename so that it loads and resolves references correctly. The following code seems to work:

const {promises: fs} = require('fs');
const path = require('path');
const { Spectral, Document, Parsers, isOpenApiv3 } = require('@stoplight/spectral');

// Load the API Spec as a string
const filepath = path.join(__dirname, '../api/spec.yaml'); // TODO Adjust per your own path.
const contents = await fs.readFile(filepath, 'utf8');

// Create a spectral document from it.
const apiSpec = new Document(contents, Parsers.Yaml, filepath);

// Configure spectral to lint OAS3.
const spectral = new Spectral();
spectral.registerFormat('oas3', isOpenApiv3);
await spectral.loadRuleset('spectral:oas');

// Lint the API Spec
await spectral.run(apiSpec).then(results => {
        // TODO Whatever you like with the results.
});

(Note: This is a truncated snippet from real code. So you'd obviously need to have it in the appropriate async contexts for the await to work. But you get the idea.)

Since it can load other files, it seems strange that I have to load the first one manually. But maybe I'm missing something. If there's a better way, I'd love to hear it.

dillonredding commented 4 years ago

So, the difference is really passing it an "OpenAPI object":

YAML.safeLoad(openApi)

and wrapping the raw contents in a Document:

new Document(contents, Parsers.Yaml, filepath)

The following changes work fine with my original snippet:

spectral.run(new Document(openApi, Parsers.Yaml, 'C:/api/openapi.yaml'))

While the third constructor argument is optional, it's necessary to get around the problem. This makes sense since Spectral wouldn't know where the document lives without it. However, I still believe the documentation should be updated to clarify this.

nulltoken commented 4 years ago

However, I still believe the documentation should be updated to clarify this.

@dillonredding Would you feel like helping us improve the documentation through a pull request?

dillonredding commented 4 years ago

@nulltoken Absolutely! You think the third constructor argument for Document should be mentioned here? Maybe mention the lack of relative resolution here? I don't know that these things are relevant to the originally mentioned Using a Custom Resolver section.

nulltoken commented 4 years ago

Absolutely!

Awesome!

I don't know that these things are relevant to the originally mentioned Using a Custom Resolver section

You're right. I don't think there's much we can do over there.

You think the third constructor argument for Document should be mentioned here? Maybe mention the lack of relative resolution here?

Or maybe just warnings regarding that the provided documents should be complete standalone ones (or that the resolution for relative paths would occur according to the current working directory (which location is quite unpredictable)).

Additionally, maybe could you add a new section dedicated to linting files where those absolute/relative path concerns would absolutely shine!

dillonredding commented 4 years ago

Additionally, maybe could you add a new section dedicated to linting files where those absolute/relative path concerns would absolutely shine!

That's not a bad idea! Maybe between the "Linting an Object" and "Registering Formats" sections?

Or maybe just warnings regarding that the provided documents should be complete standalone ones (or that the resolution for relative paths would occur according to the current working directory (which location is quite unpredictable)).

Just want to clarify, by "warning", you mean like the blockquote here (with the theme: warning comment)? I could definitely see that being helpful in "Linting a YAML String" and "Linting an Object" sections and just point to the new section.

nulltoken commented 4 years ago

That's not a bad idea! Maybe between the "Linting an Object" and "Registering Formats" sections?

@dillonredding That would be great location.

Just want to clarify, by "warning", you mean like the blockquote here (with the theme: warning comment)?

Indeed. This would bring some attention from the reader ;-)