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

Add support for unevaluatedProperties + a few other changes. #419

Merged

Conversation

tobz
Copy link
Contributor

@tobz tobz commented Mar 15, 2023

Closes #288.

This PR adds support for unevaluatedProperties when using the draft201909 or draft202012 feature flags.

Overall approach

I took a somewhat similar approach to the Python code linked in the issue for this feature, where the validator for unevaluatedProperties uses a subvalidator for each relevant keyword -- properties, patternProperties, dependentSchemas, $ref, etc -- that performs the relevant logic for determining if the given schema for the keyword would evaluate the property.

Each subvalidator exposes an interface that looks a lot like the Validate trait, but is oriented towards checking solely if a property would be valid against the schema, and has an optional return value to indicate evaluated vs not evaluated. The validator code, overall, is a bit verbose, not unlike additionalProperties... but there's a possibility of turning the API into an actual trait which would theoretically allow us to reduce a lot of boilerplate when we check each subvalidator in UnevaluatedPropertiesValidator::is_valid_property, and so on.

Code deduplication and refactoring

Beyond that, I ended up refactoring a little bit of the shared code between unevaluatedProperties and additionalProperties. I tried, initially, to carry over the logic for switching between a small and big validator map, but it got a little too complex when considering that UnevaluatedPropertiesValidator is inherently recursive, and it didn't make a lot of sense propagating a generic validator map type all the way through. I'm happy to try and revisit this if you think it matters enough.

Fixing $ref for draft 2019-09 and newer

In order to fully support unevaluatedProperties, I had to do a small bit of refactoring in terms of how $ref is handled. This also had the nice side effect of fixing $ref usage when using draft 2019-09 and newer, as now it can be evaluated alongside other keywords.

Beyond that, there's just a lot of boilerplate, and I tried to pick a code style/pattern and just stick to that and be consistent about it, but I'm happy to do any of the aforementioned refactoring ideas, or anything else, to bring the code more in line with your expectations.

@tobz
Copy link
Contributor Author

tobz commented Mar 15, 2023

Seeing the CI: I'll rebase this locally in a little while to get this using a proper conventional commit message.

Copy link
Owner

@Stranger6667 Stranger6667 left a comment

Choose a reason for hiding this comment

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

Amazing job, thank you! I have one minor comment :)

jsonschema/src/keywords/unevaluated_properties.rs Outdated Show resolved Hide resolved
@tobz tobz force-pushed the draft-2019-09-unevaluated-properties branch from 1108842 to e43f0fd Compare March 15, 2023 19:21
@tobz tobz force-pushed the draft-2019-09-unevaluated-properties branch from e43f0fd to a37e8ca Compare March 15, 2023 19:21
@tobz tobz requested a review from Stranger6667 March 15, 2023 19:22
@codecov
Copy link

codecov bot commented Mar 15, 2023

Codecov Report

Merging #419 (a37e8ca) into master (848b7b1) will increase coverage by 0.80%.
The diff coverage is 83.68%.

@@            Coverage Diff             @@
##           master     #419      +/-   ##
==========================================
+ Coverage   78.59%   79.40%   +0.80%     
==========================================
  Files          54       56       +2     
  Lines        4481     5015     +534     
==========================================
+ Hits         3522     3982     +460     
- Misses        959     1033      +74     
Impacted Files Coverage Δ
jsonschema/src/lib.rs 66.66% <ø> (+9.52%) ⬆️
jsonschema/src/paths.rs 75.29% <0.00%> (-1.82%) ⬇️
jsonschema/src/keywords/ref_.rs 80.88% <57.14%> (-6.40%) ⬇️
jsonschema/src/error.rs 58.28% <62.50%> (+0.56%) ⬆️
jsonschema/src/properties.rs 63.63% <63.63%> (ø)
jsonschema/src/schemas.rs 89.18% <80.00%> (-0.44%) ⬇️
jsonschema/src/keywords/unevaluated_properties.rs 87.12% <87.12%> (ø)
jsonschema/src/compilation/mod.rs 85.54% <88.88%> (+4.86%) ⬆️
jsonschema/src/keywords/additional_properties.rs 84.81% <100.00%> (+1.85%) ⬆️
jsonschema/src/validator.rs 83.33% <100.00%> (+1.51%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Stranger6667
Copy link
Owner

Great PR and a great write-up! :) Thank you so much for this!

@Stranger6667 Stranger6667 merged commit 6349460 into Stranger6667:master Mar 16, 2023
@tobz tobz deleted the draft-2019-09-unevaluated-properties branch March 16, 2023 11:54
@tobz
Copy link
Contributor Author

tobz commented Mar 16, 2023

@Stranger6667 🙇🏻 Happy to be able to contribute back.

Do you have a typical release cycle for jsonschema? Just doing a little planning on my end if I should keep my fork around or not.

@Stranger6667
Copy link
Owner

I don't but I'd like to make a new release today :)

@tobz
Copy link
Contributor Author

tobz commented Mar 16, 2023

Great, I'll keep an eye out for when that happens.

Appreciate the timeliness on the review and the release; certainly did not expect my PR to be getting this much traction so quickly. 😂

@Stranger6667
Copy link
Owner

My pleasure :) Happy to see your contribution and that's great that jsonschema is getting closer to the full Draft 2019-09 support :)

@Stranger6667
Copy link
Owner

0.17.0 is released :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement unevaluatedProperties
2 participants