Merge pull request #334 from ipld/selectors-fix-recursion-with-immedi…

…ate-edge

selectors: fix for edge case around recursion clauses with an immediate edge.

traversal/selector/exploreRecursive.go

@@ -87,6 +87,7 @@ func (s ExploreRecursive) Interests() []datamodel.PathSegment {
 
 // Explore returns the node's selector for all fields
 func (s ExploreRecursive) Explore(n datamodel.Node, p datamodel.PathSegment) (Selector, error) {
+	// Check any stopAt conditions right away.
 	if s.stopAt != nil {
 		target, err := n.LookupBySegment(p)
 		if err != nil {
@@ -97,12 +98,22 @@ func (s ExploreRecursive) Explore(n datamodel.Node, p datamodel.PathSegment) (Se
 		}
 	}
 
+	// Fence against edge case: if the next selector is a recursion edge, nope, we're out.
+	//  (This is only reachable if a recursion contains nothing but an edge -- which is probably somewhat rare,
+	//   because it's certainly rather useless -- but it's not explicitly rejected as a malformed selector during compile, either, so it must be handled.)
+	if _, ok := s.current.(ExploreRecursiveEdge); ok {
+		return nil, nil
+	}
+
+	// Apply the current selector clause.  (This could be midway through something resembling the initially specified sequence.)
 	nextSelector, _ := s.current.Explore(n, p)
-	limit := s.limit
 
+	// We have to wrap the nextSelector yielded by the current clause in recursion information before returning it,
+	//  so that future levels of recursion (as well as their limits) can continue to operate correctly.
 	if nextSelector == nil {
 		return nil, nil
 	}
+	limit := s.limit
 	if !s.hasRecursiveEdge(nextSelector) {
 		return ExploreRecursive{s.sequence, nextSelector, limit, s.stopAt}, nil
 	}

traversal/selector/exploreRecursiveEdge.go

@@ -16,19 +16,19 @@ import (
 // An ExploreRecursiveEdge without an enclosing ExploreRecursive is an error.
 type ExploreRecursiveEdge struct{}
 
-// Interests should ultimately never get called for an ExploreRecursiveEdge selector
+// Interests should almost never get called for an ExploreRecursiveEdge selector
 func (s ExploreRecursiveEdge) Interests() []datamodel.PathSegment {
-	panic("Traversed Explore Recursive Edge Node With No Parent")
+	return []datamodel.PathSegment{}
 }
 
 // Explore should ultimately never get called for an ExploreRecursiveEdge selector
 func (s ExploreRecursiveEdge) Explore(n datamodel.Node, p datamodel.PathSegment) (Selector, error) {
 	panic("Traversed Explore Recursive Edge Node With No Parent")
 }
 
-// Decide should ultimately never get called for an ExploreRecursiveEdge selector
+// Decide should almost never get called for an ExploreRecursiveEdge selector
 func (s ExploreRecursiveEdge) Decide(n datamodel.Node) bool {
-	panic("Traversed Explore Recursive Edge Node With No Parent")
+	return false
 }
 
 // ParseExploreRecursiveEdge assembles a Selector

traversal/selector/selector.go

@@ -81,8 +81,9 @@ type Selector interface {
 	// (If that happens, we're currently assuming the ADL has a reasonable caching behavior.  It's very likely that the traversal will look up the same paths that Explore just looked up (assuming the Condition told exploration to continue).)
 	//
 
-	// Interests should return either a list of PageSegment we're likely interested in,
+	// Interests should return either a list of PathSegment we're likely interested in,
 	// **or nil**, which indicates we're a high-cardinality or expression-based selection clause and thus we'll need all segments proposed to us.
+	// Note that a non-nil zero length list of PathSegment is distinguished from nil: this would mean this selector is interested absolutely nothing.
 	//
 	// Traversal will call this before calling Explore, and use it to try to call Explore less often (or even avoid iterating on the data node at all).
 	Interests() []datamodel.PathSegment

traversal/selector/spec_test.go

@@ -18,7 +18,13 @@ import (
 )
 
 func TestSpecFixtures(t *testing.T) {
-	doc, err := testmark.ReadFile("../../.ipld/specs/selectors/fixtures/selector-fixtures-1.md")
+	dir := "../../.ipld/specs/selectors/fixtures/"
+	testOneSpecFixtureFile(t, dir+"selector-fixtures-1.md")
+	testOneSpecFixtureFile(t, dir+"selector-fixtures-recursion.md")
+}
+
+func testOneSpecFixtureFile(t *testing.T, filename string) {
+	doc, err := testmark.ReadFile(filename)
 	if os.IsNotExist(err) {
 		t.Skipf("not running spec suite: %s (did you clone the submodule with the data?)", err)
 	}
@@ -108,5 +114,4 @@ func TestSpecFixtures(t *testing.T) {
 			qt.Assert(t, visitLogString.String(), qt.CmpEquals(), fixtureExpectNormBuf.String())
 		})
 	}
-
 }

traversal/walk.go

@@ -203,6 +203,9 @@ func (prog Progress) walkAdv(n datamodel.Node, s selector.Selector, fn AdvVisitF
 	if attn == nil {
 		return prog.walkAdv_iterateAll(n, s, fn)
 	}
+	if len(attn) == 0 {
+		return nil
+	}
 	return prog.walkAdv_iterateSelective(n, attn, s, fn)
 
 }