Allow marshaler to omit nullable optional values

[dim]

Assuming I have a simple struct:

message TestMessage {
  optional int64  i64 = 1 [(gogoproto.nullable) = false];
  optional uint64 u64 = 2 [(gogoproto.nullable) = false];
  optional int32  i32 = 3 [(gogoproto.nullable) = false];
  optional uint32 u32 = 4 [(gogoproto.nullable) = false];
  optional double f64 = 5 [(gogoproto.nullable) = false];
  optional string str = 6 [(gogoproto.nullable) = false];
  optional bool   bol = 7 [(gogoproto.nullable) = false];
}

Which then results in:

type TestMessage struct {
  I64 int64   `protobuf:"varint,1,opt,name=i64" json:"i64"`
  U64 uint64  `protobuf:"varint,2,opt,name=u64" json:"u64"`
  I32 int32   `protobuf:"varint,3,opt,name=i32" json:"i32"`
  U32 uint32  `protobuf:"varint,4,opt,name=u32" json:"u32"`
  F64 float64 `protobuf:"fixed64,5,opt,name=f64" json:"f64"`
  Str string  `protobuf:"bytes,6,opt,name=str" json:"str"`
  Bol bool    `protobuf:"varint,7,opt,name=bol" json:"bol"`
}

func (m *TestMessage) Reset()      { *m = TestMessage{} }
func (*TestMessage) ProtoMessage() {}

As you can see, all attributes are optional and non-nullable. If I then marshal the struct, I get something like:

pb, _ := proto.Marshal(&TestMessage{})
fmt.Println(pb)

var msg TestMessage
if proto.Unmarshal(pb, &msg) == nil {
  fmt.Println(msg)
}
// Output: 
// [8 0 16 0 24 0 32 0 41 0 0 0 0 0 0 0 0 50 0]
// {0 0 0 0 0  false}  

This is obviously quite a waste and not in line with how Go works. I understand that protobuf generated by Go messages must be interoperable but it would be nice to have the option to omit zero values. I have applied a manual patch below for now, but is this a feature you would generally consider?

Here's the output with the patch:

// []
// {0 0 0 0 0  false}  

Here's the patched code:

diff --git a/proto/encode_gogo.go b/proto/encode_gogo.go
index f77cfb1..aa99d86 100644
--- a/proto/encode_gogo.go
+++ b/proto/encode_gogo.go
@@ -36,9 +36,7 @@

 package proto

