From fd23cdfe894f0f6bf0830bbf748c8e77b9b9d4d5 Mon Sep 17 00:00:00 2001 From: Levi van Noort <73097785+levivannoort@users.noreply.github.com> Date: Thu, 2 Jul 2026 18:40:30 +0200 Subject: [PATCH 1/3] feat(clickhouse): add optional retention TTL Add setRetention(?int $days) to the ClickHouse adapter. When set, setup() applies an idempotent `ALTER TABLE ... MODIFY TTL toDateTime(time) + INTERVAL DAY` so rows past the retention window are dropped by background merges. - Retention lives next to the rest of the table's schema management in setup(), consistent with how columns/indexes/settings are handled. - Applied as a separate ALTER (not in CREATE TABLE) so it also covers already-existing tables; MODIFY TTL is a no-op when unchanged, keeping setup() re-runnable. - materialize_ttl_after_modify = 0 defers the purge to background merges instead of an immediate mutation. - Default null preserves existing behaviour (no TTL). Co-Authored-By: Claude Opus 4.8 (1M context) --- src/Audit/Adapter/ClickHouse.php | 44 ++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/src/Audit/Adapter/ClickHouse.php b/src/Audit/Adapter/ClickHouse.php index 0ff5880..b66840d 100644 --- a/src/Audit/Adapter/ClickHouse.php +++ b/src/Audit/Adapter/ClickHouse.php @@ -84,6 +84,9 @@ class ClickHouse extends SQL protected bool $asyncCleanup = false; + /** @var int|null Retention in days; when set, setup() applies a TTL on the table. Null disables TTL. */ + protected ?int $retention = null; + /** * @param string $host ClickHouse host * @param string $username ClickHouse username (default: 'default') @@ -360,6 +363,34 @@ public function isAsyncCleanup(): bool return $this->asyncCleanup; } + /** + * Set the retention window in days. When set, setup() applies a TTL so + * rows older than the window are dropped by background merges. Pass null + * to disable (the default). + * + * @param int|null $days + * @return self + * @throws Exception If $days is not positive + */ + public function setRetention(?int $days): self + { + if ($days !== null && $days < 1) { + throw new Exception('Retention must be a positive number of days'); + } + $this->retention = $days; + return $this; + } + + /** + * Get the retention window in days, or null when TTL is disabled. + * + * @return int|null + */ + public function getRetention(): ?int + { + return $this->retention; + } + /** * Override getAttributes to provide extended attributes for ClickHouse. * Includes existing attributes from parent and adds new missing ones. @@ -791,6 +822,19 @@ public function setup(): void "; $this->query($createTableSql); + + // Apply retention as a separate, idempotent ALTER. CREATE TABLE IF NOT + // EXISTS won't add a TTL to a table that already exists, and MODIFY TTL + // is a no-op when the TTL already matches, so setup() stays re-runnable. + // materialize_ttl_after_modify = 0 defers the purge to background merges + // rather than an immediate, I/O-heavy mutation. + if ($this->retention !== null) { + $this->query( + "ALTER TABLE {$escapedDatabaseAndTable} " + . "MODIFY TTL toDateTime(time) + INTERVAL {$this->retention} DAY " + . 'SETTINGS materialize_ttl_after_modify = 0' + ); + } } /** From e128bf54e0debe7edd8c703f60873b8b59ce0c1c Mon Sep 17 00:00:00 2001 From: Levi van Noort <73097785+levivannoort@users.noreply.github.com> Date: Thu, 2 Jul 2026 19:18:12 +0200 Subject: [PATCH 2/3] fix(clickhouse): remove TTL when retention is disabled; make retention private; add tests Addresses review feedback on the retention TTL: - setup() now issues ALTER TABLE ... REMOVE TTL on the null path so disabling retention actually strips a previously applied TTL, matching the documented "pass null to disable" contract. REMOVE TTL is a no-op on a table without a TTL, so setup() stays idempotent. - $retention is now private, matching the other sensitive fields and preventing subclasses from bypassing the positive-day guard. - Add unit tests for setRetention/getRetention, the null path, and the zero/negative validation guard. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/Audit/Adapter/ClickHouse.php | 10 +++- tests/Audit/Adapter/ClickHouseTest.php | 68 ++++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 1 deletion(-) diff --git a/src/Audit/Adapter/ClickHouse.php b/src/Audit/Adapter/ClickHouse.php index b66840d..fd6dafb 100644 --- a/src/Audit/Adapter/ClickHouse.php +++ b/src/Audit/Adapter/ClickHouse.php @@ -85,7 +85,7 @@ class ClickHouse extends SQL protected bool $asyncCleanup = false; /** @var int|null Retention in days; when set, setup() applies a TTL on the table. Null disables TTL. */ - protected ?int $retention = null; + private ?int $retention = null; /** * @param string $host ClickHouse host @@ -834,6 +834,14 @@ public function setup(): void . "MODIFY TTL toDateTime(time) + INTERVAL {$this->retention} DAY " . 'SETTINGS materialize_ttl_after_modify = 0' ); + } else { + // Disabling retention must actively strip any TTL a previous run + // applied; otherwise rows keep being purged despite retention being + // null. REMOVE TTL is a no-op on a table without a TTL, so this is + // safe to run unconditionally and keeps setup() idempotent. + $this->query( + "ALTER TABLE {$escapedDatabaseAndTable} REMOVE TTL" + ); } } diff --git a/tests/Audit/Adapter/ClickHouseTest.php b/tests/Audit/Adapter/ClickHouseTest.php index 8074243..c3c6a40 100644 --- a/tests/Audit/Adapter/ClickHouseTest.php +++ b/tests/Audit/Adapter/ClickHouseTest.php @@ -376,6 +376,74 @@ public function testSetSecure(): void $this->assertInstanceOf(ClickHouse::class, $result); } + /** + * Test setRetention stores the value and getRetention returns it + */ + public function testSetRetention(): void + { + $adapter = new ClickHouse( + host: 'clickhouse', + username: 'default', + password: 'clickhouse' + ); + + $this->assertNull($adapter->getRetention()); + + $result = $adapter->setRetention(30); + $this->assertInstanceOf(ClickHouse::class, $result); + $this->assertEquals(30, $adapter->getRetention()); + } + + /** + * Test setRetention accepts null to disable retention + */ + public function testSetRetentionAcceptsNull(): void + { + $adapter = new ClickHouse( + host: 'clickhouse', + username: 'default', + password: 'clickhouse' + ); + + $adapter->setRetention(30); + $adapter->setRetention(null); + $this->assertNull($adapter->getRetention()); + } + + /** + * Test setRetention rejects zero days + */ + public function testSetRetentionRejectsZero(): void + { + $this->expectException(Exception::class); + $this->expectExceptionMessage('Retention must be a positive number of days'); + + $adapter = new ClickHouse( + host: 'clickhouse', + username: 'default', + password: 'clickhouse' + ); + + $adapter->setRetention(0); + } + + /** + * Test setRetention rejects negative days + */ + public function testSetRetentionRejectsNegative(): void + { + $this->expectException(Exception::class); + $this->expectExceptionMessage('Retention must be a positive number of days'); + + $adapter = new ClickHouse( + host: 'clickhouse', + username: 'default', + password: 'clickhouse' + ); + + $adapter->setRetention(-1); + } + /** * Test shared tables configuration */ From bf941e95836c0b105f86712347a86ad1fb83eede Mon Sep 17 00:00:00 2001 From: Levi van Noort <73097785+levivannoort@users.noreply.github.com> Date: Sun, 5 Jul 2026 12:50:04 +0200 Subject: [PATCH 3/3] fix(clickhouse): tolerate REMOVE TTL on tables without a TTL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit REMOVE TTL is not a no-op — ClickHouse raises error 36 (BAD_ARGUMENTS, "Table doesn't have any table TTL expression, cannot remove") when the table has no TTL. That broke setup() on every fresh table when retention is disabled. Swallow that specific error to keep setup() idempotent. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/Audit/Adapter/ClickHouse.php | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/Audit/Adapter/ClickHouse.php b/src/Audit/Adapter/ClickHouse.php index fd6dafb..1524a5d 100644 --- a/src/Audit/Adapter/ClickHouse.php +++ b/src/Audit/Adapter/ClickHouse.php @@ -837,11 +837,18 @@ public function setup(): void } else { // Disabling retention must actively strip any TTL a previous run // applied; otherwise rows keep being purged despite retention being - // null. REMOVE TTL is a no-op on a table without a TTL, so this is - // safe to run unconditionally and keeps setup() idempotent. - $this->query( - "ALTER TABLE {$escapedDatabaseAndTable} REMOVE TTL" - ); + // null. ClickHouse errors (code 36) when REMOVE TTL runs on a table + // that has no TTL, so swallow that specific case to keep setup() + // idempotent. + try { + $this->query( + "ALTER TABLE {$escapedDatabaseAndTable} REMOVE TTL" + ); + } catch (Exception $e) { + if (!str_contains($e->getMessage(), "doesn't have any table TTL expression")) { + throw $e; + } + } } }