Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spring MVC and WebFlux docs need to say method validation applies if any method parameter has constraint annotations #32082

Closed
dreis2211 opened this issue Jan 22, 2024 · 15 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: documentation A documentation task
Milestone

Comments

@dreis2211
Copy link
Contributor

dreis2211 commented Jan 22, 2024

Hi,

when upgrading to Spring-Boot 3.2.2 and therefore SF 6.1.3 we noticed a change in behaviour that causes one of our @ExceptionHandlers not to be triggered anymore, when the @RequestBody fails validation (in this case when Body#target is null).

	@PostMapping(value = "hello", produces = MediaType.TEXT_PLAIN_VALUE)
	public String hello(@RequestBody @Valid @NotNull Body body) {
		return "Hello " + body.target;
	}

	@ResponseStatus(HttpStatus.BAD_REQUEST)
	@ExceptionHandler(MethodArgumentNotValidException.class)
	public Map<String, String> handleValidationExceptions(MethodArgumentNotValidException ex) {
		System.out.println(ex.getMessage());
		Map<String, String> errors = new HashMap<>();
		ex.getBindingResult().getAllErrors().forEach((error) -> {
			String fieldName = ((FieldError) error).getField();
			String errorMessage = error.getDefaultMessage();
			errors.put(fieldName, errorMessage);
		});
		return errors;
	}

	public static class Body {

		@NotNull
		@Valid
		private String target;

		public Body() {}

		public void setTarget(String target) {
			this.target = target;
		}
	}

Interestingly, this can be worked around by removing the @NotNull from the method parameter.

I think this is somewhat related to #31711 but without the indication that the fix causes the additional annotations not to work anymore or respectively breaking the validation somehow if present.

I've created a minimal reproducer under https://github.com/dreis2211/method-argument-validation-bug to ease testing. Rolling back to SB 3.2.1 fixes it, similar to removing the @NotNull annotation. I'd appreciate if this is either fixed or explicitly documented. Use cases with List request bodies in combination with a @NotEmpty seem to be unaffected because they don't seem to throw a MethodArgumentNotValidException in the first place (also with SB 3.2.1).

In case the @NotNull annotation is not supported under certain circumstances, feel free to turn this into a documentation issue.

Let me know if you need anything.
Cheers,
Christoph

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jan 22, 2024
@quaff
Copy link
Contributor

quaff commented Jan 23, 2024

		@NotNull
		@Valid
		private String target;

You should remove @Valid here.

@dreis2211
Copy link
Contributor Author

@quaff without @Valid no validation happens at all.

@ChrAh88
Copy link

ChrAh88 commented Jan 23, 2024

I think instead of MethodArgumentNotValidException the HandlerMethodValidationException is thrown.

Is your Controller annotated with @Validated? If not, can you try it?

@dreis2211
Copy link
Contributor Author

@ChrAh88 Class-level @Validated + method level @Valid would also work. It's not so much about strategies on how to workaround the issue. In my case I can also remove the @NotNull and it works likewise - as stated above.

Method validation on SF 6.1 should not need the @Validated anymore to apply method level validation, anyway.

As said. I'm also totally fine with having this as a documentation-only issue, but it clearly needs clarification what is supported and what is not.

@crazyav1
Copy link

Same issue after update to 6.1.3 instead of MethodArgumentNotValidException the HandlerMethodValidationException is thrown.

@quaff
Copy link
Contributor

quaff commented Jan 24, 2024

@quaff without @Valid no validation happens at all.

According to the Javadoc, @Valid should be used for nested property, obviously String is not.

Have you tried to remove @NotNull from method parameter and @Valid from field?

	public String hello(@RequestBody @Valid /*@NotNull*/ Body body) {
		return "Hello " + body.target;
	}
	public static class Body {

		@NotNull
		/*@Valid*/
		private String target;

		public Body() {}

		public void setTarget(String target) {
			this.target = target;
		}
	}

@quaff
Copy link
Contributor

quaff commented Jan 24, 2024

Same issue after update to 6.1.3 instead of MethodArgumentNotValidException the HandlerMethodValidationException is thrown.

I think it's expected if you remove @Validated from class level then built-in web method validation will be used, see #30645.
@dreis2211 Can you confirm that HandlerMethodValidationException is triggered instead of MethodArgumentNotValidException with your case?

@crazyav1
Copy link

crazyav1 commented Jan 24, 2024

@Validated requirement seems like breaking change to me, but I don't find this mentioned anywhere . And if you try to find any tutorial on @valid everyone is excpecting MethodArgumentNotValidException.

@dreis2211
Copy link
Contributor Author

@quaff HandlerMethodValidationException is thrown. With a slightly adjusted error handler this would also work.

I don't want to repeat myself, but if this is a documentation-only issue I'm totally fine with that. But I think it needs documentation what the preferred way is and what is not supported.

@ChrAh88
Copy link

ChrAh88 commented Jan 24, 2024

@dreis2211 it's documented here: https://docs.spring.io/spring-framework/reference/web/webmvc/mvc-controller/ann-validation.html
Second, if Java Bean Validation is present AND other method parameters, e.g. @RequestHeader, @RequestParam, @PathVariable have @constraint annotations, then method validation is applied to all method arguments, raising HandlerMethodValidationException in case of validation errors.

With the fix of #31711, also SmartValidators with contained jakarta validator are respected. In case of spring boot the validator "ValidatorAdapter" is a SmartValidator with a contained jakarta validator.

But yes, spring boots upgrade instructions to version 3.2 or the release notes for 3.2.2 should be updated in my opinion.
https://github.com/spring-projects/spring-boot/wiki/Spring-Boot-3.2-Release-Notes#upgrading-from-spring-boot-31

@rstoyanchev rstoyanchev self-assigned this Jan 24, 2024
@rstoyanchev rstoyanchev added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Jan 24, 2024
@crazyav1
Copy link

crazyav1 commented Jan 24, 2024

@ChrAh88 From documentation it seems that if only @Valid @RequestBody SomeBody body is used in method parameters, the only MethodArgumentNotValidException is raised, and then we add some other annotation to other parameter,e.g. @Valid @RequestBody SomeBody body, @NotNull @RequestParam Param param will raise HandlerMethodValidationException even if validation error still in body?

@rstoyanchev
Copy link
Contributor

@ChrAh88 thanks for the analysis, which is spot on. And @crazyav1 you're right that the documentation needs a little update since method validation applies if any method parameter has constraint annotations.

@rstoyanchev rstoyanchev added type: documentation A documentation task and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Jan 24, 2024
@rstoyanchev rstoyanchev added this to the 6.1.4 milestone Jan 24, 2024
@rstoyanchev rstoyanchev changed the title MethodArgumentNotValidException not triggered anymore Validation section of Spring MVC and WebFlux docs need to say method validation applies if _any_ method parameter has constraint annotations Jan 24, 2024
@rstoyanchev rstoyanchev changed the title Validation section of Spring MVC and WebFlux docs need to say method validation applies if _any_ method parameter has constraint annotations Validation section of Spring MVC and WebFlux docs need to say method validation applies if any method parameter has constraint annotations Jan 24, 2024
@rstoyanchev rstoyanchev changed the title Validation section of Spring MVC and WebFlux docs need to say method validation applies if any method parameter has constraint annotations Spring MVC and WebFlux docs need to say method validation applies if any method parameter has constraint annotations Jan 24, 2024
@dreis2211
Copy link
Contributor Author

dreis2211 commented Jan 24, 2024

@rstoyanchev What is your recommendation for the use-case at hand? Dropping the @NotNull, changing the @ExceptionHandler (aka creating a duplicated one for HandlerMethodValidationException) or using @Validated or a combination of things?

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Jan 24, 2024

@dreis2211, I would suggest dropping the @NotNull since @RequestBody has a required flag, but generally both exceptions could arise from controller methods and should be handled in some global @ExceptionHandler method like an @ControllerAdvice extension of ResponseEntityExceptionHandler.

@dreis2211
Copy link
Contributor Author

Thanks. Exactly what I was hoping for (because I did it already :D )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: documentation A documentation task
Projects
None yet
Development

No branches or pull requests

6 participants