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

Support writing longs as strings (opt-in) #98

Conversation

rdesgroppes
Copy link

@rdesgroppes rdesgroppes commented Jun 9, 2023

jackson-datatype-protobuf is a missing link in the Jackson ecosystem, but it doesn't currently provide a way to write 64-bit integers using their canonical representation, which poses a problem for strict downstream parsers given the limitation of the maximum value to 2^53 - 1 (alt) in JavaScript, whereas this value is for instance 2^63 - 1 in Java.

The change proposed here is to provide users who need it with an option to activate this canonical representation, without altering the existing default behavior.

Addresses part of #76.
Supersedes #77.

@cowtowncoder
Copy link

No comment on this particular PR, but it might be worth filing an issue for jackson-core for requesting addition of basic StreamWriteFeature (if useful for multiple formats) or JsonWriteFeature (if only ever relevant for JSON) that could force this behavior at low level.

@rdesgroppes
Copy link
Author

rdesgroppes commented Jun 12, 2023

@cowtowncoder Done: FasterXML/jackson-core#1044. Since the limitation doesn't seem to be applicable to YAML, I guess this would be a JsonWriteFeature.

Having done that, I'd like the present request to be considered in its own right, as the flexibility requested here is specific to the serialization of protocol buffer messages.

`jackson-datatype-protobuf` is a missing link in the Jackson ecosystem,
but it doesn't currently provide a way to write 64-bit integers using
their canonical representation, which poses a problem for strict
downstream parsers given the limitation of the maximum value to
[2^53 - 1](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/MAX_SAFE_INTEGER)
([alt](https://developers.google.com/discovery/v1/type-format)) in
JavaScript, whereas this value is for instance
[2^63 - 1](https://docs.oracle.com/javase/tutorial/java/nutsandbolts/datatypes.html)
in Java.

The change proposed here is to provide users who need it with an option
to activate this canonical representation, without altering the existing
default behavior.
@rdesgroppes rdesgroppes force-pushed the support-writing-longs-as-strings branch from b797d1f to 9378d7c Compare June 12, 2023 11:30
@cowtowncoder
Copy link

Thank you @rdesgroppes. Agreed on all points.

@rdesgroppes
Copy link
Author

Hi @jhaber! May I kindly request a review?

@rdesgroppes
Copy link
Author

rdesgroppes commented Jun 16, 2023

@sankala-dremio or @stevie400, could you help there?

@jhaber
Copy link
Member

jhaber commented Jul 11, 2023

👋 I was on vacation, sorry for the delay. Support for writing longs as strings has been a long-standing feature request, as this is the canonical representation according to protobuf spec. However, as part of implementing this I noticed that we're not serializing/deserializing unsigned numbers properly. I'm hoping to tackle that first, and layer the string stuff on top

@rdesgroppes
Copy link
Author

[...] I'm hoping to tackle that first, and layer the string stuff on top

@jhaber is there anything I could do to help, then?

@jhaber
Copy link
Member

jhaber commented Jul 12, 2023

I opened #101 yesterday, I think it should address both issues

@rdesgroppes
Copy link
Author

Cool!
The present PR is therefore superseded. May I close it?

@jhaber
Copy link
Member

jhaber commented Jul 12, 2023

Yup, feel free to close. I'm planning to cut a release soon

@rdesgroppes
Copy link
Author

rdesgroppes commented Jul 12, 2023

[...] I'm planning to cut a release soon

I just gave a try with a locally built jar from the master branch and found out that acceptLtteralFieldNames(true) is no longer effective for proto definitions annotated with JSON names. I'll try to narrow down the problem tomorrow.

@rdesgroppes rdesgroppes deleted the support-writing-longs-as-strings branch July 12, 2023 17:39
@rdesgroppes rdesgroppes restored the support-writing-longs-as-strings branch July 12, 2023 17:39
@jhaber
Copy link
Member

jhaber commented Jul 12, 2023

Thanks for flagging, I'll try to reproduce and fix the issue

@rdesgroppes
Copy link
Author

Thanks for flagging, I'll try to reproduce and fix the issue

I filed #102 to track that.

@rdesgroppes rdesgroppes deleted the support-writing-longs-as-strings branch July 13, 2023 18:14
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.

3 participants