From 95f435754a41e0f858044923a02bb11c40024ea5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Mart=C3=AD?= <mvdan@mvdan.cc>
Date: Mon, 7 Mar 2022 12:56:31 +0000
Subject: [PATCH] node/bindnode: enforce pointer requirement for nullable maps

The implementation expected map value types in Go to be a pointer
when the IPLD schema has the value as nullable.
Unfortunately, we forgot to enforce that in verifyCompatibility,
so the user could run into weird panics:

	panic: interface conversion: basicnode.plainList is not datamodel.Node: missing method Length

The added test case in TestPrototypePointerCombinations reproduces the
panic, but is commented out, because we're fixing the check logic to
instead have bindnode panic upfront with a helpful message.

Note that I understand why the user tried to avoid the pointer to Node.
Since Node is an interface, it is already nilable, so bindnode can store
a nil value in it directly without the need for a pointer.
We do not support that right now, but soon will; #378 now tracks that.

Fixes #377.
---
 node/bindnode/infer.go      | 13 +++++++++++--
 node/bindnode/infer_test.go | 23 +++++++++++++++++++++++
 2 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/node/bindnode/infer.go b/node/bindnode/infer.go
index 65afabfc..c2a4c70f 100644
--- a/node/bindnode/infer.go
+++ b/node/bindnode/infer.go
@@ -135,8 +135,17 @@ func verifyCompatibility(seen map[seenEntry]bool, goType reflect.Type, schemaTyp
 		if fieldValues.Type.Kind() != reflect.Map {
 			doPanic("kind mismatch; need struct{Keys []K; Values map[K]V}")
 		}
-		verifyCompatibility(seen, fieldValues.Type.Key(), schemaType.KeyType())
-		verifyCompatibility(seen, fieldValues.Type.Elem(), schemaType.ValueType())
+		keyType := fieldValues.Type.Key()
+		verifyCompatibility(seen, keyType, schemaType.KeyType())
+
+		elemType := fieldValues.Type.Elem()
+		if schemaType.ValueIsNullable() {
+			if elemType.Kind() != reflect.Ptr {
+				doPanic("nullable types must be pointers")
+			}
+			elemType = elemType.Elem()
+		}
+		verifyCompatibility(seen, elemType, schemaType.ValueType())
 	case *schema.TypeStruct:
 		if goType.Kind() != reflect.Struct {
 			doPanic("kind mismatch; need struct")
diff --git a/node/bindnode/infer_test.go b/node/bindnode/infer_test.go
index cc9f4aa3..f2bbd6c3 100644
--- a/node/bindnode/infer_test.go
+++ b/node/bindnode/infer_test.go
@@ -322,6 +322,12 @@ func TestPrototypePointerCombinations(t *testing.T) {
 		{"Link_Generic", "Link", (*datamodel.Link)(nil), `{"/": "bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi"}`},
 		{"List_String", "[String]", (*[]string)(nil), `["foo", "bar"]`},
 		{"Any_Node_Int", "Any", (*datamodel.Node)(nil), `23`},
+		// TODO: reenable once we don't require pointers for nullable
+		// {"Any_Pointer_Int", "{String: nullable Any}",
+		// 	(*struct {
+		// 		Keys   []string
+		// 		Values map[string]datamodel.Node
+		// 	})(nil), `{"x":3,"y":"bar","z":[2.3,4.5]}`},
 		{"Map_String_Int", "{String:Int}", (*struct {
 			Keys   []string
 			Values map[string]int64
@@ -881,6 +887,23 @@ var verifyTests = []struct {
 			})(nil), `.*type Root .*: kind mismatch;.*`},
 		},
 	},
+	{
+		name:      "MapNullableAny",
+		schemaSrc: `type Root {String:nullable Any}`,
+		goodTypes: []interface{}{
+			(*struct {
+				Keys   []string
+				Values map[string]*datamodel.Node
+			})(nil),
+		},
+		badTypes: []verifyBadType{
+			{(*string)(nil), `.*type Root .* type string: kind mismatch;.*`},
+			{(*struct {
+				Keys   []string
+				Values map[string]datamodel.Node
+			})(nil), `.*type Root .*: nullable types must be pointers`},
+		},
+	},
 	{
 		name: "Union",
 		schemaSrc: `type Root union {