Proposal: Stop using 'OrigName' option when doing jsonpb marshaling

[larrymyers]

Referencing this part of the generated code:

twirp/protoc-gen-twirp/generator.go

Line 640 in c0c4355

t.P(` marshaler := &`, t.pkgs["jsonpb"], `.Marshaler{OrigName: true}`)

Right now the jsonpb.Marshaler is configured to use the original name of the proto Message, instead of doing the default camel casing.

Is the design of twirp that the proto and json serialization should always be equivalent? It would be less work to do idiomatic javascript twirp clients if twirp allowed jsonpb to do its default behavior of lowerCamelCasing Message fields:

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

If this isn't an option for the v5 release, can it be debated for the current v6 work since it would be a breaking change?

[spenczar]

Of course, I'm always open to design debates.

The divergence from the spec here is a little odd, I agree, but let's talk a bit about why Twirp accepts JSON payloads: it provides a way to get stuff done when Protobuf payloads aren't available for your client. That happens in two main cases:

• There's no Twirp client generator for the language, so the client must be hand-written, and hand-writing a Protobuf client is very hard.
• You're writing a request on the command line with cURL to debug something.

So, JSON shows up when people are hand-writing requests, pretty much.

Twirp uses OrigName because, in practice, people who hand-write clients usually aren't deeply familiar with the JSON-encoded-Protobuf spec. Without OrigName, they almost always get serialization wrong - they seemed to be universally surprised by the camelCase remapping. It's much easier to describe how you write the cURL request when you can just say "use the same field names as the proto document."

If we change this, I'm concerned that we will make life harder for people hand-writing JSON clients, which is bad because those are the primary user of JSON payloads.

The tradeoff is that we would probably make things easier for people who are writing client generators, and who are working on JSON clients. I don't weight that benefit terribly heavily - if you're making a client generator, you should just go the full distance and generate protobuf clients. I think client generators don't really even need to make working JSON clients at all; the Go one does to serve as reference, but the Python one just speaks Protobuf, for example.

As you pointed out, this would be a breaking change - and it would be a very significant one. v6 servers would be unable to deserialize JSON requests at all from v5 clients, which is pretty brutal. We would need very strong reasons for doing this to overcome that negative.

[larrymyers]

@spenczar I see your point, I hadn't put as much thought into hand written clients. My use case is making it easier to generate javascript / typescript clients for the browser, where protobuf support isn't an option yet.

The reason I like the design of twirp so much is because it has HTTP 1.1 JSON support. It makes it much easier to use for browser / server communication and load balancers that don't support proxying over HTTP/2.

With gRPC the only option is to wire up grpc-gateway, which is just extra complexity.

That being said, handling the snake_case => camelCase conversation in the generated client isn't that much work, and avoids all the v5 / v6 breakage.

I'm fine closing this issue since the tradeoffs lean in favor of keeping JSON serialization in agreement with the fields in the proto file.

[spenczar]

@larrymyers, could you say a little more about this:

for the browser, where protobuf support isn't an option yet.

There's an official javascript protobuf generator available these days, so I thought this should work. https://github.com/thechriswalker/protoc-gen-twirp_js/tree/master/example/browser has a fully-worked example of doing protobuf requests from a browser.

I don't need to do this myself, so I might be unaware of problems. Whats the reason protobuf support isn't an option in browsers for you?

[wora]

The official protobuf library can generate either the original name or the camel cased name, but it MUST accept both names in all cases. If we make the change, it would not be a breaking change on the server side. To keep manual clients happy, the server has to return original names by default, and uses an option to return camel case names. The question is do we want to add a request flag for this feature?

Disclosure: I worked on the proto3 specification.

[larrymyers]

@spenczar I've reviewed Google's javascript protobuf library: https://github.com/google/protobuf/tree/master/js

The big downside for me is simply that the google-protobuf.js runtime is 160kb, which increases both download and parse time in the browser. JSON support is nearly universal in browsers, and gzip negates much of a size savings when transmitting protobufs over the wire.

Anyway, I expect to be publishing my typescript twirp client generator soon, and it wasn't much work to handle snake_case / camelCase parsing in the client, which gets me an idiomatic client using the twirp v5 spec.

[spenczar]

@larrymyers Okay, thanks for explaining.

For what it's worth, the google-protobuf.js runtime compresses down to 33kb when I pass it through gzip. I agree that the byte-savings for protobuf encoding is minimal; to me, the biggest benefit would be that serialization and deserialization of protobuf is a lot more memory-efficient, which is mostly important on the server.

But these are technical decisions which involve tradeoffs that can be made per-project - what you're doing sounds fine, of course. Glad that your typescript generator is working!