[WIP] Consistently resolve binary aliases in requests and responses

[pkoenig10]

Follow up to #92 to correctly implement the conjure spec:

Accept header - Clients MUST send an Accept: application/json header for all requests UNLESS the endpoint returns binary, in which case the client MUST send Accept: application/octet-stream. This ensures changes can be made to the wire format in a non-breaking way.

This is consistent with the behavior of other generated clients:

• Jersey - set by Feign due to the class-level Produces annotation
• Typescript - Always sets Accept header

[iamdanfox]

can we make sure this still works if the endpoint returns an alias of binary? Might be able to use the DealiasingTypeVisitor

[pkoenig10]

This is the same behavior as the Jersey service generator:

conjure-java/conjure-java-core/src/main/java/com/palantir/conjure/java/services/JerseyServiceGenerator.java

Lines 146 to 150 in 40cdc41

	if (endpointDef.getReturns().map(type -> type.accept(TypeVisitor.IS_BINARY)).orElse(false)) {
	methodBuilder.addAnnotation(AnnotationSpec.builder(ClassName.get("javax.ws.rs", "Produces"))
	.addMember("value", "$T.APPLICATION_OCTET_STREAM", ClassName.get("javax.ws.rs.core", "MediaType"))
	.build());
	}

Correctly resolving aliases when settings the response media type is separate from this PR. But I'm not sure it is the correct thing to do. Aliases are completely separate types that wrap the type they are aliasing. You can't stream an alias of a binary.

[iamdanfox]

As a general rule of thumb, we try to make sure that aliases are pretty transparent so an alias of X should behave as much as possible like X.

We rely on this assumption in the conjure backcompat checker, where introducing an alias is generally considered to be backwards compatible.

[pkoenig10]

We currently resolve aliases for binary return types, but not for binary body parameters. It seems to me that we would want be handle both cases identically. Should we also be resolving aliases for binary body parameters?

[pkoenig10]

I've updated this PR to include the following fixes:

• Correctly set the Accept header for retrofit clients (see PR description)
• Add the @Streaming annotation to retrofit endpoints returning a binary alias
• Resolve binary alias body parameters for jersey and retrofit clients (do we want this?)

[pkoenig10]

Before this change, this would be generated as NestedAliasedBinary

[ferozco]

Historically, we have treated binary and alias of binary differently. Long term, we want to aliases be completely transparent but I don't think this PR is the place to make that change

[pkoenig10]

Currently they are treated differently for input types but not for returned types. I made this change in response to @dfox's comment above.

I just want clarification on the desired outcome here:

• Resolve neither returned nor parameter binary types
• Resolve only returned binary types (the current behavior)
• Resolve both returned and parameter binary types (the current impl of this PR)

[ferozco]

Aliases of binary are currently treated differently from raw binary, even for return types since it is not dealiased: jersey, retrofit.

I'm advocating for keeping the current behaviour with the intention of fixing the the inconsistency later

[pkoenig10]

Aliases of binary are currently treated differently from raw binary, even for return types

This is not entirely true:
https://github.com/palantir/conjure-java/blob/2.5.0/conjure-java-core/src/test/resources/test/api/TestService.java.jersey#L58-L69

The check you linked is only used to decide whether or not to add the @Produces annotation. This is a correctness issue. We claim to return an application/json type but actually return an application/octet-stream.

For aliased types we should either do both or neither of:

• Change the return type to StreamingOutput
• Add a @Produces annotation with application/octet-stream

[pkoenig10]

This should be rebased on #127

[pkoenig10]

Closing for now