From ba14aca2185ef578280f8b05516794423d9a24e0 Mon Sep 17 00:00:00 2001
From: Jason Tedor <jason@tedor.me>
Date: Sat, 21 May 2016 11:02:41 -0400
Subject: [PATCH] Refactor property placeholder use of env. vars

This commit is a slight refactoring of the use of environment variables
in replacing property placeholders. In commit
115f983827c0c29652bd444c072b658b76651317 the constructor for
Settings.Builder was made package visible to provide a hook for tests to
mock obtaining environment variables. But we do not need to go that far
and can instead provide a small hook for this for tests without opening
up the constructor. Thus, in this commit we refactor
Settings.Builder#replacePropertyPlaceholders to a package-visible method
that accepts a function providing environment variables by names. The
public-visible method just delegates to this method passing in
System::getenv and tests can use the package-visible method to mock the
behavior they need without relying on external environment variables.
---
 .../common/settings/Settings.java             | 55 +++++++++----------
 .../common/settings/SettingsTests.java        | 10 +---
 2 files changed, 29 insertions(+), 36 deletions(-)

diff --git a/core/src/main/java/org/elasticsearch/common/settings/Settings.java b/core/src/main/java/org/elasticsearch/common/settings/Settings.java
index 2ceafc376c5..15554e5ccaa 100644
--- a/core/src/main/java/org/elasticsearch/common/settings/Settings.java
+++ b/core/src/main/java/org/elasticsearch/common/settings/Settings.java
@@ -602,8 +602,7 @@ public final class Settings implements ToXContent {
 
         private final Map<String, String> map = new LinkedHashMap<>();
 
-        // visible for testing
-        Builder() {
+        private Builder() {
 
         }
 
@@ -961,33 +960,38 @@ public final class Settings implements ToXContent {
          * another setting already set on this builder.
          */
         public Builder replacePropertyPlaceholders() {
+            return replacePropertyPlaceholders(System::getenv);
+        }
+
+        // visible for testing
+        Builder replacePropertyPlaceholders(Function<String, String> getenv) {
             PropertyPlaceholder propertyPlaceholder = new PropertyPlaceholder("${", "}", false);
             PropertyPlaceholder.PlaceholderResolver placeholderResolver = new PropertyPlaceholder.PlaceholderResolver() {
-                    @Override
-                    public String resolvePlaceholder(String placeholderName) {
-                        final String value = getenv(placeholderName);
-                        if (value != null) {
-                            return value;
-                        }
-                        return map.get(placeholderName);
+                @Override
+                public String resolvePlaceholder(String placeholderName) {
+                    final String value = getenv.apply(placeholderName);
+                    if (value != null) {
+                        return value;
                     }
+                    return map.get(placeholderName);
+                }
 
-                    @Override
-                    public boolean shouldIgnoreMissing(String placeholderName) {
-                        if (placeholderName.startsWith("prompt.")) {
-                            return true;
-                        }
-                        return false;
-                    }
-
-                    @Override
-                    public boolean shouldRemoveMissingPlaceholder(String placeholderName) {
-                        if (placeholderName.startsWith("prompt.")) {
-                            return false;
-                        }
+                @Override
+                public boolean shouldIgnoreMissing(String placeholderName) {
+                    if (placeholderName.startsWith("prompt.")) {
                         return true;
                     }
-                };
+                    return false;
+                }
+
+                @Override
+                public boolean shouldRemoveMissingPlaceholder(String placeholderName) {
+                    if (placeholderName.startsWith("prompt.")) {
+                        return false;
+                    }
+                    return true;
+                }
+            };
             for (Map.Entry<String, String> entry : new HashMap<>(map).entrySet()) {
                 String value = propertyPlaceholder.replacePlaceholders(entry.getKey(), entry.getValue(), placeholderResolver);
                 // if the values exists and has length, we should maintain it  in the map
@@ -1001,11 +1005,6 @@ public final class Settings implements ToXContent {
             return this;
         }
 
-        // visible for testing
-        String getenv(String placeholderName) {
-            return System.getenv(placeholderName);
-        }
-
         /**
          * Checks that all settings in the builder start with the specified prefix.
          *
diff --git a/core/src/test/java/org/elasticsearch/common/settings/SettingsTests.java b/core/src/test/java/org/elasticsearch/common/settings/SettingsTests.java
index 5b3a698d328..346c5bc60de 100644
--- a/core/src/test/java/org/elasticsearch/common/settings/SettingsTests.java
+++ b/core/src/test/java/org/elasticsearch/common/settings/SettingsTests.java
@@ -63,15 +63,9 @@ public class SettingsTests extends ESTestCase {
 
     public void testReplacePropertiesPlaceholderByEnvironmentVariables() {
         final String hostname = randomAsciiOfLength(16);
-        final Settings.Builder builder = new Settings.Builder() {
-            @Override
-            protected String getenv(String placeholderName) {
-                return "HOSTNAME".equals(placeholderName) ? hostname : null;
-            }
-        };
-        final Settings implicitEnvSettings = builder
+        final Settings implicitEnvSettings = Settings.builder()
             .put("setting1", "${HOSTNAME}")
-            .replacePropertyPlaceholders()
+            .replacePropertyPlaceholders(name -> "HOSTNAME".equals(name) ? hostname : null)
             .build();
         assertThat(implicitEnvSettings.get("setting1"), equalTo(hostname));
     }