Skip to content

Commit 4bc2e91

Browse files
fmcejudovelo
fmcejudo
authored and
velo
committed
Remove null empty headers (OpenFeign#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
1 parent 1a9ae47 commit 4bc2e91

4 files changed

Lines changed: 89 additions & 3 deletions

File tree

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

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import static feign.Util.emptyToNull;
2121
import static feign.Util.toArray;
2222
import static feign.Util.valuesOrEmpty;
23+
import static java.util.stream.Collectors.toMap;
2324

2425
import java.io.Serializable;
2526
import java.io.UnsupportedEncodingException;
@@ -266,7 +267,7 @@ private String encodeValueIfNotEncoded(
266267
/* roughly analogous to {@code javax.ws.rs.client.Target.request()}. */
267268
public Request request() {
268269
Map<String, Collection<String>> safeCopy = new LinkedHashMap<String, Collection<String>>();
269-
safeCopy.putAll(headers);
270+
safeCopy.putAll(headers());
270271
return Request.create(
271272
method, url + queryLine(), Collections.unmodifiableMap(safeCopy), body, charset);
272273
}
@@ -533,7 +534,18 @@ public RequestTemplate headers(Map<String, Collection<String>> headers) {
533534
* @see Request#headers()
534535
*/
535536
public Map<String, Collection<String>> headers() {
536-
return Collections.unmodifiableMap(headers);
537+
538+
return Collections.unmodifiableMap(
539+
headers.entrySet().stream()
540+
.filter(h -> h.getValue() != null && !h.getValue().isEmpty())
541+
.collect(
542+
toMap(
543+
Entry::getKey,
544+
Entry::getValue,
545+
(e1, e2) -> {
546+
throw new IllegalStateException("headers should not have duplicated keys");
547+
},
548+
LinkedHashMap::new)));
537549
}
538550

539551
/**

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

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@
1919
import static org.assertj.core.data.MapEntry.entry;
2020

2121
import java.util.Arrays;
22+
import java.util.Collection;
23+
import java.util.Collections;
2224
import java.util.LinkedHashMap;
2325
import java.util.Map;
2426
import org.junit.Rule;
@@ -340,4 +342,30 @@ public void encodedQueryWithUnsafeCharactersMixedWithUnencoded() throws Exceptio
340342
assertThat(template.queries()).doesNotContain(entry("params[]", asList("not encoded")));
341343
assertThat(template.queries()).contains(entry("params[]", asList("encoded")));
342344
}
345+
346+
@Test
347+
public void shouldRetrieveHeadersWithoutNull() {
348+
RequestTemplate template =
349+
new RequestTemplate()
350+
.header("key1", (String) null)
351+
.header("key2", Collections.emptyList())
352+
.header("key3", (Collection) null)
353+
.header("key4", "valid")
354+
.header("key5", "valid")
355+
.header("key6", "valid")
356+
.header("key7", "valid");
357+
358+
assertThat(template.headers()).hasSize(4);
359+
assertThat(template.headers().keySet()).containsExactly("key4", "key5", "key6", "key7");
360+
}
361+
362+
@Test(expected = UnsupportedOperationException.class)
363+
public void shouldNotInsertHeadersImmutableMap() {
364+
RequestTemplate template = new RequestTemplate().header("key1", "valid");
365+
366+
assertThat(template.headers()).hasSize(1);
367+
assertThat(template.headers().keySet()).containsExactly("key1");
368+
369+
template.headers().put("key2", asList("other value"));
370+
}
343371
}

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,4 +83,9 @@ public RequestTemplateAssert hasHeaders(MapEntry... entries) {
8383
maps.assertContainsExactly(info, actual.headers(), entries);
8484
return this;
8585
}
86+
87+
public RequestTemplateAssert hasNoHeader(final String encoded) {
88+
objects.assertNull(info, actual.headers().get(encoded));
89+
return this;
90+
}
8691
}

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

Lines changed: 42 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
import static feign.Util.UTF_8;
1717
import static java.util.Arrays.asList;
1818
import static org.assertj.core.api.Assertions.assertThat;
19+
import static org.assertj.core.api.Assertions.entry;
1920
import static org.junit.Assert.assertEquals;
2021

2122
import feign.Client;
@@ -91,7 +92,7 @@ public void parsesRequestAndResponse() throws IOException, InterruptedException
9192
MockWebServerAssertions.assertThat(server.takeRequest())
9293
.hasMethod("POST")
9394
.hasPath("/?foo=bar&foo=baz&qux=")
94-
.hasHeaders("Foo: Bar", "Foo: Baz", "Qux: ", "Accept: */*", "Content-Length: 3")
95+
.hasHeaders("Foo: Bar", "Foo: Baz", "Accept: */*", "Content-Length: 3")
9596
.hasBody("foo");
9697
}
9798

@@ -283,6 +284,42 @@ public void testDefaultCollectionFormat() throws Exception {
283284
.hasPath("/?foo=bar&foo=baz");
284285
}
285286

287+
@Test
288+
public void testHeadersWithNullParams() throws InterruptedException {
289+
server.enqueue(new MockResponse().setBody("body"));
290+
291+
TestInterface api =
292+
newBuilder().target(TestInterface.class, "http://localhost:" + server.getPort());
293+
294+
Response response = api.getWithHeaders(null);
295+
296+
assertThat(response.status()).isEqualTo(200);
297+
assertThat(response.reason()).isEqualTo("OK");
298+
299+
MockWebServerAssertions.assertThat(server.takeRequest())
300+
.hasMethod("GET")
301+
.hasPath("/")
302+
.hasNoHeaderNamed("Authorization");
303+
}
304+
305+
@Test
306+
public void testHeadersWithNotEmptyParams() throws InterruptedException {
307+
server.enqueue(new MockResponse().setBody("body"));
308+
309+
TestInterface api =
310+
newBuilder().target(TestInterface.class, "http://localhost:" + server.getPort());
311+
312+
Response response = api.getWithHeaders("token");
313+
314+
assertThat(response.status()).isEqualTo(200);
315+
assertThat(response.reason()).isEqualTo("OK");
316+
317+
MockWebServerAssertions.assertThat(server.takeRequest())
318+
.hasMethod("GET")
319+
.hasPath("/")
320+
.hasHeaders(entry("authorization", asList("token")));
321+
}
322+
286323
@Test
287324
public void testAlternativeCollectionFormat() throws Exception {
288325
server.enqueue(new MockResponse().setBody("body"));
@@ -318,6 +355,10 @@ public interface TestInterface {
318355
@RequestLine("GET /?foo={multiFoo}")
319356
Response get(@Param("multiFoo") List<String> multiFoo);
320357

358+
@Headers({"Authorization: {authorization}"})
359+
@RequestLine("GET /")
360+
Response getWithHeaders(@Param("authorization") String authorization);
361+
321362
@RequestLine(value = "GET /?foo={multiFoo}", collectionFormat = CollectionFormat.CSV)
322363
Response getCSV(@Param("multiFoo") List<String> multiFoo);
323364

0 commit comments

Comments
 (0)