all: make error messages unstable

Use internal/detrand in the construction of our error messages.
This alters whether there is one or two spaces following the "proto:" prefix.
While it is easy for users to still work around this mutation,
sit at least forces them to write test infrastructure to more fuzzily
match on error strings.

Change-Id: I4ddca717526ee3fc4dbb1e0b36cfca8c6e0df36d
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/194038
Reviewed-by: Herbie Ong <herbie@google.com>
This commit is contained in:
Joe Tsai 2019-09-06 16:57:46 -07:00
parent 956cd6ddcd
commit 09217f08d2
6 changed files with 27 additions and 15 deletions

View File

@ -19,6 +19,7 @@ import (
"google.golang.org/protobuf/internal/encoding/pack" "google.golang.org/protobuf/internal/encoding/pack"
"google.golang.org/protobuf/internal/encoding/wire" "google.golang.org/protobuf/internal/encoding/wire"
"google.golang.org/protobuf/internal/errors"
"google.golang.org/protobuf/proto" "google.golang.org/protobuf/proto"
"google.golang.org/protobuf/reflect/protodesc" "google.golang.org/protobuf/reflect/protodesc"
"google.golang.org/protobuf/reflect/protoreflect" "google.golang.org/protobuf/reflect/protoreflect"
@ -174,7 +175,7 @@ func (fs fields) set(prefix, s string, k protoreflect.Kind) error {
n, _ := strconv.ParseInt(s[:i], 10, 32) n, _ := strconv.ParseInt(s[:i], 10, 32)
num := wire.Number(n) num := wire.Number(n)
if num < wire.MinValidNumber || wire.MaxValidNumber < num { if num < wire.MinValidNumber || wire.MaxValidNumber < num {
return fmt.Errorf("invalid field: %v", prefix) return errors.New("invalid field: %v", prefix)
} }
s = strings.TrimPrefix(s[i:], ".") s = strings.TrimPrefix(s[i:], ".")
@ -184,7 +185,7 @@ func (fs fields) set(prefix, s string, k protoreflect.Kind) error {
} }
if len(s) == 0 { if len(s) == 0 {
if fs[num].kind.IsValid() { if fs[num].kind.IsValid() {
return fmt.Errorf("field %v already set as %v type", prefix, fs[num].kind) return errors.New("field %v already set as %v type", prefix, fs[num].kind)
} }
fs[num].kind = k fs[num].kind = k
} }
@ -195,7 +196,7 @@ func (fs fields) set(prefix, s string, k protoreflect.Kind) error {
// Verify that only messages or groups can have sub-fields. // Verify that only messages or groups can have sub-fields.
k2 := fs[num].kind k2 := fs[num].kind
if k2 > 0 && k2 != protoreflect.MessageKind && k2 != protoreflect.GroupKind && len(fs[num].sub) > 0 { if k2 > 0 && k2 != protoreflect.MessageKind && k2 != protoreflect.GroupKind && len(fs[num].sub) > 0 {
return fmt.Errorf("field %v of %v type cannot have sub-fields", prefix, k2) return errors.New("field %v of %v type cannot have sub-fields", prefix, k2)
} }
return nil return nil
} }

View File

@ -7,6 +7,8 @@ package errors
import ( import (
"fmt" "fmt"
"google.golang.org/protobuf/internal/detrand"
) )
// New formats a string according to the format specifier and arguments and // New formats a string according to the format specifier and arguments and
@ -22,7 +24,16 @@ func New(f string, x ...interface{}) error {
type prefixError struct{ s string } type prefixError struct{ s string }
func (e *prefixError) Error() string { return "proto: " + e.s } var prefix = func() string {
if detrand.Bool() {
return "proto: "
}
return "proto: "
}()
func (e *prefixError) Error() string {
return prefix + e.s
}
func InvalidUTF8(name string) error { func InvalidUTF8(name string) error {
return New("field %v contains invalid UTF-8", name) return New("field %v contains invalid UTF-8", name)

View File

@ -423,7 +423,7 @@ func fieldCoder(fd pref.FieldDescriptor, ft reflect.Type) pointerCoderFuncs {
} }
} }
} }
panic(fmt.Errorf("invalid type: no encoder for %v %v %v/%v", fd.FullName(), fd.Cardinality(), fd.Kind(), ft)) panic(fmt.Sprintf("invalid type: no encoder for %v %v %v/%v", fd.FullName(), fd.Cardinality(), fd.Kind(), ft))
} }
// encoderFuncsForValue returns value functions for a field, used for // encoderFuncsForValue returns value functions for a field, used for
@ -547,5 +547,5 @@ func encoderFuncsForValue(fd pref.FieldDescriptor) valueCoderFuncs {
return coderGroupValue return coderGroupValue
} }
} }
panic(fmt.Errorf("invalid field: no encoder for %v %v %v", fd.FullName(), fd.Cardinality(), fd.Kind())) panic(fmt.Sprintf("invalid field: no encoder for %v %v %v", fd.FullName(), fd.Cardinality(), fd.Kind()))
} }

