Use EmitDefaults: true for jsonpb.Marshaler in generated server code.

[larrymyers]

protoc-gen-twirp/generator.go#L1012

In generateServerJSONMethod the jsonpb.Marshaler instance should likely include EmitDefaults: true as an option. While Go has a concept of zero values, javascript does not (i.e. the number 0 and undefined are very different things).

Let's say a proto Message struct in Go is:

type Hat struct {
  Name    string
  Size       int32
  IsFancy bool
}

Serializing this Hat instance with the twirp jsonpb.Marshaler does the following:

hat := Hat{Name: "", Size: 0}

JSON:

{}

Thoughts?

[spenczar]

Right, this comes up pretty often as a stumbling block for our users. As you note, it's explicitly supported, too.

I wonder whether this would break behavior for anyone, though. We should probably be cautious and treat it as a change we can only make with a major version bump.

[larrymyers]

@spenczar If you want to slot this into the v6 release I'll happily provide a pull request with the code fix and documentation.

[ijhuang]

any chance this will make it into v6?

[marioizquierdo]

Some context and comments on this issue.

Zero and Undefined are not the same

In many languages (specially in JavaScript) 0 and undefined are not the same thing, but that should not matter as long as the Twirp client is autogenerated. For example, if a field type is an integer, the client should be designed to return an integer, even if the serialized value is empty.

The network protocol doesn't differentiate between zero-values and undefined, with the goal of using as little bandwidth as possible. Autogenerated clients de-serialize using the official json-pb mapping protocol: https://developers.google.com/protocol-buffers/docs/proto3#json.

If your service actually needs to differentiate between zero and undefined values, you should add a second boolean property in the Proto file (true if the value exists), or wrap the value with one of the well known message types (message values can be undefined).

Manually handling/debugging JSON requests

If you are using curl (or similar) to debug your Twirp service, you probably find annoying having to guess if the missing fields are zero-values, undefined or actually missing values. I hear you, this may be a real issue.

Is EmitDefaults a good idea?

Yes I think it is. Looking back, I personally think Twirp should always have use EmitDefaults: true as part of the protocol.

Skipping zero values in the serialized JSON may save some bandwidth, but if you need performance you would use Protobuf instead. The JSON serialization should help with debugging and convenience.

Is EmitDefaults backwards compatible?

Changing the protocol to make EmitDefaults: true mandatory seems backwards compatible. I can't think of a case that would cause issues on old clients (that use EmitDefaults: false), but we have to explore that with much care (maybe oneof cases would break?). This change may also break hand-written clients, although whatever is being done to interpret zero and non-existing values differently would be outside of the specification.

Another option is to allow using a flag like emit_json_defaults (PR #134) to help with backwards compatibility, but it would contradict the philosophy of keeping code generation as simple as possible. Twirp by design doesn't support code generation flags, that could add much complexity to the protocol. What if a service is generated with a flag and the clients with a different flag? What if the client is written in a different language and that flag is not supported in that language? Keeping the protocol simple and clear allows to easily implement Twirp in other languages and platforms. Serialization issues can be very hard to debug.

[marwan-at-work]

I'm running into this as well :)

I think a better option than having emit_json_defaults as a param, is to give users a hook into creating the jsonpb.Marshaler struct since there are other options for json serialization such as "EnumsAsInts".

And maybe even further, to have the hook take a proto.Message and return a ([]byte, error) so that users can customize serialization based on their requirements.

That said, keeping the protocol simple and less customizable is tempting from the maintainer's perspective. On the other hand, this would unblock a lot of users. A compromise would be the protocol would only guarantee the default behavior and if a user wanted to pass a custom marshaler, it's their responsibility to ensure all of their clients are compatible.

[ilyar]

is there something new in this?

[dpolansky]

I agree with Mario's opinion above that EmitDefaults: true seems sensible for JSON in hindsight. JSON support exists in Twirp to aid with debugging, and seeing a JSON response such as {} for an empty protobuf message can be confusing to many users.

However, this behavior will not change because it has significant potential to negatively impact Twirp's users. For instance, services using JSON in production would be impacted. A service that starts including default values in its JSON responses may experience a significant increase in the size of its response payloads, which may lead to unexpected changes in performance. Including this in a new major version was mentioned above, but this issue doesn't seem to outweigh the organizational cost that would be required by Twirp's users to safely perform a major version upgrade.

I agree that Twirp could make it possible for users change the behavior of the jsonpb.Marshaler in a way that is not supported by the protocol. For instance, an insertion point could be added to the generated code to allow other generators to inject a different value for the struct. However, I'm not yet convinced that this behavior is a blocker for users, since the protobuf "well-known types" provide a means of emitting default values on the wire (at the cost of not using the default protobuf types).

[marioizquierdo]

Fixed in PR #271

JSON serialization using EmitDefaults: true is enabled by default on the v7 release. It is possible to disable it through the server constructor option twirp.WithServerJSONSkipDefaults(true), if required for full backwards compatibility.