Apply review feedback to the multi-encoder and multi-decoder surface

core/src/main/java/feign/Util.java

@@ -69,6 +69,9 @@ public class Util {
   /** The HTTP Accept-Encoding header field name. */
   public static final String ACCEPT_ENCODING = "Accept-Encoding";
 
+  /** The HTTP Accept header field name. */
+  public static final String ACCEPT = "Accept";
+
   /** The HTTP Retry-After header field name. */
   public static final String RETRY_AFTER = "Retry-After";
 

core/src/main/java/feign/codec/DefaultDecoder.java

@@ -22,6 +22,19 @@
 
 public class DefaultDecoder extends StringDecoder {
 
+  /**
+   * Accepts exactly what {@link #decode} handles: everything {@link StringDecoder} accepts, plus a
+   * {@code byte[]} return type.
+   *
+   * @param response {@inheritDoc}
+   * @param type {@inheritDoc}
+   * @return {@inheritDoc}
+   */
+  @Override
+  public boolean canDecode(Response response, Type type) {
+    return byte[].class.equals(type) || super.canDecode(response, type);
+  }
+
   @Override
   public Object decode(Response response, Type type) throws IOException {
     if (response.status() == 404 || response.status() == 204) return Util.emptyValueOf(type);

core/src/main/java/feign/codec/DefaultEncoder.java

@@ -20,7 +20,21 @@
 import feign.RequestTemplate;
 import java.lang.reflect.Type;
 
-public class DefaultEncoder implements Encoder {
+public class DefaultEncoder implements PredicatedEncoder {
+
+  /**
+   * Accepts exactly what {@link #encode} handles: a {@code String} or {@code byte[]} body, and a
+   * null body, which is sent as no body at all.
+   *
+   * @param object {@inheritDoc}
+   * @param bodyType {@inheritDoc}
+   * @param template {@inheritDoc}
+   * @return {@inheritDoc}
+   */
+  @Override
+  public boolean canEncode(Object object, Type bodyType, RequestTemplate template) {
+    return bodyType == String.class || bodyType == byte[].class || object == null;
+  }
 
   @Override
   public void encode(Object object, Type bodyType, RequestTemplate template) {

core/src/main/java/feign/codec/MultiDecoder.java

@@ -17,6 +17,7 @@
 
 import feign.Experimental;
 import feign.FeignException;
+import feign.Request;
 import feign.Response;
 import feign.Util;
 import java.io.IOException;
@@ -52,11 +53,20 @@
  * naming what was tried. Add a decoder guarded by {@link DecoderPredicate#any()} last to act as a
  * default, as above.
  *
+ * <p>A multi-decoder is itself a {@link PredicatedDecoder}, accepting whatever any of its decoders
+ * accepts, so one can be added to another. That is how a library ships a set of decoders as a
+ * single unit: given a hypothetical {@code AcmeFeign.decoders()} returning a multi-decoder over
+ * that library's decoders, the whole set is added in one go:
+ *
+ * <pre>
+ * Feign.builder().decoders(AcmeFeign.decoders(), new JacksonDecoder());
+ * </pre>
+ *
  * @see PredicatedDecoder
  * @see DecoderPredicate
  */
 @Experimental
-public class MultiDecoder implements Decoder {
+public class MultiDecoder implements PredicatedDecoder {
 
   private final List<PredicatedDecoder> decoders;
 
@@ -69,6 +79,18 @@ public static Builder builder() {
     return new Builder();
   }
 
+  /**
+   * Whether any of the decoders accepts the response.
+   *
+   * @param response {@inheritDoc}
+   * @param type {@inheritDoc}
+   * @return {@inheritDoc}
+   */
+  @Override
+  public boolean canDecode(Response response, Type type) {
+    return decoders.stream().anyMatch(decoder -> decoder.canDecode(response, type));
+  }
+
   /**
    * Decodes using the first decoder that accepts the response.
    *
@@ -95,31 +117,61 @@ private String unableToDecode(Response response, Type type) {
     StringBuilder message =
         new StringBuilder("Unable to decode ")
             .append(response.status())
-            .append(" response (Content-Type: ")
-            .append(contentTypes(response))
+            .append(" response (")
+            .append(headers(response))
             .append(") as ")
-            .append(type == null ? "the expected type" : type.getTypeName());
-    if (decoders.isEmpty()) {
-      return message.append(". No decoders were configured.").toString();
-    }
-    message.append(". Decoders tried, in order:");
-    for (PredicatedDecoder decoder : decoders) {
-      message.append("\n  - ").append(PairedDecoder.describe(decoder));
-    }
+            .append(type == null ? "the expected type" : type.getTypeName())
+            .append(". Decoders tried, in order:");
+    appendTo(message, "\n  ");
     return message
-        .append("\nAdd a decoder guarded by DecoderPredicate.any() last to act as a default.")
+        .append("\nRegister a decoder that accepts it, or add a catch-all")
+        .append(" (DecoderPredicate.any()) last.")
         .toString();
   }
 
-  private static String contentTypes(Response response) {
-    String contentTypes =
-        response.headers().entrySet().stream()
-            .filter(header -> Util.CONTENT_TYPE.equalsIgnoreCase(header.getKey()))
+  /**
+   * Lists the decoders one per line, unfolding nested multi-decoders so that a set contributed as a
+   * single unit still shows what it contains.
+   */
+  private void appendTo(StringBuilder message, String indent) {
+    for (PredicatedDecoder decoder : decoders) {
+      if (decoder instanceof MultiDecoder) {
+        message.append(indent).append("- MultiDecoder:");
+        ((MultiDecoder) decoder).appendTo(message, indent + "  ");
+      } else {
+        message.append(indent).append("- ").append(PairedDecoder.describe(decoder));
+      }
+    }
+  }
+
+  /**
+   * The headers a decoder is most likely to have been chosen on: what came back, and what was asked
+   * for. Everything else a predicate looks at belongs in that predicate's own description, which is
+   * listed alongside it.
+   */
+  private static String headers(Response response) {
+    String contentType = header(response.headers(), Util.CONTENT_TYPE);
+    StringBuilder headers =
+        new StringBuilder(Util.CONTENT_TYPE)
+            .append(": ")
+            .append(contentType == null ? "not set" : contentType);
+    Request request = response.request();
+    String accept = request == null ? null : header(request.headers(), Util.ACCEPT);
+    if (accept != null) {
+      headers.append(", ").append(Util.ACCEPT).append(": ").append(accept);
+    }
+    return headers.toString();
+  }
+
+  private static String header(Map<String, Collection<String>> headers, String name) {
+    String values =
+        headers.entrySet().stream()
+            .filter(header -> name.equalsIgnoreCase(header.getKey()))
             .map(Map.Entry::getValue)
             .filter(Objects::nonNull)
             .flatMap(Collection::stream)
             .collect(Collectors.joining(", "));
-    return contentTypes.isEmpty() ? "not set" : contentTypes;
+    return values.isEmpty() ? null : values;
   }
 
   @Override
@@ -148,7 +200,10 @@ public Builder add(PredicatedDecoder decoder) {
 
     /**
      * Adds any decoder, guarded by the given predicate. Use this for decoders that do not implement
-     * {@link PredicatedDecoder}, including ones you do not control.
+     * {@link PredicatedDecoder}, including ones you do not control. The predicate is the whole
+     * answer: whatever the decoder may declare about itself is replaced, so this can widen a
+     * decoder as well as narrow it. Use {@link #narrow(DecoderPredicate, Decoder)} to keep the
+     * decoder's own declaration.
      *
      * @param predicate decides whether the decoder handles a response
      * @param decoder the decoder to delegate to
@@ -157,8 +212,34 @@ public Builder add(DecoderPredicate predicate, Decoder decoder) {
       return add(PredicatedDecoder.of(predicate, decoder));
     }
 
-    /** Builds the multi-decoder. */
+    /**
+     * Adds a decoder, narrowed by the given predicate. If the decoder is itself a {@link
+     * PredicatedDecoder}, the predicate applies in addition to the decoder's own {@code canDecode}
+     * rather than instead of it: both have to accept the response.
+     *
+     * <pre>
+     * MultiDecoder.builder()
+     *     .narrow(DecoderPredicate.status(200), new JacksonDecoder())
+     *     .add(DecoderPredicate.any(), new DefaultDecoder())
+     *     .build();
+     * </pre>
+     *
+     * @param predicate narrows what the decoder handles
+     * @param decoder the decoder to delegate to
+     */
+    public Builder narrow(DecoderPredicate predicate, Decoder decoder) {
+      return add(PredicatedDecoder.narrowing(predicate, decoder));
+    }
+
+    /**
+     * Builds the multi-decoder.
+     *
+     * @throws IllegalStateException if no decoder was added
+     */
     public MultiDecoder build() {
+      if (decoders.isEmpty()) {
+        throw new IllegalStateException("at least one decoder is required");
+      }
       return new MultiDecoder(decoders);
     }
   }

core/src/main/java/feign/codec/MultiEncoder.java

@@ -50,11 +50,20 @@
  * naming what was tried. Add an encoder guarded by {@link EncoderPredicate#any()} last to act as a
  * default, as above.
  *
+ * <p>A multi-encoder is itself a {@link PredicatedEncoder}, accepting whatever any of its encoders
+ * accepts, so one can be added to another. That is how a library ships a set of encoders as a
+ * single unit: given a hypothetical {@code AcmeFeign.encoders()} returning a multi-encoder over
+ * that library's encoders, the whole set is added in one go:
+ *
+ * <pre>
+ * Feign.builder().encoders(AcmeFeign.encoders(), new JacksonEncoder());
+ * </pre>
+ *
  * @see PredicatedEncoder
  * @see EncoderPredicate
  */
 @Experimental
-public class MultiEncoder implements Encoder {
+public class MultiEncoder implements PredicatedEncoder {
 
   private final List<PredicatedEncoder> encoders;
 
@@ -67,6 +76,19 @@ public static Builder builder() {
     return new Builder();
   }
 
+  /**
+   * Whether any of the encoders accepts the request.
+   *
+   * @param object {@inheritDoc}
+   * @param bodyType {@inheritDoc}
+   * @param template {@inheritDoc}
+   * @return {@inheritDoc}
+   */
+  @Override
+  public boolean canEncode(Object object, Type bodyType, RequestTemplate template) {
+    return encoders.stream().anyMatch(encoder -> encoder.canEncode(object, bodyType, template));
+  }
+
   /**
    * Encodes using the first encoder that accepts the request.
    *
@@ -91,33 +113,61 @@ private String unableToEncode(Type bodyType, RequestTemplate template) {
     StringBuilder message =
         new StringBuilder("Unable to encode ")
             .append(bodyType == null ? "request body" : bodyType.getTypeName())
-            .append(" (Content-Type: ")
-            .append(contentTypes(template))
+            .append(" (")
+            .append(headers(template))
             .append(')');
     if (template.method() != null) {
       message.append(" for ").append(template.method()).append(' ').append(template.path());
     }
-    if (encoders.isEmpty()) {
-      return message.append(". No encoders were configured.").toString();
-    }
     message.append(". Encoders tried, in order:");
-    for (PredicatedEncoder encoder : encoders) {
-      message.append("\n  - ").append(PairedEncoder.describe(encoder));
-    }
+    appendTo(message, "\n  ");
     return message
-        .append("\nAdd an encoder guarded by EncoderPredicate.any() last to act as a default.")
+        .append("\nRegister an encoder that accepts it, or add a catch-all")
+        .append(" (EncoderPredicate.any()) last.")
         .toString();
   }
 
-  private static String contentTypes(RequestTemplate template) {
-    String contentTypes =
+  /**
+   * Lists the encoders one per line, unfolding nested multi-encoders so that a set contributed as a
+   * single unit still shows what it contains.
+   */
+  private void appendTo(StringBuilder message, String indent) {
+    for (PredicatedEncoder encoder : encoders) {
+      if (encoder instanceof MultiEncoder) {
+        message.append(indent).append("- MultiEncoder:");
+        ((MultiEncoder) encoder).appendTo(message, indent + "  ");
+      } else {
+        message.append(indent).append("- ").append(PairedEncoder.describe(encoder));
+      }
+    }
+  }
+
+  /**
+   * The headers an encoder is most likely to have been chosen on. Everything else a predicate looks
+   * at belongs in that predicate's own description, which is listed alongside it.
+   */
+  private static String headers(RequestTemplate template) {
+    String contentType = header(template, Util.CONTENT_TYPE);
+    StringBuilder headers =
+        new StringBuilder(Util.CONTENT_TYPE)
+            .append(": ")
+            .append(contentType == null ? "not set" : contentType);
+    String accept = header(template, Util.ACCEPT);
+    if (accept != null) {
+      headers.append(", ").append(Util.ACCEPT).append(": ").append(accept);
+    }
+    return headers.toString();
+  }
+
+  private static String header(RequestTemplate template, String name) {
+    String values =
         template.headers().entrySet().stream()
-            .filter(header -> Util.CONTENT_TYPE.equalsIgnoreCase(header.getKey()))
+            .filter(header -> name.equalsIgnoreCase(header.getKey()))
             .map(Map.Entry::getValue)
             .filter(Objects::nonNull)
             .flatMap(Collection::stream)
             .collect(Collectors.joining(", "));
-    return contentTypes.isEmpty() ? "not set" : contentTypes;
+    return values.isEmpty() ? null : values;
   }
 
   @Override
@@ -146,7 +196,10 @@ public Builder add(PredicatedEncoder encoder) {
 
     /**
      * Adds any encoder, guarded by the given predicate. Use this for encoders that do not implement
-     * {@link PredicatedEncoder}, including ones you do not control.
+     * {@link PredicatedEncoder}, including ones you do not control. The predicate is the whole
+     * answer: whatever the encoder may declare about itself is replaced, so this can widen an
+     * encoder as well as narrow it. Use {@link #narrow(EncoderPredicate, Encoder)} to keep the
+     * encoder's own declaration.
      *
      * @param predicate decides whether the encoder handles a request
      * @param encoder the encoder to delegate to
@@ -155,8 +208,34 @@ public Builder add(EncoderPredicate predicate, Encoder encoder) {
       return add(PredicatedEncoder.of(predicate, encoder));
     }
 
-    /** Builds the multi-encoder. */
+    /**
+     * Adds an encoder, narrowed by the given predicate. If the encoder is itself a {@link
+     * PredicatedEncoder}, the predicate applies in addition to the encoder's own {@code canEncode}
+     * rather than instead of it: both have to accept the request.
+     *
+     * <pre>
+     * MultiEncoder.builder()
+     *     .narrow(EncoderPredicate.contentType("application/vnd.acme+json"), new GsonEncoder())
+     *     .add(EncoderPredicate.any(), new DefaultEncoder())
+     *     .build();
+     * </pre>
+     *
+     * @param predicate narrows what the encoder handles
+     * @param encoder the encoder to delegate to
+     */
+    public Builder narrow(EncoderPredicate predicate, Encoder encoder) {
+      return add(PredicatedEncoder.narrowing(predicate, encoder));
+    }
+
+    /**
+     * Builds the multi-encoder.
+     *
+     * @throws IllegalStateException if no encoder was added
+     */
     public MultiEncoder build() {
+      if (encoders.isEmpty()) {
+        throw new IllegalStateException("at least one encoder is required");
+      }
       return new MultiEncoder(encoders);
     }
   }