mirror of
https://github.com/open-policy-agent/opa.git
synced 2026-08-12 19:32:48 -06:00
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>
This commit is contained in:
@@ -167,6 +167,37 @@ cases:
|
||||
- - a
|
||||
- c
|
||||
|
||||
- data: {}
|
||||
input_term: '{
|
||||
"graph": {
|
||||
"1": [],
|
||||
"2": ["1"],
|
||||
"3": ["2"],
|
||||
"4": ["3", "2"],
|
||||
"5": ["4"]
|
||||
},
|
||||
"initial": ["5"]
|
||||
}'
|
||||
modules:
|
||||
- |
|
||||
package reachable
|
||||
|
||||
p = result {
|
||||
graph.reachable_paths(input.graph, input.initial, result)
|
||||
}
|
||||
note: reachable_paths/shared_ancestor
|
||||
query: data.reachable.p = x
|
||||
want_result:
|
||||
- x:
|
||||
- - "5"
|
||||
- "4"
|
||||
- "2"
|
||||
- "1"
|
||||
- - "5"
|
||||
- "4"
|
||||
- "3"
|
||||
- "2"
|
||||
- "1"
|
||||
- data: {}
|
||||
input_term: '{
|
||||
"graph": {
|
||||
|
||||
@@ -117,11 +117,14 @@ cases:
|
||||
- - one
|
||||
- five
|
||||
- six
|
||||
- nine
|
||||
|
||||
- - one
|
||||
- five
|
||||
- six
|
||||
- nine
|
||||
- seven
|
||||
- eight
|
||||
- three
|
||||
|
||||
- - one
|
||||
- two
|
||||
|
||||
@@ -119,6 +119,28 @@ cases:
|
||||
- c
|
||||
- - a
|
||||
- c
|
||||
- note: reachable_paths/shared_ancestor
|
||||
query: data.reachable.p = x
|
||||
modules:
|
||||
- |
|
||||
package reachable
|
||||
|
||||
p := result if {
|
||||
graph.reachable_paths(input.graph, input.initial, result)
|
||||
}
|
||||
data: {}
|
||||
input_term: '{ "graph": { "1": [], "2": ["1"], "3": ["2"], "4": ["3", "2"], "5": ["4"] }, "initial": ["5"] }'
|
||||
want_result:
|
||||
- x:
|
||||
- - "5"
|
||||
- "4"
|
||||
- "2"
|
||||
- "1"
|
||||
- - "5"
|
||||
- "4"
|
||||
- "3"
|
||||
- "2"
|
||||
- "1"
|
||||
- note: reachable_paths/invalid_end
|
||||
query: data.reachable.p = x
|
||||
modules:
|
||||
|
||||
@@ -71,10 +71,13 @@ cases:
|
||||
- - one
|
||||
- five
|
||||
- six
|
||||
- nine
|
||||
- - one
|
||||
- five
|
||||
- six
|
||||
- nine
|
||||
- seven
|
||||
- eight
|
||||
- three
|
||||
- - one
|
||||
- two
|
||||
- four
|
||||
|
||||
+23
-31
@@ -74,39 +74,31 @@ func builtinReachable(_ BuiltinContext, operands []*ast.Term, iter func(*ast.Ter
|
||||
|
||||
// pathBuilder is called recursively to build a Set of paths that are reachable from the root
|
||||
func pathBuilder(graph ast.Object, root *ast.Term, path []*ast.Term, edgeRslt ast.Set, reached ast.Set) {
|
||||
paths := []*ast.Term{}
|
||||
|
||||
if edges := graph.Get(root); edges != nil {
|
||||
path = append(path, root)
|
||||
|
||||
if numberOfEdges(edges) >= 1 {
|
||||
|
||||
foreachVertex(edges, func(neighbor *ast.Term) {
|
||||
|
||||
if reached.Contains(neighbor) {
|
||||
// If we've already reached this node, return current path (avoid infinite recursion)
|
||||
paths = append(paths, path...)
|
||||
edgeRslt.Add(ast.ArrayTerm(paths...))
|
||||
} else {
|
||||
reached.Add(root)
|
||||
pathBuilder(graph, neighbor, path, edgeRslt, reached)
|
||||
|
||||
}
|
||||
|
||||
})
|
||||
|
||||
} else {
|
||||
paths = append(paths, path...)
|
||||
edgeRslt.Add(ast.ArrayTerm(paths...))
|
||||
|
||||
}
|
||||
} else {
|
||||
// Node is nonexistent (not in graph). Commit the current path (without adding this root)
|
||||
paths = append(paths, path...)
|
||||
edgeRslt.Add(ast.ArrayTerm(paths...))
|
||||
|
||||
edges := graph.Get(root)
|
||||
if edges == nil {
|
||||
// Node not in graph — commit path without this node.
|
||||
edgeRslt.Add(ast.ArrayTerm(path...))
|
||||
return
|
||||
}
|
||||
|
||||
path = append(path, root)
|
||||
|
||||
if numberOfEdges(edges) == 0 {
|
||||
edgeRslt.Add(ast.ArrayTerm(path...))
|
||||
return
|
||||
}
|
||||
|
||||
reached = reached.Copy()
|
||||
reached.Add(root)
|
||||
|
||||
foreachVertex(edges, func(neighbor *ast.Term) {
|
||||
if reached.Contains(neighbor) {
|
||||
// Cycle detected — commit current path.
|
||||
edgeRslt.Add(ast.ArrayTerm(path...))
|
||||
} else {
|
||||
pathBuilder(graph, neighbor, append([]*ast.Term(nil), path...), edgeRslt, reached)
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
func builtinReachablePaths(_ BuiltinContext, operands []*ast.Term, iter func(*ast.Term) error) error {
|
||||
|
||||
Reference in New Issue
Block a user