Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughPer-userset weighting added: nodes and edges gain Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Graph as WeightedAuthorizationModelGraph
participant Cache as usersetWeights Cache
participant Calc as calculateUsersetWeights
Caller->>Graph: GetWeight(node, key)
alt key contains "#" (userset)
Graph->>Cache: lookup node.usersetWeights[userset]
alt cached
Cache-->>Graph: cached weight
else not cached
Graph->>Calc: calculateUsersetWeights(node, usersetNode, visited)
Calc->>Calc: traverse edges, prune edges (canPruneEdge)
Calc->>Calc: detect cycles -> cycle helpers
Calc->>Cache: store computed weight on nodes/edges
Calc-->>Graph: computed weight
end
else terminal/keyed weight
Graph-->>Caller: return terminal weight if present
end
Graph-->>Caller: return (weight, ok)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/go/graph/weighted_graph.go (1)
802-1082: Consider concurrency safety and memory growth.The new per-userset weight calculation methods use memoization maps (
usersetWeights) without synchronization. Verify that the graph is only accessed from a single goroutine, or add synchronization if concurrent access is needed.Additionally, the
usersetWeightsmaps will grow with each unique userset queried. If the number of distinct usersets is large, consider adding a cleanup mechanism or size limits.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/go/graph/weighted_graph.go(7 hunks)pkg/go/graph/weighted_graph_edge.go(1 hunks)pkg/go/graph/weighted_graph_node.go(1 hunks)pkg/go/graph/weighted_graph_userset_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/go/graph/weighted_graph.go (2)
pkg/go/graph/weighted_graph_node.go (8)
WeightedAuthorizationModelNode(18-27)SpecificTypeAndRelation(7-7)LogicalDirectGrouping(10-10)LogicalTTUGrouping(11-11)OperatorNode(8-8)UnionOperator(13-13)IntersectionOperator(14-14)ExclusionOperator(15-15)pkg/go/graph/weighted_graph_edge.go (3)
WeightedAuthorizationModelEdge(58-76)DirectEdge(11-11)TTUEdge(21-21)
pkg/go/graph/weighted_graph_userset_test.go (3)
pkg/go/transformer/dsltojson.go (1)
MustTransformDSLToProto(553-560)pkg/go/graph/weighted_graph_builder.go (1)
NewWeightedAuthorizationModelGraphBuilder(19-21)pkg/go/graph/weighted_graph.go (1)
Infinite(11-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (11)
pkg/go/graph/weighted_graph_edge.go (1)
74-75: LGTM!The
usersetWeightsfield addition follows the existing pattern of theweightsfield and is properly initialized in the graph methods.pkg/go/graph/weighted_graph_node.go (1)
26-26: LGTM!The
usersetWeightsfield addition mirrors the existingweightsfield pattern and is properly initialized during node creation.pkg/go/graph/weighted_graph_userset_test.go (1)
10-240: Comprehensive test coverage for userset weights.The test suite effectively covers:
- Direct paths with various operators (union, intersection, exclusion)
- Recursive self-references
- Tuple cycles between types
- Dependencies on recursive definitions
The assertions verify both the presence of weights (for reachable paths) and absence (for pruned paths).
pkg/go/graph/weighted_graph.go (8)
57-57: LGTM! Proper initialization of usersetWeights maps.The
usersetWeightsmaps are correctly initialized in all node and edge creation paths, ensuring they're ready for use by the weight calculation logic.Also applies to: 69-69, 79-79, 106-106
153-166: LGTM! Clean delegation in GetWeight method.The method correctly distinguishes between userset keys (containing "#") and terminal type keys, delegating to the appropriate calculation path.
186-223: Effective early pruning optimization.The method implements a multi-stage approach:
- Cache lookup for memoized results
- Node validation
- Early pruning based on terminal type weights (avoiding traversal when paths cannot exist)
- Full graph traversal when necessary
The early pruning logic at lines 206-214 correctly identifies unreachable paths by comparing terminal type weights.
802-895: Core traversal logic is sound with proper memoization and cycle handling.The method correctly implements:
- Memoization to avoid redundant calculations
- Cycle detection to prevent infinite loops
- Special handling for tuple cycles and recursive relations
- Edge pruning to reduce unnecessary traversal
- Weight propagation with appropriate increments for Direct/TTU edges
- Strategy selection based on node type (union, intersection, exclusion)
897-994: Cycle handling logic correctly addresses recursive and tuple cycle scenarios.The three-method approach effectively handles cycles:
calculateUsersetNodeWeightWhenCycleorchestrates the processupdateUsersetCycleWeightpropagates weights throughout the cyclefindUsersetWeightInCyclediscovers weights within cycle pathsThe logic correctly differentiates between edges that are part of cycles (returning Infinite) and non-cycle edges (using regular calculation).
996-1042: Strategy methods correctly implement operator semantics.The three strategy methods properly reflect the semantics of different operators:
- Max strategy (union, direct grouping): Succeeds if any branch reaches the userset
- Max with enforcement (intersection): Succeeds only if all branches reach the userset
- Hybrid strategy (exclusion): Handles "A but not B" by ensuring A reaches the userset and considering B's impact
1044-1072: Pruning logic correctly identifies edges that cannot contribute to userset paths.The
canPruneEdgemethod implements effective pruning by:
- Checking if the edge reaches the terminal types required by the userset
- Verifying that the edge's reach is sufficient (considering the +1 hop cost for Direct/TTU edges)
- Handling the special case of Infinite weights (recursive/cycle paths)
This prevents unnecessary traversal of edges that cannot possibly reach the target userset.
1074-1082: LGTM! Clean setter utilities for memoization.The setter methods provide a consistent way to update memoization maps while returning the weight value, simplifying the calling code.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/go/graph/weighted_graph.go (2)
153-166: Document the boolean return valueThe function follows Go's getter pattern where the boolean indicates whether the weight exists, but this isn't documented. Consider adding a more detailed comment explaining what the boolean represents.
Apply this diff:
-// GetWeight returns the weight for the given key in the node, it could be a userset key or a terminal type key +// GetWeight returns the weight for the given key in the node and a boolean indicating whether the weight exists. +// The key can be either a userset key (containing "#") or a terminal type key. func (wg *WeightedAuthorizationModelGraph) GetWeight(node *WeightedAuthorizationModelNode, key string) (int, bool) {
203-214: Early pruning logic is correct but could use clearer commentsThe three pruning conditions are:
!exists: Node lacks a weight for a terminal type that the userset requires → no path possiblenodeValue < usersetValue: Node's distance to terminal is shorter than userset's → node is "upstream" and can't reach usersetnodeValue == usersetValue && nodeValue != Infinite: Equal distances but not infinite → would need +1 edge weight increment to reach, not possible with equal weightsConsider expanding the inline comments to explain this reasoning more clearly for future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/go/graph/weighted_graph.go(7 hunks)
🔇 Additional comments (6)
pkg/go/graph/weighted_graph.go (6)
57-57: LGTM: Consistent initialization of usersetWeightsThe
usersetWeightsmap is correctly initialized across all node and edge creation paths.Also applies to: 69-69, 79-79, 106-106
805-899: Complex but well-structured recursive logicThe recursive weight calculation correctly handles:
- Memoization to avoid redundant computation
- Cycle detection via visited tracking (with separate handling for tuple cycles)
- Early pruning via canPruneEdge
- Incremental weight calculation for DirectEdge/TTUEdge
- Multiple aggregation strategies based on node type
The comments at lines 812-814 helpfully explain the visited vs. cycle handling distinction.
906-998: Cycle handling logic is intricate but appears correctThe three-function cycle handling approach:
calculateUsersetNodeWeightWhenCycle: Entry point for cycle nodesfindUsersetWeightInCycle: Searches for userset within/from cycle, returns Infinite if foundupdateUsersetCycleWeight: Propagates the Infinite weight to all cycle nodesThe decision to return Infinite for any node in a cycle that can reach the userset is conservative but correct for pruning purposes—you cannot determine a finite distance from within a cycle.
1065-1080: Clarify the addOn logic in pruning conditionThe pruning condition checks
edgeWeight <= value + addOnwhereaddOnis 1 for DirectEdge/TTUEdge and 0 otherwise. However, the comment at lines 1056-1059 states rules 2 and 3 suggest pruning whenedgeWeight <= usersetWeight(for non-Infinite weights), which doesn't mention the addOn adjustment.The early pruning in
getWeightForUserset(line 210) checksnodeValue <= usersetValuewithout an addOn adjustment.Can you clarify why addOn is included in the comparison? Is it accounting for the cost of traversing the edge from the current node to edge.to?
1086-1092: LGTM: Simple setter functionsThese utility functions encapsulate the setting of userset weights on nodes and edges, providing a clear API for weight updates throughout the codebase.
1038-1054: Clarify or fix the exclusion weight calculation logic for usersetsIn the hybrid strategy for exclusion (
A but not B), the code returnsweight2(B's weight) whenweight1 <= weight2 && weight2 != 0. This contradicts the expected semantics:
- Terminal weights for exclusion (line 571-620): A's types are primary; overlapping B types use max weight
- Userset weights should follow similar semantics: A's distance should be primary, not B's
Returning B's path distance for exclusion semantically inverts the operation. For example, if A reaches the userset in 2 steps and B reaches it in 3 steps, returning weight 3 misrepresents the reachability through the "A but not B" path.
Action: Either fix the logic to return
weight1(with appropriate B-based filtering), or add documentation and tests explaining whyweight2is semantically correct for exclusion.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
3 participants
This PR provides a weight to traverse and prune branches that do not lead to the userset path, only valid when accessing the weight of a userset
Description
What problem is being solved?
How is it being solved?
What changes are made to solve it?
References
Review Checklist
mainSummary by CodeRabbit
New Features
Tests