search

ruby-protobuf / protobuf

A pure ruby implementation of Google's Protocol Buffers
https://github.com/ruby-protobuf
MIT License
462 stars 101 forks source link

64bit integer as json string #415

Open aslakhellesoy opened 4 years ago

aslakhellesoy commented 4 years ago

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 commented 4 years ago

cc @vincent-psarga

abrandoned commented 4 years ago

@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 commented 4 years ago

@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 https://github.com/ruby-protobuf/protobuf/pull/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 commented 4 years ago

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

film42 commented 4 years ago

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 commented 4 years ago

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

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