Fix transaction state management across adapters#817
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdjusted transaction handling: removed two resets of the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Adapter
participant PostgresDB
participant MongoDB
Caller->>Adapter: withTransaction(callback)
alt Adapter supports nested and inTransaction > 0 (Postgres)
Adapter->>PostgresDB: SAVEPOINT transaction{n}
Adapter->>Adapter: inTransaction++
Caller->>Adapter: execute callback
alt callback commits
Adapter->>PostgresDB: RELEASE SAVEPOINT / commit
Adapter->>Adapter: inTransaction--
else callback rolls back
Adapter->>PostgresDB: ROLLBACK TO SAVEPOINT transaction{n-1}
Adapter->>Adapter: inTransaction--
end
else Adapter is Mongo and inTransaction > 0
Adapter->>MongoDB: execute callback directly (no nested support)
else Adapter not replica set (Mongo)
Adapter->>Caller: execute callback directly
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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 (2)
tests/e2e/Adapter/Scopes/GeneralTests.php (2)
857-858: Consider usingfinallyblocks for test cleanup to avoid orphaned collections.Both
testTransactionStateAfterKnownExceptionandtestNestedTransactionStatecreate collections and clean them up at the end, but if an assertion fails mid-test,deleteCollectionis never reached. Wrapping cleanup infinally(or usingsetUp/tearDown) would prevent leftover collections from affecting subsequent test runs.Also applies to: 937-938
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/Adapter/Scopes/GeneralTests.php` around lines 857 - 858, Tests testTransactionStateAfterKnownException and testNestedTransactionState create collections via Database::createCollection and remove them with Database::deleteCollection but do not guarantee cleanup if assertions fail; wrap the create/delete logic in a try/finally or move cleanup into the test class tearDown to ensure deleteCollection always runs, i.e., perform createCollection before the try, run assertions inside try, and call deleteCollection inside finally (or register collection names in setUp and remove them in tearDown) so orphaned collections cannot remain after failures.
918-919: Hardcoded retry count couples test to internal implementation.The expected
3attempts is hardcoded at line 404 ofsrc/Database/Adapter.phpas$retries = 2(1 initial + 2 retries). If that default changes, this test fails without clear cause. Consider extracting the retry count to a class constant in Adapter and reference it in the test, or add a comment documenting the dependency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/Adapter/Scopes/GeneralTests.php` around lines 918 - 919, The test is hardcoded to expect 3 attempts which couples it to Adapter's internal $retries value; add a public class constant (e.g., Adapter::DEFAULT_RETRIES) or a public static getter (e.g., Adapter::getDefaultRetries()) in the Adapter class where $retries is defined, set it to the current default (2), and update the test in GeneralTests.php to compute expected attempts as Adapter::DEFAULT_RETRIES + 1 (initial attempt + retries) instead of the literal 3 so the test follows the implementation default.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/Adapter/Scopes/GeneralTests.php`:
- Around line 948-974: The Mongo adapter's withTransaction implementation
mishandles nested transactions by resetting $this->inTransaction to 0 in the
inner transaction's finally/cleanup path; update Mongo.php so that
withTransaction decrements $this->inTransaction (e.g., --$this->inTransaction)
rather than assigning 0 when leaving a nested transaction, ensuring
commitTransaction sees the correct nesting level and outer transactions can
commit; alternatively, if Mongo replica-set transactions cannot support nesting,
add a test skip for this case, but the preferred fix is to correct the
inTransaction counter in the withTransaction method and ensure
commitTransaction/abortTransaction rely on that counter.
---
Nitpick comments:
In `@tests/e2e/Adapter/Scopes/GeneralTests.php`:
- Around line 857-858: Tests testTransactionStateAfterKnownException and
testNestedTransactionState create collections via Database::createCollection and
remove them with Database::deleteCollection but do not guarantee cleanup if
assertions fail; wrap the create/delete logic in a try/finally or move cleanup
into the test class tearDown to ensure deleteCollection always runs, i.e.,
perform createCollection before the try, run assertions inside try, and call
deleteCollection inside finally (or register collection names in setUp and
remove them in tearDown) so orphaned collections cannot remain after failures.
- Around line 918-919: The test is hardcoded to expect 3 attempts which couples
it to Adapter's internal $retries value; add a public class constant (e.g.,
Adapter::DEFAULT_RETRIES) or a public static getter (e.g.,
Adapter::getDefaultRetries()) in the Adapter class where $retries is defined,
set it to the current default (2), and update the test in GeneralTests.php to
compute expected attempts as Adapter::DEFAULT_RETRIES + 1 (initial attempt +
retries) instead of the literal 3 so the test follows the implementation
default.
premtsd-code
changed the title
There was a problem hiding this comment.
We should avoid checks like "is this an x" and instead use a "does this support x" pattern. So here we should use a (new) method getSupportForTransactionRetries instead of explicitly checking for Mongo.
This way, adding the adapters themselves are the source of truth, so we would never add to add another condition here which might not be the case with instance checks.
premtsd-code
force-pushed
the
fix-transaction-state-reset
branch
from
88bb8c5 to
19c129d
Compare
premtsd-code
force-pushed
the
fix-transaction-state-reset
branch
from
19c129d to
143e3bb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Database/Adapter/SQL.php`:
- Around line 3580-3589: Remove the trailing whitespace on the blank line
between the methods getSupportForTransactionRetries() and
getSupportForNestedTransactions() in class SQL (the blank line after
getSupportForTransactionRetries returns) so the file no longer triggers the
PSR-12 `no_whitespace_in_blank_line` Pint lint error; after removing the
trailing spaces, save and re-run the linter to confirm the violation is
resolved.
|
|
||
| public function getSupportForTransactionRetries(): bool | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| public function getSupportForNestedTransactions(): bool | ||
| { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Fix trailing whitespace on line 3585 to resolve the Pint lint failure.
The CI pipeline reports a no_whitespace_in_blank_line PSR-12 violation. Line 3585 has trailing whitespace on the blank line between the two methods.
🧹 Fix trailing whitespace
public function getSupportForTransactionRetries(): bool
{
return true;
}
-
+
public function getSupportForNestedTransactions(): bool
{
return true;
}📝 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.
| public function getSupportForTransactionRetries(): bool | |
| { | |
| return true; | |
| } | |
| public function getSupportForNestedTransactions(): bool | |
| { | |
| return true; | |
| } | |
| public function getSupportForTransactionRetries(): bool | |
| { | |
| return true; | |
| } | |
| public function getSupportForNestedTransactions(): bool | |
| { | |
| return true; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Database/Adapter/SQL.php` around lines 3580 - 3589, Remove the trailing
whitespace on the blank line between the methods
getSupportForTransactionRetries() and getSupportForNestedTransactions() in class
SQL (the blank line after getSupportForTransactionRetries returns) so the file
no longer triggers the PSR-12 `no_whitespace_in_blank_line` Pint lint error;
after removing the trailing spaces, save and re-run the linter to confirm the
violation is resolved.
2 participants
Summary
$this->inTransaction = 0resets in baseAdapter::withTransaction()after successful rollback — all adapter implementations already handle the counter correctlywithTransaction()to bypass transaction management for nested calls, preventing state corruption of the outer transaction's counter and session$this->inTransaction = 0reset in PostgresrollbackTransaction()on PDOException, matching SQL adapter behaviorTest plan
testTransactionStateAfterKnownException— verifiesinTransaction()is false after aDuplicateExceptiontestTransactionStateAfterRetriesExhausted— verifies state after all retries are exhausted (skipped for MongoDB which has no retry logic)testNestedTransactionState— verifies outer transaction commits when inner transaction throws (skipped for MongoDB which doesn't support nested transactions)Summary by CodeRabbit
New Features
Bug Fixes
Tests