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.

[Legioth]

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.

[mstahv]

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:

    b.forMemberField(name).withNullRepresentation("");

As the case is now open, maybe a short fight for simplicity with that related issue as well?

[Legioth]

How would a binder-wide default null representation work in practice?

I can come up with two different alternatives.

1. Allow changing the default automatic null representation to be two-way instead of only for model -> presentation conversions, e.g. binder.setTwoWayNullRepresentation(true).
2. Make it possible to set a default null representation value other than the field's getEmptyValue(). This would have to be done per field type (e.g. AbstractTextField or field type type (e.g. String) (which is not always knowable because of type erasure). This could be something like binder.setDefaultNullRepresentation(AbstractTextField.class, "");. One might then wonder whether this mechanism for configuring defaults for a specific field type should be limited to only null representations, or if it should be a slightly more generic hook that is run before returning from a matching forField(fieldInstance), e.g. binder.setDefaultBindingConfigurator(AbstractTextField.class, bindingBuilder -> bindingBuilder.setNullRepresentation("")).

On the application level, it's of course also possible to define a simple subclass of Binder where the createBinding method (which is called internally from forMemberField) is overridden to do the customization so that the repetition could be moved out from individual field bindings.

[stale]

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):

• Check if the issue is still valid for the latest version. There are dozens of duplicates in our issue tracker, so it is possible that the issue is already tackled. If it appears to be fixed, close the issue, otherwise report to the issue that it is still valid.
• Provide more details how to reproduce the issue.
• Explain why it is important to get this issue fixed and politely draw others attention to it e.g. via the forum or social media.
• Add a reduced test case about the issue, so it is easier for somebody to start working on a solution.
• Try fixing the issue yourself and create a pull request that contains the test case and/or a fix for it. Handling the pull requests is the top priority for the core team.
• If the issue is clearly a bug, use the Warranty in your Vaadin subscription to raise its priority.

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!