RFC: Moving towards validators in fields

Flow
61

Just for the sake of argument, I’d like to explore an alternative approach where Binder remains in charge of validation.

Core assumption: Component validation is a simple mechanism based on constraints and pre-defined validators. It uses string error messages from i18n, with the potential for default messages in the future. The order of validators is fixed and is not intended to be configurable. In contrast, Binder validation should be a flexible tool for handling anything that is more complex.

Problem: Binder currently lacks flexibility because it doesn’t offer a coherent approach to:

  • Reusing component validators with error messages from i18n.
  • Allowing custom error messages for component validators (String or ErrorMessageProvider).
  • Adjusting component validators’ position, such as placing them after custom validators, for example.
  • Disabling component validators completely while preserving their visual aspects

API idea: Below is an API concept that could address these limitations:

datePicker.setRequiredIndicatorVisible(true);
datePicker.setMin(0);
datePicker.setMax(100);

// Default
binder.forField(datePicker) // includes component validators with i18n error messages by default (required, bad input, min, max)
  .withValidator(value -> value != LocalDate.now(), "Date must not be today")
  .bind("date");

// Cross field validator + component validators in custom positions
binder.forField(datePicker)
  .withValidator(dateOrAnotherFieldPredicate, "You must fill either one field or another")
  .withComponentValidator(datePicker.getRequiredValidator()) // error message from i18n, .asRequired() will be alias that also enables setRequiredIndicatorVisible
  .withComponentValidator(datePicker.getBadInputValidator()) // error message from i18n
  .withComponentValidator(datePicker.getMinValidator()) // error message from i18n
  .withComponentValidator(datePicker.getMaxValidator()) // error message from i18n
  .bind("date");

// Custom error messages for component validators
binder.forField(datePicker)
  .withValidator(dateOrAnotherFieldPredicate, "You must fill either one field or another")
  .withComponentValidator(datePicker.getRequiredValidator(), "Field is required") // .asRequired("Field is required") will be alias that also enables setRequiredIndicatorVisible
  .withComponentValidator(datePicker.getBadInputValidator(), "Invalid date")
  .withComponentValidator(datePicker.getMinValidator(), "Date must be after 0")
  .withComponentValidator(datePicker.getMaxValidator(), "Date must be before 100")
  .bind("date");

// Disable component validators completely, but preserve visual aspect
binder.forField(datePicker)
  .setComponentValidatorsDisabled(true) // This should be used with caution, as it disables the bad input validator too
  .withComponentValidator(datePicker.getBadInputValidator()) // can still be added individually
  .bind("date");

This approach implies that asRequired() will become an alias for withComponentValidator(component.getRequiredValidator()) + component.setRequiredIndicatorVisible(true), as follows:

Validator asRequired() {
  component.setRequiredIndicatorVisible(true);
  return withComponentValidator(component.getRequiredValidator())
}

Validator asRequired(String errorMessage) {
  component.setRequiredIndicatorVisible(true);
  return withComponentValidator(component.getRequiredValidator(), errorMessage)
}

With this, asRequired() will work pretty much the same as before except that it will start using the error message from i18n (if available) in the case it was called without arguments.

So, to conclude, the idea is to leave components unchanged and instead enhance Binder API to support moving component validators and overriding their error message.

62

Just a note about this. There is also an overloaded method asRequired(Validator<TARGET> validator); in the binder which - at least in my case - should NOT delegate to the default required validator of fields, because the field’s required validator was replaced in all our applications with a more advanced “algorithm” that takes in spaces and other blank character before comparing the value to the empty value.

1 Like
63

The most important requirement from my side: whatever you change, don’t break existing code.

4 Likes
64

That’s also how we initially viewed features like setMin. But we changed our minds once we realized that it’s quite reasonable for users to assume that the value will be properly validated when using a framework that is marketed with a strong security posture. While we could technically defend ourselves with “didn’t you read the documentation?”, that’s not a super popular approach either.

In previous versions, setMin wasn’t secure even when used through Binder (unless you also add a matching constraint on the Binder level) but we addressed that by introducing the getDefaultValidator mechanism (and unfortunately introduced a chain of other problems as a side effect).

