BeanValidationBinder says the form is valid even though required (@NotNull) field is not filled

[mstahv]

Vaadin 8.0.5

Example code to reproduce

public static class Entity {

    @NotNull
    private String name;

    public void setName(String name) {
        this.name = name;
    }

    public String getName() {
        return name;
    }

    @Override
    public String toString() {
        return "Entity, name: " + name;
    }

}

final TextField name = new TextField();

@Override
protected void init(VaadinRequest vaadinRequest) {
    final VerticalLayout layout = new VerticalLayout();
    name.setCaption("Type your name here:");

    Entity entity = new Entity();
    Binder<Entity> b = new BeanValidationBinder<>(Entity.class);
    b.bindInstanceFields(this);

    b.setBean(entity);

    Button button = new Button("Check validity");
    button.addClickListener(e -> {
        ValidatorFactory factory = Validation.buildDefaultValidatorFactory();
        Validator validator = factory.getValidator();
        Set<ConstraintViolation<Entity>> validate = validator.validate(entity);
        String msg = String.format("Binder.isValid: %s, JSR 303 valid: %s", b.isValid(), validate.isEmpty());
        Notification.show(msg);
    });

    layout.addComponents(name);
    layout.addComponent(button);

    setContent(layout);
}

[viydaag]

I might know why the @NotNull does not work on TextField. With new data binding API, TextField value cannot be null. It is always empty at the begining. So as a workaround, adding the @SiZe(min=1) does the trick.

[tsuoanttila]

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 null -> "" -> null or not. If you want to use NotNull, you should use binder.forField(textField).withNullRepresentation("") which performs the change back from empty string to null. However this will mean that if the user actually tries to input an empty string, they really can't.

[viydaag]

Thx for the precision

[jcgueriaud]

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).
It will work for NotNull, Size min or NotEmpty and if someone don't want this behaviour he can default required behaviour for one annotation.

[mstahv]

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").

[jcgueriaud]

My "patch" is not better (it's worse) because it does break some stuff and does not correct the oddity.
null is represented differently in the UI than in the backend. It may be converted in one way (default nullrepresentation) or two-way (custom). (if I understand it correctly).
And without JSR303, there is "asRequired" method for that (we don't use validator for that).
So is it really possible to try to solve the "JSR303 required" fields using a Validator ?

Should it be fix by using asRequired (and losing the custom message) ?

Any ideas ?

[micpalmia]

Maybe the default values like empty string should be passed to the bean when doing the binding

@mstahv yes, most definitely yes.

If the issue is the fact that TextFields are really never null, this should be reflected back to the Bean when doing the binding. I would expect that the (unbuffered) binder would keep bean and form fields constantly in sync, without masking magic null/empty string conversions. Otherwise, we get a Binder saying it's all good, while a Validator on the same Bean is rightfully full of broken constrains!

[mstahv]

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.

[mstahv]

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.

[Legioth]

@NotNull on a field bound to a TextField is indeed a slightly weird situation since its getValue() will never return null. There are thus two alternative interpretations of the situation.

1. @NotNull is on the bean not for the sake of Binder and the UI, but only to protect the database. This means that "" should pass validation and that the field should not be marked as required. For this to work, it also requires "" to be explicitly written to the bean in case the value there is null even if the field has not been modified according to Binder.
2. @NotNull is accidentally used instead of e.g. @Size(min = 1) or @NotEmpty with the intent that the field should indeed be required. In this case, there would have to be an explicit "backup" validator that would catch the "" case that would pass @NotNull.

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 RequiredFieldConfigurator so that we also pass either the field instance or the field's default value to it and change the default so that @NotNull only marks the field as required if it's default value is null. Correspondingly, the @NotEmpty and @Size(min = 1) implementations should only make the field required if the default value is a String or Collection. On top of this, we should also implement the explicit writeback of values that are modified when passed through the chain. I don't think we should implement an automatic isEmpty() fallback validator for the opposite case, but instead encourage developers to do an explicit withNullRepresentation("") which would cause "" from the field to be written back as null to the bean, which would cause validation to fail.