Skip to content

feat(python-sdk): add support for write conflict settings #643

Merged
rhamzeh merged 15 commits intomainfrom
feat/py-sdk/support-write-conflict-options
Oct 27, 2025
Merged

feat(python-sdk): add support for write conflict settings #643
rhamzeh merged 15 commits intomainfrom
feat/py-sdk/support-write-conflict-options

Conversation

@SoulPancake
Copy link
Copy Markdown
Member

@SoulPancake SoulPancake commented Oct 16, 2025

Description

for #610

What problem is being solved?

The linked PR in the python sdk is the following : openfga/python-sdk#237

How is it being solved?

What changes are made to solve it?

References

Review Checklist

  • I have clicked on "allow edits by maintainers".
  • I have added documentation for new/changed functionality in this PR or in a PR to openfga.dev [Provide a link to any relevant PRs in the references section above]
  • The correct base branch is being used, if not main
  • I have added tests to validate that the change in functionality is working as expected

Summary by CodeRabbit

  • New Features

    • Added support for conflict options on write operations, enabling configuration of behavior when encountering duplicate writes (ERROR or IGNORE) and missing deletes (ERROR or IGNORE). Requires OpenFGA server v1.10.0 or later.
  • Documentation

    • Added documentation and code examples demonstrating how to configure and use conflict options for write operations.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Oct 16, 2025

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Note

Other AI code review bot(s) detected

CodeRabbit 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.

Walkthrough

This PR adds support for conflict options on write operations across the Python SDK templates within the generator. It updates API references, introduces new model classes for conflict handling (ConflictOptions, ClientWriteOptions), extends client write methods to accept and propagate conflict options, and updates tests and documentation to exercise the new functionality.

Changes

Cohort / File(s) Summary
Configuration & References
.github/workflows/main.yaml, Makefile
Updated OPEN_API_REF environment variable from c0b62b28... to 0ac19aac... to reference an updated API specification.
Documentation
config/clients/python/CHANGELOG.md.mustache, config/clients/python/template/README_calling_api.mustache
Added changelog entry and documentation blocks describing new conflict options feature for write operations, including configuration examples and usage patterns.
Configuration Overrides
config/clients/python/config.overrides.json
Added template entries for generating write_conflict_opts.py and write_options.py model files.
Write Conflict Models
config/clients/python/template/src/client/models/write_conflict_opts.py.mustache, config/clients/python/template/src/client/models/write_options.py.mustache
New model files introducing ClientWriteRequestOnDuplicateWrites and ClientWriteRequestOnMissingDeletes enums, ConflictOptions class, and ClientWriteOptions container for write configuration.
Model Exports
config/clients/python/template/src/client/models/__init__.py.mustache
Extended public API to re-export newly added conflict and write option classes.
Error Message Formatting
config/clients/python/template/model.mustache
Refactored ValueError message construction from .format() calls to f-strings with intermediate variables for array, map, and scalar field validation.
Write Request Methods
config/clients/python/template/src/client/models/write_request.py.mustache
Converted writes_tuple_keys and deletes_tuple_keys from properties to methods accepting optional on_duplicate and on_missing parameters respectively.
Async Client Implementation
config/clients/python/template/src/client/client.py.mustache
Added options_to_conflict_info() helper function and extended _write_with_transaction to extract and propagate conflict options (on_duplicate, on_missing) to tuple key construction.
Sync Client Implementation
config/clients/python/template/src/sync/client/client.py.mustache
Mirrored async client changes with options_to_conflict_info() helper and conflict option propagation in _write_with_transaction.
API Tests
config/clients/python/template/test/api_test.py.mustache, config/clients/python/template/test/sync/api_test.py.mustache
Added "on_duplicate": "error" and "on_missing": "error" fields to write/delete request payloads in test assertions.
Client Tests
config/clients/python/template/test/client/client_test.py.mustache, config/clients/python/template/test/sync/client/client_test.py.mustache
Expanded test coverage to validate conflict option serialization; updated expected request bodies to include on_duplicate and on_missing fields and added test scenarios for ignore behavior.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

