fixes #468: Dialogue clients send Accept headers (#482)

Dialogue clients send Accept headers

.palantir/revapi.yml

@@ -159,3 +159,10 @@ acceptedBreaks:
       old: "method java.util.Optional<java.lang.String> com.palantir.dialogue.PlainSerDe::serializeOptionalUuid(java.util.Optional<java.util.UUID>)"
       new: null
       justification: "unused methods"
+  "0.13.0":
+    com.palantir.dialogue:dialogue-blocking-channels: []
+    com.palantir.dialogue:dialogue-target:
+    - code: "java.method.addedToInterface"
+      old: null
+      new: "method com.palantir.dialogue.Deserializer<java.io.InputStream> com.palantir.dialogue.BodySerDe::inputStreamDeserializer()"
+      justification: "fixin' apis"

changelog/@unreleased/pr-482.v2.yml

@@ -0,0 +1,5 @@
+type: improvement
+improvement:
+  description: Dialogue clients send Accept headers
+  links:
+  - https://github.com/palantir/dialogue/pull/482

dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/BinaryEncoding.java

@@ -0,0 +1,60 @@
+/*
+ * (c) Copyright 2020 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.conjure.java.dialogue.serde;
+
+import com.palantir.dialogue.TypeMarker;
+import com.palantir.logsafe.Preconditions;
+import com.palantir.logsafe.SafeArg;
+import java.io.InputStream;
+
+/**
+ * Package-private internal api.
+ * This partial Encoding implementation exists to allow binary responses to share the same safety
+ * and validation provided by structured encodings. This is only consumed internally to create
+ * a binary-specific <pre>EncodingDeserializerRegistry</pre>.
+ */
+enum BinaryEncoding implements Encoding {
+    INSTANCE;
+
+    static final String CONTENT_TYPE = "application/octet-stream";
+    static final TypeMarker<InputStream> MARKER = new TypeMarker<InputStream>() {};
+
+    @Override
+    public <T> Serializer<T> serializer(TypeMarker<T> _type) {
+        throw new UnsupportedOperationException("BinaryEncoding does not support serializers");
+    }
+
+    @Override
+    @SuppressWarnings("unchecked")
+    public <T> Deserializer<T> deserializer(TypeMarker<T> type) {
+        Preconditions.checkArgument(
+                InputStream.class.equals(type.getType()),
+                "BinaryEncoding only supports InputStream",
+                SafeArg.of("requested", type));
+        return input -> (T) input;
+    }
+
+    @Override
+    public String getContentType() {
+        return CONTENT_TYPE;
+    }
+
+    @Override
+    public boolean supportsContentType(String contentType) {
+        return Encodings.matchesContentType(CONTENT_TYPE, contentType);
+    }
+}

dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/ConjureBodySerDe.java

@@ -34,15 +34,16 @@
 import java.io.OutputStream;
 import java.util.List;
 import java.util.Optional;
