feat: adapt audit schema to query-lib Attribute and Index VOs - #133
feat: adapt audit schema to query-lib Attribute and Index VOs#133abnegate wants to merge 18 commits into
Conversation
Needed so Appwrite can take main's queue 1.3 pin.
Take main's validators ^0.5 pin and refresh database feat-query-lib so Attribute/Index/ColumnType stay current.
ClickHouse was mutating parent Attribute/Index instances in place. MariaDB tests were also losing event=delete rows because persistent PDO plus a new adapter per PHPUnit setUp rolled back uncommitted writes.
|
Thanks for contributing! This repository is a read-only mirror; development for this library happens in |
|
@greptile-apps review |
|
Schema definitions with a known column or index type now use Attribute::string() / Index::key() instead of the generic constructors plus ColumnType / IndexType.
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 feat-query-lib now requires query ^0.5 after nested joins.
main's #91 migrated the ClickHouse adapter onto the query library's own builder and schema and moved to query 0.6, which is strictly ahead of the hand-rolled SQL this branch carried. That side is taken wholesale: the builder, the schema, the SQL snapshot test, Query.php and the 0.6 constraint are all main's. What main does not have is the database 7 adaptation, which is the reason this branch exists. Released database 7.2.2 still takes createCollection(string, array, array) and still exposes VAR_STRING, INDEX_KEY and VAR_DATETIME; the dev branch behind PR #823 replaces all of that with Collection, Attribute and Index value objects and drops the constants. main's ClickHouse.php therefore does not load against it -- 48 PHPStan errors, all undefined constants. So getAttributes() and getIndexes() are ported onto main's file as value objects, and the three consumers that still read $attribute['type'] and $attribute['required'] now read the properties. The port is attribute-for-attribute equal: all 35 columns match on type and required, and Attribute::string() defaults size to Database::LENGTH_KEY, which is what the raw arrays passed. The eight indexes map onto Index::key(), which is the same type/lengths/orders triple. Three of main's ClickHouse tests asserted the array shape and now assert the value objects. database is pinned to dev-feat-query-lib as 7.0.0 with the VCS repositories, matching abuse#124 and migration#222. All four pins come off together when database 8 is tagged. Verified against the real backends: PHPStan clean, 22 unit tests, 91 e2e tests with 979 assertions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
utopia-php/database#823 moved its ORM, repository/seeder and migration-runner layers into stacked PRs, so its branch head moved. Nothing here referenced any of them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
utopia-php/database moved to ^0.6 and appwrite/appwrite followed on main, so a
consumer that pins both cannot resolve while this one still asks for ^0.5:
utopia-php/audit dev-feat-query-lib requires utopia-php/validators ^0.5
-> found 0.5.0 but it conflicts with your root composer.json require (^0.6)
Also re-pins the database lock to the current chain head.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Keep query-lib database pins and drop the lockfile main no longer tracks.
Picks up the utopia-php/validators ^1.0 bump. database's feat-query-lib now requires ^1.0, so audit's ^0.6 made the pair unresolvable for anything depending on both. validators 1.0.0 is a stability re-tag of 0.7.0, whose only change is Assoc declaring an object instead of an array. Audit does not use that validator. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
||
| $attributes = $adapter->getAttributes(); | ||
| $attributeIds = array_map(fn(array $attr): mixed => $attr['$id'], $attributes); | ||
| $attributeIds = array_map(fn (Attribute $attr): string => $attr->key, $attributes); |
There was a problem hiding this comment.
The updated test imports Attribute and reads its key property to mirror the adapter's schema definitions. The same pattern directly inspects attribute metadata around lines 633–652 and reads Index::$key around line 811. This violates the repository directive to test observable behavior rather than source representation; use ClickHouse schema or query behavior for these checks and leave structural validation to schema checks and linters. This repository requirement must be satisfied before merging.
Context Used: Call out and harshly judge implementation-coupled ... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/Audit/Adapter/ClickHouseTest.php
Line: 521
Comment:
**Tests Mirror Schema Internals**
The updated test imports `Attribute` and reads its `key` property to mirror the adapter's schema definitions. The same pattern directly inspects attribute metadata around lines 633–652 and reads `Index::$key` around line 811. This violates the repository directive to test observable behavior rather than source representation; use ClickHouse schema or query behavior for these checks and leave structural validation to schema checks and linters. This repository requirement must be satisfied before merging.
**Context Used:** Call out and harshly judge implementation-coupled ... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Adapts the audit schema to the query-lib
AttributeandIndexvalue objects.Why this approach
Database::createCollection()takes aCollectiononly, so audit setup now passesnew Collection(id:, attributes:, indexes:)built from the typed Attribute and Index factories instead of raw arrays andDatabase::VAR_*strings.The audit schema itself is unchanged.
utopia-php/databaseis pinned todev-feat-query-lib as 7.0.0, re-pinned to that branch's head whenever it moves.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.