Skip to content

Commit adfdf20

Browse files
abnegateabnegate
authored
Merge pull request #813 from utopia-php/fix-ghost-perms
Fix ghost perms
2 parents 5c89b39 + a375c36 commit adfdf20

File tree

8 files changed

+296
-7
lines changed

8 files changed

+296
-7
lines changed

‎src/Database/Adapter/MariaDB.php‎

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -927,7 +927,29 @@ public function createDocument(Document $collection, Document $document): Docume
927927
}
928928

929929
if (isset($stmtPermissions)) {
930-
$stmtPermissions->execute();
930+
try {
931+
$stmtPermissions->execute();
932+
} catch (PDOException $e) {
933+
$isOrphanedPermission = $e->getCode() === '23000'
934+
&& isset($e->errorInfo[1])
935+
&& $e->errorInfo[1] === 1062
936+
&& \str_contains($e->getMessage(), '_index1');
937+
938+
if (!$isOrphanedPermission) {
939+
throw $e;
940+
}
941+
942+
// Clean up orphaned permissions from a previous failed delete, then retry
943+
$sql = "DELETE FROM {$this->getSQLTable($name . '_perms')} WHERE _document = :_uid {$this->getTenantQuery($collection)}";
944+
$cleanup = $this->getPDO()->prepare($sql);
945+
$cleanup->bindValue(':_uid', $document->getId());
946+
if ($this->sharedTables) {
947+
$cleanup->bindValue(':_tenant', $document->getTenant());
948+
}
949+
$cleanup->execute();
950+
951+
$stmtPermissions->execute();
952+
}
931953
}
932954
} catch (PDOException $e) {
933955
throw $this->processException($e);
@@ -1883,6 +1905,13 @@ protected function processException(PDOException $e): \Exception
18831905

18841906
// Duplicate row
18851907
if ($e->getCode() === '23000' && isset($e->errorInfo[1]) && $e->errorInfo[1] === 1062) {
1908+
$message = $e->getMessage();
1909+
if (\str_contains($message, '_index1')) {
1910+
return new DuplicateException('Duplicate permissions for document', $e->getCode(), $e);
1911+
}
1912+
if (!\str_contains($message, '_uid')) {
1913+
return new DuplicateException('Document with the requested unique attributes already exists', $e->getCode(), $e);
1914+
}
18861915
return new DuplicateException('Document already exists', $e->getCode(), $e);
18871916
}
18881917

‎src/Database/Adapter/Mongo.php‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3483,12 +3483,11 @@ protected function processException(\Throwable $e): \Throwable
34833483
}
34843484

