feat: add support for write conflict settings #276
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 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. WalkthroughThe PR adds support for write-operation conflict options (on_duplicate for duplicate writes, on_missing for missing deletes) across the SDK by introducing new API model enums, client configuration interfaces, updated method signatures, and comprehensive documentation and test coverage. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The PR spans multiple files with consistent pattern changes (new enums, interfaces, and option propagation). While the logic is relatively straightforward—adding optional conflict configuration that flows through to API calls—understanding the complete implementation requires reviewing type definitions, client method modifications, documentation, and test scenarios across the codebase. Possibly related issues
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 |
|
No dependency changes detected. Learn more about Socket for GitHub. 👍 No dependency changes detected in pull request |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
example/example1/example1.mjs (1)
149-151: Use exported enum for conflict option.Since we now surface
OnDuplicateWrites, please lean on that constant instead of the raw string so the example mirrors the public API surface.-import { CredentialsMethod, FgaApiValidationError, OpenFgaClient, TypeName } from "@openfga/sdk"; +import { CredentialsMethod, FgaApiValidationError, OnDuplicateWrites, OpenFgaClient, TypeName } from "@openfga/sdk"; @@ }, { authorizationModelId, - conflict: { onDuplicateWrites: 'ignore' } + conflict: { onDuplicateWrites: OnDuplicateWrites.Ignore } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
README.md(1 hunks)api.ts(4 hunks)apiModel.ts(2 hunks)client.ts(9 hunks)example/example1/example1.mjs(1 hunks)tests/client.test.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/client.test.ts (2)
client.ts (2)
OnDuplicateWrites(192-192)OnMissingDeletes(194-194)tests/helpers/default-config.ts (1)
baseConfig(54-67)
client.ts (1)
apiModel.ts (2)
TupleKey(1432-1457)TupleKeyWithoutCondition(1463-1482)
phamhieu
force-pushed
the
feat/add-write-conflict-options
branch
from
e23c5e3 to
a019ea2
Compare
|
Thanks for your submission @phamhieu. We will assign someone to review your PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #276 +/- ##
==========================================
+ Coverage 88.88% 88.98% +0.09%
==========================================
Files 23 23
Lines 1260 1271 +11
Branches 211 213 +2
==========================================
+ Hits 1120 1131 +11
Misses 84 84
Partials 56 56 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for write conflict handling options introduced in OpenFGA v1.10.0, enabling developers to control behavior when writing duplicate tuples or deleting non-existent tuples.
- Added
OnDuplicateWritesandOnMissingDeletesenums withErrorandIgnoreoptions - Extended
write,writeTuples, anddeleteTuplesmethods to accept conflict configuration - Updated API documentation to reflect the new conflict handling behavior
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| apiModel.ts | Added on_duplicate and on_missing fields to write/delete request interfaces with corresponding enums |
| client.ts | Extended client interfaces and methods to support conflict options with proper type safety |
| api.ts | Updated API documentation to describe conflict handling behavior and examples |
| tests/client.test.ts | Added comprehensive test coverage for conflict options across write, writeTuples, and deleteTuples methods |
| example/example1/example1.mjs | Demonstrated usage of conflict options in example code |
| README.md | Added detailed documentation section explaining conflict options usage and behavior |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
tests/client.test.ts
Outdated
| expect.objectContaining({ | ||
| writes: { | ||
| tuple_keys: [tuple], | ||
| on_duplicate: undefined, |
There was a problem hiding this comment.
This might just be how the JS generator template works but in other languages the default value for each property is being sent ("error"), I'm not sure if we should make them aligned (which would require template updates) or just accept the language differences.
There was a problem hiding this comment.
Good point! Just updated it to send "error" as the default for conflict options when they're not specified
There was a problem hiding this comment.
@phamhieu Would it be possible for you to take a look at the lockfile changes once? I think it might be related to the latest merge
https://github.com/openfga/js-sdk/actions/runs/18768778003/job/53559732838#step:4:9
The CI check is currently failing because of this
phamhieu
force-pushed
the
feat/add-write-conflict-options
branch
from
2fa3e77 to
bb062f5
Compare
Fixed! I've resolved the conflicts. |
There was a problem hiding this comment.
Thank you for the submission—this is a solid PR overall.
I left a few focused comments to avoid overwhelming the review. There are still areas to address, notably:
-
Complete test coverage of write/delete conflict combinations and defaults.
-
Error-path tests (4xx/5xx mapping), timeouts/abort, and transaction options (chunking/concurrency).
-
Naming/typing consistency (singular option names, remove “Enum” suffix, prefer final types).
Happy to re-review once these are incorporated.
apiModel.ts
Outdated
| * @export | ||
| * @enum {string} | ||
| */ | ||
| export enum WriteRequestDeletesOnMissingEnum { |
There was a problem hiding this comment.
-
Drop the “Enum” suffix from names.
-
Optional: Prefer const object + string-literal unions for better tree-shaking and DX.
export const WriteRequestDeletesOnMissing = {
Error: "error",
Ignore: "ignore",
} as const;
export type WriteRequestDeletesOnMissing =
typeof WriteRequestDeletesOnMissing[keyof typeof WriteRequestDeletesOnMissing];
export const WriteRequestWritesOnDuplicate = {
Error: "error",
Ignore: "ignore",
} as const;
export type WriteRequestWritesOnDuplicate =
typeof WriteRequestWritesOnDuplicate[keyof typeof WriteRequestWritesOnDuplicate];
client.ts
Outdated
| result: ClientBatchCheckSingleResponse[]; | ||
| } | ||
|
|
||
| export const OnDuplicateWrites = WriteRequestWritesOnDuplicateEnum; |
There was a problem hiding this comment.
-
Prefer singular option names for per-item behavior. For deletes, OnMissingDeletes reads better as OnMissingDelete. Likewise for writes, OnDuplicateWrites should be OnDuplicateWrite.
-
If you drop the enums, you don’t need extra const + typeof wrappers inside ClientWriteConflictOptions. Reuse the original const objects and their string-literal union type aliases directly to avoid duplication and keep types single-sourced.
-
Aim for consistent naming across options: onMissingDelete and onDuplicateWrite. This keeps the API readable, predictable, and aligned with common TS conventions.
| ); | ||
|
|
||
| mockWrite.mockRestore(); | ||
| }); |
There was a problem hiding this comment.
- Add explicit tests for the Error mode. Current tests cover Ignore and implicitly assume Error via defaults; verify Error behavior directly instead of relying on defaults.
| onDuplicateWrites: OnDuplicateWrites.Ignore, | ||
| onMissingDeletes: OnMissingDeletes.Error, | ||
| } | ||
| }); |
There was a problem hiding this comment.
- Add matrix tests to exercise both knobs together:
writes only: onDuplicateWrite = Error | Ignore
deletes only: onMissingDelete = Error | Ignore
mixed writes + deletes:
(Ignore, Ignore)
(Ignore, Error)
(Error, Ignore)
(Error, Error)
6 participants
Description
Adds support for write conflict options in the JavaScript SDK to handle duplicate writes and missing deletes, as introduced in OpenFGA v1.10.0.
This implementation adds
onDuplicateWritesandonMissingDeletesoptions to control behavior when:onDuplicateWrites)onMissingDeletes)References
Review Checklist
mainSummary by CodeRabbit
New Features
OnDuplicateWritesandOnMissingDeletesto control behavior when duplicates are encountered or tuples are missing during deletes.Documentation