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 look-around patterns using fancy-regex #220

Merged
merged 7 commits into from
May 7, 2021

Conversation

duckontheweb
Copy link
Contributor

Uses fancy-regex in place of regex for matching against all patterns in JSON schemas.

regex is still used when converting a RegEx for ECMA 262 to take advantage of the Regex::replace_all method, which is not included in fancy-regex. Since fancy-regex depends on regex anyway, this does not affect the final size of the compiled binary (thanks to this commit for the idea).

I also removed the test from #214, since that pattern is supported in this PR.

@codecov
Copy link

codecov bot commented May 6, 2021

Codecov Report

Merging #220 (aa5c67f) into master (0aabba8) will decrease coverage by 0.04%.
The diff coverage is 79.24%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #220      +/-   ##
==========================================
- Coverage   82.70%   82.65%   -0.05%     
==========================================
  Files          53       53              
  Lines        3590     3615      +25     
==========================================
+ Hits         2969     2988      +19     
- Misses        621      627       +6     
Impacted Files Coverage Δ
jsonschema/src/error.rs 63.27% <25.00%> (-0.73%) ⬇️
jsonschema/src/keywords/pattern_properties.rs 80.26% <66.66%> (-1.08%) ⬇️
jsonschema/src/keywords/pattern.rs 88.23% <78.57%> (-6.11%) ⬇️
jsonschema/src/keywords/additional_properties.rs 83.78% <100.00%> (-0.48%) ⬇️
jsonschema/src/keywords/format.rs 93.54% <100.00%> (+1.34%) ⬆️
jsonschema/src/keywords/one_of.rs 87.17% <0.00%> (-0.63%) ⬇️
jsonschema/src/lib.rs 100.00% <0.00%> (ø)
jsonschema/src/paths.rs 83.33% <0.00%> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0aabba8...aa5c67f. Read the comment docs.

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.

I like the direction and it looks good to me :)

I think that it is important to add a new variant to the ValidationErrorKind enum that will indicate the backtracking issue and use it in PatternValidator::validate and other cases when patterns are used (additionalProperties)

jsonschema/src/keywords/pattern.rs Outdated Show resolved Hide resolved
jsonschema/src/keywords/pattern.rs Outdated Show resolved Hide resolved
jsonschema/src/keywords/pattern.rs Outdated Show resolved Hide resolved
@Stranger6667
Copy link
Owner

Maybe one extra thing - it will be nice to have a test case for pattern with backtracking

@duckontheweb
Copy link
Contributor Author

@Stranger6667 How would you like me to handle the commits to address these comments. Should I squash them with the existing commit?

@Stranger6667
Copy link
Owner

In the end, I plan to squash them into one, so feel free to push more :)

@duckontheweb
Copy link
Contributor Author

I think I addressed all of the suggestions above via the new commits. I also added matching and non-matching test cases to pattern.rs.

I had a thought that it would be good to add an integration test for the combination of patternProperties, properties, and additionalProperties that I mentioned in this comment, but I wasn't sure how to fit it in with the official JSON Schema test suite.

@Stranger6667
Copy link
Owner

Looks great to me! :) Thank you for contributing. I appreciate it :)

I'll check it one more time tomorrow and probably will release these changes in the new release :)

@duckontheweb
Copy link
Contributor Author

I'm not sure what to make of that clippy failure. It doesn't show up when I run cargo clippy --all-targets --all-features -- -D warnings locally, but I'm on a Mac. I'll try running in an Ubuntu container as well to see if I can replicate it.

@duckontheweb
Copy link
Contributor Author

Looks great to me! :) Thank you for contributing. I appreciate it :)

I'll check it one more time tomorrow and probably will release these changes in the new release :)

No problem, thanks for bearing with me on this. It's my first contribution to a Rust project and it's been really educational to see how this project is set up.

@Stranger6667
Copy link
Owner

I'm not sure what to make of that clippy failure. It doesn't show up when I run cargo clippy --all-targets --all-features -- -D warnings locally, but I'm on a Mac. I'll try running in an Ubuntu container as well to see if I can replicate it.

Hm, maybe it is connected to the recent Rust toolchain update 🤷 I'll apply a fix after merge :)

pub(crate) const fn backtrack_limit(

@Stranger6667
Copy link
Owner

No problem, thanks for bearing with me on this. It's my first contribution to a Rust project and it's been really educational to see how this project is set up.

I am glad to hear that! :)

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.

2 participants