proto, internal/impl: make IsInitialized more consistent
Make the fast-path and slow-path versions of IsInitialized report
exactly the same errors: An errors.RequiredNotSet containing the
full name of one of the unset required fields.
Bugfix: Fast-path IsInitialized on a nil message reports an error only
when the message directly contains required fields.
Bugfix: Include fast-path IsInitialized in legacy messageIfaceWrapper.
Fixes golang/protobuf#887
Change-Id: Ia5e4b386f8c23f6f855d995f4a098b1338acbae3
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/185397
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
diff --git a/proto/decode_test.go b/proto/decode_test.go
index 3b919aa..03f337a 100644
--- a/proto/decode_test.go
+++ b/proto/decode_test.go
@@ -952,6 +952,11 @@
}.Marshal(),
},
{
+ desc: "required field in nil message unset",
+ partial: true,
+ decodeTo: []proto.Message{(*testpb.TestRequired)(nil)},
+ },
+ {
desc: "required field unset",
partial: true,
decodeTo: []proto.Message{&testpb.TestRequired{}},
@@ -1224,6 +1229,14 @@
}.Marshal(),
},
{
+ desc: "nil messages",
+ decodeTo: []proto.Message{
+ (*testpb.TestAllTypes)(nil),
+ (*test3pb.TestAllTypes)(nil),
+ (*testpb.TestAllExtensions)(nil),
+ },
+ },
+ {
desc: "legacy",
partial: true,
decodeTo: []proto.Message{
diff --git a/proto/doc.go b/proto/doc.go
new file mode 100644
index 0000000..0638dd5
--- /dev/null
+++ b/proto/doc.go
@@ -0,0 +1,9 @@
+// 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 proto needs documentation.
+//
+// TODO: Add package docs.
+// TODO: Document guarantees (or lack thereof) made about errors.
+package proto
diff --git a/proto/isinit.go b/proto/isinit.go
index 1bb29bd..e0fd947 100644
--- a/proto/isinit.go
+++ b/proto/isinit.go
@@ -5,9 +5,6 @@
package proto
import (
- "bytes"
- "fmt"
-
"google.golang.org/protobuf/internal/errors"
pref "google.golang.org/protobuf/reflect/protoreflect"
)
@@ -15,66 +12,46 @@
// IsInitialized returns an error if any required fields in m are not set.
func IsInitialized(m Message) error {
if methods := protoMethods(m); methods != nil && methods.IsInitialized != nil {
- if err := methods.IsInitialized(m); err == nil {
- return nil
- }
- // Fall through to the slow path, since the fast-path
- // implementation doesn't produce nice errors.
- //
- // TODO: Consider producing better errors from the fast path.
+ return methods.IsInitialized(m)
}
- return isInitialized(m.ProtoReflect(), nil)
+ return isInitialized(m.ProtoReflect())
}
// IsInitialized returns an error if any required fields in m are not set.
-func isInitialized(m pref.Message, stack []interface{}) error {
+func isInitialized(m pref.Message) error {
md := m.Descriptor()
fds := md.Fields()
for i, nums := 0, md.RequiredNumbers(); i < nums.Len(); i++ {
fd := fds.ByNumber(nums.Get(i))
if !m.Has(fd) {
- stack = append(stack, fd.Name())
- return newRequiredNotSetError(stack)
+ return errors.RequiredNotSet(string(fd.FullName()))
}
}
var err error
m.Range(func(fd pref.FieldDescriptor, v pref.Value) bool {
- // Recurse into fields containing message values.
- stack := append(stack, fd.Name())
switch {
case fd.IsList():
if fd.Message() == nil {
return true
}
for i, list := 0, v.List(); i < list.Len() && err == nil; i++ {
- stack := append(stack, "[", i, "].")
- err = isInitialized(list.Get(i).Message(), stack)
+ err = IsInitialized(list.Get(i).Message().Interface())
}
case fd.IsMap():
if fd.MapValue().Message() == nil {
return true
}
v.Map().Range(func(key pref.MapKey, v pref.Value) bool {
- stack := append(stack, "[", key, "].")
- err = isInitialized(v.Message(), stack)
+ err = IsInitialized(v.Message().Interface())
return err == nil
})
default:
if fd.Message() == nil {
return true
}
- stack := append(stack, ".")
- err = isInitialized(v.Message(), stack)
+ err = IsInitialized(v.Message().Interface())
}
return err == nil
})
return err
}
-
-func newRequiredNotSetError(stack []interface{}) error {
- var buf bytes.Buffer
- for _, s := range stack {
- fmt.Fprint(&buf, s)
- }
- return errors.RequiredNotSet(buf.String())
-}
diff --git a/proto/isinit_test.go b/proto/isinit_test.go
index 232202f..1edbfb4 100644
--- a/proto/isinit_test.go
+++ b/proto/isinit_test.go
@@ -21,13 +21,13 @@
}{
{
&testpb.TestRequired{},
- `proto: required field required_field not set`,
+ `proto: required field goproto.proto.test.TestRequired.required_field not set`,
},
{
&testpb.TestRequiredForeign{
OptionalMessage: &testpb.TestRequired{},
},
- `proto: required field optional_message.required_field not set`,
+ `proto: required field goproto.proto.test.TestRequired.required_field not set`,
},
{
&testpb.TestRequiredForeign{
@@ -36,7 +36,7 @@
{},
},
},
- `proto: required field repeated_message[1].required_field not set`,
+ `proto: required field goproto.proto.test.TestRequired.required_field not set`,
},
{
&testpb.TestRequiredForeign{
@@ -44,7 +44,7 @@
1: {},
},
},
- `proto: required field map_message[1].required_field not set`,
+ `proto: required field goproto.proto.test.TestRequired.required_field not set`,
},
} {
err := proto.IsInitialized(test.m)