From 9e5da0e0a217fc9de5b6824ceab12dc29bc5ceec Mon Sep 17 00:00:00 2001 From: Eric Myhre Date: Wed, 19 Jan 2022 03:48:34 +0100 Subject: [PATCH] selectors: fix for edge case around recursion clauses with an immediate edge. See also the diff in the ipld/ipld repo for the new fixture, which includes some explanation. This would be a somewhat silly selector to write, but, it doesn't seem to be something we should reject at compile time, either; so, we must handle it gracefully. Along the way, I documented some code that was sparse in comments. I also added a shortcut to traversals so that a selector that explicitly states it's not interested in anything (by returning a non-nil but empty slice for its interests) is treated differently than a nil return for interests (which means "I don't know; hit me with everything you've got and lemme see"). This means that our edge case here with edges (...heh) doesn't cause the traversal to create an iterator that it doesn't really need, etc. We turn out to need both that shortcut, *and* less panicky methods on ExploreRecursiveEdge, *and* the edge case branch in ExploreRecursive... because traversals are pre-order. Decide is called first, then Interests, and then Explore. Therefore informing the Explore method alone about this situation is not sufficient. Previously, a recursion clause with an edge as its immediate and only child would get a passing grade by the compiler (same as now), but would panic when actually used (oh dear). --- .ipld | 2 +- traversal/selector/exploreRecursive.go | 13 ++++++++++++- traversal/selector/exploreRecursiveEdge.go | 8 ++++---- traversal/selector/selector.go | 3 ++- traversal/selector/spec_test.go | 9 +++++++-- traversal/walk.go | 3 +++ 6 files changed, 29 insertions(+), 9 deletions(-) diff --git a/.ipld b/.ipld index 9b274602..3b42ad2f 160000 --- a/.ipld +++ b/.ipld @@ -1 +1 @@ -Subproject commit 9b274602cacd590fe0017944f69efac29690dd78 +Subproject commit 3b42ad2f22e6cb114cc884307fa316cdc3e121e8 diff --git a/traversal/selector/exploreRecursive.go b/traversal/selector/exploreRecursive.go index 287eda8e..17614025 100644 --- a/traversal/selector/exploreRecursive.go +++ b/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 } diff --git a/traversal/selector/exploreRecursiveEdge.go b/traversal/selector/exploreRecursiveEdge.go index 75c74433..65438572 100644 --- a/traversal/selector/exploreRecursiveEdge.go +++ b/traversal/selector/exploreRecursiveEdge.go @@ -16,9 +16,9 @@ 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 @@ -26,9 +26,9 @@ func (s ExploreRecursiveEdge) Explore(n datamodel.Node, p datamodel.PathSegment) 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 diff --git a/traversal/selector/selector.go b/traversal/selector/selector.go index 9905a214..4d0a7814 100644 --- a/traversal/selector/selector.go +++ b/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 diff --git a/traversal/selector/spec_test.go b/traversal/selector/spec_test.go index 7a043930..215bfd6b 100644 --- a/traversal/selector/spec_test.go +++ b/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()) }) } - } diff --git a/traversal/walk.go b/traversal/walk.go index 26b2af40..80250170 100644 --- a/traversal/walk.go +++ b/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) }