Address comments

palantir · Nov 23, 2023 · 938a5fc · 938a5fc
1 parent d31847a
commit 938a5fc
Show file tree

Hide file tree

Showing 10 changed files with 140 additions and 99 deletions.
diff --git a/dialogue-annotations-example/src/main/java/com/palantir/myservice/example/MyService.java b/dialogue-annotations-example/src/main/java/com/palantir/myservice/example/MyService.java
@@ -48,6 +48,7 @@ String greet(
     @Request(method = HttpMethod.GET, path = "/greeting", accept = CustomStringDeserializer.class)
     ListenableFuture<String> getGreetingAsync();
 
+    @MustBeClosed
     @Request(method = HttpMethod.GET, path = "/input-stream")
     InputStream inputStream();
 

diff --git a/...ations-example/src/test/java/com/palantir/myservice/example/MyServiceIntegrationTest.java b/...ations-example/src/test/java/com/palantir/myservice/example/MyServiceIntegrationTest.java
@@ -49,6 +49,7 @@
 import io.undertow.util.Headers;
 import io.undertow.util.HttpString;
 import java.io.IOException;
+import java.io.InputStream;
 import java.io.InputStreamReader;
 import java.io.OutputStream;
 import java.io.PrintWriter;
@@ -196,6 +197,24 @@ public void testGetGreetingAsyncRemoteException() {
                 });
     }
 