+import java.util.stream.Collectors;
 import javax.ws.rs.core.HttpHeaders;
 
 /** Package private internal API. */
 final class ConjureBodySerDe implements BodySerDe {
-    private static final String BINARY_CONTENT_TYPE = "application/octet-stream";
 
     private final List<Encoding> encodings;
     private final ErrorDecoder errorDecoder;
     private final Encoding defaultEncoding;
+    private final Deserializer<InputStream> binaryInputStreamDeserializer;
 
     /**
      * Selects the first (based on input order) of the provided encodings that
@@ -61,6 +62,8 @@ final class ConjureBodySerDe implements BodySerDe {
         this.errorDecoder = errorDecoder;
         Preconditions.checkArgument(encodings.size() > 0, "At least one Encoding is required");
         this.defaultEncoding = encodings.get(0);
+        this.binaryInputStreamDeserializer = new EncodingDeserializerRegistry<>(
+                ImmutableList.of(BinaryEncoding.INSTANCE), errorDecoder, BinaryEncoding.MARKER);
     }
 
     @Override
@@ -74,13 +77,13 @@ public <T> Deserializer<T> deserializer(TypeMarker<T> token) {
     }
 
     @Override
-    @SuppressWarnings("NullAway") // empty body is a special case
     public Deserializer<Void> emptyBodyDeserializer() {
-        return input -> {
-            // We should not fail if a server that previously returned nothing starts returning a response
-            input.close();
-            return null;
-        };
+        return EmptyBodyDeserializer.INSTANCE;
+    }
+
+    @Override
+    public Deserializer<InputStream> inputStreamDeserializer() {
+        return binaryInputStreamDeserializer;
     }
 
     @Override
@@ -95,23 +98,11 @@ public void writeTo(OutputStream output) throws IOException {
 
             @Override
             public String contentType() {
-                return BINARY_CONTENT_TYPE;
+                return BinaryEncoding.CONTENT_TYPE;
             }
         };
     }
 
-    @Override
-    public InputStream deserializeInputStream(Response exchange) {
-        Optional<String> contentType = exchange.getFirstHeader(HttpHeaders.CONTENT_TYPE);
-        if (!contentType.isPresent()) {
-            throw new SafeIllegalArgumentException("Response is missing Content-Type header");
-        }
-        if (!contentType.get().startsWith(BINARY_CONTENT_TYPE)) {
-            throw new SafeIllegalArgumentException("Unsupported Content-Type", SafeArg.of("Content-Type", contentType));
-        }
-        return exchange.body();
-    }
-
     private static final class EncodingSerializerRegistry<T> implements Serializer<T> {
 
         private final EncodingSerializerContainer<T> encoding;
@@ -154,12 +145,16 @@ private static final class EncodingDeserializerRegistry<T> implements Deserializ
 
         private final ImmutableList<EncodingDeserializerContainer<T>> encodings;
         private final ErrorDecoder errorDecoder;
+        private final Optional<String> acceptValue;
 
         EncodingDeserializerRegistry(List<Encoding> encodings, ErrorDecoder errorDecoder, TypeMarker<T> token) {
             this.encodings = encodings.stream()
                     .map(encoding -> new EncodingDeserializerContainer<>(encoding, token))
                     .collect(ImmutableList.toImmutableList());
             this.errorDecoder = errorDecoder;
+            // Encodings are applied to the accept header in the order of preference based on the provided list.
+            this.acceptValue =
+                    Optional.of(encodings.stream().map(Encoding::getContentType).collect(Collectors.joining(", ")));
         }
 
         @Override
@@ -177,6 +172,11 @@ public T deserialize(Response response) {
             }
         }
 
+        @Override
+        public Optional<String> accepts() {
+            return acceptValue;
+        }
+
         /** Returns the {@link EncodingDeserializerContainer} to use to deserialize the request body. */
         @SuppressWarnings("ForLoopReplaceableByForEach")
         // performance sensitive code avoids iterator allocation
@@ -204,4 +204,21 @@ private static final class EncodingDeserializerContainer<T> {
             this.deserializer = encoding.deserializer(token);
         }
     }
+
+    private enum EmptyBodyDeserializer implements Deserializer<Void> {
+        INSTANCE;
+
+        @Override
+        @SuppressWarnings("NullAway") // empty body is a special case
+        public Void deserialize(Response response) {
+            // We should not fail if a server that previously returned nothing starts returning a response
+            response.close();
+            return null;
+        }
+
+        @Override
+        public Optional<String> accepts() {
+            return Optional.empty();
+        }
+    }
 }

dialogue-serde/src/main/java/com/palantir/conjure/java/dialogue/serde/DefaultClients.java

@@ -16,6 +16,7 @@
 
 package com.palantir.conjure.java.dialogue.serde;
 
+import com.google.common.net.HttpHeaders;
 import com.google.common.util.concurrent.ExecutionError;
 import com.google.common.util.concurrent.FutureCallback;
 import com.google.common.util.concurrent.Futures;
@@ -29,10 +30,13 @@
 import com.palantir.dialogue.Deserializer;
 import com.palantir.dialogue.Endpoint;
 import com.palantir.dialogue.Request;
+import com.palantir.logsafe.Preconditions;
+import com.palantir.logsafe.SafeArg;
 import com.palantir.logsafe.UnsafeArg;
 import com.palantir.logsafe.exceptions.SafeRuntimeException;
 import java.io.Closeable;
 import java.io.IOException;
+import java.util.Optional;
 import java.util.concurrent.ExecutionException;
 import org.checkerframework.checker.nullness.qual.Nullable;
 import org.slf4j.Logger;
@@ -47,8 +51,10 @@ enum DefaultClients implements Clients {
     @Override
     public <T> ListenableFuture<T> call(
             Channel channel, Endpoint endpoint, Request request, Deserializer<T> deserializer) {
+        Optional<String> accepts = deserializer.accepts();
+        Request outgoingRequest = accepts.isPresent() ? accepting(request, accepts.get()) : request;
         return Futures.transform(
-                channel.execute(endpoint, request), deserializer::deserialize, MoreExecutors.directExecutor());
+                channel.execute(endpoint, outgoingRequest), deserializer::deserialize, MoreExecutors.directExecutor());
     }
 
     @Override
@@ -120,4 +126,20 @@ public void onFailure(Throwable throwable) {
             log.info("Canceled call failed", throwable);
         }
     }
+
+    private static Request accepting(Request original, String acceptValue) {
+        Preconditions.checkNotNull(acceptValue, "Accept value is required");
+        Preconditions.checkState(!acceptValue.isEmpty(), "Accept value must not be empty");
+        if (original.headerParams().containsKey(HttpHeaders.ACCEPT)) {
+            log.warn(
+                    "Request {} already contains an Accept header value {}",
+                    UnsafeArg.of("request", original),
+                    SafeArg.of("existingAcceptValue", original.headerParams().get(HttpHeaders.ACCEPT)));
+            return original;
+        }
+        return Request.builder()
+                .from(original)
+                .putHeaderParams(HttpHeaders.ACCEPT, acceptValue)
+                .build();
+    }
 }

