From 09217f08d2e2a27f9e654798c1f2f106e64ae9b8 Mon Sep 17 00:00:00 2001 From: Joe Tsai Date: Fri, 6 Sep 2019 16:57:46 -0700 Subject: [PATCH] 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 --- internal/cmd/pbdump/pbdump.go | 7 ++++--- internal/errors/errors.go | 13 ++++++++++++- internal/impl/codec_tables.go | 4 ++-- internal/impl/message_reflect.go | 2 +- proto/isinit_test.go | 14 +++++++------- reflect/protodesc/file_test.go | 2 +- 6 files changed, 27 insertions(+), 15 deletions(-) diff --git a/internal/cmd/pbdump/pbdump.go b/internal/cmd/pbdump/pbdump.go index 71912b1e..62e3a274 100644 --- a/internal/cmd/pbdump/pbdump.go +++ b/internal/cmd/pbdump/pbdump.go @@ -19,6 +19,7 @@ import ( "google.golang.org/protobuf/internal/encoding/pack" "google.golang.org/protobuf/internal/encoding/wire" + "google.golang.org/protobuf/internal/errors" "google.golang.org/protobuf/proto" "google.golang.org/protobuf/reflect/protodesc" "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) num := wire.Number(n) 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:], ".") @@ -184,7 +185,7 @@ func (fs fields) set(prefix, s string, k protoreflect.Kind) error { } if len(s) == 0 { 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 } @@ -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. k2 := fs[num].kind 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 } diff --git a/internal/errors/errors.go b/internal/errors/errors.go index e22035ef..db1005df 100644 --- a/internal/errors/errors.go +++ b/internal/errors/errors.go @@ -7,6 +7,8 @@ package errors import ( "fmt" + + "google.golang.org/protobuf/internal/detrand" ) // 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 } -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 { return New("field %v contains invalid UTF-8", name) diff --git a/internal/impl/codec_tables.go b/internal/impl/codec_tables.go index 77fc4c0c..f421c782 100644 --- a/internal/impl/codec_tables.go +++ b/internal/impl/codec_tables.go @@ -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 @@ -547,5 +547,5 @@ func encoderFuncsForValue(fd pref.FieldDescriptor) valueCoderFuncs { 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())) } diff --git a/internal/impl/message_reflect.go b/internal/impl/message_reflect.go index b34e479a..04e8f7ca 100644 --- a/internal/impl/message_reflect.go +++ b/internal/impl/message_reflect.go @@ -145,7 +145,7 @@ func (m *extensionMap) Get(xt pref.ExtensionType) pref.Value { } func (m *extensionMap) Set(xt pref.ExtensionType, v pref.Value) { 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 { *m = make(map[int32]ExtensionField) diff --git a/proto/isinit_test.go b/proto/isinit_test.go index 72dd1087..d935d3a0 100644 --- a/proto/isinit_test.go +++ b/proto/isinit_test.go @@ -6,6 +6,7 @@ package proto_test import ( "fmt" + "strings" "testing" "google.golang.org/protobuf/proto" @@ -20,13 +21,13 @@ func TestIsInitializedErrors(t *testing.T) { }{ { &testpb.TestRequired{}, - `proto: required field goproto.proto.test.TestRequired.required_field not set`, + `goproto.proto.test.TestRequired.required_field`, }, { &testpb.TestRequiredForeign{ OptionalMessage: &testpb.TestRequired{}, }, - `proto: required field goproto.proto.test.TestRequired.required_field not set`, + `goproto.proto.test.TestRequired.required_field`, }, { &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{ @@ -43,7 +44,7 @@ func TestIsInitializedErrors(t *testing.T) { 1: {}, }, }, - `proto: required field goproto.proto.test.TestRequired.required_field not set`, + `goproto.proto.test.TestRequired.required_field`, }, } { err := proto.IsInitialized(test.m) @@ -51,9 +52,8 @@ func TestIsInitializedErrors(t *testing.T) { if err != nil { got = fmt.Sprintf("%q", err) } - want := fmt.Sprintf("%q", test.want) - if got != want { - t.Errorf("IsInitialized(m):\n got: %v\nwant: %v\nMessage:\n%v", got, want, marshalText(test.m)) + if !strings.Contains(got, test.want) { + t.Errorf("IsInitialized(m):\n got: %v\nwant contains: %v\nMessage:\n%v", got, test.want, marshalText(test.m)) } } } diff --git a/reflect/protodesc/file_test.go b/reflect/protodesc/file_test.go index 5319b526..38e8b4c6 100644 --- a/reflect/protodesc/file_test.go +++ b/reflect/protodesc/file_test.go @@ -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", inDesc: mustParseFile(`