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

@@ -20,6 +20,7 @@
 import static feign.Util.emptyToNull;
 import static feign.Util.toArray;
 import static feign.Util.valuesOrEmpty;
+import static java.util.stream.Collectors.toMap;
 
 import java.io.Serializable;
 import java.io.UnsupportedEncodingException;
@@ -266,7 +267,7 @@ private String encodeValueIfNotEncoded(
   /* 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), body, charset);
   }
@@ -533,7 +534,18 @@ 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

@@ -19,6 +19,8 @@
 import static org.assertj.core.data.MapEntry.entry;
 
 import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
 import java.util.LinkedHashMap;
 import java.util.Map;
 import org.junit.Rule;
@@ -340,4 +342,30 @@ 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

@@ -83,4 +83,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

@@ -16,6 +16,7 @@
 import static feign.Util.UTF_8;
 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 feign.Client;
@@ -91,7 +92,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");
   }
 
@@ -283,6 +284,42 @@ 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"));
@@ -318,6 +355,10 @@ 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);