fix: improve error source detection #685

P0lip · 2019-10-15T13:14:07Z

Fixes #658

Checklist

Tests added / updated
Docs added / updated

Does this PR introduce a breaking change?

Yes
No

Additional context

We need to figure #608 out as soon as possible.
Basically, we need to make a call whether or not to return validation results that cannot be associated with any document, as we do now.
If we happen to do so, we'll keep reporting duplicate errors under certain circumstances.

marbemac

We should probably dig in re the question you pose in the original PR description soon.

marbemac · 2019-12-01T01:33:14Z

src/resolved.ts

+        while ($ref in refMap) {
+          $ref = refMap[$ref];
+        }


This is an odd looking piece of code, could you add a comment describing what this is for?

Or just in general maybe this entire hasRef logic tree (line 55-74) could be pulled out to its own function, with a comment describing what it does (or some descriptive tests)?

marbemac

From code perspective looks alright to me, but I didn't do any verification it fixes the original issue in a satisfactory way. @nulltoken if you have time, would be awesome if you could spin this PR branch up and do a quick verification that the changes here solve #658.

marbemac · 2019-12-06T00:18:12Z

src/utils/__tests__/refs.jest.test.ts

@@ -0,0 +1,66 @@
+import { extractSourceFromRef, traverseObjUntilRef } from '..';
+
+describe('$ref utils', () => {


These might be a good fit for the JSON package (where we already have a bunch of pointer helpers etc)?

stoplightio/json#38

src/utils/hasRef.ts

nulltoken · 2019-12-06T08:56:04Z

@marbemac @P0lip Did a quick test. No more URIError warns! 🎉

REDACTED@XXXXX MINGW64 /c/_work/spectral (fix/uri-error)
$ git log -1
commit a1cf6c206eb3cd11c4bdcb680bc54a382f13e61d (HEAD -> fix/uri-error, upstream/fix/uri-error)
Author: Jakub Rożek <jakub@stoplight.io>
Date:   Tue Dec 3 14:29:33 2019 +0100

    fix: normalize source

REDACTED@XXXXX MINGW64 /c/_work/spectral (fix/uri-error)
$ yarn cli lint --ruleset ./repro/ruleset.yaml ./repro/URIError.yaml
yarn run v1.19.2
$ node -r ts-node/register -r tsconfig-paths/register src/cli/index.ts lint --ruleset ./repro/ruleset.yaml ./repro/URIError.yaml
Missing baseUrl in compilerOptions. tsconfig-paths will be skipped
Function 'oasOpSecurityDefined' could not be loaded: Not Found
Function 'oasPathParam' could not be loaded: Not Found
Function 'oasOp2xxResponse' could not be loaded: Not Found
Function 'oasOpFormDataConsumeCheck' could not be loaded: Not Found
Function 'oasOpIdUnique' could not be loaded: Not Found
Function 'oasOpParams' could not be loaded: Not Found
Function 'refSiblings' could not be loaded: Not Found
Function 'oasTagDefined' could not be loaded: Not Found
OpenAPI 3.x detected

c:/_work/spectral/repro/URIError.yaml
 23:22  warning  schema-strings-maxLength  String typed properties MUST be further described using 'maxLength'. Error: paths./test.get.responses.200.content.application/json.schema.maxLength is not truthy

https://[REDACTED]]/static/schemas/common/v2/library.openapi.yaml
 65:15  warning  schema-strings-maxLength  String typed properties MUST be further described using 'maxLength'. Error: components.schemas.Error.properties.error.maxLength is not truthy
 65:15  warning  schema-strings-maxLength  String typed properties MUST be further described using 'maxLength'. Error: paths./test.get.responses.400.content.application/json.schema.properties.error.maxLength is not truthy
 67:27  warning  schema-strings-maxLength  String typed properties MUST be further described using 'maxLength'. Error: components.schemas.Error.properties.error_description.maxLength is not truthy
 67:27  warning  schema-strings-maxLength  String typed properties MUST be further described using 'maxLength'. Error: paths./test.get.responses.400.content.application/json.schema.properties.error_description.maxLength is not truthy
 69:21  warning  schema-strings-maxLength  String typed properties MUST be further described using 'maxLength'. Error: components.schemas.Error.properties.status_code.maxLength is not truthy
 69:21  warning  schema-strings-maxLength  String typed properties MUST be further described using 'maxLength'. Error: paths./test.get.responses.400.content.application/json.schema.properties.status_code.maxLength is not truthy

✖ 7 problems (0 errors, 7 warnings, 0 infos, 0 hints)

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

However, I'm on a bit on the fence regarding the reported errors....

c:/_work/spectral/repro/URIError.yaml
 23:22  warning  schema-strings-maxLength  String typed properties MUST be further described
using 'maxLength'. Error: paths./test.get.responses.200.content.application/json.schema.maxLength
is not truthy

https://[REDACTED]]/static/schemas/common/v2/library.openapi.yaml
 65:15  warning  schema-strings-maxLength  String typed properties MUST be further 
described using 'maxLength'. Error: components.schemas.Error.properties.error.maxLength is 
not truthy
 65:15  warning  schema-strings-maxLength  String typed properties MUST be further
described using 'maxLength'. Error: paths./test.get.responses.400.content.application/json
.schema.properties.error.maxLength is not truthy

From a "spec fixer" perspective, second [65:15] line reads a bit strange as this file doesn't contain any paths section.
This path only makes sense in the context of the first file. But there's no hint to the file that indirectly suffers from this issue.
If only the issue in URIError.yaml is fixed, my understanding is that Spectral will report the file as fine. Which is somehow wrong as it still references parts of a remote file containing issues.

Thoughts?

P0lip · 2019-12-06T11:34:12Z

This path only makes sense in the context of the first file. But there's no hint to the file that indirectly suffers from this issue.

Yeaaah, I guess we'd need to provide a json path relevant to that file.

If only the issue in URIError.yaml is fixed, my understanding is that Spectral will report the file as fine. Which is somehow wrong as it still references parts of a remote file containing issues.

It the referenced remote file contains issues, that error will still be reported, but for the file containing errors (the referenced one)
Technically, URIError.yaml does not contain any errors, it's the remote reference that does.
I suppose we could add a hint that URIError.yaml references a XYZ file that contains ABC errors.

Note, this change was partially driven by Studio where we show errors relevant to the current files and ignore errors coming from $refs.

nulltoken · 2019-12-06T12:30:46Z

@P0lip

Yeaaah, I guess we'd need to provide a json path relevant to that file.

👍

Technically, URIError.yaml does not contain any errors, it's the remote reference that does.

I do agree.

I suppose we could add a hint that URIError.yaml references a XYZ file that contains ABC errors.

That would also awesome. If that hint would be tied to some new built-in rule and that the user could change the severity of that rule, that would be even awesome-er ;-)

P0lip · 2019-12-06T13:54:00Z

cc @philsturgeon you may find the above convo quite interesting considering the Spectral VS Code plugin.

P0lip · 2019-12-06T19:18:20Z

Alright, so I'm working on the path issue.
Pushed a commit 88eecf6.
Still needs to cleanup this up a little bit as well as update a few tests

marbemac · 2019-12-09T04:40:03Z

Not sure what the two files in question look like, but assuming the following:

c:/_work/spectral/repro/URIError.yaml has a ref at paths./test.get.responses.400 that targets https://[REDACTED]]/static/schemas/common/v2/library.openapi.yaml#/responses/bad-request

I'd expect the resulting error report to look like:

https://[REDACTED]]/static/schemas/common/v2/library.openapi.yaml
 65:15  warning  schema-strings-maxLength  String typed properties MUST be further 
described using 'maxLength'. Error: components.schemas.Error.properties.error.maxLength is 
not truthy
 65:15  warning  schema-strings-maxLength  String typed properties MUST be further
described using 'maxLength'. Error: responses.bad-request.content.application/json
.schema.properties.error.maxLength is not truthy

Basically if we're reporting on another file, still reporting on inner paths relative to that file - in the example above it's the #/responses/bad-request root pointer that's being targeted on the target $ref.

Does that make sense / is that what you were thinking?

nulltoken · 2019-12-09T11:30:33Z

@marbemac

Not sure what the two files in question look like, but assuming the following:

You can the content of the files #658

marbemac

Assuming plan is to consider improvements to the path reporting stuff in a separate PR, this looks good to me 👍.

P0lip added the t/bug Something isn't working label Oct 15, 2019

P0lip self-assigned this Oct 15, 2019

P0lip force-pushed the fix/uri-error branch from 322768e to 0a2519a Compare November 12, 2019 15:18

P0lip marked this pull request as ready for review November 12, 2019 15:20

P0lip requested a review from marbemac November 12, 2019 15:20

P0lip force-pushed the fix/uri-error branch from 94665f7 to 425c2d4 Compare November 27, 2019 23:03

marbemac suggested changes Dec 1, 2019

View reviewed changes

fix: consume graph data in doesBelongToSourceDoc

59daebd

P0lip force-pushed the fix/uri-error branch from 7a54069 to ee47cc1 Compare December 3, 2019 04:37

fix: write detection from scratch

b61fefa

P0lip mentioned this pull request Dec 3, 2019

fix: resolve the source of errors for top-level references #828

Closed

4 tasks

P0lip force-pushed the fix/uri-error branch from ee47cc1 to b61fefa Compare December 3, 2019 05:00

P0lip changed the title ~~fix: consume graph data in doesBelongToSourceDoc~~ fix: improve error source detection Dec 3, 2019

P0lip added 2 commits December 3, 2019 14:16

refactor: flatten parsedMap structure

7030d17

fix: normalize source

a1cf6c2

P0lip requested a review from marbemac December 3, 2019 13:54

marbemac reviewed Dec 6, 2019

View reviewed changes

Merge remote-tracking branch 'origin/develop' into fix/uri-error

2d098cf

P0lip mentioned this pull request Dec 7, 2019

fix: absolute paths #839

Merged

4 tasks

P0lip added 2 commits December 9, 2019 11:52

chore: bump @stoplight/json and use utils

e7ba421

build: rollup config

3444f50

P0lip requested a review from marbemac December 9, 2019 22:54

marbemac approved these changes Dec 10, 2019

View reviewed changes

Merge branch 'develop' into fix/uri-error

b315a87

P0lip merged commit 4d14fa0 into develop Dec 10, 2019

P0lip deleted the fix/uri-error branch December 10, 2019 10:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: improve error source detection #685

fix: improve error source detection #685

P0lip commented Oct 15, 2019 •

edited

marbemac left a comment

marbemac Dec 1, 2019

marbemac Dec 1, 2019

marbemac left a comment

marbemac Dec 6, 2019

P0lip Dec 6, 2019

nulltoken commented Dec 6, 2019

P0lip commented Dec 6, 2019 •

edited

nulltoken commented Dec 6, 2019

P0lip commented Dec 6, 2019

P0lip commented Dec 6, 2019

marbemac commented Dec 9, 2019

nulltoken commented Dec 9, 2019

marbemac left a comment

		@@ -0,0 +1,66 @@
		import { extractSourceFromRef, traverseObjUntilRef } from '..';

		describe('$ref utils', () => {

fix: improve error source detection #685

fix: improve error source detection #685

Conversation

P0lip commented Oct 15, 2019 • edited

marbemac left a comment

Choose a reason for hiding this comment

marbemac Dec 1, 2019

Choose a reason for hiding this comment

marbemac Dec 1, 2019

Choose a reason for hiding this comment

marbemac left a comment

Choose a reason for hiding this comment

marbemac Dec 6, 2019

Choose a reason for hiding this comment

P0lip Dec 6, 2019

Choose a reason for hiding this comment

nulltoken commented Dec 6, 2019

P0lip commented Dec 6, 2019 • edited

nulltoken commented Dec 6, 2019

P0lip commented Dec 6, 2019

P0lip commented Dec 6, 2019

marbemac commented Dec 9, 2019

nulltoken commented Dec 9, 2019

marbemac left a comment

Choose a reason for hiding this comment

P0lip commented Oct 15, 2019 •

edited

P0lip commented Dec 6, 2019 •

edited