From 873d0020a5a6549390a9b4e4cfeccb332734729f Mon Sep 17 00:00:00 2001 From: David Turner Date: Fri, 3 May 2019 14:49:23 +0200 Subject: [PATCH] Reject null customs at build time (#41782) Today you can add a null `Custom` to the cluster state or its metadata, but attempting to publish such a cluster state will fail. Unfortunately, the publication-time failure gives very little information about the source of the problem. This change causes the failure to manifest earlier and adds information about which `Custom` was null in order to simplify the investigation. Relates #41090. --- .../org/elasticsearch/cluster/ClusterState.java | 5 ++++- .../cluster/metadata/MetaData.java | 5 ++++- .../cluster/ClusterStateTests.java | 17 +++++++++++++++++ .../cluster/metadata/MetaDataTests.java | 16 ++++++++++++++++ 4 files changed, 41 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/ClusterState.java b/server/src/main/java/org/elasticsearch/cluster/ClusterState.java index cf605117bc0..c77d3f01e5f 100644 --- a/server/src/main/java/org/elasticsearch/cluster/ClusterState.java +++ b/server/src/main/java/org/elasticsearch/cluster/ClusterState.java @@ -66,8 +66,10 @@ import java.util.EnumSet; import java.util.HashMap; import java.util.Locale; import java.util.Map; +import java.util.Objects; import java.util.Optional; import java.util.Set; +import java.util.stream.StreamSupport; import static org.elasticsearch.cluster.coordination.Coordinator.ZEN1_BWC_TERM; @@ -736,7 +738,7 @@ public class ClusterState implements ToXContentFragment, Diffable } public Builder putCustom(String type, Custom custom) { - customs.put(type, custom); + customs.put(type, Objects.requireNonNull(custom, type)); return this; } @@ -746,6 +748,7 @@ public class ClusterState implements ToXContentFragment, Diffable } public Builder customs(ImmutableOpenMap customs) { + StreamSupport.stream(customs.spliterator(), false).forEach(cursor -> Objects.requireNonNull(cursor.value, cursor.key)); this.customs.putAll(customs); return this; } diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java index 54c3001d903..436de2e2e7a 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetaData.java @@ -74,11 +74,13 @@ import java.util.HashSet; import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; import java.util.SortedMap; import java.util.TreeMap; import java.util.function.Function; import java.util.function.Predicate; +import java.util.stream.StreamSupport; import static org.elasticsearch.common.settings.Settings.readSettingsFromStream; import static org.elasticsearch.common.settings.Settings.writeSettingsToStream; @@ -1069,7 +1071,7 @@ public class MetaData implements Iterable, Diffable, To } public Builder putCustom(String type, Custom custom) { - customs.put(type, custom); + customs.put(type, Objects.requireNonNull(custom, type)); return this; } @@ -1079,6 +1081,7 @@ public class MetaData implements Iterable, Diffable, To } public Builder customs(ImmutableOpenMap customs) { + StreamSupport.stream(customs.spliterator(), false).forEach(cursor -> Objects.requireNonNull(cursor.value, cursor.key)); this.customs.putAll(customs); return this; } diff --git a/server/src/test/java/org/elasticsearch/cluster/ClusterStateTests.java b/server/src/test/java/org/elasticsearch/cluster/ClusterStateTests.java index 3138fb19b6c..86d3eec9c9c 100644 --- a/server/src/test/java/org/elasticsearch/cluster/ClusterStateTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/ClusterStateTests.java @@ -21,11 +21,13 @@ package org.elasticsearch.cluster; import org.elasticsearch.Version; import org.elasticsearch.cluster.node.DiscoveryNode; import org.elasticsearch.cluster.node.DiscoveryNodes; +import org.elasticsearch.common.collect.ImmutableOpenMap; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.test.ESTestCase; import static java.util.Collections.emptyMap; import static java.util.Collections.emptySet; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; public class ClusterStateTests extends ESTestCase { @@ -56,4 +58,19 @@ public class ClusterStateTests extends ESTestCase { // state from the same master compare by version assertThat(withMaster1a.supersedes(withMaster1b), equalTo(withMaster1a.version() > withMaster1b.version())); } + + public void testBuilderRejectsNullCustom() { + final ClusterState.Builder builder = ClusterState.builder(ClusterName.DEFAULT); + final String key = randomAlphaOfLength(10); + assertThat(expectThrows(NullPointerException.class, () -> builder.putCustom(key, null)).getMessage(), containsString(key)); + } + + public void testBuilderRejectsNullInCustoms() { + final ClusterState.Builder builder = ClusterState.builder(ClusterName.DEFAULT); + final String key = randomAlphaOfLength(10); + final ImmutableOpenMap.Builder mapBuilder = ImmutableOpenMap.builder(); + mapBuilder.put(key, null); + final ImmutableOpenMap map = mapBuilder.build(); + assertThat(expectThrows(NullPointerException.class, () -> builder.customs(map)).getMessage(), containsString(key)); + } } diff --git a/server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataTests.java b/server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataTests.java index 685b7cca98a..e9893f16f95 100644 --- a/server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/metadata/MetaDataTests.java @@ -50,6 +50,7 @@ import java.util.List; import java.util.Map; import java.util.Set; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.startsWith; @@ -891,4 +892,19 @@ public class MetaDataTests extends ESTestCase { .transientSettings(Settings.builder().put(setting.getKey(), "transient-value").build()).build(); assertThat(setting.get(metaData.settings()), equalTo("transient-value")); } + + public void testBuilderRejectsNullCustom() { + final MetaData.Builder builder = MetaData.builder(); + final String key = randomAlphaOfLength(10); + assertThat(expectThrows(NullPointerException.class, () -> builder.putCustom(key, null)).getMessage(), containsString(key)); + } + + public void testBuilderRejectsNullInCustoms() { + final MetaData.Builder builder = MetaData.builder(); + final String key = randomAlphaOfLength(10); + final ImmutableOpenMap.Builder mapBuilder = ImmutableOpenMap.builder(); + mapBuilder.put(key, null); + final ImmutableOpenMap map = mapBuilder.build(); + assertThat(expectThrows(NullPointerException.class, () -> builder.customs(map)).getMessage(), containsString(key)); + } }