Ensure inherited @⁠HttpExchange annotation can be overridden in contr…

…oller 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
spring-projects · Jan 19, 2024 · 4cc91a2 · 4cc91a2
1 parent 17cef18
commit 4cc91a2
Show file tree

Hide file tree

Showing 4 changed files with 157 additions and 32 deletions.
diff --git a/...g/springframework/web/reactive/result/method/annotation/RequestMappingHandlerMapping.java b/...g/springframework/web/reactive/result/method/annotation/RequestMappingHandlerMapping.java
@@ -191,29 +191,28 @@ private RequestMappingInfo createRequestMappingInfo(AnnotatedElement element) {
 		RequestCondition<?> customCondition = (element instanceof Class<?> clazz ?
 				getCustomTypeCondition(clazz) : getCustomMethodCondition((Method) element));
 
-		MergedAnnotations mergedAnnotations = MergedAnnotations.from(element, SearchStrategy.TYPE_HIERARCHY,
-				RepeatableContainers.none());
+		List<AnnotationDescriptor> descriptors = getAnnotationDescriptors(element);
 
-		List<AnnotationDescriptor<RequestMapping>> requestMappings = getAnnotationDescriptors(
-				mergedAnnotations, RequestMapping.class);
+		List<AnnotationDescriptor> 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<AnnotationDescriptor<HttpExchange>> httpExchanges = getAnnotationDescriptors(
-				mergedAnnotations, HttpExchange.class);
+		List<AnnotationDescriptor> 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"
 							.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));
-			requestMappingInfo = createRequestMappingInfo(httpExchanges.get(0).annotation, customCondition);
+			requestMappingInfo = createRequestMappingInfo((HttpExchange) httpExchanges.get(0).annotation, customCondition);
 		}
 
 		return requestMappingInfo;
@@ -438,29 +437,29 @@ private String resolveCorsAnnotationValue(String value) {
 		}
 	}
 
-	private static <A extends Annotation> List<AnnotationDescriptor<A>> getAnnotationDescriptors(
-			MergedAnnotations mergedAnnotations, Class<A> annotationType) {
-
-		return mergedAnnotations.stream(annotationType)
+	private static List<AnnotationDescriptor> 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<A extends Annotation> {
+	private static class AnnotationDescriptor {
 
-		private final A annotation;
+		private final Annotation annotation;
 		private final MergedAnnotation<?> root;
 
-		AnnotationDescriptor(MergedAnnotation<A> mergedAnnotation) {
+		AnnotationDescriptor(MergedAnnotation<Annotation> 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/...ingframework/web/reactive/result/method/annotation/RequestMappingHandlerMappingTests.java b/...ingframework/web/reactive/result/method/annotation/RequestMappingHandlerMappingTests.java
@@ -222,6 +222,36 @@ void httpExchangeWithMixedAnnotationsAtMethodLevel() throws NoSuchMethodExceptio
 				);
 	}
 
+	@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 @@ static class MixedMethodLevelAnnotationsController {
 		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/...a/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMapping.java b/...a/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMapping.java
@@ -351,29 +351,28 @@ private RequestMappingInfo createRequestMappingInfo(AnnotatedElement element) {
 		RequestCondition<?> customCondition = (element instanceof Class<?> clazz ?
 				getCustomTypeCondition(clazz) : getCustomMethodCondition((Method) element));
 
-		MergedAnnotations mergedAnnotations = MergedAnnotations.from(element, SearchStrategy.TYPE_HIERARCHY,
-				RepeatableContainers.none());
+		List<AnnotationDescriptor> descriptors = getAnnotationDescriptors(element);
 
-		List<AnnotationDescriptor<RequestMapping>> requestMappings = getAnnotationDescriptors(
-				mergedAnnotations, RequestMapping.class);
+		List<AnnotationDescriptor> 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<AnnotationDescriptor<HttpExchange>> httpExchanges = getAnnotationDescriptors(
-				mergedAnnotations, HttpExchange.class);
+		List<AnnotationDescriptor> 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"
 							.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));
-			requestMappingInfo = createRequestMappingInfo(httpExchanges.get(0).annotation, customCondition);
+			requestMappingInfo = createRequestMappingInfo((HttpExchange) httpExchanges.get(0).annotation, customCondition);
 		}
 
 		return requestMappingInfo;
@@ -617,29 +616,29 @@ private String resolveCorsAnnotationValue(String value) {
 		}
 	}
 
-	private static <A extends Annotation> List<AnnotationDescriptor<A>> getAnnotationDescriptors(
-			MergedAnnotations mergedAnnotations, Class<A> annotationType) {
-
-		return mergedAnnotations.stream(annotationType)
+	private static List<AnnotationDescriptor> 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<A extends Annotation> {
+	private static class AnnotationDescriptor {
 
-		private final A annotation;
+		private final Annotation annotation;
 		private final MergedAnnotation<?> root;
 
-		AnnotationDescriptor(MergedAnnotation<A> mergedAnnotation) {
+		AnnotationDescriptor(MergedAnnotation<Annotation> 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/.../springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMappingTests.java b/.../springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMappingTests.java
@@ -344,6 +344,48 @@ void httpExchangeWithMixedAnnotationsAtMethodLevel() throws NoSuchMethodExceptio
 				);
 	}
 
+	@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 @@ 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)
 	@Retention(RetentionPolicy.RUNTIME)