From a0f98727460aab99a641ed558aec84ebd2997c44 Mon Sep 17 00:00:00 2001
From: rstoyanchev <rossen.stoyanchev@broadcom.com>
Date: Wed, 18 Jun 2025 16:48:32 +0100
Subject: [PATCH] Refactor construction of VersionRequestCondition

The single constructor now supports all combinations of having a version
attribute set or not, and ApiVersionStrategy, configured or not.

In effective, ensure the configured ApiVersionStrategy is passed even
when the RequestMapping version attribute is not set.

See gh-35082
---
 .../condition/VersionRequestCondition.java    | 49 ++++++++++---------
 .../result/method/RequestMappingInfo.java     | 29 ++++-------
 .../VersionRequestConditionTests.java         | 23 +++++----
 .../condition/VersionRequestCondition.java    | 49 ++++++++++---------
 .../mvc/method/RequestMappingInfo.java        | 29 ++++-------
 .../VersionRequestConditionTests.java         | 23 +++++----
 6 files changed, 94 insertions(+), 108 deletions(-)

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 76eb0b82060..ceb1d51f255 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
@@ -23,6 +23,7 @@ import java.util.Set;
 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;
@@ -58,20 +59,24 @@ public final class VersionRequestCondition extends AbstractRequestCondition<Vers
 	private final Set<String> content;
 
 
-	public VersionRequestCondition() {
-		this.versionValue = null;
-		this.version = null;
-		this.baselineVersion = false;
-		this.versionStrategy = NO_OP_VERSION_STRATEGY;
-		this.content = Collections.emptySet();
-	}
-
-	public VersionRequestCondition(String configuredVersion, ApiVersionStrategy versionStrategy) {
-		this.baselineVersion = configuredVersion.endsWith("+");
-		this.versionValue = updateVersion(configuredVersion, this.baselineVersion);
-		this.version = versionStrategy.parseVersion(this.versionValue);
-		this.versionStrategy = versionStrategy;
-		this.content = Set.of(configuredVersion);
+	/**
+	 * Constructor with the version, if set on the {@code @RequestMapping}, and
+	 * the {@code ApiVersionStrategy}, if API versioning is enabled.
+	 */
+	public VersionRequestCondition(@Nullable String version, @Nullable ApiVersionStrategy strategy) {
+		this.versionStrategy = (strategy != null ? strategy : NO_OP_VERSION_STRATEGY);
+		if (StringUtils.hasText(version)) {
+			this.baselineVersion = version.endsWith("+");
+			this.versionValue = updateVersion(version, this.baselineVersion);
+			this.version = this.versionStrategy.parseVersion(this.versionValue);
+			this.content = Set.of(version);
+		}
+		else {
+			this.versionValue = null;
+			this.version = null;
+			this.baselineVersion = false;
+			this.content = Collections.emptySet();
+		}
 	}
 
 	private static String updateVersion(String version, boolean baselineVersion) {
@@ -104,23 +109,23 @@ public final class VersionRequestCondition extends AbstractRequestCondition<Vers
 			return this;
 		}
 
-		Comparable<?> version = exchange.getAttribute(VERSION_ATTRIBUTE_NAME);
-		if (version == null) {
+		Comparable<?> requestVersion = exchange.getAttribute(VERSION_ATTRIBUTE_NAME);
+		if (requestVersion == null) {
 			String value = this.versionStrategy.resolveVersion(exchange);
-			version = (value != null ? parseVersion(value) : this.versionStrategy.getDefaultVersion());
-			this.versionStrategy.validateVersion(version, exchange);
-			version = (version != null ? version : NO_VERSION_ATTRIBUTE);
-			exchange.getAttributes().put(VERSION_ATTRIBUTE_NAME, (version));
+			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));
 		}
 
-		if (version == NO_VERSION_ATTRIBUTE) {
+		if (requestVersion == NO_VERSION_ATTRIBUTE) {
 			return this;
 		}
 
 		// At this stage, match all versions as baseline versions.
 		// Strict matching for fixed versions is enforced at the end in handleMatch.
 
-		int result = compareVersions(this.version, version);
+		int result = compareVersions(this.version, requestVersion);
 		return (result <= 0 ? this : null);
 	}
 
diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/RequestMappingInfo.java b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/RequestMappingInfo.java
index 7ee51821822..b70ab318f24 100644
--- a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/RequestMappingInfo.java
+++ b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/RequestMappingInfo.java
@@ -71,8 +71,6 @@ public final class RequestMappingInfo implements RequestCondition<RequestMapping
 
 	private static final ProducesRequestCondition EMPTY_PRODUCES = new ProducesRequestCondition();
 
