-
Notifications
You must be signed in to change notification settings - Fork 729
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
BeanValidationBinder says the form is valid even though required (@NotNull) field is not filled #9000
Comments
Indeed the default "null representation" in the case of TextField is one-way change only. There was a long discussion on wether it should change |
Thx for the precision |
I tried to fix this issue but I think it will break some stuff (it does fix NotNull issue but may break NotEmpty or Size min). And there are some differences between withNullRepresentaiton and the "default null representation" that I'm not sure to understand. I'm not quite sure if it's possible to have exactly the "validation" between BeanValidator and JSR303 validation because of null representation. (but I think Vaadin validation should never be ok if JSR303 is not). Perhaps a simple solution is to change setRequiredIndicator to asRequired. The BeanValidationBinder will required a default required message but the behaviour is more as expected (in my opinion). |
Thanks @jcgueriaud, this really needs to be solved somehow. Too bad the null representation is still an issue :-( Maybe the default values like empty string should be passed to the bean when doing the binding. This wouldn't fix all oddities, but at least the most major one (JSR303 validation does not match "vaadin validation"). |
My "patch" is not better (it's worse) because it does break some stuff and does not correct the oddity. Should it be fix by using asRequired (and losing the custom message) ? Any ideas ? |
@mstahv yes, most definitely yes. If the issue is the fact that |
Just verified that the issue is still in Vaadin 10 beta9 as well. Would be really nice to get this sort of issues fixed for 10. |
My suggestion how this should be solved: When doing the non-buffered binding with setBean method, the values that fields effectively use for null values, like empty string by TextField, are put back to the bean right away. cc: @denis-anisimov @Legioth Maybe you can consider this to Flow binder right now as we are not yet in LTS. It could then be backported to 8 series in 8.5/8.6. |
The current implementation is thus bad in different ways for both cases. We should pick one of the cases as the default and offer a way of choosing the other. My suggestion would be to change |
Also fixes #2261 Related to vaadin/framework#9000
To be fixed in Flow by vaadin/flow#4138 and vaadin/flow#4139. I'll leave it to @tsuoanttila and/or @elmot to determine whether these patches should be picked to 8.x. The first patch is API compatible but changes an assumption that a unit test depends on. The second changes a relatively rarely used API and also requires some test adjustments. |
Also fixes #2261 Related to vaadin/framework#9000
Thanks Leif for finally taking this on the table. Looks like this will make things work in more consistent manner. Maybe this could be worked into Framework 8.5 too. I'd still argue that @NotNull is most often not there to "shield database". I don't see too many reasons to do that. For queries and their readability it is easier to keep empty strings away from the database (map "" -> NULL in application level). I guess the original idea of "shielding database from null values" comes from ancient versions of an old "almost relational" database whose performance got worse if indexed column allowed null values? But all cases should be possible and easily achievable. It may also be that NULL means user has not filled it yet and "" that user has submitted a form with no value for the column. Thus, to make this area "complete", I'd hope there was a (Binder) global to configure "null representation" of binder. I have seen a customer project (cc @peterl1084) with a number repetitive (for all textfields) calls like:
As the case is now open, maybe a short fight for simplicity with that related issue as well? |
How would a binder-wide default null representation work in practice? I can come up with two different alternatives.
On the application level, it's of course also possible to define a simple subclass of |
Also fixes #2261 Related to vaadin/framework#9000
Hello there! We are sorry that this issue hasn't progressed lately. We are prioritizing issues by severity and the number of customers we expect are experiencing this and haven't gotten around to fix this issue yet. There are a couple of things you could help to get things rolling on this issue (this is an automated message, so expect that some of these are already in use):
Thanks again for your contributions! Even though we haven't been able to get this issue fixed, we hope you to report your findings and enhancement ideas in the future too! |
Addresses: #9000 Addresses: #11109 These changes are adopted from vaadin/flow#4138 and vaadin/flow#6757
* Cherry picks of Binder fixes in Flow Addresses: #9000 Addresses: #11109 These changes are adopted from vaadin/flow#4138 and vaadin/flow#6757
Vaadin 8.0.5
Example code to reproduce
The text was updated successfully, but these errors were encountered: