jakartaee / rest

Jakarta RESTful Web Services
Other
353 stars 114 forks source link

Seeking Clarification on the Mutability of the Response Headers Views #1136

Open gilday opened 1 year ago

gilday commented 1 year ago

The Javadoc for the [Response.getHeaders()](https://javadoc.io/static/jakarta.ws.rs/jakarta.ws.rs-api/3.1.0/jakarta.ws.rs/jakarta/ws/rs/core/Response.html#getHeaders()), [Response.getStringHeaders()](https://javadoc.io/static/jakarta.ws.rs/jakarta.ws.rs-api/3.1.0/jakarta.ws.rs/jakarta/ws/rs/core/Response.html#getStringHeaders()), and the (deprecated) [Response.getMetadata()](https://javadoc.io/static/jakarta.ws.rs/jakarta.ws.rs-api/3.1.0/jakarta.ws.rs/jakarta/ws/rs/core/Response.html#getMetadata()) methods indicate that the MultivaluedMap returned by these methods is a view.

The term view sometimes signals that the view has different mutability constraints than the original object it projects. java.util.Map.entrySet is a familiar example. The Javadoc for [Map.entrySet()](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/Map.html#entrySet()) states that it does not support add or addAll operations:

The set supports element removal, which removes the corresponding mapping from the map, via the Iterator.remove, Set.remove, removeAll, retainAll and clear operations. It does not support the add or addAll operations.

When it comes to the headers accessors on Response, it's not clear whether or not the view returned is mutable. In my testing, I've found that it depends on both the accessor called and the implementation.

In the Jersey, CXF, and RESTEasy versions I've tested, the map returned by Response.getHeaders() and Response.getMetadata() may be added to, but the map returned by Response.getStringHeaders() cannot be. RESTEasy and CXF ignore additions to the map returned by Response.getStringHeaders(), while Jersey throws an UnsupportedOperationException when adding to the map.

In the Jersey versions I've tested, the map returned by Response.getStringHeaders() is like java.util.Map.entrySet() in that it does not support adding items, but it does support removing items. The map returned by the same call in RESTEasy and CXF is a copy of the original data structure, so adding and removing items from it does nothing to affect the response sent to the client.

Can the specification maintainers please provide guidance on the expected mutability constraints of the response headers maps returned by these APIs?

mkarg commented 1 year ago

Thank you for bringing up this inconsistency. It seems we should find a consensus and clarify this issue unambiguously in JAX-RS 4.0.

Proposal: getHeaders() should be read-write, while getHeaderStrings() should be read-only.

@spericas @jansupol @jamezp @arjantijms @chkal @andymc12 WDYT?

NicoNes commented 1 year ago

Hi guys,

Why not try to keep both getHeaders() and getHeaderStrings() read-only as suggested by the view term ? This way all response properties (headers, status, body) will be read only through Response object and editable through ResponseBuilder object, instead of having only headers being editable through Response and ResponseBuilder object.

Nicolas

chkal commented 1 year ago

I also think that currently, the word "view" suggests read-only behavior. And maybe making this "official" would be a good solution?

@mkarg Is there any special reason for your proposal to make getHeaders() read-write?

mkarg commented 1 year ago

@chkal I want to keep some possibility to modify the response headers, as Response itself is not a read-only class.

NicoNes commented 1 year ago

@mkarg Do you have a case in mind that requires getHeaders() to be editable ? To my opinion, if we choose to make response headers modifiable we should also do the same for response status and response entity to remain consistent.

mkarg commented 1 year ago

(1) Whether or not I personally do know such a case is not the point here. The point is that we need to be careful. If we break an existing application just because we hastily restrict existing features without any need, community will be (at least) disappointed. I do not say that I will vote -1 for restricting Response to be fully read-only in 4.0, but we should not do so without taking care that all existing applications can be easily fixed (e. g. by providing a clear migration path in the JavaDocs how to replace read-write use cases of getHeaders).

(2) It is rather odd that Response will become read-only while Request still is read-write.

(3) Yes, I would prefer adding read-write modifiers for hstatus and entity, actually. The reason is that when I invented filters, and when Bill Burke invented Interceptors, it was clear that one day the question would arise, why not simply using CDI interceptors instead. As we now discuss replacing @Context by @Inject (hence enforce CDI), this discussion could (and possibly should) get warmed-up again. In that case (depreacting filters in favor of CDI interceptors) how should headers get modified if not by injecting Response? If we keep getHeaders mutable as a last resort, we keep the future open for such a discussion. If we don't, such a discussion will get much harder.

NicoNes commented 1 year ago

(1) Whether or not I personally do know such a case is not the point here. The point is that we need to be careful. If we break an existing application just because we hastily restrict existing features without any need, community will be (at least) disappointed.

Actually, I was asking if you have any real use case in mind to find out if this discussion wasn't just about theorical use of Reponse object and thus if making getHeaders read-only will make a hudge difference for the community.

but we should not do so without taking care that all existing applications can be easily fixed (e. g. by providing a clear migration path in the JavaDocs how to replace read-write use cases of getHeaders).

:+1:

(2) It is rather odd that Response will become read-only while Request still is read-write.

You mean jakarta.ws.rs.core.Request ? If true how is it read-write ?

If we keep getHeaders mutable as a last resort, we keep the future open for such a discussion. If we don't, such a discussion will get much harder.

To my opinion, even if we keep getHeaders read-only for now, it will not bother us to make it read-write in the future if any structural change as using CDI interceptors requires it. Anyway, I think that as long as that feature of using CDI interceptor is still hypothetical, it should not be the argument in favor of making getHeaders read-write or not for now.

So, to sum up, I'm +1 to have getHeaders read-write if we add read-write modifiers for status and entity. Else +1 to have getHeaders read only.

Thanks

jamezp commented 1 year ago

I don't have a strong opinion on whether or not the headers in a response should be read-only or not, but it does seem like it should be clarified. I'd have a slight preference for read-only because you can just do Response.fromResponse(response) to get a ResponseBuilder to add headers.

That said in the JavaDoc for Response.getHeaders() it does say:

Changes in the underlying header data are reflected in this view.

To me, that indicates it's mutable and a change to read-only would not be backwards compatible. That said, Response.getStringHeaders() has the same text so it could be considered a bug in the implementations. It could be debated on what "this view" is though.

NicoNes commented 1 year ago

To me, that indicates it's mutable and a change to read-only would not be backwards compatible.

Or it could also mean that Response.getHeaders() should not return a snapshot map of the headers but instead a live view. Where a live view is an read-only map that allows you to always see the last version of the mutable Object values stored in the map as soon as those mutable Object instance get modified. That is consistent with both getHeaders() and getStringHeaders() having the same text. That is exactly what [Map.entrySet()](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/Map.html#entrySet()) view does

jansupol commented 1 year ago

@gilday I wonder what is the use-case for you checking the mutability? The Response object is provided by a client code in the Resource Method, or it is obtained on the Client side. Is either of these two use-cases a subject of your changing the headers? Or is there any other way the customer can be provided with the Response object I do not recall from the top of my head at the moment?

gilday commented 1 year ago

@jansupol my use case is abnormal and probably doesn't contribute much to the discussion, but I'm happy to share it.

I maintain a security observability tool implemented as a byte code instrumentation agent. The setting of response headers from a JAX-RS resource is a security relevant event the tool wants to observe. I was hopeful that I might only observe relevant calls to ResponseBuilder APIs, but then I discovered that some of the MultivaluedMap accessors on Response return mutable data structures. The Javadoc for these accessors didn't tell me about the mutability of the MultivaluedMap, so I tested each implementation and found discrepancies.

jansupol commented 1 year ago

@gilday Thank you for the explanation. Yet, I do not see the relevance for the user, since the user has limited use for the Response and its mutability. Right now this feels like a corner case.

I do not see any advantage for the Response#getHeaders() being mutable and I would not change that regard in the Spec, unless we found a relevant use-case for the end user.

gilday commented 1 year ago

Agree that it feels like a corner case, because the user presumably created that Response from a ResponseBuilder where they had access to APIs for mutating headers prior to calling build().

But, to be clear, I'm not advocating strongly for any particular solution. Instead, I am asking that the specification clarify the expected mutability constraints (if any), because the ambiguity in Javadoc has lead to discrepancies in the implementations.

mkarg commented 1 year ago

(2) It is rather odd that Response will become read-only while Request still is read-write. You mean jakarta.ws.rs.core.Request ? If true how is it read-write ?

Actually I was wrong here.