From 56318df10ce35bb778ed1b99012e3a0093685f0d Mon Sep 17 00:00:00 2001
From: Nik Everett <nik9000@gmail.com>
Date: Fri, 9 Oct 2015 10:28:13 -0400
Subject: [PATCH] Be more careful with RoutingTable.Builder

RoutingTable.Builder#build can only be called once but didn't have any
checks to make sure it _was_ only called once. And the error message it
threw when called more than once was a NullPointerException. This makes
it throw IllegalStateException when you try to reuse the builder in any way.
---
 .../cluster/routing/RoutingTable.java         | 27 +++++++++++++---
 .../cluster/routing/RoutingTableTests.java    | 32 +++++++++++++++++++
 2 files changed, 54 insertions(+), 5 deletions(-)

diff --git a/core/src/main/java/org/elasticsearch/cluster/routing/RoutingTable.java b/core/src/main/java/org/elasticsearch/cluster/routing/RoutingTable.java
index 1cbf8605210..10d7ff9deaf 100644
--- a/core/src/main/java/org/elasticsearch/cluster/routing/RoutingTable.java
+++ b/core/src/main/java/org/elasticsearch/cluster/routing/RoutingTable.java
@@ -350,7 +350,7 @@ public class RoutingTable implements Iterable<IndexRoutingTable>, Diffable<Routi
     public static class Builder {
 
         private long version;
-        private final ImmutableOpenMap.Builder<String, IndexRoutingTable> indicesRouting = ImmutableOpenMap.builder();
+        private ImmutableOpenMap.Builder<String, IndexRoutingTable> indicesRouting = ImmutableOpenMap.builder();
 
         public Builder() {
 
@@ -406,6 +406,9 @@ public class RoutingTable implements Iterable<IndexRoutingTable>, Diffable<Routi
         }
 
         public Builder updateNumberOfReplicas(int numberOfReplicas, String... indices) {
+            if (indicesRouting == null) {
+                throw new IllegalStateException("once build is called the builder cannot be reused");
+            }
             if (indices == null || indices.length == 0) {
                 indices = indicesRouting.keys().toArray(String.class);
             }
@@ -492,6 +495,9 @@ public class RoutingTable implements Iterable<IndexRoutingTable>, Diffable<Routi
         }
 
         public Builder add(IndexRoutingTable indexRoutingTable) {
+            if (indicesRouting == null) {
+                throw new IllegalStateException("once build is called the builder cannot be reused");
+            }
             indexRoutingTable.validate();
             indicesRouting.put(indexRoutingTable.index(), indexRoutingTable);
             return this;
@@ -503,11 +509,17 @@ public class RoutingTable implements Iterable<IndexRoutingTable>, Diffable<Routi
         }
 
         public Builder indicesRouting(Map<String, IndexRoutingTable> indicesRouting) {
+            if (indicesRouting == null) {
+                throw new IllegalStateException("once build is called the builder cannot be reused");
+            }
             this.indicesRouting.putAll(indicesRouting);
             return this;
         }
 
         public Builder remove(String index) {
+            if (indicesRouting == null) {
+                throw new IllegalStateException("once build is called the builder cannot be reused");
+            }
             indicesRouting.remove(index);
             return this;
         }
@@ -518,16 +530,21 @@ public class RoutingTable implements Iterable<IndexRoutingTable>, Diffable<Routi
         }
 
         /**
-         * Builds the routing table. Note that this can only be called one time.
-         * If you need to build a new RoutingTable as a copy of this one you'll
-         * need to build a new RoutingTable.Builder.
+         * Builds the routing table. Note that once this is called the builder
+         * must be thrown away. If you need to build a new RoutingTable as a
+         * copy of this one you'll need to build a new RoutingTable.Builder.
          */
         public RoutingTable build() {
+            if (indicesRouting == null) {
+                throw new IllegalStateException("once build is called the builder cannot be reused");
+            }
             // normalize the versions right before we build it...
             for (ObjectCursor<IndexRoutingTable> indexRoutingTable : indicesRouting.values()) {
                 indicesRouting.put(indexRoutingTable.value.index(), indexRoutingTable.value.normalizeVersions());
             }
-            return new RoutingTable(version, indicesRouting.build());
+            RoutingTable table = new RoutingTable(version, indicesRouting.build());
+            indicesRouting = null;
+            return table;
         }
 
         public static RoutingTable readFrom(StreamInput in) throws IOException {
diff --git a/core/src/test/java/org/elasticsearch/cluster/routing/RoutingTableTests.java b/core/src/test/java/org/elasticsearch/cluster/routing/RoutingTableTests.java
index ea5dda0128b..2a7ed6a4a93 100644
--- a/core/src/test/java/org/elasticsearch/cluster/routing/RoutingTableTests.java
+++ b/core/src/test/java/org/elasticsearch/cluster/routing/RoutingTableTests.java
@@ -35,6 +35,7 @@ import org.junit.Test;
 
 import static org.elasticsearch.cluster.routing.ShardRoutingState.INITIALIZING;
 import static org.elasticsearch.common.settings.Settings.settingsBuilder;
+import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.nullValue;
 
@@ -55,6 +56,7 @@ public class RoutingTableTests extends ESAllocationTestCase {
             .build());
     private ClusterState clusterState;
 
+    @Override
     @Before
     public void setUp() throws Exception {
         super.setUp();
@@ -264,4 +266,34 @@ public class RoutingTableTests extends ESAllocationTestCase {
             fail("Calling with non-existing index should be ignored at the moment");
         }
     }
+
+    public void testRoutingTableBuiltMoreThanOnce() {
+        RoutingTable.Builder b = RoutingTable.builder();
+        b.build(); // Ok the first time
+        try {
+            b.build();
+            fail("expected exception");
+        } catch (IllegalStateException e) {
+            assertThat(e.getMessage(), containsString("cannot be reused"));
+        }
+        try {
+            b.add((IndexRoutingTable) null);
+            fail("expected exception");
+        } catch (IllegalStateException e) {
+            assertThat(e.getMessage(), containsString("cannot be reused"));
+        }
+        try {
+            b.updateNumberOfReplicas(1, "foo");
+            fail("expected exception");
+        } catch (IllegalStateException e) {
+            assertThat(e.getMessage(), containsString("cannot be reused"));
+        }
+        try {
+            b.remove("foo");
+            fail("expected exception");
+        } catch (IllegalStateException e) {
+            assertThat(e.getMessage(), containsString("cannot be reused"));
+        }
+
+    }
 }