Reject multiple @⁠HttpExchange declarations in MVC and WebFlux

This commit updates the RequestMappingHandlerMapping implementations in Spring MVC and Spring WebFlux so that multiple @⁠HttpExchange declarations on the same element are rejected. Closes gh-32049
spring-projects · Jan 18, 2024 · b8b31ff · b8b31ff
1 parent c5c77b9
commit b8b31ff
Show file tree

Hide file tree

Showing 4 changed files with 129 additions and 6 deletions.
diff --git a/...g/springframework/web/reactive/result/method/annotation/RequestMappingHandlerMapping.java b/...g/springframework/web/reactive/result/method/annotation/RequestMappingHandlerMapping.java
@@ -200,9 +200,13 @@ private RequestMappingInfo createRequestMappingInfo(AnnotatedElement element) {
 			return createRequestMappingInfo(requestMappings.get(0).annotation, customCondition);
 		}
 
-		HttpExchange httpExchange = AnnotatedElementUtils.findMergedAnnotation(element, HttpExchange.class);
-		if (httpExchange != null) {
-			return createRequestMappingInfo(httpExchange, customCondition);
+		List<AnnotationDescriptor<HttpExchange>> httpExchanges = getAnnotationDescriptors(
+				mergedAnnotations, HttpExchange.class);
+		if (!httpExchanges.isEmpty()) {
+			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);
 		}
 
 		return null;

diff --git a/...ingframework/web/reactive/result/method/annotation/RequestMappingHandlerMappingTests.java b/...ingframework/web/reactive/result/method/annotation/RequestMappingHandlerMappingTests.java
@@ -48,10 +48,12 @@
 import org.springframework.web.reactive.result.method.RequestMappingInfo;
 import org.springframework.web.service.annotation.HttpExchange;
 import org.springframework.web.service.annotation.PostExchange;