The changes span 16+ files with a mix of new model definitions, client logic extensions, and comprehensive test updates. While the modifications follow a consistent, homogeneous pattern (threading conflict options through write operations), the scope encompasses both async and sync variants, introduces new public APIs, and modifies client core logic. Template refactoring and straightforward model additions reduce overall complexity, but logic density in client changes and breadth of affected areas warrant moderate review effort.

Possibly related PRs

Suggested reviewers

  • evansims

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The PR title "feat(python-sdk): add support for write conflict settings" accurately reflects the primary objective of the changeset. The raw summary confirms the PR implements support for write conflict settings across multiple files, including new ConflictOptions and ClientWriteOptions classes, updates to write request handling, new enums for conflict behavior (on_duplicate_writes, on_missing_deletes), and corresponding documentation and test coverage. The title is specific enough to convey the feature (write conflict settings) and component (python-sdk), follows conventional commit format, and avoids vague terminology or unnecessary noise.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@SoulPancake
Copy link
Copy Markdown
Member Author

@evansims I ran into a small issue here
The ruff linter expects f strings for the error and I used the fix flag to fix it in the sdk
however, I am a bit confused how to get the generator to generate it right

@evansims
Copy link
Copy Markdown
Contributor

evansims commented Oct 16, 2025

👋 Hey @SoulPancake — you'll just need some minor tweaks on config/clients/python/template/model.mustache to address those ruff errors:

After line 119:

invalid_values = ", ".join(map(str, set({{{name}}}) - set(allowed_values)))
valid_values = ", ".join(map(str, allowed_values))
raise ValueError(
    f"Invalid values for `{{{name}}}` [{invalid_values}], must be a subset of [{valid_values}]"
)

After line 128:

invalid_keys = ", ".join(map(str, set({{{name}}}.keys()) - set(allowed_values)))
valid_values = ", ".join(map(str, allowed_values))
raise ValueError(
    f"Invalid keys in `{{{name}}}` [{invalid_keys}], must be a subset of [{valid_values}]"
)

And after line 139:

raise ValueError(
    f"Invalid value for `{{{name}}}` ({{{name}}}), must be one of {allowed_values}"
)

@SoulPancake
Copy link
Copy Markdown
Member Author

I am still reverse-synching this PR with the py-sdk PR. I will mark this ready for review post that

@SoulPancake SoulPancake marked this pull request as ready for review October 17, 2025 17:20
@SoulPancake SoulPancake requested a review from a team as a code owner October 17, 2025 17:20
Copilot AI review requested due to automatic review settings October 17, 2025 17:20
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Adds support for write conflict handling in the Python SDK templates by introducing conflict options for duplicate writes and missing deletes, updates tests, and documents usage.

  • Introduces ConflictOptions with enum controls for on_duplicate_writes and on_missing_deletes.
  • Wires conflict options into write request construction; updates tests to assert request payloads and adds new tests for ignore behavior.
  • Updates OpenAPI ref, CI workflow, README docs, and improves some template error messages and formatting.

Reviewed Changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
config/clients/python/template/src/client/models/write_conflict_opts.py.mustache New model for conflict options/enums enabling duplicate/missing handling.
config/clients/python/template/src/client/models/write_options.py.mustache New typed write options container including conflict options (not yet wired as primary input type).
config/clients/python/template/src/client/models/write_request.py.mustache Changes writes/deletes tuple-key helpers to accept conflict parameters.
config/clients/python/template/src/client/client.py.mustache Async client: plumbs conflict options into WriteRequest build.
config/clients/python/template/src/sync/client/client.py.mustache Sync client: plumbs conflict options into WriteRequest build.
config/clients/python/template/test/client/client_test.py.mustache Async tests updated/added to validate conflict behavior and formatting tweaks.
config/clients/python/template/test/sync/client/client_test.py.mustache Sync tests updated/added to validate conflict behavior and formatting tweaks.
config/clients/python/template/test/sync/api_test.py.mustache Adjusts expected payloads to include default on_duplicate/on_missing.
config/clients/python/template/test/api_test.py.mustache Adjusts expected payloads to include default on_duplicate/on_missing.
config/clients/python/template/model.mustache Refactors enum validation error messages to f-strings.
config/clients/python/template/README_calling_api.mustache Documents conflict options for write operations.
config/clients/python/template/src/client/models/init.py.mustache Exports new conflict/write options in public API.
config/clients/python/config.overrides.json Maps new support files into package outputs.
Makefile Updates OPEN_API_REF to a newer API commit supporting the new fields.
.github/workflows/main.yaml Aligns CI OPEN_API_REF with Makefile.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (2)
config/clients/python/template/src/client/models/write_request.py.mustache (2)

