ReadOnlyHttpHeaders is package-private that prevents instanceOf

[muchnik]

Affects: latest 5.3.13

Class org.springframework.http.ReadOnlyHttpHeaders from module spring-web is package-private that prevents doing instanceOf or other interactions.
I suggest making it public-visibility class, which is really helpfull in some cases.

There was a reason doing it package-private or I can follow and make a PR?

[poutsma]

It is package private because that allows us to make changes that break backward compatibility, something that we cannot do with public types. We use private types quite a lot in Spring Framework, and generally only expose types when absolute necessary. In this case, exposing the type is not necessary, similarly to how the JDK does not expose the UnmodifiableList private class that is returned from Collections::unmodifiableList.

[dscham]

How do I check if HttpHeaders are ReadOnly then?

[bclozel]

@dscham See #32116

[dscham]

Thanks @bclozel. I see how I could modify the headers there, which looks like it's going to be deprecated though. But I don't see how to check if the HttpHeaders are actually ReadOnlyHttpHeaders. And since I can't import the class, I can't check it's instance.

As an aside: IMO making the HttpHeaders opaque like that is a really bad design choice. It's not debuggable. In WebFilterChains, I don't see how I should wrap the requests, or worse, responses. And I don't even know what it would cost us if we'd have to change to wrapped HTTP objects at some point, if this breaks. But mostly, it's annoying that I only see failures at runtime and that I can't analyze where the ReadOnlyHttpHeaders come from because: It's not debuggable.

We run a Spring Gateway service as our API Gateway and I got the honors of taking ownership from the team that created it. It's cool stuff. 95% just configuration and 5% Filters. But I had to add configurable X-Frame-Options, which I did as a filter, and in some cases I crash the tests now, because I can't set headers. But I can't find out where the ReadOnly comes from. I hacked my way around with a try-catch. But that feels very botched.

[bclozel]

@dscham Thanks for your feedback. You can use this method in the meantime, and with #32116 you will be able to just build a new instance of HttpHeaders with an existing one and you'll get a mutable instance out of it - it will perform the unwrapping or will be a no-op depending on the situation.

You should always assume that incoming HTTP headers are immutable and act accordingly in your implementation. #32116 points to possible improvements, including in our testing story to make this more reliable. We will consider those enhancements and create issues accordingly.

[dscham]

I changed from skipping read-only headers to using writableHeaders, as I found that it changed the behaviour of the service in an unintended way.

So, if I understand your message correctly and we update to a version that deprecates the function, I just have to remove the .writableHeaders(headers) and move the parameter to the constructor call, then it should work as before?

[bclozel]

No, you can use writableHeaders for the time being, and switch to using the constructor as soon as you'll see it deprecated.

[dscham]

No, you can use writableHeaders for the time being, and switch to using the constructor as soon as you'll see it deprecated.

Nice, then it is as I thought. Well, that's good then.

Otherwise, Is there anything planned to make it more visible if the headers are read-only? Because right now, the try-catch is the only way I found. And, as is wrote, that's feeling really bad. Also, it's hard for debugging, at least in IntelliJ, I couldn't find a way to distinguish them easily.

In any case, thanks for your support @bclozel 😁