Assert versionRequired and defaultVersion

Closes gh-35387

Author: rstoyanchev

spring-web/src/main/java/org/springframework/web/accept/DefaultApiVersionStrategy.java

@@ -61,9 +61,9 @@ public class DefaultApiVersionStrategy implements ApiVersionStrategy {
 	 * @param versionResolvers one or more resolvers to try; the first non-null
 	 * value returned by any resolver becomes the value used
 	 * @param versionParser parser for raw version values
-	 * @param versionRequired whether a version is required; if a request
-	 * does not have a version, and a {@code defaultVersion} is not specified,
-	 * validation fails with {@link MissingApiVersionException}
+	 * @param versionRequired whether a version is required leading to
+	 * {@link MissingApiVersionException} for requests that don't have one;
+	 * by default set to true unless there is a defaultVersion
 	 * @param defaultVersion a default version to assign to requests that
 	 * don't specify one
 	 * @param detectSupportedVersions whether to use API versions that appear in
@@ -74,16 +74,18 @@ public class DefaultApiVersionStrategy implements ApiVersionStrategy {
 	 */
 	public DefaultApiVersionStrategy(
 			List<ApiVersionResolver> versionResolvers, ApiVersionParser<?> versionParser,
-			boolean versionRequired, @Nullable String defaultVersion,
+			@Nullable Boolean versionRequired, @Nullable String defaultVersion,
 			boolean detectSupportedVersions, @Nullable Predicate<Comparable<?>> supportedVersionPredicate,
 			@Nullable ApiVersionDeprecationHandler deprecationHandler) {
 
 		Assert.notEmpty(versionResolvers, "At least one ApiVersionResolver is required");
 		Assert.notNull(versionParser, "ApiVersionParser is required");
+		Assert.isTrue(defaultVersion == null || versionRequired == null || !versionRequired,
+				"versionRequired cannot be set to true if a defaultVersion is also configured");
 
 		this.versionResolvers = new ArrayList<>(versionResolvers);
 		this.versionParser = versionParser;
-		this.versionRequired = (versionRequired && defaultVersion == null);
+		this.versionRequired = (versionRequired != null ? versionRequired : defaultVersion == null);
 		this.defaultVersion = (defaultVersion != null ? versionParser.parseVersion(defaultVersion) : null);
 		this.detectSupportedVersions = detectSupportedVersions;
 		this.supportedVersionPredicate = initSupportedVersionPredicate(supportedVersionPredicate);

spring-web/src/test/java/org/springframework/web/accept/DefaultApiVersionStrategiesTests.java

@@ -25,6 +25,7 @@
 import org.springframework.web.testfixture.servlet.MockHttpServletRequest;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
 /**
@@ -93,6 +94,16 @@ void validateUnsupportedWithPredicate() {
 		assertThatThrownBy(() -> validateVersion("1.2", strategy)).isInstanceOf(InvalidApiVersionException.class);
 	}
 
+	@Test
+	void versionRequiredAndDefaultVersionSet() {
+		assertThatIllegalArgumentException()
+				.isThrownBy(() ->
+						new DefaultApiVersionStrategy(
+								List.of(request -> request.getParameter("api-version")), new SemanticApiVersionParser(),
+								true, "1.2", true, version -> true, null))
+				.withMessage("versionRequired cannot be set to true if a defaultVersion is also configured");
+	}
+
 	private static DefaultApiVersionStrategy apiVersionStrategy() {
 		return apiVersionStrategy(null, false, null);
 	}
@@ -107,7 +118,7 @@ private static DefaultApiVersionStrategy apiVersionStrategy(
 
 		return new DefaultApiVersionStrategy(
 				List.of(request -> request.getParameter("api-version")), new SemanticApiVersionParser(),
-				true, defaultVersion, detectSupportedVersions, supportedVersionPredicate, null);
+				null, defaultVersion, detectSupportedVersions, supportedVersionPredicate, null);
 	}
 
 	private void validateVersion(@Nullable String version, DefaultApiVersionStrategy strategy) {

spring-webflux/src/main/java/org/springframework/web/reactive/accept/DefaultApiVersionStrategy.java

@@ -63,9 +63,9 @@ public class DefaultApiVersionStrategy implements ApiVersionStrategy {
 	 * @param versionResolvers one or more resolvers to try; the first non-null
 	 * value returned by any resolver becomes the resolved used
 	 * @param versionParser parser for to raw version values
-	 * @param versionRequired whether a version is required; if a request
-	 * does not have a version, and a {@code defaultVersion} is not specified,
-	 * validation fails with {@link MissingApiVersionException}
+	 * @param versionRequired whether a version is required leading to
+	 * {@link MissingApiVersionException} for requests that don't have one;
+	 * by default set to true unless there is a defaultVersion
 	 * @param defaultVersion a default version to assign to requests that
 	 * don't specify one
 	 * @param detectSupportedVersions whether to use API versions that appear in
@@ -76,16 +76,18 @@ public class DefaultApiVersionStrategy implements ApiVersionStrategy {
 	 */
 	public DefaultApiVersionStrategy(
 			List<ApiVersionResolver> versionResolvers, ApiVersionParser<?> versionParser,
-			boolean versionRequired, @Nullable String defaultVersion,
+			@Nullable Boolean versionRequired, @Nullable String defaultVersion,
 			boolean detectSupportedVersions, @Nullable Predicate<Comparable<?>> supportedVersionPredicate,
 			@Nullable ApiVersionDeprecationHandler deprecationHandler) {
 
 		Assert.notEmpty(versionResolvers, "At least one ApiVersionResolver is required");
 		Assert.notNull(versionParser, "ApiVersionParser is required");
+		Assert.isTrue(defaultVersion == null || versionRequired == null || !versionRequired,
+				"versionRequired cannot be set to true if a defaultVersion is also configured");
 
 		this.versionResolvers = new ArrayList<>(versionResolvers);
 		this.versionParser = versionParser;
-		this.versionRequired = (versionRequired && defaultVersion == null);
+		this.versionRequired = (versionRequired != null ? versionRequired : defaultVersion == null);
 		this.defaultVersion = (defaultVersion != null ? versionParser.parseVersion(defaultVersion) : null);
 		this.detectSupportedVersions = detectSupportedVersions;
 		this.supportedVersionPredicate = initSupportedVersionPredicate(supportedVersionPredicate);

spring-webflux/src/main/java/org/springframework/web/reactive/config/ApiVersionConfigurer.java

@@ -212,7 +212,7 @@ public ApiVersionConfigurer setDeprecationHandler(ApiVersionDeprecationHandler h
 
 		DefaultApiVersionStrategy strategy = new DefaultApiVersionStrategy(this.versionResolvers,
 				(this.versionParser != null ? this.versionParser : new SemanticApiVersionParser()),
-				(this.versionRequired != null ? this.versionRequired : true), this.defaultVersion,
+				this.versionRequired, this.defaultVersion,
 				this.detectSupportedVersions, this.supportedVersionPredicate,
 				this.deprecationHandler);
 

spring-webflux/src/test/java/org/springframework/web/reactive/accept/DefaultApiVersionStrategiesTests.java

@@ -29,6 +29,7 @@
 import org.springframework.web.testfixture.server.MockServerWebExchange;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
 /**
@@ -97,6 +98,16 @@ void validateUnsupportedWithPredicate() {
 		assertThatThrownBy(() -> validateVersion("1.2", strategy)).isInstanceOf(InvalidApiVersionException.class);
 	}
 
+	@Test
+	void versionRequiredAndDefaultVersionSet() {
+		assertThatIllegalArgumentException()
+				.isThrownBy(() ->
+						new org.springframework.web.accept.DefaultApiVersionStrategy(
+								List.of(request -> request.getParameter("api-version")), new SemanticApiVersionParser(),
+								true, "1.2", true, version -> true, null))
+				.withMessage("versionRequired cannot be set to true if a defaultVersion is also configured");
+	}
+
 	private static DefaultApiVersionStrategy apiVersionStrategy() {
 		return apiVersionStrategy(null, false, null);
 	}
@@ -107,15 +118,14 @@ private static DefaultApiVersionStrategy apiVersionStrategy(
 
 			return new DefaultApiVersionStrategy(
 				List.of(exchange -> exchange.getRequest().getQueryParams().getFirst("api-version")),
-				parser, true, defaultVersion, detectSupportedVersions, supportedVersionPredicate, null);
+				parser, null, defaultVersion, detectSupportedVersions, supportedVersionPredicate, null);
 	}
 
 	private void validateVersion(@Nullable String version, DefaultApiVersionStrategy strategy) {
 		MockServerHttpRequest.BaseBuilder<?> requestBuilder = MockServerHttpRequest.get("/");
 		if (version != null) {
 			requestBuilder.queryParam("api-version", version);
 		}
-		Comparable<?> parsedVersion = (version != null ? parser.parseVersion(version) : null);
 		strategy.resolveParseAndValidateVersion(MockServerWebExchange.builder(requestBuilder).build());
 	}
 

spring-webflux/src/test/java/org/springframework/web/reactive/result/condition/VersionRequestConditionTests.java

@@ -52,7 +52,7 @@ void setUp() {
 	private static DefaultApiVersionStrategy initVersionStrategy(@Nullable String defaultVersion) {
 		return new DefaultApiVersionStrategy(
 				List.of(exchange -> exchange.getRequest().getQueryParams().getFirst("api-version")),
-				new SemanticApiVersionParser(), true, defaultVersion, false, null, null);
+				new SemanticApiVersionParser(), null, defaultVersion, false, null, null);
 	}
 
 	@Test

spring-webmvc/src/main/java/org/springframework/web/servlet/config/annotation/ApiVersionConfigurer.java

@@ -213,7 +213,7 @@ public ApiVersionConfigurer setDeprecationHandler(ApiVersionDeprecationHandler h
 
 		DefaultApiVersionStrategy strategy = new DefaultApiVersionStrategy(this.versionResolvers,
 				(this.versionParser != null ? this.versionParser : new SemanticApiVersionParser()),
-				(this.versionRequired != null ? this.versionRequired : true), this.defaultVersion,
+				this.versionRequired, this.defaultVersion,
 				this.detectSupportedVersions, this.supportedVersionPredicate,
 				this.deprecationHandler);
 

spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/condition/VersionRequestConditionTests.java

@@ -50,7 +50,7 @@ void setUp() {
 	private static DefaultApiVersionStrategy initVersionStrategy(@Nullable String defaultVersion) {
 		return new DefaultApiVersionStrategy(
 				List.of(request -> request.getParameter("api-version")),
-				new SemanticApiVersionParser(), true, defaultVersion, false, null, null);
+				new SemanticApiVersionParser(), null, defaultVersion, false, null, null);
 	}
 
 	@Test