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 5 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
5 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
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
40 changes: 28 additions & 12 deletions src/Audit/Adapter/ClickHouse.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ class ClickHouse extends SQL

protected string $namespace = '';

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

protected bool $sharedTables = false;

Expand Down Expand Up @@ -208,10 +208,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 @@ -220,9 +220,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 @@ -631,7 +631,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 Nullable(String)';
}
Comment on lines 632 to 635
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 Down Expand Up @@ -799,7 +799,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 @@ -850,7 +850,7 @@ public function find(array $queries = []): array
FORMAT JSON
";

$result = $this->query($sql, $parsed['params']);
$result = $this->query($sql, array_merge($parsed['params'], $this->getTenantParams()));
return $this->parseJsonResults($result);
}

Expand Down Expand Up @@ -890,7 +890,7 @@ public function count(array $queries = []): int
FORMAT TabSeparated
";

$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 @@ -1197,11 +1197,13 @@ private function parseJsonResults(string $result): array
$document[$columnName] = $value ?? [];
}
} elseif ($columnName === 'tenant') {
// Parse tenant as integer or null
// Parse tenant as int, string, or null
if ($value === null || $value === '') {
$document[$columnName] = null;
} elseif (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.
Expand Down Expand Up @@ -1279,7 +1281,21 @@ private function getTenantFilter(): string
}

$escapedTenant = $this->escapeIdentifier('tenant');
return " AND {$escapedTenant} = {$this->tenant}";
return " AND {$escapedTenant} = {_tenant:String}";
}

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

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

/**
Expand Down Expand Up @@ -1570,7 +1586,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
8 changes: 4 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,8 @@ public function getTenant(): ?int
return $tenant;
}

if (is_numeric($tenant)) {
return (int) $tenant;
if (is_string($tenant)) {
return $tenant;
}

return null;
Expand Down
6 changes: 5 additions & 1 deletion tests/Audit/Adapter/ClickHouseTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,11 +299,15 @@ public function testSharedTablesConfiguration(): void
$this->assertInstanceOf(ClickHouse::class, $result);
$this->assertTrue($adapter->isSharedTables());

// Test setting tenant
// Test setting tenant to int
$result2 = $adapter->setTenant(12345);
$this->assertInstanceOf(ClickHouse::class, $result2);
$this->assertEquals(12345, $adapter->getTenant());

// Test setting tenant to string
$adapter->setTenant('tenant_abc');
$this->assertSame('tenant_abc', $adapter->getTenant());

// Test setting tenant to null
$adapter->setTenant(null);
$this->assertNull($adapter->getTenant());
Expand Down
Loading