HandlerMethodValidator should support simple Cross-Parameter constraints

[jmax01]

It seems sensible that the HandlerMethodValidator should directly handle Cross-Parameter constraints or at a minimum not throw an IllegalArgumentException.

Similarly to @Valid annotations on @RequestBody arguments, Cross-Parameter constraints are applied only if another argument of the method has an @Constriant declared.

Unfortunately, unless the ConstraintValidatorContext of the ConstraintValidator.isValid method is manipulated with a new ConstraintViolation that has a node kind in its path that MethodValidationAdapter.adaptViolations handles (PROPERTY, RETURN_VALUE, PARAMETER) an exception is thrown:

	at org.springframework.util.Assert.notEmpty(Assert.java:381)
	at org.springframework.validation.method.DefaultMethodValidationResult.<init>(DefaultMethodValidationResult.java:42)
	at org.springframework.validation.method.MethodValidationResult.create(MethodValidationResult.java:115)
	at org.springframework.validation.beanvalidation.MethodValidationAdapter.adaptViolations(MethodValidationAdapter.java:384)
	at org.springframework.validation.beanvalidation.MethodValidationAdapter.validateArguments(MethodValidationAdapter.java:246)
	at org.springframework.web.method.annotation.HandlerMethodValidator.validateArguments(HandlerMethodValidator.java:115)
	at org.springframework.web.method.annotation.HandlerMethodValidator.applyArgumentValidation(HandlerMethodValidator.java:83)
	at org.springframework.web.method.support.InvocableHandlerMethod.invokeForRequest(InvocableHandlerMethod.java:184)
	at org.springframework.web.servlet.mvc.method.annotation.ServletInvocableHandlerMethod.invokeAndHandle(ServletInvocableHandlerMethod.java:118)
	at org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerAdapter.invokeHandlerMethod(RequestMappingHandlerAdapter.java:926)
	at org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerAdapter.handleInternal(RequestMappingHandlerAdapter.java:831)
	at org.springframework.web.servlet.mvc.method.AbstractHandlerMethodAdapter.handle(AbstractHandlerMethodAdapter.java:87)
	at org.springframework.web.servlet.DispatcherServlet.doDispatch(DispatcherServlet.java:1089)
	at org.springframework.web.servlet.DispatcherServlet.doService(DispatcherServlet.java:979)
	at org.springframework.web.servlet.FrameworkServlet.processRequest(FrameworkServlet.java:1014)
	... 82 more

Here is a spring-boot-based example (apologies for not having time to do a 'pure' spring-framework one without security)

package org.springframework.gh33271;

import static java.lang.annotation.ElementType.*;
import static java.lang.annotation.RetentionPolicy.*;
import static org.springframework.http.MediaType.*;
import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.*;
import static org.springframework.test.context.TestConstructor.AutowireMode.ALL;
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.*;
import static org.springframework.test.web.servlet.result.MockMvcResultHandlers.*;

import java.lang.annotation.Documented;
import java.lang.annotation.Retention;
import java.lang.annotation.Target;

import jakarta.validation.Constraint;
import jakarta.validation.ConstraintValidator;
import jakarta.validation.ConstraintValidatorContext;
import jakarta.validation.Payload;
import jakarta.validation.constraints.NotBlank;
import jakarta.validation.constraintvalidation.SupportedValidationTarget;
import jakarta.validation.constraintvalidation.ValidationTarget;
import lombok.AllArgsConstructor;
import lombok.Value;
import lombok.experimental.Accessors;
import lombok.experimental.NonFinal;
import org.junit.jupiter.api.Test;
import org.springframework.boot.autoconfigure.EnableAutoConfiguration;
import org.springframework.boot.autoconfigure.ImportAutoConfiguration;
import org.springframework.boot.test.autoconfigure.web.servlet.MockMvcAutoConfiguration;
import org.springframework.boot.test.autoconfigure.web.servlet.WebMvcTest;
import org.springframework.context.annotation.Configuration;
import org.springframework.gh33271.Gh33271Test.CrossParameterSimpleExample.CrossParameterSimpleExampleValidator;
import org.springframework.security.test.context.support.WithMockUser;
import org.springframework.test.context.TestConstructor;
import org.springframework.test.web.servlet.MockMvc;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.PathVariable;
import org.springframework.web.bind.annotation.RequestParam;
import org.springframework.web.bind.annotation.RestController;

