From 66974610034724b9f36cded93a7b0a4cfa44d20f Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Fri, 19 Jan 2024 16:23:37 +0100 Subject: [PATCH] =?UTF-8?q?Reject=20mixed=20@=E2=81=A0RequestMapping=20and?= =?UTF-8?q?=20@=E2=81=A0HttpExchange=20declarations=20in=20MVC=20&=20WebFl?= =?UTF-8?q?ux?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit updates the RequestMappingHandlerMapping implementations in Spring MVC and Spring WebFlux so that mixed @⁠RequestMapping and @⁠HttpExchange declarations on the same element are rejected. Note, however, that a @⁠Controller class which implements an interface annotated with @⁠HttpExchange annotations can still inherit the @⁠HttpExchange declarations from the interface or optionally override them locally with @⁠HttpExchange or @⁠RequestMapping annotations. See gh-31962 See gh-32049 Closes gh-32065 --- .../RequestMappingHandlerMapping.java | 11 ++-- .../RequestMappingHandlerMappingTests.java | 54 +++++++++++++++++++ .../RequestMappingHandlerMapping.java | 11 ++-- .../RequestMappingHandlerMappingTests.java | 54 +++++++++++++++++++ 4 files changed, 124 insertions(+), 6 deletions(-) diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/RequestMappingHandlerMapping.java b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/RequestMappingHandlerMapping.java index ef7b17b38e..dac7216e0f 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/RequestMappingHandlerMapping.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/RequestMappingHandlerMapping.java @@ -25,6 +25,7 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.function.Predicate; +import java.util.stream.Stream; import org.springframework.context.EmbeddedValueResolverAware; import org.springframework.core.annotation.AnnotatedElementUtils; @@ -186,6 +187,7 @@ public class RequestMappingHandlerMapping extends RequestMappingInfoHandlerMappi @Nullable private RequestMappingInfo createRequestMappingInfo(AnnotatedElement element) { + RequestMappingInfo requestMappingInfo = null; RequestCondition customCondition = (element instanceof Class clazz ? getCustomTypeCondition(clazz) : getCustomMethodCondition((Method) element)); @@ -199,19 +201,22 @@ public class RequestMappingHandlerMapping extends RequestMappingInfoHandlerMappi logger.warn("Multiple @RequestMapping annotations found on %s, but only the first will be used: %s" .formatted(element, requestMappings)); } - return createRequestMappingInfo(requestMappings.get(0).annotation, customCondition); + requestMappingInfo = createRequestMappingInfo(requestMappings.get(0).annotation, customCondition); } List> httpExchanges = getAnnotationDescriptors( mergedAnnotations, HttpExchange.class); if (!httpExchanges.isEmpty()) { + Assert.state(requestMappingInfo == null, + () -> "%s is annotated with @RequestMapping and @HttpExchange annotations, but only one is allowed: %s" + .formatted(element, Stream.of(requestMappings, httpExchanges).flatMap(List::stream).toList())); Assert.state(httpExchanges.size() == 1, () -> "Multiple @HttpExchange annotations found on %s, but only one is allowed: %s" .formatted(element, httpExchanges)); - return createRequestMappingInfo(httpExchanges.get(0).annotation, customCondition); + requestMappingInfo = createRequestMappingInfo(httpExchanges.get(0).annotation, customCondition); } - return null; + return requestMappingInfo; } /** diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/RequestMappingHandlerMappingTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/RequestMappingHandlerMappingTests.java index aebe253414..4c91e81984 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/RequestMappingHandlerMappingTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/RequestMappingHandlerMappingTests.java @@ -188,6 +188,40 @@ class RequestMappingHandlerMappingTests { ); } + @Test // gh-32065 + void httpExchangeWithMixedAnnotationsAtClassLevel() throws NoSuchMethodException { + this.handlerMapping.afterPropertiesSet(); + + Class controllerClass = MixedClassLevelAnnotationsController.class; + Method method = controllerClass.getDeclaredMethod("post"); + + assertThatIllegalStateException() + .isThrownBy(() -> this.handlerMapping.getMappingForMethod(method, controllerClass)) + .withMessageContainingAll( + controllerClass.getName(), + "is annotated with @RequestMapping and @HttpExchange annotations, but only one is allowed:", + "@" + RequestMapping.class.getName(), + "@" + HttpExchange.class.getName() + ); + } + + @Test // gh-32065 + void httpExchangeWithMixedAnnotationsAtMethodLevel() throws NoSuchMethodException { + this.handlerMapping.afterPropertiesSet(); + + Class controllerClass = MixedMethodLevelAnnotationsController.class; + Method method = controllerClass.getDeclaredMethod("post"); + + assertThatIllegalStateException() + .isThrownBy(() -> this.handlerMapping.getMappingForMethod(method, controllerClass)) + .withMessageContainingAll( + method.toString(), + "is annotated with @RequestMapping and @HttpExchange annotations, but only one is allowed:", + "@" + PostMapping.class.getName(), + "@" + PostExchange.class.getName() + ); + } + @SuppressWarnings("DataFlowIssue") @Test void httpExchangeWithDefaultValues() throws NoSuchMethodException { @@ -364,6 +398,26 @@ class RequestMappingHandlerMappingTests { } + @Controller + @RequestMapping("/api") + @HttpExchange("/api") + static class MixedClassLevelAnnotationsController { + + @PostExchange("/post") + void post() {} + } + + + @Controller + @RequestMapping("/api") + static class MixedMethodLevelAnnotationsController { + + @PostMapping("/post") + @PostExchange("/post") + void post() {} + } + + @HttpExchange @Target(ElementType.TYPE) @Retention(RetentionPolicy.RUNTIME) diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMapping.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMapping.java index 4d390fec70..d3340bc78c 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMapping.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMapping.java @@ -25,6 +25,7 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.function.Predicate; +import java.util.stream.Stream; import jakarta.servlet.http.HttpServletRequest; @@ -346,6 +347,7 @@ public class RequestMappingHandlerMapping extends RequestMappingInfoHandlerMappi @Nullable private RequestMappingInfo createRequestMappingInfo(AnnotatedElement element) { + RequestMappingInfo requestMappingInfo = null; RequestCondition customCondition = (element instanceof Class clazz ? getCustomTypeCondition(clazz) : getCustomMethodCondition((Method) element)); @@ -359,19 +361,22 @@ public class RequestMappingHandlerMapping extends RequestMappingInfoHandlerMappi logger.warn("Multiple @RequestMapping annotations found on %s, but only the first will be used: %s" .formatted(element, requestMappings)); } - return createRequestMappingInfo(requestMappings.get(0).annotation, customCondition); + requestMappingInfo = createRequestMappingInfo(requestMappings.get(0).annotation, customCondition); } List> httpExchanges = getAnnotationDescriptors( mergedAnnotations, HttpExchange.class); if (!httpExchanges.isEmpty()) { + Assert.state(requestMappingInfo == null, + () -> "%s is annotated with @RequestMapping and @HttpExchange annotations, but only one is allowed: %s" + .formatted(element, Stream.of(requestMappings, httpExchanges).flatMap(List::stream).toList())); Assert.state(httpExchanges.size() == 1, () -> "Multiple @HttpExchange annotations found on %s, but only one is allowed: %s" .formatted(element, httpExchanges)); - return createRequestMappingInfo(httpExchanges.get(0).annotation, customCondition); + requestMappingInfo = createRequestMappingInfo(httpExchanges.get(0).annotation, customCondition); } - return null; + return requestMappingInfo; } /** diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMappingTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMappingTests.java index 51ba6a27a3..a0c6f7c1e6 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMappingTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMappingTests.java @@ -310,6 +310,40 @@ class RequestMappingHandlerMappingTests { ); } + @Test // gh-32065 + void httpExchangeWithMixedAnnotationsAtClassLevel() throws NoSuchMethodException { + RequestMappingHandlerMapping mapping = createMapping(); + + Class controllerClass = MixedClassLevelAnnotationsController.class; + Method method = controllerClass.getDeclaredMethod("post"); + + assertThatIllegalStateException() + .isThrownBy(() -> mapping.getMappingForMethod(method, controllerClass)) + .withMessageContainingAll( + controllerClass.getName(), + "is annotated with @RequestMapping and @HttpExchange annotations, but only one is allowed:", + "@" + RequestMapping.class.getName(), + "@" + HttpExchange.class.getName() + ); + } + + @Test // gh-32065 + void httpExchangeWithMixedAnnotationsAtMethodLevel() throws NoSuchMethodException { + RequestMappingHandlerMapping mapping = createMapping(); + + Class controllerClass = MixedMethodLevelAnnotationsController.class; + Method method = controllerClass.getDeclaredMethod("post"); + + assertThatIllegalStateException() + .isThrownBy(() -> mapping.getMappingForMethod(method, controllerClass)) + .withMessageContainingAll( + method.toString(), + "is annotated with @RequestMapping and @HttpExchange annotations, but only one is allowed:", + "@" + PostMapping.class.getName(), + "@" + PostExchange.class.getName() + ); + } + @SuppressWarnings("DataFlowIssue") @Test void httpExchangeWithDefaultValues() throws NoSuchMethodException { @@ -488,6 +522,26 @@ class RequestMappingHandlerMappingTests { } + @Controller + @RequestMapping("/api") + @HttpExchange("/api") + static class MixedClassLevelAnnotationsController { + + @PostExchange("/post") + void post() {} + } + + + @Controller + @RequestMapping("/api") + static class MixedMethodLevelAnnotationsController { + + @PostMapping("/post") + @PostExchange("/post") + void post() {} + } + + @HttpExchange @Target(ElementType.TYPE) @Retention(RetentionPolicy.RUNTIME)