From 72c2e6a5b81bdffb6ecac96280ce70279a2a3b9e Mon Sep 17 00:00:00 2001 From: "M. J. Fromberger" Date: Fri, 21 Jan 2022 09:51:21 -0800 Subject: [PATCH] jsontypes: improve tests and error diagnostics (#7669) Avert panics for corner cases (e.g., nil pointers) and for implementations that reside only on the pointer type. Add documentation and tests. --- internal/jsontypes/jsontypes.go | 25 ++--- internal/jsontypes/jsontypes_test.go | 137 +++++++++++++++++++++++---- 2 files changed, 135 insertions(+), 27 deletions(-) diff --git a/internal/jsontypes/jsontypes.go b/internal/jsontypes/jsontypes.go index a4c3c53ff..16095ee2f 100644 --- a/internal/jsontypes/jsontypes.go +++ b/internal/jsontypes/jsontypes.go @@ -25,8 +25,7 @@ type Tagged interface { TypeTag() string } -// registry records the mapping from type tags to value types. Values in this -// map must be normalized to non-pointer types. +// registry records the mapping from type tags to value types. var registry = struct { types map[string]reflect.Type }{types: make(map[string]reflect.Type)} @@ -38,11 +37,7 @@ func register(v Tagged) error { if t, ok := registry.types[tag]; ok { return fmt.Errorf("type tag %q already registered to %v", tag, t) } - typ := reflect.TypeOf(v) - if typ.Kind() == reflect.Ptr { - typ = typ.Elem() - } - registry.types[tag] = typ + registry.types[tag] = reflect.TypeOf(v) return nil } @@ -85,7 +80,10 @@ func Unmarshal(data []byte, v interface{}) error { target := reflect.ValueOf(v) if target.Kind() != reflect.Ptr { return fmt.Errorf("target %T is not a pointer", v) + } else if target.IsZero() { + return fmt.Errorf("target is a nil %T", v) } + baseType := target.Type().Elem() var w wrapper dec := json.NewDecoder(bytes.NewReader(data)) @@ -96,11 +94,16 @@ func Unmarshal(data []byte, v interface{}) error { typ, ok := registry.types[w.Type] if !ok { return fmt.Errorf("unknown type tag: %q", w.Type) - } else if !typ.AssignableTo(target.Elem().Type()) { - return fmt.Errorf("type %v not assignable to %T", typ, v) } - - obj := reflect.New(typ) + if typ.AssignableTo(baseType) { + // ok: registered type is directly assignable to the target + } else if typ.Kind() == reflect.Ptr && typ.Elem().AssignableTo(baseType) { + typ = typ.Elem() + // ok: registered type is a pointer to a value assignable to the target + } else { + return fmt.Errorf("type %v is not assignable to %v", typ, baseType) + } + obj := reflect.New(typ) // we need a pointer to unmarshal if err := json.Unmarshal(w.Value, obj.Interface()); err != nil { return fmt.Errorf("decoding wrapped value: %w", err) } diff --git a/internal/jsontypes/jsontypes_test.go b/internal/jsontypes/jsontypes_test.go index b721e2b84..223e25c34 100644 --- a/internal/jsontypes/jsontypes_test.go +++ b/internal/jsontypes/jsontypes_test.go @@ -6,35 +6,44 @@ import ( "github.com/tendermint/tendermint/internal/jsontypes" ) -type testType struct { +type testPtrType struct { Field string `json:"field"` } -func (*testType) TypeTag() string { return "test/TaggedType" } +func (*testPtrType) TypeTag() string { return "test/PointerType" } +func (t *testPtrType) Value() string { return t.Field } -func TestRoundTrip(t *testing.T) { - const wantEncoded = `{"type":"test/TaggedType","value":{"field":"hello"}}` +type testBareType struct { + Field string `json:"field"` +} - t.Run("MustRegisterOK", func(t *testing.T) { +func (testBareType) TypeTag() string { return "test/BareType" } +func (t testBareType) Value() string { return t.Field } + +type fielder interface{ Value() string } + +func TestRoundTrip(t *testing.T) { + t.Run("MustRegister_ok", func(t *testing.T) { defer func() { if x := recover(); x != nil { t.Fatalf("Registration panicked: %v", x) } }() - jsontypes.MustRegister((*testType)(nil)) + jsontypes.MustRegister((*testPtrType)(nil)) + jsontypes.MustRegister(testBareType{}) }) - t.Run("MustRegisterFail", func(t *testing.T) { + t.Run("MustRegister_fail", func(t *testing.T) { defer func() { if x := recover(); x != nil { t.Logf("Got expected panic: %v", x) } }() - jsontypes.MustRegister((*testType)(nil)) + jsontypes.MustRegister((*testPtrType)(nil)) t.Fatal("Registration should not have succeeded") }) - t.Run("MarshalNil", func(t *testing.T) { + t.Run("Marshal_nilTagged", func(t *testing.T) { bits, err := jsontypes.Marshal(nil) if err != nil { t.Fatalf("Marshal failed: %v", err) @@ -44,8 +53,10 @@ func TestRoundTrip(t *testing.T) { } }) - t.Run("RoundTrip", func(t *testing.T) { - obj := testType{Field: "hello"} + t.Run("RoundTrip_pointerType", func(t *testing.T) { + const wantEncoded = `{"type":"test/PointerType","value":{"field":"hello"}}` + + obj := testPtrType{Field: "hello"} bits, err := jsontypes.Marshal(&obj) if err != nil { t.Fatalf("Marshal %T failed: %v", obj, err) @@ -54,7 +65,7 @@ func TestRoundTrip(t *testing.T) { t.Errorf("Marshal %T: got %#q, want %#q", obj, got, wantEncoded) } - var cmp testType + var cmp testPtrType if err := jsontypes.Unmarshal(bits, &cmp); err != nil { t.Errorf("Unmarshal %#q failed: %v", string(bits), err) } @@ -63,8 +74,10 @@ func TestRoundTrip(t *testing.T) { } }) - t.Run("Unregistered", func(t *testing.T) { - obj := testType{Field: "hello"} + t.Run("RoundTrip_bareType", func(t *testing.T) { + const wantEncoded = `{"type":"test/BareType","value":{"field":"hello"}}` + + obj := testBareType{Field: "hello"} bits, err := jsontypes.Marshal(&obj) if err != nil { t.Fatalf("Marshal %T failed: %v", obj, err) @@ -73,11 +86,103 @@ func TestRoundTrip(t *testing.T) { t.Errorf("Marshal %T: got %#q, want %#q", obj, got, wantEncoded) } + var cmp testBareType + if err := jsontypes.Unmarshal(bits, &cmp); err != nil { + t.Errorf("Unmarshal %#q failed: %v", string(bits), err) + } + if obj != cmp { + t.Errorf("Unmarshal %#q: got %+v, want %+v", string(bits), cmp, obj) + } + }) + + t.Run("Unmarshal_nilPointer", func(t *testing.T) { + var obj *testBareType + + // Unmarshaling to a nil pointer target should report an error. + if err := jsontypes.Unmarshal([]byte(`null`), obj); err == nil { + t.Errorf("Unmarshal nil: got %+v, wanted error", obj) + } else { + t.Logf("Unmarshal correctly failed: %v", err) + } + }) + + t.Run("Unmarshal_bareType", func(t *testing.T) { + const want = "foobar" + const input = `{"type":"test/BareType","value":{"field":"` + want + `"}}` + + var obj testBareType + if err := jsontypes.Unmarshal([]byte(input), &obj); err != nil { + t.Fatalf("Unmarshal failed: %v", err) + } + if obj.Field != want { + t.Errorf("Unmarshal result: got %q, want %q", obj.Field, want) + } + }) + + t.Run("Unmarshal_bareType_interface", func(t *testing.T) { + const want = "foobar" + const input = `{"type":"test/BareType","value":{"field":"` + want + `"}}` + + var obj fielder + if err := jsontypes.Unmarshal([]byte(input), &obj); err != nil { + t.Fatalf("Unmarshal failed: %v", err) + } + if got := obj.Value(); got != want { + t.Errorf("Unmarshal result: got %q, want %q", got, want) + } + }) + + t.Run("Unmarshal_pointerType", func(t *testing.T) { + const want = "bazquux" + const input = `{"type":"test/PointerType","value":{"field":"` + want + `"}}` + + var obj testPtrType + if err := jsontypes.Unmarshal([]byte(input), &obj); err != nil { + t.Fatalf("Unmarshal failed: %v", err) + } + if obj.Field != want { + t.Errorf("Unmarshal result: got %q, want %q", obj.Field, want) + } + }) + + t.Run("Unmarshal_pointerType_interface", func(t *testing.T) { + const want = "foobar" + const input = `{"type":"test/PointerType","value":{"field":"` + want + `"}}` + + var obj fielder + if err := jsontypes.Unmarshal([]byte(input), &obj); err != nil { + t.Fatalf("Unmarshal failed: %v", err) + } + if got := obj.Value(); got != want { + t.Errorf("Unmarshal result: got %q, want %q", got, want) + } + }) + + t.Run("Unmarshal_unknownTypeTag", func(t *testing.T) { + const input = `{"type":"test/Nonesuch","value":null}` + + // An unregistered type tag in a valid envelope should report an error. + var obj interface{} + if err := jsontypes.Unmarshal([]byte(input), &obj); err == nil { + t.Errorf("Unmarshal: got %+v, wanted error", obj) + } else { + t.Logf("Unmarshal correctly failed: %v", err) + } + }) + + t.Run("Unmarshal_similarTarget", func(t *testing.T) { + const want = "zootie-zoot-zoot" + const input = `{"type":"test/PointerType","value":{"field":"` + want + `"}}` + + // The target has a compatible (i.e., assignable) shape to the registered + // type. This should work even though it's not the original named type. var cmp struct { Field string `json:"field"` } - if err := jsontypes.Unmarshal(bits, &cmp); err != nil { - t.Errorf("Unmarshal %#q: got %+v, want %+v", string(bits), cmp, obj) + if err := jsontypes.Unmarshal([]byte(input), &cmp); err != nil { + t.Errorf("Unmarshal %#q failed: %v", input, err) + } else if cmp.Field != want { + t.Errorf("Unmarshal result: got %q, want %q", cmp.Field, want) } }) }