49-66: Preserve backward compatibility for writes_tuple_keys

Previously this was a property; changing it to a method is a breaking API change. Keep the new parameterized builder, but reintroduce a legacy property that returns the default behavior.

Apply these diffs:

  1. Rename the new methods to builder names and add legacy properties.
-    def writes_tuple_keys(
-        self, on_duplicate: str | None = None
-    ) -> WriteRequestWrites | None:
+    def build_writes_tuple_keys(
+        self, on_duplicate: str | None = None
+    ) -> WriteRequestWrites | None:
@@
-        if on_duplicate is not None:
-            return WriteRequestWrites(tuple_keys=keys, on_duplicate=on_duplicate)
-        return WriteRequestWrites(tuple_keys=keys)
+        if on_duplicate is not None:
+            return WriteRequestWrites(tuple_keys=keys, on_duplicate=on_duplicate)
+        return WriteRequestWrites(tuple_keys=keys)
+
+    @property
+    def writes_tuple_keys(self) -> WriteRequestWrites | None:
+        # Legacy property: default behavior
+        return self.build_writes_tuple_keys()

67-83: Do the same for deletes_tuple_keys

Reintroduce the legacy property and add builder method.

-    def deletes_tuple_keys(
-        self, on_missing: str | None = None
-    ) -> WriteRequestDeletes | None:
+    def build_deletes_tuple_keys(
+        self, on_missing: str | None = None
+    ) -> WriteRequestDeletes | None:
@@
-        if on_missing is not None:
-            return WriteRequestDeletes(tuple_keys=keys, on_missing=on_missing)
-        return WriteRequestDeletes(tuple_keys=keys)
+        if on_missing is not None:
+            return WriteRequestDeletes(tuple_keys=keys, on_missing=on_missing)
+        return WriteRequestDeletes(tuple_keys=keys)
+
+    @property
+    def deletes_tuple_keys(self) -> WriteRequestDeletes | None:
+        # Legacy property: default behavior
+        return self.build_deletes_tuple_keys()
🧹 Nitpick comments (8)
config/clients/python/CHANGELOG.md.mustache (1)

