Skip to content

Commit a0f9872

Browse files
rstoyanchevrstoyanchev
committed
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
1 parent 5d34f9c commit a0f9872

File tree

6 files changed

+94
-108
lines changed

6 files changed

+94
-108
lines changed

‎spring-webflux/src/main/java/org/springframework/web/reactive/result/condition/VersionRequestCondition.java

Lines changed: 27 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import org.jspecify.annotations.Nullable;
2424

2525
import org.springframework.util.Assert;
26+
import org.springframework.util.StringUtils;
2627
import org.springframework.web.accept.InvalidApiVersionException;
2728
import org.springframework.web.accept.NotAcceptableApiVersionException;
2829
import org.springframework.web.bind.annotation.RequestMapping;
@@ -58,20 +59,24 @@ public final class VersionRequestCondition extends AbstractRequestCondition<Vers
5859
private final Set<String> content;
5960

6061

61-
public VersionRequestCondition() {
62-
this.versionValue = null;
63-
this.version = null;
64-
this.baselineVersion = false;
65-
this.versionStrategy = NO_OP_VERSION_STRATEGY;
66-
this.content = Collections.emptySet();
67-
}
68-
69-
public VersionRequestCondition(String configuredVersion, ApiVersionStrategy versionStrategy) {
70-
this.baselineVersion = configuredVersion.endsWith("+");
71-
this.versionValue = updateVersion(configuredVersion, this.baselineVersion);
72-
this.version = versionStrategy.parseVersion(this.versionValue);
73-
this.versionStrategy = versionStrategy;
74-
this.content = Set.of(configuredVersion);
62+
/**
63+
* Constructor with the version, if set on the {@code @RequestMapping}, and
64+
* the {@code ApiVersionStrategy}, if API versioning is enabled.
65+
*/
66+
public VersionRequestCondition(@Nullable String version, @Nullable ApiVersionStrategy strategy) {
67+
this.versionStrategy = (strategy != null ? strategy : NO_OP_VERSION_STRATEGY);
68+
if (StringUtils.hasText(version)) {
69+
this.baselineVersion = version.endsWith("+");
70+
this.versionValue = updateVersion(version, this.baselineVersion);
71+
this.version = this.versionStrategy.parseVersion(this.versionValue);
72+
this.content = Set.of(version);
73+
}
74+
else {
75+
this.versionValue = null;
76+
this.version = null;
77+
this.baselineVersion = false;
78+
this.content = Collections.emptySet();
79+
}
7580
}
7681

7782
private static String updateVersion(String version, boolean baselineVersion) {
@@ -104,23 +109,23 @@ public VersionRequestCondition combine(VersionRequestCondition other) {
104109
return this;
105110
}
106111

107-
Comparable<?> version = exchange.getAttribute(VERSION_ATTRIBUTE_NAME);
108-
if (version == null) {
112+
Comparable<?> requestVersion = exchange.getAttribute(VERSION_ATTRIBUTE_NAME);
113+
if (requestVersion == null) {
109114
String value = this.versionStrategy.resolveVersion(exchange);
110-
version = (value != null ? parseVersion(value) : this.versionStrategy.getDefaultVersion());
111-
this.versionStrategy.validateVersion(version, exchange);
112-
version = (version != null ? version : NO_VERSION_ATTRIBUTE);
113-
exchange.getAttributes().put(VERSION_ATTRIBUTE_NAME, (version));
115+
requestVersion = (value != null ? parseVersion(value) : this.versionStrategy.getDefaultVersion());
116+
this.versionStrategy.validateVersion(requestVersion, exchange);
117+
requestVersion = (requestVersion != null ? requestVersion : NO_VERSION_ATTRIBUTE);
118+
exchange.getAttributes().put(VERSION_ATTRIBUTE_NAME, (requestVersion));
114119
}
115120

116-
if (version == NO_VERSION_ATTRIBUTE) {
121+
if (requestVersion == NO_VERSION_ATTRIBUTE) {
117122
return this;
118123
}
119124

120125
// At this stage, match all versions as baseline versions.
121126
// Strict matching for fixed versions is enforced at the end in handleMatch.
122127

123-
int result = compareVersions(this.version, version);
128+
int result = compareVersions(this.version, requestVersion);
124129
return (result <= 0 ? this : null);
125130
}
126131

‎spring-webflux/src/main/java/org/springframework/web/reactive/result/method/RequestMappingInfo.java

Lines changed: 9 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -71,8 +71,6 @@ public final class RequestMappingInfo implements RequestCondition<RequestMapping
7171

7272
private static final ProducesRequestCondition EMPTY_PRODUCES = new ProducesRequestCondition();
7373

74-
private static final VersionRequestCondition EMPTY_VERSION = new VersionRequestCondition();
75-
7674
private static final RequestConditionHolder EMPTY_CUSTOM = new RequestConditionHolder(null);
7775

7876

@@ -112,7 +110,7 @@ private RequestMappingInfo(@Nullable String name, @Nullable PatternsRequestCondi
112110
this.headersCondition = (headers != null ? headers : EMPTY_HEADERS);
113111
this.consumesCondition = (consumes != null ? consumes : EMPTY_CONSUMES);
114112
this.producesCondition = (produces != null ? produces : EMPTY_PRODUCES);
115-
this.versionCondition = (version != null ? version : EMPTY_VERSION);
113+
this.versionCondition = (version != null ? version : new VersionRequestCondition(null, null));
116114
this.customConditionHolder = (custom != null ? new RequestConditionHolder(custom) : EMPTY_CUSTOM);
117115
this.options = options;
118116

@@ -572,15 +570,10 @@ public RequestMappingInfo build() {
572570

573571
RequestedContentTypeResolver contentTypeResolver = this.options.getContentTypeResolver();
574572

575-
VersionRequestCondition versionCondition;
576-
ApiVersionStrategy versionStrategy = this.options.getApiVersionStrategy();
577-
if (StringUtils.hasText(this.version)) {
578-
Assert.state(versionStrategy != null, "API version specified, but no ApiVersionStrategy configured");
579-
versionCondition = new VersionRequestCondition(this.version, versionStrategy);
580-
}
581-
else {
582-
versionCondition = EMPTY_VERSION;
583-
}
573+
ApiVersionStrategy strategy = this.options.getApiVersionStrategy();
574+
Assert.state(strategy != null || !StringUtils.hasText(this.version),
575+
"API version specified, but no ApiVersionStrategy configured");
576+
VersionRequestCondition versionCondition = new VersionRequestCondition(this.version, strategy);
584577

585578
return new RequestMappingInfo(this.mappingName,
586579
isEmpty(this.paths) ? null : new PatternsRequestCondition(parse(this.paths, parser)),
@@ -706,14 +699,10 @@ public Builder produces(String... produces) {
706699

707700
@Override
708701
public Builder version(@Nullable String version) {
709-
if (version != null) {
710-
ApiVersionStrategy strategy = this.options.getApiVersionStrategy();
711-
Assert.state(strategy != null, "API version specified, but no ApiVersionStrategy configured");
712-
this.versionCondition = new VersionRequestCondition(version, strategy);
713-
}
714-
else {
715-
this.versionCondition = EMPTY_VERSION;
716-
}
702+
ApiVersionStrategy strategy = this.options.getApiVersionStrategy();
703+
Assert.state(strategy != null || !StringUtils.hasText(version),
704+
"API version specified, but no ApiVersionStrategy configured");
705+
this.versionCondition = new VersionRequestCondition(version, strategy);
717706
return this;
718707
}
719708

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

Lines changed: 11 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -72,32 +72,31 @@ void combineTypeAndMethodLevel() {
7272

7373
@Test
7474
void fixedVersionMatch() {
75-
String conditionVersion = "1.2";
75+
VersionRequestCondition condition = condition("1.2");
7676
this.strategy.addSupportedVersion("1.1", "1.3");
7777

78-
testMatch("v1.1", conditionVersion, true, false);
79-
testMatch("v1.2", conditionVersion, false, false);
80-
testMatch("v1.3", conditionVersion, false, true);
78+
testMatch("v1.1", condition, false, false);
79+
testMatch("v1.2", condition, true, false);
80+
testMatch("v1.3", condition, true, true); // match initially, reject if chosen
8181
}
8282

8383
@Test
8484
void baselineVersionMatch() {
85-
String conditionVersion = "1.2+";
85+
VersionRequestCondition condition = condition("1.2+");
8686
this.strategy.addSupportedVersion("1.1", "1.3");
8787

88-
testMatch("v1.1", conditionVersion, true, false);
89-
testMatch("v1.2", conditionVersion, false, false);
90-
testMatch("v1.3", conditionVersion, false, false);
88+
testMatch("v1.1", condition, false, false);
89+
testMatch("v1.2", condition, true, false);
90+
testMatch("v1.3", condition, true, false);
9191
}
9292

9393
private void testMatch(
94-
String requestVersion, String conditionVersion, boolean notCompatible, boolean notAcceptable) {
94+
String requestVersion, VersionRequestCondition condition, boolean matches, boolean notAcceptable) {
9595

9696
ServerWebExchange exchange = exchangeWithVersion(requestVersion);
97-
VersionRequestCondition condition = condition(conditionVersion);
9897
VersionRequestCondition match = condition.getMatchingCondition(exchange);
9998

100-
if (notCompatible) {
99+
if (!matches) {
101100
assertThat(match).isNull();
102101
return;
103102
}
@@ -157,7 +156,7 @@ private VersionRequestCondition condition(String v) {
157156
}
158157

159158
private VersionRequestCondition emptyCondition() {
160-
return new VersionRequestCondition();
159+
return new VersionRequestCondition(null, this.strategy);
161160
}
162161

163162
private static MockServerWebExchange exchange() {

‎spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/condition/VersionRequestCondition.java

Lines changed: 27 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
import org.jspecify.annotations.Nullable;
2525

2626
import org.springframework.util.Assert;
27+
import org.springframework.util.StringUtils;
2728
import org.springframework.web.accept.ApiVersionStrategy;
2829
import org.springframework.web.accept.InvalidApiVersionException;
2930
import org.springframework.web.accept.NotAcceptableApiVersionException;
@@ -57,20 +58,24 @@ public final class VersionRequestCondition extends AbstractRequestCondition<Vers
5758
private final Set<String> content;
5859

5960

60-
public VersionRequestCondition() {
61-
this.versionValue = null;
62-
this.version = null;
63-
this.baselineVersion = false;
64-
this.versionStrategy = NO_OP_VERSION_STRATEGY;
65-
this.content = Collections.emptySet();
66-
}
67-
68-
public VersionRequestCondition(String configuredVersion, ApiVersionStrategy versionStrategy) {
69-
this.baselineVersion = configuredVersion.endsWith("+");
70-
this.versionValue = updateVersion(configuredVersion, this.baselineVersion);
71-
this.version = versionStrategy.parseVersion(this.versionValue);
72-
this.versionStrategy = versionStrategy;
73-
this.content = Set.of(configuredVersion);
61+
/**
62+
* Constructor with the version, if set on the {@code @RequestMapping}, and
63+
* the {@code ApiVersionStrategy}, if API versioning is enabled.
64+
*/
65+
public VersionRequestCondition(@Nullable String version, @Nullable ApiVersionStrategy strategy) {
66+
this.versionStrategy = (strategy != null ? strategy : NO_OP_VERSION_STRATEGY);
67+
if (StringUtils.hasText(version)) {
68+
this.baselineVersion = version.endsWith("+");
69+
this.versionValue = updateVersion(version, this.baselineVersion);
70+
this.version = this.versionStrategy.parseVersion(this.versionValue);
71+
this.content = Set.of(version);
72+
}
73+
else {
74+
this.versionValue = null;
75+
this.version = null;
76+
this.baselineVersion = false;
77+
this.content = Collections.emptySet();
78+
}
7479
}
7580

7681
private static String updateVersion(String version, boolean baselineVersion) {
@@ -103,23 +108,23 @@ public VersionRequestCondition combine(VersionRequestCondition other) {
103108
return this;
104109
}
105110

106-
Comparable<?> version = (Comparable<?>) request.getAttribute(VERSION_ATTRIBUTE_NAME);
107-
if (version == null) {
111+
Comparable<?> requestVersion = (Comparable<?>) request.getAttribute(VERSION_ATTRIBUTE_NAME);
112+
if (requestVersion == null) {
108113
String value = this.versionStrategy.resolveVersion(request);
109-
version = (value != null ? parseVersion(value) : this.versionStrategy.getDefaultVersion());
110-
this.versionStrategy.validateVersion(version, request);
111-
version = (version != null ? version : NO_VERSION_ATTRIBUTE);
112-
request.setAttribute(VERSION_ATTRIBUTE_NAME, (version));
114+
requestVersion = (value != null ? parseVersion(value) : this.versionStrategy.getDefaultVersion());
115+
this.versionStrategy.validateVersion(requestVersion, request);
116+
requestVersion = (requestVersion != null ? requestVersion : NO_VERSION_ATTRIBUTE);
117+
request.setAttribute(VERSION_ATTRIBUTE_NAME, (requestVersion));
113118
}
114119

115-
if (version == NO_VERSION_ATTRIBUTE) {
120+
if (requestVersion == NO_VERSION_ATTRIBUTE) {
116121
return this;
117122
}
118123

119124
// At this stage, match all versions as baseline versions.
120125
// Strict matching for fixed versions is enforced at the end in handleMatch.
121126

122-
int result = compareVersions(this.version, version);
127+
int result = compareVersions(this.version, requestVersion);
123128
return (result <= 0 ? this : null);
124129
}
125130

‎spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/RequestMappingInfo.java

Lines changed: 9 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -80,8 +80,6 @@ public final class RequestMappingInfo implements RequestCondition<RequestMapping
8080

8181
private static final ProducesRequestCondition EMPTY_PRODUCES = new ProducesRequestCondition();
8282

83-
private static final VersionRequestCondition EMPTY_VERSION = new VersionRequestCondition();
84-
8583
private static final RequestConditionHolder EMPTY_CUSTOM = new RequestConditionHolder(null);
8684

8785

@@ -130,7 +128,7 @@ public RequestMappingInfo(@Nullable String name, @Nullable PatternsRequestCondit
130128
(headers != null ? headers : EMPTY_HEADERS),
131129
(consumes != null ? consumes : EMPTY_CONSUMES),
132130
(produces != null ? produces : EMPTY_PRODUCES),
133-
(version != null ? version : EMPTY_VERSION),
131+
(version != null ? version : new VersionRequestCondition(null, null)),
134132
(custom != null ? new RequestConditionHolder(custom) : EMPTY_CUSTOM),
135133
new BuilderConfiguration());
136134
}
@@ -771,15 +769,10 @@ public RequestMappingInfo build() {
771769

772770
ContentNegotiationManager manager = this.options.getContentNegotiationManager();
773771

774-
VersionRequestCondition versionCondition;
775-
ApiVersionStrategy versionStrategy = this.options.getApiVersionStrategy();
776-
if (StringUtils.hasText(this.version)) {
777-
Assert.state(versionStrategy != null, "API version specified, but no ApiVersionStrategy configured");
778-
versionCondition = new VersionRequestCondition(this.version, versionStrategy);
779-
}
780-
else {
781-
versionCondition = EMPTY_VERSION;
782-
}
772+
ApiVersionStrategy strategy = this.options.getApiVersionStrategy();
773+
Assert.state(strategy != null || !StringUtils.hasText(this.version),
774+
"API version specified, but no ApiVersionStrategy configured");
775+
VersionRequestCondition versionCondition = new VersionRequestCondition(this.version, strategy);
783776

784777
return new RequestMappingInfo(
785778
this.mappingName, pathPatternsCondition, patternsCondition,
@@ -894,14 +887,10 @@ public Builder produces(String... produces) {
894887

895888
@Override
896889
public Builder version(@Nullable String version) {
897-
if (version != null) {
898-
ApiVersionStrategy strategy = this.options.getApiVersionStrategy();
899-
Assert.state(strategy != null, "API version specified, but no ApiVersionStrategy configured");
900-
this.versionCondition = new VersionRequestCondition(version, strategy);
901-
}
902-
else {
903-
this.versionCondition = EMPTY_VERSION;
904-
}
890+
ApiVersionStrategy strategy = this.options.getApiVersionStrategy();
891+
Assert.state(strategy != null || !StringUtils.hasText(version),
892+
"API version specified, but no ApiVersionStrategy configured");
893+
this.versionCondition = new VersionRequestCondition(version, strategy);
905894
return this;
906895
}
907896

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

Lines changed: 11 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -70,32 +70,31 @@ void combineTypeAndMethodLevel() {
7070

7171
@Test
7272
void fixedVersionMatch() {
73-
String conditionVersion = "1.2";
73+
VersionRequestCondition condition = condition("1.2");
7474
this.strategy.addSupportedVersion("1.1", "1.3");
7575

76-
testMatch("v1.1", conditionVersion, true, false);
77-
testMatch("v1.2", conditionVersion, false, false);
78-
testMatch("v1.3", conditionVersion, false, true);
76+
testMatch("v1.1", condition, false, false);
77+
testMatch("v1.2", condition, true, false);
78+
testMatch("v1.3", condition, true, true); // match initially, reject if chosen
7979
}
8080

8181
@Test
8282
void baselineVersionMatch() {
83-
String conditionVersion = "1.2+";
83+
VersionRequestCondition condition = condition("1.2+");
8484
this.strategy.addSupportedVersion("1.1", "1.3");
8585

86-
testMatch("v1.1", conditionVersion, true, false);
87-
testMatch("v1.2", conditionVersion, false, false);
88-
testMatch("v1.3", conditionVersion, false, false);
86+
testMatch("v1.1", condition, false, false);
87+
testMatch("v1.2", condition, true, false);
88+
testMatch("v1.3", condition, true, false);
8989
}
9090

9191
private void testMatch(
92-
String requestVersion, String conditionVersion, boolean notCompatible, boolean notAcceptable) {
92+
String requestVersion, VersionRequestCondition condition, boolean matches, boolean notAcceptable) {
9393

9494
MockHttpServletRequest request = requestWithVersion(requestVersion);
95-
VersionRequestCondition condition = condition(conditionVersion);
9695
VersionRequestCondition match = condition.getMatchingCondition(request);
9796

98-
if (notCompatible) {
97+
if (!matches) {
9998
assertThat(match).isNull();
10099
return;
101100
}
@@ -155,7 +154,7 @@ private VersionRequestCondition condition(String v) {
155154
}
156155

157156
private VersionRequestCondition emptyCondition() {
158-
return new VersionRequestCondition();
157+
return new VersionRequestCondition(null, this.strategy);
159158
}
160159

161160
private MockHttpServletRequest requestWithVersion(String v) {

0 commit comments

Comments
 (0)