HV-1328 Add an option to validate class-level constraints only if all property constraints are valid #1031
Conversation
There was a problem hiding this comment.
@marko-bekhta thanks for working on this. I added a couple of comments.
| @@ -471,7 +471,7 @@ private void validateConstraintsForCurrentGroup(BaseBeanValidationContext<?> val | |||
|
|
|||
| for ( Group defaultSequenceMember : groupOfGroups ) { | |||
| validationSuccessful = validateConstraintsForSingleDefaultGroupElement( validationContext, valueContext, validatedInterfaces, clazz, | |||
| metaConstraints, defaultSequenceMember ); | |||
| metaConstraints, defaultSequenceMember ) && validationSuccessful; | |||
There was a problem hiding this comment.
Could you open a separate issue for that one? A test case might be in order too as it looks a lot like a bug!
| @@ -119,6 +119,16 @@ | |||
| @Incubating | |||
| String GETTER_PROPERTY_SELECTION_STRATEGY_CLASSNAME = "hibernate.validator.getter_property_selection_strategy"; | |||
There was a problem hiding this comment.
typo in commit message: s/thorough/through/
| * @since 6.1.0 | ||
| */ | ||
| @Incubating | ||
| String IGNORE_CLASS_LEVEL_ON_PROPERTY_FAIL = "hibernate.validator.ignore_class_level_on_property_fail"; |
There was a problem hiding this comment.
I wonder if we should try to find a naming similar to the fail fast mode as it's really similar to it.
Maybe fail_fast_on_property_violation?
It's not easy to find a right name for this sort of configuration knob.
Happy to discuss it further (maybe it will be easier on Zulip).
| metaConstraints, defaultSequenceMember ) && validationSuccessful; | ||
| boolean propertyValidationResult = validateConstraintsForSingleDefaultGroupElement( validationContext, valueContext, validatedInterfaces, clazz, propertyMetaConstraints, defaultSequenceMember ); | ||
| validationSuccessful = propertyValidationResult && validationSuccessful; | ||
| if ( !validationContext.isIgnoreClassLevelOnPropertyFailModeEnabled() || propertyValidationResult ) { |
There was a problem hiding this comment.
I'm wondering if having these two calls could slow down things. I wonder if we should have a shortcut if this mode is disabled.
Also wondering if we should test if the set is empty (can't say if it will be a win though but it looks like something we should do - and bench).
In any case, we should test it.
| ); | ||
| } | ||
|
|
||
| interface Group1 extends Group11, Group12, Group13, Group14, Group15, Group16, Group17, Group18, Group19 { |
There was a problem hiding this comment.
As that loop in which validationSuccessful is set is based on a Set it actually doesn't give consistent results. To be able to reproduce it more often I've added a lot of groups, which increase the probability that true will override the result in the end.
EDIT I think this was introduced in 94f4f56 So we probably need to backport this one.
|
And here's the benchmark results that I've got from yesterday: 6.1
Benchmark Mode Cnt Score Error Units
SimpleValidation.testSimpleBeanValidation thrpt 20 1595.544 ± 127.297 ops/ms
CascadedValidation.testCascadedValidation thrpt 20 377.533 ± 6.882 ops/ms
CascadedWithLotsOfItemsAndMoreConstraintsValidation.testCascadedValidationWithLotsOfItems thrpt 20 1012.516 ± 12.189 ops/s
CascadedWithLotsOfItemsValidation.testCascadedValidationWithLotsOfItems thrpt 20 1147.559 ± 13.166 ops/s
current:
Benchmark Mode Cnt Score Error Units
SimpleValidation.testSimpleBeanValidation thrpt 20 1628.147 ± 185.557 ops/ms
CascadedValidation.testCascadedValidation thrpt 20 332.512 ± 5.129 ops/ms
CascadedWithLotsOfItemsAndMoreConstraintsValidation.testCascadedValidationWithLotsOfItems thrpt 20 1039.538 ± 12.060 ops/s
CascadedWithLotsOfItemsValidation.testCascadedValidationWithLotsOfItems thrpt 20 1127.804 ± 13.872 ops/s |
|
@marko-bekhta do we have a benchmark with property constraints + class level constraints? Because that would be an interesting one :). I'm not sure the others are significant in this case. Although we should probably have a closer look to |
|
jenkins retest this please |
|
I've added another benchmark that has both class and property level constraints. The results are: with changes: I also tried to run the cascading benchmarks again. The results are all around the place 🙄 . I'll try to run these on my laptop later today to see how they would behave there. |
|
While looking through the PRs I've realized that I had not added those benchmarks.... So here they are: #nochange (current)
Benchmark Mode Cnt Score Error Units
SimpleClassPropertyValidation.testSimpleBeanValidation thrpt 20 1694.371 ± 22.571 ops/ms
#chagne (changes in this PR)
Benchmark Mode Cnt Score Error Units
SimpleClassPropertyValidation.testSimpleBeanValidation thrpt 20 1518.953 ± 29.393 ops/ms
# update (not in this PR, additional if to check if the option is enabled and if not use the "current" all constraints collections)
Benchmark Mode Cnt Score Error Units
SimpleClassPropertyValidation.testSimpleBeanValidation thrpt 20 1679.028 ± 33.595 ops/ms |
|
@marko-bekhta so I think it makes it clear we need the additional test. Any chance you could push this to the PR (your report above says it wasn't pushed)? |
|
I need to do a quick release so I'm merging the first commit to master and 6.0 as it looks like a bug to me. |
|
@gsmet yes, it totally make sense to break this into two pieces. I'll look for that additional commit and push it here when I find it :) |
|
@marko-bekhta hey Marko. I tried to contact you by email but I didn't hear from you? Did you get my email from December? |
- split set of all constraints in bean metadata into class level and property constraints. - allow users to configure failFastOnPropertyViolation through property or programmatic config - use the property to determine if the class level constraints should be ignored.
marko-bekhta
force-pushed
the
HV-1328
branch
from
6d6dbbc
to
4661a86
Compare
First commit has a change for what looked like a strange behavior. the value only for the last group was considered....
Any ideas on naming of the "mode" is also welcomed. For now PR is missing the doc.