Consider the re-design of the HttpHeaders inheritance hierarchy

[mipo256]

Hey Spring Team, thank you all for the great work you're doing! I kindly request to review the following proposition:

Currently, we have HttpHeaders class in the spring http package. That class defines the methods like add or addAll to add headers. Sometimes it really helps and become really useful. However, we recently faced a bug in production, that I think will explain the problem.

Imagine the following method:

public void addRedirectionIfNecessary(HttpHeaders headers) {
    if (someCondition) {
        headers.add(HttpHeaders.LOCATION, computeLocation());
    }
}

The problem is that this code would not always work. Currently, nothing prevents a developer to either accidentally, or by totally innocent mistake pass here an instance of ReadOnlyHttpHeaders. The ReadOnlyHttpHeaders overrides the add/addAll methods and throws an UnsupportedOperationException.

This design resembles a Java Collection API. As admitted by Oracle, this is one of the most "controversial" decisions that was ever made in the Collections Framework design. Kotlin, for instance, that Spring Framework recently announced partnership with not that long ago, differentiates between mutable collections and immutable ones (like MutableList vs just List). I would take a responsibility to state, that, at least, based on my knowledge, the Kotlin's approach is generally considered to be superior of some sort in the industry since it prevents the kind of bugs that would otherwise happen.

I hope that this proposal makes sense and resonates for the core Spring team. I'll leave this issue as the placeholder for future discussions.

[rstoyanchev]

ReadOnlyHttpHeaders models cases like server request headers or client response headers that are logically exposed for reading purposes and intended to be immutable. From the description, it's not very clear what the scenario is. Presumably these are server response headers, but how did they become readonly?

[spring-projects-issues]

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

[mipo256]

Hey Rossen @rstoyanchev!

I highly appreciate your response and your time investment into this issue. Thank you!

I understand that ReadOnlyHttpHeaders is mostly exposed for reading purposes. At the end of the day, the request comes in and it has N headers. That is clear. However, my point is that currently HttpHeaders is in fact, sometimes, meant to be modified, and sometimes it isn't. This is not something hypothetical, people actually do modify headers (please, check here, and here), and we do that as well.

The problem lies more in a practical plane. Let me illustrate the example. For instance, the abstract java developers team may have something like this in their code base:

interface HttpHeadersProcessor {
    
    void process(HttpHeaders headers);
}

And this interface is used when creating the ResponseEntity, for instance, like this:

HttpHeadersProcessor httpHeadersProcessor = // got from somehere

ResponseEntity.ok().headers(httpHeadersProcessor::process).build();

This is a valid example and it works. Even if the retrieved HttpHeadersProcessor modifies the headers that is fine since HttpHeaders are by themselves modifiable.

And someone decided to re-use particular HttpHeadersProcessor for processing the specific ResponseEntity, like this:

public class ResponseEntityPostProcessor {

    void postProcess(ResponseEntity entity) {
         HttpHeadersProcessor headersProcessor = // get from somewhere;
         headersProcessor.process(entity.getHeaders());
    }   
}

The problem lies in what ResponseEntity#build() is actually doing this under the hood. How would the developer that written ResponseEntityPostProcessor know that headers became unmodifable?

There is no way other than looking into the source code, or writing a specific integration test. The HttpHeaders interface just does not communicate that this headers are in fact immutable.

It is like when you have a method:

public void sort(List<Integer> source) {
    Collections.sort(source);
}

Would it be safe to call sort...? Unfortunately, God knows, since List might be immutable and created via List.of...

There is no real way to know this in production, unless we do things like instanceof and etc, which completely defeats the purpose of accepting the List in the first place, since actually, we are not expecting just the List, we're expecting the List that can be sorted, and in Java not every List can be sorted in place.

Solution as I see it

HttpHeaders should not have the add/addAll methods and throw UOE in some implementations like ReadOnlyHttpHeaders. That is a trap that is going to explode in production.

As stated, following the Kotlin's approach, the new ModifiableHttpHeaders interface needs to be created (like MutableList in Kotlin) that would extend the HttpHeaders. And ReadOnlyHttpHeaders would continue to extend the HttpHeaders, but it would not have methods like add/addAll so it would be impossible to call them. The error is prevented on the javac level, which is great.

I can assist in implementation, if I may.

Have a great day, Rossen!

[bclozel]

Hey @mipo256

We discussed this today and while we agree that a clearer type separation between mutable and immutable headers would be beneficial, we don't see a practical way forward. Right now a large number of applications instantiate HttpHeaders or get injected with an HttpHeaders instance on a controller method. Introducing a MutableHttpHeaders or ReadOnlyHttpHeaders public type will break half of those use cases and cause upgrade pain. Your solution still needs to retire HttpHeaders at some point to have any effect. Right now the tradeoffs are not in favor of such a broad change. We can always reconsider later if we get new feedback from the community rooted in other use cases.

Speaking about concrete use cases, your last comment about ResponseEntity is interesting and we will have a look in #35888. This generally looks like a behavior from the original HttpEntity created in 2010 and we don't see any reason why we can't change the behavior there.

Thanks!