From 50f00eeb833b5a876875ac388b859c7d5c10c99b Mon Sep 17 00:00:00 2001 From: Michal Witkowski Date: Wed, 15 Mar 2017 21:46:59 +0000 Subject: [PATCH] Add nicejsonpb error handling (against dev) --- .gitignore | 3 ++ jsonpb/errors.go | 95 +++++++++++++++++++++++++++++++++++++++++++ jsonpb/jsonpb.go | 32 +++++++-------- jsonpb/jsonpb_test.go | 64 +++++++++++++++++++++++++++++ 4 files changed, 178 insertions(+), 16 deletions(-) create mode 100644 jsonpb/errors.go diff --git a/.gitignore b/.gitignore index c7dd40587c..7265d17e1e 100644 --- a/.gitignore +++ b/.gitignore @@ -13,5 +13,8 @@ _obj _test _testmain.go +# Ignore Gogland stuff +.idea/ + # Conformance test output and transient files. conformance/failing_tests.txt diff --git a/jsonpb/errors.go b/jsonpb/errors.go new file mode 100644 index 0000000000..05f593e54c --- /dev/null +++ b/jsonpb/errors.go @@ -0,0 +1,95 @@ +// Go support for Protocol Buffers - Google's data interchange format +// +// Copyright 2015 The Go Authors. All rights reserved. +// https://github.com/golang/protobuf +// +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are +// met: +// +// * Redistributions of source code must retain the above copyright +// notice, this list of conditions and the following disclaimer. +// * Redistributions in binary form must reproduce the above +// copyright notice, this list of conditions and the following disclaimer +// in the documentation and/or other materials provided with the +// distribution. +// * Neither the name of Google Inc. nor the names of its +// contributors may be used to endorse or promote products derived from +// this software without specific prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +package jsonpb + +import ( + "encoding/json" + "fmt" + "reflect" + "strings" + + "github.com/golang/protobuf/proto" +) + +type fieldError struct { + fieldStack []string + nestedErr error +} + +func (f *fieldError) Error() string { + return "unparsable field " + strings.Join(f.fieldStack, ".") + ": " + f.nestedErr.Error() +} + +func (f *fieldError) FieldStack() []string { + return f.fieldStack +} + +func (f *fieldError) UnderlyingError() error { + return f.nestedErr +} + +// newFieldError wraps a given error providing a message call stack. +func newFieldError(fieldName string, err error) error { + if fErr, ok := err.(*fieldError); ok { + fErr.fieldStack = append([]string{fieldName}, fErr.fieldStack...) + return err + } + return &fieldError{ + fieldStack: []string{fieldName}, + nestedErr: err, + } +} + +// correctJsonType gets rid of the dredded json.RawMessage errors and casts them to the right type. +func correctJsonType(err error, realType reflect.Type) error { + if uErr, ok := err.(*json.UnmarshalTypeError); ok { + uErr.Type = realType + return uErr + } + return err +} + +func getFieldMismatchError(remainingFields map[string]json.RawMessage, structProps *proto.StructProperties) error { + remaining := []string{} + for k, _ := range remainingFields { + remaining = append(remaining, k) + } + known := []string{} + for _, prop := range structProps.Prop { + jsonNames := acceptedJSONFieldNames(prop) + if strings.HasPrefix(jsonNames.camel, "XXX_") { + continue + } + known = append(known, jsonNames.camel) + } + return fmt.Errorf("fields %v do not exist in set of known fields %v", remaining, known) +} diff --git a/jsonpb/jsonpb.go b/jsonpb/jsonpb.go index ff368f33c5..7125f32e34 100644 --- a/jsonpb/jsonpb.go +++ b/jsonpb/jsonpb.go @@ -886,7 +886,7 @@ func (u *Unmarshaler) unmarshalValue(target reflect.Value, inputValue json.RawMe if targetType.Kind() == reflect.Struct { var jsonFields map[string]json.RawMessage if err := json.Unmarshal(inputValue, &jsonFields); err != nil { - return err + return correctJsonType(err, targetType) } consumeField := func(prop *proto.Properties) (json.RawMessage, bool) { @@ -924,7 +924,7 @@ func (u *Unmarshaler) unmarshalValue(target reflect.Value, inputValue json.RawMe } if err := u.unmarshalValue(target.Field(i), valueForField, sprops.Prop[i]); err != nil { - return err + return newFieldError(sprops.Prop[i].Name, err) } } // Check for any oneof fields. @@ -937,7 +937,7 @@ func (u *Unmarshaler) unmarshalValue(target reflect.Value, inputValue json.RawMe nv := reflect.New(oop.Type.Elem()) target.Field(oop.Field).Set(nv) if err := u.unmarshalValue(nv.Elem().Field(0), raw, oop.Prop); err != nil { - return err + return newFieldError(oop.Prop.Name, err) } } } @@ -953,22 +953,16 @@ func (u *Unmarshaler) unmarshalValue(target reflect.Value, inputValue json.RawMe delete(jsonFields, name) nv := reflect.New(reflect.TypeOf(ext.ExtensionType).Elem()) if err := u.unmarshalValue(nv.Elem(), raw, nil); err != nil { - return err + return newFieldError(name, err) } if err := proto.SetExtension(ep, ext, nv.Interface()); err != nil { - return err + return newFieldError(name, err) } } } } if !u.AllowUnknownFields && len(jsonFields) > 0 { - // Pick any field to be the scapegoat. - var f string - for fname := range jsonFields { - f = fname - break - } - return fmt.Errorf("unknown field %q in %v", f, targetType) + return getFieldMismatchError(jsonFields, sprops) } return nil } @@ -977,14 +971,14 @@ func (u *Unmarshaler) unmarshalValue(target reflect.Value, inputValue json.RawMe if targetType.Kind() == reflect.Slice && targetType.Elem().Kind() != reflect.Uint8 { var slc []json.RawMessage if err := json.Unmarshal(inputValue, &slc); err != nil { - return err + return correctJsonType(err, targetType) } if slc != nil { l := len(slc) target.Set(reflect.MakeSlice(targetType, l, l)) for i := 0; i < l; i++ { if err := u.unmarshalValue(target.Index(i), slc[i], prop); err != nil { - return err + return newFieldError(fmt.Sprintf("[%d]", i), err) } } } @@ -999,6 +993,12 @@ func (u *Unmarshaler) unmarshalValue(target reflect.Value, inputValue json.RawMe } if mp != nil { target.Set(reflect.MakeMap(targetType)) + if prop != nil { + // These could still be nil if the protobuf metadata is broken somehow. + // TODO: This won't work because the fields are unexported. + // We should probably just reparse them. + //keyprop, valprop = prop.mkeyprop, prop.mvalprop + } for ks, raw := range mp { // Unmarshal map key. The core json library already decoded the key into a // string, so we handle that specially. Other types were quoted post-serialization. @@ -1009,7 +1009,7 @@ func (u *Unmarshaler) unmarshalValue(target reflect.Value, inputValue json.RawMe k = reflect.New(targetType.Key()).Elem() // TODO: pass the correct Properties if needed. if err := u.unmarshalValue(k, json.RawMessage(ks), nil); err != nil { - return err + return newFieldError(fmt.Sprintf("['%s']", ks), newFieldError("key", err)) } } @@ -1017,7 +1017,7 @@ func (u *Unmarshaler) unmarshalValue(target reflect.Value, inputValue json.RawMe v := reflect.New(targetType.Elem()).Elem() // TODO: pass the correct Properties if needed. if err := u.unmarshalValue(v, raw, nil); err != nil { - return err + return newFieldError(fmt.Sprintf("['%s']", ks), newFieldError("value", err)) } target.SetMapIndex(k, v) } diff --git a/jsonpb/jsonpb_test.go b/jsonpb/jsonpb_test.go index c9934d9700..4c3f666f89 100644 --- a/jsonpb/jsonpb_test.go +++ b/jsonpb/jsonpb_test.go @@ -1148,3 +1148,67 @@ func TestUnmarshalUnsetRequiredFields(t *testing.T) { } } } + + +func TestUnmarshalReturnsUsefulErrors(t *testing.T) { + for _, entry := range []struct { + name string + input string + msg proto.Message + expectedError string + }{ + { + name: "FindsErrorsInNested", + input: `{"simple": {"oString": 3.1}}`, + msg: &pb.Widget{}, + expectedError: `unparsable field Simple.OString: json: cannot unmarshal number into Go value of type string`, + }, + { + name: "FindsErrorsInArrays", + input: `{"rInt32": [2,5,1,"sdas",2.1]}`, + msg: &pb.Repeats{}, + expectedError: `unparsable field RInt32.[3]: json: cannot unmarshal string into Go value of type int32`, + }, + { + name: "FindErrorsInMapKeys", + input: `{"mInt64Str": {"1": "something", "badKey": "foo"}}`, + msg: &pb.Maps{}, + expectedError: `unparsable field MInt64Str.['badKey'].key: invalid character 'b' looking for beginning of value`, + }, + { + name: "FindErrorsInMapValues", + input: `{"mInt64Str": {"1": "something", "2": 4.2}}`, + msg: &pb.Maps{}, + expectedError: `unparsable field MInt64Str.['2'].value: json: cannot unmarshal number into Go value of type string`, + }, + { + name: "HandlesGoodFormattingOfInt64AsString", + input: `{"oInt64": "should_be_int"}`, + msg: &pb.Simple{}, + expectedError: `unparsable field OInt64: invalid character 's' looking for beginning of value`, + }, + { + name: "RemapsRawMessageToRealArrayType", + input: `{"rInt32": "not_an_array"}`, + msg: &pb.Repeats{}, + expectedError: `unparsable field RInt32: json: cannot unmarshal string into Go value of type []int32`, + }, + { + name: "UnknownFieldErrors", + input: `{"simple": {"oString": "something", "unknownOne": 1, "unknownTwo": 21}}`, + msg: &pb.Widget{}, + expectedError: `unparsable field Simple: fields [unknownOne unknownTwo] do not exist in set of known fields [oBool oInt32 oInt64 oUint32 oUint64 oSint32 oSint64 oFloat oDouble oString oBytes]`, + }, + } { + u := Unmarshaler{AllowUnknownFields: false} + err := u.Unmarshal(strings.NewReader(entry.input), entry.msg) + if err == nil { + t.Errorf("in %v there should have been an error", entry.name) + continue + } + if err.Error() != entry.expectedError { + t.Errorf("in %v expected error \n'%v'\n but got \n'%v'", entry.name, entry.expectedError, err) + continue + } + } +} \ No newline at end of file