feat(dotnet-sdk): Added support for an interface on the client#168
feat(dotnet-sdk): Added support for an interface on the client#168SoulPancake merged 5 commits intoopenfga:mainfrom
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 Note Other AI code review bot(s) detectedCodeRabbit 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. WalkthroughThe changes introduce a new public interface Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
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 |
|
Related Documentation Checked 7 published document(s) in 1 knowledge base(s). No updates required. How did I do? Any feedback? |
![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 (4)
src/OpenFga.Sdk/Client/IOpenFgaClient.cs (2)
15-16: Consider extendingIDisposablefor resource management.The
OpenFgaClientclass implementsIDisposable, butIOpenFgaClientdoes not extend it. This means consumers using the interface cannot properly dispose of resources without casting to the concrete type, which defeats the purpose of the abstraction for DI scenarios.-public interface IOpenFgaClient +public interface IOpenFgaClient : IDisposable
1-5: Using directives ordering.Minor: The
System.*namespaces are typically placed before project-specific namespaces in C# conventions. Consider reordering for consistency.-using OpenFga.Sdk.Client.Model; -using OpenFga.Sdk.Model; using System.Collections.Generic; using System.Threading; using System.Threading.Tasks; +using OpenFga.Sdk.Client.Model; +using OpenFga.Sdk.Model;src/OpenFga.Sdk/Client/Client.cs (2)
757-812: Consider usingConcurrentBaginstead ofListwith locking for consistency.This method uses
List<ClientBatchCheckSingleResponse>with explicitlockstatements (line 791), while other parallel processing methods (ProcessWriteChunksAsync,ProcessCheckRequestsAsync) useConcurrentBag<T>for thread-safe collection of results.The current approach works correctly, but using
ConcurrentBagwould be more consistent with the rest of the codebase and avoid the locking overhead.Change the parameter type and remove the lock:
private async Task ProcessBatchAsync( List<BatchCheckItem> batch, IClientBatchCheckOptions? options, Dictionary<string, string> headers, Dictionary<string, ClientBatchCheckItem> correlationIdToCheck, - List<ClientBatchCheckSingleResponse> results, + ConcurrentBag<ClientBatchCheckSingleResponse> results, CancellationToken cancellationToken )And update the result handling:
if (batchResponse.Result != null) { - lock (results) - { foreach (var kvp in batchResponse.Result) { // ... - results.Add( + results.Add( new ClientBatchCheckSingleResponse(...) ); } - } }
31-32: Minor: Extra space in XML doc comments.The
/// <inheritdoc/>comments have an extra space after///. This is inconsistent with standard C# XML documentation conventions and may cause issues with some documentation tools.- /// <inheritdoc/> + /// <inheritdoc/>This pattern appears throughout the file on multiple methods.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/OpenFga.Sdk/Client/Client.cs(12 hunks)src/OpenFga.Sdk/Client/IOpenFgaClient.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/OpenFga.Sdk/Client/IOpenFgaClient.cs (7)
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)src/OpenFga.Sdk/Client/Model/ClientWriteResponse.cs (1)
ClientWriteResponse(133-134)src/OpenFga.Sdk/Model/JsonStringEnumMemberConverter.cs (1)
Write(60-62)src/OpenFga.Sdk/Client/Model/ClientWriteAssertionsRequest.cs (1)
ClientAssertion(83-84)
⏰ 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). (2)
- GitHub Check: CodeQL analysis (csharp)
- GitHub Check: Agent
🔇 Additional comments (8)
src/OpenFga.Sdk/Client/Client.cs (8)
47-99: LGTM!The parallel processing implementation correctly uses
Parallel.ForEachAsyncwith proper cancellation token propagation and degree of parallelism control. The error handling appropriately marks all items in a failed chunk with the exception.
146-208: LGTM!The semaphore-based fallback implementation for older .NET frameworks correctly handles throttling with proper disposal and release patterns.
634-668: LGTM - Good input validation.The validation for empty checks, MaxBatchSize, and MaxParallelRequests prevents invalid configurations and potential deadlocks. The bulkRequestId generation for request tracking is a good practice.
869-923: LGTM!The
ListRelationsmethod correctly validates input and usesClientBatchCheckto parallelize relation checks. The error handling propagates the first encountered error appropriately.
467-555: LGTM!The
Writemethod correctly handles both transactional (single batch) and non-transactional (chunked parallel) paths with proper response mapping and cancellation token propagation.
20-45: LGTM!The class declaration, constructor, properties, and disposal implementation are well-structured. The
protectedaccess modifier for theapifield allows for extension through subclassing if needed.
394-419: LGTM!The implementation correctly fetches the latest authorization model by requesting only one item and handling the empty case appropriately.
575-603: LGTM!The
Checkmethod correctly constructs the request with tuple key, contextual tuples, context, authorization model ID, and consistency options.
There was a problem hiding this comment.
Pull request overview
This PR introduces an IOpenFgaClient interface to enable mocking and dependency injection scenarios for the OpenFGA .NET SDK. The interface extracts all public methods and properties from the OpenFgaClient class, allowing developers to create mocks and substitutes for unit testing.
Key Changes
- Adds
IOpenFgaClientinterface with 22 method signatures and 2 properties - Updates
OpenFgaClientto implementIOpenFgaClient - Replaces XML documentation comments with
<inheritdoc/>tags in the implementation
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| src/OpenFga.Sdk/Client/IOpenFgaClient.cs | New interface defining all public methods and properties of OpenFgaClient, enabling mocking for unit tests |
| src/OpenFga.Sdk/Client/Client.cs | Implements IOpenFgaClient interface, reformats code for consistency, and replaces documentation with inheritdoc tags |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
SoulPancake
left a comment
SoulPancake
left a comment
There was a problem hiding this comment.
Thank you so much @CharlieDigital
|
Nice! Will make our integration testing scenarios much easier :) |
Provides an interface for the
OpenFgaClientto improve testability in DI scenarios by allowing mocks and substitutes to be created.(Redux: #158)
Description
What problem is being solved?
It is currently not possible to mock the
OpenFgaClientin unit tests in .NET as its methods are not virtual and it does not provide an interface for mockingHow is it being solved?
This PR adds an interface extracted from the
OpenFgaClientWhat changes are made to solve it?
OpenFgaClient<inheritdoc />References
Review Checklist
mainSummary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.