Repository: stoplightio/spectral
Pull request #228 (2019-05-24 13:15:11)

fix: invalid-ref error
### Please check if the PR fulfills these requirements
- [x] Tests for the changes have been added (for bug fixes / features)
- [ ] Docs have been added / updated (for bug fixes / features)

### What kind of change does this PR introduce?

Feature.

### What is the current behavior?

At the moment, we don't report any issues related to invalid (missing) $refs.
Moreover, `invalid-example` rule bails out entirely if a schema points to incorrect $ref, as we don't handle the exception.

### If this is a feature change, what is the new behavior?

invalid-ref error + improved handling of ajv missing ref errors.

### Does this PR introduce a breaking change?

I'd say it does.

What changes might users need to make in their application due to this PR?

### Other information

(e.g. detailed explanation, related issues, links for us to have context, eg. stackoverflow, issues outside of the repo, forum, etc.)



Patch:
diff --git a/src/functions/schema.ts b/src/functions/schema.ts
index b5cc4e8c..a1110338 100644
--- a/src/functions/schema.ts
+++ b/src/functions/schema.ts
@@ -57,32 +57,43 @@ export const schema: IFunction<ISchemaOptions> = (targetVal, opts, paths) => {
 
   const { schema: schemaObj } = opts;
 
-  if (!ajv.validate(schemaObj, targetVal) && ajv.errors) {
-    // TODO: potential performance improvements (compile, etc)?
-    const collectedErrors: string[] = [];
+  try {
+    if (!ajv.validate(schemaObj, targetVal) && ajv.errors) {
+      // TODO: potential performance improvements (compile, etc)?
+      const collectedErrors: string[] = [];
 
-    for (const error of ajv.errors) {
-      if (collectedErrors.length > 0) {
-        const index = collectedErrors.indexOf(error.keyword);
-        if (index !== -1) {
-          if (mergeErrors(results[index], error)) continue;
+      for (const error of ajv.errors) {
+        if (collectedErrors.length > 0) {
+          const index = collectedErrors.indexOf(error.keyword);
+          if (index !== -1) {
+            if (mergeErrors(results[index], error)) continue;
+          }
         }
-      }
 
-      let message = error.message || '';
+        let message = error.message || '';
 
-      if (
-        error.keyword === 'additionalProperties' &&
-        (error.params as AJV.AdditionalPropertiesParams).additionalProperty
-      ) {
-        message += `: ${(error.params as AJV.AdditionalPropertiesParams).additionalProperty}`;
-      }
+        if (
+          error.keyword === 'additionalProperties' &&
+          (error.params as AJV.AdditionalPropertiesParams).additionalProperty
+        ) {
+          message += `: ${(error.params as AJV.AdditionalPropertiesParams).additionalProperty}`;
+        }
 
-      collectedErrors.push(error.keyword);
+        collectedErrors.push(error.keyword);
+        results.push({
+          path: [...path, ...formatPath(error.dataPath)],
+          message,
+        });
+      }
+    }
+  } catch (ex) {
+    if (ex instanceof AJV.MissingRefError) {
       results.push({
-        path: [...path, ...formatPath(error.dataPath)],
-        message,
+        message: ex.message,
+        path,
       });
+    } else {
+      throw ex;
     }
   }
 
diff --git a/src/spectral.ts b/src/spectral.ts
index 48869644..aa68713c 100644
--- a/src/spectral.ts
+++ b/src/spectral.ts
@@ -1,10 +1,10 @@
 import { safeStringify } from '@stoplight/json';
 import { Resolver } from '@stoplight/json-ref-resolver';
+import { IResolveError } from '@stoplight/json-ref-resolver/types';
+import { DiagnosticSeverity, IParserResult } from '@stoplight/types';
 import { getLocationForJsonPath, parseWithPointers } from '@stoplight/yaml';
+import { merge, uniqBy } from 'lodash';
 
-import { merge } from 'lodash';
-
-import { IParserResult } from '@stoplight/types';
 import { functions as defaultFunctions } from './functions';
 import { runRules } from './runner';
 import {
@@ -24,7 +24,7 @@ export class Spectral {
   private _rules: RuleCollection = {};
   private _functions: FunctionCollection = defaultFunctions;
 
-  private resolver: any;
+  private resolver: Resolver;
   constructor(opts?: IConstructorOpts) {
     this.resolver = opts && opts.resolver ? opts.resolver : new Resolver();
   }
@@ -43,9 +43,13 @@ export class Spectral {
       parsedResult = target;
     }
 
-    const resolvedTarget = (await this.resolver.resolve(parsedResult.parsed.data)).result;
+    const { result: resolvedTarget, errors } = await this.resolver.resolve(parsedResult.parsed.data);
 
-    return results.concat(runRules(parsedResult, this.rules, this.functions, { resolvedTarget }));
+    return [
+      ...results,
+      ...formatResolverErrors(errors, parsedResult),
+      ...runRules(parsedResult, this.rules, this.functions, { resolvedTarget }),
+    ];
   }
 
   /**
@@ -124,3 +128,24 @@ function formatParserDiagnostics(parsed: IParserResult, source?: string): IRuleR
     source,
   }));
 }
+
+const prettyPrintResolverError = (message: string) => message.replace(/^Error\s*:\s*/, '');
+
+const formatResolverErrors = (resolveErrors: IResolveError[], result: IParsedResult): IRuleResult[] => {
+  return uniqBy(resolveErrors, 'message').reduce<IRuleResult[]>((errors, error) => {
+    const path = [...error.path, '$ref'];
+    const location = result.getLocationForJsonPath(result.parsed, path);
+
+    if (location) {
+      errors.push({
+        code: 'invalid-ref',
+        path,
+        message: prettyPrintResolverError(error.message),
+        severity: DiagnosticSeverity.Error,
+        range: location.range,
+      });
+    }
+
+    return errors;
+  }, []);
+};
diff --git a/src/types/spectral.ts b/src/types/spectral.ts
index 745c00e4..1d0d05c5 100644
--- a/src/types/spectral.ts
+++ b/src/types/spectral.ts
@@ -1,5 +1,6 @@
 import { Dictionary, GetLocationForJsonPath, IDiagnostic, IParserResult, JsonPath } from '@stoplight/types';
 
+import { Resolver } from '@stoplight/json-ref-resolver';
 import { IFunction } from './function';
 import { IRule, Rule } from './rule';
 
@@ -20,7 +21,7 @@ export interface IRunRule extends IRule {
 export type RuleDeclarationCollection = Dictionary<boolean, string>;
 
 export interface IConstructorOpts {
-  resolver?: object;
+  resolver?: Resolver;
 }
 
 export interface IRunOpts {


Test patch:
diff --git a/src/__tests__/__fixtures__/todos.invalid.oas2.json b/src/__tests__/__fixtures__/todos.invalid.oas2.json
index 2b6d1498..4051ebcb 100644
--- a/src/__tests__/__fixtures__/todos.invalid.oas2.json
+++ b/src/__tests__/__fixtures__/todos.invalid.oas2.json
@@ -79,6 +79,14 @@
                 "completed": false
               }
             }
+          },
+          {
+            "name": "missing",
+            "in": "query",
+            "schema": {
+              "$ref": "#/parameters/missing",
+              "example": "test"
+            }
           }
         ],
         "responses": {
diff --git a/src/__tests__/linter.test.ts b/src/__tests__/linter.test.ts
index 3ade2062..5abc9568 100644
--- a/src/__tests__/linter.test.ts
+++ b/src/__tests__/linter.test.ts
@@ -12,6 +12,7 @@ const invalidSchema = fs.readFileSync(
   path.join(__dirname, './__fixtures__/petstore.invalid-schema.oas3.yaml'),
   'utf-8',
 );
+const todosInvalid = fs.readFileSync(path.join(__dirname, './__fixtures__/todos.invalid.oas2.json'), 'utf-8');
 
 const fnName = 'fake';
 const fnName2 = 'fake2';
@@ -205,26 +206,69 @@ responses:: !!foo
 
     const result = await spectral.run(invalidSchema);
 
-    expect(result).toEqual([
-      expect.objectContaining({
-        code: 'oas3-schema',
-        message: 'should NOT have additional properties: type',
-        summary: 'should NOT have additional properties: type',
-        path: ['paths', '/pets', 'get', 'responses', '200', 'headers', 'header-1'],
-      }),
-      expect.objectContaining({
-        code: 'oas3-schema',
-        message: 'should match exactly one schema in oneOf',
-        summary: 'should match exactly one schema in oneOf',
-        path: ['paths', '/pets', 'get', 'responses', '200', 'headers', 'header-1'],
-      }),
-      expect.objectContaining({
-        code: 'oas3-schema',
-        message: "should have required property '$ref'",
-        summary: "should have required property '$ref'",
-        path: ['paths', '/pets', 'get', 'responses', '200', 'headers', 'header-1'],
-      }),
-    ]);
+    expect(result).toEqual(
+      expect.arrayContaining([
+        expect.objectContaining({
+          code: 'oas3-schema',
+          message: 'should NOT have additional properties: type',
+          summary: 'should NOT have additional properties: type',
+          path: ['paths', '/pets', 'get', 'responses', '200', 'headers', 'header-1'],
+        }),
+        expect.objectContaining({
+          code: 'oas3-schema',
+          message: 'should match exactly one schema in oneOf',
+          summary: 'should match exactly one schema in oneOf',
+          path: ['paths', '/pets', 'get', 'responses', '200', 'headers', 'header-1'],
+        }),
+        expect.objectContaining({
+          code: 'oas3-schema',
+          message: "should have required property '$ref'",
+          summary: "should have required property '$ref'",
+          path: ['paths', '/pets', 'get', 'responses', '200', 'headers', 'header-1'],
+        }),
+      ]),
+    );
+  });
+
+  test('should report invalid schema $refs', async () => {
+    spectral.addRules(oas3Ruleset.rules as RuleCollection);
+    spectral.addFunctions(oas3Functions());
+
+    const result = await spectral.run(todosInvalid);
+
+    expect(result).toEqual(
+      expect.arrayContaining([
+        expect.objectContaining({
+          code: 'valid-example',
+          message: '"schema" property can\'t resolve reference #/parameters/missing from id #',
+          path: ['paths', '/todos/{todoId}', 'put', 'parameters', 1, 'schema'],
+        }),
+      ]),
+    );
+  });
+
+  test('should report invalid $refs', async () => {
+    spectral.addRules(oas3Ruleset.rules as RuleCollection);
+    spectral.addFunctions(oas3Functions());
+
+    const result = await spectral.run(invalidSchema);
+
+    expect(result).toEqual(
+      expect.arrayContaining([
+        expect.objectContaining({
+          code: 'invalid-ref',
+          message: "No reader defined for scheme 'file' in ref file://models/pet.yaml",
+          path: ['paths', '/pets', 'get', 'responses', '200', 'content', 'application/json', 'schema', '$ref'],
+          severity: DiagnosticSeverity.Error,
+        }),
+        expect.objectContaining({
+          code: 'invalid-ref',
+          message: "No reader defined for scheme 'file' in ref file://../common/models/error.yaml",
+          path: ['paths', '/pets', 'get', 'responses', 'default', 'content', 'application/json', 'schema', '$ref'],
+          severity: DiagnosticSeverity.Error,
+        }),
+      ]),
+    );
   });
 
   describe('functional tests for the given property', () => {
diff --git a/src/__tests__/spectral.test.ts b/src/__tests__/spectral.test.ts
index e800f962..43923579 100644
--- a/src/__tests__/spectral.test.ts
+++ b/src/__tests__/spectral.test.ts
@@ -104,7 +104,7 @@ describe('spectral', () => {
       };
 
       const s = new Spectral({
-        resolver: fakeResolver,
+        resolver: fakeResolver as any,
       });
 
       const target = { foo: 'bar' };
diff --git a/src/rulesets/oas/__tests__/__snapshots__/only-local-references.ts.snap b/src/rulesets/oas/__tests__/__snapshots__/only-local-references.ts.snap
deleted file mode 100644
index e96e443f..00000000
--- a/src/rulesets/oas/__tests__/__snapshots__/only-local-references.ts.snap
+++ /dev/null
@@ -1,28 +0,0 @@
-// Jest Snapshot v1, https://goo.gl/fbAQLP
-
-exports[`only-local-references return errors if not local ref 1`] = `
-Array [
-  Object {
-    "code": "only-local-references",
-    "message": "References should start with \`#/\`.",
-    "path": Array [
-      "parameters",
-      0,
-      "$ref",
-    ],
-    "range": Object {
-      "end": Object {
-        "character": 52,
-        "line": 5,
-      },
-      "start": Object {
-        "character": 14,
-        "line": 5,
-      },
-    },
-    "severity": 1,
-    "source": undefined,
-    "summary": "References should start with \`#/\`.",
-  },
-]
-`;
diff --git a/src/rulesets/oas/__tests__/only-local-references.ts b/src/rulesets/oas/__tests__/only-local-references.ts
index ab6e8972..89c242db 100644
--- a/src/rulesets/oas/__tests__/only-local-references.ts
+++ b/src/rulesets/oas/__tests__/only-local-references.ts
@@ -1,3 +1,4 @@
+import { DiagnosticSeverity } from '@stoplight/types';
 import { RuleType, Spectral } from '../../../spectral';
 import * as ruleset from '../ruleset.json';
 
@@ -16,7 +17,11 @@ describe('only-local-references', () => {
       paths: {},
       parameters: [{ $ref: '#/reference/path' }],
     });
-    expect(results.length).toEqual(0);
+    expect(results).toEqual([
+      expect.objectContaining({
+        code: 'invalid-ref',
+      }),
+    ]);
   });
 
   test('return errors if not local ref', async () => {
@@ -25,6 +30,14 @@ describe('only-local-references', () => {
       paths: {},
       parameters: [{ $ref: 'https://stoplight.io#/reference/path' }],
     });
-    expect(results).toMatchSnapshot();
+    expect(results).toEqual(
+      expect.arrayContaining([
+        expect.objectContaining({
+          code: 'only-local-references',
+          message: 'References should start with `#/`.',
+          severity: DiagnosticSeverity.Warning,
+        }),
+      ]),
+    );
   });
 });
diff --git a/src/rulesets/oas/__tests__/parameter-description.ts b/src/rulesets/oas/__tests__/parameter-description.ts
index 4386f33b..5992c678 100644
--- a/src/rulesets/oas/__tests__/parameter-description.ts
+++ b/src/rulesets/oas/__tests__/parameter-description.ts
@@ -139,19 +139,20 @@ describe('parameter-description', () => {
     ]);
   });
 
-  test('does not throw on refs', async () => {
-    const results = await s.run({
-      swagger: '2.0',
-      paths: {
-        '/todos': {
-          parameters: [
-            {
-              $ref: '#/parameters/limit',
-            },
-          ],
+  test('does not throw on refs', () => {
+    return expect(
+      s.run({
+        swagger: '2.0',
+        paths: {
+          '/todos': {
+            parameters: [
+              {
+                $ref: '#/parameters/limit',
+              },
+            ],
+          },
         },
-      },
-    });
-    expect(results.length).toEqual(0);
+      }),
+    ).not.rejects;
   });
 });