+import org.springframework.web.service.annotation.PutExchange;
 import org.springframework.web.util.pattern.PathPattern;
 import org.springframework.web.util.pattern.PathPatternParser;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatIllegalStateException;
 import static org.mockito.Mockito.mock;
 
 /**
@@ -154,6 +156,38 @@ void patchMapping() {
 		assertComposedAnnotationMapping(RequestMethod.PATCH);
 	}
 
+	@Test  // gh-32049
+	void httpExchangeWithMultipleAnnotationsAtClassLevel() throws NoSuchMethodException {
+		this.handlerMapping.afterPropertiesSet();
+
+		Class<?> controllerClass = MultipleClassLevelAnnotationsHttpExchangeController.class;
+		Method method = controllerClass.getDeclaredMethod("post");
+
+		assertThatIllegalStateException()
+				.isThrownBy(() -> this.handlerMapping.getMappingForMethod(method, controllerClass))
+				.withMessageContainingAll(
+					"Multiple @HttpExchange annotations found on " + controllerClass,
+					"@" + HttpExchange.class.getName(),
+					"@" + ExtraHttpExchange.class.getName()
+				);
+	}
+
+	@Test  // gh-32049
+	void httpExchangeWithMultipleAnnotationsAtMethodLevel() throws NoSuchMethodException {
+		this.handlerMapping.afterPropertiesSet();
+
+		Class<?> controllerClass = MultipleMethodLevelAnnotationsHttpExchangeController.class;
+		Method method = controllerClass.getDeclaredMethod("post");
+
+		assertThatIllegalStateException()
+				.isThrownBy(() -> this.handlerMapping.getMappingForMethod(method, controllerClass))
+				.withMessageContainingAll(
+					"Multiple @HttpExchange annotations found on " + method,
+					"@" + PostExchange.class.getName(),
+					"@" + PutExchange.class.getName()
+				);
+	}
+
 	@SuppressWarnings("DataFlowIssue")
 	@Test
 	void httpExchangeWithDefaultValues() throws NoSuchMethodException {
@@ -313,4 +347,27 @@ public void defaultValuesExchange() {}
 		public void customValuesExchange(){}
 	}
 
+	@HttpExchange("/exchange")
+	@ExtraHttpExchange
+	static class MultipleClassLevelAnnotationsHttpExchangeController {
+
+		@PostExchange("/post")
+		void post() {}
+	}
+
+
+	static class MultipleMethodLevelAnnotationsHttpExchangeController {
+
+		@PostExchange("/post")
+		@PutExchange("/post")
+		void post() {}
+	}
+
+
+	@HttpExchange
+	@Target(ElementType.TYPE)
+	@Retention(RetentionPolicy.RUNTIME)
+	@interface ExtraHttpExchange {
+	}
+
 }
diff --git a/...a/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMapping.java b/...a/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMapping.java
@@ -360,9 +360,13 @@ private RequestMappingInfo createRequestMappingInfo(AnnotatedElement element) {
 			return createRequestMappingInfo(requestMappings.get(0).annotation, customCondition);
 		}
 
-		HttpExchange httpExchange = AnnotatedElementUtils.findMergedAnnotation(element, HttpExchange.class);
-		if (httpExchange != null) {
-			return createRequestMappingInfo(httpExchange, customCondition);
+		List<AnnotationDescriptor<HttpExchange>> httpExchanges = getAnnotationDescriptors(
+				mergedAnnotations, HttpExchange.class);
+		if (!httpExchanges.isEmpty()) {
+			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);
 		}
 
 		return null;

diff --git a/.../springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMappingTests.java b/.../springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMappingTests.java
@@ -48,6 +48,7 @@
 import org.springframework.web.method.HandlerTypePredicate;
 import org.springframework.web.service.annotation.HttpExchange;
 import org.springframework.web.service.annotation.PostExchange;
+import org.springframework.web.service.annotation.PutExchange;
 import org.springframework.web.servlet.handler.PathPatternsParameterizedTest;
 import org.springframework.web.servlet.mvc.condition.ConsumesRequestCondition;
 import org.springframework.web.servlet.mvc.condition.MediaTypeExpression;
@@ -58,6 +59,7 @@
 import org.springframework.web.util.pattern.PathPatternParser;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatIllegalStateException;
 import static org.mockito.Mockito.mock;
 
 /**
@@ -276,6 +278,38 @@ void patchMapping() {
 		assertComposedAnnotationMapping(RequestMethod.PATCH);
 	}
 
+	@Test  // gh-32049
+	void httpExchangeWithMultipleAnnotationsAtClassLevel() throws NoSuchMethodException {
+		RequestMappingHandlerMapping mapping = createMapping();
+
+		Class<?> controllerClass = MultipleClassLevelAnnotationsHttpExchangeController.class;
+		Method method = controllerClass.getDeclaredMethod("post");
+
+		assertThatIllegalStateException()
+				.isThrownBy(() -> mapping.getMappingForMethod(method, controllerClass))
+				.withMessageContainingAll(
+					"Multiple @HttpExchange annotations found on " + controllerClass,
+					"@" + HttpExchange.class.getName(),
+					"@" + ExtraHttpExchange.class.getName()
+				);
+	}
+
+	@Test  // gh-32049
+	void httpExchangeWithMultipleAnnotationsAtMethodLevel() throws NoSuchMethodException {
+		RequestMappingHandlerMapping mapping = createMapping();
+
+		Class<?> controllerClass = MultipleMethodLevelAnnotationsHttpExchangeController.class;
+		Method method = controllerClass.getDeclaredMethod("post");
+
+		assertThatIllegalStateException()
+				.isThrownBy(() -> mapping.getMappingForMethod(method, controllerClass))
+				.withMessageContainingAll(
+					"Multiple @HttpExchange annotations found on " + method,
+					"@" + PostExchange.class.getName(),
+					"@" + PutExchange.class.getName()
+				);
+	}
+
 	@SuppressWarnings("DataFlowIssue")
 	@Test
 	void httpExchangeWithDefaultValues() throws NoSuchMethodException {
@@ -437,6 +471,30 @@ public void customValuesExchange(){}
 	}
 
 
+	@HttpExchange("/exchange")
+	@ExtraHttpExchange
+	static class MultipleClassLevelAnnotationsHttpExchangeController {
+
+		@PostExchange("/post")
+		void post() {}
+	}
+
+
+	static class MultipleMethodLevelAnnotationsHttpExchangeController {
+
+		@PostExchange("/post")
+		@PutExchange("/post")
+		void post() {}
+	}
+
+
+	@HttpExchange
+	@Target(ElementType.TYPE)
+	@Retention(RetentionPolicy.RUNTIME)
+	@interface ExtraHttpExchange {
+	}
+
+
 	private static class Foo {
 	}