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 2 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 %v produced invalid JSON: %v", reflect.TypeOf(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 %v: %v", reflect.TypeOf(m), err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor. Use %T instead of reflect.TypeOf.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}
} 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 %v: %v", reflect.TypeOf(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 %v into Any.Value: %v", reflect.TypeOf(m), err)
}
target.Field(1).SetBytes(b)

Expand Down
85 changes: 80 additions & 5 deletions jsonpb/jsonpb_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ package jsonpb
import (
"bytes"
"encoding/json"
"fmt"
"io"
"reflect"
"strings"
Expand All @@ -43,6 +44,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 +444,7 @@ func TestMarshaling(t *testing.T) {
}
}

func TestMarshalingWithJSONPBMarshaler(t *testing.T) {
func TestMarshalWithJSONPBMarshaler(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor. TestMarshalJSONPBMarshaler. Same for TestUnmarshalJSONPBMarshaler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

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

func TestMarshalToAnyWithJSONPBMarshaler(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

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 @@ -654,15 +674,42 @@ func TestUnmarshalingBadInput(t *testing.T) {
func TestUnmarshalWithJSONPBUnmarshaler(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 TestUnmarshalFromAnyWithJSONPBUnmarshaler(t *testing.T) {
rawJson := `{ "@type": "blah.com/` + dynamicMessageName + `", "foo": "bar", "baz": [0, 1, 2, 3] }`
var a anypb.Any
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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

if err := ptypes.UnmarshalAny(&a, &msg); err != nil {
t.Errorf("an unexpected error occurred when parsing from Any type: %v", err)
}
dm := msg.Message.(*dynamicMessage)
// when it gets to the custom unmarshal, it's round-tripped through JSON decoding/encoding
// already, so the keys are sorted, whitespace is compacted, and "@type" key has been removed
expected := `{"baz":[0,1,2,3],"foo":"bar"}`
if dm.rawJson != expected {
t.Errorf("message contents not set correctly after unmarshalling JSON: got %s, wanted %s", dm.rawJson, expected)
}
}

const (
dynamicMessageName = "google.protobuf.jsonpb.testing.dynamicMessage"
dynamicMessageCustomJsonName = "google.protobuf.jsonpb.testing.dynamicMessageCustomJson"
Copy link
Contributor

Choose a reason for hiding this comment

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

Not used. Remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

)
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 {
Expand All @@ -684,7 +731,35 @@ 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
}

func (m *dynamicMessage) Marshal() ([]byte, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yes, it's the protobuf tag instead.

var b proto.Buffer
// tag #1, wire type length-delimited
if err := b.EncodeVarint(uint64((1 << 3) | proto.WireBytes)); err != nil {
return nil, err
}
if err := b.EncodeRawBytes([]byte(m.rawJson)); err != nil {
return nil, err
}
return b.Bytes(), nil
}

func (m *dynamicMessage) Unmarshal(b []byte) error {
// really dumb unmarshalling, just for test
buf := proto.NewBuffer(b)
if tagAndWire, err := buf.DecodeVarint(); err != nil {
return err
} else if tagAndWire != uint64((1 << 3) | proto.WireBytes) {
return fmt.Errorf("Unknown tag/wire type: %x", tagAndWire)
}
if bb, err := buf.DecodeRawBytes(true); err != nil {
return err
} else {
m.rawJson = string(bb)
}
return nil
}