@WebMvcTest(controllers = { org.springframework.gh33271.Gh33271Test.Config.SimpleController.class })
@TestConstructor(autowireMode = ALL)
@AllArgsConstructor
class Gh33271Test {

    MockMvc mockMvc;

    @WithMockUser(value = "spring")
    @Test
    void testCrossParam() throws Exception {

        this.mockMvc
                .perform(get("/has/cross/param/simple/{pathTestString}", "path-test-string")
                        .queryParam("queryParmTestString", "query-param-test-string")
                        .with(jwt()))
                .andDo(print())
                .andReturn();

    }

    @Configuration
    @EnableAutoConfiguration
    @Value
    @NonFinal
    @Accessors(fluent = true)
    @ImportAutoConfiguration({ MockMvcAutoConfiguration.class })
    static class Config {

        @RestController
        @Value
        @NonFinal
        public static class SimpleController {

            @SuppressWarnings("static-method")
            @GetMapping(path = { "/has/cross/param/simple/{pathTestString}" }, produces = TEXT_PLAIN_VALUE)
            @CrossParameterSimpleExample
            String hasCrossParamSimple(@SuppressWarnings("unused") @PathVariable @NotBlank final String pathTestString,
                    @SuppressWarnings("unused") @RequestParam @NotBlank final String queryParmTestString) {

                return "can't get here";
            }

        }
    }

    @Documented
    @Constraint(validatedBy = CrossParameterSimpleExampleValidator.class)
    @Target({ CONSTRUCTOR, METHOD })
    @Retention(RUNTIME)
    public @interface CrossParameterSimpleExample {

        String message()

        default "Cross ${validatedValue}";

        Class<?>[] groups() default {};

        Class<? extends Payload>[] payload() default {};

        @SupportedValidationTarget(ValidationTarget.PARAMETERS)
        @Value
        @Accessors(fluent = true)
        class CrossParameterSimpleExampleValidator
                implements ConstraintValidator<CrossParameterSimpleExample, Object[]> {

            @Override
            public boolean isValid(final Object[] parameters, final ConstraintValidatorContext context) {

                return false;

            }
        }

    }
}

(Hacky) Workaround

This workaround will create two violations, one for each parameter with the same message.

            @Override
            public boolean isValid(final Object[] parameters, final ConstraintValidatorContext context) {
                //validation of the parameters array omitted
                var hibernateCrossParameterConstraintValidatorContext
                        = context.unwrap(HibernateCrossParameterConstraintValidatorContext.class);

                hibernateCrossParameterConstraintValidatorContext.disableDefaultConstraintViolation();

                var methodParameterNames = hibernateCrossParameterConstraintValidatorContext.getMethodParameterNames();
                
                var hibernateConstraintViolationBuilder = hibernateCrossParameterConstraintValidatorContext
                        .addMessageParameter("param0Name", methodParameterNames
                            .get(0))
                        .addMessageParameter("param1Name", methodParameterNames
                            .get(1))
                        .addMessageParameter("param0Value", parameters[0])
                        .addMessageParameter("param1Value", parameters[1])
                        .buildConstraintViolationWithTemplate(
                            "A {param0Name} with a value of {param0Value} and a {param1Name} with a value of {param1Value} are not compatible.");

                hibernateConstraintViolationBuilder.addParameterNode(0);
                hibernateConstraintViolationBuilder.addConstraintViolation();
                
                hibernateConstraintViolationBuilder.addParameterNode(1);
                hibernateConstraintViolationBuilder.addConstraintViolation();

                return false;

            }

[rstoyanchev]

Thanks for the report. It would help a lot if you could provide snippets, or a small sample to experiment with.

