Skip to content

Commit 0cfbf60

Browse files
simonbaslesimonbasle
committed
Rename MockMVC matcher methods to prevent regression in user tests
This commit changes the name of two recently introduced methods in the `MockRestRequestMatchers` class for header and queryParam. These have been found to cause false negatives in user tests, due to the new overload taking precedence in some cases. Namely, using a `Matcher` factory method which can apply to both `List` and `String` will cause the compiler to select the newest list overload, by instantiating a `Matcher<Object>`. This can cause false negatives in user tests, failing tests that used to pass because the Matcher previously applied to the first String in the header or queryParam value list. For instance, `equalsTo("a")`. The new overloads are recent enough and this has enough potential to cause an arbitrary number of user tests to fail that we break the API to eliminate the ambiguity, by renaming the methods with a `*List` suffix. See gh-30220 See gh-30238 Closes gh-30235
1 parent e931fdc commit 0cfbf60

File tree

2 files changed

+73
-69
lines changed

2 files changed

+73
-69
lines changed

‎spring-test/src/main/java/org/springframework/test/web/client/match/MockRestRequestMatchers.java

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -125,11 +125,11 @@ public static RequestMatcher requestTo(URI uri) {
125125
* @param name the name of the query parameter whose value(s) will be asserted
126126
* @param matcher the Hamcrest matcher to apply to the entire list of values
127127
* for the given query parameter
128-
* @since 5.3.26
128+
* @since 5.3.27
129129
* @see #queryParam(String, Matcher...)
130130
* @see #queryParam(String, String...)
131131
*/
132-
public static RequestMatcher queryParam(String name, Matcher<? super List<String>> matcher) {
132+
public static RequestMatcher queryParamList(String name, Matcher<? super List<String>> matcher) {
133133
return request -> {
134134
MultiValueMap<String, String> params = getQueryParams(request);
135135
List<String> paramValues = params.get(name);
@@ -147,14 +147,14 @@ public static RequestMatcher queryParam(String name, Matcher<? super List<String
147147
* values, effectively ignoring the additional parameter values. If the number
148148
* of provided {@code matchers} exceeds the number of query parameter values,
149149
* an {@link AssertionError} will be thrown to signal the mismatch.
150-
* <p>See {@link #queryParam(String, Matcher)} for a variant which accepts a
150+
* <p>See {@link #queryParamList(String, Matcher)} for a variant which accepts a
151151
* {@code Matcher} that applies to the entire list of values as opposed to
152152
* applying only to individual values.
153153
* @param name the name of the query parameter whose value(s) will be asserted
154154
* @param matchers the Hamcrest matchers to apply to individual query parameter
155155
* values; the n<sup>th</sup> matcher is applied to the n<sup>th</sup> query
156156
* parameter value
157-
* @see #queryParam(String, Matcher)
157+
* @see #queryParamList(String, Matcher)
158158
* @see #queryParam(String, String...)
159159
*/
160160
@SafeVarargs
@@ -175,14 +175,14 @@ public static RequestMatcher queryParam(String name, Matcher<? super String>...
175175
* parameter values, effectively ignoring the additional parameter values. If
176176
* the number of {@code expectedValues} exceeds the number of query parameter
177177
* values, an {@link AssertionError} will be thrown to signal the mismatch.
178-
* <p>See {@link #queryParam(String, Matcher)} for a variant which accepts a
178+
* <p>See {@link #queryParamList(String, Matcher)} for a variant which accepts a
179179
* Hamcrest {@code Matcher} that applies to the entire list of values as opposed
180180
* to asserting only individual values.
181181
* @param name the name of the query parameter whose value(s) will be asserted
182182
* @param expectedValues the expected values of individual query parameter values;
183183
* the n<sup>th</sup> expected value is compared to the n<sup>th</sup> query
184184
* parameter value
185-
* @see #queryParam(String, Matcher)
185+
* @see #queryParamList(String, Matcher)
186186
* @see #queryParam(String, Matcher...)
187187
*/
188188
public static RequestMatcher queryParam(String name, String... expectedValues) {
@@ -211,11 +211,11 @@ private static MultiValueMap<String, String> getQueryParams(ClientHttpRequest re
211211
* @param name the name of the header whose value(s) will be asserted
212212
* @param matcher the Hamcrest matcher to apply to the entire list of values
213213
* for the given header
214-
* @since 5.3.26
214+
* @since 5.3.27
215215
* @see #header(String, Matcher...)
216216
* @see #header(String, String...)
217217
*/
218-
public static RequestMatcher header(String name, Matcher<? super List<String>> matcher) {
218+
public static RequestMatcher headerList(String name, Matcher<? super List<String>> matcher) {
219219
return request -> {
220220
List<String> headerValues = request.getHeaders().get(name);
221221
if (headerValues == null) {
@@ -232,13 +232,13 @@ public static RequestMatcher header(String name, Matcher<? super List<String>> m
232232
* effectively ignoring the additional header values. If the number of
233233
* provided {@code matchers} exceeds the number of header values, an
234234
* {@link AssertionError} will be thrown to signal the mismatch.
235-
* <p>See {@link #header(String, Matcher)} for a variant which accepts a
235+
* <p>See {@link #headerList(String, Matcher)} for a variant which accepts a
236236
* Hamcrest {@code Matcher} that applies to the entire list of values as
237237
* opposed to applying only to individual values.
238238
* @param name the name of the header whose value(s) will be asserted
239239
* @param matchers the Hamcrest matchers to apply to individual header values;
240240
* the n<sup>th</sup> matcher is applied to the n<sup>th</sup> header value
241-
* @see #header(String, Matcher)
241+
* @see #headerList(String, Matcher)
242242
* @see #header(String, String...)
243243
*/
244244
@SafeVarargs
@@ -260,13 +260,13 @@ public static RequestMatcher header(String name, Matcher<? super String>... matc
260260
* additional header values. If the number of {@code expectedValues} exceeds the
261261
* number of header values, an {@link AssertionError} will be thrown to signal the
262262
* mismatch.
263-
* <p>See {@link #header(String, Matcher)} for a variant which accepts a
263+
* <p>See {@link #headerList(String, Matcher)} for a variant which accepts a
264264
* Hamcrest {@code Matcher} that applies to the entire list of values as
265265
* opposed to applying only to individual values.
266266
* @param name the name of the header whose value(s) will be asserted
267267
* @param expectedValues the expected values of individual header values; the
268268
* n<sup>th</sup> expected value is compared to the n<sup>th</sup> header value
269-
* @see #header(String, Matcher)
269+
* @see #headerList(String, Matcher)
270270
* @see #header(String, Matcher...)
271271
*/
272272
public static RequestMatcher header(String name, String... expectedValues) {

‎spring-test/src/test/java/org/springframework/test/web/client/match/MockRestRequestMatchersTests.java

Lines changed: 61 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -29,19 +29,17 @@
2929

3030
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
3131
import static org.hamcrest.Matchers.allOf;
32-
import static org.hamcrest.Matchers.any;
3332
import static org.hamcrest.Matchers.anything;
3433
import static org.hamcrest.Matchers.contains;
3534
import static org.hamcrest.Matchers.containsInAnyOrder;
3635
import static org.hamcrest.Matchers.containsString;
37-
import static org.hamcrest.Matchers.either;
3836
import static org.hamcrest.Matchers.endsWith;
37+
import static org.hamcrest.Matchers.equalTo;
3938
import static org.hamcrest.Matchers.everyItem;
4039
import static org.hamcrest.Matchers.hasItem;
4140
import static org.hamcrest.Matchers.hasSize;
4241
import static org.hamcrest.Matchers.is;
4342
import static org.hamcrest.Matchers.notNullValue;
44-
import static org.hamcrest.Matchers.nullValue;
4543
import static org.hamcrest.Matchers.startsWith;
4644

4745
/**
@@ -164,65 +162,68 @@ void headerContainsWithMissingValue() {
164162
@Test
165163
void headerListMissing() {
166164
assertThatAssertionError()
167-
.isThrownBy(() -> MockRestRequestMatchers.header("foo", hasSize(2)).match(this.request))
165+
.isThrownBy(() -> MockRestRequestMatchers.headerList("foo", hasSize(2)).match(this.request))
168166
.withMessage("Expected header <foo> to exist but was null");
169167
}
170168

171169
@Test
172170
void headerListMatchers() throws IOException {
173171
this.request.getHeaders().put("foo", Arrays.asList("bar", "baz"));
174172

175-
MockRestRequestMatchers.header("foo", containsInAnyOrder(endsWith("baz"), endsWith("bar"))).match(this.request);
176-
MockRestRequestMatchers.header("foo", contains(is("bar"), is("baz"))).match(this.request);
177-
MockRestRequestMatchers.header("foo", contains(is("bar"), anything())).match(this.request);
178-
MockRestRequestMatchers.header("foo", hasItem(endsWith("baz"))).match(this.request);
179-
MockRestRequestMatchers.header("foo", everyItem(startsWith("ba"))).match(this.request);
180-
MockRestRequestMatchers.header("foo", hasSize(2)).match(this.request);
181-
182-
// These can be a bit ambiguous when reading the test (the compiler selects the list matcher):
183-
MockRestRequestMatchers.header("foo", notNullValue()).match(this.request);
184-
MockRestRequestMatchers.header("foo", is(anything())).match(this.request);
185-
MockRestRequestMatchers.header("foo", allOf(notNullValue(), notNullValue())).match(this.request);
186-
187-
// These are not as ambiguous thanks to an inner matcher that is either obviously list-oriented,
188-
// string-oriented, or obviously a vararg of matchers
189-
190-
// list matcher version
191-
MockRestRequestMatchers.header("foo", allOf(notNullValue(), hasSize(2))).match(this.request);
192-
193-
// vararg version
194-
MockRestRequestMatchers.header("foo", allOf(notNullValue(), endsWith("ar"))).match(this.request);
195-
MockRestRequestMatchers.header("foo", is((any(String.class)))).match(this.request);
196-
MockRestRequestMatchers.header("foo", either(is("bar")).or(is(nullValue()))).match(this.request);
197-
MockRestRequestMatchers.header("foo", is(notNullValue()), is(notNullValue())).match(this.request);
173+
MockRestRequestMatchers.headerList("foo", containsInAnyOrder(endsWith("baz"), endsWith("bar"))).match(this.request);
174+
MockRestRequestMatchers.headerList("foo", contains(is("bar"), is("baz"))).match(this.request);
175+
MockRestRequestMatchers.headerList("foo", contains(is("bar"), anything())).match(this.request);
176+
MockRestRequestMatchers.headerList("foo", hasItem(endsWith("baz"))).match(this.request);
177+
MockRestRequestMatchers.headerList("foo", everyItem(startsWith("ba"))).match(this.request);
178+
MockRestRequestMatchers.headerList("foo", hasSize(2)).match(this.request);
179+
180+
MockRestRequestMatchers.headerList("foo", notNullValue()).match(this.request);
181+
MockRestRequestMatchers.headerList("foo", is(anything())).match(this.request);
182+
MockRestRequestMatchers.headerList("foo", allOf(notNullValue(), notNullValue())).match(this.request);
183+
184+
MockRestRequestMatchers.headerList("foo", allOf(notNullValue(), hasSize(2))).match(this.request);
198185
}
199186

200187
@Test
201188
void headerListContainsMismatch() {
202189
this.request.getHeaders().put("foo", Arrays.asList("bar", "baz"));
203190

204191
assertThatAssertionError()
205-
.isThrownBy(() -> MockRestRequestMatchers.header("foo", contains(containsString("ba"))).match(this.request))
192+
.isThrownBy(() -> MockRestRequestMatchers.headerList("foo", contains(containsString("ba"))).match(this.request))
206193
.withMessageContainingAll(
207194
"Request header [foo] values",
208195
"Expected: iterable containing [a string containing \"ba\"]",
209196
"but: not matched: \"baz\"");
210197

211198
assertThatAssertionError()
212-
.isThrownBy(() -> MockRestRequestMatchers.header("foo", hasItem(endsWith("ba"))).match(this.request))
199+
.isThrownBy(() -> MockRestRequestMatchers.headerList("foo", hasItem(endsWith("ba"))).match(this.request))
213200
.withMessageContainingAll(
214201
"Request header [foo] values",
215202
"Expected: a collection containing a string ending with \"ba\"",
216203
"but: mismatches were: [was \"bar\", was \"baz\"]");
217204

218205
assertThatAssertionError()
219-
.isThrownBy(() -> MockRestRequestMatchers.header("foo", everyItem(endsWith("ar"))).match(this.request))
206+
.isThrownBy(() -> MockRestRequestMatchers.headerList("foo", everyItem(endsWith("ar"))).match(this.request))
220207
.withMessageContainingAll(
221208
"Request header [foo] values",
222209
"Expected: every item is a string ending with \"ar\"",
223210
"but: an item was \"baz\"");
224211
}
225212

213+
@Test
214+
void headerListDoesntHideHeaderWithSingleMatcher() throws IOException {
215+
this.request.getHeaders().put("foo", List.of("bar", "baz"));
216+
217+
MockRestRequestMatchers.header("foo", equalTo("bar")).match(this.request);
218+
219+
assertThatAssertionError()
220+
.isThrownBy(() -> MockRestRequestMatchers.headerList("foo", equalTo("bar")).match(this.request))
221+
.withMessageContainingAll(
222+
"Request header [foo] values",
223+
"Expected: \"bar\"",
224+
"but: was <[bar, baz]>");
225+
}
226+
226227
@Test
227228
void headers() throws Exception {
228229
this.request.getHeaders().put("foo", Arrays.asList("bar", "baz"));
@@ -290,65 +291,68 @@ void queryParamContainsWithMissingValue() throws Exception {
290291
@Test
291292
void queryParamListMissing() {
292293
assertThatAssertionError()
293-
.isThrownBy(() -> MockRestRequestMatchers.queryParam("foo", hasSize(2)).match(this.request))
294+
.isThrownBy(() -> MockRestRequestMatchers.queryParamList("foo", hasSize(2)).match(this.request))
294295
.withMessage("Expected query param <foo> to exist but was null");
295296
}
296297

297298
@Test
298299
void queryParamListMatchers() throws IOException {
299300
this.request.setURI(URI.create("http://www.foo.example/a?foo=bar&foo=baz"));
300301

301-
MockRestRequestMatchers.queryParam("foo", containsInAnyOrder(endsWith("baz"), endsWith("bar"))).match(this.request);
302-
MockRestRequestMatchers.queryParam("foo", contains(is("bar"), is("baz"))).match(this.request);
303-
MockRestRequestMatchers.queryParam("foo", contains(is("bar"), anything())).match(this.request);
304-
MockRestRequestMatchers.queryParam("foo", hasItem(endsWith("baz"))).match(this.request);
305-
MockRestRequestMatchers.queryParam("foo", everyItem(startsWith("ba"))).match(this.request);
306-
MockRestRequestMatchers.queryParam("foo", hasSize(2)).match(this.request);
307-
308-
// These can be a bit ambiguous when reading the test (the compiler selects the list matcher):
309-
MockRestRequestMatchers.queryParam("foo", notNullValue()).match(this.request);
310-
MockRestRequestMatchers.queryParam("foo", is(anything())).match(this.request);
311-
MockRestRequestMatchers.queryParam("foo", allOf(notNullValue(), notNullValue())).match(this.request);
312-
313-
// These are not as ambiguous thanks to an inner matcher that is either obviously list-oriented,
314-
// string-oriented, or obviously a vararg of matchers
315-
316-
// list matcher version
317-
MockRestRequestMatchers.queryParam("foo", allOf(notNullValue(), hasSize(2))).match(this.request);
318-
319-
// vararg version
320-
MockRestRequestMatchers.queryParam("foo", allOf(notNullValue(), endsWith("ar"))).match(this.request);
321-
MockRestRequestMatchers.queryParam("foo", is((any(String.class)))).match(this.request);
322-
MockRestRequestMatchers.queryParam("foo", either(is("bar")).or(is(nullValue()))).match(this.request);
323-
MockRestRequestMatchers.queryParam("foo", is(notNullValue()), is(notNullValue())).match(this.request);
302+
MockRestRequestMatchers.queryParamList("foo", containsInAnyOrder(endsWith("baz"), endsWith("bar"))).match(this.request);
303+
MockRestRequestMatchers.queryParamList("foo", contains(is("bar"), is("baz"))).match(this.request);
304+
MockRestRequestMatchers.queryParamList("foo", contains(is("bar"), anything())).match(this.request);
305+
MockRestRequestMatchers.queryParamList("foo", hasItem(endsWith("baz"))).match(this.request);
306+
MockRestRequestMatchers.queryParamList("foo", everyItem(startsWith("ba"))).match(this.request);
307+
MockRestRequestMatchers.queryParamList("foo", hasSize(2)).match(this.request);
308+
309+
MockRestRequestMatchers.queryParamList("foo", notNullValue()).match(this.request);
310+
MockRestRequestMatchers.queryParamList("foo", is(anything())).match(this.request);
311+
MockRestRequestMatchers.queryParamList("foo", allOf(notNullValue(), notNullValue())).match(this.request);
312+
313+
MockRestRequestMatchers.queryParamList("foo", allOf(notNullValue(), hasSize(2))).match(this.request);
324314
}
325315

326316
@Test
327317
void queryParamListContainsMismatch() {
328318
this.request.setURI(URI.create("http://www.foo.example/a?foo=bar&foo=baz"));
329319

330320
assertThatAssertionError()
331-
.isThrownBy(() -> MockRestRequestMatchers.queryParam("foo", contains(containsString("ba"))).match(this.request))
321+
.isThrownBy(() -> MockRestRequestMatchers.queryParamList("foo", contains(containsString("ba"))).match(this.request))
332322
.withMessageContainingAll(
333323
"Query param [foo] values",
334324
"Expected: iterable containing [a string containing \"ba\"]",
335325
"but: not matched: \"baz\"");
336326

337327
assertThatAssertionError()
338-
.isThrownBy(() -> MockRestRequestMatchers.queryParam("foo", hasItem(endsWith("ba"))).match(this.request))
328+
.isThrownBy(() -> MockRestRequestMatchers.queryParamList("foo", hasItem(endsWith("ba"))).match(this.request))
339329
.withMessageContainingAll(
340330
"Query param [foo] values",
341331
"Expected: a collection containing a string ending with \"ba\"",
342332
"but: mismatches were: [was \"bar\", was \"baz\"]");
343333

344334
assertThatAssertionError()
345-
.isThrownBy(() -> MockRestRequestMatchers.queryParam("foo", everyItem(endsWith("ar"))).match(this.request))
335+
.isThrownBy(() -> MockRestRequestMatchers.queryParamList("foo", everyItem(endsWith("ar"))).match(this.request))
346336
.withMessageContainingAll(
347337
"Query param [foo] values",
348338
"Expected: every item is a string ending with \"ar\"",
349339
"but: an item was \"baz\"");
350340
}
351341

342+
@Test
343+
void queryParamListDoesntHideQueryParamWithSingleMatcher() throws IOException {
344+
this.request.setURI(URI.create("http://www.foo.example/a?foo=bar&foo=baz"));
345+
346+
MockRestRequestMatchers.queryParam("foo", equalTo("bar")).match(this.request);
347+
348+
assertThatAssertionError()
349+
.isThrownBy(() -> MockRestRequestMatchers.queryParamList("foo", equalTo("bar")).match(this.request))
350+
.withMessageContainingAll(
351+
"Query param [foo] values",
352+
"Expected: \"bar\"",
353+
"but: was <[bar, baz]>");
354+
}
355+
352356
private static ThrowableTypeAssert<AssertionError> assertThatAssertionError() {
353357
return assertThatExceptionOfType(AssertionError.class);
354358
}

0 commit comments

Comments
 (0)