From 9809760eb0658d9918c6f4aa5b15606c6e857fb3 Mon Sep 17 00:00:00 2001 From: Simon Willnauer Date: Thu, 24 Nov 2016 21:53:04 +0100 Subject: [PATCH] Fix settings diff generation for affix, list and group settings (#21788) Group, List and Affix settings generate a bogus diff that turns the actual diff into a string containing a json structure for instance: ``` "action" : { "search" : { "remote" : { "" : "{\"my_remote_cluster\":\"[::1]:60378\"}" } } } ``` which make reading the setting impossible. This happens for instance if a group or affix setting is rendered via `_cluster/settings?include_defaults=true` This change fixes the issue as well as several minor issues with affix settings that where not accepted as valid setting today. --- .../settings/AbstractScopedSettings.java | 16 +++-- .../common/settings/Setting.java | 65 +++++++++++++++++-- .../common/settings/ScopedSettingsTests.java | 36 ++++++++-- .../common/settings/SettingTests.java | 16 ++--- .../azure/storage/AzureStorageSettings.java | 13 ++-- .../test/cluster.put_settings/10_basic.yaml | 13 ++++ 6 files changed, 122 insertions(+), 37 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java b/core/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java index b993cef9290..3622623987b 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java +++ b/core/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java @@ -57,6 +57,7 @@ public abstract class AbstractScopedSettings extends AbstractComponent { 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]+[.])+$"); + private static final Pattern AFFIX_KEY_PATTERN = Pattern.compile("^(?:[-\\w]+[.])+(?:[*][.])+[-\\w]+$"); protected AbstractScopedSettings(Settings settings, Set> settingsSet, Setting.Property scope) { super(settings); @@ -86,7 +87,8 @@ public abstract class AbstractScopedSettings extends AbstractComponent { } protected void validateSettingKey(Setting setting) { - if (isValidKey(setting.getKey()) == false && (setting.isGroupSetting() && isValidGroupKey(setting.getKey())) == false) { + if (isValidKey(setting.getKey()) == false && (setting.isGroupSetting() && isValidGroupKey(setting.getKey()) + || isValidAffixKey(setting.getKey())) == false) { throw new IllegalArgumentException("illegal settings key: [" + setting.getKey() + "]"); } } @@ -111,6 +113,10 @@ public abstract class AbstractScopedSettings extends AbstractComponent { return GROUP_KEY_PATTERN.matcher(key).matches(); } + private static boolean isValidAffixKey(String key) { + return AFFIX_KEY_PATTERN.matcher(key).matches(); + } + public Setting.Property getScope() { return this.scope; } @@ -372,14 +378,10 @@ public abstract class AbstractScopedSettings extends AbstractComponent { public Settings diff(Settings source, Settings defaultSettings) { Settings.Builder builder = Settings.builder(); for (Setting setting : keySettings.values()) { - if (setting.exists(source) == false) { - builder.put(setting.getKey(), setting.getRaw(defaultSettings)); - } + setting.diff(builder, source, defaultSettings); } for (Setting setting : complexMatchers.values()) { - if (setting.exists(source) == false) { - builder.put(setting.getKey(), setting.getRaw(defaultSettings)); - } + setting.diff(builder, source, defaultSettings); } return builder.build(); } diff --git a/core/src/main/java/org/elasticsearch/common/settings/Setting.java b/core/src/main/java/org/elasticsearch/common/settings/Setting.java index a96b47762d5..22c74afee7c 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/Setting.java +++ b/core/src/main/java/org/elasticsearch/common/settings/Setting.java @@ -311,6 +311,19 @@ public class Setting extends ToXContentToBytes { } } + /** + * Add this setting to the builder if it doesn't exists in the source settings. + * The value added to the builder is taken from the given default settings object. + * @param builder the settings builder to fill the diff into + * @param source the source settings object to diff + * @param defaultSettings the default settings object to diff against + */ + public void diff(Settings.Builder builder, Settings source, Settings defaultSettings) { + if (exists(source) == false) { + builder.put(getKey(), getRaw(defaultSettings)); + } + } + /** * Returns the raw (string) settings value. If the setting is not present in the given settings object the default value is returned * instead. This is useful if the value can't be parsed due to an invalid value to access the actual value. @@ -649,6 +662,9 @@ public class Setting extends ToXContentToBytes { public static Setting> listSetting(String key, Function> defaultStringValue, Function singleValueParser, Property... properties) { + if (defaultStringValue.apply(Settings.EMPTY) == null) { + throw new IllegalArgumentException("default value function must not return null"); + } Function> parser = (s) -> parseableStringToList(s).stream().map(singleValueParser).collect(Collectors.toList()); @@ -670,6 +686,18 @@ public class Setting extends ToXContentToBytes { boolean exists = super.exists(settings); return exists || settings.get(getKey() + ".0") != null; } + + @Override + public void diff(Settings.Builder builder, Settings source, Settings defaultSettings) { + if (exists(source) == false) { + String[] asArray = defaultSettings.getAsArray(getKey(), null); + if (asArray == null) { + builder.putArray(getKey(), defaultStringValue.apply(defaultSettings)); + } else { + builder.putArray(getKey(), asArray); + } + } + } }; } @@ -747,6 +775,17 @@ public class Setting extends ToXContentToBytes { return false; } + @Override + public void diff(Settings.Builder builder, Settings source, Settings defaultSettings) { + Map leftGroup = get(source).getAsMap(); + Settings defaultGroup = get(defaultSettings); + for (Map.Entry entry : defaultGroup.getAsMap().entrySet()) { + if (leftGroup.containsKey(entry.getKey()) == false) { + builder.put(getKey() + entry.getKey(), entry.getValue()); + } + } + } + @Override public AbstractScopedSettings.SettingUpdater newUpdater(Consumer consumer, Logger logger, Consumer validator) { @@ -856,14 +895,14 @@ public class Setting extends ToXContentToBytes { * storage.${backend}.enable=[true|false] can easily be added with this setting. Yet, adfix key settings don't support updaters * out of the box unless {@link #getConcreteSetting(String)} is used to pull the updater. */ - public static Setting adfixKeySetting(String prefix, String suffix, Function defaultValue, + public static Setting affixKeySetting(String prefix, String suffix, Function defaultValue, Function parser, Property... properties) { - return affixKeySetting(AffixKey.withAdfix(prefix, suffix), defaultValue, parser, properties); + return affixKeySetting(AffixKey.withAffix(prefix, suffix), defaultValue, parser, properties); } - public static Setting adfixKeySetting(String prefix, String suffix, String defaultValue, Function parser, + public static Setting affixKeySetting(String prefix, String suffix, String defaultValue, Function parser, Property... properties) { - return adfixKeySetting(prefix, suffix, (s) -> defaultValue, parser, properties); + return affixKeySetting(prefix, suffix, (s) -> defaultValue, parser, properties); } public static Setting affixKeySetting(AffixKey key, Function defaultValue, Function parser, @@ -888,6 +927,15 @@ public class Setting extends ToXContentToBytes { throw new IllegalArgumentException("key [" + key + "] must match [" + getKey() + "] but didn't."); } } + + @Override + public void diff(Settings.Builder builder, Settings source, Settings defaultSettings) { + for (Map.Entry entry : defaultSettings.getAsMap().entrySet()) { + if (match(entry.getKey())) { + getConcreteSetting(entry.getKey()).diff(builder, source, defaultSettings); + } + } + } }; } @@ -960,7 +1008,7 @@ public class Setting extends ToXContentToBytes { return new AffixKey(prefix, null); } - public static AffixKey withAdfix(String prefix, String suffix) { + public static AffixKey withAffix(String prefix, String suffix) { return new AffixKey(prefix, suffix); } @@ -970,6 +1018,9 @@ public class Setting extends ToXContentToBytes { public AffixKey(String prefix, String suffix) { assert prefix != null || suffix != null: "Either prefix or suffix must be non-null"; this.prefix = prefix; + if (prefix.endsWith(".") == false) { + throw new IllegalArgumentException("prefix must end with a '.'"); + } this.suffix = suffix; } @@ -1005,9 +1056,9 @@ public class Setting extends ToXContentToBytes { sb.append(prefix); } if (suffix != null) { - sb.append("*"); + sb.append('*'); + sb.append('.'); sb.append(suffix); - sb.append("."); } return sb.toString(); } diff --git a/core/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java b/core/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java index 99126dcccd4..851ea26a19d 100644 --- a/core/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java +++ b/core/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java @@ -213,20 +213,44 @@ public class ScopedSettingsTests extends ESTestCase { public void testDiff() throws IOException { Setting fooBarBaz = Setting.intSetting("foo.bar.baz", 1, Property.NodeScope); Setting fooBar = Setting.intSetting("foo.bar", 1, Property.Dynamic, Property.NodeScope); + Setting someGroup = Setting.groupSetting("some.group.", Property.Dynamic, Property.NodeScope); + Setting someAffix = Setting.affixKeySetting("some.prefix.", "somekey", "true", Boolean::parseBoolean, Property.NodeScope); Setting> foorBarQuux = Setting.listSetting("foo.bar.quux", Arrays.asList("a", "b", "c"), Function.identity(), Property.NodeScope); - ClusterSettings settings = new ClusterSettings(Settings.EMPTY, new HashSet<>(Arrays.asList(fooBar, fooBarBaz, foorBarQuux))); + ClusterSettings settings = new ClusterSettings(Settings.EMPTY, new HashSet<>(Arrays.asList(fooBar, fooBarBaz, foorBarQuux, + someGroup, someAffix))); Settings diff = settings.diff(Settings.builder().put("foo.bar", 5).build(), Settings.EMPTY); - assertThat(diff.getAsMap().size(), equalTo(2)); + assertEquals(4, diff.getAsMap().size()); // 4 since foo.bar.quux has 3 values essentially assertThat(diff.getAsInt("foo.bar.baz", null), equalTo(1)); - assertThat(diff.get("foo.bar.quux", null), equalTo("[\"a\",\"b\",\"c\"]")); + assertArrayEquals(diff.getAsArray("foo.bar.quux", null), new String[] {"a", "b", "c"}); diff = settings.diff( Settings.builder().put("foo.bar", 5).build(), - Settings.builder().put("foo.bar.baz", 17).put("foo.bar.quux", "d,e,f").build()); - assertThat(diff.getAsMap().size(), equalTo(2)); + Settings.builder().put("foo.bar.baz", 17).putArray("foo.bar.quux", "d", "e", "f").build()); + assertEquals(4, diff.getAsMap().size()); // 4 since foo.bar.quux has 3 values essentially assertThat(diff.getAsInt("foo.bar.baz", null), equalTo(17)); - assertThat(diff.get("foo.bar.quux", null), equalTo("[\"d\",\"e\",\"f\"]")); + assertArrayEquals(diff.getAsArray("foo.bar.quux", null), new String[] {"d", "e", "f"}); + + diff = settings.diff( + Settings.builder().put("some.group.foo", 5).build(), + Settings.builder().put("some.group.foobar", 17, "some.group.foo", 25).build()); + assertEquals(6, diff.getAsMap().size()); // 6 since foo.bar.quux has 3 values essentially + assertThat(diff.getAsInt("some.group.foobar", null), equalTo(17)); + assertNull(diff.get("some.group.foo")); + assertArrayEquals(diff.getAsArray("foo.bar.quux", null), new String[] {"a", "b", "c"}); + assertThat(diff.getAsInt("foo.bar.baz", null), equalTo(1)); + assertThat(diff.getAsInt("foo.bar", null), equalTo(1)); + + diff = settings.diff( + Settings.builder().put("some.prefix.foo.somekey", 5).build(), + Settings.builder().put("some.prefix.foobar.somekey", 17, + "some.prefix.foo.somekey", 18).build()); + assertEquals(6, diff.getAsMap().size()); // 6 since foo.bar.quux has 3 values essentially + assertThat(diff.getAsInt("some.prefix.foobar.somekey", null), equalTo(17)); + assertNull(diff.get("some.prefix.foo.somekey")); + assertArrayEquals(diff.getAsArray("foo.bar.quux", null), new String[] {"a", "b", "c"}); + assertThat(diff.getAsInt("foo.bar.baz", null), equalTo(1)); + assertThat(diff.getAsInt("foo.bar", null), equalTo(1)); } public void testUpdateTracer() { diff --git a/core/src/test/java/org/elasticsearch/common/settings/SettingTests.java b/core/src/test/java/org/elasticsearch/common/settings/SettingTests.java index 6ec9093536e..2bd5dea3c10 100644 --- a/core/src/test/java/org/elasticsearch/common/settings/SettingTests.java +++ b/core/src/test/java/org/elasticsearch/common/settings/SettingTests.java @@ -442,9 +442,9 @@ public class SettingTests extends ESTestCase { } } - public void testAdfixKeySetting() { + public void testAffixKeySetting() { Setting setting = - Setting.adfixKeySetting("foo", "enable", "false", Boolean::parseBoolean, Property.NodeScope); + Setting.affixKeySetting("foo.", "enable", "false", Boolean::parseBoolean, Property.NodeScope); assertTrue(setting.hasComplexMatcher()); assertTrue(setting.match("foo.bar.enable")); assertTrue(setting.match("foo.baz.enable")); @@ -456,12 +456,12 @@ public class SettingTests extends ESTestCase { assertTrue(concreteSetting.get(Settings.builder().put("foo.bar.enable", "true").build())); assertFalse(concreteSetting.get(Settings.builder().put("foo.baz.enable", "true").build())); - try { - setting.getConcreteSetting("foo"); - fail(); - } catch (IllegalArgumentException ex) { - assertEquals("key [foo] must match [foo*enable.] but didn't.", ex.getMessage()); - } + IllegalArgumentException exc = expectThrows(IllegalArgumentException.class, () -> setting.getConcreteSetting("foo")); + assertEquals("key [foo] must match [foo.*.enable] but didn't.", exc.getMessage()); + + exc = expectThrows(IllegalArgumentException.class, () -> Setting.affixKeySetting("foo", "enable", "false", + Boolean::parseBoolean, Property.NodeScope)); + assertEquals("prefix must end with a '.'", exc.getMessage()); } public void testMinMaxInt() { diff --git a/plugins/repository-azure/src/main/java/org/elasticsearch/cloud/azure/storage/AzureStorageSettings.java b/plugins/repository-azure/src/main/java/org/elasticsearch/cloud/azure/storage/AzureStorageSettings.java index 6d1ed0c1049..9d67eea628b 100644 --- a/plugins/repository-azure/src/main/java/org/elasticsearch/cloud/azure/storage/AzureStorageSettings.java +++ b/plugins/repository-azure/src/main/java/org/elasticsearch/cloud/azure/storage/AzureStorageSettings.java @@ -34,12 +34,7 @@ import java.util.Map; import java.util.function.Function; public final class AzureStorageSettings { - private static final String TIMEOUT_SUFFIX = "timeout"; - private static final String ACCOUNT_SUFFIX = "account"; - private static final String KEY_SUFFIX = "key"; - private static final String DEFAULT_SUFFIX = "default"; - - private static final Setting.AffixKey TIMEOUT_KEY = Setting.AffixKey.withAdfix(Storage.PREFIX, TIMEOUT_SUFFIX); + private static final Setting.AffixKey TIMEOUT_KEY = Setting.AffixKey.withAffix(Storage.PREFIX, "timeout"); private static final Setting TIMEOUT_SETTING = Setting.affixKeySetting( TIMEOUT_KEY, @@ -47,11 +42,11 @@ public final class AzureStorageSettings { (s) -> Setting.parseTimeValue(s, TimeValue.timeValueSeconds(-1), TIMEOUT_KEY.toString()), Setting.Property.NodeScope); private static final Setting ACCOUNT_SETTING = - Setting.adfixKeySetting(Storage.PREFIX, ACCOUNT_SUFFIX, "", Function.identity(), Setting.Property.NodeScope); + Setting.affixKeySetting(Storage.PREFIX, "account", "", Function.identity(), Setting.Property.NodeScope); private static final Setting KEY_SETTING = - Setting.adfixKeySetting(Storage.PREFIX, KEY_SUFFIX, "", Function.identity(), Setting.Property.NodeScope); + Setting.affixKeySetting(Storage.PREFIX, "key", "", Function.identity(), Setting.Property.NodeScope); private static final Setting DEFAULT_SETTING = - Setting.adfixKeySetting(Storage.PREFIX, DEFAULT_SUFFIX, "false", Boolean::valueOf, Setting.Property.NodeScope); + Setting.affixKeySetting(Storage.PREFIX, "default", "false", Boolean::valueOf, Setting.Property.NodeScope); private final String name; diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/cluster.put_settings/10_basic.yaml b/rest-api-spec/src/main/resources/rest-api-spec/test/cluster.put_settings/10_basic.yaml index 41552f217be..5031c977ccd 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/cluster.put_settings/10_basic.yaml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/cluster.put_settings/10_basic.yaml @@ -61,3 +61,16 @@ - match: {persistent: {}} +--- +"Test get a default settings": + + - skip: + version: " - 5.99.99" # this can't be bumped to 5.0.2 until snapshots are published + reason: Fetching default group setting was buggy until 5.0.3 + + - do: + cluster.get_settings: + include_defaults: true + + - match: {defaults.node.attr.testattr: "test"} +