fix: add transactions and isTransactionsEnabled to ClientWriteOptions#352
fix: add transactions and isTransactionsEnabled to ClientWriteOptions#352curfew-marathon wants to merge 5 commits into
Conversation
…ansactionsEnabled
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Walkthrough
ClientWriteOptions Transaction API
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 #352 +/- ##
============================================
- Coverage 38.68% 38.67% -0.01%
- Complexity 1288 1289 +1
============================================
Files 198 198
Lines 7704 7707 +3
Branches 900 900
============================================
+ Hits 2980 2981 +1
- Misses 4576 4579 +3
+ Partials 148 147 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR improves the ClientWriteOptions public API by introducing a positively framed transactions toggle and boolean getter, while deprecating the existing double-negative disableTransactions setter/getter to reduce ambiguity at call sites and provide a clearer migration path.
Changes:
- Added
transactions(boolean enabled)as the positively framed replacement fordisableTransactions(boolean). - Added
isTransactionsEnabled()as the positively framed replacement fordisableTransactions(). - Deprecated both legacy methods with Javadoc describing the migration and inverted semantics.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Siddhant-K-code
left a comment
Siddhant-K-code
left a comment
There was a problem hiding this comment.
Two follow-ups I found:
-
This PR deprecates
disableTransactions(...), but the public guidance still teaches it. The README write examples still use.disableTransactions(false)/.disableTransactions(true), and theOpenFgaClient.writeJavadocs still describe mode selection withoptions.disableTransactions(). Since this PR introduces the replacement API, can we update those examples/docs totransactions(true/false)andisTransactionsEnabled()so users do not copy newly deprecated methods? -
In
ClientWriteOptions, thetransactionChunkSizeJavadoc says the value “must be greater than zero,” but the setter accepts anyintandgetTransactionChunkSize()silently falls back to1when the stored value is<= 0. Could we either reject non-positive values there or document the actual fallback behavior?
Roll out transactions/isTransactionsEnabled alongside the existing disableTransactions methods without marking them deprecated yet.
curfew-marathon
changed the title
![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
🧹 Nitpick comments (1)
src/main/java/dev/openfga/sdk/api/configuration/ClientWriteOptions.java (1)
71-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd direct tests for the new/legacy inversion contract.
Codecov already flags this block as uncovered, and this API is easy to regress because two setters now mutate the same state with opposite semantics. A focused test should cover the default state plus
transactions(false) -> disableTransactions() == trueanddisableTransactions(true) -> isTransactionsEnabled() == false.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/dev/openfga/sdk/api/configuration/ClientWriteOptions.java` around lines 71 - 103, Add focused tests for the opposite-semantic setters in ClientWriteOptions to lock down the inversion contract and default behavior. Cover the default state, then verify that calling transactions(false) makes disableTransactions() return true, and that calling disableTransactions(true) makes isTransactionsEnabled() return false. Use the ClientWriteOptions methods transactions(boolean), isTransactionsEnabled(), and disableTransactions(boolean/()) directly so the shared state mutation is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/dev/openfga/sdk/api/configuration/ClientWriteOptions.java`:
- Around line 112-129: The ClientWriteOptions transactionChunkSize contract is
inconsistent: transactionChunkSize(int) currently allows zero/negative values
and getTransactionChunkSize() masks them by returning 1. Update the setter to
enforce the documented “greater than zero” requirement by rejecting invalid
values (or, if you prefer the fallback behavior, relax the Javadoc to match it)
and keep getTransactionChunkSize() aligned with that public contract.
---
Nitpick comments:
In `@src/main/java/dev/openfga/sdk/api/configuration/ClientWriteOptions.java`:
- Around line 71-103: Add focused tests for the opposite-semantic setters in
ClientWriteOptions to lock down the inversion contract and default behavior.
Cover the default state, then verify that calling transactions(false) makes
disableTransactions() return true, and that calling disableTransactions(true)
makes isTransactionsEnabled() return false. Use the ClientWriteOptions methods
transactions(boolean), isTransactionsEnabled(), and
disableTransactions(boolean/()) directly so the shared state mutation is
exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bbb3a37f-c598-4add-850a-bc2cfd286ff0
📒 Files selected for processing (1)
src/main/java/dev/openfga/sdk/api/configuration/ClientWriteOptions.java
| * @param transactionChunkSize the chunk size; must be greater than zero | ||
| * @return this {@code ClientWriteOptions} instance for method chaining | ||
| * @see #transactions(boolean) | ||
| */ | ||
| public ClientWriteOptions transactionChunkSize(int transactionChunkSize) { | ||
| this.transactionChunkSize = transactionChunkSize; | ||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Returns the chunk size for non-transactional writes. | ||
| * | ||
| * <p>Defaults to {@code 1} if not explicitly set. | ||
| * | ||
| * @return the configured chunk size, or {@code 1} if none was set | ||
| */ | ||
| public int getTransactionChunkSize() { | ||
| return transactionChunkSize > 0 ? transactionChunkSize : 1; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Enforce the chunk-size contract or relax the Javadoc.
Line 112 says the value “must be greater than zero”, but transactionChunkSize(int) accepts 0/negative values and getTransactionChunkSize() silently converts them to 1. That makes the public contract inaccurate and hides misconfiguration.
Suggested fix
public ClientWriteOptions transactionChunkSize(int transactionChunkSize) {
+ if (transactionChunkSize <= 0) {
+ throw new IllegalArgumentException("transactionChunkSize must be greater than zero");
+ }
this.transactionChunkSize = transactionChunkSize;
return this;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * @param transactionChunkSize the chunk size; must be greater than zero | |
| * @return this {@code ClientWriteOptions} instance for method chaining | |
| * @see #transactions(boolean) | |
| */ | |
| public ClientWriteOptions transactionChunkSize(int transactionChunkSize) { | |
| this.transactionChunkSize = transactionChunkSize; | |
| return this; | |
| } | |
| /** | |
| * Returns the chunk size for non-transactional writes. | |
| * | |
| * <p>Defaults to {@code 1} if not explicitly set. | |
| * | |
| * @return the configured chunk size, or {@code 1} if none was set | |
| */ | |
| public int getTransactionChunkSize() { | |
| return transactionChunkSize > 0 ? transactionChunkSize : 1; | |
| * `@param` transactionChunkSize the chunk size; must be greater than zero | |
| * `@return` this {`@code` ClientWriteOptions} instance for method chaining | |
| * `@see` `#transactions`(boolean) | |
| */ | |
| public ClientWriteOptions transactionChunkSize(int transactionChunkSize) { | |
| if (transactionChunkSize <= 0) { | |
| throw new IllegalArgumentException("transactionChunkSize must be greater than zero"); | |
| } | |
| this.transactionChunkSize = transactionChunkSize; | |
| return this; | |
| } | |
| /** | |
| * Returns the chunk size for non-transactional writes. | |
| * | |
| * <p>Defaults to {`@code` 1} if not explicitly set. | |
| * | |
| * `@return` the configured chunk size, or {`@code` 1} if none was set | |
| */ | |
| public int getTransactionChunkSize() { | |
| return transactionChunkSize > 0 ? transactionChunkSize : 1; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/dev/openfga/sdk/api/configuration/ClientWriteOptions.java`
around lines 112 - 129, The ClientWriteOptions transactionChunkSize contract is
inconsistent: transactionChunkSize(int) currently allows zero/negative values
and getTransactionChunkSize() masks them by returning 1. Update the setter to
enforce the documented “greater than zero” requirement by rejecting invalid
values (or, if you prefer the fallback behavior, relax the Javadoc to match it)
and keep getTransactionChunkSize() aligned with that public contract.
Summary
ClientWriteOptions.disableTransactions(boolean)and its getterdisableTransactions()suffer from a double-negative API design that makes call sites ambiguous — it is not obvious whether to passtrueorfalseto achieve a given outcome, and the getter name reads confusingly in conditionals.This PR introduces a cleaner API alongside the existing methods, maintaining full backwards compatibility.
New API
transactions(boolean enabled)A positively-framed alternative to
disableTransactions(boolean)— passtrueto enable transactions (the default),falseto disable them:isTransactionsEnabled()A positively-framed alternative to
disableTransactions()(the getter), consistent with standard Java boolean getter naming conventions:Backwards compatibility
No existing call sites are broken. Both existing methods remain fully functional and are kept as-is alongside the new API.
Summary by CodeRabbit
New Features
Bug Fixes