Extract multitenancy into a SharedTables decorator with safe withTenant scoping#11
Extract multitenancy into a SharedTables decorator with safe withTenant scoping#11loks0n wants to merge 3 commits into
Conversation
…xplicitly Namespace and shared-tables mode are deployment-fixed, yet were mutable setters on the adapter — and each setter flushed the buffer. Tenant was likewise adapter state, so writing one project's metrics required a setTenant()+flush() per message (one ClickHouse round-trip each). - ClickHouse adapter takes namespace + sharedTables as constructor args; setNamespace/setSharedTables/setTenant setters and their getters removed, along with all adapter tenant state. - collect() gains an optional per-row $tenant, stored on the buffer entry and folded into the dedup key, so one buffer holds many tenants and flushes without per-tenant round-trips. - Read/purge methods take an explicit ?string $tenant, applied as a normal query filter (parseQueries already supports the 'tenant' attribute) — no hidden adapter state. Base Adapter + Database adapter signatures updated to match; Database honours it via its existing setTenant. PHPStan max clean. Test migration to the new signatures follows in the next commit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ily purge - ClickHouse tests construct with namespace/sharedTables and use a ScopedClickHouse test helper that supplies a default tenant to writes (per-row) and reads (per-call), so the existing call sites exercise the real tenant code paths without threading tenant through each one. - Rewrote the per-row tenant-override test to use the new API directly (addBatch with $tenant on the row, find/purge with an explicit tenant). - Reflection helpers target the parent ClickHouse class so they resolve private members through the test subclass. Also fixes a purge regression surfaced by the migration: with tenant now an explicit query filter, purgeDaily treated a tenant scope as a daily-safe narrowing filter, so a path-only purge fell through to `DELETE FROM daily WHERE tenant=X` and wiped the tenant's unrelated daily rows. Tenant is now excluded from the compatibility / no-op decision and re-attached only to the final scoped delete. 246 tests green, PHPStan max + Pint clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Greptile SummaryThis PR extracts multi-tenancy out of the core
Confidence Score: 5/5Safe to merge; the core refactoring is architecturally sound and the concurrency guarantee (scoped clone, no shared mutable tenant state) is correctly implemented. The tenant-scoping logic is correct across all public read paths. The purge/daily-purge split properly separates scope-only and narrowing queries. The three issues noted are design polish rather than data correctness problems: redundant tenant filters on the daily routing path produce SQL that is logically equivalent to a single filter, the Nullable(String) type hint mismatch only matters for null bindings which never occur in the active scope path, and the shared HTTP client concern only applies to coroutine runtimes not yet in scope. ClickHouse.php deserves a second look at the interaction between sum()/getTotal() and sumDaily() in the daily routing path, where scopeQueries runs twice on the same query set. Important Files Changed
Reviews (2): Last reviewed commit: "refactor: move multitenancy into a Share..." | Re-trigger Greptile |
…ithTenant
The base ClickHouse adapter is now strictly single-tenant — no tenant column,
no tenant SQL. Multi-tenancy lives entirely in a SharedTables subclass that
controls SQL generation through a handful of protected seams on the base:
tenantColumnDefs() tenant column on every table
keyPrefix() tenant as the leading ORDER BY / projection / GROUP BY key
extraQueryableColumns() tenant accepted as a query attribute
scopeOnlyAttributes() tenant scopes but never narrows a daily purge
scopeQueries() inject the active tenant filter on reads/purges
decorateRow() stamp the per-row tenant on writes
Tenant is no longer adapter state and no longer a parameter on every read
method (reverted). Instead:
- Reads/purges scope through SharedTables::withTenant($tenant, $fn), which
hands the callback a tenant-bound *clone* — no shared mutable state, so it
is safe under concurrent coroutines, and the scope can't leak past the
closure. Usage::withTenant mirrors it for the wrapper.
- Writes keep the per-row $tenant on collect(), so one buffer still batches
many tenants in a single flush.
Tests pin a tenant via a ScopedClickHouse(extends SharedTables) helper; added
explicit withTenant override + isolation tests. 247 tests green, PHPStan max
and Pint clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Why
The ClickHouse adapter baked multi-tenancy in via mutable
setNamespace/setSharedTables/setTenantsetters — and each flushed the buffer.setTenantper message (multi-tenant workers) meant one ClickHouse round-trip per message, which in Appwrite Cloud's stats-usage worker capped throughput at ~1 message per cross-region round-trip and OOM'd the queue.This pulls multitenancy out of the core adapter entirely so the failure pattern isn't even expressible, and gives reads a tenant scope that's safe under concurrency.
Design
Base
ClickHouseis now strictly single-tenant — no tenant column, no tenant SQL. Multi-tenancy lives in aSharedTablessubclass that controls SQL generation through protected seams on the base:tenantColumnDefs()keyPrefix()ORDER BY/ projection /GROUP BYkeyextraQueryableColumns()scopeOnlyAttributes()scopeQueries()decorateRow()Tenant is no longer adapter state, nor a parameter on every read method. Instead:
SharedTables::withTenant($tenant, fn($scoped) => …), which hands the callback a tenant-bound clone — no shared mutable state, safe under concurrent coroutines, and the scope can't leak past the closure.Usage::withTenantmirrors it for the wrapper.$tenantoncollect(), so one buffer still batches many tenants in a single flush (the throughput win).Notable
path-only purge can't fall through toDELETE … WHERE tenant=Xand wipe a tenant's unrelated daily rows.Databaseadapter reverted to no tenant params (keeps its own backend tenant handling).Migration (breaking)
new ClickHouse(host, user, pass, port, secure)→new SharedTables(host, user, pass, port, secure, namespace)for multi-tenant; baseClickHouseis single-tenant.->setNamespace()/->setSharedTables()/->setTenant()removed; namespace is a constructor arg.withTenant(). Writes:collect(..., tenant: $t).Tests
247 tests green against ClickHouse + MariaDB (docker-compose); PHPStan level max and Pint clean on
srcand changed tests. Added explicitwithTenantoverride + cross-tenant isolation tests. Shared-mode tests pin a tenant via aScopedClickHouse extends SharedTableshelper, still exercising the real tenant SQL.🤖 Generated with Claude Code