Skip to content

Properly handle optional<binary> endpoint return type and binary-related aliases #1052

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jun 10, 2025
Merged

Properly handle optional<binary> endpoint return type and binary-related aliases #1052

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
10f5bdb
handle optional binary and aliases
BenjaminLowry Jun 6, 2025
d8ba14c
comment
BenjaminLowry Jun 6, 2025
dbecb01
spotless
BenjaminLowry Jun 6, 2025
cd2d716
Add generated changelog entries
svc-changelog Jun 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions changelog/@unreleased/pr-1052.v2.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
type: fix
fix:
description: Properly handle optional<binary> endpoint return type and binary-related
aliases
links:
- https://github.com/palantir/conjure-python/pull/1052
24 changes: 19 additions & 5 deletions conjure-python-core/src/main/java/com/palantir/conjure/python/client/ClientGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
import com.palantir.conjure.python.types.MyPyTypeNameVisitor;
import com.palantir.conjure.python.types.PythonTypeNameVisitor;
import com.palantir.conjure.spec.EndpointDefinition;
import com.palantir.conjure.spec.PrimitiveType;
import com.palantir.conjure.spec.ServiceDefinition;
import com.palantir.conjure.spec.Type;
import com.palantir.conjure.visitor.DealiasingTypeVisitor;
Expand Down Expand Up @@ -125,14 +124,29 @@ private PythonEndpointDefinition generateEndpoint(
.params(params)
.pythonReturnType(endpointDef.getReturns().map(type -> type.accept(pythonTypeNameVisitor)))
.myPyReturnType(endpointDef.getReturns().map(type -> type.accept(myPyTypeNameVisitor)))
// Set to true iff the de-aliased type is binary.
.isRequestBinary(endpointDef.getArgs().stream()
.anyMatch(argumentDef -> argumentDef.getParamType().accept(ParameterTypeVisitor.IS_BODY)
&& argumentDef.getType().accept(TypeVisitor.IS_BINARY)))
&& dealiasingTypeVisitor
.dealias(argumentDef.getType())
.fold(_typeDefinition -> false, type -> type.accept(TypeVisitor.IS_BINARY))))
// Set to true iff 1) the de-aliased type is binary or 2) the de-aliased type is optional<binary> (both
// 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
Copy link
Contributor

@psyclaudeZ psyclaudeZ Jun 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@BenjaminLowry BenjaminLowry Jun 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.map(rt -> rt.accept(TypeVisitor.IS_PRIMITIVE)
&& rt.accept(TypeVisitor.PRIMITIVE).get() == PrimitiveType.Value.BINARY)
.map(rt -> dealiasingTypeVisitor
.dealias(rt)
.fold(
_typeDefinition -> false,
type -> type.accept(TypeVisitor.IS_BINARY)
|| (type.accept(TypeVisitor.IS_OPTIONAL)
&& dealiasingTypeVisitor
.dealias(type.accept(TypeVisitor.OPTIONAL)
.getItemType())
.fold(
_typeDefinition -> false,
itemType -> itemType.accept(
TypeVisitor.IS_BINARY)))))
.orElse(false))
.isOptionalReturnType(endpointDef
.getReturns()
Expand Down
19 changes: 9 additions & 10 deletions ...-python-core/src/main/java/com/palantir/conjure/python/poet/PythonEndpointDefinition.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,22 +225,21 @@ default void emit(PythonPoetWriter poetWriter) {
poetWriter.decreaseIndent();

poetWriter.writeLine();
if (isOptionalReturnType()) {
poetWriter.writeIndentedLine("if _response.status_code == 204:");
poetWriter.increaseIndent();
poetWriter.writeIndentedLine("return None");
poetWriter.decreaseIndent();
}
if (isResponseBinary()) {
poetWriter.writeIndentedLine("_raw = _response.raw");
poetWriter.writeIndentedLine("_raw.decode_content = True");
poetWriter.writeIndentedLine("return _raw");
} else if (pythonReturnType().isPresent()) {
poetWriter.writeIndentedLine("_decoder = ConjureDecoder()");
if (isOptionalReturnType()) {
poetWriter.writeIndentedLine(
"return None if _response.status_code == 204 else _decoder.decode(_response.json()"
+ ", %s, self._return_none_for_unknown_union_types)",
pythonReturnType().get());
} else {
poetWriter.writeIndentedLine(
"return _decoder.decode(_response.json(), %s, self._return_none_for_unknown_union_types)",
pythonReturnType().get());
}
poetWriter.writeIndentedLine(
"return _decoder.decode(_response.json(), %s, self._return_none_for_unknown_union_types)",
pythonReturnType().get());
} else {
poetWriter.writeIndentedLine("return");
}
Expand Down
149 changes: 144 additions & 5 deletions conjure-python-core/src/test/resources/services/expected/package_name/_impl.py
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions conjure-python-core/src/test/resources/services/expected/package_name/product/__init__.py
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

30 changes: 30 additions & 0 deletions conjure-python-core/src/test/resources/types/example-service.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,12 @@ types:
- value: PYPI
- value: CONDA

BinaryAlias:
alias: binary

OptionalStringAlias:
alias: optional<string>

services:
TestService:
name: Test Service
Expand Down Expand Up @@ -98,6 +104,10 @@ services:
- Safe
returns: binary

getAliasedRawData:
http: GET /get-aliased-raw
returns: BinaryAlias

maybeGetRawData:
http: GET /datasets/{datasetRid}/raw-maybe
args:
Expand All @@ -107,13 +117,33 @@ services:
- Safe
returns: optional<binary>

maybeGetAliasedRawData:
http: GET /datasets/{datasetRid}/raw-maybe-alias
args:
datasetRid:
type: rid
markers:
- Safe
returns: optional<BinaryAlias>

getAliasedReturn:
http: GET /aliased-return
returns: OptionalStringAlias

uploadRawData:
http: POST /datasets/upload-raw
args:
input:
type: binary
param-type: body

uploadAliasedRawData:
http: POST /datasets/upload-raw-alias
args:
input:
type: BinaryAlias
param-type: body

uploadPythonPackage:
http: POST /data/python/{uploadType}
args:
Expand Down
Loading