From 78630e03a2cb3b70edec00869db163c07079d9bf Mon Sep 17 00:00:00 2001 From: jaymode Date: Sat, 6 Jun 2015 10:41:07 -0400 Subject: [PATCH] make prompt placeholders consistent with existing placeholders In #10918, we introduced the prompt placeholders. These were had a different format than our existing placeholders. This changes the prompt placeholders to follow the format of the existing placeholders. Relates to #11455 --- .../common/property/PropertyPlaceholder.java | 14 ++++++- .../common/settings/Settings.java | 26 ++++++------- .../internal/InternalSettingsPreparer.java | 13 +++---- .../property/PropertyPlaceholderTest.java | 39 +++++++++++++------ .../common/settings/SettingsTests.java | 12 +++--- docs/reference/setup/configuration.asciidoc | 10 ++--- 6 files changed, 67 insertions(+), 47 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/common/property/PropertyPlaceholder.java b/core/src/main/java/org/elasticsearch/common/property/PropertyPlaceholder.java index 5b4515f7b96..a4b067e1cea 100644 --- a/core/src/main/java/org/elasticsearch/common/property/PropertyPlaceholder.java +++ b/core/src/main/java/org/elasticsearch/common/property/PropertyPlaceholder.java @@ -109,7 +109,11 @@ public class PropertyPlaceholder { propVal = defaultValue; } if (propVal == null && placeholderResolver.shouldIgnoreMissing(placeholder)) { - propVal = ""; + if (placeholderResolver.shouldRemoveMissingPlaceholder(placeholder)) { + propVal = ""; + } else { + return strVal; + } } if (propVal != null) { // Recursive invocation, parsing placeholders contained in the @@ -170,5 +174,13 @@ public class PropertyPlaceholder { String resolvePlaceholder(String placeholderName); boolean shouldIgnoreMissing(String placeholderName); + + /** + * Allows for special handling for ignored missing placeholders that may be resolved elsewhere + * + * @param placeholderName the name of the placeholder to resolve. + * @return true if the placeholder should be replaced with a empty string + */ + boolean shouldRemoveMissingPlaceholder(String placeholderName); } } 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 a5929d5fa90..24743afe980 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/Settings.java +++ b/core/src/main/java/org/elasticsearch/common/settings/Settings.java @@ -1245,7 +1245,7 @@ public final class Settings implements ToXContent { * tries and resolve it against an environment variable ({@link System#getenv(String)}), and last, tries * and replace it with another setting already set on this builder. */ - public Builder replacePropertyPlaceholders(String... ignoredValues) { + public Builder replacePropertyPlaceholders() { PropertyPlaceholder propertyPlaceholder = new PropertyPlaceholder("${", "}", false); PropertyPlaceholder.PlaceholderResolver placeholderResolver = new PropertyPlaceholder.PlaceholderResolver() { @Override @@ -1268,26 +1268,22 @@ public final class Settings implements ToXContent { @Override public boolean shouldIgnoreMissing(String placeholderName) { // if its an explicit env var, we are ok with not having a value for it and treat it as optional - if (placeholderName.startsWith("env.")) { + if (placeholderName.startsWith("env.") || placeholderName.startsWith("prompt.")) { return true; } return false; } + + @Override + public boolean shouldRemoveMissingPlaceholder(String placeholderName) { + if (placeholderName.startsWith("prompt.")) { + return false; + } + return true; + } }; for (Map.Entry entry : Maps.newHashMap(map).entrySet()) { - String possiblePlaceholder = entry.getValue(); - boolean ignored = false; - for (String ignoredValue : ignoredValues) { - if (ignoredValue.equals(possiblePlaceholder)) { - ignored = true; - break; - } - } - if (ignored) { - continue; - } - - String value = propertyPlaceholder.replacePlaceholders(possiblePlaceholder, placeholderResolver); + 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)) { diff --git a/core/src/main/java/org/elasticsearch/node/internal/InternalSettingsPreparer.java b/core/src/main/java/org/elasticsearch/node/internal/InternalSettingsPreparer.java index 324158f894a..dbdaa5a1aeb 100644 --- a/core/src/main/java/org/elasticsearch/node/internal/InternalSettingsPreparer.java +++ b/core/src/main/java/org/elasticsearch/node/internal/InternalSettingsPreparer.java @@ -44,8 +44,8 @@ public class InternalSettingsPreparer { static final List ALLOWED_SUFFIXES = ImmutableList.of(".yml", ".yaml", ".json", ".properties"); - public static final String SECRET_PROMPT_VALUE = "${prompt::secret}"; - public static final String TEXT_PROMPT_VALUE = "${prompt::text}"; + public static final String SECRET_PROMPT_VALUE = "${prompt.secret}"; + public static final String TEXT_PROMPT_VALUE = "${prompt.text}"; public static final String IGNORE_SYSTEM_PROPERTIES_SETTING = "config.ignore_system_properties"; /** @@ -72,9 +72,6 @@ public class InternalSettingsPreparer { public static Tuple prepareSettings(Settings pSettings, boolean loadConfigSettings, Terminal terminal) { // ignore this prefixes when getting properties from es. and elasticsearch. String[] ignorePrefixes = new String[]{"es.default.", "elasticsearch.default."}; - // ignore the special prompt placeholders since they have the same format as property placeholders and will be resolved - // as having a default value because of the ':' in the format - String[] ignoredPlaceholders = new String[] { SECRET_PROMPT_VALUE, TEXT_PROMPT_VALUE }; boolean useSystemProperties = !pSettings.getAsBoolean(IGNORE_SYSTEM_PROPERTIES_SETTING, false); // just create enough settings to build the environment Settings.Builder settingsBuilder = settingsBuilder().put(pSettings); @@ -84,7 +81,7 @@ public class InternalSettingsPreparer { .putProperties("elasticsearch.", System.getProperties(), ignorePrefixes) .putProperties("es.", System.getProperties(), ignorePrefixes); } - settingsBuilder.replacePropertyPlaceholders(ignoredPlaceholders); + settingsBuilder.replacePropertyPlaceholders(); Environment environment = new Environment(settingsBuilder.build()); @@ -122,7 +119,7 @@ public class InternalSettingsPreparer { settingsBuilder.putProperties("elasticsearch.", System.getProperties(), ignorePrefixes) .putProperties("es.", System.getProperties(), ignorePrefixes); } - settingsBuilder.replacePropertyPlaceholders(ignoredPlaceholders); + settingsBuilder.replacePropertyPlaceholders(); // allow to force set properties based on configuration of the settings provided for (Map.Entry entry : pSettings.getAsMap().entrySet()) { @@ -132,7 +129,7 @@ public class InternalSettingsPreparer { settingsBuilder.put(setting.substring("force.".length()), entry.getValue()); } } - settingsBuilder.replacePropertyPlaceholders(ignoredPlaceholders); + settingsBuilder.replacePropertyPlaceholders(); // generate the name if (settingsBuilder.get("name") == null) { diff --git a/core/src/test/java/org/elasticsearch/common/property/PropertyPlaceholderTest.java b/core/src/test/java/org/elasticsearch/common/property/PropertyPlaceholderTest.java index c79d0917c16..3e4d3fffece 100644 --- a/core/src/test/java/org/elasticsearch/common/property/PropertyPlaceholderTest.java +++ b/core/src/test/java/org/elasticsearch/common/property/PropertyPlaceholderTest.java @@ -33,7 +33,7 @@ public class PropertyPlaceholderTest extends ElasticsearchTestCase { Map map = new LinkedHashMap<>(); map.put("foo1", "bar1"); map.put("foo2", "bar2"); - PropertyPlaceholder.PlaceholderResolver placeholderResolver = new SimplePlaceholderResolver(map, false); + PropertyPlaceholder.PlaceholderResolver placeholderResolver = new SimplePlaceholderResolver(map, false, true); assertEquals("bar1", propertyPlaceholder.replacePlaceholders("{foo1}", placeholderResolver)); assertEquals("a bar1b", propertyPlaceholder.replacePlaceholders("a {foo1}b", placeholderResolver)); assertEquals("bar1bar2", propertyPlaceholder.replacePlaceholders("{foo1}{foo2}", placeholderResolver)); @@ -48,7 +48,7 @@ public class PropertyPlaceholderTest extends ElasticsearchTestCase { PropertyPlaceholder ppShorterPrefix = new PropertyPlaceholder("{", "}}", false); Map map = new LinkedHashMap<>(); map.put("foo", "bar"); - PropertyPlaceholder.PlaceholderResolver placeholderResolver = new SimplePlaceholderResolver(map, false); + PropertyPlaceholder.PlaceholderResolver placeholderResolver = new SimplePlaceholderResolver(map, false, true); assertEquals("bar", ppEqualsPrefix.replacePlaceholders("{foo}", placeholderResolver)); assertEquals("bar", ppLongerPrefix.replacePlaceholders("${foo}", placeholderResolver)); assertEquals("bar", ppShorterPrefix.replacePlaceholders("{foo}}", placeholderResolver)); @@ -58,7 +58,7 @@ public class PropertyPlaceholderTest extends ElasticsearchTestCase { public void testDefaultValue() { PropertyPlaceholder propertyPlaceholder = new PropertyPlaceholder("${", "}", false); Map map = new LinkedHashMap<>(); - PropertyPlaceholder.PlaceholderResolver placeholderResolver = new SimplePlaceholderResolver(map, false); + PropertyPlaceholder.PlaceholderResolver placeholderResolver = new SimplePlaceholderResolver(map, false, true); assertEquals("bar", propertyPlaceholder.replacePlaceholders("${foo:bar}", placeholderResolver)); assertEquals("", propertyPlaceholder.replacePlaceholders("${foo:}", placeholderResolver)); } @@ -67,7 +67,7 @@ public class PropertyPlaceholderTest extends ElasticsearchTestCase { public void testIgnoredUnresolvedPlaceholder() { PropertyPlaceholder propertyPlaceholder = new PropertyPlaceholder("${", "}", true); Map map = new LinkedHashMap<>(); - PropertyPlaceholder.PlaceholderResolver placeholderResolver = new SimplePlaceholderResolver(map, false); + PropertyPlaceholder.PlaceholderResolver placeholderResolver = new SimplePlaceholderResolver(map, false, true); assertEquals("${foo}", propertyPlaceholder.replacePlaceholders("${foo}", placeholderResolver)); } @@ -75,7 +75,7 @@ public class PropertyPlaceholderTest extends ElasticsearchTestCase { public void testNotIgnoredUnresolvedPlaceholder() { PropertyPlaceholder propertyPlaceholder = new PropertyPlaceholder("${", "}", false); Map map = new LinkedHashMap<>(); - PropertyPlaceholder.PlaceholderResolver placeholderResolver = new SimplePlaceholderResolver(map, false); + PropertyPlaceholder.PlaceholderResolver placeholderResolver = new SimplePlaceholderResolver(map, false, true); propertyPlaceholder.replacePlaceholders("${foo}", placeholderResolver); } @@ -83,7 +83,7 @@ public class PropertyPlaceholderTest extends ElasticsearchTestCase { public void testShouldIgnoreMissing() { PropertyPlaceholder propertyPlaceholder = new PropertyPlaceholder("${", "}", false); Map map = new LinkedHashMap<>(); - PropertyPlaceholder.PlaceholderResolver placeholderResolver = new SimplePlaceholderResolver(map, true); + PropertyPlaceholder.PlaceholderResolver placeholderResolver = new SimplePlaceholderResolver(map, true, true); assertEquals("bar", propertyPlaceholder.replacePlaceholders("bar${foo}", placeholderResolver)); } @@ -94,7 +94,7 @@ public class PropertyPlaceholderTest extends ElasticsearchTestCase { map.put("foo", "${foo1}"); map.put("foo1", "${foo2}"); map.put("foo2", "bar"); - PropertyPlaceholder.PlaceholderResolver placeholderResolver = new SimplePlaceholderResolver(map, false); + PropertyPlaceholder.PlaceholderResolver placeholderResolver = new SimplePlaceholderResolver(map, false, true); assertEquals("bar", propertyPlaceholder.replacePlaceholders("${foo}", placeholderResolver)); assertEquals("abarb", propertyPlaceholder.replacePlaceholders("a${foo}b", placeholderResolver)); } @@ -107,7 +107,7 @@ public class PropertyPlaceholderTest extends ElasticsearchTestCase { map.put("foo1", "${foo2}"); map.put("foo2", "bar"); map.put("barbar", "baz"); - PropertyPlaceholder.PlaceholderResolver placeholderResolver = new SimplePlaceholderResolver(map, false); + PropertyPlaceholder.PlaceholderResolver placeholderResolver = new SimplePlaceholderResolver(map, false, true); assertEquals("baz", propertyPlaceholder.replacePlaceholders("${bar${foo}}", placeholderResolver)); } @@ -119,7 +119,7 @@ public class PropertyPlaceholderTest extends ElasticsearchTestCase { map.put("foo1", "{foo2}"); map.put("foo2", "bar"); map.put("barbar", "baz"); - PropertyPlaceholder.PlaceholderResolver placeholderResolver = new SimplePlaceholderResolver(map, false); + PropertyPlaceholder.PlaceholderResolver placeholderResolver = new SimplePlaceholderResolver(map, false, true); assertEquals("baz", propertyPlaceholder.replacePlaceholders("{bar{foo}}", placeholderResolver)); } @@ -131,7 +131,7 @@ public class PropertyPlaceholderTest extends ElasticsearchTestCase { map.put("foo1", "{foo2}}"); map.put("foo2", "bar"); map.put("barbar", "baz"); - PropertyPlaceholder.PlaceholderResolver placeholderResolver = new SimplePlaceholderResolver(map, false); + PropertyPlaceholder.PlaceholderResolver placeholderResolver = new SimplePlaceholderResolver(map, false, true); assertEquals("baz", propertyPlaceholder.replacePlaceholders("{bar{foo}}}}", placeholderResolver)); } @@ -141,17 +141,27 @@ public class PropertyPlaceholderTest extends ElasticsearchTestCase { Map map = new LinkedHashMap<>(); map.put("foo", "${bar}"); map.put("bar", "${foo}"); - PropertyPlaceholder.PlaceholderResolver placeholderResolver = new SimplePlaceholderResolver(map, false); + PropertyPlaceholder.PlaceholderResolver placeholderResolver = new SimplePlaceholderResolver(map, false, true); propertyPlaceholder.replacePlaceholders("${foo}", placeholderResolver); } + @Test + public void testShouldRemoveMissing() { + PropertyPlaceholder propertyPlaceholder = new PropertyPlaceholder("${", "}", false); + Map map = new LinkedHashMap<>(); + PropertyPlaceholder.PlaceholderResolver placeholderResolver = new SimplePlaceholderResolver(map, true, false); + assertEquals("bar${foo}", propertyPlaceholder.replacePlaceholders("bar${foo}", placeholderResolver)); + } + private class SimplePlaceholderResolver implements PropertyPlaceholder.PlaceholderResolver { private Map map; private boolean shouldIgnoreMissing; + private boolean shouldRemoveMissing; - SimplePlaceholderResolver(Map map, boolean shouldIgnoreMissing) { + SimplePlaceholderResolver(Map map, boolean shouldIgnoreMissing, boolean shouldRemoveMissing) { this.map = map; this.shouldIgnoreMissing = shouldIgnoreMissing; + this.shouldRemoveMissing = shouldRemoveMissing; } @Override @@ -163,5 +173,10 @@ public class PropertyPlaceholderTest extends ElasticsearchTestCase { public boolean shouldIgnoreMissing(String placeholderName) { return shouldIgnoreMissing; } + + @Override + public boolean shouldRemoveMissingPlaceholder(String placeholderName) { + return shouldRemoveMissing; + } } } 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 1dbaf32f06c..cf76a01ee0b 100644 --- a/core/src/test/java/org/elasticsearch/common/settings/SettingsTests.java +++ b/core/src/test/java/org/elasticsearch/common/settings/SettingsTests.java @@ -138,14 +138,14 @@ public class SettingsTests extends ElasticsearchTestCase { } @Test - public void testReplacePropertiesPlaceholderIgnores() { + public void testReplacePropertiesPlaceholderIgnoresPrompt() { Settings settings = settingsBuilder() - .put("setting1", "${foo.bar}") - .put("setting2", "${foo.bar1}") - .replacePropertyPlaceholders("${foo.bar}", "${foo.bar1}") + .put("setting1", "${prompt.text}") + .put("setting2", "${prompt.secret}") + .replacePropertyPlaceholders() .build(); - assertThat(settings.get("setting1"), is("${foo.bar}")); - assertThat(settings.get("setting2"), is("${foo.bar1}")); + assertThat(settings.get("setting1"), is("${prompt.text}")); + assertThat(settings.get("setting2"), is("${prompt.secret}")); } @Test diff --git a/docs/reference/setup/configuration.asciidoc b/docs/reference/setup/configuration.asciidoc index 2fb985b39f3..5e43e9daf01 100644 --- a/docs/reference/setup/configuration.asciidoc +++ b/docs/reference/setup/configuration.asciidoc @@ -271,15 +271,15 @@ file which will resolve to an environment setting, for example: -------------------------------------------------- Additionally, for settings that you do not wish to store in the configuration -file, you can use the value `${prompt::text}` or `${prompt::secret}` and start -Elasticsearch in the foreground. `${prompt::secret}` has echoing disabled so -that the value entered will not be shown in your terminal; `${prompt::text}` +file, you can use the value `${prompt.text}` or `${prompt.secret}` and start +Elasticsearch in the foreground. `${prompt.secret}` has echoing disabled so +that the value entered will not be shown in your terminal; `${prompt.text}` will allow you to see the value as you type it in. For example: [source,yaml] -------------------------------------------------- node: - name: ${prompt::text} + name: ${prompt.text} -------------------------------------------------- On execution of the `elasticsearch` command, you will be prompted to enter @@ -290,7 +290,7 @@ the actual value like so: Enter value for [node.name]: -------------------------------------------------- -NOTE: Elasticsearch will not start if `${prompt::text}` or `${prompt::secret}` +NOTE: Elasticsearch will not start if `${prompt.text}` or `${prompt.secret}` is used in the settings and the process is run as a service or in the background. The location of the configuration file can be set externally using a