feat(orm): map entities onto collections - #947
Conversation
|
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 SummaryThe PR introduces attribute-driven entity metadata, entity/document mapping, identity tracking, unit-of-work persistence, schema creation and synchronization, and Database facade entry points.
Confidence Score: 2/5The PR is not yet safe to merge because cached soft-deleted entities remain visible, existing schemas do not reconcile relationships, and transaction retries can repeat lifecycle effects. Default ID reads can return identity-mapped soft-deleted entities before the visibility check, schema synchronization ignores generated relationship definitions for existing collections, and retryable flush callbacks repeat hooks and in-memory mutations that rollback does not reverse. Files Needing Attention: src/Database/ORM/EntityManager.php, src/Database/ORM/UnitOfWork.php Important Files Changed
Reviews (4): Last reviewed commit: "fix(orm): hide soft-deleted entities fro..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| $existing = $this->identityMap->get($metadata->collection, $id); | ||
| if ($existing !== null) { | ||
| /** @var T $existing */ | ||
| return $existing; | ||
| } | ||
|
|
||
| $document = $this->db->getDocument($metadata->collection, $id); |
There was a problem hiding this comment.
When an ID belongs to a soft-deleted entity, find() returns it from the identity map or loads it through getDocument() without applying the soft-delete filter, causing ID lookups to expose records that findMany() and findOne() hide by default.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Database/ORM/EntityManager.php
Line: 68-74
Comment:
**Soft-deleted entities remain visible**
When an ID belongs to a soft-deleted entity, `find()` returns it from the identity map or loads it through `getDocument()` without applying the soft-delete filter, causing ID lookups to expose records that `findMany()` and `findOne()` hide by default.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| $defs = $this->entityMapper->toCollectionDefinitions($metadata); | ||
|
|
||
| /** @var \Utopia\Database\Collection $desired */ | ||
| $desired = $defs['collection']; | ||
|
|
||
| if (! $this->db->exists($this->db->getAdapter()->getDatabase(), $metadata->collection)) { | ||
| $this->createCollectionFromEntity($className); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| $current = $this->db->getCollection($metadata->collection); | ||
|
|
||
| $differ = new \Utopia\Database\Schema\Diff(); | ||
| $diff = $differ->diff($current, $desired); |
There was a problem hiding this comment.
When an existing collection's relationship annotations are added, removed, or changed, this branch applies only the collection attribute/index diff and ignores defs['relationships'], leaving relationship metadata and backend structures missing or stale and potentially treating existing relationship attributes as invalid attribute removals.
Knowledge Base Used: Collection schema management
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Database/ORM/EntityManager.php
Line: 155-169
Comment:
**Relationship synchronization is omitted**
When an existing collection's relationship annotations are added, removed, or changed, this branch applies only the collection attribute/index diff and ignores `defs['relationships']`, leaving relationship metadata and backend structures missing or stale and potentially treating existing relationship attributes as invalid attribute removals.
**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.| $db->withTransaction(function () use ($db, $inserts, $updates, $deletes): void { | ||
| foreach ($inserts as $collection => $entities) { | ||
| $documents = []; | ||
| $entityMap = []; | ||
|
|
||
| foreach ($entities as $entity) { | ||
| $metadata = $this->metadataFactory->getMetadata($entity::class); | ||
| $this->invokeCallbacks($entity, $metadata->prePersistCallbacks); |
There was a problem hiding this comment.
When a retryable failure occurs after an earlier operation in the flush has completed, withTransaction() replays this callback after it has already invoked lifecycle hooks and mutated entity state, causing hooks and entity mutations to run multiple times or survive a final rollback.
Knowledge Base Used: Transactions, retries, and caching
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Database/ORM/UnitOfWork.php
Line: 221-228
Comment:
**Transaction retries repeat callbacks**
When a retryable failure occurs after an earlier operation in the flush has completed, `withTransaction()` replays this callback after it has already invoked lifecycle hooks and mutated entity state, causing hooks and entity mutations to run multiple times or survive a final rollback.
**Knowledge Base Used:** [Transactions, retries, and caching](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/utopia-php/database/-/docs/transactions-and-cache.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| $existing = $this->identityMap->get($metadata->collection, $id); | ||
| if ($existing !== null) { | ||
| /** @var T $existing */ | ||
| return $existing; |
There was a problem hiding this comment.
When the same EntityManager first loads a soft-deleted entity with withTrashed=true and then performs a default lookup for that ID, find() returns the identity-mapped instance before reaching the soft-delete check, causing the default lookup to expose an entity that default listings exclude.
Knowledge Base Used: Document lifecycle and representation
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Database/ORM/EntityManager.php
Line: 68-71
Comment:
**Cached soft-deletes remain visible**
When the same `EntityManager` first loads a soft-deleted entity with `withTrashed=true` and then performs a default lookup for that ID, `find()` returns the identity-mapped instance before reaching the soft-delete check, causing the default lookup to expose an entity that default listings exclude.
**Knowledge Base Used:** [Document lifecycle and representation](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/utopia-php/database/-/docs/document-lifecycle.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>
…ated defaults Two of the four review findings, both contained. find() returned a soft-deleted entity where findMany() hides it, so the same record was absent from a listing and present from a direct fetch. It takes withTrashed like findMany does, and Database::findEntity() passes it through. Introspector interpolated a string default straight between single quotes, so a default carrying an apostrophe, backslash or newline emitted malformed PHP. var_export renders every scalar as a valid literal. The other two findings -- relationship synchronisation in syncCollectionFromEntity(), and withTransaction() replaying a flush callback whose lifecycle hooks have already run -- are architectural and belong with the "before this is used anywhere" list in the PR body rather than a patch here. Nothing consumes this yet. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Split out of #823, which had carried this along with the query-lib migration.
Why separately
The migration is a move every caller has to make. An entity mapper is a feature we chose. While they shared a branch a reviewer could not take one and leave the other, and this is the larger half of what put that diff past Greptile's file limit — 5,681 lines across 53 files, needing a manual review bypass on every push.
What it is
An entity is a plain class carrying attributes —
Entity,Column,Id,BelongsTo,Embedded,SoftDelete, lifecycle hooks.MetadataFactoryreads them once;EntityMapperturns them into theAttributeandIndexobjectscreateCollection()now takes.EntityManagertracks what has been loaded and what changed, andflush()writes the difference.Databasegains the entry points:persistEntity,removeEntity,flushEntities,findEntity,findEntities,findOneEntity,createCollectionFromEntity,syncCollectionFromEntity,detachEntity,clearEntityManager,getEntityManager.What it does not include
Introspector::generateEntityClass()— read a collection, emit the entity class for it — is in no branch right now. It belongs to the mapper, but it lives insrc/Database/Schema/Introspector.php, which went to #949. This branch is based on #823, where that file does not exist, so the method has nowhere to land until one of the two merges. It is recoverable from771a8e2f^. Nothing in the mapper calls it and no caller in this library or downstream references it, so its absence costs only the codegen convenience.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
Nothing on this head. The branch was rebuilt on #823's current head after the repository/seeder and migration-runner splits moved it, so the earlier green — phpstan at level max, pint, 1826 tests — was earned on a base that no longer exists.
php -lover the 52 changed files is clean and that is all that has been re-run. This PR is not ready to merge and is not being driven to green; it is parked behind #823.Not verified
Coroutine scoping is unresolved.
IdentityMapis a plain array on anEntityManagerheld by theDatabasehandle, and nothing here is coroutine-scoped. Cloud shares one handle across coroutines — that is why its pool pins per coroutine — so an identity map on that handle would accumulate process-wide and be shared between concurrent requests. This needs the same scoping the pool pins get, or anEntityManagerper request, before any Swoole caller touches it. Unused it is a hazard rather than a bug, which is the other reason not to land it inside the migration.Nothing exercises it against a real engine. The tests are unit tests over the mapping and the unit of work. There is no E2E, and no caller in appwrite or cloud. Flush ordering against relationships, and the interaction with the existing document cache, are what I would want a live test over first.