feat: add support for write conflict settings #234
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 WalkthroughAdds conflict-handling options for write/delete requests: on_duplicate (writes) and on_missing (deletes). Threads these options through client options to request models, updates docs and Javadocs, adds enums and serialization logic, extends helpers/overloads, and updates tests. Integration tests now use OpenFGA v1.10.2. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant Client as OpenFgaClient
participant API as OpenFgaApi
participant Svc as OpenFGA Server
App->>Client: write(..., options{onDuplicate?, onMissing?})
Client->>Client: Build WriteRequestWrites/Deletes\n(apply on_duplicate/on_missing if provided)
Client->>API: write(request)
API->>Svc: POST /write {writes{..., on_duplicate}, deletes{..., on_missing}}
alt Success
Svc-->>API: 200 OK
API-->>Client: Response
Client-->>App: Return success
else Error (conflict/precedence)
Svc-->>API: 4xx (transaction rolled back)
API-->>Client: Error
Client-->>App: Propagate error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #234 +/- ##
============================================
+ Coverage 35.97% 36.44% +0.46%
- Complexity 1116 1140 +24
============================================
Files 187 187
Lines 7096 7170 +74
Branches 806 822 +16
============================================
+ Hits 2553 2613 +60
- Misses 4438 4452 +14
Partials 105 105 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ewanharris
force-pushed
the
feat/write-conflict-options
branch
from
c861b1e to
57be69e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (12)
docs/WriteRequestDeletes.md (1)
11-21: Fix anchor and minor formatting in docs.The fragment [#OnMissingEnum] is likely invalid (MD051). GitHub generates “## Enum: OnMissingEnum” as #enum-onmissingenum. Also, prefer plain quotes over HTML entities. Optionally note the default.
Apply:
-|**onMissing** | [**OnMissingEnum**](#OnMissingEnum) | On 'error', the API returns an error when deleting a tuple that does not exist. On 'ignore', deletes of non-existent tuples are treated as no-ops. | [optional] | +|**onMissing** | [**OnMissingEnum**](#enum-onmissingenum) | On 'error', the API returns an error when deleting a tuple that does not exist. On 'ignore', deletes of non-existent tuples are treated as no-ops. | [optional] | - | ERROR | "error" | - | IGNORE | "ignore" | - | UNKNOWN_DEFAULT_OPEN_API | "unknown_default_open_api" | + | ERROR | "error" | + | IGNORE | "ignore" | + | UNKNOWN_DEFAULT_OPEN_API | "unknown_default_open_api" |Optional:
- |**onMissing** | ... | [optional] | + |**onMissing** | ... | [optional] (Default: ERROR) |Based on static analysis hints
src/test/java/dev/openfga/sdk/api/client/model/ClientTupleKeyWithoutConditionTest.java (1)
1-82: Good coverage of onMissing defaulting and explicit values.Tests read clearly and validate key paths.
Consider a parameterized test to reduce duplication across the three explicit/default cases.
src/test/java/dev/openfga/sdk/api/client/model/ClientTupleKeyTest.java (1)
1-82: OnDuplicate behavior covered well.Assertions for null/explicit/default are correct.
You can parameterize these scenarios to DRY the setup.
src/test/java/dev/openfga/sdk/api/client/OpenFgaClientTest.java (1)
1598-1639: Strengthen verification in non-transaction conflict-options test.Currently verifies only total POST count. Mirror earlier tests by asserting one request used writeBody and one used deleteBody (and headers), reducing false positives.
- // Then - mockHttpClient.verify().post(postPath).called(2); // One for writes, one for deletes + // Then + mockHttpClient + .verify() + .post(postPath) + .withBody(is(writeBody)) + .withHeader(CLIENT_METHOD_HEADER, "Write") + .withHeader(CLIENT_BULK_REQUEST_ID_HEADER, anyValidUUID()) + .called(1); + mockHttpClient + .verify() + .post(postPath) + .withBody(is(deleteBody)) + .withHeader(CLIENT_METHOD_HEADER, "Write") + .withHeader(CLIENT_BULK_REQUEST_ID_HEADER, anyValidUUID()) + .called(1);src/main/java/dev/openfga/sdk/api/configuration/ClientWriteTuplesOptions.java (1)
15-39: onDuplicate option wiring looks good; add a short Javadoc about defaultsMinor polish: document that null omits the field (server defaults to "error"). Optionally note that UNKNOWN_DEFAULT_OPEN_API should not be set by callers.
public ClientWriteTuplesOptions onDuplicate(WriteRequestWrites.OnDuplicateEnum onDuplicate) { this.onDuplicate = onDuplicate; return this; } + /** + * Controls behavior when writing a tuple that already exists. + * If null, the field is omitted (server defaults to "error"). + */ public WriteRequestWrites.OnDuplicateEnum getOnDuplicate() { return onDuplicate; }src/main/java/dev/openfga/sdk/api/client/model/ClientTupleKey.java (1)
48-60: Guard against sending UNKNOWN_DEFAULT_OPEN_APISkip setting onDuplicate when it's UNKNOWN_DEFAULT_OPEN_API to avoid emitting an unrecognized value.
- if (onDuplicate != null) { - writes.onDuplicate(onDuplicate); - } + if (onDuplicate != null + && onDuplicate != WriteRequestWrites.OnDuplicateEnum.UNKNOWN_DEFAULT_OPEN_API) { + writes.onDuplicate(onDuplicate); + }src/main/java/dev/openfga/sdk/api/client/model/ClientTupleKeyWithoutCondition.java (1)
30-43: Also skip UNKNOWN_DEFAULT_OPEN_API for onMissingMirror the writes guard to prevent sending an unknown enum value.
- if (onMissing != null) { - deletes.onMissing(onMissing); - } + if (onMissing != null + && onMissing != WriteRequestDeletes.OnMissingEnum.UNKNOWN_DEFAULT_OPEN_API) { + deletes.onMissing(onMissing); + }src/main/java/dev/openfga/sdk/api/configuration/ClientDeleteTuplesOptions.java (1)
15-39: onMissing option is wired correctly; consider brief JavadocNote that null omits the field (server defaults to "error"). Helps users choose behavior.
public ClientDeleteTuplesOptions onMissing(WriteRequestDeletes.OnMissingEnum onMissing) { this.onMissing = onMissing; return this; } + /** + * Controls behavior when deleting a tuple that does not exist. + * If null, the field is omitted (server defaults to "error"). + */ public WriteRequestDeletes.OnMissingEnum getOnMissing() { return onMissing; }docs/OpenFgaApi.md (1)
2306-2306: Docs read well; minor grammar + clarify defaults
- Replace “an user” with “a user”.
- Add sentence: “If omitted, both options default to "error".”
-The Write API will transactionally update the tuples for a certain store. Tuples and type definitions allow OpenFGA to determine whether a relationship exists between an object and an user. +The Write API will transactionally update the tuples for a certain store. Tuples and type definitions allow OpenFGA to determine whether a relationship exists between an object and a user. +If omitted, both on_duplicate and on_missing default to "error".Also applies to: 2381-2381
src/test/java/dev/openfga/sdk/api/OpenFgaApiTest.java (1)
1431-1462: Combined conflict options exercised end-to-endCovers simultaneous on_duplicate/on_missing. Good integration signal.
To cut duplication/noise across similar tests, consider a small helper to build WriteRequest with (writes|deletes) and a parameterized JUnit test over {IGNORE, ERROR}.
src/main/java/dev/openfga/sdk/api/model/WriteRequestDeletes.java (1)
220-228: Prefer using enum getValue() for URL encodingMinor clarity tweak: avoid relying on toString(); encode the JSON value directly.
Apply:
- URLEncoder.encode(String.valueOf(getOnMissing()), StandardCharsets.UTF_8) + URLEncoder.encode(getOnMissing().getValue(), StandardCharsets.UTF_8)src/main/java/dev/openfga/sdk/api/model/WriteRequestWrites.java (1)
220-228: Prefer using enum getValue() for URL encodingMirror the deletes suggestion for consistency/clarity.
Apply:
- URLEncoder.encode(String.valueOf(getOnDuplicate()), StandardCharsets.UTF_8) + URLEncoder.encode(getOnDuplicate().getValue(), StandardCharsets.UTF_8)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
README.md(1 hunks)docs/OpenFgaApi.md(2 hunks)docs/WriteRequestDeletes.md(1 hunks)docs/WriteRequestWrites.md(1 hunks)src/main/java/dev/openfga/sdk/api/OpenFgaApi.java(2 hunks)src/main/java/dev/openfga/sdk/api/client/OpenFgaClient.java(3 hunks)src/main/java/dev/openfga/sdk/api/client/model/ClientTupleKey.java(1 hunks)src/main/java/dev/openfga/sdk/api/client/model/ClientTupleKeyWithoutCondition.java(1 hunks)src/main/java/dev/openfga/sdk/api/configuration/ClientDeleteTuplesOptions.java(2 hunks)src/main/java/dev/openfga/sdk/api/configuration/ClientWriteOptions.java(2 hunks)src/main/java/dev/openfga/sdk/api/configuration/ClientWriteTuplesOptions.java(2 hunks)src/main/java/dev/openfga/sdk/api/model/WriteRequestDeletes.java(5 hunks)src/main/java/dev/openfga/sdk/api/model/WriteRequestWrites.java(5 hunks)src/test-integration/java/dev/openfga/sdk/api/OpenFgaApiIntegrationTest.java(1 hunks)src/test-integration/java/dev/openfga/sdk/api/client/OpenFgaClientIntegrationTest.java(1 hunks)src/test-integration/java/dev/openfga/sdk/example/ExampleTest.java(1 hunks)src/test/java/dev/openfga/sdk/api/OpenFgaApiTest.java(5 hunks)src/test/java/dev/openfga/sdk/api/client/OpenFgaClientHeadersTest.java(2 hunks)src/test/java/dev/openfga/sdk/api/client/OpenFgaClientTest.java(9 hunks)src/test/java/dev/openfga/sdk/api/client/model/ClientTupleKeyTest.java(1 hunks)src/test/java/dev/openfga/sdk/api/client/model/ClientTupleKeyWithoutConditionTest.java(1 hunks)src/test/java/dev/openfga/sdk/api/model/WriteRequestDeletesTest.java(1 hunks)src/test/java/dev/openfga/sdk/api/model/WriteRequestWritesTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/test/java/dev/openfga/sdk/api/client/model/ClientTupleKeyWithoutConditionTest.java (1)
src/main/java/dev/openfga/sdk/api/client/model/ClientTupleKeyWithoutCondition.java (1)
ClientTupleKeyWithoutCondition(20-97)
src/main/java/dev/openfga/sdk/api/model/WriteRequestDeletes.java (1)
src/main/java/dev/openfga/sdk/api/model/WriteRequestWrites.java (1)
JsonPropertyOrder(30-232)
src/test/java/dev/openfga/sdk/api/client/model/ClientTupleKeyTest.java (1)
src/main/java/dev/openfga/sdk/api/client/model/ClientTupleKey.java (1)
ClientTupleKey(21-81)
src/main/java/dev/openfga/sdk/api/client/OpenFgaClient.java (2)
src/main/java/dev/openfga/sdk/api/client/model/ClientTupleKey.java (1)
ClientTupleKey(21-81)src/main/java/dev/openfga/sdk/api/client/model/ClientTupleKeyWithoutCondition.java (1)
ClientTupleKeyWithoutCondition(20-97)
src/main/java/dev/openfga/sdk/api/model/WriteRequestWrites.java (1)
src/main/java/dev/openfga/sdk/api/model/WriteRequestDeletes.java (1)
JsonPropertyOrder(30-232)
🪛 LanguageTool
docs/WriteRequestDeletes.md
[grammar] ~17-~17: There might be a mistake here.
Context: ...## Enum: OnMissingEnum | Name | Value | |---- | -----| | ERROR | "error&quo...
(QB_NEW_EN)
[grammar] ~18-~18: There might be a mistake here.
Context: ...ingEnum | Name | Value | |---- | -----| | ERROR | "error" | | IGNORE |...
(QB_NEW_EN)
[grammar] ~19-~19: There might be a mistake here.
Context: ...- | -----| | ERROR | "error" | | IGNORE | "ignore" | | UNKNOW...
(QB_NEW_EN)
[grammar] ~20-~20: There might be a mistake here.
Context: ..." | | IGNORE | "ignore" | | UNKNOWN_DEFAULT_OPEN_API | "unkno...
(QB_NEW_EN)
docs/WriteRequestWrites.md
[grammar] ~17-~17: There might be a mistake here.
Context: ... Enum: OnDuplicateEnum | Name | Value | |---- | -----| | ERROR | "error&quo...
(QB_NEW_EN)
[grammar] ~18-~18: There might be a mistake here.
Context: ...ateEnum | Name | Value | |---- | -----| | ERROR | "error" | | IGNORE |...
(QB_NEW_EN)
[grammar] ~19-~19: There might be a mistake here.
Context: ...- | -----| | ERROR | "error" | | IGNORE | "ignore" | | UNKNOW...
(QB_NEW_EN)
[grammar] ~20-~20: There might be a mistake here.
Context: ..." | | IGNORE | "ignore" | | UNKNOWN_DEFAULT_OPEN_API | "unkno...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.18.1)
docs/WriteRequestDeletes.md
11-11: Link fragments should be valid
(MD051, link-fragments)
docs/WriteRequestWrites.md
11-11: Link fragments should be valid
(MD051, link-fragments)
🔇 Additional comments (26)
src/test-integration/java/dev/openfga/sdk/example/ExampleTest.java (1)
11-11: LGTM! Container version updated to support new conflict options.The container version update to v1.10.2 aligns with the PR's requirement to support the write conflict options introduced in OpenFGA v1.10.0.
src/main/java/dev/openfga/sdk/api/configuration/ClientWriteOptions.java (2)
15-16: LGTM! Imports added for new conflict handling enums.
24-25: LGTM! Conflict options added with proper fluent API pattern.The
onDuplicateandonMissingfields with their corresponding fluent setters and getters follow the existing patterns in this class. The fields default tonull, which allows the SDK to omit these options when not explicitly set, letting the server apply its defaults.Also applies to: 64-80
docs/WriteRequestWrites.md (2)
11-11: Verify the enum anchor link.The
OnDuplicateEnumreference on this line should link to the enum section below (line 15). Ensure the anchor link works correctly in the rendered documentation.Based on static analysis hints.
11-21: LGTM! Clear documentation for the new onDuplicate property.The documentation clearly explains:
- The optional nature of the property
- The behavior for each enum value (error vs. ignore)
- The available enum values including the fallback (UNKNOWN_DEFAULT_OPEN_API)
src/main/java/dev/openfga/sdk/api/OpenFgaApi.java (1)
909-909: LGTM! Comprehensive documentation for write conflict handling.The updated Javadoc provides clear guidance on:
- Default non-idempotent behavior
- How to enable idempotency with
on_duplicate: "ignore"andon_missing: "ignore"- Precedence rules when mixing ignore and error operations
- Transaction rollback behavior on failures
The examples clearly demonstrate the usage patterns.
Also applies to: 922-922
README.md (2)
644-657: LGTM! Clear introduction to conflict handling options.The documentation effectively explains:
- The purpose of each option (onDuplicate for writes, onMissing for deletes)
- Version requirement (OpenFGA v1.10.0+)
- Available enum values with their behavior
- Default behavior (ERROR)
658-712: LGTM! Comprehensive usage examples for all write methods.The three examples demonstrate conflict handling with:
write()method - showing both onDuplicate and onMissing togetherwriteTuples()convenience method - showing onDuplicatedeleteTuples()convenience method - showing onMissingThe code examples follow proper Java syntax and fluent builder patterns.
src/test/java/dev/openfga/sdk/api/model/WriteRequestDeletesTest.java (1)
1-36: LGTM! Comprehensive unit tests for OnMissingEnum.The test suite provides good coverage:
- Valid enum values map correctly (ERROR, IGNORE)
- Unknown values safely fall back to UNKNOWN_DEFAULT_OPEN_API
- Null input is handled gracefully
The test structure follows JUnit 5 best practices with descriptive test names and clear assertions.
src/test/java/dev/openfga/sdk/api/client/OpenFgaClientHeadersTest.java (2)
402-403: Write payload now asserts default on_duplicate — LGTM.Matches the new default behavior. Consider updating writeNonTransaction_withHeaders in this file to also include "on_duplicate":"error" for consistency with non‑transaction tests elsewhere.
Example patch to align that test’s expectedBody:
- String expectedBody = String.format( - "{\"writes\":{\"tuple_keys\":[{\"user\":\"%s\",\"relation\":\"%s\",\"object\":\"%s\",\"condition\":null}]},\"deletes\":null,\"authorization_model_id\":\"%s\"}", - DEFAULT_USER, DEFAULT_RELATION, DEFAULT_OBJECT, DEFAULT_AUTH_MODEL_ID); + String expectedBody = String.format( + "{\"writes\":{\"tuple_keys\":[{\"user\":\"%s\",\"relation\":\"%s\",\"object\":\"%s\",\"condition\":null}],\"on_duplicate\":\"error\"},\"deletes\":null,\"authorization_model_id\":\"%s\"}", + DEFAULT_USER, DEFAULT_RELATION, DEFAULT_OBJECT, DEFAULT_AUTH_MODEL_ID);
908-909: Multiple overrides test updated for on_duplicate — LGTM.Expectation reflects default on_duplicate in writes.
src/test/java/dev/openfga/sdk/api/client/OpenFgaClientTest.java (1)
1146-1147: Assertion updates for conflict options — LGTM.Expected payloads consistently include on_duplicate/on_missing defaults and option-driven overrides across transaction and non-transaction paths.
Also applies to: 1178-1179, 1207-1212, 1216-1221, 1357-1365, 1397-1405, 1436-1438, 1459-1461, 1475-1496, 1498-1519, 1521-1549, 1550-1573, 1575-1596
src/main/java/dev/openfga/sdk/api/client/OpenFgaClient.java (1)
487-495: All onDuplicate/onMissing variants are covered and call sites verifiedTests validate null/default, IGNORE, and ERROR for both writes and deletes; no further changes needed.
src/test/java/dev/openfga/sdk/api/model/WriteRequestWritesTest.java (1)
1-36: Remove redundant test suggestions; these tests already exist
WriteRequestDeletesTest.java covers OnMissingEnum.fromValue cases, and ClientTupleKeyTest.java/ClientTupleKeyWithoutConditionTest.java include asWriteRequestWrites and asWriteRequestDeletes builder tests.Likely an incorrect or invalid review comment.
src/test/java/dev/openfga/sdk/api/OpenFgaApiTest.java (8)
1101-1101: Good: default on_duplicate asserted in writes payloadAsserting explicit "on_duplicate":"error" matches the new default in WriteRequestWrites.
1127-1127: Good: default on_missing asserted in deletes payloadExplicit "on_missing":"error" aligns with WriteRequestDeletes default.
1150-1151: Writes + condition context still include on_duplicate defaultConfirms serialization with conditions and default conflict option. Looks correct.
1182-1184: Modeled context serialization + on_duplicate default verifiedCovers complex context shape and default "error". Solid.
1323-1349: onDuplicate=IGNORE test is precisePayload and builder usage (WriteRequestWrites.OnDuplicateEnum.IGNORE) are correct.
1350-1376: onDuplicate=ERROR test matches default behaviorExplicit ERROR path validated; mirrors ignore test.
1377-1403: onMissing=IGNORE test is preciseDelete-path conflict option wired and asserted correctly.
1404-1429: onMissing=ERROR test matches default behaviorExplicit ERROR path validated for deletes.
src/main/java/dev/openfga/sdk/api/model/WriteRequestDeletes.java (2)
35-74: Enum with UNKNOWN fallback is appropriateOnMissingEnum with @JsonCreator/@jsonvalue and UNKNOWN_DEFAULT_OPEN_API safeguards deserialization.
107-127: onMissing field + JSON wiring look correctDefault ERROR, property order, getters/setters, equals/hashCode updates are consistent with writes.
src/main/java/dev/openfga/sdk/api/model/WriteRequestWrites.java (2)
35-74: Enum with UNKNOWN fallback is appropriateOnDuplicateEnum with @JsonCreator/@jsonvalue and UNKNOWN_DEFAULT_OPEN_API ensures forward-compatible parsing.
107-127: onDuplicate field + JSON wiring look correctDefault ERROR, property order, getters/setters, equals/hashCode updates align with expectations.
src/test/java/dev/openfga/sdk/api/client/model/ClientTupleKeyTest.java
Outdated
Show resolved
Hide resolved
In openfga/sdk-generator#610 it states
Any reason we can't do that here? |
|
I think with some edits to the underlying template we could, but it would impact other properties as the code is how the existing enums are rendered today, it's just that The OpenAPI spec declares the properties to have |
Yes it does seem we have some inconsistency in the API between consistency "unset" value and here... @rhamzeh do you have any insight into if/why the Java SDK handles optional params differently than other SDKs? In general I think if params are optional in the API and not specified by the SDK consumer, we shouldn't send them, but obviously should avoid or at least be intentional about introducing any breaking changes to do so. |
This is another example of me putting things in the issue which are not correct (I had assumed that it was implemented the same as consistency, but here it's different as Ewan mentioned) - in this case the SDKs can match the API as the API does state that error is the default - any change on that would be a breaking change on the API and the SDK. So to match that the SDKs send error as the default (as that's what the API definition states). I think we should modify the original issue to remove that statement. @jimmyjames the Java SDK isn't being inconsistent with the other SDKs here - they'll all have the same behaviour (I think) - the default defined by the API spec will get set (Just FYI coderabbit called that out on the Go SDK too, and I had resolved it as working as intended. If we decide something different here - let's go back and update that too.) |
|
Thanks for clarifying @rhamzeh! I missed the actual go implementation piece (when I asked copilot about the go SDK implementation, it must have taken the shortcut and just looked up the issue description, and not the actual code ;))
I updated the issue to specify that the default value should be set if not specified by the user. |
src/test/java/dev/openfga/sdk/api/client/model/ClientTupleKeyTest.java
Outdated
Show resolved
Hide resolved
ewanharris
force-pushed
the
feat/write-conflict-options
branch
from
57be69e to
690d07b
Compare
4 participants
Description
Adds support for the write conflict options introduced in OpenFGA v1.10.0
Adds
onDuplicateandonMissingmethods to theClientWriteOptions/ClientWriteTuplesOptions/ClientDeleteTuplesOptionsthat can be used to control the behaviour for when a tuple already exists on write (onDuplicateor does not exist on delete (onMissing).The SDK defaults to sending
errorfor these values (see test updates e.g here) if not provided as that is the default in the API/default behaviour in the server, this is different to theconsistencyoption that has a default value ofUNSPECIFIEDrather thanMINIMIZE_LATENCYto allow differentiating between "not sent by client" vs "set to default value by client".References
Generated from https://github.com/openfga/sdk-generator/tree/feat/java-write-options using
OPEN_API_REF=0ac19aac54f21f3c78970126b84b4c69c6e3b9a2 make build-client-java(and some edits to remove thelistStoreschanges.Review Checklist
mainSummary by CodeRabbit
New Features
Documentation
Tests
Chores