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

[mspiess]

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.

[sbrannen]

Hi @mspiess,

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

.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.

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

Lines 126 to 131 in 93b340e

	@Test
	public void headerContains() throws Exception {
	this.request.getHeaders().put("foo", Arrays.asList("bar", "baz"));
	MockRestRequestMatchers.header("foo", containsString("ba")).match(this.request);
	}

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.

[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.

[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.

[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).

[rstoyanchev]

Keeping the header method name sounds good.

[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 hamcrest/JavaHamcrest#289 (comment) for details.

[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);

[sbrannen]

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

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:

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 #28660 (comment), 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.

[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 #28660 (comment), 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.

[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()));
}

[simonbasle]

Superseded by gh-29953

That PR will allow to do something like this:

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