We haven’t (yet) fixed isInvalid to also offer the same security when not using Binder. Maybe we should introduce an isValid method that delegates to getDefaultValidator and change the name of isInvalid to something that avoids giving the impression that it actually validates anything (while leaving the original as deprecated)?

My example has two parameters to configure the component: the reference color and the required contrast ratio. It would thus be necessary to have a type wrapping both values and using it as bindingBuilder.setMin(new ColorContrast(blue, 4.5)). I’m also quite sure that there are field-like components in Directory where neither setMin, setMax or setPattern would be a good match.

An even more important observation here is that the field class implementing HasMinValue<ColorContrast> for use with Binder would also have a public setMin method that allows writing field.setMin(new ColorContrast(blue, 4.5)). We would thus be back on square one with two different ways of doing what seems to be the same thing and a reasonable assumption that they are indeed functionally equivalent for a field bound with Binder. Moving field value validation out of Binder would remove that ambiguity and also make it possible for the field to provide a streamlined syntax like field.setMinColorContrast(blue, 4.5).

The key point is that my suggested field.addValidator would be functionally equivalent to the current bindingBuilder.addValidator and your suggested binding.setMin would be functionally equivalent to the current field.setMin. It’s only a question about what would be the most clear and convenient location of that API but not a question about how the UI should behave when the API is used (in combination with Binder).

65

IMHO min/max values are always a security concern - but lets not mock the dark past. ;)

While I understand the idea behind both directions; I’m not sure how good this could be done in a backwards compatible manner… I’ve seen so many people using isInvalid or overwriting it in their own code or even creating custom fields with… so that can rapidly create another disaster… additionally with all the side effects thanks to HasValidator or HasValidationProperties interface and so on.

Sounds quite reasonable and nothing I would speak against :) HasMinValue<?> does not have to be the same as the value type of the field.

