feat(clickhouse): rename actor columns in ClickHouse adapter only#122
Conversation
Greptile SummaryThis PR scopes the
Confidence Score: 4/5Safe to merge after fixing the Query::select() translation gap; all other read/write paths correctly translate legacy userId keys. The write path and most query types correctly translate legacy userId/userType/userInternalId references. The one gap is Query::select(): column names are passed as $values rather than $attribute, so translateAttribute is never called on them — a caller passing Query::select(['userId']) hits validateAttributeName before translation and receives an exception. src/Audit/Adapter/ClickHouse.php — specifically the TYPE_SELECT branch in parseQueries around line 1360. Important Files Changed
|
…/actorInternalId) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
b724a7a to
39f47b5
Compare
| ## 2.4.0 | ||
|
|
||
| ### ClickHouse adapter — actor terminology | ||
|
|
||
| The ClickHouse adapter now stores its principal columns under "actor" terminology: | ||
| `actorId`, `actorType`, `actorInternalId`. The shared SQL base, the Database adapter, | ||
| and the public `Audit` API are unchanged — Database-backed audit logs continue to use | ||
| `userId`. | ||
|
|
||
| This is a non-breaking change for callers of the public API. `Audit::log($userId, ...)`, | ||
| `Audit::getLogsByUser(...)`, `Audit::countLogsByUser(...)`, and the equivalent | ||
| `*ByUserAndEvents` methods all keep their original signatures. The ClickHouse adapter | ||
| translates the legacy `userId` array key and `Query::equal('userId', ...)` filter | ||
| internally to the renamed `actorId` column. |
There was a problem hiding this comment.
Version number contradicts PR title and description
The CHANGELOG header says 2.4.0 and explicitly states "This is a non-breaking change for callers of the public API," but the PR title uses the feat! conventional-commit prefix, says "BC break," and states the next tag should be 3.0.0. Additionally, the PR description's renames table lists Adapter::getByActor(), Audit::getLogsByActor(), etc. — but none of those method renames appear in the code; Adapter.php and Audit.php still expose getByUser, countByUser, log(?string $userId, ...). A consumer reading the PR description will look for getByActor() and find it absent. Before merging, the CHANGELOG version and the PR description must agree: either the ClickHouse-only schema rename is a non-breaking 2.x release (matching the code), or the public PHP API methods still need to be renamed to make this a true 3.0.0 BC break.
| ClickHouse audit tables will be recreated by `setup()` with the new column names. | ||
| Existing ClickHouse audit data is not preserved automatically — this is acceptable | ||
| because the activity-events surface backed by this schema is not yet in public use. | ||
| If preservation is needed, run `ALTER TABLE ... RENAME COLUMN` for each renamed | ||
| column before redeploying. |
There was a problem hiding this comment.
"Recreated by setup()" is factually incorrect
setup() issues CREATE TABLE IF NOT EXISTS, so an existing table is never touched — it is not dropped and recreated. For any operator upgrading the library against a live ClickHouse instance, the table retains the old userId/userType/userInternalId columns, and every query the new code emits (which references actorId) will fail immediately. The phrase "will be recreated by setup()" may mislead operators into believing no manual action is required before redeploying. The sentence should clarify that setup() only creates the table for fresh installations, and that existing installations require the ALTER TABLE ... RENAME COLUMN steps listed below before redeploying.
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.
Summary
This PR narrows the scope to a ClickHouse-only column rename. The original draft renamed columns and the public API across both adapters — that broke Cloud (which still uses the Database adapter for per-project audit logs) and every library caller. Per product decision, the Database adapter and the public Audit API stay unchanged for this cycle; the Database backend will be deprecated separately.
What changed
userId→actorId,userType→actorType,userInternalId→actorInternalId(columns + matching indexes).userIdarray key andQuery::equal('userId', ...)filter and translates them to the new column internally.Loginstances expose bothactorId(canonical) anduserId(legacy mirror) attribute keys so existing callers keep working.Log::getActorId(),Log::getActorType(),Log::getActorInternalId()getters added for ClickHouse-aware callers.CHANGELOG.mdwritten for2.4.0(minor bump from2.3.2).What did NOT change
AuditAPI:Audit::log($userId, ...),Audit::getLogsByUser(...),Audit::countLogsByUser(...),Audit::getLogsByUserAndEvents(...),Audit::countLogsByUserAndEvents(...)keep their original names and signatures.Adapter.phpabstract contract:getByUser/countByUser/getByUserAndEvents/countByUserAndEventsunchanged.Adapter/SQL.phpshared base:userIdattribute andidx_userId_eventindex unchanged.Adapter/Database.php: completely unchanged — Cloud's per-project audit tables are unaffected.Log::getUserId()still present and behaves as before for Database-backed logs.Migration
setup()will create new tables withactorId/actorType/actorInternalIdcolumns. Existing ClickHouse data is not preserved automatically; this is acceptable because the activity-events surface backed by this schema is not yet in public use.Log::getActorId()/getActorType()/getActorInternalId()getters when reading ClickHouse-backed logs.Version
2.4.0— minor bump. No BC break.Test plan
vendor/bin/pint --testpasses on PHP 8.4vendor/bin/phpstan analyse --level max src testspassesvendor/bin/phpunitpasses against MariaDB 10.11 + ClickHouse 25.11 (88 tests, 841 assertions)