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.

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<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 +69,8 @@ public abstract class AbstractScopedSettings extends AbstractComponent {
 
         this.settingUpgraders =
                 Collections.unmodifiableMap(
-                        settingUpgraders
+                        settingUpgraders.stream().collect(Collectors.toMap(SettingUpgrader::getSetting, Function.identity())));
-                                .stream()
+
-                                .collect(
-                                        Collectors.toMap(
-                                                SettingUpgrader::getSetting,
-                                                u -> e -> new AbstractMap.SimpleEntry<>(u.getKey(e.getKey()), u.getValue(e.getValue())))));
 
         this.scope = scope;
         Map<String, Setting<?>> 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<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));
+                if (setting.isListSetting()) {
-                builder.put(upgrade.getKey(), upgrade.getValue());
+                    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

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;
+    }
+
 }

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