From ee47cc1938986e2846378c72e74b91a999b6fb5a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ro=C5=BCek?= Date: Tue, 3 Dec 2019 05:37:22 +0100 Subject: [PATCH] fix: write detection from scratch --- .../__fixtures__/gh-658/URIError.yaml | 8 + src/__tests__/__fixtures__/gh-658/lib.yaml | 4 + src/__tests__/spectral.jest.test.ts | 266 +++++++++++++----- src/cli/services/__tests__/linter.test.ts | 7 + src/resolved.ts | 108 +++---- src/utils/__tests__/refs.jest.test.ts | 66 +++++ src/utils/hasRef.ts | 7 +- src/utils/index.ts | 3 +- src/utils/refs.ts | 63 +++++ 9 files changed, 397 insertions(+), 135 deletions(-) create mode 100644 src/utils/__tests__/refs.jest.test.ts create mode 100644 src/utils/refs.ts diff --git a/src/__tests__/__fixtures__/gh-658/URIError.yaml b/src/__tests__/__fixtures__/gh-658/URIError.yaml index 82f88b081..4d545fa39 100644 --- a/src/__tests__/__fixtures__/gh-658/URIError.yaml +++ b/src/__tests__/__fixtures__/gh-658/URIError.yaml @@ -28,9 +28,17 @@ paths: application/json: schema: $ref: "#/components/schemas/Error" + "500": + description: No Bueno. + content: + application/json: + schema: + $ref: "./lib.yaml#/components/schemas/Error" components: schemas: Error: $ref: "#/components/schemas/Baz" Baz: $ref: ./lib.yaml#/components/schemas/Error + Foo: + type: string diff --git a/src/__tests__/__fixtures__/gh-658/lib.yaml b/src/__tests__/__fixtures__/gh-658/lib.yaml index 7b41e12db..be265ae0c 100644 --- a/src/__tests__/__fixtures__/gh-658/lib.yaml +++ b/src/__tests__/__fixtures__/gh-658/lib.yaml @@ -14,7 +14,11 @@ components: properties: error: type: string + maxLength: 1 error_description: type: string + maxLength: 1 status_code: type: string + test: + $ref: ./URIError.yaml#/components/schemas/Foo diff --git a/src/__tests__/spectral.jest.test.ts b/src/__tests__/spectral.jest.test.ts index 5af37c906..c37d922d3 100644 --- a/src/__tests__/spectral.jest.test.ts +++ b/src/__tests__/spectral.jest.test.ts @@ -105,6 +105,7 @@ describe('Spectral', () => { const parsed = { parsed: parseWithPointers(fs.readFileSync(documentUri, 'utf8')), getLocationForJsonPath, + source: documentUri, }; const results = await spectral.run(parsed, { @@ -128,7 +129,7 @@ describe('Spectral', () => { line: 16, }, }, - source: undefined, + source: documentUri, }), expect.objectContaining({ code: 'oas2-schema', @@ -158,7 +159,7 @@ describe('Spectral', () => { line: 10, }, }, - source: undefined, + source: documentUri, }), expect.objectContaining({ code: 'info-contact', @@ -173,7 +174,7 @@ describe('Spectral', () => { line: 2, }, }, - source: undefined, + source: documentUri, }), expect.objectContaining({ code: 'operation-description', @@ -188,13 +189,13 @@ describe('Spectral', () => { line: 11, }, }, - source: undefined, + source: documentUri, }), ]), ); }); - test('should recognize the source of remote $refs', () => { + test('should recognize the source of remote $refs', async () => { const s = new Spectral({ resolver: httpAndFileResolver }); const documentUri = path.join(__dirname, './__fixtures__/gh-658/URIError.yaml'); @@ -203,7 +204,7 @@ describe('Spectral', () => { severity: DiagnosticSeverity.Warning, recommended: true, message: "String typed properties MUST be further described using 'maxLength'. Error: {{error}}", - given: "$..*[?(@.type === 'string')]", + given: "$..[?(@.type === 'string')]", then: { field: 'maxLength', function: 'truthy', @@ -211,73 +212,198 @@ describe('Spectral', () => { }, }); - return expect(s.run(fs.readFileSync(documentUri, 'utf8'), { resolve: { documentUri } })).resolves.toEqual( - expect.arrayContaining([ - expect.objectContaining({ - code: 'schema-strings-maxLength', - path: ['paths', '/test', 'get', 'responses', '200', 'content', 'application/json', 'schema'], - range: { - end: { - character: 28, - line: 23, - }, - start: { - character: 21, - line: 22, - }, + const results = await s.run(fs.readFileSync(documentUri, 'utf8'), { resolve: { documentUri } }); + + return expect(results).toEqual([ + expect.objectContaining({ + path: ['paths', '/test', 'get', 'responses', '200', 'content', 'application/json', 'schema'], + source: expect.stringContaining('/src/__tests__/__fixtures__/gh-658/URIError.yaml'), + range: { + end: { + character: 28, + line: 23, }, - severity: DiagnosticSeverity.Warning, - source: expect.stringContaining('/src/__tests__/__fixtures__/gh-658/URIError.yaml'), - }), - expect.objectContaining({ - code: 'schema-strings-maxLength', - path: ['components', 'schemas', 'Baz', 'properties', 'error'], - range: { - end: { - character: 22, - line: 15, - }, - start: { - character: 14, - line: 14, - }, + start: { + character: 21, + line: 22, }, - severity: DiagnosticSeverity.Warning, - source: expect.stringContaining('/src/__tests__/__fixtures__/gh-658/lib.yaml'), - }), - expect.objectContaining({ - code: 'schema-strings-maxLength', - path: ['components', 'schemas', 'Baz', 'properties', 'error_description'], - range: { - end: { - character: 22, - line: 17, - }, - start: { - character: 26, - line: 16, - }, + }, + }), + + expect.objectContaining({ + path: [ + 'paths', + '/test', + 'get', + 'responses', + '400', + 'content', + 'application/json', + 'schema', + 'properties', + 'status_code', + ], + source: expect.stringContaining('/src/__tests__/__fixtures__/gh-658/lib.yaml'), + range: { + end: { + character: 22, + line: 21, }, - severity: DiagnosticSeverity.Warning, - source: expect.stringContaining('/src/__tests__/__fixtures__/gh-658/lib.yaml'), - }), - expect.objectContaining({ - code: 'schema-strings-maxLength', - path: ['components', 'schemas', 'Baz', 'properties', 'status_code'], - range: { - end: { - character: 22, - line: 19, - }, - start: { - character: 20, - line: 18, - }, + start: { + character: 20, + line: 20, }, - severity: DiagnosticSeverity.Warning, - source: expect.stringContaining('/src/__tests__/__fixtures__/gh-658/lib.yaml'), - }), - ]), - ); + }, + }), + expect.objectContaining({ + path: [ + 'paths', + '/test', + 'get', + 'responses', + '400', + 'content', + 'application/json', + 'schema', + 'properties', + 'test', + ], + source: expect.stringContaining('/src/__tests__/__fixtures__/gh-658/URIError.yaml'), + range: { + end: { + character: 18, + line: 43, + }, + start: { + character: 8, + line: 42, + }, + }, + }), + + expect.objectContaining({ + path: [ + 'paths', + '/test', + 'get', + 'responses', + '500', + 'content', + 'application/json', + 'schema', + 'properties', + 'status_code', + ], + source: expect.stringContaining('/src/__tests__/__fixtures__/gh-658/lib.yaml'), + range: { + end: { + character: 22, + line: 21, + }, + start: { + character: 20, + line: 20, + }, + }, + }), + expect.objectContaining({ + path: [ + 'paths', + '/test', + 'get', + 'responses', + '500', + 'content', + 'application/json', + 'schema', + 'properties', + 'test', + ], + source: expect.stringContaining('/src/__tests__/__fixtures__/gh-658/URIError.yaml'), + range: { + end: { + character: 18, + line: 43, + }, + start: { + character: 8, + line: 42, + }, + }, + }), + + expect.objectContaining({ + path: ['components', 'schemas', 'Foo'], + source: expect.stringContaining('/src/__tests__/__fixtures__/gh-658/URIError.yaml'), + range: { + end: { + character: 18, + line: 43, + }, + start: { + character: 8, + line: 42, + }, + }, + }), + + expect.objectContaining({ + path: ['components', 'schemas', 'Error', 'properties', 'status_code'], + source: expect.stringContaining('/src/__tests__/__fixtures__/gh-658/lib.yaml'), + range: { + end: { + character: 22, + line: 21, + }, + start: { + character: 20, + line: 20, + }, + }, + }), + expect.objectContaining({ + path: ['components', 'schemas', 'Error', 'properties', 'test'], + source: expect.stringContaining('/src/__tests__/__fixtures__/gh-658/URIError.yaml'), + range: { + end: { + character: 18, + line: 43, + }, + start: { + character: 8, + line: 42, + }, + }, + }), + + expect.objectContaining({ + path: ['components', 'schemas', 'Baz', 'properties', 'status_code'], + source: expect.stringContaining('/src/__tests__/__fixtures__/gh-658/lib.yaml'), + range: { + end: { + character: 22, + line: 21, + }, + start: { + character: 20, + line: 20, + }, + }, + }), + expect.objectContaining({ + path: ['components', 'schemas', 'Baz', 'properties', 'test'], + source: expect.stringContaining('/src/__tests__/__fixtures__/gh-658/URIError.yaml'), + range: { + end: { + character: 18, + line: 43, + }, + start: { + character: 8, + line: 42, + }, + }, + }), + ]); }); }); diff --git a/src/cli/services/__tests__/linter.test.ts b/src/cli/services/__tests__/linter.test.ts index 7a5cfbcdf..4c11262f7 100644 --- a/src/cli/services/__tests__/linter.test.ts +++ b/src/cli/services/__tests__/linter.test.ts @@ -55,6 +55,7 @@ describe('Linter service', () => { expect.arrayContaining([ expect.objectContaining({ code: 'oas3-schema', + path: ['info', 'contact', 'name'], range: { end: { character: 14, @@ -709,6 +710,12 @@ describe('Linter service', () => { describe('--resolver', () => { it('uses provided resolver for $ref resolving', async () => { expect(await run(`lint --resolver ${fooResolver} ${fooDocument}`)).toEqual([ + expect.objectContaining({ + code: 'info-contact', + }), + expect.objectContaining({ + code: 'info-description', + }), expect.objectContaining({ code: 'oas3-api-servers', }), diff --git a/src/resolved.ts b/src/resolved.ts index 73e77002c..fd6630753 100644 --- a/src/resolved.ts +++ b/src/resolved.ts @@ -1,11 +1,12 @@ -import { decodePointerFragment } from '@stoplight/json'; +import { pointerToPath } from '@stoplight/json'; import { IGraphNodeData, IResolveError } from '@stoplight/json-ref-resolver/types'; -import { Dictionary, ILocation, IRange, JsonPath, Segment } from '@stoplight/types'; +import { resolve } from '@stoplight/path'; +import { Dictionary, ILocation, IRange, JsonPath } from '@stoplight/types'; import { DepGraph } from 'dependency-graph'; import { get } from 'lodash'; -import { IParseMap, REF_METADATA, ResolveResult } from './spectral'; +import { IParseMap, ResolveResult } from './spectral'; import { IParsedResult } from './types'; -import { hasRef, isObject } from './utils'; +import { extractSourceFromRef, getEndRef, hasRef, isLocalRef, safePointerToPath, traverseObjUntilRef } from './utils'; const getDefaultRange = (): IRange => ({ start: { @@ -36,78 +37,61 @@ export class Resolved { this.errors = resolveResult.errors; } - // this method detects whether given json path points to a place in original unresolved document - public doesBelongToSourceDoc(path: JsonPath): boolean { + public getParsedForJsonPath(path: JsonPath) { if (path.length === 0) { - // todo: each rule and their function should be context-aware, meaning they should aware of the fact they operate on resolved content - // let's assume the error was reported correctly by any custom rule /shrug - return true; + return { + path, + doc: this.spec, + }; } - let piece = this.unresolved; + try { + const newPath: JsonPath = [...path]; + let $ref = traverseObjUntilRef(this.unresolved, newPath); - for (const segment of path) { - if (!isObject(piece)) return false; + if ($ref === null) { + return { + path, + doc: this.spec, + }; + } - if (segment in piece) { - piece = piece[segment]; - } else if (hasRef(piece)) { - if (this.spec.source === void 0) return false; - let nodeData; - try { - nodeData = this.graph.getNodeData(this.spec.source); - } catch { - return false; - } + let source = this.spec.source; - const { refMap } = nodeData; - let { $ref } = piece; + while (true) { + if (source === void 0) return null; - while ($ref in refMap) { - $ref = refMap[$ref]; - } + $ref = getEndRef(this.graph.getNodeData(source).refMap, $ref); - return ( - $ref.length === 0 || - $ref[0] === '#' || - $ref.slice(0, Math.max(0, Math.min($ref.length, $ref.indexOf('#')))) === this.spec.source - ); - } - } + if ($ref === null) return null; - return true; - } + if (isLocalRef($ref)) { + return { + path: pointerToPath($ref), + doc: source === this.spec.source ? this.spec : this.parsedMap.parsed[source], + }; + } - public getParsedForJsonPath(path: JsonPath) { - let target: object = this.parsedMap.refs; - const newPath = [...path]; - let segment: Segment; - - while (newPath.length > 0) { - segment = newPath.shift()!; - if (segment && segment in target) { - target = target[segment]; - } else { - newPath.unshift(segment); - break; - } - } + source = resolve(source, '..', extractSourceFromRef($ref)!); - if (target && target[REF_METADATA]) { - return { - path: [...get(target, [REF_METADATA, 'root'], []).map(decodePointerFragment), ...newPath], - doc: get(this.parsedMap.parsed, get(target, [REF_METADATA, 'ref']), this.spec), - }; - } + const doc = source === this.spec.source ? this.spec : this.parsedMap.parsed[source]; + const { parsed } = doc; + const scopedPath = [...safePointerToPath($ref), ...newPath]; + + const obj = scopedPath.length === 0 || hasRef(parsed.data) ? parsed.data : get(parsed.data, scopedPath); - if (!this.doesBelongToSourceDoc(path)) { + if (hasRef(obj)) { + $ref = obj.$ref; + } else { + return { + doc, + path: scopedPath, + }; + } + } + } catch { return null; } - - return { - path, - doc: this.spec, - }; } public getLocationForJsonPath(path: JsonPath, closest?: boolean): ILocation { diff --git a/src/utils/__tests__/refs.jest.test.ts b/src/utils/__tests__/refs.jest.test.ts new file mode 100644 index 000000000..187bb0de3 --- /dev/null +++ b/src/utils/__tests__/refs.jest.test.ts @@ -0,0 +1,66 @@ +import { extractSourceFromRef, traverseObjUntilRef } from '..'; + +describe('$ref utils', () => { + describe('extractSourceFromRef', () => { + it.each` + ref | expected + ${1} | ${null} + ${'../foo.json#'} | ${'../foo.json'} + ${'../foo.json#/'} | ${'../foo.json'} + ${'../foo.json#/foo/bar'} | ${'../foo.json'} + ${'../foo.json'} | ${'../foo.json'} + ${'foo.json'} | ${'foo.json'} + ${'http://foo.com#/foo'} | ${'http://foo.com'} + ${''} | ${null} + ${'#'} | ${null} + ${'#/foo/bar'} | ${null} + `('returns proper $expected source for $ref', ({ ref, expected }) => { + expect(extractSourceFromRef(ref)).toEqual(expected); + }); + }); + + describe('traverseObjUntilRef', () => { + it('given a broken json path, throws', () => { + const obj = { + foo: { + bar: { + baz: '', + }, + }, + bar: '', + }; + + expect(traverseObjUntilRef.bind(null, obj, ['foo', 'baz'])).toThrow('Segment is not a part of the object'); + expect(traverseObjUntilRef.bind(null, obj, ['bar', 'foo'])).toThrow('Segment is not a part of the object'); + }); + + it('given a json path pointing at object with ref, returns the ref', () => { + const obj = { + x: { + $ref: 'test.json#', + }, + foo: { + bar: { + $ref: '../a.json#', + }, + }, + }; + + expect(traverseObjUntilRef(obj, ['x', 'bar'])).toEqual('test.json#'); + expect(traverseObjUntilRef(obj, ['foo', 'bar', 'baz'])).toEqual('../a.json#'); + }); + + it('given a finite json path pointing at value in project, returns null', () => { + const obj = { + x: {}, + foo: { + bar: 'test', + }, + }; + + expect(traverseObjUntilRef(obj, ['x'])).toBeNull(); + expect(traverseObjUntilRef(obj, ['foo'])).toBeNull(); + expect(traverseObjUntilRef(obj, ['foo', 'bar'])).toBeNull(); + }); + }); +}); diff --git a/src/utils/hasRef.ts b/src/utils/hasRef.ts index 3d42e5f2e..69164eb09 100644 --- a/src/utils/hasRef.ts +++ b/src/utils/hasRef.ts @@ -1,2 +1,5 @@ -export const hasRef = (obj: object): obj is object & { $ref: string } => - '$ref' in obj && typeof (obj as Partial<{ $ref: unknown }>).$ref === 'string'; +import { isObject } from 'lodash'; + +// needs to be in separate file due to rollup :/ +export const hasRef = (obj: unknown): obj is object & { $ref: string } => + isObject(obj) && '$ref' in obj && typeof (obj as Partial<{ $ref: unknown }>).$ref === 'string'; diff --git a/src/utils/index.ts b/src/utils/index.ts index d075e157a..38daad001 100644 --- a/src/utils/index.ts +++ b/src/utils/index.ts @@ -1,4 +1,5 @@ export * from './empty'; -export * from './hasRef'; export * from './hasIntersectingElement'; export * from './isObject'; +export * from './refs'; +export * from './hasRef'; diff --git a/src/utils/refs.ts b/src/utils/refs.ts new file mode 100644 index 000000000..f430c0fab --- /dev/null +++ b/src/utils/refs.ts @@ -0,0 +1,63 @@ +import { pointerToPath } from '@stoplight/json'; +import { Dictionary, JsonPath } from '@stoplight/types'; +import { isObject } from 'lodash'; +import { hasRef } from './hasRef'; + +export const isLocalRef = (pointer: string) => pointer.length > 0 && pointer[0] === '#'; + +export const extractSourceFromRef = (ref: unknown): string | null => { + if (typeof ref !== 'string' || ref.length === 0 || isLocalRef(ref)) { + return null; + } + + const index = ref.indexOf('#'); + + if (index === -1) { + return ref; + } + + return ref.slice(0, index); +}; + +export const traverseObjUntilRef = (obj: unknown, path: JsonPath): string | null => { + let piece: unknown = obj; + + for (const segment of path.slice()) { + if (!isObject(piece)) { + throw new TypeError('Segment is not a part of the object'); + } + + if (segment in piece) { + piece = piece[segment]; + } else if (hasRef(piece)) { + return piece.$ref; + } else { + throw new Error('Segment is not a part of the object'); + } + + path.shift(); + } + + if (isObject(piece) && hasRef(piece) && Object.keys(piece).length === 1) { + return piece.$ref; + } + + return null; +}; + +export const getEndRef = (refMap: Dictionary, $ref: string): string => { + while ($ref in refMap) { + $ref = refMap[$ref]; + } + + return $ref; +}; + +export const safePointerToPath = (pointer: string): JsonPath => { + const index = pointer.indexOf('#'); + if (index === -1) { + return []; + } + + return pointerToPath(pointer.slice(index)); +};