feat: support per-request headers options #233
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 WalkthroughAdds per-request custom HTTP headers support via a new RequestOptions type and Options methods across request builders. Merges per-request headers with default headers without overriding existing values. Updates client propagation, documentation, examples, and adds comprehensive tests. Also adjusts prepareRequest to apply default headers only when not already set. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Caller
participant C as High-level Client
participant SDK as SDK API (api_open_fga.go)
participant HTTP as HTTP Layer (api_client.go)
participant S as OpenFGA Server
Dev->>C: Invoke Operation(options.Headers, default headers configured)
Note over C: Build request body<br/>Assemble RequestOptions (embedded)
C->>SDK: api.Xxx(ctx).Options(RequestOptions).Execute(...)
Note over SDK: Merge per-request headers into local header map
SDK->>HTTP: prepareRequest(req, headerParams)
Note over HTTP: Apply cfg.DefaultHeaders only if header not already set
HTTP->>S: HTTP request with final headers
S-->>HTTP: Response
HTTP-->>SDK: Response
SDK-->>C: Result
C-->>Dev: Result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ 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 |
Codecov Report❌ Patch coverage is
❌ Your project status has failed because the head coverage (33.81%) 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 #233 +/- ##
==========================================
+ Coverage 28.29% 33.81% +5.52%
==========================================
Files 111 111
Lines 12000 12223 +223
==========================================
+ Hits 3395 4133 +738
+ Misses 8325 7740 -585
- Partials 280 350 +70 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
client/client.go (4)
1032-1047: WriteAuthorizationModelExecute can nil-deref BodyAdd a nil check before dereferencing.
func (client *OpenFgaClient) WriteAuthorizationModelExecute(request SdkClientWriteAuthorizationModelRequestInterface) (*ClientWriteAuthorizationModelResponse, error) { storeId, err := client.getStoreId(request.GetStoreIdOverride()) if err != nil { return nil, err } + if request.GetBody() == nil { + return nil, FgaRequiredParamError{param: "body"} + } requestOptions := RequestOptions{} if request.GetOptions() != nil { requestOptions = request.GetOptions().RequestOptions }
1631-1641: WriteExecute: add nil Body guard to avoid panicMultiple dereferences of
request.GetBody()assume it’s set. Guard early.func (client *OpenFgaClient) WriteExecute(request SdkClientWriteRequestInterface) (*ClientWriteResponse, error) { options := request.GetOptions() + if request.GetBody() == nil { + return nil, FgaRequiredParamError{param: "body"} + } transactionOptionsSet := options != nil && options.Transaction != nil
1774-1778: Bug: wrong chunk-size variable used for deletes
writeChunkSizeis used when slicing deletes. UsedeleteChunkSize; otherwise future divergence between the two sizes will slice incorrectly.- end := int(math.Min(float64(i+writeChunkSize), float64(len(request.GetBody().Deletes)))) + end := int(math.Min(float64(i+deleteChunkSize), float64(len(request.GetBody().Deletes))))
2978-2986: Compile-time issue: taking address of field on a temporary
&fgaSdk.NewContextualTupleKeys(...).TupleKeysis not addressable. Bind to a variable first and take its address.- body := fgaSdk.ListUsersRequest{ + tupleKeys := fgaSdk.NewContextualTupleKeys(contextualTuples).TupleKeys + body := fgaSdk.ListUsersRequest{ Object: request.GetBody().Object, Relation: request.GetBody().Relation, UserFilters: request.GetBody().UserFilters, - ContextualTuples: &fgaSdk.NewContextualTupleKeys(contextualTuples).TupleKeys, + ContextualTuples: &tupleKeys, Context: request.GetBody().Context, AuthorizationModelId: authorizationModelId, }
🧹 Nitpick comments (9)
CHANGELOG.md (1)
5-5: Clarify scope: headers shipped; per-request retry options not wiredEntry reads well. If per-request “connection options” (MaxRetry/MinWaitInMs) won’t ship in this release, call that out (or drop mention elsewhere) to avoid implying they’re active.
README.md (1)
220-259: Docs compile/use-style nits
- Keep import style consistent. Other sections use a dot import; this snippet uses the
client.alias. Prefer one style across the README.- Consider noting that User-Agent isn’t overridden by per-request headers (current code sets it unconditionally), to avoid confusion.
Example (alias style):
import ( "context" client "github.com/openfga/go-sdk/client" ) fgaClient, err := client.NewSdkClient(&client.ClientConfiguration{ /* ... */ })api_open_fga.go (1)
1045-1047: Factor header application to reduce duplication (optional)Consider a small helper to apply
RequestOptions.Headersto amap[string]stringto cut repetition and future drift.example/example1/example1.go (2)
239-241: Fix format string: don’t use %w with a string
%wis for wrapping errors and should be used with an error, noterr.Error(). Use%vand passerr.- fmt.Printf("Failed due to: %w\n", err.Error()) + fmt.Printf("Failed due to: %v\n", err)
87-87: Incorrect Println formatting
Printlnignores formatting verbs. UsePrintfand a proper verb.- fmt.Println("Current Store Name: %v\n" + currentStore.Name) + fmt.Printf("Current Store Name: %s\n", currentStore.Name)api_headers_test.go (2)
25-26: Gitleaks false positive: annotate the test ULIDThe constant resembles a key to scanners. Add an inline allow annotation to quiet false positives in CI.
- apiTestStoreId = "01H0H015178Y2V4CX10C2KGHF4" + apiTestStoreId = "01H0H015178Y2V4CX10C2KGHF4" // gitleaks:allow - test ULID, not a secret
272-315: Optional: make the BatchCheck stub match the real shapeConsider returning
{"result": {"test-correlation-id": {"allowed": true}}}to mirror the API more closely, reducing future drift.client/client_headers_test.go (1)
118-156: Great coverage of per-request vs default headers across client methodsThorough checks for overrides, absence, and empty headers. Consider adding one test that sets
Transaction.Disable = truewith smallMaxPerChunkto ensure headers propagate across chunked writes.Also applies to: 157-186, 187-218, 219-246, 248-322, 324-364, 366-405, 407-447, 449-494, 496-540, 542-579, 580-629, 631-770, 772-810, 812-892
client/client.go (1)
1446-1453: Minor DRY opportunity: extract helper to pull RequestOptionsMany methods repeat the same pattern to extract
RequestOptionsand set consistency. Consider a small helper to reduce duplication.Also applies to: 1472-1476, 2558-2594, 2686-2723, 2963-3002, 3069-3095, 3204-3238
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.openapi-generator/FILES(1 hunks)CHANGELOG.md(1 hunks)README.md(1 hunks)api_client.go(1 hunks)api_headers_test.go(1 hunks)api_open_fga.go(38 hunks)client/client.go(49 hunks)client/client_headers_test.go(1 hunks)example/example1/example1.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
api_headers_test.go (5)
api_client.go (2)
APIClient(52-59)NewAPIClient(68-99)configuration.go (1)
NewConfiguration(55-102)api_open_fga.go (2)
RequestOptions(35-39)OpenFgaApi(41-899)client/client.go (1)
RequestOptions(121-121)utils.go (2)
PtrInt(25-25)PtrString(40-40)
client/client_headers_test.go (3)
client/client.go (30)
OpenFgaClient(75-79)ClientConfiguration(40-58)NewSdkClient(81-119)ClientCheckOptions(1988-1994)RequestOptions(121-121)ClientCheckRequest(1980-1986)ClientWriteRequest(1504-1507)ClientTupleKey(132-132)ClientWriteOptions(1518-1524)ClientReadRequest(1387-1391)ClientReadOptions(1393-1400)ClientExpandRequest(2494-2498)ClientExpandOptions(2500-2506)ClientListObjectsRequest(2621-2627)ClientListObjectsOptions(2629-2635)ClientListUsersRequest(2897-2904)ClientListUsersOptions(2906-2912)ClientBatchCheckClientOptions(2116-2123)ClientReadAuthorizationModelOptions(1085-1090)ClientWriteAuthorizationModelOptions(991-995)ClientListStoresOptions(579-585)ClientCreateStoreRequest(655-657)ClientCreateStoreOptions(659-661)ClientGetStoreOptions(736-740)ClientDeleteStoreOptions(813-817)ClientReadChangesRequest(1272-1275)ClientReadChangesOptions(1277-1283)ClientReadAssertionsOptions(3022-3027)ClientAssertion(3118-3125)ClientWriteAssertionsOptions(3147-3152)api_open_fga.go (1)
RequestOptions(35-39)utils.go (2)
PtrInt(25-25)PtrString(40-40)
example/example1/example1.go (2)
client/client.go (3)
ClientCheckRequest(1980-1986)ClientCheckOptions(1988-1994)RequestOptions(121-121)api_open_fga.go (1)
RequestOptions(35-39)
api_open_fga.go (1)
client/client.go (1)
RequestOptions(121-121)
client/client.go (4)
api_open_fga.go (1)
RequestOptions(35-39)model_create_store_request.go (1)
CreateStoreRequest(22-24)model_consistency_preference.go (1)
ConsistencyPreference(21-21)client/errors.go (1)
FgaRequiredParamError(16-19)
🪛 Gitleaks (8.27.2)
api_headers_test.go
[high] 25-25: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (16)
.openapi-generator/FILES (1)
16-16: Tests listed — LGTMThe manifest includes the new header-behavior tests. No action needed.
Also applies to: 20-20
api_open_fga.go (2)
917-921: Fluent Options builder — LGTMThe
Options(RequestOptions)builder reads well and keeps call sites clean. Applies consistently across endpoints.Also applies to: 1164-1167, 1494-1497, 1672-1675, 1848-1851, 2211-2214, 2398-2401, 2608-2611, 2989-3004, 3299-3302, 3488-3491, 3730-3733, 3979-3982, 4183-4186, 4418-4421, 4606-4609
1001-1149: Per-request retries: ensure resolved RetryParams are used for telemetry and backoffrg shows all retry backoff calls use retryParams := a.client.cfg.RetryParams (e.g. GetTimeToWait at api_open_fga.go:1059, 1076; BuildTelemetryAttributes at 1124). Confirm retryParams is the resolved/merged value (client defaults + any r.options per-request overrides) before calling retryutils.GetTimeToWait and before passing retry-related attrs into telemetry; if it isn’t, merge/apply per-request overrides and use that merged value.
api_client.go (1)
287-289: Allow per-request User-Agent to override defaultReplace the unconditional set at api_client.go:284 with a conditional set so per-request User-Agent headers are preserved:
if localVarRequest.Header.Get("User-Agent") == "" && c.cfg.UserAgent != "" { localVarRequest.Header.Set("User-Agent", c.cfg.UserAgent) }
Note: telemetry/attributes_test.go:14 sets User-Agent — run tests after this change.example/example1/example1.go (1)
257-274: Good example of per-request headers usageThe example correctly demonstrates passing RequestOptions with custom headers to a single call.
api_headers_test.go (3)
32-48: Test server/header capture is solidHelper neatly captures and exposes headers for assertions; JSON stubs and OK responses keep tests focused.
67-114: RequestOptions struct coverage looks goodCovers headers, MaxRetry, and MinWaitInMs, including nil vs empty map cases.
116-166: Header override/merge behavior tests are comprehensiveBroad surface coverage and clear expectations for overrides vs defaults. Nicely done.
Also applies to: 169-270, 272-315, 317-359, 361-399, 401-438, 440-476, 478-519, 521-552, 553-586, 587-617, 618-649, 650-681, 683-728, 730-762, 764-838
client/client_headers_test.go (1)
34-49: Helpers are clean and reusableServer/client helpers are minimal and effective; good separation for repeated use.
Also applies to: 51-67
client/client.go (7)
121-121: Public alias for RequestOptions is the right callKeeps the public surface consistent with the core SDK type.
127-130: Consistent options embeddingEmbedding RequestOptions in wrapper structs keeps a uniform API.
Also applies to: 156-163
579-585: Option structs updated to embed RequestOptions — LGTMUniform embedding across the surface improves discoverability and composition.
Also applies to: 659-661, 736-740, 813-817, 892-899, 991-996, 1085-1091, 1186-1190, 1277-1284, 1393-1400, 1518-1524, 2116-2123, 2500-2506, 2629-2636, 2755-2762, 2906-2912, 3022-3027, 3147-3152
606-611: Correct propagation of RequestOptions into underlying API callsPassing RequestOptions to each underlying request ensures headers/timeouts/etc. flow per-call. Well applied.
Also applies to: 701-705, 779-783, 857-859, 943-946, 1043-1047, 1152-1155, 1229-1231, 1341-1344, 1472-1476, 1674-1678, 1750-1751, 1795-1796, 2589-2593, 2714-2718, 2850-2851, 2993-2997, 3087-3090, 3227-3231
2049-2051: Good addition: nil Body check in CheckExecutePrevents panic and returns a clear error to the caller.
2182-2189: ClientBatchCheck: per-request options and limits handled correctlyProperly threads RequestOptions and concurrency parameters, and reuses Check options for each item.
Also applies to: 2205-2210, 2383-2387
2850-2853: ListRelationsExecute: options plumbing looks correctForwarding RequestOptions, consistency, and max parallel requests is consistent with the rest.
jimmyjames
left a comment
jimmyjames
left a comment
There was a problem hiding this comment.
Couple questions around a potential breaking change and the intent of adding retry options to the new RequestOptions along with the headers support.
rhamzeh
force-pushed
the
feat/per-request-headers
branch
from
ceed0bd to
d940d26
Compare
rhamzeh
force-pushed
the
feat/per-request-headers
branch
from
d940d26 to
c8ebc36
Compare
feat: support per-request headers and connection options (#233)
rhamzeh
changed the title
Description
This PR adds support for sending custom headers per request.
It is complete, however it's kept in draft as it will have many conflicts with the other Go SDK PRs that will need to be resolved once the others are merged.
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
Bug Fixes
Documentation
Tests
Examples