Fix ClassCastException when calling *HeadersBuilder.build()… (lin…

…e#2193) Motivation: The following code, which calls `*HeadersBuilder.build()` twice, fails with a `ClassCastException`. HttpHeadersBuilder b = HttpHeaders.builder(); b.add("foo", "bar"); b.build(); b.build(); // Throws a ClassCastException Modifications: - Make sure the `parent` is of a desired type. Rebuild if not. Result: - Fixes line#2190
fmguerreiro · Oct 17, 2019 · 9979677 · 9979677
1 parent e959c55
commit 9979677
Show file tree

Hide file tree

Showing 7 changed files with 56 additions and 15 deletions.
diff --git a/core/src/main/java/com/linecorp/armeria/common/AbstractHttpHeadersBuilder.java b/core/src/main/java/com/linecorp/armeria/common/AbstractHttpHeadersBuilder.java
@@ -69,6 +69,11 @@ final HttpHeadersBase parent() {
         return parent;
     }
 
+    final <V extends HttpHeadersBase> V updateParent(V parent) {
+        this.parent = requireNonNull(parent, "parent");
+        return parent;
+    }
+
     /**
      * Makes the current {@link #delegate()} a new {@link #parent()} and clears the current {@link #delegate()}.
      * Call this method when you create a new {@link HttpHeaders} derived from the {@link #delegate()},

diff --git a/core/src/main/java/com/linecorp/armeria/common/DefaultHttpHeadersBuilder.java b/core/src/main/java/com/linecorp/armeria/common/DefaultHttpHeadersBuilder.java
@@ -35,6 +35,13 @@ public HttpHeaders build() {
         }
 
         final HttpHeadersBase parent = parent();
-        return parent != null ? (HttpHeaders) parent : DefaultHttpHeaders.EMPTY;
+        if (parent != null) {
+            if (parent instanceof HttpHeaders) {
+                return (HttpHeaders) parent;
+            }
+            return updateParent(new DefaultHttpHeaders(parent));
+        }
+
+        return DefaultHttpHeaders.EMPTY;
     }
 }
diff --git a/core/src/main/java/com/linecorp/armeria/common/DefaultRequestHeadersBuilder.java b/core/src/main/java/com/linecorp/armeria/common/DefaultRequestHeadersBuilder.java
@@ -42,7 +42,11 @@ public RequestHeaders build() {
 
         final HttpHeadersBase parent = parent();
         if (parent != null) {
-            return (RequestHeaders) parent;
+            if (parent instanceof RequestHeaders) {
+                return (RequestHeaders) parent;
+            } else {
+                return updateParent(new DefaultRequestHeaders(parent));
+            }
         }
 
         // No headers were set.

diff --git a/core/src/main/java/com/linecorp/armeria/common/DefaultResponseHeadersBuilder.java b/core/src/main/java/com/linecorp/armeria/common/DefaultResponseHeadersBuilder.java
@@ -39,7 +39,11 @@ public ResponseHeaders build() {
 
         final HttpHeadersBase parent = parent();
         if (parent != null) {
-            return (ResponseHeaders) parent;
+            if (parent instanceof ResponseHeaders) {
+                return (ResponseHeaders) parent;
+            } else {
+                return updateParent(new DefaultResponseHeaders(parent));
+            }
         }
 
         // No headers were set.

diff --git a/core/src/test/java/com/linecorp/armeria/common/DefaultHttpHeadersBuilderTest.java b/core/src/test/java/com/linecorp/armeria/common/DefaultHttpHeadersBuilderTest.java
@@ -23,10 +23,10 @@
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Maps;
 
-public class DefaultHttpHeadersBuilderTest {
+class DefaultHttpHeadersBuilderTest {
 
     @Test
-    public void add() {
+    void add() {
         final HttpHeaders headers = HttpHeaders.builder()
                                                .add("a", "b")
                                                .add("c", ImmutableList.of("d", "e"))
@@ -43,7 +43,7 @@ public void add() {
     }
 
     @Test
-    public void set() {
+    void set() {
         final HttpHeaders headers = HttpHeaders.builder()
                                                .add("a", "b")
                                                .add("c", ImmutableList.of("d", "e"))
@@ -64,7 +64,7 @@ public void set() {
     }
 
     @Test
-    public void mutation() {
+    void mutation() {
         final HttpHeaders headers = HttpHeaders.of("a", "b");
         final HttpHeaders headers2 = headers.toBuilder().set("a", "c").build();
         assertThat(headers).isNotSameAs(headers2);
@@ -74,7 +74,7 @@ public void mutation() {
     }
 
     @Test
-    public void mutationEndOfStreamOnly() {
+    void mutationEndOfStreamOnly() {
         final HttpHeaders headers = HttpHeaders.of("a", "b");
         final HttpHeaders headers2 = headers.toBuilder().endOfStream(true).build();
         assertThat(headers.isEndOfStream()).isFalse();
@@ -86,7 +86,7 @@ public void mutationEndOfStreamOnly() {
     }
 
     @Test
-    public void mutationAfterBuild() {
+    void mutationAfterBuild() {
         final HttpHeaders headers = HttpHeaders.of("a", "b");
         final DefaultHttpHeadersBuilder builder = (DefaultHttpHeadersBuilder) headers.toBuilder();
 
@@ -136,7 +136,7 @@ public void mutationAfterBuild() {
     }
 
     @Test
-    public void noMutationNoCopy() {
+    void noMutationNoCopy() {
         final HttpHeaders headers = HttpHeaders.of("a", "b");
         assertThat(headers.toBuilder().build()).isSameAs(headers);
 
@@ -163,7 +163,7 @@ public void noMutationNoCopy() {
     }
 
     @Test
-    public void empty() {
+    void empty() {
         final HttpHeaders headers = HttpHeaders.of("a", "b");
         assertThat(headers.toBuilder()
                           .clear()
@@ -173,4 +173,11 @@ public void empty() {
                           .clear()
                           .build()).isSameAs(DefaultHttpHeaders.EMPTY_EOS);
     }
+
+    @Test
+    void buildTwice() {
+        final HttpHeadersBuilder builder = HttpHeaders.builder().add("foo", "bar");
+        assertThat(builder.build()).isEqualTo(HttpHeaders.of("foo", "bar"));
+        assertThat(builder.build()).isEqualTo(HttpHeaders.of("foo", "bar"));
+    }
 }
diff --git a/core/src/test/java/com/linecorp/armeria/common/DefaultRequestHeadersBuilderTest.java b/core/src/test/java/com/linecorp/armeria/common/DefaultRequestHeadersBuilderTest.java
@@ -88,6 +88,13 @@ void noMutationNoCopy() {
         assertThat(builder.delegate()).isNull();
     }
 
+    @Test
+    void buildTwice() {
+        final RequestHeadersBuilder builder = RequestHeaders.builder(HttpMethod.GET, "/").add("foo", "bar");
+        assertThat(builder.build()).isEqualTo(RequestHeaders.of(HttpMethod.GET, "/", "foo", "bar"));
+        assertThat(builder.build()).isEqualTo(RequestHeaders.of(HttpMethod.GET, "/", "foo", "bar"));
+    }
+
     @Test
     void validation() {
         assertThatThrownBy(() -> RequestHeaders.builder().build())

diff --git a/core/src/test/java/com/linecorp/armeria/common/DefaultResponseHeadersBuilderTest.java b/core/src/test/java/com/linecorp/armeria/common/DefaultResponseHeadersBuilderTest.java
@@ -22,9 +22,9 @@
 
 import com.google.common.collect.Maps;
 
-public class DefaultResponseHeadersBuilderTest {
+class DefaultResponseHeadersBuilderTest {
     @Test
-    public void mutationAfterBuild() {
+    void mutationAfterBuild() {
         final ResponseHeaders headers = ResponseHeaders.of(200);
         final DefaultResponseHeadersBuilder builder = (DefaultResponseHeadersBuilder) headers.toBuilder();
 
@@ -73,15 +73,22 @@ public void mutationAfterBuild() {
     }
 
     @Test
-    public void noMutationNoCopy() {
+    void noMutationNoCopy() {
         final ResponseHeaders headers = ResponseHeaders.of(200);
         final DefaultResponseHeadersBuilder builder = (DefaultResponseHeadersBuilder) headers.toBuilder();
         assertThat(builder.build()).isSameAs(headers);
         assertThat(builder.delegate()).isNull();
     }
 
     @Test
-    public void validation() {
+    void buildTwice() {
+        final ResponseHeadersBuilder builder = ResponseHeaders.builder(200).add("foo", "bar");
+        assertThat(builder.build()).isEqualTo(ResponseHeaders.of(HttpStatus.OK, "foo", "bar"));
+        assertThat(builder.build()).isEqualTo(ResponseHeaders.of(HttpStatus.OK, "foo", "bar"));
+    }
+
+    @Test
+    void validation() {
         // When delegate is null.
         assertThatThrownBy(() -> ResponseHeaders.builder().build())
                 .isInstanceOf(IllegalStateException.class)