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

Hex capitalization for JsonWriter should be configurable #717

Closed
pakoito opened this issue Sep 16, 2021 · 12 comments
Closed

Hex capitalization for JsonWriter should be configurable #717

pakoito opened this issue Sep 16, 2021 · 12 comments
Labels
2.14 Issue planned (at earliest) for 2.14 good first issue Issue that seems easy to resolve and is likely a good candidate for contributors new to project pr-needed Feature request for which PR likely needed (no active development but idea is workable)
Milestone

Comments

@pakoito
Copy link

pakoito commented Sep 16, 2021

We're seeing inconsistencies between JS's and Jackson's representation of non-printable characters embedded in JSON strings. Where JS sends \u001b, once it goes through Jackson the field holds the string \u001B, causing all kind of hashing issues.

In

protected final static char[] HC = "0123456789ABCDEF".toCharArray();
we find that the hex values are hardcoded as capital A-F, whereas other representations such as JS's JSON.stringify indicate you should use lowercase a-f instead.

These are used in the standard JsonWriter via

CharTypes.appendQuoted(sb, currentName);

We would like to see a configuration parameter or recommended approach to overcome this issue.

Seen in 2.13, made from before 2.0

@cowtowncoder
Copy link
Member

This sounds like a good idea, an addition to JsonWriteFeature enums (not being universal one, although some other formats may need similar handling).
I wish I had time to work on this; if anyone has time I'd be happy to help but do not have time as is.
2.13.0 is being finalized as well so timing may not work for inclusion there, unless PR was ready within couple of days.

@pakoito
Copy link
Author

pakoito commented Sep 27, 2021

The implementation seems straightforward but I am not familiar enough with the repo to understand how to drill down this configuration, or write the tests. If you could give me an intro I could spend some time on it.

@cowtowncoder cowtowncoder added the 2.14 Issue planned (at earliest) for 2.14 label Oct 13, 2021
@cowtowncoder
Copy link
Member

Might be good idea to look into how other configuration settings are accessed. Or where helper method(s) for encoding are called by generators.

@cowtowncoder cowtowncoder added pr-needed Feature request for which PR likely needed (no active development but idea is workable) good first issue Issue that seems easy to resolve and is likely a good candidate for contributors new to project labels Jun 20, 2022
@cowtowncoder
Copy link
Member

On API choice: could either have a simple JsonWriteFeature with 2 choices (upper- / lower-case; defaulting to upper-case) -- like JsonWriteFeature.WRITE_HEX_UPPER_CASE(true) -- or something more advanced. I am guessing simple choice might work well enough.

Tagging as "good first issue" as this seems reasonable straight-forward (not necessarily super easy of course, just less complicated than many other open issues).

@pakoito
Copy link
Author

pakoito commented Jun 23, 2022

I'm okay with the simpler option. Sadly I'm not at that employer anymore, so tagging @isaacasensio

@isaacasensio
Copy link

👋 I can take this one. Thanks for the heads up @pakoito :)

@cowtowncoder
Copy link
Member

Very cool -- looking forward to a possible PR against 2.14 branch!
There have been a few recent additions so hopefully this works.

I can also help with details; f.ex in 2.x there's some non-ideal complexity since "same" feature needs to be added both in JsonGenerator.Feature and JsonWriteFeature (in 3.0 only latter needed).

@Richie94
Copy link
Contributor

Richie94 commented Oct 3, 2022

Hey @cowtowncoder I tried to pick this up since it seems it had not been fixed in the meantime.

@cowtowncoder
Copy link
Member

@Richie94 much appreciated!

@cowtowncoder cowtowncoder added this to the 2.14.0 milestone Oct 4, 2022
@cowtowncoder
Copy link
Member

As per PR this can now be configured using

JsonWriteFeature.WRITE_HEX_UPPER_CASE

(or its deprecated counterpart, JsonGenerator.Feature.WRITE_HEX_UPPER_CASE -- will be removed in 3.0)

and the default setting is true for backwards compatibility.

@cowtowncoder
Copy link
Member

Implemented via #819; will be in 2.14.0-rc2 (and final 2.14.0)

@pakoito
Copy link
Author

pakoito commented Oct 8, 2022

Thank you all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.14 Issue planned (at earliest) for 2.14 good first issue Issue that seems easy to resolve and is likely a good candidate for contributors new to project pr-needed Feature request for which PR likely needed (no active development but idea is workable)
Projects
None yet
Development

No branches or pull requests

4 participants