fixed recursive nested tuple cycles assignations#497
Conversation
WalkthroughThe change propagates a node’s tupleCycle flag into dependent edges and nodes during weight-fixing in weighted_graph.go. Tests were added to validate nested and recursive tuple cycle scenarios, and one existing test expectation for a node’s tupleCycle flag was updated. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant GB as GraphBuilder
participant WG as WeightedGraph
participant FEN as fixDependantNodesWeight
participant FEE as fixDependantEdgesWeight
participant N as Node
participant E as Edge
GB->>WG: build()
WG->>FEN: adjust dependent node weights
FEN->>N: read node.tupleCycle
FEN->>N: update fromNode weights
alt node.tupleCycle == true
FEN->>N: set fromNode.tupleCycle = true
end
WG->>FEE: adjust dependent edge weights
FEE->>N: read node.tupleCycle
FEE->>E: update edge weights
alt node.tupleCycle == true
FEE->>E: set edge.tupleCycle = true
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 0
🧹 Nitpick comments (5)
pkg/go/graph/weighted_graph.go (2)
659-660: Tuple-cycle propagation to dependent edges — LGTM; tiny naming nit.Behavior is correct and matches the PR intent. Consider renaming the local boolean to avoid confusion with the slice/term “tuple cycle”.
Apply:
- tupleCycle := node.tupleCycle + isTupleCycle := node.tupleCycle ... - if tupleCycle { + if isTupleCycle { edge.tupleCycle = true }Also applies to: 693-695
704-705: Tuple-cycle propagation to dependent from-nodes — LGTM; keep naming consistent.Same nit as above for readability.
- tupleCycle := node.tupleCycle + isTupleCycle := node.tupleCycle ... - if tupleCycle { + if isTupleCycle { fromNode.tupleCycle = true }Also applies to: 726-728
pkg/go/graph/weighted_graph_builder_test.go (3)
796-853: Strong coverage for super‑nested cycles; add GetWeight ok assertions to harden tests.Current checks ignore the boolean return; add it to fail fast on missing keys. Also, index‑based edge assertions are a bit order‑sensitive.
Example pattern to apply in this test (and similar spots):
- weight, _ := graph.edges["document#viewer"][0].GetWeight("user") + weight, ok := graph.edges["document#viewer"][0].GetWeight("user") + require.True(t, ok) require.True(t, graph.edges["document#viewer"][0].IsPartOfTupleCycle()) require.Equal(t, Infinite, weight)
855-929: Algebraic + nested cycles test looks good; minor robustness tweak.Same suggestion: assert the ok flag from GetWeight; optional: prefer selecting edges by to.uniqueLabel over fixed indices to reduce brittleness.
931-1011: Typo in test name; rename for clarity.Function name: “Recursionc” → “Recursion”. Also consider adding ok checks on GetWeight as above.
-func TestGraphConstructionRecursioncWithNestedCycles(t *testing.T) { +func TestGraphConstructionRecursionWithNestedCycles(t *testing.T) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/go/graph/weighted_graph.go(4 hunks)pkg/go/graph/weighted_graph_builder_test.go(1 hunks)pkg/go/graph/weighted_graph_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/go/graph/weighted_graph_builder_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 (1)
pkg/go/graph/weighted_graph_test.go (1)
1212-1212: Expectation flip to true is correct.Matches the new tuple-cycle propagation to dependent nodes.
2 participants
For the recursive nested tuples cycles model, some edges were missing the tuple cycle flag. This issue does not affect the weight of the node or the edge that are part of the tuple cycle. The weight is correctly calculated in the recursion or cycles, but the inclusion of all possible edges as part of the tuple cycle was missing some use cases.
Description
What problem is being solved?
For example for a model like this:
All the edges were part of the tuple cycle except the last edge between document#viewer and the org#employee, and the reason was the org#employee was already declared as part of the tuple cycles and had a referential weight based on document#viewer. However, the edge between document#viewer and org@employee didn't detect a cycle because the node was already visited, and a cycle was already detected.
How is it being solved?
When fixing the referential weight in the presence of a cycle, if the node owning the reference id has the tuple cycles, all the dependent cycle edges need to be declared as a tuple cycle.
What changes are made to solve it?
References
Review Checklist
main