Fully Fix Repeatable Constraint Issues #269
Conversation
|
@rbygrave please review and deploy an RC when you get time. |
|
Do you want to state where the bug was in the code? Hence the design thinking around the fix. |
| @@ -32,7 +32,6 @@ public final boolean validate(T value, ValidationRequest req, String propertyNam | |||
| } | |||
| if (!isValid(value)) { | |||
| req.addViolation(message, propertyName); | |||
| return false; | |||
There was a problem hiding this comment.
this was the problem, validation should continue even if a violation is found
There was a problem hiding this comment.
hmm, if so that looks more like a design issue that we need to step back and look at. I'm pondering why the issue was not solely in the andThen() processing [that didn't always execute the second validation].
There was a problem hiding this comment.
that didn't always execute the second validation
the reason is because of the above, the adapter returns false, so the after validator doesn't run
There was a problem hiding this comment.
in this way, only some after handlers would run.
There was a problem hiding this comment.
It's easier to see it when you debug through the code
There was a problem hiding this comment.
[and thus we return void rather than boolean that is always true].
For what purpose? Validators should have the ability to disable further validation if they want. (We can table this discussion for later though)
There was a problem hiding this comment.
What is the use case for halting validation for NotNull and NotBlank? Hibernate doesn't do it, is there a reason for it?
preserve the existing behaviour of avaje validator,
if we're saying it's to preserve existing behavior that doesn't make sense to me as all the adapters have this halting problem and we're changing them.
There was a problem hiding this comment.
Why should avaje differ in this respect and prevent all custom constraints from running unlike hibernate?
This is the debate we can keep going ... in the meantime, lets merge the other PR and rollout the fix preserving the current behaviour that we have for NotBlank.
hibernate doesn't stop validation like we do ...
I knew that, but you are no seeing the consistency issue. For example, some DBMS treat NULL and empty string as the same thing - that's not per spec but that the same consistency issue in a way. I'm saying its not black and white, and that the NotBlank behaviour on avaje validator was intentional [and I'm thinking for you it was a detail that you didn't dig into].
There was a problem hiding this comment.
all the adapters have this halting problem and we're changing them.
Not ALL of them. Most of them yes but not ALL - NotNull, NotBlank, Size etc were deemed correct behaviour.
What is the use case for halting validation for NotNull
It's fairly academic, due to all the validators expecting to treat null as valid apart from NullableAdapter.
and NotBlank? Hibernate doesn't do it, is there a reason for it?
For Strings we pretty much always use NotBlank and not NotNull. For strings, the debate is if there is a semantic difference between a null string and an empty string. Technically the difference is obvious but semantically most of the time we can argue that an empty string is equivalent to "the value is absent".
It's a grey area.
There was a problem hiding this comment.
It's fairly academic, due to all the validators expecting to treat null as valid apart from NullableAdapter.
If that's the case we should commit fully and change AbstractConstraintAdapter to return true on null.
AbstractConstraintAdapterto attempt to execute all constraints instead of canceling a few when a violation is foundFixes #266 for real this time