protobuf icon indicating copy to clipboard operation
protobuf copied to clipboard

encoding/protojson: marshaled `element in array/map` and `[]byte(nil)` to zero value instead with `null`

Open molon opened this issue 3 years ago • 4 comments

What version of protobuf and what language are you using? Version: v1.28.1

What did you do?

syntax = "proto3";

package test.v1;

import "google/protobuf/wrappers.proto";

message Message {
  string id = 1;
}

message CaseNull {
  bytes b1 = 1;
  bytes b2 = 2;
  repeated bytes rpt_b = 3;
  map<string,bytes> map_b = 4;
  repeated google.protobuf.Int32Value rpt_wkt_i32 = 5; 
  map<string,google.protobuf.Int32Value> map_wkt_i32 = 6;
  repeated Message rpt_msg = 7;
  map<string,Message> map_msg = 8;
}
func TestCaseNull(t *testing.T) {
	m := &testv1.CaseNull{
		B1:   nil, // marshaled to "" instead with null
		B2:   []byte(`abc`),
		RptB: [][]byte{[]byte(`ABC`), nil, []byte(``), []byte(`EFG`)},
		MapB: map[string][]byte{"keyA": nil, "keyB": []byte(`HIJ`)},
		RptWktI32: []*wrapperspb.Int32Value{
			wrapperspb.Int32(-1),
			wrapperspb.Int32(0),
			nil, // marshaled to 0 instead with null
			wrapperspb.Int32(1),
		},
		MapWktI32: map[string]*wrapperspb.Int32Value{
			"a": nil, // marshaled to 0 instead with null
			"b": wrapperspb.Int32(0),
		},
		RptMsg: []*testv1.Message{
			&testv1.Message{Id: "id1"},
			nil, // marshaled to {"id":""} instead with null
			&testv1.Message{Id: "id3"},
		},
		MapMsg: map[string]*testv1.Message{
			"msgA": &testv1.Message{Id: "ida"},
			"msgB": nil, // marshaled to {"id":""} instead with null
			"msgC": &testv1.Message{Id: "idc"},
		},
	}

	jsn, _ := protojson.Marshal(m)
	log.Println(string(jsn))
}

What did you expect to see?

{"b1":"", "b2":"YWJj", "rptB":["QUJD", null, "", "RUZH"], "mapB":{"keyA":null, "keyB":"SElK"}, "rptWktI32":[-1, 0, null, 1], "mapWktI32":{"a":null, "b":0}, "rptMsg":[{"id":"id1"}, null, {"id":"id3"}], "mapMsg":{"msgA":{"id":"ida"}, "msgB":null, "msgC":{"id":"idc"}}}

What did you see instead?

{"b1":"", "b2":"YWJj", "rptB":["QUJD", "", "", "RUZH"], "mapB":{"keyA":"", "keyB":"SElK"}, "rptWktI32":[-1, 0, 0, 1], "mapWktI32":{"a":0, "b":0}, "rptMsg":[{"id":"id1"}, {"id":""}, {"id":"id3"}], "mapMsg":{"msgA":{"id":"ida"}, "msgB":{"id":""}, "msgC":{"id":"idc"}}}

Anything else we should know about your project / environment?

	jsn, _ = json.Marshal(struct {
		B    [][]byte
		Msgs []*testv1.Message
	}{
		B: [][]byte{
			[]byte(nil),
		},
		Msgs: []*testv1.Message{
			nil,
		},
	})
	fmt.Print(string(jsn))
	// Output:
	// {"B":[null],"Msgs":[null]}

protojson can't even unmarshal such json now

	m.Reset()
	err := protojson.Unmarshal([]byte(`{"rptB":["QUJD",null,"","RUZH"]}`), m)
	fmt.Print(err)
	// Output:
	// proto: (line 1:17): invalid value for bytes type: null

molon avatar Sep 17 '22 19:09 molon

This is working as intended. See https://github.com/protocolbuffers/protobuf-go/releases/tag/v1.20.0#v1.20-nil-values for details.

dsnet avatar Sep 19 '22 03:09 dsnet

This is working as intended. See https://github.com/protocolbuffers/protobuf-go/releases/tag/v1.20.0#v1.20-nil-values for details.

Is fuzzy unmarshal acceptable? For example, 64bit integers will be marshaled to string type, but json.Number can still be properly unmarshaled now. Even if the nil value existing in the map/list will be marshaled to a zero value, is it acceptable that json.NullValue can be unmarshaled to a default value?

I understand that the current way is for consistency, marshalA=>unmarshal=>marshalB(expect marshalA==marshalB instead with A==B). But if it's for consistency.

  • Since we can marshal []type{"a",nil,"c"} to ["a","","c"] , should we also accept unmarshal ["a",null,"c"] to []type{"a","","c"}
  • or if rejecting unmarshal ["a",null,"c"] to []type{"a","","c"} , shouldn't that also reject marshal []type{"a",nil,"c"} to ["a","","c"]

molon avatar Sep 19 '22 07:09 molon

Another case:

	&testv1.CaseValue{
		Vs: []*structpb.Value{
			structpb.NewNullValue(),
			nil,
		},
	}

This results in an error google.protobuf.Value: none of the oneof fields is set Shouldn't the zero value of google.protobuf.Value bestructpb.NewNullValue()?

molon avatar Sep 20 '22 02:09 molon

Another case:

	&testv1.CaseValue{
		Vs: []*structpb.Value{
			structpb.NewNullValue(),
			nil,
		},
	}

This results in an error google.protobuf.Value: none of the oneof fields is set Shouldn't the zero value of google.protobuf.Value bestructpb.NewNullValue()?

https://developers.google.com/protocol-buffers/docs/reference/google.protobuf#google.protobuf.Value

A producer of value is expected to set one of that variants, absence of any variant indicates an error.

cybrcodr avatar Sep 20 '22 03:09 cybrcodr