From ad4b5e427004351ae7e88dadfe4210db437ab764 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Tue, 11 Sep 2018 08:35:42 -0400 Subject: [PATCH] Fix upgrading of list settings (#33589) Upgrading list settings is broken because of the conversion that we do to strings, and then when we try to put back the upgraded value we do not know that it is a representation of a list. This commit addresses this by adding special handling for list settings. --- .../settings/AbstractScopedSettings.java | 26 ++++++----- .../common/settings/SettingUpgrader.java | 6 +++ .../common/settings/ScopedSettingsTests.java | 44 +++++++++++++++++++ 3 files changed, 65 insertions(+), 11 deletions(-) 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..b010d7982fd 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java +++ b/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java @@ -27,7 +27,6 @@ import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.component.AbstractComponent; import org.elasticsearch.common.regex.Regex; -import java.util.AbstractMap; import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; @@ -54,7 +53,7 @@ public abstract class AbstractScopedSettings extends AbstractComponent { private final List> settingUpdaters = new CopyOnWriteArrayList<>(); private final Map> complexMatchers; private final Map> keySettings; - private final Map, Function, Map.Entry>> settingUpgraders; + private final Map, 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 +69,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> complexMatchers = new HashMap<>(); @@ -786,15 +781,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> 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 upgrade = upgrader.apply(new Entry(key, settings)); - builder.put(upgrade.getKey(), upgrade.getValue()); + if (setting.isListSetting()) { + final List value = settings.getAsList(key); + final String upgradedKey = upgrader.getKey(key); + final List 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 { return value; } + default List getListValue(final List 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> oldSetting = + Setting.listSetting("foo.old", Collections.emptyList(), Function.identity(), Property.NodeScope); + final Setting> 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>() { + + @Override + public Setting> getSetting() { + return oldSetting; + } + + @Override + public String getKey(final String key) { + return "foo.new"; + } + + @Override + public List getListValue(final List value) { + return value.stream().map(s -> "new." + s).collect(Collectors.toList()); + } + })); + + final int length = randomIntBetween(0, 16); + final List 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()))); + } + }