From 61dbc136cf5d2f08d68a011382652244990a53a9 Mon Sep 17 00:00:00 2001 From: Donald Graham Date: Tue, 25 Sep 2018 10:36:12 +0200 Subject: [PATCH] Jsonpb custom type - #411 (#491) * added test for MarshalJSONPB not being called for custom type if it's an alias to a primitive (#411) * removed work-around so test fails * added comment thanking contributor * bumped the go version * regenerated after nil check * regenerated after nil check * #411 - check for UnmarshalJSON interface, regardless of reflect.Kind() --- Makefile | 1 + jsonpb/jsonpb.go | 16 +-- test/issue411/Makefile | 3 + test/issue411/ids.go | 216 +++++++++++++++++++++++++++++++++++ test/issue411/ids_test.go | 69 +++++++++++ test/issue411/issue411.pb.go | 72 ++++++++++++ test/issue411/issue411.proto | 49 ++++++++ 7 files changed, 418 insertions(+), 8 deletions(-) create mode 100644 test/issue411/Makefile create mode 100644 test/issue411/ids.go create mode 100644 test/issue411/ids_test.go create mode 100644 test/issue411/issue411.pb.go create mode 100644 test/issue411/issue411.proto diff --git a/Makefile b/Makefile index bbfed244c2..4f2b9edd37 100644 --- a/Makefile +++ b/Makefile @@ -131,6 +131,7 @@ regenerate: make -C test/issue449 regenerate make -C test/xxxfields regenerate make -C test/issue435 regenerate + make -C test/issue411 regenerate make gofmt diff --git a/jsonpb/jsonpb.go b/jsonpb/jsonpb.go index b047d13e50..83ecb05154 100644 --- a/jsonpb/jsonpb.go +++ b/jsonpb/jsonpb.go @@ -991,16 +991,16 @@ func (u *Unmarshaler) unmarshalValue(target reflect.Value, inputValue json.RawMe return nil } - // Handle nested messages. - if targetType.Kind() == reflect.Struct { - if prop != nil && len(prop.CustomType) > 0 && target.CanAddr() { - if m, ok := target.Addr().Interface().(interface { - UnmarshalJSON([]byte) error - }); ok { - return json.Unmarshal(inputValue, m) - } + if prop != nil && len(prop.CustomType) > 0 && target.CanAddr() { + if m, ok := target.Addr().Interface().(interface { + UnmarshalJSON([]byte) error + }); ok { + return json.Unmarshal(inputValue, m) } + } + // Handle nested messages. + if targetType.Kind() == reflect.Struct { var jsonFields map[string]json.RawMessage if err := json.Unmarshal(inputValue, &jsonFields); err != nil { return err diff --git a/test/issue411/Makefile b/test/issue411/Makefile new file mode 100644 index 0000000000..7e0573b61e --- /dev/null +++ b/test/issue411/Makefile @@ -0,0 +1,3 @@ +regenerate: + go install github.com/gogo/protobuf/protoc-gen-gogo + protoc-min-version --version="3.0.0" --gogo_out=Mgoogle/protobuf/timestamp.proto=github.com/gogo/protobuf/types:. --proto_path=../../../../../:../../protobuf/:. issue411.proto diff --git a/test/issue411/ids.go b/test/issue411/ids.go new file mode 100644 index 0000000000..7045f2ecc8 --- /dev/null +++ b/test/issue411/ids.go @@ -0,0 +1,216 @@ +package issue411 + +import ( + "encoding/base64" + "encoding/binary" + "fmt" + "strconv" +) + +// TraceID is a random 128bit identifier for a trace +type TraceID struct { + Low uint64 `json:"lo"` + High uint64 `json:"hi"` +} + +// SpanID is a random 64bit identifier for a span +type SpanID uint64 + +// ------- TraceID ------- + +// NewTraceID creates a new TraceID from two 64bit unsigned ints. +func NewTraceID(high, low uint64) TraceID { + return TraceID{High: high, Low: low} +} + +func (t TraceID) String() string { + if t.High == 0 { + return fmt.Sprintf("%x", t.Low) + } + return fmt.Sprintf("%x%016x", t.High, t.Low) +} + +// TraceIDFromString creates a TraceID from a hexadecimal string +func TraceIDFromString(s string) (TraceID, error) { + var hi, lo uint64 + var err error + if len(s) > 32 { + return TraceID{}, fmt.Errorf("TraceID cannot be longer than 32 hex characters: %s", s) + } else if len(s) > 16 { + hiLen := len(s) - 16 + if hi, err = strconv.ParseUint(s[0:hiLen], 16, 64); err != nil { + return TraceID{}, err + } + if lo, err = strconv.ParseUint(s[hiLen:], 16, 64); err != nil { + return TraceID{}, err + } + } else { + if lo, err = strconv.ParseUint(s, 16, 64); err != nil { + return TraceID{}, err + } + } + return TraceID{High: hi, Low: lo}, nil +} + +// MarshalText is called by encoding/json, which we do not want people to use. +func (t TraceID) MarshalText() ([]byte, error) { + return nil, fmt.Errorf("unsupported method TraceID.MarshalText; please use github.com/gogo/protobuf/jsonpb for marshalling") +} + +// UnmarshalText is called by encoding/json, which we do not want people to use. +func (t *TraceID) UnmarshalText(text []byte) error { + return fmt.Errorf("unsupported method TraceID.UnmarshalText; please use github.com/gogo/protobuf/jsonpb for marshalling") +} + +// Size returns the size of this datum in protobuf. It is always 16 bytes. +func (t *TraceID) Size() int { + return 16 +} + +// Marshal converts trace ID into a binary representation. Called by protobuf serialization. +func (t TraceID) Marshal() ([]byte, error) { + b := make([]byte, t.Size()) + _, err := t.MarshalTo(b) + return b, err +} + +// MarshalTo converts trace ID into a binary representation. Called by protobuf serialization. +func (t *TraceID) MarshalTo(data []byte) (n int, err error) { + var b [16]byte + binary.BigEndian.PutUint64(b[:8], uint64(t.High)) + binary.BigEndian.PutUint64(b[8:], uint64(t.Low)) + return marshalBytes(data, b[:]) +} + +// Unmarshal inflates this trace ID from binary representation. Called by protobuf serialization. +func (t *TraceID) Unmarshal(data []byte) error { + if len(data) < 16 { + return fmt.Errorf("buffer is too short") + } + t.High = binary.BigEndian.Uint64(data[:8]) + t.Low = binary.BigEndian.Uint64(data[8:]) + return nil +} + +func marshalBytes(dst []byte, src []byte) (n int, err error) { + if len(dst) < len(src) { + return 0, fmt.Errorf("buffer is too short") + } + return copy(dst, src), nil +} + +// MarshalJSON converts trace id into a base64 string enclosed in quotes. +// Used by protobuf JSON serialization. +// Example: {high:2, low:1} => "AAAAAAAAAAIAAAAAAAAAAQ==". +func (t TraceID) MarshalJSON() ([]byte, error) { + var b [16]byte + t.MarshalTo(b[:]) // can only error on incorrect buffer size + s := make([]byte, 24+2) + base64.StdEncoding.Encode(s[1:25], b[:]) + s[0], s[25] = '"', '"' + return s, nil +} + +// UnmarshalJSON inflates trace id from base64 string, possibly enclosed in quotes. +// User by protobuf JSON serialization. +func (t *TraceID) UnmarshalJSON(data []byte) error { + s := string(data) + if l := len(s); l > 2 && s[0] == '"' && s[l-1] == '"' { + s = s[1 : l-1] + } + b, err := base64.StdEncoding.DecodeString(s) + if err != nil { + return fmt.Errorf("cannot unmarshal TraceID from string '%s': %v", string(data), err) + } + return t.Unmarshal(b) +} + +// ------- SpanID ------- + +// NewSpanID creates a new SpanID from a 64bit unsigned int. +func NewSpanID(v uint64) SpanID { + return SpanID(v) +} + +func (s SpanID) String() string { + return fmt.Sprintf("%x", uint64(s)) +} + +// SpanIDFromString creates a SpanID from a hexadecimal string +func SpanIDFromString(s string) (SpanID, error) { + if len(s) > 16 { + return SpanID(0), fmt.Errorf("SpanID cannot be longer than 16 hex characters: %s", s) + } + id, err := strconv.ParseUint(s, 16, 64) + if err != nil { + return SpanID(0), err + } + return SpanID(id), nil +} + +// MarshalText is called by encoding/json, which we do not want people to use. +func (s SpanID) MarshalText() ([]byte, error) { + return nil, fmt.Errorf("unsupported method SpanID.MarshalText; please use github.com/gogo/protobuf/jsonpb for marshalling") +} + +// UnmarshalText is called by encoding/json, which we do not want people to use. +func (s *SpanID) UnmarshalText(text []byte) error { + return fmt.Errorf("unsupported method SpanID.UnmarshalText; please use github.com/gogo/protobuf/jsonpb for marshalling") +} + +// Size returns the size of this datum in protobuf. It is always 8 bytes. +func (s *SpanID) Size() int { + return 8 +} + +// Marshal converts span ID into a binary representation. Called by protobuf serialization. +func (s SpanID) Marshal() ([]byte, error) { + b := make([]byte, s.Size()) + _, err := s.MarshalTo(b) + return b, err +} + +// MarshalTo converts span ID into a binary representation. Called by protobuf serialization. +func (s *SpanID) MarshalTo(data []byte) (n int, err error) { + var b [8]byte + binary.BigEndian.PutUint64(b[:], uint64(*s)) + return marshalBytes(data, b[:]) +} + +// Unmarshal inflates span ID from a binary representation. Called by protobuf serialization. +func (s *SpanID) Unmarshal(data []byte) error { + if len(data) < 8 { + return fmt.Errorf("buffer is too short") + } + *s = NewSpanID(binary.BigEndian.Uint64(data)) + return nil +} + +// MarshalJSON converts span id into a base64 string enclosed in quotes. +// Used by protobuf JSON serialization. +// Example: {1} => "AAAAAAAAAAE=". +func (s SpanID) MarshalJSON() ([]byte, error) { + var b [8]byte + s.MarshalTo(b[:]) // can only error on incorrect buffer size + v := make([]byte, 12+2) + base64.StdEncoding.Encode(v[1:13], b[:]) + v[0], v[13] = '"', '"' + return v, nil +} + +// UnmarshalJSON inflates span id from base64 string, possibly enclosed in quotes. +// User by protobuf JSON serialization. +// +// There appears to be a bug in gogoproto, as this function is only called for numeric values. +// https://github.com/gogo/protobuf/issues/411#issuecomment-393856837 +func (s *SpanID) UnmarshalJSON(data []byte) error { + str := string(data) + if l := len(str); l > 2 && str[0] == '"' && str[l-1] == '"' { + str = str[1 : l-1] + } + b, err := base64.StdEncoding.DecodeString(str) + if err != nil { + return fmt.Errorf("cannot unmarshal SpanID from string '%s': %v", string(data), err) + } + return s.Unmarshal(b) +} diff --git a/test/issue411/ids_test.go b/test/issue411/ids_test.go new file mode 100644 index 0000000000..60269cf503 --- /dev/null +++ b/test/issue411/ids_test.go @@ -0,0 +1,69 @@ +package issue411_test + +import ( + "bytes" + "testing" + + "github.com/gogo/protobuf/jsonpb" + "github.com/gogo/protobuf/proto" + "github.com/gogo/protobuf/test/issue411" +) + +// Thanks to @yurishkuro for reporting this issue (#411) and providing this test case + +// TraceID/SpanID fields are defined as bytes in proto, backed by custom types in Go. +// Unfortunately, that means they require manual implementations of proto & json serialization. +// To ensure that it's the same as the default protobuf serialization, file jaeger_test.proto +// contains a copy of SpanRef message without any gogo options. This test file is compiled with +// plain protoc -go_out (without gogo). This test performs proto and JSON marshaling/unmarshaling +// to ensure that the outputs of manual and standard serialization are identical. +func TestTraceSpanIDMarshalProto(t *testing.T) { + testCases := []struct { + name string + marshal func(proto.Message) ([]byte, error) + unmarshal func([]byte, proto.Message) error + expected string + }{ + { + name: "protobuf", + marshal: proto.Marshal, + unmarshal: proto.Unmarshal, + }, + { + name: "JSON", + marshal: func(m proto.Message) ([]byte, error) { + out := new(bytes.Buffer) + err := new(jsonpb.Marshaler).Marshal(out, m) + if err != nil { + return nil, err + } + return out.Bytes(), nil + }, + unmarshal: func(in []byte, m proto.Message) error { + return jsonpb.Unmarshal(bytes.NewReader(in), m) + }, + expected: `{"traceId":"AAAAAAAAAAIAAAAAAAAAAw==","spanId":"AAAAAAAAAAs="}`, + }, + } + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + o1 := issue411.Span{TraceID: issue411.NewTraceID(2, 3), SpanID: issue411.NewSpanID(11)} + d1, err := testCase.marshal(&o1) + if err != nil { + t.Fatalf("marshal error: %v", err) + } + // test unmarshal + var o2 issue411.Span + err = testCase.unmarshal(d1, &o2) + if err != nil { + t.Fatalf("umarshal error: %v", err) + } + if o1.TraceID != o2.TraceID { + t.Fatalf("TraceID: expected %v but got %v", o1, o2) + } + if o1.SpanID != o2.SpanID { + t.Fatalf("SpanID: expected %v but got %v", o1, o2) + } + }) + } +} diff --git a/test/issue411/issue411.pb.go b/test/issue411/issue411.pb.go new file mode 100644 index 0000000000..6a31ecedaf --- /dev/null +++ b/test/issue411/issue411.pb.go @@ -0,0 +1,72 @@ +// Code generated by protoc-gen-gogo. DO NOT EDIT. +// source: issue411.proto + +package issue411 + +import proto "github.com/gogo/protobuf/proto" +import fmt "fmt" +import math "math" +import _ "github.com/gogo/protobuf/gogoproto" + +// Reference imports to suppress errors if they are not otherwise used. +var _ = proto.Marshal +var _ = fmt.Errorf +var _ = math.Inf + +// This is a compile-time assertion to ensure that this generated file +// is compatible with the proto package it is being compiled against. +// A compilation error at this line likely means your copy of the +// proto package needs to be updated. +const _ = proto.GoGoProtoPackageIsVersion2 // please upgrade the proto package + +type Span struct { + TraceID TraceID `protobuf:"bytes,1,opt,name=trace_id,json=traceId,proto3,customtype=TraceID" json:"trace_id"` + SpanID SpanID `protobuf:"bytes,2,opt,name=span_id,json=spanId,proto3,customtype=SpanID" json:"span_id"` + XXX_NoUnkeyedLiteral struct{} `json:"-"` + XXX_unrecognized []byte `json:"-"` + XXX_sizecache int32 `json:"-"` +} + +func (m *Span) Reset() { *m = Span{} } +func (m *Span) String() string { return proto.CompactTextString(m) } +func (*Span) ProtoMessage() {} +func (*Span) Descriptor() ([]byte, []int) { + return fileDescriptor_issue411_3de9ea40a93d370b, []int{0} +} +func (m *Span) XXX_Unmarshal(b []byte) error { + return xxx_messageInfo_Span.Unmarshal(m, b) +} +func (m *Span) XXX_Marshal(b []byte, deterministic bool) ([]byte, error) { + return xxx_messageInfo_Span.Marshal(b, m, deterministic) +} +func (dst *Span) XXX_Merge(src proto.Message) { + xxx_messageInfo_Span.Merge(dst, src) +} +func (m *Span) XXX_Size() int { + return xxx_messageInfo_Span.Size(m) +} +func (m *Span) XXX_DiscardUnknown() { + xxx_messageInfo_Span.DiscardUnknown(m) +} + +var xxx_messageInfo_Span proto.InternalMessageInfo + +func init() { + proto.RegisterType((*Span)(nil), "issue411.Span") +} + +func init() { proto.RegisterFile("issue411.proto", fileDescriptor_issue411_3de9ea40a93d370b) } + +var fileDescriptor_issue411_3de9ea40a93d370b = []byte{ + // 158 bytes of a gzipped FileDescriptorProto + 0x1f, 0x8b, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0xff, 0xe2, 0xe2, 0xcb, 0x2c, 0x2e, 0x2e, + 0x4d, 0x35, 0x31, 0x34, 0xd4, 0x2b, 0x28, 0xca, 0x2f, 0xc9, 0x17, 0xe2, 0x80, 0xf1, 0xa5, 0x74, + 0xd3, 0x33, 0x4b, 0x32, 0x4a, 0x93, 0xf4, 0x92, 0xf3, 0x73, 0xf5, 0xd3, 0xf3, 0xd3, 0xf3, 0xf5, + 0xc1, 0x0a, 0x92, 0x4a, 0xd3, 0xc0, 0x3c, 0x30, 0x07, 0xcc, 0x82, 0x68, 0x54, 0x2a, 0xe0, 0x62, + 0x09, 0x2e, 0x48, 0xcc, 0x13, 0x32, 0xe5, 0xe2, 0x28, 0x29, 0x4a, 0x4c, 0x4e, 0x8d, 0xcf, 0x4c, + 0x91, 0x60, 0x54, 0x60, 0xd4, 0xe0, 0x71, 0x92, 0x3a, 0x71, 0x4f, 0x9e, 0xe1, 0xd6, 0x3d, 0x79, + 0xf6, 0x10, 0x90, 0xb8, 0xa7, 0xcb, 0x23, 0x04, 0x33, 0x88, 0x1d, 0xac, 0xd6, 0x33, 0x45, 0xc8, + 0x90, 0x8b, 0xbd, 0xb8, 0x20, 0x31, 0x0f, 0xa4, 0x8b, 0x09, 0xac, 0x4b, 0x02, 0xaa, 0x8b, 0x0d, + 0x64, 0x2a, 0x58, 0x13, 0x94, 0x15, 0xc4, 0x06, 0x52, 0xe8, 0x99, 0x92, 0xc4, 0x06, 0xb6, 0xd8, + 0x18, 0x10, 0x00, 0x00, 0xff, 0xff, 0xd9, 0x68, 0x60, 0x2b, 0xc3, 0x00, 0x00, 0x00, +} diff --git a/test/issue411/issue411.proto b/test/issue411/issue411.proto new file mode 100644 index 0000000000..e9f592f7c9 --- /dev/null +++ b/test/issue411/issue411.proto @@ -0,0 +1,49 @@ +// Go support for Protocol Buffers - Google's data interchange format +// +// Copyright 2015 The Go Authors. All rights reserved. +// https://github.com/golang/protobuf +// +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are +// met: +// +// * Redistributions of source code must retain the above copyright +// notice, this list of conditions and the following disclaimer. +// * Redistributions in binary form must reproduce the above +// copyright notice, this list of conditions and the following disclaimer +// in the documentation and/or other materials provided with the +// distribution. +// * Neither the name of Google Inc. nor the names of its +// contributors may be used to endorse or promote products derived from +// this software without specific prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +syntax = "proto3"; + +package issue411; + +import "github.com/gogo/protobuf/gogoproto/gogo.proto"; + +message Span { + bytes trace_id = 1 [ + (gogoproto.nullable) = false, + (gogoproto.customtype) = "TraceID", + (gogoproto.customname) = "TraceID" + ]; + bytes span_id = 2 [ + (gogoproto.nullable) = false, + (gogoproto.customtype) = "SpanID", + (gogoproto.customname) = "SpanID" + ]; +}