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

Ensure all keywords are stringified by java-util-hashmappify-vals #68

Merged
merged 2 commits into from
Nov 28, 2024

Conversation

sjamaan
Copy link
Contributor

@sjamaan sjamaan commented Nov 27, 2024

Change java-util-hashmappify-vals to also change keywords to strings for lone values nested under a map's direct values.

When postwalk would encounter a non-map value, the value would be returned as-is. This means a keyword which would be nested inside another non-map structure would not be converted to a string. For maps, if the keys or values are keywords themselves, it would always directly convert them into strings, which masked the problem.

This results in weird-looking serialization which exposes the underlying Java Keyword object, as per #67.

Change java-util-hashmappify-vals to also change keywords to strings
for lone values nested under a map's direct values.

When postwalk would encounter a non-map value, the value would be
returned as-is.  This means a keyword which would be nested inside
another non-map structure would not be converted to a string.  For
maps, if the keys or values are keywords themselves, it would always
directly convert them into strings, which masked the problem.

This results in weird-looking serialization which exposes the
underlying Java Keyword object, as per getsentry#67.
@sjamaan sjamaan requested a review from dharrigan as a code owner November 27, 2024 12:46
Instead of doing the map? test twice, do it once, using postwalk.
Postwalk already traverses all objects depth-first, so there's no need
to test for map? again in the inner "f" function.

To preserve existing functionality, use walk on the outer map (or
whatever object) to prevent it from being converted to a HashMap.

Add a test for the class of the resulting value and nested values to
ensure the functionality is still as before.
@sjamaan
Copy link
Contributor Author

sjamaan commented Nov 27, 2024

The second commit can be simplified further by dropping the outer walk call if it's acceptable to return HashMap for the outer map as well. I wasn't sure why this is the case, but this behaviour is explicitly documented, so I kept it the same for now.

@dharrigan dharrigan merged commit 5dca07d into getsentry:master Nov 28, 2024
1 check passed
@dharrigan
Copy link
Collaborator

Hi!

Looks good! Thank you kindly for your contribution.

-=david=-

@sjamaan
Copy link
Contributor Author

sjamaan commented Nov 28, 2024

Hi!

Looks good! Thank you kindly for your contribution.

-=david=-

Thanks David! I'd still be interested to learn the reason why the outer map isn't transformed to a HashMap. I think it'd be a worthwhile simplification if that distinction can be dropped.

@dharrigan
Copy link
Collaborator

Hi,

TBH, it's not something that I've spent much time looking at. I inherited the codebase, did a bit of tidy up, added some new things and generally maintain it.

If you think that it's better to transform to a hashmap, then sure, do a PR, let's look at it and we'll see :-)

-=david=-

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.

2 participants