Repository: zalando/logbook (java)

Issue:
HeadersBuilder builds a mutable data structure
Even though the map is immutable, its values aren't
https://github.com/zalando/logbook/blob/d95a0ca62bd75d6c9055b734c1ebba90761870ba/logbook-api/src/main/java/org/zalando/logbook/BaseHttpMessage.java#L73

Pull request:
Make headers immutable
Resolve #97 

Patch:
diff --git a/logbook-api/src/main/java/org/zalando/logbook/BaseHttpMessage.java b/logbook-api/src/main/java/org/zalando/logbook/BaseHttpMessage.java
index 1cbc6f86..855bf8fc 100644
--- a/logbook-api/src/main/java/org/zalando/logbook/BaseHttpMessage.java
+++ b/logbook-api/src/main/java/org/zalando/logbook/BaseHttpMessage.java
@@ -43,7 +43,7 @@ public interface BaseHttpMessage {
 
     class HeadersBuilder {
 
-        private final Map<String, List<String>> headers;
+        private Map<String, List<String>> headers;
 
         public HeadersBuilder() {
             // package private so we can trick code coverage
@@ -70,7 +70,11 @@ public interface BaseHttpMessage {
         }
 
         public Map<String, List<String>> build() {
-            return Collections.unmodifiableMap(headers);
+            for (Map.Entry<String, List<String>> e : headers.entrySet()) {
+                e.setValue(Collections.unmodifiableList(e.getValue()));
+            }
+            headers = Collections.unmodifiableMap(headers);
+            return headers;
         }
     }
 }


Test patch:
diff --git a/logbook-api/src/test/java/org/zalando/logbook/BaseHttpMessageTest.java b/logbook-api/src/test/java/org/zalando/logbook/BaseHttpMessageTest.java
index b67d0f80..8b3db1ce 100644
--- a/logbook-api/src/test/java/org/zalando/logbook/BaseHttpMessageTest.java
+++ b/logbook-api/src/test/java/org/zalando/logbook/BaseHttpMessageTest.java
@@ -27,7 +27,10 @@ import java.util.List;
 import java.util.Map;
 
 import static org.hamcrest.Matchers.hasItem;
+import static org.hamcrest.Matchers.hasItems;
+import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertThat;
+import static org.junit.Assert.fail;
 
 public final class BaseHttpMessageTest {
 
@@ -44,4 +47,49 @@ public final class BaseHttpMessageTest {
         assertThat(headers.get("Y-SECRET"), hasItem("one"));
         assertThat(headers.get("Y-SECRET"), hasItem("two"));
     }
+
+    @Test
+    public void shouldBuildImmutableHeaders() {
+        final Map<String, List<String>> headers = new BaseHttpMessage.HeadersBuilder()
+            .put("a", "b")
+            .put("a", "c")
+            .put("d", Arrays.asList("e", "f"))
+            .build();
+
+        try {
+            headers.put("x", Arrays.asList("y", "z"));
+            fail("Headers supposed to be immutable");
+        } catch (UnsupportedOperationException ex) {
+        }
+
+        final List<String> a = headers.get("a");
+        assertNotNull(a);
+        assertThat(a, hasItems("b", "c"));
+
+        try {
+            a.add("x");
+            fail("Headers supposed to be immutable");
+        } catch (UnsupportedOperationException ex) {
+        }
+    }
+
+    @Test
+    public void shouldRefuseUpdateHeadersAfterBuild() {
+        final BaseHttpMessage.HeadersBuilder builder = new BaseHttpMessage.HeadersBuilder();
+        builder.put("a", "b").build();
+
+        try {
+            // existing key
+            builder.put("a", "b");
+            fail("Builder can not be reused");
+        } catch (UnsupportedOperationException ex) {
+        }
+
+        try {
+            // new key
+            builder.put("x", "y");
+            fail("Builder can not be reused");
+        } catch (UnsupportedOperationException ex) {
+        }
+    }
 }