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

Fix parsing of unclean map values in Config. #4032

Merged
merged 1 commit into from
Aug 31, 2021

Conversation

anuraaga
Copy link
Contributor

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 :)

}

@Test
@Disabled("Currently invalid values are not handled the same as the SDK")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mateuszrzeszutek I think we should make sure the behavior is the same as the SDK for these too

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think that's fine

Comment on lines +26 to +32
.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);
}
Copy link
Member

Choose a reason for hiding this comment

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

should we allow empty values? e.g. key1=val1,key2=,key3=val3

Copy link
Contributor Author

@anuraaga anuraaga Aug 31, 2021

Choose a reason for hiding this comment

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants