Skip to content

Commit

Permalink
node/bindnode: improve support for pointer types
Browse files Browse the repository at this point in the history
In particular, write an extensive test that exercises all the edge
cases we care about:

1) Each schema type, and its corresponding Go type
   (including multiple Go types where appropriate, e.g. links)

2) Each modifier: none, "optional", and "nullable"

3) Each data to decode: present, missing (absent), null

The fix is to dereference or initialize pointers as needed.
Some more edge cases remain, such as other schema types and non-default
representation strategies; a TODO tracks those.

While here, we improve the errors returned by AssignLink,
so that it now reports a bindnode-specific error
when the schema type is a Link but we can't assign to the Go value.
  • Loading branch information
mvdan committed Jan 20, 2022
1 parent 1930bab commit ac5b5fd
Show file tree
Hide file tree
Showing 4 changed files with 184 additions and 63 deletions.
5 changes: 5 additions & 0 deletions node/bindnode/infer.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,11 @@ type seenEntry struct {
}

func verifyCompatibility(seen map[seenEntry]bool, goType reflect.Type, schemaType schema.Type) {
// TODO(mvdan): support **T as well?
if goType.Kind() == reflect.Ptr {
goType = goType.Elem()
}

// Avoid endless loops.
//
// TODO(mvdan): this is easy but fairly allocation-happy.
Expand Down
98 changes: 97 additions & 1 deletion node/bindnode/infer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"bytes"
"encoding/json"
"fmt"
"html/template"
"io/ioutil"
"os/exec"
"path/filepath"
Expand Down Expand Up @@ -79,8 +80,8 @@ var prototypeTests = []struct {
linkImpl Link
}`,
ptrType: (*struct {
LinkGeneric datamodel.Link
LinkCID cid.Cid
LinkGeneric datamodel.Link
LinkImpl cidlink.Link
})(nil),
prettyDagJSON: `{
Expand Down Expand Up @@ -291,6 +292,101 @@ func TestPrototype(t *testing.T) {
}
}

func TestPrototypePointerCombinations(t *testing.T) {
t.Parallel()

// TODO: Null
// TODO: cover more schema types and repr strategies.
// Some of them are still using w.val directly without "nonPtr" calls.
kindTests := []struct {
name string
schemaType string
fieldPtrType interface{}
fieldDagJSON string
}{
{"Bool", "Bool", (*bool)(nil), `true`},
{"Int", "Int", (*int64)(nil), `23`},
{"Float", "Float", (*float64)(nil), `34.5`},
{"String", "String", (*string)(nil), `"foo"`},
{"Bytes", "Bytes", (*[]byte)(nil), `{"/": {"bytes": "34cd"}}`},
{"Link_CID", "Link", (*cid.Cid)(nil), `{"/": "bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi"}`},
{"Link_Impl", "Link", (*cidlink.Link)(nil), `{"/": "bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi"}`},
{"Link_Generic", "Link", (*datamodel.Link)(nil), `{"/": "bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi"}`},
{"List_String", "[String]", (*[]string)(nil), `["foo", "bar"]`},
{"Map_String_Int", "{String:Int}", (*struct {
Keys []string
Values map[string]int64
})(nil), `{"x":3,"y":4}`},
}

for _, kindTest := range kindTests {
for _, modifier := range []string{"", "optional", "nullable"} {
// don't reuse range vars
kindTest := kindTest
modifier := modifier
t.Run(fmt.Sprintf("%s/%s", kindTest.name, modifier), func(t *testing.T) {
t.Parallel()

var buf bytes.Buffer
err := template.Must(template.New("").Parse(`
type Root struct {
field {{.Modifier}} {{.Type}}
}`)).Execute(&buf, struct {
Type, Modifier string
}{kindTest.schemaType, modifier})
qt.Assert(t, err, qt.IsNil)
schemaSrc := buf.String()

// *struct { Field {{.fieldPtrType}} }
ptrType := reflect.Zero(reflect.PtrTo(reflect.StructOf([]reflect.StructField{
{Name: "Field", Type: reflect.TypeOf(kindTest.fieldPtrType)},
}))).Interface()

ts, err := ipld.LoadSchemaBytes([]byte(schemaSrc))
qt.Assert(t, err, qt.IsNil)
schemaType := ts.TypeByName("Root")
qt.Assert(t, schemaType, qt.Not(qt.IsNil))

proto := bindnode.Prototype(ptrType, schemaType)
wantEncodedBytes, err := json.Marshal(map[string]interface{}{"field": json.RawMessage(kindTest.fieldDagJSON)})
qt.Assert(t, err, qt.IsNil)
wantEncoded := string(wantEncodedBytes)

node := dagjsonDecode(t, proto.Representation(), wantEncoded).(schema.TypedNode)

encoded := dagjsonEncode(t, node.Representation())
qt.Assert(t, encoded, qt.Equals, wantEncoded)

// Assigning with the missing field should only work with optional.
nb := proto.NewBuilder()
err = dagjson.Decode(nb, strings.NewReader(`{}`))
if modifier == "optional" {
qt.Assert(t, err, qt.IsNil)
node := nb.Build()
// The resulting node should be non-nil with a nil field.
nodeVal := reflect.ValueOf(bindnode.Unwrap(node))
qt.Assert(t, nodeVal.Elem().FieldByName("Field").IsNil(), qt.IsTrue)
} else {
qt.Assert(t, err, qt.Not(qt.IsNil))
}

// Assigning with a null field should only work with nullable.
nb = proto.NewBuilder()
err = dagjson.Decode(nb, strings.NewReader(`{"field":null}`))
if modifier == "nullable" {
qt.Assert(t, err, qt.IsNil)
node := nb.Build()
// The resulting node should be non-nil with a nil field.
nodeVal := reflect.ValueOf(bindnode.Unwrap(node))
qt.Assert(t, nodeVal.Elem().FieldByName("Field").IsNil(), qt.IsTrue)
} else {
qt.Assert(t, err, qt.Not(qt.IsNil))
}
})
}
}
}

type verifyBadType struct {
ptrType interface{}
panicRegexp string
Expand Down
Loading

0 comments on commit ac5b5fd

Please sign in to comment.