View File

@ -145,7 +145,7 @@ func (m *extensionMap) Get(xt pref.ExtensionType) pref.Value {
} }
func (m *extensionMap) Set(xt pref.ExtensionType, v pref.Value) { func (m *extensionMap) Set(xt pref.ExtensionType, v pref.Value) {
if !xt.IsValidValue(v) { if !xt.IsValidValue(v) {
panic(fmt.Errorf("%v: assigning invalid value", xt.TypeDescriptor().FullName())) panic(fmt.Sprintf("%v: assigning invalid value", xt.TypeDescriptor().FullName()))
} }
if *m == nil { if *m == nil {
*m = make(map[int32]ExtensionField) *m = make(map[int32]ExtensionField)

View File

@ -6,6 +6,7 @@ package proto_test
import ( import (
"fmt" "fmt"
"strings"
"testing" "testing"
"google.golang.org/protobuf/proto" "google.golang.org/protobuf/proto"
@ -20,13 +21,13 @@ func TestIsInitializedErrors(t *testing.T) {
}{ }{
{ {
&testpb.TestRequired{}, &testpb.TestRequired{},
`proto: required field goproto.proto.test.TestRequired.required_field not set`, `goproto.proto.test.TestRequired.required_field`,
}, },
{ {
&testpb.TestRequiredForeign{ &testpb.TestRequiredForeign{
OptionalMessage: &testpb.TestRequired{}, OptionalMessage: &testpb.TestRequired{},
}, },
`proto: required field goproto.proto.test.TestRequired.required_field not set`, `goproto.proto.test.TestRequired.required_field`,
}, },
{ {
&testpb.TestRequiredForeign{ &testpb.TestRequiredForeign{
@ -35,7 +36,7 @@ func TestIsInitializedErrors(t *testing.T) {
{}, {},
}, },
}, },
`proto: required field goproto.proto.test.TestRequired.required_field not set`, `goproto.proto.test.TestRequired.required_field`,
}, },
{ {
&testpb.TestRequiredForeign{ &testpb.TestRequiredForeign{
@ -43,7 +44,7 @@ func TestIsInitializedErrors(t *testing.T) {
1: {}, 1: {},
}, },
}, },
`proto: required field goproto.proto.test.TestRequired.required_field not set`, `goproto.proto.test.TestRequired.required_field`,
}, },
} { } {
err := proto.IsInitialized(test.m) err := proto.IsInitialized(test.m)
@ -51,9 +52,8 @@ func TestIsInitializedErrors(t *testing.T) {
if err != nil { if err != nil {
got = fmt.Sprintf("%q", err) got = fmt.Sprintf("%q", err)
} }
want := fmt.Sprintf("%q", test.want) if !strings.Contains(got, test.want) {
if got != want { t.Errorf("IsInitialized(m):\n got: %v\nwant contains: %v\nMessage:\n%v", got, test.want, marshalText(test.m))
t.Errorf("IsInitialized(m):\n got: %v\nwant: %v\nMessage:\n%v", got, want, marshalText(test.m))
} }
} }
} }

View File

@ -819,7 +819,7 @@ func TestNewFile(t *testing.T) {
] ]
}]}] }]}]
`), `),
wantErr: `proto: message "M.M" using proto3 semantics has conflict: "baz" with "_b_a_z_"`, wantErr: `message "M.M" using proto3 semantics has conflict: "baz" with "_b_a_z_"`,
}, { }, {
label: "proto3 message fields", label: "proto3 message fields",
inDesc: mustParseFile(` inDesc: mustParseFile(`