feat: add APIExecutor for raw HTTP calls to OpenFGA endpoints #258
Conversation
Codecov Report❌ Patch coverage is
❌ Your project status has failed because the head coverage (33.28%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #258 +/- ##
==========================================
- Coverage 33.82% 33.28% -0.54%
==========================================
Files 114 115 +1
Lines 10990 9735 -1255
==========================================
- Hits 3717 3240 -477
+ Misses 6911 6235 -676
+ Partials 362 260 -102 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
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. WalkthroughA new APIExecutor feature enables arbitrary OpenFGA API endpoint calls via a builder pattern and fluent interface. The executor integrates with existing client retry logic, telemetry, and error handling. Updates include documentation, examples, core client methods, validation utilities, and comprehensive tests. Changes
Sequence DiagramsequenceDiagram
participant Client as APIClient
participant Executor as APIExecutor
participant HTTPClient as HTTP Client
participant Retry as Retry/Backoff
participant Telemetry as Telemetry
Client->>Executor: GetAPIExecutor() / NewAPIExecutor()
Note over Client,Executor: Builder Pattern
Client->>Executor: NewAPIExecutorRequestBuilder()
Executor->>Executor: WithPathParameter/Query/Header/Body()
Executor->>Executor: Build() → APIExecutorRequest
Note over Client,Executor: Execution Flow
Client->>Executor: Execute(ctx, request) or ExecuteWithDecode()
Executor->>Executor: validateRequest()
Executor->>Executor: buildPath(with URL-escaped params)
Executor->>Executor: prepareHeaders(defaults + overrides)
rect rgba(100, 149, 237, 0.2)
Note over Executor,Retry: Retry Loop
loop Attempt ≤ maxRetries
Executor->>HTTPClient: Execute HTTP Request
HTTPClient-->>Executor: Response (status, body, headers)
Executor->>Executor: Read & Preserve Response Body
alt Status < 300
Executor->>Executor: Optional: decode(body → result)
Executor->>Telemetry: recordTelemetry(success, duration)
Executor-->>Client: *APIExecutorResponse, nil
else Status ≥ 300
Executor->>Executor: handleError(response)
Executor->>Retry: determineRetry(error, attempt)
alt Retry Eligible & Attempts Left
Executor->>Retry: Calculate Backoff
Retry-->>Executor: Wait Duration
else No Retry
Executor-->>Client: *APIExecutorResponse, error
end
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Areas requiring extra attention:
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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
rhamzeh
force-pushed
the
poc/expose-api-endpoint
branch
3 times, most recently
from
f6568ee to
efd50fd
Compare
rhamzeh
changed the title
rhamzeh
force-pushed
the
poc/expose-api-endpoint
branch
from
efd50fd to
17db863
Compare
rhamzeh
force-pushed
the
poc/expose-api-endpoint
branch
from
17db863 to
8eb39ec
Compare
rhamzeh
force-pushed
the
poc/expose-api-endpoint
branch
from
8eb39ec to
bce1207
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds a generic APIExecutor capability to the OpenFGA Go SDK, allowing users to make raw HTTP calls to any OpenFGA endpoint while still benefiting from the SDK's configuration, authentication, retry logic, telemetry, and error handling.
Key Changes
- Introduces
APIExecutorinterface withExecuteandExecuteWithDecodemethods for making raw API calls - Refactors existing endpoint methods (Check, Write, ListStores, etc.) to internally use the new APIExecutor, significantly reducing code duplication
- Adds comprehensive test coverage for the new functionality with 19 test functions covering various scenarios
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| api_executor.go | Core implementation of APIExecutor interface, request builder pattern, and execution logic with retry/telemetry |
| api_executor_test.go | Comprehensive test suite with 1252 lines covering request building, validation, path handling, headers, and edge cases |
| api_client.go | Adds GetAPIExecutor() method to expose the executor functionality |
| api_open_fga.go | Refactors 17 endpoint methods to use APIExecutor, reducing ~2400 lines of duplicated code |
| utils.go | Adds validation helper functions for path and body parameters |
| utils_test.go | Tests for the new validation functions |
| example/example1/example1.go | Demonstrates usage of custom executor with both decoded and raw response handling |
| README.md | Documents the new feature with usage examples and use cases |
| CHANGELOG.md | Records the new feature addition |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
🧹 Nitpick comments (4)
README.md (1)
49-49: APIExecutor docs look solid; fix minor markdownlint and style nitsThe new “Calling Other Endpoints” section clearly explains APIExecutor usage and matches the new API (GetAPIExecutor, NewAPIExecutorRequestBuilder, Execute/ExecuteWithDecode). A couple of small cleanups:
- Lines 1126–1129 use hard tabs in the code block (
WithQueryParameter/WithHeaderlines), which markdownlint flags (MD010). Replacing them with spaces will keep linters happy.- The sentence on Line 1099 (“while still taking into account the configuration and handling authentication, telemetry, retry and error handling”) is slightly wordy; consider tightening to something like “while still honoring the client configuration (authentication, telemetry, retries, and error handling).”
These are purely documentation/lint polish; the technical content looks correct.
Also applies to: 1096-1178
utils.go (1)
343-356: Validation helpers are fine; consider handling typed nil pointers in futureThe new
validatePathParameter/validateParameterhelpers centralize required-field checks and match the test expectations (only empty strings are rejected for path params; any non‑nil value passesvalidateParameter).If you plan to use
validateParameterwith pointer-typed arguments (e.g., a*MyTypethat may be nil), note thatvalue == nilwon’t catch a typed nil pointer stored in an interface. If you later need that behavior, you might extend it with a smallreflectcheck to treat(*T)(nil)as invalid too. Not required for current usages, but worth keeping in mind.api_executor.go (1)
197-219: Consider validating unresolved path placeholders for clearer errors
validateRequestonly checks thatOperationName,Method, andPathare non‑empty, andbuildPathblindly replaces any{key}occurrences it finds. If the caller forgets to pass a required path parameter (e.g.{store_id}), the request will go out with a literal{store_id}segment and fail server‑side, which is harder to diagnose.As a usability improvement, you could:
- After calling
buildPath, detect remaining{...}placeholders (e.g.strings.Contains(path, "{")) and return areportErrorindicating which parameter is missing; or- More generically, parse all
{name}tokens from the template and ensure each has a non‑empty entry inPathParameters(possibly usingvalidatePathParameter).Not required for correctness, but would make the executor’s errors much more actionable.
example/example1/example1.go (1)
290-290: Minor: Inconsistent comment indentation.The comment has a leading tab while other comments use consistent indentation. This is a minor formatting inconsistency.
Apply this diff:
- // custom executor + decoded response + // custom executor + decoded response
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CHANGELOG.md(1 hunks)README.md(2 hunks)api_client.go(1 hunks)api_executor.go(1 hunks)api_executor_test.go(1 hunks)example/example1/example1.go(4 hunks)utils.go(1 hunks)utils_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
utils_test.go (1)
utils.go (1)
ToPtr(10-10)
api_executor_test.go (3)
api_client.go (2)
APIClient(40-47)NewAPIClient(56-87)internal/constants/constants.go (1)
TestApiUrl(26-26)api_executor.go (5)
APIExecutorRequest(20-48)APIExecutorRequestBuilder(51-53)NewAPIExecutorRequestBuilder(59-70)NewAPIExecutor(252-254)APIExecutorResponse(136-148)
api_client.go (1)
api_executor.go (2)
APIExecutor(151-195)NewAPIExecutor(252-254)
example/example1/example1.go (3)
client/client.go (2)
ClientCheckRequest(2040-2046)ClientCheckOptions(2048-2054)api_executor.go (1)
NewAPIExecutorRequestBuilder(59-70)model_check_response.go (1)
CheckResponse(22-26)
api_executor.go (5)
api_client.go (1)
APIClient(40-47)telemetry/telemetry.go (4)
Get(65-85)GetMetrics(107-109)TelemetryFactoryParameters(21-23)Telemetry(16-19)errors.go (3)
GenericOpenAPIError(13-17)FgaApiRateLimitExceededError(632-653)FgaApiInternalError(485-503)internal/constants/constants.go (2)
DefaultMaxRetry(40-40)DefaultMinWaitInMs(43-43)internal/utils/retryutils/retryutils.go (1)
GetTimeToWait(89-107)
🪛 LanguageTool
README.md
[style] ~1099-~1099: ‘taking into account’ might be wordy. Consider a shorter alternative.
Context: ...ameters, body, and headers, while still taking into account the configuration and handling authenti...
(EN_WORDINESS_PREMIUM_TAKING_INTO_ACCOUNT)
🪛 markdownlint-cli2 (0.18.1)
README.md
1126-1126: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1127-1127: Hard tabs
Column: 1
(MD010, no-hard-tabs)
1129-1129: Hard tabs
Column: 1
(MD010, no-hard-tabs)
⏰ 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 (go)
🔇 Additional comments (16)
CHANGELOG.md (1)
3-10: Changelog entries correctly reflect the new featuresThe Unreleased section accurately documents the new
StreamedListObjectssupport and the genericfgaClient.GetAPIExecutor()feature, and the Makefile change is appropriately labeled as a chore. No issues from a release-notes perspective.api_client.go (1)
217-221: LGTM! Clean integration point for the API executor.The method provides a clear, well-documented entry point to the new APIExecutor functionality, enabling users to call arbitrary OpenFGA endpoints while reusing the SDK's authentication and configuration.
example/example1/example1.go (6)
5-5: LGTM! Required imports for the API executor demonstration.The new imports support manual JSON unmarshaling and HTTP method constants used in the custom executor examples below.
Also applies to: 7-7
63-64: LGTM! Good refactoring to enable store ID reuse.Extracting the store ID into a variable makes it available for the API executor demonstration later in the example.
246-246: LGTM! Standard error formatting.Using
%switherr.Error()is appropriate for string formatting.
253-259: LGTM! Good refactoring to eliminate duplication.Extracting the check request into a variable enables reuse across multiple check operations (lines 259, 266, and 286).
282-296: LGTM! Clear demonstration of ExecuteWithDecode.This example effectively demonstrates the API executor's
ExecuteWithDecodemethod, showing how to build a custom request and decode the response directly into a typed structure.
298-309: LGTM! Demonstrates raw response handling.This example effectively shows the alternative Execute method for cases where manual unmarshaling is preferred, complementing the ExecuteWithDecode example above.
api_executor_test.go (8)
18-48: LGTM! Clean and reusable test helpers.The test infrastructure is well-designed with clear separation of concerns. The
testRoundTripper,makeResp, andnewTestClienthelpers provide a solid foundation for testing the API executor functionality.
52-152: LGTM! Comprehensive validation testing.The test cases thoroughly cover all combinations of required fields (operationName, method, path) including valid requests, missing fields, and empty string values. The table-driven approach with explicit error message verification is excellent.
154-251: LGTM! Thorough path building tests.Excellent coverage of path template substitution including special characters, URL encoding, Unicode, nil/empty parameters, and multiple placeholder scenarios. The expected results correctly show URL encoding behavior.
253-355: LGTM! Comprehensive header preparation tests.The tests cover default headers, custom additions, overrides, and importantly case-sensitivity behavior. This ensures the header preparation logic handles all practical scenarios correctly.
431-595: LGTM! Thorough builder pattern testing.These tests comprehensively verify the builder's behavior including nil map initialization, multiple query values, parameter overwrites, and various body types (string, struct, map, slice, nil). The coverage ensures robust builder functionality.
597-630: LGTM! Executor creation and basic execution verified.The tests confirm that
NewAPIExecutorcreates a valid executor instance and that basic request execution works correctly with the test infrastructure.
632-782: LGTM! Retry behavior thoroughly tested.The tests verify retry parameter retrieval and retry determination logic across various error scenarios, attempt numbers, and retry configurations. The validation of wait durations ensures proper backoff behavior.
784-1252: LGTM! Comprehensive edge case coverage.The remaining tests provide excellent coverage of edge cases for path building, header preparation, response fields, builder chaining, overrides, and validation. The test suite is thorough and well-structured with proper use of table-driven tests and parallel execution.
rhamzeh
force-pushed
the
poc/expose-api-endpoint
branch
from
85ae031 to
cdd9bf3
Compare
rhamzeh
force-pushed
the
poc/expose-api-endpoint
branch
from
cdd9bf3 to
fa58c97
Compare
Description
This adds the capability to call any FGA endpoint.
What problem is being solved?
How is it being solved?
What changes are made to solve it?
References
closes #255
generated from openfga/sdk-generator#660
Review Checklist
mainSummary by CodeRabbit
New Features
Documentation
Examples
✏️ Tip: You can customize this high-level summary in your review settings.