Fix with transactions #709
Conversation
WalkthroughMoved handling of six specific exceptions in Adapter::withTransaction so they abort immediately after resetting inTransaction; updated test runner to stop on first failure; added a test prep block creating a collection, inserting duplicate documents, and a failing assertion in an e2e test. No public API signature changes. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Caller
participant A as Adapter::withTransaction
participant DB as Database
C->>A: withTransaction(callback)
A->>DB: beginTransaction()
loop attempts
A->>A: execute callback
alt success
A->>DB: commit()
A-->>C: return result
else exception thrown
alt Non-retriable (Duplicate/Restricted/Authorization/Relationship/Conflict/Limit)
A->>A: set inTransaction = 0
A-->>C: rethrow (abort)
else other exception
A->>DB: rollback()
A->>A: decide retry/backoff
opt retry
A->>DB: beginTransaction()
end
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 0
🧹 Nitpick comments (1)
src/Database/Adapter.php (1)
401-411: Deduplicate inTransaction resets (set once after rollback).We set
$this->inTransaction = 0in two places (Lines 409–411 and 418–420). Set it once right after a successful rollback, then branch on exception type. Behavior is unchanged; duplication is removed.Apply:
@@ - if ( - $action instanceof DuplicateException || - $action instanceof RestrictedException || - $action instanceof AuthorizationException || - $action instanceof RelationshipException || - $action instanceof ConflictException || - $action instanceof LimitException - ) { - $this->inTransaction = 0; - throw $action; - } + // Reset once after a successful rollback, before deciding on retry vs. abort + $this->inTransaction = 0; + + if ( + $action instanceof DuplicateException || + $action instanceof RestrictedException || + $action instanceof AuthorizationException || + $action instanceof RelationshipException || + $action instanceof ConflictException || + $action instanceof LimitException + ) { + throw $action; + } @@ - $this->inTransaction = 0; - throw $action; + throw $action;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Database/Adapter.php(1 hunks)
🔇 Additional comments (1)
src/Database/Adapter.php (1)
401-411: Good fix: irrecoverable exceptions now bypass retry.Moving handling of Duplicate/Restricted/Authorization/Relationship/Conflict/Limit exceptions outside the rollback try/catch correctly prevents pointless retries and immediately aborts after cleanup. This aligns with expected transactional semantics.
Please confirm this is the full set of non-retriable database exceptions for all adapters and add a focused test asserting that each of these triggers zero retries (e.g., callback invoked once and attempt counter stays at 1).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
phpunit.xml (1)
10-10: Revert stopOnFailure in CI; use CLI locally instead.Fail-fast hides subsequent failures and reduces signal in CI. Prefer the phpunit CLI flag when debugging locally, or gate it behind env.
Apply this diff:
- stopOnFailure="true"> + stopOnFailure="false">tests/e2e/Adapter/Scopes/CollectionTests.php (1)
61-61: Fix PSR‑12 spacing or drop the line (preferred).The linter error originates here: missing space after the comma. Our previous patch removes this line; if you keep it, fix spacing.
- $this->assertEquals(1,222); + $this->assertEquals(1, 222);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
phpunit.xml(1 hunks)tests/e2e/Adapter/Scopes/CollectionTests.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/e2e/Adapter/Scopes/CollectionTests.php (5)
src/Database/Adapter.php (3)
createCollection(525-525)create(488-488)createDocument(701-701)src/Database/Database.php (3)
createCollection(1328-1462)create(1241-1260)createDocument(3837-3928)src/Database/Helpers/Permission.php (2)
Permission(9-264)read(186-195)src/Database/Helpers/Role.php (2)
Role(5-178)any(159-162)src/Database/Document.php (1)
Document(12-470)
🪛 GitHub Actions: Linter
tests/e2e/Adapter/Scopes/CollectionTests.php
[error] 1-1: PSR-12: method_argument_space violation.
⏰ 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). (11)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Unit Test
|
|
||
| $database->createCollection('shmuel', permissions: [ | ||
| Permission::create(Role::any()), | ||
| Permission::read(Role::any()), | ||
| ]); | ||
|
|
||
| $database->createDocument('shmuel', new Document([ | ||
| '$id' => 'doc1', | ||
| ])); | ||
|
|
||
| $database->createDocument('shmuel', new Document([ | ||
| '$id' => 'doc1', | ||
| ])); | ||
|
|
||
| $this->assertEquals(1,222); | ||
|
|
||
|
|
There was a problem hiding this comment.
Remove failing debug block; assert DuplicateException and clean up to avoid state leakage.
This prep code will (a) error on duplicate insert without asserting, (b) fail the suite with an intentional bad assert, and (c) pollute later collection count assertions by leaving 'shmuel' around. Replace with an explicit DuplicateException check and delete the temp collection.
Apply this diff:
@@
- $database->createCollection('shmuel', permissions: [
- Permission::create(Role::any()),
- Permission::read(Role::any()),
- ]);
-
- $database->createDocument('shmuel', new Document([
- '$id' => 'doc1',
- ]));
-
- $database->createDocument('shmuel', new Document([
- '$id' => 'doc1',
- ]));
-
- $this->assertEquals(1,222);
+ // Sanity check for transaction behavior on duplicate document IDs.
+ $database->createCollection('shmuel', permissions: [
+ Permission::create(Role::any()),
+ Permission::read(Role::any()),
+ ]);
+ $database->createDocument('shmuel', new Document([
+ '$id' => 'doc1',
+ ]));
+ try {
+ $database->createDocument('shmuel', new Document([
+ '$id' => 'doc1',
+ ]));
+ $this->fail('Failed to throw exception');
+ } catch (Exception $e) {
+ $this->assertInstanceOf(DuplicateException::class, $e);
+ }
+ $this->assertTrue($database->deleteCollection('shmuel'));🤖 Prompt for AI Agents
In tests/e2e/Adapter/Scopes/CollectionTests.php around lines 47 to 63, the block
attempts to insert a duplicate document then leaves an intentional failing
assert and the temporary 'shmuel' collection behind; replace that debug code
with a proper assertion that a DuplicateException is thrown on the second
createDocument (use $this->expectException(DuplicateException::class) or
equivalent try/catch asserting the exception), and after the check explicitly
delete the 'shmuel' collection to avoid state leakage for later tests.
Summary by CodeRabbit