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: absolute paths #839

Merged
merged 11 commits into from Dec 19, 2019
Merged

fix: absolute paths #839

merged 11 commits into from Dec 19, 2019

Conversation

P0lip
Copy link
Contributor

@P0lip P0lip commented Dec 7, 2019

Fixes #608 and #685 (comment)

Checklist

  • Tests added / updated
  • Docs added / updated

Does this PR introduce a breaking change?

  • Yes
  • No

Additional context

From now on, Spectral will never try to generate paths that do not point to any value in the document.
Previously, we used to produce paths that did exist in the resolved document, but not necessarily in the actual unresolved document.
This is particularly crucial for tooling which mostly works with resolved documents, therefore need to operate on accurate paths. In the former specification, we simply threw a path pointing to a place in a resolved document.
https://github.com/stoplightio/spectral/pull/839/files#diff-62b31a98be4036e65207ec7971253e93 - this can be easily observed over here. The actual error was caused in the referenced file, which does not have any paths object with responses.

@P0lip P0lip self-assigned this Dec 7, 2019
@P0lip P0lip added the t/bug Something isn't working label Dec 11, 2019
@P0lip P0lip marked this pull request as ready for review December 11, 2019 18:27
@P0lip
Copy link
Contributor Author

P0lip commented Dec 11, 2019

cc @nulltoken

@nulltoken
Copy link
Contributor

cc @nulltoken

@P0lip Thanks for the ping!

I'll come back to you tomorrow with some feedback.

Cool with you?

@nulltoken
Copy link
Contributor

nulltoken commented Dec 12, 2019

@P0lip Feedback!!

Running the tip of this branch against #658 repro case generates the following:

$ git log -1
commit 4ccc94c4678899ad15076c4c14f8c8c1d0deac13 (HEAD -> fix/path-result, upstream/fix/path-result)
Merge: ee8b7d9 c9017a5
Author: Jakub Rożek <jakub@stoplight.io>
Date:   Wed Dec 11 19:26:44 2019 +0100

    Merge branch 'develop' into fix/path-result

$ 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 ./rep
ro/URIError.yaml
Missing baseUrl in compilerOptions. tsconfig-paths will be skipped
Function 'oasOpSecurityDefined' could not be loaded: Not Found
Function 'oasOpParams' 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 'oasTagDefined' could not be loaded: Not Found
Function 'refSiblings' could not be loaded: Not Found
Function 'oasPathParam' 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: 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: paths./test.get.responses.400.content.application/json.schema.properties.error_description.maxLength is n
ot 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 tru
thy

✖ 4 problems (0 errors, 4 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.

Which slightly puzzles me as the produced error in the referenced file

  • does not contain any reference any more to components.schemas.Error.properties.error. paths
  • still references paths./test.get.responses.400.* paths that do not exist in this file (and the Additional context section of that PR seems to hint that you were willing to actually prevent that ("we used to produce paths that did exist in the resolved document, but not necessarily in the actual unresolved document. [snip] The actual error was caused in the referenced file, which does not have any paths object with responses."))

@P0lip
Copy link
Contributor Author

P0lip commented Dec 12, 2019

@nulltoken ah yeah, the paths included in error message generated by Ajv or custom functions are the whole different story yet certainly still something we need to address.
What I meant is that path in the validation result should contain correct path, but well, error message may indeed point at different place, which is kind of misleading.

@P0lip
Copy link
Contributor Author

P0lip commented Dec 12, 2019

@nulltoken
image we need something more or less like this, I'm going to make it happen today.

@P0lip
Copy link
Contributor Author

P0lip commented Dec 12, 2019

@nulltoken dooone, but I'll need to cover other spots too. At the very moment it's only truthy function.
Will be a good chance to improve paths in general and address #815

'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

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

src/functions/truthy.ts Outdated Show resolved Hide resolved
@nulltoken
Copy link
Contributor

@P0lip Do you foresee to include this in the 5.0 release?

@P0lip
Copy link
Contributor Author

P0lip commented Dec 17, 2019

@nulltoken yup! It's the last item blocking the release.

Copy link
Contributor

@marbemac marbemac left a comment

Choose a reason for hiding this comment

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

This looks good to me, can follow up re path cleanup and better de-duping in the other two PRs. Nice work!

@github-actions github-actions bot merged commit 22301f4 into develop Dec 19, 2019
@P0lip P0lip deleted the fix/path-result branch December 19, 2019 01:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spectral should generate paths for unresolved documents
3 participants