Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds Pool::withTransaction to pin a single pooled adapter for the duration of a transaction, and updates MariaDB adapter to detect, delete, and retry on orphaned permission duplicate-key errors while refining duplicate-key error messages across adapters. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant Client
participant Pool
participant Adapter
participant DB as Database (PDO)
end
Client->>Pool: withTransaction(callback)
Pool->>Pool: Pin adapter instance from pool
Pool->>Adapter: Reconfigure adapter (db, namespace, tenant, auth, timeout, debug, metadata)
Adapter->>DB: beginTransaction()
Adapter->>Adapter: Execute callback()
alt Callback succeeds
Adapter->>DB: commit()
Adapter-->>Pool: result
else Callback fails
Adapter->>DB: rollback()
Adapter-->>Pool: throw
end
Pool-->>Client: Return/Throw
sequenceDiagram
rect rgba(255,220,200,0.5)
participant Client
participant MariaDB as Adapter (MariaDB)
participant DB as Database (PDO)
end
Client->>MariaDB: createDocument(with permissions)
MariaDB->>DB: INSERT permission
alt PDOException (SQLSTATE 23000 / errorCode 1062) and message contains "_index1"
DB-->>MariaDB: PDOException
MariaDB->>DB: SELECT orphaned permission rows for document (tenant scoped if present)
MariaDB->>DB: DELETE orphaned permission rows
MariaDB->>DB: Retry INSERT permission
DB-->>MariaDB: Success
else Other duplicate / unique violation
DB-->>MariaDB: PDOException
MariaDB->>MariaDB: processException -> map to DuplicateException (permission vs document vs generic)
MariaDB-->>Client: Throw DuplicateException
end
MariaDB-->>Client: Return created document (on success)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/Database/Adapter/MariaDB.php`:
- Around line 1908-1914: The current handling of SQL error messages incorrectly
constructs DuplicateException and uses inverted logic: in the block around
$message = $e->getMessage() check the condition for _uid should be positive (if
(str_contains($message, '_uid')) ...) and when instantiating DuplicateException
pass the exception code and the exception object as the second and third args
(i.e. $e->getCode(), $e) instead of passing $e as the code; update the branch
that checks for '_uid' to throw new DuplicateException('Document with the
requested unique attributes already exists', $e->getCode(), $e) and ensure the
other branch that checks '_index1' still returns DuplicateException('Duplicate
permissions for document', $e->getCode(), $e).
In `@tests/e2e/Adapter/Scopes/DocumentTests.php`:
- Around line 5052-5064: The test currently only accepts DuplicateException for
unique-index collisions; update the assertion in DocumentTests::(the test around
createDocument('duplicateMessages', ...)) to accept either DuplicateException or
UniqueException (or branch by adapter using the adapter class name/identifier)
and still assert the error message contains "unique attributes"; specifically
modify the try/catch to catch UniqueException as well (or inspect the adapter
via the MariaDB adapter symbol to decide expected exception) while keeping the
assertStringContainsString on $e->getMessage() unchanged so both MariaDB and
other adapters pass.
- Around line 5025-5067: Wrap the test's setup, actions, and assertions that
operate on the 'duplicateMessages' collection in a try/finally so
deleteCollection('duplicateMessages') is always executed; specifically, after
calling createCollection('duplicateMessages') (and creating attributes/index),
run the two createDocument(...) blocks and their DuplicateException assertions
inside a try block and move the existing deleteCollection('duplicateMessages')
into the finally block to guarantee cleanup even if an assertion or exception
(from createDocument, DuplicateException checks, or Document construction)
fails.
In `@tests/e2e/Adapter/Scopes/GeneralTests.php`:
- Around line 825-840: The call to $this->fail('Expected exception was not
thrown') is unreachable because Database::withTransaction(...) (used here)
re-throws the exception, so remove that dead statement; leave the try/catch that
calls $database->withTransaction(...) and keep the catch asserting the exception
message from the thrown \Exception, or alternatively replace the try/catch with
$this->expectExceptionMessage('Intentional failure to trigger rollback') before
invoking withTransaction() — locate the invocation of withTransaction and the
surrounding try/catch block to remove the unreachable $this->fail(...) call.
🧹 Nitpick comments (1)
src/Database/Adapter/Pool.php (1)
107-127: Consider extracting the shared configuration logic to reduce duplication.The adapter configuration block (lines 108–124) is an exact copy of the one in
delegate()(lines 41–57). If either changes independently, they'll drift apart.♻️ Extract a helper method
+ private function configureAdapter(Adapter $adapter): void + { + $adapter->setDatabase($this->getDatabase()); + $adapter->setNamespace($this->getNamespace()); + $adapter->setSharedTables($this->getSharedTables()); + $adapter->setTenant($this->getTenant()); + $adapter->setAuthorization($this->authorization); + + if ($this->getTimeout() > 0) { + $adapter->setTimeout($this->getTimeout()); + } + $adapter->resetDebug(); + foreach ($this->getDebug() as $key => $value) { + $adapter->setDebug($key, $value); + } + $adapter->resetMetadata(); + foreach ($this->getMetadata() as $key => $value) { + $adapter->setMetadata($key, $value); + } + } + public function delegate(string $method, array $args): mixed { - return $this->pool->use(function (Adapter $adapter) use ($method, $args) { - $adapter->setDatabase($this->getDatabase()); - $adapter->setNamespace($this->getNamespace()); - $adapter->setSharedTables($this->getSharedTables()); - $adapter->setTenant($this->getTenant()); - $adapter->setAuthorization($this->authorization); - - if ($this->getTimeout() > 0) { - $adapter->setTimeout($this->getTimeout()); - } - $adapter->resetDebug(); - foreach ($this->getDebug() as $key => $value) { - $adapter->setDebug($key, $value); - } - $adapter->resetMetadata(); - foreach ($this->getMetadata() as $key => $value) { - $adapter->setMetadata($key, $value); - } - + return $this->pool->use(function (Adapter $adapter) use ($method, $args) { + $this->configureAdapter($adapter); return $adapter->{$method}(...$args); }); }Then use the same helper in
withTransaction:public function withTransaction(callable $callback): mixed { return $this->pool->use(function (Adapter $adapter) use ($callback) { - $adapter->setDatabase($this->getDatabase()); - ... + $this->configureAdapter($adapter); return $adapter->withTransaction($callback); }); }
…transaction pinning
- Mongo, Postgres, SQLite adapters now return distinct messages for duplicate document ID
("Document already exists") vs unique index violations ("Document with the requested
unique attributes already exists"), matching MariaDB behavior
- Pool adapter now pins a single connection for the entire withTransaction lifecycle,
ensuring all delegate calls within a transaction use the same underlying connection
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When Database::createDocument (and other methods) internally call withTransaction, the Pool adapter now reuses the already-pinned adapter instead of checking out a new one from the pool. This ensures all nested transactions share the same connection. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/Database/Adapter/Mongo.php`:
- Around line 3486-3491: The duplicate-key handling in Mongo.php currently
returns DuplicateException for unique constraint errors (codes 11000/11001)
unless the message contains '_uid'; change it so that only collisions involving
the internal document identity (i.e., messages containing '_uid') return
DuplicateException, while all other unique-key violations (including
PRIMARY/_id) return UniqueException instead; update the branch that checks
$e->getCode() === 11000 || $e->getCode() === 11001 to return a new
UniqueException('Document with the requested unique attributes already exists',
$e->getCode(), $e) for non-'_uid' messages and keep DuplicateException for
'_uid' cases, referencing the same $e, str_contains check, DuplicateException
and UniqueException symbols.
In `@src/Database/Adapter/Pool.php`:
- Around line 115-149: The Pool class currently stores pinnedAdapter as instance
state which races when a Pool is shared across concurrent coroutines/fibers;
update the codebase to either (A) clearly document the ownership model so
callers know Pool must be instantiated per-request/per-coroutine (add a
class-level docblock on Pool and notes in constructor and DI config) or (B) make
pinnedAdapter context-aware by replacing the single property with a context map
keyed by the current execution context (e.g., Fiber::getCurrent() or the
runtime's coroutine id) so withTransaction and the pinnedAdapter lookup
store/retrieve adapter using that key; refer to the Pool::withTransaction method
and the pinnedAdapter property when making the change.
In `@src/Database/Adapter/Postgres.php`:
- Around line 2191-2196: The current Postgres exception handling block in the
code that inspects $e (the caught PDOException) returns DuplicateException for
non-_uid unique violations; change it so that when the error message does NOT
contain '_uid' you throw UniqueException('Document with the requested unique
attributes already exists', $e->getCode(), $e) instead of DuplicateException,
and keep DuplicateException only for cases where the message contains '_uid'.
Update the branch that currently returns new DuplicateException when
!\str_contains($message, '_uid') to return new UniqueException with the same
message and constructor args; leave the existing DuplicateException return for
the '_uid' path unchanged so behavior matches MariaDB adapter (see the block
handling $e->getCode() === '23505' and $e->errorInfo[1] === 7).
In `@src/Database/Adapter/SQLite.php`:
- Around line 1331-1334: The current SQLite adapter returns DuplicateException
for all unique constraint violations; change the non-_uid branch to throw
UniqueException instead so only violations whose message contains '_uid' return
DuplicateException; update the conditional in the code around where
DuplicateException is created (the block referencing $message and $e) to return
new DuplicateException('Document with the requested unique attributes already
exists', $e->getCode(), $e) when str_contains($message, '_uid') is true, and
return new UniqueException('Unique constraint violation', $e->getCode(), $e) for
all other unique constraint messages (mirror the behavior used in MariaDB.php).
🧹 Nitpick comments (1)
src/Database/Adapter/Pool.php (1)
123-140: Extract adapter configuration into a reusable private method.The configuration block (lines 124–140) is a near-exact copy of lines 50–67 in
delegate(). Extracting it avoids the risk of the two copies diverging when a new config setter is added.♻️ Proposed refactor
+ private function configureAdapter(Adapter $adapter): void + { + $adapter->setDatabase($this->getDatabase()); + $adapter->setNamespace($this->getNamespace()); + $adapter->setSharedTables($this->getSharedTables()); + $adapter->setTenant($this->getTenant()); + $adapter->setAuthorization($this->authorization); + + if ($this->getTimeout() > 0) { + $adapter->setTimeout($this->getTimeout()); + } + $adapter->resetDebug(); + foreach ($this->getDebug() as $key => $value) { + $adapter->setDebug($key, $value); + } + $adapter->resetMetadata(); + foreach ($this->getMetadata() as $key => $value) { + $adapter->setMetadata($key, $value); + } + }Then in both
delegate()andwithTransaction():return $this->pool->use(function (Adapter $adapter) use ($method, $args) { - // Run setters in case config changed since this connection was last used - $adapter->setDatabase($this->getDatabase()); - $adapter->setNamespace($this->getNamespace()); - ... + $this->configureAdapter($adapter); return $adapter->{$method}(...$args); });
Comment on lines
+115
to
+149
| public function withTransaction(callable $callback): mixed | ||
| { | ||
| // If already inside a transaction, reuse the pinned adapter | ||
| // so nested withTransaction calls use the same connection | ||
| if ($this->pinnedAdapter !== null) { | ||
| return $this->pinnedAdapter->withTransaction($callback); | ||
| } | ||
|
|
||
| return $this->pool->use(function (Adapter $adapter) use ($callback) { | ||
| $adapter->setDatabase($this->getDatabase()); | ||
| $adapter->setNamespace($this->getNamespace()); | ||
| $adapter->setSharedTables($this->getSharedTables()); | ||
| $adapter->setTenant($this->getTenant()); | ||
| $adapter->setAuthorization($this->authorization); | ||
|
|
||
| if ($this->getTimeout() > 0) { | ||
| $adapter->setTimeout($this->getTimeout()); | ||
| } | ||
| $adapter->resetDebug(); | ||
| foreach ($this->getDebug() as $key => $value) { | ||
| $adapter->setDebug($key, $value); | ||
| } | ||
| $adapter->resetMetadata(); | ||
| foreach ($this->getMetadata() as $key => $value) { | ||
| $adapter->setMetadata($key, $value); | ||
| } | ||
|
|
||
| $this->pinnedAdapter = $adapter; | ||
| try { | ||
| return $adapter->withTransaction($callback); | ||
| } finally { | ||
| $this->pinnedAdapter = null; | ||
| } | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how Pool adapter instances are created/shared in the codebase
echo "=== Pool instantiation patterns ==="
rg -n 'new Pool\(' --type php -g '!vendor/**' -g '!tests/**' | head -100
echo ""
echo "=== Pool::class bindings in DI containers ==="
rg -n 'Pool::class' --type php -g '!vendor/**' -g '!tests/**' | head -100
echo ""
echo "=== Check for singleton/shared container bindings ==="
rg -n -B2 -A2 '(singleton|share|shared).*[Pp]ool' --type php -g '!vendor/**' -g '!tests/**' | head -100Repository: utopia-php/database
Length of output: 200
🏁 Script executed:
#!/bin/bash
# Check for Pool usage patterns (imports, instanceof, etc.)
echo "=== Import/use Pool patterns ==="
rg -n '(use.*Pool|instanceof Pool)' --type php -g '!vendor/**' | head -100
echo ""
echo "=== Pool class definition and interface ==="
rg -n 'class Pool\|interface.*Pool' --type php -g '!vendor/**' | head -50
echo ""
echo "=== Check test files for Pool usage ==="
rg -n 'new Pool\|Pool::class' --type php tests/ 2>/dev/null | head -100
echo ""
echo "=== Check if Pool is created through a factory or builder ==="
rg -n -B3 -A3 'function.*[Pp]ool|Pool.*factory' --type php -g '!vendor/**' | head -150Repository: utopia-php/database
Length of output: 2067
🏁 Script executed:
#!/bin/bash
# Read the Pool test file to understand usage patterns
echo "=== Pool Test File ==="
cat -n tests/e2e/Adapter/PoolTest.php
echo ""
echo "=== Search for pool->use() patterns ==="
rg -n -B2 -A5 'pool->use\(' --type php -g '!vendor/**' | head -150
echo ""
echo "=== Check for Fiber/async documentation or comments ==="
rg -n -i '(fiber|coroutine|concurrent|async|swoole|thread)' --type php src/Database/Adapter/Pool.phpRepository: utopia-php/database
Length of output: 11805
🏁 Script executed:
#!/bin/bash
# Check for Database instantiation patterns in production code
echo "=== Database instantiation patterns ==="
rg -n 'new Database\(' --type php src/ | head -100
echo ""
echo "=== Check Pool class structure and properties ==="
head -100 src/Database/Adapter/Pool.php
echo ""
echo "=== Look for README or documentation ==="
find . -maxdepth 2 -name 'README*' -o -name '*.md' | head -10Repository: utopia-php/database
Length of output: 3362
🏁 Script executed:
#!/bin/bash
# Check README for Pool usage documentation
echo "=== README Pool usage section ==="
grep -n -i -A10 -B2 'pool' README.md | head -100
echo ""
echo "=== Check SPEC for Pool design ==="
grep -n -i -A10 'pool\|transaction\|concurrent\|coroutine' SPEC.md | head -150Repository: utopia-php/database
Length of output: 132
🏁 Script executed:
#!/bin/bash
# Read full README
echo "=== Full README.md ==="
cat README.md
echo ""
echo "=== Search for UtopiaPool usage documentation ==="
rg -n 'UtopiaPool\|Pools\\Pool' --type php -B5 -A5 src/ | head -200Repository: utopia-php/database
Length of output: 26019
🏁 Script executed:
#!/bin/bash
# Check if there are any concurrency-related comments or patterns
echo "=== Search for concurrency/thread safety patterns in Pool ==="
cat -n src/Database/Adapter/Pool.php | grep -i -A5 -B5 'thread\|concurrent\|coroutine\|fiber\|async\|race\|mutex\|lock'
echo ""
echo "=== Check Pool's usage of delegate() with pinnedAdapter ==="
cat -n src/Database/Adapter/Pool.php | head -150
echo ""
echo "=== Search for any container/DI configuration files ==="
find . -maxdepth 3 -name 'config*' -o -name 'container*' -o -name 'services*' | grep -v vendor | grep -v node_modules
echo ""
echo "=== Check if there are any examples or integration tests showing shared Pool usage ==="
find . -path ./vendor -prune -o -name '*example*' -o -name '*integration*' -type f -print 2>/dev/null | head -20Repository: utopia-php/database
Length of output: 7023
Document Pool ownership model regarding shared instances in async runtimes.
pinnedAdapter stored as mutable instance state could race if a single Pool instance were shared across concurrent coroutines (e.g., Swoole, Fibers). Coroutine B entering withTransaction while A's transaction is active would see A's pinnedAdapter and incorrectly route to A's connection, silently corrupting both transactions.
While Pool is dependency-injected (suggesting per-request instantiation), the codebase lacks documentation or configuration showing the intended ownership model. Add documentation clarifying whether Pool instances must be instantiated per-request/per-coroutine, or refactor to use context-aware storage (e.g., Fiber::getCurrent() key or similar) instead of instance state to make the design explicitly safe for shared instances.
🤖 Prompt for AI Agents
In `@src/Database/Adapter/Pool.php` around lines 115 - 149, The Pool class
currently stores pinnedAdapter as instance state which races when a Pool is
shared across concurrent coroutines/fibers; update the codebase to either (A)
clearly document the ownership model so callers know Pool must be instantiated
per-request/per-coroutine (add a class-level docblock on Pool and notes in
constructor and DI config) or (B) make pinnedAdapter context-aware by replacing
the single property with a context map keyed by the current execution context
(e.g., Fiber::getCurrent() or the runtime's coroutine id) so withTransaction and
the pinnedAdapter lookup store/retrieve adapter using that key; refer to the
Pool::withTransaction method and the pinnedAdapter property when making the
change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests