Add new method ValueContext.getBinder() with access to the current binder

[archiecobbs]

Describe your motivation

There are many examples of field-level validation logic where the determination of "valid" depends on other fields as well as the field being validated.

A simple example is a "Confirm password" field, where you have a "Password" field and a "Confirm password" field. The "Confirm password" field has the following field-level validation constraint: This field's value must be the same as the "Password" field's value.

While this validation constraint does involve multiple fields, it is intuitively "field-level" because when the constraint is violated, there is only one field that is "wrong" (namely, the "Confirm password" field) and moreover you want the validation error to appear next to that field.

Other examples:

• You have numeric "min" and "max" fields, where "max" is always required to be at least "min"
• You have two fields describing a date range, where the end date can't be prior to the start date
• You have a TextField who's interpretation depends on some field, e.g., checkbox determines whether the TextField is interpreted as a substring or a regular expression depending on whether the "Interpret as regular expression" checkbox is checked.
• You have a TextField who's validation constraints depends on some other field, e.g., a "Postal Code" field, where the validation constraints depend on the current choice of the "Country" field.

Describe the solution you'd like

Add a new method ValueContext.getBinder() which returns the Binder associated with the validation operation being performed.

Then the "Confirm password" problem is easily solved like this:

public class ConfirmPasswordValidator implements Validator<String> {

    @Override
    public ValidationResult apply(String confirmation, ValueContext context) {
        PasswordField passwordField = context.getBinder().getBinding("password").get().getField();
        if (!Objects.equals(confirmation, passwordField.getValue()))
            return ValidationResult.error("Passwords must match"));
        return ValidationResult.ok();
    }
}

Describe alternatives you've considered

Various ugly hacks.

Additional context

This ticket is split off from this comment https://github.com/vaadin/flow/issues/14191#issuecomment-2023902038 and that bug also contains some related motivation for this enhancement.

[mstahv]

I like the enhancment idea, I could maybe check soon if it is easy to throw together a PR for it.

But I hate to say I don't like validation code I end up writing with our core Binder (and what is visible in your potential code snippet). I think it would be way cleaner if the validation code would execute against domain objects instead of UI fields. And potential re-use of that would become a lot easier. Hoping we can fix this in a planned project later this year.

Did you @archiecobbs look into the FormBinder in Viritin? I think I hinted you about it a while ago. Trying that could help use gather some input for potential improvements later this year. Drafted an example with it based on this input:

https://github.com/viritin/flow-viritin/blob/1eef5d285361666ad56a9c841aa651da47d77843/src/test/java/org/vaadin/firitin/formbinder/FormBinderRegistrationFormView.java#L18-L48

[mstahv]

Quickly checked. The ValueContext seems to be public standalone class, several constructors, called from various places, including the FormBinder in Viritin (to support the "Converter" interface from Vaadin core). Making the change in backwards compatible manner is not as trivial as I hoped when seeing this issue. Or then it should be optional, which would again complicate the validation login.

[archiecobbs]

Are you the Vaadin salesman or the Viritin salesman? :)

Frankly, I'm confused by all of the Viritin references. I agree it's good and healthy for "ecosystem" projects to exist around Vaadin and Viritin looks very helpful. But speaking only for myself, I'm not familiar with Viritin and frankly don't care to learn about (and have to start tracking) yet another project. It's hard enough to keep up with Vaadin itself.

Moreover, it doesn't seem appropriate to answer a question about Vaadin with "We don't want to do that because it might break Viritin". That's implies we don't really have an ecosystem but rather a dysfunctional co-dependency. Either Vaadin should officially incorporate Viritin, or else Viritin should have the same "Vaadin influence" priority level as any other project (including mine FWIW).

OK end of Viritin rant. As far as I can tell the only place anywhere in Vaadin where ValueContext is instantiated is in the Binder class. And we could add a Binder property to ValueContext without removing the default constructor, thereby making the Binder property optional for outside code like Viritin. So, problem solved?

[mstahv]

Every Viritin user is a pro-Vaadin user 🧸 To purpose of Viritin is to collect actaul real world experiences for concepts that may our may not become official pieces in Vaadin. Many have. I feel very good if it at the same time makes our active community member succeed better with their projects.

We have (too many times) don't our implemenations within our R&D chamber and too late then figured out the design mistakes. It is much more agile to re-iterate this kind of largish changes in add-ons than in the core, for which we have quite strict backwards compatibility requirements. That's why I want active community members to give these PoCs a try.

For this change, Viritin wouldn't be a problem, that can be changed any time and even stop using this class, but the problem is we don't know who else has customuised something regarding ValueContext. I'd be dropping in the Binder to all three constructors, which would be good for the API and implementation. The API could then guarantee it is always there, without optional dance. But backwareds compatibility vice, would be rather risky change. I might be ready to do it in a minor version, but I know there are a lot more conservative fellows in the team (which is probably good 😁 ).

[mstahv]

If only that ValueContext didn't have public constructors, this would be easy one... But maybe there is a reason for those 🤷‍♂️

[archiecobbs]

the problem is we don't know who else has customuised something regarding ValueContext. I'd be dropping in the Binder to all three constructors, which would be good for the API and implementation. The API could then guarantee it is always there, without optional dance. But backwareds compatibility vice, would be rather risky change.

I still don't see any problem here. This situation is common, and there is a standard way to handle it:

1. Add the new constructors taking a Binder parameter
2. Leave the existing constructors alone but add @deprecated warning
3. Add ValueContext.binder as an optional property and have Binder set that property via the new setter method or constructor.
4. When Vaadin N+1 is released (or N+2, or whenever), remove the deprecated constructors

[knoobie]

If only that ValueContext didn't have public constructors, this would be easy one... But maybe there is a reason for those 🤷‍♂️

Yes, my unit test need it 🤓 so don't take that one away from me 😉

Main topic: yes, having access to the binder would be interesting.. problematic would probably be that it is <?> and people have to cast to get their proper typed bean if they don't access the fields but the bean directly.

[mshabarov]

This is a good point IMO to extend the ValueContext with the Binder reference. However, the cross-validation use case is more relevant to another issue, which is already linked above (#14191).

Having a Binder object might be helpful when you want to get some information from Binder except bindings, something that relates to a given component/Bean field. Would be great if we figure out any use case for it.

[mstahv]

Pushed my unfinished work for this to https://github.com/vaadin/flow/compare/feature/binder-via-binding-context in case somebody wants to continue. Tests and proper typing missing at least (not sure if it can be accomplished properly). I have a hunch the Binder concept might see such a big changes soon that don't have a big motivation to continue on this myself.