feat: support write conflicts options #237
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 WalkthroughAdded conflict handling options for write operations by introducing Changes
Sequence DiagramsequenceDiagram
participant User
participant Client
participant WriteModel
participant API
User->>Client: write(body, options={conflict: {...}})
rect rgb(200, 220, 255)
Note over Client: Extract conflict options
Client->>Client: conflict_info = options_to_conflict_info(options)
Client->>Client: on_duplicate = conflict_info.on_duplicate_writes
Client->>Client: on_missing = conflict_info.on_missing_deletes
end
rect rgb(220, 240, 220)
Note over Client: Build request with conflict info
Client->>WriteModel: writes_tuple_keys(on_duplicate=on_duplicate)
WriteModel->>WriteModel: return WriteRequestWrites(tuple_keys=..., on_duplicate=...)
Client->>WriteModel: deletes_tuple_keys(on_missing=on_missing)
WriteModel->>WriteModel: return WriteRequestDeletes(tuple_keys=..., on_missing=...)
end
Client->>API: POST /writes with conflict fields
API->>API: Handle duplicate/missing per on_duplicate/on_missing
API-->>Client: Success/Error response
Client-->>User: Result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The changes introduce new model classes and plumb conflict options throughout the write flow across both sync and async implementations. While the changes follow a consistent, well-structured pattern (reducing cognitive load), the heterogeneity of modifications—spanning new models, API signatures, client logic, documentation, and test updates—requires contextual reasoning for each cohort of changes. Possibly related issues
Pre-merge checks and finishing touches✅ Passed checks (5 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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Codecov Report❌ Patch coverage is ❌ Your project status has failed because the head coverage (70.99%) 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 #237 +/- ##
==========================================
+ Coverage 70.86% 70.99% +0.12%
==========================================
Files 135 137 +2
Lines 10932 11038 +106
==========================================
+ Hits 7747 7836 +89
- Misses 3185 3202 +17 ☔ 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 options to the OpenFGA Python SDK, allowing developers to control behavior when writing duplicate tuples or deleting non-existent tuples. The feature requires OpenFGA server v1.10.0 or later.
- Adds new
ConflictOptionsclass withon_duplicate_writesandon_missing_deletessettings - Updates existing test cases to include default conflict option values
- Adds comprehensive test coverage for the new conflict handling functionality
Reviewed Changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| openfga_sdk/models/write_request_writes.py | Added on_duplicate field to control duplicate write behavior |
| openfga_sdk/models/write_request_deletes.py | Added on_missing field to control missing delete behavior |
| openfga_sdk/client/models/write_conflict_opts.py | New model defining conflict option enums and ConflictOptions class |
| openfga_sdk/client/models/write_options.py | New model for client write options including conflict settings |
| openfga_sdk/client/models/write_request.py | Updated to support conflict options in tuple key methods |
| openfga_sdk/sync/client/client.py | Added conflict option handling in write operations |
| openfga_sdk/client/client.py | Added conflict option handling in async write operations |
| test files | Updated existing tests to include default conflict values and added new conflict option tests |
| documentation files | Updated API documentation and README with conflict options examples |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or 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: 1
♻️ Duplicate comments (1)
openfga_sdk/client/client.py (1)
544-554: Conflict options plumbed correctly to writes/deletesExtract/translate enum values to strings and pass into WriteRequestWrites/Deletes is correct and matches API: key on writes/deletes. Nice.
Also applies to: 557-560
🧹 Nitpick comments (17)
README.md (1)
751-793: Promote the example to a real headinglint is complaining (MD036) because we’re using bold text in place of a heading. Switching to
###### Example: Ignore duplicate writes and missing deletes(or similar) keeps the formatting consistent and unblocks the markdown check.test/client/client_test.py (3)
3986-4042: Refactor: Move imports to module level and remove redundant configuration.The test logic is correct, but there are maintainability issues:
Imports inside test method (lines 3990-3993): Move the
ConflictOptionsand enum imports to the module level alongside other imports for consistency with the rest of the file.Redundant configuration setup (lines 3997-4001): The
setUp()method already initializesself.configuration, so recreating it here is unnecessary. Simply reuse the existing configuration:- self.configuration = ClientConfiguration( - api_url="http://api.fga.example", - ) - configuration = self.configuration + configuration = self.configuration
4044-4102: Refactor: Move imports to module level and remove redundant configuration.Same maintainability issues as the previous test:
Imports inside test method (lines 4050-4053): Move to module level.
Redundant configuration setup (lines 4057-4061): Remove and reuse
self.configurationfromsetUp():- self.configuration = ClientConfiguration( - api_url="http://api.fga.example", - ) - configuration = self.configuration + configuration = self.configuration
4104-4179: Refactor: Move imports to module level and remove redundant configuration.This test provides good coverage for the combined conflict options scenario. However, apply the same refactoring as the previous tests:
Imports inside test method (lines 4108-4112): Move to module level.
Redundant configuration setup (lines 4116-4120): Reuse
self.configuration:- self.configuration = ClientConfiguration( - api_url="http://api.fga.example", - ) - configuration = self.configuration + configuration = self.configurationtest/sync/client/client_test.py (2)
1027-1029: Reduce duplication of default "error" expectations in bodiesYou repeatedly assert literal bodies including "on_duplicate": "error" / "on_missing": "error". Consider a tiny helper to build expected bodies to avoid drift and ease future changes.
Example helper and usage:
+def expected_write_body( + writes=None, deletes=None, authz_id=None, on_dup="error", on_miss="error" +): + body = {} + if writes is not None: + body["writes"] = {"tuple_keys": writes, "on_duplicate": on_dup} + if deletes is not None: + body["deletes"] = {"tuple_keys": deletes, "on_missing": on_miss} + if authz_id: + body["authorization_model_id"] = authz_id + return bodyThen in assertions:
- body={ ... literal dict ... } + body=expected_write_body( + writes=[{ "object": "...", "relation": "reader", "user": "..." }], + authz_id="01G5JAVJ41T49E9TT3SKVS7X1J", +)Also applies to: 1073-1075, 1174-1176, 1196-1198, 1218-1220, 1319-1321, 1341-1343, 1363-1365, 1469-1471, 1491-1493, 1705-1707, 1769-1771, 1890-1892
4087-4160: Add a mixed-options test (only one flag set with both writes and deletes)Add a case where both writes and deletes are present but only on_duplicate_writes=IGNORE is set. Expect writes.on_duplicate="ignore" and deletes.on_missing="error" (default). This guards regressions in selective flag application.
openfga_sdk/client/client.py (3)
147-156: Type hint/options shape too narrow for conflict optionsoptions_to_conflict_info accepts dict[str, int|str|dict], but options["conflict"] is a ConflictOptions object. Loosen the type to avoid false-positives in static checks and clarify return type.
Apply:
+from typing import Any, Mapping, Optional +from openfga_sdk.client.models.write_conflict_opts import ConflictOptions @@ -def options_to_conflict_info( - options: dict[str, int | str | dict[str, int | str]] | None = None, -): +def options_to_conflict_info( + options: Mapping[str, Any] | None = None, +) -> Optional[ConflictOptions]: @@ - if options is not None and options.get("conflict"): - return options["conflict"] + if options is not None: + conflict = options.get("conflict") + if isinstance(conflict, ConflictOptions): + return conflict return None
588-595: Document the new conflict option in write() docstringAdd the new option to the docstring so users discover it.
:param retryParams.minWaitInMs(options) - Override the minimum wait before a retry is initiated +:param conflict(options) - ConflictOptions controlling on_duplicate_writes/on_missing_deletes behavior
619-635: Mirror conflict option mention in write_tuples/delete_tuples docstringsAdd the same docstring note here for parity.
- :param retryParams.minWaitInMs(options) - Override the minimum wait before a retry is initiated + :param retryParams.minWaitInMs(options) - Override the minimum wait before a retry is initiated + :param conflict(options) - ConflictOptions controlling on_duplicate_writes/on_missing_deletes behaviorAlso applies to: 636-652
openfga_sdk/sync/client/client.py (3)
148-157: Type hint/options shape too narrow for conflict options (sync client)Same suggestion as async: loosen types and return Optional[ConflictOptions].
+from typing import Any, Mapping, Optional +from openfga_sdk.client.models.write_conflict_opts import ConflictOptions @@ -def options_to_conflict_info( - options: dict[str, int | str | dict[str, int | str]] | None = None, -): +def options_to_conflict_info( + options: Mapping[str, Any] | None = None, +) -> Optional[ConflictOptions]: @@ - if options is not None and options.get("conflict"): - return options["conflict"] + if options is not None: + conflict = options.get("conflict") + if isinstance(conflict, ConflictOptions): + return conflict return None
596-596: Header value typo/inconsistency: "Writes" vs "Write"Async client sets "Write"; sync sets "Writes". Align to one ("Write") for consistency and easier log analysis.
- options = set_heading_if_not_set(options, CLIENT_METHOD_HEADER, "Writes") + options = set_heading_if_not_set(options, CLIENT_METHOD_HEADER, "Write")
589-595: Add conflict option note to sync docstrings (write, write_tuples, delete_tuples)Mirror the doc updates suggested for async client.
- :param retryParams.minWaitInMs(options) - Override the minimum wait before a retry is initiated + :param retryParams.minWaitInMs(options) - Override the minimum wait before a retry is initiated + :param conflict(options) - ConflictOptions controlling on_duplicate_writes/on_missing_deletes behaviorAlso applies to: 619-635, 636-652
openfga_sdk/models/write_request_deletes.py (1)
34-42: on_missing enum + default look good; add a small test for invalid values.Validation and defaults are correct. Note: because the constructor default is "error", requests will always include on_missing="error" when not provided. If omission was desired, you’d need a sentinel instead of defaulting to "error".
Please add a unit test that asserts ValueError on invalid input (e.g., "fail") and positive cases for "error"/"ignore" to close the small coverage gap.
Example test skeleton:
def test_write_request_deletes_on_missing_validation(): d = WriteRequestDeletes(tuple_keys=[]) d.on_missing = "error" d.on_missing = "ignore" with pytest.raises(ValueError): d.on_missing = "fail"Also applies to: 44-46, 56-59, 83-113
openfga_sdk/client/models/write_options.py (1)
1-11: API surface LGTM; drop the “auto generated” header and add minimal usage tests.
- Header says “auto generated”; this class is hand-written client API. Remove to avoid confusion with generator syncs.
- Please add a small test to instantiate ClientWriteOptions and exercise getters/setters (will close most of the patch coverage gap).
Diff (header tweak):
- NOTE: This file was auto generated by OpenAPI Generator (https://openapi-generator.tech). DO NOT EDIT. + NOTE: Client helper, maintained in-repo (not generated).Quick test idea:
def test_client_write_options_roundtrip(): tx = WriteTransactionOpts(disabled=True, max_per_chunk=2, max_parallel_requests=3) cf = ConflictOptions() o = ClientWriteOptions("am1", tx, cf) assert o.authorization_model_id == "am1" assert o.transaction is tx assert o.conflict is cfAlso ensure it’s exported in openfga_sdk/client/models/init.py so users can import it directly.
Also applies to: 22-31, 32-81
openfga_sdk/client/models/write_request.py (2)
59-76: Type the on_duplicate param more strictly.Use a narrow type to prevent invalid values at call sites (and leverage editor/type checking).
+from typing import Literal @@ - def writes_tuple_keys( - self, on_duplicate: str | None = None + def writes_tuple_keys( + self, on_duplicate: Literal["error", "ignore"] | None = None ) -> WriteRequestWrites | None:If you already have client enums (e.g., ClientWriteRequestOnDuplicateWrites), consider using that instead for a single source of truth.
77-93: Mirror typing for on_missing and fix docstring grammar.
- Apply the same strict type for on_missing.
- Minor: “Return the delete as tuple keys” → “Return the deletes as tuple keys”.
- def deletes_tuple_keys( - self, on_missing: str | None = None + def deletes_tuple_keys( + self, on_missing: Literal["error", "ignore"] | None = None ) -> WriteRequestDeletes | None: @@ - """ - Return the delete as tuple keys - """ + """ + Return the deletes as tuple keys + """openfga_sdk/models/write_request_writes.py (1)
83-113: on_duplicate implementation is correct; small doc polish + test.
- Validation/defaults are right. Consider tightening the docstring punctuation: “On 'error' (or unspecified), ...” to match style elsewhere.
- Add a unit test mirroring deletes to cover invalid values and both valid options; this should address the single missing line in coverage.
Doc tweak:
- On 'error' ( or unspecified ), the API returns an error ... + On 'error' (or unspecified), the API returns an error ...Test idea:
def test_write_request_writes_on_duplicate_validation(): w = WriteRequestWrites(tuple_keys=[]) w.on_duplicate = "error" w.on_duplicate = "ignore" with pytest.raises(ValueError): w.on_duplicate = "fail"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
.openapi-generator/FILES(1 hunks)CHANGELOG.md(1 hunks)README.md(2 hunks)docs/OpenFgaApi.md(1 hunks)docs/WriteRequestDeletes.md(1 hunks)docs/WriteRequestWrites.md(1 hunks)openfga_sdk/api/open_fga_api.py(2 hunks)openfga_sdk/client/client.py(2 hunks)openfga_sdk/client/models/__init__.py(2 hunks)openfga_sdk/client/models/write_conflict_opts.py(1 hunks)openfga_sdk/client/models/write_options.py(1 hunks)openfga_sdk/client/models/write_request.py(3 hunks)openfga_sdk/models/write_request_deletes.py(2 hunks)openfga_sdk/models/write_request_writes.py(2 hunks)openfga_sdk/sync/client/client.py(2 hunks)openfga_sdk/sync/open_fga_api.py(2 hunks)test/api/open_fga_api_test.py(2 hunks)test/client/client_test.py(18 hunks)test/sync/client/client_test.py(18 hunks)test/sync/open_fga_api_test.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
openfga_sdk/client/models/__init__.py (2)
openfga_sdk/client/models/write_conflict_opts.py (3)
ClientWriteRequestOnDuplicateWrites(16-18)ClientWriteRequestOnMissingDeletes(21-23)ConflictOptions(26-71)openfga_sdk/client/models/write_options.py (1)
ClientWriteOptions(17-81)
openfga_sdk/client/client.py (5)
openfga_sdk/sync/client/client.py (1)
options_to_conflict_info(148-156)openfga_sdk/models/write_request_writes.py (2)
on_duplicate(84-92)on_duplicate(95-112)openfga_sdk/models/write_request_deletes.py (2)
on_missing(84-92)on_missing(95-112)openfga_sdk/client/models/write_conflict_opts.py (4)
on_duplicate_writes(40-44)on_duplicate_writes(47-54)on_missing_deletes(57-61)on_missing_deletes(64-71)openfga_sdk/client/models/write_request.py (6)
writes_tuple_keys(59-75)deletes_tuple_keys(77-93)writes(32-36)writes(39-43)deletes(46-50)deletes(53-57)
openfga_sdk/models/write_request_deletes.py (2)
openfga_sdk/models/write_request_writes.py (2)
tuple_keys(61-68)tuple_keys(71-81)openfga_sdk/configuration.py (2)
Configuration(120-786)get_default_copy(360-371)
openfga_sdk/client/models/write_options.py (2)
openfga_sdk/client/models/write_conflict_opts.py (1)
ConflictOptions(26-71)openfga_sdk/client/models/write_transaction_opts.py (1)
WriteTransactionOpts(14-78)
test/client/client_test.py (3)
openfga_sdk/client/models/write_conflict_opts.py (7)
ClientWriteRequestOnDuplicateWrites(16-18)ConflictOptions(26-71)on_duplicate_writes(40-44)on_duplicate_writes(47-54)ClientWriteRequestOnMissingDeletes(21-23)on_missing_deletes(57-61)on_missing_deletes(64-71)openfga_sdk/client/client.py (1)
write(583-617)openfga_sdk/client/models/write_request.py (5)
ClientWriteRequest(18-93)writes(32-36)writes(39-43)deletes(46-50)deletes(53-57)
openfga_sdk/models/write_request_writes.py (3)
openfga_sdk/models/write_request_deletes.py (2)
tuple_keys(61-68)tuple_keys(71-81)openfga_sdk/models/contextual_tuple_keys.py (2)
tuple_keys(50-57)tuple_keys(60-70)openfga_sdk/configuration.py (2)
Configuration(120-786)get_default_copy(360-371)
openfga_sdk/client/models/write_request.py (2)
openfga_sdk/models/write_request_writes.py (5)
on_duplicate(84-92)on_duplicate(95-112)WriteRequestWrites(20-162)tuple_keys(61-68)tuple_keys(71-81)openfga_sdk/models/write_request_deletes.py (5)
tuple_keys(61-68)tuple_keys(71-81)on_missing(84-92)on_missing(95-112)WriteRequestDeletes(20-162)
test/sync/client/client_test.py (3)
openfga_sdk/client/models/write_conflict_opts.py (7)
ClientWriteRequestOnDuplicateWrites(16-18)ConflictOptions(26-71)on_duplicate_writes(40-44)on_duplicate_writes(47-54)ClientWriteRequestOnMissingDeletes(21-23)on_missing_deletes(57-61)on_missing_deletes(64-71)openfga_sdk/sync/client/client.py (1)
write(583-617)openfga_sdk/client/models/write_request.py (5)
ClientWriteRequest(18-93)writes(32-36)writes(39-43)deletes(46-50)deletes(53-57)
openfga_sdk/sync/client/client.py (4)
openfga_sdk/models/write_request_writes.py (2)
on_duplicate(84-92)on_duplicate(95-112)openfga_sdk/models/write_request_deletes.py (2)
on_missing(84-92)on_missing(95-112)openfga_sdk/client/models/write_conflict_opts.py (4)
on_duplicate_writes(40-44)on_duplicate_writes(47-54)on_missing_deletes(57-61)on_missing_deletes(64-71)openfga_sdk/client/models/write_request.py (6)
writes_tuple_keys(59-75)deletes_tuple_keys(77-93)writes(32-36)writes(39-43)deletes(46-50)deletes(53-57)
🪛 markdownlint-cli2 (0.18.1)
README.md
755-755: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
docs/OpenFgaApi.md
1294-1294: Spaces inside code span elements
(MD038, no-space-in-code)
1294-1294: Spaces inside code span elements
(MD038, no-space-in-code)
🔇 Additional comments (18)
openfga_sdk/sync/open_fga_api.py (1)
2666-2694: Docs now cover conflict-handling semantics clearlyThe expanded guidance accurately mirrors the new
on_duplicate/on_missingbehavior and will help users reason about mixed error/ignore combinations. Nice one.openfga_sdk/client/models/write_conflict_opts.py (1)
16-71: Solid conflict option surfaceEnums plus the lightweight container keep the API ergonomic while remaining explicit about accepted values—looks good.
.openapi-generator/FILES (1)
142-143: Generator manifest updated appropriatelyIncluding the new client model modules in the tracked list keeps regeneration in sync. All good.
README.md (1)
205-263: Good call documenting custom headersThe async + per-request examples make header overrides crystal clear.
test/sync/open_fga_api_test.py (2)
953-954: LGTM! Test correctly validates default conflict option.The test expectation now includes
on_duplicate: "error"in the writes payload, which correctly validates that the new conflict option field is serialized with its default value.
1008-1009: LGTM! Test correctly validates default conflict option for deletes.The test expectation now includes
on_missing: "error"in the deletes payload, which correctly validates that the new conflict option field is serialized with its default value.CHANGELOG.md (1)
4-9: LGTM! Changelog entry is comprehensive and clear.The changelog entry provides all the necessary information:
- Feature description with issue reference
- Server version requirement (v1.10.0+)
- Documentation link
- Clear explanation of both conflict options
The current format is more detailed and informative than the suggestion in the past review comment, and it follows the established pattern of other entries in this changelog.
docs/WriteRequestWrites.md (1)
8-8: LGTM! Documentation is clear and accurate.The documentation clearly describes the behavior of the
on_duplicatefield:
- Default behavior ('error') returns an error for duplicate tuples
- Alternative behavior ('ignore') treats duplicates as no-ops
- Properly marked as optional with default value
docs/WriteRequestDeletes.md (1)
8-8: LGTM! Documentation is clear and accurate.The documentation clearly describes the behavior of the
on_missingfield:
- Default behavior ('error') returns an error when deleting non-existent tuples
- Alternative behavior ('ignore') treats missing deletes as no-ops
- Properly marked as optional with default value
The documentation is consistent with the
on_duplicatefield for writes.test/api/open_fga_api_test.py (2)
886-887: LGTM! Async test correctly validates default conflict option.The async test expectation now includes
on_duplicate: "error"in the writes payload, consistent with the sync test. This ensures both async and sync clients serialize the conflict option field correctly.
941-942: LGTM! Async test correctly validates default conflict option for deletes.The async test expectation now includes
on_missing: "error"in the deletes payload, consistent with the sync test. This ensures both async and sync clients serialize the conflict option field correctly.openfga_sdk/api/open_fga_api.py (1)
2670-2670: LGTM! Comprehensive documentation for conflict handling options.The docstring updates clearly explain the new idempotency controls (
on_duplicateandon_missing), including:
- Default non-idempotent behavior
- How to enable idempotency for writes and deletes
- Precedence rules when mixing idempotent and non-idempotent operations
- Rollback semantics on failures
- Practical examples demonstrating usage
The documentation aligns perfectly with the PR objectives to support write conflict options as described in issue #235.
Also applies to: 2697-2697
test/client/client_test.py (1)
1029-1030: LGTM: Default conflict behavior correctly enforced in tests.The updates consistently verify that write operations now include default conflict handling values (
"on_duplicate": "error"for writes and"on_missing": "error"for deletes), aligning with the API's default behavior introduced in OpenFGA v1.10.0.Also applies to: 1075-1076, 1177-1177, 1199-1199, 1221-1221, 1322-1322, 1344-1344, 1366-1366, 1471-1471, 1493-1493, 1608-1608, 1630-1630, 1652-1652, 1707-1707, 1771-1771, 1835-1835, 1892-1892
test/sync/client/client_test.py (3)
3973-4027: Conflict options tests (IGNORE on duplicates): solid coverageGood addition validating that options["conflict"].on_duplicate_writes=IGNORE yields "on_duplicate": "ignore" under writes. LGTM.
4030-4085: Conflict options tests (IGNORE on missing deletes): solid coverageGood coverage for deletes path; assertion correctly expects "on_missing": "ignore". LGTM.
4087-4160: Conflict options tests (both flags together): solid coverageEnd‑to‑end path when both are present looks correct. LGTM.
openfga_sdk/sync/client/client.py (1)
543-554: Conflict options plumbing looks goodLogic mirrors async path; correct behavior. LGTM.
Also applies to: 557-560
openfga_sdk/client/models/__init__.py (1)
29-35: Public API exports: looks goodNew enums, ConflictOptions, and ClientWriteOptions are correctly re-exported. LGTM.
Also applies to: 56-60
Description
Closes #235
This is generated from openfga/sdk-generator#643
But there are some lint fixes in the sdk using ruff, which are not in sync right now with the generator.
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
on_duplicate(ERROR/IGNORE) andon_missing(ERROR/IGNORE) to govern write-operation semantics. Requires OpenFGA server v1.10.0 or later.Documentation