Skip to content

Commit 104dec8

Browse files
vroldanbetclaude
vroldanbet
and
claude
authored
fix(schemav2): improve PostOrder walker cycle detection (#2902)
Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
1 parent b402674 commit 104dec8

File tree

2 files changed

+140
-36
lines changed

2 files changed

+140
-36
lines changed

‎pkg/schema/v2/walk.go‎

Lines changed: 37 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,28 @@ type WalkOptions struct {
3535
visitedTargets map[string]bool
3636
}
3737

38+
// namedNode is an interface for schema elements that have a Name() method.
39+
type namedNode interface {
40+
Name() string
41+
}
42+
43+
// shouldSkipForCycleDetection checks if a relation/permission has been visited in PostOrder mode.
44+
// If not visited, marks it as visited. Returns true if already visited (should skip), false otherwise.
45+
// This is used for cycle detection in PostOrder traversal to ensure each node is visited at most once.
46+
func (opts *WalkOptions) shouldSkipForCycleDetection(parent namedNode, node namedNode) bool {
47+
if parent == nil || node == nil {
48+
return false
49+
}
50+
51+
targetKey := parent.Name() + "#" + node.Name()
52+
if opts.visitedTargets[targetKey] {
53+
return true // Already visited, should skip
54+
}
55+
56+
opts.visitedTargets[targetKey] = true
57+
return false // Not visited yet, should process
58+
}
59+
3860
// WalkOptionsBuilder provides a fluent interface for building WalkOptions with error handling.
3961
type WalkOptionsBuilder struct {
4062
options WalkOptions
@@ -435,6 +457,13 @@ func walkRelationWithOptions[T any](r *Relation, v Visitor[T], value T, options
435457
currentValue := value
436458

437459
if options.strategy == WalkPostOrder {
460+
// Mark this relation as visited BEFORE walking children to prevent re-traversal
461+
// via ResolvedRelationReference or arrow references within the children
462+
// (e.g. permission view = view or permission view = parent->view, with parent having the same type as this definition)
463+
if options.shouldSkipForCycleDetection(r.parent, r) {
464+
return currentValue, nil
465+
}
466+
438467
// PostOrder: Visit children first, then parent
439468
for _, br := range r.baseRelations {
440469
newValue, err := walkBaseRelationWithOptions(br, v, currentValue, options)
@@ -507,6 +536,12 @@ func walkPermissionWithOptions[T any](p *Permission, v Visitor[T], value T, opti
507536
currentValue := value
508537

509538
if options.strategy == WalkPostOrder {
539+
// Mark this permission as visited BEFORE walking children to prevent re-traversal
540+
// via ResolvedRelationReference or arrow references within the operation tree
541+
if options.shouldSkipForCycleDetection(p.parent, p) {
542+
return currentValue, nil
543+
}
544+
510545
// PostOrder: Visit children (operation tree) first, then parent
511546
newValue, err := walkOperationWithOptions(p.operation, v, currentValue, options)
512547
if err != nil {
@@ -642,8 +677,6 @@ func walkOperationPostOrder[T any](op Operation, v Visitor[T], value T, options
642677

643678
// Only walk into the resolved target if we haven't visited it yet
644679
if targetKey != "" && !options.visitedTargets[targetKey] {
645-
options.visitedTargets[targetKey] = true
646-
647680
switch resolved := o.Resolved().(type) {
648681
case *Relation:
649682
newValue, err := walkRelationWithOptions(resolved, v, currentValue, options)
@@ -681,17 +714,10 @@ func walkOperationPostOrder[T any](op Operation, v Visitor[T], value T, options
681714
// Get the resolved left relation to find allowed subject types
682715
resolvedLeft := o.ResolvedLeft()
683716
if resolvedLeft != nil {
684-
// For each base relation (allowed subject type), walk the target permission
717+
// For each base relation (allowed subject type), walk the target permission/relation.
685718
for _, br := range resolvedLeft.BaseRelations() {
686719
targetDefName := br.Type()
687720
targetPermName := o.Right()
688-
targetKey := targetDefName + "#" + targetPermName
689-
690-
// Skip if already visited to prevent infinite recursion
691-
if options.visitedTargets[targetKey] {
692-
continue
693-
}
694-
options.visitedTargets[targetKey] = true
695721

696722
// Find the target definition in the schema
697723
if targetDef, ok := options.schema.GetTypeDefinition(targetDefName); ok {
@@ -744,17 +770,10 @@ func walkOperationPostOrder[T any](op Operation, v Visitor[T], value T, options
744770
// Get the resolved left relation to find allowed subject types
745771
resolvedLeft := o.ResolvedLeft()
746772
if resolvedLeft != nil {
747-
// For each base relation (allowed subject type), walk the target permission
773+
// For each base relation (allowed subject type), walk the target permission/relation.
748774
for _, br := range resolvedLeft.BaseRelations() {
749775
targetDefName := br.Type()
750776
targetPermName := o.Right()
751-
targetKey := targetDefName + "#" + targetPermName
752-
753-
// Skip if already visited to prevent infinite recursion
754-
if options.visitedTargets[targetKey] {
755-
continue
756-
}
757-
options.visitedTargets[targetKey] = true
758777

759778
// Find the target definition in the schema
760779
if targetDef, ok := options.schema.GetTypeDefinition(targetDefName); ok {

‎pkg/schema/v2/walk_recursion_test.go‎

Lines changed: 103 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -93,8 +93,15 @@ definition group {
9393
require.NotEmpty(t, tracker.visitedRelations, "Should have visited at least one relation")
9494
require.Contains(t, tracker.visitedRelations, "group#member", "Should have visited the member relation")
9595

96+
// Verify each relation/permission is visited exactly once (proper cycle detection)
97+
memberCount := countOccurrences(tracker.visitedRelations, "group#member")
98+
isMemberCount := countOccurrences(tracker.visitedPermissions, "group#is_member")
99+
100+
require.Equal(t, 1, memberCount, "group#member should be visited exactly once")
101+
require.Equal(t, 1, isMemberCount, "group#is_member should be visited exactly once")
102+
96103
// Check that we didn't get stuck in infinite recursion
97-
require.Equal(t, 2, tracker.maxDepth, "Depth should not indicate infinite recursion")
104+
require.Equal(t, 1, tracker.maxDepth, "Depth should not indicate infinite recursion")
98105
}
99106

100107
// TestWalkPostOrder_MutuallyRecursivePermissions tests that the walker can handle
@@ -142,18 +149,17 @@ definition document {
142149
require.True(t, containsPermission(tracker.visitedPermissions, "view"), "Should have visited view permission")
143150
require.True(t, containsPermission(tracker.visitedPermissions, "edit"), "Should have visited edit permission")
144151

145-
// Check depth - this will reveal if there's cycle detection or if it walks the cycle multiple times
146-
require.Equal(t, 4, tracker.maxDepth, "Depth should not indicate infinite recursion")
152+
// Verify each permission is visited exactly once (proper cycle detection)
153+
ownerCount := countOccurrences(tracker.visitedRelations, "document#owner")
154+
viewCount := countOccurrences(tracker.visitedPermissions, "document#view")
155+
editCount := countOccurrences(tracker.visitedPermissions, "document#edit")
147156

148-
// Count how many times each permission was visited
149-
viewCount := countOccurrences(tracker.visitedPermissions, "view")
150-
editCount := countOccurrences(tracker.visitedPermissions, "edit")
157+
require.Equal(t, 1, ownerCount, "document#owner should be visited exactly once")
158+
require.Equal(t, 1, viewCount, "document#view should be visited exactly once")
159+
require.Equal(t, 1, editCount, "document#edit should be visited exactly once")
151160

152-
// If there's proper cycle detection, each should be visited once
153-
// If not, they might be visited multiple times (but should still terminate)
154-
// The key is that it should NOT infinite loop
155-
require.Positive(t, viewCount, "view should be visited at least once")
156-
require.Positive(t, editCount, "edit should be visited at least once")
161+
// Check depth - this will reveal if there's cycle detection or if it walks the cycle multiple times
162+
require.Equal(t, 2, tracker.maxDepth, "Depth should not indicate infinite recursion")
157163
}
158164

159165
// TestWalkPreOrder_RecursiveGroupRelation tests pre-order with recursive group relation
@@ -321,12 +327,20 @@ definition document {
321327
// Verify no infinite recursion
322328
require.Less(t, 2, tracker.maxDepth, "Depth should not indicate infinite recursion")
323329

324-
// The arrow cycle detection should prevent folder#view from being visited multiple times
325-
// when following the parent->view arrow
326-
viewCount := countOccurrences(tracker.visitedPermissions, "folder#view")
327-
328-
// With proper cycle detection, it should be visited a reasonable number of times
329-
require.Equal(t, 2, viewCount, "folder#view should not be visited excessive times due to cycle detection")
330+
// Verify each relation/permission is visited exactly once (proper cycle detection)
331+
folderParentCount := countOccurrences(tracker.visitedRelations, "folder#parent")
332+
folderViewerCount := countOccurrences(tracker.visitedRelations, "folder#viewer")
333+
folderViewCount := countOccurrences(tracker.visitedPermissions, "folder#view")
334+
documentParentCount := countOccurrences(tracker.visitedRelations, "document#parent")
335+
documentOwnerCount := countOccurrences(tracker.visitedRelations, "document#owner")
336+
documentViewCount := countOccurrences(tracker.visitedPermissions, "document#view")
337+
338+
require.Equal(t, 1, folderParentCount, "folder#parent should be visited exactly once")
339+
require.Equal(t, 1, folderViewerCount, "folder#viewer should be visited exactly once")
340+
require.Equal(t, 1, folderViewCount, "folder#view should be visited exactly once")
341+
require.Equal(t, 1, documentParentCount, "document#parent should be visited exactly once")
342+
require.Equal(t, 1, documentOwnerCount, "document#owner should be visited exactly once")
343+
require.Equal(t, 1, documentViewCount, "document#view should be visited exactly once")
330344
}
331345

332346
// TestWalkPostOrder_DeeplyNestedRecursion tests a more complex scenario with multiple levels
@@ -380,8 +394,79 @@ definition resource {
380394
require.True(t, containsPermission(tracker.visitedPermissions, "edit"), "Should have visited edit")
381395
require.True(t, containsPermission(tracker.visitedPermissions, "delete"), "Should have visited delete")
382396

397+
// Verify each permission is visited at most once (proper cycle detection)
398+
viewCount := countOccurrences(tracker.visitedPermissions, "resource#view")
399+
editCount := countOccurrences(tracker.visitedPermissions, "resource#edit")
400+
deleteCount := countOccurrences(tracker.visitedPermissions, "resource#delete")
401+
402+
require.Equal(t, 1, viewCount, "resource#view should be visited exactly once")
403+
require.Equal(t, 1, editCount, "resource#edit should be visited exactly once")
404+
require.Equal(t, 1, deleteCount, "resource#delete should be visited exactly once")
405+
383406
// Check for reasonable depth
384-
require.Equal(t, 9, tracker.maxDepth, "Depth should not indicate infinite recursion")
407+
require.Equal(t, 5, tracker.maxDepth, "Depth should not indicate infinite recursion")
408+
}
409+
410+
// TestWalkPostOrder_RecursiveResourceWithTeamMembers tests a schema with both
411+
// recursive relation non-terminal types and recursive permission on the same resource
412+
func TestWalkPostOrder_RecursiveRelationAndPermission(t *testing.T) {
413+
schemaString := `definition user {}
414+
415+
definition team {
416+
relation members: user | team#members
417+
}
418+
419+
definition resource {
420+
relation parent: resource
421+
relation reader: user | team#members
422+
423+
permission read = reader + parent->read
424+
}`
425+
426+
// Compile the schema
427+
compiled, err := compiler.Compile(compiler.InputSchema{
428+
Source: input.Source("test"),
429+
SchemaString: schemaString,
430+
}, compiler.AllowUnprefixedObjectType())
431+
require.NoError(t, err)
432+
433+
// Convert to *Schema
434+
schema, err := BuildSchemaFromCompiledSchema(*compiled)
435+
require.NoError(t, err)
436+
require.NotNil(t, schema)
437+
438+
// Resolve the schema (required for arrow traversal)
439+
resolved, err := ResolveSchema(schema)
440+
require.NoError(t, err)
441+
require.NotNil(t, resolved)
442+
443+
// Track what gets visited
444+
tracker := &recursionTracker{}
445+
446+
// Walk with post-order traversal with arrow traversal
447+
opts := NewWalkOptions().
448+
WithStrategy(WalkPostOrder).
449+
WithTraverseArrowTargets(resolved.Schema()).MustBuild()
450+
451+
_, err = WalkSchemaWithOptions(resolved.Schema(), tracker, 0, opts)
452+
require.NoError(t, err)
453+
454+
// The walker should have completed without infinite recursion
455+
require.NotEmpty(t, tracker.visitedPermissions, "Should have visited at least one permission")
456+
require.NotEmpty(t, tracker.visitedRelations, "Should have visited at least one relation")
457+
458+
teamMembersCount := countOccurrences(tracker.visitedRelations, "team#members")
459+
resourceReaderCount := countOccurrences(tracker.visitedRelations, "resource#reader")
460+
resourceParentCount := countOccurrences(tracker.visitedRelations, "resource#parent")
461+
resourceReadCount := countOccurrences(tracker.visitedPermissions, "resource#read")
462+
463+
require.Equal(t, 1, teamMembersCount, "team#members should be visited exactly once")
464+
require.Equal(t, 1, resourceReaderCount, "resource#reader should be visited exactly once")
465+
require.Equal(t, 1, resourceParentCount, "resource#parent should be visited exactly once")
466+
require.Equal(t, 1, resourceReadCount, "resource#read should be visited exactly once")
467+
468+
// Check for reasonable depth (should not indicate infinite recursion)
469+
require.Equal(t, 3, tracker.maxDepth, "Depth should not indicate infinite recursion")
385470
}
386471

387472
// Helper functions

0 commit comments

Comments
 (0)