Multitype db #139
Multitype db #139
Conversation
WalkthroughAdds multi-database support across the migration subsystem: new database resource types (DocumentsDB, VectorDB, legacy), Collection and Document models, and DATABASE/ENTITY/FIELD/RECORD mapping constants. Introduces an abstract Attribute class and many concrete attribute/column types (text, integer, decimal, enum, vector, object, geo types, relationship, boolean, datetime, email, ip, url). Refactors Appwrite source/reader and Appwrite destination to resolve per-database contexts via callables/readers and per-database DSNs, updates CSV handling to emit document/collection/attribute payloads, and appends database type into resource cache keys. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Migration/Destinations/Appwrite.php (1)
352-363: Normalize database type before DSN resolution.The
typefield normalizes emptygetType()to'legacy', but the DSN lookup receivesgetType()directly, which could be an empty string. If the DSN resolver doesn't handle empty strings, database metadata can point to the wrong DSN.Suggested fix
+ $databaseType = $resource->getType() ?: 'legacy'; $database = $this->dbForProject->createDocument('databases', new UtopiaDocument([ '$id' => $resource->getId(), 'name' => $resource->getDatabaseName(), 'enabled' => $resource->getEnabled(), 'search' => implode(' ', [$resource->getId(), $resource->getDatabaseName()]), '$createdAt' => $resource->getCreatedAt(), '$updatedAt' => $resource->getUpdatedAt(), 'originalId' => empty($resource->getOriginalId()) ? null : $resource->getOriginalId(), - 'type' => empty($resource->getType()) ? 'legacy' : $resource->getType(), + 'type' => $databaseType, // source and destination can be in different location - 'database' => ($this->getDatabaseDSN)($resource->getType()) + 'database' => ($this->getDatabaseDSN)($databaseType) ]));
🤖 Fix all issues with AI agents
In `@src/Migration/Destinations/Appwrite.php`:
- Around line 459-464: The guard in createField(Column|Attribute $resource)
currently checks Resource::TYPE_DATABASE_DOCUMENTSDB and sets status with
message "Columns not supported for DocumentsDB" but the method receives both
Column and Attribute; either narrow the condition to only apply when $resource
is an instance of Column, or change the status message to accurately reflect
both types (e.g., "Columns and Attributes not supported for DocumentsDB" or
"Fields not supported for DocumentsDB"); update the check around createField and
the call to $resource->setStatus(Resource::STATUS_SKIPPED, ...) accordingly so
the behavior and message match (refer to createField, Column, Attribute,
Resource::TYPE_DATABASE_DOCUMENTSDB, setStatus, STATUS_SKIPPED).
In `@src/Migration/Resources/Database/Attribute/Decimal.php`:
- Around line 22-23: The current defaulting in Decimal.php uses PHP_FLOAT_MIN
for $min which is the smallest positive float, not a negative bound; update the
default to use -PHP_FLOAT_MAX (or set $min ??= -PHP_FLOAT_MAX) so decimal ranges
allow negative values, keeping $max ??= PHP_FLOAT_MAX unchanged and adjust any
references to $min/$max in the Decimal class or related validation to expect a
potentially negative lower bound.
In `@src/Migration/Resources/Database/Attribute/Integer.php`:
- Around line 78-91: The fromArray method for class Integer is not
reading/passing the constructor's $signed parameter, causing unsigned integers
to be lost; update Integer::fromArray to extract the signed flag (e.g.
$array['signed'] ?? false) and pass it into the Integer constructor (the
__construct parameter named $signed), and also update the method PHPDoc to
document the signed field being deserialized.
In `@src/Migration/Resources/Database/Attribute/Polygon.php`:
- Around line 58-67: In Polygon::fromArray change the handling of the 'default'
index to guard against missing keys: update the constructor call in the
fromArray method to pass $array['default'] ?? null (matching other attribute
factories' null fallback) so an undefined index notice is avoided; leave other
fields (required, createdAt, updatedAt) as-is and locate this in the fromArray
method of the Polygon class to apply the fix.
In `@src/Migration/Resources/Database/VectorDB.php`:
- Around line 14-24: The PHPDoc for the array parameter is missing the "type"
key even though the code reads $array['type'] (see use in the VectorDB
class/method handling that array); update the `@param` array shape to include
type: string so the documented shape matches runtime access (add "type: string"
to the array shape in the PHPDoc for the $array parameter).
In `@src/Migration/Sources/Appwrite.php`:
- Around line 851-857: The array construction is using $databaseName (which
holds the database type) for the 'name' field; update the assignment to use the
actual database name accessor on the $database object (e.g., replace
$databaseName with $database->getDatabaseName() or $database->getName()
depending on the available method) inside the block that builds the 'database'
array so the 'name' value reflects the real database name rather than its type.
In `@src/Migration/Sources/Appwrite/Reader/Database.php`:
- Around line 27-38: The Database class is missing implementations for
getDatabase() and logDebug() which are invoked from within the class; add a
private getDatabase(): UtopiaDatabase method that returns the correct DB
instance by invoking the stored callable $this->getDatabasesDB with
$this->projectId when callable (and falling back to $this->dbForProject), and
add a private logDebug(string $message, array $context = []): void method that
emits debug output (format the message with context) using an existing logger if
present (e.g., $this->logger) or error_log as a fallback so existing calls to
getDatabase() and logDebug() succeed.
In `@src/Migration/Transfer.php`:
- Line 361: The variable $groupDatabasesIndex is assigned from
array_search(Transfer::GROUP_DATABASES, $services) but never used; remove the
unused assignment line to eliminate the dead variable (i.e. delete the statement
that sets $groupDatabasesIndex) so only relevant variables and logic remain; if
the intent was to check presence of Transfer::GROUP_DATABASES in $services
instead, replace the assignment with an explicit conditional using
in_array(Transfer::GROUP_DATABASES, $services) where needed.
🧹 Nitpick comments (13)
src/Migration/Resources/Database/Database.php (1)
62-72: Consider includingdatabaseinjsonSerialize()for round-trip consistency.The new
$databaseproperty is not included injsonSerialize(). If resources are serialized and later reconstructed viafromArray(), thedatabasevalue will be lost. If this is intentional (e.g.,databaseis only for internal routing), consider documenting it; otherwise, add it to the serialized output.Suggested fix (if round-trip is needed)
public function jsonSerialize(): array { return [ 'id' => $this->id, 'name' => $this->name, 'createdAt' => $this->createdAt, 'updatedAt' => $this->updatedAt, 'enabled' => $this->enabled, 'type' => $this->type, + 'database' => $this->database, ]; }src/Migration/Resources/Database/Attribute/Vector.php (1)
61-72: Minor formatting nit: missing space after colon.Line 67 has
size:$array['size']without a space after the colon, unlike other named arguments. This is inconsistent with the rest of the codebase formatting.🔧 Suggested fix
public static function fromArray(array $array): self { return new self( $array['key'], Collection::fromArray($array['table'] ?? $array['collection']), required: $array['required'], - size:$array['size'], + size: $array['size'], default: $array['default'], createdAt: $array['createdAt'] ?? '', updatedAt: $array['updatedAt'] ?? '', ); }src/Migration/Resources/Database/Columns/Vector.php (1)
61-72: Minor formatting nit: missing space after colon.Line 67 has
size:$array['size']without a space after the colon, inconsistent with other named arguments in the file and codebase.🔧 Suggested fix
public static function fromArray(array $array): self { return new self( $array['key'], Table::fromArray($array['table'] ?? $array['collection']), required: $array['required'], - size:$array['size'], + size: $array['size'], default: $array['default'], createdAt: $array['createdAt'] ?? '', updatedAt: $array['updatedAt'] ?? '', ); }src/Migration/Resources/Database/Attribute/Text.php (1)
88-96: Redundant method overrides.Per the relevant code snippet from
src/Migration/Resources/Database/Attribute.php, the baseAttributeclass already definesgetSize()andgetFormat()with identical implementations. These overrides can be removed.♻️ Suggested removal
public function getType(): string { return Attribute::TYPE_STRING; } - - public function getSize(): int - { - return $this->size; - } - - public function getFormat(): string - { - return $this->format; - } }src/Migration/Resources/Database/DocumentsDB.php (1)
14-24: PHPDoc is missing thetypekey.The implementation uses
$array['type'] ?? 'legacy'on line 34, but the PHPDoc shape definition doesn't include thetypekey. Consider adding it for accuracy.📝 Suggested fix
/** * `@param` array{ * id: string, * name: string, * createdAt: string, * updatedAt: string, * enabled: bool, * originalId: string|null, + * type: string, * database: string * } $array */src/Migration/Resources/Database/Attribute/Enum.php (1)
98-101: Unnecessary array cast.The
formatOptions['elements']is always an array by construction (set in constructor line 33 and guaranteed by PHPDoc). The(array)cast is redundant.♻️ Suggested simplification
public function getElements(): array { - return (array)$this->formatOptions['elements']; + return $this->formatOptions['elements']; }src/Migration/Resources/Database/Attribute/Integer.php (1)
98-106: Return type?intis misleading.Both
getMin()andgetMax()are declared as returning?int, but they always return anintdue to the cast and because the constructor defaultsmin/maxtoPHP_INT_MIN/PHP_INT_MAX. Consider changing the return type tointfor accuracy, or removing the cast if null should actually be possible.♻️ Suggested fix (if null is never expected)
- public function getMin(): ?int + public function getMin(): int { return (int)$this->formatOptions['min']; } - public function getMax(): ?int + public function getMax(): int { return (int)$this->formatOptions['max']; }src/Migration/Resources/Database/Attribute/Decimal.php (1)
96-104: Return type inconsistency ingetMin/getMax.The return type is
?float(nullable), but the implementation always casts to(float), which will never returnnull. Either remove the nullable return type or handle the null case.Proposed fix
public function getMin(): ?float { - return (float)$this->formatOptions['min']; + return isset($this->formatOptions['min']) ? (float)$this->formatOptions['min'] : null; } public function getMax(): ?float { - return (float)$this->formatOptions['max']; + return isset($this->formatOptions['max']) ? (float)$this->formatOptions['max'] : null; }src/Migration/Resources/Database/Attribute/Relationship.php (1)
96-99: Minor:getRelatedTable()fallback is effectively dead code.Since the constructor always stores the value under
'relatedCollection', the'relatedTable'key will never exist in$this->options. The null coalescing fallback works correctly, but the first check is unnecessary.Optional simplification
public function getRelatedTable(): string { - return $this->options['relatedTable'] ?? $this->options['relatedCollection']; + return $this->options['relatedCollection']; }However, keeping the fallback may be intentional for compatibility if
fromArrayis used with data containing'relatedTable'.src/Migration/Sources/Appwrite.php (1)
92-95: PHPDoc typo:UtopiaDocumentshould be a valid type.The type hint
UtopiaDocumentappears to be incomplete. It should likely be\Utopia\Database\Documentor similar.Proposed fix
/** - * `@var` callable(UtopiaDocument $database|null): UtopiaDatabase + * `@var` callable(\Utopia\Database\Document|null $database): UtopiaDatabase */ protected mixed $getDatabasesDB;src/Migration/Destinations/Appwrite.php (3)
797-808: Drop unused $supportForSpatialAttributes (or pass into validator).
PHPMD flagged it as unused; keeping it adds noise.♻️ Suggested cleanup
- $supportForSpatialAttributes = $dbForDatabases->getAdapter()->getSupportForSpatialAttributes();
962-975: Avoid unused $value in the row scrub loop.
Small cleanup to satisfy PHPMD.♻️ Suggested cleanup
- foreach ($row as $key => $value) { + foreach ($row as $key => $_) {
1522-1525: Remove unused $columnSize.
PHPMD flags this; drop it unless you plan to use size for index lengths.♻️ Suggested cleanup
- $columnSize = $oldColumns[$columnIndex]['size'];
| $min ??= PHP_FLOAT_MIN; | ||
| $max ??= PHP_FLOAT_MAX; |
There was a problem hiding this comment.
Potential issue with PHP_FLOAT_MIN as minimum bound.
PHP_FLOAT_MIN represents the smallest positive float (~2.2e-308), not a negative minimum. This differs from Integer.php which correctly uses PHP_INT_MIN (a negative value). For decimal/float ranges, if you intend to allow negative values, consider using -PHP_FLOAT_MAX as the lower bound.
Proposed fix
- $min ??= PHP_FLOAT_MIN;
+ $min ??= -PHP_FLOAT_MAX;🤖 Prompt for AI Agents
In `@src/Migration/Resources/Database/Attribute/Decimal.php` around lines 22 - 23,
The current defaulting in Decimal.php uses PHP_FLOAT_MIN for $min which is the
smallest positive float, not a negative bound; update the default to use
-PHP_FLOAT_MAX (or set $min ??= -PHP_FLOAT_MAX) so decimal ranges allow negative
values, keeping $max ??= PHP_FLOAT_MAX unchanged and adjust any references to
$min/$max in the Decimal class or related validation to expect a potentially
negative lower bound.
| public static function fromArray(array $array): self | ||
| { | ||
| return new self( | ||
| $array['key'], | ||
| Collection::fromArray($array['table'] ?? $array['collection']), | ||
| required: $array['required'], | ||
| default: $array['default'], | ||
| array: $array['array'], | ||
| min: $array['formatOptions']['min'] ?? null, | ||
| max: $array['formatOptions']['max'] ?? null, | ||
| createdAt: $array['createdAt'] ?? '', | ||
| updatedAt: $array['updatedAt'] ?? '', | ||
| ); | ||
| } |
There was a problem hiding this comment.
Missing signed parameter in fromArray.
The constructor accepts a $signed parameter (line 18), but fromArray doesn't extract or pass it. This causes data loss during deserialization—unsigned integers will be incorrectly reconstructed as signed.
🐛 Suggested fix
public static function fromArray(array $array): self
{
return new self(
$array['key'],
Collection::fromArray($array['table'] ?? $array['collection']),
required: $array['required'],
default: $array['default'],
array: $array['array'],
min: $array['formatOptions']['min'] ?? null,
max: $array['formatOptions']['max'] ?? null,
+ signed: $array['signed'] ?? true,
createdAt: $array['createdAt'] ?? '',
updatedAt: $array['updatedAt'] ?? '',
);
}Also update the PHPDoc to include:
* formatOptions: array{
* min: ?int,
* max: ?int
* },
+ * signed: bool,
* createdAt: string,📝 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.
| public static function fromArray(array $array): self | |
| { | |
| return new self( | |
| $array['key'], | |
| Collection::fromArray($array['table'] ?? $array['collection']), | |
| required: $array['required'], | |
| default: $array['default'], | |
| array: $array['array'], | |
| min: $array['formatOptions']['min'] ?? null, | |
| max: $array['formatOptions']['max'] ?? null, | |
| createdAt: $array['createdAt'] ?? '', | |
| updatedAt: $array['updatedAt'] ?? '', | |
| ); | |
| } | |
| public static function fromArray(array $array): self | |
| { | |
| return new self( | |
| $array['key'], | |
| Collection::fromArray($array['table'] ?? $array['collection']), | |
| required: $array['required'], | |
| default: $array['default'], | |
| array: $array['array'], | |
| min: $array['formatOptions']['min'] ?? null, | |
| max: $array['formatOptions']['max'] ?? null, | |
| signed: $array['signed'] ?? true, | |
| createdAt: $array['createdAt'] ?? '', | |
| updatedAt: $array['updatedAt'] ?? '', | |
| ); | |
| } |
🤖 Prompt for AI Agents
In `@src/Migration/Resources/Database/Attribute/Integer.php` around lines 78 - 91,
The fromArray method for class Integer is not reading/passing the constructor's
$signed parameter, causing unsigned integers to be lost; update
Integer::fromArray to extract the signed flag (e.g. $array['signed'] ?? false)
and pass it into the Integer constructor (the __construct parameter named
$signed), and also update the method PHPDoc to document the signed field being
deserialized.
| public static function fromArray(array $array): self | ||
| { | ||
| return new self( | ||
| $array['key'], | ||
| Collection::fromArray($array['table'] ?? $array['collection']), | ||
| required: $array['required'], | ||
| default: $array['default'], | ||
| createdAt: $array['createdAt'] ?? '', | ||
| updatedAt: $array['updatedAt'] ?? '', | ||
| ); |
There was a problem hiding this comment.
Guard against missing default key.
If default is absent, this raises an undefined index notice. Use a null fallback for consistency with other attribute factories.
🛠️ Proposed fix
- default: $array['default'],
+ default: $array['default'] ?? null,🤖 Prompt for AI Agents
In `@src/Migration/Resources/Database/Attribute/Polygon.php` around lines 58 - 67,
In Polygon::fromArray change the handling of the 'default' index to guard
against missing keys: update the constructor call in the fromArray method to
pass $array['default'] ?? null (matching other attribute factories' null
fallback) so an undefined index notice is avoided; leave other fields (required,
createdAt, updatedAt) as-is and locate this in the fromArray method of the
Polygon class to apply the fix.
| /** | ||
| * @param array{ | ||
| * id: string, | ||
| * name: string, | ||
| * createdAt: string, | ||
| * updatedAt: string, | ||
| * enabled: bool, | ||
| * originalId: string|null, | ||
| * database: string | ||
| * } $array | ||
| */ |
There was a problem hiding this comment.
PHPDoc is missing the type key.
The @param array shape documents database: string but omits the type key, even though line 34 accesses $array['type']. Add type: string to the PHPDoc for consistency.
Suggested fix
/**
* `@param` array{
* id: string,
* name: string,
* createdAt: string,
* updatedAt: string,
* enabled: bool,
* originalId: string|null,
+ * type: string,
* database: string
* } $array
*/📝 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.
| /** | |
| * @param array{ | |
| * id: string, | |
| * name: string, | |
| * createdAt: string, | |
| * updatedAt: string, | |
| * enabled: bool, | |
| * originalId: string|null, | |
| * database: string | |
| * } $array | |
| */ | |
| /** | |
| * `@param` array{ | |
| * id: string, | |
| * name: string, | |
| * createdAt: string, | |
| * updatedAt: string, | |
| * enabled: bool, | |
| * originalId: string|null, | |
| * type: string, | |
| * database: string | |
| * } $array | |
| */ |
🤖 Prompt for AI Agents
In `@src/Migration/Resources/Database/VectorDB.php` around lines 14 - 24, The
PHPDoc for the array parameter is missing the "type" key even though the code
reads $array['type'] (see use in the VectorDB class/method handling that array);
update the `@param` array shape to include type: string so the documented shape
matches runtime access (add "type: string" to the array shape in the PHPDoc for
the $array parameter).
| 'database' => [ | ||
| 'id' => $database->getId(), | ||
| 'name' => $databaseName, | ||
| 'type' => $database->getType(), | ||
| 'database' => $database->getDatabase(), | ||
| ] | ||
| ]); |
There was a problem hiding this comment.
Potential bug: 'name' => $databaseName passes the database type instead of actual name.
At line 853, $databaseName is the database type string (e.g., 'database', 'documentsdb'), not the actual database name. This should likely be $database->getDatabaseName() or similar.
Proposed fix
$newTable = self::getEntity($databaseName, [
'id' => $table['$id'],
'name' => $table['name'],
'documentSecurity' => $table['documentSecurity'],
'permissions' => $table['$permissions'],
'createdAt' => $table['$createdAt'],
'updatedAt' => $table['$updatedAt'],
'database' => [
'id' => $database->getId(),
- 'name' => $databaseName,
+ 'name' => $database->getDatabaseName(),
'type' => $database->getType(),
'database' => $database->getDatabase(),
]
]);📝 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.
| 'database' => [ | |
| 'id' => $database->getId(), | |
| 'name' => $databaseName, | |
| 'type' => $database->getType(), | |
| 'database' => $database->getDatabase(), | |
| ] | |
| ]); | |
| 'database' => [ | |
| 'id' => $database->getId(), | |
| 'name' => $database->getDatabaseName(), | |
| 'type' => $database->getType(), | |
| 'database' => $database->getDatabase(), | |
| ] | |
| ]); |
🤖 Prompt for AI Agents
In `@src/Migration/Sources/Appwrite.php` around lines 851 - 857, The array
construction is using $databaseName (which holds the database type) for the
'name' field; update the assignment to use the actual database name accessor on
the $database object (e.g., replace $databaseName with
$database->getDatabaseName() or $database->getName() depending on the available
method) inside the block that builds the 'database' array so the 'name' value
reflects the real database name rather than its type.
| /** | ||
| * @var callable(UtopiaDocument|null): UtopiaDatabase | ||
| */ | ||
| private mixed $getDatabasesDB; | ||
|
|
||
| public function __construct( | ||
| private readonly UtopiaDatabase $dbForProject, | ||
| ?callable $getDatabasesDB = null, | ||
| private readonly ?string $projectId = null | ||
| ) { | ||
| $this->getDatabasesDB = $getDatabasesDB; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify getDatabase and logDebug methods are defined in Reader interface or related files
echo "=== Checking Reader interface ==="
cat src/Migration/Sources/Appwrite/Reader.php 2>/dev/null || echo "Reader.php not found"
echo ""
echo "=== Searching for getDatabase method definition ==="
rg -n 'function getDatabase\(' --type=php -C2
echo ""
echo "=== Searching for logDebug method definition ==="
rg -n 'function logDebug\(' --type=php -C2Repository: utopia-php/migration
Length of output: 4579
🏁 Script executed:
#!/bin/bash
# Check the Database.php class definition (parent class, traits, etc.)
echo "=== Checking Database.php class definition ==="
head -60 src/Migration/Sources/Appwrite/Reader/Database.php
echo ""
echo "=== Searching for logDebug in the entire codebase with context ==="
rg -i 'logdebug' --type=php -C2
echo ""
echo "=== Checking if Database class extends a parent or uses traits ==="
rg -n 'class Database' src/Migration/Sources/Appwrite/Reader/Database.php -A 5
echo ""
echo "=== Checking if getDatabase is called in Database.php and what parameters it takes ==="
rg -n 'getDatabase' src/Migration/Sources/Appwrite/Reader/Database.php -B2 -A2Repository: utopia-php/migration
Length of output: 9920
Fix missing method implementations: getDatabase() and logDebug()
The Database class calls $this->getDatabase() at lines 343, 398, and 496, and $this->logDebug() at lines 346 and 348, but these methods do not exist. The class implements only the Reader interface, has no parent class, and no traits that would provide these methods. The logDebug() method does not exist anywhere in the codebase, and getDatabase() is not defined in this class, its interface, or any parent. Either implement these methods in the Database class or provide them through a parent class or trait.
🤖 Prompt for AI Agents
In `@src/Migration/Sources/Appwrite/Reader/Database.php` around lines 27 - 38, The
Database class is missing implementations for getDatabase() and logDebug() which
are invoked from within the class; add a private getDatabase(): UtopiaDatabase
method that returns the correct DB instance by invoking the stored callable
$this->getDatabasesDB with $this->projectId when callable (and falling back to
$this->dbForProject), and add a private logDebug(string $message, array $context
= []): void method that emits debug output (format the message with context)
using an existing logger if present (e.g., $this->logger) or error_log as a
fallback so existing calls to getDatabase() and logDebug() succeed.
| public static function extractServices(array $services): array | ||
| { | ||
| $resources = []; | ||
| $groupDatabasesIndex = array_search(Transfer::GROUP_DATABASES, $services); |
There was a problem hiding this comment.
Remove unused variable $groupDatabasesIndex.
This variable is assigned but never used, as flagged by static analysis.
Proposed fix
- $groupDatabasesIndex = array_search(Transfer::GROUP_DATABASES, $services);📝 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.
| $groupDatabasesIndex = array_search(Transfer::GROUP_DATABASES, $services); |
🧰 Tools
🪛 PHPMD (2.15.0)
361-361: Avoid unused local variables such as '$groupDatabasesIndex'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
In `@src/Migration/Transfer.php` at line 361, The variable $groupDatabasesIndex is
assigned from array_search(Transfer::GROUP_DATABASES, $services) but never used;
remove the unused assignment line to eliminate the dead variable (i.e. delete
the statement that sets $groupDatabasesIndex) so only relevant variables and
logic remain; if the intent was to check presence of Transfer::GROUP_DATABASES
in $services instead, replace the assignment with an explicit conditional using
in_array(Transfer::GROUP_DATABASES, $services) where needed.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Migration/Sources/Appwrite.php (1)
733-745: Root database filter skips VectorDB.Line 733 only checks
TYPE_DATABASEandTYPE_DATABASE_DOCUMENTSDB, so a VectorDB root export won’t be filtered by ID and can export all databases. AddTYPE_DATABASE_VECTORDBto the condition.🔧 Suggested fix
- if ($this->rootResourceId !== '' && ($this->rootResourceType === Resource::TYPE_DATABASE || $this->rootResourceType === Resource::TYPE_DATABASE_DOCUMENTSDB)) { + if ( + $this->rootResourceId !== '' && + \in_array($this->rootResourceType, [ + Resource::TYPE_DATABASE, + Resource::TYPE_DATABASE_DOCUMENTSDB, + Resource::TYPE_DATABASE_VECTORDB, + ], true) + ) {
🤖 Fix all issues with AI agents
In `@src/Migration/Sources/Appwrite.php`:
- Around line 1072-1084: The code calls getRecord with the database name instead
of its type which makes record creation default to Row; change the first
argument of getRecord to use $table->getDatabase()->getType() (instead of
getDatabaseName()) so getRecord receives the expected database type; update the
call site in the Appwrite migration code where getRecord(...) is invoked and
ensure related metadata (the rest of the associative array built from $table and
$table->getDatabase()) remains unchanged.
- Around line 991-1034: Remove the unused local variables that PHPMD flags:
delete the $memUsage assignment and the two $timestamp assignments around the
listRows call in the method that iterates table rows (the block using
$this->reader->listRows($table, $queries)); alternatively, if you want to keep
those diagnostics, re-enable the commented $this->logDebugTrackedProject(...)
calls or guard them behind a debug flag so the variables are actually used
(refer to the logDebugTrackedProject call sites and the $this->reader->listRows
invocation when making the change).
In `@src/Migration/Sources/Appwrite/Reader/Database.php`:
- Around line 91-97: The code reads $database->getAttribute('type') and
immediately indexes Resource::DATABASE_TYPE_RESOURCE_MAP[$databaseType], which
can throw an undefined index for missing/unexpected types; update the logic in
the block around $databaseType and $databaseSpecificResources to default to
Resource::TYPE_DATABASE when getAttribute('type') returns null/empty or when
$databaseType is not a key in Resource::DATABASE_TYPE_RESOURCE_MAP (also keep
the existing normalization for Resource::TYPE_DATABASE_LEGACY and
Resource::TYPE_DATABASE_TABLESDB), e.g. compute $databaseType with a fallback
and guard the lookup with isset/array_key_exists before assigning
$databaseSpecificResources so it never indexes an unknown key.
♻️ Duplicate comments (1)
src/Migration/Sources/Appwrite.php (1)
830-842: Database name is set to type string, not actual name.Line 839 still assigns
'name' => $databaseName, but$databaseNameholds the database type (e.g.,database,documentsdb) rather than the actual database name. This was already raised previously.🔧 Suggested fix
- 'name' => $databaseName, + 'name' => $database->getDatabaseName(),
| $iterationCount++; | ||
|
|
||
| $memUsage = round(memory_get_usage(true) / 1024 / 1024, 2); | ||
| // $this->logDebugTrackedProject("Table: {$table->getName()} | Iteration: $iterationCount | Memory: {$memUsage}MB | LastRow: " . ($lastRow ? $lastRow->getId() : 'null')); | ||
|
|
||
| $queries = [ | ||
| $this->database->queryLimit($batchSize), | ||
| $this->reader->queryLimit($batchSize), | ||
| ...$this->queries, | ||
| ]; | ||
|
|
||
| $rows = []; | ||
|
|
||
| if ($lastRow) { | ||
| $queries[] = $this->database->queryCursorAfter($lastRow); | ||
| $queries[] = $this->reader->queryCursorAfter($lastRow); | ||
| } | ||
|
|
||
| $selects = ['*', '$id', '$permissions', '$updatedAt', '$createdAt']; // We want relations flat! | ||
| $manyToMany = []; | ||
|
|
||
| $attributes = $this->cache->get(Column::getName()); | ||
| foreach ($attributes as $attribute) { | ||
| /** @var Relationship $attribute */ | ||
| if ( | ||
| $attribute->getTable()->getId() === $table->getId() && | ||
| $attribute->getType() === Column::TYPE_RELATIONSHIP && | ||
| $attribute->getSide() === 'parent' && | ||
| $attribute->getRelationType() == 'manyToMany' | ||
| ) { | ||
| /** | ||
| * Blockers: | ||
| * we should use but Does not work properly: | ||
| * $selects[] = $attribute->getKey() . '.$id'; | ||
| * when selecting for a relation we get all relations not just the one we were asking. | ||
| * when selecting for a relation like select(*, relation.$id) , all relations get resolve | ||
| */ | ||
| $manyToMany[] = $attribute->getKey(); | ||
| if ($this->reader->getSupportForAttributes()) { | ||
| $attributes = $this->cache->get($fieldName); | ||
|
|
||
| foreach ($attributes as $attribute) { | ||
| /** @var Relationship $attribute */ | ||
| if ( | ||
| $attribute->getTable()->getId() === $table->getId() && | ||
| $attribute->getType() === Column::TYPE_RELATIONSHIP && | ||
| $attribute->getSide() === 'parent' && | ||
| $attribute->getRelationType() == 'manyToMany' | ||
| ) { | ||
| $manyToMany[] = $attribute->getKey(); | ||
| } | ||
| } | ||
| } | ||
| /** @var Column|Relationship $attribute */ | ||
|
|
||
| $queries[] = $this->database->querySelect($selects); | ||
| $queries[] = $this->reader->querySelect($selects); | ||
|
|
||
| $timestamp = microtime(true); | ||
| // $this->logDebugTrackedProject("BEFORE listRows() | Table: {$table->getName()} | Batch: $batchSize | Timestamp: {$timestamp}"); | ||
|
|
||
| $response = $this->database->listRows($table, $queries); | ||
| $response = $this->reader->listRows($table, $queries); | ||
|
|
||
| $timestamp = microtime(true); | ||
| // $this->logDebugTrackedProject("AFTER listRows() | Table: {$table->getName()} | Rows: " . count($response) . " | Timestamp: $timestamp"); |
There was a problem hiding this comment.
Remove unused local variables to satisfy PHPMD.
$memUsage (Line 993) and $timestamp (Lines 1028/1033) are computed but never used because the debug logs are commented out. PHPMD flags these as unused locals. Either re-enable logging behind a flag or remove the variables.
🔧 Suggested cleanup
- $memUsage = round(memory_get_usage(true) / 1024 / 1024, 2);
- // $this->logDebugTrackedProject("Table: {$table->getName()} | Iteration: $iterationCount | Memory: {$memUsage}MB | LastRow: " . ($lastRow ? $lastRow->getId() : 'null'));
+ // $this->logDebugTrackedProject("Table: {$table->getName()} | Iteration: $iterationCount | Memory: " . round(memory_get_usage(true) / 1024 / 1024, 2) . "MB | LastRow: " . ($lastRow ? $lastRow->getId() : 'null'));
- $timestamp = microtime(true);
// $this->logDebugTrackedProject("BEFORE listRows() | Table: {$table->getName()} | Batch: $batchSize | Timestamp: {$timestamp}");
- $response = $this->reader->listRows($table, $queries);
-
- $timestamp = microtime(true);
- // $this->logDebugTrackedProject("AFTER listRows() | Table: {$table->getName()} | Rows: " . count($response) . " | Timestamp: $timestamp");
+ $response = $this->reader->listRows($table, $queries);
+ // $this->logDebugTrackedProject("AFTER listRows() | Table: {$table->getName()} | Rows: " . count($response) . " | Timestamp: " . microtime(true));🧰 Tools
🪛 PHPMD (2.15.0)
993-993: Avoid unused local variables such as '$memUsage'. (undefined)
(UnusedLocalVariable)
1028-1028: Avoid unused local variables such as '$timestamp'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
In `@src/Migration/Sources/Appwrite.php` around lines 991 - 1034, Remove the
unused local variables that PHPMD flags: delete the $memUsage assignment and the
two $timestamp assignments around the listRows call in the method that iterates
table rows (the block using $this->reader->listRows($table, $queries));
alternatively, if you want to keep those diagnostics, re-enable the commented
$this->logDebugTrackedProject(...) calls or guard them behind a debug flag so
the variables are actually used (refer to the logDebugTrackedProject call sites
and the $this->reader->listRows invocation when making the change).
| $row = self::getRecord($table->getDatabase()->getDatabaseName(), [ | ||
| 'id' => $id, | ||
| 'table' => [ | ||
| 'id' => $table->getId(), | ||
| 'name' => $table->getTableName(), | ||
| 'rowSecurity' => $table->getRowSecurity(), | ||
| 'permissions' => $table->getPermissions(), | ||
| 'database' => [ | ||
| 'id' => $table->getDatabase()->getId(), | ||
| 'name' => $table->getDatabase()->getDatabaseName(), | ||
| 'type' => $table->getDatabase()->getType(), | ||
| 'database' => $table->getDatabase()->getDatabase(), | ||
| ] |
There was a problem hiding this comment.
Record type selection uses database name instead of database type.
Line 1072 calls getRecord($table->getDatabase()->getDatabaseName(), ...). getRecord() expects a database type (TYPE_DATABASE_DOCUMENTSDB, TYPE_DATABASE_VECTORDB, etc.), so passing the database name will cause Document DB/Vector DB records to be created as Row by default.
🔧 Suggested fix
- $row = self::getRecord($table->getDatabase()->getDatabaseName(), [
+ $row = self::getRecord($table->getDatabase()->getType(), [📝 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.
| $row = self::getRecord($table->getDatabase()->getDatabaseName(), [ | |
| 'id' => $id, | |
| 'table' => [ | |
| 'id' => $table->getId(), | |
| 'name' => $table->getTableName(), | |
| 'rowSecurity' => $table->getRowSecurity(), | |
| 'permissions' => $table->getPermissions(), | |
| 'database' => [ | |
| 'id' => $table->getDatabase()->getId(), | |
| 'name' => $table->getDatabase()->getDatabaseName(), | |
| 'type' => $table->getDatabase()->getType(), | |
| 'database' => $table->getDatabase()->getDatabase(), | |
| ] | |
| $row = self::getRecord($table->getDatabase()->getType(), [ | |
| 'id' => $id, | |
| 'table' => [ | |
| 'id' => $table->getId(), | |
| 'name' => $table->getTableName(), | |
| 'rowSecurity' => $table->getRowSecurity(), | |
| 'permissions' => $table->getPermissions(), | |
| 'database' => [ | |
| 'id' => $table->getDatabase()->getId(), | |
| 'name' => $table->getDatabase()->getDatabaseName(), | |
| 'type' => $table->getDatabase()->getType(), | |
| 'database' => $table->getDatabase()->getDatabase(), | |
| ] |
🤖 Prompt for AI Agents
In `@src/Migration/Sources/Appwrite.php` around lines 1072 - 1084, The code calls
getRecord with the database name instead of its type which makes record creation
default to Row; change the first argument of getRecord to use
$table->getDatabase()->getType() (instead of getDatabaseName()) so getRecord
receives the expected database type; update the call site in the Appwrite
migration code where getRecord(...) is invoked and ensure related metadata (the
rest of the associative array built from $table and $table->getDatabase())
remains unchanged.
| $databaseType = $database->getAttribute('type'); | ||
| if (in_array($databaseType, [Resource::TYPE_DATABASE_LEGACY,Resource::TYPE_DATABASE_TABLESDB])) { | ||
| $databaseType = Resource::TYPE_DATABASE; | ||
| } | ||
|
|
||
| if (!Resource::isSupported([Resource::TYPE_ROW, Resource::TYPE_COLUMN, Resource::TYPE_INDEX], $resources)) { | ||
| continue; | ||
| } | ||
| $databaseSpecificResources = Resource::DATABASE_TYPE_RESOURCE_MAP[$databaseType]; | ||
|
|
There was a problem hiding this comment.
Guard against missing/unknown database type before indexing the resource map.
Line 91 uses $database->getAttribute('type') without a default and immediately indexes Resource::DATABASE_TYPE_RESOURCE_MAP[$databaseType]. If older DB metadata lacks type (or has an unexpected value), this will trigger an undefined index and break the report. Consider defaulting to Resource::TYPE_DATABASE when the value is missing or not in the map.
🔧 Suggested fix
- $databaseType = $database->getAttribute('type');
- if (in_array($databaseType, [Resource::TYPE_DATABASE_LEGACY,Resource::TYPE_DATABASE_TABLESDB])) {
- $databaseType = Resource::TYPE_DATABASE;
- }
-
- $databaseSpecificResources = Resource::DATABASE_TYPE_RESOURCE_MAP[$databaseType];
+ $databaseType = $database->getAttribute('type', Resource::TYPE_DATABASE);
+ if (\in_array($databaseType, [Resource::TYPE_DATABASE_LEGACY, Resource::TYPE_DATABASE_TABLESDB], true)) {
+ $databaseType = Resource::TYPE_DATABASE;
+ }
+ if (!isset(Resource::DATABASE_TYPE_RESOURCE_MAP[$databaseType])) {
+ $databaseType = Resource::TYPE_DATABASE;
+ }
+
+ $databaseSpecificResources = Resource::DATABASE_TYPE_RESOURCE_MAP[$databaseType];🤖 Prompt for AI Agents
In `@src/Migration/Sources/Appwrite/Reader/Database.php` around lines 91 - 97, The
code reads $database->getAttribute('type') and immediately indexes
Resource::DATABASE_TYPE_RESOURCE_MAP[$databaseType], which can throw an
undefined index for missing/unexpected types; update the logic in the block
around $databaseType and $databaseSpecificResources to default to
Resource::TYPE_DATABASE when getAttribute('type') returns null/empty or when
$databaseType is not a key in Resource::DATABASE_TYPE_RESOURCE_MAP (also keep
the existing normalization for Resource::TYPE_DATABASE_LEGACY and
Resource::TYPE_DATABASE_TABLESDB), e.g. compute $databaseType with a fallback
and guard the lookup with isset/array_key_exists before assigning
$databaseSpecificResources so it never indexes an unknown key.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Migration/Destinations/Appwrite.php (1)
796-813: Normalized dates computed but not used.Lines 796-797 normalize the dates, but lines 811-812 use the raw resource values instead. This inconsistency could cause date format issues.
Proposed fix
$index = new UtopiaDocument([ '$id' => ID::custom($database->getSequence() . '_' . $table->getSequence() . '_' . $resource->getKey()), 'key' => $resource->getKey(), 'status' => 'available', // processing, available, failed, deleting, stuck 'databaseInternalId' => $database->getSequence(), 'databaseId' => $database->getId(), 'collectionInternalId' => $table->getSequence(), 'collectionId' => $table->getId(), 'type' => $resource->getType(), 'attributes' => $resource->getColumns(), 'lengths' => $lengths, 'orders' => $resource->getOrders(), - '$createdAt' => $resource->getCreatedAt(), - '$updatedAt' => $resource->getUpdatedAt(), + '$createdAt' => $createdAt, + '$updatedAt' => $updatedAt, ]);
🤖 Fix all issues with AI agents
In `@src/Migration/Destinations/Appwrite.php`:
- Around line 82-84: The PHPDoc parameter name for the callable is incorrect: it
documents $getDatabasesDSN but the actual parameter is named $getDatabaseDSN;
update the PHPDoc in src/Migration/Destinations/Appwrite.php so the param
annotation matches the real parameter name ($getDatabaseDSN) (or rename the
function parameter to $getDatabasesDSN if you prefer consistency), ensuring the
callable signature stays callable(string $databaseType):string and references
the correct symbol ($getDatabaseDSN) used in the class/method.
- Around line 352-357: The code duplicates the normalizeDateTime calls for
$createdAt and $updatedAt in the Appwrite migration (duplicate lines call
normalizeDateTime twice); remove the second pair of duplicate calls so
$createdAt and $updatedAt are only set once (keep the first calls that use
normalizeDateTime($resource->getCreatedAt()) and
normalizeDateTime($resource->getUpdatedAt(), $createdAt)), ensuring the
$updatedAt uses the $createdAt value from the initial normalization.
- Around line 973-994: The foreach currently declares an unused $value when
iterating $this->rowBuffer rows; change the inner loop to iterate keys only so
$value is not declared (e.g. replace foreach ($row as $key => $value) with a
keys-only iteration such as foreach (array_keys($row->getArrayCopy()) as $key)),
keeping the surrounding logic that checks $table->getAttribute('attributes', [])
and calls $row->removeAttribute($key) intact; this removes the unused $value
while preserving the orphan-attribute removal behavior tied to
$dbForDatabases->getAdapter()->getSupportForAttributes().
- Around line 429-434: The code duplicates calls to $this->normalizeDateTime for
$createdAt and $updatedAt in the Appwrite migration class; remove the second
pair of duplicate lines so only the first calls remain (keep the first
assignments: $createdAt = $this->normalizeDateTime($resource->getCreatedAt());
and $updatedAt = $this->normalizeDateTime($resource->getUpdatedAt(),
$createdAt);), ensuring the variables are assigned once in the method in class
Appwrite (look for the normalizeDateTime calls around the existing assignments
to $createdAt and $updatedAt).
- Line 15: Remove the unused import Dom\Document from the Appwrite migration
file: locate the top-level use statement "use Dom\Document" in the Appwrite
class and delete it (or replace it with the correct import if it was intended to
be used), ensuring no references to Document remain in the class (check methods
in class Appwrite) so the file no longer contains the unused import.
♻️ Duplicate comments (1)
src/Migration/Destinations/Appwrite.php (1)
471-476: Clarify DocumentsDB guard behavior and message.The guard blocks both
ColumnandAttributeresources for DocumentsDB, but the status message only mentions "Columns". Consider updating the message to accurately reflect the intended behavior:Proposed fix
if ($resource->getTable()->getDatabase()->getType() === Resource::TYPE_DATABASE_DOCUMENTSDB) { - $resource->setStatus(Resource::STATUS_SKIPPED, 'Columns not supported for DocumentsDB'); + $resource->setStatus(Resource::STATUS_SKIPPED, 'Schema fields not supported for DocumentsDB'); return false; }
🧹 Nitpick comments (1)
src/Migration/Destinations/Appwrite.php (1)
1534-1537: Unused variable$columnSize.The variable
$columnSizeis extracted but never used. Either remove it or add a comment indicating it's reserved for future use.Proposed fix (remove unused variable)
$columnStatus = $oldColumns[$columnIndex]['status']; $columnType = $oldColumns[$columnIndex]['type']; - $columnSize = $oldColumns[$columnIndex]['size']; $columnArray = $oldColumns[$columnIndex]['array'] ?? false;
| use Appwrite\Services\Storage; | ||
| use Appwrite\Services\Teams; | ||
| use Appwrite\Services\Users; | ||
| use Dom\Document; |
There was a problem hiding this comment.
Unused import detected.
Dom\Document is imported but not used anywhere in the file. This appears to be a leftover from refactoring.
Proposed fix
-use Dom\Document;📝 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.
| use Dom\Document; |
🤖 Prompt for AI Agents
In `@src/Migration/Destinations/Appwrite.php` at line 15, Remove the unused import
Dom\Document from the Appwrite migration file: locate the top-level use
statement "use Dom\Document" in the Appwrite class and delete it (or replace it
with the correct import if it was intended to be used), ensuring no references
to Document remain in the class (check methods in class Appwrite) so the file no
longer contains the unused import.
| * @param UtopiaDatabase $dbForProject | ||
| * @param callable(UtopiaDocument $database):UtopiaDatabase $getDatabasesDB | ||
| * @param callable(string $databaseType):string $getDatabasesDSN |
There was a problem hiding this comment.
PHPDoc parameter name mismatch.
The PHPDoc shows $getDatabasesDSN but the actual parameter is $getDatabaseDSN (without the 's').
Proposed fix
- * `@param` callable(string $databaseType):string $getDatabasesDSN
+ * `@param` callable(string $databaseType):string $getDatabaseDSN🤖 Prompt for AI Agents
In `@src/Migration/Destinations/Appwrite.php` around lines 82 - 84, The PHPDoc
parameter name for the callable is incorrect: it documents $getDatabasesDSN but
the actual parameter is named $getDatabaseDSN; update the PHPDoc in
src/Migration/Destinations/Appwrite.php so the param annotation matches the real
parameter name ($getDatabaseDSN) (or rename the function parameter to
$getDatabasesDSN if you prefer consistency), ensuring the callable signature
stays callable(string $databaseType):string and references the correct symbol
($getDatabaseDSN) used in the class/method.
| $createdAt = $this->normalizeDateTime($resource->getCreatedAt()); | ||
| $updatedAt = $this->normalizeDateTime($resource->getUpdatedAt(), $createdAt); | ||
|
|
||
| $database = $this->database->createDocument('databases', new UtopiaDocument([ | ||
| $createdAt = $this->normalizeDateTime($resource->getCreatedAt()); | ||
| $updatedAt = $this->normalizeDateTime($resource->getUpdatedAt(), $createdAt); | ||
|
|
There was a problem hiding this comment.
Duplicate normalizeDateTime calls.
Lines 355-356 duplicate the exact same normalization already performed on lines 352-353. This appears to be a copy-paste error.
Proposed fix
$createdAt = $this->normalizeDateTime($resource->getCreatedAt());
$updatedAt = $this->normalizeDateTime($resource->getUpdatedAt(), $createdAt);
- $createdAt = $this->normalizeDateTime($resource->getCreatedAt());
- $updatedAt = $this->normalizeDateTime($resource->getUpdatedAt(), $createdAt);
-
$database = $this->dbForProject->createDocument('databases', new UtopiaDocument([🤖 Prompt for AI Agents
In `@src/Migration/Destinations/Appwrite.php` around lines 352 - 357, The code
duplicates the normalizeDateTime calls for $createdAt and $updatedAt in the
Appwrite migration (duplicate lines call normalizeDateTime twice); remove the
second pair of duplicate calls so $createdAt and $updatedAt are only set once
(keep the first calls that use normalizeDateTime($resource->getCreatedAt()) and
normalizeDateTime($resource->getUpdatedAt(), $createdAt)), ensuring the
$updatedAt uses the $createdAt value from the initial normalization.
| $createdAt = $this->normalizeDateTime($resource->getCreatedAt()); | ||
| $updatedAt = $this->normalizeDateTime($resource->getUpdatedAt(), $createdAt); | ||
|
|
||
| $table = $this->database->createDocument('database_' . $database->getSequence(), new UtopiaDocument([ | ||
| $createdAt = $this->normalizeDateTime($resource->getCreatedAt()); | ||
| $updatedAt = $this->normalizeDateTime($resource->getUpdatedAt(), $createdAt); | ||
|
|
There was a problem hiding this comment.
Duplicate normalizeDateTime calls.
Lines 432-433 duplicate the normalization already performed on lines 429-430.
Proposed fix
$createdAt = $this->normalizeDateTime($resource->getCreatedAt());
$updatedAt = $this->normalizeDateTime($resource->getUpdatedAt(), $createdAt);
- $createdAt = $this->normalizeDateTime($resource->getCreatedAt());
- $updatedAt = $this->normalizeDateTime($resource->getUpdatedAt(), $createdAt);
-
$dbForDatabases = ($this->getDatabasesDB)($database);📝 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.
| $createdAt = $this->normalizeDateTime($resource->getCreatedAt()); | |
| $updatedAt = $this->normalizeDateTime($resource->getUpdatedAt(), $createdAt); | |
| $table = $this->database->createDocument('database_' . $database->getSequence(), new UtopiaDocument([ | |
| $createdAt = $this->normalizeDateTime($resource->getCreatedAt()); | |
| $updatedAt = $this->normalizeDateTime($resource->getUpdatedAt(), $createdAt); | |
| $createdAt = $this->normalizeDateTime($resource->getCreatedAt()); | |
| $updatedAt = $this->normalizeDateTime($resource->getUpdatedAt(), $createdAt); | |
| $dbForDatabases = ($this->getDatabasesDB)($database); |
🤖 Prompt for AI Agents
In `@src/Migration/Destinations/Appwrite.php` around lines 429 - 434, The code
duplicates calls to $this->normalizeDateTime for $createdAt and $updatedAt in
the Appwrite migration class; remove the second pair of duplicate lines so only
the first calls remain (keep the first assignments: $createdAt =
$this->normalizeDateTime($resource->getCreatedAt()); and $updatedAt =
$this->normalizeDateTime($resource->getUpdatedAt(), $createdAt);), ensuring the
variables are assigned once in the method in class Appwrite (look for the
normalizeDateTime calls around the existing assignments to $createdAt and
$updatedAt).
| if ($dbForDatabases->getAdapter()->getSupportForAttributes()) { | ||
| foreach ($this->rowBuffer as $row) { | ||
| foreach ($row as $key => $value) { | ||
| if (\str_starts_with($key, '$')) { | ||
| continue; | ||
| } | ||
|
|
||
| /** @var \Utopia\Database\Document $attribute */ | ||
| $found = false; | ||
| foreach ($table->getAttribute('attributes', []) as $attribute) { | ||
| if ($attribute->getAttribute('key') == $key) { | ||
| $found = true; | ||
| break; | ||
| /** @var \Utopia\Database\Document $attribute */ | ||
| $found = false; | ||
| foreach ($table->getAttribute('attributes', []) as $attribute) { | ||
| if ($attribute->getAttribute('key') == $key) { | ||
| $found = true; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (! $found) { | ||
| $row->removeAttribute($key); | ||
| if (! $found) { | ||
| $row->removeAttribute($key); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Unused variable $value in foreach loop.
The $value variable is declared but never used. Only $key is needed for the orphan attribute check.
Proposed fix
if ($dbForDatabases->getAdapter()->getSupportForAttributes()) {
foreach ($this->rowBuffer as $row) {
- foreach ($row as $key => $value) {
+ foreach ($row as $key => $_) {
if (\str_starts_with($key, '$')) {
continue;
}📝 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.
| if ($dbForDatabases->getAdapter()->getSupportForAttributes()) { | |
| foreach ($this->rowBuffer as $row) { | |
| foreach ($row as $key => $value) { | |
| if (\str_starts_with($key, '$')) { | |
| continue; | |
| } | |
| /** @var \Utopia\Database\Document $attribute */ | |
| $found = false; | |
| foreach ($table->getAttribute('attributes', []) as $attribute) { | |
| if ($attribute->getAttribute('key') == $key) { | |
| $found = true; | |
| break; | |
| /** @var \Utopia\Database\Document $attribute */ | |
| $found = false; | |
| foreach ($table->getAttribute('attributes', []) as $attribute) { | |
| if ($attribute->getAttribute('key') == $key) { | |
| $found = true; | |
| break; | |
| } | |
| } | |
| } | |
| if (! $found) { | |
| $row->removeAttribute($key); | |
| if (! $found) { | |
| $row->removeAttribute($key); | |
| } | |
| } | |
| } | |
| } | |
| if ($dbForDatabases->getAdapter()->getSupportForAttributes()) { | |
| foreach ($this->rowBuffer as $row) { | |
| foreach ($row as $key => $_) { | |
| if (\str_starts_with($key, '$')) { | |
| continue; | |
| } | |
| /** `@var` \Utopia\Database\Document $attribute */ | |
| $found = false; | |
| foreach ($table->getAttribute('attributes', []) as $attribute) { | |
| if ($attribute->getAttribute('key') == $key) { | |
| $found = true; | |
| break; | |
| } | |
| } | |
| if (! $found) { | |
| $row->removeAttribute($key); | |
| } | |
| } | |
| } | |
| } |
🧰 Tools
🪛 PHPMD (2.15.0)
975-975: Avoid unused local variables such as '$value'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
In `@src/Migration/Destinations/Appwrite.php` around lines 973 - 994, The foreach
currently declares an unused $value when iterating $this->rowBuffer rows; change
the inner loop to iterate keys only so $value is not declared (e.g. replace
foreach ($row as $key => $value) with a keys-only iteration such as foreach
(array_keys($row->getArrayCopy()) as $key)), keeping the surrounding logic that
checks $table->getAttribute('attributes', []) and calls
$row->removeAttribute($key) intact; this removes the unused $value while
preserving the orphan-attribute removal behavior tied to
$dbForDatabases->getAdapter()->getSupportForAttributes().
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Migration/Destinations/Appwrite.php (2)
796-813: Use normalized timestamps for index metadata.
$createdAt/$updatedAtare computed but not used. If the raw resource values aren’t normalized, index creation can fail validation.✅ Fix
- '$createdAt' => $resource->getCreatedAt(), - '$updatedAt' => $resource->getUpdatedAt(), + '$createdAt' => $createdAt, + '$updatedAt' => $updatedAt,
570-598: Use the target database adapter for attribute validation.The
checkAttribute()call validates against$this->dbForProject, which is the project metadata store, not the target database adapter. Since the attribute definition includes adapter-specific fields (type,format,formatOptions,filters), validation should occur against$dbForDatabasesto ensure compatibility with the target adapter's capabilities (e.g., vector support, spatial rules). Additionally,$dbForDatabasesis already initialized on line 572 but remains unused, suggesting this is the intended validation point.🔧 Proposed fix
- $this->dbForProject->checkAttribute($table, $column); + $dbForDatabases->checkAttribute($table, $column);
🤖 Fix all issues with AI agents
In `@src/Migration/Destinations/Appwrite.php`:
- Around line 1539-1542: Remove the unused variable assignment $columnSize from
the code that reads column metadata (the block that assigns $columnStatus,
$columnType, $columnSize, $columnArray from $oldColumns[$columnIndex]);
specifically delete the line assigning $columnSize (or, if size is actually
needed, use $columnSize where appropriate), leaving only $columnStatus,
$columnType and $columnArray assignments to resolve the PHPMD unused-variable
warning.
| $columnStatus = $oldColumns[$columnIndex]['status']; | ||
| $columnType = $oldColumns[$columnIndex]['type']; | ||
| $columnSize = $oldColumns[$columnIndex]['size']; | ||
| $columnArray = $oldColumns[$columnIndex]['array'] ?? false; |
There was a problem hiding this comment.
Remove unused $columnSize.
$columnSize is assigned but never used (PHPMD).
🧹 Cleanup
- $columnSize = $oldColumns[$columnIndex]['size'];📝 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.
| $columnStatus = $oldColumns[$columnIndex]['status']; | |
| $columnType = $oldColumns[$columnIndex]['type']; | |
| $columnSize = $oldColumns[$columnIndex]['size']; | |
| $columnArray = $oldColumns[$columnIndex]['array'] ?? false; | |
| $columnStatus = $oldColumns[$columnIndex]['status']; | |
| $columnType = $oldColumns[$columnIndex]['type']; | |
| $columnArray = $oldColumns[$columnIndex]['array'] ?? false; |
🧰 Tools
🪛 PHPMD (2.15.0)
1541-1541: Avoid unused local variables such as '$columnSize'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
In `@src/Migration/Destinations/Appwrite.php` around lines 1539 - 1542, Remove the
unused variable assignment $columnSize from the code that reads column metadata
(the block that assigns $columnStatus, $columnType, $columnSize, $columnArray
from $oldColumns[$columnIndex]); specifically delete the line assigning
$columnSize (or, if size is actually needed, use $columnSize where appropriate),
leaving only $columnStatus, $columnType and $columnArray assignments to resolve
the PHPMD unused-variable warning.
Multi-type db was reverted
So re-reverting the commits
Fix Double-Counting of
row/documentMigration Status CountersProblem
E2E test
testAppwriteMigrationMixedDatabaseswas failing with incorrectstatusCounters(e.g.,row.success = 2when only 1 row was migrated, orpendinggoing negative). Debug logs fromCache::add()showed the same row/document IDs being processed twice within a single migration run.Root Cause: Source + Destination Both Updated Cache for Rows/Documents
Rows and documents are special-cased in
Cacheto store aggregated counters per status (as strings), not per-resource objects. But they were being counted twice:cache->update($responseResource)during importcache->addAll($prunedResources)after destination callback returnsFor
rowanddocument,Cache::update()delegates toCache::add(), which increments the aggregated counter. So if source also adds them, it becomes double counting.Evidence in Code (Before Fix)
Source adds exported resources to cache after destination callback:
Destination updates cache for every imported resource:
Rows/documents are aggregated in cache, and
update()callsadd()for them:Solution
Skip source-side cache population for
row/document- destination import is already the single source of truth for these counters:Also clamp row/document
pendingsubtraction to prevent negative values if counts drift:Summary by CodeRabbit
New Features
Changes
✏️ Tip: You can customize this high-level summary in your review settings.