Feat/grouping userset ttus #493
Conversation
WalkthroughRemoves legacy public NodeType/EdgeType declarations from core graph files, introduces new NodeType/EdgeType and WeightedAuthorizationModelEdge in weighted_graph files, adds logical grouping nodes/edges in the builder, updates weight-assignment logic to treat logical operators/unions, and adapts tests to the new grouping topology. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant B as Builder
participant P as Parent Node
participant G as Logical Grouping Node
participant T as Target Node
rect rgba(90,140,200,0.06)
note over B: When multiple direct/TTU related targets detected
B->>G: Create LogicalGrouping (Direct or TTU)
B->>P: Upsert LogicalEdge (DirectLogicalEdge/TTULogicalEdge)
G-->>T: Upsert edge(s) (DirectEdge/TTUEdge) to each target
end
sequenceDiagram
autonumber
participant WG as WeightedGraph
participant N as Node
participant E as Edges
rect rgba(120,180,120,0.06)
note over WG: AssignWeights traversal
WG->>N: Visit node
alt isLogicalOperator(N)
WG-->>N: Defer weight assignment for later
else
WG->>E: calculateNodeWeightFromTheEdges
alt isLogicalUnionOperator(N)
WG-->>N: Use union-aware cycle/weight handling
else
WG-->>N: Use standard edge-based weight calculation
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/go/graph/weighted_graph_builder.go (1)
144-159: Remove conditions from TTU edges
Replace the condition argument with an empty string so TTU edges remain condition-agnostic:- wg.UpsertEdge(node, nodeSource, TTUEdge, typeTuplesetRelation, relatedType.GetCondition()) + // TTU edges represent rewrite hops; conditions live on direct relation edges. + wg.UpsertEdge(node, nodeSource, TTUEdge, typeTuplesetRelation, "")
🧹 Nitpick comments (8)
pkg/go/graph/graph_node.go (1)
15-16: Tighten comments; fix typo ("wherer") and clarify examples.Small doc nits only; enums look fine. Recommend correcting the TTU comment and making both examples consistent.
Apply this diff:
- LogicalUserset NodeType = 4 // e.g. `[user, employee, type1#rel]` - LogicalTTU NodeType = 5 // e.g. `member from parent, wherer parent can have multiple terminal types` + LogicalUserset NodeType = 4 // e.g., `[user, employee, type1#rel]` + LogicalTTU NodeType = 5 // e.g., `viewer from parent`, where parent can resolve to multiple terminal typesOptional: add a String() for NodeType (or use
stringer) to improve logs/graph dumps.pkg/go/graph/graph_edge.go (1)
11-16: Add DOT attributes for new logical edges
DirectLogicalEdge and TTULogicalEdge are emitted by the builder but currently fall through to an empty Attributes() case—update the EdgeType Attributes() method in pkg/go/graph/graph_edge.go to return distinct labels (e.g."logical"and"logical-ttu").pkg/go/graph/weighted_graph_builder_test.go (2)
601-603: Avoid brittle total node/edge counts; assert structure instead.Grouping nodes will keep evolving; fixed totals tend to break. Prefer asserting presence of key nodes/edges/weights (e.g., LogicalUserset node exists, edge types/conditions correct) rather than
require.Len(graph.nodes, 13)andrequire.Len(graph.edges, 9).Example:
// Assert at least one LogicalUserset exists and conditions merged: found := false for _, n := range graph.nodes { if n.nodeType == LogicalUserset { found = true break } } require.True(t, found) ```<!-- review_comment_end --> --- `1222-1222`: **Reduce reliance on global node/edge counts across intersection suites.** The added `require.Len(graph.nodes, X)` and `require.Len(graph.edges, Y)` are fragile with grouping changes. Keep assertions that truly define correctness (weights, types, operator labels), and drop or relax these totals. Example pattern: ```go // Instead of total counts: andEdge := graph.edges["document#viewer"][0] require.Equal(t, OperatorNode, andEdge.to.nodeType) require.Equal(t, "intersection", andEdge.to.label) require.Equal(t, expected, andEdge.GetWeights()) ```<!-- review_comment_end --> Also applies to: 1247-1247, 1270-1270, 1396-1396, 1489-1491, 1514-1516 </blockquote></details> <details> <summary>pkg/go/graph/weighted_graph.go (4)</summary><blockquote> `132-136`: **Deferring logical nodes during initial pass is fine; fix typo** Typo: “Inititally” → “Initially”. --- `153-160`: **Helper for logical-operator detection is correct; fix minor typos** Typos in comments: “asignations” → “assignments”. --- `161-168`: **Helper for union-like logical operators is correct; fix minor typos** Same typo note as above. --- `467-486`: **Cycle handling for union-like logical nodes: LGTM; fix typo** Typo: “depencies” → “dependencies”. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 222b2371c53f943473b90a323c3dd3d3ec7b92a9 and 2e7e449a39fab2a75b8913ab42d37fa25857ab43. </details> <details> <summary>📒 Files selected for processing (5)</summary> * `pkg/go/graph/graph_edge.go` (1 hunks) * `pkg/go/graph/graph_node.go` (1 hunks) * `pkg/go/graph/weighted_graph.go` (8 hunks) * `pkg/go/graph/weighted_graph_builder.go` (5 hunks) * `pkg/go/graph/weighted_graph_builder_test.go` (12 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code graph analysis (3)</summary> <details> <summary>pkg/go/graph/weighted_graph_builder.go (2)</summary><blockquote> <details> <summary>pkg/go/graph/graph_node.go (3)</summary> * `SpecificTypeAndRelation` (12-12) * `LogicalTTU` (16-16) * `LogicalUserset` (15-15) </details> <details> <summary>pkg/go/graph/graph_edge.go (4)</summary> * `TTULogicalEdge` (16-16) * `TTUEdge` (13-13) * `DirectLogicalEdge` (15-15) * `DirectEdge` (11-11) </details> </blockquote></details> <details> <summary>pkg/go/graph/weighted_graph_builder_test.go (3)</summary><blockquote> <details> <summary>pkg/go/graph/graph_node.go (1)</summary> * `LogicalUserset` (15-15) </details> <details> <summary>pkg/go/transformer/dsltojson.go (1)</summary> * `MustTransformDSLToProto` (553-560) </details> <details> <summary>pkg/go/graph/weighted_graph_builder.go (1)</summary> * `NewWeightedAuthorizationModelGraphBuilder` (19-21) </details> </blockquote></details> <details> <summary>pkg/go/graph/weighted_graph.go (2)</summary><blockquote> <details> <summary>pkg/go/graph/weighted_graph_node.go (1)</summary> * `WeightedAuthorizationModelNode` (3-11) </details> <details> <summary>pkg/go/graph/graph_node.go (5)</summary> * `OperatorNode` (13-13) * `LogicalTTU` (16-16) * `LogicalUserset` (15-15) * `UnionOperator` (18-18) * `SpecificTypeAndRelation` (12-12) </details> </blockquote></details> </details> </details> <details> <summary>🔇 Additional comments (7)</summary><blockquote> <details> <summary>pkg/go/graph/weighted_graph_builder_test.go (2)</summary><blockquote> `1203-1216`: **Non-functional DSL formatting only.** These model string edits are cosmetic; no issues. <!-- review_comment_end --> Also applies to: 1231-1241, 1260-1264, 1280-1291, 1303-1314, 1329-1340, 1354-1365, 1379-1390, 1404-1415, 1473-1484, 1498-1508 --- `1445-1445`: **Good switch to ErrorContains for message assertions.** Less brittle than exact equality; keeps tests resilient to wording tweaks. <!-- review_comment_end --> Also applies to: 1465-1465 </blockquote></details> <details> <summary>pkg/go/graph/weighted_graph_builder.go (3)</summary><blockquote> `124-133`: **TTU logical-grouping creation looks good** Condition and labeling are sensible; node reuse via a deterministic uniqueLabel is fine. --- `182-189`: **Direct logical-grouping creation looks good** Creation gate (non-STR parent + >1 directlyRelated) and edge type are correct. --- `209-209`: **Direct edge upsert with condition is correct** De-dupes and aggregates conditions as expected. </blockquote></details> <details> <summary>pkg/go/graph/weighted_graph.go (2)</summary><blockquote> `456-466`: **Cycle handling for non-logical nodes: LGTM** Branching to max-strategy when not logical is consistent. --- `573-574`: **Intersection strategy assignment: LGTM** Directly assigning rewriteWeights is correct. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
yissellokta
force-pushed
the
feat/grouping_userset_ttus
branch
from
3aa835c to
360c743
Compare
|
@yissellokta is it accurate to say that |
yissellokta
force-pushed
the
feat/grouping_userset_ttus
branch
from
da7e28f to
85f5d4f
Compare
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
pkg/go/graph/weighted_graph_edge.go (1)
58-75: Doc/impl mismatch: tuplesetRelation should be TTUEdge-onlyComment says tuplesetRelation is “only present when the edgeType is a TTUEdge”. In builder, TTULogicalEdge is created with a non-empty tuplesetRelation. Either:
- Do not set tuplesetRelation for non-TTU edges (preferred), or
- Update the contract and all consumers accordingly.
This likely affects filtering/weighting logic that keys on tuplesetRelation. See builder comment with a proposed fix.
pkg/go/graph/weighted_graph_node.go (1)
66-69: Minor wording“returns a true” → “returns true”.
pkg/go/graph/weighted_graph_builder.go (2)
96-99: Outdated comment vs codeComment says the edge from relation to operator “won’t be added” in some cases, but the code always adds it. Update the comment or adjust logic.
145-156: Comment contradicts implementation re: TTU edge conditionsThe comment states TTU edges carry no conditions, yet conditions are passed later. Either:
- Stop attaching conditions to TTU edges (preferred), or
- Update the comment and all downstream logic to support TTU-edge conditions.
See next comment for a concrete fix.
🧹 Nitpick comments (3)
pkg/go/graph/weighted_graph_edge.go (1)
5-56: EdgeType docs: tighten semantics; fix typos; consider iota
- Clarify that LogicalDirectGrouping/LogicalTTUGrouping are created only when the parent is an operator node; align wording with builder.
- Fix typos: “unecesary” → “unnecessary”, “is create” → “is created”.
- Optional: switch to iota for maintainability.
pkg/go/graph/weighted_graph_node.go (1)
5-16: NodeType/Operator constants: polish + iota
- Fix typo: “wherer” → “where”.
- Optional: use iota for NodeType values.
- Consider String() via stringer for better logs/diagnostics.
pkg/go/graph/weighted_graph_builder.go (1)
108-124: Error message clarityThe TTU parsing errors are good; consider including typeDef.GetType()#relation to aid debugging large models.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
pkg/go/graph/graph_edge.go(0 hunks)pkg/go/graph/graph_node.go(0 hunks)pkg/go/graph/weighted_graph.go(8 hunks)pkg/go/graph/weighted_graph_builder.go(5 hunks)pkg/go/graph/weighted_graph_builder_test.go(16 hunks)pkg/go/graph/weighted_graph_edge.go(1 hunks)pkg/go/graph/weighted_graph_node.go(1 hunks)
💤 Files with no reviewable changes (2)
- pkg/go/graph/graph_edge.go
- pkg/go/graph/graph_node.go
🔇 Additional comments (5)
pkg/go/graph/weighted_graph_edge.go (1)
82-86: “Infinite” weight reference needs a defined constantDocs mention returning “Infinite” for recursion; ensure a shared constant exists and is documented, or reword to reference the actual symbol name.
pkg/go/graph/weighted_graph_builder.go (4)
125-133: Grouping condition matches reviewer expectation (operator-only)The check parentNode.nodeType != SpecificTypeAndRelation && len(directlyRelated) > 1 ensures TTU grouping only under operator/non-relation parents. This satisfies the “create grouping only from an operator node” requirement.
183-190: Direct grouping condition looks goodGrouping only when parent is an operator and there are multiple direct entries prevents unnecessary nodes for simple lists.
93-101: Determinism of operator node IDs (ULID)Using ULID in operator unique labels may affect reproducibility if serialized outputs include uniqueLabel. Ensure downstream comparison/printing relies on label/type, not uniqueLabel; otherwise, consider a deterministic counter or stable hash.
Also applies to: 176-216
63-91: Answer to the clarifying question: grouping created only from operator nodes
- define rel1: [user, employee, other] → three DirectEdge edges, no LogicalDirectGrouping (parent is SpecificTypeAndRelation).
- define rel2: [user, employee, other] or rel3 → one RewriteEdge to rel3; one DirectLogicalEdge to a LogicalDirectGrouping which then fans out via DirectEdge to the three entries.
This matches the intended behavior.
| uniqueLabel := typeDef.GetType() + "#ttu:" + tuplesetRelation + "#" + computedRelation | ||
| // add a logical ttu node for grouping of TTU that are part of the same tuplesetrelation and computed relation | ||
| logicalNode := wg.GetOrAddNode(uniqueLabel, uniqueLabel, LogicalTTUGrouping) | ||
| wg.AddEdge(parentNode.uniqueLabel, logicalNode.uniqueLabel, TTULogicalEdge, parentRelationName, typeTuplesetRelation, nil) |
There was a problem hiding this comment.
Do not set tuplesetRelation on TTULogicalEdge
To honor the “TTUEdge-only” tuplesetRelation contract, pass an empty value when creating the TTULogicalEdge.
Apply:
- wg.AddEdge(parentNode.uniqueLabel, logicalNode.uniqueLabel, TTULogicalEdge, parentRelationName, typeTuplesetRelation, nil)
+ wg.AddEdge(parentNode.uniqueLabel, logicalNode.uniqueLabel, TTULogicalEdge, parentRelationName, "", nil)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| wg.AddEdge(parentNode.uniqueLabel, logicalNode.uniqueLabel, TTULogicalEdge, parentRelationName, typeTuplesetRelation, nil) | |
| wg.AddEdge(parentNode.uniqueLabel, logicalNode.uniqueLabel, TTULogicalEdge, parentRelationName, "", nil) |
🤖 Prompt for AI Agents
In pkg/go/graph/weighted_graph_builder.go around lines 131-131, the call to
wg.AddEdge currently passes typeTuplesetRelation for a TTULogicalEdge; to honor
the TTUEdge-only tuplesetRelation contract replace that argument with an empty
value (e.g. an empty string "" or nil as appropriate for AddEdge’s parameter
type) so the TTULogicalEdge is created without a tuplesetRelation.
| // new edge from "xxx#admin" to "yyy#viewer" tuplesetRelation on "yyy#parent" | ||
| wg.UpsertEdge(parentNode, nodeSource, TTUEdge, parentRelationName, typeTuplesetRelation, relatedType.GetCondition()) | ||
| wg.UpsertEdge(node, nodeSource, TTUEdge, parentRelationName, typeTuplesetRelation, relatedType.GetCondition()) | ||
| } |
There was a problem hiding this comment.
Avoid conditions on TTU edges and handle UpsertEdge errors
- Pass nil for condition to keep TTU edges conditionless (as per comment and model semantics).
- Capture and propagate UpsertEdge error for consistency with parseThis.
Apply:
- wg.UpsertEdge(node, nodeSource, TTUEdge, parentRelationName, typeTuplesetRelation, relatedType.GetCondition())
+ if err := wg.UpsertEdge(node, nodeSource, TTUEdge, parentRelationName, typeTuplesetRelation, nil); err != nil {
+ return err
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // new edge from "xxx#admin" to "yyy#viewer" tuplesetRelation on "yyy#parent" | |
| wg.UpsertEdge(parentNode, nodeSource, TTUEdge, parentRelationName, typeTuplesetRelation, relatedType.GetCondition()) | |
| wg.UpsertEdge(node, nodeSource, TTUEdge, parentRelationName, typeTuplesetRelation, relatedType.GetCondition()) | |
| } | |
| // new edge from "xxx#admin" to "yyy#viewer" tuplesetRelation on "yyy#parent" | |
| if err := wg.UpsertEdge(node, nodeSource, TTUEdge, parentRelationName, typeTuplesetRelation, nil); err != nil { | |
| return err | |
| } | |
| } |
🤖 Prompt for AI Agents
In pkg/go/graph/weighted_graph_builder.go around lines 158 to 160, the TTU edge
is being created with a condition (relatedType.GetCondition()) and the
UpsertEdge return value is ignored; change the call to pass nil for the
condition so TTU edges remain conditionless per the model, and capture the error
returned by wg.UpsertEdge, returning or propagating that error to match
parseThis's error handling conventions.
| for key, value := range edges[1].weights { | ||
| if w, ok := nodeWeights[key]; ok { | ||
| nodeWeights[key] = int(math.Max(float64(w), float64(value))) | ||
| } |
There was a problem hiding this comment.
Fix exclusion to remove B keys instead of merging weights
calculateNodeWeightWithMixedStrategy is still performing a max-merge with the subtracting edge. For exclusion, we need set difference semantics—“A but not B” must drop every type contributed by B. Leaving those keys in place (even at a higher weight) means exclusion nodes can still return users that only appear in the subtracting branch. Please delete the keys instead of merging their weights so the node truly behaves like A \ B. (planetmath.org)
- for key, value := range edges[1].weights {
- if w, ok := nodeWeights[key]; ok {
- nodeWeights[key] = int(math.Max(float64(w), float64(value)))
- }
- }
+ for key := range edges[1].weights {
+ delete(nodeWeights, key)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for key, value := range edges[1].weights { | |
| if w, ok := nodeWeights[key]; ok { | |
| nodeWeights[key] = int(math.Max(float64(w), float64(value))) | |
| } | |
| // Exclusion: drop any types contributed by the subtracting edge (edges[1]). | |
| for key := range edges[1].weights { | |
| delete(nodeWeights, key) | |
| } |
🤖 Prompt for AI Agents
In pkg/go/graph/weighted_graph.go around lines 542 to 545, the loop handling
edges[1].weights incorrectly merges weights via Max, but for exclusion (A \ B)
you must remove any keys contributed by the subtracting edge; change the body so
that when a key from edges[1].weights is found in nodeWeights you delete that
key from nodeWeights (nodeWeights[key] = deleted), rather than taking the
max—ensure you perform a simple delete(nodeWeights, key) and handle the case
where nodeWeights or edges[1].weights might be nil to avoid panics.
To simplify graph operations, the system now introduces logical nodes (LogicalUsersetNode, LogicalTTUNode) to group related edges together. 🤝
Previously, multiple direct user assignments or multiple "tuple-to-userset" (TTU) edges were treated individually, which complicated algorithms, especially during algebraic operations.
Now, these new "union" nodes are created dynamically to consolidate multiple edges into a single logical unit, significantly reducing complexity for core processes like Check and ListObjects.
Description
The Challenge: Simplifying Complex Edge Relationships
The previous weighted graph model did not inherently group multiple direct userset assignments or multiple "tuple-to-userset" (TTU) edges, even when they originated from the same definition. This created significant implementation challenges for core algorithms in OpenFGA, especially when performing algebraic operations. Essentially, the system failed to treat multiple direct edges or multiple TTU edges as a single logical union, unnecessarily increasing complexity.
The Solution: Introducing Logical Nodes and Edges
To reduce complexity in the Check, ListObjects, and indexer compilation algorithms, we are introducing two new types of "union" nodes and two corresponding "rewrite" edges. These new structures help group related edges into single, manageable units.
New Node Types:
LogicalUsersetNode: Consolidates multiple direct user assignments.
LogicalTTUNode: Consolidates multiple TTU edges that belong to the same definition.
New Edge Types:
DirectLogicalEdge: A rewrite edge connecting to a LogicalUsersetNode.
TTULogicalEdge: A rewrite edge connecting to a LogicalTTUNode.
How It Works: Creation Logic
These new logical nodes are created dynamically under specific conditions:
A LogicalUsersetNode is created when an algebraic operation involves more than one direct edge. All relevant direct edges are then extracted and connected to this new logical node.
A LogicalTTUNode is created when an algebraic operation involves more than one TTU edge from the same definition. These TTU edges are then extracted and grouped under the new logical node.
What problem is being solved?
References
Review Checklist
mainSummary by CodeRabbit
New Features
Refactor
Tests