From 39e9acf532aa9f69a431d5e5fa34c0b1605946a0 Mon Sep 17 00:00:00 2001 From: Akshay Shah Date: Sun, 4 Jun 2023 20:24:05 -0700 Subject: [PATCH 1/2] Restructure JSON codec tests In preparation for changing the logic a bit, restructure the unit tests for the JSON codec to use named subtests and exercise the happy path. --- codec_test.go | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/codec_test.go b/codec_test.go index b27e2562..fcad73fe 100644 --- a/codec_test.go +++ b/codec_test.go @@ -100,13 +100,22 @@ func TestStableCodec(t *testing.T) { func TestJSONCodec(t *testing.T) { t.Parallel() - var empty emptypb.Empty codec := &protoJSONCodec{name: "json"} - err := codec.Unmarshal([]byte{}, &empty) - assert.NotNil(t, err) - assert.True( - t, - strings.Contains(err.Error(), "valid JSON"), - assert.Sprintf(`error message should explain that "" is not a valid JSON object`), - ) + + t.Run("success", func(t *testing.T) { + t.Parallel() + err := codec.Unmarshal([]byte("{}"), &emptypb.Empty{}) + assert.Nil(t, err) + }) + + t.Run("empty string", func(t *testing.T) { + t.Parallel() + err := codec.Unmarshal([]byte{}, &emptypb.Empty{}) + assert.NotNil(t, err) + assert.True( + t, + strings.Contains(err.Error(), "valid JSON"), + assert.Sprintf(`error message should explain that "" is not a valid JSON object`), + ) + }) } From 128a8967ce645721a14f72d7f4b251901cb758e2 Mon Sep 17 00:00:00 2001 From: Akshay Shah Date: Sun, 4 Jun 2023 20:25:05 -0700 Subject: [PATCH 2/2] Discard unknown JSON fields by default The current JSON unmarshaling logic tightly couples clients and servers: clients can't unmarshal messages containing new fields, and servers can't unmarshal messages containing ancient fields that have since been deleted. This is counter to the typical protobuf forward- and backward-compatibility story, and is far more restrictive than the binary codec. This PR loosens the default behavior to discard unknown fields. Fixes #477. --- codec.go | 4 +++- codec_test.go | 6 ++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/codec.go b/codec.go index bc0fe989..fab38f0c 100644 --- a/codec.go +++ b/codec.go @@ -144,7 +144,9 @@ func (c *protoJSONCodec) Unmarshal(binary []byte, message any) error { if len(binary) == 0 { return errors.New("zero-length payload is not a valid JSON object") } - var options protojson.UnmarshalOptions + // Discard unknown fields so clients and servers aren't forced to always use + // exactly the same version of the schema. + options := protojson.UnmarshalOptions{DiscardUnknown: true} return options.Unmarshal(binary, protoMessage) } diff --git a/codec_test.go b/codec_test.go index fcad73fe..56f78bf3 100644 --- a/codec_test.go +++ b/codec_test.go @@ -108,6 +108,12 @@ func TestJSONCodec(t *testing.T) { assert.Nil(t, err) }) + t.Run("unknown fields", func(t *testing.T) { + t.Parallel() + err := codec.Unmarshal([]byte(`{"foo": "bar"}`), &emptypb.Empty{}) + assert.Nil(t, err) + }) + t.Run("empty string", func(t *testing.T) { t.Parallel() err := codec.Unmarshal([]byte{}, &emptypb.Empty{})