From b4b78dcc72cc3db60485ab6ca18236dfc6e82aae Mon Sep 17 00:00:00 2001 From: Armin Braun Date: Fri, 7 Sep 2018 15:15:50 +0200 Subject: [PATCH 1/4] CORE: Validate Type for String Settings * 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 #33135 --- .../org/elasticsearch/common/settings/Setting.java | 11 +++++++++++ .../org/elasticsearch/common/settings/Settings.java | 5 +++++ .../elasticsearch/common/settings/SettingTests.java | 7 +++++++ 3 files changed, 23 insertions(+) diff --git a/server/src/main/java/org/elasticsearch/common/settings/Setting.java b/server/src/main/java/org/elasticsearch/common/settings/Setting.java index eabf2ef498406..d432dbc3f2130 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/Setting.java +++ b/server/src/main/java/org/elasticsearch/common/settings/Setting.java @@ -19,6 +19,7 @@ package org.elasticsearch.common.settings; +import java.util.Collection; import org.apache.logging.log4j.Logger; import org.elasticsearch.ElasticsearchException; import org.elasticsearch.ElasticsearchParseException; @@ -385,6 +386,16 @@ private T get(Settings settings, boolean validate) { try { T parsed = parser.apply(value); if (validate) { + if (parsed instanceof String) { + Class rawValueClass = settings.getRawType(getKey()); + if (rawValueClass != null + && (Map.class.isAssignableFrom(rawValueClass) || Collection.class.isAssignableFrom(rawValueClass)) + ) { + throw new IllegalArgumentException( + "Failed to parse [" + value + "] for string setting [" + getKey() + "] because it is not scalar." + ); + } + } final Iterator> it = validator.settings(); final Map, T> map; if (it.hasNext()) { diff --git a/server/src/main/java/org/elasticsearch/common/settings/Settings.java b/server/src/main/java/org/elasticsearch/common/settings/Settings.java index 2eb14f7ac6592..d4595bcc1b8a2 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/Settings.java +++ b/server/src/main/java/org/elasticsearch/common/settings/Settings.java @@ -245,6 +245,11 @@ public String get(String setting, String defaultValue) { return retVal == null ? defaultValue : retVal; } + Class getRawType(String setting) { + Object value = settings.get(setting); + return value == null ? null : value.getClass(); + } + /** * Returns the setting value (as float) associated with the setting key. If it does not exists, * returns the default value provided. diff --git a/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java b/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java index d82b620660249..862703a260ce7 100644 --- a/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java +++ b/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java @@ -180,6 +180,13 @@ public void testSimpleUpdate() { } } + public void testValidateStringSetting() { + Settings settings = Settings.builder().putList("foo.bar", Arrays.asList("bla-a", "bla-b")).build(); + Setting stringSetting = Setting.simpleString("foo.bar", Property.NodeScope); + IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> stringSetting.get(settings)); + assertEquals("Failed to parse [[bla-a, bla-b]] for string setting [foo.bar] because it is not scalar.", e.getMessage()); + } + private static final Setting FOO_BAR_SETTING = new Setting<>( "foo.bar", "foobar", From a3073cc6bad01f086dacb5d822b2c359a4ce90af Mon Sep 17 00:00:00 2001 From: Armin Braun Date: Mon, 10 Sep 2018 17:54:58 +0200 Subject: [PATCH 2/4] Changed approach to providing isList value with Setting class --- .../common/settings/Setting.java | 23 +++++++++---------- .../common/settings/Settings.java | 23 +++++++++++++++++-- .../common/settings/SettingTests.java | 2 +- 3 files changed, 33 insertions(+), 15 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/settings/Setting.java b/server/src/main/java/org/elasticsearch/common/settings/Setting.java index cd5d5b88f9b69..d67b4a23336b0 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/Setting.java +++ b/server/src/main/java/org/elasticsearch/common/settings/Setting.java @@ -19,7 +19,6 @@ package org.elasticsearch.common.settings; -import java.util.Collection; import org.apache.logging.log4j.Logger; import org.elasticsearch.ElasticsearchException; import org.elasticsearch.ElasticsearchParseException; @@ -346,6 +345,11 @@ boolean isGroupSetting() { return false; } + + boolean isListSetting() { + return false; + } + boolean hasComplexMatcher() { return isGroupSetting(); } @@ -399,16 +403,6 @@ private T get(Settings settings, boolean validate) { try { T parsed = parser.apply(value); if (validate) { - if (parsed instanceof String) { - Class rawValueClass = settings.getRawType(getKey()); - if (rawValueClass != null - && (Map.class.isAssignableFrom(rawValueClass) || Collection.class.isAssignableFrom(rawValueClass)) - ) { - throw new IllegalArgumentException( - "Failed to parse [" + value + "] for string setting [" + getKey() + "] because it is not scalar." - ); - } - } final Iterator> it = validator.settings(); final Map, T> map; if (it.hasNext()) { @@ -464,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()); } /** Logs a deprecation warning if the setting is deprecated and used. */ @@ -1317,6 +1311,11 @@ public void diff(Settings.Builder builder, Settings source, Settings defaultSett } } + @Override + boolean isListSetting() { + return true; + } + } static void logSettingUpdate(Setting setting, Settings current, Settings previous, Logger logger) { diff --git a/server/src/main/java/org/elasticsearch/common/settings/Settings.java b/server/src/main/java/org/elasticsearch/common/settings/Settings.java index d4595bcc1b8a2..a742ddc40dee5 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/Settings.java +++ b/server/src/main/java/org/elasticsearch/common/settings/Settings.java @@ -245,9 +245,28 @@ public String get(String setting, String defaultValue) { return retVal == null ? defaultValue : retVal; } - Class getRawType(String setting) { + /** + * 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) { Object value = settings.get(setting); - return value == null ? null : value.getClass(); + if(value != null) { + if (value instanceof List) { + if (isList == false) { + throw new IllegalArgumentException( + "Found list type value for setting [" + setting + "] but but did not expect a list for it." + ); + } + } else if (isList) { + throw new IllegalArgumentException( + "Expected list type value for setting [" + setting + "] but found [" + value.getClass() + ']' + ); + } + return toString(value); + } else { + return defaultValue; + } } /** diff --git a/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java b/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java index 67ac2e68b94c1..30cfee81ddd40 100644 --- a/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java +++ b/server/src/test/java/org/elasticsearch/common/settings/SettingTests.java @@ -184,7 +184,7 @@ public void testValidateStringSetting() { Settings settings = Settings.builder().putList("foo.bar", Arrays.asList("bla-a", "bla-b")).build(); Setting stringSetting = Setting.simpleString("foo.bar", Property.NodeScope); IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> stringSetting.get(settings)); - assertEquals("Failed to parse [[bla-a, bla-b]] for string setting [foo.bar] because it is not scalar.", e.getMessage()); + assertEquals("Found list type value for setting [foo.bar] but but did not expect a list for it.", e.getMessage()); } private static final Setting FOO_BAR_SETTING = new Setting<>( From eda6e8771b2b2a8fd4737c65131f4f5fbd07ec73 Mon Sep 17 00:00:00 2001 From: Armin Braun Date: Mon, 10 Sep 2018 18:06:29 +0200 Subject: [PATCH 3/4] CR: Add missing space --- .../main/java/org/elasticsearch/common/settings/Settings.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/common/settings/Settings.java b/server/src/main/java/org/elasticsearch/common/settings/Settings.java index a742ddc40dee5..75ccf284fea08 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/Settings.java +++ b/server/src/main/java/org/elasticsearch/common/settings/Settings.java @@ -251,7 +251,7 @@ public String get(String setting, String defaultValue) { */ public String get(String setting, String defaultValue, boolean isList) { Object value = settings.get(setting); - if(value != null) { + if (value != null) { if (value instanceof List) { if (isList == false) { throw new IllegalArgumentException( From 1ec003a65d3e46a9dbd0f8fae45974fb5a7b7711 Mon Sep 17 00:00:00 2001 From: Armin Braun Date: Mon, 10 Sep 2018 20:50:22 +0200 Subject: [PATCH 4/4] CR: -public; move to isInstance check in final method --- .../org/elasticsearch/common/settings/Setting.java | 10 ++-------- .../org/elasticsearch/common/settings/Settings.java | 2 +- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/settings/Setting.java b/server/src/main/java/org/elasticsearch/common/settings/Setting.java index d67b4a23336b0..5244cdd726d05 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/Setting.java +++ b/server/src/main/java/org/elasticsearch/common/settings/Setting.java @@ -346,8 +346,8 @@ boolean isGroupSetting() { } - boolean isListSetting() { - return false; + final boolean isListSetting() { + return this instanceof ListSetting; } boolean hasComplexMatcher() { @@ -1310,12 +1310,6 @@ public void diff(Settings.Builder builder, Settings source, Settings defaultSett } } } - - @Override - boolean isListSetting() { - return true; - } - } static void logSettingUpdate(Setting setting, Settings current, Settings previous, Logger logger) { diff --git a/server/src/main/java/org/elasticsearch/common/settings/Settings.java b/server/src/main/java/org/elasticsearch/common/settings/Settings.java index 75ccf284fea08..1aeed2aee5115 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/Settings.java +++ b/server/src/main/java/org/elasticsearch/common/settings/Settings.java @@ -249,7 +249,7 @@ public String get(String setting, String defaultValue) { * 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) { + String get(String setting, String defaultValue, boolean isList) { Object value = settings.get(setting); if (value != null) { if (value instanceof List) {