Empty strings, nulls, JSON and EmitDefaults

[atombender]

(I wasn't sure whether this was an issue for golang/protobuf or gogo/protobuf. I start here, since Gogo is what I'm using, and offers extensions that might apply.)

When using jsonpb, it's pretty much necessary to enable EmitDefaults, otherwise numeric values are omitted entirely if they are zero, which is surprising and usually nonsensical in the context of APIs.

However, EmitDefaults also causes empty strings to be included, and there doesn't seem to be any way to enforce nullable strings:

message Result {
  string id = 1 [
    (gogoproto.customname) = "ID",
    (gogoproto.nullable) = true
  ];
}

This is the default, and declares the field as string, not *string. Fine on input, but not on JSON output.

Is there a way to force a string pointer so that the field is properly omitted from JSON output?

[awalterschulze]

This is quite interesting. nullable=false has some effect on the output serialization format of the binary proto output. The jsonpb library mostly relies on reflect and tries not to interpret the proto too much.

I wouldn't say that not emitting zero values is non sensical.

Are you using proto2 or proto3?

[atombender]

By nonsensical I mean that from the API point of view, the idea that zero means "missing" is nonsensical. If you do GET /sheep/count you don't want an empty response if there are no sheep; you want zero. Conversely, JavaScript/JSON does have null, and Go's convention of using an empty string to mean a missing value doesn't translate.

Just to be clear, I want to be able to define a string field as *string. An empty string should not be omitted from JSON output; only nil strings should be.

I'm using proto3.

[awalterschulze]

Does nullable=true generate a pointer for a string in proto3? Can't remember that I have thought that through. Typically people want to get rid of the pointer for performance reasons.

[atombender]

No, as my original comment says, gogoproto.nullable does nothing to strings in the outputted Go structs. They stay string.

Performance is one thing, but the only way in Go to distinguish between a "missing" and "present" values is a pointer.

[awalterschulze]

Yes but this is the trade off of proto3. They made this design decision and you are not only one that is unhappy about it.

[awalterschulze]

I would be interested if you could fix this with some of the well known types https://developers.google.com/protocol-buffers/docs/reference/google.protobuf But I am not sure. Maybe this is what StringValue is for?

[atombender]

Well, the Protobuf wire format is one thing, how you map between that and Go is another. Surely there's no conceptual difference whether a field is a pointer on the Go side or a regular string; the marshaler/unmarshaler decides what consistutes a missing value.

Yes, I suppose we could use the Value WKT, but that makes the Go code harder to write. Those types are really unidiomatic Go (Struct is particularly horrible). Besides, it means there's no way to enforce the type of a field at the protocol level, since Value can hold any type.

[atombender]

...and you do support gogoproto.nullable for message fields. Just not strings (or, presumably, other primitives).

[awalterschulze]

Yes that makes sense your structures are not going to be idiomatic go structures anymore if you using the other WKTs.

nullable=false breaks the promise of knowing whether a field is set of not. This is stated in the documentation

Warning about nullable: according to the Protocol Buffer specification, you should be able to tell whether a field is set or unset. With the option nullable=false this feature is lost, since your non-nullable fields will always be set.

The pointer is how you tell whether the field is set of not, or at least it was in proto2.

So actually, at this point, I only support nullable=false to get rid of pointers.

[awalterschulze]

To ad a feature where nullable=true ads back pointers would need to be well thought through.

[awalterschulze]

And could have subtle effects.

[awalterschulze]

Reopen if you want to discuss it further

[atombender]

I'd like to reopen this.

Some more background, in case it wasn't clear: This is a real problem for APIs that expose a gRPC API that also has a JSON equivalent generated from the same Protobuf structs. I don't care about the gRPC/Protobuf part, that's fine; but I do need the JSON mapping to be sane.

EmitDefaults is useful, but too blunt an instrument. What I'd like is a Gogo-specific JSON field option to say whether a zero field is omittable, just like omitempty. Gogo's JSON emitter ignores the JSON tag, otherwise that would work (since you can override the JSON tag).

[liranp]

We would also like to have this fixed, to offer a more natural JSON API on top of gRPC. Any ideas how to solve this? How can I help with this one?

[awalterschulze]

This might be interesting https://github.com/google/protobuf/issues/731 Seems there is an option to change the json_name, which makes me wonder if they are open to requests like yours? Maybe we can first try this route, before doing something gogoprotobuf specific? Or maybe you or someone already has?

[awalterschulze]

I think there was at least a golang/protobuf issue about this? If someone can post the link here that would be semi helpful.

[atombender]

Is there a particular reason that Gogo needs to track Google's JSON marshaling code and can't diverge from it by adding JSON-specific extensions? Gogo already has lots of stuff Google doesn't. Making the JSON better would, I suspect, please a lot of people (e.g. see this Slack message about another awkwardness related to zero values and defaults) who want a more natural JSON output.

[awalterschulze]

Every feature requires extra maintenance work. So keeping the features, especially ones that isn't just another generated method to a minimum is quite important. So I am always first looking if we can do this another way, because then more people, not just gogo users, can benefit and I have less maintenance work. PS I can't seem to log into that slack link, maybe you can extract the message? I am very new to slack.

[johanbrandhorst]

Side note: sign up for the gopher slack and join us in #grpc and #protobuf :)

[awalterschulze]

Ok signed up and saw the message. Seems like the enum is the way to go, since there is also no way to tell in the go whether the false was set on purpose or is just unset.

[atombender]

That was just one of the problems, though (and it was the easy one!).

[awalterschulze]

Are you talking about the empty slice of strings vs a nil slice of strings?

[awalterschulze]

Since then this might be interesting for you https://github.com/gogo/protobuf/issues/181

[atombender]

Yes, though I suspect he was also thinking about the other fields (age and name).

[awalterschulze]

I guess this is the same problem and this is where ageIsSet and nameIsSet bool fields come in handy. Or simply using proto2 instead of proto3 if you really care about whether a field is set or not, this is one of the big differences in the two protocols.

[monajalal]

https://github.com/google/protobuf/issues/4358

[liues1992]

By default the plugin add omitempty to the json tag, just add an option to the plugin will solve the problem.

[pkieltyka]

@liues1992 in my tests, changing omitempty has no effects as jsonpb works different then encoding/json stdlib. I as well would like to see some kind of gogoproto extension to allow optional types (aka pointer types) in Go generator.. please please please

[johanbrandhorst]

@pkieltyka if you need optional scalars in proto3, that's what google/protobuf/wrappers.proto types are for.

[pkieltyka]

Yes, which is an okay workaround in Go, but we generate a typescript client too and that’ll make the client pretty ugly instead of just value?: number

On Dec 19, 2018, at 5:19 PM, Johan Brandhorst notifications@github.com wrote:

@pkieltyka if you need optional scalars in proto3, that's what google/protobuf/wrappers.proto types are for.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[johanbrandhorst]

The typescript generator should ideally recognise that it's a well known type and provide a special case. I don't know if it can do that, but it sounds like a reasonable thing.

[pkieltyka]

@johanbrandhorst my team and I wrote our own ts generator that works quite well: https://github.com/horizon-games/protoc-gen-twirp_ts -- and it is totally possible for us to build this case into the generator, but the best way would be some kind of annotation/hint in the proto like [ (protohint.optional) = 'value' ] where it could nest to find the under the structure, and build an optional type from the wrapped type.. or protohint.wrapped = true

[johanbrandhorst]

The reason I don't think this kind of annotation is necessary even in proto3 is because everything but scalars and enums are already optional, and for scalars you have the wrappers.