Fix parsing of unclean map values in Config. #4032
Conversation
anuraaga
requested review from
iNikem,
jkwatson,
laurit,
mateuszrzeszutek,
pavolloffay,
trask and
tylerbenson
as code owners
| } | ||
|
|
||
| @Test | ||
| @Disabled("Currently invalid values are not handled the same as the SDK") |
There was a problem hiding this comment.
@mateuszrzeszutek I think we should make sure the behavior is the same as the SDK for these too
There was a problem hiding this comment.
Yeah, that'd certainly make sense to do so. On the other hand, in the agent code I think we'd probably prefer to fall back on the default value instead of throwing an exception in the middle of an advice class. So, how about the following:
- in the
@Nullable X getX(String name)methods (these are used only in the SDK bridge, I think) we'll throw an exception if the value is invalid; - but in the
X getX(String name, X default)(which are used pretty much everywhere in the agent) we'll return the default value in case of any errors
There was a problem hiding this comment.
Yeah I think that's fine
| .map(keyValuePair -> filterBlanksAndNulls(keyValuePair.split("=", 2))) | ||
| .map( | ||
| splitKeyValuePairs -> { | ||
| if (splitKeyValuePairs.size() != 2) { | ||
| throw new IllegalArgumentException( | ||
| "Invalid map property, should be formatted key1=value1,key2=value2: " + value); | ||
| } |
There was a problem hiding this comment.
should we allow empty values? e.g. key1=val1,key2=,key3=val3
There was a problem hiding this comment.
We do for getString (at least in SDK) though seem to be missing tests for it, and the map with empty values. I'd expect the two to be consistent. I think returning empty when it's empty is reasonable behavior, it's similar to how query params work
We currently reject map values where elements within them are e.g., empty, but we should ignore those (it's easy to have empty values, trailing commas, etc when creating a variable in a shell script).
I just copied the implementation as is from SDK.
Also added the ConfigProperties "TCK" from the SDK :)