Fix cache clears in finally block #659
Conversation
…ions Co-authored-by: jakeb994 <[email protected]>
…teDocument don't handle it - Remove unnecessary cache clearing from methods that call updateDocument/deleteDocument - Only keep explicit cache clearing where actually needed - deleteCollection: Keep purgeCachedCollection since deleteDocument handles document cache - All other schema operations rely on updateDocument cache clearing
- Ensures purgeCachedCollection is called even if trigger fails - This prevents cache staleness when collection is deleted from adapter but trigger throws exception
- createCollection: Clear collection and metadata document cache if trigger fails after adapter changes - renameAttribute: Clear collection cache if trigger fails after adapter rename - renameIndex: Clear collection cache if trigger fails after adapter rename These prevent cache staleness when adapter operations succeed but subsequent triggers throw exceptions.
- createAttribute: Clear collection cache if trigger fails after adapter creates attribute - deleteAttribute: Clear collection cache if trigger fails after adapter deletes attribute - createIndex: Clear collection cache if trigger fails after adapter creates index - deleteIndex: Clear collection cache if trigger fails after adapter deletes index These operations all follow the pattern: adapter change -> updateDocument -> trigger. If trigger fails, the collection cache could be stale even though updateDocument clears the metadata document cache.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughCentralizes cache invalidation in Database.php by adding try/finally blocks and per-operation success flags across mutating methods so purge calls run deterministically after adapter mutations and triggers; public method signatures remain unchanged. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Database
participant Adapter
participant Triggers
participant Cache
Client->>Database: createCollection(params)
activate Database
Note over Database: try { adapter mutates -> triggers run }
Database->>Adapter: createCollection(...)
Adapter-->>Database: result / exception
Database->>Triggers: onAfterCreateCollection(...)
Triggers-->>Database: result / exception
Note over Database: finally { purge caches if $created or $metadataModified }
Database->>Cache: purgeCachedCollection(collectionId)
Database->>Cache: purgeCachedDocument(METADATA, collectionId)
deactivate Database
Database-->>Client: response / error
sequenceDiagram
participant Client
participant Database
participant Adapter
participant Triggers
participant Cache
Client->>Database: deleteRelationship(colA, colB, relId)
activate Database
Note over Database: try { adapter deletes -> triggers run }
Database->>Adapter: deleteRelationship(...)
Adapter-->>Database: success / exception
Database->>Triggers: onAfterDeleteRelationship(...)
Triggers-->>Database: result / exception
Note over Database: finally { if $relationshipDeleted purge both collections' caches }
Database->>Cache: purgeCachedCollection(colA)
Database->>Cache: purgeCachedCollection(colB)
deactivate Database
Database-->>Client: response / error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Co-authored-by: jakeb994 <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
src/Database/Database.php (1)
2699-2756: Missing finally + misleading comment: caches aren’t cleared for relationship updatesThis block states “Cache clearing is handled by the finally block,” but the method lacks any try/finally. Relationship updates may modify both sides’ metadata (and a junction collection for many-to-many) and rename indexes; deterministic purging is needed for:
- the parent collection,
- the related collection,
- the junction collection for many-to-many relations.
Proposed approach (illustrative; wrap the mutation path and purge both sides deterministically):
// Near the beginning of updateRelationship(), after resolving $collection, $attribute, $type, $side, and $relatedCollection: $collectionIdToPurge = $collection->getId(); $relatedCollectionIdToPurge = $relatedCollection->getId(); $typeToPurge = $type; $sideToPurge = $side; try { // ... existing mutation logic (updateAttributeMeta calls, adapter->updateRelationship, index renames) ... return true; } finally { // Ensure caches are cleared even if triggers fail $this->purgeCachedCollection($collectionIdToPurge); $this->purgeCachedCollection($relatedCollectionIdToPurge); if ($typeToPurge === self::RELATION_MANY_TO_MANY) { $junction = $this->getJunctionCollection($collection, $relatedCollection, $sideToPurge); $this->purgeCachedCollection($junction); } }Also, update or remove the misleading in-line comment stating that a finally block handles cache clearing.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Database/Database.php(10 hunks)
🔇 Additional comments (5)
src/Database/Database.php (5)
1322-1333: Good: finally-based purge only when the collection document was createdThe try/finally structure is correct. Purging on successful metadata creation via createdCollection !== null aligns with the PR objective. No change needed.
1525-1542: Good: deterministic purge on collection deletionUnconditional purge in finally ensures caches are cleared even if the trigger fails. Since the metadata document deletion path purges its own cache, this is consistent. No change needed.
2337-2362: Good: purge gated by successful adapter delete; NotFound path handledThe flow updates metadata only after a successful adapter delete and purges in finally when $deleted is true. The NotFound case returns true without persisting metadata and skips purge, which is consistent. No change needed.
2430-2446: Good: rename attribute purges collection in finallyUnconditional purge in finally ensures determinism even if downstream triggers fail. LGTM.
3013-3026: Good: rename index purges collection in finallyThis is consistent with the objective; deterministic purge even if triggers fail. LGTM.
Co-authored-by: jakeb994 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/Database/Database.php (4)
1597-1630: Create attribute: migration/duplicate path updates metadata but skips purge (stale cache risk)If DuplicateException is swallowed during migration, you persist metadata and emit the event, but
$createdstays false so finally won’t purge. Add a secondary flag and purge on either success condition.Apply this minimal diff:
- $created = false; + $created = false; + $shouldPurge = false; try { $created = $this->adapter->createAttribute($collection->getId(), $id, $type, $size, $signed, $array); @@ - if ($collection->getId() !== self::METADATA) { - $this->silent(fn () => $this->updateDocument(self::METADATA, $collection->getId(), $collection)); - } + if ($collection->getId() !== self::METADATA) { + $this->silent(fn () => $this->updateDocument(self::METADATA, $collection->getId(), $collection)); + $shouldPurge = true; + } @@ - if ($collection->getId() !== self::METADATA) { - $this->silent(fn () => $this->updateDocument(self::METADATA, $collection->getId(), $collection)); - } + if ($collection->getId() !== self::METADATA) { + $this->silent(fn () => $this->updateDocument(self::METADATA, $collection->getId(), $collection)); + $shouldPurge = true; + } @@ - if ($created) { + if ($created || $shouldPurge) { $this->purgeCachedCollection($collection->getId()); }
1715-1750: Create attributes (batch): migration/duplicate path can skip purge; gate purge on metadata persistence tooSame issue as single-attribute create: you update metadata + fire event in the DuplicateException branch but
$createdremains false, so finally won't purge the collection cache.Proposed patch:
- $created = false; + $created = false; + $shouldPurge = false; try { $created = $this->adapter->createAttributes($collection->getId(), $attributes); @@ - if ($collection->getId() !== self::METADATA) { - $this->silent(fn () => $this->updateDocument(self::METADATA, $collection->getId(), $collection)); - } + if ($collection->getId() !== self::METADATA) { + $this->silent(fn () => $this->updateDocument(self::METADATA, $collection->getId(), $collection)); + $shouldPurge = true; + } @@ - if ($collection->getId() !== self::METADATA) { - $this->silent(fn () => $this->updateDocument(self::METADATA, $collection->getId(), $collection)); - } + if ($collection->getId() !== self::METADATA) { + $this->silent(fn () => $this->updateDocument(self::METADATA, $collection->getId(), $collection)); + $shouldPurge = true; + } @@ - if ($created) { + if ($created || $shouldPurge) { $this->purgeCachedCollection($collection->getId()); $this->purgeCachedDocument(self::METADATA, $collection->getId()); }
3160-3193: Create index: migration/duplicate path can skip purge like attributesMetadata is updated and event fired in DuplicateException migration branch, but
$createdremains false, so finally won’t purge the collection cache. Track a secondary flag and purge on either success.Suggested change:
- $created = false; + $created = false; + $shouldPurge = false; try { $created = $this->adapter->createIndex($collection->getId(), $id, $type, $attributes, $lengths, $orders, $indexAttributesWithTypes); @@ - if ($collection->getId() !== self::METADATA) { - $this->silent(fn () => $this->updateDocument(self::METADATA, $collection->getId(), $collection)); - } + if ($collection->getId() !== self::METADATA) { + $this->silent(fn () => $this->updateDocument(self::METADATA, $collection->getId(), $collection)); + $shouldPurge = true; + } @@ - if ($collection->getId() !== self::METADATA) { - $this->silent(fn () => $this->updateDocument(self::METADATA, $collection->getId(), $collection)); - } + if ($collection->getId() !== self::METADATA) { + $this->silent(fn () => $this->updateDocument(self::METADATA, $collection->getId(), $collection)); + $shouldPurge = true; + } @@ - if ($created) { + if ($created || $shouldPurge) { $this->purgeCachedCollection($collection->getId()); }
3222-3240: Delete index: metadata update + event fire even when adapter delete failsIf the adapter returns false, you still persist metadata and emit EVENT_INDEX_DELETE, and finally won’t purge. Throw on failure and only persist metadata + trigger on success.
Apply this diff:
- $deleted = false; + $deleted = false; try { - $deleted = $this->adapter->deleteIndex($collection->getId(), $id); + $deleted = $this->adapter->deleteIndex($collection->getId(), $id); + if (!$deleted) { + throw new DatabaseException('Failed to delete index'); + } $collection->setAttribute('indexes', \array_values($indexes)); if ($collection->getId() !== self::METADATA) { $this->silent(fn () => $this->updateDocument(self::METADATA, $collection->getId(), $collection)); } $this->trigger(self::EVENT_INDEX_DELETE, $indexDeleted); return $deleted; } finally { // Ensure collection cache is cleared even if trigger fails after adapter changes if ($deleted) { $this->purgeCachedCollection($collection->getId()); } }
🧹 Nitpick comments (2)
src/Database/Database.php (2)
2713-2713: Fix Pint error: whitespace on blank line(s)Pint is failing with no_whitespace_in_blank_line. There are whitespace-only blank lines introduced (e.g., Lines 1723, 2713). Strip trailing spaces on blank lines.
Example fix:
- +Please run Pint locally to confirm:
- php -d memory_limit=2G ./vendor/bin/pint
Also applies to: 1723-1723
2770-2771: Misleading comment: there is no finally block here“Cache clearing is handled by the finally block” doesn’t apply in this scope. Purge occurs in downstream calls (e.g., renameIndex has its own finally; updateDocument purges metadata). Adjust the comment or add a small try/finally here if you want to standardize the pattern.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Database/Database.php(11 hunks)
🧰 Additional context used
🪛 GitHub Actions: Linter
src/Database/Database.php
[error] 1-1: Pint lint failed due to PSR-12 violation: no_whitespace_in_blank_line in src/Database/Database.php. Command: 'php -d memory_limit=2G ./vendor/bin/pint --test'
⏰ 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). (8)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (MySQL)
🔇 Additional comments (4)
src/Database/Database.php (4)
1322-1333: createCollection: finally-based dual cache purge looks correctGood: purge both the collection and corresponding metadata document on success; guarded in finally to survive trigger failures.
1525-1542: deleteCollection: unconditional purge in finally is appropriateGiven the adapter delete is already attempted, clearing the collection cache regardless is reasonable to avoid stale entries.
2445-2461: renameAttribute: finally-based purge is consistent with the PR’s patternGood shift to finally; even if triggers fail after adapter change, collection cache is cleared.
3028-3041: renameIndex: finally-based purge is correctGood move to clear cache regardless of downstream trigger failures.
Co-authored-by: jakeb994 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/Database/Database.php (3)
1718-1756: createAttributes: mirrors attribute flow and purges metadata doc — LGTMThe additional purge of the metadata document is fine, even if redundant in some paths.
2720-2872: updateRelationship: finalization clears both collections — LGTMThe try/finally ensures deterministic invalidation across both collections. Acknowledging potential redundancy, per prior guidance to keep redundant purges.
3253-3271: deleteIndex: persist/trigger happen even when adapter delete fails; add failure guardMetadata and event emit even if $deleted is false, and finally purges only when $deleted is true. This diverges state and can leave caches stale. Align with other mutators: throw on failure before persisting metadata and emitting events.
$deleted = false; try { $deleted = $this->adapter->deleteIndex($collection->getId(), $id); + if (!$deleted) { + throw new DatabaseException('Failed to delete index'); + } $collection->setAttribute('indexes', \array_values($indexes)); if ($collection->getId() !== self::METADATA) { $this->silent(fn () => $this->updateDocument(self::METADATA, $collection->getId(), $collection)); } $this->trigger(self::EVENT_INDEX_DELETE, $indexDeleted); return $deleted; } finally { // Ensure collection cache is cleared even if trigger fails after adapter changes if ($deleted) { $this->purgeCachedCollection($collection->getId()); } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Database/Database.php(11 hunks)
🧰 Additional context used
🪛 GitHub Actions: Linter
src/Database/Database.php
[error] 1-1: PSR-12: no_whitespace_in_blank_line violation detected by Pint in this file.
⏰ 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). (3)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (Pool)
🔇 Additional comments (7)
src/Database/Database.php (7)
1322-1333: Deterministic purge in finally for createCollection — LGTMGood use of finally with a success sentinel; purging both the collection and its metadata document is correct.
1525-1542: Unconditional purge after adapter delete — LGTMClearing collection cache in finally regardless of trigger outcome matches the PR goal and avoids stale state.
1597-1633: createAttribute: success/metadata flags + finally purge — LGTMGating purge on either adapter success or metadata update handles migration/duplicate paths correctly.
2925-3010: deleteRelationship: success flag + finally purge for both collections — LGTMThe relationshipDeleted flag protects against unnecessary clears while ensuring consistency when deletion completes.
3056-3069: renameIndex: finally-based purge — LGTMPurge on exit aligns with the centralized invalidation approach.
3188-3224: createIndex: success/metadata flags + finally purge — LGTMCovers duplicate/migration paths and ensures purge when metadata changed even if adapter path differs.
1-2: Fix Pint PSR-12 lint error: whitespace in blank lineThe linter reports “no_whitespace_in_blank_line” for this file. Please run Pint and commit formatting fixes.
You can verify and fix locally with:
- vendor/bin/pint -v
- Or php-cs-fixer equivalent if you use it in this repo.
| $deleted = false; | ||
| try { | ||
| if (!$this->adapter->deleteAttribute($collection->getId(), $id)) { | ||
| $deleted = $this->adapter->deleteAttribute($collection->getId(), $id); | ||
| if (!$deleted) { | ||
| throw new DatabaseException('Failed to delete attribute'); | ||
| } | ||
| } catch (NotFoundException) { | ||
| // Ignore | ||
| } | ||
|
|
||
| $collection->setAttribute('attributes', \array_values($attributes)); | ||
| $collection->setAttribute('indexes', \array_values($indexes)); | ||
|
|
||
| if ($collection->getId() !== self::METADATA) { | ||
| $this->silent(fn () => $this->updateDocument(self::METADATA, $collection->getId(), $collection)); | ||
| } | ||
| $collection->setAttribute('attributes', \array_values($attributes)); | ||
| $collection->setAttribute('indexes', \array_values($indexes)); | ||
|
|
||
| $this->purgeCachedCollection($collection->getId()); | ||
| $this->purgeCachedDocument(self::METADATA, $collection->getId()); | ||
| if ($collection->getId() !== self::METADATA) { | ||
| $this->silent(fn () => $this->updateDocument(self::METADATA, $collection->getId(), $collection)); | ||
| } | ||
|
|
||
| $this->trigger(self::EVENT_ATTRIBUTE_DELETE, $attribute); | ||
| $this->trigger(self::EVENT_ATTRIBUTE_DELETE, $attribute); | ||
|
|
||
| return true; | ||
| return true; | ||
| } catch (NotFoundException) { | ||
| // Ignore - attribute doesn't exist | ||
| return true; | ||
| } finally { | ||
| // Ensure collection cache is cleared even if trigger fails after adapter changes | ||
| if ($deleted) { | ||
| $this->purgeCachedCollection($collection->getId()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Idempotent deleteAttribute should still persist metadata and purge caches on NotFound
Currently, catching NotFound returns early without persisting metadata changes or purging, leaving metadata/caches stale if the attribute exists only in metadata. Treat NotFound as idempotent success: persist the metadata removal, fire the event, mark as deleted to ensure purge in finally.
} catch (NotFoundException) {
- // Ignore - attribute doesn't exist
- return true;
+ // Idempotent: persist metadata and clear caches even if adapter had nothing to delete
+ $collection->setAttribute('attributes', \array_values($attributes));
+ $collection->setAttribute('indexes', \array_values($indexes));
+ if ($collection->getId() !== self::METADATA) {
+ $this->silent(fn () => $this->updateDocument(self::METADATA, $collection->getId(), $collection));
+ }
+ $this->trigger(self::EVENT_ATTRIBUTE_DELETE, $attribute);
+ $deleted = true; // ensure finally purges
+ return true;
} finally {
// Ensure collection cache is cleared even if trigger fails after adapter changes
if ($deleted) {
$this->purgeCachedCollection($collection->getId());
}
}📝 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.
| $deleted = false; | |
| try { | |
| if (!$this->adapter->deleteAttribute($collection->getId(), $id)) { | |
| $deleted = $this->adapter->deleteAttribute($collection->getId(), $id); | |
| if (!$deleted) { | |
| throw new DatabaseException('Failed to delete attribute'); | |
| } | |
| } catch (NotFoundException) { | |
| // Ignore | |
| } | |
| $collection->setAttribute('attributes', \array_values($attributes)); | |
| $collection->setAttribute('indexes', \array_values($indexes)); | |
| if ($collection->getId() !== self::METADATA) { | |
| $this->silent(fn () => $this->updateDocument(self::METADATA, $collection->getId(), $collection)); | |
| } | |
| $collection->setAttribute('attributes', \array_values($attributes)); | |
| $collection->setAttribute('indexes', \array_values($indexes)); | |
| $this->purgeCachedCollection($collection->getId()); | |
| $this->purgeCachedDocument(self::METADATA, $collection->getId()); | |
| if ($collection->getId() !== self::METADATA) { | |
| $this->silent(fn () => $this->updateDocument(self::METADATA, $collection->getId(), $collection)); | |
| } | |
| $this->trigger(self::EVENT_ATTRIBUTE_DELETE, $attribute); | |
| $this->trigger(self::EVENT_ATTRIBUTE_DELETE, $attribute); | |
| return true; | |
| return true; | |
| } catch (NotFoundException) { | |
| // Ignore - attribute doesn't exist | |
| return true; | |
| } finally { | |
| // Ensure collection cache is cleared even if trigger fails after adapter changes | |
| if ($deleted) { | |
| $this->purgeCachedCollection($collection->getId()); | |
| } | |
| } | |
| $deleted = false; | |
| try { | |
| $deleted = $this->adapter->deleteAttribute($collection->getId(), $id); | |
| if (!$deleted) { | |
| throw new DatabaseException('Failed to delete attribute'); | |
| } | |
| $collection->setAttribute('attributes', \array_values($attributes)); | |
| $collection->setAttribute('indexes', \array_values($indexes)); | |
| if ($collection->getId() !== self::METADATA) { | |
| $this->silent(fn () => $this->updateDocument(self::METADATA, $collection->getId(), $collection)); | |
| } | |
| $this->trigger(self::EVENT_ATTRIBUTE_DELETE, $attribute); | |
| return true; | |
| } catch (NotFoundException) { | |
| // Idempotent: persist metadata and clear caches even if adapter had nothing to delete | |
| $collection->setAttribute('attributes', \array_values($attributes)); | |
| $collection->setAttribute('indexes', \array_values($indexes)); | |
| if ($collection->getId() !== self::METADATA) { | |
| $this->silent(fn () => $this->updateDocument(self::METADATA, $collection->getId(), $collection)); | |
| } | |
| $this->trigger(self::EVENT_ATTRIBUTE_DELETE, $attribute); | |
| $deleted = true; // ensure finally purges | |
| return true; | |
| } finally { | |
| // Ensure collection cache is cleared even if trigger fails after adapter changes | |
| if ($deleted) { | |
| $this->purgeCachedCollection($collection->getId()); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/Database/Database.php around lines 2358 to 2383, the NotFound catch
returns early so metadata updates and cache purge are skipped; change the catch
to treat NotFound as idempotent success by persisting the metadata changes (call
updateDocument for METADATA-as-before), firing the EVENT_ATTRIBUTE_DELETE, and
setting $deleted = true so the finally block will purge the cached collection —
then return true. Ensure the same guards (skip updateDocument when collection id
is self::METADATA) are applied and preserve existing behavior when delete
actually succeeds.
| try { | ||
| $renamed = $this->adapter->renameAttribute($collection->getId(), $old, $new); | ||
|
|
||
| $collection->setAttribute('attributes', $attributes); | ||
| $collection->setAttribute('indexes', $indexes); | ||
| $collection->setAttribute('attributes', $attributes); | ||
| $collection->setAttribute('indexes', $indexes); | ||
|
|
||
| if ($collection->getId() !== self::METADATA) { | ||
| $this->silent(fn () => $this->updateDocument(self::METADATA, $collection->getId(), $collection)); | ||
| } | ||
| if ($collection->getId() !== self::METADATA) { | ||
| $this->silent(fn () => $this->updateDocument(self::METADATA, $collection->getId(), $collection)); | ||
| } | ||
|
|
||
| $this->trigger(self::EVENT_ATTRIBUTE_UPDATE, $attribute); | ||
| $this->trigger(self::EVENT_ATTRIBUTE_UPDATE, $attribute); | ||
|
|
||
| return $renamed; | ||
| return $renamed; | ||
| } finally { | ||
| // Ensure collection cache is cleared even if trigger fails after adapter changes | ||
| $this->purgeCachedCollection($collection->getId()); | ||
| } |
There was a problem hiding this comment.
Guard on adapter rename result to avoid diverging metadata
If adapter->renameAttribute fails (returns false), we should not persist metadata or emit events. Throw on failure before updating metadata; keep the finally-based purge.
try {
$renamed = $this->adapter->renameAttribute($collection->getId(), $old, $new);
+ if (!$renamed) {
+ throw new DatabaseException('Failed to rename attribute');
+ }
$collection->setAttribute('attributes', $attributes);
$collection->setAttribute('indexes', $indexes);
if ($collection->getId() !== self::METADATA) {
$this->silent(fn () => $this->updateDocument(self::METADATA, $collection->getId(), $collection));
}
$this->trigger(self::EVENT_ATTRIBUTE_UPDATE, $attribute);
return $renamed;
} finally {
// Ensure collection cache is cleared even if trigger fails after adapter changes
$this->purgeCachedCollection($collection->getId());
}📝 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.
| try { | |
| $renamed = $this->adapter->renameAttribute($collection->getId(), $old, $new); | |
| $collection->setAttribute('attributes', $attributes); | |
| $collection->setAttribute('indexes', $indexes); | |
| $collection->setAttribute('attributes', $attributes); | |
| $collection->setAttribute('indexes', $indexes); | |
| if ($collection->getId() !== self::METADATA) { | |
| $this->silent(fn () => $this->updateDocument(self::METADATA, $collection->getId(), $collection)); | |
| } | |
| if ($collection->getId() !== self::METADATA) { | |
| $this->silent(fn () => $this->updateDocument(self::METADATA, $collection->getId(), $collection)); | |
| } | |
| $this->trigger(self::EVENT_ATTRIBUTE_UPDATE, $attribute); | |
| $this->trigger(self::EVENT_ATTRIBUTE_UPDATE, $attribute); | |
| return $renamed; | |
| return $renamed; | |
| } finally { | |
| // Ensure collection cache is cleared even if trigger fails after adapter changes | |
| $this->purgeCachedCollection($collection->getId()); | |
| } | |
| try { | |
| $renamed = $this->adapter->renameAttribute($collection->getId(), $old, $new); | |
| if (!$renamed) { | |
| throw new DatabaseException('Failed to rename attribute'); | |
| } | |
| $collection->setAttribute('attributes', $attributes); | |
| $collection->setAttribute('indexes', $indexes); | |
| if ($collection->getId() !== self::METADATA) { | |
| $this->silent(fn () => $this->updateDocument(self::METADATA, $collection->getId(), $collection)); | |
| } | |
| $this->trigger(self::EVENT_ATTRIBUTE_UPDATE, $attribute); | |
| return $renamed; | |
| } finally { | |
| // Ensure collection cache is cleared even if trigger fails after adapter changes | |
| $this->purgeCachedCollection($collection->getId()); | |
| } |
🤖 Prompt for AI Agents
In src/Database/Database.php around lines 2451 to 2467, the code proceeds to
update metadata and emit events even if $this->adapter->renameAttribute returned
false; change the flow so you first capture the adapter result into $renamed,
and if $renamed is false throw an exception (or return/fail) immediately before
mutating $collection or calling updateDocument/trigger, while leaving the
existing finally block that calls
$this->purgeCachedCollection($collection->getId()) intact.
Co-authored-by: jakeb994 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/Database/Database.php (1)
1-1: Remove whitespace from blank lines in Database.phpTwo blank lines in
src/Database/Database.phpcontain spaces, violating PSR-12’sno_whitespace_in_blank_linerule. Please strip the whitespace (e.g., re-run Pint or delete spaces manually):• src/Database/Database.php:1725
• src/Database/Database.php:1740
♻️ Duplicate comments (3)
src/Database/Database.php (3)
2356-2381: deleteAttribute NotFound path returns early without persisting metadata or purgingOn NotFound, you return before updating metadata and before setting
$deleted, so finally won’t purge. This can leave metadata/caches stale if metadata contained the attribute but the adapter did not. Treat NotFound as idempotent success: persist metadata removal, fire the event, mark$deleted = trueto ensure purge.Apply this diff:
} catch (NotFoundException) { - // Ignore - attribute doesn't exist - return true; + // Idempotent: persist metadata and clear caches even if adapter had nothing to delete + $collection->setAttribute('attributes', \array_values($attributes)); + $collection->setAttribute('indexes', \array_values($indexes)); + if ($collection->getId() !== self::METADATA) { + $this->silent(fn () => $this->updateDocument(self::METADATA, $collection->getId(), $collection)); + } + $this->trigger(self::EVENT_ATTRIBUTE_DELETE, $attribute); + $deleted = true; // ensure finally purges + return true; } finally { // Ensure collection cache is cleared even if trigger fails after adapter changes if ($deleted) { $this->purgeCachedCollection($collection->getId()); } }
2449-2465: Guard on adapter rename result to avoid diverging metadataIf
adapter->renameAttributefails, code still persists metadata and triggers an update, diverging metadata from schema. Throw on failure before metadata changes to keep state consistent.Apply this diff:
try { - $renamed = $this->adapter->renameAttribute($collection->getId(), $old, $new); + $renamed = $this->adapter->renameAttribute($collection->getId(), $old, $new); + if (!$renamed) { + throw new DatabaseException('Failed to rename attribute'); + } $collection->setAttribute('attributes', $attributes); $collection->setAttribute('indexes', $indexes); if ($collection->getId() !== self::METADATA) { $this->silent(fn () => $this->updateDocument(self::METADATA, $collection->getId(), $collection)); } $this->trigger(self::EVENT_ATTRIBUTE_UPDATE, $attribute); return $renamed; } finally { // Ensure collection cache is cleared even if trigger fails after adapter changes $this->purgeCachedCollection($collection->getId()); }
3251-3269: deleteIndex persists metadata and triggers regardless of adapter resultCurrently metadata is updated and event emitted even if
$deletedis false, and finally purges only when$deletedis true. Throw on failure to keep state consistent and ensure purge reflects actual change.Apply this diff:
- $deleted = false; + $deleted = false; try { $deleted = $this->adapter->deleteIndex($collection->getId(), $id); + if (!$deleted) { + throw new DatabaseException('Failed to delete index'); + } $collection->setAttribute('indexes', \array_values($indexes)); if ($collection->getId() !== self::METADATA) { $this->silent(fn () => $this->updateDocument(self::METADATA, $collection->getId(), $collection)); } $this->trigger(self::EVENT_INDEX_DELETE, $indexDeleted); return $deleted; } finally { // Ensure collection cache is cleared even if trigger fails after adapter changes if ($deleted) { $this->purgeCachedCollection($collection->getId()); } }
🧹 Nitpick comments (1)
src/Database/Database.php (1)
2797-2809: Consider avoiding double metadata writes when renaming indexesYou first call
updateIndexMetato update index attributes, then callrenameIndex, which updates metadata again and emits a separate event. This doubles metadata writes/events for the same rename intent.If both events are not required, consider consolidating to one path (prefer
renameIndexonly, or extendrenameIndexto accept an updated attributes list).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Database/Database.php(11 hunks)
🧰 Additional context used
🪛 GitHub Actions: Linter
src/Database/Database.php
[error] 1-1: Pint lint failed (exit code 1): PSR-12 'no_whitespace_in_blank_line' violation detected in src/Database/Database.php.
🪛 GitHub Actions: Tests
src/Database/Database.php
[error] 6178-6178: Utopia\Database\Exception\Query: Invalid query: Attribute not found in schema: with-dash
[error] 6178-6178: Utopia\Database\Exception\Query: Invalid query: Attribute not found in schema: active
[error] 6178-6178: Utopia\Database\Exception\Query: Invalid query: Attribute not found in schema: director
[error] 6178-6178: Utopia\Database\Exception\Query: Invalid query: Attribute not found in schema: director
[error] 6178-6178: Utopia\Database\Exception\Query: Invalid query: Attribute not found in schema: price
[error] 6178-6178: Utopia\Database\Exception\Query: Invalid query: Attribute not found in schema: price
[error] 6178-6178: Utopia\Database\Exception\Query: Invalid query: Attribute not found in schema: director
[error] 6178-6178: Utopia\Database\Exception\Query: Invalid query: Attribute not found in schema: price
[error] 6178-6178: Utopia\Database\Exception\Query: Invalid query: Attribute not found in schema: price
[error] 6178-6178: Utopia\Database\Exception\Query: Invalid query: Attribute not found in schema: year
[error] 6178-6178: Utopia\Database\Exception\Query: Invalid query: Attribute not found in schema: year
[error] 6178-6178: Utopia\Database\Exception\Query: Invalid query: Attribute not found in schema: price
[error] 6178-6178: Utopia\Database\Exception\Query: Invalid query: Attribute not found in schema: price
[error] 6178-6178: Utopia\Database\Exception\Query: Invalid query: Attribute not found in schema: name
[error] 6178-6178: Utopia\Database\Exception\Query: Invalid query: Attribute not found in schema: name
[error] 6178-6178: Utopia\Database\Exception\Query: Invalid query: Attribute not found in schema: director
[error] 6178-6178: Utopia\Database\Exception\Query: Invalid query: Attribute not found in schema: director
[error] 6178-6178: Utopia\Database\Exception\Query: Invalid query: Attribute not found in schema: active
[error] 6178-6178: Utopia\Database\Exception\Query: Invalid query: Attribute not found in schema: active
[error] 6178-6178: Utopia\Database\Exception\Query: Invalid query: Attribute not found in schema: nullable
[error] 6178-6178: Utopia\Database\Exception\Query: Invalid query: Attribute not found in schema: nullable
[error] 6178-6178: Utopia\Database\Exception\Query: Invalid query: Attribute not found in schema: name
[error] 6178-6178: Utopia\Database\Exception\Query: Invalid query: Attribute not found in schema: name
[error] 6178-6178: Utopia\Database\Exception\Query: Invalid query: Attribute not found in schema: price
[error] 6178-6178: Utopia\Database\Exception\Query: Invalid query: Attribute not found in schema: name
[error] 6178-6178: Utopia\Database\Exception\Query: Invalid query: Attribute not found in schema: name
[error] 6178-6178: Utopia\Database\Exception\Query: Invalid query: Attribute not found in schema: price
[error] 6178-6178: Utopia\Database\Exception\Query: Invalid query: Attribute not found in schema: name
[error] 6178-6178: Utopia\Database\Exception\Query: Invalid query: Attribute not found in schema: name
[error] 6178-6178: Utopia\Database\Exception\Query: Invalid query: Attribute not found in schema: price
[error] 6178-6178: Utopia\Database\Exception\Query: Invalid query: Attribute not found in schema: name
[error] 6178-6178: Utopia\Database\Exception\Query: Invalid query: Attribute not found in schema: name
[error] 6178-6178: Utopia\Database\Exception\Query: Invalid query: Attribute not found in schema: price
[error] 6178-6178: Utopia\Database\Exception\Query: Invalid query: Attribute not found in schema: name
[error] 6178-6178: Utopia\Database\Exception\Query: Invalid query: Attribute not found in schema: name
[error] 6178-6178: Utopia\Database\Exception\Query: Invalid query: Attribute not found in schema: price
[error] 6178-6178: Utopia\Database\Exception\Query: Invalid query: Attribute not found in schema: name
[error] 1248-1248: Utopia\Database\Exception\Duplicate: Collection indexes already exists
[error] 3770-3770: Utopia\Database\Exception\Structure: Invalid document structure: Unknown attribute: 'string'
[error] 3770-3770: Utopia\Database\Exception\Structure: Invalid document structure: Unknown attribute: 'string'
🔇 Additional comments (9)
src/Database/Database.php (9)
1322-1332: createCollection: finally-based purge looks goodPurging only when the metadata document was successfully created (
$createdCollection !== null) is correct and avoids unnecessary cache churn.
1524-1541: deleteCollection: unconditional final purge aligned with intentGood move to clear cache in finally regardless of
$deleted. This matches the “adapter.deleteCollection() was called” rationale and avoids stale entries if trigger fails.
1596-1632: createAttribute: fixes stale-cache risks in both normal and migration pathsThe
$created+$metadataModifiedflags with a finally purge cover both success and DuplicateException migration flows. This addresses the prior “skip purge on migration path” risk.
1717-1754: createAttributes: mirrors robust purge semantics from createAttributeSame solid pattern with
$createdand$metadataModifiedand a guarded finally purge.
2718-2870: updateRelationship: deterministic cache cleanup for both collectionsWrapping the entire flow and purging both involved collection caches in finally is the right call given multi-collection impact.
2923-3008: deleteRelationship: finally-based dual purge executed only on successful adapter deleteThe
$relationshipDeletedflag ensures finally purges both collections only when the adapter deletion succeeded. Given index deletions already purge during the metadata phase, this keeps the semantics safe. LGTM.
3186-3222: createIndex: correct purge semantics in normal and migration pathsThe
$created/$metadataModifiedflags and finally purge cover adapter-success and migration branch consistently. Nice.
6109-6117: Removing EVENT_DOCUMENT_PURGE emission: confirm no listeners rely on it
purgeCachedDocumentno longer triggersEVENT_DOCUMENT_PURGE. If downstream listeners use this event, this is a behavioral change. If intentional, consider documenting; otherwise, reintroduce the trigger here or where appropriate.Would you like me to re-add the event trigger here (non-silent) or emit a dedicated “cache_purged” event to avoid coupling?
6159-6206: Verify selections and schema mapping before merging
Ensure that attribute-based queries and selections correctly reference the collection’s schema at runtime:
- Database::validateSelections (src/Database/Database.php around line 6734) calls getSchemaAttributes($collection->getId()) to build the list of valid attribute IDs.
- All adapters (SQL, SQLite, MariaDB, Postgres, Pool) implement getSchemaAttributes(); confirm they return every column name, including dashed or nullable fields.
- DocumentsValidator is invoked in Database::find/select paths (around lines 1798, 3320, 6216) to validate filters and selections.
- Tests in tests/unit/Validator/QueryTest.php and tests/e2e/Adapter/Scopes/DocumentTests.php reference attributes “name”, “director”, “year”, “price”, “active”, “nullable”, “with-dash” – run the full suite against each adapter to catch any QueryException (“Attribute not found in schema…”).
Please manually verify that getSchemaAttributes correctly lists all attributes for each adapter and that validateSelections/ DocumentsValidator accept selections matching your intended schema.
| try { | ||
| $this->adapter->renameIndex($collection->getId(), $old, $new); | ||
|
|
||
| if ($collection->getId() !== self::METADATA) { | ||
| $this->silent(fn () => $this->updateDocument(self::METADATA, $collection->getId(), $collection)); | ||
| } | ||
| if ($collection->getId() !== self::METADATA) { | ||
| $this->silent(fn () => $this->updateDocument(self::METADATA, $collection->getId(), $collection)); | ||
| } | ||
|
|
||
| $this->trigger(self::EVENT_INDEX_RENAME, $indexNew); | ||
| $this->trigger(self::EVENT_INDEX_RENAME, $indexNew); | ||
|
|
||
| return true; | ||
| return true; | ||
| } finally { | ||
| // Ensure collection cache is cleared even if trigger fails after adapter changes | ||
| $this->purgeCachedCollection($collection->getId()); | ||
| } |
There was a problem hiding this comment.
renameIndex lacks adapter-result guard; risk of metadata/schema divergence
If the adapter rename fails, metadata is still persisted and event emitted. Gate metadata update and event emission on actual adapter success.
Apply this diff:
try {
- $this->adapter->renameIndex($collection->getId(), $old, $new);
+ $renamed = $this->adapter->renameIndex($collection->getId(), $old, $new);
+ if (!$renamed) {
+ throw new DatabaseException('Failed to rename index');
+ }
if ($collection->getId() !== self::METADATA) {
$this->silent(fn () => $this->updateDocument(self::METADATA, $collection->getId(), $collection));
}
$this->trigger(self::EVENT_INDEX_RENAME, $indexNew);
- return true;
+ return true;
} finally {
// Ensure collection cache is cleared even if trigger fails after adapter changes
$this->purgeCachedCollection($collection->getId());
}📝 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.
| try { | |
| $this->adapter->renameIndex($collection->getId(), $old, $new); | |
| if ($collection->getId() !== self::METADATA) { | |
| $this->silent(fn () => $this->updateDocument(self::METADATA, $collection->getId(), $collection)); | |
| } | |
| if ($collection->getId() !== self::METADATA) { | |
| $this->silent(fn () => $this->updateDocument(self::METADATA, $collection->getId(), $collection)); | |
| } | |
| $this->trigger(self::EVENT_INDEX_RENAME, $indexNew); | |
| $this->trigger(self::EVENT_INDEX_RENAME, $indexNew); | |
| return true; | |
| return true; | |
| } finally { | |
| // Ensure collection cache is cleared even if trigger fails after adapter changes | |
| $this->purgeCachedCollection($collection->getId()); | |
| } | |
| try { | |
| $renamed = $this->adapter->renameIndex($collection->getId(), $old, $new); | |
| if (!$renamed) { | |
| throw new DatabaseException('Failed to rename index'); | |
| } | |
| if ($collection->getId() !== self::METADATA) { | |
| $this->silent(fn () => $this->updateDocument(self::METADATA, $collection->getId(), $collection)); | |
| } | |
| $this->trigger(self::EVENT_INDEX_RENAME, $indexNew); | |
| return true; | |
| } finally { | |
| // Ensure collection cache is cleared even if trigger fails after adapter changes | |
| $this->purgeCachedCollection($collection->getId()); | |
| } |
🤖 Prompt for AI Agents
In src/Database/Database.php around lines 3054 to 3067, the code updates
metadata and emits the rename event unconditionally after calling
$this->adapter->renameIndex; change this so the metadata update and
$this->trigger call only run when the adapter call actually succeeds: store the
result of $this->adapter->renameIndex(...), if it returns falsy return false
immediately (still run the finally purge), otherwise proceed to updateDocument
and trigger the event and then return true; keep the existing finally block
intact so the collection cache is always purged even on failure or exception.
Summary by CodeRabbit
Bug Fixes
Refactor