Skip to content

Support string tenant IDs in multi-tenant audit logging #111

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
lohanidamodar wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Support string tenant IDs in multi-tenant audit logging #111

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
06d4292
feat: support int, string, or null types for tenant
claude Mar 16, 2026
3f2ab03
fix: use parameterized queries for tenant filter to prevent SQL injec…
claude Mar 16, 2026
6a040eb
fix: add type check before casting mixed tenant value to string
claude Mar 16, 2026
a7f3251
fix: return int for numeric tenant values in getTenant()
claude Mar 16, 2026
c7abe55
revert: keep numeric string tenants as strings in getTenant()
claude Mar 16, 2026
b71aafc
Merge remote-tracking branch 'origin/main' into claude/tenant-nullabl…
lohanidamodar May 14, 2026
881546e
fix: address greptile review on tenant scheme handling
lohanidamodar May 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
189 changes: 171 additions & 18 deletions src/Audit/Adapter/ClickHouse.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,28 @@ class ClickHouse extends SQL

protected string $namespace = '';

protected ?int $tenant = null;
protected int|string|null $tenant = null;

protected bool $sharedTables = false;

/**
* Tenant identifier scheme used by the host application. Mirrors the
* `getIdAttributeType()` contract on `utopia-php/database` adapters
* (`Database::VAR_INTEGER` or `Database::VAR_UUID7`).
*
* Determines the ClickHouse column type for the `tenant` column when
* `setSharedTables(true)` is enabled:
* - `integer` (default) → `Nullable(UInt64)`
* - anything else → `Nullable(String)`
*
* Defaults to `VAR_INTEGER` to preserve the previous schema on existing
* deployments: `reconcileTenantColumnType()` would otherwise trigger an
* irreversible `ALTER TABLE ... MODIFY COLUMN` from UInt64 to String on
* first `setup()` after upgrade. Callers using string IDs must opt in
* explicitly via {@see setIdAttributeType()}.
*/
protected string $idAttributeType = Database::VAR_INTEGER;

/**
* @param string $host ClickHouse host
* @param string $username ClickHouse username (default: 'default')
Expand Down Expand Up @@ -254,10 +272,10 @@ public function getNamespace(): string
* Set the tenant ID for multi-tenant support.
* Tenant is used to isolate audit logs by tenant.
*
* @param int|null $tenant
* @param int|string|null $tenant
* @return self
*/
public function setTenant(?int $tenant): self
public function setTenant(int|string|null $tenant): self
{
$this->tenant = $tenant;
return $this;
Expand All @@ -266,9 +284,9 @@ public function setTenant(?int $tenant): self
/**
* Get the tenant ID.
*
* @return int|null
* @return int|string|null
*/
public function getTenant(): ?int
public function getTenant(): int|string|null
{
return $this->tenant;
}
Expand Down Expand Up @@ -296,6 +314,49 @@ public function isSharedTables(): bool
return $this->sharedTables;
}

/**
* Set the tenant identifier scheme. Pass `Database::VAR_INTEGER` for legacy
* integer-based tenant IDs (column will be `Nullable(UInt64)`), or
* `Database::VAR_UUID7` / any other value for string-based IDs
* (column will be `Nullable(String)`).
*
* Mirrors `getIdAttributeType()` on `utopia-php/database` adapters so the
* audit table tenant column matches the host application's tenant scheme.
*
* Must be called before {@see setup()} for the schema to reflect the
* chosen type. After setup, {@see setup()} will reconcile any mismatched
* existing column type via `ALTER TABLE ... MODIFY COLUMN`.
*
* @param string $type
* @return self
*/
public function setIdAttributeType(string $type): self
{
$this->idAttributeType = $type;
return $this;
}

/**
* Get the tenant identifier scheme.
*
* @return string
*/
public function getIdAttributeType(): string
{
return $this->idAttributeType;
}

/**
* Resolve the ClickHouse SQL type for the tenant column based on the
* configured ID attribute type.
*/
private function getTenantColumnType(): string
{
return $this->idAttributeType === Database::VAR_INTEGER
? 'Nullable(UInt64)'
: 'Nullable(String)';
}