34853485
// Duplicate key error
3486-
if ($e->getCode() === 11000) {
3487-
return new DuplicateException('Document already exists', $e->getCode(), $e);
3488-
}
3489-
3490-
// Duplicate key error for unique index
3491-
if ($e->getCode() === 11001) {
3486+
if ($e->getCode() === 11000 || $e->getCode() === 11001) {
3487+
$message = $e->getMessage();
3488+
if (!\str_contains($message, '_uid')) {
3489+
return new DuplicateException('Document with the requested unique attributes already exists', $e->getCode(), $e);
3490+
}
34923491
return new DuplicateException('Document already exists', $e->getCode(), $e);
34933492
}
34943493

‎src/Database/Adapter/Pool.php‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,12 @@ class Pool extends Adapter
1616
*/
1717
protected UtopiaPool $pool;
1818

19+
/**
20+
* When a transaction is active, all delegate calls are routed through
21+
* this pinned adapter to ensure they run on the same connection.
22+
*/
23+
protected ?Adapter $pinnedAdapter = null;
24+
1925
/**
2026
* @param UtopiaPool<covariant Adapter> $pool The pool to use for connections. Must contain instances of Adapter.
2127
*/
@@ -36,6 +42,10 @@ public function __construct(UtopiaPool $pool)
3642
*/
3743
public function delegate(string $method, array $args): mixed
3844
{
45+
if ($this->pinnedAdapter !== null) {
46+
return $this->pinnedAdapter->{$method}(...$args);
47+
}
48+
3949
return $this->pool->use(function (Adapter $adapter) use ($method, $args) {
4050
// Run setters in case config changed since this connection was last used
4151
$adapter->setDatabase($this->getDatabase());
@@ -92,6 +102,52 @@ public function rollbackTransaction(): bool
92102
return $this->delegate(__FUNCTION__, \func_get_args());
93103
}
94104

105+
/**
106+
* Pin a single connection from the pool for the entire transaction lifecycle.
107+
* This prevents startTransaction(), the callback, and commitTransaction()
108+
* from running on different connections.
109+
*
110+
* @template T
111+
* @param callable(): T $callback
112+
* @return T
113+
* @throws \Throwable
114+
*/
115+
public function withTransaction(callable $callback): mixed
116+
{
117+
// If already inside a transaction, reuse the pinned adapter
118+
// so nested withTransaction calls use the same connection
119+
if ($this->pinnedAdapter !== null) {
120+
return $this->pinnedAdapter->withTransaction($callback);
121+
}
122+
123+
return $this->pool->use(function (Adapter $adapter) use ($callback) {
124+
$adapter->setDatabase($this->getDatabase());
125+
$adapter->setNamespace($this->getNamespace());
126+
$adapter->setSharedTables($this->getSharedTables());
127+
$adapter->setTenant($this->getTenant());
128+
$adapter->setAuthorization($this->authorization);
129+
130+
if ($this->getTimeout() > 0) {
131+
$adapter->setTimeout($this->getTimeout());
132+
}
133+
$adapter->resetDebug();
134+
foreach ($this->getDebug() as $key => $value) {
135+
$adapter->setDebug($key, $value);
136+
}
137+
$adapter->resetMetadata();
138+
foreach ($this->getMetadata() as $key => $value) {
139+
$adapter->setMetadata($key, $value);
140+
}
141+
142+
$this->pinnedAdapter = $adapter;
143+
try {
144+
return $adapter->withTransaction($callback);
145+
} finally {
146+
$this->pinnedAdapter = null;
147+
}
148+
});
149+
}
150+
95151
protected function quote(string $string): string
96152
{
97153
return $this->delegate(__FUNCTION__, \func_get_args());

‎src/Database/Adapter/Postgres.php‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2189,6 +2189,10 @@ protected function processException(PDOException $e): \Exception
21892189

21902190
// Duplicate row
21912191
if ($e->getCode() === '23505' && isset($e->errorInfo[1]) && $e->errorInfo[1] === 7) {
2192+
$message = $e->getMessage();
2193+
if (!\str_contains($message, '_uid')) {
2194+
return new DuplicateException('Document with the requested unique attributes already exists', $e->getCode(), $e);
2195+
}
21922196
return new DuplicateException('Document already exists', $e->getCode(), $e);
21932197
}
21942198

‎src/Database/Adapter/SQLite.php‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1328,6 +1328,9 @@ protected function processException(PDOException $e): \Exception
13281328
stripos($message, 'unique') !== false ||
13291329
stripos($message, 'duplicate') !== false
13301330
) {
1331+
if (!\str_contains($message, '_uid')) {
1332+
return new DuplicateException('Document with the requested unique attributes already exists', $e->getCode(), $e);
1333+
}
13311334
return new DuplicateException('Document already exists', $e->getCode(), $e);
13321335
}
13331336
}

‎tests/e2e/Adapter/PoolTest.php‎

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,12 @@
1010
use Utopia\Database\Adapter\MySQL;
1111
use Utopia\Database\Adapter\Pool;
1212
use Utopia\Database\Database;
13+
use Utopia\Database\Document;
1314
use Utopia\Database\Exception;
1415
use Utopia\Database\Exception\Duplicate;
1516
use Utopia\Database\Exception\Limit;
17+
use Utopia\Database\Helpers\Permission;
18+
use Utopia\Database\Helpers\Role;
1619
use Utopia\Database\PDO;
1720
use Utopia\Pools\Adapter\Stack;
1821
use Utopia\Pools\Pool as UtopiaPool;
@@ -109,4 +112,89 @@ protected function deleteIndex(string $collection, string $index): bool
109112

110113
return true;
111114
}
115+
116+
/**
117+
* Execute raw SQL via the pool using reflection to access the adapter's PDO.
118+
*
119+
* @param string $sql
120+
* @param array<string, mixed> $binds
121+
*/
122+
private function execRawSQL(string $sql, array $binds = []): void
123+
{
124+
self::$pool->use(function (Adapter $adapter) use ($sql, $binds) {
125+
$class = new ReflectionClass($adapter);
126+
$property = $class->getProperty('pdo');
127+
$property->setAccessible(true);
128+
$pdo = $property->getValue($adapter);
129+
$stmt = $pdo->prepare($sql);
130+
foreach ($binds as $key => $value) {
131+
$stmt->bindValue($key, $value);
132+
}
133+
$stmt->execute();
134+
});
135+
}
136+
137+
/**
138+
* Test that orphaned permission records from a previous failed delete
139+
* don't block document recreation. The createDocument method should
140+
* clean up orphaned perms and retry.
141+
*/
142+
public function testOrphanedPermissionsRecovery(): void
143+
{
144+
$database = $this->getDatabase();
145+
$collection = 'orphanedPermsRecovery';
146+
147+
$database->createCollection($collection);
148+
$database->createAttribute($collection, 'title', Database::VAR_STRING, 128, true);
149+
150+
// Step 1: Create a document with permissions
151+
$doc = $database->createDocument($collection, new Document([
152+
'$id' => 'orphan_test',
153+
'$permissions' => [
154+
Permission::read(Role::any()),
155+
Permission::update(Role::any()),
156+
Permission::delete(Role::any()),
157+
],
158+
'title' => 'Original',
159+
]));
160+
$this->assertEquals('orphan_test', $doc->getId());
161+
162+
// Step 2: Delete the document normally (cleans up both doc and perms)
163+
$database->deleteDocument($collection, 'orphan_test');
164+
$deleted = $database->getDocument($collection, 'orphan_test');
165+
$this->assertTrue($deleted->isEmpty());
166+
167+
// Step 3: Manually re-insert orphaned permission rows (simulating a partial delete failure)
168+
$namespace = $this->getDatabase()->getNamespace();
169+
$dbName = $this->getDatabase()->getDatabase();
170+
$permsTable = "`{$dbName}`.`{$namespace}_{$collection}_perms`";
171+
172+
$this->execRawSQL(
173+
"INSERT INTO {$permsTable} (_type, _permission, _document) VALUES (:type, :perm, :doc)",
174+
[':type' => 'read', ':perm' => 'any', ':doc' => 'orphan_test']
175+
);
176+
$this->execRawSQL(
177+
"INSERT INTO {$permsTable} (_type, _permission, _document) VALUES (:type, :perm, :doc)",
178+
[':type' => 'update', ':perm' => 'any', ':doc' => 'orphan_test']
179+
);
180+
181+
// Step 4: Recreate a document with the same ID - should succeed by cleaning up orphaned perms
182+
$newDoc = $database->createDocument($collection, new Document([
183+
'$id' => 'orphan_test',
184+
'$permissions' => [
185+
Permission::read(Role::any()),
186+
Permission::update(Role::any()),
187+
],
188+
'title' => 'Recreated',
189+
]));
190+
$this->assertEquals('orphan_test', $newDoc->getId());
191+
$this->assertEquals('Recreated', $newDoc->getAttribute('title'));
192+
193+
// Verify the document can be fetched
194+
$found = $database->getDocument($collection, 'orphan_test');
195+
$this->assertFalse($found->isEmpty());
196+
$this->assertEquals('Recreated', $found->getAttribute('title'));
197+
198+
$database->deleteCollection($collection);
199+
}
112200
}

