Skip to content

Commit

Permalink
Validate list values for settings (#33503)
Browse files Browse the repository at this point in the history
When we see a settings value, it could be a list. Yet this should only
happen if the underlying setting type is a list setting type. This
commit adds validation that when we get a setting value that is a list,
that the setting that we are getting is a list setting. And similarly,
if we get a value for a list setting, the underlying value should be a
list.
  • Loading branch information
original-brownbear authored and jasontedor committed Sep 10, 2018
1 parent 624b6bb commit 6075e15
Show file tree
Hide file tree
Showing 3 changed files with 37 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,11 @@ boolean isGroupSetting() {
return false;
}


final boolean isListSetting() {
return this instanceof ListSetting;
}

boolean hasComplexMatcher() {
return isGroupSetting();
}
Expand Down Expand Up @@ -453,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. */
Expand Down Expand Up @@ -1305,7 +1310,6 @@ public void diff(Settings.Builder builder, Settings source, Settings defaultSett
}
}
}

}

static void logSettingUpdate(Setting setting, Settings current, Settings previous, Logger logger) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,30 @@ public String get(String setting, String defaultValue) {
return retVal == null ? defaultValue : retVal;
}

/**
* Returns the setting value associated with the setting key. If it does not exists,
* returns the default value provided.
*/
String get(String setting, String defaultValue, boolean isList) {
Object value = settings.get(setting);
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;
}
}

/**
* Returns the setting value (as float) associated with the setting key. If it does not exists,
* returns the default value provided.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<String> stringSetting = Setting.simpleString("foo.bar", Property.NodeScope);
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> stringSetting.get(settings));
assertEquals("Found list type value for setting [foo.bar] but but did not expect a list for it.", e.getMessage());
}

private static final Setting<String> FOO_BAR_SETTING = new Setting<>(
"foo.bar",
"foobar",
Expand Down

0 comments on commit 6075e15

Please sign in to comment.