feat(repository): add the repository and seeder layers - #948
Conversation
Splits out of the query-lib migration, which had carried these along with it. Repository gives a typed collection facade -- Repository, Scope, Specification and CompositeSpecification -- so a caller composes reusable query fragments instead of assembling Query arrays at each call site. Seeder gives Factory, FactoryDefinition, Fixture and SeederRunner for building test and development data. Stacked on feat-query-lib rather than main: both are written against the Attribute, Collection and Query value objects that migration introduces. Nothing consumes either one yet, in this library or in appwrite or cloud, so neither has been exercised against a real engine. The tests here are unit tests over composition and the factory definitions. 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 SummaryThe PR adds repository abstractions for reusable query scopes/specifications and seeder abstractions for factories, fixtures, dependency ordering, and execution.
Confidence Score: 3/5The PR should not merge until global scopes cover ID-based repository operations and fixture cleanup preserves or reports failed deletions. ID-based repository methods can operate outside configured scopes, and fixture cleanup silently loses tracking for records that remain after deletion errors. Files Needing Attention: src/Database/Repository/Repository.php, src/Database/Seeder/Fixture.php Important Files Changed
Prompt To Fix All With AI### Issue 1
src/Database/Repository/Repository.php:53-56
**Global scopes are bypassed**
When a repository registers a tenant, soft-delete, or other business scope, `findById()` calls the database directly without applying it; `update()` and `delete()` have the same bypass. An out-of-scope document can therefore be returned, updated, or deleted.
### Issue 2
src/Database/Seeder/Fixture.php:53
**Failed cleanup loses tracking**
When `deleteDocument()` throws during cleanup, the exception is suppressed and the entire tracking list is still cleared. The undeleted record remains in the database while `getCreated()` reports nothing outstanding, preventing cleanup from being retried and contaminating later tests or development data.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "feat(repository): add the repository and..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| public function findById(string $id): Document | ||
| { | ||
| return $this->db->getDocument($this->collection(), $id); | ||
| } |
There was a problem hiding this comment.
When a repository registers a tenant, soft-delete, or other business scope, findById() calls the database directly without applying it; update() and delete() have the same bypass. An out-of-scope document can therefore be returned, updated, or deleted.
Knowledge Base Used:
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Database/Repository/Repository.php
Line: 53-56
Comment:
**Global scopes are bypassed**
When a repository registers a tenant, soft-delete, or other business scope, `findById()` calls the database directly without applying it; `update()` and `delete()` have the same bypass. An out-of-scope document can therefore be returned, updated, or deleted.
**Knowledge Base Used:**
- [Query construction and execution](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/utopia-php/database/-/docs/query-execution.md)
- [Database orchestration](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/utopia-php/database/-/docs/database-orchestration.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
When deleteDocument() throws during cleanup, the exception is suppressed and the entire tracking list is still cleared. The undeleted record remains in the database while getCreated() reports nothing outstanding, preventing cleanup from being retried and contaminating later tests or development data.
Knowledge Base Used: Document lifecycle and representation
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Database/Seeder/Fixture.php
Line: 53
Comment:
**Failed cleanup loses tracking**
When `deleteDocument()` throws during cleanup, the exception is suppressed and the entire tracking list is still cleared. The undeleted record remains in the database while `getCreated()` reports nothing outstanding, preventing cleanup from being retried and contaminating later tests or development data.
**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.1 participant
Split out of #823, which had carried these along with the query-lib migration.
Repositorygives a typed collection facade —Repository,Scope,Specification,CompositeSpecification— so a caller composes reusable query fragments instead of assemblingQueryarrays at each call site.SeedergivesFactory,FactoryDefinition,FixtureandSeederRunnerfor building test and development data.Why separately
The migration is a move every caller has to make. These are capabilities we chose. While they shared a branch a reviewer could not take one and leave the other.
Why it targets feat-query-lib
Both are written against the
Attribute,CollectionandQueryvalue objects #823 introduces, so neither can sit onmainuntil that lands. Retarget once it does.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 consumes either one — not this library, not appwrite, not cloud. The tests here are unit tests over composition and the factory definitions; neither has been exercised against a real engine.
CI is not being driven to green on this PR. It is parked behind #823 and will be rebased and fixed once that lands.