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

Supporting emit_defaults in json serialization #56

Closed
arlandism opened this issue Oct 26, 2020 · 3 comments · Fixed by #57
Closed

Supporting emit_defaults in json serialization #56

arlandism opened this issue Oct 26, 2020 · 3 comments · Fixed by #57

Comments

@arlandism
Copy link
Contributor

arlandism commented Oct 26, 2020

👋

I recently came across some behavior I wasn't expecting where falsey values are omitted from the json. Here's an example illustrating the issue:

[55] pry(API::Rpc::Groups)> a = MyProtobuf.new(a: true, b: true); b = MyProtobuf.new(a: false, b: false)
...
[60] pry(API::Rpc::Groups)> Twirp::Encoding.encode(a, a.class, 'application/json')
=> "{\"a\":true,\"b\":true}"
[61] pry(API::Rpc::Groups)> Twirp::Encoding.encode(b, b.class, 'application/json')
=> "{}"

The underlying Google protobuf supports additional options, one of which is the emit_defaults flag and including that directly in the encoding call seems to do the trick:

[65] pry(API::Rpc::Groups)> b.class.encode_json(b, emit_defaults: true)
=> "{\"a\":false,\"b\":false}"

I'm thinking we can just support additional pass-through options here: https://github.com/twitchtv/twirp-ruby/blob/master/lib/twirp/encoding.rb#L34. From there, I'm not sure of the best place to allow the caller to configure. Maybe in the rack env?

@arlandism
Copy link
Contributor Author

Notably, this won't pose an issue if you're hydrating Twirp objects from the generated JSON given that they'll populate those missing fields as the default anyway (e.g. false in this case), so it's really only a problem if you're viewing/using the raw response.

@marioizquierdo
Copy link
Collaborator

emit_defaults: true should be used by default. It was false before to be compatible with the Go implementation, but the newer v7.0.0 release uses emit_defaults by default. We also have to include other changes.

Enhanced customization is avoided in Twirp, but if there's a need for it we could use an option on the service constructor. I would think most options are desired on all-or-none of the api methods.

@arlandism
Copy link
Contributor Author

Ah, missed the new release. Thanks!

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 a pull request may close this issue.

2 participants