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 Use the checkbox below for a quick retry:
WalkthroughIntroduces ApiExecutor feature enabling raw HTTP requests against OpenFGA APIs without typed SDK methods. Adds public classes ApiExecutor, ApiExecutorRequestBuilder, and ApiResponse. Includes internal ApiClient method for response wrapper support and public GetApiExecutor() on OpenFgaClient. Adds comprehensive example project and tests. Changes
Sequence DiagramsequenceDiagram
participant Client as OpenFgaClient
participant Executor as ApiExecutor
participant Builder as ApiExecutorRequestBuilder
participant ApiClient as ApiClient
participant HTTP as HttpClient
participant Response as ApiResponse
Client->>Client: GetApiExecutor() creates lazy instance
Client->>Executor: new ApiExecutor(apiClient, config)
Note over Client,Builder: Build Request
Builder->>Builder: Of(method, path)
Builder->>Builder: PathParam/QueryParam/Header/Body
Builder->>Builder: Build() validates
Note over Executor,HTTP: Send Request
Executor->>Executor: SendAsync(request)
Executor->>ApiClient: SendRequestWithWrapperAsync<T>()
ApiClient->>ApiClient: BuildHeaders (merge defaults + custom)
ApiClient->>HTTP: Send with auth, retries
HTTP-->>ApiClient: HttpResponseMessage
ApiClient-->>Executor: ResponseWrapper<T>
Note over Executor,Response: Process Response
Executor->>Response: FromHttpResponse(message, raw, data)
Response-->>Executor: ApiResponse<T>
Executor-->>Client: ApiResponse<T> with StatusCode, Headers, Data
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
There was a problem hiding this comment.
Pull request overview
This PR introduces an ApiExecutor feature that allows developers to make custom API requests to OpenFGA endpoints using a fluent builder pattern. The implementation provides both typed and raw JSON response handling while automatically leveraging the SDK's authentication, retry logic, and error handling infrastructure.
Changes:
- Added
ApiExecutorclass for executing arbitrary API requests with automatic auth/retry handling - Added
ApiExecutorRequestBuilderfor fluent request construction with path params, query params, headers, and body - Added
ApiResponse<T>class to encapsulate response data including status, headers, raw response, and typed data - Exposed internal
ApiClientthroughOpenFgaApito enableApiExecutorfunctionality - Added comprehensive unit tests, integration tests, and documentation
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 31 comments.
Show a summary per file
| File | Description |
|---|---|
| src/OpenFga.Sdk/Client/Client.cs | Adds ApiExecutor property with lazy initialization and disposal handling |
| src/OpenFga.Sdk/Client/ApiExecutor/ApiResponse.cs | Response wrapper providing status, headers, raw and typed response data |
| src/OpenFga.Sdk/Client/ApiExecutor/ApiExecutorRequestBuilder.cs | Fluent builder for constructing API requests with validation |
| src/OpenFga.Sdk/Client/ApiExecutor/ApiExecutor.cs | Executes requests with typed or raw responses using SDK infrastructure |
| src/OpenFga.Sdk/ApiClient/ApiClient.cs | Adds SendRequestWithWrapperAsync to expose raw response alongside typed data |
| src/OpenFga.Sdk/Api/OpenFgaApi.cs | Exposes internal ApiClient for ApiExecutor usage |
| src/OpenFga.Sdk.Test/OpenFga.Sdk.Test.csproj | Adds Testcontainers dependency for integration testing |
| src/OpenFga.Sdk.Test/Client/ApiExecutor/*.cs | Comprehensive unit and integration tests for all ApiExecutor components |
| README.md | Documents ApiExecutor usage with examples and feature descriptions |
| CHANGELOG.md | Records new ApiExecutor feature in unreleased section |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/OpenFga.Sdk.Test/Client/ApiExecutor/ApiExecutorIntegrationTests.cs
Outdated
Show resolved
Hide resolved
SoulPancake
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 101 out of 102 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/OpenFga.Sdk.Test/Client/ApiExecutor/ApiExecutorIntegrationTests.cs
Outdated
Show resolved
Hide resolved
|
Related Documentation Checked 5 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: 3
🤖 Fix all issues with AI agents
In `@example/ApiExecutorExample/Makefile`:
- Line 1: Add the missing run-all target to the .PHONY declaration so the
Makefile treats the run-all target as phony; update the .PHONY line (which
currently lists help start-openfga stop-openfga run clean) to include run-all to
prevent accidental conflicts with a filesystem entry named "run-all".
In `@example/ApiExecutorExample/README.md`:
- Line 66: Update the fenced code block that contains "=== OpenFGA ApiExecutor
Example ===" to include a language specifier (e.g., add ```text or ```console)
so the block renders consistently; locate the markdown fenced block in README.md
and replace the opening ``` with ```text (or ```console) ensuring the closing
``` remains unchanged.
In `@src/OpenFga.Sdk/Api/OpenFgaApi.cs`:
- Around line 44-48: The ApiClientInternal property will be overwritten by
codegen; remove it from the generated OpenFgaApi class and instead add it to a
new partial class declaration (e.g., class OpenFgaApi in OpenFgaApi.partial.cs)
in the same namespace so the generated class and your partial class merge at
compile time; implement the internal ApiClientInternal => _apiClient property in
that partial file and ensure the new file is kept outside the generator's
overwrite scope (or add the generated file to the generator ignore list if you
intend to fully manage it manually).
🧹 Nitpick comments (6)
example/README.md (1)
7-18: Documentation additions look good.Minor grammar nit: "bare bones" on line 8 should be hyphenated as "bare-bones" when used as a compound adjective.
📝 Suggested fix
**Example 1:** -A bare bones example. It creates a store, and runs a set of calls against it including creating a model, writing tuples and checking for access. +A bare-bones example. It creates a store, and runs a set of calls against it including creating a model, writing tuples and checking for access.src/OpenFga.Sdk/ApiClient/ApiClient.cs (1)
127-153: Header validation is bypassed for additionalHeaders.The
SendRequestWithWrapperAsyncmethod callsBuildHeaderswithnullfor options (line 135), then manually mergesadditionalHeaders(lines 138-142). This bypasses theConfiguration.ValidateHeaderscall that occurs inBuildHeadersfor per-request headers.Additionally, the manual merge allows
additionalHeadersto override theAuthorizationheader set by the SDK's authentication flow. While this may be intentional for advanced use cases, it could also inadvertently allow callers to corrupt authentication.Consider:
- Validating
additionalHeadersbefore merging- Preventing override of security-sensitive headers like
Authorizationunless explicitly intended♻️ Suggested validation
internal async Task<ResponseWrapper<TRes>> SendRequestWithWrapperAsync<TReq, TRes>( RequestBuilder<TReq> requestBuilder, string apiName, IDictionary<string, string>? additionalHeaders = null, CancellationToken cancellationToken = default) { var sw = Stopwatch.StartNew(); var authToken = await GetAuthenticationTokenAsync(apiName); var mergedHeaders = BuildHeaders(_configuration, authToken, null); + // Validate additional headers + Configuration.Configuration.ValidateHeaders(additionalHeaders, "additionalHeaders"); + // Merge additional headers (from ApiExecutor) with auth headers if (additionalHeaders != null) { foreach (var header in additionalHeaders) { + // Optionally protect Authorization header from being overwritten + // if (string.Equals(header.Key, "Authorization", StringComparison.OrdinalIgnoreCase)) continue; mergedHeaders[header.Key] = header.Value; } }src/OpenFga.Sdk/Client/Client.cs (1)
61-66: Minor: Redundant null-conditional operator.On line 63,
_apiExecutor.Value?.Dispose()uses a null-conditional operator, butLazy<T>.Valuewill never be null whenIsValueCreatedis true. This is harmless but slightly misleading.🔧 Optional simplification
public void Dispose() { if (_apiExecutor.IsValueCreated) { - _apiExecutor.Value?.Dispose(); + _apiExecutor.Value.Dispose(); } api.Dispose(); }example/ApiExecutorExample/Makefile (1)
17-19: Fixed sleep duration may cause flaky behavior.A fixed 3-second sleep may not be enough on slower systems or may be unnecessarily long on fast systems. Consider a retry loop with the health check instead.
🔧 Suggested improvement with retry loop
start-openfga: `@echo` "Starting OpenFGA server on localhost:8080..." `@docker` run -d --name openfga-example -p 8080:8080 openfga/openfga:latest run `@echo` "Waiting for OpenFGA to be ready..." - `@sleep` 3 - `@curl` -s http://localhost:8080/healthz || (echo "OpenFGA failed to start" && exit 1) + `@for` i in 1 2 3 4 5 6 7 8 9 10; do \ + curl -s http://localhost:8080/healthz && break || sleep 1; \ + done || (echo "OpenFGA failed to start within 10 seconds" && exit 1) `@echo` "✅ OpenFGA is ready!"src/OpenFga.Sdk/Client/ApiExecutor/ApiExecutor.cs (1)
83-115: Non-genericSendAsyncwill fail if response is not valid JSON.The use of
JsonElementas an intermediate type means deserialization is attempted, which will throw if the response body is not valid JSON (e.g., plain text error messages or empty responses). If the goal is truly "raw" response handling, consider catching deserialization errors or using a different approach that doesn't require JSON parsing.src/OpenFga.Sdk.Test/Client/ApiExecutor/ApiExecutorTests.cs (1)
298-327: Consider using mocked HttpClient for consistency.These tests create
OpenFgaClientwithout a mockedHttpClient. While they work correctly (the null request test throws before network access, and the singleton test doesn't make requests), using mocked clients would be more consistent with the other tests and prevent any accidental network calls if the implementation changes.
There was a problem hiding this comment.
This SDK already includes an early version of the API executor called RequestBuilder + SendRequestAsync that handles the retry logic, auth and standardizes the interface, e.g.
var requestBuilder = new RequestBuilder<BatchCheckRequest> {
Method = new HttpMethod("POST"),
BasePath = _configuration.BasePath,
PathTemplate = "/stores/{store_id}/batch-check",
PathParameters = pathParams,
Body = body,
QueryParameters = queryParams,
};
return await _apiClient.SendRequestAsync<BatchCheckRequest, BatchCheckResponse>(requestBuilder,
"BatchCheck", options, cancellationToken);
What the API Executor should be doing is either building on top of that work fully or replacing it. Currently, it is building on top of RequestBuilder, but not fully.
It is duplicating the logic by introducing ApiExecutorRequestBuilder, that is separate from RequestBuilder, duplicating some of the work.
It should just be an improved iteration/abstraction on top of RequestBuilder + SendRequestAsync.
Basically what I expect to see is:
- OpenFgaApi using ApiExecutor for all API interactions instead of directly calling RequestBuilder (if it offers a material improvement), otherwise enhancing RequestBuilder to include what is needed.
- The ApiExecutor should be part of the ApiClient, rather than Client. The difference is that ApiClient is the core building block that both OpenFgaApi and Client share and the executor belong there alongside/replacing RequestBuilder
- I would expect it to have a streaming variant that works for StreamedListObjects too, but I'm OK with that being a separate PR
|
@rhamzeh
|
2 participants
Generated from SDK generator PR : openfga/sdk-generator#676
Description
What problem is being solved?
How is it being solved?
What changes are made to solve it?
References
Review Checklist
mainSummary by CodeRabbit
New Features
Documentation
Tests