golang / protobuf

Go support for Google's protocol buffers
BSD 3-Clause "New" or "Revised" License
9.64k stars 1.58k forks source link

Using EmitUnpopulated in protojson for optional message attributes #1571

Closed honzap closed 7 months ago

honzap commented 8 months ago

Hello, I'm sending this first as a question (maybe my misunderstanding of documentation) although it might be a bug.

Let's have two simple proto messages:

message FooChild {
  string param = 1;
}

message Foo {
  string param1 = 1;
  optional FooChild param2 = 2;
}

Then, there's a Go struct generated out of this message (just a snippet of structs):

type FooChild struct {
    state         protoimpl.MessageState
    sizeCache     protoimpl.SizeCache
    unknownFields protoimpl.UnknownFields

    Param string `protobuf:"bytes,1,opt,name=param,proto3" json:"param,omitempty"`
}

type Foo struct {
    state         protoimpl.MessageState
    sizeCache     protoimpl.SizeCache
    unknownFields protoimpl.UnknownFields

    Param1 string  `protobuf:"bytes,1,opt,name=param1,proto3" json:"param1,omitempty"`
    Param2 *FooChild `protobuf:"bytes,2,opt,name=param2,proto3,oneof" json:"param2,omitempty"`
}

The Param2 is nullable as requested (optional) and has oneof annotation.

When I ask this struct to be marshalled via protojson with EmitUnpopulated, I'd assume (according to docs) the param2 will be null in that json. The reality is that it's not in that json.

foo := &Foo{}
res, err := protojson.MarshalOptions{EmitUnpopulated: true}.Marshal(foo)

The result is:

{"param1":""}

Although the param2 a message type, I'd expect it'll be null, so, I'd expect this result:

{"param1":"", "param2":null}

The "problem" is that optional field is annotated as oneof, so this condition makes the field skipped.

Could you please explain me this behavior?

Basically, I'd need to reach the situation that protojson writes all the optional fields as null if they are not set. I wrote my own simple marshaler via protoreflect to solve that situation but I'd assume, according to docs, there should be null in json.

Thank you for your explanation!

Best,

Honza

puellanivis commented 8 months ago

I think this is an interaction of the optional keyword and message fields. Messages were already presence sensing the whole time, and never required the use of optional in order to permit presence sensing. Removing the optional keyword will indeed make the output {"param1":"", "param2":null}

I think the thinking here is that when a message field is unset, and optional, the correct thing to do is to not emit field, the same as if this were an optional scalar value. In this case the default value of the message is &pb.FooChild{} not nil, which says “no presence”, which is reflected in the JSON as field does not appear.

PS: I think we discussed during the change to produce an EmitDefaultValues flag that it was unclear that for EmitUnpopulated an optional field that is not set will not appear.

honzap commented 7 months ago

Thanks for explanation.

Basically, optional on message type doesn't affect that it's in Go nillable or not. It's always nillable regardless optional. In case protojson, there's a difference if that field is populated there or not. In the gRPC contract (of Go services), sides should always check nillability regardless on optional.

I know there was some discussion about EmitUnpopulated and that's why I placed this question here.

puellanivis commented 7 months ago

The semantic used in protobuf is “presence” not so much “nilability”. Golang just simply represents it as nilable.

Whether a sub-message is actually present, but empty, or whether a sub-message is the default value matters really only in the encoding. Using GetParam2().GetParam() will still return "" for both, since the default value of a non-present message is the same as an empty message that appears. The GetParam() function is specially written however to ensure that even if the sub-message is nil it will not panic.

There’s really no reason for either side to check for nilability of a sub-message rather than simply use the GetParam() receiver methods, which do the checking for you.

honzap commented 7 months ago

thanks for explanation, I'm closing this topic.