-
Notifications
You must be signed in to change notification settings - Fork 14
feat: Implement server-side BatchCheck using /batch-check endpoint #150
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
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 WalkthroughThis PR introduces a new server-side batch check API ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client as OpenFgaClient
participant Utils as ClientUtils
participant API as OpenFga API
Note over Client: ClientBatchCheck<br/>(client-side)
Client->>Utils: List<ClientCheckRequest>
Utils-->>Client: ClientBatchCheckClientResponse
Note over Client: BatchCheck<br/>(server-side)
Client->>Utils: ClientBatchCheckRequest<br/>(List<ClientBatchCheckItem>)
Utils->>Utils: For each item:<br/>GenerateCorrelationId()
Utils->>Utils: ChunkList() by MaxBatchSize
par Parallel Execution
Utils->>API: Batch 1<br/>(with CorrelationIds)
Utils->>API: Batch 2<br/>(with CorrelationIds)
Utils->>API: Batch N<br/>(with CorrelationIds)
end
API-->>Utils: BatchCheckItem results
Utils->>Utils: Map responses back<br/>using CorrelationId
Utils-->>Client: ClientBatchCheckResponse<br/>(List<ClientBatchCheckSingleResponse>)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this 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/Model/ClientBatchCheckRequest.cs (1)
66-112: Enforce the documented correlation ID contract.The XML doc promises we accept only letters/numbers/hyphen (≤36 chars), but
Validatecurrently yields nothing, so a bad value slips through until the server rejects it. Please add client-side validation so we can surface a friendlyValidationResultbefore hitting the API.- public IEnumerable<ValidationResult> Validate(ValidationContext validationContext) { - yield break; - } + public IEnumerable<ValidationResult> Validate(ValidationContext validationContext) { + if (!string.IsNullOrWhiteSpace(CorrelationId)) { + if (CorrelationId.Length > 36 || + !CorrelationId.All(static c => char.IsLetterOrDigit(c) || c == '-')) { + yield return new ValidationResult( + "CorrelationId must contain only letters, numbers, or hyphens and be at most 36 characters long.", + new[] { nameof(CorrelationId) }); + } + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
example/Example1/Example1.cs(1 hunks)example/Example1/Example1.csproj(1 hunks)src/OpenFga.Sdk.Test/Client/ClientBatchCheckTests.cs(1 hunks)src/OpenFga.Sdk.Test/Client/OpenFgaClientTests.cs(1 hunks)src/OpenFga.Sdk/Client/Client.cs(3 hunks)src/OpenFga.Sdk/Client/Model/ClientBatchCheckOptions.cs(2 hunks)src/OpenFga.Sdk/Client/Model/ClientBatchCheckRequest.cs(1 hunks)src/OpenFga.Sdk/Client/Model/ClientListRelationsOptions.cs(1 hunks)src/OpenFga.Sdk/Client/Utils/ClientUtils.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-30T04:00:16.791Z
Learnt from: evansims
PR: openfga/dotnet-sdk#122
File: docs/Nodes.md:7-7
Timestamp: 2025-09-30T04:00:16.791Z
Learning: The docs/ directory in the openfga/dotnet-sdk repository contains auto-generated documentation files that should not be manually edited. Markdown formatting issues in these files should be disregarded during review.
Applied to files:
example/Example1/Example1.csproj
🧬 Code graph analysis (3)
src/OpenFga.Sdk.Test/Client/ClientBatchCheckTests.cs (3)
src/OpenFga.Sdk/Client/Model/ClientBatchCheckRequest.cs (8)
ClientBatchCheckItem(19-19)ClientBatchCheckItem(30-43)ClientBatchCheckRequest(135-137)ClientBatchCheckRequest(143-145)ClientBatchCheckSingleResponse(195-195)ClientBatchCheckSingleResponse(204-213)ClientBatchCheckResponse(286-288)ClientBatchCheckResponse(294-296)src/OpenFga.Sdk/Client/Utils/ClientUtils.cs (2)
ClientUtils(11-77)GenerateCorrelationId(16-18)src/OpenFga.Sdk/Client/Model/ClientBatchCheckOptions.cs (1)
ClientBatchCheckOptions(18-40)
example/Example1/Example1.cs (1)
src/OpenFga.Sdk/Client/Model/ClientBatchCheckRequest.cs (4)
ClientBatchCheckRequest(135-137)ClientBatchCheckRequest(143-145)ClientBatchCheckItem(19-19)ClientBatchCheckItem(30-43)
src/OpenFga.Sdk/Client/Client.cs (7)
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/Model/ClientBatchCheckRequest.cs (8)
ClientBatchCheckResponse(286-288)ClientBatchCheckResponse(294-296)ClientBatchCheckRequest(135-137)ClientBatchCheckRequest(143-145)ClientBatchCheckSingleResponse(195-195)ClientBatchCheckSingleResponse(204-213)ClientBatchCheckItem(19-19)ClientBatchCheckItem(30-43)src/OpenFga.Sdk/Constants/FgaConstants.cs (1)
FgaConstants(19-156)src/OpenFga.Sdk/Client/Utils/ClientUtils.cs (3)
BatchCheckItem(61-76)ClientUtils(11-77)GenerateCorrelationId(16-18)src/OpenFga.Sdk/Model/BatchCheckItem.cs (2)
BatchCheckItem(44-58)BatchCheckItem(112-114)src/OpenFga.Sdk/Exceptions/ValidationError.cs (4)
FgaValidationError(8-22)FgaValidationError(12-13)FgaValidationError(16-17)FgaValidationError(20-21)src/OpenFga.Sdk/Model/BatchCheckRequest.cs (2)
BatchCheckRequest(50-59)BatchCheckRequest(96-98)
⏰ 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 (csharp)
🔇 Additional comments (13)
example/Example1/Example1.csproj (1)
11-19: LGTM!The switch to a local project reference is appropriate for the example project, allowing it to reference the latest local build during development.
src/OpenFga.Sdk.Test/Client/OpenFgaClientTests.cs (1)
1414-1414: LGTM!The method name update from
BatchChecktoClientBatchCheckcorrectly reflects the renamed client-side batch check API, maintaining backward compatibility while introducing the new server-sideBatchCheckmethod.src/OpenFga.Sdk.Test/Client/ClientBatchCheckTests.cs (5)
13-93: LGTM!The model tests comprehensively cover:
- Constructor property assignments
- Null argument validations for required fields (User, Relation, Object)
- Request and response object construction
All tests follow proper xUnit conventions and validate expected behavior.
95-111: LGTM!The correlation ID generation tests properly verify:
- Valid GUID format using
Guid.TryParse- Uniqueness of generated IDs
113-147: LGTM!The ChunkList tests provide comprehensive coverage:
- Normal chunking with remainder
- Exact division scenarios
- Null input validation
- Invalid chunk size validation (≤ 0)
All edge cases are properly tested.
149-183: LGTM!The TransformToBatchCheckItem tests properly validate:
- Correct transformation from ClientBatchCheckItem to BatchCheckItem
- Proper tuple key construction with user, relation, and object
- Correlation ID propagation
- Null validation for both item and correlation ID
185-201: LGTM!The options property tests validate that MaxBatchSize and MaxParallelRequests can be properly set and retrieved.
src/OpenFga.Sdk/Client/Utils/ClientUtils.cs (4)
16-18: LGTM!Simple and effective correlation ID generation using standard GUID format. The implementation is straightforward and well-tested.
46-53: LGTM!Clean and efficient implementation using
List<T>.GetRange(). The method properly validates inputs and handles chunking including remainder items correctly.
61-76: LGTM!The transformation method correctly:
- Validates all required inputs
- Maps ClientBatchCheckItem to BatchCheckItem (API model)
- Constructs CheckRequestTupleKey with proper field mapping
- Preserves contextual tuples and context
The design appropriately requires correlation ID to be generated before transformation.
27-37: Verify whether ChunkArray should remain in the public API.No internal usage of
ChunkArraywas found in the codebase. However, since it's a public static method in the utility class, it may be part of the SDK's intentional public API (similar to the parallelChunkListmethod). Confirm whether both chunking methods should be exposed to external consumers or if one should be removed as dead code.example/Example1/Example1.cs (1)
235-270: LGTM! Excellent example code.The batch check examples effectively demonstrate:
- The distinction between client-side (
ClientBatchCheck) and server-side (BatchCheck) batching- Auto-generation of correlation IDs when not provided
- Custom correlation ID assignment
- Accessing results using correlation IDs
The clear comments and console output make it easy to understand the new functionality.
src/OpenFga.Sdk/Client/Model/ClientBatchCheckOptions.cs (1)
11-15: LGTM.The MaxBatchSize property is properly integrated into the batch check implementation. It's assigned with a sensible default at line 491, passed to ChunkList() for chunking at line 517, and validated there to ensure it's greater than zero. The implementation follows the established pattern used for MaxParallelRequests.
Addresses CodeRabbit and CodeQL feedback from PR #150. Changes: - Add validation to prevent MaxBatchSize <= 0 and MaxParallelRequests <= 0 - Prevents potential deadlock in NETSTANDARD2.0/NET48 builds - Throws FgaValidationError with descriptive message - Create IClientServerBatchCheckOptions for server-side BatchCheck - Includes MaxBatchSize and MaxParallelRequests properties - Removes MaxBatchSize from IClientBatchCheckOptions (unused by ClientBatchCheck) - Removes MaxBatchSize from ClientListRelationsOptions (unused by ListRelations) - Fix Equals(object) methods to use exact type checking - Changed from 'is' pattern to obj.GetType() == this.GetType() - Prevents improper equality checks with subclasses (CodeQL warning) - Applied to ClientBatchCheckItem, ClientBatchCheckRequest, ClientBatchCheckSingleResponse, ClientBatchCheckResponse - Update CHANGELOG.md with unreleased features All fixes validated with no linter errors.
ewanharris
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we also make sure that there's an update to the README to include this under the Relationship Queries section like in JS
- Update BatchCheck example to show new server-side API - Use ClientBatchCheckRequest with ClientBatchCheckItem - Show correlation ID generation and filtering by ID - Add MaxBatchSize option example - Simplify example for clarity (2 checks instead of 4) - Add note about ClientBatchCheck() for older servers - Align with JS SDK README structure Addresses feedback from PR #150
- Remove ChunkArray<T> method (not used anywhere) - Keep ChunkList<T> which is actively used by BatchCheck Addresses feedback from PR #150
- Use built-in Enumerable.Chunk for .NET 6.0 and greater - Fall back to manual implementation for older frameworks - Improves performance on modern .NET versions Addresses feedback from PR #150
- Verify X-OpenFGA-Client-Bulk-Request-Id header is present - Validate header value is a valid GUID - Add missing FgaConstants using statement - Follows existing header test patterns Addresses feedback from PR #150
ewanharris
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me, just needs the issues with the merge bringing over some commits fixing
Implements server-side batch check functionality to address issue #94. Changes: - Add new BatchCheck() method using the server-side /batch-check API endpoint - Rename existing BatchCheck() to ClientBatchCheck() (fully supported, not deprecated) - Add ClientBatchCheckRequest, ClientBatchCheckItem models for user-friendly API - Add ClientBatchCheckResponse, ClientBatchCheckSingleResponse for result mapping - Add ClientUtils helper methods (UUID correlation ID generation, list chunking, transformations) - Add MaxBatchSize property to IClientBatchCheckOptions (default: 50) - Update ListRelations to use renamed ClientBatchCheck method Features: - Auto-generates UUID correlation IDs (cross-SDK compatible with JS SDK) - Validates correlation IDs are unique - Chunks requests by MaxBatchSize (default: 50 checks per batch) - Executes batches in parallel with MaxParallelRequests (default: 10) - Supports all target frameworks (netstandard2.0, net48, net8.0, net9.0) - Returns empty result for empty checks (matches JS SDK behavior) - Fail-fast error handling Tests: - Add 18 unit tests for new models and utilities - Update existing tests to use renamed ClientBatchCheck method - Integration tested against live OpenFGA server Fixes #94
Addresses CodeRabbit and CodeQL feedback from PR #150. Changes: - Add validation to prevent MaxBatchSize <= 0 and MaxParallelRequests <= 0 - Prevents potential deadlock in NETSTANDARD2.0/NET48 builds - Throws FgaValidationError with descriptive message - Create IClientServerBatchCheckOptions for server-side BatchCheck - Includes MaxBatchSize and MaxParallelRequests properties - Removes MaxBatchSize from IClientBatchCheckOptions (unused by ClientBatchCheck) - Removes MaxBatchSize from ClientListRelationsOptions (unused by ListRelations) - Fix Equals(object) methods to use exact type checking - Changed from 'is' pattern to obj.GetType() == this.GetType() - Prevents improper equality checks with subclasses (CodeQL warning) - Applied to ClientBatchCheckItem, ClientBatchCheckRequest, ClientBatchCheckSingleResponse, ClientBatchCheckResponse - Update CHANGELOG.md with unreleased features All fixes validated with no linter errors.
- Update BatchCheck example to show new server-side API - Use ClientBatchCheckRequest with ClientBatchCheckItem - Show correlation ID generation and filtering by ID - Add MaxBatchSize option example - Simplify example for clarity (2 checks instead of 4) - Add note about ClientBatchCheck() for older servers - Align with JS SDK README structure Addresses feedback from PR #150
- Remove ChunkArray<T> method (not used anywhere) - Keep ChunkList<T> which is actively used by BatchCheck Addresses feedback from PR #150
- Use built-in Enumerable.Chunk for .NET 6.0 and greater - Fall back to manual implementation for older frameworks - Improves performance on modern .NET versions Addresses feedback from PR #150
- Verify X-OpenFGA-Client-Bulk-Request-Id header is present - Validate header value is a valid GUID - Add missing FgaConstants using statement - Follows existing header test patterns Addresses feedback from PR #150
b79e8d4 to
0445345
Compare
- Change mockHandler to _ in BatchCheck header test - Unused variable doesn't need to be captured - Improves code cleanliness Addresses code review feedback
ewanharris
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I overlooked the casing last time but I think other than we're good!
- Rename ClientServerBatchCheckOptions to ClientBatchCheckOptions - Create ClientBatchCheckClientOptions for client-side ClientBatchCheck() - ClientBatchCheckOptions (server-side) has MaxBatchSize + MaxParallelRequests - ClientBatchCheckClientOptions (client-side) has only MaxParallelRequests - Update ClientListRelationsOptions to inherit from IClientBatchCheckClientOptions - Fix README casing: batchCheck -> BatchCheck, correlationId -> CorrelationId - Update all tests to use correct option types Addresses PR feedback from @ewanharris and @curfew-marathon
Address review feedback from PR #150: 1. Use hash code constants instead of magic numbers - Replace hardcoded 9661/9923 with FgaConstants references - Applied to all GetHashCode() methods in batch check models 2. Reduce code duplication in parallel processing - Extract ProcessBatchAsync() helper method - Share logic between NET6+ and older framework paths 3. Update documentation - Add OpenFGA server v1.8.0+ requirement to CHANGELOG - Regenerate README with correct examples from updated templates - Fix response property names and model usage All tests passing (274/274 on .NET 9.0).
| Credentials = new Credentials() { | ||
| Method = CredentialsMethod.ClientCredentials, | ||
| Config = new CredentialsConfig() { | ||
| // API Token Issuer can contain: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to remove this? @daniel-jonathan
Implement Server-Side BatchCheck Using /batch-check Endpoint
Fixes #94
Summary
This PR implements server-side batch check functionality using the
/batch-checkAPI endpoint. Previously, the SDK performed batch checks by making individual parallel/checkAPI calls. The new implementation uses the server-side batching endpoint, significantly reducing network overhead for checking multiple authorization relationships.Changes
New Server-Side BatchCheck Method
BatchCheck()method that uses the/batch-checkAPI endpointMaxBatchSize(default: 50 checks per batch)MaxParallelRequests(default: 10 concurrent batches)Renamed Existing Method
BatchCheck()toClientBatchCheck()/checkcalls (useful for small numbers of checks)ListRelations()to use the renamedClientBatchCheck()methodNew Models
Client Models (User-Facing API):
ClientBatchCheckItem- User-friendly check item with simple propertiesClientBatchCheckRequest- Request container with list of checksClientBatchCheckSingleResponse- Individual check result with correlation IDClientBatchCheckResponse- Response container with list of resultsUtilities:
ClientUtils.GenerateCorrelationId()- Generates UUID correlation IDsClientUtils.ChunkList<T>()- Splits lists into batchesClientUtils.TransformToBatchCheckItem()- Transforms client model to API modelUpdated Options
MaxBatchSizeproperty toIClientBatchCheckOptionsandClientBatchCheckOptionsMaxBatchSizeproperty toClientListRelationsOptions(implementsIClientBatchCheckOptions)Naming Conventions
All naming follows the JavaScript SDK patterns for cross-SDK consistency:
ClientBatchCheckItemClientBatchCheckItemClientBatchCheckRequestClientBatchCheckRequestClientBatchCheckSingleResponseClientBatchCheckSingleResponseClientBatchCheckResponseClientBatchCheckResponsemaxBatchSizeMaxBatchSizecorrelationIdCorrelationIdThe
Client*prefix pattern distinguishes user-facing models from auto-generated API models (BatchCheckItem,BatchCheckRequest, etc.).Framework Support
The implementation supports all existing target frameworks with conditional compilation:
Uses
Parallel.ForEachAsyncfor .NET 6+ andSemaphoreSlimfor older frameworks.Testing
ClientBatchCheck()methodBreaking Changes
None. The renamed
ClientBatchCheck()method maintains the same signature and behavior. Existing code will continue to work without modification.Example Usage
References
js-sdk/client.ts(lines 684-759)Summary by CodeRabbit