/**
* Override getAttributes to provide extended attributes for ClickHouse.
* Includes existing attributes from parent and adds new missing ones.
Expand Down Expand Up @@ -677,7 +738,7 @@ public function setup(): void

// Add tenant column only if tables are shared across tenants
if ($this->sharedTables) {
$columns[] = 'tenant Nullable(UInt64)'; // Supports 11-digit MySQL auto-increment IDs
Copy link
Copy Markdown
Member

@abnegate abnegate Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we should check getIdAttributeType which will be integer or uuid7, let's just use string unless it's int

$columns[] = 'tenant ' . $this->getTenantColumnType();
}
Comment on lines 739 to 742
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add an upgrade path for existing tenant columns.

setup() still uses CREATE TABLE IF NOT EXISTS, so existing shared tables keep their old Nullable(UInt64) column. On upgrade, string tenants will not work there, and Line 1283 now filters with a string comparison. Please add an ALTER TABLE ... MODIFY COLUMN path or fail fast on incompatible schemas.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/Audit/Adapter/ClickHouse.php` around lines 632 - 635, The setup() path
currently leaves existing shared tables with tenant Nullable(UInt64), which will
break the new string-based tenant logic; update setup() in ClickHouse.php to
detect existing tenant column types on shared tables and either run an ALTER
TABLE <table> MODIFY COLUMN tenant Nullable(String) for each affected table
(using the same ClickHouse client used elsewhere in the class) or throw a clear,
fast failure explaining the incompatible schema; target the code paths that
check $this->sharedTables and the tenant column handling so upgrades convert
Nullable(UInt64) -> Nullable(String) (or fail) before any queries that expect
string tenants.


// Build indexes from base adapter schema
Expand All @@ -693,6 +754,10 @@ public function setup(): void
$indexes[] = "INDEX {$indexName} ({$attributeList}) TYPE bloom_filter GRANULARITY 1";
}

if ($this->sharedTables) {
$indexes[] = 'INDEX _key_tenant tenant TYPE bloom_filter GRANULARITY 1';
}

$tableName = $this->getTableName();
$escapedDatabaseAndTable = $this->escapeIdentifier($this->database) . '.' . $this->escapeIdentifier($tableName);

Expand All @@ -709,6 +774,59 @@ public function setup(): void
";

$this->query($createTableSql);

// Reconcile the tenant column type with the configured ID attribute
// scheme. Tables created on an older version of this adapter (which
// hardcoded `Nullable(UInt64)`) need to be migrated to `Nullable(String)`
// once the host switches to a string-based tenant ID scheme.
if ($this->sharedTables) {
$this->reconcileTenantColumnType($escapedDatabaseAndTable);
}
}

/**
* Compare the actual `tenant` column type against the configured scheme
* and emit an `ALTER TABLE ... MODIFY COLUMN` if they differ. A no-op
* when the schema is already correct or when the table was just freshly
* created with the right type.
*
* @param string $escapedDatabaseAndTable Fully qualified, identifier-quoted table name
* @throws Exception
*/
private function reconcileTenantColumnType(string $escapedDatabaseAndTable): void
{
$expected = $this->getTenantColumnType();

$describeSql = "
SELECT type
FROM system.columns
WHERE database = {database:String}
AND table = {table:String}
AND name = 'tenant'
FORMAT TabSeparated
";

$tableName = $this->getTableName();
$result = $this->query($describeSql, [
'database' => $this->database,
'table' => $tableName,
]);
$actual = trim($result);

// Column missing on an existing table (e.g. sharedTables turned on
// post-create) is handled by ALTER TABLE ADD COLUMN.
if ($actual === '') {
$alterSql = "ALTER TABLE {$escapedDatabaseAndTable} ADD COLUMN IF NOT EXISTS tenant {$expected}";
$this->query($alterSql);
return;
}

if ($actual === $expected) {
return;
}

$alterSql = "ALTER TABLE {$escapedDatabaseAndTable} MODIFY COLUMN tenant {$expected}";
$this->query($alterSql);
}

