Validator factory methods with BiConsumer

[making]

This commit introduces of method in Validator to provide a way to create a validator for the specific type <T> using BiConsumer<T, Errors> and define the validator in a functional way.

Validator passwordEqualsValidator = Validator.of(PasswordResetForm.class)
    .apply((form, errors) -> {
        if (!Objects.equals(form.getPassword(), form.getConfirmPassword())) {
            errors.rejectValue("confirmPassword",
                "PasswordEqualsValidator.passwordResetForm.password",
                "password and confirm password must be same.");
        }
    });

This also eliminates the boilerplate for implementing the supports method.

BONUS: Supporting BiConsumer allows us to magically adapt 3rd party Validators (like YAVI) that return BiConsumer.
(I'm the creator of YAVI.)

(original proposal)

Validator validator = Validator.of(CartItem.class)
    .apply(ValidatorBuilder.<CartItem>of()
        .constraint(CartItem::itemId, "itemId", c -> c.notEmpty())
        .constraint(CartItem::amount, "amount", c -> c.greaterThanOrEqual(0))
        .constraint(CartItem::price, "price", c -> c.greaterThanOrEqual(0))
        .build(Errors::rejectValue));

(revised version)

Validator validator = Validator.forInstanceOf(CartItem.class, ValidatorBuilder.<CartItem>of()
    .constraint(CartItem::itemId, "itemId", c -> c.notEmpty())
    .constraint(CartItem::amount, "amount", c -> c.greaterThanOrEqual(0))
    .constraint(CartItem::price, "price", c -> c.greaterThanOrEqual(0))
    .build(Errors::rejectValue));

[poutsma]

A nice proposal to modernise Validator, thanks @making!

I do find the usage of Function (and the resulting apply) a bit complicated, and I don't believe it's necessary. Instead, you can have the BiConsumer as a second parameter for the of method, like so:

static <T> Validator of(Class<T> targetClass, BiConsumer<T, Errors> validator) {
	return new Validator() {
		@Override
		public boolean supports(Class<?> clazz) {
			return targetClass.isAssignableFrom(clazz);
		}

		@Override
		public void validate(Object target, Errors errors) {
			validator.accept(targetClass.cast(target), errors);
		}
	};
}

[poutsma]

@making, We would like to make this part of Spring Framework 6.1. Are you ok with changing the PR to reflect our suggestions, or would you like me to pick it up from here?

[sdeleuze]

+1 for such functional approach in Validator.

[making]

@poutsma Updated the pr!

[sbrannen]

Instead, you can have the BiConsumer as a second parameter for the of method

That's a big improvement. 👍

[poutsma]

@poutsma Updated the pr!

Looking good!

A couple of improvements that our team discussed before it can be merged in 6.1.

• It would be nice to have a non-opinionated variant of the factory method that takes just a predicate and biconsumer, something like:
  static <T> Validator of(Predicate<Class<?>> supports, BiConsumer<T, Errors> delegate)
  This allows users to have their own supports logic in a predicate.
• I think it's cleaner to move the anonymous Validator implementation into its own package-protected class named DefaultValidator (or something similar). This helps keep the Validator interface cleaner, and also improves the readability of any potential stacktrace.
• Sometimes you want to specify an exact class match, instead of allowing all subclasses. So instead of using targetClass.isAssignableFrom, you'd use targetClass.equals. @rstoyanchev suggested to have two separate methods:
  • forInstanceOf(Class<T>, BiConsumer<T, Errors>) (using isAssignableFrom, similar to the current PR), and
  • forType(Class<T>, BiConsumer<T, Errors>) (using equals).
    Note that forInstanceOf and forType can be implemented using the predicate-based of method suggested above (i.e. of(targetClass::isAssignableFrom, delegate) and of(targetClass::equals, delegate)).

Let me know what you think about these suggestions.

[making]

@poutsma
I'm in favor of having both forInstanceOf and forType, but I'm not sure it makes sense to have the of(Predicate<Class<?>> supports, BiConsumer<T, Errors> delegate) method.
Since the validate methods accepts any object, to delegate validation logic to a BiConsumer<T, Errors>, the supports method must ensure that the object is instance of the T class.

I think it would make more sense to have a class like bellow, what do you think?

class TypedValidator<T> implements Validator {

	private final Class<T> targetClass;

	private final BiPredicate<Class<T>, Class<?>> typeVerifier;

	private final BiConsumer<T, Errors> delegate;

	public TypedValidator(Class<T> targetClass, BiPredicate<Class<T>, Class<?>> classVerifier, BiConsumer<T, Errors> delegate) {
		this.targetClass = targetClass;
		this.typeVerifier = classVerifier;
		this.delegate = delegate;
	}

	@Override
	public boolean supports(Class<?> clazz) {
		return this.typeVerifier.test(this.targetClass, clazz);
	}

	@Override
	public void validate(Object target, Errors errors) {
		this.delegate.accept(this.targetClass.cast(target), errors);
	}
}

and factory methods in Validator

	static <T> Validator forInstanceOf(Class<T> targetClass, BiConsumer<T, Errors> delegate) {
		return new TypedValidator<>(targetClass, Class::isAssignableFrom, delegate);
	}

	static <T> Validator forType(Class<T> targetClass, BiConsumer<T, Errors> delegate) {
		return new TypedValidator<>(targetClass, Objects::equals, delegate);
	}

[poutsma]

@making Sorry for not responding earlier.

I'm liking that TypedValidator, but if I find the BiPredicate<Class<T>, Class<?>> a bit confusing. I think it would be simpler and clearer to have a Predicate<Class<?>> instead, and in forInstanceOf and forType use the targetClass parameter instead of the field.

[making]

@poutsma
Sorry for the late reply. Did you mean

package org.springframework.validation;

import java.util.function.BiConsumer;
import java.util.function.Predicate;

class TypedValidator<T> implements Validator {

	private final Class<T> targetClass;

	private final Predicate<Class<?>> typeVerifier;

	private final BiConsumer<T, Errors> delegate;

	public TypedValidator(Class<T> targetClass, Predicate<Class<?>> typeVerifier, BiConsumer<T, Errors> delegate) {
		this.targetClass = targetClass;
		this.typeVerifier = typeVerifier;
		this.delegate = delegate;
	}

	@Override
	public boolean supports(Class<?> clazz) {
		return this.typeVerifier.test(clazz);
	}

	@Override
	public void validate(Object target, Errors errors) {
		this.delegate.accept(this.targetClass.cast(target), errors);
	}
}

and

	static <T> Validator forType(Class<T> targetClass, BiConsumer<T, Errors> delegate) {
		return new TypedValidator<>(targetClass, clazz -> Objects.equals(targetClass, clazz), delegate);
	}

	static <T> Validator forInstanceOf(Class<T> targetClass, BiConsumer<T, Errors> delegate) {
		return new TypedValidator<>(targetClass, targetClass::isAssignableFrom, delegate);
	}

?
It looks as same as proposed in #29890 (comment)
Maybe DefaultValidator is better as there is no longer guarantee that the input object is instance of the T class from TypedValidator perspective

[poutsma]

Thank you, @making, for submitting this PR. It has now been merged , see dd9f03d.

I made some changes, including the TypedValidator that was mentioned above. Hopefully it is now clear what I meant.

Feel free to comment or create subsequent PRs if you have any further suggestions regarding this piece of functionality.