jackson-core icon indicating copy to clipboard operation
jackson-core copied to clipboard

Hex capitalization for JsonWriter should be configurable

Open pakoito opened this issue 2 years ago • 7 comments

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 https://github.com/FasterXML/jackson-core/blob/efe51e83648ed1b4c33ad1ffd5e2cbee5e0c6091/src/main/java/com/fasterxml/jackson/core/io/CharTypes.java#L7 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 https://github.com/FasterXML/jackson-core/blob/94a0e1cab04a218cb189e635727ca803dc56a958/src/main/java/com/fasterxml/jackson/core/JsonStreamContext.java#L331

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

Seen in 2.13, made from before 2.0

pakoito avatar Sep 16 '21 14:09 pakoito

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.

cowtowncoder avatar Sep 24 '21 23:09 cowtowncoder

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.

pakoito avatar Sep 27 '21 08:09 pakoito

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 avatar Oct 13 '21 20:10 cowtowncoder

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

cowtowncoder avatar Jun 20 '22 23:06 cowtowncoder

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

pakoito avatar Jun 23 '22 00:06 pakoito

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

isaacasensio avatar Jun 23 '22 07:06 isaacasensio

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

cowtowncoder avatar Jun 23 '22 21:06 cowtowncoder

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

Richie94 avatar Oct 03 '22 10:10 Richie94

@Richie94 much appreciated!

cowtowncoder avatar Oct 04 '22 00:10 cowtowncoder

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 avatar Oct 04 '22 19:10 cowtowncoder

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

cowtowncoder avatar Oct 04 '22 23:10 cowtowncoder

Thank you all!

pakoito avatar Oct 08 '22 09:10 pakoito