Spring Add support in MockRestServiceServer to assert ALL header and queryParam values with a single Matcher

Affects: 5.3.21

The current API for MockRestServiceServer and MockRestRequestMatchers does not allow a test to validate that all values of a certain header key satisfy a matcher. Trying to do

.andExpect(header("Accept", endsWith("json")))

does not work because it passes if the first value for the header ends with "json". I would like to be able to do something like this:

.andExpect(header("Accept", allMatch(endsWith("json"))))

or something to that effect.

Comment From: sbrannen

Hi @mspiess,

I would like to be able to do something like this:

java .andExpect(header("Accept", allMatch(endsWith("json"))))

or something to that effect.

That's a good point. In fact, it almost appears that the current API would support that. At least, the current tests in place are based on that assumption.

https://github.com/spring-projects/spring-framework/blob/93b340e5633d623e0899dd296bda7ce86ce02b20/spring-test/src/test/java/org/springframework/test/web/client/match/MockRestRequestMatchersTests.java#L126-L131

That headerContains() test actually does not assert what it appears to.

Since bar and baz both contain ba, the test appears to assert that both bar and baz are checked, but in fact -- as you've pointed out -- only bar is checked.

In other words, rewriting the assertion as follows would also cause the test to pass.

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

In order to perform an assertion on each header value, you have to provide a matcher for each value. Thus, the following fails:

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

Whereas, the following pass:

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

MockRestRequestMatchers.header("foo", containsString("ba"), containsString("ba")).match(this.request);

Thus, there is in fact support for performing assertions against all of the values for a given header, but it is awkward to use and not really documented.

Furthermore, with the current implementation of this feature you have to know exactly how many values are expected and you have to repeat the Matcher or expected value.

FYI: this applies to all variants of queryParam(...) and header(...).

One way to address this would be to introduce new variants of these methods that accept a single String or Matcher (instead of var-args) such as the following.

public static RequestMatcher header(String name, Matcher<? super String> matcher) {
    return request -> {
        List<String> headerValues = request.getHeaders().get(name);
        assertNotNull("No header values", headerValues);
        String reason = "Request header [" + name + "]";
        headerValues.forEach(value -> assertThat(reason, value, matcher));
    };
}

This would achieve what you are requesting; however, there is a downside: any invocation of header() or queryParam() with a single argument for the var-args array would now invoke this new method when recompiled, which would be a breaking change. To avoid that, we could come up with a new name for the single-argument variants.

Comment From: mspiess

Hi @sbrannen, thanks for the quick response and the nice summary.

Perhaps a new overload would be a less of a breaking change:

public static RequestMatcher header(String name, Matcher<? super Collection<? super String>> matcher) {
    return request -> {
        List<String> headerValues = request.getHeaders().get(name);
        assertThat("Request header [" + name + "]", headerValues, matcher);
    };
}

This would allow the following:

MockRestRequestMatchers.header("foo", everyItem(containsString("ba"))).match(this.request);

This brings more control to the call site, as the caller can decide how to match the collection. E.g. one may want to assert that any value matches, which is not possible currently without knowing the amount and order of the header values. Although I have to acknowledge that this is a strong difference in behavior for an overload and may cause some confusion.

To avoid that, we could come up with a new name for the single-argument variants.

I suggest headerValues.

Comment From: rstoyanchev

When it comes to headers, I don't think the expectation that you would know how many and match each individually is all that unreasonable. I'd expect that matching across all headers in one condition is probably a less common case.

The Javadoc on the existing methods could certainly use an improvement, even if it's obvious that a vararg means, one value or Matcher for each value.

Nothing wrong with introducing a variant that accepts a Matcher<? super Collection<? super String>> but I wouldn't call it headerValues since the other one is also capable of matching all header values. Perhaps headerList would be more explicit about the difference.

Comment From: sbrannen

I'd expect that matching across all headers in one condition is probably a less common case.

I agree.

The Javadoc on the existing methods could certainly use an improvement

Yes, let's improve the documentation for the affected methods.

Nothing wrong with introducing a variant that accepts a Matcher<? super Collection<? super String>>

I did some research on what's available in Hamcrest. Instead of Collection, they use Iterable. So let's go with Iterable for compatibility.

