proto, internal/impl: make wire output more consistent with v1
The v1 wire marshaler sorts fields as follows:
- All extensions, sorted by field number.
- All non-oneof fields, sorted by field number.
- All oneof fields, in indeterminate order.
We already make some steps toward supporting this ordering: The
fast path encoder places extensions in sorted order at the start
of the message.
This commit moves oneof fields to the end of the message, makes the
reflection-based encoder use this ordering when deterministic marshaling
is enabled, and adds a test to catch unintentional changes to the
ordering.
Users SHOULD NOT depend on stability of the marshal output. It is
subject to change over time. Without deterministic marshaling enabled,
it is subject to change over calls to Marshal.
Change-Id: I6cfd89090d790a3bb50785f32b94d2781d7d08db
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/206800
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
diff --git a/internal/fieldsort/fieldsort.go b/internal/fieldsort/fieldsort.go
new file mode 100644
index 0000000..1cb2d74
--- /dev/null
+++ b/internal/fieldsort/fieldsort.go
@@ -0,0 +1,40 @@
+// Copyright 2019 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// Package fieldsort defines an ordering of fields.
+//
+// The ordering defined by this package matches the historic behavior of the proto
+// package, placing extensions first and oneofs last.
+//
+// There is no guarantee about stability of the wire encoding, and users should not
+// depend on the order defined in this package as it is subject to change without
+// notice.
+package fieldsort
+
+import (
+ "google.golang.org/protobuf/reflect/protoreflect"
+)
+
+// Less returns true if field a comes before field j in ordered wire marshal output.
+func Less(a, b protoreflect.FieldDescriptor) bool {
+ ea := a.IsExtension()
+ eb := b.IsExtension()
+ oa := a.ContainingOneof()
+ ob := b.ContainingOneof()
+ switch {
+ case ea != eb:
+ return ea
+ case oa != nil && ob != nil:
+ if oa == ob {
+ return a.Number() < b.Number()
+ }
+ return oa.Index() < ob.Index()
+ case oa != nil:
+ return false
+ case ob != nil:
+ return true
+ default:
+ return a.Number() < b.Number()
+ }
+}
diff --git a/internal/impl/codec_message.go b/internal/impl/codec_message.go
index ab1d4ec..b4d632e 100644
--- a/internal/impl/codec_message.go
+++ b/internal/impl/codec_message.go
@@ -12,6 +12,7 @@
"google.golang.org/protobuf/internal/encoding/messageset"
"google.golang.org/protobuf/internal/encoding/wire"
+ "google.golang.org/protobuf/internal/fieldsort"
pref "google.golang.org/protobuf/reflect/protoreflect"
piface "google.golang.org/protobuf/runtime/protoiface"
)
@@ -122,6 +123,15 @@
mi.denseCoderFields[cf.num] = cf
}
+ // To preserve compatibility with historic wire output, marshal oneofs last.
+ if mi.Desc.Oneofs().Len() > 0 {
+ sort.Slice(mi.orderedCoderFields, func(i, j int) bool {
+ fi := fields.ByNumber(mi.orderedCoderFields[i].num)
+ fj := fields.ByNumber(mi.orderedCoderFields[j].num)
+ return fieldsort.Less(fi, fj)
+ })
+ }
+
mi.needsInitCheck = needsInitCheck(mi.Desc)
if mi.methods.MarshalAppend == nil && mi.methods.Size == nil {
mi.methods.Flags |= piface.SupportMarshalDeterministic
diff --git a/internal/testprotos/order/order.pb.go b/internal/testprotos/order/order.pb.go
new file mode 100644
index 0000000..d2d0375
--- /dev/null
+++ b/internal/testprotos/order/order.pb.go
@@ -0,0 +1,257 @@
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// Messages in this file are used to test wire encoding order.
+
+// Code generated by protoc-gen-go. DO NOT EDIT.
+// source: order/order.proto
+
+package order
+
+import (
+ protoreflect "google.golang.org/protobuf/reflect/protoreflect"
+ protoiface "google.golang.org/protobuf/runtime/protoiface"
+ protoimpl "google.golang.org/protobuf/runtime/protoimpl"
+ reflect "reflect"
+ sync "sync"
+)
+
+type Message struct {
+ state protoimpl.MessageState
+ sizeCache protoimpl.SizeCache
+ unknownFields protoimpl.UnknownFields
+ extensionFields protoimpl.ExtensionFields
+
+ Field_2 *string `protobuf:"bytes,2,opt,name=field_2,json=field2" json:"field_2,omitempty"`
+ Field_1 *string `protobuf:"bytes,1,opt,name=field_1,json=field1" json:"field_1,omitempty"`
+ // Types that are assignable to Oneof_1:
+ // *Message_Field_10
+ Oneof_1 isMessage_Oneof_1 `protobuf_oneof:"oneof_1"`
+ Field_20 *string `protobuf:"bytes,20,opt,name=field_20,json=field20" json:"field_20,omitempty"`
+}
+
+func (x *Message) Reset() {
+ *x = Message{}
+ if protoimpl.UnsafeEnabled {
+ mi := &file_order_order_proto_msgTypes[0]
+ ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x))
+ ms.StoreMessageInfo(mi)
+ }
+}
+
+func (x *Message) String() string {
+ return protoimpl.X.MessageStringOf(x)
+}
+
+func (*Message) ProtoMessage() {}
+
+func (x *Message) ProtoReflect() protoreflect.Message {
+ mi := &file_order_order_proto_msgTypes[0]
+ if protoimpl.UnsafeEnabled && x != nil {
+ ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x))
+ if ms.LoadMessageInfo() == nil {
+ ms.StoreMessageInfo(mi)
+ }
+ return ms
+ }
+ return mi.MessageOf(x)
+}
+
+// Deprecated: Use Message.ProtoReflect.Descriptor instead.
+func (*Message) Descriptor() ([]byte, []int) {
+ return file_order_order_proto_rawDescGZIP(), []int{0}
+}
+
+var extRange_Message = []protoiface.ExtensionRangeV1{
+ {Start: 30, End: 40},
+}
+
+// Deprecated: Use Message.ProtoReflect.Descriptor.ExtensionRanges instead.
+func (*Message) ExtensionRangeArray() []protoiface.ExtensionRangeV1 {
+ return extRange_Message
+}
+
+func (x *Message) GetField_2() string {
+ if x != nil && x.Field_2 != nil {
+ return *x.Field_2
+ }
+ return ""
+}
+
+func (x *Message) GetField_1() string {
+ if x != nil && x.Field_1 != nil {
+ return *x.Field_1
+ }
+ return ""
+}
+
+func (m *Message) GetOneof_1() isMessage_Oneof_1 {
+ if m != nil {
+ return m.Oneof_1
+ }
+ return nil
+}
+
+func (x *Message) GetField_10() string {
+ if x, ok := x.GetOneof_1().(*Message_Field_10); ok {
+ return x.Field_10
+ }
+ return ""
+}
+
+func (x *Message) GetField_20() string {
+ if x != nil && x.Field_20 != nil {
+ return *x.Field_20
+ }
+ return ""
+}
+
+type isMessage_Oneof_1 interface {
+ isMessage_Oneof_1()
+}
+
+type Message_Field_10 struct {
+ Field_10 string `protobuf:"bytes,10,opt,name=field_10,json=field10,oneof"`
+}
+
+func (*Message_Field_10) isMessage_Oneof_1() {}
+
+var file_order_order_proto_extTypes = []protoimpl.ExtensionInfo{
+ {
+ ExtendedType: (*Message)(nil),
+ ExtensionType: (*string)(nil),
+ Field: 30,
+ Name: "goproto.proto.order.field_30",
+ Tag: "bytes,30,opt,name=field_30",
+ Filename: "order/order.proto",
+ },
+ {
+ ExtendedType: (*Message)(nil),
+ ExtensionType: (*string)(nil),
+ Field: 31,
+ Name: "goproto.proto.order.field_31",
+ Tag: "bytes,31,opt,name=field_31",
+ Filename: "order/order.proto",
+ },
+ {
+ ExtendedType: (*Message)(nil),
+ ExtensionType: (*string)(nil),
+ Field: 32,
+ Name: "goproto.proto.order.field_32",
+ Tag: "bytes,32,opt,name=field_32",
+ Filename: "order/order.proto",
+ },
+}
+
+// Extension fields to Message.
+var (
+ // optional string field_30 = 30;
+ E_Field_30 = &file_order_order_proto_extTypes[0]
+ // optional string field_31 = 31;
+ E_Field_31 = &file_order_order_proto_extTypes[1]
+ // optional string field_32 = 32;
+ E_Field_32 = &file_order_order_proto_extTypes[2]
+)
+
+var File_order_order_proto protoreflect.FileDescriptor
+
+var file_order_order_proto_rawDesc = []byte{
+ 0x0a, 0x11, 0x6f, 0x72, 0x64, 0x65, 0x72, 0x2f, 0x6f, 0x72, 0x64, 0x65, 0x72, 0x2e, 0x70, 0x72,
+ 0x6f, 0x74, 0x6f, 0x12, 0x13, 0x67, 0x6f, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x2e, 0x70, 0x72, 0x6f,
+ 0x74, 0x6f, 0x2e, 0x6f, 0x72, 0x64, 0x65, 0x72, 0x22, 0x84, 0x01, 0x0a, 0x07, 0x4d, 0x65, 0x73,
+ 0x73, 0x61, 0x67, 0x65, 0x12, 0x17, 0x0a, 0x07, 0x66, 0x69, 0x65, 0x6c, 0x64, 0x5f, 0x32, 0x18,
+ 0x02, 0x20, 0x01, 0x28, 0x09, 0x52, 0x06, 0x66, 0x69, 0x65, 0x6c, 0x64, 0x32, 0x12, 0x17, 0x0a,
+ 0x07, 0x66, 0x69, 0x65, 0x6c, 0x64, 0x5f, 0x31, 0x18, 0x01, 0x20, 0x01, 0x28, 0x09, 0x52, 0x06,
+ 0x66, 0x69, 0x65, 0x6c, 0x64, 0x31, 0x12, 0x1b, 0x0a, 0x08, 0x66, 0x69, 0x65, 0x6c, 0x64, 0x5f,
+ 0x31, 0x30, 0x18, 0x0a, 0x20, 0x01, 0x28, 0x09, 0x48, 0x00, 0x52, 0x07, 0x66, 0x69, 0x65, 0x6c,
+ 0x64, 0x31, 0x30, 0x12, 0x19, 0x0a, 0x08, 0x66, 0x69, 0x65, 0x6c, 0x64, 0x5f, 0x32, 0x30, 0x18,
+ 0x14, 0x20, 0x01, 0x28, 0x09, 0x52, 0x07, 0x66, 0x69, 0x65, 0x6c, 0x64, 0x32, 0x30, 0x2a, 0x04,
+ 0x08, 0x1e, 0x10, 0x29, 0x42, 0x09, 0x0a, 0x07, 0x6f, 0x6e, 0x65, 0x6f, 0x66, 0x5f, 0x31, 0x3a,
+ 0x37, 0x0a, 0x08, 0x66, 0x69, 0x65, 0x6c, 0x64, 0x5f, 0x33, 0x30, 0x12, 0x1c, 0x2e, 0x67, 0x6f,
+ 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x2e, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x2e, 0x6f, 0x72, 0x64, 0x65,
+ 0x72, 0x2e, 0x4d, 0x65, 0x73, 0x73, 0x61, 0x67, 0x65, 0x18, 0x1e, 0x20, 0x01, 0x28, 0x09, 0x52,
+ 0x07, 0x66, 0x69, 0x65, 0x6c, 0x64, 0x33, 0x30, 0x3a, 0x37, 0x0a, 0x08, 0x66, 0x69, 0x65, 0x6c,
+ 0x64, 0x5f, 0x33, 0x31, 0x12, 0x1c, 0x2e, 0x67, 0x6f, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x2e, 0x70,
+ 0x72, 0x6f, 0x74, 0x6f, 0x2e, 0x6f, 0x72, 0x64, 0x65, 0x72, 0x2e, 0x4d, 0x65, 0x73, 0x73, 0x61,
+ 0x67, 0x65, 0x18, 0x1f, 0x20, 0x01, 0x28, 0x09, 0x52, 0x07, 0x66, 0x69, 0x65, 0x6c, 0x64, 0x33,
+ 0x31, 0x3a, 0x37, 0x0a, 0x08, 0x66, 0x69, 0x65, 0x6c, 0x64, 0x5f, 0x33, 0x32, 0x12, 0x1c, 0x2e,
+ 0x67, 0x6f, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x2e, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x2e, 0x6f, 0x72,
+ 0x64, 0x65, 0x72, 0x2e, 0x4d, 0x65, 0x73, 0x73, 0x61, 0x67, 0x65, 0x18, 0x20, 0x20, 0x01, 0x28,
+ 0x09, 0x52, 0x07, 0x66, 0x69, 0x65, 0x6c, 0x64, 0x33, 0x32, 0x42, 0x36, 0x5a, 0x34, 0x67, 0x6f,
+ 0x6f, 0x67, 0x6c, 0x65, 0x2e, 0x67, 0x6f, 0x6c, 0x61, 0x6e, 0x67, 0x2e, 0x6f, 0x72, 0x67, 0x2f,
+ 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x62, 0x75, 0x66, 0x2f, 0x69, 0x6e, 0x74, 0x65, 0x72, 0x6e, 0x61,
+ 0x6c, 0x2f, 0x74, 0x65, 0x73, 0x74, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x73, 0x2f, 0x6f, 0x72, 0x64,
+ 0x65, 0x72,
+}
+
+var (
+ file_order_order_proto_rawDescOnce sync.Once
+ file_order_order_proto_rawDescData = file_order_order_proto_rawDesc
+)
+
+func file_order_order_proto_rawDescGZIP() []byte {
+ file_order_order_proto_rawDescOnce.Do(func() {
+ file_order_order_proto_rawDescData = protoimpl.X.CompressGZIP(file_order_order_proto_rawDescData)
+ })
+ return file_order_order_proto_rawDescData
+}
+
+var file_order_order_proto_msgTypes = make([]protoimpl.MessageInfo, 1)
+var file_order_order_proto_goTypes = []interface{}{
+ (*Message)(nil), // 0: goproto.proto.order.Message
+}
+var file_order_order_proto_depIdxs = []int32{
+ 0, // 0: goproto.proto.order.field_30:extendee -> goproto.proto.order.Message
+ 0, // 1: goproto.proto.order.field_31:extendee -> goproto.proto.order.Message
+ 0, // 2: goproto.proto.order.field_32:extendee -> goproto.proto.order.Message
+ 3, // [3:3] is the sub-list for method output_type
+ 3, // [3:3] is the sub-list for method input_type
+ 3, // [3:3] is the sub-list for extension type_name
+ 0, // [0:3] is the sub-list for extension extendee
+ 0, // [0:0] is the sub-list for field type_name
+}
+
+func init() { file_order_order_proto_init() }
+func file_order_order_proto_init() {
+ if File_order_order_proto != nil {
+ return
+ }
+ if !protoimpl.UnsafeEnabled {
+ file_order_order_proto_msgTypes[0].Exporter = func(v interface{}, i int) interface{} {
+ switch v := v.(*Message); i {
+ case 0:
+ return &v.state
+ case 1:
+ return &v.sizeCache
+ case 2:
+ return &v.unknownFields
+ case 3:
+ return &v.extensionFields
+ default:
+ return nil
+ }
+ }
+ }
+ file_order_order_proto_msgTypes[0].OneofWrappers = []interface{}{
+ (*Message_Field_10)(nil),
+ }
+ type x struct{}
+ out := protoimpl.TypeBuilder{
+ File: protoimpl.DescBuilder{
+ GoPackagePath: reflect.TypeOf(x{}).PkgPath(),
+ RawDescriptor: file_order_order_proto_rawDesc,
+ NumEnums: 0,
+ NumMessages: 1,
+ NumExtensions: 3,
+ NumServices: 0,
+ },
+ GoTypes: file_order_order_proto_goTypes,
+ DependencyIndexes: file_order_order_proto_depIdxs,
+ MessageInfos: file_order_order_proto_msgTypes,
+ ExtensionInfos: file_order_order_proto_extTypes,
+ }.Build()
+ File_order_order_proto = out.File
+ file_order_order_proto_rawDesc = nil
+ file_order_order_proto_goTypes = nil
+ file_order_order_proto_depIdxs = nil
+}
diff --git a/internal/testprotos/order/order.proto b/internal/testprotos/order/order.proto
new file mode 100644
index 0000000..2a214dd
--- /dev/null
+++ b/internal/testprotos/order/order.proto
@@ -0,0 +1,29 @@
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// Messages in this file are used to test wire encoding order.
+
+syntax = "proto2";
+
+package goproto.proto.order;
+
+option go_package = "google.golang.org/protobuf/internal/testprotos/order";
+
+message Message {
+ optional string field_2 = 2;
+ optional string field_1 = 1;
+
+ oneof oneof_1 {
+ string field_10 = 10;
+ }
+
+ extensions 30 to 40;
+
+ optional string field_20 = 20;
+}
+
+extend Message {
+ optional string field_30 = 30;
+ optional string field_31 = 31;
+ optional string field_32 = 32;
+}