NPE when resolving a template with binary body (#821)

* NPE when resolving a template with binary body

* Initial change to introduce body object to Request's

core/src/main/java/feign/Request.java

@@ -17,14 +17,73 @@
 import static feign.Util.valuesOrEmpty;
 import java.net.HttpURLConnection;
 import java.nio.charset.Charset;
-import java.util.Collection;
-import java.util.Map;
+import java.util.*;
+import feign.template.BodyTemplate;
 
 /**
  * An immutable request to an http server.
  */
 public final class Request {
 
+  public static class Body {
+
+    private final byte[] data;
+    private final Charset encoding;
+    private final BodyTemplate bodyTemplate;
+
+    private Body(byte[] data, Charset encoding, BodyTemplate bodyTemplate) {
+      super();
+      this.data = data;
+      this.encoding = encoding;
+      this.bodyTemplate = bodyTemplate;
+    }
+
+    public Request.Body expand(Map<String, ?> variables) {
+      if (bodyTemplate == null)
+        return this;
+
+      return encoded(bodyTemplate.expand(variables).getBytes(encoding), encoding);
+    }
+
+    public List<String> getVariables() {
+      if (bodyTemplate == null)
+        return Collections.emptyList();
+      return bodyTemplate.getVariables();
+    }
+
+    public static Request.Body encoded(byte[] bodyData, Charset encoding) {
+      return new Request.Body(bodyData, encoding, null);
+    }
+
+    public int length() {
+      /* calculate the content length based on the data provided */
+      return data != null ? data.length : 0;
+    }
+
+    public byte[] asBytes() {
+      return data;
+    }
+
+    public static Request.Body bodyTemplate(String bodyTemplate, Charset encoding) {
+      return new Request.Body(null, encoding, BodyTemplate.create(bodyTemplate));
+    }
+
+    public String bodyTemplate() {
+      return (bodyTemplate != null) ? bodyTemplate.toString() : null;
+    }
+
+    public String asString() {
+      return encoding != null && data != null
+          ? new String(data, encoding)
+          : "Binary data";
+    }
+
+    public static Body empty() {
+      return new Request.Body(null, null, null);
+    }
+
+  }
+
   public enum HttpMethod {
     GET, HEAD, POST, PUT, DELETE, CONNECT, OPTIONS, TRACE, PATCH
   }
@@ -60,23 +119,35 @@ public static Request create(HttpMethod httpMethod,
                                Map<String, Collection<String>> headers,
                                byte[] body,
                                Charset charset) {
-    return new Request(httpMethod, url, headers, body, charset);
+    return create(httpMethod, url, headers, Body.encoded(body, charset));
+  }
 
+  /**
+   * Builds a Request. All parameters must be effectively immutable, via safe copies.
+   *
+   * @param httpMethod for the request.
+   * @param url for the request.
+   * @param headers to include.
+   * @param body of the request, can be {@literal null}
+   * @return a Request
+   */
+  public static Request create(HttpMethod httpMethod,
+                               String url,
+                               Map<String, Collection<String>> headers,
+                               Body body) {
+    return new Request(httpMethod, url, headers, body);
   }
 
   private final HttpMethod httpMethod;
   private final String url;
   private final Map<String, Collection<String>> headers;
-  private final byte[] body;
-  private final Charset charset;
+  private final Body body;
 
-  Request(HttpMethod method, String url, Map<String, Collection<String>> headers, byte[] body,
-      Charset charset) {
+  Request(HttpMethod method, String url, Map<String, Collection<String>> headers, Body body) {
     this.httpMethod = checkNotNull(method, "httpMethod of %s", method.name());
     this.url = checkNotNull(url, "url");
     this.headers = checkNotNull(headers, "headers of %s %s", method, url);
-    this.body = body; // nullable
-    this.charset = charset; // nullable
+    this.body = body;
   }
 
   /**
@@ -112,18 +183,25 @@ public Map<String, Collection<String>> headers() {
    * The character set with which the body is encoded, or null if unknown or not applicable. When
    * this is present, you can use {@code new String(req.body(), req.charset())} to access the body
    * as a String.
+   *
+   * @deprecated use {@link #requestBody()} instead
    */
   public Charset charset() {
-    return charset;
+    return body.encoding;
   }
 
   /**
    * If present, this is the replayable body to send to the server. In some cases, this may be
    * interpretable as text.
    *
    * @see #charset()
+   * @deprecated use {@link #requestBody()} instead
    */
   public byte[] body() {
+    return body.data;
+  }
+
+  public Body requestBody() {
     return body;
   }
 
@@ -137,7 +215,7 @@ public String toString() {
       }
     }
     if (body != null) {
-      builder.append('\n').append(charset != null ? new String(body, charset) : "Binary data");
+      builder.append('\n').append(body.asString());
     }
     return builder.toString();
   }

core/src/main/java/feign/RequestTemplate.java

@@ -55,10 +55,9 @@ public final class RequestTemplate implements Serializable {
   private String target;
   private boolean resolved = false;
   private UriTemplate uriTemplate;
-  private BodyTemplate bodyTemplate;
   private HttpMethod method;
   private transient Charset charset = Util.UTF_8;
-  private byte[] body;
+  private Request.Body body = Request.Body.empty();
   private boolean decodeSlash = true;
   private CollectionFormat collectionFormat = CollectionFormat.EXPLODED;
 
@@ -83,15 +82,13 @@ public RequestTemplate() {
    */
   private RequestTemplate(String target,
       UriTemplate uriTemplate,
-      BodyTemplate bodyTemplate,
       HttpMethod method,
       Charset charset,
-      byte[] body,
+      Request.Body body,
       boolean decodeSlash,
       CollectionFormat collectionFormat) {
     this.target = target;
     this.uriTemplate = uriTemplate;
-    this.bodyTemplate = bodyTemplate;
     this.method = method;
     this.charset = charset;
     this.body = body;
@@ -109,7 +106,7 @@ private RequestTemplate(String target,
   public static RequestTemplate from(RequestTemplate requestTemplate) {
     RequestTemplate template =
         new RequestTemplate(requestTemplate.target, requestTemplate.uriTemplate,
-            requestTemplate.bodyTemplate, requestTemplate.method, requestTemplate.charset,
+            requestTemplate.method, requestTemplate.charset,
             requestTemplate.body, requestTemplate.decodeSlash, requestTemplate.collectionFormat);
 
     if (!requestTemplate.queries().isEmpty()) {
@@ -137,7 +134,6 @@ public RequestTemplate(RequestTemplate toCopy) {
     this.headers.putAll(toCopy.headers);
     this.charset = toCopy.charset;
     this.body = toCopy.body;
-    this.bodyTemplate = toCopy.bodyTemplate;
     this.decodeSlash = toCopy.decodeSlash;
     this.collectionFormat =
         (toCopy.collectionFormat != null) ? toCopy.collectionFormat : CollectionFormat.EXPLODED;
@@ -225,9 +221,7 @@ public RequestTemplate resolve(Map<String, ?> variables) {
       }
     }
 
-    if (this.bodyTemplate != null) {
-      resolved.body(this.bodyTemplate.expand(variables));
-    }
+    resolved.body(this.body.expand(variables));
 
     /* mark the new template resolved */
     resolved.resolved = true;
@@ -261,7 +255,7 @@ public Request request() {
     if (!this.resolved) {
       throw new IllegalStateException("template has not been resolved.");
     }
-    return Request.create(this.method, this.url(), this.headers(), this.body(), this.charset);
+    return Request.create(this.method, this.url(), this.headers(), this.requestBody());
   }
 
   /**
@@ -541,9 +535,7 @@ public List<String> variables() {
     }
 
     /* body */
-    if (this.bodyTemplate != null) {
-      variables.addAll(this.bodyTemplate.getVariables());
-    }
+    variables.addAll(this.body.getVariables());
 
     return variables;
   }
@@ -717,20 +709,11 @@ public Map<String, Collection<String>> headers() {
    * @param bodyData to send, can be null.
    * @param charset of the encoded data.
    * @return a RequestTemplate for chaining.
+   * @deprecated use {@link RequestTemplate#body(feign.Request.Body)} instead
    */
+  @Deprecated
   public RequestTemplate body(byte[] bodyData, Charset charset) {
-
-    /*
-     * since the body is being set directly, we need to clear out any existing body template
-     * information to prevent unintended side effects.
-     */
-    this.bodyTemplate = null;
-    this.charset = charset;
-    this.body = bodyData;
-
-    /* calculate the content length based on the data provided */
-    int bodyLength = bodyData != null ? bodyData.length : 0;
-    header(CONTENT_LENGTH, String.valueOf(bodyLength));
+    this.body(Request.Body.encoded(bodyData, charset));
 
     return this;
   }
@@ -740,28 +723,49 @@ public RequestTemplate body(byte[] bodyData, Charset charset) {
    *
    * @param bodyText to send.
    * @return a RequestTemplate for chaining.
+   * @deprecated use {@link RequestTemplate#body(feign.Request.Body)} instead
    */
+  @Deprecated
   public RequestTemplate body(String bodyText) {
     byte[] bodyData = bodyText != null ? bodyText.getBytes(UTF_8) : null;
     return body(bodyData, UTF_8);
   }
 
+  /**
+   * Set the Body for this request.
+   *
+   * @param body to send.
+   * @return a RequestTemplate for chaining.
+   */
+  public RequestTemplate body(Request.Body body) {
+    this.body = body;
+
+    header(CONTENT_LENGTH);
+    if (body.length() > 0) {
+      header(CONTENT_LENGTH, String.valueOf(body.length()));
+    }
+
+    return this;
+  }
+
   /**
    * Charset of the Request Body, if known.
    *
    * @return the currently applied Charset.
    */
-  public Charset charset() {
+  public Charset requestCharset() {
     return charset;
   }
 
   /**
    * The Request Body.
    *
    * @return the request body.
+   * @deprecated replaced by {@link RequestTemplate#requestBody()}
    */
+  @Deprecated
   public byte[] body() {
-    return body;
+    return body.asBytes();
   }
 
 
@@ -770,11 +774,11 @@ public byte[] body() {
    *
    * @param bodyTemplate to use.
    * @return a RequestTemplate for chaining.
+   * @deprecated replaced by {@link RequestTemplate#body(feign.Request.Body)}
    */
+  @Deprecated
   public RequestTemplate bodyTemplate(String bodyTemplate) {
-    this.bodyTemplate = BodyTemplate.create(bodyTemplate);
-    this.charset = Util.UTF_8;
-    this.body = null;
+    this.body(Request.Body.bodyTemplate(bodyTemplate, Util.UTF_8));
     return this;
   }
 
@@ -784,7 +788,7 @@ public RequestTemplate bodyTemplate(String bodyTemplate) {
    * @return the unresolved body template.
    */
   public String bodyTemplate() {
-    return (bodyTemplate != null) ? bodyTemplate.toString() : null;
+    return body.bodyTemplate();
   }
 
   @Override
@@ -884,6 +888,10 @@ private SimpleImmutableEntry<String, String> splitQueryParameter(String pair) {
     return new SimpleImmutableEntry<>(name, value);
   }
 
+  public Request.Body requestBody() {
+    return this.body;
+  }
+
   /**
    * Factory for creating RequestTemplate.
    */
@@ -894,4 +902,5 @@ interface Factory {
      */
     RequestTemplate create(Object[] argv);
   }
+
 }

core/src/test/java/feign/RequestTemplateTest.java

@@ -96,6 +96,18 @@ public void resolveTemplateWithParameterizedPathSkipsEncodingSlash() {
         .hasUrl("/hostedzone/Z1PA6795UKMFR9");
   }
 
+  @Test
+  public void resolveTemplateWithBinaryBody() {
+    RequestTemplate template = new RequestTemplate().method(HttpMethod.GET)
+        .uri("{zoneId}")
+        .body(new byte[] {7, 3, -3, -7}, null);
+
+    template = template.resolve(mapOf("zoneId", "/hostedzone/Z1PA6795UKMFR9"));
+
+    assertThat(template)
+        .hasUrl("/hostedzone/Z1PA6795UKMFR9");
+  }
+
   @Test
   public void canInsertAbsoluteHref() {
     RequestTemplate template = new RequestTemplate().method(HttpMethod.GET)

jaxb/src/test/java/feign/jaxb/examples/AWSSignatureVersion4.java

@@ -75,9 +75,7 @@ private static String canonicalString(RequestTemplate input, String host) {
     canonicalRequest.append("host").append('\n');
 
     // HexEncode(Hash(Payload))
-    String bodyText =
-        input.charset() != null && input.body() != null ? new String(input.body(), input.charset())
-            : null;
+    String bodyText = input.requestBody().asString();
     if (bodyText != null) {
       canonicalRequest.append(hex(sha256(bodyText)));
     } else {

mock/src/main/java/feign/mock/MockClient.java

@@ -83,7 +83,7 @@ private Response.Builder executeSequential(RequestKey requestKey) {
 
     RequestResponse expectedRequestResponse = responseIterator.next();
     if (!expectedRequestResponse.requestKey.equalsExtended(requestKey)) {
-      throw new VerificationAssertionError("Expected %s, but was %s",
+      throw new VerificationAssertionError("Expected: \n%s,\nbut was: \n%s",
           expectedRequestResponse.requestKey,
           requestKey);
     }

mock/src/test/java/feign/mock/MockClientSequentialTest.java

@@ -167,7 +167,7 @@ public void sequentialRequestsInWrongOrder() throws Exception {
       githubSequential.contributors("7 7", "netflix", "feign");
       fail();
     } catch (VerificationAssertionError e) {
-      assertThat(e.getMessage(), startsWith("Expected Request ["));
+      assertThat(e.getMessage(), startsWith("Expected: \nRequest ["));
     }
   }