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

Allow JSON lowerCamelCase names #299

Merged
merged 4 commits into from
Jun 14, 2021
Merged

Conversation

mterwill
Copy link
Contributor

Relates to #84. My organization has some tooling that expects lowerCamelCase fields and Twirp currently does not allow you configure this behavior. Mirrors implementation of similar option added in #271.

@marioizquierdo
Copy link
Contributor

Twirp is not meant to have customizable serialization, but it makes sense to allow some basic settings for JSON. We should make sure to document what are the implications of enabling/disabling lower/camel case fields.

@mterwill
Copy link
Contributor Author

mterwill commented Apr 6, 2021

Happy to add docs – were you thinking inline or in the docs folder? Anything in particular you want to call out? Not seeing any caveats in https://developers.google.com/protocol-buffers/docs/proto3#json.

@marioizquierdo
Copy link
Contributor

Just inline on the server_options.go file. Seems good to me, I would review and help merge a PR with this functionality 👍

@mterwill
Copy link
Contributor Author

mterwill commented Apr 6, 2021

Thanks @marioizquierdo! This is a PR 😄

@marioizquierdo
Copy link
Contributor

😅 never read Github prs/issues from your email. Gotcha!

@marioizquierdo
Copy link
Contributor

Okay. There are two things I would like to tighten before merging this one:

  • Twirp has been long waiting to update protob to protojson. Let's make sure that this option will play well with protojson, and maybe we can a more generic way to pass server options into protojson. We don't want to be forcing twirp upgrades everytime we need to add a new option here.
  • I would like to explore a way to read server options from the generated code with default values for non-existing options, for example serverOpts.BoolVal("JSONCamelCaseNames") instead of serverOpts.JSONCamelCaseNames, so when there's a new option added to server (or client) options, older generated code still compiles.

I will allocate some time this week to work through this, but this will slow down merging this PR. If you need this functionality now, you can operate from this branch, because the resulting "final" version will be very similar and easy update for you. The important thing is that we will eventually support camelCased fields in the JSON serialization.

@mterwill
Copy link
Contributor Author

mterwill commented Apr 6, 2021

This all sounds great. No rush from my end and I'm also happy to put in some time to polish this up.

I know you mentioned earlier that Twirp isn't meant to have customizable serialization, but if we're exploring arbitrary options anyway, one solution might be taking a page from gRPC's WithCodec (h/t @bakins for the idea)

@marioizquierdo marioizquierdo mentioned this pull request Apr 16, 2021
server_options.go Outdated Show resolved Hide resolved
@mterwill
Copy link
Contributor Author

@marioizquierdo 👋 just rebased this to resolve conflicts from the v8 release

@marioizquierdo
Copy link
Contributor

marioizquierdo commented May 11, 2021

Perfect! The next thing we need is to add the generic settings getter on the runtime library code, so we don't have to force library updates if clients include a new option. The issue #316 should have a PR implemented sometime in the next few weeks. Thank you so much for the patience!

@marioizquierdo
Copy link
Contributor

Here is the PR that allows adding backwards-compatible options: #319

@marioizquierdo
Copy link
Contributor

@mterwill I fixed the conflicts with main for you. This PR should be ready now! Do you mind taking a last look before getting it ready for release?

@mterwill
Copy link
Contributor Author

💯 looks great, thank you!

@marioizquierdo marioizquierdo merged commit b3cd5ce into twitchtv:main Jun 14, 2021
@marioizquierdo marioizquierdo changed the title Add server option to use default JSON names Allow JSON lowerCamelCase names Jun 16, 2021
@marioizquierdo
Copy link
Contributor

Using twirp.WithServerJSONCamelCaseNames(true) will result in the proto3 JSON default serialization method, that uses lowerCamelCase names by default, but also honors json_name annotations. If the json_name field option is specified in the proto file, the specified value will be used as the key instead.

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.

2 participants