-
-
Notifications
You must be signed in to change notification settings - Fork 94
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
feat: configure patterns regex engine #487
base: master
Are you sure you want to change the base?
Conversation
Hello @Stranger6667! Will it be interesting for you guys or it's just my specific use case? |
This looks cool! Sorry for the delay, I’ll check it in detail in the next couple of days and will let you know |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #487 +/- ##
==========================================
- Coverage 89.90% 89.69% -0.21%
==========================================
Files 58 59 +1
Lines 9942 10007 +65
==========================================
+ Hits 8938 8976 +38
- Misses 1004 1031 +27 ☔ View full report in Codecov by Sentry. |
CodSpeed Performance ReportMerging #487 will degrade performances by 20.9%Comparing Summary
Benchmarks breakdown
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the changes! One minor thing re options
Also, please, add a changelog entry :)
Also, maybe instead of
options.with_patterns_regex_engine(RegexEngine::Regex(RegexOptions {
size_limit: Some(5 * (1 << 20)),
..Default::default()
}));
It could be
options.with_patterns_regex_engine(RegexOptions {
size_limit: Some(5 * (1 << 20)),
..Default::default()
});
?
It could work if with_patterns_regex_engine
will accept a generic that can do Into<RegexEngine>
, so implementing Into
will make the API a bit more ergonomic.
P.S. cargo fmt
& clippy
are complaining, but it should be straightforward to fix
format_validator!(RegexValidator, "regex"); | ||
struct RegexValidator { | ||
schema_path: JSONPointer, | ||
config: Arc<CompilationOptions>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe keeping the regex engine options will be enough, instead of storing the whole set of options? Thinking more about just storing the necessary minimum, rather than performance. However, I think as cloning RegexEngine
is cheap, it could be also better performance wise during validation (though for a cost of a pointer indirection)
Fancy Regex supports backtracking which is required for some cases but as a downside is vulnerable to ReDoS attacks. This becomes a decisive factor when an application operates with user-defined schemas. Regex, in turn, doesn't support look-around and backreferences but guarantees linear time matching that mitigates the attack.
This PR enables the configuration of the regex engine for pattern-based keywords:
Regex
orFancyRegex
(by default).The formats still use Fancy Regex. I didn't find a simple way to keep patterns static and configurable at the same time. Probably, the right approach is to add an option to use fast formats such as ajv-formats but this is out of the scope of this PR.