From 41ce42e3226a0bb7dfa889d17fa1b5ffe37c645d Mon Sep 17 00:00:00 2001 From: Mayya Sharipova Date: Fri, 5 Jan 2018 11:34:27 -0500 Subject: [PATCH 1/3] Fix environment variable substitutions in list setting Since Elasticsearch 6.1.0 environment variable substitutions in lists do not work This commit fixes it. Closes #27926 --- .../common/settings/Settings.java | 20 ++++++++++++++++--- .../common/settings/SettingsTests.java | 11 ++++++++++ 2 files changed, 28 insertions(+), 3 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 0a0a01c3fe39a..7b4e52017aef4 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/Settings.java +++ b/core/src/main/java/org/elasticsearch/common/settings/Settings.java @@ -64,6 +64,7 @@ import java.util.NoSuchElementException; import java.util.Set; import java.util.TreeMap; +import java.util.ListIterator; import java.util.concurrent.TimeUnit; import java.util.function.Function; import java.util.function.Predicate; @@ -414,7 +415,7 @@ public List getAsList(String key, List defaultValue, Boolean com final Object valueFromPrefix = settings.get(key); if (valueFromPrefix != null) { if (valueFromPrefix instanceof List) { - return ((List) valueFromPrefix); // it's already unmodifiable since the builder puts it as a such + return Collections.unmodifiableList((List) valueFromPrefix); } else if (commaDelimited) { String[] strings = Strings.splitStringByCommaToArray(get(key)); if (strings.length > 0) { @@ -1042,7 +1043,7 @@ public Builder putList(String setting, String... values) { */ public Builder putList(String setting, List values) { remove(setting); - map.put(setting, Collections.unmodifiableList(new ArrayList<>(values))); + map.put(setting, new ArrayList<>(values)); return this; } @@ -1210,10 +1211,23 @@ public boolean shouldRemoveMissingPlaceholder(String placeholderName) { Iterator> entryItr = map.entrySet().iterator(); while (entryItr.hasNext()) { Map.Entry entry = entryItr.next(); - if (entry.getValue() == null || entry.getValue() instanceof List) { + if (entry.getValue() == null) { // a null value obviously can't be replaced continue; } + if (entry.getValue() instanceof List) { + List ls = (List) entry.getValue(); + ListIterator li = ls.listIterator(); + while(li.hasNext()){ + String value = li.next(); + String value2 = propertyPlaceholder.replacePlaceholders(value, placeholderResolver); + if (! value.equals(value2)){ + li.set(value2); + } + } + continue; + } + String value = propertyPlaceholder.replacePlaceholders(Settings.toString(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 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 039de112fac36..de9b52c44d709 100644 --- a/core/src/test/java/org/elasticsearch/common/settings/SettingsTests.java +++ b/core/src/test/java/org/elasticsearch/common/settings/SettingsTests.java @@ -68,6 +68,17 @@ public void testReplacePropertiesPlaceholderSystemProperty() { assertThat(settings.get("setting1"), equalTo(value)); } + public void testReplacePropertiesPlaceholderSystemPropertyList() { + String value = System.getProperty("java.home"); + assertFalse(value.isEmpty()); + Settings settings = Settings.builder() + .put("property.placeholder", value) + .putList("setting1", "${property.placeholder}", "${property.placeholder}") + .replacePropertyPlaceholders() + .build(); + assertThat(settings.getAsList("setting1"), contains(value, value)); + } + public void testReplacePropertiesPlaceholderSystemVariablesHaveNoEffect() { final String value = System.getProperty("java.home"); assertNotNull(value); From 72c2bc304dbca7c91af45c5b070ea9147548af44 Mon Sep 17 00:00:00 2001 From: Mayya Sharipova Date: Tue, 9 Jan 2018 09:35:01 -0500 Subject: [PATCH 2/3] Correcting code --- .../org/elasticsearch/common/settings/Settings.java | 13 ++++++------- .../common/settings/SettingsTests.java | 12 +++++------- 2 files changed, 11 insertions(+), 14 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 7b4e52017aef4..be133ba34ad2f 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/Settings.java +++ b/core/src/main/java/org/elasticsearch/common/settings/Settings.java @@ -1216,13 +1216,12 @@ public boolean shouldRemoveMissingPlaceholder(String placeholderName) { continue; } if (entry.getValue() instanceof List) { - List ls = (List) entry.getValue(); - ListIterator li = ls.listIterator(); - while(li.hasNext()){ - String value = li.next(); - String value2 = propertyPlaceholder.replacePlaceholders(value, placeholderResolver); - if (! value.equals(value2)){ - li.set(value2); + final ListIterator li = ((List) entry.getValue()).listIterator(); + while (li.hasNext()){ + final String settingValueRaw = li.next(); + final String settingValueResolved = propertyPlaceholder.replacePlaceholders(settingValueRaw, placeholderResolver); + if (!settingValueRaw.equals(settingValueResolved)){ + li.set(settingValueResolved); } } continue; 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 de9b52c44d709..a9727b416c118 100644 --- a/core/src/test/java/org/elasticsearch/common/settings/SettingsTests.java +++ b/core/src/test/java/org/elasticsearch/common/settings/SettingsTests.java @@ -69,14 +69,12 @@ public void testReplacePropertiesPlaceholderSystemProperty() { } public void testReplacePropertiesPlaceholderSystemPropertyList() { - String value = System.getProperty("java.home"); - assertFalse(value.isEmpty()); - Settings settings = Settings.builder() - .put("property.placeholder", value) - .putList("setting1", "${property.placeholder}", "${property.placeholder}") - .replacePropertyPlaceholders() + final String hostname = randomAlphaOfLength(16); + final Settings settings = Settings.builder() + .putList("setting1", "${HOSTNAME}", "${HOSTNAME}") + .replacePropertyPlaceholders(name -> "HOSTNAME".equals(name) ? hostname : null) .build(); - assertThat(settings.getAsList("setting1"), contains(value, value)); + assertThat(settings.getAsList("setting1"), contains(hostname, hostname)); } public void testReplacePropertiesPlaceholderSystemVariablesHaveNoEffect() { From 3a697fd2af3f8fecc40e3c994939ca76fb411c12 Mon Sep 17 00:00:00 2001 From: Mayya Sharipova Date: Wed, 10 Jan 2018 08:54:46 -0500 Subject: [PATCH 3/3] Correcting code --- .../java/org/elasticsearch/common/settings/Settings.java | 6 ++---- .../org/elasticsearch/common/settings/SettingsTests.java | 7 ++++--- 2 files changed, 6 insertions(+), 7 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 be133ba34ad2f..c9a4c0f796b9f 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/Settings.java +++ b/core/src/main/java/org/elasticsearch/common/settings/Settings.java @@ -1217,12 +1217,10 @@ public boolean shouldRemoveMissingPlaceholder(String placeholderName) { } if (entry.getValue() instanceof List) { final ListIterator li = ((List) entry.getValue()).listIterator(); - while (li.hasNext()){ + while (li.hasNext()) { final String settingValueRaw = li.next(); final String settingValueResolved = propertyPlaceholder.replacePlaceholders(settingValueRaw, placeholderResolver); - if (!settingValueRaw.equals(settingValueResolved)){ - li.set(settingValueResolved); - } + li.set(settingValueResolved); } continue; } 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 a9727b416c118..52502acb61fe4 100644 --- a/core/src/test/java/org/elasticsearch/common/settings/SettingsTests.java +++ b/core/src/test/java/org/elasticsearch/common/settings/SettingsTests.java @@ -70,11 +70,12 @@ public void testReplacePropertiesPlaceholderSystemProperty() { public void testReplacePropertiesPlaceholderSystemPropertyList() { final String hostname = randomAlphaOfLength(16); + final String hostip = randomAlphaOfLength(16); final Settings settings = Settings.builder() - .putList("setting1", "${HOSTNAME}", "${HOSTNAME}") - .replacePropertyPlaceholders(name -> "HOSTNAME".equals(name) ? hostname : null) + .putList("setting1", "${HOSTNAME}", "${HOSTIP}") + .replacePropertyPlaceholders(name -> name.equals("HOSTNAME") ? hostname : name.equals("HOSTIP") ? hostip : null) .build(); - assertThat(settings.getAsList("setting1"), contains(hostname, hostname)); + assertThat(settings.getAsList("setting1"), contains(hostname, hostip)); } public void testReplacePropertiesPlaceholderSystemVariablesHaveNoEffect() {