Add response headers to FeignException #1277
Conversation
- Add response headers to FeignException and its subclasses
| super(message, cause); | ||
| this.status = status; | ||
| this.responseBody = responseBody; | ||
| this.responseHeaders = responseHeaders; |
There was a problem hiding this comment.
Despite the fact that Strings are immutable, the Map is not. Can you change this to create a copy of the map? Some examples are Map.of(responseHeaders) or new LinkedHashMap<>(responseHeaders)
There was a problem hiding this comment.
Will do. I'm just wondering if wrapping the map itself is enough? The values of this map are of type Collection<String>, so although wrapping the map would make it immutable, the values would still be mutable. To make this "truly" immutable, we would need to wrap the values as well. Or do you think that's an overkill?
There was a problem hiding this comment.
Strings are immutable by nature, so to ensure immutability, you must lock down the map.
There was a problem hiding this comment.
True that but having an immutable map still gives you the option of responseHeaders().get("Something").clear() unless Collection<String> isn't also wrapped in an Collections.unmodifiableCollection() at some point before.
There was a problem hiding this comment.
Response has the method caseInsensitiveCopyOf. I think it should be moved to Utll and reused here.
| * @see Response#headers() | ||
| */ | ||
| public Map<String, Collection<String>> responseHeaders() { | ||
| return this.responseHeaders; |
There was a problem hiding this comment.
We should also wrap this as an immutable collection. Can we wrap with Collections.unmodifiableMap?
There was a problem hiding this comment.
Sure, but if we wrap the response headers as an immutable collection inside the constructor, shouldn't that be sufficient?
There was a problem hiding this comment.
I should be, but we don't advertise that to the consumers. It's also good practice to return a copy of the internal collection just in case.
There was a problem hiding this comment.
I'd argue that it's better to do the copy on construction (and optionally wrap it in Collections.unmodifiableMap()) instead of returning a new copy each time.
|
@pzapasni Apologies for the late replies. |
|
Any chance this gets released for X-mas? 🎅 EDIT: There are a few places I'd also want some headers as an end-user. For example |
|
Love this change. +1 for sorting out the remaining issues. |
|
As far as I understand the source repository is no longer exists. Could I re-create PR as new one? Update: new PR #1452 was created. |
Implementation of #1268
This PR adds the ability to retrieve the response headers from a
FeignException.