Skip to content

Commit 55a9eb6

Browse files
fix: reachable_paths does not return all reachable paths (#8510)
* topdown: add failing tests for graph.reachable_paths with shared ancestors Adds a regression test (shared_ancestor) reproducing the bug reported in #5871: given a graph where node 4 has edges to both 3 and 2, and 3 also has an edge to 2, graph.reachable_paths from 5 drops the path [5,4,2,1]. Also corrects the expected result for the existing cycle_1022_3 test, which was asserting the buggy output ([one,five,six] truncated) rather than the correct complete path [one,five,six,seven,eight,three]. Signed-off-by: David Marne <david.marne@workiva.com> * topdown: fix graph.reachable_paths dropping paths with shared ancestors graph.reachable_paths had two bugs that caused paths to be silently dropped when a node is reachable via multiple routes (diamond-shaped graphs). Bug 1: the `reached` set was mutated and shared across sibling recursive calls. After branch A finished traversal and added nodes to `reached`, branch B would see those nodes as already visited and terminate early, emitting a truncated path instead of continuing. Fix: copy `reached` once per pathBuilder invocation and pass the copy to all recursive calls. Each branch now has its own ancestor-only visited set. Bug 2: ast.NewArray stores the slice it receives directly (elems: a) without copying. When sibling calls appended to a shared backing array, the in-place write by one sibling corrupted the already-committed path term of a previous sibling. Fix: pass append([]*ast.Term(nil), path...) to each recursive call, giving each branch its own independent backing array. Signed-off-by: David Marne <david.marne@workiva.com> --------- Signed-off-by: David Marne <david.marne@workiva.com>
1 parent 6a79368 commit 55a9eb6

5 files changed

Lines changed: 80 additions & 29 deletions

File tree

v1/test/cases/testdata/v0/reachable/test-reachable-paths-0422.yaml

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,37 @@ cases:
167167
- - a
168168
- c
169169

170+
- data: {}
171+
input_term: '{
172+
"graph": {
173+
"1": [],
174+
"2": ["1"],
175+
"3": ["2"],
176+
"4": ["3", "2"],
177+
"5": ["4"]
178+
},
179+
"initial": ["5"]
180+
}'
181+
modules:
182+
- |
183+
package reachable
184+
185+
p = result {
186+
graph.reachable_paths(input.graph, input.initial, result)
187+
}
188+
note: reachable_paths/shared_ancestor
189+
query: data.reachable.p = x
190+
want_result:
191+
- x:
192+
- - "5"
193+
- "4"
194+
- "2"
195+
- "1"
196+
- - "5"
197+
- "4"
198+
- "3"
199+
- "2"
200+
- "1"
170201
- data: {}
171202
input_term: '{
172203
"graph": {

v1/test/cases/testdata/v0/reachable/test-reachable-paths-1022.yaml

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -117,11 +117,14 @@ cases:
117117
- - one
118118
- five
119119
- six
120+
- nine
120121

121122
- - one
122123
- five
123124
- six
124-
- nine
125+
- seven
126+
- eight
127+
- three
125128

126129
- - one
127130
- two

v1/test/cases/testdata/v1/reachable/test-reachable-paths-0422.yaml

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,28 @@ cases:
119119
- c
120120
- - a
121121
- c
122+
- note: reachable_paths/shared_ancestor
123+
query: data.reachable.p = x
124+
modules:
125+
- |
126+
package reachable
127+
128+
p := result if {
129+
graph.reachable_paths(input.graph, input.initial, result)
130+
}
131+
data: {}
132+
input_term: '{ "graph": { "1": [], "2": ["1"], "3": ["2"], "4": ["3", "2"], "5": ["4"] }, "initial": ["5"] }'
133+
want_result:
134+
- x:
135+
- - "5"
136+
- "4"
137+
- "2"
138+
- "1"
139+
- - "5"
140+
- "4"
141+
- "3"
142+
- "2"
143+
- "1"
122144
- note: reachable_paths/invalid_end
123145
query: data.reachable.p = x
124146
modules:

v1/test/cases/testdata/v1/reachable/test-reachable-paths-1022.yaml

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,10 +71,13 @@ cases:
7171
- - one
7272
- five
7373
- six
74+
- nine
7475
- - one
7576
- five
7677
- six
77-
- nine
78+
- seven
79+
- eight
80+
- three
7881
- - one
7982
- two
8083
- four

v1/topdown/reachable.go

Lines changed: 19 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -74,39 +74,31 @@ func builtinReachable(_ BuiltinContext, operands []*ast.Term, iter func(*ast.Ter
7474

7575
// pathBuilder is called recursively to build a Set of paths that are reachable from the root
7676
func pathBuilder(graph ast.Object, root *ast.Term, path []*ast.Term, edgeRslt ast.Set, reached ast.Set) {
77-
paths := []*ast.Term{}
78-
79-
if edges := graph.Get(root); edges != nil {
80-
path = append(path, root)
81-
82-
if numberOfEdges(edges) >= 1 {
83-
84-
foreachVertex(edges, func(neighbor *ast.Term) {
77+
edges := graph.Get(root)
78+
if edges == nil {
79+
// Node not in graph — commit path without this node.
80+
edgeRslt.Add(ast.ArrayTerm(path...))
81+
return
82+
}
8583

86-
if reached.Contains(neighbor) {
87-
// If we've already reached this node, return current path (avoid infinite recursion)
88-
paths = append(paths, path...)
89-
edgeRslt.Add(ast.ArrayTerm(paths...))
90-
} else {
91-
reached.Add(root)
92-
pathBuilder(graph, neighbor, path, edgeRslt, reached)
84+
path = append(path, root)
9385

94-
}
86+
if numberOfEdges(edges) == 0 {
87+
edgeRslt.Add(ast.ArrayTerm(path...))
88+
return
89+
}
9590

96-
})
91+
reached = reached.Copy()
92+
reached.Add(root)
9793

94+
foreachVertex(edges, func(neighbor *ast.Term) {
95+
if reached.Contains(neighbor) {
96+
// Cycle detected — commit current path.
97+
edgeRslt.Add(ast.ArrayTerm(path...))
9898
} else {
99-
paths = append(paths, path...)
100-
edgeRslt.Add(ast.ArrayTerm(paths...))
101-
99+
pathBuilder(graph, neighbor, append([]*ast.Term(nil), path...), edgeRslt, reached)
102100
}
103-
} else {
104-
// Node is nonexistent (not in graph). Commit the current path (without adding this root)
105-
paths = append(paths, path...)
106-
edgeRslt.Add(ast.ArrayTerm(paths...))
107-
108-
}
109-
101+
})
110102
}
111103

112104
func builtinReachablePaths(_ BuiltinContext, operands []*ast.Term, iter func(*ast.Term) error) error {

0 commit comments

Comments
 (0)