Skip to content

Commit

Permalink
node/bindnode: enforce pointer requirement for nullable maps
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
mvdan committed Mar 8, 2022
1 parent 4a1bd4f commit 95f4357
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 2 deletions.
13 changes: 11 additions & 2 deletions node/bindnode/infer.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
23 changes: 23 additions & 0 deletions node/bindnode/infer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand Down

0 comments on commit 95f4357

Please sign in to comment.