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;
+}
diff --git a/proto/encode.go b/proto/encode.go
index 756082c..702190f 100644
--- a/proto/encode.go
+++ b/proto/encode.go
@@ -9,6 +9,7 @@
"google.golang.org/protobuf/internal/encoding/messageset"
"google.golang.org/protobuf/internal/encoding/wire"
+ "google.golang.org/protobuf/internal/fieldsort"
"google.golang.org/protobuf/internal/mapsort"
"google.golang.org/protobuf/internal/pragma"
"google.golang.org/protobuf/reflect/protoreflect"
@@ -146,7 +147,7 @@
// It is not deterministic, since Message.Range does not return fields in any
// defined order.
//
- // When using deterministic serialization, we sort the known fields by field number.
+ // When using deterministic serialization, we sort the known fields.
var err error
o.rangeFields(m, func(fd protoreflect.FieldDescriptor, v protoreflect.Value) bool {
b, err = o.marshalField(b, fd, v)
@@ -159,8 +160,7 @@
return b, nil
}
-// rangeFields visits fields in field number order when deterministic
-// serialization is enabled.
+// rangeFields visits fields in a defined order when deterministic serialization is enabled.
func (o MarshalOptions) rangeFields(m protoreflect.Message, f func(protoreflect.FieldDescriptor, protoreflect.Value) bool) {
if !o.Deterministic {
m.Range(f)
@@ -172,7 +172,7 @@
return true
})
sort.Slice(fds, func(a, b int) bool {
- return fds[a].Number() < fds[b].Number()
+ return fieldsort.Less(fds[a], fds[b])
})
for _, fd := range fds {
if !f(fd, m.Get(fd)) {
diff --git a/proto/encode_test.go b/proto/encode_test.go
index 663612d..348d900 100644
--- a/proto/encode_test.go
+++ b/proto/encode_test.go
@@ -7,12 +7,16 @@
import (
"bytes"
"fmt"
+ "reflect"
"testing"
"github.com/google/go-cmp/cmp"
+ "google.golang.org/protobuf/internal/encoding/wire"
"google.golang.org/protobuf/internal/flags"
"google.golang.org/protobuf/proto"
+ pref "google.golang.org/protobuf/reflect/protoreflect"
+ orderpb "google.golang.org/protobuf/internal/testprotos/order"
testpb "google.golang.org/protobuf/internal/testprotos/test"
test3pb "google.golang.org/protobuf/internal/testprotos/test3"
)
@@ -182,3 +186,48 @@
t.Errorf("expect amortized allocs/op to be identical")
}
}
+
+func TestEncodeOrder(t *testing.T) {
+ // We make no guarantees about the stability of wire marshal output.
+ // The order in which fields are marshaled may change over time.
+ // If deterministic marshaling is not enabled, it may change over
+ // successive calls to proto.Marshal in the same binary.
+ //
+ // Unfortunately, many users have come to rely on the specific current
+ // wire marshal output. Perhaps someday we will choose to deliberately
+ // change the marshal output; until that day comes, this test verifies
+ // that we don't unintentionally change it.
+ m := &orderpb.Message{
+ Field_1: proto.String("one"),
+ Field_2: proto.String("two"),
+ Field_20: proto.String("twenty"),
+ Oneof_1: &orderpb.Message_Field_10{"ten"},
+ }
+ proto.SetExtension(m, orderpb.E_Field_30, "thirty")
+ proto.SetExtension(m, orderpb.E_Field_31, "thirty-one")
+ proto.SetExtension(m, orderpb.E_Field_32, "thirty-two")
+ want := []pref.FieldNumber{
+ 30, 31, 32, // extensions first, in number order
+ 1, 2, 20, // non-extension, non-oneof in number order
+ 10, // oneofs last, undefined order
+ }
+
+ // Test with deterministic serialization, since fields are not sorted without
+ // it when -tags=protoreflect.
+ b, err := proto.MarshalOptions{Deterministic: true}.Marshal(m)
+ if err != nil {
+ t.Fatal(err)
+ }
+ var got []pref.FieldNumber
+ for len(b) > 0 {
+ num, _, n := wire.ConsumeField(b)
+ if n < 0 {
+ t.Fatal(wire.ParseError(n))
+ }
+ b = b[n:]
+ got = append(got, num)
+ }
+ if !reflect.DeepEqual(got, want) {
+ t.Errorf("unexpected field marshal order:\ngot: %v\nwant: %v\nmessage:\n%v", got, want, m)
+ }
+}
