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.
This commit is contained in:
Simon Willnauer
GitHub
parent
72ef6fa0d7
commit
9809760eb0
|
|
@ -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<Setting<?>> 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();
|
||||
}
|
||||
|
|
|
|||
|
|
@ -311,6 +311,19 @@ public class Setting<T> 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<T> extends ToXContentToBytes {
|
|||
|
||||
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());
|
||||
|
||||
|
|
@ -670,6 +686,18 @@ public class Setting<T> 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<T> extends ToXContentToBytes {
|
|||
return false;
|
||||
}
|
||||
|
||||
@Override
|
||||
public void diff(Settings.Builder builder, Settings source, Settings defaultSettings) {
|
||||
Map<String, String> leftGroup = get(source).getAsMap();
|
||||
Settings defaultGroup = get(defaultSettings);
|
||||
for (Map.Entry<String, String> entry : defaultGroup.getAsMap().entrySet()) {
|
||||
if (leftGroup.containsKey(entry.getKey()) == false) {
|
||||
builder.put(getKey() + entry.getKey(), entry.getValue());
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@Override
|
||||
public AbstractScopedSettings.SettingUpdater<Settings> newUpdater(Consumer<Settings> consumer, Logger logger,
|
||||
Consumer<Settings> validator) {
|
||||
|
|
@ -856,14 +895,14 @@ public class Setting<T> 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 <T> Setting<T> adfixKeySetting(String prefix, String suffix, Function<Settings, String> defaultValue,
|
||||
public static <T> Setting<T> affixKeySetting(String prefix, String suffix, Function<Settings, String> defaultValue,
|
||||
Function<String, T> parser, Property... properties) {
|
||||
return affixKeySetting(AffixKey.withAdfix(prefix, suffix), defaultValue, parser, properties);
|
||||
return affixKeySetting(AffixKey.withAffix(prefix, suffix), defaultValue, parser, properties);
|
||||
}
|
||||
|
||||
public static <T> Setting<T> adfixKeySetting(String prefix, String suffix, String defaultValue, Function<String, T> parser,
|
||||
public static <T> Setting<T> affixKeySetting(String prefix, String suffix, String defaultValue, Function<String, T> parser,
|
||||
Property... properties) {
|
||||
return adfixKeySetting(prefix, suffix, (s) -> defaultValue, parser, properties);
|
||||
return affixKeySetting(prefix, suffix, (s) -> defaultValue, parser, properties);
|
||||
}
|
||||
|
||||
public static <T> Setting<T> affixKeySetting(AffixKey key, Function<Settings, String> defaultValue, Function<String, T> parser,
|
||||
|
|
@ -888,6 +927,15 @@ public class Setting<T> 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<String, String> entry : defaultSettings.getAsMap().entrySet()) {
|
||||
if (match(entry.getKey())) {
|
||||
getConcreteSetting(entry.getKey()).diff(builder, source, defaultSettings);
|
||||
}
|
||||
}
|
||||
}
|
||||
};
|
||||
}
|
||||
|
||||
|
|
@ -960,7 +1008,7 @@ public class Setting<T> 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<T> 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<T> extends ToXContentToBytes {
|
|||
sb.append(prefix);
|
||||
}
|
||||
if (suffix != null) {
|
||||
sb.append("*");
|
||||
sb.append('*');
|
||||
sb.append('.');
|
||||
sb.append(suffix);
|
||||
sb.append(".");
|
||||
}
|
||||
return sb.toString();
|
||||
}
|
||||
|
|
|
|||
|
|
@ -213,20 +213,44 @@ public class ScopedSettingsTests extends ESTestCase {
|
|||
public void testDiff() throws IOException {
|
||||
Setting<Integer> fooBarBaz = Setting.intSetting("foo.bar.baz", 1, Property.NodeScope);
|
||||
Setting<Integer> fooBar = Setting.intSetting("foo.bar", 1, Property.Dynamic, Property.NodeScope);
|
||||
Setting<Settings> someGroup = Setting.groupSetting("some.group.", Property.Dynamic, Property.NodeScope);
|
||||
Setting<Boolean> someAffix = Setting.affixKeySetting("some.prefix.", "somekey", "true", Boolean::parseBoolean, Property.NodeScope);
|
||||
Setting<List<String>> 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() {
|
||||
|
|
|
|||
|
|
@ -442,9 +442,9 @@ public class SettingTests extends ESTestCase {
|
|||
}
|
||||
}
|
||||
|
||||
public void testAdfixKeySetting() {
|
||||
public void testAffixKeySetting() {
|
||||
Setting<Boolean> 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() {
|
||||
|
|
|
|||
|
|
@ -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<TimeValue> 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<String> 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<String> 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<Boolean> 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;
|
||||
|
|
|
|||
|
|
@ -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"}
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue