Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
May 31, 2017
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
03a0232
fix marshaling of any types to JSON when contained object has custom …
jhump May 26, 2017
7f15c93
address review feedback; remove support for non-object JSON type from…
jhump May 27, 2017
b74d3db
address review comments; use struct tags instead of custom marshal in…
jhump May 31, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 21 additions & 11 deletions jsonpb/jsonpb.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,22 @@ func (m *Marshaler) marshalObject(out *errWriter, v proto.Message, indent, typeU
if err != nil {
return err
}
if typeURL != "" {
// we are marshaling this object to an Any type
var js map[string]*json.RawMessage
if err = json.Unmarshal(b, &js); err != nil {
return fmt.Errorf("type %T produced invalid JSON: %v", v, err)
}
turl, err := json.Marshal(typeURL)
if err != nil {
return fmt.Errorf("failed to marshal type URL %q to JSON: %v", typeURL, err)
}
js["@type"] = (*json.RawMessage)(&turl)
if b, err = json.Marshal(js); err != nil {
return err
}
}

out.write(string(b))
return out.err
}
Expand Down Expand Up @@ -441,9 +457,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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for these cleanups.

XXX_WellKnownType() string
}
if wkt, ok := v.Interface().(wkt); ok {
switch wkt.XXX_WellKnownType() {
case "NullValue":
Expand Down Expand Up @@ -611,9 +624,6 @@ func (u *Unmarshaler) unmarshalValue(target reflect.Value, inputValue json.RawMe
}

// Handle well-known types.
type wkt interface {
XXX_WellKnownType() string
}
if w, ok := target.Addr().Interface().(wkt); ok {
switch w.XXX_WellKnownType() {
case "DoubleValue", "FloatValue", "Int64Value", "UInt64Value",
Expand All @@ -633,13 +643,13 @@ func (u *Unmarshaler) unmarshalValue(target reflect.Value, inputValue json.RawMe
}

val, ok := jsonFields["@type"]
if !ok {
if !ok || val == nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


Expand All @@ -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 %T: %v", m, err)
}
} else {
delete(jsonFields, "@type")
Expand All @@ -670,13 +680,13 @@ func (u *Unmarshaler) unmarshalValue(target reflect.Value, inputValue json.RawMe
}

if err = u.unmarshalValue(reflect.ValueOf(m).Elem(), nestedProto, nil); err != nil {
return fmt.Errorf("can't unmarshal nested Any proto: %v", err)
return fmt.Errorf("can't unmarshal Any nested proto %T: %v", m, err)
}
}

b, err := proto.Marshal(m)
if err != nil {
return fmt.Errorf("can't marshal proto into Any.Value: %v", err)
return fmt.Errorf("can't marshal proto %T into Any.Value: %v", m, err)
}
target.Field(1).SetBytes(b)

Expand Down
63 changes: 55 additions & 8 deletions jsonpb/jsonpb_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import (

pb "github.com/golang/protobuf/jsonpb/jsonpb_test_proto"
proto3pb "github.com/golang/protobuf/proto/proto3_proto"
"github.com/golang/protobuf/ptypes"
anypb "github.com/golang/protobuf/ptypes/any"
durpb "github.com/golang/protobuf/ptypes/duration"
stpb "github.com/golang/protobuf/ptypes/struct"
Expand Down Expand Up @@ -442,7 +443,7 @@ func TestMarshaling(t *testing.T) {
}
}

func TestMarshalingWithJSONPBMarshaler(t *testing.T) {
func TestMarshalJSONPBMarshaler(t *testing.T) {
rawJson := `{ "foo": "bar", "baz": [0, 1, 2, 3] }`
msg := dynamicMessage{rawJson: rawJson}
str, err := new(Marshaler).MarshalToString(&msg)
Expand All @@ -454,6 +455,24 @@ func TestMarshalingWithJSONPBMarshaler(t *testing.T) {
}
}

func TestMarshalAnyJSONPBMarshaler(t *testing.T) {
msg := dynamicMessage{rawJson: `{ "foo": "bar", "baz": [0, 1, 2, 3] }`}
a, err := ptypes.MarshalAny(&msg)
if err != nil {
t.Errorf("an unexpected error occurred when marshalling to Any: %v", err)
}
str, err := new(Marshaler).MarshalToString(a)
if err != nil {
t.Errorf("an unexpected error occurred when marshalling Any to JSON: %v", err)
}
// after custom marshaling, it's round-tripped through JSON decoding/encoding already,
// so the keys are sorted, whitespace is compacted, and "@type" key has been added
expected := `{"@type":"type.googleapis.com/` + dynamicMessageName +`","baz":[0,1,2,3],"foo":"bar"}`
if str != expected {
t.Errorf("marshalling JSON produced incorrect output: got %s, wanted %s", str, expected)
}
}

var unmarshalingTests = []struct {
desc string
unmarshaler Unmarshaler
Expand Down Expand Up @@ -651,22 +670,50 @@ func TestUnmarshalingBadInput(t *testing.T) {
}
}

func TestUnmarshalWithJSONPBUnmarshaler(t *testing.T) {
func TestUnmarshalJSONPBUnmarshaler(t *testing.T) {
rawJson := `{ "foo": "bar", "baz": [0, 1, 2, 3] }`
var msg dynamicMessage
err := Unmarshal(strings.NewReader(rawJson), &msg)
if err != nil {
if err := Unmarshal(strings.NewReader(rawJson), &msg); err != nil {
t.Errorf("an unexpected error occurred when parsing into JSONPBUnmarshaler: %v", err)
}
if msg.rawJson != rawJson {
t.Errorf("message contents not set correctly after unmarshalling JSON: got %s, wanted %s", msg.rawJson, rawJson)
}
}

func TestUnmarshalAnyJSONPBUnmarshaler(t *testing.T) {
rawJson := `{ "@type": "blah.com/` + dynamicMessageName + `", "foo": "bar", "baz": [0, 1, 2, 3] }`
var got anypb.Any
if err := Unmarshal(strings.NewReader(rawJson), &got); err != nil {
t.Errorf("an unexpected error occurred when parsing into JSONPBUnmarshaler: %v", err)
}

dm := &dynamicMessage{rawJson: `{"baz":[0,1,2,3],"foo":"bar"}`}
var want anypb.Any
if b, err := proto.Marshal(dm); err != nil {
t.Errorf("an unexpected error occurred when marshaling message: %v", err)
} else {
want.TypeUrl = "blah.com/" + dynamicMessageName
want.Value = b
}

if !proto.Equal(&got, &want) {
t.Errorf("message contents not set correctly after unmarshalling JSON: got %s, wanted %s", got, want)
}
}

const (
dynamicMessageName = "google.protobuf.jsonpb.testing.dynamicMessage"
)
func init() {
// we register the custom type below so that we can use it in Any types
proto.RegisterType((*dynamicMessage)(nil), dynamicMessageName)
}

// dynamicMessage implements protobuf.Message but is not a normal generated message type.
// It provides implementations of JSONPBMarshaler and JSONPBUnmarshaler for JSON support.
type dynamicMessage struct {
rawJson string
rawJson string `protobuf:"bytes,1,opt,name=rawJson"`
}

func (m *dynamicMessage) Reset() {
Expand All @@ -684,7 +731,7 @@ func (m *dynamicMessage) MarshalJSONPB(jm *Marshaler) ([]byte, error) {
return []byte(m.rawJson), nil
}

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return nil
}
}