Maintain user-given order for header values (OpenFeign#1009)

mbarkley · kdavisk6 · commit aa565e41568f · 2019-07-22T13:47:38.000-04:00
Fixes bug where HeaderTemplate stored values in a HashSet which caused the following issues: * Header values could be written in wrong order * Order was not stable between JVM instances Fixes OpenFeign#1007
diff --git a/core/src/main/java/feign/template/HeaderTemplate.java b/core/src/main/java/feign/template/HeaderTemplate.java
@@ -19,7 +19,6 @@
 import java.util.Collections;
 import java.util.Iterator;
 import java.util.LinkedHashSet;
-import java.util.Set;
 import java.util.stream.Collectors;
 import java.util.stream.StreamSupport;
 
@@ -30,7 +29,7 @@
 public final class HeaderTemplate extends Template {
 
   /* cache a copy of the variables for lookup later */
-  private Set<String> values;
+  private LinkedHashSet<String> values;
   private String name;
 
   public static HeaderTemplate create(String name, Iterable<String> values) {
@@ -66,10 +65,10 @@ public static HeaderTemplate create(String name, Iterable<String> values) {
    * @return a new Header Template with the values added.
    */
   public static HeaderTemplate append(HeaderTemplate headerTemplate, Iterable<String> values) {
-    Set<String> headerValues = new LinkedHashSet<>(headerTemplate.getValues());
+    LinkedHashSet<String> headerValues = new LinkedHashSet<>(headerTemplate.getValues());
     headerValues.addAll(StreamSupport.stream(values.spliterator(), false)
         .filter(Util::isNotBlank)
-        .collect(Collectors.toSet()));
+        .collect(Collectors.toCollection(LinkedHashSet::new)));
     return create(headerTemplate.getName(), headerValues);
   }
 
@@ -82,7 +81,7 @@ private HeaderTemplate(String template, String name, Iterable<String> values, Ch
     super(template, ExpansionOptions.REQUIRED, EncodingOptions.NOT_REQUIRED, false, charset);
     this.values = StreamSupport.stream(values.spliterator(), false)
         .filter(Util::isNotBlank)
-        .collect(Collectors.toSet());
+        .collect(Collectors.toCollection(LinkedHashSet::new));
     this.name = name;
   }
 
diff --git a/core/src/test/java/feign/template/HeaderTemplateTest.java b/core/src/test/java/feign/template/HeaderTemplateTest.java
@@ -13,12 +13,17 @@
  */
 package feign.template;
 
+import static org.hamcrest.CoreMatchers.equalTo;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertThat;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
-import java.util.Arrays;
-import java.util.Collections;
-import static org.junit.Assert.assertEquals;
 
 public class HeaderTemplateTest {
 
@@ -58,4 +63,40 @@ public void it_should_return_expanded() {
         headerTemplate.expand(Collections.singletonMap("name", "firsts")));
   }
 
+  @Test
+  public void create_should_preserve_order() {
+    /*
+     * Since Java 7, HashSet order is stable within a since JVM process, so one of these assertions
+     * should fail if a HashSet is used.
+     */
+    HeaderTemplate headerTemplateWithFirstOrdering =
+        HeaderTemplate.create("hello", Arrays.asList("test 1", "test 2"));
+    assertThat(new ArrayList<>(headerTemplateWithFirstOrdering.getValues()),
+        equalTo(Arrays.asList("test 1", "test 2")));
+
+    HeaderTemplate headerTemplateWithSecondOrdering =
+        HeaderTemplate.create("hello", Arrays.asList("test 2", "test 1"));
+    assertThat(new ArrayList<>(headerTemplateWithSecondOrdering.getValues()),
+        equalTo(Arrays.asList("test 2", "test 1")));
+  }
+
+  @Test
+  public void append_should_preserve_order() {
+    /*
+     * Since Java 7, HashSet order is stable within a since JVM process, so one of these assertions
+     * should fail if a HashSet is used.
+     */
+    HeaderTemplate headerTemplateWithFirstOrdering =
+        HeaderTemplate.append(HeaderTemplate.create("hello", Collections.emptyList()),
+            Arrays.asList("test 1", "test 2"));
+    assertThat(new ArrayList<>(headerTemplateWithFirstOrdering.getValues()),
+        equalTo(Arrays.asList("test 1", "test 2")));
+
+    HeaderTemplate headerTemplateWithSecondOrdering =
+        HeaderTemplate.append(HeaderTemplate.create("hello", Collections.emptyList()),
+            Arrays.asList("test 2", "test 1"));
+    assertThat(new ArrayList<>(headerTemplateWithSecondOrdering.getValues()),
+        equalTo(Arrays.asList("test 2", "test 1")));
+  }
+
 }
diff --git a/jaxrs/src/test/java/feign/jaxrs/JAXRSContractTest.java b/jaxrs/src/test/java/feign/jaxrs/JAXRSContractTest.java
@@ -401,7 +401,7 @@ public void producesWithHeaderParamContainAllHeaders() throws Exception {
     assertThat(parseAndValidateMetadata(MixedAnnotations.class, "getWithHeaders",
         String.class, String.class, String.class)
             .template())
-                .hasHeaders(entry("Accept", Arrays.asList("{Accept}", "application/json")))
+                .hasHeaders(entry("Accept", Arrays.asList("application/json", "{Accept}")))
                 .hasQueries(
                     entry("multiple", Arrays.asList("stuff", "{multiple}")),
                     entry("another", Collections.singletonList("{another}")));
