MockMvc request builders should expose HttpHeaders

[mieseprem]

Does it fix #35576 ?

If that's the case: here you go — enjoy

[rstoyanchev]

The MultiValueMap<String, Object> matches to how MockHttpServletRequest manages header values, allowing Collections, arrays, as well as date and number values, which it then formats when headers are obtained. So this change is unlikely to go without causing regressions.

The code in this method could match the behavior of getDateHeader and getIntHeader of MockHttpServletRequest, in reverse by formatting to String values in the same way, to ensure backwards compatibility.

[mieseprem]

To avoid heading in the wrong direction, I’ll briefly confirm what I’ve understood:

Working internally with HttpHeaders isn’t the best idea because of potential regressions. It’s better to continue using MultiValueMap internally.

So, if we still want to offer a method headers(Consumer<HttpHeaders>), we should consume the HttpHeaders and then call the existing headers(HttpHeaders) method. I can change it in that direction.

[sbrannen]

So, if we still want to offer a method headers(Consumer<HttpHeaders>), we should consume the HttpHeaders and then call the existing headers(HttpHeaders) method.

That unfortunately does not work.

If you create a new instance of HttpHeaders and provide that to the Consumer<HttpHeaders>, the consumer can only add new headers. It cannot remove or process existing headers.

[mieseprem]

If you create a new instance of HttpHeaders and provide that to the Consumer<HttpHeaders>, the consumer can only add new headers. It cannot remove or process existing headers

Ah, I think I’ve got it now. Yes — my approach indeed won’t work.

The challenge is "extracting" the HttpHeaders from the Consumer without first converting the existing MultiValueMap to HttpHeaders and then converting back after consumption. That seems cumbersome. I’ll think about possible solutions.

[mieseprem]

Do you think there’s any sensible way to implement this? As long as the internal headers map is a MultiValueMap<String, Object>, I can’t see how to make it compatible with the MultiValueMap<String, String> used by HttpHeaders. I can’t simply change the value type of objects already placed into the builder’s headers map into their string representations — that would almost certainly cause regressions.

For a moment I considered an implementation along these lines, but that was based on the incorrect assumption that the Consumer would only contain HttpHeaders instructions for purely String‑based header values (to be clear, this code produces the wrong result):

public B headers(Consumer<HttpHeaders> headersConsumer){
	HttpHeaders intermediateHeaders = new HttpHeaders();
	
	var stringHeaders = this.headers.entrySet().stream().
		filter(entry -> entry.getValue().stream().allMatch(obj -> obj instanceof String))
		.peek(entry -> intermediateHeaders.addAll(entry.getKey(), entry.getValue().stream().map(String.class::cast).toList()))
		.map(Map.Entry::getKey)
		.toList();
	stringHeaders.forEach(this.headers::remove);

	headersConsumer.accept(intermediateHeaders);
	return headers(intermediateHeaders);
}

If I'm not (again) totally wrong, this should be a test that need to pass:

@Test
void headersConsumer() {
	this.builder.header("X-Foo-String", "bar");
	this.builder.header("X-Foo-Date", LocalDate.now());
	this.builder.header("X-Foo-List-Int", List.of(1, 2, 3));

	this.builder.headers(httpHeaders -> {
		httpHeaders.put("X-Baz", Arrays.asList("qux", "quux"));
		httpHeaders.remove("X-Foo-Date");
	});

	MockHttpServletRequest request = this.builder.buildRequest(this.servletContext);
	List<String> headerNames = Collections.list(request.getHeaderNames());

	assertThat(headerNames).containsExactly("X-Foo-String", "X-Foo-List-Int", "X-Baz");
}

I think for this to work at all, roughly the following would need to happen:

• create from each MultiValueMap<String, Object> entry a MultiValueMap<String, String> one (this step alone seems very complex — I’m thinking of your point about getDateHeader(String) and getIntHeader(String))
• it’s easy to create an HttpHeaders object from a MultiValueMap<String, String> (call that X)
• consume the Consumer with the created object X
• afterwards, add, remove or modify entries in the original headers map (we must not copy back untouched entries that we converted to String in X)

Either I’m overthinking this, I’ve gone completely off track, or I’m perhaps not the right person to implement this task 🤷

[rstoyanchev]

Sorry if this wasn't clear, but my suggestion was not to change to MultiValueMap. Keep HttpHeaders internally, as you have done, and improve the way values are formatted. So instead of a simple String#valueOf, we would need to match the formatting that would be done otherwise in MockHttpServletRequest.

For this you can trace the code to add and obtain headers in MockHttpServletRequest. You will see that doAddHeaderValue recognizes collections and arrays. Also getDateHeader and getIntHeader recognize various additional value types.

[mieseprem]

Okay, got it. So I will give it another try 👍

[mieseprem]

o could itself be a Collection or Array -> need to handle (see CollectionUtils)

[rstoyanchev]

@mieseprem, this issue has proven to be more involved than it appeared, and there are also further changes I'd like to make to align the MockMvc request builder with that of RestTestClient and WebTestClient. At the same time we're running out of time for RC1, so I'm going to take over this request. Thanks in any case for the effort to help!