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/internal/impl/isinit.go b/internal/impl/isinit.go
index 29f9ad8..a533cf4 100644
--- a/internal/impl/isinit.go
+++ b/internal/impl/isinit.go
@@ -7,15 +7,11 @@
import (
"sync"
+ "google.golang.org/protobuf/internal/errors"
"google.golang.org/protobuf/proto"
pref "google.golang.org/protobuf/reflect/protoreflect"
)
-type errRequiredNotSet struct{}
-
-func (errRequiredNotSet) Error() string { return "proto: required field not set" }
-func (errRequiredNotSet) RequiredNotSet() bool { return true }
-
func (mi *MessageInfo) isInitialized(msg proto.Message) error {
return mi.isInitializedPointer(pointerOfIface(msg))
}
@@ -26,7 +22,12 @@
return nil
}
if p.IsNil() {
- return errRequiredNotSet{}
+ for _, f := range mi.orderedCoderFields {
+ if f.isRequired {
+ return errors.RequiredNotSet(string(mi.PBType.Fields().ByNumber(f.num).FullName()))
+ }
+ }
+ return nil
}
if mi.extensionOffset.IsValid() {
e := p.Apply(mi.extensionOffset).Extensions()
@@ -41,7 +42,7 @@
fptr := p.Apply(f.offset)
if f.isPointer && fptr.Elem().IsNil() {
if f.isRequired {
- return errRequiredNotSet{}
+ return errors.RequiredNotSet(string(mi.PBType.Fields().ByNumber(f.num).FullName()))
}
continue
}
diff --git a/internal/impl/message.go b/internal/impl/message.go
index ec9be4e..e9a38c2 100644
--- a/internal/impl/message.go
+++ b/internal/impl/message.go
@@ -517,6 +517,7 @@
Flags: piface.MethodFlagDeterministicMarshal,
MarshalAppend: m.marshalAppend,
Size: m.size,
+ IsInitialized: m.isInitialized,
}
}
func (m *messageIfaceWrapper) ProtoUnwrap() interface{} {
@@ -528,3 +529,6 @@
func (m *messageIfaceWrapper) size(msg pref.ProtoMessage) (size int) {
return m.mi.sizePointer(m.p, 0)
}
+func (m *messageIfaceWrapper) isInitialized(msg pref.ProtoMessage) error {
+ return m.mi.isInitializedPointer(m.p)
+}
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)
