From ffdf941219954ba6603d6d612d70597029221b6e Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Mon, 23 Jun 2025 17:59:01 +0100 Subject: [PATCH] Resolve API version in RequestMappingHandlerMapping API version resolution and parsing is already applied as long as an ApiVersionStrategy is configured and irrespective of whether a given RequestMapping has a version or not. RequestMappingHandlerMapping also needs to be aware of the API version in order to apply deprecated version handling. So it is better to resolve, parse, and validate the version in the beginning of handler mapping rather than in the first call to any VersionRequestCondition. Closes gh-35049 --- .../DefaultApiVersionStrategiesTests.java | 30 +++++--- .../condition/VersionRequestCondition.java | 69 ++---------------- .../RequestMappingHandlerMapping.java | 33 +++++++++ .../DefaultApiVersionStrategiesTests.java | 12 +++- .../VersionRequestConditionTests.java | 27 ++----- .../condition/VersionRequestCondition.java | 70 ++----------------- .../RequestMappingHandlerMapping.java | 33 +++++++++ .../VersionRequestConditionTests.java | 23 +----- 8 files changed, 115 insertions(+), 182 deletions(-) diff --git a/spring-web/src/test/java/org/springframework/web/accept/DefaultApiVersionStrategiesTests.java b/spring-web/src/test/java/org/springframework/web/accept/DefaultApiVersionStrategiesTests.java index 812831953f5..d24f139b695 100644 --- a/spring-web/src/test/java/org/springframework/web/accept/DefaultApiVersionStrategiesTests.java +++ b/spring-web/src/test/java/org/springframework/web/accept/DefaultApiVersionStrategiesTests.java @@ -36,7 +36,7 @@ public class DefaultApiVersionStrategiesTests { @Test - void defaultVersion() { + void defaultVersionIsParsed() { SemanticApiVersionParser.Version version = this.parser.parseVersion("1.2.3"); ApiVersionStrategy strategy = initVersionStrategy(version.toString()); @@ -44,21 +44,29 @@ public class DefaultApiVersionStrategiesTests { } @Test - void supportedVersions() { - SemanticApiVersionParser.Version v1 = this.parser.parseVersion("1"); - SemanticApiVersionParser.Version v2 = this.parser.parseVersion("2"); - SemanticApiVersionParser.Version v9 = this.parser.parseVersion("9"); + void validateSupportedVersion() { + SemanticApiVersionParser.Version v12 = this.parser.parseVersion("1.2"); DefaultApiVersionStrategy strategy = initVersionStrategy(null); - strategy.addSupportedVersion(v1.toString()); - strategy.addSupportedVersion(v2.toString()); + strategy.addSupportedVersion(v12.toString()); MockHttpServletRequest request = new MockHttpServletRequest(); - strategy.validateVersion(v1, request); - strategy.validateVersion(v2, request); + strategy.validateVersion(v12, request); + } + + @Test + void validateUnsupportedVersion() { + assertThatThrownBy(() -> initVersionStrategy(null).validateVersion("1.2", new MockHttpServletRequest())) + .isInstanceOf(InvalidApiVersionException.class) + .hasMessage("400 BAD_REQUEST \"Invalid API version: '1.2'.\""); + } - assertThatThrownBy(() -> strategy.validateVersion(v9, request)) - .isInstanceOf(InvalidApiVersionException.class); + @Test + void missingRequiredVersion() { + DefaultApiVersionStrategy strategy = initVersionStrategy(null); + assertThatThrownBy(() -> strategy.validateVersion(null, new MockHttpServletRequest())) + .isInstanceOf(MissingApiVersionException.class) + .hasMessage("400 BAD_REQUEST \"API version is required.\""); } private static DefaultApiVersionStrategy initVersionStrategy(@Nullable String defaultValue) { diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/result/condition/VersionRequestCondition.java b/spring-webflux/src/main/java/org/springframework/web/reactive/result/condition/VersionRequestCondition.java index 0a24c8475a9..5d701f28f30 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/result/condition/VersionRequestCondition.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/result/condition/VersionRequestCondition.java @@ -24,7 +24,6 @@ import org.jspecify.annotations.Nullable; import org.springframework.util.Assert; import org.springframework.util.StringUtils; -import org.springframework.web.accept.InvalidApiVersionException; import org.springframework.web.accept.NotAcceptableApiVersionException; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.reactive.HandlerMapping; @@ -42,21 +41,12 @@ import org.springframework.web.server.ServerWebExchange; */ public final class VersionRequestCondition extends AbstractRequestCondition { - private static final String VERSION_ATTRIBUTE_NAME = VersionRequestCondition.class.getName() + ".VERSION"; - - private static final String NO_VERSION_ATTRIBUTE = "NO_VERSION"; - - private static final ApiVersionStrategy NO_OP_VERSION_STRATEGY = new NoOpApiVersionStrategy(); - - private final @Nullable String versionValue; private final @Nullable Object version; private final boolean baselineVersion; - private final ApiVersionStrategy versionStrategy; - private final Set content; @@ -67,14 +57,12 @@ public final class VersionRequestCondition extends AbstractRequestCondition requestVersion = exchange.getAttribute(VERSION_ATTRIBUTE_NAME); - if (requestVersion == null) { - String value = this.versionStrategy.resolveVersion(exchange); - requestVersion = (value != null ? parseVersion(value) : this.versionStrategy.getDefaultVersion()); - this.versionStrategy.validateVersion(requestVersion, exchange); - requestVersion = (requestVersion != null ? requestVersion : NO_VERSION_ATTRIBUTE); - exchange.getAttributes().put(VERSION_ATTRIBUTE_NAME, requestVersion); - } + Comparable requestVersion = exchange.getAttribute(HandlerMapping.API_VERSION_ATTRIBUTE); - if (this.version == null || requestVersion == NO_VERSION_ATTRIBUTE) { + if (this.version == null || requestVersion == null) { return this; } - exchange.getAttributes().put(HandlerMapping.API_VERSION_ATTRIBUTE, requestVersion); - - // At this stage, match all versions as baseline versions. - // Strict matching for fixed versions is enforced at the end in handleMatch. + // Always use a baseline match here in order to select the highest version (baseline or fixed) + // The fixed version match is enforced at the end in handleMatch() int result = compareVersions(this.version, requestVersion); return (result <= 0 ? this : null); } - private Comparable parseVersion(String value) { - try { - return this.versionStrategy.parseVersion(value); - } - catch (Exception ex) { - throw new InvalidApiVersionException(value, null, ex); - } - } - @SuppressWarnings("unchecked") private > int compareVersions(Object v1, Object v2) { return ((V) v1).compareTo((V) v2); @@ -178,7 +148,7 @@ public final class VersionRequestCondition extends AbstractRequestCondition version = exchange.getAttribute(VERSION_ATTRIBUTE_NAME); + Comparable version = exchange.getAttribute(HandlerMapping.API_VERSION_ATTRIBUTE); Assert.state(version != null, "No API version attribute"); if (!this.version.equals(version)) { throw new NotAcceptableApiVersionException(version.toString()); @@ -186,31 +156,4 @@ public final class VersionRequestCondition extends AbstractRequestCondition requestVersion, ServerWebExchange exchange) { - } - - @Override - public @Nullable Comparable getDefaultVersion() { - return null; - } - - @Override - public void handleDeprecations(Comparable version, ServerWebExchange exchange) { - } - } - } 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 117c6661145..56f2f98c5a9 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 @@ -28,6 +28,7 @@ import java.util.function.Predicate; import java.util.stream.Stream; import org.jspecify.annotations.Nullable; +import reactor.core.publisher.Mono; import org.springframework.context.EmbeddedValueResolverAware; import org.springframework.core.annotation.AnnotatedElementUtils; @@ -41,6 +42,7 @@ import org.springframework.util.Assert; import org.springframework.util.CollectionUtils; import org.springframework.util.StringUtils; import org.springframework.util.StringValueResolver; +import org.springframework.web.accept.InvalidApiVersionException; import org.springframework.web.bind.annotation.CrossOrigin; import org.springframework.web.bind.annotation.RequestBody; import org.springframework.web.bind.annotation.RequestMapping; @@ -173,6 +175,37 @@ public class RequestMappingHandlerMapping extends RequestMappingInfoHandlerMappi return AnnotatedElementUtils.hasAnnotation(beanType, Controller.class); } + @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); + } + } + } + return super.getHandlerInternal(exchange); + } + + private static @Nullable Comparable getApiVersion( + ServerWebExchange exchange, ApiVersionStrategy versionStrategy) { + + String value = versionStrategy.resolveVersion(exchange); + if (value == null) { + return versionStrategy.getDefaultVersion(); + } + try { + Comparable version = versionStrategy.parseVersion(value); + versionStrategy.validateVersion(version, exchange); + return version; + } + catch (Exception ex) { + throw new InvalidApiVersionException(value, null, ex); + } + } + /** * Uses type-level and method-level {@link RequestMapping @RequestMapping} * and {@link HttpExchange @HttpExchange} annotations to create the diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/accept/DefaultApiVersionStrategiesTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/accept/DefaultApiVersionStrategiesTests.java index 0da6278b761..82d62ef6c7a 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/accept/DefaultApiVersionStrategiesTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/accept/DefaultApiVersionStrategiesTests.java @@ -22,6 +22,7 @@ import org.jspecify.annotations.Nullable; import org.junit.jupiter.api.Test; import org.springframework.web.accept.InvalidApiVersionException; +import org.springframework.web.accept.MissingApiVersionException; import org.springframework.web.accept.SemanticApiVersionParser; import org.springframework.web.server.ServerWebExchange; import org.springframework.web.testfixture.http.server.reactive.MockServerHttpRequest; @@ -59,7 +60,8 @@ public class DefaultApiVersionStrategiesTests { @Test void validateUnsupportedVersion() { assertThatThrownBy(() -> validateVersion("1.2", apiVersionStrategy())) - .isInstanceOf(InvalidApiVersionException.class); + .isInstanceOf(InvalidApiVersionException.class) + .hasMessage("400 BAD_REQUEST \"Invalid API version: '1.2.0'.\""); } @Test @@ -80,6 +82,14 @@ public class DefaultApiVersionStrategiesTests { .isInstanceOf(InvalidApiVersionException.class); } + @Test + void missingRequiredVersion() { + DefaultApiVersionStrategy strategy = apiVersionStrategy(); + assertThatThrownBy(() -> strategy.validateVersion(null, exchange)) + .isInstanceOf(MissingApiVersionException.class) + .hasMessage("400 BAD_REQUEST \"API version is required.\""); + } + private static DefaultApiVersionStrategy apiVersionStrategy() { return apiVersionStrategy(null, false); } diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/result/condition/VersionRequestConditionTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/result/condition/VersionRequestConditionTests.java index 933c91635a4..ef5fa635480 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/result/condition/VersionRequestConditionTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/result/condition/VersionRequestConditionTests.java @@ -23,10 +23,9 @@ import org.jspecify.annotations.Nullable; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -import org.springframework.web.accept.InvalidApiVersionException; -import org.springframework.web.accept.MissingApiVersionException; import org.springframework.web.accept.NotAcceptableApiVersionException; import org.springframework.web.accept.SemanticApiVersionParser; +import org.springframework.web.reactive.HandlerMapping; import org.springframework.web.reactive.accept.DefaultApiVersionStrategy; import org.springframework.web.server.ServerWebExchange; import org.springframework.web.testfixture.http.server.reactive.MockServerHttpRequest; @@ -99,12 +98,6 @@ public class VersionRequestConditionTests { testMatch("v1.1", condition, true, false); testMatch("v1.3", condition, true, false); - - assertThatThrownBy(() -> condition.getMatchingCondition(exchangeWithVersion("1.2"))) - .isInstanceOf(InvalidApiVersionException.class); - - assertThatThrownBy(() -> condition.getMatchingCondition(MockServerWebExchange.from(MockServerHttpRequest.get("/")))) - .isInstanceOf(MissingApiVersionException.class); } private void testMatch( @@ -128,12 +121,6 @@ public class VersionRequestConditionTests { condition.handleMatch(exchange); } - @Test - void missingRequiredVersion() { - assertThatThrownBy(() -> condition("1.2").getMatchingCondition(exchange())) - .hasMessage("400 BAD_REQUEST \"API version is required.\""); - } - @Test void defaultVersion() { String version = "1.2"; @@ -144,12 +131,6 @@ public class VersionRequestConditionTests { assertThat(match).isSameAs(condition); } - @Test - void unsupportedVersion() { - assertThatThrownBy(() -> condition("1.2").getMatchingCondition(exchangeWithVersion("1.3"))) - .hasMessage("400 BAD_REQUEST \"Invalid API version: '1.3.0'.\""); - } - @Test void compare() { testCompare("1.1", "1", "1.1"); @@ -181,8 +162,10 @@ public class VersionRequestConditionTests { } private ServerWebExchange exchangeWithVersion(String v) { - return MockServerWebExchange.from( - MockServerHttpRequest.get("/path").queryParam("api-version", v)); + Comparable version = this.strategy.parseVersion(v); + MockServerWebExchange exchange = MockServerWebExchange.from(MockServerHttpRequest.get("/path")); + exchange.getAttributes().put(HandlerMapping.API_VERSION_ATTRIBUTE, version); + return exchange; } } diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/condition/VersionRequestCondition.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/condition/VersionRequestCondition.java index 0dcd117e7e1..fd824ee8305 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/condition/VersionRequestCondition.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/condition/VersionRequestCondition.java @@ -21,13 +21,11 @@ import java.util.Collections; import java.util.Set; import jakarta.servlet.http.HttpServletRequest; -import jakarta.servlet.http.HttpServletResponse; import org.jspecify.annotations.Nullable; import org.springframework.util.Assert; import org.springframework.util.StringUtils; import org.springframework.web.accept.ApiVersionStrategy; -import org.springframework.web.accept.InvalidApiVersionException; import org.springframework.web.accept.NotAcceptableApiVersionException; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.servlet.HandlerMapping; @@ -42,21 +40,12 @@ import org.springframework.web.servlet.HandlerMapping; */ public final class VersionRequestCondition extends AbstractRequestCondition { - private static final String VERSION_ATTRIBUTE_NAME = VersionRequestCondition.class.getName() + ".VERSION"; - - private static final String NO_VERSION_ATTRIBUTE = "NO_VERSION"; - - private static final ApiVersionStrategy NO_OP_VERSION_STRATEGY = new NoOpApiVersionStrategy(); - - private final @Nullable String versionValue; private final @Nullable Comparable version; private final boolean baselineVersion; - private final ApiVersionStrategy versionStrategy; - private final Set content; @@ -67,14 +56,12 @@ public final class VersionRequestCondition extends AbstractRequestCondition requestVersion = (Comparable) request.getAttribute(VERSION_ATTRIBUTE_NAME); - if (requestVersion == null) { - String value = this.versionStrategy.resolveVersion(request); - requestVersion = (value != null ? parseVersion(value) : this.versionStrategy.getDefaultVersion()); - this.versionStrategy.validateVersion(requestVersion, request); - requestVersion = (requestVersion != null ? requestVersion : NO_VERSION_ATTRIBUTE); - request.setAttribute(VERSION_ATTRIBUTE_NAME, requestVersion); - } + Comparable requestVersion = (Comparable) request.getAttribute(HandlerMapping.API_VERSION_ATTRIBUTE); - if (this.version == null || requestVersion == NO_VERSION_ATTRIBUTE) { + if (this.version == null || requestVersion == null) { return this; } - request.setAttribute(HandlerMapping.API_VERSION_ATTRIBUTE, requestVersion); - - // At this stage, match all versions as baseline versions. - // Strict matching for fixed versions is enforced at the end in handleMatch. + // Always use a baseline match here in order to select the highest version (baseline or fixed) + // The fixed version match is enforced at the end in handleMatch() int result = compareVersions(this.version, requestVersion); return (result <= 0 ? this : null); } - private Comparable parseVersion(String value) { - try { - return this.versionStrategy.parseVersion(value); - } - catch (Exception ex) { - throw new InvalidApiVersionException(value, null, ex); - } - } - @SuppressWarnings("unchecked") private > int compareVersions(Object v1, Object v2) { return ((V) v1).compareTo((V) v2); @@ -178,7 +147,7 @@ public final class VersionRequestCondition extends AbstractRequestCondition version = (Comparable) request.getAttribute(VERSION_ATTRIBUTE_NAME); + Comparable version = (Comparable) request.getAttribute(HandlerMapping.API_VERSION_ATTRIBUTE); Assert.state(version != null, "No API version attribute"); if (!this.version.equals(version)) { throw new NotAcceptableApiVersionException(version.toString()); @@ -186,31 +155,4 @@ public final class VersionRequestCondition extends AbstractRequestCondition requestVersion, HttpServletRequest request) { - } - - @Override - public @Nullable Comparable getDefaultVersion() { - return null; - } - - @Override - public void handleDeprecations(Comparable version, HttpServletRequest request, HttpServletResponse response) { - } - } - } 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 a51170d7737..ffbff6537b1 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 @@ -46,6 +46,7 @@ import org.springframework.util.StringValueResolver; import org.springframework.web.accept.ApiVersionStrategy; import org.springframework.web.accept.ContentNegotiationManager; import org.springframework.web.accept.DefaultApiVersionStrategy; +import org.springframework.web.accept.InvalidApiVersionException; import org.springframework.web.bind.annotation.CrossOrigin; import org.springframework.web.bind.annotation.RequestBody; import org.springframework.web.bind.annotation.RequestMapping; @@ -200,6 +201,38 @@ public class RequestMappingHandlerMapping extends RequestMappingInfoHandlerMappi return AnnotatedElementUtils.hasAnnotation(beanType, Controller.class); } + + @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); + } + } + } + return super.getHandlerInternal(request); + } + + private static @Nullable Comparable getApiVersion( + HttpServletRequest request, ApiVersionStrategy versionStrategy) { + + String value = versionStrategy.resolveVersion(request); + if (value == null) { + return versionStrategy.getDefaultVersion(); + } + try { + Comparable version = versionStrategy.parseVersion(value); + versionStrategy.validateVersion(version, request); + return version; + } + catch (Exception ex) { + throw new InvalidApiVersionException(value, null, ex); + } + } + /** * Uses type-level and method-level {@link RequestMapping @RequestMapping} * and {@link HttpExchange @HttpExchange} annotations to create the diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/condition/VersionRequestConditionTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/condition/VersionRequestConditionTests.java index 394ad04b9d3..5e14295c48f 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/condition/VersionRequestConditionTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/condition/VersionRequestConditionTests.java @@ -24,10 +24,9 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.springframework.web.accept.DefaultApiVersionStrategy; -import org.springframework.web.accept.InvalidApiVersionException; -import org.springframework.web.accept.MissingApiVersionException; import org.springframework.web.accept.NotAcceptableApiVersionException; import org.springframework.web.accept.SemanticApiVersionParser; +import org.springframework.web.servlet.HandlerMapping; import org.springframework.web.testfixture.servlet.MockHttpServletRequest; import static org.assertj.core.api.Assertions.assertThat; @@ -97,12 +96,6 @@ public class VersionRequestConditionTests { testMatch("v1.1", condition, true, false); testMatch("v1.3", condition, true, false); - - assertThatThrownBy(() -> condition.getMatchingCondition(requestWithVersion("1.2"))) - .isInstanceOf(InvalidApiVersionException.class); - - assertThatThrownBy(() -> condition.getMatchingCondition(new MockHttpServletRequest("GET", "/path"))) - .isInstanceOf(MissingApiVersionException.class); } private void testMatch( @@ -126,12 +119,6 @@ public class VersionRequestConditionTests { condition.handleMatch(request); } - @Test - void missingRequiredVersion() { - assertThatThrownBy(() -> condition("1.2").getMatchingCondition(new MockHttpServletRequest("GET", "/path"))) - .hasMessage("400 BAD_REQUEST \"API version is required.\""); - } - @Test void defaultVersion() { String version = "1.2"; @@ -142,12 +129,6 @@ public class VersionRequestConditionTests { assertThat(match).isSameAs(condition); } - @Test - void unsupportedVersion() { - assertThatThrownBy(() -> condition("1.2").getMatchingCondition(requestWithVersion("1.3"))) - .hasMessage("400 BAD_REQUEST \"Invalid API version: '1.3.0'.\""); - } - @Test void compare() { testCompare("1.1", "1", "1.1"); @@ -176,7 +157,7 @@ public class VersionRequestConditionTests { private MockHttpServletRequest requestWithVersion(String v) { MockHttpServletRequest request = new MockHttpServletRequest("GET", "/path"); - request.addParameter("api-version", v); + request.setAttribute(HandlerMapping.API_VERSION_ATTRIBUTE, this.strategy.parseVersion(v)); return request; }