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

BeanValidationBinder and @NotNull, @NotEmpty validations #10422

Closed
wants to merge 2 commits into from

Conversation

jcgueriaud
Copy link
Contributor

@jcgueriaud jcgueriaud commented Dec 7, 2017

I tried to fix this issue #9000
I'm pretty sure that my pull is not enough and have to be changed because it causes breaking change.

Instead of trying to check the field value, if the field value equals the defaultValue then I check null.
Example:
An attribute @SiZe(min = 1) bound

  • Vaadin (before): invalid (it checks "")
  • JSR303 Validation: valid (attribute is null)
  • Vaadin (with this modification): Valid (it checks null)
    So it breaks some old behaviours.

Here some thoughts about that change (why I made this one):

  • The main problem is that Vaadin validates an invalid bean (BeanValidator <> JSR303 Validation).
  • If I understand it correctly, nullRepresentation is a converter. It convert null to "nullRepresentation" and "nullRepresentation" to null.
    --> Instead of check if fieldValue (here "") is useless because the fieldValue will be null (not "").
    (For example, asRequired don't check if the field value is null but if the value equals emptyValue.)

Perhaps the discussion should be done in #9000


This change is Reviewable

@tsuoanttila
Copy link
Contributor

Thank you for the pull request and pointing out the issue. I do apologise for being so late with the review. Can you consider my feedback and respond soon so we can get this to an upcoming release for testing?


Reviewed 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


server/src/main/java/com/vaadin/data/BeanValidationBinder.java, line 102 at r2 (raw file):

        if (requiredConfigurator != null) {
            configureRequired(binding, definition, validator);
            validator.setEmptyValue(binding.getField().getEmptyValue());

We might be able to skip this.


server/src/main/java/com/vaadin/data/validator/BeanValidator.java, line 121 at r2 (raw file):

    @Override
    public ValidationResult apply(final Object value, ValueContext context) {
        Object valueToValidate = Objects.equals(emptyValue,value)?null:value;

This could use context.getHasValue().isEmpty() instead of depending on the emptyValue provided through the new API.


server/src/main/java/com/vaadin/data/validator/BeanValidator.java, line 139 at r2 (raw file):

     * @param emptyValue the new empty value
     */
    public void setEmptyValue(Object emptyValue) {

Is this really needed? see suggestion in the applymethod.


Comments from Reviewable

@pleku
Copy link
Contributor

pleku commented Sep 3, 2018

@jcgueriaud Thanks for the contribution! This PR has been waiting for a while, would you have time to respond to the questions brought up in the review ? We don't have time at the moment to fix the issues ourselves at the moment, so we'll close this PR unless someone takes it forward.

@TatuLund
Copy link
Contributor

TatuLund commented Nov 6, 2019

I am closing this PR in favor of #11758

The new fix is made same way the issue has been solved in Flow.

@TatuLund TatuLund closed this Nov 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants