Skip to content

refactor(clickhouse): make retention a readonly constructor property #129

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
levivannoort wants to merge 2 commits into
base: main
Choose a base branch
from
Open

refactor(clickhouse): make retention a readonly constructor property #129

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0fc1698
refactor(clickhouse): make retention a readonly constructor property
levivannoort Jul 8, 2026
014555f
feat(clickhouse): make retention settable per table via setRetention()
levivannoort Jul 13, 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
46 changes: 23 additions & 23 deletions src/Audit/Adapter/ClickHouse.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,26 +84,26 @@ 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. */
private ?int $retention = null;

/**
* @param string $host ClickHouse host
* @param string $username ClickHouse username (default: 'default')
* @param string $password ClickHouse password (default: '')
* @param int $port ClickHouse HTTP port (default: 8123)
* @param bool $secure Whether to use HTTPS (default: false)
* @param int|null $retention Retention window in days; when set, setup() applies a TTL so rows older than the window are dropped. Null disables TTL.
* @throws Exception If validation fails
*/
public function __construct(
string $host,
string $username = 'default',
string $password = '',
int $port = self::DEFAULT_PORT,
bool $secure = false
bool $secure = false,
public ?int $retention = null,
) {
$this->validateHost($host);
$this->validatePort($port);
$this->validateRetention($retention);

$this->host = $host;
$this->port = $port;
Expand Down Expand Up @@ -354,41 +354,41 @@ public function setAsyncCleanup(bool $asyncCleanup): self
}

/**
* Get whether cleanup() runs asynchronously.
* Set the retention window in days applied by setup() as a TTL, or null to
* disable it. Call before setup(); a single adapter can setup() several
* tables (differing namespaces), so retention is settable per table rather
* than fixed at construction.
*
* @return bool
* @param int|null $retention
* @return self
* @throws Exception If retention is not a positive number of days.
*/
public function isAsyncCleanup(): bool
public function setRetention(?int $retention): self
{
return $this->asyncCleanup;
$this->validateRetention($retention);
$this->retention = $retention;
return $this;
}
Comment on lines +366 to 371

@greptile-apps greptile-apps Bot Jul 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P0 setRetention() writes to a readonly property — PHP Fatal Error

$retention is declared public readonly ?int $retention at line 102. PHP does not permit re-assignment of a readonly property after object construction; any call to setRetention() will throw \Error: Cannot modify readonly property ClickHouse::$retention. The PR description states "the fluent setter is dropped", but the method was left in. It needs to be removed (or the readonly modifier removed from the property if the setter is intentional).

Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/Audit/Adapter/ClickHouse.php
Line: 366-371

Comment:
**`setRetention()` writes to a `readonly` property — PHP Fatal Error**

`$retention` is declared `public readonly ?int $retention` at line 102. PHP does not permit re-assignment of a `readonly` property after object construction; any call to `setRetention()` will throw `\Error: Cannot modify readonly property ClickHouse::$retention`. The PR description states "the fluent setter is dropped", but the method was left in. It needs to be removed (or the `readonly` modifier removed from the property if the setter is intentional).

How can I resolve this? If you propose a fix, please make it concise.

Fix in Claude Code Fix in Codex


/**
* 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
* @param int|null $retention
* @throws Exception If retention is set but not a positive number of days.
*/
public function setRetention(?int $days): self
private function validateRetention(?int $retention): void
{
if ($days !== null && $days < 1) {
if ($retention !== null && $retention < 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.
* Get whether cleanup() runs asynchronously.
*
* @return int|null
* @return bool
*/
public function getRetention(): ?int
public function isAsyncCleanup(): bool
{
return $this->retention;
return $this->asyncCleanup;
}

/**
Expand Down
53 changes: 30 additions & 23 deletions tests/Audit/Adapter/ClickHouseTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,71 +377,78 @@ public function testSetSecure(): void
}

/**
* Test setRetention stores the value and getRetention returns it
* Test retention defaults to null and is stored when passed to the constructor
*/
public function testSetRetention(): void
public function testRetention(): void
{
$adapter = new ClickHouse(
host: 'clickhouse',
username: 'default',
password: 'clickhouse'
);
$this->assertNull($adapter->retention);

$this->assertNull($adapter->getRetention());

$result = $adapter->setRetention(30);
$this->assertInstanceOf(ClickHouse::class, $result);
$this->assertEquals(30, $adapter->getRetention());
$adapter = new ClickHouse(
host: 'clickhouse',
username: 'default',
password: 'clickhouse',
retention: 30
);
$this->assertEquals(30, $adapter->retention);
}

/**
* Test setRetention accepts null to disable retention
* Test retention can be set and cleared per table via the fluent setter
*/
public function testSetRetentionAcceptsNull(): void
public function testSetRetention(): void
{
$adapter = new ClickHouse(
host: 'clickhouse',
username: 'default',
password: 'clickhouse'
);

$adapter->setRetention(30);
$this->assertSame($adapter, $adapter->setRetention(30));
$this->assertEquals(30, $adapter->retention);

$adapter->setRetention(null);
$this->assertNull($adapter->getRetention());
$this->assertNull($adapter->retention);

$this->expectException(Exception::class);
$this->expectExceptionMessage('Retention must be a positive number of days');
$adapter->setRetention(0);
}
Comment on lines 407 to 420

@greptile-apps greptile-apps Bot Jul 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P0 Test calls the broken setter — will always error before any assertion

testSetRetention invokes $adapter->setRetention(30) on an already-constructed object whose $retention is readonly. This immediately throws \Error: Cannot modify readonly property ClickHouse::$retention, so none of the assertions below that line are ever reached. If setRetention() is removed (the PR's stated intent), this entire test block should be removed. If the setter is kept (and readonly dropped), the test is correct as-is.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: tests/Audit/Adapter/ClickHouseTest.php
Line: 407-420

Comment:
**Test calls the broken setter — will always error before any assertion**

`testSetRetention` invokes `$adapter->setRetention(30)` on an already-constructed object whose `$retention` is `readonly`. This immediately throws `\Error: Cannot modify readonly property ClickHouse::$retention`, so none of the assertions below that line are ever reached. If `setRetention()` is removed (the PR's stated intent), this entire test block should be removed. If the setter is kept (and `readonly` dropped), the test is correct as-is.

How can I resolve this? If you propose a fix, please make it concise.

Fix in Claude Code Fix in Codex


/**
* Test setRetention rejects zero days
* Test retention rejects zero days
*/
public function testSetRetentionRejectsZero(): void
public function testRetentionRejectsZero(): void
{
$this->expectException(Exception::class);
$this->expectExceptionMessage('Retention must be a positive number of days');

$adapter = new ClickHouse(
new ClickHouse(
host: 'clickhouse',
username: 'default',
password: 'clickhouse'
password: 'clickhouse',
retention: 0
);

$adapter->setRetention(0);
}

/**
* Test setRetention rejects negative days
* Test retention rejects negative days
*/
public function testSetRetentionRejectsNegative(): void
public function testRetentionRejectsNegative(): void
{
$this->expectException(Exception::class);
$this->expectExceptionMessage('Retention must be a positive number of days');

$adapter = new ClickHouse(
new ClickHouse(
host: 'clickhouse',
username: 'default',
password: 'clickhouse'
password: 'clickhouse',
retention: -1
);

$adapter->setRetention(-1);
}

/**
Expand Down
Loading