From d045f44693e49a900aa810a39d2b257539b9a087 Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Thu, 26 Jun 2025 10:15:06 +0100 Subject: [PATCH] Polishing in RequestMappingHandlerMapping --- .../RequestMappingHandlerMapping.java | 107 ++++++++++-------- .../RequestMappingHandlerMapping.java | 101 ++++++++++------- 2 files changed, 117 insertions(+), 91 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 56f2f98c5a..4ce5262f76 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 @@ -178,27 +178,25 @@ public class RequestMappingHandlerMapping extends RequestMappingInfoHandlerMappi @Override public Mono getHandlerInternal(ServerWebExchange exchange) { if (this.apiVersionStrategy != null) { - Comparable requestVersion = exchange.getAttribute(API_VERSION_ATTRIBUTE); - if (requestVersion == null) { - requestVersion = getApiVersion(exchange, this.apiVersionStrategy); - if (requestVersion != null) { - exchange.getAttributes().put(API_VERSION_ATTRIBUTE, requestVersion); + Comparable version = exchange.getAttribute(API_VERSION_ATTRIBUTE); + if (version == null) { + version = getApiVersion(exchange, this.apiVersionStrategy); + if (version != null) { + exchange.getAttributes().put(API_VERSION_ATTRIBUTE, version); } } } return super.getHandlerInternal(exchange); } - private static @Nullable Comparable getApiVersion( - ServerWebExchange exchange, ApiVersionStrategy versionStrategy) { - - String value = versionStrategy.resolveVersion(exchange); + private static @Nullable Comparable getApiVersion(ServerWebExchange exchange, ApiVersionStrategy strategy) { + String value = strategy.resolveVersion(exchange); if (value == null) { - return versionStrategy.getDefaultVersion(); + return strategy.getDefaultVersion(); } try { - Comparable version = versionStrategy.parseVersion(value); - versionStrategy.validateVersion(version, exchange); + Comparable version = strategy.parseVersion(value); + strategy.validateVersion(version, exchange); return version; } catch (Exception ex) { @@ -242,42 +240,43 @@ public class RequestMappingHandlerMapping extends RequestMappingInfoHandlerMappi } private @Nullable RequestMappingInfo createRequestMappingInfo(AnnotatedElement element) { - RequestMappingInfo requestMappingInfo = null; + + List descriptors = + 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(); + + RequestMappingInfo info = null; RequestCondition customCondition = (element instanceof Class clazz ? getCustomTypeCondition(clazz) : getCustomMethodCondition((Method) element)); - List descriptors = getAnnotationDescriptors(element); + List mappingDescriptors = + descriptors.stream().filter(desc -> desc.annotation instanceof RequestMapping).toList(); - 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((RequestMapping) requestMappings.get(0).annotation, customCondition); + if (!mappingDescriptors.isEmpty()) { + checkMultipleAnnotations(element, mappingDescriptors); + info = createRequestMappingInfo((RequestMapping) mappingDescriptors.get(0).annotation, customCondition); } - 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" - .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((HttpExchange) httpExchanges.get(0).annotation, customCondition); + List exchangeDescriptors = + descriptors.stream().filter(desc -> desc.annotation instanceof HttpExchange).toList(); + + if (!exchangeDescriptors.isEmpty()) { + checkMultipleAnnotations(element, info, mappingDescriptors, exchangeDescriptors); + info = createRequestMappingInfo((HttpExchange) exchangeDescriptors.get(0).annotation, customCondition); } - if (requestMappingInfo != null && this.apiVersionStrategy instanceof DefaultApiVersionStrategy davs) { - String version = requestMappingInfo.getVersionCondition().getVersion(); + if (info != null && this.apiVersionStrategy instanceof DefaultApiVersionStrategy davs) { + String version = info.getVersionCondition().getVersion(); if (version != null) { davs.addMappedVersion(version); } } - return requestMappingInfo; + return info; } /** @@ -316,6 +315,28 @@ public class RequestMappingHandlerMapping extends RequestMappingInfoHandlerMappi return null; } + private void checkMultipleAnnotations( + AnnotatedElement element, List mappingDescriptors) { + + if (logger.isWarnEnabled() && mappingDescriptors.size() > 1) { + logger.warn("Multiple @RequestMapping annotations found on %s, but only the first will be used: %s" + .formatted(element, mappingDescriptors)); + } + } + + private static void checkMultipleAnnotations( + AnnotatedElement element, @Nullable RequestMappingInfo info, + List mappingDescriptors, List exchangeDescriptors) { + + Assert.state(info == null, + () -> "%s is annotated with @RequestMapping and @HttpExchange annotations, but only one is allowed: %s" + .formatted(element, Stream.of(mappingDescriptors, exchangeDescriptors).flatMap(List::stream).toList())); + + Assert.state(exchangeDescriptors.size() == 1, + () -> "Multiple @HttpExchange annotations found on %s, but only one is allowed: %s" + .formatted(element, exchangeDescriptors)); + } + /** * Create a {@link RequestMappingInfo} from the supplied * {@link RequestMapping @RequestMapping} annotation, meta-annotation, @@ -510,24 +531,16 @@ public class RequestMappingHandlerMapping extends RequestMappingInfoHandlerMappi } } - 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 final Annotation annotation; + private final MergedAnnotation root; - AnnotationDescriptor(MergedAnnotation mergedAnnotation) { - this.annotation = mergedAnnotation.synthesize(); - this.root = mergedAnnotation.getRoot(); + AnnotationDescriptor(MergedAnnotation annotation) { + this.annotation = annotation.synthesize(); + this.root = annotation.getRoot(); } @Override 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 d1eeab8f35..0c2e9d3c34 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 @@ -205,27 +205,25 @@ public class RequestMappingHandlerMapping extends RequestMappingInfoHandlerMappi @Override protected @Nullable HandlerMethod getHandlerInternal(HttpServletRequest request) throws Exception { if (this.apiVersionStrategy != null) { - Comparable requestVersion = (Comparable) request.getAttribute(API_VERSION_ATTRIBUTE); - if (requestVersion == null) { - requestVersion = getApiVersion(request, this.apiVersionStrategy); - if (requestVersion != null) { - request.setAttribute(API_VERSION_ATTRIBUTE, requestVersion); + Comparable version = (Comparable) request.getAttribute(API_VERSION_ATTRIBUTE); + if (version == null) { + version = getApiVersion(request, this.apiVersionStrategy); + if (version != null) { + request.setAttribute(API_VERSION_ATTRIBUTE, version); } } } return super.getHandlerInternal(request); } - private static @Nullable Comparable getApiVersion( - HttpServletRequest request, ApiVersionStrategy versionStrategy) { - - String value = versionStrategy.resolveVersion(request); + private static @Nullable Comparable getApiVersion(HttpServletRequest request, ApiVersionStrategy strategy) { + String value = strategy.resolveVersion(request); if (value == null) { - return versionStrategy.getDefaultVersion(); + return strategy.getDefaultVersion(); } try { - Comparable version = versionStrategy.parseVersion(value); - versionStrategy.validateVersion(version, request); + Comparable version = strategy.parseVersion(value); + strategy.validateVersion(version, request); return version; } catch (Exception ex) { @@ -275,42 +273,44 @@ public class RequestMappingHandlerMapping extends RequestMappingInfoHandlerMappi } private @Nullable RequestMappingInfo createRequestMappingInfo(AnnotatedElement element) { - RequestMappingInfo requestMappingInfo = null; + + List descriptors = + 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(); + + RequestMappingInfo info = null; RequestCondition customCondition = (element instanceof Class clazz ? getCustomTypeCondition(clazz) : getCustomMethodCondition((Method) element)); - List descriptors = getAnnotationDescriptors(element); + List mappingDescriptors = + descriptors.stream().filter(desc -> desc.annotation instanceof RequestMapping).toList(); - 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((RequestMapping) requestMappings.get(0).annotation, customCondition); + if (!mappingDescriptors.isEmpty()) { + checkMultipleAnnotations(element, mappingDescriptors); + info = createRequestMappingInfo((RequestMapping) mappingDescriptors.get(0).annotation, customCondition); } - 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" - .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((HttpExchange) httpExchanges.get(0).annotation, customCondition); + List exchangeDescriptors = + descriptors.stream().filter(desc -> desc.annotation instanceof HttpExchange).toList(); + + if (!exchangeDescriptors.isEmpty()) { + checkMultipleAnnotations(element, info, mappingDescriptors, exchangeDescriptors); + info = createRequestMappingInfo((HttpExchange) exchangeDescriptors.get(0).annotation, customCondition); } - if (requestMappingInfo != null && this.apiVersionStrategy instanceof DefaultApiVersionStrategy davs) { - String version = requestMappingInfo.getVersionCondition().getVersion(); + if (info != null && this.apiVersionStrategy instanceof DefaultApiVersionStrategy davs) { + String version = info.getVersionCondition().getVersion(); if (version != null) { davs.addMappedVersion(version); } } - return requestMappingInfo; + return info; } /** @@ -343,6 +343,28 @@ public class RequestMappingHandlerMapping extends RequestMappingInfoHandlerMappi return null; } + private void checkMultipleAnnotations( + AnnotatedElement element, List mappingDescriptors) { + + if (logger.isWarnEnabled() && mappingDescriptors.size() > 1) { + logger.warn("Multiple @RequestMapping annotations found on %s, but only the first will be used: %s" + .formatted(element, mappingDescriptors)); + } + } + + private static void checkMultipleAnnotations( + AnnotatedElement element, @Nullable RequestMappingInfo info, + List mappingDescriptors, List exchangeDescriptors) { + + Assert.state(info == null, + () -> "%s is annotated with @RequestMapping and @HttpExchange annotations, but only one is allowed: %s" + .formatted(element, Stream.of(mappingDescriptors, exchangeDescriptors).flatMap(List::stream).toList())); + + Assert.state(exchangeDescriptors.size() == 1, + () -> "Multiple @HttpExchange annotations found on %s, but only one is allowed: %s" + .formatted(element, exchangeDescriptors)); + } + /** * Create a {@link RequestMappingInfo} from the supplied * {@link RequestMapping @RequestMapping} annotation, meta-annotation, @@ -563,15 +585,6 @@ public class RequestMappingHandlerMapping extends RequestMappingInfoHandlerMappi } } - 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 {