Upgrade JSON schema validator + fixes #491
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This didn't seem to fail due to missing evaluation in the currently used version of `com.networknt:json-schema-validator`.
yuanchen233
previously approved these changes
Oct 26, 2024
Whathecode
force-pushed
the
fix-json-schemas
branch
from
7048b4e to
a229b93
Compare
|
@yuanchen233 This should be a cleaner workaround, to be resolved in #492. This has the benefit that extra unexpected properties on concrete types will still correctly fail. And existing properties are properly evaluated. Just a messy/redundant schema definition which shouldn't be necessary. |
This seems to have introduced better evaluation of `allOf` and `oneOf` references. However, it does seem to introduce a bug which causes it not to consider properties in nested schemas more than 1 level deep when evaluating `unevaluatedProperties`: networknt/json-schema-validator#1123 Therefore, a hack was introduced to make sure validation succeeds.. Note that not all concrete types are currently evaluated (#404), so there are many other schemas which would also fail if they were to be evaluated, and they would need to apply a similar fix.
With the upgrade to the latest JSON schema validator, internal anchor references (within a schema) now work. Types which should only be used internally still use the longer paths to `$defs`. This has the added benefit that these anchors are also evaluated. It turned out that some anchors in `ParticipantGroupStatus` were wrong, and in `DataStreamServiceRequest` one was missing. Closes #353
Whathecode
force-pushed
the
fix-json-schemas
branch
from
a229b93 to
f48e063
Compare
yuanchen233
approved these changes
Oct 27, 2024
There was a problem hiding this comment.
Are there conventions for when to use an anchor and when not?
Are we using anchor for all subschemas? Could you also leave a note/answer to your own question in #353 for future referance? 😀
Of course! 👍🙂 Check the last commit description of this PR. I can't copy paste it now (I'm on phone). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was an error in the
DeviceDeploymentStatusJSON schema which wasn't spotted by the JSON schema validator.Upgrading to the latest version definitely provides better validation, but in addition also seems to introduce a bug in the evaluation of
unevaluatedProperties(networknt/json-schema-validator#1123). For now, I have removed these where it caused validation to fail, thus resulting in less strict/correct schema definitions. If we decide to merge this PR, we need to create a new bug to fix this once the bug is fixed in the schema validator.On the upside, with the new version of
networknt /json-schema-validatorit should be possible to use more concise anchor references (#353). I'll add this as a separate commit later.