dialogue-serde/src/test/java/com/palantir/conjure/java/dialogue/serde/DefaultClientsTest.java

@@ -17,19 +17,26 @@
 package com.palantir.conjure.java.dialogue.serde;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
+import com.google.common.net.HttpHeaders;
+import com.google.common.util.concurrent.Futures;
 import com.google.common.util.concurrent.ListenableFuture;
 import com.google.common.util.concurrent.SettableFuture;
 import com.palantir.dialogue.Channel;
 import com.palantir.dialogue.Deserializer;
 import com.palantir.dialogue.Endpoint;
 import com.palantir.dialogue.Request;
 import com.palantir.dialogue.Response;
+import java.util.Optional;
 import java.util.concurrent.ExecutionException;
 import org.junit.Test;
 import org.junit.runner.RunWith;
+import org.mockito.ArgumentCaptor;
+import org.mockito.Captor;
 import org.mockito.Mock;
 import org.mockito.junit.MockitoJUnitRunner;
 
@@ -48,6 +55,9 @@ public final class DefaultClientsTest {
     @Mock
     private Deserializer<String> deserializer;
 
+    @Captor
+    private ArgumentCaptor<Request> requestCaptor;
+
     @Test
     public void testCall() throws ExecutionException, InterruptedException {
         Request request = Request.builder().build();
@@ -60,4 +70,22 @@ public void testCall() throws ExecutionException, InterruptedException {
         assertThat(result).isDone();
         assertThat(result.get()).isEqualTo("value");
     }
+
+    @Test
+    public void testAddsAcceptHeader() throws ExecutionException, InterruptedException {
+        String expectedAccept = "application/json";
+        Request request = Request.builder().build();
+
+        when(deserializer.deserialize(eq(response))).thenReturn("value");
+        when(deserializer.accepts()).thenReturn(Optional.of(expectedAccept));
+        when(channel.execute(eq(endpoint), any())).thenReturn(Futures.immediateFuture(response));
+
+        ListenableFuture<String> result = DefaultClients.INSTANCE.call(channel, endpoint, request, deserializer);
+
+        assertThat(result).isDone();
+        assertThat(result.get()).isEqualTo("value");
+        verify(channel).execute(eq(endpoint), requestCaptor.capture());
+        assertThat(requestCaptor.getValue().headerParams().get(HttpHeaders.ACCEPT))
+                .containsExactly(expectedAccept);
+    }
 }

dialogue-target/src/main/java/com/palantir/dialogue/BodySerDe.java

@@ -32,6 +32,14 @@ public interface BodySerDe {
      */
     Deserializer<Void> emptyBodyDeserializer();
 
+    /**
+     * Returns a {@link Deserializer} that reads an {@link InputStream} from the {@link Response} body.
+     * <p>
+     * This method is named <pre>inputStreamDeserializer</pre> not <pre>binaryDeserializer</pre>
+     * to support future streaming binary bindings without conflicting method signatures.
+     */
+    Deserializer<InputStream> inputStreamDeserializer();
+
     /** Serializes a {@link BinaryRequestBody} to <pre>application/octet-stream</pre>. */
     RequestBody serialize(BinaryRequestBody value);
 
@@ -40,6 +48,11 @@ public interface BodySerDe {
      * <p>
      * This method is named <pre>deserializeInputStream</pre> not <pre>deserializeBinary</pre>
      * to support future streaming binary bindings without conflicting method signatures.
+     *
+     * @deprecated Prefer {@link #inputStreamDeserializer()}
      */
-    InputStream deserializeInputStream(Response response);
+    @Deprecated
+    default InputStream deserializeInputStream(Response response) {
+        return inputStreamDeserializer().deserialize(response);
+    }
 }

dialogue-target/src/main/java/com/palantir/dialogue/Deserializer.java

@@ -16,9 +16,22 @@
 
 package com.palantir.dialogue;
 
+import java.util.Optional;
+
 /** Reads objects from a response. */
 public interface Deserializer<T> {
 
     /** Deserializes the response body. */
     T deserialize(Response response);
+
+    /**
+     * Returns the content types this deserializer accepts, if any.
+     * Values are structured for the <pre>Accept</pre> header based
+     * on <a href="https://tools.ietf.org/html/rfc7231#section-5.3.2">rfc 7231 section 5.3.2</a>.
+     */
+    default Optional<String> accepts() {
+        // TODO(ckozak): This should not be a functional interface. This method is default
+        // only to allow the generator to be updated to avoid deserializer method references.
+        return Optional.empty();
+    }
 }