Skip to content

Conversation

@CharlieDigital
Copy link

@CharlieDigital CharlieDigital commented Nov 8, 2025

Provides an interface for the OpenFgaClient to improve testability in DI scenarios by allowing mocks and substitutes to be created.

Description

What problem is being solved?

It is currently not possible to mock the OpenFgaClient in unit tests in .NET as its methods are not virtual and it does not provide an interface for mocking

How is it being solved?

This PR adds an interface extracted from the OpenFgaClient

What changes are made to solve it?

  • Added an interface for the OpenFgaClient
  • Reformatted comments to be idiomatic C# comment style for editor support
  • Changed implementation comments to <inheritdoc />

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

  • Bug Fixes
    • Added validation to prevent null authorization model identifiers during read operations, improving error handling.

Copilot AI review requested due to automatic review settings November 8, 2025 20:22
@CharlieDigital CharlieDigital requested a review from a team as a code owner November 8, 2025 20:22
@coderabbitai
Copy link

coderabbitai bot commented Nov 8, 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.

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

Extracts the public API surface of OpenFgaClient into a new IOpenFgaClient interface, with OpenFgaClient now implementing both IDisposable and IOpenFgaClient. Added runtime validation in ReadAuthorizationModel to throw when AuthorizationModelId is null. Doc comments replaced with /// <inheritdoc /> directives.

Changes

Cohort / File(s) Summary
Interface Definition
src/OpenFga.Sdk/Client/IOpenFgaClient.cs
New public interface defining 20+ async methods for store management, authorization model handling, relationship operations, and assertion management. Methods accept request bodies/options and cancellation tokens, returning task-based responses.
Class Implementation
src/OpenFga.Sdk/Client/Client.cs
OpenFgaClient class signature updated to implement IOpenFgaClient in addition to IDisposable. Added null validation in ReadAuthorizationModel to throw if AuthorizationModelId is null. Doc comments replaced with /// <inheritdoc /> across multiple methods.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Interface method signatures: Verify all 20+ method signatures are correctly defined with proper parameter types, return types, and async patterns.
  • Implementation completeness: Confirm OpenFgaClient implements all interface members from IOpenFgaClient without breaking existing behavior.
  • Validation logic: Review the new null-check in ReadAuthorizationModel for correctness and ensure it aligns with intended runtime behavior.
  • Doc comment migration: Verify /// <inheritdoc /> replacements preserve semantic documentation across methods.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main change: adding an interface (IOpenFgaClient) to the OpenFgaClient class to enable mocking/substitution in dependency injection and testing scenarios.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces the IOpenFgaClient interface to formalize the contract for the OpenFGA client. The OpenFgaClient class now implements this interface, improving testability and enabling dependency injection scenarios.

  • Extracted interface with all 21 public methods from OpenFgaClient
  • Replaced block-style comments (/** */) with XML documentation comments (///)
  • Implementation uses <inheritdoc /> tags to reference interface documentation

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File Description
src/OpenFga.Sdk/Client/IOpenFgaClient.cs New interface defining the contract for OpenFGA client with comprehensive XML documentation for all 21 methods
src/OpenFga.Sdk/Client/Client.cs Updated class declaration to implement IOpenFgaClient and replaced method documentation with <inheritdoc /> tags

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link

@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)
src/OpenFga.Sdk/Client/Client.cs (1)

468-468: Optional: Remove trailing whitespace.

Line 468 appears to contain trailing whitespace on an otherwise empty line. Consider removing it to maintain consistent formatting.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e7a67f3 and 9ba0705.