[jmax01]

Thanks for the report. It would help a lot if you could provide snippets, or a small sample to experiment with.

I will post one later today.

Note that it will simply be a GetMapping method with 2 arguments that have constraints and a method level cross-parameter constraint whose isValid method always returns false.

[jmax01]

Example added.

[jmax01]

Workaround added.

[rstoyanchev]

Thanks for the sample.

For cross-parameter validation, the property path has the method node and then a cross-parameter node, but no other useful info on which parameters were validated or what the argument values were. Is that because the ConstraintValidator implementation is empty and just returns false? In other words is the validator able to and expected to update the context in a way that would expose more information in the resulting ConstraintViolation?

At any rate, a cross-parameter violation does not fit into the current model where a MethodValidationResult holds value results (violations of parameter constraints) or bean results (violations of nested object field constraints) both of which aggregate violations for a specific method parameter.

We could create a CrossParameterValidationResult that holds basic MessageSourceResolvable's prepared from the annotation name and the target class/method name, but I'm not sure if we can get anything more useful than that.

[jmax01]

I 100% agree that the current model doesn't fit.

The challenge in my mind is, at the end of the day, we need a single exception type that holds all method argument ConstraintViolations.

This exception type should contain violations of cross-parameter constraints and violations of individual arguments annotated with @Constraints or annotated with @Valid.

This would provide a complete solution for handling pre-method execution ConstraintViolations.

The MethodValidationResult and MethodValidationException names imply this functionality but currently require a lot of finagling to achieve it (ConstraintViolation manipulation in the case of cross-parameter constraints and the requirement to have additional @Constraints on the method for '@Valid` to be evaluated ).

Thanks for the sample.

For cross-parameter validation, the property path has the method node and then a cross-parameter node, but no other useful info on which parameters were validated or what the argument values were. Is that because the ConstraintValidator implementation is empty and just returns false? In other words, is the validator able to and expected to update the context in a way that would expose more information in the resulting ConstraintViolation?

At any rate, a cross-parameter violation does not fit into the current model where a MethodValidationResult holds value results (violations of parameter constraints) or bean results (violations of nested object field constraints) both of which aggregate violations for a specific method parameter.

We could create a CrossParameterValidationResult that holds basic MessageSourceResolvable's prepared from the annotation name and the target class/method name, but I'm not sure if we can get anything more useful than that.

[rstoyanchev]

the requirement to have additional @Constraints on the method for '@Valid` to be evaluated

I'm not sure what you mean. @Valid does not need anything else to trigger validation. It is true that if there are no @Constraints at all on method parameter, then it is validated on its own, raising MethodArgumentNotValidException, which is designed to be very similar to MethodValidationException and one can be adapted to the other. This is covered in the docs.

You do raise a point that with method level @Constraints, currently we assume they are for the return value, but it may be for cross-parameter validation. If this is what you are pointing out, then yes, we do need to update that.

ConstraintViolation manipulation in the case of cross-parameter constraints

For the rest, I proposed above a CrossParameterValidationResult held in MethodValidationResult in addition to ParameterValidationResults. However, I don't see a way to tell which specific parameters were involved, which means we can't aggregate cross-parameter violations in any way. So we might just expose a list of MessageSourceResolvables from MethodValidationResult that represent cross-param violations.

[breun]

I see that getAllValidationResults() in MethodValidationResult has been deprecated and now delegates to getParameterValidationResults().

Cross-parameter validation results are not included in the result of getParameterValidationResults(), right?

I noticed that getAllErrors() and hasErrors() only use getParameterValidationResults(), so that would then also not include cross-parameter validation results? If so, is this intentional?

[breun]

I've been trying to create a test that results in a HandlerMethodValidationException with cross-parameter validation results, but in order to get the cross-parameter validation executed I need to annotate the @RestController class with @Validated, and once I do that I no longer get a HandlerValidationException, but a ConstraintViolationException, so maybe a HandlerMethodValidationException with cross-parameter validation results is just not something that can ever really occur in the real world?