refactor(audit): migrate the ClickHouse adapter to utopia-php/query 0.6 - #91
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}
…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.
The branch was pinned at 0.3.* while query had released 0.4, 0.5 and 0.6. The adapter needed no changes: 0.4's Parser -> Classifier rename and its UnsupportedException removal are symbols this package never used, 0.5's nested join conditions are unused, and 0.6's Schema\Order left the index() call compatible. The one thing 0.6 did surface was Query::contains() being deprecated in favour of containsString() for substring matching and containsAny() for array attributes. Only tests called the deprecated factory - the adapter switches on Method::Contains, which is unchanged - and containsString() returns that same Method, so the swap is behaviour-preserving. Verified rather than assumed: the SQL snapshots hold at the same 37 assertions and the deprecation notices are gone. bin/monorepo check audit: pint, phpstan [OK] No errors, rector [OK]. Snapshot and query suites green. The ClickHouse integration suite needs a live server and runs in CI.
lohanidamodar
changed the title
Now on utopia-php/query 0.6, and ready for reviewBoth this PR and its sibling were pinned at No adapter changes were needed. The breaking changes in between are all symbols these packages don't use:
The one thing 0.6 surfaced was CI is green on 0.6 including the ClickHouse integration suites, which is what actually exercises the emitted SQL against a server. Release order this unblocks: merge and tag here, then cloud swaps its |
| @@ -1450,208 +1520,33 @@ private function parseQueries(array $queries): array | |||
| // otherwise turn `Query::contains('attr', [])` into a full-table | |||
| // match instead of an empty result. | |||
| if (\in_array($method, self::VALUE_REQUIRED_METHODS, true) && $values === []) { | |||
There was a problem hiding this comment.
$values === [] is this needed?
There was a problem hiding this comment.
It is needed — this is the one of the five I'd push back on.
VALUE_REQUIRED_METHODS covers the filters that are meaningless without a value (equal, contains, between, …). Without the guard, Query::contains('event', []) compiles to a WHERE fragment with nothing in it, which ClickHouse reads as match everything — so a filter the caller asked for silently doesn't apply and they get the full table back instead of an empty result. A wrong answer that looks like a working query, rather than an error.
isNull / isNotNull are deliberately not in that list, since taking no values is correct for them.
This isn't hypothetical: the sibling usage PR shipped exactly this shape, and review found Method::ContainsAny falling out of the Database adapter's switch with no arm and no default — the predicate contributed no SQL and the query returned every row. Both adapters there now refuse instead. The guard here is the same defence one level down, and it mirrors Validator/Query/Filter.php in utopia-php/database, so the two libraries reject the same input.
| */ | ||
| private function normalizeFilterValues(string $attribute, array $values): array | ||
| { | ||
| if ($this->getParamType($attribute) !== 'DateTime64(3)') { |
There was a problem hiding this comment.
should we use getColumnTypeMap here? whats the difference between getParamType and getColumnTypeMap?
There was a problem hiding this comment.
Good catch — they were two type tables that could disagree, and getParamType() now derives from getColumnTypeMap() in f772c93c.
The difference was:
getColumnTypeMap()builds fromgetAttributes(), so every attribute withtype === VAR_DATETIMEgetsDateTime64(3), plusid,tenant(when shared) and thelimit/offset/maxbinding slots. It is what the builder is handed viawithParamTypes().getParamType()was a hardcodedmatchrecognising only'time'and'tenant'.
Today they agree because time is the only datetime attribute. Add a second one and the same column would be bound as String by getParamType() and declared DateTime64(3) in the builder's map — the kind of mismatch that shows up as a ClickHouse parse error at the top of a range rather than in a test. One map now, getColumnTypeMap()[$attribute] ?? 'String'.
…he type maps, bind the tenant filter
Four of Chirag's five comments were actionable.
mapAttributeType() had one call site and held a one-line ternary. AGENTS.md is
explicit that a helper whose only job is to stop a short expression appearing
twice should be inlined, so it is.
getParamType() and getColumnTypeMap() were two type tables that could disagree.
getColumnTypeMap() derives from getAttributes(), typing every VAR_DATETIME
attribute as DateTime64(3); getParamType() hardcoded a match that recognised
only 'time'. A second datetime column would have been bound as String by one and
declared DateTime64(3) by the other. getParamType() now reads the same map.
getTenantFilter() returned a raw ' AND `tenant` = 42' fragment that four call
sites re-attached with $builder->whereRaw(ltrim($fragment, ' AND')). Two
problems: the tenant was interpolated rather than bound, alone among this
adapter's predicates, and ltrim() with ' AND' trims a character set - any
leading space, A, N or D - not a prefix, so it only worked because of what
followed it. Replaced with applyTenantFilter($builder), which pushes
Query::equal('tenant', [...]) through the builder like every other filter.
CHANGELOG said 'query 0.3 builder' and 'to 0.3.* (locked at 0.3.3)' after the
bump to 0.6 - the file a consumer reads to decide whether an upgrade breaks
them.
The fifth comment asked whether the empty-values guard is needed. It is, and it
is answered on the thread.
bin/monorepo check audit: pint, phpstan [OK] No errors, rector [OK]. Snapshot
and query suites 22 tests / 106 assertions. The tenant path is covered by the
live-server shared-tables test in CI.
2 participants
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.