proto: never return nil []byte from Marshal when successful
It is a sufficiently common pattern to do something like the following:
m.Proto2BytesField, ... = proto.Marshal(m2)
where the user is relying on Marshal to never return a nil byte slice,
otherwise it subtly changes the semantics of how the generated API
handles whether a proto2 "optional bytes" fields is populated or not.
Change-Id: Ie7508dbcdcc5f3295885609a91907c6eb4f04c1e
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/228838
Reviewed-by: Damien Neil <dneil@google.com>
diff --git a/proto/encode.go b/proto/encode.go
index 862c38b..456bfda 100644
--- a/proto/encode.go
+++ b/proto/encode.go
@@ -80,6 +80,9 @@
}
out, err := MarshalOptions{}.marshal(nil, m.ProtoReflect())
+ if len(out.Buf) == 0 && err == nil {
+ out.Buf = emptyBytesForMessage(m)
+ }
return out.Buf, err
}
@@ -91,9 +94,26 @@
}
out, err := o.marshal(nil, m.ProtoReflect())
+ if len(out.Buf) == 0 && err == nil {
+ out.Buf = emptyBytesForMessage(m)
+ }
return out.Buf, err
}
+// emptyBytesForMessage returns a nil buffer if and only if m is invalid,
+// otherwise it returns a non-nil empty buffer.
+//
+// This is to assist the edge-case where user-code does the following:
+// m1.OptionalBytes, _ = proto.Marshal(m2)
+// where they expect the proto2 "optional_bytes" field to be populated
+// if any only if m2 is a valid message.
+func emptyBytesForMessage(m Message) []byte {
+ if m == nil || !m.ProtoReflect().IsValid() {
+ return nil
+ }
+ return emptyBuf[:]
+}
+
// MarshalAppend appends the wire-format encoding of m to b,
// returning the result.
func (o MarshalOptions) MarshalAppend(b []byte, m Message) ([]byte, error) {
diff --git a/proto/encode_test.go b/proto/encode_test.go
index d8aabd9..1cdbabc 100644
--- a/proto/encode_test.go
+++ b/proto/encode_test.go
@@ -247,3 +247,28 @@
t.Errorf("after round-trip marshal, got len(m.OptionalBytes) = %v, want %v", got, want)
}
}
+
+// TestEncodeEmpty tests for boundary conditions when producing an empty output.
+// These tests are not necessarily a statement of proper behavior,
+// but exist to detect accidental changes in behavior.
+func TestEncodeEmpty(t *testing.T) {
+ for _, m := range []proto.Message{nil, (*testpb.TestAllTypes)(nil), &testpb.TestAllTypes{}} {
+ isValid := m != nil && m.ProtoReflect().IsValid()
+
+ b, err := proto.Marshal(m)
+ if err != nil {
+ t.Errorf("proto.Marshal() = %v", err)
+ }
+ if isNil := b == nil; isNil == isValid {
+ t.Errorf("proto.Marshal() == nil: %v, want %v", isNil, !isValid)
+ }
+
+ b, err = proto.MarshalOptions{}.Marshal(m)
+ if err != nil {
+ t.Errorf("proto.MarshalOptions{}.Marshal() = %v", err)
+ }
+ if isNil := b == nil; isNil == isValid {
+ t.Errorf("proto.MarshalOptions{}.Marshal() = %v, want %v", isNil, !isValid)
+ }
+ }
+}
