feat: migrate ClickHouse adapter to utopia-php/query 0.3.x builder#120
feat: migrate ClickHouse adapter to utopia-php/query 0.3.x builder#120lohanidamodar wants to merge 25 commits into
Conversation
Pins to dev-feat/clickhouse-insert-delete-settings-mv as 0.3.2 — the branch on utopia-php/query (PR #11) that adds the three ClickHouse builder extras audit needs: - Builder\ClickHouse::insertFormat(...) for INSERT ... FORMAT JSONEachRow - Trailing SETTINGS clause on DELETE (for async cleanup) - Schema\ClickHouse::create/dropMaterializedView (not used here) TODO: flip to ^0.3.2 once PR #11 lands on utopia-php/query main and a 0.3.2 tag exists. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The 0.3.x utopia-php/query base class replaced the legacy `TYPE_*` string constants with a `Method` enum. Audit's adapter switches and tests all still compare against the string constants, so they're re-declared here mapped to the same string values (`equal`, `lessThan`, ...). Tests that compared `$query->getMethod()` (now returns a `Method` enum) against the constants are updated to compare against `getMethod()->value`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
utopia-php/query 0.3.x uses PHP 8.4 asymmetric visibility (`public protected(set)`) on schema and builder properties. PHPStan 1.12's bundled PhpParser can't tokenize that syntax — `composer check` crashes with a Lexer internal error on any audit source file that imports a query schema/builder class. PHPStan 2.x ships an updated PhpParser that handles asymmetric visibility, so bumping the dev dependency is the smallest change that restores the static-analysis gate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…sites utopia-php/query 0.3.x's `Query::getMethod()` returns a `Method` enum instead of a string. The ClickHouse adapter's parseQueries switch, and the Database adapter's count-time filter, both compare against the legacy string `TYPE_*` constants. Switch from `getMethod()` to `getMethod()->value` at the two call sites so the existing comparisons keep matching. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bumping PHPStan to 2.x to handle PHP 8.4 syntax surfaces a handful of pre-existing diagnostics in code paths the migration does not otherwise touch: - Database adapter: redundant `instanceof Utopia\Audit\Query` guards inside a method whose signature already constrains the parameter. Kept as runtime defense (real callers occasionally pass mixed arrays); annotated with @phpstan-ignore-next-line. - Log::getData(): PHPStan can't widen the ArrayObject return without a local @var hint. - AuditBase batch test: removed a duplicate `applyRequiredAttributesToBatch` call (typo; the second invocation already re-merged the same row). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the hand-built CREATE TABLE in `setup()` with a `Schema\ClickHouse` table builder. Engine, ORDER BY tuple, PARTITION BY expression, table settings, columns, and data-skipping (bloom_filter) indexes are now declared through the schema API; the resulting Statement is executed verbatim through the existing HTTP layer. Column definitions follow the audit attribute descriptors: - `id String` (primary) - `time DateTime64(3)` (NOT NULL — partition key) - `<id> [Nullable(]String[)]` for every other attribute, nullability driven by the descriptor's `required` flag - `tenant Nullable(UInt64)` when sharedTables is on CREATE DATABASE IF NOT EXISTS still goes through a raw string — the schema's `createDatabase()` helper has no IF NOT EXISTS form and we'd otherwise break the second `setup()` call. Also folds in a few PHPStan-driven cleanups in this file (collapsed `!empty($inParams)` guards into unconditional emits since the upstream VALUE_REQUIRED_METHODS guard already rejects empty value lists; dropped the redundant is_array($row) inside parseJsonResults whose row type was already array<string,mixed>). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…eBatch Build the JSONEachRow INSERT statement through `Builder\ClickHouse::insertFormat` instead of a hand-rolled string. Column list is derived from the audit schema (id, time, the remaining attributes, optional tenant) in the same insertion order as the row maps so the JSON keys line up against ClickHouse's declared columns. The JSONEachRow body itself still serializes in the adapter — that's an HTTP-payload concern that stays in the runtime layer. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Compile the cleanup DELETE through `Builder\ClickHouse::delete()` with a
trailing SETTINGS clause emitted by the builder when async cleanup is
enabled. The DELETE WHERE expression is supplied via `whereRaw()` so the
ClickHouse-typed parameter syntax (`{datetime:DateTime64(3)}`) and the
existing tenant filter stay in the runtime layer — the builder still
emits generic `?` placeholders today (dry-run gap #7).
Behaviour note: the ClickHouse builder compiles DELETE as
`ALTER TABLE ... DELETE` (mutation) instead of the lightweight
`DELETE FROM ...` the previous string emitted. The async setting tracks
the same shift: `mutations_sync = 0` for mutations replaces the
previous `lightweight_deletes_sync = 0`. End-state row visibility is
the same; the storage path is different (mutations rewrite parts,
lightweight deletes mask rows).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Route the SELECT skeleton (FROM, raw projection, WHERE conjunction,
ORDER BY) for `find()`, `count()`, and `getById()` through
`Builder\ClickHouse`. Filter expressions emitted by `parseQueries()`
still carry ClickHouse-typed parameter hints (`{name:Type}`) and the
audit-side associative params map; both ride along via `whereRaw()` /
`orderByRaw()` so the typed-binding HTTP layer keeps working.
`LIMIT` / `OFFSET` and the trailing `FORMAT JSON` / `FORMAT TabSeparated`
are appended on the compiled SQL string — the base builder emits
positional `?` placeholders for limit/offset, which would collide with
the ClickHouse `{name:UInt64}` placeholders the runtime layer binds.
Cleaning that up belongs with the gap #7 (ClickHouse param hint) work
on utopia-php/query.
Cursor pagination still builds tuple WHERE fragments through the
existing `buildCursorWhere()` helper and feeds them in via `whereRaw()`
(gap #8, deferred).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pin the SQL emitted by `Schema\ClickHouse` and `Builder\ClickHouse` through the audit configurations the adapter relies on: - `setup()` CREATE TABLE — engine, columns, indexes, ORDER BY, PARTITION BY, SETTINGS - `createBatch()` `INSERT ... FORMAT JSONEachRow` with the audit column list - `cleanup()` async DELETE with trailing SETTINGS clause - `cleanup()` sync DELETE with no SETTINGS - `find()` SELECT with whereRaw filters, cursor tuple comparison, ORDER BY tiebreaker, LIMIT and FORMAT JSON tail These do not require a live ClickHouse so they run as fast unit tests and prevent silent SQL drift from a query-lib upgrade. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Greptile SummaryThis PR migrates the ClickHouse audit adapter from the legacy
Confidence Score: 5/5Well-structured migration with passing integration tests; all SQL generation goes through the query builder and parameterised bindings are used throughout. The adapter logic is thoroughly integration-tested (ClickHouse 25.11, 99 tests), snapshot tests pin every new SQL shape, and the dependency on the dev-branch pin is explicitly called out with a clear migration path to a stable tag. src/Audit/Adapter/ClickHouse.php — verify country low-cardinality behavior matches intent; tests/Audit/Adapter/ClickHouseSqlSnapshotTest.php — add a shared-tables DDL snapshot before the dev-branch pin is promoted to stable. Important Files Changed
Reviews (9): Last reviewed commit: "chore(deps): pin utopia-php/query to sta..." | Re-trigger Greptile |
Bump utopia-php/query to the upstream branch HEAD that ships named-typed
{name:Type} placeholder support and the lightweight DELETE form, then add
two helpers to the adapter:
- getColumnTypeMap() derives a column → ClickHouse-type map from the
schema attributes (DateTime → DateTime64(3), all other columns →
String) plus id, tenant (when sharedTables is enabled) and the
limit/offset/max pseudo-columns used by the count/find SQL wrappers.
- newBuilder() returns a Builder\ClickHouse with useNamedBindings() and
withParamTypes() pre-applied, so positional `?` bindings flow through
the typed-binding rewriter at Statement-emission time.
Every existing adapter call site is rerouted from `new ClickHouseBuilder()`
to `$this->newBuilder()`. The current call sites all hand-construct their
{name:Type} placeholders inside whereRaw fragments with zero `?` bindings,
so this change is a no-op for the existing SQL shape — it just preps the
infra that the find()/count()/getById() reads will switch to next.
…ter API
Drop every hand-built WHERE / ORDER BY fragment in find(), count() and
getById() and feed Query value objects straight to Builder\ClickHouse via
filter(), sortAsc/sortDesc/sortRandom, limit() and offset(). Positional
`?` bindings are rewritten to `{paramN:Type}` placeholders by the
typed-binding registration installed in newBuilder(), so HTTP params now
flow through $statement->namedBindings instead of a hand-maintained
$paramCounter dict.
parseQueries() is reduced to a list of Query objects plus auxiliary
metadata (orderAttributes, limit, offset, cursor, select). Two
audit-specific rewrites stay in this layer:
- Contains / NotContains are remapped to Equal / NotEqual so they keep
audit's historical IN / NOT IN semantics. The base builder compiles
Contains to substring-match `position(x, ?) > 0`, which is not what
callers like Audit::getByUserAndEvents() expect.
- `time`-column DateTime values are stringified at parse time so they
appear in namedBindings as `Y-m-d H:i:s.v` literals rather than raw
objects the HTTP layer can't serialise.
Cursor pagination keeps its whereRaw escape hatch with explicit
{name:Type} placeholders — Builder\ClickHouse still has no tuple-compare
helper, so the existing buildCursorWhere() output is appended to the
builder and its params are merged into the final HTTP request alongside
$statement->namedBindings. The dead buildOrderBySql() helper is removed;
applyOrderBy() drives the builder directly.
Builder\ClickHouse now defaults to DELETE_MODE_LIGHTWEIGHT, so cleanup() emits `DELETE FROM t WHERE …` again — matching audit's pre-migration baseline. The previous mutation form (`ALTER TABLE t DELETE WHERE …`) was a workaround forced by the older builder API and changed the storage-path semantics: lightweight marks rows deleted via a mask and is the right tool for row-level cleanup, while mutations rewrite parts on disk and are heavier. The async SETTINGS knob switches from `mutations_sync = 0` to `lightweight_deletes_sync = 0` to match the new DELETE form. The public setAsyncCleanup() docblock already referenced lightweight_deletes_sync, so no docs change is needed.
…LETE
Pin the new SQL shape on the adapter's hot paths:
- cleanup() emits `DELETE FROM t WHERE …` with an optional trailing
`SETTINGS lightweight_deletes_sync=0` for the async path.
- find() / count() filter via Builder\ClickHouse::filter() and
sortAsc/sortDesc/limit, producing typed `{paramN:Type}` placeholders
and a populated `namedBindings` map on the Statement.
- The cursor whereRaw fragment with explicit `{name:Type}` placeholders
composes cleanly with the typed positional bindings — both end up in
the final HTTP params dict by the adapter merging them.
- count(max) wraps the inner Statement in `SELECT COUNT(*) FROM (… LIMIT ?) sub`
and reuses the inner builder's namedBindings unchanged.
A small newAuditBuilder() / auditTypeMap() helper mirrors the adapter's
own column → ClickHouse-type registration so the snapshot tests exercise
the same typed-binding flow callers will see in production.
The Dockerfile pinned php:8.3.3-cli-alpine3.19, but composer.json requires php >=8.4. Once utopia-php/query: ^0.3.0 was adopted, the CI Tests check failed because the library uses 8.4-only asymmetric visibility syntax (public protected(set)). Bump the test image to php:8.4.21-cli-alpine3.23 so the CI environment matches the package requirement.
ClickHouse JSON responses come from json_decode and aren't statically guaranteed to satisfy any inner shape. The pre-migration code skipped non-array rows defensively; restore that guard and loosen the @var on \$data to array<int, mixed> so PHPStan max accepts the runtime check instead of pruning it as already-narrowed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a snapshot test that locks the SQL emitted when TYPE_NOT_CONTAINS
is mapped to Method::NotEqual with multiple values. The base builder
compiles NotEqual with 2+ non-null values via compileNotIn(), producing
`attr NOT IN (?, ?)`; with named typed bindings on Builder\ClickHouse
this becomes `attr NOT IN ({param0:Type}, {param1:Type})`. Pinning the
shape guards against a query-lib drift silently degrading multi-value
notContains() to a single-value `!=` filter.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| if ($this->asyncCleanup) { | ||
| $builder->settings(['lightweight_deletes_sync' => '0']); | ||
| } |
There was a problem hiding this comment.
The
cleanup() builder uses lightweight_deletes_sync here, but the PR description states the setting was changed to mutations_sync = 0. DELETE FROM is ClickHouse's lightweight-delete syntax, and lightweight_deletes_sync is the correct setting for it — so the code and snapshot test are internally consistent. However, the PR description is factually wrong when it says this was switched to mutations_sync, which could mislead future maintainers into thinking the mutation semantics changed.
| if ($this->asyncCleanup) { | |
| $builder->settings(['lightweight_deletes_sync' => '0']); | |
| } | |
| if ($this->asyncCleanup) { | |
| $builder->settings(['mutations_sync' => '0']); | |
| } |
Brings in actor-terminology rename (PR #122), location-column drop / N-part resource parser (PR #118), and country / userAgent test refactors. Conflict resolution (src/Audit/Adapter/ClickHouse.php parseQueries): - Kept main's call to translateAttribute() right after reading $query->getAttribute() so callers passing legacy 'userId' / 'userType' / 'userInternalId' Query filters are rewritten to the renamed 'actorId' / 'actorType' / 'actorInternalId' columns before the typed-bindings layer compiles them. - The column type map registered via withParamTypes() is derived from getAttributes() and already uses the actor* names, so once the attribute is translated the map binds it as String correctly. - Public Audit API (Audit::log, getLogsByUser, countLogsByUser, getLogsByUserAndEvents, countLogsByUserAndEvents) is unchanged -- the ClickHouse adapter's getByUser / countByUser implementations already build internal Query::equal('actorId', $userId) filters. ClickHouseSqlSnapshotTest: - Updated the synthetic audit type map and DDL snapshot to reflect actorId / actorType / actorInternalId columns, idx_actorId_event, and the dropped location column. Find / count / cursor snapshots now filter on actorId.
utopia-php/query 0.3.2 is now tagged and published with all ClickHouse builder/schema features this PR depends on. Switch from the temporary dev-branch alias to the stable semver constraint. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Re-introduces the dev-branch pin (b26baf9) for the duration of utopia-php/query PR #13's lifecycle so audit can adopt the new typed Builder\ClickHouse::bulkInsert(Format, rows, columns) bulk-ingest entry point ahead of the 0.3.3 tag. Relaxes composer's minimum-stability to dev with prefer-stable=true so the alias resolves. TODO: flip composer.json back to "utopia-php/query": "^0.3.3" and minimum-stability: "stable" once 0.3.3 is tagged.
…nual JSONEachRow body assembly
Switches createBatch() to the typed Builder\ClickHouse::bulkInsert(
Format::JSONEachRow, $rows, $columns) entry point so the INSERT envelope
and the serialized row body are produced together. The HTTP helper now
takes a pre-serialized $rawBody string instead of a $jsonRows array,
deleting the inline array_map(json_encode, ...) + implode("\n", ...)
body assembly loop — that serialization now lives in Format::serialize()
on the builder side, with the JSON flags (JSON_THROW_ON_ERROR,
JSON_UNESCAPED_SLASHES, JSON_UNESCAPED_UNICODE) baked into Format::JSONEachRow.
The SQL envelope shape is unchanged (`INSERT INTO ... (cols) FORMAT
JSONEachRow`); the snapshot test is updated to drive bulkInsert and now
pins the JSONEachRow body serialization with a fixture row, so an
upstream regression in Format::serialize() would be caught by CI.
Adopt utopia-php/database ^6.0.0 from main and express main's shared-table sort key (tenant, time, id), allow_nullable_key and LowCardinality columns through the Schema builder. LowCardinality is applied to required columns only because the query builder emits Nullable(LowCardinality(String)) for nullable columns, which ClickHouse rejects.
… utopia-php/client Swap the Fetch client for the PSR-18 utopia-php/client cURL adapter with connection reuse so the TCP handshake is paid once across queries, matching the transport used by utopia-php/usage. The constructor accepts an optional injected ClientInterface for custom transports.
…inality on country Query 0.3.3 compiles lowCardinality()->nullable() to LowCardinality(Nullable(T)), so the nullable country column again matches main's DDL.
Summary
Migrate the ClickHouse audit adapter off the legacy 0.1.x
utopia-php/querycontract onto the 0.3.x
Builder\ClickHouseandSchema\ClickHouseAPI. EverySQL/DDL string the adapter emits now goes through the query library; the audit
side keeps only the HTTP transport, ClickHouse-typed parameter hint binding,
JSONEachRow body serialization, and tuple cursor compilation.
What was replaced:
setup()— CREATE TABLE / engine / ORDER BY / PARTITION BY / SETTINGS /bloom_filter indexes through
Schema\ClickHouse.createBatch()—INSERT INTO … (cols) FORMAT JSONEachRowthroughBuilder\ClickHouse::insertFormat()(rows still serialized in theHTTP-payload layer).
cleanup()—ALTER TABLE … DELETE WHERE …with optional trailingSETTINGSclause throughBuilder\ClickHouse::settings()+delete(). Async cleanup now drivesmutations_sync = 0instead oflightweight_deletes_sync = 0because the builder compiles mutations,not lightweight deletes; row visibility is identical.
find(),count(),getById()—SELECT … FROM … WHERE … ORDER BY …skeleton through
Builder\ClickHouse. Filter / cursor / tenantexpressions ride along via
whereRaw(); the existing ClickHouse-typed{name:Type}parameters and associative params map stay in the auditruntime layer.
Dependency
All required query-side work (
insertFormat(), trailingSETTINGSondelete(), materialized-view DDL,bulkInsert()+Format::serialize(),identifier quoting, and the
LowCardinality(Nullable(T))wrapping-order fix)has been merged upstream and released in the stable
utopia-php/query 0.3.3tag. The Composer requirement is the stable range
0.3.*(resolves to0.3.3); no dev-branch pins remain.The ranges PHPStan deps were bumped to
^2.0because PHP-Parser bundled byPHPStan 1.x can't tokenize the asymmetric visibility (
public protected(set))the query library now uses on schema and builder properties.
Deferred / known gaps
Builder\ClickHouseemits generic?placeholders for compiled filters, limits, and offsets. Audit bindsby associative
{name:Type}so the runtime layer can attach ClickHousetype hints (
UInt64,DateTime64(3), etc.). The adapter still relies onits own
parseQueries()to emit the typed clauses (passed throughwhereRaw()), and appendsLIMIT/OFFSETwith typed placeholders by hand.Once the query lib gains a typed-placeholder hook this can collapse into a
single builder call chain.
(a > A) OR (a = A AND b > B) …cascade in the adapter and feeds itthrough
whereRaw(). The base builder does not yet emitClickHouse-flavoured tuple comparisons.
Commit topology
chore(deps): bump utopia-php/query to 0.3.x (dev-branch pin)refactor(query): re-expose TYPE_* constants on Audit\Query for 0.3.xchore(deps): bump phpstan to ^2.0 for PHP 8.4 query librefactor(adapters): unwrap Method enum to string at getMethod() call siteschore: silence PHPStan 2.x diagnostics unrelated to the migrationrefactor(clickhouse): use Schema\ClickHouse for setup() DDLrefactor(clickhouse): use Builder INSERT FORMAT JSONEachRow for createBatchrefactor(clickhouse): use Builder DELETE + SETTINGS for cleanup()refactor(clickhouse): use Builder for find/count/getById SQL shapetest(clickhouse): add SQL snapshot tests for migrated builder pathsTest plan
composer lint+composer check(PHPStan max) all clean locally.vendor/bin/phpunit --filter "QueryTest|ClickHouseSqlSnapshotTest"green (no docker needed).
MariaDB) and run the full audit suite; cursor / count / shared-tables /
async-cleanup paths pass — 99 tests, 893 assertions.
setup()matches main'sDDL (engine, ORDER BY, PARTITION BY, SETTINGS, column types including
LowCardinality(Nullable(String))forcountry).Update: switched to bulkInsert() for createBatch()
Switched
createBatch()frominsertFormat()+manual-body tobulkInsert(Format::JSONEachRow, ...)onceutopia-php/query#13landed.The HTTP helper now takes a pre-serialized
$rawBodystring instead of a$jsonRowsarray; the inlinearray_map(json_encode, ...) + implode("\n", ...)JSONEachRow body assembly has been deleted — thatserialization lives in
Format::serialize()on the builder side with theJSON_THROW_ON_ERROR | JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODEflags baked into the
Format::JSONEachRowcase.The snapshot test
testInsertFormatJsonEachRowSnapshotwas renamed totestBulkInsertJsonEachRowSnapshotand now pins both the envelopequeryAND the JSONEachRowbodyagainst a fixture row so an upstreamregression in
Format::serialize()would be caught by CI.Update: refreshed against main (database ^6.0.0) + PSR-18 transport
Synced the branch with
main(merge commit) and layered two changes on top:Main sync. Adopted
utopia-php/database ^6.0.0and re-expressed main'sraw-DDL changes through
Schema\ClickHouse:orderBy(['tenant', 'time', 'id'])plusallow_nullable_key = 1inSETTINGS when
sharedTablesis enabled (single-tenant DDL unchanged).LowCardinalityis applied through the schema builder'slowCardinality()for all four low-cardinality columns (event,actorType,resourceType,country), matching main's DDL exactly —including
LowCardinality(Nullable(String))for the nullablecountry.Transport. Replaced
utopia-php/fetchwith the PSR-18utopia-php/clientcURL adapter usingwithConnectionReuse(), mirroringthe utopia-php/usage adapter: the TCP/TLS handshake is paid once and reused
across queries. Auth/database headers are attached per request, parameterized
queries go as multipart form data,
bulkInsertbodies asapplication/x-ndjson, and the constructor accepts an optional injectedPsr\Http\Client\ClientInterface. Composer:utopia-php/fetch ^1.1→psr/http-client ^1.0+utopia-php/client ^0.1|^0.2.Update: stable utopia-php/query 0.3.* pin — no remaining blockers
utopia-php/query 0.3.3is tagged and includes everything this branchneeded (quoting +
bulkInsert()from query#13, and thelowCardinality()->nullable()wrapping-order fix from query#14 so itcompiles to
LowCardinality(Nullable(T))). The Composer requirement nowreads
"utopia-php/query": "0.3.*"and the lock resolves to0.3.3.The interim workaround that skipped
lowCardinality()on nullable columnshas been removed, so
countryisLowCardinality(Nullable(String))again —byte-identical DDL to main.
Verified:
composer lint,composer check(PHPStan max) clean; full suitevia docker compose (ClickHouse 25.11 + MariaDB) green — 99 tests,
893 assertions — and
system.columnsinspected to confirm thecountrycolumn type.