RFC: Moving towards validators in fields

Flow
41

Could you emulate what setBean does while still using readBean by adding a binder-level value change listener that does writeChangedBindingsToBean every time anything changes?

42

Interesting, but I already have an (ugly) workaround, so I’m not looking for a solution; I’m just arguing for a simpler Binder that is easier to work with, instead of fighting against it.

My workaround looks something like this:

public class MyBinder<BEAN> extends Binder<BEAN> {

    private static final Method initFieldValueMethod;

    static {

        initFieldValueMethod = Binder.BindingImpl.class.getDeclaredMethod("initFieldValue", Object.class, boolean.class);
        initFieldValueMethod.setAccessible(true);

    }
    
    @Override
    public void setBean(BEAN bean) {
        ...
        
        // This is the only thing I wanted to override: We call this with *false* to prevent writeback
        //b.initFieldValue(bean, true)
        try {
            initFieldValueMethod.invoke(b, bean, false);
        }
        catch (IllegalAccessException | IllegalArgumentException | InvocationTargetException e) {
            throw new RuntimeException(e);
        }
        ...
    } 
}

Imho, writeback is a bad idea, but its there so I guess you can’t change it.
So, maybe make it configurable instead?: binder.setWriteBackChangedValuesWhenSettingBean(false)
Then I wouldn’t need this workaround at all

43

If there was a setWriteBackChangedValuesWhenSettingBean method, that would essentially create a third method of binding; there’d be buffered, unbuffered, and this new somewhat hidden type (half-buffered?). I’m not sure this increased complexity would be good from a maintainability point of view.

44

Let me just restate what I want to achieve then: Displaying data should never change data.
To me that is so basic that I’m surprised we’re even discussing it.

Ideally it should be the default behavior, but if not it should be possible to change the behavior in a sensible way. One way would be to make it configurable. Another to make the class easier to override or replace.

I have my my overridden Binder, so I’ve solved the immediate problem but I think the way I had to do the override is unfortunate. (but it is a neat technique, which I might use more now that I know about it)

I’d be happy to write my own Binder replacement, but since the grid editor requires a Binder instance, I can’t. (I think that is the only component that takes in a Binder reference?)

So, that is my input to this thread:

  • Either make Binder fatter to support all possible cases (no writeback!)
  • or make it easier to override
  • or make it replaceable
45

I agree that Binder should be easier to replace or extend. Easier said than done, of course.

46

You can set the binder instance in Grid using

grid.getEditor().setBinder(binder);

47

Well, that has been the trend. As I stated above, the amount of API in Binder has roughly tripled since it was introduced.

48

Yes, I, like @ollit.1 too, would very much appreciate that too. This is a bit architectural limitation due selected builder pattern approach. Fluent API was in fashion when Binder was created. Some builder pattern approaches could be easier to extend, but usually Fluent API causes problems like this, which is a bit inherent Java problem.

49

It still has to be a com.vaadin.flow.data.binder.Binder

50

Personally, I’d like to see a completely different approach. I don’t think the fields themselves should have validators, nor do I like Binder. I would actually like to see a return to the old Property based data bindings, but modernized of course (they would probably be called “full stack signals” instead of properties).

I would like to be able to build an observable data model that supports validation, conversion, side effects, etc. I would then be able to bind my user interface components to this data model in any way I like. I could do one-way bindings in either direction, I could do two-way bindings, I could choose where to show validation errors, I could bind many fields to the same property, bind the visible and enabled flags to properties, etc. I could also attach other actions to my observable model that e.g. communicate with the database, and whenever the model is updated, the UI is updated accordingly.

To make everyday work easier, there would also be a higher level API that simulates what Binder does today, but under the hood, it would be setting up these observable properties and all their bindings. When I need to, I can go down to the lower levels, otherwise I can stay at the higher level.

I have a prototype here in case somebody is interested: crud20playground/Flow/components/src/main/java/org/vaadin/playground/crud20/data/property at main · peholmst-sandbox/crud20playground · GitHub

The unit tests here explain how it works: crud20playground/Flow/components/src/test/java/org/vaadin/playground/crud20/data/property at main · peholmst-sandbox/crud20playground · GitHub

51

Interesting. On the one hand that sounds great, on the other it sounds terrifying


The move away from Properties in Vaadin8 was one reason why we never did that upgrade fully and ended up with the Vaadin7 compatibility kit. (Another was Touchkit, I believe)

With the old approach I ended up coupling our data model to Vaadin.
For instance, our data model already had the notion of value changed listeners and convertes. I migrated those to use Vaadin’s listeners and converters, and then Vaadin changed.

That was my mistake I guess, but at the time I didn’t see the point in having two kinds of converters. I hesitate to put my eggs in this basket again.

One thing great with the current Binder In that regard, is that it sits on the outside; It is just an utility. If it wasn’t for the grid editor, I believe you could manage completely without one.

I assume one reason why Vaadin moved away from Property was the memory overhead?
You wanted to have Grid display millions of rows without having to create millions of Property instances?
I was all for that in theory, but in practice I still had the same requirements as before, so I’ve ended up creating my own Property class doing what yours used to do.

52

The data model would not have to be coupled to Vaadin, but that depends on what you mean by the word “data model”. I should have used a different term in my post, because I was thinking about the “view model” from the MVVM-pattern. There, the VM (the view model) would be Vaadin aware and observable, and responsible for communicating with the M (the [data] model) which is not necessarily Vaadin aware.

Using this analogy, the VM would replace the old Binder. If you don’t want to build a separate VM (because it is overkill for simpler UIs), there would be a new, higher level “Binder” that does this for you. You would still be able to bind a field to a bean, and the binder would then set up the necessary properties and bindings. However, you would not be bound to the way Binder works. You could break free, go down a level, and do everything manually when and if you wanted to.

