proto: reset message by default in Unmarshal
We change Unmarshal to reset a message by default.
* We add a Merge option to UnmarshalOptions for explicit merging.
* We speed up Reset by checking for the Reset method.
* Remove TODOs in prototext and protojson about reset behavior.
Fixes golang/protobuf#890
Change-Id: Ibd8963c741053f564acf061fbdb846699942109c
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/195457
Reviewed-by: Damien Neil <dneil@google.com>
diff --git a/encoding/protojson/decode.go b/encoding/protojson/decode.go
index cb4f7fb..799dc1d 100644
--- a/encoding/protojson/decode.go
+++ b/encoding/protojson/decode.go
@@ -54,9 +54,6 @@
// setting the fields. If it returns an error, the given message may be
// partially set.
func (o UnmarshalOptions) Unmarshal(b []byte, m proto.Message) error {
- // TODO: Determine if we would like to have an option for merging or only
- // have merging behavior. We should at least be consistent with textproto
- // marshaling.
proto.Reset(m)
if o.Resolver == nil {
diff --git a/encoding/prototext/decode.go b/encoding/prototext/decode.go
index ef46a6d..2f2a0e9 100644
--- a/encoding/prototext/decode.go
+++ b/encoding/prototext/decode.go
@@ -53,10 +53,6 @@
// Unmarshal reads the given []byte and populates the given proto.Message using options in
// UnmarshalOptions object.
func (o UnmarshalOptions) Unmarshal(b []byte, m proto.Message) error {
- // Clear all fields before populating it.
- // TODO: Determine if this needs to be consistent with protojson and binary unmarshal where
- // behavior is to merge values into existing message. If decision is to not clear the fields
- // ahead, code will need to be updated properly when merging nested messages.
proto.Reset(m)
// Parse into text.Value of message type.
diff --git a/internal/impl/decode.go b/internal/impl/decode.go
index f71d19d..4441d4b 100644
--- a/internal/impl/decode.go
+++ b/internal/impl/decode.go
@@ -40,6 +40,7 @@
func (o unmarshalOptions) Options() proto.UnmarshalOptions {
return proto.UnmarshalOptions{
+ Merge: true,
AllowPartial: true,
DiscardUnknown: o.DiscardUnknown(),
Resolver: o.Resolver(),
diff --git a/proto/decode.go b/proto/decode.go
index 8974139..ab62ff3 100644
--- a/proto/decode.go
+++ b/proto/decode.go
@@ -21,6 +21,11 @@
type UnmarshalOptions struct {
pragma.NoUnkeyedLiterals
+ // Merge merges the input into the destination message.
+ // The default behavior is to always reset the message before unmarshaling,
+ // unless Merge is specified.
+ Merge bool
+
// AllowPartial accepts input for messages that will result in missing
// required fields. If AllowPartial is false (the default), Unmarshal will
// return an error if there are any missing required fields.
@@ -49,7 +54,9 @@
o.Resolver = protoregistry.GlobalTypes
}
- // TODO: Reset m?
+ if !o.Merge {
+ Reset(m)
+ }
err := o.unmarshalMessage(b, m.ProtoReflect())
if err != nil {
return err
diff --git a/proto/proto_methods.go b/proto/proto_methods.go
index 50abbd6..86dde4a 100644
--- a/proto/proto_methods.go
+++ b/proto/proto_methods.go
@@ -12,6 +12,8 @@
"google.golang.org/protobuf/runtime/protoiface"
)
+const hasProtoMethods = true
+
func protoMethods(m protoreflect.Message) *protoiface.Methods {
if x, ok := m.(protoiface.Methoder); ok {
return x.ProtoMethods()
diff --git a/proto/proto_reflect.go b/proto/proto_reflect.go
index 79ec456..b103d43 100644
--- a/proto/proto_reflect.go
+++ b/proto/proto_reflect.go
@@ -12,6 +12,8 @@
"google.golang.org/protobuf/runtime/protoiface"
)
+const hasProtoMethods = false
+
func protoMethods(m protoreflect.Message) *protoiface.Methods {
return nil
}
diff --git a/proto/reset.go b/proto/reset.go
index f14d9ae..dcf70b9 100644
--- a/proto/reset.go
+++ b/proto/reset.go
@@ -9,7 +9,10 @@
// Reset clears every field in the message.
func Reset(m Message) {
// TODO: Document memory aliasing guarantees.
- // TODO: Add fast-path for reset?
+ if mr, ok := m.(interface{ Reset() }); ok && hasProtoMethods {
+ mr.Reset()
+ return
+ }
resetMessage(m.ProtoReflect())
}
diff --git a/runtime/protoiface/methods.go b/runtime/protoiface/methods.go
index 5984a26..e0adf9d 100644
--- a/runtime/protoiface/methods.go
+++ b/runtime/protoiface/methods.go
@@ -73,6 +73,7 @@
type UnmarshalOptions struct {
pragma.NoUnkeyedLiterals
+ Merge bool // must be treated as true by method implementations
AllowPartial bool // must be treated as true by method implementations
DiscardUnknown bool
Resolver interface {
