From c23161935796592fb61cf50672c79f4d3903c519 Mon Sep 17 00:00:00 2001 From: Joe Tsai Date: Wed, 31 Oct 2018 12:57:32 -0700 Subject: [PATCH] proto: deprecate {Unm,M}arshalMessageSet{JSON} Despite the naming, these are "internal-only" functions that are intended to only be called from generated code for MessageSets. Furthermore, MessageSets are themselves a deprecated feature from proto1 days, such that descriptor.proto even warns against their use since the initial open-source release of protocol buffers in 2008. Within Google, there are no direct usages of these functions, and all existing usages of MessageSets go through the new table-driven implementation. In addition to deprecating the {Unm,M}arshalMessageSet{JSON} top-level functions, also modify the generator to stop emitting MarshalJSON and UnmarshalJSON methods for messages sets. The UnmarshalJSON method is not implemented and the MarshalJSON method does not seem to be called anywhere in Google (verified by making the method panic and doing a global test). The jsonpb package continues to work with MessageSets. I should note that when the table-driven implementation was open sourced in late 2017 (see 8cc9e46429bfb16289d40d30b2ee3f4923b47345), it accidentally removed generation of the Marshal and Unmarshal method. However, no one seemed to have noticed that those methods were no longer generated. --- proto/deprecated.go | 22 ++++ proto/message_set.go | 119 +----------------- proto/message_set_test.go | 77 +++++++----- proto/table_unmarshal.go | 2 +- proto/test_proto/test.pb.go | 7 -- protoc-gen-go/generator/generator.go | 11 -- .../extension_base/extension_base.pb.go | 7 -- protoc-gen-go/testdata/my_test/test.pb.go | 7 -- 8 files changed, 69 insertions(+), 183 deletions(-) diff --git a/proto/deprecated.go b/proto/deprecated.go index 69de0ea0ef..108cdd1b53 100644 --- a/proto/deprecated.go +++ b/proto/deprecated.go @@ -31,8 +31,30 @@ package proto +import "errors" + // Deprecated: do not use. type Stats struct{ Emalloc, Dmalloc, Encode, Decode, Chit, Cmiss, Size uint64 } // Deprecated: do not use. func GetStats() Stats { return Stats{} } + +// Deprecated: do not use. +func MarshalMessageSet(interface{}) ([]byte, error) { + return nil, errors.New("proto: not implemented") +} + +// Deprecated: do not use. +func UnmarshalMessageSet([]byte, interface{}) error { + return errors.New("proto: not implemented") +} + +// Deprecated: do not use. +func MarshalMessageSetJSON(interface{}) ([]byte, error) { + return nil, errors.New("proto: not implemented") +} + +// Deprecated: do not use. +func UnmarshalMessageSetJSON([]byte, interface{}) error { + return errors.New("proto: not implemented") +} diff --git a/proto/message_set.go b/proto/message_set.go index 3b6ca41d5e..d4663ab162 100644 --- a/proto/message_set.go +++ b/proto/message_set.go @@ -36,13 +36,8 @@ package proto */ import ( - "bytes" - "encoding/json" "errors" - "fmt" "reflect" - "sort" - "sync" ) // errNoMessageTypeID occurs when a protocol buffer does not have a message type ID. @@ -145,46 +140,9 @@ func skipVarint(buf []byte) []byte { return buf[i+1:] } -// MarshalMessageSet encodes the extension map represented by m in the message set wire format. -// It is called by generated Marshal methods on protocol buffer messages with the message_set_wire_format option. -func MarshalMessageSet(exts interface{}) ([]byte, error) { - return marshalMessageSet(exts, false) -} - -// marshaMessageSet implements above function, with the opt to turn on / off deterministic during Marshal. -func marshalMessageSet(exts interface{}, deterministic bool) ([]byte, error) { - switch exts := exts.(type) { - case *XXX_InternalExtensions: - var u marshalInfo - siz := u.sizeMessageSet(exts) - b := make([]byte, 0, siz) - return u.appendMessageSet(b, exts, deterministic) - - case map[int32]Extension: - // This is an old-style extension map. - // Wrap it in a new-style XXX_InternalExtensions. - ie := XXX_InternalExtensions{ - p: &struct { - mu sync.Mutex - extensionMap map[int32]Extension - }{ - extensionMap: exts, - }, - } - - var u marshalInfo - siz := u.sizeMessageSet(&ie) - b := make([]byte, 0, siz) - return u.appendMessageSet(b, &ie, deterministic) - - default: - return nil, errors.New("proto: not an extension map") - } -} - -// UnmarshalMessageSet decodes the extension map encoded in buf in the message set wire format. +// unmarshalMessageSet decodes the extension map encoded in buf in the message set wire format. // It is called by Unmarshal methods on protocol buffer messages with the message_set_wire_format option. -func UnmarshalMessageSet(buf []byte, exts interface{}) error { +func unmarshalMessageSet(buf []byte, exts interface{}) error { var m map[int32]Extension switch exts := exts.(type) { case *XXX_InternalExtensions: @@ -223,79 +181,6 @@ func UnmarshalMessageSet(buf []byte, exts interface{}) error { return nil } -// MarshalMessageSetJSON encodes the extension map represented by m in JSON format. -// It is called by generated MarshalJSON methods on protocol buffer messages with the message_set_wire_format option. -func MarshalMessageSetJSON(exts interface{}) ([]byte, error) { - var m map[int32]Extension - switch exts := exts.(type) { - case *XXX_InternalExtensions: - var mu sync.Locker - m, mu = exts.extensionsRead() - if m != nil { - // Keep the extensions map locked until we're done marshaling to prevent - // races between marshaling and unmarshaling the lazily-{en,de}coded - // values. - mu.Lock() - defer mu.Unlock() - } - case map[int32]Extension: - m = exts - default: - return nil, errors.New("proto: not an extension map") - } - var b bytes.Buffer - b.WriteByte('{') - - // Process the map in key order for deterministic output. - ids := make([]int32, 0, len(m)) - for id := range m { - ids = append(ids, id) - } - sort.Sort(int32Slice(ids)) // int32Slice defined in text.go - - for i, id := range ids { - ext := m[id] - msd, ok := messageSetMap[id] - if !ok { - // Unknown type; we can't render it, so skip it. - continue - } - - if i > 0 && b.Len() > 1 { - b.WriteByte(',') - } - - fmt.Fprintf(&b, `"[%s]":`, msd.name) - - x := ext.value - if x == nil { - x = reflect.New(msd.t.Elem()).Interface() - if err := Unmarshal(ext.enc, x.(Message)); err != nil { - return nil, err - } - } - d, err := json.Marshal(x) - if err != nil { - return nil, err - } - b.Write(d) - } - b.WriteByte('}') - return b.Bytes(), nil -} - -// UnmarshalMessageSetJSON decodes the extension map encoded in buf in JSON format. -// It is called by generated UnmarshalJSON methods on protocol buffer messages with the message_set_wire_format option. -func UnmarshalMessageSetJSON(buf []byte, exts interface{}) error { - // Common-case fast path. - if len(buf) == 0 || bytes.Equal(buf, []byte("{}")) { - return nil - } - - // This is fairly tricky, and it's not clear that it is needed. - return errors.New("TODO: UnmarshalMessageSetJSON not yet implemented") -} - // A global registry of types that can be used in a MessageSet. var messageSetMap = make(map[int32]messageSetDesc) diff --git a/proto/message_set_test.go b/proto/message_set_test.go index 2c170c5f2a..1bd11aaec1 100644 --- a/proto/message_set_test.go +++ b/proto/message_set_test.go @@ -29,49 +29,60 @@ // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. -package proto +package proto_test import ( "bytes" + "fmt" "testing" + + "github.com/golang/protobuf/proto" + . "github.com/golang/protobuf/proto/test_proto" ) func TestUnmarshalMessageSetWithDuplicate(t *testing.T) { - // Check that a repeated message set entry will be concatenated. - in := &messageSet{ - Item: []*_MessageSet_Item{ - {TypeId: Int32(12345), Message: []byte("hoo")}, - {TypeId: Int32(12345), Message: []byte("hah")}, - }, - } - b, err := Marshal(in) - if err != nil { - t.Fatalf("Marshal: %v", err) - } - t.Logf("Marshaled bytes: %q", b) + /* + Message{ + Tag{1, StartGroup}, + Message{ + Tag{2, Varint}, Uvarint(12345), + Tag{3, Bytes}, Bytes("hoo"), + }, + Tag{1, EndGroup}, + Tag{1, StartGroup}, + Message{ + Tag{2, Varint}, Uvarint(12345), + Tag{3, Bytes}, Bytes("hah"), + }, + Tag{1, EndGroup}, + } + */ + var in []byte + fmt.Sscanf("0b10b9601a03686f6f0c0b10b9601a036861680c", "%x", &in) - var extensions XXX_InternalExtensions - if err := UnmarshalMessageSet(b, &extensions); err != nil { - t.Fatalf("UnmarshalMessageSet: %v", err) - } - ext, ok := extensions.p.extensionMap[12345] - if !ok { - t.Fatalf("Didn't retrieve extension 12345; map is %v", extensions.p.extensionMap) - } - // Skip wire type/field number and length varints. - got := skipVarint(skipVarint(ext.enc)) - if want := []byte("hoohah"); !bytes.Equal(got, want) { - t.Errorf("Combined extension is %q, want %q", got, want) - } -} + /* + Message{ + Tag{1, StartGroup}, + Message{ + Tag{2, Varint}, Uvarint(12345), + Tag{3, Bytes}, Bytes("hoohah"), + }, + Tag{1, EndGroup}, + } + */ + var want []byte + fmt.Sscanf("0b10b9601a06686f6f6861680c", "%x", &want) -func TestMarshalMessageSetJSON_UnknownType(t *testing.T) { - extMap := map[int32]Extension{12345: Extension{}} - got, err := MarshalMessageSetJSON(extMap) + var m MyMessageSet + if err := proto.Unmarshal(in, &m); err != nil { + t.Fatalf("unexpected Unmarshal error: %v", err) + } + got, err := proto.Marshal(&m) if err != nil { - t.Fatalf("MarshalMessageSetJSON: %v", err) + t.Fatalf("unexpected Marshal error: %v", err) } - if want := []byte("{}"); !bytes.Equal(got, want) { - t.Errorf("MarshalMessageSetJSON(%v) = %q, want %q", extMap, got, want) + + if !bytes.Equal(got, want) { + t.Errorf("output mismatch:\ngot %x\nwant %x", got, want) } } diff --git a/proto/table_unmarshal.go b/proto/table_unmarshal.go index fd4afec8d2..5da26ed523 100644 --- a/proto/table_unmarshal.go +++ b/proto/table_unmarshal.go @@ -136,7 +136,7 @@ func (u *unmarshalInfo) unmarshal(m pointer, b []byte) error { u.computeUnmarshalInfo() } if u.isMessageSet { - return UnmarshalMessageSet(b, m.offset(u.extensions).toExtensions()) + return unmarshalMessageSet(b, m.offset(u.extensions).toExtensions()) } var reqMask uint64 // bitmask of required fields we've seen. var errLater error diff --git a/proto/test_proto/test.pb.go b/proto/test_proto/test.pb.go index 3b9012ad9f..e986d51ab5 100644 --- a/proto/test_proto/test.pb.go +++ b/proto/test_proto/test.pb.go @@ -2247,13 +2247,6 @@ func (*MyMessageSet) Descriptor() ([]byte, []int) { return fileDescriptor_8ca34d01332f1402, []int{17} } -func (m *MyMessageSet) MarshalJSON() ([]byte, error) { - return proto.MarshalMessageSetJSON(&m.XXX_InternalExtensions) -} -func (m *MyMessageSet) UnmarshalJSON(buf []byte) error { - return proto.UnmarshalMessageSetJSON(buf, &m.XXX_InternalExtensions) -} - var extRange_MyMessageSet = []proto.ExtensionRange{ {Start: 100, End: 2147483646}, } diff --git a/protoc-gen-go/generator/generator.go b/protoc-gen-go/generator/generator.go index b2c480f26c..3975955b77 100644 --- a/protoc-gen-go/generator/generator.go +++ b/protoc-gen-go/generator/generator.go @@ -2370,17 +2370,6 @@ func (g *Generator) generateCommonMethods(mc *msgCtx) { // Extension support methods if len(mc.message.ExtensionRange) > 0 { - // message_set_wire_format only makes sense when extensions are defined. - if opts := mc.message.Options; opts != nil && opts.GetMessageSetWireFormat() { - g.P() - g.P("func (m *", mc.goName, ") MarshalJSON() ([]byte, error) {") - g.P("return ", g.Pkg["proto"], ".MarshalMessageSetJSON(&m.XXX_InternalExtensions)") - g.P("}") - g.P("func (m *", mc.goName, ") UnmarshalJSON(buf []byte) error {") - g.P("return ", g.Pkg["proto"], ".UnmarshalMessageSetJSON(buf, &m.XXX_InternalExtensions)") - g.P("}") - } - g.P() g.P("var extRange_", mc.goName, " = []", g.Pkg["proto"], ".ExtensionRange{") for _, r := range mc.message.ExtensionRange { diff --git a/protoc-gen-go/testdata/extension_base/extension_base.pb.go b/protoc-gen-go/testdata/extension_base/extension_base.pb.go index 7cd6bb94a1..bbcae6eea0 100644 --- a/protoc-gen-go/testdata/extension_base/extension_base.pb.go +++ b/protoc-gen-go/testdata/extension_base/extension_base.pb.go @@ -84,13 +84,6 @@ func (*OldStyleMessage) Descriptor() ([]byte, []int) { return fileDescriptor_2fbd53bac0b7ca8a, []int{1} } -func (m *OldStyleMessage) MarshalJSON() ([]byte, error) { - return proto.MarshalMessageSetJSON(&m.XXX_InternalExtensions) -} -func (m *OldStyleMessage) UnmarshalJSON(buf []byte) error { - return proto.UnmarshalMessageSetJSON(buf, &m.XXX_InternalExtensions) -} - var extRange_OldStyleMessage = []proto.ExtensionRange{ {Start: 100, End: 2147483646}, } diff --git a/protoc-gen-go/testdata/my_test/test.pb.go b/protoc-gen-go/testdata/my_test/test.pb.go index b241933f2a..61cb444c01 100644 --- a/protoc-gen-go/testdata/my_test/test.pb.go +++ b/protoc-gen-go/testdata/my_test/test.pb.go @@ -615,13 +615,6 @@ func (*OldReply) Descriptor() ([]byte, []int) { return fileDescriptor_2c9b60a40d5131b9, []int{5} } -func (m *OldReply) MarshalJSON() ([]byte, error) { - return proto.MarshalMessageSetJSON(&m.XXX_InternalExtensions) -} -func (m *OldReply) UnmarshalJSON(buf []byte) error { - return proto.UnmarshalMessageSetJSON(buf, &m.XXX_InternalExtensions) -} - var extRange_OldReply = []proto.ExtensionRange{ {Start: 100, End: 2147483646}, }