From 21d749a6aa0796ce466b83d0ac3fb5f3081d9a99 Mon Sep 17 00:00:00 2001
From: Shay Banon <kimchy@gmail.com>
Date: Sun, 12 May 2013 05:18:03 +0200
Subject: [PATCH] resolved empty setting values should be removed when
 resolving empty settings values, their value should be removed, for example,
 when using ${env.ENV_VAR}, and ENV_VAR is not set, then the setting should be
 removed

---
 .../common/settings/ImmutableSettings.java    |  7 +++-
 .../settings/ImmutableSettingsTests.java      | 37 ++++++++++++++++++-
 2 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/src/main/java/org/elasticsearch/common/settings/ImmutableSettings.java b/src/main/java/org/elasticsearch/common/settings/ImmutableSettings.java
index 4f2fb4831f2..a783de44693 100644
--- a/src/main/java/org/elasticsearch/common/settings/ImmutableSettings.java
+++ b/src/main/java/org/elasticsearch/common/settings/ImmutableSettings.java
@@ -21,6 +21,7 @@ package org.elasticsearch.common.settings;
 
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Lists;
+import com.google.common.collect.Maps;
 import org.elasticsearch.ElasticSearchIllegalArgumentException;
 import org.elasticsearch.Version;
 import org.elasticsearch.common.Booleans;
@@ -861,10 +862,14 @@ public class ImmutableSettings implements Settings {
                     return false;
                 }
             };
-            for (Map.Entry<String, String> entry : map.entrySet()) {
+            for (Map.Entry<String, String> entry : Maps.newHashMap(map).entrySet()) {
                 String value = propertyPlaceholder.replacePlaceholders(entry.getValue(), placeholderResolver);
+                // if the values exists and has length, we should maintain it  in the map
+                // otherwise, the replace process resolved into removing it
                 if (Strings.hasLength(value)) {
                     map.put(entry.getKey(), value);
+                } else {
+                    map.remove(entry.getKey());
                 }
             }
             return this;
diff --git a/src/test/java/org/elasticsearch/test/unit/common/settings/ImmutableSettingsTests.java b/src/test/java/org/elasticsearch/test/unit/common/settings/ImmutableSettingsTests.java
index b1fd9fda5ae..5ddc70758c0 100644
--- a/src/test/java/org/elasticsearch/test/unit/common/settings/ImmutableSettingsTests.java
+++ b/src/test/java/org/elasticsearch/test/unit/common/settings/ImmutableSettingsTests.java
@@ -27,7 +27,7 @@ import org.testng.annotations.Test;
 
 import static org.elasticsearch.common.settings.ImmutableSettings.settingsBuilder;
 import static org.hamcrest.MatcherAssert.assertThat;
-import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.*;
 
 /**
  */
@@ -87,4 +87,39 @@ public class ImmutableSettingsTests {
         Settings settings = settingsBuilder().put("type", "ngram").build();
         settings.getAsClass("type", null, "org.elasticsearch.index.analysis.", "TokenFilterFactory");
     }
+
+    @Test
+    public void testReplacePropertiesPlaceholderSystemProperty() {
+        System.setProperty("sysProp1", "sysVal1");
+        try {
+            Settings settings = settingsBuilder()
+                    .put("setting1", "${sysProp1}")
+                    .replacePropertyPlaceholders()
+                    .build();
+            assertThat(settings.get("setting1"), equalTo("sysVal1"));
+        } finally {
+            System.clearProperty("sysProp1");
+        }
+
+        Settings settings = settingsBuilder()
+                .put("setting1", "${sysProp1:defaultVal1}")
+                .replacePropertyPlaceholders()
+                .build();
+        assertThat(settings.get("setting1"), equalTo("defaultVal1"));
+
+        settings = settingsBuilder()
+                .put("setting1", "${sysProp1:}")
+                .replacePropertyPlaceholders()
+                .build();
+        assertThat(settings.get("setting1"), is(nullValue()));
+    }
+
+    @Test
+    public void testReplacePropertiesPlaceholderIgnoreEnvUnset() {
+        Settings settings = settingsBuilder()
+                .put("setting1", "${env.UNSET_ENV_VAR}")
+                .replacePropertyPlaceholders()
+                .build();
+        assertThat(settings.get("setting1"), is(nullValue()));
+    }
 }