Skip to content

Feat/write validation#517

Merged
yissellokta merged 4 commits intomainfrom
feat/write_validation
Dec 1, 2025
Merged

Feat/write validation#517
yissellokta merged 4 commits intomainfrom
feat/write_validation

Conversation

@yissellokta
Copy link
Copy Markdown
Contributor

@yissellokta yissellokta commented Dec 1, 2025

Weighted graph now supports functionality to be able to validate write tuples. We only allow writing tuples when direct edges exist, direct edges can be directly assigned to a node relation, or it can connect operational nodes. A new function was added to the weighted graph to return directly assigned edges that are defined by a node relation.

Description

What problem is being solved?

How is it being solved?

What changes are made to solve it?

References

Review Checklist

  • I have clicked on "allow edits by maintainers".
  • I have added documentation for new/changed functionality in this PR or in a PR to openfga.dev [Provide a link to any relevant PRs in the references section above]
  • The correct base branch is being used, if not main
  • I have added tests to validate that the change in functionality is working as expected

Summary by CodeRabbit

  • New Features

    • Added tracking of direct assignments within the authorization graph for improved relationship visibility.
    • Introduced direct edge retrieval capability to efficiently access authorization mappings.
  • Tests

    • Added validation tests for direct assignment tracking across multiple authorization relationship types.
    • Added functional tests for direct edge retrieval and edge mapping consistency.

✏️ Tip: You can customize this high-level summary in your review settings.

@yissellokta yissellokta requested a review from a team as a code owner December 1, 2025 16:26
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Dec 1, 2025

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

The changes introduce a new directAssigns tracking field on authorization model nodes to record direct assignments, along with a method to retrieve direct edges for nodes with direct assignments. The graph builder now populates this field during parsing, and comprehensive tests validate the new functionality.

Changes

Cohort / File(s) Summary
Node structure enhancement
pkg/go/graph/weighted_graph_node.go
Added directAssigns []string field to WeightedAuthorizationModelNode struct and introduced GetDirectAssigns() accessor method for retrieving the field value.
Direct edge retrieval
pkg/go/graph/weighted_graph.go
Implemented GetDirectEdgesAssignation(node *WeightedAuthorizationModelNode) method that returns direct edges for nodes with direct assignments, handling direct, grouped, and nested operator node cases.
Graph builder integration
pkg/go/graph/weighted_graph_builder.go
Modified parseThis function to retrieve parent relation nodes and append current node unique labels to parentRelationNode.directAssigns during iteration over directly related definitions.
Test coverage
pkg/go/graph/weighted_graph_test.go
Added TestDirectAssignsGraph to validate direct assignation tracking across multiple relations, and TestGetDirectEdgesAssignation to verify edge retrieval behavior and consistency.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • The GetDirectEdgesAssignation method contains logic for handling multiple edge cases (direct, grouped, nested under operators) that warrants careful review
  • Changes span three implementation files with interconnected logic for tracking and retrieving direct assignments
  • Test coverage adds clarity but requires understanding the expected graph structure behavior

Possibly related PRs

Suggested reviewers

  • adriantam
  • justincoh
  • elbuo8

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title check ❓ Inconclusive The title 'Feat/write validation' is vague and generic. While it references a real component of the PR (write validation), it uses non-descriptive formatting (Feat/ prefix) and doesn't clearly convey the specific functionality being added (direct edge tracking and validation in the weighted graph). Revise the title to be more specific and descriptive, such as 'Add direct edge assignment tracking for write validation' or 'Implement GetDirectEdgesAssignation for weighted graph validation'.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@dosubot
Copy link
Copy Markdown

dosubot bot commented Dec 1, 2025

Documentation Updates

1 document(s) were updated by changes in this PR:

OpenFGA's Space

How did I do? Any feedback?  Join Discord

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
pkg/go/graph/weighted_graph_node.go (1)

29-29: Minor typo in comment.

The comment refers to "this maps" but the field is a slice ([]string), not a map.

-	directAssigns     []string // refers to the direct assignments that a node relation can have, this maps will allow to in o(1) know if a write is correct or a contextual tuple is correct
+	directAssigns     []string // refers to the direct assignments that a node relation can have, this slice will allow to know if a write is correct or a contextual tuple is correct
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e4d9ef4 and 27dd9fe.

📒 Files selected for processing (4)
  • pkg/go/graph/weighted_graph.go (3 hunks)
  • pkg/go/graph/weighted_graph_builder.go (2 hunks)
  • pkg/go/graph/weighted_graph_node.go (1 hunks)
  • pkg/go/graph/weighted_graph_test.go (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/go/graph/weighted_graph_test.go (2)
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 (2)
pkg/go/graph/weighted_graph_node.go (2)
  • WeightedAuthorizationModelNode (20-30)
  • SpecificTypeAndRelation (9-9)
pkg/go/graph/weighted_graph_edge.go (2)
  • WeightedAuthorizationModelEdge (60-78)
  • DirectEdge (13-13)
⏰ 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 (8)
pkg/go/graph/weighted_graph_node.go (1)

37-39: LGTM!

The accessor follows the established pattern of other getters in this struct.

pkg/go/graph/weighted_graph.go (3)

62-62: LGTM!

Consistent initialization of directAssigns with an empty slice.


74-74: LGTM!

Consistent initialization of directAssigns matching the pattern in AddNode.


154-169: Verify the "unreachable" return statement.

The comment at line 168 states "it will never get here", but if len(node.directAssigns) == 1 and the single direct edge is nested beyond operator nodes in a way that never gets found (e.g., only TTU or computed edges), the loop will exit with traverseEdges empty and return nil.

Confirm this is the intended behavior for such edge cases, or add defensive handling/logging if this represents an unexpected model state.

pkg/go/graph/weighted_graph_builder.go (1)

209-209: LGTM!

Appending to directAssigns correctly tracks direct assignments for the parent relation node.

pkg/go/graph/weighted_graph_test.go (3)

8-8: LGTM!

Import alias language is clear and follows common Go conventions.


1356-1415: LGTM!

Comprehensive test coverage for directAssigns field population across various relation configurations including simple relations, grouped direct assignments, recursive usersets, and cross-type references.


1417-1481: LGTM!

Good test coverage for GetDirectEdgesAssignation including:

  • Direct edges on simple relations
  • Grouped direct edges via LogicalDirectGrouping
  • Nested operator scenarios
  • Relations without direct assignments (verifier)
  • But-not exclusion patterns (locator)

Comment thread pkg/go/graph/weighted_graph_builder.go Outdated
Comment thread pkg/go/graph/weighted_graph.go
elbuo8
elbuo8 previously approved these changes Dec 1, 2025
@yissellokta yissellokta added this pull request to the merge queue Dec 1, 2025
Merged via the queue into main with commit 8f149e7 Dec 1, 2025
10 checks passed
@yissellokta yissellokta deleted the feat/write_validation branch December 1, 2025 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants