From d902bd7696ade86cc82491fa15e19c30537dc33e Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Thu, 27 Jun 2024 16:16:48 +0200 Subject: [PATCH] Reject ModelMap argument types in WebFlux Prior to this commit, if ModelMap was used as an argument type in a WebFlux controller method, the user encountered an exception similar to the following. java.lang.IllegalStateException: argument type mismatch Controller [example.SampleController] Method [java.lang.String example.SampleController.index(org.springframework.ui.ModelMap)] with argument values: [0] [type=org.springframework.validation.support.BindingAwareConcurrentModel] [value={}] However, the above error message is a bit cryptic since the error occurs while attempting to invoke the controller method with an instance of BindingAwareConcurrentModel which is not compatible with ModelMap. More importantly, this error message does not explicitly convey to the user that a ModelMap is not supported. This commit improve the diagnostics for the user in such scenarios by rejecting the use of ModelMap upfront in WebFlux. Consequently, for the same use case as above, the user now encounters an exception similar to the following. java.lang.IllegalStateException: Could not resolve parameter [0] in java.lang.String example.SampleController.index(org.springframework.ui.ModelMap): No suitable resolver Closes gh-33109 --- .../ModelAttributeMethodArgumentResolver.java | 13 +++++++---- .../ModelMethodArgumentResolver.java | 23 +++++++++++-------- .../ModelMethodArgumentResolverTests.java | 12 ++++++++-- 3 files changed, 32 insertions(+), 16 deletions(-) diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/ModelAttributeMethodArgumentResolver.java b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/ModelAttributeMethodArgumentResolver.java index 7f41c3b6b9..b45c3ba0da 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/ModelAttributeMethodArgumentResolver.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/ModelAttributeMethodArgumentResolver.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2023 the original author or authors. + * Copyright 2002-2024 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -31,6 +31,7 @@ import org.springframework.core.ReactiveAdapterRegistry; import org.springframework.core.ResolvableType; import org.springframework.lang.Nullable; import org.springframework.ui.Model; +import org.springframework.ui.ModelMap; import org.springframework.util.Assert; import org.springframework.util.ClassUtils; import org.springframework.validation.BindingResult; @@ -53,9 +54,10 @@ import org.springframework.web.server.ServerWebExchange; * {@code @jakarta.validation.Valid} or Spring's own * {@code @org.springframework.validation.annotation.Validated}. * - *
When this handler is created with {@code useDefaultResolution=true} - * any non-simple type argument and return value is regarded as a model - * attribute with or without the presence of an {@code @ModelAttribute}. + *
When this handler is created with {@code useDefaultResolution=true} any + * non-simple type argument or return value (excluding those of type + * {@link ModelMap}) is regarded as a model attribute with or without the + * presence of {@code @ModelAttribute}. * * @author Rossen Stoyanchev * @author Juergen Hoeller @@ -88,7 +90,8 @@ public class ModelAttributeMethodArgumentResolver extends HandlerMethodArgumentR return true; } else if (this.useDefaultResolution) { - return checkParameterType(parameter, type -> !BeanUtils.isSimpleProperty(type)); + return checkParameterType(parameter, type -> + !(ModelMap.class.isAssignableFrom(type) || BeanUtils.isSimpleProperty(type))); } return false; } diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/ModelMethodArgumentResolver.java b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/ModelMethodArgumentResolver.java index d155f1bc61..3a5304481e 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/ModelMethodArgumentResolver.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/ModelMethodArgumentResolver.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2019 the original author or authors. + * Copyright 2002-2024 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -21,6 +21,7 @@ import java.util.Map; import org.springframework.core.MethodParameter; import org.springframework.core.ReactiveAdapterRegistry; import org.springframework.ui.Model; +import org.springframework.ui.ModelMap; import org.springframework.web.reactive.BindingContext; import org.springframework.web.reactive.result.method.HandlerMethodArgumentResolverSupport; import org.springframework.web.reactive.result.method.SyncHandlerMethodArgumentResolver; @@ -30,12 +31,16 @@ import org.springframework.web.server.ServerWebExchange; * Resolver for a controller method argument of type {@link Model} that can * also be resolved as a {@link java.util.Map}. * - *
A Map return value can be interpreted in more than one way depending + *
A {@code Map} return value can be interpreted in more than one way depending * on the presence of annotations like {@code @ModelAttribute} or - * {@code @ResponseBody}. As of 5.2 this resolver returns false if a - * parameter of type {@code Map} is also annotated. + * {@code @ResponseBody}. + * + *
As of 5.2 this resolver returns {@code false} if a parameter of type + * {@code Map} is also annotated. As of 6.2 this resolver returns {@code false} + * for a parameter of type {@link ModelMap}. * * @author Rossen Stoyanchev + * @author Sam Brannen * @since 5.2 */ public class ModelMethodArgumentResolver extends HandlerMethodArgumentResolverSupport @@ -48,9 +53,9 @@ public class ModelMethodArgumentResolver extends HandlerMethodArgumentResolverSu @Override public boolean supportsParameter(MethodParameter param) { - return checkParameterTypeNoReactiveWrapper(param, type -> - Model.class.isAssignableFrom(type) || - (Map.class.isAssignableFrom(type) && param.getParameterAnnotations().length == 0)); + return checkParameterTypeNoReactiveWrapper(param, type -> Model.class.isAssignableFrom(type) || + (Map.class.isAssignableFrom(type) && !ModelMap.class.isAssignableFrom(type) && + !param.hasParameterAnnotations())); } @Override @@ -61,12 +66,12 @@ public class ModelMethodArgumentResolver extends HandlerMethodArgumentResolverSu if (Model.class.isAssignableFrom(type)) { return context.getModel(); } - else if (Map.class.isAssignableFrom(type)) { + else if (Map.class.isAssignableFrom(type) && !ModelMap.class.isAssignableFrom(type)) { return context.getModel().asMap(); } else { // Should never happen.. - throw new IllegalStateException("Unexpected method parameter type: " + type); + throw new IllegalStateException("Unexpected method parameter type: " + type.getName()); } } diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/ModelMethodArgumentResolverTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/ModelMethodArgumentResolverTests.java index 1183a1eb5b..92bd5e0577 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/ModelMethodArgumentResolverTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/ModelMethodArgumentResolverTests.java @@ -32,12 +32,14 @@ import org.springframework.web.testfixture.method.ResolvableMethod; import org.springframework.web.testfixture.server.MockServerWebExchange; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatIllegalStateException; import static org.springframework.web.testfixture.http.server.reactive.MockServerHttpRequest.get; /** * Tests for {@link ModelMethodArgumentResolver}. * * @author Rossen Stoyanchev + * @author Sam Brannen */ class ModelMethodArgumentResolverTests { @@ -52,11 +54,11 @@ class ModelMethodArgumentResolverTests { @Test void supportsParameter() { assertThat(this.resolver.supportsParameter(this.resolvable.arg(Model.class))).isTrue(); - assertThat(this.resolver.supportsParameter(this.resolvable.arg(ModelMap.class))).isTrue(); assertThat(this.resolver.supportsParameter( this.resolvable.annotNotPresent().arg(Map.class, String.class, Object.class))).isTrue(); assertThat(this.resolver.supportsParameter(this.resolvable.arg(Object.class))).isFalse(); + assertThat(this.resolver.supportsParameter(this.resolvable.arg(ModelMap.class))).isFalse(); assertThat(this.resolver.supportsParameter( this.resolvable.annotPresent(RequestBody.class).arg(Map.class, String.class, Object.class))).isFalse(); } @@ -65,7 +67,13 @@ class ModelMethodArgumentResolverTests { void resolveArgument() { testResolveArgument(this.resolvable.arg(Model.class)); testResolveArgument(this.resolvable.annotNotPresent().arg(Map.class, String.class, Object.class)); - testResolveArgument(this.resolvable.arg(ModelMap.class)); + + assertThatIllegalStateException() + .isThrownBy(() -> testResolveArgument(this.resolvable.arg(Object.class))) + .withMessage("Unexpected method parameter type: " + Object.class.getName()); + assertThatIllegalStateException() + .isThrownBy(() -> testResolveArgument(this.resolvable.arg(ModelMap.class))) + .withMessage("Unexpected method parameter type: " + ModelMap.class.getName()); } private void testResolveArgument(MethodParameter parameter) {