4-9: Align naming and references in Unreleased entry.

  • Use the actual PR/issue reference for this generator change (current PR is #643), or drop the number to avoid drift.
  • Consider matching field names to the Python surface to reduce confusion: “on_duplicate_writes” and “on_missing_deletes” (enums) rather than “on_duplicate/on_missing”.
- - feat: add support for conflict options for Write operations: (#235)
+ - feat: add support for conflict options for Write operations (#643)
@@
-  - `on_duplicate` for handling duplicate tuple writes (ERROR or IGNORE)
-  - `on_missing` for handling deletes of non-existent tuples (ERROR or IGNORE)
+  - `on_duplicate_writes` for handling duplicate tuple writes (ERROR or IGNORE)
+  - `on_missing_deletes` for handling deletes of non-existent tuples (ERROR or IGNORE)

Ensure the README anchor exists at README.md#conflict-options-for-write-operations in the generated package.

config/clients/python/template/README_calling_api.mustache (1)

455-496: Use templated package name and re-exported imports in the example.

Avoid hardcoding “openfga_sdk”; import from {{packageName}} and from client.models (init re-exports). Keeps docs stable if packageName changes.

-# from openfga_sdk import OpenFgaClient
-# from openfga_sdk.client.models import ClientTuple, ClientWriteRequest
-# from openfga_sdk.client.models.write_conflict_opts import (
-#     ClientWriteRequestOnDuplicateWrites,
-#     ClientWriteRequestOnMissingDeletes,
-#     ConflictOptions,
-# )
+# from {{packageName}} import OpenFgaClient
+# from {{packageName}}.client.models import ClientTuple, ClientWriteRequest
+# from {{packageName}}.client.models import (
+#     ClientWriteRequestOnDuplicateWrites,
+#     ClientWriteRequestOnMissingDeletes,
+#     ConflictOptions,
+# )

Confirm client/models/init.py.mustache re-exports the three symbols so the above import works.

config/clients/python/template/src/sync/client/client.py.mustache (1)

115-122: Broaden options typing; add return type for clarity.

options may now hold ConflictOptions objects, so the current dict[str, int|str|dict] hint is too narrow. Recommend widening and adding typing imports.

+from typing import Any, Optional
@@
-def options_to_conflict_info(options: dict[str, int | str | dict[str, int | str]] | None = None):
+def options_to_conflict_info(options: dict[str, Any] | None = None) -> Optional[object]:
     """
     Return the conflict info
     """
     if options is not None and options.get("conflict"):
         return options["conflict"]
     return None

Optionally, annotate return to the actual ConflictOptions type if importing it here won’t create a circular import.

If you prefer stronger typing end-to-end, I can propose a follow-up to standardize options: dict[str, Any] across helpers in this file.

config/clients/python/template/test/sync/client/client_test.py.mustache (1)

3963-4150: Consider optional test enhancements for more comprehensive coverage.

The current conflict options tests are solid, but you might consider adding:

  1. Explicit tests for ERROR enum values (currently only tested implicitly through default behavior)
  2. Tests for conflict options in batch operations (write_batch, delete_batch with transaction.disabled=True)
  3. Edge case: conflict options with empty writes/deletes arrays

These additions would make the test suite more robust but are not critical since the core functionality is well-tested.

config/clients/python/template/src/client/client.py.mustache (1)

87-106: Consider loosening options typing (non-blocking)

options is annotated as dict[str, int | str | dict[...] ], but you store objects (e.g., RetryParams, ConflictOptions). Prefer Mapping[str, Any] across helpers to reflect actual usage.

- from {{packageName}}.api_client import ApiClient
+ from {{packageName}}.api_client import ApiClient
+ from typing import Any, Mapping
@@
-def options_to_kwargs(
-    options: dict[str, int | str | dict[str, int | str]] | None = None,
-) -> dict[str, int | str | dict[str, int | str]]:
+def options_to_kwargs(
+    options: Mapping[str, Any] | None = None,
+) -> dict[str, Any]:
@@
-def options_to_transaction_info(options: dict[str, int | str | dict[str, int | str]] | None = None):
+def options_to_transaction_info(options: Mapping[str, Any] | None = None):
@@
-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):
config/clients/python/template/src/client/models/write_options.py.mustache (1)

7-71: Optional: simplify with @dataclass

This is a plain data container; consider a dataclass for brevity and type clarity. Non-blocking.

+from dataclasses import dataclass
@@
-class ClientWriteOptions:
-    """
-    OpenFGA client write options
-    """
-    def __init__(..., conflict: ConflictOptions | None = None) -> None:
-        self._authorization_model_id = authorization_model_id
-        self._transaction = transaction
-        self._conflict = conflict
-    @property
-    def authorization_model_id(self) -> str | None: ...
-    @authorization_model_id.setter
-    def authorization_model_id(self, value: str | None) -> None: ...
-    @property
-    def transaction(self) -> WriteTransactionOpts | None: ...
-    @transaction.setter
-    def transaction(self, value: WriteTransactionOpts | None) -> None: ...
-    @property
-    def conflict(self) -> ConflictOptions | None: ...
-    @conflict.setter
-    def conflict(self, value: ConflictOptions | None) -> None: ...
+@dataclass
+class ClientWriteOptions:
+    """OpenFGA client write options"""
+    authorization_model_id: str | None = None
+    transaction: WriteTransactionOpts | None = None
+    conflict: ConflictOptions | None = None
config/clients/python/template/src/client/models/write_conflict_opts.py.mustache (2)

16-27: LGTM! Clean initialization with modern type hints.

The implementation correctly uses Python 3.10+ union syntax and follows the private attribute pattern. Runtime validation is appropriately deferred to the API call level.

Consider enhancing the class docstring to include usage examples or describe the purpose of the conflict options:

 class ConflictOptions:
     """
-    OpenFGA client write conflict options
+    OpenFGA client write conflict options.
+    
+    Attributes:
+        on_duplicate_writes: How to handle duplicate write operations (ERROR or IGNORE)
+        on_missing_deletes: How to handle deletes on missing tuples (ERROR or IGNORE)
     """

29-61: Properties implemented correctly.

The getter/setter pattern is properly implemented. The absence of runtime validation is appropriate for a client-side options class where type hints provide static checking.

Consider enriching the property docstrings to be more descriptive:

     @property
     def on_duplicate_writes(self) -> ClientWriteRequestOnDuplicateWrites | None:
         """
-        Return on_duplicate_writes
+        Get the policy for handling duplicate write operations.
+        
+        Returns:
+            ClientWriteRequestOnDuplicateWrites | None: ERROR or IGNORE, or None if not set
         """
         return self._on_duplicate_writes

     @on_duplicate_writes.setter
     def on_duplicate_writes(
         self,
         value: ClientWriteRequestOnDuplicateWrites | None,
     ) -> None:
         """
-        Set on_duplicate_writes
+        Set the policy for handling duplicate write operations.
+        
+        Args:
+            value: ERROR or IGNORE, or None to unset
         """
         self._on_duplicate_writes = value

Apply similar improvements to on_missing_deletes properties.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 75dbea2 and ceb366c.

📒 Files selected for processing (16)
  • .github/workflows/main.yaml (1 hunks)
  • Makefile (1 hunks)
  • config/clients/python/CHANGELOG.md.mustache (1 hunks)
  • config/clients/python/config.overrides.json (1 hunks)
  • config/clients/python/template/README_calling_api.mustache (1 hunks)
  • config/clients/python/template/model.mustache (1 hunks)
  • config/clients/python/template/src/client/client.py.mustache (4 hunks)
  • config/clients/python/template/src/client/models/__init__.py.mustache (2 hunks)
  • config/clients/python/template/src/client/models/write_conflict_opts.py.mustache (1 hunks)
  • config/clients/python/template/src/client/models/write_options.py.mustache (1 hunks)
  • config/clients/python/template/src/client/models/write_request.py.mustache (3 hunks)
  • config/clients/python/template/src/sync/client/client.py.mustache (4 hunks)
  • config/clients/python/template/test/api_test.py.mustache (2 hunks)
  • config/clients/python/template/test/client/client_test.py.mustache (24 hunks)
  • config/clients/python/template/test/sync/api_test.py.mustache (2 hunks)
  • config/clients/python/template/test/sync/client/client_test.py.mustache (21 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
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/python/template/src/client/client.py.mustache
  • config/clients/python/CHANGELOG.md.mustache
  • config/clients/python/template/test/sync/api_test.py.mustache
  • config/clients/python/template/test/api_test.py.mustache
  • config/clients/python/template/src/client/models/__init__.py.mustache
  • config/clients/python/template/src/sync/client/client.py.mustache
  • config/clients/python/template/test/sync/client/client_test.py.mustache
  • config/clients/python/template/src/client/models/write_request.py.mustache
  • config/clients/python/template/src/client/models/write_conflict_opts.py.mustache
  • config/clients/python/template/test/client/client_test.py.mustache
  • config/clients/python/template/model.mustache
  • config/clients/python/template/src/client/models/write_options.py.mustache
  • config/clients/python/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/python/template/src/client/client.py.mustache
  • config/clients/python/CHANGELOG.md.mustache
  • config/clients/python/template/test/sync/api_test.py.mustache
  • config/clients/python/template/test/api_test.py.mustache
  • config/clients/python/template/src/client/models/__init__.py.mustache
  • config/clients/python/config.overrides.json
  • config/clients/python/template/src/sync/client/client.py.mustache
  • config/clients/python/template/test/sync/client/client_test.py.mustache
  • config/clients/python/template/src/client/models/write_request.py.mustache
  • config/clients/python/template/src/client/models/write_conflict_opts.py.mustache
  • config/clients/python/template/test/client/client_test.py.mustache
  • config/clients/python/template/model.mustache
  • config/clients/python/template/src/client/models/write_options.py.mustache
  • config/clients/python/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
config/clients/*/config.overrides.json

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

config/clients/*/config.overrides.json: Always update the packageVersion in each language-specific config.overrides.json when making version changes
Maintain FOSSA compliance notice IDs in each language’s config.overrides.json

Files:

  • config/clients/python/config.overrides.json
config/{common/config.base.json,clients/*/config.overrides.json}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Ensure consistent versioning across base and language override configuration files to avoid version conflicts

Files:

  • config/clients/python/config.overrides.json
🧠 Learnings (2)
📚 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
📚 Learning: 2025-08-12T14:18:58.827Z
Learnt from: rhamzeh
PR: openfga/sdk-generator#573
File: config/clients/java/config.overrides.json:184-191
Timestamp: 2025-08-12T14:18:58.827Z
Learning: In the OpenFGA SDK generator, templates are inherited from the upstream OpenAPI Generator (openapitools/openapi-generator-cli). Only custom or modified templates need to be explicitly defined in the config overrides files. Base templates like `settings.gradle.mustache` are provided by the upstream generator and don't require override mappings.

Applied to files:

  • config/clients/python/config.overrides.json
🪛 Gitleaks (8.28.0)
.github/workflows/main.yaml

[high] 139-139: 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). (1)
  • GitHub Check: build-and-test-java-sdk
🔇 Additional comments (20)
.github/workflows/main.yaml (1)

139-139: Update OPEN_API_REF to v1.10.0 API for write conflict support.

The change updates the Python SDK test to use the same API reference already in use by Go, .NET, and Java SDKs. This aligns with the PR objective to support write conflict options, which were introduced in OpenFGA v1.10.0. The static analysis tool flags this as a potential API key, but this is a false positive—the value is a git commit SHA and poses no security risk.

Makefile (1)

3-3: LGTM!

The update properly sets the default OPEN_API_REF to v1.10.0 API, consistent with the workflow changes and aligned with coding guidelines. The OPEN_API_URL interpolation (line 4) will automatically resolve to the new OpenFGA API documentation. No additional configuration changes are needed.

config/clients/python/config.overrides.json (1)

156-163: Additions look correct; paths and destinations match existing pattern.

Both templates are mapped consistently under src/client/models and use SupportingFiles like peers. No hardcoded secrets found.

Please confirm:

  • packageVersion consistency across common/config.base.json and other language overrides, and whether this feature ships with 0.9.7 or a new tag.
  • FOSSA notice ID unchanged intentionally. As per coding guidelines.
config/clients/python/template/model.mustache (1)

120-124: Good switch to f-strings; preserves behavior and satisfies ruff.

Temporary vars (invalid_values/valid_values) improve readability; f-strings avoid .format. No semantic change.

Tiny gotcha: set(...) requires hashable elements; for enum containers that’s fine. If any enum container could carry non-hashables in future, consider list comprehensions instead of sets.

Also applies to: 129-133, 140-141

config/clients/python/template/src/sync/client/client.py.mustache (1)

444-491: Verification successful—conflict options correctly plumbed and signatures match.

All checks pass:

  • writes_tuple_keys and deletes_tuple_keys accept on_duplicate and on_missing as named parameters
  • ClientWriteRequestOnDuplicateWrites, ClientWriteRequestOnMissingDeletes, and ConflictOptions are properly re-exported
  • Enum values are correctly extracted and passed to tuple-key builders without leaking into kwargs
config/clients/python/template/test/api_test.py.mustache (2)

931-932: Verify default conflict behavior for deletes is correct.

Similar to the write test above, this test now expects "on_missing": "error" to always be present in delete requests.

The same verification concerns apply here:

  1. Confirm "error" is the correct default for on_missing
  2. Consider adding tests for explicit conflict option configuration on deletes

876-877: No changes required — test implementation is correct.

The test at lines 876-877 correctly expects "on_duplicate": "error" when conflict options are explicitly configured with ClientWriteRequestOnDuplicateWrites.ERROR. The codebase shows:

  • on_duplicate is conditionally included only when conflict options are explicitly set (not a hardcoded default)
  • When no conflict options are provided, the field is omitted entirely
  • Both "error" and "ignore" values are properly tested elsewhere in the same test file (e.g., line 4011)

The implementation properly handles the conditional inclusion of conflict options in the write request payload.

config/clients/python/template/test/sync/api_test.py.mustache (2)

943-944: Consistent conflict option handling in sync client tests.

The synchronous client tests correctly mirror the async version by expecting "on_duplicate": "error" in write requests.

The same verification and test coverage considerations mentioned for the async tests (lines 876-877 in api_test.py.mustache) apply here.


998-999: Consistent conflict option handling for deletes in sync client tests.

The synchronous client tests correctly mirror the async version by expecting "on_missing": "error" in delete requests.

The same verification considerations mentioned for the async tests (lines 931-932 in api_test.py.mustache) apply here.

config/clients/python/template/test/sync/client/client_test.py.mustache (4)

1017-1024: LGTM! Consistent default conflict options added to write/delete tests.

The updates correctly add default conflict option assertions to existing tests, ensuring writes default to "on_duplicate": "error" and deletes default to "on_missing": "error". This maintains backward compatibility testing while supporting the new feature.

Also applies to: 1063-1070


3963-4017: LGTM! Conflict options test implementation is correct.

The test properly verifies that ConflictOptions with on_duplicate_writes=IGNORE results in "on_duplicate": "ignore" in the request body.


4020-4075: LGTM! Missing deletes conflict option test is correct.

The test properly verifies that ConflictOptions with on_missing_deletes=IGNORE results in "on_missing": "ignore" in the request body.


4078-4150: LGTM! Combined conflict options test is thorough.

The test correctly verifies that both conflict options can be used simultaneously, with writes including "on_duplicate": "ignore" and deletes including "on_missing": "ignore".

config/clients/python/template/src/client/client.py.mustache (3)

115-122: Good utility for extracting conflict options

Helper cleanly isolates conflict extraction. No issues.


470-485: Conflict enums correctly mapped to request payloads

Passing on_duplicate/on_missing only when provided preserves previous defaults. Looks solid.

If the generator’s model defaults ever stop serializing "error" when omitted, tests relying on explicit keys will fail. Please keep an eye on that in CI.


481-485: Follow-up if you keep method rename in ClientWriteRequest

If you introduce builder methods in ClientWriteRequest for tuple keys (see separate comment), update these calls accordingly. Proposed names: build_writes_tuple_keys(...), build_deletes_tuple_keys(...).

config/clients/python/template/src/client/models/__init__.py.mustache (2)

18-24: Public exports updated correctly

New conflict types and ClientWriteOptions are properly re-exported.


41-45: all looks consistent

Surface area is coherent; no duplicates found.

config/clients/python/template/src/client/models/write_request.py.mustache (1)

1-83: Review comment is incorrect—call sites already pass conflict parameters

The call sites in src/sync/client/client.py.mustache and src/client/client.py.mustache already invoke the builder methods with the required parameters (on_duplicate and on_missing), making the requested updates unnecessary.

Likely an incorrect or invalid review comment.

config/clients/python/template/src/client/models/write_conflict_opts.py.mustache (1)

1-13: LGTM! Clean enum definitions.

The string enum pattern is correctly implemented, and the use of both str and Enum inheritance allows these enums to be used as strings while maintaining type safety.

SoulPancake and others added 3 commits October 17, 2025 23:03
@dyeam0 dyeam0 linked an issue Oct 23, 2025 that may be closed by this pull request
1 task
@rhamzeh rhamzeh added this pull request to the merge queue Oct 27, 2025
Merged via the queue into main with commit cface85 Oct 27, 2025
15 checks passed
@rhamzeh rhamzeh deleted the feat/py-sdk/support-write-conflict-options branch October 27, 2025 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[All SDKs] Add on_duplicate/on_missing options support for Write operations

4 participants