The interesting (disappointing?) part is that everyItem() and hasItem() have different generic signatures. everyItem() produces Matcher<Iterable<? extends U>>; whereas, hasItem() produces Matcher<Iterable<? super T>>.

This makes everyItem() incompatible with the proposed new method in MockRestRequestMatchers, but there's nothing we can do about that since it's baked into Hamcrest like that.

Though you can create a custom everyItem() implementation that ignores the generics as follows, and this will work with the new method in MockRestRequestMatchers.

@SuppressWarnings({ "rawtypes", "unchecked" })
public static <U> Matcher<Iterable<? super U>> everyItem(Matcher<U> itemMatcher) {
    return new Every(itemMatcher);
}

but I wouldn't call it headerValues since the other one is also capable of matching all header values. Perhaps headerList would be more explicit about the difference.

With the proposed signature below, we don't actually run into any issues with the compiler (in terms of source compatibility).

public static RequestMatcher header(String name, Matcher<Iterable<? super String>> matcher)

So we could choose to keep the header method name, but perhaps a different name would help to highlight the difference in behavior (and avoid binary incompatibility).

Comment From: rstoyanchev

Keeping the header method name sounds good.

Comment From: sbrannen

The interesting (disappointing?) part is that everyItem() and hasItem() have different generic signatures. everyItem() produces Matcher<Iterable<? extends U>>; whereas, hasItem() produces Matcher<Iterable<? super T>>.

It turns out that there are open issues for Hamcrest on this topic.

See https://github.com/hamcrest/JavaHamcrest/issues/289#issuecomment-1162016379 for details.

Comment From: rstoyanchev

@sbrannen, do we go with your most recent proposal?

public static RequestMatcher header(String name, Matcher<Iterable<? super String>> matcher)

Its usefulness is reduced by the Hamcrest issue, however, that doesn't seem to be moving. Perhaps we can just go back to the earlier suggestion with a slightly different name:

public static RequestMatcher everyHeader(String name, Matcher<? super String> matcher);

Comment From: sbrannen

@sbrannen, do we go with your most recent proposal?

java public static RequestMatcher header(String name, Matcher<Iterable<? super String>> matcher)

Yes, I believe that's the route we need to go.

Its usefulness is reduced by the Hamcrest issue, however, that doesn't seem to be moving.

Indeed, unfortunately.

Perhaps we can just go back to the earlier suggestion with a slightly different name:

java public static RequestMatcher everyHeader(String name, Matcher<? super String> matcher);

I think this is too limiting. The same matcher would be applied to each queryParam/header value individually instead of acting on the collection as a whole -- effectively the same semantics as Matchers.everyItem().

In other words, if we go this route users would not be able to benefit from features like Matchers.hasItem().

Of course, if we wanted to provide a workaround for the generics issues in Hamcrest... we could introduce both methods. However, I'm not convinced we should do that; as I mentioned in https://github.com/spring-projects/spring-framework/issues/28660#issuecomment-1161839634, users can come up with their own workarounds for the generics issues in Hamcrest.

@simonbasle has expressed in interest in taking on this issue, so I've assigned it to him for tentative inclusion in 6.0.5.

Please note that I changed the title of this issue to reflect a changed focus on both header() and queryParam() from MockRestRequestMatchers.

Comment From: rstoyanchev

Of course, if we wanted to provide a workaround for the generics issues in Hamcrest... we could introduce both methods. However, I'm not convinced we should do that; as I mentioned in https://github.com/spring-projects/spring-framework/issues/28660#issuecomment-1161839634, users can come up with their own workarounds for the generics issues in Hamcrest.

Very much worth explaining and illustrating this in the Javadoc of such a new method.

Comment From: simonbasle

I'm having a look and I need to better understand why, but it appears that the following signature works both for hasItem and everyItem (among others):

public static RequestMatcher header(String name, Matcher<? super List<String>> matcher) {

See test below demonstrating usage with various matchers

@Test
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"), Matchers.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", CoreMatchers.either(is("bar")).or(is(nullValue()))).match(this.request);
    MockRestRequestMatchers.header("foo", is(notNullValue()), is(notNullValue()));
}

Comment From: simonbasle

Superseded by gh-29953

That PR will allow to do something like this:

.andExpect(header("Accept", everyItem(endsWith("json"))))