Skip to content

Commit 95883b9

Browse files
simonbaslesimonbasle
authored
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. Closes gh-30220 Closes gh-30238 See gh-29953 See gh-28660
1 parent f0eb43a commit 95883b9

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
@@ -28,19 +28,17 @@
2828

2929
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
3030
import static org.hamcrest.Matchers.allOf;
31-
import static org.hamcrest.Matchers.any;
3231
import static org.hamcrest.Matchers.anything;
3332
import static org.hamcrest.Matchers.contains;
3433
import static org.hamcrest.Matchers.containsInAnyOrder;
3534
import static org.hamcrest.Matchers.containsString;
36-
import static org.hamcrest.Matchers.either;
3735
import static org.hamcrest.Matchers.endsWith;
36+
import static org.hamcrest.Matchers.equalTo;
3837
import static org.hamcrest.Matchers.everyItem;
3938
import static org.hamcrest.Matchers.hasItem;
4039
import static org.hamcrest.Matchers.hasSize;
4140
import static org.hamcrest.Matchers.is;
4241
import static org.hamcrest.Matchers.notNullValue;
43-
import static org.hamcrest.Matchers.nullValue;
4442
import static org.hamcrest.Matchers.startsWith;
4543

4644
/**
@@ -163,65 +161,68 @@ void headerContainsWithMissingValue() {
163161
@Test
164162
void headerListMissing() {
165163
assertThatAssertionError()
166-
.isThrownBy(() -> MockRestRequestMatchers.header("foo", hasSize(2)).match(this.request))
164+
.isThrownBy(() -> MockRestRequestMatchers.headerList("foo", hasSize(2)).match(this.request))
167165
.withMessage("Expected header <foo> to exist but was null");
168166
}
169167

170168
@Test
171169
void headerListMatchers() throws IOException {
172170
this.request.getHeaders().put("foo", List.of("bar", "baz"));
173171

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

199186
@Test
200187
void headerListContainsMismatch() {
201188
this.request.getHeaders().put("foo", List.of("bar", "baz"));
202189

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

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

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

212+
@Test
213+
void headerListDoesntHideHeaderWithSingleMatcher() throws IOException {
214+
this.request.getHeaders().put("foo", List.of("bar", "baz"));
215+
216+
MockRestRequestMatchers.header("foo", equalTo("bar")).match(this.request);
217+
218+
assertThatAssertionError()
219+
.isThrownBy(() -> MockRestRequestMatchers.headerList("foo", equalTo("bar")).match(this.request))
220+
.withMessageContainingAll(
221+
"Request header [foo] values",
222+
"Expected: \"bar\"",
223+
"but: was <[bar, baz]>");
224+
}
225+
225226
@Test
226227
void headers() throws Exception {
227228
this.request.getHeaders().put("foo", List.of("bar", "baz"));
@@ -290,65 +291,68 @@ void queryParamContainsWithMissingValue() {
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)