From 56f88450dd3d8e49f51d5fc1833d4126a030ec70 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Tue, 22 Jan 2019 19:08:26 +0000 Subject: [PATCH] [fix] optional equivalent to binary for retrofit + Optional 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` 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` being equivalent to `binary` for the retrofit generator and change the jersey generator to return `Optional`. --- .../palantir/product/EteBinaryServiceRetrofit.java | 14 ++++++-------- .../java/services/JerseyServiceGenerator.java | 10 +++++++++- .../java/services/Retrofit2ServiceGenerator.java | 2 +- .../java/types/ReturnTypeClassNameVisitor.java | 12 +++++++++++- .../resources/test/api/TestService.java.jersey | 3 +-- .../api/TestService.java.jersey_require_not_null | 3 +-- .../test/api/TestServiceRetrofit.java.retrofit | 6 +++--- ...erviceRetrofit.java.retrofit_completable_future | 6 +++--- ...ServiceRetrofit.java.retrofit_listenable_future | 6 +++--- 9 files changed, 38 insertions(+), 24 deletions(-) diff --git a/conjure-java-core/src/integrationInput/java/com/palantir/product/EteBinaryServiceRetrofit.java b/conjure-java-core/src/integrationInput/java/com/palantir/product/EteBinaryServiceRetrofit.java index c37b4c628..a82ab6726 100644 --- a/conjure-java-core/src/integrationInput/java/com/palantir/product/EteBinaryServiceRetrofit.java +++ b/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 postBinary( @Header("Authorization") AuthHeader authHeader, @Body RequestBody body); @GET("./binary/optional/present") - @Headers({"hr-path-template: /binary/optional/present", "Accept: application/json"}) - Call> getOptionalBinaryPresent( - @Header("Authorization") AuthHeader authHeader); + @Headers({"hr-path-template: /binary/optional/present", "Accept: application/octet-stream"}) + @Streaming + Call getOptionalBinaryPresent(@Header("Authorization") AuthHeader authHeader); @GET("./binary/optional/empty") - @Headers({"hr-path-template: /binary/optional/empty", "Accept: application/json"}) - Call> getOptionalBinaryEmpty( - @Header("Authorization") AuthHeader authHeader); + @Headers({"hr-path-template: /binary/optional/empty", "Accept: application/octet-stream"}) + @Streaming + Call getOptionalBinaryEmpty(@Header("Authorization") AuthHeader authHeader); /** Throws an exception after partially writing a binary response. */ @GET("./binary/failure") diff --git a/conjure-java-core/src/main/java/com/palantir/conjure/java/services/JerseyServiceGenerator.java b/conjure-java-core/src/main/java/com/palantir/conjure/java/services/JerseyServiceGenerator.java index c5f653132..8c5a54174 100644 --- a/conjure-java-core/src/main/java/com/palantir/conjure/java/services/JerseyServiceGenerator.java +++ b/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 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(), diff --git a/conjure-java-core/src/main/java/com/palantir/conjure/java/services/Retrofit2ServiceGenerator.java b/conjure-java-core/src/main/java/com/palantir/conjure/java/services/Retrofit2ServiceGenerator.java index 16985f190..a13caa2b3 100644 --- a/conjure-java-core/src/main/java/com/palantir/conjure/java/services/Retrofit2ServiceGenerator.java +++ b/conjure-java-core/src/main/java/com/palantir/conjure/java/services/Retrofit2ServiceGenerator.java @@ -89,7 +89,7 @@ public Retrofit2ServiceGenerator(Set experimentalFeatures) { public Set 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(), diff --git a/conjure-java-core/src/main/java/com/palantir/conjure/java/types/ReturnTypeClassNameVisitor.java b/conjure-java-core/src/main/java/com/palantir/conjure/java/types/ReturnTypeClassNameVisitor.java index 078b7cc49..ba398a118 100644 --- a/conjure-java-core/src/main/java/com/palantir/conjure/java/types/ReturnTypeClassNameVisitor.java +++ b/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 types; private final ClassName binaryClassName; + private final TypeName optionalBinaryTypeName; - public ReturnTypeClassNameVisitor(List types, ClassName binaryClassName) { + public ReturnTypeClassNameVisitor( + List 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); } diff --git a/conjure-java-core/src/test/resources/test/api/TestService.java.jersey b/conjure-java-core/src/test/resources/test/api/TestService.java.jersey index 8fb51bd92..56c4d41b4 100644 --- a/conjure-java-core/src/test/resources/test/api/TestService.java.jersey +++ b/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 maybeGetRawData( + Optional maybeGetRawData( @HeaderParam("Authorization") @NotNull AuthHeader authHeader, @PathParam("datasetRid") @Safe ResourceIdentifier datasetRid); diff --git a/conjure-java-core/src/test/resources/test/api/TestService.java.jersey_require_not_null b/conjure-java-core/src/test/resources/test/api/TestService.java.jersey_require_not_null index 8fb51bd92..56c4d41b4 100644 --- a/conjure-java-core/src/test/resources/test/api/TestService.java.jersey_require_not_null +++ b/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 maybeGetRawData( + Optional maybeGetRawData( @HeaderParam("Authorization") @NotNull AuthHeader authHeader, @PathParam("datasetRid") @Safe ResourceIdentifier datasetRid); diff --git a/conjure-java-core/src/test/resources/test/api/TestServiceRetrofit.java.retrofit b/conjure-java-core/src/test/resources/test/api/TestServiceRetrofit.java.retrofit index eb3c20fb8..e06c90735 100644 --- a/conjure-java-core/src/test/resources/test/api/TestServiceRetrofit.java.retrofit +++ b/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> maybeGetRawData( + @Streaming + Call maybeGetRawData( @Header("Authorization") AuthHeader authHeader, @Path("datasetRid") ResourceIdentifier datasetRid); diff --git a/conjure-java-core/src/test/resources/test/api/TestServiceRetrofit.java.retrofit_completable_future b/conjure-java-core/src/test/resources/test/api/TestServiceRetrofit.java.retrofit_completable_future index a3c143683..0adb78601 100644 --- a/conjure-java-core/src/test/resources/test/api/TestServiceRetrofit.java.retrofit_completable_future +++ b/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> maybeGetRawData( + @Streaming + CompletableFuture maybeGetRawData( @Header("Authorization") AuthHeader authHeader, @Path("datasetRid") ResourceIdentifier datasetRid); diff --git a/conjure-java-core/src/test/resources/test/api/TestServiceRetrofit.java.retrofit_listenable_future b/conjure-java-core/src/test/resources/test/api/TestServiceRetrofit.java.retrofit_listenable_future index e938cea1c..76f449a93 100644 --- a/conjure-java-core/src/test/resources/test/api/TestServiceRetrofit.java.retrofit_listenable_future +++ b/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> maybeGetRawData( + @Streaming + ListenableFuture maybeGetRawData( @Header("Authorization") AuthHeader authHeader, @Path("datasetRid") ResourceIdentifier datasetRid);