As for memory overhead, this is something that would have to be properly investigated. And no, I don’t think Grid should contain millions of observable properties. I haven’t even thought of how to integrate these observable properties with Grid, as I never use editable grids myself.

53

What about validator-adjacent functionality? One simple example is IntegerField when configured with field.setMin(0). Not only does this set the field to validate itself but it also configures the step buttons so that - is disabled in case the value is at or beyond the limit.

You basically have to choose one of these options:

  1. The field has built-in validation that is enabled through setMin(0) and the form binding logic (regardless of whether that’s Binder or full-stack signals) is automatically aware of that validation status.
  2. setMin(0) only configures how the - button behaves while the application must have separate logic to validate the actual value. Furthermore, the application developer must manually ensure the limit used by that logic is the same as the limit passed to setMin. This is how it used to be in previous versions but we came to the realization that it can lead to security issues if the developer doesn’t realize that setMin on its own cannot be trusted.
  3. Remove setMin from IntegerField. This is how it was in even earlier versions that didn’t have step buttons.
  4. Some other alternative that I’m not aware of.

Which one would you prefer?

I prefer option 1 not because I think it’s good but because I think all other options are even worse. This in turn leads to the question that if some of the validation has to be in the field itself, then would it be better if all field value validation would be in the same place instead of spreading it out over multiple places? Validation of “business values” is then a different thing than validation of “field values” but that does anyways have to be in a different place since there might be converters between the field value and the business value.

54

I don’t remember that being a big factor. The main reason was really that the whole Property - Item - Container API was designed before Java had lambdas or even generic types which made it quite difficult to use. We could hypothetically have retrofitted those features into the existing API but that would probably have been a backwards compatibility nightmare.

Instead, we went with the simplest possible solution that we could imagine. We have since then learned to appreciate a fully reactive approach for form binding whereas I would say the simple POJO → column callback structure remains optimal for grids and other cases that may either have large amounts of data or data that is mostly immutable.

55

What about validator-adjacent functionality? One simple example is IntegerField when configured with field.setMin(0) . Not only does this set the field to validate itself but it also configures the step buttons so that - is disabled in case the value is at or beyond the limit.


Which one would you prefer?

I would prefer the field to expose both its validation state, its valid value, and, if applicable, its raw, invalid value as observable properties. Then I could choose to do whatever I want with them. I can bind the validation state to the corresponding view model’s validation state, or merge the field’s validation state with the corresponding view models’ validation state, or something else.

56

I interpret that as keeping a fully functional setMin on IntegerField so that it validates the field (i.e. so that the field automatically is visually shown as invalid based on the value) unless overridden by application logic.

I believe that all the state management that you describe can still be possible in combination with a functional setMin and I don’t see why additionally also introducing an API for adding generic field value validators would change that. The question is just whether there’s any benefit from putting different types of field value validation in different places or if it would be better to have it all in one place.

57

1.1: Add HasMinValue interface to the field and let the Binder configure the field’s minValue and disable the fields internal validation so that the binder handles everything. This would allow to have all validation in one place. Additionally this could be combined with Bean Validation logic where Min/Max would automatically call those interfaces respectively to configure the field. ;)

58

Not all the validation, but only the validation that the Binder API designers have considered as important enough. Which means that you would still be back at configuring validation on the field for e.g. a color picker field that limits its UI to only allow picking colors with a high enough contrast ratio compared to a chosen reference color.

And that raises the question of whether it would be acceptable for that field component to have the same security gotchas with trivially bypassed “validation” that made us start making things complicated for e.g. IntegerField.

And that means that we would still need the getDefaultValidator() mechanism. And then we have gone another full circle around the whole design space allowed by the overall Binder structure.

59

All Vaadin first-class citizen / fields’ validation should be in the binder - once that baseline is created and an additional validation is added, it has to be added to the binder Imho.

Coming back to your example with the color picker: I would not think that this is a security feature but a simple UX help
 but if you really wanna add this to the binder as well
 if HasMinValue is typed, it could also get a Color and you can calculate the contrast with the java colors if it is dark enough (which you have to do on the client side anyway)

After I got really surprised by V10+ that field.isInvalid does not what I expect it does
 the fields are insecure by design for me and I told every person I’ve spoken with: use the binder
 We are now at the fourth or fifth iteration of fields “validation” “improvements” and every time a new release comes out other things break
 it’s really frustrating as customer. Document the gotchas and focus on real things to improve the framework. If a real application uses only fields validation: I’m sorry not sorry.

60

A couple of thoughts. I my code the Binder the the one that is aware of all fields. Use case:

  1. Validate as a batch that the entire form is valid, as a part of a little MVP template we have. There is a usability preference where the customers care about if what they entered is valid once on the form submit. We’d lose that with per-field binding. Here is what works now just fine:
      binder.setValidatorsDisabled(false);
      final boolean ok = binder.validate().isOk();
      binder.setValidatorsDisabled(true);
      return ok;

With the new approach, can we preserve this behavior?

  1. Continuity. This sounds like a major departure from a lot of code written. Can we have both? I wasn’t there at 8.x break, but is it worth losing users of discontinuous changes?

  2. A small one, but I think the community would benefit a lot from more constructors supporting basic things like initial values, helper texts, styles, themes etc. Why should we create factory methods for every field? In your own example, which of these setters cannot be rolled into a constructor?

IntegerField participants = new IntegerField();
participants.setMin(1, "At least one participant is required");
participants.setRequired("Must define the number of participants");