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

64bit integer as json string #415

Open
wants to merge 9 commits into
base: 3-10-stable
Choose a base branch
from

Conversation

aslakhellesoy
Copy link

This patch fixes incorrect encoding of 64 bit integers, which, according to the official protobuf docs must be encoded as a string.

It also removes fields that have the default value (such as 0 for numbers).

@aslakhellesoy
Copy link
Author

cc @vincent-psarga

@abrandoned
Copy link
Contributor

@film42 how should we be controlling for proto3/proto2 in this scenario? while proto3 defines a json serialization format it would not be compat with what we are using for proto2, thoughts?

@aslakhellesoy
Copy link
Author

@abrandoned which part would be incompatible with what's being used for proto2? Encoding 64 bit integers as strings, or omitting fields with default values?

In #410 I preserved backwards compatibility by using the :lower_camel_case option. Would it help if I added a similar option to this PR? Rather than having lots of options, perhaps we could just call it :proto3 to enable proto3 features for JSON serialisation?

@aslakhellesoy
Copy link
Author

@abrandoned @film42 I have added a commit to preserve backwards compatibility with proto2 - the new serialisation behaviour is opt-in with to_json(proto3: true).

Let me know what you think.

@aslakhellesoy aslakhellesoy changed the base branch from master to 3-10-stable February 4, 2020 10:12
aslakhellesoy added a commit to cucumber-attic/protobuf that referenced this pull request Feb 5, 2020
@film42
Copy link
Member

film42 commented Feb 5, 2020

Thanks for the contribution. We technically aren't supporting proto3 at this time, so having a proto3 flag seems a big strange.

Would a flag like :use_nil_when_missing => true make more sense (or something similar)? We also have a convention where message.some_field returns the zero value (ex "") but message.some_field! returns nil if no value was set. Would it make sense to have a to_json! method that returned nil instead zero values?

Btw, I'm not recommending changes, just asking for feedback.

@film42
Copy link
Member

film42 commented Feb 5, 2020

I guess the correct language would be like :exclude_default_values => true?.

https://developers.google.com/protocol-buffers/docs/proto3#json_options

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