fix marshaling of any types to JSON when contained object has custom marshaling #361

jhump · 2017-05-26T16:55:34Z

This should address #360

I included support for the custom message type to marshal itself into JSON types other than object. So it's a little more flexible than the solution @cybrcodr suggested in #360.

Also fixed a potential nil-dereference panic if an Any type was unmarshaled from bad JSON that had a null type:

{ "@type": null, "key": "value" }

…marshaling/unmarshaling

jhump · 2017-05-26T17:00:32Z

jsonpb/jsonpb.go

+// returns true, the JSON representation will not be an object (for example,
+// the message marshals itself as a string).
+type customJSON interface {
+	XXX_CustomJSON() bool


I suspect there will be opinions on what this method should be named, and possibly even whether this should even be supported (custom message type that can marshal itself into custom JSON).

The main case I can think of is a dynamic message that happens to have a well-known type as a nested field. In that case, depending on implementation, it may be possible that the well-known type is actually represented via a dynamic message vs. the generated message type. Perhaps there are others. Possibly worth pointing out that the doc for Any just talks about messages with custom JSON and doesn't specifically restrict this to only well-known types.

Yeah, the doc doesn't explain in good detail about what it means for a "value that has special JSON mapping" in order to serialize into the form {"@type": xxx, "value": yyy}. I'd like to take it that "special JSON mapping" refers to WKTs and that is already handled in marshalAny. I wish that all Any marshals into that form.

I'd like to think that a dynamic message should marshal into JSON object. This would keep it simpler. This means that if it did not marshal into a JSON object, we can error out for now. And if it did, then it would be easier to just add in "@type" key-value.

I've posted this question to [email protected]. Not sure if we'll hear back soon. But let me know what you think of my suggestion above for keeping the assumptions simpler for now.

I did see your question a few moments ago.

Sure, I can cut this stuff out for now. It's not terribly complicated to add back later if/when a more concrete need arises.

cybrcodr

Thanks for sending out a fix. Let me know what you think of my comments.

cybrcodr · 2017-05-26T18:12:36Z

jsonpb/jsonpb.go

@@ -633,13 +668,13 @@ func (u *Unmarshaler) unmarshalValue(target reflect.Value, inputValue json.RawMe
 			}

 			val, ok := jsonFields["@type"]
-			if !ok {
+			if !ok || val == nil {


Thanks for catching this. My mistake, I missed this one when I changed jsonFields to use map[string]*RawMessage.

Yeah, I had actually made the same change locally in my branch, in order for my tests to pass. After I rebased to latest master and reconciled with your fix, I figured I'd leave this change in :)

cybrcodr · 2017-05-26T18:13:18Z

jsonpb/jsonpb.go

 				return errors.New("Any JSON doesn't have '@type'")
 			}

 			var turl string
 			if err := json.Unmarshal([]byte(*val), &turl); err != nil {
-				return fmt.Errorf("can't unmarshal Any's '@type': %q", val)
+				return fmt.Errorf("can't unmarshal Any's '@type': %q", *val)
 			}
 			target.Field(0).SetString(turl)


Just realized that we should also check if turl is empty.

I think it's probably okay without such a check. What will happen is that this method will end up returning an error like unknown message type "", which seems reasonable that it's saying the empty string is not valid as the type URL.

cybrcodr · 2017-05-26T18:19:29Z

jsonpb/jsonpb.go

@@ -441,9 +482,6 @@ func (m *Marshaler) marshalValue(out *errWriter, prop *proto.Properties, v refle

 	// Handle well-known types.
 	// Most are handled up in marshalObject (because 99% are messages).
-	type wkt interface {


Thanks for these cleanups.

cybrcodr · 2017-05-26T19:02:34Z

jsonpb/jsonpb.go

+// returns true, the JSON representation will not be an object (for example,
+// the message marshals itself as a string).
+type customJSON interface {
+	XXX_CustomJSON() bool


Yeah, the doc doesn't explain in good detail about what it means for a "value that has special JSON mapping" in order to serialize into the form {"@type": xxx, "value": yyy}. I'd like to take it that "special JSON mapping" refers to WKTs and that is already handled in marshalAny. I wish that all Any marshals into that form.

I'd like to think that a dynamic message should marshal into JSON object. This would keep it simpler. This means that if it did not marshal into a JSON object, we can error out for now. And if it did, then it would be easier to just add in "@type" key-value.

I've posted this question to [email protected]. Not sure if we'll hear back soon. But let me know what you think of my suggestion above for keeping the assumptions simpler for now.

cybrcodr · 2017-05-26T21:29:27Z

jsonpb/jsonpb.go

+			if err = json.Unmarshal(b, &js); err != nil {
+				return fmt.Errorf("Type %v produced invalid JSON: %v", reflect.TypeOf(v), err)
+			}
+			m, ok := js.(map[string]interface{});


Use map[string]*json.RawMessage instead?

If I cut out the code that supports non-object JSON types, then, yes, I can unmarshal into that type (which will certainly be more efficient to parse and then to re-marshal below).

The current formulation is because I didn't know apriori whether the JSON produced will be an object or something else. So I have to supply an interface{} to json.Unmarshal, in which case the type of the value will be map[string]interface{} if it was an object/hash.

cybrcodr · 2017-05-26T21:31:04Z

jsonpb/jsonpb.go

+			if ok {
+				m["@type"] = typeURL
+			} else {
+				if cj, ok := v.(customJSON); !ok || !cj.XXX_CustomJSON() {


As mentioned above, I'm not sure if this is necessary. I'd suggest error'ing here for now. But if you think this is something needed, then I think you can simply take the RawMessage and set it to "value", right?

As mentioned above, I'm not sure if this is necessary. I'd suggest error'ing here for now.

Okay, I'll tweak this branch to remove the customJson stuff and push changes.

But if you think this is something needed, then I think you can simply take the RawMessage and set it to "value", right?

I think that's what the code is doing below. But perhaps I'm not following this remark. (Though it will be moot after removing the customJson stuff.)

Agree. Remark is moot when going with the assumption that a dynamic message should produce a JSON object. And we should go with that.

Sorry for lack of explanation on my end. My previous comment was if it did not produce a JSON object, I'd assume that it should still produce a valid JSON value and hence you can simply set the bytes output from MarshalJSONPB to "value".

Ah, I see what you meant. So, were I to have kept this code, basically change this:

m["value"] = js

to this:

m["value"] = (*json.RawMessage)(&b)

Anyhow, removed all of this code; but, now that I understand the suggestion, yes, that would have been a better way to do it! :)

cybrcodr

Sounds good.

cybrcodr · 2017-05-26T22:02:36Z

jsonpb/jsonpb.go

+			if ok {
+				m["@type"] = typeURL
+			} else {
+				if cj, ok := v.(customJSON); !ok || !cj.XXX_CustomJSON() {


Agree. Remark is moot when going with the assumption that a dynamic message should produce a JSON object. And we should go with that.

Sorry for lack of explanation on my end. My previous comment was if it did not produce a JSON object, I'd assume that it should still produce a valid JSON value and hence you can simply set the bytes output from MarshalJSONPB to "value".

… custom marshaling

jhump · 2017-05-27T01:19:37Z

@cybrcodr I removed the support for "custom JSON" (e.g. non-JSON-object output) and addressed your other comments. PTAL

cybrcodr

Looks good. Sorry for the late response, holiday and been busy today with other stuff.

All minor comments.

cybrcodr · 2017-05-31T00:15:42Z

jsonpb/jsonpb.go

@@ -660,7 +670,7 @@ func (u *Unmarshaler) unmarshalValue(target reflect.Value, inputValue json.RawMe
 				}

 				if err := u.unmarshalValue(reflect.ValueOf(m).Elem(), *val, nil); err != nil {
-					return fmt.Errorf("can't unmarshal Any's WKT: %v", err)
+					return fmt.Errorf("can't unmarshal Any nested proto %v: %v", reflect.TypeOf(m), err)


Minor. Use %T instead of reflect.TypeOf.

cybrcodr · 2017-05-31T00:18:33Z

jsonpb/jsonpb_test.go

+
+const (
+	dynamicMessageName = "google.protobuf.jsonpb.testing.dynamicMessage"
+	dynamicMessageCustomJsonName = "google.protobuf.jsonpb.testing.dynamicMessageCustomJson"


Not used. Remove?

Oops. Didn't mean to leave that in there. Now gone.

cybrcodr · 2017-05-31T00:30:43Z

jsonpb/jsonpb_test.go

@@ -454,6 +456,24 @@ func TestMarshalingWithJSONPBMarshaler(t *testing.T) {
 	}
 }

+func TestMarshalToAnyWithJSONPBMarshaler(t *testing.T) {


Minor. TestMarshalAnyWithJSONPBMarshaler. I'd remove To/From as Any can pertain to either Go Any or JSON Any.

cybrcodr · 2017-05-31T00:31:45Z

jsonpb/jsonpb_test.go

@@ -442,7 +444,7 @@ func TestMarshaling(t *testing.T) {
 	}
 }

-func TestMarshalingWithJSONPBMarshaler(t *testing.T) {
+func TestMarshalWithJSONPBMarshaler(t *testing.T) {


Minor. TestMarshalJSONPBMarshaler. Same for TestUnmarshalJSONPBMarshaler.

cybrcodr · 2017-05-31T01:01:49Z

jsonpb/jsonpb_test.go

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


How about constructing an expected Any and compared directly using proto.Equal?

d := &dynamicMessage{rawJson: {"baz":[0,1,2,3],"foo":"bar"}}
b, err := proto.Marshal(d)
if err != nil {
...
}
want := &anypb.Any{
TypeURL: "blah.com/+ dynamicMessageName +",
Value: b,
}

if proto.Equal(got, want) ...

cybrcodr · 2017-05-31T01:02:51Z

jsonpb/jsonpb_test.go

-func (m *dynamicMessage) UnmarshalJSONPB(jum *Unmarshaler, json []byte) error {
-	m.rawJson = string(json)
+func (m *dynamicMessage) UnmarshalJSONPB(jum *Unmarshaler, js []byte) error {
+	m.rawJson = string(js)


Optional. Perhaps change rawJson's type to []byte instead of string, then there'll be fewer type casting around.

I looked at doing this, but it would actually result in more casts, not fewer (6 casts instead of just 2).

cybrcodr · 2017-05-31T01:03:44Z

jsonpb/jsonpb_test.go

 	return nil
 }
+
+func (m *dynamicMessage) Marshal() ([]byte, error) {


Is this necessary? If field is exported, I think it'll marshal and unmarshal correctly. I'm ok with this, just curious though.

You're right. If I add a properly formatted struct tag to the field, this is no longer necessary. Done.

Ah yes, it's the protobuf tag instead.

… test

jhump · 2017-05-31T01:39:34Z

@cybrcodr, hopefully this is ready to merge now.

cybrcodr

Thanks!

cybrcodr · 2017-05-31T02:18:23Z

jsonpb/jsonpb_test.go

 	return nil
 }
+
+func (m *dynamicMessage) Marshal() ([]byte, error) {


Ah yes, it's the protobuf tag instead.

fix marshaling of any types to JSON when contained object has custom …

03a0232

…marshaling/unmarshaling

jhump commented May 26, 2017

View reviewed changes

cybrcodr reviewed May 26, 2017

View reviewed changes

address review feedback; remove support for non-object JSON type from…

7f15c93

… custom marshaling

cybrcodr approved these changes May 31, 2017

View reviewed changes

address review comments; use struct tags instead of custom marshal in…

b74d3db

… test

cybrcodr approved these changes May 31, 2017

View reviewed changes

cybrcodr merged commit 6e4cc92 into golang:master May 31, 2017

jhump mentioned this pull request Apr 8, 2019

fix indentation in jsonpb with Any messages #834

Merged

golang locked and limited conversation to collaborators Jun 26, 2020

fix marshaling of any types to JSON when contained object has custom marshaling #361

fix marshaling of any types to JSON when contained object has custom marshaling #361

Conversation

jhump commented May 26, 2017

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

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

Choose a reason for hiding this comment

cybrcodr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhump commented May 27, 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

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

jhump commented May 31, 2017

cybrcodr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment