From 6be6b1fe7781a34cb08adc08774d425b561f78b4 Mon Sep 17 00:00:00 2001 From: Phil Adams Date: Wed, 6 Jul 2022 17:52:29 -0500 Subject: [PATCH 1/3] test(core): add test demonstrating path mapper issue --- .../__fixtures__/gh-658/URIError.yaml | 2 ++ .../__tests__/__fixtures__/gh-658/lib.yaml | 23 ++++++++++++ .../core/src/__tests__/spectral.jest.test.ts | 36 +++++++++++++++++-- 3 files changed, 58 insertions(+), 3 deletions(-) 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, }, }, }), From 5879e290dadc26f967ed773d4e9292c23cdf8937 Mon Sep 17 00:00:00 2001 From: Phil Adams Date: Wed, 6 Jul 2022 18:11:19 -0500 Subject: [PATCH 2/3] fix(core): fix 'resolved vs unresolved' json path mapping --- .../cli/src/services/__tests__/linter.test.ts | 3 +- packages/core/src/documentInventory.ts | 138 +++++++++++------- 2 files changed, 89 insertions(+), 52 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/documentInventory.ts b/packages/core/src/documentInventory.ts index cec163beb..cdb680157 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 { 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,77 +80,119 @@ 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; + let refMap = this.graph.getNodeData(source).refMap; + let resolvedDoc = this.document; - if (isLocalRef($ref)) { - resolvedDoc = source === this.document.source ? this.document : this.referencedDocuments[source]; - } else { - const extractedSource = extractSourceFromRef($ref); + // Add '#' on the beginning of "path" to simplify the logic below. + const adjustedPath: JsonPath = [...'#', ...path]; - if (extractedSource === null) { - return { - document: resolvedDoc, - path: getClosestJsonPath(resolvedDoc.data, path), - missingPropertyPath: path, - }; + // 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) { + refMapKey = refMapKey.concat('/'); + } + refMapKey = refMapKey.concat(segment.toString().replace(/\//g, '~1')); + + // 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 === undefined) { + 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('#')) : '#'; } + } + } - source = isAbsoluteRef(extractedSource) ? extractedSource : resolve(source, '..', extractedSource); + const closestPath = getClosestJsonPath(resolvedDoc.data, this.convertRefMapKeyToPath(refMapKey)); + const item: DocumentInventoryItem = { + document: resolvedDoc, + path: closestPath, + missingPropertyPath: [...closestPath, ...missingPropertyPath], + }; + return item; + } catch (e) { + // console.warn(`Caught exception! e=${e}`); + return null; + } + } - 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); + protected convertRefMapKeyToPath(refPath: string): JsonPath { + const jsonPath: JsonPath = []; - if (hasRef(obj)) { - $ref = obj.$ref; - continue; - } - } + if (refPath.startsWith('#/')) { + refPath = refPath.slice(2); + } - const closestPath = getClosestJsonPath(resolvedDoc.data, scopedPath); - return { - document: resolvedDoc, - path: closestPath, - missingPropertyPath: [...closestPath, ...missingPropertyPath], - }; - } - } catch { - return null; + const pathSegments: string[] = refPath.split('/'); + for (const pathSegment of pathSegments) { + jsonPath.push(pathSegment.replace('~1', '/')); } + + return jsonPath; } protected parseResolveResult: Resolver['parseResolveResult'] = resolveOpts => { From c96cb03e3aca2f5f9353d084c90e6ffe378ef354 Mon Sep 17 00:00:00 2001 From: Dustin Popp Date: Wed, 20 Jul 2022 15:52:37 -0500 Subject: [PATCH 3/3] refactor(core): use stoplight/json utils --- packages/core/src/documentInventory.ts | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/packages/core/src/documentInventory.ts b/packages/core/src/documentInventory.ts index cdb680157..f449e9819 100644 --- a/packages/core/src/documentInventory.ts +++ b/packages/core/src/documentInventory.ts @@ -1,4 +1,4 @@ -import { extractSourceFromRef, 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 { isObjectLike } from 'lodash'; @@ -113,16 +113,17 @@ export class DocumentInventory implements IDocumentInventory { let resolvedDoc = this.document; // Add '#' on the beginning of "path" to simplify the logic below. - const adjustedPath: JsonPath = [...'#', ...path]; + const adjustedPath: string[] = ['#', ...path.map(String)]; // 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) { - refMapKey = refMapKey.concat('/'); + if (refMapKey.length > 0) { + refMapKey += '/'; } - refMapKey = refMapKey.concat(segment.toString().replace(/\//g, '~1')); + + 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 @@ -149,7 +150,7 @@ export class DocumentInventory implements IDocumentInventory { // 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 === undefined) { + if (newResolvedDoc === null || newResolvedDoc === void 0) { const item: DocumentInventoryItem = { document: resolvedDoc, path: getClosestJsonPath(resolvedDoc.data, path), @@ -157,6 +158,7 @@ export class DocumentInventory implements IDocumentInventory { }; return item; } + resolvedDoc = newResolvedDoc; // Update "refMap" to reflect the new "source" value. @@ -174,25 +176,17 @@ export class DocumentInventory implements IDocumentInventory { missingPropertyPath: [...closestPath, ...missingPropertyPath], }; return item; - } catch (e) { - // console.warn(`Caught exception! e=${e}`); + } catch { return null; } } protected convertRefMapKeyToPath(refPath: string): JsonPath { - const jsonPath: JsonPath = []; - if (refPath.startsWith('#/')) { refPath = refPath.slice(2); } - const pathSegments: string[] = refPath.split('/'); - for (const pathSegment of pathSegments) { - jsonPath.push(pathSegment.replace('~1', '/')); - } - - return jsonPath; + return refPath.split('/').map(decodePointerFragment); } protected parseResolveResult: Resolver['parseResolveResult'] = resolveOpts => {