Remove null empty headers (#724)

* Creating headers from Request removing those with null or empty values

* Moving back to let the empty strings as valid header

* Returning headers filtering null and empty rather removing them for the current map. Supporting with tests as it needs to be LinkedHashMap not to lost the sorting

Author: fmcejudo

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

@@ -34,6 +34,7 @@
 import static feign.Util.emptyToNull;
 import static feign.Util.toArray;
 import static feign.Util.valuesOrEmpty;
+import static java.util.stream.Collectors.toMap;
 
 /**
  * Builds a request to an http target. Not thread safe. <br>
@@ -266,7 +267,7 @@ private String encodeValueIfNotEncoded(String key,
   /* roughly analogous to {@code javax.ws.rs.client.Target.request()}. */
   public Request request() {
     Map<String, Collection<String>> safeCopy = new LinkedHashMap<String, Collection<String>>();
-    safeCopy.putAll(headers);
+    safeCopy.putAll(headers());
     return Request.create(
         method, url + queryLine(),
         Collections.unmodifiableMap(safeCopy),
@@ -535,7 +536,16 @@ public RequestTemplate headers(Map<String, Collection<String>> headers) {
    * @see Request#headers()
    */
   public Map<String, Collection<String>> headers() {
-    return Collections.unmodifiableMap(headers);
+
+    return Collections.unmodifiableMap(
+        headers.entrySet().stream().filter(h -> h.getValue() != null && !h.getValue().isEmpty())
+            .collect(toMap(
+                Entry::getKey,
+                Entry::getValue,
+                (e1, e2) -> {
+                  throw new IllegalStateException("headers should not have duplicated keys");
+                },
+                LinkedHashMap::new)));
   }
 
   /**

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

@@ -13,10 +13,13 @@
  */
 package feign;
 
+import org.assertj.core.api.Assertions;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
 import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
 import java.util.LinkedHashMap;
 import java.util.Map;
 import static feign.RequestTemplate.expand;
@@ -344,4 +347,31 @@ public void encodedQueryWithUnsafeCharactersMixedWithUnencoded() throws Exceptio
     assertThat(template.queries()).doesNotContain(entry("params[]", asList("not encoded")));
     assertThat(template.queries()).contains(entry("params[]", asList("encoded")));
   }
+
+  @Test
+  public void shouldRetrieveHeadersWithoutNull() {
+    RequestTemplate template = new RequestTemplate()
+        .header("key1", (String) null)
+        .header("key2", Collections.emptyList())
+        .header("key3", (Collection) null)
+        .header("key4", "valid")
+        .header("key5", "valid")
+        .header("key6", "valid")
+        .header("key7", "valid");
+
+    assertThat(template.headers()).hasSize(4);
+    assertThat(template.headers().keySet()).containsExactly("key4", "key5", "key6", "key7");
+
+  }
+
+  @Test(expected = UnsupportedOperationException.class)
+  public void shouldNotInsertHeadersImmutableMap() {
+    RequestTemplate template = new RequestTemplate()
+        .header("key1", "valid");
+
+    assertThat(template.headers()).hasSize(1);
+    assertThat(template.headers().keySet()).containsExactly("key1");
+
+    template.headers().put("key2", asList("other value"));
+  }
 }

core/src/test/java/feign/assertj/RequestTemplateAssert.java

@@ -82,4 +82,9 @@ public RequestTemplateAssert hasHeaders(MapEntry... entries) {
     maps.assertContainsExactly(info, actual.headers(), entries);
     return this;
   }
+
+  public RequestTemplateAssert hasNoHeader(final String encoded) {
+    objects.assertNull(info, actual.headers().get(encoded));
+    return this;
+  }
 }

core/src/test/java/feign/client/AbstractClientTest.java

@@ -35,6 +35,7 @@
 import okhttp3.mockwebserver.MockWebServer;
 import static java.util.Arrays.asList;
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.entry;
 import static org.junit.Assert.assertEquals;
 import static feign.Util.UTF_8;
 
@@ -93,7 +94,7 @@ public void parsesRequestAndResponse() throws IOException, InterruptedException
 
     MockWebServerAssertions.assertThat(server.takeRequest()).hasMethod("POST")
         .hasPath("/?foo=bar&foo=baz&qux=")
-        .hasHeaders("Foo: Bar", "Foo: Baz", "Qux: ", "Accept: */*", "Content-Length: 3")
+        .hasHeaders("Foo: Bar", "Foo: Baz", "Accept: */*", "Content-Length: 3")
         .hasBody("foo");
   }
 
@@ -282,6 +283,38 @@ public void testDefaultCollectionFormat() throws Exception {
         .hasPath("/?foo=bar&foo=baz");
   }
 
+  @Test
+  public void testHeadersWithNullParams() throws InterruptedException {
+    server.enqueue(new MockResponse().setBody("body"));
+
+    TestInterface api = newBuilder()
+        .target(TestInterface.class, "http://localhost:" + server.getPort());
+
+    Response response = api.getWithHeaders(null);
+
+    assertThat(response.status()).isEqualTo(200);
+    assertThat(response.reason()).isEqualTo("OK");
+
+    MockWebServerAssertions.assertThat(server.takeRequest()).hasMethod("GET")
+        .hasPath("/").hasNoHeaderNamed("Authorization");
+  }
+
+  @Test
+  public void testHeadersWithNotEmptyParams() throws InterruptedException {
+    server.enqueue(new MockResponse().setBody("body"));
+
+    TestInterface api = newBuilder()
+        .target(TestInterface.class, "http://localhost:" + server.getPort());
+
+    Response response = api.getWithHeaders("token");
+
+    assertThat(response.status()).isEqualTo(200);
+    assertThat(response.reason()).isEqualTo("OK");
+
+    MockWebServerAssertions.assertThat(server.takeRequest()).hasMethod("GET")
+        .hasPath("/").hasHeaders(entry("authorization", asList("token")));
+  }
+
   @Test
   public void testAlternativeCollectionFormat() throws Exception {
     server.enqueue(new MockResponse().setBody("body"));
@@ -316,6 +349,12 @@ public interface TestInterface {
     @RequestLine("GET /?foo={multiFoo}")
     Response get(@Param("multiFoo") List<String> multiFoo);
 
+    @Headers({
+        "Authorization: {authorization}"
+    })
+    @RequestLine("GET /")
+    Response getWithHeaders(@Param("authorization") String authorization);
+
     @RequestLine(value = "GET /?foo={multiFoo}", collectionFormat = CollectionFormat.CSV)
     Response getCSV(@Param("multiFoo") List<String> multiFoo);