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

Implement a failing test for InternCache churn on map keys #3859

Closed
wants to merge 1 commit into from

Conversation

carterkozak
Copy link
Contributor

This is based on the investigation described here: FasterXML/jackson-core#946 (comment)

Comment on lines +29 to +31
assertEquals("Arbitrary map key values should not be added to the InternCache because " +
"map keys may have greater cardinality than the InternCache expects",
Collections.emptyMap(), InternCache.instance);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fails with:

AssertionFailedError: Arbitrary map key values should not be added to the InternCache because map keys may have greater cardinality than the InternCache expects 
Expected :{}
Actual   :{1st=1st, 2nd=2nd}

@cowtowncoder
Copy link
Member

Um, no. This is incorrect -- due to the way interning/canonicalization works it will canonicalize EVERY JSON Object key, by design. It's a feature not bug.
Streaming parser has no knowledge of the ultimate usage of Object keys, whether for POJOs or Maps.

Jackson 3.0 improves things slightly by reducing needs/benefits of canonicalization (BeanDeserializers will create NameMatchers that do not rely on canonicalization but act as "mini" symbol tables). But there's no way to start to only use canonicalization for some keys regardless.

@carterkozak
Copy link
Contributor Author

I see, sorry for my misunderstanding -- the change in 3.x sounds great!

@carterkozak carterkozak closed this Apr 4, 2023
@cowtowncoder
Copy link
Member

@carterkozak understandable. It'd be nice if canonicalization could be avoided for Map key cases etc.

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.

None yet

2 participants