Affects: 2.7.10

Hello everyone.

I'm uncertain if this is an issue with spring-test or with Java compiler itself but, from Spring Boot 2.7.10 (which incorporates spring-test 5.3.26), we have been having an odd issue with tests calling the wrong MockRestRequestMatchers.header() method.

In spring-test 5.3.25 there were a couple of header() methods, but the one that interest us is the one with this signature:

public static RequestMatcher header(String name, Matcher<? super String>... matchers)

this was used in testing like this:

.andExpect(header("X-MY-HEADER", equalTo("myValue")))

where equalTo() is defined in Hamcrest as

public static <T> Matcher<T> equalTo(T operand)

this worked normally.

From Spring Boot 2.7.10 (spring-test 5.3.26) a new header() method has been added with this signature:

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

and this is where things are breaking: the same call above is now entering this new method, as opposed to the previous. I find this surprising because the signature doesn't match, equalTo("myValue") returns Matcher<String>, which doesn't align to Matcher<? super List<String>>, and yet it's coming in. If I change the call to header() to explicitly call out the type like this:

.andExpect(header("X-MY-HEADER", CoreMatchers.<String>equalTo("myValue")))

then things go back to normal and the call is again directed to the old header() method.

I'm guessing that, via type erasure, the signatures of the two header() methods are being simplified to just header(String, Matcher) and, there being two possible candidates that can match the call, Java is just picking the first one. I'm not sure what could be done from spring-framework's perspective, though.

Thoughts?

Comment From: simonbasle

That's a bit unfortunate indeed 😞 When these "base" Matchers that can conceptually apply to either String or List will cause the compiler to select the header(String, Matcher<List>) signature. I unfortunately didn't realize that equalTo(T) would fall in this category.

To be precise, this is because the signature is really:

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

So, to the compiler, CoreMatchers.equalTo("myValue") fits that signature as a Matcher<Object> 😮‍💨

Why was the new variant introduced?

See initial context in #28660.

Note that the vararg based signature, which takes Matcher<String>, has a blind spot: if you provide N matchers, it only validates that the first N values in the header values list do match. This means that in your example the call would match even if the header values was in reality [ "myValue", "otherValue" ]... The intent of introducing a variant which accepts a List-wide Matcher was to alleviate this limitation and enable usage of collection matchers like: - contains - hasItems - everyItem - hasSize - empty

The signature has to use ? super List because the Hamcrest collection matchers are inconsistent in the generic wildcards they use 😞

A better workaround

Your workaround of calling CoreMatchers.<String>equalTo("myValue") does unambiguously select the aforementioned string-based signature, but a better workaround would be to use:

.andExpect(header("X-MY-HEADER", 
    Machers.contains("myValue")
   //equivalent to Matchers.contains(CoreMatchers.equalTo("myValue"))
));

This is unambiguously a Matcher<Iterable<String>> AND it will validate that there are no more than one header value on top of checking for equality.

Can this be fixed in Framework?

I have toyed with modifying the list-based overload implementation to try to detect such "unexpected" matchers and apply the passed Matcher directly to the first element of the List in addition to the whole List itself, but I think that is a slippery slope of trying to be too smart.

This would result in the following puzzling statements all passing in tests:

        request.getHeaders().put("twoElements", List.of("a", "b"));

        MockRestRequestMatchers.header("twoElements", Matchers.equalTo("a")).match(this.request);
        MockRestRequestMatchers.header("twoElements", Matchers.equalTo(List.of("a", "b"))).match(this.request);

        MockRestRequestMatchers.header("twoElements", Matchers.hasToString("[a, b]")).match(this.request);
        MockRestRequestMatchers.header("twoElements", Matchers.hasToString("a")).match(this.request);

This could also short-circuit null checks (although this example is a bit contrived, nonetheless null is an acceptable value for a HttpHeaders...):

        request.getHeaders().add("m", null);
        request.getHeaders().add("m", "b");

        MockRestRequestMatchers.header("m", Matchers.notNullValue()).match(this.request);
        MockRestRequestMatchers.header("m", Matchers.nullValue()).match(this.request);

(it passes because the List is not null, even though the first element of the list is null)

Comment From: simonbasle

This can be considered a form of regression and the feature is recent enough that we might consider hot-fixing the API, renaming the list-based variant to something unambiguously different like headerList. Note that a similar change is necessary for queryParam which had the same new API.

Comment From: quiram

Hi @simonbasle.

Thanks a lot for the detailed explanation, that is very informative: I had missed the possibility that type inference could use Matcher<Object> as the type for equalTo(String), but now that you mention it it makes perfect sense.

I can see that you've opted to rename the new methods to add the *List suffix, I agree that this can be useful for the wider community (there may be many people scratching their heads right now). On the other hand, I really like your suggestion to use contains() as opposed to equalsTo(), it is a stricter assertion (in our case our headers usually only have one value but you never know!); I think we will adopt it anyway (using the new *List() variant as it becomes available).

Once again, thanks for addressing this so quickly.