feat: adapt migration schema calls to query-lib value objects - #222
feat: adapt migration schema calls to query-lib value objects#222abnegate wants to merge 18 commits into
Conversation
Published 2.0 still used Database::VAR_* and positional createAttribute/createIndex, which feat-query-lib removed. Rebase onto main and pass Attribute/Index/Relationship VOs plus ColumnType/IndexType so Appwrite can pin this branch as 2.0.0.
Appwrite stores ColumnType::BigInteger as biginteger. CSV export resolved that as an unsupported column type and wrote no rows.
createDocument can return an empty Mongo sequence while a subsequent
getDocument has the ObjectId. Creating database_{seq} from the create
return left table import looking up a collection that did not exist.
Appwrite main added huggingface as a project OAuth2 provider. Without an allow-list entry, Appwrite-to-Appwrite migrations fail on that provider even when the rest of the transfer succeeded.
Keep query-lib APIs and the huggingface PROVIDERS allow-list already on main.
Appwrite #11649 locks database at 5719edd. Staying on e593b78 would only prove the schema VO calls against an older query-lib surface.
Greptile SummaryThe PR migrates Appwrite schema operations to query-lib value objects and updates the associated dependency lock, schema conversions, and test bootstrap. It also completes database retry recovery for metadata stranded in either
Confidence Score: 5/5The PR appears safe to merge because the previously reported database retry and reload failures are addressed and no blocking failure remains. No blocking failure remains. Important Files Changed
Reviews (12): Last reviewed commit: "fix(destination): let a database strande..." | Re-trigger Greptile |
utopia-php/database feat-query-lib keys silenced events with Coroutine::getCid(). The CI image is vanilla PHP, so Memory-adapter tests fatalled before they could run.
createDocument can persist a row whose subsequent getDocument is empty. That throw sat outside the failed-status handler, so a later skip could flip the unusable database to ready without a backing collection.
|
Addressed the reload-failure finding.
Reload + Also stubbed @greptile-apps review |
Asterisk wildcards on utopia-php packages are replaced with equivalent caret constraints so Composer ranges stay consistent.
Keep composer.json and composer.lock in sync so `composer validate` passes, and pin utopia-php/database to the current query-lib HEAD.
|
@greptileai review |
|
@greptile-apps review Force re-review of HEAD |
Database::createCollection no longer accepts a string id.
Database::checkAttribute now requires Attribute. Build schema models from the resource key so metadata document IDs are not used as attribute keys.
Appwrite E2E migrations failed because checkAttribute now requires Attribute, and the destination still handed it a metadata Document.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
A reload failure leaves a metadata document in `failed` with no backing collection. Recovery only ran when onDuplicate was not Fail, so the default policy retried createDocument against the existing ID and stranded the database.
|
@greptileai review |
Index types already used IndexType; column direction was still a raw ASC string. Collection constructors with multiple named params were also jammed on one line.
…atabase The lock held utopia-php/query at dev-feat-schema-order, a branch that no longer exists on the remote, so the resolution only survived as long as nobody resolved it again. database's branch requires query 0.6.*, which is released, so this takes the release. storage 4.0.4 comes along because 4.0.3 capped utopia-php/validators at ^0.4 while database requires ^0.5; 4.0.4 dropped the validators dependency outright. database has required ^0.5 on main as well as on the branch, so this was already true before the query-lib work and only surfaced now that the lock moved. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Appwrite destination passed the 'ASC'/'DESC' strings a source hands back straight into Utopia\Database\Index, which takes Order cases and rejects anything else. The resulting InvalidArgumentException is not a Migration Exception, so instead of recording a failed index the transfer aborted. This was live before the lock re-pin and simply could not be seen: the lock held utopia-php/query at a deleted branch whose Index took plain strings, so CI never built an index against the contract the released library actually has. AppwriteIndexLengthsTest covers it -- two of its cases pass ['ASC', 'ASC'] and went red on the first run against the re-pinned lock. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Recovery was gated on `failed` alone, but `provisioning` is reachable as a terminal state on its own. markDatabaseFailed() deliberately swallows its own error so a secondary failure cannot mask the caller's throw, so a metadata store that is down for both the reload and the status write leaves the document in `provisioning` with no backing collection. Under the default OnDuplicate::Fail that document was unrecoverable: the gate opened only for `failed`, so every retry fell through to createDocument and hit "Document already exists", and the backing collection was never created. The `provisioning` handling further down, in the Skip branch, sat inside the same gate and so was unreachable in exactly the case it was written for. Both states mean the same thing -- a prior run created the metadata and did not finish -- so the predicate now covers both. The collection is still only recreated when it is actually missing, which was already the guard. testProvisioningDatabaseRetrySucceedsUnderOnDuplicateFail drives the real writer rather than seeding a status: it fails the reload and then the status write, asserts the document is stranded in `provisioning` with no collection, and then asserts the retry recovers it. Seen red as "Document already exists". Found by Greptile on #222. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Recording the reasoning for the P1 ("Provisioning state blocks retries"), since the thread anchor moved when I pushed the fix. It was valid, and is fixed in Confirmed by driving the real writer rather than seeding a status: fail the reload, then fail the status write that would have recorded the failure. The gate opened only for
For the record: this was pre-existing on |
1 participant
Adapts the migration destination's schema calls to the query-lib value objects.
Why this approach
Database::createCollection()takes aCollectiononly, so destination Appwrite now wraps the collection id, attributes and indexes — along with the named permissions anddocumentSecurity— innew Collection(...)instead of passing positional arrays.utopia-php/databaseis pinned todev-feat-query-lib as 2.0.0, re-pinned to that branch's head whenever it moves.Lock repairs that came with the re-pin
Two problems surfaced only when the lock was resolved again rather than reused:
utopia-php/querywas locked todev-feat-schema-order, a branch that no longer exists on the remote. The resolution survived only as long as nobody resolved it. database's branch requiresquery 0.6.*, which is released, so the lock now takes the release.utopia-php/storagemoved 4.0.3 → 4.0.4, because 4.0.3 cappedutopia-php/validatorsat^0.4while database requires^0.5. 4.0.4 dropped the validators dependency outright. database has required^0.5onmainas well as on the branch, so this was already true before the query-lib work and only surfaced now.A live bug the re-pin exposed
The Appwrite destination passed the
'ASC'/'DESC'strings a source hands back straight intoUtopia\Database\Index, which takesOrdercases and rejects anything else. The resultingInvalidArgumentExceptionis not a MigrationException, so instead of recording a failed index the whole transfer aborted.This was already true before this PR and simply could not be seen: the lock held
queryat the deleted branch, whoseIndextook plain strings, so CI had never built an index against the contract the released library actually has.AppwriteIndexLengthsTestcovers it — two of its cases pass['ASC', 'ASC'], and both went red on the first run against the re-pinned lock and green with the fix.It is the same defect as the one in appwrite/appwrite#11649's
Databasesworker, from the same cause.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.Verified
Not verified
utopia-php/databasedependency is still a branch pin. It becomes a released tag only once #823 merges, and this PR should not land before that.