CORE: Validate Type for String Settings #33503

original-brownbear · 2018-09-07T13:21:20Z

Special handling for String in the generic Setting class (I think this is justified though a little dirty because we parse raw values String values for all types ... I don't see how else we can do this without changing the APIs either):
- If we parse out a String type setting, validate that it came from a scalar raw value
Closes String settings leniently accept lists #33135

* Special handling for `String` in the generic Setting class because we parse raw values String values for all types: * If we parse out a `String` setting validate that it came from a scalar raw value * Closes elastic#33135

elasticmachine · 2018-09-07T13:21:21Z

Pinging @elastic/es-core-infra

rjernst

Instead of special casing this inside Setting, I think the root problem is Settings.toString used by Settings.get serializes whatever the value is to a String no matter the type. The same Map/List logic could be moved to toString to not allow any implicit conversion of object/array to String?

rjernst · 2018-09-08T20:00:20Z

Actually I wonder if toString should only work on Numeric objects? Maybe that or the value already being a String are ok, and anything else should be an error?

original-brownbear · 2018-09-08T20:09:49Z

@rjernst the problem is that we simply always expect a string here for the parsers:

e.g. all list type settings will use this (and similar) :

    public static <T> Setting<List<T>> listSetting(String key, Function<Settings, List<String>> defaultStringValue,
                                                   Function<String, T> singleValueParser, Property... properties) {
        if (defaultStringValue.apply(Settings.EMPTY) == null) {
            throw new IllegalArgumentException("default value function must not return null");
        }
        Function<String, List<T>> parser = (s) ->
                parseableStringToList(s).stream().map(singleValueParser).collect(Collectors.toList());

        return new ListSetting<>(key, defaultStringValue, parser, properties);
    }

=> If we parse an actual list out of a YAML then we currently have to serialize it to String before hitting the parser (in the method I added the change to).
=> Since we're always parsing all kinds of settings out of a string API wise at the moment + don't have have a reference to the type of the setting in the generic method I don't see how we can even add any logic based on output type that would act before that toString call. This is precisely why I first parse and then check the type of what I parsed.

=> This is really dirty :( This is the best fix I came up with that doesn't require a big change though. I could think about a bigger but cleaner change if we're willing to live with more noise here?

original-brownbear · 2018-09-08T20:12:13Z

... though any change that gives us the type inside of the setting get method and before parsing would literally mean changing 100% of Setting<T> instantiations because we'd have to pass in the class of the result there ... seems a little crazy? :)

rjernst · 2018-09-08T21:04:31Z

Ok, I understand the problem. Though, I think this hack is just leading us further down a rabbit hole of the current problem, which is that we assume all values are strings. We did already change the concrete type for lists to be a List instead of String (see #26878), but the concrete Setting parser functions all take String. Perhaps they should take Object? Then getRaw can return object instead of String, and each setting parser can decide whether to call toString() on the object (or have a helper method since it is the common case, and that helper method can error when the type is not convertible to String like with Map or List).
/cc @s1monw @jasontedor

original-brownbear · 2018-09-08T21:10:01Z

@rjernst

Perhaps they should take Object?

Just that would probably solve the issue best I think (with the slightly annoying caveat that we'd maybe have to worry about mutability of that Object within the parser when it comes to List<?> and such ... especially when that list can itself contain more mutable things ... but I guess we have some experience in cleverly wrapping that kind of structure now :P ). That's where the String serialization round-trip was kind of handy :)

=> I think either we change all the parsers (big change but nicer :) ... apart from the mutability thing) or we pass the return type class to Setting<T> (smaller change but not as nice).

original-brownbear · 2018-09-10T15:57:14Z

@jasontedor adjusted PR to reflect what we discussed just now :) Take a look if this is what you were looking for, from my end it's a fine solution to #33135.

jasontedor · 2018-09-10T15:59:02Z

I discussed an approach with @original-brownbear where we identity which settings are list settings; we think this will give us a path forward here and also let us fix upgrading of list settings. We can discuss longer-term fixes in the future, but we need an approach to unblock some in-flight work for now.

jasontedor

I left a couple of comments.

jasontedor · 2018-09-10T16:02:31Z

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

+                }
+            } else if (isList) {
+                throw new IllegalArgumentException(
+                    "Expected list type value for setting [" + setting + "] but found [" + value.getClass() + ']'


I think that will fail compilation? 😇

Compiles just fine for me? :) What am I missing?

Oh, never mind, I misread. Sorry for that. 😄

jasontedor · 2018-09-10T16:02:42Z

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

+     */
+    public String get(String setting, String defaultValue, boolean isList) {
+        Object value = settings.get(setting);
+        if(value != null) {


Nit: if( -> if ( (add space)

original-brownbear · 2018-09-10T16:07:16Z

@jasontedor Missing space added :)

jasontedor · 2018-09-10T16:26:07Z

With this change and:

diff --git a/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java b/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java
index e25d954aa4f..aee85b6ff57 100644
--- a/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java
+++ b/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java
@@ -54,7 +54,7 @@ public abstract class AbstractScopedSettings extends AbstractComponent {
     private final List<SettingUpdater<?>> settingUpdaters = new CopyOnWriteArrayList<>();
     private final Map<String, Setting<?>> complexMatchers;
     private final Map<String, Setting<?>> keySettings;
-    private final Map<Setting<?>, Function<Map.Entry<String, String>, Map.Entry<String, String>>> settingUpgraders;
+    private final Map<Setting<?>, SettingUpgrader<?>> settingUpgraders;
     private final Setting.Property scope;
     private static final Pattern KEY_PATTERN = Pattern.compile("^(?:[-\\w]+[.])*[-\\w]+$");
     private static final Pattern GROUP_KEY_PATTERN = Pattern.compile("^(?:[-\\w]+[.])+$");
@@ -70,12 +70,8 @@ public abstract class AbstractScopedSettings extends AbstractComponent {
 
         this.settingUpgraders =
                 Collections.unmodifiableMap(
-                        settingUpgraders
-                                .stream()
-                                .collect(
-                                        Collectors.toMap(
-                                                SettingUpgrader::getSetting,
-                                                u -> e -> new AbstractMap.SimpleEntry<>(u.getKey(e.getKey()), u.getValue(e.getValue())))));
+                        settingUpgraders.stream().collect(Collectors.toMap(SettingUpgrader::getSetting, Function.identity())));
+
 
         this.scope = scope;
         Map<String, Setting<?>> complexMatchers = new HashMap<>();
@@ -786,15 +782,24 @@ public abstract class AbstractScopedSettings extends AbstractComponent {
         boolean changed = false; // track if any settings were upgraded
         for (final String key : settings.keySet()) {
             final Setting<?> setting = getRaw(key);
-            final Function<Map.Entry<String, String>, Map.Entry<String, String>> upgrader = settingUpgraders.get(setting);
+            final SettingUpgrader<?> upgrader = settingUpgraders.get(setting);
             if (upgrader == null) {
                 // the setting does not have an upgrader, copy the setting
                 builder.copy(key, settings);
             } else {
                 // the setting has an upgrader, so mark that we have changed a setting and apply the upgrade logic
                 changed = true;
-                final Map.Entry<String, String> upgrade = upgrader.apply(new Entry(key, settings));
-                builder.put(upgrade.getKey(), upgrade.getValue());
+                if (setting.isListSetting()) {
+                    final List<String> value = settings.getAsList(key);
+                    final String upgradedKey = upgrader.getKey(key);
+                    final List<String> upgradedValue = upgrader.getListValue(value);
+                    builder.putList(upgradedKey, upgradedValue);
+                } else {
+                    final String value = settings.get(key);
+                    final String upgradedKey = upgrader.getKey(key);
+                    final String upgradedValue = upgrader.getValue(value);
+                    builder.put(upgradedKey, upgradedValue);
+                }
             }
         }
         // we only return a new instance if there was an upgrade
diff --git a/server/src/main/java/org/elasticsearch/common/settings/SettingUpgrader.java b/server/src/main/java/org/elasticsearch/common/settings/SettingUpgrader.java
index 91f2bead300..bc41b554905 100644
--- a/server/src/main/java/org/elasticsearch/common/settings/SettingUpgrader.java
+++ b/server/src/main/java/org/elasticsearch/common/settings/SettingUpgrader.java
@@ -19,6 +19,8 @@
 
 package org.elasticsearch.common.settings;
 
+import java.util.List;
+
 /**
  * Represents the logic to upgrade a setting.
  *
@@ -51,4 +53,8 @@ public interface SettingUpgrader<T> {
         return value;
     }
 
+    default List<String> getListValue(final List<String> value) {
+        return value;
+    }
+
 }
diff --git a/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java b/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java
index 0ee1d2e9c4a..6766316fafd 100644
--- a/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java
+++ b/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java
@@ -47,6 +47,7 @@ import java.util.concurrent.atomic.AtomicReference;
 import java.util.function.BiConsumer;
 import java.util.function.Consumer;
 import java.util.function.Function;
+import java.util.stream.Collectors;
 
 import static org.hamcrest.CoreMatchers.containsString;
 import static org.hamcrest.CoreMatchers.equalTo;
@@ -1171,4 +1172,47 @@ public class ScopedSettingsTests extends ESTestCase {
         }
     }
 
+    public void testUpgradeListSetting() {
+        final Setting<List<String>> oldSetting =
+                Setting.listSetting("foo.old", Collections.emptyList(), Function.identity(), Property.NodeScope);
+        final Setting<List<String>> newSetting =
+                Setting.listSetting("foo.new", Collections.emptyList(), Function.identity(), Property.NodeScope);
+
+        final AbstractScopedSettings service =
+                new ClusterSettings(
+                        Settings.EMPTY,
+                        new HashSet<>(Arrays.asList(oldSetting, newSetting)),
+                        Collections.singleton(new SettingUpgrader<List<String>>() {
+
+                            @Override
+                            public Setting<List<String>> getSetting() {
+                                return oldSetting;
+                            }
+
+                            @Override
+                            public String getKey(final String key) {
+                                return "foo.new";
+                            }
+
+                            @Override
+                            public List<String> getListValue(final List<String> value) {
+                                return value.stream().map(s -> "new." + s).collect(Collectors.toList());
+                            }
+                        }));
+
+        final int length = randomIntBetween(0, 16);
+        final List<String> values = length == 0 ? Collections.emptyList() : new ArrayList<>(length);
+        for (int i = 0; i < length; i++) {
+            values.add(randomAlphaOfLength(8));
+        }
+
+        final Settings settings = Settings.builder().putList("foo.old", values).build();
+        final Settings upgradedSettings = service.upgradeSettings(settings);
+        assertFalse(oldSetting.exists(upgradedSettings));
+        assertTrue(newSetting.exists(upgradedSettings));
+        assertThat(
+                newSetting.get(upgradedSettings),
+                equalTo(oldSetting.get(settings).stream().map(s -> "new." + s).collect(Collectors.toList())));
+    }
+
 }

I can also get the upgrade settings case to work too.

jasontedor

I am good with this change, but let us see what @rjernst thinks too.

rjernst

Thanks, this looks much better. Two minor suggestions.

rjernst · 2018-09-10T18:31:24Z

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

+     * Returns the setting value associated with the setting key. If it does not exists,
+     * returns the default value provided.
+     */
+    public String get(String setting, String defaultValue, boolean isList) {


can this be package private?

yea I think so will adjust in a bit :)

rjernst · 2018-09-10T18:32:11Z

server/src/main/java/org/elasticsearch/common/settings/Setting.java

@@ -453,7 +458,7 @@ public final String getRaw(final Settings settings) {
     * @return the raw string representation of the setting value
     */
    String innerGetRaw(final Settings settings) {
-        return settings.get(getKey(), defaultValue.apply(settings));
+        return settings.get(getKey(), defaultValue.apply(settings), isListSetting());


Instead of having an isListSetting, since this is already package private anyways, can this just check instanceof ListSetting?

Hmm, yea it could :) -> will change to that :)

@original-brownbear @rjernst See my diff, we need to expose Setting#isListSetting in AbstractScopedSettings.

Its implementation though can be final and be an instance of check against ListSetting.

original-brownbear · 2018-09-10T18:51:01Z

Adjusted as discussed, will merge once green :)

jasontedor · 2018-09-10T19:02:07Z

Thanks @original-brownbear.

jasontedor · 2018-09-10T23:28:20Z

@original-brownbear I merged this so I can open a PR on top of it. Can you handle backporting this to 6.x?

When we see a settings value, it could be a list. Yet this should only happen if the underlying setting type is a list setting type. This commit adds validation that when we get a setting value that is a list, that the setting that we are getting is a list setting. And similarly, if we get a value for a list setting, the underlying value should be a list.

jasontedor · 2018-09-11T00:36:17Z

@original-brownbear I ended up backporting this in e084a0a.

* master: Add full cluster restart base class (elastic#33577) Validate list values for settings (elastic#33503) Copy and validatie soft-deletes setting on resize (elastic#33517) Test: Fix package name SQL: Fix result column names for arithmetic functions (elastic#33500) Upgrade to latest Lucene snapshot (elastic#33505) Enable not wiping cluster settings after REST test (elastic#33575) MINOR: Remove Dead Code in SearchScript (elastic#33569) [Test] Remove duplicate method in TestShardRouting (elastic#32815) mute test on windows Update beats template to include apm-server metrics (elastic#33286) Fix typos (elastic#33499) [CCR] Delay auto follow license check (elastic#33557) [CCR] Add create_follow_index privilege (elastic#33559) Strengthen FilterRoutingTests (elastic#33149) Correctly handle PKCS#11 tokens for system keystore (elastic#33460) Remove some duplicate request conversion methods. (elastic#33538)

original-brownbear · 2018-09-11T07:27:44Z

@jasontedor thanks!

* master: (91 commits) Preserve cluster settings on full restart tests (elastic#33590) Use IndexWriter.getFlushingBytes() rather than tracking it ourselves (elastic#33582) Fix upgrading of list settings (elastic#33589) Add read-only Engine (elastic#33563) HLRC: Add ML get categories API (elastic#33465) SQL: Adds MONTHNAME, DAYNAME and QUARTER functions (elastic#33411) Add predicate_token_filter (elastic#33431) Fix Replace function. Adds more tests to all string functions. (elastic#33478) [ML] Rename input_fields to column_names in file structure (elastic#33568) Add full cluster restart base class (elastic#33577) Validate list values for settings (elastic#33503) Copy and validatie soft-deletes setting on resize (elastic#33517) Test: Fix package name SQL: Fix result column names for arithmetic functions (elastic#33500) Upgrade to latest Lucene snapshot (elastic#33505) Enable not wiping cluster settings after REST test (elastic#33575) MINOR: Remove Dead Code in SearchScript (elastic#33569) [Test] Remove duplicate method in TestShardRouting (elastic#32815) mute test on windows Update beats template to include apm-server metrics (elastic#33286) ...

* Reverts setting type validation introduced in elastic#33503

* Reverts setting type validation introduced in #33503

* Reverts setting type validation introduced in elastic#33503

* Reverts setting type validation introduced in #33503

CORE: Validate Type for String Settings

b4b78dc

* Special handling for `String` in the generic Setting class because we parse raw values String values for all types: * If we parse out a `String` setting validate that it came from a scalar raw value * Closes elastic#33135

original-brownbear added >bug :Core/Infra/Settings Settings infrastructure and APIs v7.0.0 v6.5.0 labels Sep 7, 2018

original-brownbear requested review from jasontedor and rjernst September 7, 2018 15:16

original-brownbear added the review label Sep 8, 2018

rjernst reviewed Sep 8, 2018

View reviewed changes

Merge remote-tracking branch 'elastic/master' into 33135

9f3ab94

jasontedor mentioned this pull request Sep 10, 2018

Upgrade remote cluster settings #33537

Merged

Changed approach to providing isList value with Setting class

a3073cc

jasontedor reviewed Sep 10, 2018

View reviewed changes

CR: Add missing space

eda6e87

jasontedor approved these changes Sep 10, 2018

View reviewed changes

rjernst approved these changes Sep 10, 2018

View reviewed changes

CR: -public; move to isInstance check in final method

1ec003a

jasontedor merged commit 6075e15 into elastic:master Sep 10, 2018

jasontedor added the backport pending label Sep 10, 2018

jasontedor mentioned this pull request Sep 11, 2018

Fix upgrading of list settings #33589

Merged

jasontedor removed the backport pending label Sep 11, 2018

original-brownbear deleted the 33135 branch September 11, 2018 07:27

original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request Sep 13, 2018

CORE: Disable Setting Type Validation

822ef72

* Reverts setting type validation introduced in elastic#33503

original-brownbear mentioned this pull request Sep 13, 2018

CORE: Disable Setting Type Validation #33660

Merged

original-brownbear added a commit that referenced this pull request Sep 13, 2018

CORE: Disable Setting Type Validation (#33660)

928fe3f

* Reverts setting type validation introduced in #33503

original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request Sep 13, 2018

CORE: Disable Setting Type Validation (elastic#33660)

8f2590a

* Reverts setting type validation introduced in elastic#33503

original-brownbear mentioned this pull request Sep 13, 2018

CORE: Disable Setting Type Validation (#33660) #33669

Merged

original-brownbear added a commit that referenced this pull request Sep 13, 2018

CORE: Disable Setting Type Validation (#33660) (#33669)

040695b

* Reverts setting type validation introduced in #33503

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

gwbrown mentioned this pull request Dec 7, 2020

String settings leniently accept lists #33135

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CORE: Validate Type for String Settings #33503

CORE: Validate Type for String Settings #33503

original-brownbear commented Sep 7, 2018

elasticmachine commented Sep 7, 2018

rjernst left a comment

rjernst commented Sep 8, 2018

original-brownbear commented Sep 8, 2018

original-brownbear commented Sep 8, 2018

rjernst commented Sep 8, 2018

original-brownbear commented Sep 8, 2018 •

edited

Loading

original-brownbear commented Sep 10, 2018

jasontedor commented Sep 10, 2018

jasontedor left a comment

jasontedor Sep 10, 2018

original-brownbear Sep 10, 2018

jasontedor Sep 10, 2018

jasontedor Sep 10, 2018

original-brownbear commented Sep 10, 2018

jasontedor commented Sep 10, 2018

jasontedor left a comment

rjernst left a comment

rjernst Sep 10, 2018

original-brownbear Sep 10, 2018

jasontedor Sep 10, 2018

rjernst Sep 10, 2018

original-brownbear Sep 10, 2018

jasontedor Sep 10, 2018 •

edited

Loading

jasontedor Sep 10, 2018 •

edited

Loading

original-brownbear commented Sep 10, 2018

jasontedor commented Sep 10, 2018

jasontedor commented Sep 10, 2018

jasontedor commented Sep 11, 2018

original-brownbear commented Sep 11, 2018

CORE: Validate Type for String Settings #33503

CORE: Validate Type for String Settings #33503

Conversation

original-brownbear commented Sep 7, 2018

elasticmachine commented Sep 7, 2018

rjernst left a comment

Choose a reason for hiding this comment

rjernst commented Sep 8, 2018

original-brownbear commented Sep 8, 2018

original-brownbear commented Sep 8, 2018

rjernst commented Sep 8, 2018

original-brownbear commented Sep 8, 2018 • edited Loading

original-brownbear commented Sep 10, 2018

jasontedor commented Sep 10, 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

original-brownbear commented Sep 10, 2018

jasontedor commented Sep 10, 2018

jasontedor left a comment

Choose a reason for hiding this comment

rjernst 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

jasontedor Sep 10, 2018 • edited Loading

Choose a reason for hiding this comment

jasontedor Sep 10, 2018 • edited Loading

Choose a reason for hiding this comment

original-brownbear commented Sep 10, 2018

jasontedor commented Sep 10, 2018

jasontedor commented Sep 10, 2018

jasontedor commented Sep 11, 2018

original-brownbear commented Sep 11, 2018

original-brownbear commented Sep 8, 2018 •

edited

Loading

jasontedor Sep 10, 2018 •

edited

Loading

jasontedor Sep 10, 2018 •

edited

Loading