From 157ec592d8b3276094284fead7a08541b3f46f61 Mon Sep 17 00:00:00 2001 From: Phil Adams Date: Thu, 21 Jul 2022 04:42:29 -0400 Subject: [PATCH] fix(core): fix 'resolved vs unresolved' json path mapping (#2202) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(core): add test demonstrating path mapper issue * fix(core): fix 'resolved vs unresolved' json path mapping * refactor(core): use stoplight/json utils Co-authored-by: Dustin Popp Co-authored-by: Jakub Rożek --- .../cli/src/services/__tests__/linter.test.ts | 3 +- .../__fixtures__/gh-658/URIError.yaml | 2 + .../__tests__/__fixtures__/gh-658/lib.yaml | 23 ++++ .../core/src/__tests__/spectral.jest.test.ts | 36 ++++- packages/core/src/documentInventory.ts | 130 +++++++++++------- 5 files changed, 140 insertions(+), 54 deletions(-) diff --git a/packages/cli/src/services/__tests__/linter.test.ts b/packages/cli/src/services/__tests__/linter.test.ts index edcda750e..3b7324bb7 100644 --- a/packages/cli/src/services/__tests__/linter.test.ts +++ b/packages/cli/src/services/__tests__/linter.test.ts @@ -472,9 +472,10 @@ describe('Linter service', () => { { code: 'info-matches-stoplight', message: 'Info must contain Stoplight', - path: ['info', 'title'], + path: [], range: expect.any(Object), severity: DiagnosticSeverity.Warning, + source: expect.stringContaining('__fixtures__/resolver/document.json'), }, ]); }); diff --git a/packages/core/src/__tests__/__fixtures__/gh-658/URIError.yaml b/packages/core/src/__tests__/__fixtures__/gh-658/URIError.yaml index 4d545fa39..062e6d78b 100644 --- a/packages/core/src/__tests__/__fixtures__/gh-658/URIError.yaml +++ b/packages/core/src/__tests__/__fixtures__/gh-658/URIError.yaml @@ -28,6 +28,8 @@ paths: application/json: schema: $ref: "#/components/schemas/Error" + "404": + $ref: './lib.yaml#/components/responses/NotFoundResponse' "500": description: No Bueno. content: diff --git a/packages/core/src/__tests__/__fixtures__/gh-658/lib.yaml b/packages/core/src/__tests__/__fixtures__/gh-658/lib.yaml index be265ae0c..64b75aebf 100644 --- a/packages/core/src/__tests__/__fixtures__/gh-658/lib.yaml +++ b/packages/core/src/__tests__/__fixtures__/gh-658/lib.yaml @@ -22,3 +22,26 @@ components: type: string test: $ref: ./URIError.yaml#/components/schemas/Foo + NotFound: + oneOf: + - $ref: '#/components/schemas/DoesntExist' + - $ref: '#/components/schemas/JustCantFindIt' + DoesntExist: + type: object + properties: + id: + type: string + JustCantFindIt: + type: object + properties: + name: + type: string + + responses: + NotFoundResponse: + description: Not Found. + content: + application/json: + schema: + $ref: '#/components/schemas/NotFound' + diff --git a/packages/core/src/__tests__/spectral.jest.test.ts b/packages/core/src/__tests__/spectral.jest.test.ts index 38a8984a4..40ebc4d42 100644 --- a/packages/core/src/__tests__/spectral.jest.test.ts +++ b/packages/core/src/__tests__/spectral.jest.test.ts @@ -190,7 +190,7 @@ describe('Spectral', () => { const results = await s.run(new Document(fs.readFileSync(documentUri, 'utf8'), Parsers.Yaml, documentUri)); - expect(results.length).toEqual(3); + expect(results.length).toEqual(5); return expect(results).toEqual([ expect.objectContaining({ @@ -208,6 +208,36 @@ describe('Spectral', () => { }, }), + expect.objectContaining({ + path: ['components', 'schemas', 'DoesntExist', 'properties', 'id'], + source: expect.stringContaining('/src/__tests__/__fixtures__/gh-658/lib.yaml'), + range: { + end: { + character: 22, + line: 32, + }, + start: { + character: 11, + line: 31, + }, + }, + }), + + expect.objectContaining({ + path: ['components', 'schemas', 'JustCantFindIt', 'properties', 'name'], + source: expect.stringContaining('/src/__tests__/__fixtures__/gh-658/lib.yaml'), + range: { + end: { + character: 22, + line: 37, + }, + start: { + character: 13, + line: 36, + }, + }, + }), + expect.objectContaining({ path: ['paths', '/test', 'get', 'responses', '200', 'content', 'application/json', 'schema'], source: expect.stringContaining('/src/__tests__/__fixtures__/gh-658/URIError.yaml'), @@ -229,11 +259,11 @@ describe('Spectral', () => { range: { end: { character: 18, - line: 43, + line: 45, }, start: { character: 8, - line: 42, + line: 44, }, }, }), diff --git a/packages/core/src/documentInventory.ts b/packages/core/src/documentInventory.ts index cec163beb..f449e9819 100644 --- a/packages/core/src/documentInventory.ts +++ b/packages/core/src/documentInventory.ts @@ -1,20 +1,14 @@ -import { extractSourceFromRef, hasRef, isLocalRef } from '@stoplight/json'; +import { decodePointerFragment, encodePointerFragment, extractSourceFromRef, isLocalRef } from '@stoplight/json'; import { extname, resolve } from '@stoplight/path'; import { Dictionary, IParserResult, JsonPath } from '@stoplight/types'; -import { get, isObjectLike } from 'lodash'; +import { isObjectLike } from 'lodash'; import { Document, IDocument } from './document'; import { Resolver, ResolveResult } from '@stoplight/spectral-ref-resolver'; import { formatParserDiagnostics, formatResolverErrors } from './errorMessages'; import * as Parsers from '@stoplight/spectral-parsers'; import { IRuleResult } from './types'; -import { - getClosestJsonPath, - getEndRef, - isAbsoluteRef, - safePointerToPath, - traverseObjUntilRef, -} from '@stoplight/spectral-runtime'; +import { getClosestJsonPath, isAbsoluteRef, traverseObjUntilRef } from '@stoplight/spectral-runtime'; import { Format } from './ruleset/format'; export type DocumentInventoryItem = { @@ -86,79 +80,115 @@ export class DocumentInventory implements IDocumentInventory { public findAssociatedItemForPath(path: JsonPath, resolved: boolean): DocumentInventoryItem | null { if (!resolved) { const newPath: JsonPath = getClosestJsonPath(this.unresolved, path); - - return { + const item: DocumentInventoryItem = { document: this.document, path: newPath, missingPropertyPath: path, }; + return item; } try { const newPath: JsonPath = getClosestJsonPath(this.resolved, path); - let $ref = traverseObjUntilRef(this.unresolved, newPath); + const $ref = traverseObjUntilRef(this.unresolved, newPath); if ($ref === null) { - return { + const item: DocumentInventoryItem = { document: this.document, path: getClosestJsonPath(this.unresolved, path), missingPropertyPath: path, }; + return item; } const missingPropertyPath = newPath.length === 0 ? [] : path.slice(path.lastIndexOf(newPath[newPath.length - 1]) + 1); let { source } = this; + if (source === null || this.graph === null) { + return null; + } - // eslint-disable-next-line no-constant-condition - while (true) { - if (source === null || this.graph === null) return null; - - $ref = getEndRef(this.graph.getNodeData(source).refMap, $ref); - - if ($ref === null) return null; - - const scopedPath = [...safePointerToPath($ref), ...newPath]; - let resolvedDoc = this.document; - - if (isLocalRef($ref)) { - resolvedDoc = source === this.document.source ? this.document : this.referencedDocuments[source]; - } else { - const extractedSource = extractSourceFromRef($ref); - - if (extractedSource === null) { - return { - document: resolvedDoc, - path: getClosestJsonPath(resolvedDoc.data, path), - missingPropertyPath: path, - }; - } + let refMap = this.graph.getNodeData(source).refMap; + let resolvedDoc = this.document; - source = isAbsoluteRef(extractedSource) ? extractedSource : resolve(source, '..', extractedSource); + // Add '#' on the beginning of "path" to simplify the logic below. + const adjustedPath: string[] = ['#', ...path.map(String)]; - resolvedDoc = source === this.document.source ? this.document : this.referencedDocuments[source]; - const obj: unknown = - scopedPath.length === 0 || hasRef(resolvedDoc.data) ? resolvedDoc.data : get(resolvedDoc.data, scopedPath); + // Walk through the segments of 'path' one at a time, looking for + // json path locations containing a $ref. + let refMapKey = ''; + for (const segment of adjustedPath) { + if (refMapKey.length > 0) { + refMapKey += '/'; + } - if (hasRef(obj)) { - $ref = obj.$ref; - continue; + refMapKey += encodePointerFragment(segment); + + // If our current refMapKey value is in fact a key in refMap, + // then we'll "reverse-resolve" it by replacing refMapKey with + // the actual value of that key within refMap. + // It's possible that we have a "ref to a ref", so we'll do this + // "reverse-resolve" step in a while loop. + while (refMapKey in refMap) { + const newRef = refMap[refMapKey]; + if (isLocalRef(newRef)) { + refMapKey = newRef; + } else { + const extractedSource = extractSourceFromRef(newRef); + if (extractedSource === null) { + const item: DocumentInventoryItem = { + document: resolvedDoc, + path: getClosestJsonPath(resolvedDoc.data, path), + missingPropertyPath: path, + }; + return item; + } + + // Update 'source' to reflect the filename within the external ref. + source = isAbsoluteRef(extractedSource) ? extractedSource : resolve(source, '..', extractedSource); + + // Update "resolvedDoc" to reflect the new "source" value and make sure we found an actual document. + const newResolvedDoc = source === this.document.source ? this.document : this.referencedDocuments[source]; + if (newResolvedDoc === null || newResolvedDoc === void 0) { + const item: DocumentInventoryItem = { + document: resolvedDoc, + path: getClosestJsonPath(resolvedDoc.data, path), + missingPropertyPath: path, + }; + return item; + } + + resolvedDoc = newResolvedDoc; + + // Update "refMap" to reflect the new "source" value. + refMap = this.graph.getNodeData(source).refMap; + + refMapKey = newRef.indexOf('#') >= 0 ? newRef.slice(newRef.indexOf('#')) : '#'; } } - - const closestPath = getClosestJsonPath(resolvedDoc.data, scopedPath); - return { - document: resolvedDoc, - path: closestPath, - missingPropertyPath: [...closestPath, ...missingPropertyPath], - }; } + + const closestPath = getClosestJsonPath(resolvedDoc.data, this.convertRefMapKeyToPath(refMapKey)); + const item: DocumentInventoryItem = { + document: resolvedDoc, + path: closestPath, + missingPropertyPath: [...closestPath, ...missingPropertyPath], + }; + return item; } catch { return null; } } + protected convertRefMapKeyToPath(refPath: string): JsonPath { + if (refPath.startsWith('#/')) { + refPath = refPath.slice(2); + } + + return refPath.split('/').map(decodePointerFragment); + } + protected parseResolveResult: Resolver['parseResolveResult'] = resolveOpts => { const source = resolveOpts.targetAuthority.href().replace(/\/$/, ''); const ext = extname(source);