Repository: stoplightio/spectral
Pull request #839 (2019-12-07 20:37:00)

fix: absolute paths
Fixes https://github.com/stoplightio/spectral/issues/608 and https://github.com/stoplightio/spectral/pull/685#issuecomment-562538996

**Checklist**

- [x] Tests added / updated
- [ ] Docs added / updated

**Does this PR introduce a breaking change?**

- [ ] Yes
- [x] No


**Additional context**

From now on, Spectral will never try to generate paths that do not point to any value in the document.
Previously, we used to produce paths that did exist in the resolved document, but not necessarily in the actual unresolved document.
This is particularly crucial for tooling which mostly works with resolved documents, therefore need to operate on accurate paths. In the former specification, we simply threw a path pointing to a place in a resolved document.
https://github.com/stoplightio/spectral/pull/839/files#diff-62b31a98be4036e65207ec7971253e93 - this can be easily observed over here. The actual error was caused in the referenced file, which does not have any paths object with responses.


Patch:
diff --git a/src/linter.ts b/src/linter.ts
index b0724445..8a5e8708 100644
--- a/src/linter.ts
+++ b/src/linter.ts
@@ -1,14 +1,13 @@
-import { JsonPath } from '@stoplight/types';
-import { get, has } from 'lodash';
+import { get, isObject } from 'lodash';
 
 const { JSONPath } = require('jsonpath-plus');
 
 import { decodePointerFragment, pathToPointer } from '@stoplight/json';
-import { Resolved } from './resolved';
-import { message } from './rulesets/message';
+import { getDefaultRange, Resolved } from './resolved';
+import { IMessageVars, message } from './rulesets/message';
 import { getDiagnosticSeverity } from './rulesets/severity';
 import { IFunction, IGivenNode, IRuleResult, IRunRule, IThen } from './types';