📒 Files selected for processing (2)
  • src/OpenFga.Sdk/Client/Client.cs (14 hunks)
  • src/OpenFga.Sdk/Client/IOpenFgaClient.cs (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/OpenFga.Sdk/Client/Client.cs (2)
src/OpenFga.Sdk.Test/Client/OpenFgaClientTests.cs (1)
  • OpenFgaClient (57-77)
src/OpenFga.Sdk/Api/OpenFgaApi.cs (16)
  • Task (52-74)
  • Task (85-107)
  • Task (117-133)
  • Task (143-164)
  • Task (175-197)
  • Task (207-228)
  • Task (239-261)
  • Task (273-297)
  • Task (308-330)
  • Task (341-363)
  • Task (374-398)
  • Task (409-433)
  • Task (445-472)
  • Task (486-520)
  • Task (531-553)
  • Task (565-590)
src/OpenFga.Sdk/Client/IOpenFgaClient.cs (4)
src/OpenFga.Sdk/Client/Model/ClientCreateStoreRequest.cs (1)
  • ClientCreateStoreRequest (6-7)
src/OpenFga.Sdk/Client/Model/ClientWriteAuthorizationModelRequest.cs (1)
  • ClientWriteAuthorizationModelRequest (6-7)
src/OpenFga.Sdk/Client/Model/ClientReadChangesOptions.cs (1)
  • ClientReadChangesOptions (12-24)
src/OpenFga.Sdk/Client/Model/ClientReadRequest.cs (1)
  • ClientReadRequest (5-6)
⏰ 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: CodeQL analysis (csharp)
🔇 Additional comments (8)
src/OpenFga.Sdk/Client/Client.cs (3)

21-21: LGTM! Interface implementation enables mocking and DI.

The addition of IOpenFgaClient to the class declaration achieves the PR's goal of enabling mocking and dependency injection scenarios while maintaining the existing IDisposable implementation.


229-229: LGTM! Idiomatic use of inheritdoc for interface implementation.

Replacing the full XML documentation with /// <inheritdoc /> is the recommended C# pattern for interface implementations, as it maintains a single source of truth and ensures documentation consistency.

Also applies to: 234-234, 240-240, 244-244, 252-252, 258-258, 264-264, 276-276, 294-294, 300-300, 317-317, 389-389, 394-394, 403-403, 421-421, 434-434, 451-451, 469-469, 506-506, 524-524, 535-535


264-274: LGTM! Appropriate validation for required parameter.

The runtime null check for AuthorizationModelId is defensive and consistent with the validation pattern used elsewhere in the class (e.g., GetStoreId at lines 192-194). It provides a clear error message before passing invalid data to the API layer.

src/OpenFga.Sdk/Client/IOpenFgaClient.cs (5)

1-15: LGTM! Well-structured interface declaration.

The namespace imports, conditional compilation directives, and interface declaration are properly structured and align with the implementation in Client.cs.


17-52: LGTM! Store management methods properly documented.

The store management methods (ListStores, CreateStore, GetStore, DeleteStore) have comprehensive XML documentation and signatures that match the implementation.


54-93: LGTM! Authorization model methods properly defined.

The authorization model methods have appropriate signatures and comprehensive documentation. The nullable return type for ReadLatestAuthorizationModel correctly reflects that no model may exist.


95-145: LGTM! Relationship tuple and query methods well-documented.

The relationship tuple management and query methods (ReadChanges, Read, Write, WriteTuples, DeleteTuples, BatchCheck, Expand, ListObjects, ListUsers) have comprehensive XML documentation and correct signatures.

Also applies to: 157-189, 201-210


212-231: LGTM! Assertion methods properly defined.

The assertion methods (ReadAssertions, WriteAssertions) are properly documented and have signatures that match the implementation.

@dyeam0
Copy link
Member

dyeam0 commented Nov 10, 2025

@CharlieDigital Thanks for the PR. We will assign someone to review it.

@dyeam0
Copy link
Member

dyeam0 commented Nov 13, 2025

@CharlieDigital I'd just like to confirm that your PR's intent is to only implement for .NET SDK (as compared to the first PR openfga/sdk-generator#652 which attempted in the SDK generator for all languages)?

@CharlieDigital
Copy link
Author

CharlieDigital commented Nov 13, 2025

@dyeam0 That is correct; I am only aiming to update the .NET implementation.

Additional context: https://cloud-native.slack.com/archives/C06G1NNH47N/p1761829148780869?thread_ts=1761407689.874249&cid=C06G1NNH47N

image

@CharlieDigital
Copy link
Author

Hey there; checking in to see if there's anything missing on this PR or anything that requires additional attention.

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.

Provide client service through interfaces to improve decoupling and testability of client application

3 participants