feat(js-sdk): add support for write conflict settings #646
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 WalkthroughUpdates OpenFGA API reference hash and adds per-request conflict handling for write and delete operations in the JavaScript client template, including new enums, option types, documentation, example usage, and tests that propagate on_duplicate / on_missing into API request bodies. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Client Code
participant Client as SDK Client
participant Builder as Request Builder
participant API as API Request
participant Server
Caller->>Client: call write/writeTuples/deleteTuples(payload, options)
Client->>Builder: build apiBody (includes payload)
Builder->>Builder: extract options.conflict (onDuplicateWrites/onMissingDeletes)
alt conflict provided
Builder->>API: include on_duplicate / on_missing in request body
else no conflict
Builder->>API: omit on_duplicate / on_missing
end
API->>Server: send request
Server-->>API: response
API-->>Client: result
Client-->>Caller: return result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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[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)
config/clients/js/template/example/example1/example1.mjs (1)
148-151: Use the exported enum constant instead of string literal.The conflict option should use
OnDuplicateWrites.Ignoreinstead of the string literal'ignore'to align with the documented API and maintain type safety. The README examples (lines 281, 313) consistently use the enum constants.Apply this diff:
}, { authorizationModelId, - conflict: { onDuplicateWrites: 'ignore' } + conflict: { onDuplicateWrites: OnDuplicateWrites.Ignore } });Don't forget to import the enum at the top of the file:
-import { CredentialsMethod, FgaApiValidationError, OpenFgaClient, TypeName } from "@openfga/sdk"; +import { CredentialsMethod, FgaApiValidationError, OnDuplicateWrites, OpenFgaClient, TypeName } from "@openfga/sdk";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Makefile(1 hunks)config/clients/js/template/README_calling_api.mustache(1 hunks)config/clients/js/template/client.mustache(9 hunks)config/clients/js/template/example/example1/example1.mjs(1 hunks)config/clients/js/template/tests/client.test.ts.mustache(4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
config/**/*.mustache
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Validate mustache syntax and variable references across all template files, including CHANGELOG.md.mustache
Files:
config/clients/js/template/client.mustacheconfig/clients/js/template/tests/client.test.ts.mustacheconfig/clients/js/template/README_calling_api.mustache
config/**/*.{json,mustache}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Never hardcode API keys or credentials in configuration or template files
Files:
config/clients/js/template/client.mustacheconfig/clients/js/template/tests/client.test.ts.mustacheconfig/clients/js/template/README_calling_api.mustache
Makefile
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Makefile: Update OPEN_API_REF in the Makefile when targeting new OpenFGA API versions
Update Docker image tags in the Makefile when upgrading build tools
Use official, tagged Docker images for containerized builds
Files:
Makefile
🧠 Learnings (1)
📚 Learning: 2025-09-04T17:35:34.111Z
Learnt from: CR
PR: openfga/sdk-generator#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-04T17:35:34.111Z
Learning: Applies to Makefile : Update OPEN_API_REF in the Makefile when targeting new OpenFGA API versions
Applied to files:
Makefile
🪛 Gitleaks (8.28.0)
Makefile
[high] 3-3: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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). (3)
- GitHub Check: build-and-test-java-sdk
- GitHub Check: build-and-test-go-sdk
- GitHub Check: build-and-test-dotnet-sdk
🔇 Additional comments (10)
Makefile (1)
3-3: Code change is correct. Commit hash references appropriate OpenFGA API specification.The commit
0ac19aac54f21f3c78970126b84b4c69c6e3b9a2is dated 2025-09-09, which aligns with the OpenFGA API v1.10.0 release on 2025-09-11. The openfga/api repository uses commit SHAs rather than semantic version tags to reference API specifications—this is standard practice. The change properly implements the coding guideline requirement to updateOPEN_API_REFwhen targeting new OpenFGA API versions and enables the generator to use the v1.10.0+ specification.Regarding the Gitleaks alert: this is a confirmed false positive. The value is a git commit SHA-1 hash (40 hex characters), not a secret credential. Consider updating Gitleaks configuration to exclude git commit hash patterns from generic API key detection.
config/clients/js/template/tests/client.test.ts.mustache (4)
16-17: LGTM!The new enum imports are correctly added and will be used in the conflict options tests.
470-601: LGTM!The test suite comprehensively validates conflict option propagation:
- Tests verify correct API payload structure for write operations with conflict options
- Tests cover both
onDuplicateWritesandonMissingDeletesindependently and combined- Tests verify behavior when conflict options are omitted (undefined values)
- Proper mock setup and restoration after each test
622-649: LGTM!Tests properly validate that conflict options flow through the convenience methods (
writeTuplesanddeleteTuples) to the underlying API calls.
670-697: LGTM!The deleteTuples test correctly validates
onMissingDeletesoption propagation.config/clients/js/template/client.mustache (5)
41-42: LGTM!The new enum imports are correctly added to support conflict handling in write operations.
181-183: LGTM!Re-exporting the API enums with more user-friendly names (
OnDuplicateWritesandOnMissingDeletes) provides a cleaner public API surface.
201-230: LGTM!The new option types properly segregate conflict handling concerns:
ClientWriteRequestOptssupports both write and delete conflict optionsClientWriteTuplesRequestOptsonly includesonDuplicateWritesClientDeleteTuplesRequestOptsonly includesonMissingDeletesThis design prevents users from mistakenly passing incompatible options to convenience methods.
523-605: LGTM!The write method implementation correctly:
- Extracts conflict options from the options parameter
- Constructs API body with
tuple_keysand conflict fields (on_duplicate,on_missing)- Preserves conflict options when delegating to
writeTuples/deleteTuplesin non-transaction mode- Handles both transaction and non-transaction modes appropriately
607-649: LGTM!The convenience methods (
writeTuplesanddeleteTuples) properly delegate to the mainwritemethod while preserving conflict options, enabling both single-operation and batch-operation paths to support conflict handling.
phamhieu
force-pushed
the
feat/js-sdk-add-conflict-options
branch
from
e755eef to
9cc47e5
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
config/clients/js/template/client.mustache (1)
185-199: JSDoc casing issue already flagged in previous review.The JSDoc comments at lines 195-197 use lowercase
onMissingDeletesinstead of the correct enum nameOnMissingDeletes. This issue was already identified in a previous review comment.
🧹 Nitpick comments (1)
config/clients/js/template/example/example1/example1.mjs (1)
148-151: Use the enum constant instead of a string literal.For better type safety and to demonstrate best practices, use
OnDuplicateWrites.Ignoreinstead of the string literal'ignore'.Apply this diff:
+import { CredentialsMethod, FgaApiValidationError, OpenFgaClient, TypeName, OnDuplicateWrites } from "@openfga/sdk"; -import { CredentialsMethod, FgaApiValidationError, OpenFgaClient, TypeName } from "@openfga/sdk";Then update the write call:
}, { authorizationModelId, - conflict: { onDuplicateWrites: 'ignore' } + conflict: { onDuplicateWrites: OnDuplicateWrites.Ignore } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Makefile(1 hunks)config/clients/js/template/README_calling_api.mustache(1 hunks)config/clients/js/template/client.mustache(9 hunks)config/clients/js/template/example/example1/example1.mjs(1 hunks)config/clients/js/template/tests/client.test.ts.mustache(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Makefile
- config/clients/js/template/README_calling_api.mustache
🔇 Additional comments (11)
config/clients/js/template/tests/client.test.ts.mustache (5)
16-17: LGTM!The enum imports are correctly added to support the new conflict option tests.
470-573: LGTM!The test coverage for conflict options is comprehensive and correct. The tests properly verify that:
onDuplicateWritesis propagated correctly to the API bodyonMissingDeletesis propagated correctly to the API body- Both options can be used together
Good practice using Jest spies and restoring mocks after each test.
622-649: LGTM!The test correctly verifies that
onDuplicateWritesoption is propagated fromwriteTuplesto the underlyingwritemethod call. Mock cleanup is properly handled.
670-697: LGTM!The test correctly verifies that
onMissingDeletesoption is propagated fromdeleteTuplesto the underlyingwritemethod call. Mock cleanup is properly handled.
575-600: No action required - undefined fields are intentionally set and properly handled by JSON serialization.When the
conflictoption is omitted,on_duplicateis correctly set toundefinedin the apiBody object (line 537-539 of client.mustache). This undefined value is then automatically removed by JSON.stringify() during HTTP serialization, resulting in a clean API payload. The test expectation accurately reflects the intended implementation.config/clients/js/template/client.mustache (6)
41-42: LGTM!The enum imports are correctly added from the generated API model to support conflict handling.
181-183: LGTM!The enum re-exports provide cleaner, more user-friendly names (
OnDuplicateWritesandOnMissingDeletes) for the generated enum types.
201-230: LGTM!The option type definitions are well-structured:
ClientWriteRequestOptsincludes both conflict options sincewritecan handle both writes and deletesClientWriteTuplesRequestOptsincludes onlyonDuplicateWrites(appropriate for write-only operations)ClientDeleteTuplesRequestOptsincludes onlyonMissingDeletes(appropriate for delete-only operations)
507-605: LGTM!The
writemethod implementation correctly:
- Documents the new conflict options in JSDoc
- Extracts conflict settings from options
- Propagates
onDuplicateWritestoapiBody.writes.on_duplicate- Propagates
onMissingDeletestoapiBody.deletes.on_missing- Handles both transaction and non-transaction modes appropriately
607-627: LGTM!The
writeTuplesmethod correctly:
- Updates the parameter type to
ClientWriteTuplesRequestOptsto accept conflict options- Documents the
onDuplicateWritesoption in JSDoc- Passes options through to the
writemethod for proper conflict handling
629-649: LGTM!The
deleteTuplesmethod correctly:
- Updates the parameter type to
ClientDeleteTuplesRequestOptsto accept conflict options- Documents the
onMissingDeletesoption in JSDoc- Passes options through to the
writemethod for proper conflict handling
|
No idea why the .NET tests failed on my change to the JS client. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
2 participants
Description
Generator PR for openfga/js-sdk#276
Review Checklist
mainSummary by CodeRabbit
New Features
Documentation
Tests
Chores