updates to Settings::merge from code review

Author: masseyke

server/src/main/java/org/elasticsearch/common/settings/Settings.java

@@ -882,16 +882,22 @@ public Set<String> keySet() {
      * from the returned object.
      */
     public Settings merge(Settings newSettings) {
-        if (newSettings == null || Settings.EMPTY.equals(newSettings)) {
+        Objects.requireNonNull(newSettings);
+        if (Settings.EMPTY.equals(newSettings)) {
             return this;
         }
-        Settings.Builder settingsBuilder = Settings.builder().put(this).put(newSettings);
-        for (String settingName : new HashSet<>(settingsBuilder.keys())) {
-            if (settingsBuilder.get(settingName) == null) {
-                settingsBuilder.remove(settingName);
+        Settings.Builder builder = Settings.builder().put(this).put(newSettings);
+        for (String key : newSettings.keySet()) {
+            String rawValue = newSettings.get(key);
+            if (builder.get(key) == null) {
+                if (rawValue == null) {
+                    builder.remove(key);
+                } else {
+                    builder.put(key, rawValue);
+                }
             }
         }
-        return settingsBuilder.build();
+        return builder.build();
     }
 
     /**

server/src/test/java/org/elasticsearch/common/settings/SettingsTests.java

@@ -722,58 +722,59 @@ public void testGlobValues() throws IOException {
         assertThat(values, contains("1"));
     }
 
-    public void testMerge() {
-        {
-            assertThat(Settings.EMPTY.merge(null), equalTo(Settings.EMPTY));
-            assertThat(Settings.EMPTY.merge(Settings.EMPTY), equalTo(Settings.EMPTY));
-        }
-        {
-            Settings.Builder builder = Settings.builder();
-            for (int i = 1; i < randomInt(100); i++) {
-                builder.put(randomAlphanumericOfLength(20), randomAlphanumericOfLength(50));
-            }
-            Settings settings = builder.build();
-            assertThat(settings.merge(null), equalTo(settings));
-            assertThat(settings.merge(Settings.EMPTY), equalTo(settings));
-        }
-        {
-            Settings.Builder builder = Settings.builder();
-            for (int i = 1; i < randomInt(100); i++) {
-                builder.put(randomAlphanumericOfLength(20), randomAlphanumericOfLength(50));
-            }
-            Settings newSettings = builder.build();
-            assertThat(Settings.EMPTY.merge(newSettings), equalTo(newSettings));
-        }
-        {
-            Settings settings = Settings.builder()
-                .put("index.setting1", "templateValue")
-                .put("index.setting3", "templateValue")
-                .put("index.setting4", "templateValue")
-                .build();
-            Settings newSettings = Settings.builder()
-                .put("index.setting1", "dataStreamValue")
-                .put("index.setting2", "dataStreamValue")
-                .put("index.setting3", (String) null) // This one gets removed from the effective settings
-                .build();
-            Settings mergedSettings = Settings.builder()
-                .put("index.setting1", "dataStreamValue")
-                .put("index.setting2", "dataStreamValue")
-                .put("index.setting4", "templateValue")
-                .build();
-            assertThat(settings.merge(newSettings), equalTo(mergedSettings));
+    public void testMergeNullOrEmptySettingsIntoEmptySettings() {
+        expectThrows(NullPointerException.class, () -> Settings.EMPTY.merge(null));
+        assertThat(Settings.EMPTY.merge(Settings.EMPTY), equalTo(Settings.EMPTY));
+    }
+
+    public void testMergeEmptySettings() {
+        Settings.Builder builder = Settings.builder();
+        for (int i = 1; i < randomInt(100); i++) {
+            builder.put(randomAlphanumericOfLength(20), randomAlphanumericOfLength(50));
         }
-        {
-            Settings newSettings = Settings.builder()
-                .put("index.setting1", "dataStreamValue")
-                .put("index.setting2", "dataStreamValue")
-                .put("index.setting3", (String) null) // This one gets removed from the effective settings
-                .build();
-            Settings mergedSettings = Settings.builder()
-                .put("index.setting1", "dataStreamValue")
-                .put("index.setting2", "dataStreamValue")
-                .build();
-            assertThat(Settings.EMPTY.merge(newSettings), equalTo(mergedSettings));
+        Settings settings = builder.build();
+        assertThat(settings.merge(Settings.EMPTY), equalTo(settings));
+    }
+
+    public void testMergeNonEmptySettingsIntoEmptySettings() {
+        Settings.Builder builder = Settings.builder();
+        for (int i = 1; i < randomInt(100); i++) {
+            builder.put(randomAlphanumericOfLength(20), randomAlphanumericOfLength(50));
         }
+        Settings newSettings = builder.build();
+        assertThat(Settings.EMPTY.merge(newSettings), equalTo(newSettings));
+    }
+
+    public void testMergeNonEmptySettingsIntoNonEmptySettings() {
+        Settings settings = Settings.builder()
+            .put("index.setting1", "templateValue")
+            .put("index.setting3", "templateValue")
+            .put("index.setting4", "templateValue")
+            .build();
+        Settings newSettings = Settings.builder()
+            .put("index.setting1", "dataStreamValue")
+            .put("index.setting2", "dataStreamValue")
+            .put("index.setting3", (String) null) // This one gets removed from the effective settings
+            .build();
+        Settings mergedSettings = Settings.builder()
+            .put("index.setting1", "dataStreamValue")
+            .put("index.setting2", "dataStreamValue")
+            .put("index.setting4", "templateValue")
+            .build();
+        assertThat(settings.merge(newSettings), equalTo(mergedSettings));
+    }
+
+    public void testMergeNonEmptySettingsWithNullIntoEmptySettings() {
+        Settings newSettings = Settings.builder()
+            .put("index.setting1", "dataStreamValue")
+            .put("index.setting2", "dataStreamValue")
+            .put("index.setting3", (String) null) // This one gets removed from the effective settings
+            .build();
+        Settings mergedSettings = Settings.builder()
+            .put("index.setting1", "dataStreamValue")
+            .put("index.setting2", "dataStreamValue")
+            .build();
+        assertThat(Settings.EMPTY.merge(newSettings), equalTo(mergedSettings));
     }
 
 }