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

Binder bean validation is not triggered properly in Vaadin 24 #18163

Closed
jcgueriaud1 opened this issue Aug 9, 2023 · 5 comments · Fixed by #18535
Closed

Binder bean validation is not triggered properly in Vaadin 24 #18163

jcgueriaud1 opened this issue Aug 9, 2023 · 5 comments · Fixed by #18535

Comments

@jcgueriaud1
Copy link
Contributor

Description

When I set a validator on the binder, the setValidationStatusHandler does not show the error.
image

In Vaadin 23, the error was displayed.
image

Expected outcome

The error should be displayed and the binder status should be in error.

Minimal reproducible example

public class HelloWorldView extends HorizontalLayout {

    private Binder<FakeBean> binder = new Binder<>();

    private TextField name = new TextField("Name");
    private TextField name2 = new TextField("Name2");

    private Span errorMessage = new Span();

    public HelloWorldView() {
        FakeBean fakeBean = new FakeBean();
        binder.forField(name).asRequired("name is required")
                .bind(FakeBean::getName, FakeBean::setName);
        binder.forField(name2).asRequired("name2 is required")
                .bind(FakeBean::getName2, FakeBean::setName2);
        binder.setBean(fakeBean);
        binder.withValidator(bean ->
                        Objects.equals(bean.getName(), "test") || Objects.equals(bean.getName2(), "test"),
                "At least one field should be test"
        );
        binder.setValidationStatusHandler(status -> {
            System.out.println("Errors " + status.getValidationErrors().size());
            if (!status.getBeanValidationErrors().isEmpty()) {
                errorMessage.setText(status.getBeanValidationErrors().get(0).getErrorMessage());
                errorMessage.setVisible(true);
            } else {
                errorMessage.setVisible(false);
            }
        });
        add(name, name2, errorMessage);
    }

}

See these 2 branches:
https://github.com/jcgueriaud1/spring-proxy-issue/tree/bean-validation-v24
https://github.com/jcgueriaud1/spring-proxy-issue/tree/bean-validation-v23

Steps to reproduce

Open the view type xx as no field value = test the error should be displayed.

Environment

Vaadin version(s): 24.1.4

Browsers

No response

@vursen
Copy link
Contributor

vursen commented Aug 21, 2023

The issue goes away when I remove the validated event subscription from addValidationStatusChangeListener. It appears Binder doesn't run top-level validators when revalidating on ValidationStatusChangeEvent that we dispatch in addValidationStatusChangeListener:

https://github.com/vaadin/flow-components/blob/a33f718985c95d28f95899ee1cbf93b40d8389d0/vaadin-text-field-flow-parent/vaadin-text-field-flow/src/main/java/com/vaadin/flow/component/textfield/TextArea.java#L325-L330

Although, it does run them when the revalidation is caused by ValueChangeEvent.

This gives me the impression that it might be a bug in Binder. @mshabarov Could you check that please?

@mshabarov
Copy link
Contributor

@vursen the behaviour described above doesn't seem correct to me.
status.getValidationErrors() returns non-empty collection of errors in V23 and V24.3 and the error message is shown properly.
However, with V24.1 and V24.2, the validationStatusHandler is called 3 times, giving 1 error for the first call, and 0 errors for subsequent last two calls.
Only this single fact makes me think that it's a buggy behavior.

I would expect that if I set the binder-level validator with withValidator, then status.getValidationErrors() should return non-zero errors, if this validator predicate returns false.

So far I think this is a bug in Flow Binder.

By the way, I didn't get how addValidationStatusChangeListener impacts the status handler? They are kind of separate hooks.

@mshabarov
Copy link
Contributor

Similar behaviour is described here by the way.

@mshabarov mshabarov transferred this issue from vaadin/flow-components Nov 30, 2023
@mshabarov mshabarov moved this to 🔖 Normal Priority (P2) in Vaadin Flow bugs & maintenance (Vaadin 10+) Nov 30, 2023
@mshabarov mshabarov moved this from 🟢Ready to Go to 🪵Product backlog in Vaadin Flow ongoing work (Vaadin 10+) Nov 30, 2023
This was referenced Jan 24, 2024
mcollovati pushed a commit that referenced this issue Jan 26, 2024
Changes:

- Once Binder.handleFieldValueChange runs for a binding when readBean was used, the whole binder will be silently validated also. BinderValidationStatusHandler is called like before (only contains status from changed binding), but StatusChangeEvent is now fired considering all bindings and if possible bean validators as well.
- Once Binder.handleFieldValueChange runs for a binding when setBean was used, doWriteIfValid will validate all bindings, not only the changed ones. This prevents writing an invalid bean in cases where one or more of the initial values are in invalid state (but not marked as such since setBean resets validation status), but they have not been changed from their initial value(s).
 Calling setAsRequiredEnabled with a changed value no longer triggers validation, since that validation is now handled elsewhere when needed as stated above.

Minor Javadoc fixes, trying to align Javadocs with code.

Fixes #18163
Fixes #4988
Fixes #17515
@github-project-automation github-project-automation bot moved this from 🔖 Normal Priority (P2) to ✅ Closed in Vaadin Flow bugs & maintenance (Vaadin 10+) Jan 26, 2024
@github-project-automation github-project-automation bot moved this from 🪵Product backlog to Done in Vaadin Flow ongoing work (Vaadin 10+) Jan 26, 2024
vaadin-bot pushed a commit that referenced this issue Jan 26, 2024
Changes:

