Add guard against null-valued settings #17310
Conversation
jasontedor
added
review
:Core/Infra/Settings
|
This gives more user-friendly error messages like: instead of Note that prior to #17292, the error message here was even worse: |
This commit adds a guard against null-valued settings that are loaded from yaml or json settings files, and also adds a test that ensures the same remains true for settings loaded from properties files.
|
LGTM. I would just call the boolean "allowNullValues" though. |
| @@ -27,6 +27,10 @@ | |||
| */ | |||
| public class JsonSettingsLoader extends XContentSettingsLoader { | |||
|
|
|||
| public JsonSettingsLoader(boolean guardAgainstNullValuedSettings) { | |||
There was a problem hiding this comment.
is there any place we want this to be false? just wondering if we just make it default behavior...
There was a problem hiding this comment.
yeah, we do ... never mind :(
|
nice. Happy we found a better name for the exception than the dreaded NPE |
The sole constructor of XContentSettingsLoader accepts a boolean flag that indicates whether or not null values parsed from the source should be rejected or not. Previously a true value for this flag meant that null values should be rejected. With this commit, the meaning of this flag is reversed so that a true value means that null values should be accepted (note that this is needed due to the way that settings are unset via the cluster update settings API). The name of this flag has been changed from guardAgainstNullValuedSettings to allowNullValues, for clarity.
| * accept null-valued keys. | ||
| * | ||
| * @param source The underlying settings content. | ||
| * @return A settings loader. | ||
| */ | ||
| public static SettingsLoader loaderFromSource(String source) { | ||
| if (source.indexOf('{') != -1 && source.indexOf('}') != -1) { |
There was a problem hiding this comment.
This detection feels pretty similar to XContentFactory#xContentType(CharSequence). Maybe we can use that?
There was a problem hiding this comment.
@nik9000 I have a code cleanup PR in the works related to this; I'll investigate that change in the context of that PR.
There was a problem hiding this comment.
I don't think that we should do this; the YAML detection there is different and I'm hesitant to change it.
There was a problem hiding this comment.
I'm fine with that. It'd help me the next time I look at this code if you add a comment explaining why you don't think this should share the detection logic. Or else I'm going to think the same thing all over again and waste time coming to the same conclusions you have. Or worse, I won't see why you didn't want to merge them and I'll break something.
I agree that that is better and I pushed 4d27328. |
This commit adds a guard against null-valued settings that are loaded
from yaml or json settings files, and also adds a test that ensures
the same remains true for settings loaded from properties files.
Relates #17292