Properly handle optional<binary> endpoint return type and binary-related aliases #1052
Conversation
Generate changelog in
|
psyclaudeZ
left a comment
psyclaudeZ
left a comment
There was a problem hiding this comment.
LGTM, thanks for working on this!
I have a question inline which isn't about optional<binary> (I'd still prefer/recommend splitting this PR into two since they don't seem that integral, which means we could've already merged the optional<binary> one by now :p But I'm ok merging it as is given the size.)
| // outer and inner type de-aliased). | ||
| .isResponseBinary(endpointDef | ||
| .getReturns() | ||
| // We do not need to handle alias of binary since they are treated differently over the wire |
There was a problem hiding this comment.
Does this comment not make sense or has the spec changed? Sounds like it was intentionally not handling the binary alias and I'm curious of why.
There was a problem hiding this comment.
The comment doesn't make sense, basically everywhere the types are de-aliased in the spec, and for binary/optional returns it's no different https://github.com/palantir/conjure/blob/master/docs/spec/wire.md#32-response-body. It further doesn't make sense that an alias of binary/optional would change the behavior of our handling of raw octet-stream data which is what the spec defines must be returned by the server in these cases. This comment is from 2018, I don't see any related change to the wire.md file that would cause this and the author is no longer at Palantir so don't know why it was there.
|
Released 4.19.0 |
|
Released 4.20.0 |
Before this PR
After this PR
conjure-python wasn't properly handling the
optional<binary>return body case from servers where if present it should be written as raw bytes and if not present the body will be empty and the return type will be204. For anoptional<binary>return type we were trying to decode the.json()field of the response which shouldn't exist in this case. So the change here is to make theisResponseBinaryfield check for not only thebinarytype but also theoptional<binary>type. Also change the endpoint to return earlier if status code204is encountered so we can handle binary and non-binary optional return types.conjure-python wasn't handling aliases properly when it came to request/response binary types. So the change was to dealias the request/response types we were checking in the
isRequestBinaryandisResponseBinaryargs and when checking for anoptional<binary>type we need to dealias both the outside and inside type as either or both could be aliases.==COMMIT_MSG==
==COMMIT_MSG==
Possible downsides?
I believe this shouldn't break any existing use cases because using any endpoints that return
optional<binary>in conjure-python should currently be failing due to trying to decode a non-existent.json()field. And similarly, any endpoints that use aliases for binary request/response types should be being handled inappropriately right now (i.e. aliases to binary are going to be treated as non-binary and thus requests will be sent with incorrect metadata, args, etc.) and non-functional.