+    @Test
+    public void testInputStream() throws IOException {
+        undertowHandler = exchange -> {
+            exchange.assertMethod(HttpMethod.GET);
+            exchange.assertPath("/input-stream");
+            exchange.assertAccept().isEqualTo("application/octet-stream");
+            exchange.assertContentType().isNull();
+
+            exchange.exchange.setStatusCode(200);
+            exchange.setContentType("application/octet-stream");
+            exchange.writeStringBody("Hello");
+        };
+        try (InputStream inputStream = myServiceDialogue.inputStream()) {
+            assertThat(new String(inputStream.readAllBytes(), StandardCharsets.UTF_8))
+                    .isEqualTo("Hello");
+        }
+    }
+
     @Test
     public void testCustomRequest() {
         undertowHandler = exchange -> {

diff --git a/...-processor/src/main/java/com/palantir/dialogue/annotations/processor/data/ReturnType.java b/...-processor/src/main/java/com/palantir/dialogue/annotations/processor/data/ReturnType.java
@@ -16,9 +16,7 @@
 
 package com.palantir.dialogue.annotations.processor.data;
 
-import com.squareup.javapoet.ClassName;
 import com.squareup.javapoet.TypeName;
-import java.io.InputStream;
 import java.util.Optional;
 import org.immutables.value.Value;
 
@@ -29,10 +27,7 @@ public interface ReturnType {
 
     TypeName deserializerFactory();
 
-    @Value.Default
-    default boolean isUsingCustomDeserializer() {
-        return false;
-    }
+    boolean deserializerUsesBodySerDe();
 
     TypeName errorDecoder();
 
@@ -45,10 +40,4 @@ default boolean isVoid() {
         TypeName type = asyncInnerType().orElseGet(this::returnType);
         return type.box().equals(TypeName.VOID.box());
     }
-
-    @Value.Derived
-    default boolean isInputStream() {
-        TypeName type = asyncInnerType().orElseGet(this::returnType);
-        return type.equals(ClassName.get(InputStream.class));
-    }
 }
diff --git a/...r/src/main/java/com/palantir/dialogue/annotations/processor/data/ReturnTypesResolver.java b/...r/src/main/java/com/palantir/dialogue/annotations/processor/data/ReturnTypesResolver.java
@@ -21,9 +21,11 @@
 import com.google.errorprone.annotations.MustBeClosed;
 import com.palantir.dialogue.Response;
 import com.palantir.dialogue.annotations.ConjureErrorDecoder;
+import com.palantir.dialogue.annotations.InputStreamDeserializer;
 import com.palantir.dialogue.annotations.Json;
 import com.palantir.dialogue.annotations.ResponseDeserializer;
 import com.squareup.javapoet.TypeName;
+import java.io.InputStream;
 import java.util.Optional;
 import javax.lang.model.element.Element;
 import javax.lang.model.element.ExecutableElement;
@@ -50,28 +52,26 @@ public Optional<ReturnType> getReturnType(
                 requestAnnotation.getFieldMaybe("accept", TypeMirror.class);
         Optional<TypeMirror> maybeErrorDecoder = requestAnnotation.getFieldMaybe("errorDecoder", TypeMirror.class);
 
-        Optional<TypeName> maybeDeserializerFactory = maybeAcceptDeserializerFactory
-                .map(TypeName::get)
-                .or(() -> orDefaultDeserializerFactory(
-                        hasMustBeClosed, element, returnType, maybeListenableFutureInnerType));
+        Optional<TypeMirror> maybeDeserializerFactory = maybeAcceptDeserializerFactory.or(() ->
+                orDefaultDeserializerFactory(hasMustBeClosed, element, returnType, maybeListenableFutureInnerType));
 
         if (maybeDeserializerFactory.isEmpty()) {
             return Optional.empty();
         }
 
         return Optional.of(ImmutableReturnType.builder()
                 .returnType(TypeName.get(returnType))
-                .deserializerFactory(maybeDeserializerFactory.get())
+                .deserializerFactory(TypeName.get(maybeDeserializerFactory.get()))
+                .deserializerUsesBodySerDe(usesBodySerde(maybeDeserializerFactory.get()))
                 .errorDecoder(maybeErrorDecoder
                         .map(TypeName::get)
                         .orElseGet(() -> context.getTypeName(ConjureErrorDecoder.class)))
                 .deserializerFieldName(InstanceVariables.joinCamelCase(endpointName.get(), "Deserializer"))
                 .asyncInnerType(maybeListenableFutureInnerType.map(TypeName::get))
-                .isUsingCustomDeserializer(maybeAcceptDeserializerFactory.isPresent())
                 .build());
     }
 
-    private Optional<TypeName> orDefaultDeserializerFactory(
+    private Optional<TypeMirror> orDefaultDeserializerFactory(
             boolean hasMustBeClosed,
             Element element,
             TypeMirror returnType,
@@ -83,15 +83,29 @@ private Optional<TypeName> orDefaultDeserializerFactory(
                 context.reportError("When returning raw Response, remember to add @MustBeClosed annotation", element);
                 return Optional.empty();
             }
-            return Optional.of(context.getTypeName(ResponseDeserializer.class));
+            return Optional.of(context.getTypeMirror(ResponseDeserializer.class));
+        } else if (isInputStreamType(returnType)) {
+            if (!hasMustBeClosed) {
+                context.reportError("When returning InputStream, remember to add @MustBeClosed annotation", element);
+                return Optional.empty();
+            }
+            return Optional.of(context.getTypeMirror(InputStreamDeserializer.class));
         }
-        return Optional.of(context.getTypeName(Json.class));
+        return Optional.of(context.getTypeMirror(Json.class));
     }
 
     private boolean isResponseType(TypeMirror type) {
         return context.isSameTypes(type, Response.class);
     }
 
+    private boolean isInputStreamType(TypeMirror type) {
+        return context.isSameTypes(type, InputStream.class);
+    }
+
+    private boolean usesBodySerde(TypeMirror type) {
+        return context.isSameTypes(type, InputStreamDeserializer.class);
+    }
+
     private Optional<TypeMirror> getListenableFutureInnerType(TypeMirror typeName) {
         return context.maybeAsDeclaredType(typeName)
                 .flatMap(declaredType -> context.getGenericInnerType(ListenableFuture.class, declaredType));

diff --git a/.../com/palantir/dialogue/annotations/processor/generate/ServiceImplementationGenerator.java b/.../com/palantir/dialogue/annotations/processor/generate/ServiceImplementationGenerator.java
@@ -22,7 +22,6 @@
 import com.palantir.dialogue.Serializer;
 import com.palantir.dialogue.TypeMarker;
 import com.palantir.dialogue.annotations.DefaultParameterSerializer;
-import com.palantir.dialogue.annotations.ErrorHandlingDeserializer;
 import com.palantir.dialogue.annotations.ErrorHandlingDeserializerFactory;
 import com.palantir.dialogue.annotations.ErrorHandlingVoidDeserializer;
 import com.palantir.dialogue.annotations.ParameterSerializer;
@@ -167,29 +166,30 @@ private Optional<FieldSpec> deserializer(ReturnType type) {
         ParameterizedTypeName deserializerType =
                 ParameterizedTypeName.get(ClassName.get(Deserializer.class), innerType);
 
-        CodeBlock deserializer = CodeBlock.of(
-                "new $T<>(new $T(), new $T()).deserializerFor(new $T<$T>() {})",
-                ErrorHandlingDeserializerFactory.class,
-                deserializerFactoryType,
-                errorDecoderType,
-                TypeMarker.class,
-                innerType);
-
-        // If no custom deserializer is used, use the runtime-provided deserializers for specific types
-        if (!type.isUsingCustomDeserializer()) {
-            if (type.isVoid()) {
-                deserializer = CodeBlock.of(
-                        "new $T($L.bodySerDe().emptyBodyDeserializer(), new $T())",
-                        ErrorHandlingVoidDeserializer.class,
-                        serviceDefinition.conjureRuntimeArgName(),
-                        errorDecoderType);
-            } else if (type.isInputStream()) {
-                deserializer = CodeBlock.of(
-                        "new $T<>($L.bodySerDe().inputStreamDeserializer(), new $T())",
-                        ErrorHandlingDeserializer.class,
-                        serviceDefinition.conjureRuntimeArgName(),
-                        errorDecoderType);
-            }
+        final CodeBlock deserializer;
+        if (type.isVoid()) {
+            deserializer = CodeBlock.of(
+                    "new $T($L.bodySerDe().emptyBodyDeserializer(), new $T())",
+                    ErrorHandlingVoidDeserializer.class,
+                    serviceDefinition.conjureRuntimeArgName(),
+                    errorDecoderType);
+        } else if (type.deserializerUsesBodySerDe()) {
+            deserializer = CodeBlock.of(
+                    "new $T<>(new $T($L.bodySerDe()), new $T()).deserializerFor(new $T<$T>() {})",
+                    ErrorHandlingDeserializerFactory.class,
+                    deserializerFactoryType,
+                    serviceDefinition.conjureRuntimeArgName(),
+                    errorDecoderType,
+                    TypeMarker.class,
+                    innerType);
+        } else {
+            deserializer = CodeBlock.of(
+                    "new $T<>(new $T(), new $T()).deserializerFor(new $T<$T>() {})",
+                    ErrorHandlingDeserializerFactory.class,
+                    deserializerFactoryType,
+                    errorDecoderType,
+                    TypeMarker.class,
+                    innerType);
         }
 
         return Optional.of(FieldSpec.builder(deserializerType, type.deserializerFieldName())

diff --git a/dialogue-annotations-processor/src/test/java/com/palantir/myservice/service/MyService.java b/dialogue-annotations-processor/src/test/java/com/palantir/myservice/service/MyService.java
@@ -48,9 +48,11 @@ String greet(
     @Request(method = HttpMethod.GET, path = "/greeting", accept = CustomStringDeserializer.class)
     ListenableFuture<String> getGreetingAsync();
 
+    @MustBeClosed
     @Request(method = HttpMethod.GET, path = "/input-stream")
     InputStream inputStream();
 
+    @MustBeClosed
     @Request(method = HttpMethod.GET, path = "/input-stream-custom", accept = CustomInputStreamDeserializer.class)
     InputStream customInputStream();
 

diff --git a/...t/resources/com/palantir/myservice/service/MyServiceDialogueServiceFactory.java.generated b/...t/resources/com/palantir/myservice/service/MyServiceDialogueServiceFactory.java.generated
@@ -17,9 +17,9 @@ import com.palantir.dialogue.TypeMarker;
 import com.palantir.dialogue.UrlBuilder;
 import com.palantir.dialogue.annotations.ConjureErrorDecoder;
 import com.palantir.dialogue.annotations.DefaultParameterSerializer;
-import com.palantir.dialogue.annotations.ErrorHandlingDeserializer;
 import com.palantir.dialogue.annotations.ErrorHandlingDeserializerFactory;
 import com.palantir.dialogue.annotations.ErrorHandlingVoidDeserializer;
+import com.palantir.dialogue.annotations.InputStreamDeserializer;
 import com.palantir.dialogue.annotations.Json;
 import com.palantir.dialogue.annotations.ParameterSerializer;
 import com.palantir.dialogue.annotations.ResponseDeserializer;
@@ -61,8 +61,9 @@ public final class MyServiceDialogueServiceFactory implements DialogueServiceFac
 
             private final EndpointChannel inputStreamChannel = endpointChannelFactory.endpoint(Endpoints.inputStream);
 
-            private final Deserializer<InputStream> inputStreamDeserializer = new ErrorHandlingDeserializer<>(
-                    runtime.bodySerDe().inputStreamDeserializer(), new ConjureErrorDecoder());
+            private final Deserializer<InputStream> inputStreamDeserializer = new ErrorHandlingDeserializerFactory<>(
+                            new InputStreamDeserializer(runtime.bodySerDe()), new ConjureErrorDecoder())
+                    .deserializerFor(new TypeMarker<InputStream>() {});
 
             private final EndpointChannel customInputStreamChannel =
                     endpointChannelFactory.endpoint(Endpoints.customInputStream);

diff --git a/...nnotations/src/main/java/com/palantir/dialogue/annotations/ErrorHandlingDeserializer.java b/...nnotations/src/main/java/com/palantir/dialogue/annotations/ErrorHandlingDeserializer.java
diff --git a/...ations/src/main/java/com/palantir/dialogue/annotations/ErrorHandlingVoidDeserializer.java b/...ations/src/main/java/com/palantir/dialogue/annotations/ErrorHandlingVoidDeserializer.java
@@ -17,10 +17,33 @@
 package com.palantir.dialogue.annotations;
 
 import com.palantir.dialogue.Deserializer;
+import com.palantir.dialogue.Response;
+import com.palantir.logsafe.Preconditions;
+import java.util.Optional;
 
-public final class ErrorHandlingVoidDeserializer extends ErrorHandlingDeserializer<Void> {
+public final class ErrorHandlingVoidDeserializer implements Deserializer<Void> {
+
+    private final Deserializer<Void> delegate;
+    private final ErrorDecoder errorDecoder;
 
     public ErrorHandlingVoidDeserializer(Deserializer<Void> delegate, ErrorDecoder errorDecoder) {
-        super(delegate, errorDecoder);
+        this.delegate = Preconditions.checkNotNull(delegate, "delegate");
+        this.errorDecoder = Preconditions.checkNotNull(errorDecoder, "errorDecoder");
+    }
+
+    @Override
+    public Void deserialize(Response response) {
+        try (response) {
+            if (errorDecoder.isError(response)) {
+                throw errorDecoder.decode(response);
+            } else {
+                return delegate.deserialize(response);
+            }
+        }
+    }
+
+    @Override
+    public Optional<String> accepts() {
+        return delegate.accepts();
     }
 }
diff --git a/...-annotations/src/main/java/com/palantir/dialogue/annotations/InputStreamDeserializer.java b/...-annotations/src/main/java/com/palantir/dialogue/annotations/InputStreamDeserializer.java
@@ -0,0 +1,41 @@
+/*
+ * (c) Copyright 2023 Palantir Technologies Inc. All rights reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.palantir.dialogue.annotations;
+
+import com.palantir.dialogue.BodySerDe;
+import com.palantir.dialogue.Response;
+import java.io.InputStream;
+
+public final class InputStreamDeserializer extends StdDeserializer<InputStream> {
+
+    private final BodySerDe bodySerDe;
+
+    public InputStreamDeserializer(BodySerDe bodySerDe) {
+        super(bodySerDe.inputStreamDeserializer().accepts().orElse("application/octet-stream"));
+        this.bodySerDe = bodySerDe;
+    }
+
+    @Override
+    public InputStream deserialize(Response response) {
+        return bodySerDe.inputStreamDeserializer().deserialize(response);
+    }
+
+    @Override
+    public String toString() {
+        return "InputStreamDeserializer{}";
+    }
+}