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

Validation of external $ref property values should show error on unexpected object type #353

Closed
tfesenko opened this issue Jun 8, 2017 · 2 comments

Comments

@tfesenko
Copy link
Member

tfesenko commented Jun 8, 2017

Extracted from #323 Validation of $ref property values should show error on unexpected object type

It covers validation of property values which are defined in an external file, but which resides in the same workspace/project, e.g. $ref: "./uber_schemas.yaml#/components/schemas/Product"

[Local] Validation checks that the referenced element is actually what we expect: good for correctness, bad for performance

We check the local references by getting their actual JSON schema type and comparing it to the expected value. It's done by loading the referenced definition and matching it against the JSON schema. It's OK for local references because all elements are already loaded and schema-matched, but it can really slow down editing experience as validation is live and loading/schema-matching is executed on every change.

Solution: "Batch Validate" action

We can add a "Validate References" action. It's OK for it to be slow - it will now influence editing experience as the users will be invoking it explicitly, and it will still allow us to validate references.

Complimentary step: Check the JSON pointer first

We know that the referenced elements must live at a certain location, e.g. schema definitions must have a <optional file url>#/components/schemas/{componentName} pointer, headers must be in <...>#/components/headers/{componentName}, etc.
So, while not all invalid referenced elements have invalid (wrong format) pointers, but all elements with invalid (wrong format) JSON Pointer are invalid values for references.
We can run validation in two steps:

  1. Check the JSON Pointer. It will not guarantee that the referenced element actually has the needed fast, or even that the file is a valid OAS 3 file, but it's fast.
  2. For those elements whose JSON pointers are fine, check their actual type
ghillairet added a commit that referenced this issue Jun 9, 2017
…on unexpected object type

Validation of reference types has been moved to ReferenceValidator that previously only validate correctness of references pointer/URIs.
It now checks the type of the reference if this one is valid, this allow to reuse the code for loading the JSON document which is referenced by it.
This validation could be slow because it will parse the referenced documents into Models.
@ghillairet
Copy link
Member

ghillairet commented Jun 9, 2017

See PR #358

I moved the code that does validation of references into the reference validator that was previously only checking that the URI or pointer actually points to something. By adding a few methods I am able to load the referenced document and parse it as a Model. From that I can check the type of the referenced object.

Only missing in this PR is a few tests that I'll add next week.

Also worth to check that this way of validating references does not impact too much the performance of the editor, as multiple documents have to be parsed during validation.

@tedepstein tedepstein changed the title Validation of external $ref property values should show error on unexpected object type" Validation of external $ref property values should show error on unexpected object type Aug 1, 2017
@tedepstein
Copy link
Collaborator

I ran into this when testing multi-file BeamUp, which has an invalid reference to a parameter object, where reference to a callback object would be expected. Looks like implementation of the fix is already underway in #358.

I have a couple of comments:

  1. Requiring an explicit, specialized user action to validate references seems like a pretty significant usability compromise. Could we have an option, enabled by default, to validate references on save?
  2. As of the last time I looked at the spec, or discussed on a spec call, it's still legal to have references to objects that don't reside in their designated /components/... object map.
  • References can be to a definition that is used somewhere else in context.
  • They can be to an entire file that is a fragment of an OpenAPI document, containing only a single object that conforms to the expected schema.
  • Or they can reside somewhere inside a non-OpenAPI file, as long as it is valid JSON or YAML, and as long as the specified JSON Reference resolves to an object that conforms to the expected schema.

So I don't think we can rely on the path as an indicator that a reference is invalid.

ghillairet added a commit that referenced this issue Sep 22, 2017
@ghillairet ghillairet self-assigned this Sep 27, 2017
ghillairet added a commit that referenced this issue Sep 27, 2017
ghillairet added a commit that referenced this issue Oct 4, 2017
…on unexpected object type

Fix cache of model documents in DocumentManager.
Fix javadoc.
ghillairet added a commit that referenced this issue Oct 5, 2017
tfesenko added a commit that referenced this issue Oct 5, 2017
[#353] Validation of external $ref property values should show error …
@tfesenko tfesenko closed this as completed Oct 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants