From b96895745d8c0eea30895e3489e603e0144c1b01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ro=C5=BCek?= Date: Mon, 30 Sep 2019 13:55:01 +0200 Subject: [PATCH 1/2] fix(resolving): detect the source of local references properly --- src/__tests__/spectral.test.ts | 62 ++++++++++++++++++++++- src/linter.ts | 6 ++- src/resolved.ts | 28 +++++++++- src/rulesets/oas/functions/refSiblings.ts | 4 +- src/utils/hasRef.ts | 2 + src/utils/index.ts | 1 + 6 files changed, 96 insertions(+), 7 deletions(-) create mode 100644 src/utils/hasRef.ts diff --git a/src/__tests__/spectral.test.ts b/src/__tests__/spectral.test.ts index e7958d84b..1b29053db 100644 --- a/src/__tests__/spectral.test.ts +++ b/src/__tests__/spectral.test.ts @@ -201,11 +201,71 @@ describe('spectral', () => { line: 0, }, }, - severity: 0, + severity: DiagnosticSeverity.Error, source: void 0, }, ]); }); + + test('should recognize the source of local $refs', () => { + const s = new Spectral(); + const source = 'foo.yaml'; + + const parsedResult: IParsedResult = { + getLocationForJsonPath, + source, + parsed: parseWithPointers( + JSON.stringify( + { + paths: { + '/agreements': { + get: { + description: 'Get some Agreements', + responses: { + '200': { + $ref: '#/responses/GetAgreementsOk', + }, + default: {}, + }, + summary: 'List agreements', + tags: ['agreements', 'pagination'], + }, + }, + }, + responses: { + GetAgreementsOk: { + description: 'Successful operation', + headers: {}, + }, + }, + }, + null, + 2, + ), + ), + }; + + s.setRules({ + 'pagination-responses-have-x-next-token': { + description: 'All collection endpoints have the X-Next-Token parameter in responses', + given: "$.paths..get.responses['200'].headers", + severity: 'error', + recommended: true, + then: { field: 'X-Next-Token', function: 'truthy' }, + }, + }); + + return expect(s.run(parsedResult)).resolves.toEqual([ + { + 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'], + range: expect.any(Object), + severity: DiagnosticSeverity.Error, + source, + }, + ]); + }); }); }); diff --git a/src/linter.ts b/src/linter.ts index 77edc123c..9fe2281ea 100644 --- a/src/linter.ts +++ b/src/linter.ts @@ -96,10 +96,11 @@ export const lintNode = ( results = results.concat( targetResults.map(result => { const escapedJsonPath = (result.path || targetPath).map(segment => decodePointerFragment(String(segment))); - const path = getRealJsonPath( + 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); return { @@ -199,7 +200,8 @@ function keyAndOptionalPattern(key: string | number, pattern: string, value: any }; } -function getRealJsonPath(data: unknown, path: JsonPath) { +// todo: revisit -> https://github.com/stoplightio/spectral/issues/608 +function getClosestJsonPath(data: unknown, path: JsonPath) { if (data === null || typeof data !== 'object') return []; while (path.length > 0 && !has(data, path)) { diff --git a/src/resolved.ts b/src/resolved.ts index e3106a838..2579abe5b 100644 --- a/src/resolved.ts +++ b/src/resolved.ts @@ -1,8 +1,10 @@ +import { pointerToPath } from '@stoplight/json'; import { IResolveError } from '@stoplight/json-ref-resolver/types'; import { Dictionary, ILocation, IRange, JsonPath, Segment } from '@stoplight/types'; -import { get, has } from 'lodash'; +import { get } from 'lodash'; import { IParseMap, REF_METADATA, ResolveResult } from './spectral'; import { IParsedResult } from './types'; +import { hasRef, isObject } from './utils'; const getDefaultRange = (): IRange => ({ start: { @@ -31,6 +33,28 @@ export class Resolved { this.errors = resolveResult.errors; } + public doesBelongToDoc(path: JsonPath): boolean { + 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; + } + + let piece = this.unresolved; + + for (let i = 0; i < path.length; i++) { + if (!isObject(piece)) return false; + + if (path[i] in piece) { + piece = piece[path[i]]; + } else if (hasRef(piece)) { + return this.doesBelongToDoc([...pointerToPath(piece.$ref), ...path.slice(i)]); + } + } + + return true; + } + public getParsedForJsonPath(path: JsonPath) { let target: object = this.parsedMap.refs; const newPath = [...path]; @@ -53,7 +77,7 @@ export class Resolved { }; } - if (path.length > 0 && !has(this.spec.parsed.data, path)) { + if (!this.doesBelongToDoc(path)) { return null; } diff --git a/src/rulesets/oas/functions/refSiblings.ts b/src/rulesets/oas/functions/refSiblings.ts index 7527629fd..1b7e991a1 100644 --- a/src/rulesets/oas/functions/refSiblings.ts +++ b/src/rulesets/oas/functions/refSiblings.ts @@ -1,14 +1,14 @@ import { JsonPath } from '@stoplight/types'; import { IFunction, IFunctionResult } from '../../../types'; +import { hasRef } from '../../../utils/hasRef'; // function is needed because `$..$ref` or `$..[?(@.$ref)]` are not parsed correctly // and therefore lead to infinite recursion due to the dollar sign ('$' in '$ref') function* siblingIterator(obj: object, path: JsonPath): IterableIterator { - const hasRef = '$ref' in obj; for (const key in obj) { if (!Object.hasOwnProperty.call(obj, key)) continue; const scopedPath = [...path, key]; - if (hasRef && key !== '$ref') { + if (hasRef(obj) && key !== '$ref') { yield scopedPath; } diff --git a/src/utils/hasRef.ts b/src/utils/hasRef.ts new file mode 100644 index 000000000..3d42e5f2e --- /dev/null +++ b/src/utils/hasRef.ts @@ -0,0 +1,2 @@ +export const hasRef = (obj: object): obj is object & { $ref: string } => + '$ref' in obj && typeof (obj as Partial<{ $ref: unknown }>).$ref === 'string'; diff --git a/src/utils/index.ts b/src/utils/index.ts index e235cac3d..85f1d7b01 100644 --- a/src/utils/index.ts +++ b/src/utils/index.ts @@ -1,2 +1,3 @@ +export * from './hasRef'; export * from './hasIntersectingElement'; export * from './isObject'; From 6986b82bee725aa8733c2d07ccc65a99e14c22c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ro=C5=BCek?= Date: Mon, 30 Sep 2019 14:11:16 +0200 Subject: [PATCH 2/2] fix(resolving): decode path to produce proper ranges --- .../__fixtures__/draft-nested-ref.oas2.json | 5 ++ .../__tests__/__fixtures__/refs/paths.json | 9 ++++ src/cli/services/__tests__/linter.test.ts | 48 +++++++++++++++++++ src/resolved.ts | 4 +- 4 files changed, 64 insertions(+), 2 deletions(-) create mode 100644 src/cli/services/__tests__/__fixtures__/refs/paths.json diff --git a/src/cli/services/__tests__/__fixtures__/draft-nested-ref.oas2.json b/src/cli/services/__tests__/__fixtures__/draft-nested-ref.oas2.json index 441e923e9..da6d298de 100644 --- a/src/cli/services/__tests__/__fixtures__/draft-nested-ref.oas2.json +++ b/src/cli/services/__tests__/__fixtures__/draft-nested-ref.oas2.json @@ -3,5 +3,10 @@ "host": "d", "info": { "$ref": "./refs/info-ref.json" + }, + "paths": { + "/test": { + "$ref": "./refs/paths.json#/paths/~1test" + } } } diff --git a/src/cli/services/__tests__/__fixtures__/refs/paths.json b/src/cli/services/__tests__/__fixtures__/refs/paths.json new file mode 100644 index 000000000..fceaaf76d --- /dev/null +++ b/src/cli/services/__tests__/__fixtures__/refs/paths.json @@ -0,0 +1,9 @@ +{ + "paths": { + "/test": { + "get": { + "response": "bar" + } + } + } +} diff --git a/src/cli/services/__tests__/linter.test.ts b/src/cli/services/__tests__/linter.test.ts index 1b518bb94..b7602bd1e 100644 --- a/src/cli/services/__tests__/linter.test.ts +++ b/src/cli/services/__tests__/linter.test.ts @@ -380,6 +380,54 @@ describe('Linter service', () => { }, source: expect.stringContaining('__tests__/__fixtures__/refs/contact.json'), }), + expect.objectContaining({ + code: 'operation-description', + message: 'Operation `description` must be present and non-empty string.', + path: ['paths', '/test', 'get'], + range: { + end: { + character: 7, + line: 5, + }, + start: { + character: 13, + line: 3, + }, + }, + source: expect.stringContaining('__tests__/__fixtures__/refs/paths.json'), + }), + expect.objectContaining({ + code: 'operation-operationId', + message: 'Operation should have an `operationId`.', + path: ['paths', '/test', 'get'], + range: { + end: { + character: 7, + line: 5, + }, + start: { + character: 13, + line: 3, + }, + }, + source: expect.stringContaining('__tests__/__fixtures__/refs/paths.json'), + }), + expect.objectContaining({ + code: 'operation-tags', + message: 'Operation should have non-empty `tags` array.', + path: ['paths', '/test', 'get'], + range: { + end: { + character: 7, + line: 5, + }, + start: { + character: 13, + line: 3, + }, + }, + source: expect.stringContaining('__tests__/__fixtures__/refs/paths.json'), + }), ]); }); }); diff --git a/src/resolved.ts b/src/resolved.ts index 2579abe5b..202f5b3ef 100644 --- a/src/resolved.ts +++ b/src/resolved.ts @@ -1,4 +1,4 @@ -import { pointerToPath } from '@stoplight/json'; +import { decodePointerFragment, pointerToPath } from '@stoplight/json'; import { IResolveError } from '@stoplight/json-ref-resolver/types'; import { Dictionary, ILocation, IRange, JsonPath, Segment } from '@stoplight/types'; import { get } from 'lodash'; @@ -72,7 +72,7 @@ export class Resolved { if (target && target[REF_METADATA]) { return { - path: [...get(target, [REF_METADATA, 'root'], []), ...newPath], + path: [...get(target, [REF_METADATA, 'root'], []).map(decodePointerFragment), ...newPath], doc: get(this.parsedMap.parsed, get(target, [REF_METADATA, 'ref']), this.spec), }; }