Fix environment variable substitutions in list setting #28106

mayya-sharipova · 2018-01-05T17:31:51Z

Since Elasticsearch 6.1.0 environment variable substitutions in lists do not work
This commit fixes it.

Since Elasticsearch 6.1.0 environment variable substitutions in lists do not work This commit fixes it. Closes elastic#27926

jasontedor

Thanks @mayya-sharipova; the changes to production code look great I left a bunch of suggestions.

jasontedor · 2018-01-08T19:14:53Z

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

+        Settings settings = Settings.builder()
+            .put("property.placeholder", value)
+            .putList("setting1", "${property.placeholder}", "${property.placeholder}")
+            .replacePropertyPlaceholders()


We have replacePropertyPlaceholders(Function<String, String>); we use this rather than pulling from the same settings object as I think this will be a more realistic test?

jasontedor · 2018-01-08T19:15:03Z

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

+                if (entry.getValue() instanceof List) {
+                    List<String> ls = (List<String>) entry.getValue();
+                    ListIterator<String> li = ls.listIterator();
+                    while(li.hasNext()){


Nit: spacing between while and (.

jasontedor · 2018-01-08T19:15:17Z

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

+                    while(li.hasNext()){
+                        String value = li.next();
+                        String value2 = propertyPlaceholder.replacePlaceholders(value, placeholderResolver);
+                        if (! value.equals(value2)){


Nit: spacing between ! and value.

jasontedor · 2018-01-08T19:15:32Z

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

+                    while(li.hasNext()){
+                        String value = li.next();
+                        String value2 = propertyPlaceholder.replacePlaceholders(value, placeholderResolver);
+                        if (! value.equals(value2)){


Can we use a clearer name than value2?

jasontedor · 2018-01-08T19:15:56Z

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

                    // a null value obviously can't be replaced
                    continue;
                }
+                if (entry.getValue() instanceof List) {
+                    List<String> ls = (List<String>) entry.getValue();


ls is never used again after the next line; do we need this local variable?

jasontedor · 2018-01-08T19:16:05Z

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

                    // a null value obviously can't be replaced
                    continue;
                }
+                if (entry.getValue() instanceof List) {
+                    List<String> ls = (List<String>) entry.getValue();
+                    ListIterator<String> li = ls.listIterator();


Can this be a local final variable?

+1 for final. I am not very clear what you meant by local ? Do you recommend to declare li before the while loop on the line 1213?

Sorry for not being clear. What I mean is that these variables (which are local variables, they only have scope of the current) block I think would clearer if they were final because they I know by the declaration they are never mutated instead of having to keep checking the code "is this the same object as when the variable was first declared?".

jasontedor · 2018-01-08T19:16:20Z

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

+                    List<String> ls = (List<String>) entry.getValue();
+                    ListIterator<String> li = ls.listIterator();
+                    while(li.hasNext()){
+                        String value = li.next();


Can we use a clearer name than value?

jasontedor · 2018-01-08T19:16:54Z

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

+            .putList("setting1", "${property.placeholder}", "${property.placeholder}")
+            .replacePropertyPlaceholders()
+            .build();
+        assertThat(settings.getAsList("setting1"), contains(value, value));


Can we test replacing different placeholders?

jasontedor · 2018-01-08T19:21:48Z

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());


I don't think this will be needed if you take the suggestion below, but in general assertFalse and assertTrue should be avoided because they do not give good error messages (a failing expectation would only say AssertionError). Here, we could use assertThat(value, not(isEmptyOrNullString()) as then the expectation would say

Expected: not (null or an empty string) but: was ""

which is a lot more helpful!

thank you, this is very helpful to know

jasontedor

Thanks @mayya-sharipova; looks good. I left a few more comments.

jasontedor · 2018-01-09T22:10:55Z

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

-            .replacePropertyPlaceholders()
+        final String hostname = randomAlphaOfLength(16);
+        final Settings settings = Settings.builder()
+            .putList("setting1", "${HOSTNAME}", "${HOSTNAME}")


Can we use two different placeholders here to test that that works?

jasontedor · 2018-01-09T22:11:17Z

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

+                    while (li.hasNext()){
+                        final String settingValueRaw = li.next();
+                        final String settingValueResolved = propertyPlaceholder.replacePlaceholders(settingValueRaw, placeholderResolver);
+                        if (!settingValueRaw.equals(settingValueResolved)){


Nit: space after )) so )) }

jasontedor · 2018-01-09T22:11:35Z

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

-                        if (! value.equals(value2)){
-                            li.set(value2);
+                    final ListIterator<String> li = ((List<String>) entry.getValue()).listIterator();
+                    while (li.hasNext()){


Nit: space after )) so )) }

jasontedor · 2018-01-09T22:12:10Z

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

+                    while (li.hasNext()){
+                        final String settingValueRaw = li.next();
+                        final String settingValueResolved = propertyPlaceholder.replacePlaceholders(settingValueRaw, placeholderResolver);
+                        if (!settingValueRaw.equals(settingValueResolved)){


I wonder if it's simpler to do away with this if? If they are equal, it's okay to do the set?

jasontedor

LGTM. However, I think the commit message could be more thorough (right now it basically says: "something was broken, this fixes it"). When you merge, can you write a more thorough commit message please?

mayya-sharipova · 2018-01-10T17:03:18Z

@jasontedor thanks a lot for comprehensive review, I learned useful stuff.

Just to confirm, I intend to backport this commit to 6.1 and 6.x

jasontedor · 2018-01-10T17:21:51Z

It is not clear if we will do a 6.1 release that has this fix but please backport to both 6.1 (in case), and 6.x. The label on 6.1 should be 6.1.3.

jasontedor · 2018-01-11T01:14:07Z

@mayya-sharipova If this is backported, can you remove the backport-pending label. Separately, can you fix the 6.1 label?

Since Elasticsearch 6.1.0 environment variable substitutions in lists do not work. Environment variables in a list setting were not resolved, because settings with a list type were skipped during variables resolution. This commit fixes by processing list settings as well. Closes #27926

* master: (43 commits) Rename core module to server (#28180) upgraded jna from 4.4.0-1 to 4.5.1 (#28183) [TEST] Do not call RandomizedTest.scaledRandomIntBetween from multiple threads Primary send safe commit in file-based recovery (#28038) [Docs] Correct response json in rank-eval.asciidoc Add scroll parameter to _reindex API (#28041) Include all sentences smaller than fragment_size in the unified highlighter (#28132) Modifies the JavaAPI docs related to AggregationBuilder [Docs] Improvements in script-fields.asciidoc (#28174) [Docs] Remove Kerberos/SPNEGO Shield plugin (#28019) Ignore null value for range field (#27845) (#28116) Fix environment variable substitutions in list setting (#28106) docs: Replaces indexed script java api docs with stored script api docs test: ensure we endup with a single segment Make sure that we don't detect files as maven coordinate when installing a plugin (#28163) [Tests] temporary disable meta plugin rest tests #28163 meta-plugin should install bin and config at the top level (#28162) Painless: Add public member read/write access test. (#28156) Docs: Clarify password protection support with keystore (#28157) [Docs] fix plugin properties inclusion for plugins authors ...

* 6.x: (41 commits) Rename core module to server (#28190) [Test] Remove superfluous lines [Test] Add skip test for index range field with null values upgraded jna from 4.4.0-1 to 4.5.1 (#28183) [TEST] Do not call RandomizedTest.scaledRandomIntBetween from multiple threads [Docs] Correct response json in rank-eval.asciidoc Fix environment variable substitutions in list setting (#28106) Add scroll parameter to _reindex API (#28041) Include all sentences smaller than fragment_size in the unified highlighter (#28132) [Docs] Improvements in script-fields.asciidoc (#28174) [Docs] Remove Kerberos/SPNEGO Shield plugin (#28019) Ignore null value for range field (#27845) (#28116) docs: Replaces indexed script java api docs with stored script api docs test: ensure we endup with a single segment Make sure that we don't detect files as maven coordinate when installing a plugin (#28163) [Tests] temporary disable meta plugin rest tests #28163 meta-plugin should install bin and config at the top level (#28162) Painless: Add public member read/write access test. (#28156) Docs: Clarify password protection support with keystore (#28157) [Docs] fix plugin properties inclusion for plugins authors ...

* master: (30 commits) Fix lock accounting in releasable lock Add ability to associate an ID with tasks (elastic#27764) [DOCS] Removed differencies between text and code (elastic#27993) text fixes (elastic#28136) Update getting-started.asciidoc (elastic#28145) [Docs] Spelling fix in painless-getting-started.asciidoc (elastic#28187) Fixed the cat.health REST test to accept 4ms, not just 4.0ms (elastic#28186) Do not keep 5.x commits once having 6.x commits (elastic#28188) Rename core module to server (elastic#28180) upgraded jna from 4.4.0-1 to 4.5.1 (elastic#28183) [TEST] Do not call RandomizedTest.scaledRandomIntBetween from multiple threads Primary send safe commit in file-based recovery (elastic#28038) [Docs] Correct response json in rank-eval.asciidoc Add scroll parameter to _reindex API (elastic#28041) Include all sentences smaller than fragment_size in the unified highlighter (elastic#28132) Modifies the JavaAPI docs related to AggregationBuilder [Docs] Improvements in script-fields.asciidoc (elastic#28174) [Docs] Remove Kerberos/SPNEGO Shield plugin (elastic#28019) Ignore null value for range field (elastic#27845) (elastic#28116) Fix environment variable substitutions in list setting (elastic#28106) ...

Fix environment variable substitutions in list setting

41ce42e

Since Elasticsearch 6.1.0 environment variable substitutions in lists do not work This commit fixes it. Closes elastic#27926

mayya-sharipova added v6.1.0 v7.0.0 :Core/Infra/Settings Settings infrastructure and APIs backport pending labels Jan 5, 2018

jasontedor reviewed Jan 8, 2018

View reviewed changes

Correcting code

72c2bc3

jasontedor reviewed Jan 9, 2018

View reviewed changes

Correcting code

3a697fd

mayya-sharipova added the v6.2.0 label Jan 10, 2018

jasontedor approved these changes Jan 10, 2018

View reviewed changes

mayya-sharipova merged commit caa63ad into elastic:master Jan 10, 2018

mayya-sharipova added v6.1.3 and removed v6.1.0 labels Jan 11, 2018

mayya-sharipova removed the backport pending label Jan 11, 2018

colings86 added the >bug label Jan 22, 2018

jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix environment variable substitutions in list setting #28106

Fix environment variable substitutions in list setting #28106

mayya-sharipova commented Jan 5, 2018

jasontedor left a comment

jasontedor Jan 8, 2018

jasontedor Jan 8, 2018

jasontedor Jan 8, 2018

jasontedor Jan 8, 2018

jasontedor Jan 8, 2018

jasontedor Jan 8, 2018

mayya-sharipova Jan 8, 2018 •

edited

Loading

jasontedor Jan 8, 2018

jasontedor Jan 8, 2018

jasontedor Jan 8, 2018

jasontedor Jan 8, 2018

mayya-sharipova Jan 8, 2018

jasontedor left a comment

jasontedor Jan 9, 2018

jasontedor Jan 9, 2018

jasontedor Jan 9, 2018

jasontedor Jan 9, 2018

jasontedor left a comment

mayya-sharipova commented Jan 10, 2018

jasontedor commented Jan 10, 2018

jasontedor commented Jan 11, 2018

Fix environment variable substitutions in list setting #28106

Fix environment variable substitutions in list setting #28106

Conversation

mayya-sharipova commented Jan 5, 2018

jasontedor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayya-sharipova Jan 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasontedor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasontedor left a comment

Choose a reason for hiding this comment

mayya-sharipova commented Jan 10, 2018

jasontedor commented Jan 10, 2018

jasontedor commented Jan 11, 2018

mayya-sharipova Jan 8, 2018 •

edited

Loading