refactor(audit): migrate the ClickHouse adapter to utopia-php/query 0.3 - #91
refactor(audit): migrate the ClickHouse adapter to utopia-php/query 0.3#91lohanidamodar wants to merge 6 commits into
Conversation
Moves the audit package from utopia-php/query 0.1.* to the 0.3 line (locked at 0.3.3) and refreshes packages/audit/composer.lock.
setup() emits its DDL through Utopia\Query\Schema\ClickHouse instead of
hand-assembled SQL: column types, LowCardinality/Nullable wrapping,
bloom-filter indexes, engine, ORDER BY, PARTITION BY and SETTINGS all come
from the schema builder. The retention MODIFY TTL / REMOVE TTL statements
are unchanged.
find(), count(), getById(), createBatch() and cleanup() build their SQL
through Utopia\Query\Builder\ClickHouse. Positional bindings become typed
{paramN:Type} placeholders derived from getAttributes(). createBatch() uses
bulkInsert(Format::JSONEachRow, ...) for the INSERT envelope and body, and
cleanup() uses a lightweight DELETE FROM.
Query::getMethod() now returns the Utopia\Query\Method enum upstream, so
Database::count() compares against enum cases; Utopia\Audit\Query keeps
exposing the legacy TYPE_* string constants.
Filter semantics are unchanged: contains/notContains stay substring matches,
now compiled to position(col, ?) > 0 / = 0 instead of LIKE '%needle%', which
also drops the wildcard escaping.
Adds tests/Audit/Adapter/ClickHouseSqlSnapshotTest.php, a server-free SQL
snapshot suite pinning the emitted DDL/INSERT/DELETE/SELECT shapes; it runs
in the unit testsuite and is excluded from e2e.
Greptile SummaryThis PR migrates the audit package’s ClickHouse adapter to utopia-php/query 0.3.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (3): Last reviewed commit: "Merge origin/main into feat/audit-utopia..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| $schema = new ClickHouseSchema(); | ||
| $table = $schema->table('default.audits'); |
There was a problem hiding this comment.
These snapshots directly reconstruct the schema and query-builder calls instead of invoking the adapter methods they claim to cover. An adapter-level omission, incorrect argument, or binding-merge regression therefore leaves the suite green while production emits different SQL or parameters.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/audit/tests/Audit/Adapter/ClickHouseSqlSnapshotTest.php
Line: 53-54
Comment:
**Snapshots bypass adapter integration**
These snapshots directly reconstruct the schema and query-builder calls instead of invoking the adapter methods they claim to cover. An adapter-level omission, incorrect argument, or binding-merge regression therefore leaves the suite green while production emits different SQL or parameters.
How can I resolve this? If you propose a fix, please make it concise.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!
There was a problem hiding this comment.
Accurate about what these snapshots do, so I've tightened the file rather than argue with it — 9305cbdd.
They rebuild the schema and builder calls on purpose: their job is to pin what Schema\ClickHouse and Builder\ClickHouse emit, so a query-library upgrade can't silently change the SQL. That's the regression this migration is most exposed to.
The adapter coverage you're describing isn't missing, it's just in a different file: ClickHouseTest constructs the real ClickHouse adapter and drives setup(), log(), find(), count() and cleanup() against a live ClickHouse in CI — 76 tests across it and the AuditBase trait. A missing column or wrong argument in setup() fails there, not here.
What was genuinely wrong was the class docblock, which described these as pinning "the SQL emitted by the migrated ClickHouse adapter paths" — claiming coverage they don't provide. It now states the split explicitly, including that an adapter-level mistake will not turn these red, so nobody reads a green snapshot run as adapter coverage.
There was a problem hiding this comment.
That's the right fix. The original concern was valid only insofar as the docblock overstated what the tests cover — and you've addressed exactly that. The architectural split (snapshots pin query-library output, ClickHouseTest covers the adapter end-to-end) is sound, and the updated docblock makes the boundary explicit so no future reader draws the wrong conclusion from a green run.
Comment resolved.
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
…ry-0.3.x # Conflicts: # packages/audit/composer.lock
Conflict was packages/audit/composer.json: main bumped utopia-php/validators to ^0.5 while this branch holds utopia-php/query at 0.3.*. Kept both. The lock needed more than the usual one-line regeneration. Main's own lock is stale against its composer.json - it declares validators ^0.5 but locks 0.3.1, which is unsatisfiable because utopia-php/database 7.0.0 requires validators 0.3.*. Resolving that pulls database to 7.2.2 and 15 packages move against main's lock. Only utopia-php/query 0.1.1 -> 0.3.3 is this branch's doing; the rest is main's declared requirement catching up with its lock. bin/monorepo validate: all packages valid. bin/monorepo check audit (what CI runs): pint, phpstan [OK] No errors, rector [OK].
Review flagged that these snapshots reconstruct the schema and builder calls instead of invoking the adapter, so an adapter-level mistake would not turn them red. That is accurate, and the class docblock claimed more than the tests deliver by describing them as pinning 'the SQL emitted by the adapter paths'. The coverage itself is not missing: ClickHouseTest drives the real adapter - setup, log, find, count, cleanup - against a live ClickHouse in CI, 76 tests across it and the AuditBase trait. The docblock now states the split explicitly so the next reader does not mistake a green snapshot run for adapter coverage.
1 participant
Supersedes utopia-php/audit#120.
utopia-php/auditis now a read-only subtree-split mirror of this monorepo, so the work has to land here. Perdocs/distribution.md: "The distribution repositories become read-only mirrors: archive their open PRs, enable branch protection, and point contributors to the monorepo."The mirror's
origin/mainwas byte-identical topackages/audit/, so this is a direct transplant of that PR's net diff intopackages/audit/, reconciled with monorepo layout and tooling.What this does
Migrates the ClickHouse adapter from hand-assembled SQL to the
utopia-php/query0.3 schema/builder API.setup()emits its DDL throughUtopia\Query\Schema\ClickHouse: column types,LowCardinality(...)/Nullable(...)wrapping, bloom-filter indexes, engine,ORDER BY,PARTITION BYandSETTINGSall come from the schema builder. The retentionMODIFY TTL/REMOVE TTLstatements are unchanged.find(),count(),getById(),createBatch()andcleanup()build their SQL throughUtopia\Query\Builder\ClickHouse. Positional bindings become typed{paramN:Type}ClickHouse placeholders, derived from a column-to-type map built fromgetAttributes().createBatch()usesbulkInsert(Format::JSONEachRow, ...)for theINSERT ... FORMAT JSONEachRowenvelope and body instead of assembling the payload by hand.cleanup()uses a lightweightDELETE FROM.Query::getMethod()returns theUtopia\Query\Methodenum upstream in 0.3, soDatabase::count()compares against enum cases.Utopia\Audit\Querykeeps exposing the legacyTYPE_*string constants, which map to the same values.tests/Audit/Adapter/ClickHouseSqlSnapshotTest.php— a server-free SQL snapshot suite that pins the emitted DDL/INSERT/DELETE/SELECT shapes so a future query-lib bump cannot quietly change adapter SQL. It joins theunittestsuite and is excluded frome2e.Filter semantics are unchanged:
contains/notContainsremain substring matches, now compiled toposition(col, ?) > 0/= 0rather thanLIKE '%needle%', which also removes the need for wildcard escaping.Dependency bump
packages/audit/composer.json:utopia-php/query0.1.*->0.3.*, locked at the tagged0.3.3. No dev-branch pins.packages/audit/composer.lockregenerated; the result is byte-identical to the lock the mirror PR carried.packages/auditis the only package in the monorepo that requiresutopia-php/query, so the bump cannot conflict with a sibling.Reconciliations with monorepo layout
.gitignorechange (adding/.phpunit.cache/) — the monorepo root.gitignorealready ignores it.bin/monorepo check audit --fix):ChangeOrIfContinueToMultiContinueRectorinDatabase::count(),NewMethodCallWithoutParenthesesRector, anddeclare(strict_types=1)/final/assertSameon the new snapshot test.pint.json,phpstan.neonor CI workflow copies from the standalone repo were reintroduced; rootpint.json/phpstan.neonstay the authority andpackages/audit/phpstan.neonkeeps its../../phpstan.neoninclude.packages/audit/.github/workflows/mirror.ymlis untouched.docker-compose.ymlunchanged;phpunit.xmlonly gains the snapshot-test entries in the existingunit/e2etestsuites.Test plan
Run on PHP 8.5.8 with the monorepo toolchain.
bin/monorepo check audit— pint passed, PHPStan[OK] No errors, Rector[OK] Rector is done!,all checks passed.bin/monorepo validate—all packages valid.packages/auditunit suite (composer test, i.e.phpunit --testsuite unit) —Tests: 22, Assertions: 106, Deprecations: 3, no failures. The three deprecations areUtopia\Query\Query::contains()in 0.3 pointing atcontainsString()/containsAny(); behaviour is unchanged and switching call sites is out of scope here.The e2e tier could not run locally — no Docker daemon was available, so
composer test:e2eand its compose services (ClickHouse, MariaDB) could not be started.Utopia\Tests\Audit\Adapter\ClickHouseTestandUtopia\Tests\Audit\Adapter\DatabaseTestwere therefore unverified on my machine.CI has since covered that gap: the
test (audit)job brought upclickhouse/clickhouse-server:25.11-alpineandmariadb:10.11and ran the e2e suite green —Tests: 91, Assertions: 979, Deprecations: 8, no failures.Note on the mirror
utopia-php/audit'sfeat/utopia-query-0.3.xbranch is deliberately left in place atc1aefab—appwrite-labs/cloud#3965currently pinsutopia-php/audit: dev-feat/utopia-query-0.3.xand its lock references that commit. Only the PR is closed.