From 1c7462c1baab35e8c0ee50f4db30ee266902a21b Mon Sep 17 00:00:00 2001
From: Herbie Ong <herbie@google.com>
Date: Fri, 22 Mar 2019 17:56:55 -0700
Subject: [PATCH] encoding/jsonpb: fix encoding of empty google.protobuf.Value

Description of message Value states:

`Value` represents a dynamically typed value which can be either null, a
number, a string, a boolean, a recursive struct value, or a list of values. A
producer of value is expected to set one of that variants, absence of any
variant indicates an error.

https://github.com/protocolbuffers/protobuf/blob/3.7.x/src/google/protobuf/struct.proto#L57-L60

Previous implementation was following C++ lib behavior.

Change-Id: Id51792e2fc8cc465a05a978e63410d3b6802b522
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/168901
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
---
 encoding/jsonpb/encode.go           |  8 +-------
 encoding/jsonpb/encode_test.go      | 19 ++++++++++---------
 encoding/jsonpb/well_known_types.go | 16 ++--------------
 3 files changed, 13 insertions(+), 30 deletions(-)

diff --git a/encoding/jsonpb/encode.go b/encoding/jsonpb/encode.go
index a9ca5810..9bfadda5 100644
--- a/encoding/jsonpb/encode.go
+++ b/encoding/jsonpb/encode.go
@@ -111,14 +111,8 @@ func (e encoder) marshalFields(m pref.Message) error {
 			continue
 		}
 
-		// An empty google.protobuf.Value should NOT be marshaled out.
-		// Hence need to check ahead for this.
-		val := knownFields.Get(num)
-		if isEmptyKnownValue(val, fd.MessageType()) {
-			continue
-		}
-
 		name := fd.JSONName()
+		val := knownFields.Get(num)
 		if err := e.WriteName(name); !nerr.Merge(err) {
 			return err
 		}
diff --git a/encoding/jsonpb/encode_test.go b/encoding/jsonpb/encode_test.go
index 74dd570b..d6a2e623 100644
--- a/encoding/jsonpb/encode_test.go
+++ b/encoding/jsonpb/encode_test.go
@@ -1008,15 +1008,15 @@ func TestMarshal(t *testing.T) {
 		input: &knownpb.Empty{},
 		want:  `{}`,
 	}, {
-		desc:  "Value empty",
-		input: &knownpb.Value{},
-		want:  ``,
+		desc:    "Value empty",
+		input:   &knownpb.Value{},
+		wantErr: true,
 	}, {
 		desc: "Value empty field",
 		input: &pb2.KnownTypes{
 			OptValue: &knownpb.Value{},
 		},
-		want: `{}`,
+		wantErr: true,
 	}, {
 		desc:  "Value contains NullValue",
 		input: &knownpb.Value{Kind: &knownpb.Value_NullValue{}},
@@ -1531,7 +1531,7 @@ func TestMarshal(t *testing.T) {
 		},
 		input: func() proto.Message {
 			m := &knownpb.Value{}
-			b, err := proto.MarshalOptions{Deterministic: true}.Marshal(m)
+			b, err := proto.Marshal(m)
 			if err != nil {
 				t.Fatalf("error in binary marshaling message for Any.value: %v", err)
 			}
@@ -1540,9 +1540,7 @@ func TestMarshal(t *testing.T) {
 				Value:   b,
 			}
 		}(),
-		want: `{
-  "@type": "type.googleapis.com/google.protobuf.Value"
-}`,
+		wantErr: true,
 	}, {
 		desc: "Any with Duration",
 		mo: jsonpb.MarshalOptions{
@@ -1641,7 +1639,9 @@ func TestMarshal(t *testing.T) {
 					{Kind: &knownpb.Value_ListValue{}},
 				},
 			},
-			OptValue: &knownpb.Value{},
+			OptValue: &knownpb.Value{
+				Kind: &knownpb.Value_StringValue{"world"},
+			},
 			OptEmpty: &knownpb.Empty{},
 			OptAny: &knownpb.Any{
 				TypeUrl: "google.protobuf.Empty",
@@ -1671,6 +1671,7 @@ func TestMarshal(t *testing.T) {
     {},
     []
   ],
+  "optValue": "world",
   "optEmpty": {},
   "optAny": {
     "@type": "google.protobuf.Empty"
diff --git a/encoding/jsonpb/well_known_types.go b/encoding/jsonpb/well_known_types.go
index a9efdd30..101e4ba5 100644
--- a/encoding/jsonpb/well_known_types.go
+++ b/encoding/jsonpb/well_known_types.go
@@ -128,10 +128,6 @@ func (e encoder) marshalAny(m pref.Message) error {
 	// with corresponding custom JSON encoding of the embedded message as a
 	// field.
 	if isCustomType(emt.FullName()) {
-		// An empty google.protobuf.Value should NOT be marshaled out.
-		if isEmptyKnownValue(pref.ValueOf(em), emt) {
-			return nil
-		}
 		e.WriteName("value")
 		return e.marshalCustomType(em)
 	}
@@ -176,14 +172,6 @@ func (e encoder) marshalListValue(m pref.Message) error {
 	return e.marshalList(val.List(), fd)
 }
 
-// isEmptyKnownValue returns true if given val is of type google.protobuf.Value
-// and does not have any of its oneof fields set.
-func isEmptyKnownValue(val pref.Value, md pref.MessageDescriptor) bool {
-	return md != nil &&
-		md.FullName() == "google.protobuf.Value" &&
-		val.Message().KnownFields().Len() == 0
-}
-
 func (e encoder) marshalKnownValue(m pref.Message) error {
 	msgType := m.Type()
 	fieldDescs := msgType.Oneofs().Get(0).Fields()
@@ -200,8 +188,8 @@ func (e encoder) marshalKnownValue(m pref.Message) error {
 		return e.marshalSingular(val, fd)
 	}
 
-	// None of the fields are set.
-	return nil
+	// Return error if none of the fields are set.
+	return errors.New("%s: none of the variants is set", msgType.FullName())
 }
 
 const (