‎tests/e2e/Adapter/Scopes/DocumentTests.php‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5007,6 +5007,64 @@ public function testUniqueIndexDuplicate(): void
50075007
$this->assertInstanceOf(DuplicateException::class, $e);
50085008
}
50095009
}
5010+
5011+
/**
5012+
* Test that DuplicateException messages differentiate between
5013+
* document ID duplicates and unique index violations.
5014+
*/
5015+
public function testDuplicateExceptionMessages(): void
5016+
{
5017+
/** @var Database $database */
5018+
$database = $this->getDatabase();
5019+
5020+
if (!$database->getAdapter()->getSupportForUniqueIndex()) {
5021+
$this->expectNotToPerformAssertions();
5022+
return;
5023+
}
5024+
5025+
$database->createCollection('duplicateMessages');
5026+
$database->createAttribute('duplicateMessages', 'email', Database::VAR_STRING, 128, true);
5027+
$database->createIndex('duplicateMessages', 'emailUnique', Database::INDEX_UNIQUE, ['email'], [128]);
5028+
5029+
// Create first document
5030+
$database->createDocument('duplicateMessages', new Document([
5031+
'$id' => 'dup_msg_1',
5032+
'$permissions' => [
5033+
Permission::read(Role::any()),
5034+
],
5035+
'email' => 'test@example.com',
5036+
]));
5037+
5038+
// Test 1: Duplicate document ID should say "Document already exists"
5039+
try {
5040+
$database->createDocument('duplicateMessages', new Document([
5041+
'$id' => 'dup_msg_1',
5042+
'$permissions' => [
5043+
Permission::read(Role::any()),
5044+
],
5045+
'email' => 'different@example.com',
5046+
]));
5047+
$this->fail('Expected DuplicateException for duplicate document ID');
5048+
} catch (DuplicateException $e) {
5049+
$this->assertStringContainsString('Document already exists', $e->getMessage());
5050+
}
5051+
5052+
// Test 2: Unique index violation should mention "unique attributes"
5053+
try {
5054+
$database->createDocument('duplicateMessages', new Document([
5055+
'$id' => 'dup_msg_2',
5056+
'$permissions' => [
5057+
Permission::read(Role::any()),
5058+
],
5059+
'email' => 'test@example.com',
5060+
]));
5061+
$this->fail('Expected DuplicateException for unique index violation');
5062+
} catch (DuplicateException $e) {
5063+
$this->assertStringContainsString('unique attributes', $e->getMessage());
5064+
}
5065+
5066+
$database->deleteCollection('duplicateMessages');
5067+
}
50105068
/**
50115069
* @depends testUniqueIndexDuplicate
50125070
*/

