vaadin / flow

Vaadin Flow is a Java framework binding Vaadin web components to Java. This is part of Vaadin 10+.
Apache License 2.0
618 stars 167 forks source link

AbstractValidator should accept an ErrorMessageProvider instead of only a String as errorMessage #5867

Open sandronm opened 5 years ago

sandronm commented 5 years ago

Hello!

You provide a class AbstractValidator with this single constructor:

    /**
     * Constructs a validator with the given error message. The substring "{0}"
     * is replaced by the value that failed validation.
     *
     * @param errorMessage
     *            the message to be included in a failed result, not null
     */
    protected AbstractValidator(String errorMessage) {
        Objects.requireNonNull(errorMessage, "error message cannot be null");
        this.messageProvider = value -> errorMessage.replace("{0}",
                String.valueOf(value));
    }

And then almost all your validators extend this one.

That's perfectly fine... except if you have a webapp using i18n...

Image the following example: you are using a Binder for your form:

this.myBinder.forField(getMyTextField())
.withValidator(new DateRangeValidator("My Translated message using the current locale", null, LocalDate.now()))
.bind(Member::getBirthday, Member::setBirthday);

I created the object DateRangeValidator and because the errorMessage is a String, I have no other choice to directly provide the translated text using the current locale.

After, if the user changes his language, the only possibility for me to adapt the error message is to create again a new Binder ! :/

But if you add a constructor in AbstractValidator (and children) using your own functional interface ErrorMessageProvider, the issue is solved!

this.myBinder.forField(getMyTextField())
.withValidator(new DateRangeValidator(createErrorMessageProvider(key), null, LocalDate.now()))
.bind(Member::getBirthday, Member::setBirthday);

private ErrorMessageProvider createErrorMessageProvider(String key) {
    return context -> callMyI18NProviderToGetTranslationOfKey(key);
}
denis-anisimov commented 5 years ago

Our DateRangeValidator which extends RangeValidator accepts hardcoded error message String which is not locale aware. And that's an issue, yes: you may not localize the message since it's aways the same regardless of locale. I'm pretty sure we have mechanism to show localized message (based on ValueContext class). So what we should do at least : provide a way to show localized message for DateRangeValidator and other pre-built validators. One way what I see is adding overloaded CTOR which accepts a factory/producer to return a string based on current locale.

Legioth commented 5 years ago

There are withValidator overloads that accept an ErrorMessageProvider. But to use those, you need to provide the validation logic as a SerializablePredicate instead of a Validator instance. This gets a little tricky since you'd need to create your own ValueContext to invoke the original validator logic.

As a workaround, it would be possible to create a Validator wrapper that delegates to the original validator, but then returns a custom ValidationResult with a different error message in case the result from the original validator indicates an error.

This could be something like this

public static <T> Validator<T> wrapValidator(Validator<T> originalValidator, SerializableFunction<ValueContext, String> errorMessageSupplier) {
 return (value, context) -> {
    ValidationResult result = originalValidator.apply(value, context);
    if (result.isError()) {
      return ValidationResult.create(errorMessageSupplier.apply(context.getLocale(), result.getErrorLevel().orElse(null));
    } else {
      return result;
    }
  }
}
thigg commented 4 years ago

It is quite unfortunate, that localization with the validators is different from everything else. Still this should be fixed. It is very annoying, that some of the built-in validators do not allow to build localized apps with them.

StefanPenndorf commented 1 year ago

Hi @Legioth,

Is there any (technical) reason that prevents you from following @sandronm suggestion to add those overloaded constrcutors using an ErrorMessageProvider?

Legioth commented 1 year ago

How is new DateRangeValidator(message, minDate, maxDate) different from e.g. new Button(label, clickListener)?

Frettman commented 1 year ago

How is new DateRangeValidator(message, minDate, maxDate) different from e.g. new Button(label, clickListener)?

By that logic, you're kind of putting ErrorMessageProvider's whole existence into question, no? There is e.g. Binder#withValidator(SerializablePredicate<BEAN> predicate, ErrorMessageProvider errorMessageProvider) or Validator#from(SerializablePredicate<T> guard, ErrorMessageProvider errorMessageProvider). So it would make sense to also make use of it in AbstractValidator and its subclasses.

Legioth commented 1 year ago

One key difference is that Button also has a setText method that you can run when the language changes. This forces you to either have separate code paths for initialization and for language changes, or to structure the code in a way that always use setText and never pass values through the constructor. It also leads to a situation where you will need to keep a reference (as an instance field or captured in a closure) to every component that should be updated when the locale changes (or create subclasses that implement LocaleChangeObserver).

In the end, many applications end up handling language changes with a page refresh because that's the only way that avoids making everything else more complicated.

The other way of looking at it is that maybe all other parts of the framework should also have overloads for accepting a string provider instead of a hardcoded string? This in turn leads towards https://github.com/vaadin/flow/issues/3801.

ErrorMessageProvider has many other uses in addition to being locale aware. That's probably the main reason for why they should be supported also through constructors and not only when using the shorthands that are optimized for use with lambdas.