-import (
-       "reflect"
-)
+import "reflect"

 func NewRequiredNotSetError(field string) *RequiredNotSetError {
        return &RequiredNotSetError{field}
@@ -69,6 +67,9 @@ func size_ext_slice_byte(p *Properties, base structPointer) (n int) {
 // Encode a reference to bool pointer.
 func (o *Buffer) enc_ref_bool(p *Properties, base structPointer) error {
        v := *structPointer_BoolVal(base, p.field)
+       if !v && p.Optional {
+               return ErrNil
+       }
        x := 0
        if v {
                x = 1
@@ -79,6 +80,10 @@ func (o *Buffer) enc_ref_bool(p *Properties, base structPointer) error {
 }

 func size_ref_bool(p *Properties, base structPointer) int {
+       v := *structPointer_BoolVal(base, p.field)
+       if !v && p.Optional {
+               return 0
+       }
        return len(p.tagcode) + 1 // each bool takes exactly one byte
 }

@@ -86,6 +91,9 @@ func size_ref_bool(p *Properties, base structPointer) int {
 func (o *Buffer) enc_ref_int32(p *Properties, base structPointer) error {
        v := structPointer_Word32Val(base, p.field)
        x := int32(word32Val_Get(v))
+       if x == 0 && p.Optional {
+               return ErrNil
+       }
        o.buf = append(o.buf, p.tagcode...)
        p.valEnc(o, uint64(x))
        return nil
@@ -94,6 +102,9 @@ func (o *Buffer) enc_ref_int32(p *Properties, base structPointer) error {
 func size_ref_int32(p *Properties, base structPointer) (n int) {
        v := structPointer_Word32Val(base, p.field)
        x := int32(word32Val_Get(v))
+       if x == 0 && p.Optional {
+               return
+       }
        n += len(p.tagcode)
        n += p.valSize(uint64(x))
        return
@@ -102,6 +113,9 @@ func size_ref_int32(p *Properties, base structPointer) (n int) {
 func (o *Buffer) enc_ref_uint32(p *Properties, base structPointer) error {
        v := structPointer_Word32Val(base, p.field)
        x := word32Val_Get(v)
+       if x == 0 && p.Optional {
+               return ErrNil
+       }
        o.buf = append(o.buf, p.tagcode...)
        p.valEnc(o, uint64(x))
        return nil
@@ -110,6 +124,9 @@ func (o *Buffer) enc_ref_uint32(p *Properties, base structPointer) error {
 func size_ref_uint32(p *Properties, base structPointer) (n int) {
        v := structPointer_Word32Val(base, p.field)
        x := word32Val_Get(v)
+       if x == 0 && p.Optional {
+               return
+       }
        n += len(p.tagcode)
        n += p.valSize(uint64(x))
        return
@@ -119,6 +136,10 @@ func size_ref_uint32(p *Properties, base structPointer) (n int) {
 func (o *Buffer) enc_ref_int64(p *Properties, base structPointer) error {
        v := structPointer_Word64Val(base, p.field)
        x := word64Val_Get(v)
+       if x == 0 && p.Optional {
+               return ErrNil
+       }
+
        o.buf = append(o.buf, p.tagcode...)
        p.valEnc(o, x)
        return nil
@@ -127,6 +148,10 @@ func (o *Buffer) enc_ref_int64(p *Properties, base structPointer) error {
 func size_ref_int64(p *Properties, base structPointer) (n int) {
        v := structPointer_Word64Val(base, p.field)
        x := word64Val_Get(v)
+       if x == 0 && p.Optional {
+               return
+       }
+
        n += len(p.tagcode)
        n += p.valSize(x)
        return
@@ -135,6 +160,10 @@ func size_ref_int64(p *Properties, base structPointer) (n int) {
 // Encode a reference to a string pointer.
 func (o *Buffer) enc_ref_string(p *Properties, base structPointer) error {
        v := *structPointer_StringVal(base, p.field)
+       if v == "" && p.Optional {
+               return ErrNil
+       }
+
        o.buf = append(o.buf, p.tagcode...)
        o.EncodeStringBytes(v)
        return nil
@@ -142,6 +171,9 @@ func (o *Buffer) enc_ref_string(p *Properties, base structPointer) error {

 func size_ref_string(p *Properties, base structPointer) (n int) {
        v := *structPointer_StringVal(base, p.field)
+       if v == "" && p.Optional {
+               return
+       }
        n += len(p.tagcode)
        n += sizeStringBytes(v)
        return

[awalterschulze]

I suspect your patch will break some use cases.
If you use proto3 its optional fields are not nullable and default values are not marshaled.
So maybe that is what you are looking for?

[dim]

Oh, sorry, missed that proto3 feature. This is actually perfect, thanks!

[dim]

So I think there is still a bug here, let me demonstrate a use case.

Here's the minimal proto:

syntax = "proto2";

import "github.com/gogo/protobuf/gogoproto/gogo.proto";

option (gogoproto.marshaler_all) = true;
option (gogoproto.sizer_all) = true;

message Test {
  optional string data = 1;
}

When I run protoc --gogo_out=. --proto_path=.:vendor test.proto, I get a MarshalTo function that would correctly generate an empty data for an empty &Test{}, as m.Data is optional and nil in our case.

func (m *Test) MarshalTo(data []byte) (int, error) {
    var i int
    _ = i
    var l int
    _ = l
    if m.Data != nil {
        data[i] = 0xa
        i++
        i = encodeVarintTest(data, i, uint64(len(*m.Data)))
        i += copy(data[i:], *m.Data)
    }
    if m.XXX_unrecognized != nil {
        i += copy(data[i:], m.XXX_unrecognized)
    }
    return i, nil
}

When I repeat the same process but with protoc --gogofaster_out=. --proto_path=.:vendor test.proto, I get a MarshalTo function which always includes empty values:

func (m *Test) MarshalTo(data []byte) (int, error) {
    var i int
    _ = i
    var l int
    _ = l
    data[i] = 0xa
    i++
    i = encodeVarintTest(data, i, uint64(len(m.Data)))
    i += copy(data[i:], m.Data)
    return i, nil
}

In my opinion the output should be the same, no matter if I chose standard of faster mode. In this particular case, I also cannot just switch to proto3 as this is a third-party protocol.

Ideas?

[awalterschulze]

gogofast_out / gofast_out has no side effects.

gogofaster_out makes most fields nullable=false.

In the docs of gogoprotobuf

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.