‎tests/e2e/Adapter/Scopes/GeneralTests.php‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -793,6 +793,58 @@ public function testCacheReconnect(): void
793793
}
794794
}
795795

796+
/**
797+
* Test that withTransaction properly rolls back on failure.
798+
* With the Pool adapter, this verifies that the entire transaction
799+
* (start, callback, commit/rollback) runs on a single pinned connection.
800+
*/
801+
public function testTransactionAtomicity(): void
802+
{
803+
/** @var Database $database */
804+
$database = $this->getDatabase();
805+
806+
$database->createCollection('transactionAtomicity');
807+
$database->createAttribute('transactionAtomicity', 'title', Database::VAR_STRING, 128, true);
808+
809+
// Verify a successful transaction commits
810+
$doc = $database->withTransaction(function () use ($database) {
811+
return $database->createDocument('transactionAtomicity', new Document([
812+
'$id' => 'tx_success',
813+
'$permissions' => [
814+
Permission::read(Role::any()),
815+
],
816+
'title' => 'Committed',
817+
]));
818+
});
819+
$this->assertEquals('tx_success', $doc->getId());
820+
$found = $database->getDocument('transactionAtomicity', 'tx_success');
821+
$this->assertFalse($found->isEmpty());
822+
$this->assertEquals('Committed', $found->getAttribute('title'));
823+
824+
// Verify a failed transaction rolls back completely
825+
try {
826+
$database->withTransaction(function () use ($database) {
827+
$database->createDocument('transactionAtomicity', new Document([
828+
'$id' => 'tx_fail',
829+
'$permissions' => [
830+
Permission::read(Role::any()),
831+
],
832+
'title' => 'Should be rolled back',
833+
]));
834+
835+
throw new \Exception('Intentional failure to trigger rollback');
836+
});
837+
} catch (\Exception $e) {
838+
$this->assertEquals('Intentional failure to trigger rollback', $e->getMessage());
839+
}
840+
841+
// Document should NOT exist since the transaction was rolled back
842+
$notFound = $database->getDocument('transactionAtomicity', 'tx_fail');
843+
$this->assertTrue($notFound->isEmpty(), 'Document should not exist after transaction rollback');
844+
845+
$database->deleteCollection('transactionAtomicity');
846+
}
847+
796848
/**
797849
* Wait for Redis to be ready with a readiness probe
798850
*/

0 commit comments

Comments
 (0)