From 5462998c2cef762b1d8d7109aa73f40f35dedb6c Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Fri, 1 May 2026 13:04:20 +1200 Subject: [PATCH] fix: address Greptile review feedback on migration resource refactor - Request V25 filter no longer scrubs a non-composite resourceId so the UID validator can surface a proper error instead of an empty value. - V25 schema migration writes resourceType/resourceId only after the internal-ID lookup succeeds; if the database/collection no longer exists, the document is left untouched so the patch can be retried. - Worker getCompoundResourceId() returns null when only parentResourceId is set, instead of feeding the migration library a malformed parent-only ID. - processMigrationResourceStats() guards against missing/unresolvable parent or leaf IDs and skips the stats update with a warning, instead of casting null to '' and querying database_0. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/Appwrite/Migration/Version/V25.php | 22 +++++++++++------ src/Appwrite/Platform/Workers/Migrations.php | 25 +++++++++++++++----- src/Appwrite/Utopia/Request/Filters/V25.php | 11 +++++---- 3 files changed, 41 insertions(+), 17 deletions(-) diff --git a/src/Appwrite/Migration/Version/V25.php b/src/Appwrite/Migration/Version/V25.php index 6ba8afef0c..7d17613bfc 100644 --- a/src/Appwrite/Migration/Version/V25.php +++ b/src/Appwrite/Migration/Version/V25.php @@ -91,26 +91,34 @@ class V25 extends Migration [$parentId, $childId] = \explode(':', $resourceId, 2); $parentResourceType = $document->getAttribute('resourceType'); - $document - ->setAttribute('resourceId', $childId) - ->setAttribute('resourceType', 'collection') - ->setAttribute('parentResourceId', $parentId) - ->setAttribute('parentResourceType', $parentResourceType); + $parentResourceInternalId = ''; + $resourceInternalId = ''; try { $database = $this->dbForProject->getDocument('databases', $parentId); if (!$database->isEmpty()) { - $document->setAttribute('parentResourceInternalId', (string) $database->getSequence()); + $parentResourceInternalId = (string) $database->getSequence(); $collection = $this->dbForProject->getDocument('database_' . $database->getSequence(), $childId); if (!$collection->isEmpty()) { - $document->setAttribute('resourceInternalId', (string) $collection->getSequence()); + $resourceInternalId = (string) $collection->getSequence(); } } } catch (Throwable $th) { Console::warning("Failed to backfill internal IDs for migration {$document->getId()}: {$th->getMessage()}"); + // Lookup failed — leave the document untouched so the original + // composite resourceId is preserved and the doc can be retried. + return $document; } + $document + ->setAttribute('resourceId', $childId) + ->setAttribute('resourceInternalId', $resourceInternalId) + ->setAttribute('resourceType', 'collection') + ->setAttribute('parentResourceId', $parentId) + ->setAttribute('parentResourceInternalId', $parentResourceInternalId) + ->setAttribute('parentResourceType', $parentResourceType); + return $document; } } diff --git a/src/Appwrite/Platform/Workers/Migrations.php b/src/Appwrite/Platform/Workers/Migrations.php index 3bebc4dcc9..4997e8f330 100644 --- a/src/Appwrite/Platform/Workers/Migrations.php +++ b/src/Appwrite/Platform/Workers/Migrations.php @@ -610,9 +610,10 @@ class Migrations extends Action } /** - * Returns a "{parentResourceId}:{resourceId}" string when both are set, or just - * the available ID otherwise. The utopia-php/migration library expects this - * compound shape on its source/destination constructors. + * Returns a "{parentResourceId}:{resourceId}" string when both are set, + * or the leaf resourceId on its own otherwise. Returning a parent-only + * value would feed the utopia-php/migration library a malformed + * compound, so an unanchored parent yields null instead. */ protected function getCompoundResourceId(Document $migration): ?string { @@ -623,7 +624,7 @@ class Migrations extends Action return $parentResourceId . ':' . $resourceId; } - return $resourceId ?? $parentResourceId; + return !empty($resourceId) ? $resourceId : null; } /** @@ -916,8 +917,20 @@ class Migrations extends Action $tableInternalId = $resources['tableId']; if ($source === CSV::getName()) { - $database = $authorization->skip(fn () => $this->dbForProject->getDocument('databases', (string) $parentResourceId)); - $table = $authorization->skip(fn () => $this->dbForProject->getDocument('database_' . $database->getSequence(), (string) $resourceId)); + if (empty($parentResourceId) || empty($resourceId)) { + Console::warning("Skipping CSV migration usage stats: missing parent/leaf resource ID (parent: '{$parentResourceId}', leaf: '{$resourceId}')"); + return; + } + $database = $authorization->skip(fn () => $this->dbForProject->getDocument('databases', $parentResourceId)); + if ($database->isEmpty()) { + Console::warning("Skipping CSV migration usage stats: database '{$parentResourceId}' not found"); + return; + } + $table = $authorization->skip(fn () => $this->dbForProject->getDocument('database_' . $database->getSequence(), $resourceId)); + if ($table->isEmpty()) { + Console::warning("Skipping CSV migration usage stats: collection '{$resourceId}' not found in database '{$parentResourceId}'"); + return; + } $databaseInternalId = (int) $database->getSequence(); $tableInternalId = (int) $table->getSequence(); } diff --git a/src/Appwrite/Utopia/Request/Filters/V25.php b/src/Appwrite/Utopia/Request/Filters/V25.php index 39a682d9e6..36e5c5e19f 100644 --- a/src/Appwrite/Utopia/Request/Filters/V25.php +++ b/src/Appwrite/Utopia/Request/Filters/V25.php @@ -27,12 +27,15 @@ class V25 extends Filter return $content; } - if (\str_contains($content['resourceId'], ':')) { - [$databaseId, $collectionId] = \explode(':', $content['resourceId'], 2); - $content['databaseId'] = $content['databaseId'] ?? $databaseId; - $content['collectionId'] = $content['collectionId'] ?? $collectionId; + if (!\str_contains($content['resourceId'], ':')) { + // Leave malformed resourceId in place so the new UID validator + // surfaces it to the caller instead of silently scrubbing it. + return $content; } + [$databaseId, $collectionId] = \explode(':', $content['resourceId'], 2); + $content['databaseId'] = $content['databaseId'] ?? $databaseId; + $content['collectionId'] = $content['collectionId'] ?? $collectionId; unset($content['resourceId']); return $content;