- Once Binder.handleFieldValueChange runs for a binding when readBean was used, the whole binder will be silently validated also. BinderValidationStatusHandler is called like before (only contains status from changed binding), but StatusChangeEvent is now fired considering all bindings and if possible bean validators as well.
- Once Binder.handleFieldValueChange runs for a binding when setBean was used, doWriteIfValid will validate all bindings, not only the changed ones. This prevents writing an invalid bean in cases where one or more of the initial values are in invalid state (but not marked as such since setBean resets validation status), but they have not been changed from their initial value(s).
 Calling setAsRequiredEnabled with a changed value no longer triggers validation, since that validation is now handled elsewhere when needed as stated above.

Minor Javadoc fixes, trying to align Javadocs with code.

Fixes #18163
Fixes #4988
Fixes #17515
vaadin-bot pushed a commit that referenced this issue Jan 26, 2024
Changes:

- Once Binder.handleFieldValueChange runs for a binding when readBean was used, the whole binder will be silently validated also. BinderValidationStatusHandler is called like before (only contains status from changed binding), but StatusChangeEvent is now fired considering all bindings and if possible bean validators as well.
- Once Binder.handleFieldValueChange runs for a binding when setBean was used, doWriteIfValid will validate all bindings, not only the changed ones. This prevents writing an invalid bean in cases where one or more of the initial values are in invalid state (but not marked as such since setBean resets validation status), but they have not been changed from their initial value(s).
 Calling setAsRequiredEnabled with a changed value no longer triggers validation, since that validation is now handled elsewhere when needed as stated above.

Minor Javadoc fixes, trying to align Javadocs with code.

Fixes #18163
Fixes #4988
Fixes #17515
vaadin-bot added a commit that referenced this issue Jan 26, 2024
Changes:

- Once Binder.handleFieldValueChange runs for a binding when readBean was used, the whole binder will be silently validated also. BinderValidationStatusHandler is called like before (only contains status from changed binding), but StatusChangeEvent is now fired considering all bindings and if possible bean validators as well.
- Once Binder.handleFieldValueChange runs for a binding when setBean was used, doWriteIfValid will validate all bindings, not only the changed ones. This prevents writing an invalid bean in cases where one or more of the initial values are in invalid state (but not marked as such since setBean resets validation status), but they have not been changed from their initial value(s).
 Calling setAsRequiredEnabled with a changed value no longer triggers validation, since that validation is now handled elsewhere when needed as stated above.

Minor Javadoc fixes, trying to align Javadocs with code.

Fixes #18163
Fixes #4988
Fixes #17515

Co-authored-by: Teppo Kurki <[email protected]>
vaadin-bot added a commit that referenced this issue Jan 26, 2024
Changes:

- Once Binder.handleFieldValueChange runs for a binding when readBean was used, the whole binder will be silently validated also. BinderValidationStatusHandler is called like before (only contains status from changed binding), but StatusChangeEvent is now fired considering all bindings and if possible bean validators as well.
- Once Binder.handleFieldValueChange runs for a binding when setBean was used, doWriteIfValid will validate all bindings, not only the changed ones. This prevents writing an invalid bean in cases where one or more of the initial values are in invalid state (but not marked as such since setBean resets validation status), but they have not been changed from their initial value(s).
 Calling setAsRequiredEnabled with a changed value no longer triggers validation, since that validation is now handled elsewhere when needed as stated above.

Minor Javadoc fixes, trying to align Javadocs with code.

Fixes #18163
Fixes #4988
Fixes #17515

Co-authored-by: Teppo Kurki <[email protected]>
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 24.3.4.

@rolfwoll
Copy link

rolfwoll commented Aug 2, 2024

I have a similar problem to this related to DateTimePickers in Vaadin 24.4.7

If I change Jean-Christophe Gueriaud's code snipplet above to use LocalDateTimes and DateTimePickers, then I get the same problem again.

`public class HelloWorldView extends HorizontalLayout {

private Binder<FakeBean> binder = new Binder<>();

private DateTimePicker date1 = new DateTimePicker("Date1");
private DateTimePicker date2 = new DateTimePicker("Date2");

private IntegerField field = new IntegerField("test");

private Span errorMessage = new Span();

public HelloWorldView() {
    FakeBean fakeBean = new FakeBean();
    binder.forField(date1).asRequired("date1 is required")
            .bind(FakeBean::getDate1, FakeBean::setDate1);
    binder.forField(date2).asRequired("date2 is required")
            .bind(FakeBean::getDate2, FakeBean::setDate2);
    binder.setBean(fakeBean);
    binder.withValidator(bean ->
                    bean.getDate1() == null || bean.getDate2()== null || bean.getDate1().isBefore(bean.getDate2()),
            "date1 should be before date 2"
    );
    binder.setValidationStatusHandler(status -> {
        System.out.println("Errors " + status.getValidationErrors().size());
        if (!status.getBeanValidationErrors().isEmpty()) {
            errorMessage.setText(status.getBeanValidationErrors().get(0).getErrorMessage());
            errorMessage.setVisible(true);
        } else {
            errorMessage.setVisible(false);
        }
    });
    field.setMin(0);
    add(date1, date2, errorMessage, field, new Button("Clear", e-> field.clear()));
}

}
`
LocalDateTime date1 and date2 properties are added to FakeBean of course.

In this case, validationStatusHandler is called 2 times, first time with the error from the bean level validator and the second time without any errors which clears the previous error message, so it matches the behaviour of this ticket even though it is marked as closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

6 participants