Skip to content

Commit

Permalink
Rename MockMVC matcher methods to prevent regression in user tests
Browse files Browse the repository at this point in the history 
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 spring-projectsgh-30220
Close spring-projectsgh-30238
See spring-projectsgh-29953
See spring-projectsgh-28660
  • Loading branch information
@simonbasle
simonbasle committed Mar 30, 2023
1 parent b374824 commit a44bc02
Show file tree
Hide file tree
Showing 2 changed files with 73 additions and 66 deletions.
24 changes: 12 additions & 12 deletions ...test/src/main/java/org/springframework/test/web/client/match/MockRestRequestMatchers.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,11 +125,11 @@ public static RequestMatcher requestTo(URI uri) {
* @param name the name of the query parameter whose value(s) will be asserted
* @param matcher the Hamcrest matcher to apply to the entire list of values
* for the given query parameter
* @since 5.3.26
* @since 5.3.27
* @see #queryParam(String, Matcher...)
* @see #queryParam(String, String...)
*/
public static RequestMatcher queryParam(String name, Matcher<? super List<String>> matcher) {
public static RequestMatcher queryParamList(String name, Matcher<? super List<String>> matcher) {
return request -> {
MultiValueMap<String, String> params = getQueryParams(request);
List<String> paramValues = params.get(name);
Expand All @@ -147,14 +147,14 @@ public static RequestMatcher queryParam(String name, Matcher<? super List<String
* values, effectively ignoring the additional parameter values. If the number
* of provided {@code matchers} exceeds the number of query parameter values,
* an {@link AssertionError} will be thrown to signal the mismatch.
* <p>See {@link #queryParam(String, Matcher)} for a variant which accepts a
* <p>See {@link #queryParamList(String, Matcher)} for a variant which accepts a
* {@code Matcher} that applies to the entire list of values as opposed to
* applying only to individual values.
* @param name the name of the query parameter whose value(s) will be asserted
* @param matchers the Hamcrest matchers to apply to individual query parameter
* values; the n<sup>th</sup> matcher is applied to the n<sup>th</sup> query
* parameter value
* @see #queryParam(String, Matcher)
* @see #queryParamList(String, Matcher)
* @see #queryParam(String, String...)
*/
@SafeVarargs
Expand All @@ -175,14 +175,14 @@ public static RequestMatcher queryParam(String name, Matcher<? super String>...
* parameter values, effectively ignoring the additional parameter values. If
* the number of {@code expectedValues} exceeds the number of query parameter
* values, an {@link AssertionError} will be thrown to signal the mismatch.
* <p>See {@link #queryParam(String, Matcher)} for a variant which accepts a
* <p>See {@link #queryParamList(String, Matcher)} for a variant which accepts a
* Hamcrest {@code Matcher} that applies to the entire list of values as opposed
* to asserting only individual values.
* @param name the name of the query parameter whose value(s) will be asserted
* @param expectedValues the expected values of individual query parameter values;
* the n<sup>th</sup> expected value is compared to the n<sup>th</sup> query
* parameter value
* @see #queryParam(String, Matcher)
* @see #queryParamList(String, Matcher)
* @see #queryParam(String, Matcher...)
*/
public static RequestMatcher queryParam(String name, String... expectedValues) {
Expand Down Expand Up @@ -211,11 +211,11 @@ private static MultiValueMap<String, String> getQueryParams(ClientHttpRequest re
* @param name the name of the header whose value(s) will be asserted
* @param matcher the Hamcrest matcher to apply to the entire list of values
* for the given header
* @since 5.3.26
* @since 5.3.27
* @see #header(String, Matcher...)
* @see #header(String, String...)
*/
public static RequestMatcher header(String name, Matcher<? super List<String>> matcher) {
public static RequestMatcher headerList(String name, Matcher<? super List<String>> matcher) {
return request -> {
List<String> headerValues = request.getHeaders().get(name);
if (headerValues == null) {
Expand All @@ -232,13 +232,13 @@ public static RequestMatcher header(String name, Matcher<? super List<String>> m
* effectively ignoring the additional header values. If the number of
* provided {@code matchers} exceeds the number of header values, an
* {@link AssertionError} will be thrown to signal the mismatch.
* <p>See {@link #header(String, Matcher)} for a variant which accepts a
* <p>See {@link #headerList(String, Matcher)} for a variant which accepts a
* Hamcrest {@code Matcher} that applies to the entire list of values as
* opposed to applying only to individual values.
* @param name the name of the header whose value(s) will be asserted
* @param matchers the Hamcrest matchers to apply to individual header values;
* the n<sup>th</sup> matcher is applied to the n<sup>th</sup> header value
* @see #header(String, Matcher)
* @see #headerList(String, Matcher)
* @see #header(String, String...)
*/
@SafeVarargs
Expand All @@ -260,13 +260,13 @@ public static RequestMatcher header(String name, Matcher<? super String>... matc
* additional header values. If the number of {@code expectedValues} exceeds the
* number of header values, an {@link AssertionError} will be thrown to signal the
* mismatch.
* <p>See {@link #header(String, Matcher)} for a variant which accepts a
* <p>See {@link #headerList(String, Matcher)} for a variant which accepts a
* Hamcrest {@code Matcher} that applies to the entire list of values as
* opposed to applying only to individual values.
* @param name the name of the header whose value(s) will be asserted
* @param expectedValues the expected values of individual header values; the
* n<sup>th</sup> expected value is compared to the n<sup>th</sup> header value
* @see #header(String, Matcher)
* @see #headerList(String, Matcher)
* @see #header(String, Matcher...)
*/
public static RequestMatcher header(String name, String... expectedValues) {
Expand Down
115 changes: 61 additions & 54 deletions ...src/test/java/org/springframework/test/web/client/match/MockRestRequestMatchersTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.either;
import static org.hamcrest.Matchers.endsWith;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.everyItem;
import static org.hamcrest.Matchers.hasItem;
import static org.hamcrest.Matchers.hasSize;
Expand Down Expand Up @@ -163,65 +164,68 @@ void headerContainsWithMissingValue() {
@Test
void headerListMissing() {
assertThatAssertionError()
.isThrownBy(() -> MockRestRequestMatchers.header("foo", hasSize(2)).match(this.request))
.isThrownBy(() -> MockRestRequestMatchers.headerList("foo", hasSize(2)).match(this.request))
.withMessage("Expected header <foo> to exist but was null");
}

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

MockRestRequestMatchers.header("foo", containsInAnyOrder(endsWith("baz"), endsWith("bar"))).match(this.request);
MockRestRequestMatchers.header("foo", contains(is("bar"), is("baz"))).match(this.request);
MockRestRequestMatchers.header("foo", contains(is("bar"), anything())).match(this.request);
MockRestRequestMatchers.header("foo", hasItem(endsWith("baz"))).match(this.request);
MockRestRequestMatchers.header("foo", everyItem(startsWith("ba"))).match(this.request);
MockRestRequestMatchers.header("foo", hasSize(2)).match(this.request);

// These can be a bit ambiguous when reading the test (the compiler selects the list matcher):
MockRestRequestMatchers.header("foo", notNullValue()).match(this.request);
MockRestRequestMatchers.header("foo", is(anything())).match(this.request);
MockRestRequestMatchers.header("foo", allOf(notNullValue(), notNullValue())).match(this.request);

// These are not as ambiguous thanks to an inner matcher that is either obviously list-oriented,
// string-oriented, or obviously a vararg of matchers

// list matcher version
MockRestRequestMatchers.header("foo", allOf(notNullValue(), hasSize(2))).match(this.request);

// vararg version
MockRestRequestMatchers.header("foo", allOf(notNullValue(), endsWith("ar"))).match(this.request);
MockRestRequestMatchers.header("foo", is((any(String.class)))).match(this.request);
MockRestRequestMatchers.header("foo", either(is("bar")).or(is(nullValue()))).match(this.request);
MockRestRequestMatchers.header("foo", is(notNullValue()), is(notNullValue())).match(this.request);
MockRestRequestMatchers.headerList("foo", containsInAnyOrder(endsWith("baz"), endsWith("bar"))).match(this.request);
MockRestRequestMatchers.headerList("foo", contains(is("bar"), is("baz"))).match(this.request);
MockRestRequestMatchers.headerList("foo", contains(is("bar"), anything())).match(this.request);
MockRestRequestMatchers.headerList("foo", hasItem(endsWith("baz"))).match(this.request);
MockRestRequestMatchers.headerList("foo", everyItem(startsWith("ba"))).match(this.request);
MockRestRequestMatchers.headerList("foo", hasSize(2)).match(this.request);

MockRestRequestMatchers.headerList("foo", notNullValue()).match(this.request);
MockRestRequestMatchers.headerList("foo", is(anything())).match(this.request);
MockRestRequestMatchers.headerList("foo", allOf(notNullValue(), notNullValue())).match(this.request);

MockRestRequestMatchers.headerList("foo", allOf(notNullValue(), hasSize(2))).match(this.request);
}

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

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

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

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

@Test
void headerListDoesntHideHeaderWithSingleMatcher() throws IOException {
this.request.getHeaders().put("foo", List.of("bar", "baz"));

MockRestRequestMatchers.header("foo", equalTo("bar")).match(this.request);

assertThatAssertionError()
.isThrownBy(() -> MockRestRequestMatchers.headerList("foo", equalTo("bar")).match(this.request))
.withMessageContainingAll(
"Request header [foo] values",
"Expected: \"bar\"",
"but: was <[bar, baz]>");
}

@Test
void headers() throws Exception {
this.request.getHeaders().put("foo", List.of("bar", "baz"));
Expand Down Expand Up @@ -290,65 +294,68 @@ void queryParamContainsWithMissingValue() {
@Test
void queryParamListMissing() {
assertThatAssertionError()
.isThrownBy(() -> MockRestRequestMatchers.queryParam("foo", hasSize(2)).match(this.request))
.isThrownBy(() -> MockRestRequestMatchers.queryParamList("foo", hasSize(2)).match(this.request))
.withMessage("Expected query param <foo> to exist but was null");
}

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

MockRestRequestMatchers.queryParam("foo", containsInAnyOrder(endsWith("baz"), endsWith("bar"))).match(this.request);
MockRestRequestMatchers.queryParam("foo", contains(is("bar"), is("baz"))).match(this.request);
MockRestRequestMatchers.queryParam("foo", contains(is("bar"), anything())).match(this.request);
MockRestRequestMatchers.queryParam("foo", hasItem(endsWith("baz"))).match(this.request);
MockRestRequestMatchers.queryParam("foo", everyItem(startsWith("ba"))).match(this.request);
MockRestRequestMatchers.queryParam("foo", hasSize(2)).match(this.request);

// These can be a bit ambiguous when reading the test (the compiler selects the list matcher):
MockRestRequestMatchers.queryParam("foo", notNullValue()).match(this.request);
MockRestRequestMatchers.queryParam("foo", is(anything())).match(this.request);
MockRestRequestMatchers.queryParam("foo", allOf(notNullValue(), notNullValue())).match(this.request);

// These are not as ambiguous thanks to an inner matcher that is either obviously list-oriented,
// string-oriented, or obviously a vararg of matchers

// list matcher version
MockRestRequestMatchers.queryParam("foo", allOf(notNullValue(), hasSize(2))).match(this.request);

// vararg version
MockRestRequestMatchers.queryParam("foo", allOf(notNullValue(), endsWith("ar"))).match(this.request);
MockRestRequestMatchers.queryParam("foo", is((any(String.class)))).match(this.request);
MockRestRequestMatchers.queryParam("foo", either(is("bar")).or(is(nullValue()))).match(this.request);
MockRestRequestMatchers.queryParam("foo", is(notNullValue()), is(notNullValue())).match(this.request);
MockRestRequestMatchers.queryParamList("foo", containsInAnyOrder(endsWith("baz"), endsWith("bar"))).match(this.request);
MockRestRequestMatchers.queryParamList("foo", contains(is("bar"), is("baz"))).match(this.request);
MockRestRequestMatchers.queryParamList("foo", contains(is("bar"), anything())).match(this.request);
MockRestRequestMatchers.queryParamList("foo", hasItem(endsWith("baz"))).match(this.request);
MockRestRequestMatchers.queryParamList("foo", everyItem(startsWith("ba"))).match(this.request);
MockRestRequestMatchers.queryParamList("foo", hasSize(2)).match(this.request);

MockRestRequestMatchers.queryParamList("foo", notNullValue()).match(this.request);
MockRestRequestMatchers.queryParamList("foo", is(anything())).match(this.request);
MockRestRequestMatchers.queryParamList("foo", allOf(notNullValue(), notNullValue())).match(this.request);

MockRestRequestMatchers.queryParamList("foo", allOf(notNullValue(), hasSize(2))).match(this.request);
}

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

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

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

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

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

MockRestRequestMatchers.queryParam("foo", equalTo("bar")).match(this.request);

assertThatAssertionError()
.isThrownBy(() -> MockRestRequestMatchers.queryParamList("foo", equalTo("bar")).match(this.request))
.withMessageContainingAll(
"Query param [foo] values",
"Expected: \"bar\"",
"but: was <[bar, baz]>");
}

private static ThrowableTypeAssert<AssertionError> assertThatAssertionError() {
return assertThatExceptionOfType(AssertionError.class);
}
Expand Down

0 comments on commit a44bc02

Please sign in to comment.