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,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)
 	}

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)
 		}
 	})
 }