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.

Sign up for GitHub

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

Jump to bottom

Binder.validate() fails with "bean level validators have been configured but no bean is currently set" when in buffered mode #18120

Open
mvysny opened this issue Nov 24, 2023 · 6 comments · May be fixed by #18121
Open

Binder.validate() fails with "bean level validators have been configured but no bean is currently set" when in buffered mode #18120

mvysny opened this issue Nov 24, 2023 · 6 comments · May be fixed by #18121
Labels
binder bug Impact: Low Severity: Minor

Comments

@mvysny
Copy link
Member

mvysny commented Nov 24, 2023 •
edited
Loading

Description of the bug

When using binder in buffered mode with bean-level validators and calling validate(), the function fails with:

Caused by: java.lang.IllegalStateException: Cannot validate binder: bean level validators have been configured but no bean is currently set
	at com.vaadin.flow.data.binder.Binder.validate(Binder.java:2570)
	at com.vaadin.flow.data.binder.Binder.validate(Binder.java:2554)
	at com.vaadin.starter.skeleton.MainView.<init>(MainView.java:26)

That contradicts the javadoc of Binder.validate(), which states that

Bean level validators are ignored if there is no bound bean or if any field level validator fails.

The use-case for having validate() not failing was described at #15701 ; however that ticket is marked as enhancement while I believe this to be a buggy behavior.

Expected behavior

Binder.validate() should not fail when in buffered mode and bean-level validators are set. Instead, the bean-level validators should be skipped silently.

Minimal reproducible example

@Route("")
public class MainView extends VerticalLayout {

    public static class Foo {}

    public MainView() {
        final Binder<Foo> binder = new Binder<>(Foo.class);
        binder.withValidator(Validator.alwaysPass());
        binder.readBean(new Foo());
        binder.validate();
    }
}

Versions

  • Vaadin / Flow version: 24.2.4
  • Java version: 17
@mvysny mvysny changed the title Binder.validate() fails with bean level validators have been configured but no bean is currently set Binder.validate() fails with "bean level validators have been configured but no bean is currently set" when in buffered mode Nov 24, 2023
mvysny added a commit that referenced this issue Nov 24, 2023
@mvysny
fix: Binder.validate() no longer fails in buffered mode when bean-lev…
6f09365 
…el validations are defined. Fixes #18120
@mvysny mvysny linked a pull request Nov 24, 2023 that will close this issue
Draft
9 tasks
@mshabarov mshabarov moved this from 🆕 Needs triage to 🔖 Normal Priority (P2) in Vaadin Flow bugs & maintenance (Vaadin 10+) Nov 28, 2023
@mshabarov
Copy link
Contributor

I agree that the javadoc/contract of the method is correct, but the implementation throws unexpectedly and this isn't correct.

This inconsistency causes apparently problems like described in this issue #13393.

@tepi
Copy link
Contributor

tepi commented Dec 11, 2023

Investigated this a bit further. One thing to consider is that regardless of the discrepancy between javadoc and real functionality, this logic has been in place about six years and care should be taken if it will be changed - especially if it is done via silently ignoring bean-level validators. On the other hand, writing the bean will eventually run the bean-level validators so changing the functionality to match javadoc is probably not too dangerous.

That said, one option might be to:

  1. Update current javadoc of validate to match current functionality
  2. Provide an overloaded validate -method with a parameter where the developer has to explicitly request ignoring bean-level validators

Benefit of this would also be earlier release (or backporting) since current functionality would not be changed.

@mvysny
Copy link
Member Author

mvysny commented Dec 11, 2023 •
edited
Loading

Unfortunately there already is an overloaded validate(boolean fireEvent) function; creating another overloaded validate() would further complicate the API. Also, isValid() would need to be overloaded as well.

Also, having the overloaded function would basically bring back the old behavior of throwing an exception if bean-level validators are defined; I wonder what value this use-case brings, except for the compatibility reason of course.

@tepi
Copy link
Contributor

tepi commented Dec 11, 2023

You are right on both counts. But I'm afraid that if the validate functionality is changed to silently ignore validators, releasing this might be pushed quite far into the future. Not sure if we can do such change in a minor release, but definitely not in a bugfix release. @mshabarov any thoughts?

@mvysny
Copy link
Member Author

mvysny commented Dec 11, 2023

True, it's always good to be conscious on the backwards compatibility. I'd say that this ticket has been present in Vaadin for 6 years, so waiting a bit wouldn't hurt; and personally I would prefer to have simpler&cleaner API. So, personally I would vote to go for the "breaking change" option, and release this as part of next major version.

@mshabarov
Copy link
Contributor

As I said, the contract of validate method sounds correct to me, so I agree with you guys that it shouldn't throw, when the bean is just red to the fields and not bound.

The javadoc says for validate: Validates the values of all bound fields, which should happen when the fields aren't bound.

I cannot come up with a good example, but there might be existing use cases in projects when form validation relies on IllegalStateException thrown in validate.

Older versions need an update of javadoc to reflect the implementation (tell that this methods might throw). It means a behaviour change.

Finally, I propose to follow what Martin said and apply this patch to the next major version (I have a list of TODOs and going to add this there).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
binder bug Impact: Low Severity: Minor
Projects
Vaadin Flow bugs & maintenance (Vaadin 10+)
Status: 🔖 Normal Priority (P2)
Milestone
No milestone
3 participants
@mvysny @tepi @mshabarov