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

JsonFactory implementations should respect CANONICALIZE_FIELD_NAMES #1015

Closed
carterkozak opened this issue May 5, 2023 · 1 comment
Closed
Milestone

Comments

@carterkozak
Copy link
Contributor

This is a follow-up based on the conversation in #995.

Several places create byte quad canonicalizer instances using makeChild rather than makeChildOrPlaceholder
which avoids canonicalization.
Ideally, implementations would have a fast-path to avoid unnecessary work to search for canonicalized names, however such overhead is minimal compared to using canonicalization in cases that expect unbounded names. So, I plan to create a PR shortly which updates existing code that doesn't check the canonicalization setting to use a canonicalizer which will not canonicalize unexpectedly, by only checking _symbols.isCanonicalizing() prior to _symbols.addName, without adding branching to avoid lookups (_symbols._findName) in other cases. _findName is inexpensive on an empty table, and if we see real-world cases that this is problematic, it's possible to improve later on.

I will plan to make a similar change for the smile-parser in the dataformat-binary project as well. When I make that change, would you prefer if I reference this issue, or create another issue in that project?

Please let me know if you'd prefer an approach more similar to 3d565bd in which _findName is conditionally avoided as well.

Thanks!

carterkozak added a commit to carterkozak/jackson-core that referenced this issue May 5, 2023
…AMES`

Previously a subset of methods did not check for
`CANONICALIZE_FIELD_NAMES` in the factory features configuration.
carterkozak added a commit to carterkozak/jackson-core that referenced this issue May 5, 2023
…AMES`

Previously a subset of methods did not check for
`CANONICALIZE_FIELD_NAMES` in the factory features configuration.
carterkozak added a commit to carterkozak/jackson-core that referenced this issue May 5, 2023
…AMES`

Previously a subset of methods did not check for
`CANONICALIZE_FIELD_NAMES` in the factory features configuration.
@cowtowncoder cowtowncoder changed the title JsonFactory implementations should respect CANONICALIZE_FIELD_NAMES JsonFactory implementations should respect CANONICALIZE_FIELD_NAMES May 6, 2023
cowtowncoder added a commit that referenced this issue May 6, 2023
@cowtowncoder cowtowncoder added this to the 2.15.1 milestone May 6, 2023
@JoshRosen
Copy link

JoshRosen commented Apr 29, 2024

Hello,

I would like to confirm the release version of this patch: the milestone and release notes say that this was released in 2.15.1, but I am wondering whether that is potentially a typo: if I look at https://github.com/FasterXML/jackson-core/commits/jackson-core-2.15.1/src/main/java/com/fasterxml/jackson/core/JsonFactory.java for 2.15.1 I do not see this commit, but I do see it on https://github.com/FasterXML/jackson-core/commits/jackson-core-2.16.1/src/main/java/com/fasterxml/jackson/core/JsonFactory.java . Is this potentially a typo where 2.15.1 should be 2.16.1 in the milestone and release notes?

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

No branches or pull requests

3 participants