Ensure inherited @⁠HttpExchange annotation can be overridden in controller

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
This commit is contained in:
Sam Brannen
2024-01-19 18:54:31 +01:00
parent 17cef18760
commit 4cc91a2869
4 changed files with 157 additions and 32 deletions

View File

@@ -191,21 +191,20 @@ public class RequestMappingHandlerMapping extends RequestMappingInfoHandlerMappi
RequestCondition<?> customCondition = (element instanceof Class<?> clazz ? RequestCondition<?> customCondition = (element instanceof Class<?> clazz ?
getCustomTypeCondition(clazz) : getCustomMethodCondition((Method) element)); getCustomTypeCondition(clazz) : getCustomMethodCondition((Method) element));
MergedAnnotations mergedAnnotations = MergedAnnotations.from(element, SearchStrategy.TYPE_HIERARCHY, List<AnnotationDescriptor> descriptors = getAnnotationDescriptors(element);
RepeatableContainers.none());
List<AnnotationDescriptor<RequestMapping>> requestMappings = getAnnotationDescriptors( List<AnnotationDescriptor> requestMappings = descriptors.stream()
mergedAnnotations, RequestMapping.class); .filter(desc -> desc.annotation instanceof RequestMapping).toList();
if (!requestMappings.isEmpty()) { if (!requestMappings.isEmpty()) {
if (requestMappings.size() > 1 && logger.isWarnEnabled()) { if (requestMappings.size() > 1 && logger.isWarnEnabled()) {
logger.warn("Multiple @RequestMapping annotations found on %s, but only the first will be used: %s" logger.warn("Multiple @RequestMapping annotations found on %s, but only the first will be used: %s"
.formatted(element, requestMappings)); .formatted(element, requestMappings));
} }
requestMappingInfo = createRequestMappingInfo(requestMappings.get(0).annotation, customCondition); requestMappingInfo = createRequestMappingInfo((RequestMapping) requestMappings.get(0).annotation, customCondition);
} }
List<AnnotationDescriptor<HttpExchange>> httpExchanges = getAnnotationDescriptors( List<AnnotationDescriptor> httpExchanges = descriptors.stream()
mergedAnnotations, HttpExchange.class); .filter(desc -> desc.annotation instanceof HttpExchange).toList();
if (!httpExchanges.isEmpty()) { if (!httpExchanges.isEmpty()) {
Assert.state(requestMappingInfo == null, Assert.state(requestMappingInfo == null,
() -> "%s is annotated with @RequestMapping and @HttpExchange annotations, but only one is allowed: %s" () -> "%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, Assert.state(httpExchanges.size() == 1,
() -> "Multiple @HttpExchange annotations found on %s, but only one is allowed: %s" () -> "Multiple @HttpExchange annotations found on %s, but only one is allowed: %s"
.formatted(element, httpExchanges)); .formatted(element, httpExchanges));
requestMappingInfo = createRequestMappingInfo(httpExchanges.get(0).annotation, customCondition); requestMappingInfo = createRequestMappingInfo((HttpExchange) httpExchanges.get(0).annotation, customCondition);
} }
return requestMappingInfo; return requestMappingInfo;
@@ -438,29 +437,29 @@ public class RequestMappingHandlerMapping extends RequestMappingInfoHandlerMappi
} }
} }
private static <A extends Annotation> List<AnnotationDescriptor<A>> getAnnotationDescriptors( private static List<AnnotationDescriptor> getAnnotationDescriptors(AnnotatedElement element) {
MergedAnnotations mergedAnnotations, Class<A> annotationType) { return MergedAnnotations.from(element, SearchStrategy.TYPE_HIERARCHY, RepeatableContainers.none())
.stream()
return mergedAnnotations.stream(annotationType) .filter(MergedAnnotationPredicates.typeIn(RequestMapping.class, HttpExchange.class))
.filter(MergedAnnotationPredicates.firstRunOf(MergedAnnotation::getAggregateIndex)) .filter(MergedAnnotationPredicates.firstRunOf(MergedAnnotation::getAggregateIndex))
.map(AnnotationDescriptor::new) .map(AnnotationDescriptor::new)
.distinct() .distinct()
.toList(); .toList();
} }
private static class AnnotationDescriptor<A extends Annotation> { private static class AnnotationDescriptor {
private final A annotation; private final Annotation annotation;
private final MergedAnnotation<?> root; private final MergedAnnotation<?> root;
AnnotationDescriptor(MergedAnnotation<A> mergedAnnotation) { AnnotationDescriptor(MergedAnnotation<Annotation> mergedAnnotation) {
this.annotation = mergedAnnotation.synthesize(); this.annotation = mergedAnnotation.synthesize();
this.root = mergedAnnotation.getRoot(); this.root = mergedAnnotation.getRoot();
} }
@Override @Override
public boolean equals(Object obj) { 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 @Override

View File

@@ -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") @SuppressWarnings("DataFlowIssue")
@Test @Test
void httpExchangeWithDefaultValues() throws NoSuchMethodException { void httpExchangeWithDefaultValues() throws NoSuchMethodException {
@@ -417,6 +447,33 @@ class RequestMappingHandlerMappingTests {
void post() {} 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 @HttpExchange
@Target(ElementType.TYPE) @Target(ElementType.TYPE)

View File

@@ -351,21 +351,20 @@ public class RequestMappingHandlerMapping extends RequestMappingInfoHandlerMappi
RequestCondition<?> customCondition = (element instanceof Class<?> clazz ? RequestCondition<?> customCondition = (element instanceof Class<?> clazz ?
getCustomTypeCondition(clazz) : getCustomMethodCondition((Method) element)); getCustomTypeCondition(clazz) : getCustomMethodCondition((Method) element));
MergedAnnotations mergedAnnotations = MergedAnnotations.from(element, SearchStrategy.TYPE_HIERARCHY, List<AnnotationDescriptor> descriptors = getAnnotationDescriptors(element);
RepeatableContainers.none());
List<AnnotationDescriptor<RequestMapping>> requestMappings = getAnnotationDescriptors( List<AnnotationDescriptor> requestMappings = descriptors.stream()
mergedAnnotations, RequestMapping.class); .filter(desc -> desc.annotation instanceof RequestMapping).toList();
if (!requestMappings.isEmpty()) { if (!requestMappings.isEmpty()) {
if (requestMappings.size() > 1 && logger.isWarnEnabled()) { if (requestMappings.size() > 1 && logger.isWarnEnabled()) {
logger.warn("Multiple @RequestMapping annotations found on %s, but only the first will be used: %s" logger.warn("Multiple @RequestMapping annotations found on %s, but only the first will be used: %s"
.formatted(element, requestMappings)); .formatted(element, requestMappings));
} }
requestMappingInfo = createRequestMappingInfo(requestMappings.get(0).annotation, customCondition); requestMappingInfo = createRequestMappingInfo((RequestMapping) requestMappings.get(0).annotation, customCondition);
} }
List<AnnotationDescriptor<HttpExchange>> httpExchanges = getAnnotationDescriptors( List<AnnotationDescriptor> httpExchanges = descriptors.stream()
mergedAnnotations, HttpExchange.class); .filter(desc -> desc.annotation instanceof HttpExchange).toList();
if (!httpExchanges.isEmpty()) { if (!httpExchanges.isEmpty()) {
Assert.state(requestMappingInfo == null, Assert.state(requestMappingInfo == null,
() -> "%s is annotated with @RequestMapping and @HttpExchange annotations, but only one is allowed: %s" () -> "%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, Assert.state(httpExchanges.size() == 1,
() -> "Multiple @HttpExchange annotations found on %s, but only one is allowed: %s" () -> "Multiple @HttpExchange annotations found on %s, but only one is allowed: %s"
.formatted(element, httpExchanges)); .formatted(element, httpExchanges));
requestMappingInfo = createRequestMappingInfo(httpExchanges.get(0).annotation, customCondition); requestMappingInfo = createRequestMappingInfo((HttpExchange) httpExchanges.get(0).annotation, customCondition);
} }
return requestMappingInfo; return requestMappingInfo;
@@ -617,29 +616,29 @@ public class RequestMappingHandlerMapping extends RequestMappingInfoHandlerMappi
} }
} }
private static <A extends Annotation> List<AnnotationDescriptor<A>> getAnnotationDescriptors( private static List<AnnotationDescriptor> getAnnotationDescriptors(AnnotatedElement element) {
MergedAnnotations mergedAnnotations, Class<A> annotationType) { return MergedAnnotations.from(element, SearchStrategy.TYPE_HIERARCHY, RepeatableContainers.none())
.stream()
return mergedAnnotations.stream(annotationType) .filter(MergedAnnotationPredicates.typeIn(RequestMapping.class, HttpExchange.class))
.filter(MergedAnnotationPredicates.firstRunOf(MergedAnnotation::getAggregateIndex)) .filter(MergedAnnotationPredicates.firstRunOf(MergedAnnotation::getAggregateIndex))
.map(AnnotationDescriptor::new) .map(AnnotationDescriptor::new)
.distinct() .distinct()
.toList(); .toList();
} }
private static class AnnotationDescriptor<A extends Annotation> { private static class AnnotationDescriptor {
private final A annotation; private final Annotation annotation;
private final MergedAnnotation<?> root; private final MergedAnnotation<?> root;
AnnotationDescriptor(MergedAnnotation<A> mergedAnnotation) { AnnotationDescriptor(MergedAnnotation<Annotation> mergedAnnotation) {
this.annotation = mergedAnnotation.synthesize(); this.annotation = mergedAnnotation.synthesize();
this.root = mergedAnnotation.getRoot(); this.root = mergedAnnotation.getRoot();
} }
@Override @Override
public boolean equals(Object obj) { 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 @Override

View File

@@ -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") @SuppressWarnings("DataFlowIssue")
@Test @Test
void httpExchangeWithDefaultValues() throws NoSuchMethodException { 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 @HttpExchange
@Target(ElementType.TYPE) @Target(ElementType.TYPE)
@Retention(RetentionPolicy.RUNTIME) @Retention(RetentionPolicy.RUNTIME)