From 0cfbf6036c485d8520c70f79f3896d0fc9ada164 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Basl=C3=A9?= Date: Tue, 4 Apr 2023 15:06:14 +0200 Subject: [PATCH] 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`. 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 --- .../client/match/MockRestRequestMatchers.java | 24 ++-- .../match/MockRestRequestMatchersTests.java | 118 +++++++++--------- 2 files changed, 73 insertions(+), 69 deletions(-) diff --git a/spring-test/src/main/java/org/springframework/test/web/client/match/MockRestRequestMatchers.java b/spring-test/src/main/java/org/springframework/test/web/client/match/MockRestRequestMatchers.java index cf0d39b17eb1..efa97c27194c 100644 --- a/spring-test/src/main/java/org/springframework/test/web/client/match/MockRestRequestMatchers.java +++ b/spring-test/src/main/java/org/springframework/test/web/client/match/MockRestRequestMatchers.java @@ -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> matcher) { + public static RequestMatcher queryParamList(String name, Matcher> matcher) { return request -> { MultiValueMap params = getQueryParams(request); List paramValues = params.get(name); @@ -147,14 +147,14 @@ public static RequestMatcher queryParam(String name, MatcherSee {@link #queryParam(String, Matcher)} for a variant which accepts a + *

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 nth matcher is applied to the nth query * parameter value - * @see #queryParam(String, Matcher) + * @see #queryParamList(String, Matcher) * @see #queryParam(String, String...) */ @SafeVarargs @@ -175,14 +175,14 @@ public static RequestMatcher queryParam(String name, Matcher... * 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. - *

See {@link #queryParam(String, Matcher)} for a variant which accepts a + *

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 nth expected value is compared to the nth query * parameter value - * @see #queryParam(String, Matcher) + * @see #queryParamList(String, Matcher) * @see #queryParam(String, Matcher...) */ public static RequestMatcher queryParam(String name, String... expectedValues) { @@ -211,11 +211,11 @@ private static MultiValueMap 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> matcher) { + public static RequestMatcher headerList(String name, Matcher> matcher) { return request -> { List headerValues = request.getHeaders().get(name); if (headerValues == null) { @@ -232,13 +232,13 @@ public static RequestMatcher header(String name, Matcher> 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. - *

See {@link #header(String, Matcher)} for a variant which accepts a + *

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 nth matcher is applied to the nth header value - * @see #header(String, Matcher) + * @see #headerList(String, Matcher) * @see #header(String, String...) */ @SafeVarargs @@ -260,13 +260,13 @@ public static RequestMatcher header(String name, Matcher... 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. - *

See {@link #header(String, Matcher)} for a variant which accepts a + *

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 * nth expected value is compared to the nth header value - * @see #header(String, Matcher) + * @see #headerList(String, Matcher) * @see #header(String, Matcher...) */ public static RequestMatcher header(String name, String... expectedValues) { diff --git a/spring-test/src/test/java/org/springframework/test/web/client/match/MockRestRequestMatchersTests.java b/spring-test/src/test/java/org/springframework/test/web/client/match/MockRestRequestMatchersTests.java index 4be95efe0c94..422644c10c81 100644 --- a/spring-test/src/test/java/org/springframework/test/web/client/match/MockRestRequestMatchersTests.java +++ b/spring-test/src/test/java/org/springframework/test/web/client/match/MockRestRequestMatchersTests.java @@ -29,19 +29,17 @@ import static org.assertj.core.api.Assertions.assertThatExceptionOfType; import static org.hamcrest.Matchers.allOf; -import static org.hamcrest.Matchers.any; import static org.hamcrest.Matchers.anything; import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.containsInAnyOrder; 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; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.notNullValue; -import static org.hamcrest.Matchers.nullValue; import static org.hamcrest.Matchers.startsWith; /** @@ -164,7 +162,7 @@ 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 to exist but was null"); } @@ -172,29 +170,18 @@ void headerListMissing() { void headerListMatchers() throws IOException { this.request.getHeaders().put("foo", Arrays.asList("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 @@ -202,27 +189,41 @@ void headerListContainsMismatch() { this.request.getHeaders().put("foo", Arrays.asList("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", Arrays.asList("bar", "baz")); @@ -290,7 +291,7 @@ void queryParamContainsWithMissingValue() throws Exception { @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 to exist but was null"); } @@ -298,29 +299,18 @@ void queryParamListMissing() { 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 @@ -328,27 +318,41 @@ 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 assertThatAssertionError() { return assertThatExceptionOfType(AssertionError.class); }