Reject mixed @⁠RequestMapping and @⁠HttpExchange declarations in MVC & WebFlux

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
This commit is contained in:
Sam Brannen
2024-01-19 16:23:37 +01:00
parent c820e44a99
commit 6697461003
4 changed files with 124 additions and 6 deletions

View File

@@ -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<AnnotationDescriptor<HttpExchange>> 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;
}
/**

View File

@@ -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)

View File

@@ -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<AnnotationDescriptor<HttpExchange>> 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;
}
/**

View File

@@ -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)