Fix DDL cleanup #758
Fix DDL cleanup #758
Conversation
This commit implements comprehensive rollback mechanisms to prevent dangling database artifacts when attribute, index, or relationship creation fails during metadata updates. Changes: - createAttribute: Now rolls back created columns if metadata update fails - createIndex: Now rolls back created indexes if metadata update fails - createRelationship: Enhanced rollback to handle: * Junction table cleanup for many-to-many relationships * Index cleanup if index creation fails after metadata update * Comprehensive metadata rollback for all relationship types This ensures that if any step in the creation process fails, all modifications from previous steps are properly undone, preventing users from being left with database artifacts they cannot clean up.
|
Warning Rate limit exceeded@abnegate has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 12 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (1)
WalkthroughAdds centralized retry helpers and cleanup helpers with exponential backoff, introduces metadata persistence orchestration with rollback semantics, expands retry/rollback paths across attribute/index/relationship flows, wraps event emissions in try/catch to avoid propagation, and refactors cached-document purge into a protected retry-backed implementation with a public wrapper that emits an event. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
actor Caller
participant DB as Database
participant Retry as withRetries
participant Meta as updateMetadata
participant Cleanup as cleanup / cleanupIndex
Caller->>DB: request (create/update/delete attr/index/relationship)
DB->>Retry: withRetries(core operation)
Retry->>DB: attempt result
alt core operation succeeds
DB->>Meta: updateMetadata(collection, rollbackCallback, ...)
alt metadata write fails & rollback requested
DB->>Cleanup: invoke rollbackCallback (retry-backed)
end
else core operation fails after retries
DB->>Cleanup: cleanup rollback (retry-backed)
end
DB-->>Caller: return result
sequenceDiagram
actor Caller
participant Public as purgeCachedDocument (public)
participant Internal as purgeCachedDocumentInternal (protected)
participant Events as EventDispatcher
Caller->>Public: purgeCachedDocument(collectionId, id)
Public->>Internal: call purgeCachedDocumentInternal(...)
Internal->>Internal: perform retry-backed purge & metadata updates
Internal-->>Public: result
Public->>Events: emit EVENT_DOCUMENT_PURGE (try/catch)
Events-->>Public: handled or logged
Public-->>Caller: return boolean result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Database/Database.php(24 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
Repo: utopia-php/database PR: 721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
src/Database/Database.php
📚 Learning: 2025-10-29T12:27:57.071Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 747
File: src/Database/Adapter/Mongo.php:1449-1453
Timestamp: 2025-10-29T12:27:57.071Z
Learning: In src/Database/Adapter/Mongo.php, when getSupportForAttributes() returns false (schemaless mode), the updateDocument method intentionally uses a raw document without $set operator for replacement-style updates, as confirmed by the repository maintainer ArnabChatterjee20k.
Applied to files:
src/Database/Database.php
📚 Learning: 2025-10-20T05:29:29.487Z
Learnt from: abnegate
Repo: utopia-php/database PR: 731
File: src/Database/Database.php:6987-6988
Timestamp: 2025-10-20T05:29:29.487Z
Learning: In Utopia\Database\Database::convertRelationshipQueries, Many-to-Many filtering does not need the parent collection or a direct junction query: it calls find() on the related collection without skipping relationships, which populates relationship attributes (including the two-way key). Parent IDs are derived from that populated attribute. Therefore, calls should remain convertRelationshipQueries($relationships, $queries).
Applied to files:
src/Database/Database.php
🧬 Code graph analysis (1)
src/Database/Database.php (3)
src/Database/Document.php (3)
setAttribute(244-261)Document(12-470)getAttribute(224-231)src/Database/Adapter.php (12)
trigger(471-481)getDocumentSizeLimit(1202-1202)getAttributeWidth(1213-1213)updateAttribute(606-606)deleteAttribute(616-616)createAttribute(579-579)withTransaction(399-446)deleteRelationship(667-667)createIndex(692-692)deleteIndex(702-702)createRelationship(637-637)getDocument(713-713)src/Database/Adapter/Pool.php (11)
trigger(100-103)getDocumentSizeLimit(480-483)getAttributeWidth(485-488)updateAttribute(185-188)deleteAttribute(190-193)createAttribute(175-178)deleteRelationship(210-213)createIndex(220-223)deleteIndex(225-228)createRelationship(200-203)getDocument(230-233)
🪛 PHPMD (2.15.0)
src/Database/Database.php
1848-1848: Avoid unused local variables such as '$duplicateInSharedTable'. (undefined)
(UnusedLocalVariable)
1962-1962: Avoid unused local variables such as '$duplicateInSharedTable'. (undefined)
(UnusedLocalVariable)
3853-3853: Avoid unused local variables such as '$duplicateInSharedTable'. (undefined)
(UnusedLocalVariable)
⏰ 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). (1)
- GitHub Check: Setup & Build Docker Image
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Database/Database.php (1)
6-6: Guard Swoole usage and fix coroutine backoff unitsUnconditional Swoole usage can fatally error when the extension isn’t installed, and the coroutine sleep uses milliseconds as seconds. Guard the class existence and convert ms→seconds for Coroutine::sleep; keep usleep in microseconds.
Apply:
- use Swoole\Coroutine; + // Swoole is optional; avoid a hard import. See guarded usage in withRetries(). @@ - if (Coroutine::getCid() > 0) { - Coroutine::sleep($delayMs * 1000); - } else { - \usleep($delayMs * 1000); - } + if (\class_exists('\\Swoole\\Coroutine', false) && \Swoole\Coroutine::getCid() > 0) { + // Coroutine::sleep expects seconds, not milliseconds + \Swoole\Coroutine::sleep($delayMs / 1000); + } else { + // usleep expects microseconds + \usleep($delayMs * 1000); + }Also applies to: 8855-8859
🧹 Nitpick comments (4)
src/Database/Database.php (4)
1848-1848: Remove unused locals flagged by PHPMDDelete
$duplicateInSharedTableand$successassignments; they’re never read.@@ createAttribute - $created = false; - $duplicateInSharedTable = false; + $created = false; @@ - $duplicateInSharedTable = true; + // No-op: metadata still updated in shared-table migration paths @@ createAttributes - $created = false; - $duplicateInSharedTable = false; + $created = false; @@ - $duplicateInSharedTable = true; + // No-op: see note above @@ deleteAttribute - $success = false; @@ - $success = true; @@ - $success = false; @@ createIndex - $created = false; - $duplicateInSharedTable = false; + $created = false; @@ - $duplicateInSharedTable = true; + // No-op: see note aboveAlso applies to: 1861-1861, 1962-1962, 1976-1976, 2854-2854, 2860-2863, 3846-3846, 3858-3858
2613-2613: Consistent method casingPHP is case-insensitive, but stick to setAttribute() for consistency/readability.
- ->setattribute('key', $newKey ?? $id) + ->setAttribute('key', $newKey ?? $id)
8829-8831: Docblock: return description contradicts voidwithRetries() returns void; update the docblock.
- * @return void The result of the operation + * @return void
8884-8889: Docblock accuracy for cleanup()cleanup() rethrows the original Throwable; docblock says DatabaseException. Either wrap in DatabaseException or declare it can throw Throwable. Minimal doc-only change shown:
- * @throws DatabaseException If cleanup fails after all retries + * @throws \Throwable If cleanup fails after all retries
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Database/Database.php(25 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
Repo: utopia-php/database PR: 721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
src/Database/Database.php
📚 Learning: 2025-10-29T12:27:57.071Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 747
File: src/Database/Adapter/Mongo.php:1449-1453
Timestamp: 2025-10-29T12:27:57.071Z
Learning: In src/Database/Adapter/Mongo.php, when getSupportForAttributes() returns false (schemaless mode), the updateDocument method intentionally uses a raw document without $set operator for replacement-style updates, as confirmed by the repository maintainer ArnabChatterjee20k.
Applied to files:
src/Database/Database.php
📚 Learning: 2025-10-20T05:29:29.487Z
Learnt from: abnegate
Repo: utopia-php/database PR: 731
File: src/Database/Database.php:6987-6988
Timestamp: 2025-10-20T05:29:29.487Z
Learning: In Utopia\Database\Database::convertRelationshipQueries, Many-to-Many filtering does not need the parent collection or a direct junction query: it calls find() on the related collection without skipping relationships, which populates relationship attributes (including the two-way key). Parent IDs are derived from that populated attribute. Therefore, calls should remain convertRelationshipQueries($relationships, $queries).
Applied to files:
src/Database/Database.php
🧬 Code graph analysis (1)
src/Database/Database.php (3)
src/Database/Operator.php (2)
setAttribute(188-193)getAttribute(147-150)src/Database/Document.php (5)
setAttribute(244-261)Document(12-470)getId(63-66)getCollection(85-88)getAttribute(224-231)src/Database/Adapter.php (1)
withTransaction(399-446)
🪛 PHPMD (2.15.0)
src/Database/Database.php
1848-1848: Avoid unused local variables such as '$duplicateInSharedTable'. (undefined)
(UnusedLocalVariable)
1962-1962: Avoid unused local variables such as '$duplicateInSharedTable'. (undefined)
(UnusedLocalVariable)
2854-2854: Avoid unused local variables such as '$success'. (undefined)
(UnusedLocalVariable)
3845-3845: Avoid unused local variables such as '$duplicateInSharedTable'. (undefined)
(UnusedLocalVariable)
⏰ 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). (14)
- GitHub Check: Adapter Tests (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Unit Test
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (MySQL)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/Database/Database.php (2)
8863-8867: Backoff unit fix confirmed (ms to seconds for Coroutine::sleep)The ms-to-seconds conversion is now correct. Good catch.
2295-2301: Removed wrong ATTRIBUTE_UPDATE dispatch in updateIndexMeta()Index metadata updates now persist via updateMetadata() without emitting an attribute update event. This resolves the misfired event.
🧹 Nitpick comments (2)
src/Database/Database.php (2)
2623-2623: Use canonical method casing for readability
setattributeworks (case-insensitive), but prefersetAttributefor consistency.- ->setattribute('key', $newKey ?? $id) + ->setAttribute('key', $newKey ?? $id)
8838-8840: Docblock return type nitText says “The result of the operation” but method returns void. Adjust for clarity.
- * @return void The result of the operation + * @return void
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Database/Database.php(27 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
Repo: utopia-php/database PR: 721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
src/Database/Database.php
📚 Learning: 2025-10-29T12:27:57.071Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 747
File: src/Database/Adapter/Mongo.php:1449-1453
Timestamp: 2025-10-29T12:27:57.071Z
Learning: In src/Database/Adapter/Mongo.php, when getSupportForAttributes() returns false (schemaless mode), the updateDocument method intentionally uses a raw document without $set operator for replacement-style updates, as confirmed by the repository maintainer ArnabChatterjee20k.
Applied to files:
src/Database/Database.php
📚 Learning: 2025-10-20T05:29:29.487Z
Learnt from: abnegate
Repo: utopia-php/database PR: 731
File: src/Database/Database.php:6987-6988
Timestamp: 2025-10-20T05:29:29.487Z
Learning: In Utopia\Database\Database::convertRelationshipQueries, Many-to-Many filtering does not need the parent collection or a direct junction query: it calls find() on the related collection without skipping relationships, which populates relationship attributes (including the two-way key). Parent IDs are derived from that populated attribute. Therefore, calls should remain convertRelationshipQueries($relationships, $queries).
Applied to files:
src/Database/Database.php
🧬 Code graph analysis (1)
src/Database/Database.php (7)
src/Database/Adapter/Postgres.php (10)
createCollection(195-334)createDocument(980-1079)deleteCollection(411-419)updateAttribute(552-614)deleteAttribute(486-508)updateDocument(1093-1325)deleteRelationship(778-851)createIndex(866-921)deleteIndex(931-944)createRelationship(626-666)src/Database/Adapter/SQLite.php (8)
createCollection(144-230)createDocument(525-630)deleteCollection(291-310)updateAttribute(338-345)deleteAttribute(357-395)updateDocument(644-887)createIndex(458-483)deleteIndex(494-513)src/Database/Adapter/MariaDB.php (9)
createCollection(79-228)createDocument(825-936)deleteCollection(323-338)updateAttribute(420-441)updateDocument(951-1201)deleteRelationship(607-679)createIndex(718-781)deleteIndex(792-812)createRelationship(453-494)src/Database/Adapter/Mongo.php (12)
createCollection(404-569)createDocument(1159-1186)deleteCollection(637-641)count(2093-2180)updateAttribute(1812-1818)deleteAttribute(693-705)updateDocument(1432-1459)withTransaction(98-133)deleteRelationship(846-888)createIndex(904-1025)deleteIndex(1094-1101)createRelationship(745-748)src/Database/Adapter/Pool.php (12)
createCollection(160-163)createDocument(235-238)deleteCollection(165-168)trigger(100-103)count(280-283)updateAttribute(185-188)deleteAttribute(190-193)updateDocument(245-248)deleteRelationship(210-213)createIndex(220-223)deleteIndex(225-228)createRelationship(200-203)src/Database/Adapter.php (13)
createCollection(547-547)createDocument(723-723)deleteCollection(556-556)trigger(471-481)count(847-847)updateAttribute(606-606)deleteAttribute(616-616)updateDocument(747-747)withTransaction(399-446)deleteRelationship(667-667)createIndex(692-692)deleteIndex(702-702)createRelationship(637-637)src/Database/Document.php (2)
setAttribute(244-261)Document(12-470)
🪛 PHPMD (2.15.0)
src/Database/Database.php
2864-2864: Avoid unused local variables such as '$success'. (undefined)
(UnusedLocalVariable)
⏰ 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). (14)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Unit Test
🔇 Additional comments (4)
src/Database/Database.php (4)
7310-7347: Good split: internal purge with retries + public wrapper emitting eventsClear separation avoids retrying event dispatch. LGTM.
8939-8990: Centralized metadata persistence + rollback is solidupdateMetadata() consolidates retry + rollback semantics and keeps caller intent explicit via flags. Looks correct.
8645-8769: Relationship query conversion aligns with expected M2M behaviorconvertRelationshipQueries/processNestedRelationshipPath handle parent resolution without skipping relationships for M2M, matching repository guidance. LGTM.
Based on learnings
Also applies to: 8458-8466, 8694-8705, 8718-8757
3411-3412: Defensive cache purges with retries are appropriateUsing withRetries around cache purge calls at critical points (relationship/attribute changes) improves resilience under transient cache errors.
Also applies to: 3498-3500, 2885-2890, 2001-2007, 1885-1892
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Database/Database.php (1)
3701-3709: Fix M2M cleanup: drop junction collection, not just metadataUsing deleteDocument(self::METADATA, $junction) leaves the physical junction table orphaned. Call deleteCollection($junction) so the table is dropped too.
- $this->deleteDocument(self::METADATA, $junction); + $this->deleteCollection($junction);
♻️ Duplicate comments (3)
src/Database/Database.php (3)
2345-2351: Good fix: index metadata update no longer emits ATTRIBUTE_UPDATEupdateIndexMeta now persists metadata via updateMetadata without misfiring ATTRIBUTE_UPDATE. This resolves the earlier event confusion.
8988-8991: Backoff units corrected for Coroutine sleepms-to-seconds conversion is now correct (/$1000 for Coroutine, *1000 for usleep). Nice catch.
6-6: Guard optional Swoole usage and avoid hard importAvoid hard import and use FQCN with class_exists to prevent fatals on environments without Swoole. Keep correct time units.
- use Swoole\Coroutine;- if (\extension_loaded('swoole') && Coroutine::getCid() > 0) { - Coroutine::sleep($delayMs / 1000); + if (\class_exists(\Swoole\Coroutine::class, false) && \Swoole\Coroutine::getCid() > 0) { + \Swoole\Coroutine::sleep($delayMs / 1000); } else { \usleep($delayMs * 1000); }Also applies to: 8987-8991
🧹 Nitpick comments (4)
src/Database/Database.php (4)
8956-8964: Docblock says “return void … result”; make it consistentwithRetries returns nothing. Update the docblock accordingly.
- * @return void The result of the operation + * @return void
7434-7471: Don’t let purge events break cache cleanupWrap EVENT_DOCUMENT_PURGE dispatch in try/catch (consistent with other triggers) so a listener failure doesn’t fail purgeCachedDocument.
public function purgeCachedDocument(string $collectionId, ?string $id): bool { $result = $this->purgeCachedDocumentInternal($collectionId, $id); if ($id !== null) { - $this->trigger(self::EVENT_DOCUMENT_PURGE, new Document([ - '$id' => $id, - '$collection' => $collectionId - ])); + try { + $this->trigger(self::EVENT_DOCUMENT_PURGE, new Document([ + '$id' => $id, + '$collection' => $collectionId + ])); + } catch (\Throwable $e) { + // Optionally: Console::warning('document_purge listener failed: ' . $e->getMessage()); + } } return $result; }
2671-2683: Method casing nit: use setAttribute consistentlyMinor readability polish: correct setattribute to setAttribute.
- ->setattribute('key', $newKey ?? $id) + ->setAttribute('key', $newKey ?? $id)
3206-3244: Add retry around junction collection creationCreating the M2M junction collection can transiently fail (DDL). Wrap createCollection in withRetries to align with the new retry strategy used elsewhere.
- $this->silent(fn () => $this->createCollection($junctionCollection, [ + $this->silent(fn () => $this->withRetries(fn () => $this->createCollection($junctionCollection, [ new Document([ '$id' => $id, 'key' => $id, 'type' => self::VAR_STRING, 'size' => Database::LENGTH_KEY, 'required' => true, 'signed' => true, 'array' => false, 'filters' => [], ]), new Document([ '$id' => $twoWayKey, 'key' => $twoWayKey, 'type' => self::VAR_STRING, 'size' => Database::LENGTH_KEY, 'required' => true, 'signed' => true, 'array' => false, 'filters' => [], ]), - ], [ + ], [ new Document([ '$id' => '_index_' . $id, 'key' => 'index_' . $id, 'type' => self::INDEX_KEY, 'attributes' => [$id], ]), new Document([ '$id' => '_index_' . $twoWayKey, 'key' => '_index_' . $twoWayKey, 'type' => self::INDEX_KEY, 'attributes' => [$twoWayKey], ]), - ])); + ])));
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Database/Database.php(35 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
Repo: utopia-php/database PR: 721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
src/Database/Database.php
📚 Learning: 2025-10-29T12:27:57.071Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 747
File: src/Database/Adapter/Mongo.php:1449-1453
Timestamp: 2025-10-29T12:27:57.071Z
Learning: In src/Database/Adapter/Mongo.php, when getSupportForAttributes() returns false (schemaless mode), the updateDocument method intentionally uses a raw document without $set operator for replacement-style updates, as confirmed by the repository maintainer ArnabChatterjee20k.
Applied to files:
src/Database/Database.php
📚 Learning: 2025-10-20T05:29:29.487Z
Learnt from: abnegate
Repo: utopia-php/database PR: 731
File: src/Database/Database.php:6987-6988
Timestamp: 2025-10-20T05:29:29.487Z
Learning: In Utopia\Database\Database::convertRelationshipQueries, Many-to-Many filtering does not need the parent collection or a direct junction query: it calls find() on the related collection without skipping relationships, which populates relationship attributes (including the two-way key). Parent IDs are derived from that populated attribute. Therefore, calls should remain convertRelationshipQueries($relationships, $queries).
Applied to files:
src/Database/Database.php
🧬 Code graph analysis (1)
src/Database/Database.php (3)
src/Database/Adapter/Pool.php (1)
trigger(100-103)src/Database/Adapter.php (2)
trigger(471-481)withTransaction(399-446)src/Database/Document.php (2)
setAttribute(244-261)Document(12-470)
⏰ 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). (14)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Unit Test
🔇 Additional comments (1)
src/Database/Database.php (1)
8695-8712: Relationship query conversion path retainedconvertRelationshipQueries is kept as the integration point (no parent-collection shortcuts), matching prior maintainers’ guidance.
Based on learnings
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/Database/Database.php (2)
6-6: Guard Swoole usage; drop hard importRemove the top-level
use Swoole\Coroutine;and use FQCN behind a class_exists guard to avoid fatals when Swoole isn't installed.Apply with the withRetries change below.
-use Swoole\Coroutine;
8964-8997: withRetries: guard Coroutine with class_exists and FQCNKeep the ms→seconds fix, but avoid referencing
Coroutineunless the class exists. Also prefer FQCN to avoid import-time coupling.- if (\extension_loaded('swoole') && Coroutine::getCid() > 0) { - Coroutine::sleep($delayMs / 1000); - } else { + if (\class_exists(\Swoole\Coroutine::class, false) && \Swoole\Coroutine::getCid() > 0) { + \Swoole\Coroutine::sleep($delayMs / 1000); + } else { \usleep($delayMs * 1000); }
🧹 Nitpick comments (6)
src/Database/Database.php (6)
4001-4001: Wrap EVENT_INDEX_CREATE to avoid failing the operation on listener errorsAlign with other event triggers that are guarded.
- $this->trigger(self::EVENT_INDEX_CREATE, $index); + try { + $this->trigger(self::EVENT_INDEX_CREATE, $index); + } catch (\Throwable $e) { + // Log but don't throw - event failures shouldn't fail the operation + }
3044-3056: Emit purge event for METADATA after renameAttribute metadata writeupdateMetadata runs inside silent() so purge events are suppressed. For consistency with create/delete attribute flows, purge METADATA explicitly and emit EVENT_DOCUMENT_PURGE outside silent.
$this->updateMetadata( collection: $collection, rollbackOperation: fn () => $this->adapter->renameAttribute($collection->getId(), $new, $old), shouldRollback: $renamed, operationDescription: "attribute rename '{$old}' to '{$new}'" ); + + // Purge collection metadata and emit purge event outside silent + $this->withRetries(fn () => $this->purgeCachedDocumentInternal(self::METADATA, $collection->getId())); + try { + $this->trigger(self::EVENT_DOCUMENT_PURGE, new Document([ + '$id' => $collection->getId(), + '$collection' => self::METADATA + ])); + } catch (\Throwable $e) { + // Log but don't throw + }
3487-3546: updateRelationship: purge METADATA docs and emit purge events for both sidesAfter both metadata updates succeed, explicitly purge METADATA docs and emit purge events (outside silent) to keep downstream caches in sync.
} catch (\Throwable $e) { if ($adapterUpdated) { try { $this->adapter->updateRelationship( $collection->getId(), @@ } throw $e; } + + // Purge metadata docs for both collections and emit purge events + $this->withRetries(fn () => $this->purgeCachedDocumentInternal(self::METADATA, $collection->getId())); + $this->withRetries(fn () => $this->purgeCachedDocumentInternal(self::METADATA, $relatedCollection->getId())); + try { + $this->trigger(self::EVENT_DOCUMENT_PURGE, new Document([ + '$id' => $collection->getId(), + '$collection' => self::METADATA + ])); + $this->trigger(self::EVENT_DOCUMENT_PURGE, new Document([ + '$id' => $relatedCollection->getId(), + '$collection' => self::METADATA + ])); + } catch (\Throwable $e) { + // Log but don't throw + }
3732-3755: deleteRelationship: also purge METADATA docs and emit purge eventsPersisting metadata is done inside silent(); explicitly purge + emit to mirror attribute flows and notify listeners.
} catch (\Throwable $e) { try { $this->adapter->createRelationship( $collection->getId(), $relatedCollection->getId(), @@ throw new DatabaseException('Failed to persist metadata after retries: ' . $e->getMessage()); } + // Purge metadata docs and emit purge events for both collections + $this->withRetries(fn () => $this->purgeCachedDocumentInternal(self::METADATA, $collection->getId())); + $this->withRetries(fn () => $this->purgeCachedDocumentInternal(self::METADATA, $relatedCollection->getId())); + try { + $this->trigger(self::EVENT_DOCUMENT_PURGE, new Document([ + '$id' => $collection->getId(), + '$collection' => self::METADATA + ])); + $this->trigger(self::EVENT_DOCUMENT_PURGE, new Document([ + '$id' => $relatedCollection->getId(), + '$collection' => self::METADATA + ])); + } catch (\Throwable $e) { + // Log but don't throw + }
8954-8963: Docblock nit: return description conflictswithRetries returns void; fix “The result of the operation” text.
- * @return void The result of the operation + * @return void
2325-2331: Doc summary nit: method updates index metadata, not attribute metadataUpdate summary to reflect “index”.
- * Update attribute metadata. Utility method for update attribute methods. + * Update index metadata. Utility method for index metadata mutations.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Database/Database.php(35 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
Repo: utopia-php/database PR: 721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
src/Database/Database.php
📚 Learning: 2025-10-29T12:27:57.071Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 747
File: src/Database/Adapter/Mongo.php:1449-1453
Timestamp: 2025-10-29T12:27:57.071Z
Learning: In src/Database/Adapter/Mongo.php, when getSupportForAttributes() returns false (schemaless mode), the updateDocument method intentionally uses a raw document without $set operator for replacement-style updates, as confirmed by the repository maintainer ArnabChatterjee20k.
Applied to files:
src/Database/Database.php
📚 Learning: 2025-10-20T05:29:29.487Z
Learnt from: abnegate
Repo: utopia-php/database PR: 731
File: src/Database/Database.php:6987-6988
Timestamp: 2025-10-20T05:29:29.487Z
Learning: In Utopia\Database\Database::convertRelationshipQueries, Many-to-Many filtering does not need the parent collection or a direct junction query: it calls find() on the related collection without skipping relationships, which populates relationship attributes (including the two-way key). Parent IDs are derived from that populated attribute. Therefore, calls should remain convertRelationshipQueries($relationships, $queries).
Applied to files:
src/Database/Database.php
🧬 Code graph analysis (1)
src/Database/Database.php (3)
src/Database/Adapter/Pool.php (32)
trigger(100-103)createCollection(160-163)createDocument(235-238)deleteCollection(165-168)getSupportForSpatialIndexNull(455-458)getLimitForString(295-298)getLimitForInt(300-303)getSupportForSpatialAttributes(450-453)getSupportForVectors(425-428)count(280-283)getDocumentSizeLimit(480-483)getAttributeWidth(485-488)getSupportForCastIndexArray(355-358)getMaxIndexLength(315-318)getInternalIndexesKeys(510-513)getSupportForIndexArray(350-353)getSupportForSpatialIndexOrder(545-548)getSupportForAttributes(335-338)getSupportForMultipleFulltextIndexes(565-568)getSupportForIdenticalIndexes(570-573)updateAttribute(185-188)deleteAttribute(190-193)createAttribute(175-178)renameAttribute(195-198)updateDocument(245-248)deleteRelationship(210-213)createIndex(220-223)deleteIndex(225-228)updateRelationship(205-208)renameIndex(215-218)createRelationship(200-203)getDocument(230-233)src/Database/Adapter.php (33)
trigger(471-481)createCollection(547-547)createDocument(723-723)deleteCollection(556-556)getSupportForSpatialIndexNull(1100-1100)getLimitForString(872-872)getLimitForInt(879-879)getSupportForSpatialAttributes(1093-1093)getSupportForVectors(1058-1058)count(847-847)getDocumentSizeLimit(1202-1202)getAttributeWidth(1213-1213)getSupportForCastIndexArray(971-971)getMaxIndexLength(898-898)getInternalIndexesKeys(1334-1334)getSupportForIndexArray(964-964)getSupportForSpatialIndexOrder(1121-1121)getSupportForAttributes(943-943)getSupportForMultipleFulltextIndexes(1149-1149)getSupportForIdenticalIndexes(1157-1157)updateAttribute(606-606)deleteAttribute(616-616)createAttribute(579-579)renameAttribute(626-626)withTransaction(399-446)updateDocument(747-747)deleteRelationship(667-667)createIndex(692-692)deleteIndex(702-702)updateRelationship(653-653)renameIndex(677-677)createRelationship(637-637)getDocument(713-713)src/Database/Document.php (3)
setAttribute(244-261)Document(12-470)getAttribute(224-231)
⏰ 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). (14)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Unit Test
🔇 Additional comments (2)
src/Database/Database.php (2)
2345-2351: Good: index metadata update no longer emits attribute eventsRemoving the misleading ATTRIBUTE_UPDATE dispatch here is correct and reduces duplicate/incorrect signals.
7433-7470: API design and documentation already compliantVerification confirms the docblock already includes the type hint suggestion: both
purgeCachedDocumentInternal(line 7429) andpurgeCachedDocumenthave@param string|null $iddocumented. Usage patterns validate the design—internal calls consistently usewithRetries()wrapper (lines 1918, 2043, 2805, 2946), while public calls do not (12 instances), confirming the intended separation and the documented caution about not retrying the event-emitting wrapper.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/Database/Database.php (1)
9014-9018: Harden Swoole guard and drop hard dependency on CoroutinePrefer class_exists + FQCN to avoid accidental fatals and remove the top-level import. Keeps us safe when swoole isn’t installed.
Apply these diffs:
- use Swoole\Coroutine;- if (\extension_loaded('swoole') && Coroutine::getCid() > 0) { - Coroutine::sleep($delayMs / 1000); + if (\class_exists(\Swoole\Coroutine::class, false) && \Swoole\Coroutine::getCid() > 0) { + \Swoole\Coroutine::sleep($delayMs / 1000); } else { \usleep($delayMs * 1000); }Also applies to: 6-6
🧹 Nitpick comments (2)
src/Database/Database.php (2)
2671-2682: Fix method casing: use setAttribute() consistentlyPHP method names are case-insensitive, but keeping canonical casing improves readability and tooling.
Apply this diff:
- ->setattribute('key', $newKey ?? $id) + ->setAttribute('key', $newKey ?? $id)
8982-8992: Docblock nit: return typewithRetries() returns void; the summary says “The result of the operation”. Consider updating for clarity.
- * @return void The result of the operation + * @return void
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Database/Database.php(35 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-03T02:04:17.803Z
Learnt from: abnegate
Repo: utopia-php/database PR: 721
File: tests/e2e/Adapter/Scopes/DocumentTests.php:6418-6439
Timestamp: 2025-10-03T02:04:17.803Z
Learning: In tests/e2e/Adapter/Scopes/DocumentTests::testSchemalessDocumentInvalidInteralAttributeValidation (PHP), when the adapter reports getSupportForAttributes() === false (schemaless), the test should not expect exceptions from createDocuments for “invalid” internal attributes; remove try/catch and ensure the test passes without exceptions, keeping at least one assertion.
Applied to files:
src/Database/Database.php
📚 Learning: 2025-10-29T12:27:57.071Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 747
File: src/Database/Adapter/Mongo.php:1449-1453
Timestamp: 2025-10-29T12:27:57.071Z
Learning: In src/Database/Adapter/Mongo.php, when getSupportForAttributes() returns false (schemaless mode), the updateDocument method intentionally uses a raw document without $set operator for replacement-style updates, as confirmed by the repository maintainer ArnabChatterjee20k.
Applied to files:
src/Database/Database.php
📚 Learning: 2025-10-20T05:29:29.487Z
Learnt from: abnegate
Repo: utopia-php/database PR: 731
File: src/Database/Database.php:6987-6988
Timestamp: 2025-10-20T05:29:29.487Z
Learning: In Utopia\Database\Database::convertRelationshipQueries, Many-to-Many filtering does not need the parent collection or a direct junction query: it calls find() on the related collection without skipping relationships, which populates relationship attributes (including the two-way key). Parent IDs are derived from that populated attribute. Therefore, calls should remain convertRelationshipQueries($relationships, $queries).
Applied to files:
src/Database/Database.php
🧬 Code graph analysis (1)
src/Database/Database.php (4)
src/Database/Adapter/Pool.php (30)
trigger(100-103)createCollection(160-163)createDocument(235-238)getSupportForSpatialIndexNull(455-458)getLimitForString(295-298)getLimitForInt(300-303)getSupportForSpatialAttributes(450-453)getSupportForVectors(425-428)count(280-283)getDocumentSizeLimit(480-483)getAttributeWidth(485-488)getSupportForCastIndexArray(355-358)getMaxIndexLength(315-318)getInternalIndexesKeys(510-513)getSupportForIndexArray(350-353)getSupportForSpatialIndexOrder(545-548)getSupportForAttributes(335-338)getSupportForMultipleFulltextIndexes(565-568)getSupportForIdenticalIndexes(570-573)updateAttribute(185-188)deleteAttribute(190-193)renameAttribute(195-198)deleteCollection(165-168)deleteRelationship(210-213)updateDocument(245-248)createIndex(220-223)deleteIndex(225-228)updateRelationship(205-208)renameIndex(215-218)getDocument(230-233)src/Database/Adapter.php (31)
trigger(471-481)createCollection(547-547)createDocument(723-723)getSupportForSpatialIndexNull(1100-1100)getLimitForString(872-872)getLimitForInt(879-879)getSupportForSpatialAttributes(1093-1093)getSupportForVectors(1058-1058)count(847-847)getDocumentSizeLimit(1202-1202)getAttributeWidth(1213-1213)getSupportForCastIndexArray(971-971)getMaxIndexLength(898-898)getInternalIndexesKeys(1334-1334)getSupportForIndexArray(964-964)getSupportForSpatialIndexOrder(1121-1121)getSupportForAttributes(943-943)getSupportForMultipleFulltextIndexes(1149-1149)getSupportForIdenticalIndexes(1157-1157)updateAttribute(606-606)deleteAttribute(616-616)renameAttribute(626-626)deleteCollection(556-556)deleteRelationship(667-667)withTransaction(399-446)updateDocument(747-747)createIndex(692-692)deleteIndex(702-702)updateRelationship(653-653)renameIndex(677-677)getDocument(713-713)src/Database/Mirror.php (14)
trigger(167-170)createCollection(197-242)silent(172-175)createDocument(540-585)updateAttribute(393-444)deleteAttribute(446-470)renameAttribute(971-974)deleteCollection(270-293)deleteRelationship(999-1002)updateDocument(658-704)createIndex(472-512)deleteIndex(514-538)updateRelationship(988-997)renameIndex(1005-1008)src/Database/Document.php (4)
setAttribute(244-261)Document(12-470)getId(63-66)getAttribute(224-231)
🪛 GitHub Actions: CodeQL
src/Database/Database.php
[error] 9129-9129: PHPStan: Undefined variable: $rollbackException at line 9129 during analysis. Command that failed: './vendor/bin/phpstan analyse --level 7 src tests --memory-limit 2G'.
🪛 GitHub Actions: Linter
src/Database/Database.php
[error] 1-1: Pint lint failed. PSR-12: no_whitespace_in_blank_line in src/Database/Database.php. Command: 'php -d memory_limit=2G ./vendor/bin/pint --test'.
🪛 PHPMD (2.15.0)
src/Database/Database.php
2919-2919: Avoid unused local variables such as '$shouldRollback'. (undefined)
(UnusedLocalVariable)
9129-9129: Avoid unused local variables such as '$rollbackException'. (undefined)
(UnusedLocalVariable)
⏰ 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). (14)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Unit Test
🔇 Additional comments (3)
src/Database/Database.php (3)
7461-7498: Good split: internal purge with retries vs public purge with eventPromoting purgeCachedDocumentInternal() and wrapping purgeCachedDocument() to emit EVENT_DOCUMENT_PURGE prevents event duplication while enabling retryable internal purges. LGTM.
2345-2351: Index metadata update routed through updateMetadata – no misleading ATTRIBUTE_UPDATE eventThis correctly avoids emitting ATTRIBUTE_UPDATE for index changes as flagged earlier. LGTM.
8722-8930: Relationship filter conversion aligns with documented behaviorThe convertRelationshipQueries() path (with nested path handling) mirrors the intended “find on related collection then filter parents” approach, including many‑to‑many without direct junction queries. LGTM.
Based on learnings
Summary by CodeRabbit
Bug Fixes
Refactor