-import { isObject } from './utils';
+import { getClosestJsonPath } from './utils';
 
 // TODO(SO-23): unit test but mock whatShouldBeLinted
 export const lintNode = (
@@ -91,38 +90,37 @@ export const lintNode = (
     results.push(
       ...targetResults.map<IRuleResult>(result => {
         const escapedJsonPath = (result.path || targetPath).map(segment => decodePointerFragment(String(segment)));
-        const path = getClosestJsonPath(
-          rule.resolved === false ? resolved.unresolved : resolved.resolved,
-          escapedJsonPath,
-        );
-        // todo: https://github.com/stoplightio/spectral/issues/608
-        const location = resolved.getLocationForJsonPath(path, true);
+        const parsed = resolved.getParsedForJsonPath(escapedJsonPath);
+        const path = parsed?.path || getClosestJsonPath(resolved.resolved, escapedJsonPath);
+        const doc = parsed?.doc || resolved.parsed;
+        const range = doc.getLocationForJsonPath(doc.parsed, path, true)?.range || getDefaultRange();
+
+        const vars: IMessageVars = {
+          property: path.length > 0 ? path[path.length - 1] : '',
+          path: pathToPointer(path),
+          error: result.message,
+          description: rule.description,
+          get value() {
+            // let's make `value` lazy
+            const value = get(parsed?.doc.parsed.data, path);
+            if (isObject(value)) {
+              return Array.isArray(value) ? 'Array[]' : 'Object{}';
+            }
+
+            return JSON.stringify(value);
+          },
+        };
+
+        const resultMessage = message(result.message, vars);
+        vars.error = resultMessage;
 
         return {
           code: rule.name,
-
-          message:
-            rule.message === undefined
-              ? rule.description || result.message
-              : message(rule.message, {
-                  error: result.message,
-                  property: path.length > 0 ? path[path.length - 1] : '',
-                  path: pathToPointer(path),
-                  description: rule.description,
-                  get value() {
-                    // let's make it `value` lazy
-                    const value = resolved.getValueForJsonPath(path);
-                    if (isObject(value)) {
-                      return Array.isArray(value) ? 'Array[]' : 'Object{}';
-                    }
-
-                    return JSON.stringify(value);
-                  },
-                }),
+          message: rule.message === void 0 ? rule.description || resultMessage : message(rule.message, vars),
           path,
           severity: getDiagnosticSeverity(rule.severity),
-          source: location.uri,
-          range: location.range,
+          source: parsed?.doc.source,
+          range,
         };
       }),
     );
@@ -130,14 +128,3 @@ export const lintNode = (
 
   return results;
 };
-
-// todo: revisit -> https://github.com/stoplightio/spectral/issues/608
-function getClosestJsonPath(data: unknown, path: JsonPath) {
-  if (!isObject(data)) return [];
-
-  while (path.length > 0 && !has(data, path)) {
-    path.pop();
-  }
-
-  return path;
-}
diff --git a/src/resolved.ts b/src/resolved.ts
index 8e1ac820..8aeaaae9 100644
--- a/src/resolved.ts
+++ b/src/resolved.ts
@@ -1,13 +1,13 @@
-import { extractSourceFromRef, hasRef, isLocalRef, pointerToPath } from '@stoplight/json';
+import { extractSourceFromRef, hasRef, isLocalRef } from '@stoplight/json';
 import { IGraphNodeData, IResolveError } from '@stoplight/json-ref-resolver/types';
 import { normalize, resolve } from '@stoplight/path';
 import { Dictionary, ILocation, IRange, JsonPath } from '@stoplight/types';
 import { DepGraph } from 'dependency-graph';
 import { get } from 'lodash';
 import { IParsedResult, ResolveResult } from './types';
-import { getEndRef, isAbsoluteRef, safePointerToPath, traverseObjUntilRef } from './utils';
+import { getClosestJsonPath, getEndRef, isAbsoluteRef, safePointerToPath, traverseObjUntilRef } from './utils';
 
-const getDefaultRange = (): IRange => ({
+export const getDefaultRange = (): IRange => ({
   start: {
     line: 0,
     character: 0,
@@ -31,7 +31,7 @@ export class Resolved {
   }
 
   constructor(
-    protected parsed: IParsedResult,
+    public readonly parsed: IParsedResult,
     resolveResult: ResolveResult,
     public parsedRefs: Dictionary<IParsedResult>,
   ) {
@@ -46,16 +46,20 @@ export class Resolved {
 
   public getParsedForJsonPath(path: JsonPath) {
     try {
-      const newPath: JsonPath = [...path];
+      const newPath: JsonPath = getClosestJsonPath(this.resolved, path);
       let $ref = traverseObjUntilRef(this.unresolved, newPath);
 
       if ($ref === null) {
         return {
-          path,
+          path: getClosestJsonPath(this.unresolved, path),
           doc: this.parsed,
+          missingPropertyPath: path,
         };
       }
 
+      const missingPropertyPath =
+        newPath.length === 0 ? [] : path.slice(path.lastIndexOf(newPath[newPath.length - 1]) + 1);
+
       let { source } = this;
 
       while (true) {
@@ -65,30 +69,32 @@ export class Resolved {
 
         if ($ref === null) return null;
 
+        const scopedPath = [...safePointerToPath($ref), ...newPath];
+        let resolvedDoc;
+
         if (isLocalRef($ref)) {
-          return {
-            path: pointerToPath($ref),
-            doc: source === this.parsed.source ? this.parsed : this.parsedRefs[source],
-          };
-        }
+          resolvedDoc = source === this.parsed.source ? this.parsed : this.parsedRefs[source];
+        } else {
+          const extractedSource = extractSourceFromRef($ref)!;
+          source = isAbsoluteRef(extractedSource) ? extractedSource : resolve(source, '..', extractedSource);
 
-        const extractedSource = extractSourceFromRef($ref)!;
-        source = isAbsoluteRef(extractedSource) ? extractedSource : resolve(source, '..', extractedSource);
+          resolvedDoc = source === this.parsed.source ? this.parsed : this.parsedRefs[source];
+          const { parsed } = resolvedDoc;
 
-        const doc = source === this.parsed.source ? this.parsed : this.parsedRefs[source];
-        const { parsed } = doc;
-        const scopedPath = [...safePointerToPath($ref), ...newPath];
+          const obj = scopedPath.length === 0 || hasRef(parsed.data) ? parsed.data : get(parsed.data, scopedPath);
 
-        const obj = scopedPath.length === 0 || hasRef(parsed.data) ? parsed.data : get(parsed.data, scopedPath);
-
-        if (hasRef(obj)) {
-          $ref = obj.$ref;
-        } else {
-          return {
-            doc,
-            path: scopedPath,
-          };
+          if (hasRef(obj)) {
+            $ref = obj.$ref;
+            continue;
+          }
         }
+
+        const closestPath = getClosestJsonPath(resolvedDoc.parsed.data, scopedPath);
+        return {
+          doc: resolvedDoc,
+          path: closestPath,
+          missingPropertyPath: [...closestPath, ...missingPropertyPath],
+        };
       }
     } catch {
       return null;
diff --git a/src/rulesets/message.ts b/src/rulesets/message.ts
index cece5596..6475bf77 100644
--- a/src/rulesets/message.ts
+++ b/src/rulesets/message.ts
@@ -6,6 +6,7 @@ export interface IMessageVars {
   description?: string;
   value: unknown;
   path: string;
+  fullPath?: string;
 }
 
 export type MessageInterpolator = (str: string, values: IMessageVars) => string;
diff --git a/src/utils/refs.ts b/src/utils/refs.ts
index 03935be6..fbc0f023 100644
--- a/src/utils/refs.ts
+++ b/src/utils/refs.ts
@@ -43,3 +43,20 @@ export const safePointerToPath = (pointer: string): JsonPath => {
   const rawPointer = extractPointerFromRef(pointer);
   return rawPointer ? pointerToPath(rawPointer) : [];
 };
+
+export const getClosestJsonPath = (data: unknown, path: JsonPath) => {
+  const closestPath: JsonPath = [];
+
+  if (!isObject(data)) return closestPath;
+
+  let piece = data;
+
+  for (const segment of path) {
+    if (!(segment in piece)) break;
+    closestPath.push(segment);
+    if (!isObject(piece[segment])) break;
+    piece = piece[segment];
+  }
+
+  return closestPath;
+};


Test patch:
diff --git a/src/__tests__/linter.jest.test.ts b/src/__tests__/linter.jest.test.ts
index 06f9f8f5..c320b347 100644
--- a/src/__tests__/linter.jest.test.ts
+++ b/src/__tests__/linter.jest.test.ts
@@ -120,17 +120,17 @@ describe('Linter', () => {
           expect.objectContaining({
             code: 'empty-is-falsy',
             message: 'Value "https://example.com" should be falsy',
-            path: ['empty'],
+            path: ['info', 'contact', 'url'],
           }),
           expect.objectContaining({
             code: 'empty-is-falsy',
             message: 'Value Array[] should be falsy',
-            path: ['bar', 'empty'],
+            path: ['servers'],
           }),
           expect.objectContaining({
             code: 'empty-is-falsy',
             message: 'Value Object{} should be falsy',
-            path: ['foo', 'empty'],
+            path: ['info'],
           }),
         ]),
       );
diff --git a/src/__tests__/spectral.jest.test.ts b/src/__tests__/spectral.jest.test.ts
index c37d922d..847840c8 100644
--- a/src/__tests__/spectral.jest.test.ts
+++ b/src/__tests__/spectral.jest.test.ts
@@ -133,7 +133,7 @@ describe('Spectral', () => {
         }),
         expect.objectContaining({
           code: 'oas2-schema',
-          path: ['paths', '/todos/{todoId}', 'get', 'responses', '200', 'schema'],
+          path: [],
           range: {
             end: {
               character: 1,
@@ -231,18 +231,7 @@ describe('Spectral', () => {
       }),
 
       expect.objectContaining({
-        path: [
-          'paths',
-          '/test',
-          'get',
-          'responses',
-          '400',
-          'content',
-          'application/json',
-          'schema',
-          'properties',
-          'status_code',
-        ],
+        path: ['components', 'schemas', 'Error', 'properties', 'status_code'],
         source: expect.stringContaining('/src/__tests__/__fixtures__/gh-658/lib.yaml'),
         range: {
           end: {
@@ -256,18 +245,7 @@ describe('Spectral', () => {
         },
       }),
       expect.objectContaining({
-        path: [
-          'paths',
-          '/test',
-          'get',
-          'responses',
-          '400',
-          'content',
-          'application/json',
-          'schema',
-          'properties',
-          'test',
-        ],
+        path: ['components', 'schemas', 'Foo'],
         source: expect.stringContaining('/src/__tests__/__fixtures__/gh-658/URIError.yaml'),
         range: {
           end: {
@@ -282,18 +260,7 @@ describe('Spectral', () => {
       }),
 
       expect.objectContaining({
-        path: [
-          'paths',
-          '/test',
-          'get',
-          'responses',
-          '500',
-          'content',
-          'application/json',
-          'schema',
-          'properties',
-          'status_code',
-        ],
+        path: ['components', 'schemas', 'Error', 'properties', 'status_code'],
         source: expect.stringContaining('/src/__tests__/__fixtures__/gh-658/lib.yaml'),
         range: {
           end: {
@@ -307,18 +274,7 @@ describe('Spectral', () => {
         },
       }),
       expect.objectContaining({
-        path: [
-          'paths',
-          '/test',
-          'get',
-          'responses',
-          '500',
-          'content',
-          'application/json',
-          'schema',
-          'properties',
-          'test',
-        ],
+        path: ['components', 'schemas', 'Foo'],
         source: expect.stringContaining('/src/__tests__/__fixtures__/gh-658/URIError.yaml'),
         range: {
           end: {
@@ -362,7 +318,7 @@ describe('Spectral', () => {
         },
       }),
       expect.objectContaining({
-        path: ['components', 'schemas', 'Error', 'properties', 'test'],
+        path: ['components', 'schemas', 'Foo'],
         source: expect.stringContaining('/src/__tests__/__fixtures__/gh-658/URIError.yaml'),
         range: {
           end: {
@@ -377,7 +333,7 @@ describe('Spectral', () => {
       }),
 
       expect.objectContaining({
-        path: ['components', 'schemas', 'Baz', 'properties', 'status_code'],
+        path: ['components', 'schemas', 'Error', 'properties', 'status_code'],
         source: expect.stringContaining('/src/__tests__/__fixtures__/gh-658/lib.yaml'),
         range: {
           end: {
@@ -391,7 +347,7 @@ describe('Spectral', () => {
         },
       }),
       expect.objectContaining({
-        path: ['components', 'schemas', 'Baz', 'properties', 'test'],
+        path: ['components', 'schemas', 'Foo'],
         source: expect.stringContaining('/src/__tests__/__fixtures__/gh-658/URIError.yaml'),
         range: {
           end: {
diff --git a/src/__tests__/spectral.test.ts b/src/__tests__/spectral.test.ts
index 9c512f7a..c96f3190 100644
--- a/src/__tests__/spectral.test.ts
+++ b/src/__tests__/spectral.test.ts
@@ -188,11 +188,11 @@ describe('spectral', () => {
             path: ['foo', 'bar', 'baz'],
             range: {
               end: {
-                character: 0,
+                character: 12,
                 line: 0,
               },
               start: {
-                character: 0,
+                character: 7,
                 line: 0,
               },
             },
@@ -254,7 +254,7 @@ describe('spectral', () => {
           {
             code: 'pagination-responses-have-x-next-token',
             message: 'All collection endpoints have the X-Next-Token parameter in responses',
-            path: ['paths', '/agreements', 'get', 'responses', '200', 'headers'],
+            path: ['responses', 'GetAgreementsOk', 'headers'],
             range: expect.any(Object),
             severity: DiagnosticSeverity.Error,
             source,
diff --git a/src/cli/services/__tests__/linter.test.ts b/src/cli/services/__tests__/linter.test.ts
index ece84797..332f55c6 100644
--- a/src/cli/services/__tests__/linter.test.ts
+++ b/src/cli/services/__tests__/linter.test.ts
@@ -575,7 +575,7 @@ describe('Linter service', () => {
         expect.objectContaining({
           code: 'oas2-schema',
           message: '/info Property foo is not expected to be here',
-          path: ['info'],
+          path: ['definitions', 'info'],
           range: {
             end: {
               character: 5,
@@ -591,7 +591,7 @@ describe('Linter service', () => {
         expect.objectContaining({
           code: 'info-description',
           message: 'OpenAPI object info `description` must be present and non-empty string.',
-          path: ['info', 'description'], // todo: relative path or absolute path? there is no such path in linted file, but there is such in spec when working on resolved file
+          path: ['definitions', 'info', 'description'],
           range: {
             end: {
               character: 22,
@@ -626,7 +626,7 @@ describe('Linter service', () => {
         expect.objectContaining({
           code: 'oas2-schema',
           message: "/info should have required property 'title'",
-          path: ['info'],
+          path: [],
           range: {
             end: {
               character: 1,
@@ -642,7 +642,7 @@ describe('Linter service', () => {
         expect.objectContaining({
           code: 'info-description',
           message: 'OpenAPI object info `description` must be present and non-empty string.',
-          path: ['info', 'description'],
+          path: ['description'],
           range: {
             end: {
               character: 18,
diff --git a/src/rulesets/__tests__/message.spec.ts b/src/rulesets/__tests__/message.spec.ts
index 0b5f4c07..feee949a 100644
--- a/src/rulesets/__tests__/message.spec.ts
+++ b/src/rulesets/__tests__/message.spec.ts
@@ -8,6 +8,7 @@ describe('message util', () => {
         property: 'description',
         error: 'expected property to be truthy',
         path: '',
+        fullPath: '',
         value: void 0,
       }),
     ).toEqual('oops... "description" is missing;error: expected property to be truthy');
@@ -20,6 +21,7 @@ describe('message util', () => {
         property: 'description',
         error: 'expected property to be truthy',
         path: '',
+        fullPath: '',
         value,
       }),
     ).toEqual(`Value must not equal ${value}`);
@@ -32,6 +34,7 @@ describe('message util', () => {
         property: 'baz',
         error: 'foo',
         path: '',
+        fullPath: '',
         value: void 0,
       }),
     ).toEqual('foofoobazfoofoo');
@@ -44,6 +47,7 @@ describe('message util', () => {
         property: 'description',
         error: 'expected property to be truthy',
         path: '',
+        fullPath: '',
         value: void 0,
       }),
     ).toEqual('missing :(');