feat: add support for Write API idempotence with on_duplicate and on_missing options
#256
Conversation
…on_missing` options Signed-off-by: zitro <[email protected]>
|
WalkthroughDocumentation updates, API surface extensions, and client enhancements: added an optional name filter to listStores across API/client layers; introduced idempotence flags for write requests via new enums; adjusted batchCheck return types; updated docs and CHANGELOG date; expanded tests and nocks to support query parameters. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Application
participant Client as FGA Client
participant API as OpenFGA API Layer
participant Server as OpenFGA Server
rect rgba(220,235,255,0.3)
note over App,API: listStores with optional name filter
App->>Client: listStores({ pageSize?, continuationToken?, name? })
Client->>API: listStores(pageSize?, continuationToken?, name?)
API->>Server: GET /stores?{page_size,continuation_token,name}
Server-->>API: 200 ListStoresResponse
API-->>Client: CallResult<ListStoresResponse>
Client-->>App: ListStoresResponse
end
sequenceDiagram
autonumber
participant App as Application
participant Client as FGA Client
participant API as OpenFGA API Layer
participant Server as OpenFGA Server
rect rgba(220,255,220,0.3)
note over App,API: Write with idempotence flags
App->>Client: write({writes[], deletes[]}, {idempotence: {on_duplicate?, on_missing?}})
Client->>Client: Map client enums → API enums
Client->>API: write({writes{on_duplicate?}, deletes{on_missing?}})
API->>Server: POST /stores/{id}/write
alt duplicate/missing per flags
Server-->>API: 200 OK (ignored per flag)
else error behavior
Server-->>API: 4xx Error
end
API-->>Client: CallResult<WriteResponse|Error>
Client-->>App: Response
end
sequenceDiagram
autonumber
participant App as Application
participant API as OpenFGA API Layer
participant Server as OpenFGA Server
rect rgba(255,235,220,0.3)
note over App,API: BatchCheck public API return type
App->>API: batchCheck(storeId, body)
API->>Server: POST /stores/{id}/batch-check
Server-->>API: 200 BatchCheckResponse
API-->>App: CallResult<BatchCheckResponse>
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
471-474: Fix typo: “correliationId” → “correlationId”.User-facing docs should be typo-free.
Apply:
- // optional correliationId to associate request and response. One will be generated + // optional correlationId to associate request and response. One will be generated
🧹 Nitpick comments (8)
CHANGELOG.md (1)
4-6: Add Unreleased notes for new features introduced in this PR.Please document the new Write API idempotence flags and ListStores name filter.
Proposed diff:
## [Unreleased](https://github.com/openfga/js-sdk/compare/v0.9.0...HEAD) + +- feat: add Write API idempotence options (`on_duplicate`, `on_missing`) +- feat: ListStores supports optional `name` filter +- docs: clarify BatchCheck result ordering and using `correlationId`README.md (1)
452-453: Clarify correlationId constraints in the note.Mention that correlation IDs must be unique per request and follow format limits (≤36 chars; letters, numbers, hyphens).
Proposed tweak:
-> **Note**: The order of `batchCheck` results is not guaranteed to match the order of the checks provided. Use `correlationId` to pair responses with requests. +> **Note**: The order of `batchCheck` results is not guaranteed to match the order of the checks provided. Use a unique `correlationId` (≤36 chars, letters/numbers/hyphens) to pair responses with requests.tests/helpers/nocks.ts (1)
70-76: Match query params as a subset to reduce test brittleness.Exact object matching can break if additional params are sent later. Prefer subset matching.
Apply:
- if (queryParams) { - mock.query(queryParams); - } + if (queryParams) { + mock.query(actual => Object.entries(queryParams!).every(([k, v]) => actual[k] === v)); + }client.ts (1)
335-336: listStores: plumbsnamefilter correctly.Consider updating README “List Stores” section to show the
nameoption.Also applies to: 343-344
api.ts (4)
129-136: Fix malformed JSON in BatchCheck examples.The examples are missing a comma after the
objectfield, which breaks copy/paste. Apply across all repeated docblocks in this file.- "object": "document:2021-budget" "relation": "reader", + "object": "document:2021-budget", "relation": "reader",Also applies to: 810-817, 1086-1093, 1286-1294
391-395: Polish the new listStoresnameparameter docs and ensure consistent threading.LGTM on wiring
namethrough all surfaces. Please fix a punctuation nit in the doc text.- * @param {string} [name] The name parameter instructs the API to only include results that match that name.Multiple results may be returned. Only exact matches will be returned; substring matches and regexes will not be evaluated + * @param {string} [name] The name parameter instructs the API to only include results that match that name. Multiple results may be returned. Only exact matches will be returned; substring matches and regexes will not be evaluated.Also applies to: 920-925, 1164-1170, 1378-1385
395-405: Handle empty-stringnamedefensively (optional).If
name === "", we currently send?name=. If the API treats empty as invalid, consider omitting it.- if (name !== undefined) { + if (name !== undefined && name !== "") { localVarQueryParameter["name"] = name; }Also applies to: 417-419
680-687: Great: Write API idempotence is documented; minor copy edits.Solid explanation and examples. Two tiny edits improve clarity.
- determine whether a relationship exists between an object and an user. + determine whether a relationship exists between an object and a user.- the most restrictive action (error) will take precedence. + the most restrictive action ("error") will take precedence.Also applies to: 1027-1034, 1242-1249, 1470-1478
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
CHANGELOG.md(1 hunks)README.md(1 hunks)api.ts(17 hunks)apiModel.ts(2 hunks)client.ts(5 hunks)tests/client.test.ts(1 hunks)tests/helpers/nocks.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/client.test.ts (2)
example/example1/example1.mjs (11)
store(31-31)fgaClient(18-23)fgaClient(38-38)fgaClient(46-46)fgaClient(50-50)fgaClient(155-155)fgaClient(159-159)fgaClient(164-168)fgaClient(179-186)fgaClient(191-210)fgaClient(236-236)tests/helpers/default-config.ts (1)
defaultConfiguration(69-69)
client.ts (2)
common.ts (1)
PromiseResult(129-129)apiModel.ts (1)
ListStoresResponse(868-881)
api.ts (3)
base.ts (1)
RequestArgs(29-32)common.ts (3)
RequestArgs(43-46)PromiseResult(129-129)CallResult(125-127)apiModel.ts (1)
ListStoresResponse(868-881)
🔇 Additional comments (11)
CHANGELOG.md (1)
8-8: Date bump looks fine.No issues with updating the v0.9.0 date.
tests/client.test.ts (1)
114-132: LGTM: validates ListStores name filter is forwarded.Nice coverage and assertion via nock scope.
apiModel.ts (4)
1923-1929: LGTM: adds deletes.on_missing option with clear semantics.
1931-1939: LGTM: enum for deletes.on_missing.
1952-1958: LGTM: adds writes.on_duplicate option with clear semantics.
1960-1968: LGTM: enum for writes.on_duplicate.client.ts (4)
19-21: Type-only imports are correct here.Safe since they’re used only for typing in casts.
132-132: PaginationOptions: addingnamelooks good.Keeps backward compatibility; used only by listStores.
207-216: Public idempotence options: good API design; add tests to assert body mapping.Please add tests that verify
writes.on_duplicateanddeletes.on_missingare included in the POST body for both transaction mode and chunked mode.Proposed test additions:
- Update helpers/nocks.write to validate request body includes
writes.on_duplicate/deletes.on_missingwhen provided.- Add two tests: transaction (single call) and non-transaction (chunked) paths asserting those flags are sent.
522-524: Idempotence mapping to API body looks correct.Casts to API enums are fine; fields only set when provided.
Also applies to: 528-530
api.ts (1)
471-478: No action on unrelated READ docs.These appear to be formatting-only churn; nothing functionally tied to this PR.
Also applies to: 947-954, 1183-1190, 1401-1410
| export declare enum ClientWriteRequestWritesOnDuplicateEnum { | ||
| Error = "error", | ||
| Ignore = "ignore" | ||
| } | ||
| export declare enum ClientWriteRequestDeletesOnMissingEnum { | ||
| Error = "error", | ||
| Ignore = "ignore" | ||
| } | ||
|
|
There was a problem hiding this comment.
Do not use declare enum in .ts source — exports won’t exist at runtime.
Consumers will likely import these enums as values. Use regular exported enums so runtime values are emitted.
Apply:
-export declare enum ClientWriteRequestWritesOnDuplicateEnum {
+export enum ClientWriteRequestWritesOnDuplicateEnum {
Error = "error",
Ignore = "ignore"
}
-export declare enum ClientWriteRequestDeletesOnMissingEnum {
+export enum ClientWriteRequestDeletesOnMissingEnum {
Error = "error",
Ignore = "ignore"
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export declare enum ClientWriteRequestWritesOnDuplicateEnum { | |
| Error = "error", | |
| Ignore = "ignore" | |
| } | |
| export declare enum ClientWriteRequestDeletesOnMissingEnum { | |
| Error = "error", | |
| Ignore = "ignore" | |
| } | |
| export enum ClientWriteRequestWritesOnDuplicateEnum { | |
| Error = "error", | |
| Ignore = "ignore" | |
| } | |
| export enum ClientWriteRequestDeletesOnMissingEnum { | |
| Error = "error", | |
| Ignore = "ignore" | |
| } |
🤖 Prompt for AI Agents
In client.ts around lines 192 to 200, the enums are declared with "declare enum"
which emits no runtime values; replace those with regular exported enums (e.g.,
"export enum ClientWriteRequestWritesOnDuplicateEnum { ... }" and "export enum
ClientWriteRequestDeletesOnMissingEnum { ... }") so the enum values are emitted
into the compiled JS, update any imports/usages if needed, and run the
build/type-check to verify consumers can import the enums as runtime values.
|
Thanks @0zitro! We'll create the JS SDK issue for this soon, just getting approvals from the rest of the team around the interface. The suggested interface (see openfga/sdk-generator#610) is similar to the one you've built but might need minor tweaking. Some notes:
|
|
@rhamzeh thanks for the feedback! This was actually meant more like a draft PR with an implementation sketch. Feel free to use the commit code as you wish, it's like 6 lines total. Closing this. |
2 participants
Description
What problem is being solved?
How is it being solved?
What changes are made to solve it?
References
followup openfga/api#233
Review Checklist
mainSummary by CodeRabbit
New Features
Documentation
Tests
Chores