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.

Sign up for GitHub

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

Jump to bottom

fix: absolute paths #839

Merged
merged 11 commits into from Dec 19, 2019
Merged

fix: absolute paths #839

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
9f17f02
fix: absolute paths
P0lip Dec 6, 2019
fbf9679
test: update paths in tests
P0lip Dec 6, 2019
a797ff0
refactor: move getClosestJsonPath
P0lip Dec 10, 2019
ee8b7d9
feat: generate scoped path
P0lip Dec 11, 2019
4ccc94c
Merge branch 'develop' into fix/path-result
P0lip Dec 11, 2019
a5c7f1c
Merge branch 'develop' into fix/path-result
P0lip Dec 12, 2019
fa39945
feat: missingPropertyPath
P0lip Dec 12, 2019
f93c84f
revert: parts of fa3994
P0lip Dec 18, 2019
7d82f0d
Merge branch 'develop' into fix/path-result
P0lip Dec 18, 2019
f6ee8ba
style: new line
P0lip Dec 18, 2019
8f14d4c
Merge branch 'develop' into fix/path-result
P0lip Dec 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/__tests__/linter.jest.test.ts
View file
Open in desktop
Expand Up @@ -123,17 +123,17 @@ describe('Linter', () => {
expect.objectContaining({
code: 'empty-is-falsy',
message: 'Value "https://example.com" should be falsy',
path: ['empty'],
path: ['info', 'contact', 'url'],
}),
expect.objectContaining({
code: 'empty-is-falsy',
message: 'Value Array[] should be falsy',
path: ['bar', 'empty'],
path: ['servers'],
}),
expect.objectContaining({
code: 'empty-is-falsy',
message: 'Value Object{} should be falsy',
path: ['foo', 'empty'],
path: ['info'],
}),
]),
);
Expand Down
65 changes: 13 additions & 52 deletions src/__tests__/spectral.jest.test.ts
View file
Open in desktop
Expand Up @@ -133,7 +133,7 @@ describe('Spectral', () => {
}),
expect.objectContaining({
code: 'oas2-schema',
path: ['paths', '/todos/{todoId}', 'get', 'responses', '200', 'schema'],
path: [],
range: {
end: {
character: 1,
Expand Down Expand Up @@ -231,18 +231,10 @@ describe('Spectral', () => {
}),

expect.objectContaining({
path: [
'paths',
'/test',
'get',
'responses',
'400',
'content',
'application/json',
'schema',
'properties',
'status_code',
],
path: ['components', 'schemas', 'Error', 'properties', 'status_code'],
message:
"String typed properties MUST be further described using 'maxLength'. Error: #/components/schemas/Error/properties/status_code/maxLength property is not truthy",

source: expect.stringContaining('/src/__tests__/__fixtures__/gh-658/lib.yaml'),
range: {
end: {
Expand All @@ -256,19 +248,10 @@ describe('Spectral', () => {
},
}),
expect.objectContaining({
path: [
'paths',
'/test',
'get',
'responses',
'400',
'content',
'application/json',
'schema',
'properties',
'test',
],
path: ['components', 'schemas', 'Foo'],
source: expect.stringContaining('/src/__tests__/__fixtures__/gh-658/URIError.yaml'),
message:
"String typed properties MUST be further described using 'maxLength'. Error: #/components/schemas/Foo/maxLength property is not truthy",
range: {
end: {
character: 18,
Expand All @@ -282,18 +265,7 @@ describe('Spectral', () => {
}),

expect.objectContaining({
path: [
'paths',
'/test',
'get',
'responses',
'500',
'content',
'application/json',
'schema',
'properties',
'status_code',
],
path: ['components', 'schemas', 'Error', 'properties', 'status_code'],
Copy link
Contributor

Choose a reason for hiding this comment

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

This test shows that we're reporting far more issues than we should - especially now that errors are reported in context of source file (which is great).

This test should be, I think, 3 errors instead of 10 (!) errors. Could have a simple util that, given a IRuleResult, returns a fingerprint that is basically a unique id that can be used to compare if two results should be considered equal. Default fingerprint implementation could just hash(code + JSON.stringify(path) + source) or something. Fingerprints could be used by another func to remove duplicates. from a result set.

Copy link
Contributor

@marbemac marbemac Dec 16, 2019 •
edited

Choose a reason for hiding this comment

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

Ok I took a stab at this here, should result in far less noise in results - #856

source: expect.stringContaining('/src/__tests__/__fixtures__/gh-658/lib.yaml'),
range: {
end: {
Expand All @@ -307,18 +279,7 @@ describe('Spectral', () => {
},
}),
expect.objectContaining({
path: [
'paths',
'/test',
'get',
'responses',
'500',
'content',
'application/json',
'schema',
'properties',
'test',
],
path: ['components', 'schemas', 'Foo'],
source: expect.stringContaining('/src/__tests__/__fixtures__/gh-658/URIError.yaml'),
range: {
end: {
Expand Down Expand Up @@ -362,7 +323,7 @@ describe('Spectral', () => {
},
}),
expect.objectContaining({
path: ['components', 'schemas', 'Error', 'properties', 'test'],
path: ['components', 'schemas', 'Foo'],
source: expect.stringContaining('/src/__tests__/__fixtures__/gh-658/URIError.yaml'),
range: {
end: {
Expand All @@ -377,7 +338,7 @@ describe('Spectral', () => {
}),

expect.objectContaining({
path: ['components', 'schemas', 'Baz', 'properties', 'status_code'],
path: ['components', 'schemas', 'Error', 'properties', 'status_code'],
source: expect.stringContaining('/src/__tests__/__fixtures__/gh-658/lib.yaml'),
range: {
end: {
Expand All @@ -391,7 +352,7 @@ describe('Spectral', () => {
},
}),
expect.objectContaining({
path: ['components', 'schemas', 'Baz', 'properties', 'test'],
path: ['components', 'schemas', 'Foo'],
source: expect.stringContaining('/src/__tests__/__fixtures__/gh-658/URIError.yaml'),
range: {
end: {
Expand Down
6 changes: 3 additions & 3 deletions src/__tests__/spectral.test.ts
View file
Open in desktop
Expand Up @@ -188,11 +188,11 @@ describe('spectral', () => {
path: ['foo', 'bar', 'baz'],
range: {
end: {
character: 0,
character: 12,
line: 0,
},
start: {
character: 0,
character: 7,
line: 0,
},
},
Expand Down Expand Up @@ -254,7 +254,7 @@ describe('spectral', () => {
{
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'],
path: ['responses', 'GetAgreementsOk', 'headers'],
range: expect.any(Object),
severity: DiagnosticSeverity.Error,
source,
Expand Down
8 changes: 4 additions & 4 deletions src/cli/services/__tests__/linter.test.ts
View file
Open in desktop
Expand Up @@ -575,7 +575,7 @@ describe('Linter service', () => {
expect.objectContaining({
code: 'oas2-schema',
message: '/info Property foo is not expected to be here',
path: ['info'],
path: ['definitions', 'info'],
range: {
end: {
character: 5,
Expand All @@ -591,7 +591,7 @@ describe('Linter service', () => {
expect.objectContaining({
code: 'info-description',
message: 'OpenAPI object info `description` must be present and non-empty string.',
path: ['info', 'description'], // todo: relative path or absolute path? there is no such path in linted file, but there is such in spec when working on resolved file
path: ['definitions', 'info', 'description'],
range: {
end: {
character: 22,
Expand Down Expand Up @@ -626,7 +626,7 @@ describe('Linter service', () => {
expect.objectContaining({
code: 'oas2-schema',
message: "/info should have required property 'title'",
path: ['info'],
path: [],
range: {
end: {
character: 1,
Expand All @@ -642,7 +642,7 @@ describe('Linter service', () => {
expect.objectContaining({
code: 'info-description',
message: 'OpenAPI object info `description` must be present and non-empty string.',
path: ['info', 'description'],
path: ['description'],
range: {
end: {
character: 18,
Expand Down
12 changes: 2 additions & 10 deletions src/functions/__tests__/truthy.test.ts
View file
Open in desktop
Expand Up @@ -23,23 +23,15 @@ describe('truthy', () => {
test('should return an error message if target value is falsy', () => {
expect(runTruthy(false)).toEqual([
{
message: 'property is not truthy',
message: '{{fullPath}} property is not truthy',
},
]);
});

test('should return an error message if target value is null', () => {
expect(runTruthy(null)).toEqual([
{
message: 'property is not truthy',
},
]);
});

test('should return a detailed error message if target path is set', () => {
expect(runTruthy(null, ['a', 'b'])).toEqual([
{
message: 'a.b is not truthy',
message: '{{fullPath}} property is not truthy',
},
]);
});
Expand Down
2 changes: 1 addition & 1 deletion src/functions/truthy.ts
View file
Open in desktop
Expand Up @@ -4,7 +4,7 @@ export const truthy: IFunction = (targetVal, _opts, paths): void | IFunctionResu
if (!targetVal) {
return [
{
message: `${paths.target ? paths.target.join('.') : 'property'} is not truthy`,
message: '{{fullPath}} property is not truthy',
P0lip marked this conversation as resolved.
Show resolved Hide resolved
},
];
}
Expand Down
73 changes: 31 additions & 42 deletions src/linter.ts
View file
Open in desktop
@@ -1,14 +1,13 @@
import { JsonPath } from '@stoplight/types';
import { get, has } from 'lodash';
import { get, isObject } from 'lodash';

const { JSONPath } = require('jsonpath-plus');

import { decodePointerFragment, pathToPointer } from '@stoplight/json';
import { Resolved } from './resolved';
import { message } from './rulesets/message';
import { getDefaultRange, Resolved } from './resolved';
import { IMessageVars, message } from './rulesets/message';
import { getDiagnosticSeverity } from './rulesets/severity';
import { IFunction, IGivenNode, IRuleResult, IRunRule, IThen } from './types';
import { isObject } from './utils';
import { getClosestJsonPath } from './utils';

// TODO(SO-23): unit test but mock whatShouldBeLinted
export const lintNode = (
Expand Down Expand Up @@ -91,53 +90,43 @@ export const lintNode = (
results.push(
...targetResults.map<IRuleResult>(result => {
const escapedJsonPath = (result.path || targetPath).map(segment => decodePointerFragment(String(segment)));
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);
const parsed = resolved.getParsedForJsonPath(escapedJsonPath);
const path = parsed?.path || getClosestJsonPath(resolved.resolved, escapedJsonPath);
const doc = parsed?.doc || resolved.parsed;
const range = doc.getLocationForJsonPath(doc.parsed, path, true)?.range || getDefaultRange();

const vars: IMessageVars = {
property: path.length > 0 ? path[path.length - 1] : '',
path: pathToPointer(path),
error: result.message,
...(parsed?.missingPropertyPath && { fullPath: pathToPointer(parsed.missingPropertyPath) }),
description: rule.description,
get value() {
// let's make `value` lazy
const value = get(parsed?.doc.parsed.data, path);
if (isObject(value)) {
return Array.isArray(value) ? 'Array[]' : 'Object{}';
}

return JSON.stringify(value);
},
};

const resultMessage = message(result.message, vars);
vars.error = resultMessage;

return {
code: rule.name,

message:
rule.message === undefined
? rule.description || result.message
: message(rule.message, {
error: result.message,
property: path.length > 0 ? path[path.length - 1] : '',
path: pathToPointer(path),
description: rule.description,
get value() {
// let's make it `value` lazy
const value = resolved.getValueForJsonPath(path);
if (isObject(value)) {
return Array.isArray(value) ? 'Array[]' : 'Object{}';
}

return JSON.stringify(value);
},
}),
message: rule.message === undefined ? rule.description || resultMessage : message(rule.message, vars),
path,
severity: getDiagnosticSeverity(rule.severity),
source: location.uri,
range: location.range,
source: parsed?.doc.source,
range,
};
}),
);
}

return results;
};

// todo: revisit -> https://github.com/stoplightio/spectral/issues/608
function getClosestJsonPath(data: unknown, path: JsonPath) {
if (!isObject(data)) return [];

while (path.length > 0 && !has(data, path)) {
path.pop();
}

return path;
}