Merge branch 'iterator-refactor'

encoding/dagcbor/marshal.go

@@ -31,18 +31,17 @@ func Marshal(n ipld.Node, sink shared.TokenSink) error {
 			return err
 		}
 		// Emit map contents (and recurse).
-		for itr := n.Keys(); itr.HasNext(); {
-			k, err := itr.Next()
+		for itr := n.MapIterator(); !itr.Done(); {
+			k, v, err := itr.Next()
 			if err != nil {
 				return err
 			}
 			tk.Type = tok.TString
-			tk.Str = k
-			if _, err := sink.Step(&tk); err != nil {
+			tk.Str, err = k.AsString()
+			if err != nil {
 				return err
 			}
-			v, err := n.TraverseField(k)
-			if err != nil {
+			if _, err := sink.Step(&tk); err != nil {
 				return err
 			}
 			if err := Marshal(v, sink); err != nil {

encoding/dagjson/marshal.go

@@ -31,18 +31,17 @@ func Marshal(n ipld.Node, sink shared.TokenSink) error {
 			return err
 		}
 		// Emit map contents (and recurse).
-		for itr := n.Keys(); itr.HasNext(); {
-			k, err := itr.Next()
+		for itr := n.MapIterator(); !itr.Done(); {
+			k, v, err := itr.Next()
 			if err != nil {
 				return err
 			}
 			tk.Type = tok.TString
-			tk.Str = k
-			if _, err := sink.Step(&tk); err != nil {
+			tk.Str, err = k.AsString()
+			if err != nil {
 				return err
 			}
-			v, err := n.TraverseField(k)
-			if err != nil {
+			if _, err := sink.Step(&tk); err != nil {
 				return err
 			}
 			if err := Marshal(v, sink); err != nil {

encoding/marshal.go

@@ -36,18 +36,17 @@ func Marshal(n ipld.Node, sink shared.TokenSink) error {
 			return err
 		}
 		// Emit map contents (and recurse).
-		for itr := n.Keys(); itr.HasNext(); {
-			k, err := itr.Next()
+		for itr := n.MapIterator(); !itr.Done(); {
+			k, v, err := itr.Next()
 			if err != nil {
 				return err
 			}
 			tk.Type = tok.TString
-			tk.Str = k
-			if _, err := sink.Step(&tk); err != nil {
+			tk.Str, err = k.AsString()
+			if err != nil {
 				return err
 			}
-			v, err := n.TraverseField(k)
-			if err != nil {
+			if _, err := sink.Step(&tk); err != nil {
 				return err
 			}
 			if err := Marshal(v, sink); err != nil {

fluent/fluentNode.go

@@ -17,8 +17,8 @@ type Node interface {
 	ReprKind() ipld.ReprKind
 	TraverseField(path string) Node
 	TraverseIndex(idx int) Node
-	Keys() KeyIterator
-	KeysImmediate() []string
+	MapIterator() MapIterator
+	ListIterator() ListIterator
 	Length() int
 	IsNull() bool
 	AsBool() bool
@@ -76,21 +76,17 @@ func (n node) TraverseIndex(idx int) Node {
 	}
 	return node{v, nil}
 }
-func (n node) Keys() KeyIterator {
+func (n node) MapIterator() MapIterator {
 	if n.err != nil {
 		panic(Error{n.err})
 	}
-	return &keyIterator{n.n.Keys()}
+	return &mapIterator{n.n.MapIterator()}
 }
-func (n node) KeysImmediate() []string {
+func (n node) ListIterator() ListIterator {
 	if n.err != nil {
 		panic(Error{n.err})
 	}
-	v, err := n.n.KeysImmediate()
-	if err != nil {
-		panic(Error{err})
-	}
-	return v
+	return &listIterator{n.n.ListIterator()}
 }
 func (n node) Length() int {
 	if n.err != nil {
@@ -165,22 +161,42 @@ func (n node) AsLink() ipld.Link {
 	return v
 }
 
-type KeyIterator interface {
-	Next() string
-	HasNext() bool
+type MapIterator interface {
+	Next() (key Node, value Node)
+	Done() bool
 }
 
-type keyIterator struct {
-	d ipld.KeyIterator
+type mapIterator struct {
+	d ipld.MapIterator
 }
 
-func (ki *keyIterator) Next() string {
-	v, err := ki.d.Next()
+func (itr *mapIterator) Next() (Node, Node) {
+	k, v, err := itr.d.Next()
 	if err != nil {
 		panic(Error{err})
 	}
-	return v
+	return node{k, nil}, node{v, nil}
+}
+func (itr *mapIterator) Done() bool {
+	return itr.d.Done()
+}
+
+type ListIterator interface {
+	Next() (idx int, value Node)
+	Done() bool
+}
+
+type listIterator struct {
+	d ipld.ListIterator
+}
+
+func (itr *listIterator) Next() (int, Node) {
+	idx, v, err := itr.d.Next()
+	if err != nil {
+		panic(Error{err})
+	}
+	return idx, node{v, nil}
 }
-func (ki *keyIterator) HasNext() bool {
-	return ki.d.HasNext()
+func (itr *listIterator) Done() bool {
+	return itr.d.Done()
 }

fluent/fluentUtilities.go

+package fluent
+
+// AllKeyStrings is a shorthand to iterate a map node and collect all the keys
+// (and convert them to strings), returning them in a slice.
+func AllKeyStrings(n Node) []string {
+	itr := n.MapIterator()
+	res := make([]string, n.Length())
+	for i := 0; !itr.Done(); i++ {
+		k, _ := itr.Next()
+		res[i] = k.AsString()
+	}
+	return res
+}

impl/bind/boundNode.go

@@ -107,11 +107,11 @@ func (n Node) NodeBuilder() ipld.NodeBuilder {
 	panic("NYI")
 }
 
-func (n Node) Keys() ipld.KeyIterator {
+func (n Node) MapIterator() ipld.MapIterator {
 	panic("NYI")
 }
 
-func (n Node) KeysImmediate() ([]string, error) {
+func (n Node) ListIterator() ipld.ListIterator {
 	panic("NYI")
 }
 

impl/free/freeNode.go

@@ -64,27 +64,56 @@ func (n *Node) NodeBuilder() ipld.NodeBuilder {
 	return nodeBuilder{n}
 }
 
-func (n *Node) Keys() ipld.KeyIterator {
-	return &keyIterator{n, 0}
+func (n *Node) MapIterator() ipld.MapIterator {
+	return &mapIterator{n, 0, expectTyp(ipld.ReprKind_Map, n.kind)}
 }
 
-type keyIterator struct {
+type mapIterator struct {
 	node *Node
 	idx  int
+	err  error
 }
 
-func (ki *keyIterator) Next() (string, error) {
-	// TODO kind check and safer range handling.
-	v := ki.node._mapOrd[ki.idx]
-	ki.idx++
-	return v, nil
+func (itr *mapIterator) Next() (ipld.Node, ipld.Node, error) {
+	if itr.err != nil {
+		return nil, nil, itr.err
+	}
+	k := itr.node._mapOrd[itr.idx]
+	v := itr.node._map[k]
+	itr.idx++
+	return &Node{kind: ipld.ReprKind_String, _str: k}, v, nil
+}
+func (itr *mapIterator) Done() bool {
+	if itr.err != nil {
+		return false
+	}
+	return itr.idx >= len(itr.node._mapOrd)
+}
+
+func (n *Node) ListIterator() ipld.ListIterator {
+	return &listIterator{n, 0, expectTyp(ipld.ReprKind_List, n.kind)}
 }
-func (ki *keyIterator) HasNext() bool {
-	return len(ki.node._mapOrd) > ki.idx
+
+type listIterator struct {
+	node *Node
+	idx  int
+	err  error
 }
 
-func (n *Node) KeysImmediate() ([]string, error) {
-	return n._mapOrd, expectTyp(ipld.ReprKind_Map, n.kind)
+func (itr *listIterator) Next() (int, ipld.Node, error) {
+	if itr.err != nil {
+		return -1, nil, itr.err
+	}
+	v := itr.node._arr[itr.idx]
+	idx := itr.idx
+	itr.idx++
+	return idx, v, nil
+}
+func (itr *listIterator) Done() bool {
+	if itr.err != nil {
+		return false
+	}
+	return itr.idx >= len(itr.node._arr)
 }
 
 func (n *Node) Length() int {

node.go

@@ -27,20 +27,25 @@ type Node interface {
 	// If idx is out of range, a nil node and an error will be returned.
 	TraverseIndex(idx int) (Node, error)
 
-	// Keys returns an iterator which will yield keys for traversing the node.
+	// MapIterator returns an iterator which yields key-value pairs
+	// traversing the node.
 	// If the node kind is anything other than a map, the iterator will
 	// yield error values.
-	Keys() KeyIterator
+	//
+	// The iterator will yield every entry in the map; that is, it
+	// can be expected that itr.Next will be called node.Length times
+	// before itr.Done becomes true.
+	MapIterator() MapIterator
 
-	// KeysImmediate returns a slice containing all keys for traversing the node.
-	// The semantics are otherwise identical to using the Keys() iterator.
+	// ListIterator returns an iterator which yields key-value pairs
+	// traversing the node.
+	// If the node kind is anything other than a list, the iterator will
+	// yield error values.
 	//
-	// KeysImmediate is for convenience of usage; callers should prefer to use
-	// the iterator approach where possible, as it continues to behave well
-	// even when using collections of extremely large size (and even when
-	// the collection is split between multiple serial nodes, as with
-	// Advanced Layouts, etc).
-	KeysImmediate() ([]string, error)
+	// The iterator will yield every entry in the list; that is, it
+	// can be expected that itr.Next will be called node.Length times
+	// before itr.Done becomes true.
+	ListIterator() ListIterator
 
 	// Length returns the length of a list, or the number of entries in a map,
 	// or -1 if the node is not of list nor map kind.
@@ -79,21 +84,64 @@ type Node interface {
 	NodeBuilder() NodeBuilder
 }
 
-// KeyIterator is an interface for traversing nodes of kind map.
-// Sequential calls to Next() will yield keys; HasNext() describes whether
-// iteration should continue.
+// MapIterator is an interface for traversing map nodes.
+// Sequential calls to Next() will yield key-value pairs;
+// Done() describes whether iteration should continue.
 //
-// Iteration order is defined to be stable.
+// Iteration order is defined to be stable: two separate MapIterator
+// created to iterate the same Node will yield the same key-value pairs
+// in the same order.
+// The order itself may be defined by the Node implementation: some
+// Nodes may retain insertion order, and some may return iterators which
+// always yield data in sorted order, for example.
+type MapIterator interface {
+	// Next returns the next key-value pair.
+	//
+	// An error value can also be returned at any step: in the case of advanced
+	// data structures with incremental loading, it's possible to encounter
+	// cancellation or I/O errors at any point in iteration.
+	// If an error is returned, the boolean will always be false (so it's
+	// correct to check the bool first and short circuit to continuing if true).
+	// If an error is returned, the key and value may be nil.
+	Next() (key Node, value Node, err error)
+
+	// Done returns false as long as there's at least one more entry to iterate.
+	// When Done returns false, iteration can stop.
+	//
+	// Implementers of iterators for advanced data layouts (e.g. more than
+	// one chunk of backing data, which is loaded incrementally), if your
+	// implementation does any I/O during the Done method, and it encounters
+	// an error, it must return 'true', so that the following Next call
+	// has an opportunity to return the error.
+	Done() bool
+}
+
+// ListIterator is an interface for traversing list nodes.
+// Sequential calls to Next() will yield index-value pairs;
+// Done() describes whether iteration should continue.
 //
-// REVIEW: should Next return error?
-// Other parts of the Node interface use that for kind mismatch rejection;
-// so on those grounds, I'd say "no", because we know what the key kind is
-// (but then Node.Keys should return error).
-// In big nodes (composites using an AdvLayout), where do we return errors?
-// Since we might be streaming, there are questions here.
-type KeyIterator interface {
-	Next() (string, error)
-	HasNext() bool
+// A loop which iterates from 0 to Node.Length is a valid
+// alternative to using a ListIterator.
+type ListIterator interface {
+	// Next returns the next index and value.
+	//
+	// An error value can also be returned at any step: in the case of advanced
+	// data structures with incremental loading, it's possible to encounter
+	// cancellation or I/O errors at any point in iteration.
+	// If an error is returned, the boolean will always be false (so it's
+	// correct to check the bool first and short circuit to continuing if true).
+	// If an error is returned, the key and value may be nil.
+	Next() (idx int, value Node, err error)
+
+	// Done returns false as long as there's at least one more entry to iterate.
+	// When Done returns false, iteration can stop.
+	//
+	// Implementers of iterators for advanced data layouts (e.g. more than
+	// one chunk of backing data, which is loaded incrementally), if your
+	// implementation does any I/O during the Done method, and it encounters
+	// an error, it must return 'true', so that the following Next call
+	// has an opportunity to return the error.
+	Done() bool
 }
 
 // REVIEW: immediate-mode AsBytes() method (as opposed to e.g. returning

tests/unmarshalling.go

@@ -101,7 +101,7 @@ func TestRecursiveUnmarshal(t *testing.T, nb ipld.NodeBuilder) {
 		Require(t, err, ShouldEqual, nil)
 		Require(t, n.ReprKind(), ShouldEqual, ipld.ReprKind_Map)
 		Require(t, n.Length(), ShouldEqual, 1)
-		Require(t, fluent.WrapNode(n).KeysImmediate(), ShouldEqual, []string{"asdf"})
+		Require(t, fluent.AllKeyStrings(fluent.WrapNode(n)), ShouldEqual, []string{"asdf"})
 		Wish(t, fluent.WrapNode(n).TraverseField("asdf").ReprKind(), ShouldEqual, ipld.ReprKind_String)
 		Wish(t, fluent.WrapNode(n).TraverseField("asdf").AsString(), ShouldEqual, "zomzom")
 		Wish(t, tb.read, ShouldEqual, 4)
@@ -122,7 +122,7 @@ func TestRecursiveUnmarshal(t *testing.T, nb ipld.NodeBuilder) {
 		Require(t, err, ShouldEqual, nil)
 		Require(t, n.ReprKind(), ShouldEqual, ipld.ReprKind_Map)
 		Require(t, n.Length(), ShouldEqual, 2)
-		Require(t, fluent.WrapNode(n).KeysImmediate(), ShouldEqual, []string{"asdf", "zyzzy"})
+		Require(t, fluent.AllKeyStrings(fluent.WrapNode(n)), ShouldEqual, []string{"asdf", "zyzzy"})
 		Wish(t, fluent.WrapNode(n).TraverseField("asdf").ReprKind(), ShouldEqual, ipld.ReprKind_Map)
 		Wish(t, fluent.WrapNode(n).TraverseField("asdf").TraverseField("awoo").AsString(), ShouldEqual, "gah")
 		Wish(t, fluent.WrapNode(n).TraverseField("zyzzy").AsInt(), ShouldEqual, 9)

traversal/selector/selector.go

@@ -3,7 +3,7 @@ package selector
 import ipld "github.com/ipld/go-ipld-prime"
 
 type Selector interface {
-	Explore(ipld.Node) (ipld.KeyIterator, Selector)
+	Explore(ipld.Node) (ipld.MapIterator, ipld.ListIterator, Selector)
 	Decide(ipld.Node) bool
 }
 

typed/system/validate.go

@@ -43,17 +43,20 @@ func validate(ts Universe, t Type, node ipld.Node, pth string) []error {
 		if node.ReprKind() != ipld.ReprKind_Map {
 			return []error{fmt.Errorf("Schema match failed: expected type %q (which is kind %v) at path %q, but found kind %v", t2.Name(), t.ReprKind(), pth, node.ReprKind())}
 		}
-		keys, _ := node.KeysImmediate()
 		errs := []error(nil)
-		for _, k := range keys {
+		for itr := node.MapIterator(); !itr.Done(); {
+			k, v, err := itr.Next()
+			if err != nil {
+				return []error{err}
+			}
 			// FUTURE: if KeyType is an enum rather than string, do membership check.
-			child, _ := node.TraverseField(k)
-			if child.IsNull() {
+			ks, _ := k.AsString()
+			if v.IsNull() {
 				if !t2.ValueIsNullable() {
-					errs = append(errs, fmt.Errorf("Schema match failed: map at path %q contains unpermitted null in key %q", pth, k))
+					errs = append(errs, fmt.Errorf("Schema match failed: map at path %q contains unpermitted null in key %q", pth, ks))
 				}
 			} else {
-				errs = append(errs, validate(ts, t2.ValueType(), child, path.Join(pth, k))...)
+				errs = append(errs, validate(ts, t2.ValueType(), v, path.Join(pth, ks))...)
 			}
 		}
 		return errs