feat: make field components implement HasValidator interface

[DiegoCardoso]

Description

• Add HasValidator<T> to field components and make them override getDefaultValidator method
• Make field components return string containing the reason for the validation error
• Extract some common logic to an util class on the shared module

Fixes #3381

Type of change

• Bugfix
• Feature

Checklist

• I have read the contribution guide: https://vaadin.com/docs-beta/latest/guide/contributing/overview/
• I have added a description following the guideline.
• The issue is created in the corresponding repository and I have referenced it.
• I have added tests to ensure my change is effective and works as intended.
• New and existing tests are passing locally with my change.
• I have performed self-review and corrected misspellings.

Additional for Feature type of change

• Enhancement / new feature was discussed in a corresponding GitHub issue and Acceptance Criteria were created.

[knoobie]

Make sure those non-explaining error messages are not overriding errors provided by the binder (like asRequired("My awesome name of the field is empty").

Additionally, if those strings are ever shown to the end-user there should be a way to translate them. All fields should provide an i18n mapping for the possible errors and their outcome. Consider using an enum of possible errors with a translation key that can be look-up via getTranslation or other means.

[DiegoCardoso]

Make sure those non-explaining error messages are not overriding errors provided by the binder (like asRequired("My awesome name of the field is empty").

Hmmm. That's a valid point. Currently the HasValidator with Binder works so that the getDefaultValidator() is called before the validation constraints from Binder are called. So if asRequired() is called on the binding definition, that will effectively set the component's required property to true and the validation will fail with the VALIDATION_REQUIRED_ERROR message. From getDefaulltValidator(), I can't see a way of checking if there's a custom message defined on asRequired(), though.
I also don't think we can't remove the required check from the component (even when used with Binder), since my assumption is that it should work if the required is set on the component setRequired()/setRequiredIndicatorVisible()`. So I need to investigate how to proceed here.

Additionally, if those strings are ever shown to the end-user there should be a way to translate them. All fields should provide an i18n mapping for the possible errors and their outcome.

Good point. There should be a way of defining custom messages.

[knoobie]

Just a thought: I don't think it's possible to fix this behavior on your side if getDefaultValidator runs before the binder validations. I think this should be fixed / done inside flow - and the getDefaultValidator() should run AFTER the binder validation; meaning the getDefaultValidator() should be registered once the field and it's validations are bind. This allows to stop the validations once the binder fails and uses this message instead of the messages / validations provided later by the field

[DiegoCardoso]

We decided to exclude required from the validation done by getDefaultValidator(), so if one wants to use binder, then only asRequired() should be used.

[knoobie]

That raises multiple concerns.. short question: Can the field validation be disabled? We have moved all our validations to the binder over the years, because the client side wasn't secure and intuitive.

Just another example we currently use:

• DatePicker:.setMax(LocalDate.now()); is used to disable future dates to get the user instant visible feedback that this date isn't selectable, but the user can still type tomorrow in.. resulting on a validation on the server with a proper error message like "Birthday can't be set to the future."

With this change, what error message is shown? The server side error message or the field validation? I would want the server side error message to be shown.

[DiegoCardoso]

First, thanks for all the feedback so far, they have been raising some internal conversation about the feature.

For now, we want to build a MVP to make this behaviour change on the fields to make client and flow constraints part of the binder validation. Later on, we will make the adjustments needed, like the ones you suggested.

With that said, we have been considering have an API to disable component validation and also to deliver these changes under a feature flag, so that we can release it and validate the new behaviour with more customers before making it the default behaviour.

[knoobie]

deliver these changes under a feature flag

This would be really good! Thanks for the discussion!

Just to clarify: I'm totally with the addition of the feature, because I've requested this as "bug" 2 years ago (vaadin/flow#7767) - but it feels the currently MVP is behind the feature richness of the binder in terms of user and developer experience, that's why I wanted to mention it. Additionally because of the issue mentioned, we have migrated all our Vaadin 8 applications to the Binder only paradigm in V23.1 - where it would be a huge effort to adopt to a surprising change like this.

Something similar happened to us with an addtional Binder feature / behaviour change in the past, where it did something we didn't expect and had problems with the behavior change, because we heavily heavily really on Binder as the stable foundation of all our applications (validation and security) - vaadin/flow#10342

[knoobie]

Suggestion to overcome the current limitation of the hard-coded error messages:

Provide overloaded methods for the validation methods, like setPattern, setMax and so on with the new error message, default is used if no other message was provided.

Example:

var field = new TextField();
 // uses default error message
field.setPattern("^[a-zA-Z]$");
// uses default error message 
field.setPattern("^[a-zA-Z]$",  "Given input could not be validated. Pattern doesn't match."); 
// Function or something else that provides Locale 
field.setPattern("^[a-zA-Z]$",  locale -> getTranslation().translate("field.pattern.no-match", locale)); 
// BiFunction or something else that provides Field + Locale 
field.setPattern("^[a-zA-Z]$",  (field, locale) -> "Field" + field.getCaption + " is not valid.." ); 

[vursen]

A couple of more suggestions to keep everything consistent.

[vursen]

It would probably make sense to implement HasValidator also for RadioGroup and CheckboxGroup as they will need ValidationStatusChangeListener to trigger the binder validation on the validated event.

[sonarqubecloud]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
132 Code Smells

No Coverage information
4.6% Duplication

[vaadin-bot]

This ticket/PR has been released with Vaadin 23.2.0.