Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(resolving): detect the source of local references properly #615

Merged
merged 3 commits into from
Oct 2, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 61 additions & 1 deletion src/__tests__/spectral.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
]);
});
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,10 @@
"host": "d",
"info": {
"$ref": "./refs/info-ref.json"
},
"paths": {
"/test": {
"$ref": "./refs/paths.json#/paths/~1test"
}
}
}
9 changes: 9 additions & 0 deletions src/cli/services/__tests__/__fixtures__/refs/paths.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"paths": {
"/test": {
"get": {
"response": "bar"
}
}
}
}
48 changes: 48 additions & 0 deletions src/cli/services/__tests__/linter.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
}),
]);
});
});
Expand Down
6 changes: 4 additions & 2 deletions src/linter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -96,10 +96,11 @@ export const lintNode = (
results = results.concat(
targetResults.map<IRuleResult>(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 {
Expand Down Expand Up @@ -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)) {
Expand Down
30 changes: 27 additions & 3 deletions src/resolved.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
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, 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: {
Expand Down Expand Up @@ -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];
Expand All @@ -48,12 +72,12 @@ 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],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is decodePointerFragment required now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the path stored under get(target, [REF_METADATA, 'root'] is escaped, meaning /foo is represented as ~1foo.
The path we expect to return should be unescaped (/foo), so that the YAML/JSON getLocationForJsonPath can find a node correctly.
If you revert the change, the test I added will fail - produced ranges will be less precise (they will point at paths rather than paths//test/get.

doc: get(this.parsedMap.parsed, get(target, [REF_METADATA, 'ref']), this.spec),
};
}

if (path.length > 0 && !has(this.spec.parsed.data, path)) {
if (!this.doesBelongToDoc(path)) {
return null;
}

Expand Down
4 changes: 2 additions & 2 deletions src/rulesets/oas/functions/refSiblings.ts
Original file line number Diff line number Diff line change
@@ -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<JsonPath> {
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;
}

Expand Down
2 changes: 2 additions & 0 deletions src/utils/hasRef.ts
Original file line number Diff line number Diff line change
@@ -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';
1 change: 1 addition & 0 deletions src/utils/index.ts
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
export * from './hasRef';
export * from './hasIntersectingElement';
export * from './isObject';