From 951a149f90371fb8858c6c979d03bb2583611052 Mon Sep 17 00:00:00 2001 From: Joe Tsai Date: Thu, 8 Nov 2018 09:20:01 -0800 Subject: [PATCH] proto: deprecate {Unm,M}arshalMessageSet{JSON} (#741) 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 | 25 ++++ proto/message_set.go | 137 +----------------- proto/message_set_test.go | 77 +++++----- proto/table_unmarshal.go | 2 +- proto/test_proto/test.pb.go | 7 - protoc-gen-go/generator/generator.go | 17 --- .../extension_base/extension_base.pb.go | 7 - .../extension_user/extension_user.pb.go | 1 - protoc-gen-go/testdata/my_test/test.pb.go | 7 - 9 files changed, 72 insertions(+), 208 deletions(-) diff --git a/proto/deprecated.go b/proto/deprecated.go index 69de0ea0ef..35b882c09a 100644 --- a/proto/deprecated.go +++ b/proto/deprecated.go @@ -31,8 +31,33 @@ 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") +} + +// Deprecated: do not use. +func RegisterMessageSetType(Message, int32, string) {} diff --git a/proto/message_set.go b/proto/message_set.go index 3b6ca41d5e..f48a756761 100644 --- a/proto/message_set.go +++ b/proto/message_set.go @@ -36,13 +36,7 @@ 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 +139,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: @@ -222,93 +179,3 @@ 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) - -type messageSetDesc struct { - t reflect.Type // pointer to struct - name string -} - -// RegisterMessageSetType is called from the generated code. -func RegisterMessageSetType(m Message, fieldNum int32, name string) { - messageSetMap[fieldNum] = messageSetDesc{ - t: reflect.TypeOf(m), - name: name, - } -} 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 e01b75d448..a9f414f5f0 100644 --- a/protoc-gen-go/generator/generator.go +++ b/protoc-gen-go/generator/generator.go @@ -2408,17 +2408,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 { @@ -2824,10 +2813,8 @@ func (g *Generator) generateExtension(ext *ExtensionDescriptor) { // In addition, the situation for when to apply this special case is implemented // differently in other languages: // https://github.com/google/protobuf/blob/aff10976/src/google/protobuf/text_format.cc#L1560 - mset := false if extDesc.GetOptions().GetMessageSetWireFormat() && typeName[len(typeName)-1] == "message_set_extension" { typeName = typeName[:len(typeName)-1] - mset = true } // For text formatting, the package must be exactly what the .proto file declares, @@ -2849,10 +2836,6 @@ func (g *Generator) generateExtension(ext *ExtensionDescriptor) { g.P() g.addInitf("%s.RegisterExtension(%s)", g.Pkg["proto"], ext.DescName()) - if mset { - // Generate a bit more code to register with message_set.go. - g.addInitf("%s.RegisterMessageSetType((%s)(nil), %d, %q)", g.Pkg["proto"], fieldType, *field.Number, extName) - } g.file.addExport(ext, constOrVarSymbol{ccTypeName, "var", ""}) } 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/extension_user/extension_user.pb.go b/protoc-gen-go/testdata/extension_user/extension_user.pb.go index 2f7fe3a457..2332dffc03 100644 --- a/protoc-gen-go/testdata/extension_user/extension_user.pb.go +++ b/protoc-gen-go/testdata/extension_user/extension_user.pb.go @@ -360,7 +360,6 @@ func init() { proto.RegisterExtension(E_Announcement_LoudExt) proto.RegisterType((*Announcement)(nil), "extension_user.Announcement") proto.RegisterExtension(E_OldStyleParcel_MessageSetExtension) - proto.RegisterMessageSetType((*OldStyleParcel)(nil), 2001, "extension_user.OldStyleParcel") proto.RegisterType((*OldStyleParcel)(nil), "extension_user.OldStyleParcel") proto.RegisterExtension(E_UserMessage) proto.RegisterExtension(E_ExtraMessage) diff --git a/protoc-gen-go/testdata/my_test/test.pb.go b/protoc-gen-go/testdata/my_test/test.pb.go index 066e33e8d6..08a23fa9bb 100644 --- a/protoc-gen-go/testdata/my_test/test.pb.go +++ b/protoc-gen-go/testdata/my_test/test.pb.go @@ -672,13 +672,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}, }