1.7.x ce migration #9738
1.7.x ce migration #9738
Conversation
WalkthroughThe changes refactor and enhance the migration framework, focusing on database handler renaming, improved document and collection migration logic, and streamlined project migration execution. New migration utilities and document transformation methods are introduced, additional attributes are added to the 'identities' collection, and concurrency is removed in favor of synchronous processing. Redis cache clearing and manual project pagination are eliminated. Changes
Sequence Diagram(s)sequenceDiagram
participant PlatformDB as Platform Database
participant MigrationTask as Migration Task
participant MigrationClass as Migration Class
participant ProjectDB as Project Database
PlatformDB->>MigrationTask: foreach(project)
loop For each project
MigrationTask->>MigrationClass: setProject(project, ProjectDB, PlatformDB, getProjectDB)
MigrationClass->>MigrationClass: execute()
MigrationClass->>MigrationClass: migrateCollections()
MigrationClass->>MigrationClass: forEachDocument(migrateDocument)
MigrationClass->>MigrationClass: cleanCollections()
MigrationTask->>MigrationTask: Log progress
end
MigrationTask->>MigrationClass: setProject(consoleProject, ProjectDB, PlatformDB, getProjectDB)
MigrationClass->>MigrationClass: execute()
MigrationTask->>MigrationTask: Log migration completed
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/Appwrite/Migration/Version/V23.php (1)
377-384: Misleading log message & unguardeddeleteCollection()The message “Migrating Collection …” is printed while deleting collections, which can mislead operators scanning migration logs.
Additionally,deleteCollection('builds')isn’t wrapped intry/catch, so the migration will abort if the collection has already been removed.-Console::log("Migrating Collection \"{$id}\""); +Console::log("Cleaning Collection \"{$id}\""); ... - $this->projectDB->deleteCollection('builds'); + try { + $this->projectDB->deleteCollection('builds'); + } catch (Throwable $th) { + Console::warning("'builds' collection already deleted or inaccessible: {$th->getMessage()}"); + }Clarifying the log and safeguarding the call will make re-runs and production roll-backs safer.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Appwrite/Migration/Version/V23.php(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
✨ Benchmark results
⚡ Benchmark Comparison
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/Appwrite/Migration/Version/V23.php (2)
372-389:⚠️ Potential issueMissing-deployment / missing-build still not guarded (see prior review).
getDocument('deployments', $deploymentId)and latergetDocument('builds', …)will throw if the document was deleted or the ID is empty, aborting the whole migration.
Previous review already flagged this – please wrap both calls intry/catchand proceed gracefully.
425-439:⚠️ Potential issueUncaught exception risk when fetching build document.
getDocument('builds', $document->getAttribute('buildId'))isn’t wrapped in atry/catch.
If the build was purged, the entire migration halts inside thedeploymentsbranch.- $build = Authorization::skip(fn () => $this->projectDB->getDocument('builds', $document->getAttribute('buildId'))); + try { + $build = Authorization::skip(fn () => $this->projectDB->getDocument('builds', $document->getAttribute('buildId'))); + } catch (Throwable $e) { + Console::warning("Missing build '{$document->getAttribute('buildId')}' for deployment '{$document->getId()}': {$e->getMessage()}"); + $build = new Document(); + }Prevents fatal errors & keeps the migration idempotent.
🧹 Nitpick comments (2)
app/config/collections/common.php (1)
1104-1114: Consider validating & indexing the newscopesarray.Great to see fine-grained OAuth scopes stored per identity.
Two small follow-ups you may want to schedule:
- Validation – an empty string or leading/trailing spaces would sabotage filtering downstream. A simple
trimfilter or a regex (scope-namepattern) keeps the column clean.- Index – many queries will likely filter by a single scope (
"email"for example). A KEY index onscopesreduces a full-table scan on large user bases.Neither is blocking, but both are low-effort wins.
src/Appwrite/Migration/Migration.php (1)
175-191: Synchronous updates could thrash the DB – consider batching / transaction safety.Removing Swoole coroutines simplifies control-flow 👍, but now every document update is executed sequentially:
- Large projects (-> tens of thousands of docs) will hold the DB connection for a long time.
- If the process aborts mid-way, we leave the project half-migrated with no easy rollback.
Two options:
-foreach ($this->documentsIterator($collection['$id']) as $document) { +// 1) Wrap updates in a transaction when engine allows it +$this->projectDB->beginTransaction(); +foreach ($this->documentsIterator($collection['$id']) as $document) { ... } +$this->projectDB->commit(); # 2) Or buffer writes & flush every N docsNot a blocker, but worth considering before GA.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/config/collections/common.php(1 hunks)src/Appwrite/Migration/Migration.php(1 hunks)src/Appwrite/Migration/Version/V23.php(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (3)
src/Appwrite/Migration/Version/V15.php (2)
109-115:⚠️ Potential issueMismatch between checked and assigned attribute names (
permissionvspermissions)The conditional checks
permissionbut the assignment and comparison rely onpermissions.
If the collection actually stores only one of these keys, the condition will always be false or the comparison always wrong, leading tofileSecuritybeing set to an unexpected value.-if (!is_null($bucket->getAttribute('permission'))) { - $bucket->setAttribute('fileSecurity', $bucket->getAttribute('permissions') === 'document'); +if (!is_null($bucket->getAttribute('permissions'))) { + $bucket->setAttribute('fileSecurity', $bucket->getAttribute('permissions') === 'document'); }Please verify which attribute is the canonical one and update both the check and the assignment consistently.
328-336: 🛠️ Refactor suggestionPossible undefined-index notice when the attribute column is missing
$columns[$attribute]is accessed without first confirming that the column actually exists.
If the target table does not have the expected column, PHP will raise a notice and the migration will silently skip the conversion.-if ($columns[$attribute] === 'int') { +if (isset($columns[$attribute]) && $columns[$attribute] === 'int') { … } -if ($columns[$attribute] === 'varchar') { +if (isset($columns[$attribute]) && $columns[$attribute] === 'varchar') { … }This small guard makes the migration resilient to partially-configured databases.
src/Appwrite/Migration/Version/V19.php (1)
731-741:⚠️ Potential issueUndefined variable
$project– use$this->projectinsteadInside the
functionsbranch you attempt to access$project->getAttribute('region'), but$projectis not defined in this scope.
The intention is clearly the project currently being migrated, which is stored in$this->project.- $schedule = $this->dbForPlatform->createDocument('schedules', new Document([ - 'region' => $project->getAttribute('region'), + $schedule = $this->dbForPlatform->createDocument('schedules', new Document([ + 'region' => $this->project->getAttribute('region'),This bug throws an undefined-variable error and aborts the migration for every project that reaches this code path.
♻️ Duplicate comments (2)
src/Appwrite/Migration/Version/V23.php (2)
438-450: Same unguardedbuildsfetch – wrap intry/catch
getDocument('builds', $document->getAttribute('buildId'))can fail if the build was purged.
Use the same defensive pattern as above to keep the migration idempotent.
330-358:⚠️ Potential issueMissing try/catch around
functionslookup can abort migration
getDocument('functions', $resourceId)will throw if the function was deleted orresourceIdis empty, stopping the whole project migration.- $function = $this->dbForProject->getDocument('functions', $resourceId); + try { + $function = $this->dbForProject->getDocument('functions', $resourceId); + } catch (Throwable $e) { + Console::warning("Rule '{$document->getId()}' references missing function '{$resourceId}': {$e->getMessage()}"); + $function = new Document(); // proceed with empty defaults + }
🧹 Nitpick comments (5)
src/Appwrite/Migration/Version/V15.php (1)
412-417: Raw SQL identifiers are not escapedThe query concatenates table names directly into SQL. Although the names come from internal IDs, using
prepare()does not protect identifiers – only values.
If an unexpected character sneaks into a collection name, the statement will break (or worse, cause SQL-injection-like behaviour).Consider wrapping identifiers in back-ticks and validating/whitelisting the table name beforehand:
$tableSafe = preg_replace('/[^a-zA-Z0-9_]/', '', $table); $query = $this->pdo->prepare( "SELECT * FROM `{$this->dbForProject->getDatabase()}`.`_{$this->project->getInternalId()}_{$tableSafe}_perms` WHERE _document = :id" );src/Appwrite/Platform/Tasks/Migrate.php (1)
5-17: Imports flagged by the linter as unused / style violationsThe pipeline reported PSR-12 issues (
no_unused_import,braces_position).
Utopia\App,Utopia\Database\Query, and possibly others are not referenced after the refactor, and some doc-blocks/braces seem mis-aligned.Cleaning up the
usesection and runningphp-cs-fixer(or the project’s preferred formatter) will unblock the linter.-use Utopia\App; -use Utopia\Database\Query; +// (removed – no longer used)Also re-check brace placement around class and method definitions to satisfy PSR-12.
app/config/collections/platform.php (2)
1945-1954:nameserversshould probably be an array instead of a long stringA domain can have multiple name-servers, yet the attribute is declared as
VAR_STRINGwitharray= false and an empty string as default.
Consider using an array or comma-separated‐string + proper filter so you don’t hit the 255-char limit and avoid manual parsing downstream.- 'type' => Database::VAR_STRING, - 'size' => 255, - 'array' => false, - 'default' => '', + 'type' => Database::VAR_STRING, + 'size' => Database::LENGTH_KEY, + 'array' => true, // multiple NS + 'default'=> [], + 'filters'=> ['json'], // or a custom `dns` filter
2146-2177: Unsigned integers recommended forttl,priority,weight
ttl,priority, andweightcannot be negative.
'signed' => trueallows negative values and may hide data-validation mistakes.- 'signed' => true, + 'signed' => false,Apply the same change to all three attributes.
src/Appwrite/Migration/Migration.php (1)
10-14: Unused imports are triggering the linter
Duplicate,Limit,Structure,Timeout, andConflictare only referenced in php-doc blocks.
Removing them (or using fully-qualified names inside the doc-blocks) will satisfy PSR-12 and keep CI green.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (14)
app/config/collections/common.php(1 hunks)app/config/collections/platform.php(2 hunks)app/config/collections/projects.php(1 hunks)src/Appwrite/Migration/Migration.php(11 hunks)src/Appwrite/Migration/Version/V15.php(64 hunks)src/Appwrite/Migration/Version/V16.php(4 hunks)src/Appwrite/Migration/Version/V17.php(16 hunks)src/Appwrite/Migration/Version/V18.php(11 hunks)src/Appwrite/Migration/Version/V19.php(30 hunks)src/Appwrite/Migration/Version/V20.php(19 hunks)src/Appwrite/Migration/Version/V21.php(8 hunks)src/Appwrite/Migration/Version/V22.php(1 hunks)src/Appwrite/Migration/Version/V23.php(4 hunks)src/Appwrite/Platform/Tasks/Migrate.php(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/Appwrite/Migration/Version/V20.php
🚧 Files skipped from review as they are similar to previous changes (1)
- app/config/collections/common.php
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/Appwrite/Migration/Version/V16.php (2)
src/Appwrite/SDK/Method.php (1)
setNamespace(211-215)src/Appwrite/Migration/Migration.php (2)
createAttributeFromCollection(302-351)createIndexFromCollection(365-398)
src/Appwrite/Migration/Migration.php (3)
src/Appwrite/Extend/Exception.php (1)
Exception(7-406)src/Appwrite/Migration/Version/V19.php (1)
forEachDocument(800-840)src/Appwrite/Vcs/Comment.php (1)
isEmpty(26-29)
🪛 GitHub Actions: Linter
src/Appwrite/Platform/Tasks/Migrate.php
[error] 1-1: PSR 12 linting failed: braces_position, no_unused_import issues detected.
src/Appwrite/Migration/Migration.php
[error] 1-1: PSR 12 linting failed: single_space_around_construct, brace style issues detected.
src/Appwrite/Migration/Version/V23.php
[error] 1-1: PSR 12 linting failed: method_argument_space, no_unused_import issues detected.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (25)
src/Appwrite/Migration/Version/V22.php (2)
53-53: LGTM - Database handler renamed consistentlyThe change from
projectDBtodbForProjectfor setting the namespace is consistent with the codebase-wide property renaming pattern.
59-59: LGTM - Database handler renamed consistentlyThe database handler property has been updated from
projectDBtodbForProjectin all attribute creation calls, maintaining consistent usage of the renamed property.Also applies to: 66-66, 73-73
src/Appwrite/Migration/Version/V16.php (3)
48-48: LGTM - Database handler renamed consistentlyThe change from
projectDBtodbForProjectfor setting the namespace is consistent with the codebase-wide property renaming pattern.
56-56: LGTM - Database handler renamed consistentlyThe database handler property has been updated from
projectDBtodbForProjectfor attribute deletion operations.
68-68: LGTM - Database handler renamed consistentlyThe database handler property has been updated from
projectDBtodbForProjectin attribute and index creation operations, maintaining consistent usage pattern.Also applies to: 77-77, 88-88
src/Appwrite/Migration/Version/V21.php (3)
32-32: LGTM - Database handler renamed consistentlyThe database handler property has been updated from
projectDBtodbForProjectwhen setting the namespace in multiple locations.Also applies to: 66-66
72-72: LGTM - Database handler renamed consistentlyThe database handler property has been updated from
projectDBtodbForProjectacross all attribute and index creation operations in the collections migration logic.Also applies to: 80-80, 88-88, 96-96, 105-105, 113-113, 120-120, 129-129, 136-136, 143-143, 152-152, 161-161, 170-170
221-222: LGTM - Database handler renamed consistentlyThe database handler property has been updated from
projectDBtodbForProjectfor attribute update and collection cache purging operations in the bucket migration logic.src/Appwrite/Migration/Version/V17.php (3)
50-51: LGTM - Database handler renamed consistentlyThe database handler property has been updated from
projectDBtodbForProjectfor attribute update and cache purging operations in the bucket migration code.
70-70: LGTM - Database handler renamed consistentlyThe database handler property has been updated from
projectDBtodbForProjectwhen setting the namespace.
78-79: LGTM - Database handler renamed consistentlyThe database handler property has been updated from
projectDBtodbForProjectacross all database operations including attribute creation, deletion, renaming, updates, and cache purging operations throughout the collections migration code.Also applies to: 90-91, 100-101, 112-113, 122-123, 132-133, 144-145, 156-157, 168-169, 178-179, 188-189, 200-201, 210-211, 222-223, 232-233
app/config/collections/projects.php (1)
2377-2387:⚠️ Potential issueDuplicate attribute definition for 'options'
The 'options' attribute is already defined in the 'migrations' collection at lines 2290-2300, and this exact same definition is duplicated here. Having duplicate attribute definitions with the same ID will cause schema validation errors during migration.
- [ - '$id' => ID::custom('options'), - 'type' => Database::VAR_STRING, - 'format' => '', - 'size' => 65536, - 'signed' => true, - 'required' => false, - 'default' => [], - 'array' => false, - 'filters' => ['json'], - ]Likely an incorrect or invalid review comment.
src/Appwrite/Migration/Version/V18.php (11)
29-29: Consistent rename of database propertyChanged reference from
$this->projectDBto$this->dbForProjectfor better naming consistency with other migration versions.
69-69: Consistent rename of database propertyChanged reference from
$this->projectDBto$this->dbForProjectfor better naming consistency.
97-97: Consistent rename of database propertyChanged reference from
$this->projectDBto$this->dbForProjectfor better naming consistency.
108-109: Consistent rename of database propertiesChanged references from
$this->projectDBto$this->dbForProjectfor better naming consistency across the migration framework.
119-120: Consistent rename of database propertiesChanged references from
$this->projectDBto$this->dbForProjectfor better naming consistency.
130-131: Consistent rename of database propertiesChanged references from
$this->projectDBto$this->dbForProjectfor better naming consistency.
141-141: Consistent rename of database propertyChanged reference from
$this->projectDBto$this->dbForProjectfor better naming consistency.
203-203: Consistent rename of database propertyChanged reference from
$this->projectDBto$this->dbForProjectfor better naming consistency.
217-218: Consistent rename of database propertiesChanged references from
$this->consoleDBand$this->projectDBto$this->dbForPlatformand$this->dbForProjectrespectively for better naming consistency.
247-247: Consistent rename of database propertyChanged reference from
$this->projectDBto$this->dbForProjectfor better naming consistency in SQL execution.
256-256: Consistent rename of database propertyChanged reference from
$this->projectDBto$this->dbForProjectfor better naming consistency in SQL execution.app/config/collections/platform.php (1)
226-229: Verify collection size limit – 1 000 000 may exceed engine constraints
'size' => 1_000_000(line 228) is large; depending on the SQL engine and column type this can silently truncate or fail.
Please confirm the maximum supported length forVAR_STRINGon your driver and, if needed, split the payload (e.g. JSON blob + secondary table or object-storage).src/Appwrite/Migration/Migration.php (1)
160-167:cachecollection is now migrated – intentional?The previous implementation skipped the
cachecollection to avoid cursor-pagination issues.
The new guard only checks$collection['$collection'] !== METADATA, so_metadata,cache, etc. are all processed.If
cachestill suffers from large-ID pagination problems this may re-introduce time-outs or OOM during migration.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/Appwrite/Migration/Version/V23.php (1)
1-609: 💡 Verification agent🧩 Analysis chain
Fix PSR 12 linting errors
The pipeline is reporting PSR 12 linting failures related to method argument spacing and unused imports.
Run the following to identify the specific PSR 12 issues:
🏁 Script executed:
#!/bin/bash # Check for specific PSR 12 issues in the file phpcs --standard=PSR12 src/Appwrite/Migration/Version/V23.php # Check for unused imports ast-grep --pattern 'use $CLASS;' src/Appwrite/Migration/Version/V23.php | grep -v -f <(grep -o '\b[A-Za-z0-9_\\]*\b' src/Appwrite/Migration/Version/V23.php | sort | uniq) || echo "No unused imports found"Length of output: 446
🏁 Script executed:
#!/bin/bash # Search for arrow functions with space before parentheses grep -n "fn (" src/Appwrite/Migration/Version/V23.php || echo "No matches for 'fn ('" # Search for arrow functions without space (for comparison) grep -n "fn(" src/Appwrite/Migration/Version/V23.php || echo "No matches for 'fn('"Length of output: 232
Fix PSR-12 lint violations in V23.php
Two issues need addressing in
src/Appwrite/Migration/Version/V23.php:• Remove the unused import of
Utopia\Database\Validator\Authorization.
• Remove the extra space in the two arrow functions so they readfn()instead offn ().Suggested diff:
--- a/src/Appwrite/Migration/Version/V23.php +++ b/src/Appwrite/Migration/Version/V23.php @@ -9,7 +9,6 @@ use Utopia\Database\Exception\Timeout; use Utopia\Database\Query; -use Utopia\Database\Validator\Authorization; class V23 extends Migration { @@ -30,7 +29,7 @@ class V23 extends Migration foreach (['subQueryIndexes', 'subQueryPlatforms', 'subQueryDomains', 'subQueryKeys', 'subQueryDevKeys', 'subQueryWebhooks', 'subQuerySessions', 'subQueryTokens', 'subQueryMemberships', 'subQueryVariables', 'subQueryChallenges', 'subQueryProjectVariables', 'subQueryTargets', 'subQueryTopicTargets'] as $name) { Database::addFilter( $name, - fn () => null, - fn () => [] + fn() => null, + fn() => [] ); }🧰 Tools
🪛 GitHub Actions: Linter
[error] 1-1: PSR 12 linting failed: method_argument_space, no_unused_import issues detected.
♻️ Duplicate comments (4)
src/Appwrite/Migration/Migration.php (1)
210-222:⚠️ Potential issueCollection-creation guard inverted – may recreate existing collections
The condition for creating a collection is inverted. You're currently creating a collection when it's NOT empty (i.e., when it already exists), which can cause duplicate errors.
Apply this fix:
- if (!$this->dbForProject->getCollection($id)->isEmpty()) { + if ($this->dbForProject->getCollection($id)->isEmpty()) {This ensures the collection is only created when it doesn't already exist.
src/Appwrite/Migration/Version/V23.php (3)
394-411:⚠️ Potential issueHandle missing deployments & builds with defensive coding
The code doesn't properly handle cases where deployments or builds might not exist, which could lead to fatal errors during migration.
Apply defensive checks:
- $deploymentId = $document->getAttribute('deploymentId'); - $deployment = $this->dbForProject->getDocument('deployments', $deploymentId); - $document->setAttribute('deploymentCreatedAt', $deployment->getCreatedAt()); + $deploymentId = $document->getAttribute('deploymentId', ''); + try { + if (!empty($deploymentId)) { + $deployment = $this->dbForProject->getDocument('deployments', $deploymentId); + $document->setAttribute('deploymentCreatedAt', $deployment->getCreatedAt()); + } + } catch (\Throwable $th) { + Console::warning("Failed to get deployment '{$deploymentId}' for function '{$document->getId()}': {$th->getMessage()}"); + } - $latestDeployment = $this->dbForProject->findOne('deployments',[ + $latestDeployment = $this->dbForProject->findOne('deployments', [ Query::orderDesc(), Query::equal('resourceId', [$document->getId()]), Query::equal('resourceType', ['functions']), - ]); + ]) ?? new Document(); - $latestBuild = $this->dbForProject->getDocument('builds', $latestDeployment->getAttribute('buildId', '')); + $latestBuild = new Document(); + if (!$latestDeployment->isEmpty()) { + $buildId = $latestDeployment->getAttribute('buildId', ''); + if (!empty($buildId)) { + try { + $latestBuild = $this->dbForProject->getDocument('builds', $buildId); + } catch (\Throwable $th) { + Console::warning("Failed to get build '{$buildId}' for deployment '{$latestDeployment->getId()}': {$th->getMessage()}"); + } + } + }
439-442:⚠️ Potential issueAdd proper error handling for build document retrieval
The build document retrieval lacks proper error handling, which could cause migration failures if a build is missing.
Add try-catch block:
$build = new Document(); if (!empty($document->getAttribute('buildId'))) { - $build = $this->dbForProject->getDocument('builds', $document->getAttribute('buildId')); + try { + $build = $this->dbForProject->getDocument('builds', $document->getAttribute('buildId')); + } catch (\Throwable $th) { + Console::warning("Failed to get build '{$document->getAttribute('buildId')}' for deployment '{$document->getId()}': {$th->getMessage()}"); + } }
352-360:⚠️ Potential issueHandle potential missing function documents
When accessing function documents by ID, there's no error handling if the function doesn't exist, which could cause the migration to fail.
Add error handling:
if ($deploymentResourceType === 'function') { if ($this->project->getInternalId() !== 'console') { - $function = $this->dbForProject->getDocument('functions', $resourceId); - - $document - ->setAttribute('deploymentId', $function->getAttribute('deployment', $function->getAttribute('deploymentId', ''))) - ->setAttribute('deploymentInternalId', $function->getAttribute('deployment', $function->getAttribute('deploymentId', ''))); + try { + $function = $this->dbForProject->getDocument('functions', $resourceId); + $document + ->setAttribute('deploymentId', $function->getAttribute('deployment', $function->getAttribute('deploymentId', ''))) + ->setAttribute('deploymentInternalId', $function->getAttribute('deployment', $function->getAttribute('deploymentId', ''))); + } catch (\Throwable $th) { + Console::warning("Failed to get function '{$resourceId}': {$th->getMessage()}"); + } } }
🧹 Nitpick comments (2)
src/Appwrite/Migration/Version/V23.php (2)
453-456: Avoid potential NaN total size calculationThe total size calculation doesn't account for potential null values, which could lead to unexpected results.
Add explicit type casting:
- $totalSize = $document->getAttribute('buildSize', 0) - + $document->getAttribute('sourceSize', 0); + $totalSize = (int)$document->getAttribute('buildSize', 0) + + (int)$document->getAttribute('sourceSize', 0); $document->setAttribute('totalSize', $totalSize);
485-605: Improve code reuse in cleanCollections methodThe
cleanCollectionsmethod contains repetitive code patterns for deleting attributes and indexes that could be refactored for better maintainability.Consider extracting the common delete operations into helper methods:
+ /** + * Delete attributes from a collection with error handling + * + * @param string $collectionId The collection ID + * @param array $attributes The attributes to delete + * @return void + */ + private function deleteAttributesWithErrorHandling(string $collectionId, array $attributes): void + { + foreach ($attributes as $attribute) { + try { + $this->dbForProject->deleteAttribute($collectionId, $attribute); + } catch (\Throwable $th) { + Console::warning("Failed to delete attribute \"$attribute\" from collection {$collectionId}: {$th->getMessage()}"); + } + } + } + + /** + * Delete indexes from a collection with error handling + * + * @param string $collectionId The collection ID + * @param array $indexes The indexes to delete + * @return void + */ + private function deleteIndexesWithErrorHandling(string $collectionId, array $indexes): void + { + foreach ($indexes as $index) { + try { + $this->dbForProject->deleteIndex($collectionId, $index); + } catch (\Throwable $th) { + Console::warning("Failed to delete index \"$index\" from collection {$collectionId}: {$th->getMessage()}"); + } + } + }Then use these helper methods in the switch cases:
switch ($id) { case '_metadata': // ... existing code break; case 'rules': - $attributes = [/* ... */]; - foreach ($attributes as $attribute) { - try { - $this->dbForProject->deleteAttribute($id, $attribute); - } catch (\Throwable $th) { - Console::warning("Failed to delete attribute \"$attribute\" from collection {$id}: {$th->getMessage()}"); - } - } - - $indexesToDelete = [/* ... */]; - foreach ($indexesToDelete as $index) { - try { - $this->dbForProject->deleteIndex($id, $index); - } catch (\Throwable $th) { - Console::warning("Failed to delete index \"$index\" from collection {$id}: {$th->getMessage()}"); - } - } + $this->deleteAttributesWithErrorHandling($id, [ + 'resourceId', + 'resourceInternalId', + 'resourceType', + ]); + + $this->deleteIndexesWithErrorHandling($id, [ + '_key_resourceId', + '_key_resourceInternalId', + '_key_resourceType', + ]);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
src/Appwrite/Migration/Migration.php(11 hunks)src/Appwrite/Migration/Version/V23.php(4 hunks)src/Appwrite/Platform/Tasks/Migrate.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Appwrite/Platform/Tasks/Migrate.php
🧰 Additional context used
🪛 GitHub Actions: Linter
src/Appwrite/Migration/Migration.php
[error] 1-1: PSR 12 linting failed: single_space_around_construct, brace style issues detected.
src/Appwrite/Migration/Version/V23.php
[error] 1-1: PSR 12 linting failed: method_argument_space, no_unused_import issues detected.
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: E2E Service Test (Messaging)
- GitHub Check: E2E Service Test (Migrations)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Databases)
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: E2E General Test
- GitHub Check: Unit Test
- GitHub Check: Benchmark
- GitHub Check: scan
🔇 Additional comments (7)
src/Appwrite/Migration/Migration.php (4)
167-189: Good refactoring of document processing to synchronous modeThe rewritten
forEachDocumentmethod now processes documents synchronously using the database'sforeachmethod, which simplifies the code flow and improves error reporting. The added error handling on document update failures is a good defensive practice.
240-289: Well-structured attribute creation utilityThe new
createAttributesFromCollectionmethod is well-designed with proper error handling, JSON encoding for default values, and clear parameter types. This will help standardize attribute creation across migration versions.
306-355: Improved attribute creation method with better default handlingThe updated
createAttributeFromCollectionmethod now proper F438 ly handles defaults for JSON fields and provides better exception handling. The added type hints and parameter documentation improve maintainability.
1-429:❓ Verification inconclusive
Fix PSR 12 linting errors
The pipeline is reporting PSR 12 linting failures related to brace style and spacing around constructs.
Run the following to identify the specific PSR 12 issues:
🏁 Script executed:
#!/bin/bash # Check for specific PSR 12 issues in the file phpcs --standard=PSR12 src/Appwrite/Migration/Migration.phpLength of output: 157
Verify PSR-12 Compliance
I wasn’t able to run
phpcsin this environment (command not found), so please verify PSR-12 linting locally. For example:
- Run via Composer:
composer run phpcs -- --standard=PSR12 src/Appwrite/Migration/Migration.php- Or directly (if available):
vendor/bin/phpcs --standard=PSR12 src/Appwrite/Migration/Migration.phpLook specifically for violations around:
- Brace placement (opening braces on the next line for classes, methods, control structures)
- Spacing around constructs (e.g., keywords, parentheses, operators)
Fix any reported errors to satisfy PSR-12.
🧰 Tools
🪛 GitHub Actions: Linter
[error] 1-1: PSR 12 linting failed: single_space_around_construct, brace style issues detected.
src/Appwrite/Migration/Version/V23.php (3)
24-43: Well-structured migration strategyThe
executemethod follows a clear, logical sequence:
- Disabling subqueries for performance
- Migrating collections (schema changes)
- Migrating documents (data transformations)
- Cleaning up collections (removing obsolete elements)
This structured approach ensures a comprehensive migration with proper error handling.
55-57: Good defensive check for project IDAdding a null check for the project internal ID ensures early failure with a clear error message rather than proceeding with invalid data.
66-302: Well-structured collection migration with proper error handlingThe migration collections method demonstrates good practices:
- Try-catch blocks around individual operations
- Detailed error messages with specifics about which attributes failed
- Cache purging after changes
- Clear grouping of related attributes and indexes
This will make debugging migration issues much easier.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/Appwrite/Migration/Migration.php (1)
213-215:⚠️ Potential issueCollection-creation condition is still inverted
The condition is checking if a collection is NOT empty before returning, meaning we only try to create a collection when it already exists.
Apply this diff to fix the logic:
- if (!$this->dbForProject->getCollection($id)->isEmpty()) { + if ($this->dbForProject->getCollection($id)->isEmpty()) {This matches the previous fix mentioned in the past review comments.
src/Appwrite/Migration/Version/V23.php (2)
438-441:⚠️ Potential issueAdd error handling for build document retrieval in deployments migration.
Similar to the functions migration, this code doesn't handle the case where build documents might not exist.
Add proper error handling:
- $build = new Document(); - if (!empty($document->getAttribute('buildId'))) { - $build = $this->dbForProject->getDocument('builds', $document->getAttribute('buildId')); - } + $build = new Document(); + if (!empty($document->getAttribute('buildId'))) { + try { + $build = $this->dbForProject->getDocument('builds', $document->getAttribute('buildId')); + } catch (\Throwable $th) { + Console::warning("Failed to retrieve build '{$document->getAttribute('buildId')}' for deployment '{$document->getId()}': {$th->getMessage()}"); + } + }
389-410:⚠️ Potential issueAdd error handling for document retrieval in functions migration.
This code doesn't handle the case where deployment or build documents might not exist, which could cause the migration to fail.
Add proper error handling around document retrievals:
- $deploymentId = $document->getAttribute('deploymentId'); - $deployment = $this->dbForProject->getDocument('deployments', $deploymentId); - $document->setAttribute('deploymentCreatedAt', $deployment->getCreatedAt()); - - $latestDeployment = $this->dbForProject->findOne('deployments', [ - Query::orderDesc(), - Query::equal('resourceId', [$document->getId()]), - Query::equal('resourceType', ['functions']), - ]); - - $latestBuild = $this->dbForProject->getDocument('builds', $latestDeployment->getAttribute('buildId', '')); + $deploymentId = $document->getAttribute('deploymentId'); + try { + $deployment = $this->dbForProject->getDocument('deployments', $deploymentId); + $document->setAttribute('deploymentCreatedAt', $deployment->getCreatedAt()); + } catch (\Throwable $th) { + Console::warning("Failed to retrieve deployment '{$deploymentId}' for function '{$document->getId()}': {$th->getMessage()}"); + } + + $latestDeployment = $this->dbForProject->findOne('deployments', [ + Query::orderDesc(), + Query::equal('resourceId', [$document->getId()]), + Query::equal('resourceType', ['functions']), + ]) ?? new Document(); + + $latestBuild = new Document(); + if (!$latestDeployment->isEmpty()) { + try { + $latestBuild = $this->dbForProject->getDocument('builds', $latestDeployment->getAttribute('buildId', '')); + } catch (\Throwable $th) { + Console::warning("Failed to retrieve build for latest deployment of function '{$document->getId()}': {$th->getMessage()}"); + } + }
🧹 Nitpick comments (1)
src/Appwrite/Migration/Migration.php (1)
288-290: Ensure JSON encoding is valid for default values.JSON encoding of the default value doesn't include error handling, which could cause issues for complex default values that can't be properly encoded.
Consider adding error handling:
- $attribute['default'] = \in_array('json', $attribute['filters']) - ? \json_encode($attribute['default']) - : $attribute['default']; + if (\in_array('json', $attribute['filters']) && $attribute['default'] !== null) { + $json = \json_encode($attribute['default']); + if ($json === false) { + Console::warning("Failed to JSON encode default value for attribute {$attribute['$id']}"); + } + $attribute['default'] = $json; + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
src/Appwrite/Migration/Migration.php(10 hunks)src/Appwrite/Migration/Version/V23.php(3 hunks)src/Appwrite/Platform/Tasks/Migrate.php(2 hunks)tests/e2e/Services/Sites/SitesConsoleClientTest.php(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/e2e/Services/Sites/SitesConsoleClientTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Appwrite/Platform/Tasks/Migrate.php
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (4)
src/Appwrite/Migration/Migration.php (2)
147-193: Improved document iteration approach.The switch from coroutine-based to synchronous document iteration is a good improvement for reliability and simplicity. The error handling with detailed logging will help diagnose issues during migration.
424-424: Potential SQL injection risk in changeAttributeInternalType.While this appears to be an internal method, the SQL query construction uses variables directly without prepared statements for the attribute name and type.
Verify that inputs to this method are properly sanitized before reaching this point, particularly the
$attributeand$typeparameters. If these can come from user input, consider using prepared statements for those variables as well.src/Appwrite/Migration/Version/V23.php (2)
25-32: Good practice: Disable subqueries for performance improvement.Disabling subqueries during migration is a good practice as it can significantly improve performance by preventing unnecessary database queries.
37-41: Well-structured migration with clear phases.Breaking the migration into distinct phases (collections, documents, cleanup) improves clarity and maintainability.
What does this PR do?
(Provide a description of what this PR does and why it's needed.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Screenshots may also be helpful.)
Related PRs and Issues
Checklist
Summary by CodeRabbit
New Features
scopesandexpireto identity records for enhanced scope management and expiration tracking.Bug Fixes
Refactor
Style