Binder setter method is not invoked until the entire form is completed

[alexanoid]

Description of the bug

With recent updates to the form validation process, the binder setter method is now not invoked until the entire form is completed.

For example, in the following code, I set the name value to DTO:

        bindingBuilder.bind(dto -> value, (dto, name) -> {
            name = StringUtils.isNoneBlank(name) ? name.trim() : null;
            dto.setName(name);
        });

And then, in another field, I check the name for some custom validation. For example:

binder.forField(skillComboBox).withValidator(entity -> {
                    JobableDto dto = binder.getBean();
                    if(dto.getName ... some logic

Now I'm unable to get the dto.getName() value until I completely fill out the entire form and press Save button with binder.writeBean(dto). Previously, I was able to see these values without that.

Is there a way to achieve this with the current implementation?

This is not very convenient, as it requires the user to wait until the entire form is completed before being notified of any additional validation errors

Expected behavior

The binder setter method should be invoked immediately after the field value is added and validated on the filed level.

Minimal reproducible example

n/a

Versions

• Vaadin / Flow version: 24.4.0.alpha8
• Java version: 17

[Legioth]

Expected behavior The binder setter method should be invoked immediately after the field value is added.

The intention is that values are written to the bean only when that would leave the bean in a consistent state, i.e. when all validations pass.

There are two ways around this:

• If you're concerned about recent validation changes causing more strict checking, then you could test the newly introduced setDefaultValidatorsEnabled(false) API either for some specific binding or for the entire binder.
• If you explicitly want to write out the values that pass validation while ignoring invalid values, then you can use the writeBeanAsDraft API

[knoobie]

The intention is that values are written to the bean only when that would leave the bean in a consistent state, i.e. when all validations pass.

I haven't tested 24.4-alpha; but once beta is released, I'm going to test it with out applications with some advanced binder usages, but just looking at this I would say it would break our apps because we rely on getBean() to always returns the up-to-date instance.. why should I use setBean if the values aren't written instantly? If I want to have a consistent bean I would have used read/writeBean in the first place.

At least in the past it was like this if setBean is used:

• only changed fields are checked; so having two required fields; adding a value in the first.. and checking in the second field's validator that getBean().getValue1() returns != null worked and should still work.. meaning value1 was written to the bean if it was valid.. it does not matter what other fields say; this binding is valid - therefore it should have been written to the bean
• Binder::withValidator would block the whole write-through; even tho I didn't like it.. but it was so

I would doubt that setDefaultValidatorsEnabled(false) does anything in this case here.

[alexanoid]

At least in the past it was like this if setBean is used:

* only changed fields are checked; so having two required fields; adding a value in the first.. and checking in the second field's validator that getBean().getValue1() returns != null worked and should still work.. meaning value1 was written to the bean if it was valid.. it does not matter what other fields say; this binding is valid - therefore it should have been written to the bean

* Binder::withValidator would block the whole write-through; even tho I didn't like it.. but it was so

I would doubt that setDefaultValidatorsEnabled(false) does anything in this case here.

+1

[tepi]

So in short, we must decide if we want to:

a) protect the validity of the whole bean when setBean is used, or b) write only the changed binding if it passes field-level validation, regardless of non-validity of other fields - and also document clearly that using setBean nothing is guaranteed, which was not the case before the recent changes

Issues with solution b are:

• we have no idea of validity of other bindings, so we can't fire reliable status change for the binders validation status - it can only be found out via manual validation (which also highlights all non-valid fields by default which is usually not wanted)
• binder-level validation failing will still prevent writing the changed binding to the bean

[alexanoid]

My vote is for the solution, as it was previously before the recent changes. Everything was working fine.

[tepi]

An additional question, is it not convenient to use the cross-validation as described in the docs, relying on field value instead of bean value?

[knoobie]

it can only be found out via manual validation (which also highlights all non-valid fields by default which is usually not wanted)

Interesting point, because that is exactly what I want (and do). If we submit a form used with setBean;

We use a method on top of the binder to enforce exactly this behavior; show all fields that are invalid as invalid if submit is pressed.

  public boolean isBinderValid() {
    // true is used to enforce validation change events to all components (trigger error themes etc.)
    return validate(true).isOk();
  }

[knoobie]

An additional question, is it not convenient to use the cross-validation as described in the docs, relying on field value instead of bean value?

It's not convenient and close to impossible with forms that have hundreds of fields, split over multiple classes.. the only thing in common is the bean.

[tepi]

Interesting point, because that is exactly what I want (and do). If we submit a form used with setBean;

I should've been more clear, I meant a case where for example a Save-button should enable/disable based on whole binder validity state. I think your case is for when Save is pressed?

[tepi]

It's not convenient and close to impossible with forms that have hundreds of fields, split over multiple classes.. the only thing in common is the bean.

Just as a note in that case if we go with the b option, also need to update the cross-validation documentation to reflect this.

[alexanoid]

An additional question, is it not convenient to use the cross-validation as described in the docs, relying on field value instead of bean value?

Unfortunately, no, because I use extremely dynamic forms with many different related fields that, under the hood, supply values to my internal complex structures in DTOs. Based on them, I perform my cross-field validations.

[knoobie]

I should've been more clear, I meant a case where for example a Save-button should enable/disable based on whole binder validity state. I think your case is for when Save is pressed?

Yes, our case comes from an always-on save button. A disabled save Button is against our accessibility policies.. and yes; I understand that concern you have with that

[Legioth]

Seems like I misunderstood the cause of the original report. Here's a miminimal reproducible example that works differently between Vaadin versions.

@Route
public class BinderTest extends VerticalLayout {
    public BinderTest() {
        TextField oneField = new TextField("One");
        TextField twoField = new TextField("Two");

        add(oneField, twoField);

        Binder<Object> binder = new Binder<>();
        binder.forField(oneField).asRequired().bind(ignore -> "",
                (ignore, value) -> add(new Span("Set one to " + value)));
        binder.forField(twoField).asRequired().bind(ignore -> "",
                (ignore, value) -> add(new Span("Set two to " + value)));
        binder.setBean(new Object());
    }
}

It should be noted that this change isn't new for Vaadin 24.4. Based on my testing, 24.3.4 and 24.3.5 also work in the same way as 24.4 whereas 24.3.3 and older work in a different way. This is a quite significant behavior-changing incompatibility so I don't understand how it was shipped in a maintenance release.

[knoobie]

Exactly Leif! The binder "fixes" were backported.. that's why I stayed with 24.3.0 with out applications and waited for 24.4 to resolve those.

[knoobie]

This https://github.com/vaadin/flow/pull/18535 change made me stop upgrading once I saw that PR, because my guts told me.. that's going to break things; and I don't have time to check that with all our use-cases in a bugfix release

After that change; a lot of fixes had to be developed and backported also. See the linked issues

[Legioth]

Because of the wide-reaching impact of these changes on existing code in complex applications out there, I would say that we shouldn't change (or have changed) how it works without at the least giving a way for the developer to choose whether they want to have the old or the new behavior.

I can thus see three different alternatives with different merits:

1. Make it possible to choose and have the old behavior as the default
2. Make it possible to choose and have the new behavior as the default
3. Keep the old behavior without making it possible to choose the new way

The current approach that forces the new behavior without any (apparent) way of getting the old back doesn't seem like a sensible option.

If we want to make it configurable, then I'm wondering if it would be better to introduce that as an explicit setThisAndThat(boolean) mode or if it would be better to introduce separate variants of setBean that work in different ways?

[knoobie]

I personally would go with 1. - because I can understand why you guys wanted to change the way how it currently works; see Teppo's comment here https://github.com/vaadin/flow/issues/18735#issuecomment-1952117463 - but sadly the old behavior was there for years(?) and kinda accepted and relied upon; even tho you could say it's a bug (even tho a convenient and needed bug for such complex forms).

[knoobie]

If we want to make it configurable, then I'm wondering if it would be better to introduce that as an explicit setThisAndThat(boolean) mode or if it would be better to introduce separate variants of setBean that work in different ways?

Thinking out load without much thoughs:

Having a overloaded method for setBean(Object bean, BeanValidationType validationType) with the modes (BeanValidationType.BINDING_ONLY, BeanValidationType.BEAN) would be my first suggestion, where BINDING_ONLY would be the default / old way of things, and BEAN the alternative where everything has to be valid before the bean is changed.

[tepi]

We have a few days until beta, so I'd suggest reverting change here https://github.com/vaadin/flow/blob/main/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java#L1930-L1946 to essentially this https://github.com/vaadin/flow/blob/23.2/flow-data/src/main/java/com/vaadin/flow/data/binder/Binder.java#L1810-L1814. That would restore previous functionality, the other changes can be kept.

Also, need to ensure Javadocs reflect the actual functionality reverted to. Probably also would not hurt to add to Javadocs that despite it's name, BinderValidationStatus can in these cases mean the validation status of a single binding, so it should not be relied on for Binder's validation state in cases setBean is used.

[tepi]

Additionally, need to enhance tests for flow-data module. Running module unit tests with either current code or reverted code passes. Same is true for when I replace BinderTest with a version from before these changes. So apparently we do not and did not have any test that would check this part of setBean and bean-writing functionality.

[Legioth]

The new behavior is the "safe" option which means that it would be preferable if that would be the option with lowest friction so that it would be used in newly written code unless there's some specific reason to do something else. This implies that it should also be the default.

The main reason to consider keeping the old behavior as the default would be for backwards compatibility. We have considered providing OpenRewrite recipes for automatically updating existing code in cases where we make these types of simple but potentially laborious changes. I wonder if this change would be a good reason to try out that approach?

[knoobie]

I'm not an expert with OpenRewrite, but I have a feeling this could be a really hard change to check and apply with OpenRewrite.. The Hilla Migration on the other hand sounds simple with package names and pom changes.. here.. we would have to check Binder usage with setBean.. and all the possibilities with custom Binders are literally endless. (I have my own "CompanyBinder extends Binder" in use in all projects

[Legioth]

Sounds really easy to change all existing invocations of b.setBean(x) to b.setBean(x, BeanValidationType.BINDING_ONLY). I think OpenRewrite has a way of applying that to all cases where the compile-time type of b is Binder or a subclass.

[Legioth]

But the good thing is that we can start by adding new API to make it configurable and preserving the default and then we have the option of changing the default in a later release.

[alexanoid]

@Legioth sorry for the off-topic, but I couldn't help but ask - have you optimized something in the latest versions? It seems like my website started working faster.

[Legioth]

Nothing significant for Vaadin 24.4 but there have been various tweaks throughout the 24.x releases. Difficult to guess if there's any specific improvement that would be particularly impactful without knowing details about your case.

[knoobie]

@tepi should this stay open; or another issue be created to add the API improvements @Legioth suggested?

[tepi]

I'll create an enhancement ticket and reference this one