Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CORE: Validate Type for String Settings #33503

Merged
merged 5 commits into from
Sep 10, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,11 @@ boolean isGroupSetting() {
return false;
}


boolean isListSetting() {
return false;
}

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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of having an isListSetting, since this is already package private anyways, can this just check instanceof ListSetting?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, yea it could :) -> will change to that :)

Copy link
Member

@jasontedor jasontedor Sep 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@original-brownbear @rjernst See my diff, we need to expose Setting#isListSetting in AbstractScopedSettings.

Copy link
Member

@jasontedor jasontedor Sep 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its implementation though can be final and be an instance of check against ListSetting.

}

/** Logs a deprecation warning if the setting is deprecated and used. */
Expand Down Expand Up @@ -1306,6 +1311,11 @@ public void diff(Settings.Builder builder, Settings source, Settings defaultSett
}
}

@Override
boolean isListSetting() {
return true;
}

}

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.
*/
public String get(String setting, String defaultValue, boolean isList) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this be package private?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea I think so will adjust in a bit :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Object value = settings.get(setting);
if(value != null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: if( -> if ( (add space)

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() + ']'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that will fail compilation? 😇

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compiles just fine for me? :) What am I missing?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, never mind, I misread. Sorry for that. 😄

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