-	private static final VersionRequestCondition EMPTY_VERSION = new VersionRequestCondition();
-
 	private static final RequestConditionHolder EMPTY_CUSTOM = new RequestConditionHolder(null);
 
 
@@ -112,7 +110,7 @@ public final class RequestMappingInfo implements RequestCondition<RequestMapping
 		this.headersCondition = (headers != null ? headers : EMPTY_HEADERS);
 		this.consumesCondition = (consumes != null ? consumes : EMPTY_CONSUMES);
 		this.producesCondition = (produces != null ? produces : EMPTY_PRODUCES);
-		this.versionCondition = (version != null ? version : EMPTY_VERSION);
+		this.versionCondition = (version != null ? version : new VersionRequestCondition(null, null));
 		this.customConditionHolder = (custom != null ? new RequestConditionHolder(custom) : EMPTY_CUSTOM);
 		this.options = options;
 
@@ -572,15 +570,10 @@ public final class RequestMappingInfo implements RequestCondition<RequestMapping
 
 			RequestedContentTypeResolver contentTypeResolver = this.options.getContentTypeResolver();
 
-			VersionRequestCondition versionCondition;
-			ApiVersionStrategy versionStrategy = this.options.getApiVersionStrategy();
-			if (StringUtils.hasText(this.version)) {
-				Assert.state(versionStrategy != null, "API version specified, but no ApiVersionStrategy configured");
-				versionCondition = new VersionRequestCondition(this.version, versionStrategy);
-			}
-			else {
-				versionCondition = EMPTY_VERSION;
-			}
+			ApiVersionStrategy strategy = this.options.getApiVersionStrategy();
+			Assert.state(strategy != null || !StringUtils.hasText(this.version),
+					"API version specified, but no ApiVersionStrategy configured");
+			VersionRequestCondition versionCondition = new VersionRequestCondition(this.version, strategy);
 
 			return new RequestMappingInfo(this.mappingName,
 					isEmpty(this.paths) ? null : new PatternsRequestCondition(parse(this.paths, parser)),
@@ -706,14 +699,10 @@ public final class RequestMappingInfo implements RequestCondition<RequestMapping
 
 		@Override
 		public Builder version(@Nullable String version) {
-			if (version != null) {
-				ApiVersionStrategy strategy = this.options.getApiVersionStrategy();
-				Assert.state(strategy != null, "API version specified, but no ApiVersionStrategy configured");
-				this.versionCondition = new VersionRequestCondition(version, strategy);
-			}
-			else {
-				this.versionCondition = EMPTY_VERSION;
-			}
+			ApiVersionStrategy strategy = this.options.getApiVersionStrategy();
+			Assert.state(strategy != null || !StringUtils.hasText(version),
+					"API version specified, but no ApiVersionStrategy configured");
+			this.versionCondition = new VersionRequestCondition(version, strategy);
 			return this;
 		}
 
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 16728ebeeb9..e97f6cb4883 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
@@ -72,32 +72,31 @@ public class VersionRequestConditionTests {
 
 	@Test
 	void fixedVersionMatch() {
-		String conditionVersion = "1.2";
+		VersionRequestCondition condition = condition("1.2");
 		this.strategy.addSupportedVersion("1.1", "1.3");
 
-		testMatch("v1.1", conditionVersion, true, false);
-		testMatch("v1.2", conditionVersion, false, false);
-		testMatch("v1.3", conditionVersion, false, true);
+		testMatch("v1.1", condition, false, false);
+		testMatch("v1.2", condition, true, false);
+		testMatch("v1.3", condition, true, true); // match initially, reject if chosen
 	}
 
 	@Test
 	void baselineVersionMatch() {
-		String conditionVersion = "1.2+";
+		VersionRequestCondition condition = condition("1.2+");
 		this.strategy.addSupportedVersion("1.1", "1.3");
 
-		testMatch("v1.1", conditionVersion, true, false);
-		testMatch("v1.2", conditionVersion, false, false);
-		testMatch("v1.3", conditionVersion, false, false);
+		testMatch("v1.1", condition, false, false);
+		testMatch("v1.2", condition, true, false);
+		testMatch("v1.3", condition, true, false);
 	}
 
 	private void testMatch(
-			String requestVersion, String conditionVersion, boolean notCompatible, boolean notAcceptable) {
+			String requestVersion, VersionRequestCondition condition, boolean matches, boolean notAcceptable) {
 
 		ServerWebExchange exchange = exchangeWithVersion(requestVersion);
-		VersionRequestCondition condition = condition(conditionVersion);
 		VersionRequestCondition match = condition.getMatchingCondition(exchange);
 
-		if (notCompatible) {
+		if (!matches) {
 			assertThat(match).isNull();
 			return;
 		}
@@ -157,7 +156,7 @@ public class VersionRequestConditionTests {
 	}
 
 	private VersionRequestCondition emptyCondition() {
-		return new VersionRequestCondition();
+		return new VersionRequestCondition(null, this.strategy);
 	}
 
 	private static MockServerWebExchange 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 db094785322..401c257229b 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
@@ -24,6 +24,7 @@ import jakarta.servlet.http.HttpServletRequest;
 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;
@@ -57,20 +58,24 @@ public final class VersionRequestCondition extends AbstractRequestCondition<Vers
 	private final Set<String> content;
 
 
-	public VersionRequestCondition() {
-		this.versionValue = null;
-		this.version = null;
-		this.baselineVersion = false;
-		this.versionStrategy = NO_OP_VERSION_STRATEGY;
-		this.content = Collections.emptySet();
-	}
-
-	public VersionRequestCondition(String configuredVersion, ApiVersionStrategy versionStrategy) {
-		this.baselineVersion = configuredVersion.endsWith("+");
-		this.versionValue = updateVersion(configuredVersion, this.baselineVersion);
-		this.version = versionStrategy.parseVersion(this.versionValue);
-		this.versionStrategy = versionStrategy;
-		this.content = Set.of(configuredVersion);
+	/**
+	 * Constructor with the version, if set on the {@code @RequestMapping}, and
+	 * the {@code ApiVersionStrategy}, if API versioning is enabled.
+	 */
+	public VersionRequestCondition(@Nullable String version, @Nullable ApiVersionStrategy strategy) {
+		this.versionStrategy = (strategy != null ? strategy : NO_OP_VERSION_STRATEGY);
+		if (StringUtils.hasText(version)) {
+			this.baselineVersion = version.endsWith("+");
+			this.versionValue = updateVersion(version, this.baselineVersion);
+			this.version = this.versionStrategy.parseVersion(this.versionValue);
+			this.content = Set.of(version);
+		}
+		else {
+			this.versionValue = null;
+			this.version = null;
+			this.baselineVersion = false;
+			this.content = Collections.emptySet();
+		}
 	}
 
 	private static String updateVersion(String version, boolean baselineVersion) {
@@ -103,23 +108,23 @@ public final class VersionRequestCondition extends AbstractRequestCondition<Vers
 			return this;
 		}
 
-		Comparable<?> version = (Comparable<?>) request.getAttribute(VERSION_ATTRIBUTE_NAME);
-		if (version == null) {
+		Comparable<?> requestVersion = (Comparable<?>) request.getAttribute(VERSION_ATTRIBUTE_NAME);
+		if (requestVersion == null) {
 			String value = this.versionStrategy.resolveVersion(request);
-			version = (value != null ? parseVersion(value) : this.versionStrategy.getDefaultVersion());
-			this.versionStrategy.validateVersion(version, request);
-			version = (version != null ? version : NO_VERSION_ATTRIBUTE);
-			request.setAttribute(VERSION_ATTRIBUTE_NAME, (version));
+			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));
 		}
 
-		if (version == NO_VERSION_ATTRIBUTE) {
+		if (requestVersion == NO_VERSION_ATTRIBUTE) {
 			return this;
 		}
 
 		// At this stage, match all versions as baseline versions.
 		// Strict matching for fixed versions is enforced at the end in handleMatch.
 
-		int result = compareVersions(this.version, version);
+		int result = compareVersions(this.version, requestVersion);
 		return (result <= 0 ? this : null);
 	}
 
diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/RequestMappingInfo.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/RequestMappingInfo.java
index 87db7f4f5b0..3a400624b4f 100644
--- a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/RequestMappingInfo.java
+++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/RequestMappingInfo.java
@@ -80,8 +80,6 @@ public final class RequestMappingInfo implements RequestCondition<RequestMapping
 
 	private static final ProducesRequestCondition EMPTY_PRODUCES = new ProducesRequestCondition();
 
-	private static final VersionRequestCondition EMPTY_VERSION = new VersionRequestCondition();
-
 	private static final RequestConditionHolder EMPTY_CUSTOM = new RequestConditionHolder(null);
 
 
@@ -130,7 +128,7 @@ public final class RequestMappingInfo implements RequestCondition<RequestMapping
 				(headers != null ? headers : EMPTY_HEADERS),
 				(consumes != null ? consumes : EMPTY_CONSUMES),
 				(produces != null ? produces : EMPTY_PRODUCES),
-				(version != null ? version : EMPTY_VERSION),
+				(version != null ? version : new VersionRequestCondition(null, null)),
 				(custom != null ? new RequestConditionHolder(custom) : EMPTY_CUSTOM),
 				new BuilderConfiguration());
 	}
@@ -771,15 +769,10 @@ public final class RequestMappingInfo implements RequestCondition<RequestMapping
 
 			ContentNegotiationManager manager = this.options.getContentNegotiationManager();
 
-			VersionRequestCondition versionCondition;
-			ApiVersionStrategy versionStrategy = this.options.getApiVersionStrategy();
-			if (StringUtils.hasText(this.version)) {
-				Assert.state(versionStrategy != null, "API version specified, but no ApiVersionStrategy configured");
-				versionCondition = new VersionRequestCondition(this.version, versionStrategy);
-			}
-			else {
-				versionCondition = EMPTY_VERSION;
-			}
+			ApiVersionStrategy strategy = this.options.getApiVersionStrategy();
+			Assert.state(strategy != null || !StringUtils.hasText(this.version),
+					"API version specified, but no ApiVersionStrategy configured");
+			VersionRequestCondition versionCondition = new VersionRequestCondition(this.version, strategy);
 
 			return new RequestMappingInfo(
 					this.mappingName, pathPatternsCondition, patternsCondition,
@@ -894,14 +887,10 @@ public final class RequestMappingInfo implements RequestCondition<RequestMapping
 
 		@Override
 		public Builder version(@Nullable String version) {
-			if (version != null) {
-				ApiVersionStrategy strategy = this.options.getApiVersionStrategy();
-				Assert.state(strategy != null, "API version specified, but no ApiVersionStrategy configured");
-				this.versionCondition = new VersionRequestCondition(version, strategy);
-			}
-			else {
-				this.versionCondition = EMPTY_VERSION;
-			}
+			ApiVersionStrategy strategy = this.options.getApiVersionStrategy();
+			Assert.state(strategy != null || !StringUtils.hasText(version),
+					"API version specified, but no ApiVersionStrategy configured");
+			this.versionCondition = new VersionRequestCondition(version, strategy);
 			return this;
 		}
 
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 40dcb79453b..9d90263c808 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
@@ -70,32 +70,31 @@ public class VersionRequestConditionTests {
 
 	@Test
 	void fixedVersionMatch() {
-		String conditionVersion = "1.2";
+		VersionRequestCondition condition = condition("1.2");
 		this.strategy.addSupportedVersion("1.1", "1.3");
 
-		testMatch("v1.1", conditionVersion, true, false);
-		testMatch("v1.2", conditionVersion, false, false);
-		testMatch("v1.3", conditionVersion, false, true);
+		testMatch("v1.1", condition, false, false);
+		testMatch("v1.2", condition, true, false);
+		testMatch("v1.3", condition, true, true); // match initially, reject if chosen
 	}
 
 	@Test
 	void baselineVersionMatch() {
-		String conditionVersion = "1.2+";
+		VersionRequestCondition condition = condition("1.2+");
 		this.strategy.addSupportedVersion("1.1", "1.3");
 
-		testMatch("v1.1", conditionVersion, true, false);
-		testMatch("v1.2", conditionVersion, false, false);
-		testMatch("v1.3", conditionVersion, false, false);
+		testMatch("v1.1", condition, false, false);
+		testMatch("v1.2", condition, true, false);
+		testMatch("v1.3", condition, true, false);
 	}
 
 	private void testMatch(
-			String requestVersion, String conditionVersion, boolean notCompatible, boolean notAcceptable) {
+			String requestVersion, VersionRequestCondition condition, boolean matches, boolean notAcceptable) {
 
 		MockHttpServletRequest request = requestWithVersion(requestVersion);
-		VersionRequestCondition condition = condition(conditionVersion);
 		VersionRequestCondition match = condition.getMatchingCondition(request);
 
-		if (notCompatible) {
+		if (!matches) {
 			assertThat(match).isNull();
 			return;
 		}
@@ -155,7 +154,7 @@ public class VersionRequestConditionTests {
 	}
 
 	private VersionRequestCondition emptyCondition() {
-		return new VersionRequestCondition();
+		return new VersionRequestCondition(null, this.strategy);
 	}
 
 	private MockHttpServletRequest requestWithVersion(String v) {