fixes #468: Dialogue clients send Accept headers #482
Conversation
Generate changelog in
|
carterkozak
changed the title
| */ | ||
| InputStream deserializeInputStream(Response response); | ||
| @Deprecated | ||
| default InputStream deserializeInputStream(Response response) { |
There was a problem hiding this comment.
Why not just get rid of this
There was a problem hiding this comment.
our circular dependency on generated code would break
| private static Request accepting(Request original, String acceptValue) { | ||
| Preconditions.checkNotNull(acceptValue, "Accept value is required"); | ||
| Preconditions.checkState(!acceptValue.isEmpty(), "Accept value must not be empty"); | ||
| if (original.headerParams().containsKey(HttpHeaders.ACCEPT)) { |
There was a problem hiding this comment.
should we append the acceptValue?
There was a problem hiding this comment.
I don't think so, we'd need to deduplicate repeated accept values, and the existing values will be preferred since we're using a list, which isn't what we want here. I considered throwing instead of logging but it's difficult to trace where the value was added (unlike the null/empty inputs) so we would want to return a future.
| @@ -177,6 +172,11 @@ public T deserialize(Response response) { | |||
| } | |||
| } | |||
|
|
|||
| @Override | |||
| public Optional<String> accepts() { | |||
There was a problem hiding this comment.
Instead of joining the separate accept headers into a single string should we expose a List<String>
There was a problem hiding this comment.
I tried this initially, but found that it maps to adding multiple Accept headers each with a single value, but it's most common to use Accept header with multiple values. The optional approach allows us to pre-calculate the value.
There was a problem hiding this comment.
It seems surprising to encode multiple accept values as a header value since in a multimap set up I would expect each value in the map to be a single header value.
There was a problem hiding this comment.
I completely agree that multiple accept header key/value pairs would be clean, but I don't think I've seen it used that way in practice. I think we'll reduce odd edge case failures by following the principle of least astonishment.
dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/DefaultClientsTest.java
Outdated
Show resolved
Hide resolved
…e/serde/DefaultClientsTest.java Co-Authored-By: Felipe Orozco <ferozco@users.noreply.github.com>
|
👍 |
|
Released 0.13.1 |
Before this PR
binaryfailed to parse errors, instead producing an illegal argument exception "Unsupported Content-Type" (with no response status check).After this PR
==COMMIT_MSG==
Dialogue clients send Accept headers
==COMMIT_MSG==
Possible downsides?
api churn