[fix] optional<binary> equivalent to binary for retrofit + Optional<S…

…treamingOutput> for jersey (#201)

Fix #182 

For jersey and retrofit, the current behavior of `binary` enables an implicit tri-state with different status code:
- non-empty stream: 200
- empty stream: 200
- null: 204 + null stream on retrofit client

The way forward to achieve a standard behavior with all clients and server is to consider that service that take advantage of this behavior should declare a return type of `optional<binary>` instead. The jersey resource implementation will need to stay the same and keep encoding empty as null.

In this PR, we are making the return type `optional<binary>` being equivalent to `binary` for the retrofit generator and change the jersey generator to return `Optional<StreamingOutput>`.

conjure-java-core/src/integrationInput/java/com/palantir/product/EteBinaryServiceRetrofit.java

@@ -1,8 +1,6 @@
 package com.palantir.product;
 
 import com.palantir.tokens.auth.AuthHeader;
-import java.nio.ByteBuffer;
-import java.util.Optional;
 import javax.annotation.Generated;
 import okhttp3.RequestBody;
 import okhttp3.ResponseBody;
@@ -24,14 +22,14 @@ Call<ResponseBody> postBinary(
             @Header("Authorization") AuthHeader authHeader, @Body RequestBody body);
 
     @GET("./binary/optional/present")
-    @Headers({"hr-path-template: /binary/optional/present", "Accept: application/json"})
-    Call<Optional<ByteBuffer>> getOptionalBinaryPresent(
-            @Header("Authorization") AuthHeader authHeader);
+    @Headers({"hr-path-template: /binary/optional/present", "Accept: application/octet-stream"})
+    @Streaming
+    Call<ResponseBody> getOptionalBinaryPresent(@Header("Authorization") AuthHeader authHeader);
 
     @GET("./binary/optional/empty")
-    @Headers({"hr-path-template: /binary/optional/empty", "Accept: application/json"})
-    Call<Optional<ByteBuffer>> getOptionalBinaryEmpty(
-            @Header("Authorization") AuthHeader authHeader);
+    @Headers({"hr-path-template: /binary/optional/empty", "Accept: application/octet-stream"})
+    @Streaming
+    Call<ResponseBody> getOptionalBinaryEmpty(@Header("Authorization") AuthHeader authHeader);
 
     /** Throws an exception after partially writing a binary response. */
     @GET("./binary/failure")

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

@@ -53,6 +53,7 @@
 import com.squareup.javapoet.JavaFile;
 import com.squareup.javapoet.MethodSpec;
 import com.squareup.javapoet.ParameterSpec;
+import com.squareup.javapoet.ParameterizedTypeName;
 import com.squareup.javapoet.TypeName;
 import com.squareup.javapoet.TypeSpec;
 import java.io.InputStream;
@@ -96,9 +97,16 @@ public Set<JavaFile> generate(ConjureDefinition conjureDefinition) {
                 ? BINARY_RETURN_TYPE_RESPONSE
                 : BINARY_RETURN_TYPE_OUTPUT;
 
+        TypeName optionalBinaryReturnType = experimentalFeatures.contains(FeatureFlags.JerseyBinaryAsResponse)
+                ? BINARY_RETURN_TYPE_RESPONSE
+                : ParameterizedTypeName.get(ClassName.get(Optional.class), binaryReturnType);
+
         TypeMapper returnTypeMapper = new TypeMapper(
                 conjureDefinition.getTypes(),
-                new ReturnTypeClassNameVisitor(conjureDefinition.getTypes(), binaryReturnType));
+                new ReturnTypeClassNameVisitor(
+                        conjureDefinition.getTypes(),
+                        binaryReturnType,
+                        optionalBinaryReturnType));
 
         TypeMapper argumentTypeMapper = new TypeMapper(
                 conjureDefinition.getTypes(),

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

@@ -89,7 +89,7 @@ public Retrofit2ServiceGenerator(Set<FeatureFlags> experimentalFeatures) {
     public Set<JavaFile> generate(ConjureDefinition conjureDefinition) {
         TypeMapper returnTypeMapper = new TypeMapper(
                 conjureDefinition.getTypes(),
-                new ReturnTypeClassNameVisitor(conjureDefinition.getTypes(), BINARY_RETURN_TYPE));
+                new ReturnTypeClassNameVisitor(conjureDefinition.getTypes(), BINARY_RETURN_TYPE, BINARY_RETURN_TYPE));
 
         TypeMapper argumentTypeMapper = new TypeMapper(
                 conjureDefinition.getTypes(),

conjure-java-core/src/main/java/com/palantir/conjure/java/types/ReturnTypeClassNameVisitor.java

@@ -25,6 +25,7 @@
 import com.palantir.conjure.spec.Type;
 import com.palantir.conjure.spec.TypeDefinition;
 import com.palantir.conjure.visitor.TypeDefinitionVisitor;
+import com.palantir.conjure.visitor.TypeVisitor;
 import com.squareup.javapoet.ClassName;
 import com.squareup.javapoet.TypeName;
 import java.util.List;
@@ -37,13 +38,18 @@ public final class ReturnTypeClassNameVisitor implements ClassNameVisitor {
     private final DefaultClassNameVisitor delegate;
     private final Map<com.palantir.conjure.spec.TypeName, TypeDefinition> types;
     private final ClassName binaryClassName;
+    private final TypeName optionalBinaryTypeName;
 
-    public ReturnTypeClassNameVisitor(List<TypeDefinition> types, ClassName binaryClassName) {
+    public ReturnTypeClassNameVisitor(
+            List<TypeDefinition> types,
+            ClassName binaryClassName,
+            TypeName optionalBinaryTypeName) {
         this.delegate = new DefaultClassNameVisitor(types);
         this.types = types.stream().collect(Collectors.toMap(
                 t -> t.accept(TypeDefinitionVisitor.TYPE_NAME),
                 Function.identity()));
         this.binaryClassName = binaryClassName;
+        this.optionalBinaryTypeName = optionalBinaryTypeName;
     }
 
     @Override
@@ -58,6 +64,10 @@ public TypeName visitMap(MapType type) {
 
     @Override
     public TypeName visitOptional(OptionalType type) {
+        if (type.getItemType().accept(TypeVisitor.IS_PRIMITIVE)
+                && type.getItemType().accept(TypeVisitor.PRIMITIVE).equals(PrimitiveType.BINARY)) {
+            return optionalBinaryTypeName;
+        }
         return delegate.visitOptional(type);
     }
 

conjure-java-core/src/test/resources/test/api/TestService.java.jersey

@@ -9,7 +9,6 @@ import com.palantir.redaction.Safe;
 import com.palantir.ri.ResourceIdentifier;
 import com.palantir.tokens.auth.AuthHeader;
 import java.io.InputStream;
-import java.nio.ByteBuffer;
 import java.util.Collections;
 import java.util.Map;
 import java.util.Optional;
@@ -70,7 +69,7 @@ public interface TestService {
 
     @GET
     @Path("catalog/datasets/{datasetRid}/raw-maybe")
-    Optional<ByteBuffer> maybeGetRawData(
+    Optional<StreamingOutput> maybeGetRawData(
             @HeaderParam("Authorization") @NotNull AuthHeader authHeader,
             @PathParam("datasetRid") @Safe ResourceIdentifier datasetRid);
 

conjure-java-core/src/test/resources/test/api/TestService.java.jersey_require_not_null

@@ -9,7 +9,6 @@ import com.palantir.redaction.Safe;
 import com.palantir.ri.ResourceIdentifier;
 import com.palantir.tokens.auth.AuthHeader;
 import java.io.InputStream;
-import java.nio.ByteBuffer;
 import java.util.Collections;
 import java.util.Map;
 import java.util.Optional;
@@ -70,7 +69,7 @@ public interface TestService {
 
     @GET
     @Path("catalog/datasets/{datasetRid}/raw-maybe")
-    Optional<ByteBuffer> maybeGetRawData(
+    Optional<StreamingOutput> maybeGetRawData(
             @HeaderParam("Authorization") @NotNull AuthHeader authHeader,
             @PathParam("datasetRid") @Safe ResourceIdentifier datasetRid);
 

conjure-java-core/src/test/resources/test/api/TestServiceRetrofit.java.retrofit

@@ -7,7 +7,6 @@ import com.palantir.product.datasets.BackingFileSystem;
 import com.palantir.product.datasets.Dataset;
 import com.palantir.ri.ResourceIdentifier;
 import com.palantir.tokens.auth.AuthHeader;
-import java.nio.ByteBuffer;
 import java.util.Map;
 import java.util.Optional;
 import java.util.OptionalDouble;
@@ -71,9 +70,10 @@ public interface TestServiceRetrofit {
     @GET("./catalog/datasets/{datasetRid}/raw-maybe")
     @Headers({
         "hr-path-template: /catalog/datasets/{datasetRid}/raw-maybe",
-        "Accept: application/json"
+        "Accept: application/octet-stream"
     })
-    Call<Optional<ByteBuffer>> maybeGetRawData(
+    @Streaming
+    Call<ResponseBody> maybeGetRawData(
             @Header("Authorization") AuthHeader authHeader,
             @Path("datasetRid") ResourceIdentifier datasetRid);
 

conjure-java-core/src/test/resources/test/api/TestServiceRetrofit.java.retrofit_completable_future

@@ -7,7 +7,6 @@ import com.palantir.product.datasets.BackingFileSystem;
 import com.palantir.product.datasets.Dataset;
 import com.palantir.ri.ResourceIdentifier;
 import com.palantir.tokens.auth.AuthHeader;
-import java.nio.ByteBuffer;
 import java.util.Map;
 import java.util.Optional;
 import java.util.OptionalDouble;
@@ -71,9 +70,10 @@ public interface TestServiceRetrofit {
     @GET("./catalog/datasets/{datasetRid}/raw-maybe")
     @Headers({
         "hr-path-template: /catalog/datasets/{datasetRid}/raw-maybe",
-        "Accept: application/json"
+        "Accept: application/octet-stream"
     })
-    CompletableFuture<Optional<ByteBuffer>> maybeGetRawData(
+    @Streaming
+    CompletableFuture<ResponseBody> maybeGetRawData(
             @Header("Authorization") AuthHeader authHeader,
             @Path("datasetRid") ResourceIdentifier datasetRid);
 

conjure-java-core/src/test/resources/test/api/TestServiceRetrofit.java.retrofit_listenable_future

@@ -8,7 +8,6 @@ import com.palantir.product.datasets.BackingFileSystem;
 import com.palantir.product.datasets.Dataset;
 import com.palantir.ri.ResourceIdentifier;
 import com.palantir.tokens.auth.AuthHeader;
-import java.nio.ByteBuffer;
 import java.util.Map;
 import java.util.Optional;
 import java.util.OptionalDouble;
@@ -71,9 +70,10 @@ public interface TestServiceRetrofit {
     @GET("./catalog/datasets/{datasetRid}/raw-maybe")
     @Headers({
         "hr-path-template: /catalog/datasets/{datasetRid}/raw-maybe",
-        "Accept: application/json"
+        "Accept: application/octet-stream"
     })
-    ListenableFuture<Optional<ByteBuffer>> maybeGetRawData(
+    @Streaming
+    ListenableFuture<ResponseBody> maybeGetRawData(
             @Header("Authorization") AuthHeader authHeader,
             @Path("datasetRid") ResourceIdentifier datasetRid);