From 4cc91a2869566eb3c8e2e87ac0890341cee1d1bf Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Fri, 19 Jan 2024 18:54:31 +0100 Subject: [PATCH] =?UTF-8?q?Ensure=20inherited=20@=E2=81=A0HttpExchange=20a?= =?UTF-8?q?nnotation=20can=20be=20overridden=20in=20controller?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit revises the RequestMappingHandlerMapping implementations in Spring MVC and Spring WebFlux to ensure that a @⁠Controller class which implements an interface annotated with @⁠HttpExchange annotations can inherit the @⁠HttpExchange declarations from the interface or optionally override them locally with @⁠HttpExchange or @⁠RequestMapping annotations. Closes gh-32065 --- .../RequestMappingHandlerMapping.java | 31 ++++---- .../RequestMappingHandlerMappingTests.java | 57 +++++++++++++++ .../RequestMappingHandlerMapping.java | 31 ++++---- .../RequestMappingHandlerMappingTests.java | 70 +++++++++++++++++++ 4 files changed, 157 insertions(+), 32 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 dac7216e0fe..0196a47b749 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 @@ -191,21 +191,20 @@ public class RequestMappingHandlerMapping extends RequestMappingInfoHandlerMappi RequestCondition customCondition = (element instanceof Class clazz ? getCustomTypeCondition(clazz) : getCustomMethodCondition((Method) element)); - MergedAnnotations mergedAnnotations = MergedAnnotations.from(element, SearchStrategy.TYPE_HIERARCHY, - RepeatableContainers.none()); + List descriptors = getAnnotationDescriptors(element); - List> requestMappings = getAnnotationDescriptors( - mergedAnnotations, RequestMapping.class); + List requestMappings = descriptors.stream() + .filter(desc -> desc.annotation instanceof RequestMapping).toList(); if (!requestMappings.isEmpty()) { if (requestMappings.size() > 1 && logger.isWarnEnabled()) { logger.warn("Multiple @RequestMapping annotations found on %s, but only the first will be used: %s" .formatted(element, requestMappings)); } - requestMappingInfo = createRequestMappingInfo(requestMappings.get(0).annotation, customCondition); + requestMappingInfo = createRequestMappingInfo((RequestMapping) requestMappings.get(0).annotation, customCondition); } - List> httpExchanges = getAnnotationDescriptors( - mergedAnnotations, HttpExchange.class); + List httpExchanges = descriptors.stream() + .filter(desc -> desc.annotation instanceof HttpExchange).toList(); if (!httpExchanges.isEmpty()) { Assert.state(requestMappingInfo == null, () -> "%s is annotated with @RequestMapping and @HttpExchange annotations, but only one is allowed: %s" @@ -213,7 +212,7 @@ public class RequestMappingHandlerMapping extends RequestMappingInfoHandlerMappi Assert.state(httpExchanges.size() == 1, () -> "Multiple @HttpExchange annotations found on %s, but only one is allowed: %s" .formatted(element, httpExchanges)); - requestMappingInfo = createRequestMappingInfo(httpExchanges.get(0).annotation, customCondition); + requestMappingInfo = createRequestMappingInfo((HttpExchange) httpExchanges.get(0).annotation, customCondition); } return requestMappingInfo; @@ -438,29 +437,29 @@ public class RequestMappingHandlerMapping extends RequestMappingInfoHandlerMappi } } - private static List> getAnnotationDescriptors( - MergedAnnotations mergedAnnotations, Class annotationType) { - - return mergedAnnotations.stream(annotationType) + private static List getAnnotationDescriptors(AnnotatedElement element) { + return MergedAnnotations.from(element, SearchStrategy.TYPE_HIERARCHY, RepeatableContainers.none()) + .stream() + .filter(MergedAnnotationPredicates.typeIn(RequestMapping.class, HttpExchange.class)) .filter(MergedAnnotationPredicates.firstRunOf(MergedAnnotation::getAggregateIndex)) .map(AnnotationDescriptor::new) .distinct() .toList(); } - private static class AnnotationDescriptor { + private static class AnnotationDescriptor { - private final A annotation; + private final Annotation annotation; private final MergedAnnotation root; - AnnotationDescriptor(MergedAnnotation mergedAnnotation) { + AnnotationDescriptor(MergedAnnotation mergedAnnotation) { this.annotation = mergedAnnotation.synthesize(); this.root = mergedAnnotation.getRoot(); } @Override public boolean equals(Object obj) { - return (obj instanceof AnnotationDescriptor that && this.annotation.equals(that.annotation)); + return (obj instanceof AnnotationDescriptor that && this.annotation.equals(that.annotation)); } @Override 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 4c91e819842..4662a529265 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 @@ -222,6 +222,36 @@ class RequestMappingHandlerMappingTests { ); } + @Test // gh-32065 + void httpExchangeAnnotationsOverriddenAtClassLevel() throws NoSuchMethodException { + this.handlerMapping.afterPropertiesSet(); + + Class controllerClass = ClassLevelOverriddenHttpExchangeAnnotationsController.class; + Method method = controllerClass.getDeclaredMethod("post"); + + RequestMappingInfo info = this.handlerMapping.getMappingForMethod(method, controllerClass); + + assertThat(info).isNotNull(); + assertThat(info.getPatternsCondition()).isNotNull(); + assertThat(info.getPatternsCondition().getPatterns()).extracting(PathPattern::getPatternString) + .containsOnly("/controller/postExchange"); + } + + @Test // gh-32065 + void httpExchangeAnnotationsOverriddenAtMethodLevel() throws NoSuchMethodException { + this.handlerMapping.afterPropertiesSet(); + + Class controllerClass = MethodLevelOverriddenHttpExchangeAnnotationsController.class; + Method method = controllerClass.getDeclaredMethod("post"); + + RequestMappingInfo info = this.handlerMapping.getMappingForMethod(method, controllerClass); + + assertThat(info).isNotNull(); + assertThat(info.getPatternsCondition()).isNotNull(); + assertThat(info.getPatternsCondition().getPatterns()).extracting(PathPattern::getPatternString) + .containsOnly("/controller/postMapping"); + } + @SuppressWarnings("DataFlowIssue") @Test void httpExchangeWithDefaultValues() throws NoSuchMethodException { @@ -417,6 +447,33 @@ class RequestMappingHandlerMappingTests { void post() {} } + @HttpExchange("/service") + interface Service { + + @PostExchange("/postExchange") + void post(); + + } + + + @Controller + @RequestMapping("/controller") + static class ClassLevelOverriddenHttpExchangeAnnotationsController implements Service { + + @Override + public void post() {} + } + + + @Controller + @RequestMapping("/controller") + static class MethodLevelOverriddenHttpExchangeAnnotationsController implements Service { + + @PostMapping("/postMapping") + @Override + public void post() {} + } + @HttpExchange @Target(ElementType.TYPE) 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 d3340bc78c9..02bfe40ee0b 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 @@ -351,21 +351,20 @@ public class RequestMappingHandlerMapping extends RequestMappingInfoHandlerMappi RequestCondition customCondition = (element instanceof Class clazz ? getCustomTypeCondition(clazz) : getCustomMethodCondition((Method) element)); - MergedAnnotations mergedAnnotations = MergedAnnotations.from(element, SearchStrategy.TYPE_HIERARCHY, - RepeatableContainers.none()); + List descriptors = getAnnotationDescriptors(element); - List> requestMappings = getAnnotationDescriptors( - mergedAnnotations, RequestMapping.class); + List requestMappings = descriptors.stream() + .filter(desc -> desc.annotation instanceof RequestMapping).toList(); if (!requestMappings.isEmpty()) { if (requestMappings.size() > 1 && logger.isWarnEnabled()) { logger.warn("Multiple @RequestMapping annotations found on %s, but only the first will be used: %s" .formatted(element, requestMappings)); } - requestMappingInfo = createRequestMappingInfo(requestMappings.get(0).annotation, customCondition); + requestMappingInfo = createRequestMappingInfo((RequestMapping) requestMappings.get(0).annotation, customCondition); } - List> httpExchanges = getAnnotationDescriptors( - mergedAnnotations, HttpExchange.class); + List httpExchanges = descriptors.stream() + .filter(desc -> desc.annotation instanceof HttpExchange).toList(); if (!httpExchanges.isEmpty()) { Assert.state(requestMappingInfo == null, () -> "%s is annotated with @RequestMapping and @HttpExchange annotations, but only one is allowed: %s" @@ -373,7 +372,7 @@ public class RequestMappingHandlerMapping extends RequestMappingInfoHandlerMappi Assert.state(httpExchanges.size() == 1, () -> "Multiple @HttpExchange annotations found on %s, but only one is allowed: %s" .formatted(element, httpExchanges)); - requestMappingInfo = createRequestMappingInfo(httpExchanges.get(0).annotation, customCondition); + requestMappingInfo = createRequestMappingInfo((HttpExchange) httpExchanges.get(0).annotation, customCondition); } return requestMappingInfo; @@ -617,29 +616,29 @@ public class RequestMappingHandlerMapping extends RequestMappingInfoHandlerMappi } } - private static List> getAnnotationDescriptors( - MergedAnnotations mergedAnnotations, Class annotationType) { - - return mergedAnnotations.stream(annotationType) + private static List getAnnotationDescriptors(AnnotatedElement element) { + return MergedAnnotations.from(element, SearchStrategy.TYPE_HIERARCHY, RepeatableContainers.none()) + .stream() + .filter(MergedAnnotationPredicates.typeIn(RequestMapping.class, HttpExchange.class)) .filter(MergedAnnotationPredicates.firstRunOf(MergedAnnotation::getAggregateIndex)) .map(AnnotationDescriptor::new) .distinct() .toList(); } - private static class AnnotationDescriptor { + private static class AnnotationDescriptor { - private final A annotation; + private final Annotation annotation; private final MergedAnnotation root; - AnnotationDescriptor(MergedAnnotation mergedAnnotation) { + AnnotationDescriptor(MergedAnnotation mergedAnnotation) { this.annotation = mergedAnnotation.synthesize(); this.root = mergedAnnotation.getRoot(); } @Override public boolean equals(Object obj) { - return (obj instanceof AnnotationDescriptor that && this.annotation.equals(that.annotation)); + return (obj instanceof AnnotationDescriptor that && this.annotation.equals(that.annotation)); } @Override 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 a0c6f7c1e6c..b52fc844f46 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 @@ -344,6 +344,48 @@ class RequestMappingHandlerMappingTests { ); } + @Test // gh-32065 + void httpExchangeAnnotationsOverriddenAtClassLevel() throws NoSuchMethodException { + RequestMappingHandlerMapping mapping = createMapping(); + + Class controllerClass = ClassLevelOverriddenHttpExchangeAnnotationsController.class; + Method method = controllerClass.getDeclaredMethod("post"); + + RequestMappingInfo info = mapping.getMappingForMethod(method, controllerClass); + + assertThat(info).isNotNull(); + assertThat(info.getActivePatternsCondition()).isNotNull(); + + MockHttpServletRequest request = new MockHttpServletRequest("POST", "/service/postExchange"); + initRequestPath(mapping, request); + assertThat(info.getActivePatternsCondition().getMatchingCondition(request)).isNull(); + + request = new MockHttpServletRequest("POST", "/controller/postExchange"); + initRequestPath(mapping, request); + assertThat(info.getActivePatternsCondition().getMatchingCondition(request)).isNotNull(); + } + + @Test // gh-32065 + void httpExchangeAnnotationsOverriddenAtMethodLevel() throws NoSuchMethodException { + RequestMappingHandlerMapping mapping = createMapping(); + + Class controllerClass = MethodLevelOverriddenHttpExchangeAnnotationsController.class; + Method method = controllerClass.getDeclaredMethod("post"); + + RequestMappingInfo info = mapping.getMappingForMethod(method, controllerClass); + + assertThat(info).isNotNull(); + assertThat(info.getActivePatternsCondition()).isNotNull(); + + MockHttpServletRequest request = new MockHttpServletRequest("POST", "/service/postExchange"); + initRequestPath(mapping, request); + assertThat(info.getActivePatternsCondition().getMatchingCondition(request)).isNull(); + + request = new MockHttpServletRequest("POST", "/controller/postMapping"); + initRequestPath(mapping, request); + assertThat(info.getActivePatternsCondition().getMatchingCondition(request)).isNotNull(); + } + @SuppressWarnings("DataFlowIssue") @Test void httpExchangeWithDefaultValues() throws NoSuchMethodException { @@ -542,6 +584,34 @@ class RequestMappingHandlerMappingTests { } + @HttpExchange("/service") + interface Service { + + @PostExchange("/postExchange") + void post(); + + } + + + @Controller + @RequestMapping("/controller") + static class ClassLevelOverriddenHttpExchangeAnnotationsController implements Service { + + @Override + public void post() {} + } + + + @Controller + @RequestMapping("/controller") + static class MethodLevelOverriddenHttpExchangeAnnotationsController implements Service { + + @PostMapping("/postMapping") + @Override + public void post() {} + } + + @HttpExchange @Target(ElementType.TYPE) @Retention(RetentionPolicy.RUNTIME)