That’s exactly where my concern comes from. The fields have to have the setMin(ColorContrast, String or setMin(ColorContrast, ErrorMessageProvider otherwise even a single field would be forced to use the Binder - which I’m personally in favor… but I can see why people don’t wanna do it. On the other side fields are IMHO purely “cosmetic” and the Binder is the glue-code that is used to validate, convert and bind properties to fields. A lot of applications I’ve seen and developed over the last decades look like this:

addFieldToLayoutAndBind(new TextField("Name"), 2 /* col */, MyBinder::bindName);

where the MyBinder class extends Binder, is in a separate file and has all validation logic allowing to instantiate the Binder in Unit Tests and Test that logic without UI / Layout. And as you can see… the fields are not even stored in a variable… making additional configurations to the fields even more cumbersome without proper Builder.

Example with such a custom Binder I’ve written:

grafik
grafik901×422 46.2 KB

66

It seems like this suggestion is for the separate question on how to deal with validator ordering while it does nothing to address the original concern about having some field value validation configuration on the field instance (setMin) and some configuration on the binding (withValidator).

Practical details

Some component validators must come before other validators for the validator chain to make any sense at all. The bad input validator is the most obvious example of this but there might also be other similar cases.

binder.forField(datePicker)
  .withValidator(date -> checkSomething(date), "Lorem")
  .withComponentValidator(datePicker.getBadInputValidator())
  .bind("date")

What will the value of the date argument to the callback be in the case of bad input from the user? One way of “fixing” this is to say that the validator callback would assumed to automatically fail which means that Lorem would be used as the error message for bad input. But this raises the question of what the purpose would then be from setting getBadInputValidator() in some specific location of the validator order if its own error message would never be used anyways?

What happens in this case and why should we have an API that makes this possible in the first place?

binder.forField(datePicker)
  .withComponetValidator(someOtherDatePicker.getMinValidator())
  .bind(x)

Finally, I assume there’s a type parameter for the component validator type so that you couldn’t accidentally add a text field component validator to the binding of a date field? The problem is that this would require adding another type parameter to BindingBuilder which in turn leads to a breaking change.

Alternative variation

I assume withComponentValidator delegates to something along the lines of addComponentValidator on the field instance so that the field can know how to behave? This leads to the question of whether dateField.addComponentValidator(dateField.datePicker.getMinValidator()) should be different from bindingBuilder.withComponentValidator(dateField.datePicker.getMinValidator()) and why we provide two different ways of doing what seems to be the same thing?

If the component anyways has API related to how component validation is ordered, then what’s the benefit of using Binder as an intermediary instead of doing it directly on the field like this?

datePicker.setRequiredIndicatorVisible(true);
datePicker.setMin(0);
datePicker.setMax(100);

datePicker.addValidator(dateOrAnotherFieldPredicate, "You must fill either one field or another");
datePicker.addComponentValidator(datePicker.getRequiredValidator());
datePicker.addComponentValidator(datePicker.getBadInputValidator());
datePicker.addComponentValidator(datePicker.getMinValidator());
datePicker.addComponentValidator(datePicker.getMaxValidator());

binder.bind(datePicker, "date");

One benefit of doing it directly on the field is that we can get rid of the indirection through addComponentValidator and instead introduce streamlined methods that still allow ordering component validators (while preserving the current behavior of e.g. setMin).

datePicker.addValidator(dateOrAnotherFieldPredicate, "You must fill either one field or another");
datePicker.addRequiredValidator(); // Also sets the required indicator as visible with an alternative overload that doesn't change the indicator
datePicker.addBadInputValidator();
datePicker.addMinValidator(0);
datePicker.addMaxValidator(100);

binder.bind(datePicker, "date");
1 Like
67

I don’t think this case should change at all. The intent is to change where the API is defined without changing the behavior of that API. If a field is bound by a Binder, then its validation status should still respect things like binder.setValidatorsDisabled regardless of where the API for configuring validators is located.

I think the current API could be kept as deprecated for the foreseeable future since this proposal wouldn’t mean that we change any behavior.

There are many traps on that route due to the way it leads to an explosion of permutations for the constructor parameters and I have a hard time seeing how we could agree on which are the most important permutations. As one example, the overload for setMin + setRequired would have the same signature as the overload for setMax + setRequired.

68

Interaction with a new isValid method is not a concern for backwards compatibility since no existing code uses that method (except if custom classes introduce their own method with a colliding name). The deprecation of isInvalid would have to be implemented so that the method with the new name delegates to the old one so that any custom implementations of the old method are still respected.

I don’t see any immediate practical problem there. addFieldToLayoutAndBind gets access to the field instance which means that it could do field.setMax(someValue) just as well as bindingBuilder.setMax(someValue).

The test logic already depends on a UI component instance (assuming that’s a com.vaadin.flow.component.textfield.TextField) and indirectly depends on how that instance implements isEmpty() (assuming forRequiredField delegates to the regular asRequired). Would anything change if it would additionally also depend on how that instance implements e.g. addValidator or setMax?

I do still see a “philosophical” problem here. It would be preferable to completely get rid of the TextField dependency from unit tests rather than becoming even more dependent on it. I suspect this leads towards the Property/signal concept mentioned by Peter and me. This would allow having a “binding” API that is specific to the field type without becoming tied to a field instance. The reason this works is that the generic “binder” API enters the picture at the end of the configuration chain instead of starting from it: field.getValueBinding().addFieldSpecificValidator(someParameter).bind(binder, Bean:getFoo, Bean::setFoo);. You could then create a standalone “value binding” instance of the same type without a component instance in the unit tests.

69

The practicle problem arises from the fact that addFieldToLayoutAndBind soul reason would be to apply the field to the layout and executing the given bind call to glue field and appropriate binding together. It should not know about which type of field is added, neither should it add additional validations - this would defeat its purpose.

I’ve shown you the most basic example I had on hand - “real” projects of mine don’t have bindName(TextField field), instead they use bindName(HasValue<?, V> field) so that I don’t have to use the fields in unit tests. The assumption about a delegating asRequired() sadly isn’t true as well because I had to resort to custom logic since text fields don’t check for blank in the default implementation - therefore I had to resort to use asRequired(Validator<V> validator). Users trying everything to hack their way around typing too much ;)

    // Method in "TagBinder extends VtBinder<Tag>"
    public void registerIdentifier(HasValue<String> field) {
      forRequiredField(field)
        .withValidator(new StringLengthValidator("Identifier too long.", 1, TagConstants.IDENTIFIER_LENGTH))
        .withConverter(new TagIdentifierConverter())
        .bind(Tag::getIdentifier, Tag::setIdentifier);
    }

That would be a “real” example which allows me to test the full chain of validation, converting and binding from / to my property by “just” instantiating a new TagBinder() with a “simple” TestHasValueField<T> to get rid of the UI dependency.

When I “have to use” field “exclusive” APIs to configure validations / constrains… I have to additionally test those… either by making my unit tests “component aware” or find some other way.

70

This suggestion was an attempt to extend the current concept, where Binder is used to enhance the component validation, to avoid the need to move validators to the component level by providing a proper way to handle constraint validation from the Binder. It was intentional to keep some configuration at the component level. It was supposed to follow the web platform approach, where you have constraints and can build your own validation on top of them; where after implementing your validation, you can still use the maxlength property to prevent users from entering more text than allowed.

That’s a good question. I assumed it would be beneficial to have the ability to move getBadInputValidator() for cases involving cross field validation. It seemed logical to inform the user that he has filled in the wrong field as soon as possible, even if he has entered something unparsable into that field. However, in practice, once you allow it to be moved, the mentioned problem arises, which is what happens if the prior validator treats the unparsable value as the empty value. To implement this cross field validator correctly, the developer would likely have to use getBadInputValidator() inside to determine if the empty value is actually bad input. So, maybe it’s ok to keep it immovable considering these challenges.

I considered this to be an edge case, so not a requirement, but you make a good point.

My assumption was that Binder would be able to detect if the same instance of the validator was already used in the chain and move it to the new position rather than inserting a duplicate.

I now seem to understand why I’m concerned about shifting validators to the component. It’s because this would allow you to add validators practically anywhere in your codebase where you have a reference to the field. The mentioned example looks pretty nice. However, in a real project, this ability to scatter validation configuration can result in a mess unless you come up with your own abstractions. Binder seems to avoid this issue by requiring the use of the Builder pattern. This concern was behind the suggestion to explore ways to keep Binder (or other abstraction outside the field) managing validation.

1 Like
71

I guess that validation would be added by MyBinder::bindName but that doesn’t change my original point. bindName can also have access to the field instance and can thus configure the validation regardless of where setMin is defined (though a HasMinValue interface might be useful regardless to keep bindName and addFieldToLayoutAndBind generic enough)

I wonder if that should be seen as a bug in TextField.isEmpty (as well as any other field that has "" as its empty value)? Though I it might also be a breaking change in some special case to suddenly make it behave differently.

As a user, I try with non-breaking spaces as the next thing. And then -. And then a suitable amount of Lorem Ipsum. In any case, it’s the application that loses if the validation requirements are not aligned with the user’s interest. :slight_smile:

Is the problem that the APi is on the HasValue instance or that it’s some API that is defined directly on a specific field class? In other words, could your concerns be mitigated “simply” by making IntegerField implement HasMinValue even without also making binder.forField “aware” of that HasMinValue interface?

72

I think I’ve raised this “is blank” concern in the past in the VIP discord channel because it feels like a security problem (evasion of validation) but because of the possible nature of such a breaking change, I came up with my own required validator instead of pushing / pursuing the idea to get you to change it.

Don’t get me started here :wink: Yes, yes and yes… I also have to check for NBSP / NNBSP and so on… I’ve literally build a rule engine on top of DIN 91379 – Wikipedia - i think I’ve talked briefly about this with Matti at JCon this year. (Topic for another day)

I personally would be more in favor of having top level API on the binder (centralized validation location in my code base) but this would allow me to build it myself without requiring direct dependencies to components - so that would be a okayish trade of Imho. What I’m currently struggle is how I envision the usage of error messages in that regard or let’s say: what methods / overloads would be in such HasMinValue interface? Overloaded methods that allows me to supply an ErrorMessageProvider? Validator? Only a number?

73

There are many traps on that route due to the way it leads to an explosion of permutations for the constructor parameters and I have a hard time seeing how we could agree on which are the most important permutations. As one example, the overload for setMin+setRequiredwould have the same signature as the overload forsetMax+setRequired .

I cannot tell if anyone would care the order of parameters.

74

It’s not about the order but rather how to distinguish between different permutations of optional parameters. With three optional validation factors for IntegerField (required, min and max), we have these different permutations:

  1. No validation rule
  2. Only required
  3. Only min
  4. Only max
  5. Required and min
  6. Required and max
  7. Min and max
  8. All three

The challenge is that 3 and 4 would have the same method signature (one number and one string) and the same between 5 and 6. There are also six existing constructors that need to be taken into account. This leads to a significant increase in the potential number of permutations to consider and leads to even more cases where reasonable permutations wouldn’t have a unique signature.

All in all, there would be at least 9 different parameters (label, placeholder, initial value, change listener, required message, min limt, min message, max limit, max message) in an “ultimate” constructor and that’s more than the 6 or 7 that seems to be the maximum recommendation in most code style guidelines that I have seen.

75

Optional parameters should not be part of the constructor. The builder pattern could help here

76

I don’t think that’s any better than your suggestion that leads towards spreading out the configuration into one part for the component configuration (datePicker.setMin(0)) and one part for the binder configuration (withComponentValidator(datePicker.getMinValidator())). One could of course put both lines at the place where the binding is configured but that argument works just as well with my suggestion.

In general, we should make avoid making “bad” solutions easier than the “good” ones but I don’t think it’s reasonable to make the “good” approach worse than it needs to be only to prevent a “bad” solution from being equally easy.

At the same time, I’m more and more convinced that neither of the alternatives presented here is the right one. What we really need is something like a Property that has a configuration API that is separate from the UI component class while still being a standalone entry point rather than wrapping through something like Binder::forField which forces the API towards the lowest common denominator. But that’s a bigger design effort than what I would want to dig into in detail in this already long thread.

One additional insight that I had is that the type-specific validation API would be based on the data type rather than the UI component type. All field components with a String value does reasonably have the same validation constraints (e.g. allowed chars, mask / regular expression for the final value, min length, max length). There needs to be an optional hook back to the UI component so that it can configure its UI state management based on the used constraints that it supports (and their order) but the key point is that the actual validation of the value is independent of the component instance. If some UI component wants to add “native” support for some additional type of constraint, then it can define a custom property type subclass (i.e. SpecialStringProperty extends StringProperty) that adds an API for configuring that specific constraint in a way that the component can use.

77

There will anyways be a point where any security restrictions become highly context dependent. As a trivial example, the limit of the max length of a string might be the number of bytes with some arbitrary charset, or the number of Java-native UTF-16 characters, or the number of Unicode code points. Which one to use will be dependent on how the backend logic uses the string. (I recently encountered a plain text form in a government system that explicitly expands \n into \r\n before counting the number of characters. My report that was carefully trimmed down to what I counted as 3998 characters needed some additional rework to fit into their 4000 char limit)

At the same time, I think it makes lots of sense to trim whitespace from strings before checking if the field is “empty” since I assume that makes sense in most cases. The question is just whether we could introduce that change in a minor release of if we’d have to wait for a major? Let’s have that discussion in Ignore white space when evaluating `isEmpty()` for String-typed `HasValue` implementations · Issue #19847 · vaadin/flow · GitHub.

That is an excellent question. As the baseline, we have the upcoming changes in Vaadin 24.5 which lets you provide a translated error message string specifically for that case with IntegerField::setI18n. This means that only a number could satisfy many cases at the expense of requiring an setI18n call for each field instance and not allowing to include the current value in the error message. I assume setManualValidation(true) could be used to make the field update the state of the - step button while still not using the static error message.

The question is thus whether it would be enough with just a number? It might be convenient with some helpers around it but that’s a separate discussion once the bare minimum has been identified.

78

I would go with a major release and really good documentation.

Similar to other discussion we had, I would prefer that I can pass at least a message, at best something along the lines of ErrorMessageProvider.

public interface HasMinValidator<V> {
  
  void setMinValidator(V minValue, String errorMessage);
}

Fields would implement this interface; allowing me to use this instead of relying on the i18n property. We could even say: The fields setMinValue(1) calls setMinValue(1, getI18n().getMinErrorMessage(). If the errorMessage does not match the one provided by getMinErrorMessage(), the field could update itself and its corresponding i18n object - allowing me to fully customize it from the Binding without the i18n “implementation detail”.

79

My gut instinct is to do what users ask for. If no one asked for 3, 4, 7 or 8, why bother?

80

If we have 1 user asking for option A and 0 users asking for option B, then we cannot rule out the possibility that the next 5 users may ask for option B. We would need to have a bigger sample than only a single request to have any degree of certainty about what are the most commonly needed variants.