/**
Expand Down Expand Up @@ -845,7 +963,7 @@ public function getById(string $id): ?Log
FORMAT JSON
";

$result = $this->query($sql, ['id' => $id]);
$result = $this->query($sql, array_merge(['id' => $id], $this->getTenantParams()));
$logs = $this->parseJsonResults($result);

return $logs[0] ?? null;
Expand Down Expand Up @@ -913,7 +1031,7 @@ public function find(array $queries = []): array
FORMAT JSON
";

$result = $this->query($sql, $params);
$result = $this->query($sql, array_merge($params, $this->getTenantParams()));
$rows = $this->parseJsonResults($result);

if ($cursorDirection === 'before') {
Expand Down Expand Up @@ -975,7 +1093,7 @@ public function count(array $queries = [], ?int $max = null): int
";
}

$result = $this->query($sql, $params);
$result = $this->query($sql, array_merge($params, $this->getTenantParams()));
$trimmed = trim($result);

return $trimmed !== '' ? (int) $trimmed : 0;
Expand Down Expand Up @@ -1296,11 +1414,17 @@ private function normalizeCursorRow(mixed $rawCursor): array
*/
private function getParamType(string $attribute): string
{
return match (true) {
$attribute === 'time' => 'DateTime64(3)',
$attribute === 'tenant' && $this->sharedTables => 'UInt64',
default => 'String',
};
if ($attribute === 'time') {
return 'DateTime64(3)';
}

if ($attribute === 'tenant' && $this->sharedTables) {
// Match the column type chosen at setup() — UInt64 only for
// integer-based tenant schemes, String otherwise.
return $this->idAttributeType === Database::VAR_INTEGER ? 'UInt64' : 'String';
}

return 'String';
}

/**
Expand Down Expand Up @@ -1600,11 +1724,17 @@ private function parseJsonResults(string $result): array
$document[$columnName] = $value ?? [];
}
} elseif ($columnName === 'tenant') {
// Parse tenant as integer or null
// Parse tenant according to the configured scheme:
// - integer scheme → cast numeric values to int
// - string scheme (uuid7 etc.) → keep verbatim so that
// numeric-looking IDs like "00123" or "42" round-trip
// without lossy coercion to int
if ($value === null || $value === '') {
$document[$columnName] = null;
} elseif (is_numeric($value)) {
} elseif ($this->idAttributeType === Database::VAR_INTEGER && is_numeric($value)) {
$document[$columnName] = (int) $value;
} elseif (is_scalar($value)) {
$document[$columnName] = (string) $value;
} else {
$document[$columnName] = null;
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Comment thread
greptile-apps[bot] marked this conversation as resolved.
Expand Down Expand Up @@ -1682,7 +1812,30 @@ private function getTenantFilter(): string
}

$escapedTenant = $this->escapeIdentifier('tenant');
return " AND {$escapedTenant} = {$this->tenant}";
$type = $this->getParamType('tenant');

return " AND {$escapedTenant} = {_tenant:{$type}}";
}

/**
* Get query parameters for tenant filtering.
*
* @return array<string, mixed>
*/
private function getTenantParams(): array
{
if (!$this->sharedTables || $this->tenant === null) {
return [];
}

// Integer scheme binds to a UInt64 placeholder so ClickHouse can
// coerce/validate the value as a number. String scheme passes the
// value verbatim.
if ($this->idAttributeType === Database::VAR_INTEGER) {
return ['_tenant' => (int) $this->tenant];
}

return ['_tenant' => (string) $this->tenant];
}

/**
Expand Down Expand Up @@ -1977,7 +2130,7 @@ public function cleanup(\DateTime $datetime): bool
WHERE time < {datetime:String}{$tenantFilter}
";

$this->query($sql, ['datetime' => $datetimeString]);
$this->query($sql, array_merge(['datetime' => $datetimeString], $this->getTenantParams()));

return true;
}
Expand Down
13 changes: 9 additions & 4 deletions src/Audit/Log.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,9 +126,9 @@ public function getData(): array
/**
* Get the tenant ID (for multi-tenant setups).
*
* @return int|null
* @return int|string|null
*/
public function getTenant(): ?int
public function getTenant(): int|string|null
{
$tenant = $this->getAttribute('tenant');

Expand All @@ -140,8 +140,13 @@ public function getTenant(): ?int
return $tenant;
}

if (is_numeric($tenant)) {
return (int) $tenant;
// Strings are returned as-is. The ClickHouse parser converts tenant
// values to int when the integer scheme is configured, so a string
// here means the host application is using a string-typed tenant
// (uuid7, slug, etc.) — coercing numeric-looking strings like
// "00123" would silently corrupt them.
if (is_string($tenant)) {
return $tenant;
}
Comment thread
greptile-apps[bot] marked this conversation as resolved.

return null;
Expand Down
Loading
Loading