feat(migration): add the migration runner and schema differ - #949
feat(migration): add the migration runner and schema differ#949abnegate wants to merge 1 commit into
Conversation
Splits out of the query-lib migration, which had carried these along with it. Migration gives versioned up/down migrations with a Runner, an online schema change path and a Generator that scaffolds a migration file. Schema gives the introspector and the Diff that reports what changed between a declared collection and the one the engine holds. The two are a closed loop: Schema serves Migration\Generator, and bin/tasks/migrate.php is the only entry point, registered from bin/cli.php. Stacked on feat-query-lib rather than main: both are written against the Attribute, Collection and Index value objects that migration introduces. loadMigrations() discovers migration classes by what the file declares rather than by its filename, because migrate:generate writes the class under a namespace and a filename lookup silently found nothing -- the run reported success having skipped the migration. Two tests in CLITasksTest cover that, and they must run after the test that includes bin/tasks/migrate.php, since a second include redeclares the function. Nothing outside this library consumes it. Neither appwrite nor cloud references Utopia\Database\Migration or Utopia\Database\Schema. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Greptile SummaryAdds a versioned migration framework, CLI migration commands, online schema-change helpers, schema introspection and diffing, plus tests for the new flows.
Confidence Score: 2/5The PR should not merge until fresh migrations reliably remove every collection and surface deletion failures, and generated migrations preserve attribute modifications. The fresh path can leave collections or partial schema behind while reporting completion, and the generator turns a supported attribute modification into a successful no-op migration. Files Needing Attention: src/Database/Migration/Runner.php, src/Database/Migration/Generator.php Important Files Changed
Prompt To Fix All With AI### Issue 1
src/Database/Migration/Runner.php:112
**Fresh truncates collection list**
If the database contains more than 25 collections, `listCollections()` returns only its default-sized first page, so `fresh()` leaves later collections and their data intact while the command reports completion.
### Issue 2
src/Database/Migration/Runner.php:117-120
**Fresh suppresses deletion failures**
When `deleteCollection()` throws or returns false, `fresh()` ignores the failure and proceeds to reset migration tracking and rerun migrations, leaving retained or partially deleted schema while the CLI can still report successful completion.
### Issue 3
src/Database/Migration/Generator.php:108-122
**Generator drops attribute modifications**
When a diff contains `ModifyAttribute`, both statement generators fall through to `null`, producing a no-op migration that can be marked applied while the stored attribute remains unchanged.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "feat(migration): add the migration runne..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| */ | ||
| public function fresh(array $migrations): int | ||
| { | ||
| $collections = $this->db->listCollections(); |
There was a problem hiding this comment.
If the database contains more than 25 collections, listCollections() returns only its default-sized first page, so fresh() leaves later collections and their data intact while the command reports completion.
Knowledge Base Used: Collection schema management
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Database/Migration/Runner.php
Line: 112
Comment:
**Fresh truncates collection list**
If the database contains more than 25 collections, `listCollections()` returns only its default-sized first page, so `fresh()` leaves later collections and their data intact while the command reports completion.
**Knowledge Base Used:** [Collection schema management](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/utopia-php/database/-/docs/collection-schema-management.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| try { | ||
| $this->db->deleteCollection($id); | ||
| } catch (\Throwable) { | ||
| } |
There was a problem hiding this comment.
When deleteCollection() throws or returns false, fresh() ignores the failure and proceeds to reset migration tracking and rerun migrations, leaving retained or partially deleted schema while the CLI can still report successful completion.
Knowledge Base Used: Collection schema management
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Database/Migration/Runner.php
Line: 117-120
Comment:
**Fresh suppresses deletion failures**
When `deleteCollection()` throws or returns false, `fresh()` ignores the failure and proceeds to reset migration tracking and rerun migrations, leaving retained or partially deleted schema while the CLI can still report successful completion.
**Knowledge Base Used:** [Collection schema management](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/utopia-php/database/-/docs/collection-schema-management.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| return match ($change->type) { | ||
| ChangeType::AddAttribute => $change->attribute !== null | ||
| ? "\$db->createAttribute('{$collectionId}', new \\Utopia\\Database\\Attribute(key: '{$change->attribute->key}', type: \\Utopia\\Query\\Schema\\ColumnType::" . \ucfirst($change->attribute->type->value) . ", size: {$change->attribute->size}));" | ||
| : null, | ||
| ChangeType::DropAttribute => $change->attribute !== null | ||
| ? "\$db->deleteAttribute('{$collectionId}', '{$change->attribute->key}');" | ||
| : null, | ||
| ChangeType::AddIndex => $change->index !== null | ||
| ? "\$db->createIndex('{$collectionId}', new \\Utopia\\Database\\Index(key: '{$change->index->key}', type: \\Utopia\\Query\\Schema\\IndexType::" . \ucfirst($change->index->type->value) . ", attributes: " . \var_export($change->index->attributes, true) . '));' | ||
| : null, | ||
| ChangeType::DropIndex => $change->index !== null | ||
| ? "\$db->deleteIndex('{$collectionId}', '{$change->index->key}');" | ||
| : null, | ||
| default => null, | ||
| }; |
There was a problem hiding this comment.
When a diff contains ModifyAttribute, both statement generators fall through to null, producing a no-op migration that can be marked applied while the stored attribute remains unchanged.
Knowledge Base Used: Collection schema management
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Database/Migration/Generator.php
Line: 108-122
Comment:
**Generator drops attribute modifications**
When a diff contains `ModifyAttribute`, both statement generators fall through to `null`, producing a no-op migration that can be marked applied while the stored attribute remains unchanged.
**Knowledge Base Used:** [Collection schema management](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/utopia-php/database/-/docs/collection-schema-management.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Re-adds the entity mapper that came in with the query-lib migration and was split back out of it, rebased onto the migration's current head. Introspector::generateEntityClass() does not come back with it. That method emits the mapping attributes as text, so it belongs to the mapper, but it lives in Schema/Introspector.php, which moved to the migration-runner change (#949). This branch is based on the query-lib migration, where that file does not exist. Whoever lands both can put the codegen back on top; nothing in the mapper calls it, and no caller in this library or downstream references it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Split out of #823, which had carried these along with the query-lib migration.
Migrationgives versioned up/down migrations with aRunner, an online schema change path, and aGeneratorthat scaffolds a migration file.Schemagives the introspector and theDiffthat reports what changed between a declared collection and the one the engine holds.The two are a closed loop:
Schemaexists to serveMigration\Generator, andbin/tasks/migrate.phpis the only entry point, registered frombin/cli.php.Why separately
The migration is a forced move; a migration runner is a capability we chose, and it arrived on the same branch.
Why it targets feat-query-lib
Written against the
Attribute,CollectionandIndexvalue objects #823 introduces. Retarget tomainonce that lands.One fix worth noting
loadMigrations()discovers migration classes by what the file declares rather than by its filename.migrate:generatewrites the class under a namespace (App\Migrationby default), so a filename lookup found nothing — silently, leaving the run reporting success having skipped the migration. Two tests inCLITasksTestcover it, and they have to run after the test that includesbin/tasks/migrate.php, since a second include redeclares the function.Introspector
This PR owns
src/Database/Schema/Introspector.php, in the form it has after the ORM split:introspectCollection()andintrospectDatabase(), which read schema and need nothing from the entity mapper.generateEntityClass()and the four helpers only it used went with the mapper to #947 — and are currently in neither branch, since #947 is based on #823 where this file does not exist. Whichever of the two lands second can put the codegen back.Chain
Landing order, bottom up:
Stacked on #823 but not part of it, and not required by anything above: #947 (ORM), #948 (repositories and seeding), #949 (migration runner and schema differ).
Every
dev-feat-query-libpin in this train is re-pinned to its branch head whenever one of them moves, so each PR's CI runs against what the others actually contain.Not verified
Nothing outside this library consumes it. Neither appwrite nor cloud references
Utopia\Database\MigrationorUtopia\Database\Schema.CI is not being driven to green on this PR. It is parked behind #823 and will be rebased and fixed once that lands.