Unmarshal a value which implements JSONPBUnmarshaler #429

vaporz · 2017-09-18T10:59:04Z

Hi, our test cases are failed by #394 .
We defined a struct which implements interface JSONPBUnmarshaler, and use it as a field in another message, for example:

message MyString {
  bool null = 1;
  string value = 2;
}
message Request {
  MyString name = 1;
}

MyString implements JSONPBUnmarshaler .

Before #394 , everything works fine.
After #394 , if input json is {"name": null}, func UnmarshalJSONPB on MyString is not executed. Because it will return earlier:

if string(inputValue) == "null" && targetType != reflect.TypeOf(&stpb.Value{}) {
	return nil   // <---returned here
}

cybrcodr

Can you also add a unit test for this? Thanks.

cybrcodr · 2017-09-18T21:53:41Z

jsonpb/jsonpb.go

@@ -673,7 +673,8 @@ func (u *Unmarshaler) unmarshalValue(target reflect.Value, inputValue json.RawMe
 	if targetType.Kind() == reflect.Ptr {
 		// If input value is "null" and target is a pointer type, then the field should be treated as not set
 		// UNLESS the target is structpb.Value, in which case it should be set to structpb.NullValue.
-		if string(inputValue) == "null" && targetType != reflect.TypeOf(&stpb.Value{}) {
+		_, ok := target.Interface().(JSONPBUnmarshaler)


Minor naming suggestion. s/ok/isJSONPBUnmarshaler/

cybrcodr · 2017-09-18T22:11:53Z

Curious, how do you make MyString implement JSONPBUnmarshaler? Do you hand modify the generated pb.go file or add another file under the same generated package?

vaporz · 2017-09-19T00:01:17Z

Hi, yes, we add another file under the same generated package.

vaporz · 2017-09-19T05:34:34Z

Hi, @cybrcodr , renamed "ok" to "isJSONPBUnmarshaler", and added a test.

cybrcodr

Thanks. Minor suggestions.

cybrcodr · 2017-09-19T18:44:53Z

jsonpb/jsonpb_test.go

@@ -789,6 +789,18 @@ func TestUnmarshalJSONPBUnmarshaler(t *testing.T) {
 	if msg.rawJson != rawJson {
 		t.Errorf("message contents not set correctly after unmarshalling JSON: got %s, wanted %s", msg.rawJson, rawJson)
 	}
+


Separate this out to a different Test function to make it easier to identify. Suggest naming TestUnmarshalNullWithJSONPBUnmarshaler.

cybrcodr · 2017-09-19T18:47:10Z

jsonpb/jsonpb_test.go

+	rawJson = `{"stringField":null}`
+	var ptrFieldMsg ptrFieldMessage
+	if err := Unmarshal(strings.NewReader(rawJson), &ptrFieldMsg); err != nil {
+		t.Errorf("an unexpected error occurred when parsing into JSONPBUnmarshaler: %v", err)


Simplify the error message -- "unmarshal error: %v", err

cybrcodr · 2017-09-19T20:35:29Z

jsonpb/jsonpb_test.go

+	if err := Unmarshal(strings.NewReader(rawJson), &ptrFieldMsg); err != nil {
+		t.Errorf("an unexpected error occurred when parsing into JSONPBUnmarshaler: %v", err)
+	}
+	if ptrFieldMsg.StringField == nil {


May be more precise to compare with expected value. Use proto.Equal to compare.

Error message can be something like "unmarshal result StringField: got %v, want %v"

cybrcodr · 2017-09-19T20:36:52Z

jsonpb/jsonpb_test.go

+	if ptrFieldMsg.StringField == nil {
+		t.Errorf("ptr field which implements JSONPBUnmarshaler is nil after unmarshalling with JSON {\"stringField\":null}:")
+	}
+	if ptrFieldMsg.StringField != nil && ptrFieldMsg.StringField.IsSet == false {


Not sure if you need the IsSet field. But if you really want to keep it, the validation here should not need to include check for StringField != nil, i.e. it should be independent.

Removed this validation, "proto.Equal" is enough, but "IsSet" is not removed, I'd like to use it to make sure func (s *stringField) UnmarshalJSONPB is executed.

cybrcodr · 2017-09-19T20:37:31Z

jsonpb/jsonpb_test.go

+func (s *stringField) ProtoMessage() {
+}
+
+func (s *stringField) MarshalJSONPB(jm *Marshaler) ([]byte, error) {


Optional. This is not necessary since the test is only on JSONPBUnmarshaler.

OK, removed

cybrcodr

Thanks!

cybrcodr self-requested a review September 18, 2017 20:52

cybrcodr reviewed Sep 18, 2017

View reviewed changes

cybrcodr reviewed Sep 19, 2017

View reviewed changes

unmarshal a value which implements JSONPBUnmarshaler

1728256

cybrcodr approved these changes Sep 20, 2017

View reviewed changes

cybrcodr merged commit 130e6b0 into golang:master Sep 20, 2017

golang locked and limited conversation to collaborators Jun 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unmarshal a value which implements JSONPBUnmarshaler #429

Unmarshal a value which implements JSONPBUnmarshaler #429

vaporz commented Sep 18, 2017 •

edited

Loading

cybrcodr left a comment

cybrcodr Sep 18, 2017

vaporz Sep 19, 2017

cybrcodr commented Sep 18, 2017

vaporz commented Sep 19, 2017

vaporz commented Sep 19, 2017

cybrcodr left a comment

cybrcodr Sep 19, 2017

vaporz Sep 20, 2017

cybrcodr Sep 19, 2017

vaporz Sep 20, 2017

cybrcodr Sep 19, 2017

vaporz Sep 20, 2017

cybrcodr Sep 19, 2017

vaporz Sep 20, 2017

cybrcodr Sep 19, 2017

vaporz Sep 20, 2017

cybrcodr left a comment

Unmarshal a value which implements JSONPBUnmarshaler #429

Unmarshal a value which implements JSONPBUnmarshaler #429

Conversation

vaporz commented Sep 18, 2017 • edited Loading

cybrcodr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cybrcodr commented Sep 18, 2017

vaporz commented Sep 19, 2017

vaporz commented Sep 19, 2017

cybrcodr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cybrcodr left a comment

Choose a reason for hiding this comment

vaporz commented Sep 18, 2017 •

edited

Loading