Feat: adapter and clickhouse adapter #94
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a pluggable adapter architecture for audit storage: new abstract Adapter and SQL base classes, concrete Database and ClickHouse adapters, and a Log value object. Audit now accepts an Adapter and delegates all persistence operations. Adds reusable PHPUnit trait AuditBase and adapter tests for Database and ClickHouse. Expands README with usage and adapter examples. Updates composer dependencies, adds ClickHouse and healthchecks to docker-compose, adjusts CI tests workflow, removes a CodeQL workflow, and bumps the Dockerfile composer image. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
README.md (1)
136-137: Consider using a proper heading for consistency.The static analysis tool flagged that "Batch Logging" uses emphasis (bold) instead of a heading. For consistency with other sections, consider using a proper markdown heading.
-**Batch Logging** +#### Batch Loggingsrc/Audit/Adapter/SQL.php (1)
187-204: ClickHouse-specific column types in generic SQL base class.The
getColumnDefinition()method uses ClickHouse-specific SQL syntax (String,DateTime64(3),Nullable()) but this is an abstract base class namedSQLthat's meant to be generic. TheDatabaseadapter (which uses Utopia Database, not raw SQL) doesn't use these column definitions at all.Consider either:
- Rename this class to
ClickHouseBaseor move these methods to theClickHouseadapter- Make
getColumnDefinition()abstract so each adapter provides its own implementationsrc/Audit/Adapter/ClickHouse.php (1)
536-551: Unused$queriesparameter in count methods.The
$queriesparameter is accepted but never used incountByUser(),countByResource(),countByUserAndEvents(), andcountByResourceAndEvents(). This creates behavioral inconsistency with theDatabaseadapter which does apply additional query filters.Either implement query filtering or document that ClickHouse adapter ignores additional queries in count methods.
src/Audit/Adapter/Database.php (1)
298-316: Performance concern:cleanup()deletes documents one at a time.The cleanup method finds all documents older than the datetime and deletes them individually in a loop. For large audit tables, this could be very slow and resource-intensive.
Consider using batch deletion if the underlying database supports it, or at minimum add a
LIMITto the find query to process in bounded batches:public function cleanup(string $datetime): bool { $this->db->getAuthorization()->skip(function () use ($datetime) { do { $documents = $this->db->find( collection: $this->getCollectionName(), queries: [ Query::lessThan('time', $datetime), + Query::limit(1000), ] ); foreach ($documents as $document) { $this->db->deleteDocument($this->getCollectionName(), $document->getId()); } } while (! empty($documents)); }); return true; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
README.md(4 hunks)composer.json(2 hunks)example.php(1 hunks)src/Audit/Adapter.php(1 hunks)src/Audit/Adapter/ClickHouse.php(1 hunks)src/Audit/Adapter/Database.php(1 hunks)src/Audit/Adapter/SQL.php(1 hunks)src/Audit/Audit.php(1 hunks)tests/Audit/AuditTest.php(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
example.php (3)
src/Audit/Audit.php (5)
Audit(15-259)withDatabase(25-28)setup(67-70)log(86-97)logBatch(107-110)src/Audit/Adapter.php (4)
Adapter(14-167)create(45-45)setup(27-27)getName(19-19)src/Audit/Adapter/Database.php (4)
Database(19-317)create(69-84)setup(42-60)getName(31-34)
src/Audit/Adapter.php (2)
src/Audit/Audit.php (4)
Audit(15-259)setup(67-70)log(86-97)cleanup(255-258)src/Audit/Adapter/Database.php (14)
Database(19-317)getName(31-34)setup(42-60)create(69-84)createBatch(93-113)getByUser(123-134)countByUser(144-155)getByResource(165-176)countByResource(186-197)getByUserAndEvents(208-220)countByUserAndEvents(231-243)getByResourceAndEvents(254-266)countByResourceAndEvents(277-289)cleanup(298-316)
src/Audit/Adapter/SQL.php (3)
src/Audit/Audit.php (1)
Audit(15-259)src/Audit/Adapter.php (1)
Adapter(14-167)src/Audit/Adapter/Database.php (1)
Database(19-317)
tests/Audit/AuditTest.php (1)
src/Audit/Audit.php (4)
Audit(15-259)withDatabase(25-28)log(86-97)logBatch(107-110)
src/Audit/Adapter/ClickHouse.php (3)
src/Audit/Adapter.php (14)
Adapter(14-167)getName(19-19)setup(27-27)create(45-45)createBatch(64-64)getByUser(75-75)countByUser(86-86)getByResource(97-97)countByResource(108-108)getByUserAndEvents(120-120)countByUserAndEvents(132-132)getByResourceAndEvents(144-144)countByResourceAndEvents(156-156)cleanup(166-166)src/Audit/Adapter/Database.php (14)
Database(19-317)getName(31-34)setup(42-60)create(69-84)createBatch(93-113)getByUser(123-134)countByUser(144-155)getByResource(165-176)countByResource(186-197)getByUserAndEvents(208-220)countByUserAndEvents(231-243)getByResourceAndEvents(254-266)countByResourceAndEvents(277-289)cleanup(298-316)src/Audit/Adapter/SQL.php (3)
SQL(15-220)getAllColumnDefinitions(211-219)getIndexes(128-152)
src/Audit/Audit.php (2)
src/Audit/Adapter.php (13)
Adapter(14-167)setup(27-27)create(45-45)createBatch(64-64)getByUser(75-75)countByUser(86-86)getByResource(97-97)countByResource(108-108)getByUserAndEvents(120-120)countByUserAndEvents(132-132)getByResourceAndEvents(144-144)countByResourceAndEvents(156-156)cleanup(166-166)src/Audit/Adapter/Database.php (14)
Database(19-317)__construct(23-26)setup(42-60)create(69-84)createBatch(93-113)getByUser(123-134)countByUser(144-155)getByResource(165-176)countByResource(186-197)getByUserAndEvents(208-220)countByUserAndEvents(231-243)getByResourceAndEvents(254-266)countByResourceAndEvents(277-289)cleanup(298-316)
🪛 GitHub Actions: CodeQL
src/Audit/Adapter/SQL.php
[error] 215-215: Parameter #1 $id of method Utopia\Audit\Adapter\SQL::getColumnDefinition() expects string, mixed given.
src/Audit/Adapter/Database.php
[error] 69-277: PHPStan detected multiple type issues in Database adapter: iterable value-type specifications missing for several parameters across create, createBatch, getByUser, countByUser, and related methods.
src/Audit/Adapter/ClickHouse.php
[error] 162-696: PHPStan detected multiple type/return issues in ClickHouse adapter: several no-value-type in iterable signatures, improper throws type in @throws, and various cast/return mismatches.
🪛 markdownlint-cli2 (0.18.1)
README.md
136-136: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🪛 PHPMD (2.15.0)
src/Audit/Adapter/ClickHouse.php
536-536: Avoid unused parameters such as '$queries'. (undefined)
(UnusedFormalParameter)
599-599: Avoid unused parameters such as '$queries'. (undefined)
(UnusedFormalParameter)
663-663: Avoid unused parameters such as '$queries'. (undefined)
(UnusedFormalParameter)
728-728: Avoid unused parameters such as '$queries'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🔇 Additional comments (14)
composer.json (1)
29-33: Dependencies look appropriate for the adapter architecture.The addition of
utopia-php/fetchsupports the ClickHouse adapter's HTTP interface requirements. Theutopia-php/databaseconstraint4.*allows any minor/patch within major version 4.README.md (1)
245-274: Documentation provides a good starting point for custom adapters.The scaffold demonstrates the pattern well. Users can reference the abstract
Adapterclass for the complete list of required methods (getByUser,countByUser,getByResource, etc.).tests/Audit/AuditTest.php (3)
32-32: Test correctly uses the new factory method.The change from direct constructor to
Audit::withDatabase($database)aligns with the new adapter-based API design.
54-57: Assertions properly validate the new return type.The switch from boolean assertions to
assertInstanceOf('Utopia\\Database\\Document', ...)correctly validates thatlog()now returns aDocumentinstance.
214-216: Batch logging test correctly validates new return type.The assertions verify that
logBatch()returns an array of 4 documents, matching the newarray<Document>return type contract.example.php (1)
1-143: Well-structured example file demonstrating the adapter-based API.The example comprehensively covers:
- Database adapter setup via
withDatabase()- Single and batch logging
- Various query methods
- Custom adapter instantiation
The error handling and output formatting make it easy to follow.
src/Audit/Adapter.php (4)
14-19: Clean abstract class definition with clear naming contract.The
getName()method provides a simple way for implementations to identify themselves, useful for logging and debugging.
29-64: Note the intentional timestamp handling difference betweencreate()andcreateBatch().The PHPDoc array shapes show that
create()doesn't require a timestamp (adapter generates it viaDateTime::now()), whilecreateBatch()requires an explicittimestampfield. This design allows batch operations to preserve original event times during bulk imports or replays.This is a reasonable design choice documented in the types.
66-156: Comprehensive query interface with consistent patterns.The get/count method pairs for user, resource, and combined user+events / resource+events queries provide a complete API for retrieving audit logs. The
$queriesparameter allows for additional filtering flexibility.
158-167: Cleanup method properly documented.The ISO 8601 datetime string expectation is clearly documented. Implementations should delete logs with timestamps older than the provided datetime.
src/Audit/Adapter/ClickHouse.php (1)
260-316:create()method looks well-structured.The single-record insert properly handles nullable fields, tenant isolation, and returns a correctly populated Document. The parameterized approach using the
query()method is cleaner than the batch approach.src/Audit/Adapter/Database.php (1)
69-113: LGTM - create and createBatch implementations.Both methods correctly:
- Skip authorization for internal audit operations
- Handle nullable fields with defaults
- Use appropriate time sources (
DateTime::now()for single, caller-providedtimestampfor batch)src/Audit/Audit.php (2)
25-49: Well-designed factory pattern and adapter delegation.The factory methods
withDatabase()andwithAdapter()provide clean instantiation ergonomics while maintaining flexibility for custom adapters. The private constructor enforces use of factory methods.
86-110: Breaking API change:log()andlogBatch()return types.The return types changed from
booltoDocumentandarray<Document>respectively. This is a breaking change for existing consumers who may be checking the boolean return value.Ensure this is documented in the changelog/release notes and consider a major version bump per semver.
There was a problem hiding this comment.
Pull request overview
This pull request introduces an adapter pattern to the Utopia Audit library, enabling support for multiple storage backends. The main change refactors the monolithic Audit class to use pluggable adapters, with two concrete implementations: a Database adapter (wrapping the existing utopia-php/database functionality) and a new ClickHouse adapter for high-performance analytical workloads.
Key Changes:
- Introduced an abstract
Adapterbase class defining the interface for audit storage backends - Refactored
Auditclass to delegate operations to adapters via composition - Created
Databaseadapter that preserves the original database-backed functionality - Added
ClickHouseadapter for large-scale audit log storage and analytics - Changed return types from
booltoDocument/array<Document>forlog()andlogBatch()methods (breaking changes)
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Audit/Audit.php | Refactored to use adapter pattern; added static factory methods; changed method return types from bool to Document/array |
| src/Audit/Adapter.php | New abstract base class defining the adapter interface with methods for CRUD operations and querying |
| src/Audit/Adapter/SQL.php | New abstract base class for SQL-based adapters providing shared schema definitions and helper methods |
| src/Audit/Adapter/Database.php | New adapter wrapping utopia-php/database functionality; preserves original behavior with new adapter interface |
| src/Audit/Adapter/ClickHouse.php | New adapter for ClickHouse with HTTP-based communication; supports multi-tenancy and namespace isolation |
| tests/Audit/AuditTest.php | Updated to use new factory method and changed assertions to match new return types |
| example.php | New example file demonstrating usage of the Database adapter |
| composer.json | Added utopia-php/fetch dependency; formatted arrays for better readability |
| composer.lock | Updated dependencies to latest versions |
| README.md | Expanded documentation with adapter pattern explanation, ClickHouse adapter usage, and custom adapter guide |
Comments suppressed due to low confidence (1)
src/Audit/Adapter/ClickHouse.php:164
- The connection uses HTTP instead of HTTPS on line 164. This means credentials (username and password) and all audit data are transmitted in plaintext over the network, exposing them to potential interception. Consider:
- Supporting HTTPS connections with a configuration option
- Defaulting to HTTPS and only allowing HTTP in development/testing scenarios
- Adding a warning in documentation about the security implications of HTTP
$url = "http://{$this->host}:{$this->port}/";
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Audit/Audit.php (1)
225-236: Fixcleanup()docblock to describe datetime correctly.The docblock still says “Delete all logs older than
$datetimeseconds”, but$datetimeis an ISO 8601 datetime string (and not a duration), matching theAdapter::cleanup()contract. Suggest updating the comment accordingly:- /** - * Delete all logs older than `$datetime` seconds - * - * @param string $datetime - * @return bool - * - * @throws \Exception - */ + /** + * Delete all logs older than the specified datetime. + * + * @param string $datetime ISO 8601 datetime string + * @return bool + * + * @throws \Exception + */This keeps the public API documentation accurate and aligned with the adapter interface.
🧹 Nitpick comments (3)
tests/Audit/AuditBase.php (2)
9-27: Trait-based shared tests look good; contract is clear.The AuditBase trait cleanly centralizes adapter-agnostic tests, and the abstract
initializeAudit()contract is straightforward. The only implicit requirement is that consuming classes extendPHPUnit\Framework\TestCase, which is fine but could optionally be documented if you ever expose this beyond internal tests.
212-247: Consider asserting element types forlogBatchresults.You already assert that
logBatch()returns an array of 4 entries. To fully exercise the new contract, you could also assert that each element is aDocument:- $result = $this->audit->logBatch($batchEvents); - $this->assertIsArray($result); - $this->assertEquals(4, count($result)); + $result = $this->audit->logBatch($batchEvents); + $this->assertIsArray($result); + $this->assertCount(4, $result); + foreach ($result as $doc) { + $this->assertInstanceOf('Utopia\\Database\\Document', $doc); + }Not mandatory, but it would strengthen coverage of the new adapter API.
tests/Audit/AuditDatabaseTest.php (1)
19-42: Database adapter test wiring is solid; consider always runningsetup().The PDO,
Database, cache, andAdapter\Databasewiring all look correct and match the adapter contract.One subtle point:
Audit::setup()is only called when! $database->exists('utopiaTests'). If the database already exists but the audit collection/indexes do not (or schema changes), tests could fail in a non-obvious way. You might want to decouple DB creation from audit setup:- $adapter = new Adapter\Database($database); - $this->audit = new Audit($adapter); - if (! $database->exists('utopiaTests')) { - $database->create(); - $this->audit->setup(); - } + $adapter = new Adapter\Database($database); + $this->audit = new Audit($adapter); + + if (! $database->exists('utopiaTests')) { + $database->create(); + } + + // Idempotent: safe to call every time + $this->audit->setup();This keeps the DB bootstrap logic robust across re-runs and schema evolutions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Audit/Audit.php(1 hunks)tests/Audit/AuditBase.php(4 hunks)tests/Audit/AuditClickHouseTest.php(1 hunks)tests/Audit/AuditDatabaseTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
tests/Audit/AuditDatabaseTest.php (4)
src/Audit/Audit.php (2)
Audit(15-237)setup(45-48)src/Audit/Adapter.php (1)
Adapter(14-167)src/Audit/Adapter/Database.php (1)
Database(19-317)tests/Audit/AuditBase.php (1)
initializeAudit(26-26)
tests/Audit/AuditClickHouseTest.php (3)
src/Audit/Audit.php (2)
Audit(15-237)setup(45-48)src/Audit/Adapter.php (1)
Adapter(14-167)src/Audit/Adapter/ClickHouse.php (1)
ClickHouse(15-767)
tests/Audit/AuditBase.php (3)
src/Audit/Audit.php (3)
Audit(15-237)log(64-75)logBatch(85-88)tests/Audit/AuditClickHouseTest.php (1)
initializeAudit(19-32)tests/Audit/AuditDatabaseTest.php (1)
initializeAudit(23-42)
src/Audit/Audit.php (3)
src/Audit/Adapter.php (13)
Adapter(14-167)setup(27-27)create(45-45)createBatch(64-64)getByUser(75-75)countByUser(86-86)getByResource(97-97)countByResource(108-108)getByUserAndEvents(120-120)countByUserAndEvents(132-132)getByResourceAndEvents(144-144)countByResourceAndEvents(156-156)cleanup(166-166)src/Audit/Adapter/Database.php (13)
__construct(23-26)setup(42-60)create(69-84)createBatch(93-113)getByUser(123-134)countByUser(144-155)getByResource(165-176)countByResource(186-197)getByUserAndEvents(208-220)countByUserAndEvents(231-243)getByResourceAndEvents(254-266)countByResourceAndEvents(277-289)cleanup(298-316)src/Audit/Adapter/ClickHouse.php (13)
__construct(47-61)setup(212-253)create(260-316)createBatch(323-408)getByUser(494-529)countByUser(536-551)getByResource(558-592)countByResource(599-614)getByUserAndEvents(621-656)countByUserAndEvents(663-679)getByResourceAndEvents(686-721)countByResourceAndEvents(728-744)cleanup(753-766)
🪛 GitHub Actions: Tests
tests/Audit/AuditClickHouseTest.php
[error] 31-31: Test failed due to ClickHouse connection error: Could not resolve host: clickhouse
tests/Audit/AuditBase.php
[error] 33-33: Test setup failed due to ClickHouse connection error: Could not resolve host: clickhouse
src/Audit/Audit.php
[error] 47-47: Audit flow failed due to ClickHouse connection error: Could not resolve host: clickhouse
🔇 Additional comments (4)
tests/Audit/AuditBase.php (3)
31-43: setUp/tearDown lifecycle is consistent across adapters.Calling
initializeAudit()followed bycreateLogs()insetUp()and cleaning up intearDown()matches the adapters’ behavior and keeps each test isolated. No functional issues here.
53-57: Updated expectations onlog()return type are correct.Asserting
Utopia\Database\Documentinstances fromAudit::log()matches the new adapter contract and will catch regressions if a future adapter returns the wrong type.
276-290: Cleanup test correctly validateslog()’s new behavior.The additional
assertInstanceOfchecks aroundlog()in this test align with the adapter API and still correctly exercise the time-based cleanup logic.src/Audit/Audit.php (1)
17-223: Adapter-based refactor ofAuditis clean and aligns with the new contract.Constructor injection of
Adapter, thegetAdapter()accessor, and simple delegations forlog,logBatch, the variousget*/count*methods, andsetup()all look correct and match the abstractAdapterAPI. Return types (Document/array<Document>/int/bool) line up with the adapter signatures, and the method-level docblocks are consistent with the new behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (10)
src/Audit/Adapter/SQL.php (2)
161-161: Inconsistent index naming pattern.Three indexes follow the pattern
idx_{attribute}(e.g.,idx_event,idx_userId_event,idx_resource_event), but this one usesidx_time_desc. For consistency and maintainability, consider usingidx_timeto match the other index names.Apply this diff:
[ - '$id' => 'idx_time_desc', + '$id' => 'idx_time', 'type' => 'key', 'attributes' => ['time'], ],
201-218: Database-specific types in base SQL class violate abstraction.The
getColumnDefinition()method uses ClickHouse-specific types (String,DateTime64(3)) but is defined in the abstract SQL base class. This limits reusability for other SQL databases (PostgreSQL, MySQL, etc.) that would need different type mappings. The Database adapter doesn't currently use this method, but the architecture suggests shared functionality.Consider one of these solutions:
Solution 1 (preferred): Make the method abstract
- protected function getColumnDefinition(string $id): string - { - $attribute = $this->getAttribute($id); - - if (! $attribute) { - throw new \Exception("Attribute {$id} not found"); - } - - $type = match ($id) { - 'userId', 'event', 'resource', 'userAgent', 'ip', 'location', 'data' => 'String', - 'time' => 'DateTime64(3)', - default => 'String', - }; - - $nullable = ! $attribute['required'] ? 'Nullable(' . $type . ')' : $type; - - return "{$id} {$nullable}"; - } + /** + * Get SQL column definition for a given attribute ID. + * Must be implemented by each concrete SQL adapter for database-specific type mapping. + * + * @param string $id + * @return string + */ + abstract protected function getColumnDefinition(string $id): string;Solution 2: Move to ClickHouse adapter only
Remove this method from SQL class and implement it only in the ClickHouse adapter where it's actually used.
src/Audit/Adapter/ClickHouse.php (7)
586-594: Reduce code duplication in query parsing.The limit/offset parsing logic is duplicated across four methods (lines 586-594, 649-657, 712-720, 777-785). Consider extracting this into a helper method for maintainability.
/** * Parse limit and offset from query objects. * * @param array<int, Query> $queries * @return array{limit: int, offset: int} */ private function parseQueryParams(array $queries): array { $limit = 25; $offset = 0; foreach ($queries as $query) { if (is_object($query) && method_exists($query, 'getMethod') && method_exists($query, 'getValue')) { if ($query->getMethod() === 'limit') { $limit = (int) $query->getValue(); } elseif ($query->getMethod() === 'offset') { $offset = (int) $query->getValue(); } } } return ['limit' => $limit, 'offset' => $offset]; }Then in each method:
$params = $this->parseQueryParams($queries); $limit = $params['limit']; $offset = $params['offset'];
237-241: Consider reusing the Client instance.A new
Clientis created for every query, preventing connection reuse. For better performance, consider creating the client once in the constructor and reusing it across queries.+ private Client $client; + public function __construct( string $host, string $username = 'default', string $password = '', int $port = self::DEFAULT_PORT, bool $secure = false ) { $this->host = $host; $this->port = $port; $this->username = $username; $this->password = $password; $this->secure = $secure; + $this->client = new Client(); } private function query(string $sql, array $params = []): string { // ... parameter replacement ... - $client = new Client(); - $client->addHeader('X-ClickHouse-User', $this->username); - $client->addHeader('X-ClickHouse-Key', $this->password); - $client->addHeader('X-ClickHouse-Database', $this->database); - $client->setTimeout(30); + $this->client->addHeader('X-ClickHouse-User', $this->username); + $this->client->addHeader('X-ClickHouse-Key', $this->password); + $this->client->addHeader('X-ClickHouse-Database', $this->database); + $this->client->setTimeout(30); try { - $response = $client->fetch( + $response = $this->client->fetch(
51-63: Validate constructor parameters to prevent SQL injection.The constructor parameters (
$host, database names set viasetDatabase(), namespace, etc.) are directly interpolated into SQL queries without validation. If these values come from untrusted sources, they could enable SQL injection. Consider validating that identifiers match ClickHouse naming rules (alphanumeric + underscores) and that$portis in the valid range (1-65535).Example validation:
public function __construct( string $host, string $username = 'default', string $password = '', int $port = self::DEFAULT_PORT, bool $secure = false ) { if ($port < 1 || $port > 65535) { throw new \InvalidArgumentException('Port must be between 1 and 65535'); } if (!preg_match('/^[a-zA-Z0-9_.-]+$/', $host)) { throw new \InvalidArgumentException('Invalid host format'); } $this->host = $host; $this->port = $port; $this->username = $username; $this->password = $password; $this->secure = $secure; } public function setDatabase(string $database): self { if (!preg_match('/^[a-zA-Z0-9_]+$/', $database)) { throw new \InvalidArgumentException('Database name must contain only alphanumeric characters and underscores'); } $this->database = $database; return $this; }
206-234: Critical: SQL injection vulnerability withaddslashes().Using
addslashes()for SQL escaping is insufficient for ClickHouse and creates a SQL injection vulnerability. ClickHouse uses single-quote doubling ('→'') rather than backslash escaping. An attacker can craft inputs that bypassaddslashes()protection.Replace
addslashes()with proper ClickHouse escaping:foreach ($params as $key => $value) { if (is_int($value) || is_float($value)) { // Numeric values should not be quoted $strValue = (string) $value; } elseif (is_string($value)) { - $strValue = "'" . addslashes($value) . "'"; + $strValue = "'" . str_replace("'", "''", $value) . "'"; } elseif (is_null($value)) { $strValue = 'NULL'; } elseif (is_bool($value)) { $strValue = $value ? '1' : '0'; } elseif (is_array($value)) { $encoded = json_encode($value); if (is_string($encoded)) { - $strValue = "'" . addslashes($encoded) . "'"; + $strValue = "'" . str_replace("'", "''", $encoded) . "'"; } else { $strValue = 'NULL'; } } else { /** @var scalar $value */ - $strValue = "'" . addslashes((string) $value) . "'"; + $strValue = "'" . str_replace("'", "''", (string) $value) . "'"; } $sql = str_replace(":{$key}", $strValue, $sql); }Note: The same issue exists in
createBatch()(lines 409, 413, 424-430, 438-444) which also needs fixing.
398-446: Critical: Multiple SQL injection vulnerabilities increateBatch().This method has several critical security issues:
addslashes()is insufficient (same issue asquery()method) - usestr_replace("'", "''", $value)instead- Timestamp not escaped - Line 416 formats the timestamp but it's inserted raw into SQL at lines 429 and 443, allowing SQL injection through the
timestampfield- Events list built with string concatenation - Lines 722, 751, 787, 816 use
implode("', '", array_map('addslashes', $events))which is also vulnerableApply this diff to fix timestamp escaping:
- $formattedTimestamp = $this->formatTimestamp($log['timestamp']); + $formattedTimestamp = str_replace("'", "''", $this->formatTimestamp($log['timestamp']));Additionally, replace all
addslashes()calls in this method withstr_replace("'", "''", $value).
464-486: Critical: Returned document IDs don't match inserted records.Line 468 generates new IDs with
uniqid('audit_', true)for the return documents, but these don't match the IDs generated and inserted at line 406. This makes the returnedDocumentobjects unusable for lookups or references.Store and reuse the generated IDs:
$values = []; + $ids = []; foreach ($logs as $log) { $id = uniqid('audit_', true); + $ids[] = $id; $userIdVal = $log['userId'] ?? null; // ... rest of sprintf logic ... } // ... query execution ... // Return documents $documents = []; - foreach ($logs as $log) { + foreach ($logs as $index => $log) { $result = [ - '$id' => uniqid('audit_', true), + '$id' => $ids[$index], 'userId' => $log['userId'] ?? null, // ... rest of fields ...
722-722: Critical: SQL injection in event list construction.Building the events list with
implode("', '", array_map('addslashes', $events))and directly interpolating it into the SQL IN clause (line 729) creates a SQL injection vulnerability. The same issue appears at lines 751, 787, and 816.Consider using proper parameterization or at minimum replace
addslasheswith ClickHouse-safe escaping:- $eventsList = implode("', '", array_map('addslashes', $events)); + $eventsList = implode("', '", array_map(fn($e) => str_replace("'", "''", $e), $events));Better yet, extract this logic into a helper method to reduce duplication across lines 722, 751, 787, and 816:
private function buildEventsList(array $events): string { return implode("', '", array_map(fn($e) => str_replace("'", "''", $e), $events)); }src/Audit/Adapter/Database.php (1)
122-133: Critical:Query::orderDesc()requires an attribute parameter.The
Query::orderDesc()method requires a mandatoryattributeparameter to specify the sort field, but it's called without arguments on line 126. This will cause a runtime error. The same issue occurs at lines 166, 210, and 256.Based on the audit log schema, sort by
'time'in descending order:public function getByUser(string $userId, array $queries = []): array { return $this->db->getAuthorization()->skip(function () use ($userId, $queries) { $queries[] = Query::equal('userId', [$userId]); - $queries[] = Query::orderDesc(); + $queries[] = Query::orderDesc('time'); return $this->db->find( collection: $this->getCollectionName(), queries: $queries, ); }); }Apply the same fix to:
- Line 166 in
getByResource()- Line 210 in
getByUserAndEvents()- Line 256 in
getByResourceAndEvents()
🧹 Nitpick comments (1)
tests/Audit/Adapter/DatabaseTest.php (1)
37-40: Consider calling setup() unconditionally for consistency.The
setup()method is only called when the database is created, but the Database adapter'ssetup()already handles duplicate collections gracefully (line 57-59). For consistency with ClickHouseTest (which callssetup()unconditionally) and to ensure idempotence, consider callingsetup()every time.Apply this diff:
$adapter = new Adapter\Database($database); $this->audit = new Audit($adapter); - if (! $database->exists('utopiaTests')) { + if (!$database->exists('utopiaTests')) { $database->create(); - $this->audit->setup(); } + $this->audit->setup();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
composer.json(2 hunks)docker-compose.yml(1 hunks)src/Audit/Adapter/ClickHouse.php(1 hunks)src/Audit/Adapter/Database.php(1 hunks)src/Audit/Adapter/SQL.php(1 hunks)tests/Audit/Adapter/ClickHouseTest.php(1 hunks)tests/Audit/Adapter/DatabaseTest.php(1 hunks)tests/Audit/AuditBase.php(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- composer.json
🧰 Additional context used
🧬 Code graph analysis (2)
src/Audit/Adapter/SQL.php (3)
src/Audit/Audit.php (1)
Audit(15-237)src/Audit/Adapter.php (1)
Adapter(14-167)src/Audit/Adapter/Database.php (1)
Database(19-314)
tests/Audit/AuditBase.php (4)
src/Audit/Audit.php (3)
Audit(15-237)log(64-75)logBatch(85-88)src/Audit/Adapter/Database.php (1)
Database(19-314)tests/Audit/Adapter/ClickHouseTest.php (1)
initializeAudit(17-30)tests/Audit/Adapter/DatabaseTest.php (1)
initializeAudit(22-41)
🪛 PHPMD (2.15.0)
src/Audit/Adapter/ClickHouse.php
622-622: Avoid unused parameters such as '$queries'. (undefined)
(UnusedFormalParameter)
685-685: Avoid unused parameters such as '$queries'. (undefined)
(UnusedFormalParameter)
749-749: Avoid unused parameters such as '$queries'. (undefined)
(UnusedFormalParameter)
814-814: Avoid unused parameters such as '$queries'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (4)
docker-compose.yml (1)
11-40: LGTM! ClickHouse service configured correctly.The ClickHouse service is properly configured with appropriate healthcheck, environment variables, and service dependencies. The healthcheck ensures the service is ready before tests run, which prevents flaky test failures.
tests/Audit/Adapter/ClickHouseTest.php (1)
17-30: LGTM! Test initialization is correct.The ClickHouse adapter is properly initialized with configuration matching the docker-compose service. The initialization sequence (construct → setDatabase → create Audit → setup) follows the expected pattern.
tests/Audit/AuditBase.php (2)
53-56: LGTM! Improved test assertions.The change from
assertTruetoassertInstanceOfprovides more specific type checking and aligns with the adapter API wherelog()returns aDocumentinstance. This improves test quality and catches type-related issues earlier.
213-215: LGTM! Batch logging assertions are thorough.The test now captures the result, verifies it's an array, and checks the expected count. This provides better validation of the
logBatch()functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Audit/Adapter/ClickHouse.php (1)
760-762: Escape database and table identifiers in queries.Database and table names are directly interpolated without
escapeIdentifier()throughout the query methods (e.g., lines 762, 790, 816, 843, 871, 900, 926, 956, 981), whilesetup()properly escapes them (line 441). Although validation prevents injection, identifiers with special characters could cause SQL syntax errors, and consistency is important.Apply this pattern throughout all query methods:
+ $escapedDatabaseAndTable = $this->escapeIdentifier($this->database) . '.' . $this->escapeIdentifier($tableName); + $sql = " SELECT " . $this->getSelectColumns() . " - FROM {$this->database}.{$tableName} + FROM {$escapedDatabaseAndTable} WHERE userId = :userId{$tenantFilter} ORDER BY time DESC LIMIT :limit OFFSET :offset FORMAT TabSeparated ";
🧹 Nitpick comments (1)
src/Audit/Adapter/ClickHouse.php (1)
783-783: Consider utilizing query parameters in count methods.The
$queriesparameter is unused in all fourcount*methods, though it's required by theAdapterinterface. While count operations typically don't need limit/offset, future requirements might include other filters from the queries array.Also applies to: 837-837, 892-892, 948-948
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Audit/Adapter/ClickHouse.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Audit/Adapter/ClickHouse.php (2)
src/Audit/Adapter.php (14)
Adapter(14-167)getName(19-19)setup(27-27)create(45-45)createBatch(64-64)getByUser(75-75)countByUser(86-86)getByResource(97-97)countByResource(108-108)getByUserAndEvents(120-120)countByUserAndEvents(132-132)getByResourceAndEvents(144-144)countByResourceAndEvents(156-156)cleanup(166-166)src/Audit/Adapter/SQL.php (4)
SQL(15-236)getAttributes(43-120)getColumnDefinition(201-218)getIndexes(142-166)
🪛 PHPMD (2.15.0)
src/Audit/Adapter/ClickHouse.php
783-783: Avoid unused parameters such as '$queries'. (undefined)
(UnusedFormalParameter)
837-837: Avoid unused parameters such as '$queries'. (undefined)
(UnusedFormalParameter)
892-892: Avoid unused parameters such as '$queries'. (undefined)
(UnusedFormalParameter)
948-948: Avoid unused parameters such as '$queries'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (13)
src/Audit/Adapter/ClickHouse.php (13)
54-75: LGTM: Constructor properly initializes client for reuse.The constructor correctly validates inputs, stores configuration, and initializes the HTTP client once for connection reuse. The database header is appropriately set per-query (line 363) to support
setDatabase()calls after construction.
175-184: LGTM: Proper SQL escaping for ClickHouse.The
escapeString()method correctly escapes both backslashes and single quotes in the proper order, following SQL standards for ClickHouse. This addresses previous SQL injection concerns.
332-389: LGTM: Query execution properly implements secure HTTPS and exception chaining.The
query()method correctly:
- Uses HTTPS when
$secureflag is set (line 334)- Reuses the HTTP client initialized in the constructor
- Escapes string parameters with
escapeString()- Preserves the original exception chain for better debugging (line 385-386)
These changes address multiple concerns from previous reviews.
399-456: LGTM: Setup properly configures ClickHouse schema.The
setup()method correctly:
- Escapes database and table identifiers (lines 402, 441)
- Handles time column as NOT NULL for the partition key (line 418)
- Conditionally adds tenant column for shared tables (lines 425-427)
- Uses MergeTree engine with appropriate partitioning strategy
463-522: LGTM: Create method properly generates and returns consistent IDs.The
create()method correctly generates a unique ID (line 465) and returns a Document with that same ID (line 506), ensuring consistency between inserted records and returned documents.
529-621: LGTM: Batch insert correctly reuses generated IDs.The
createBatch()method properly:
- Pre-generates and stores IDs (lines 538-539)
- Returns documents using those same IDs (line 602)
- Escapes all string values throughout the VALUES construction
This addresses the previous issue where returned document IDs didn't match inserted records.
628-680: LGTM: Result parsing correctly handles dynamic column counts.The
parseResults()method properly validates the expected column count based onsharedTablesflag (line 644), fixing the previous bug where non-shared-table results were incorrectly rejected.
741-744: LGTM: Helper reduces code duplication.The
buildEventsList()helper method properly centralizes the events list building logic, addressing previous concerns about code duplication. Each event is correctly escaped.
859-964: LGTM: Events filtering properly escapes values.The events query methods correctly use the
buildEventsList()helper (lines 865, 894, 921, 950) which escapes each event before interpolation. While not true parameterization, this is safe given ClickHouse's HTTP interface limitations with array parameters.
701-708: LGTM: Tenant filtering correctly handles integer values.The
getTenantFilter()method safely interpolates the tenant ID (line 707) since it's validated as an integer via thesetTenant()method signature (line 244).
716-732: LGTM: Query parameter parsing properly centralized.The
parseQueryParams()helper consolidates limit/offset extraction logic that was previously duplicated across multiple methods, improving maintainability.
973-988: LGTM: Cleanup uses modern ClickHouse DELETE syntax.The
cleanup()method correctly uses theDELETEstatement (line 981) supported in ClickHouse 23.3+, with proper parameterization and tenant filtering.
105-108: Remove redundant validation check.Line 106 is unreachable. If the regex on line 101 passes, the condition
!preg_match('/^[a-zA-Z0-9._\-]+$/', $host)(which is a subset of line 101's pattern) will never be true.- // Prevent localhost references that might bypass security - if (filter_var($host, FILTER_VALIDATE_IP) === false && !preg_match('/^[a-zA-Z0-9._\-]+$/', $host)) { - throw new Exception('ClickHouse host must be a valid hostname or IP address'); - }Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Audit/Adapter/Database.php (1)
122-133: VerifyQuery::orderDesc()parameter requirement.The code calls
Query::orderDesc()without arguments on line 126 (and similarly on lines 166, 210, 256). A previous review indicated that this method requires a mandatoryattributeparameter. Please verify whether the current version of utopia-php/database (4.0.0) supports callingorderDesc()without parameters, or if a field name should be provided.#!/bin/bash # Verify Query::orderDesc usage patterns in the codebase and check method signature rg -nP --type=php -C3 'Query::orderDesc\(' -g '!vendor/**'utopia-php/database version 4.0.0 Query::orderDesc method signature and parameters
🧹 Nitpick comments (2)
src/Audit/Adapter/SQL.php (1)
142-165: Index definitions are functional.The indexes cover the primary query patterns (by event, by userId+event, by resource+event, by time). Note that a past review suggested renaming
idx_time_desctoidx_timefor consistency with other index names, but the current naming may be intentional to indicate sort order.src/Audit/Adapter/ClickHouse.php (1)
778-803: Consider escaping table/database names in query methods for consistency.The query construction in
getByUser()(line 789),getByResource()(line 843),getByUserAndEvents()(line 899), andgetByResourceAndEvents()(line 954) directly interpolates{$this->database}.{$tableName}without usingescapeIdentifier(). While these values are validated insetDatabase()andgetTableName(), usingescapeIdentifier()consistently (as done insetup()at lines 402, 441, 494, 590) would provide defense-in-depth.Apply a pattern like this across all query methods:
+ $escapedTable = $this->escapeIdentifier($this->database) . '.' . $this->escapeIdentifier($tableName); $sql = " SELECT " . $this->getSelectColumns() . " - FROM {$this->database}.{$tableName} + FROM {$escapedTable} WHERE userId = :userId{$tenantFilter} ORDER BY time DESC LIMIT :limit OFFSET :offset FORMAT TabSeparated ";Also applies to: 832-857, 886-912, 942-968
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Audit/Adapter/ClickHouse.php(1 hunks)src/Audit/Adapter/Database.php(1 hunks)src/Audit/Adapter/SQL.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/Audit/Adapter/SQL.php (2)
src/Audit/Adapter/Database.php (2)
Database(19-348)getColumnDefinition(327-347)src/Audit/Adapter/ClickHouse.php (1)
getColumnDefinition(753-771)
src/Audit/Adapter/Database.php (3)
src/Audit/Adapter.php (14)
Adapter(14-167)getName(19-19)setup(27-27)create(45-45)createBatch(64-64)getByUser(75-75)countByUser(86-86)getByResource(97-97)countByResource(108-108)getByUserAndEvents(120-120)countByUserAndEvents(132-132)getByResourceAndEvents(144-144)countByResourceAndEvents(156-156)cleanup(166-166)src/Audit/Adapter/SQL.php (6)
SQL(15-221)getAttributeDocuments(127-130)getIndexDocuments(173-176)getCollectionName(24-27)getColumnDefinition(202-202)getAttribute(184-193)src/Audit/Adapter/ClickHouse.php (15)
__construct(54-75)getName(80-83)setup(399-456)create(463-522)createBatch(529-621)getByUser(778-803)countByUser(810-825)getByResource(832-857)countByResource(864-879)getByUserAndEvents(886-912)countByUserAndEvents(919-935)getByResourceAndEvents(942-968)countByResourceAndEvents(975-991)cleanup(1000-1015)getColumnDefinition(753-771)
src/Audit/Adapter/ClickHouse.php (3)
src/Audit/Adapter.php (14)
Adapter(14-167)getName(19-19)setup(27-27)create(45-45)createBatch(64-64)getByUser(75-75)countByUser(86-86)getByResource(97-97)countByResource(108-108)getByUserAndEvents(120-120)countByUserAndEvents(132-132)getByResourceAndEvents(144-144)countByResourceAndEvents(156-156)cleanup(166-166)src/Audit/Adapter/Database.php (15)
Database(19-348)getName(31-34)setup(42-60)getColumnDefinition(327-347)create(69-84)createBatch(93-113)getByUser(122-133)countByUser(141-152)getByResource(162-173)countByResource(183-194)getByUserAndEvents(205-217)countByUserAndEvents(228-240)getByResourceAndEvents(251-263)countByResourceAndEvents(274-286)cleanup(295-313)src/Audit/Adapter/SQL.php (4)
SQL(15-221)getAttributes(43-120)getColumnDefinition(202-202)getAttribute(184-193)
🪛 PHPMD (2.15.0)
src/Audit/Adapter/ClickHouse.php
810-810: Avoid unused parameters such as '$queries'. (undefined)
(UnusedFormalParameter)
864-864: Avoid unused parameters such as '$queries'. (undefined)
(UnusedFormalParameter)
919-919: Avoid unused parameters such as '$queries'. (undefined)
(UnusedFormalParameter)
975-975: Avoid unused parameters such as '$queries'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (17)
src/Audit/Adapter/SQL.php (3)
1-27: LGTM! Clean abstract base structure.The class structure is well-designed, with a clear separation of concerns. The public constant for the collection name and the protected getter method provide the right level of encapsulation for derived adapters.
43-120: Well-structured attribute schema.The attribute definitions are comprehensive and appropriate for audit logging. Field sizes are reasonable (e.g., ip: 45 chars supports IPv6, userAgent: 65534 for long user agents, data: 16777216 for large payloads).
210-220: Type annotation correctly addresses the previous type concern.The addition of the
@var stringannotation on line 214 ensures thatgetColumnDefinition()receives the expected string type, resolving the previous static analysis error.src/Audit/Adapter/Database.php (4)
1-60: LGTM! Clean setup implementation.The constructor and setup logic properly initialize the database adapter. The exception handling for
DuplicateExceptionis appropriate for idempotent setup operations.
69-113: Create and batch operations implemented correctly.The distinction between
create()(usingDateTime::now()) andcreateBatch()(using provided timestamps) is appropriate. Authorization skip boundaries are correctly applied for system-level audit operations.
295-313: Batch cleanup implementation is sound.The iterative batch deletion pattern ensures that large cleanup operations don't overwhelm the database. The unconditional
return trueis acceptable as exceptions will propagate any actual failures.
327-347: Column definition implementation satisfies the abstract contract.The implementation provides a descriptive format that satisfies the abstract method requirement while acknowledging that the Database adapter relies on utopia-php/database's native type mapping. The documentation clearly explains this design choice.
src/Audit/Adapter/ClickHouse.php (10)
54-122: LGTM! Robust parameter validation.The constructor and validation methods implement comprehensive input validation, which is essential for preventing SQL injection and configuration errors. The identifier validation on lines 132-152 properly enforces ClickHouse naming conventions and guards against SQL keywords.
161-184: Proper SQL escaping implemented.The escaping methods correctly implement SQL-standard escaping. The
escapeString()method on lines 175-184 properly doubles single quotes and escapes backslashes, addressing the previous SQL injection concerns about usingaddslashes().
332-389: Query execution with proper exception context.The query method implements secure parameter replacement using the
escapeString()helper for string values. The exception wrapping on lines 383-387 properly preserves the original exception context while adding helpful debugging information. Client reuse from the constructor enables connection pooling.
399-456: Well-designed ClickHouse table structure.The setup creates an appropriate MergeTree table with monthly partitioning by time (line 451), which is optimal for time-series audit data. The special handling of the
timecolumn as NOT NULL (lines 417-418) is correct since it's used in the partition key. Identifiers are properly escaped on lines 402 and 441.
463-522: Create method properly escapes identifiers and uses parameterized queries.The single log creation correctly generates a unique ID, formats the timestamp with millisecond precision (line 468), and uses parameterized queries with the
:placeholdersyntax. Identifiers are properly escaped on line 494.
529-621: Batch creation correctly preserves IDs.The batch insert properly stores generated IDs (lines 536, 539) and reuses them when constructing return documents (line 602), fixing the previous issue where returned documents had different IDs than inserted records. All string values are escaped using
escapeString(), and identifiers are properly escaped on line 590.
628-680: Result parsing correctly handles tenancy modes.The parsing logic properly adjusts the expected column count based on
sharedTablesmode (line 644), fixing the previous issue. The conversion of ClickHouse's\Nnull representation to PHP null (lines 661, 666, 673) and timestamp format conversion (lines 655-656) are correct.
810-825: Count methods correctly implement the interface contract.The
$queriesparameter on line 810 (and similarly on lines 864, 919, 975) is required by the abstractAdapterinterface but intentionally unused in count operations where limit/offset don't apply. The PHPMD warning is a false positive in this case.
886-912: Event filtering properly escapes values.The IN clause construction on line 899 uses the
buildEventsList()helper (defined at lines 741-744), which appliesescapeString()to each event before interpolation. This provides proper SQL injection protection for the event array filtering.
1000-1015: Cleanup implementation uses appropriate ClickHouse DELETE syntax.The cleanup method uses the synchronous DELETE statement (line 1008), which is appropriate for ClickHouse 23.3+. The tenant filtering is correctly applied. As noted in the previous comment about query methods, consider escaping the table identifier for consistency.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/Audit/Audit.php (1)
236-247: Docblock still references "seconds" incorrectly.The docblock says "older than
$datetimeseconds" but the parameter is an ISO 8601 datetime string. This was flagged in a previous review.
🧹 Nitpick comments (2)
tests/Audit/AuditBase.php (1)
67-74: Duplicate assertions: identical test calls appear redundant.Lines 67-69 and 71-73 execute the same query with identical parameters (
limit: 1, offset: 1) and perform the same assertions. This pattern repeats in other test methods (lines 90/95, 115/120, 140/144). If these are intentional idempotency checks, consider adding a clarifying comment; otherwise, remove the duplicates.src/Audit/Adapter/ClickHouse.php (1)
715-722: Tenant ID interpolation bypasses parameter mechanism.The
getTenantFilter()method directly interpolates$this->tenantinto SQL without using the parameterized query mechanism. Whiletenantis an integer (reducing injection risk), this is inconsistent with the otherwise careful parameterization approach used throughout the adapter.Consider returning the filter with a placeholder and merging tenant into params:
- private function getTenantFilter(): string + /** + * @return array{clause: string, params: array<string, mixed>} + */ + private function getTenantFilter(): array { if (!$this->sharedTables || $this->tenant === null) { - return ''; + return ['clause' => '', 'params' => []]; } - return " AND tenant = {$this->tenant}"; + return [ + 'clause' => ' AND tenant = {tenant:UInt64}', + 'params' => ['tenant' => $this->tenant], + ]; }Then update callers to merge the tenant params similar to how time params are merged.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Audit/Adapter.php(1 hunks)src/Audit/Adapter/ClickHouse.php(1 hunks)src/Audit/Adapter/Database.php(1 hunks)src/Audit/Audit.php(1 hunks)tests/Audit/AuditBase.php(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Audit/Adapter/Database.php
🧰 Additional context used
🧬 Code graph analysis (4)
tests/Audit/AuditBase.php (2)
src/Audit/Audit.php (7)
Audit(14-248)log(63-74)getLogsByUser(97-106)getLogsByUserAndEvents(167-177)getLogsByResourceAndEvents(206-216)getLogsByResource(131-140)logBatch(84-87)src/Audit/Log.php (2)
Log(15-209)getId(32-36)
src/Audit/Audit.php (4)
src/Audit/Adapter.php (13)
Adapter(12-205)setup(25-25)create(43-43)createBatch(62-62)getByUser(72-79)countByUser(89-93)getByResource(103-110)countByResource(120-124)getByUserAndEvents(135-143)countByUserAndEvents(154-159)getByResourceAndEvents(170-178)countByResourceAndEvents(189-194)cleanup(204-204)src/Audit/Adapter/ClickHouse.php (13)
__construct(55-76)setup(398-455)create(462-521)createBatch(528-626)getByUser(821-852)countByUser(859-882)getByResource(889-920)countByResource(927-950)getByUserAndEvents(957-989)countByUserAndEvents(996-1020)getByResourceAndEvents(1027-1059)countByResourceAndEvents(1066-1090)cleanup(1099-1115)src/Audit/Adapter/Database.php (13)
__construct(24-27)setup(43-61)create(70-78)createBatch(87-97)getByUser(132-157)countByUser(164-179)getByResource(188-213)countByResource(222-237)getByUserAndEvents(247-274)countByUserAndEvents(284-301)getByResourceAndEvents(311-338)countByResourceAndEvents(348-365)cleanup(374-385)src/Audit/Log.php (2)
__construct(22-25)Log(15-209)
src/Audit/Adapter.php (4)
src/Audit/Audit.php (4)
Audit(14-248)setup(44-47)log(63-74)cleanup(244-247)src/Audit/Adapter/ClickHouse.php (13)
getName(81-84)setup(398-455)create(462-521)createBatch(528-626)getByUser(821-852)countByUser(859-882)getByResource(889-920)countByResource(927-950)getByUserAndEvents(957-989)countByUserAndEvents(996-1020)getByResourceAndEvents(1027-1059)countByResourceAndEvents(1066-1090)cleanup(1099-1115)src/Audit/Adapter/Database.php (13)
getName(32-35)setup(43-61)create(70-78)createBatch(87-97)getByUser(132-157)countByUser(164-179)getByResource(188-213)countByResource(222-237)getByUserAndEvents(247-274)countByUserAndEvents(284-301)getByResourceAndEvents(311-338)countByResourceAndEvents(348-365)cleanup(374-385)src/Audit/Log.php (1)
Log(15-209)
src/Audit/Adapter/ClickHouse.php (3)
src/Audit/Adapter.php (14)
Adapter(12-205)getName(17-17)setup(25-25)create(43-43)createBatch(62-62)getByUser(72-79)countByUser(89-93)getByResource(103-110)countByResource(120-124)getByUserAndEvents(135-143)countByUserAndEvents(154-159)getByResourceAndEvents(170-178)countByResourceAndEvents(189-194)cleanup(204-204)src/Audit/Log.php (4)
Log(15-209)__construct(22-25)getTenant(131-148)getAttribute(157-160)src/Audit/Adapter/SQL.php (5)
SQL(15-221)getAttributes(43-120)getColumnDefinition(202-202)getIndexes(142-166)getAttribute(184-193)
🔇 Additional comments (8)
tests/Audit/AuditBase.php (1)
9-26: Well-structured trait for adapter-agnostic testing.The trait design with
initializeAudit()abstract method provides a clean pattern for testing multiple adapter implementations with shared test logic.src/Audit/Adapter.php (1)
12-204: Clean adapter interface design.The abstract
Adapterclass provides a comprehensive and well-documented contract for audit storage backends. The method signatures are consistent, with appropriate typing for parameters and return values. The use of PHPDoc array shapes (e.g.,array{userId?: string|null, ...}) aids static analysis and IDE support.src/Audit/Audit.php (1)
14-46: Clean adapter delegation pattern.The refactoring to use a pluggable
Adapteris well-executed. The constructor accepts an adapter, and all persistence operations are cleanly delegated. ThegetAdapter()method allows access when adapter-specific configuration is needed.src/Audit/Adapter/ClickHouse.php (5)
55-76: Good: Host and port validation in constructor.The constructor properly validates host using the
Hostnamevalidator and validates the port range. The HTTP client is initialized once and reused across queries, addressing connection reuse concerns from previous reviews.
310-348: Proper use of ClickHouse native query parameters.The
query()method correctly uses ClickHouse's native parameter mechanism via multipart form data (param_*keys). This approach is injection-safe as ClickHouse handles all escaping internally. The exception handling also preserves the original exception context.
528-626: Batch insert correctly reuses generated IDs.The
createBatch()method now correctly stores generated IDs in the$idsarray and reuses them when constructing the returnedLogobjects, addressing the previously flagged ID mismatch issue.
648-652: Column count validation handles sharedTables mode correctly.The
parseResults()method now correctly expects 9 or 10 columns based on thesharedTablessetting, fixing the previously reported bug.
411-421: Column definition result is discarded in else branch.The
elsebranch calls$this->getColumnDefinition($id)but doesn't use the result. The column is never added to$columns, meaning all attributes excepttimeare silently skipped from the table schema.Apply this diff to fix the table schema creation:
foreach ($this->getAttributes() as $attribute) { /** @var string $id */ $id = $attribute['$id']; // Special handling for time column - must be NOT NULL for partition key if ($id === 'time') { $columns[] = 'time DateTime64(3)'; } else { - $this->getColumnDefinition($id); + $columns[] = $this->getColumnDefinition($id); } }Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/Audit/Adapter/ClickHouseTest.php (4)
36-82: Constructor validation tests are solid; consider minor DRYingHost and port range validation are well-covered with explicit message checks; you could optionally consolidate the two port tests via a data provider to reduce duplication, but it's fine as is.
114-195: Database validation tests are comprehensive; maybe add a whitespace caseEmpty, over-length, bad-format, keyword, and valid identifiers are all exercised; you might optionally add a case for whitespace-only database names if
validateIdentifiertreats them differently.
246-291: Secure flag and shared-tables behavior are partially coveredShared-tables and tenant flows are validated nicely, but
testSetSecurecurrently only asserts fluent return type; if the adapter exposes any way to observe the secure setting (e.g., a DSN or client config getter), consider asserting that too.
293-318: Batch special-character test is useful; assertions could be stricterThe test ensures batch logging doesn’t blow up and that records are retrievable, but you could strengthen it by asserting that at least one returned log matches the exact user ID and key fields (event, resource, userAgent, data) to verify escaping/round‑trip correctness, not just count.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/Audit/Adapter/ClickHouseTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/Audit/Adapter/ClickHouseTest.php (1)
src/Audit/Adapter/ClickHouse.php (8)
ClickHouse(16-1116)setDatabase(179-184)setNamespace(163-170)getNamespace(200-203)setSecure(189-193)isSharedTables(246-249)setSharedTables(235-239)setTenant(212-216)
🔇 Additional comments (3)
tests/Audit/Adapter/ClickHouseTest.php (3)
21-34: Audit initialization is clear and adapter-focusedInitialization cleanly wires the ClickHouse adapter into
Auditand callssetup(), matching the intent ofAuditBaseand the ClickHouse docker service name; no changes needed here.
83-112: Happy-path constructor andgetNamecoverage look goodThe tests verify both successful construction with non-default parameters and the adapter name, giving basic “smoke” coverage for the ClickHouse adapter creation.
197-244: Namespace tests correctly reflect nullable/empty semanticsAllowing an empty namespace while still validating bad and good formats is well captured; this matches typical multi-tenant scoping patterns and seems appropriate.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/Audit/Audit.php (2)
5-5: Remove unused import.The
use Utopia\Database\Database;import is no longer used after the adapter pattern refactoring.Apply this diff:
namespace Utopia\Audit; -use Utopia\Database\Database; - /** * Audit Log Manager
237-239: Fix incorrect docblock.The docblock incorrectly states "older than
$datetimeseconds" but the parameter is an ISO 8601 datetime string, not a number of seconds.Apply this diff:
/** - * Delete all logs older than `$datetime` seconds + * Delete all logs older than the specified datetime. * - * @param string $datetime + * @param string $datetime ISO 8601 datetime string * @return bool
🧹 Nitpick comments (2)
tests/Audit/AuditBase.php (2)
398-398: Remove unused variable.The variable
$baseTimeis assigned but never used in the test. This creates unnecessary noise.Apply this diff:
$this->audit->cleanup(DateTime::now()); $userId = 'paramtestuser'; $userAgent = 'Mozilla/5.0'; $ip = '192.168.1.1'; $location = 'US'; // Create 5 logs with different timestamps - $baseTime = new \DateTime('2024-06-15 12:00:00'); $batchEvents = []; for ($i = 0; $i < 5; $i++) {
481-503: Remove duplicate test code.Lines 493-503 are exact duplicates of lines 481-491 (Test 7 is repeated as Test 8). This duplication reduces maintainability and may indicate a copy-paste error.
Apply this diff to remove the duplicate:
// Test 7: Combination of ascending + limit $ascLimit2 = $this->audit->getLogsByUser($userId, limit: 2, ascending: true); $this->assertEquals(2, \count($ascLimit2)); // First log should be oldest in ascending order $this->assertEquals('event_0', $ascLimit2[0]->getAttribute('event')); // Test 8: Combination of after + before (time range) $afterTimeObj2 = new \DateTime('2024-06-15 12:01:00'); // After 1st log $beforeTimeObj2 = new \DateTime('2024-06-15 12:04:00'); // Before 4th log $logsRange = $this->audit->getLogsByUser($userId, after: $afterTimeObj2, before: $beforeTimeObj2); $this->assertGreaterThanOrEqual(1, \count($logsRange)); - // Test 7: Combination of ascending + limit - $ascLimit2 = $this->audit->getLogsByUser($userId, limit: 2, ascending: true); - $this->assertEquals(2, \count($ascLimit2)); - // First log should be oldest in ascending order - $this->assertEquals('event_0', $ascLimit2[0]->getAttribute('event')); - - // Test 8: Combination of after + before (time range) - $afterTimeObj2 = new \DateTime('2024-06-15 12:01:00'); // After 1st log - $beforeTimeObj2 = new \DateTime('2024-06-15 12:04:00'); // Before 4th log - $logsRange = $this->audit->getLogsByUser($userId, after: $afterTimeObj2, before: $beforeTimeObj2); - $this->assertGreaterThanOrEqual(1, \count($logsRange)); - // Test 9: Test with getLogsByResource using parameters
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Audit/Adapter.php(1 hunks)src/Audit/Adapter/ClickHouse.php(1 hunks)src/Audit/Adapter/Database.php(1 hunks)src/Audit/Audit.php(1 hunks)tests/Audit/AuditBase.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/Audit/Audit.php (3)
src/Audit/Adapter.php (13)
Adapter(12-205)setup(25-25)create(43-43)createBatch(62-62)getByUser(72-79)countByUser(89-93)getByResource(103-110)countByResource(120-124)getByUserAndEvents(135-143)countByUserAndEvents(154-159)getByResourceAndEvents(170-178)countByResourceAndEvents(189-194)cleanup(204-204)src/Audit/Adapter/Database.php (13)
__construct(24-27)setup(43-61)create(70-78)createBatch(87-97)getByUser(135-160)countByUser(167-182)getByResource(191-216)countByResource(225-240)getByUserAndEvents(250-277)countByUserAndEvents(287-304)getByResourceAndEvents(314-341)countByResourceAndEvents(351-368)cleanup(377-388)src/Audit/Log.php (2)
__construct(22-25)Log(15-209)
src/Audit/Adapter/ClickHouse.php (2)
src/Audit/Log.php (2)
Log(15-209)getAttribute(157-160)src/Audit/Adapter/SQL.php (4)
SQL(15-221)getAttributes(43-120)getColumnDefinition(202-202)getAttribute(184-193)
tests/Audit/AuditBase.php (6)
src/Audit/Audit.php (12)
Audit(14-248)cleanup(244-247)log(63-74)getLogsByUser(97-106)countLogsByUser(115-121)getLogsByUserAndEvents(167-177)countLogsByUserAndEvents(188-195)getLogsByResourceAndEvents(206-216)countLogsByResourceAndEvents(227-234)getLogsByResource(131-140)countLogsByResource(150-156)logBatch(84-87)src/Audit/Log.php (2)
Log(15-209)getId(32-36)src/Audit/Adapter/Database.php (2)
Database(20-423)cleanup(377-388)tests/Audit/Adapter/DatabaseTest.php (1)
initializeAudit(22-41)src/Audit/Adapter.php (1)
cleanup(204-204)src/Audit/Adapter/ClickHouse.php (1)
cleanup(1112-1128)
🪛 PHPMD (2.15.0)
tests/Audit/AuditBase.php
398-398: Avoid unused local variables such as '$baseTime'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (3)
src/Audit/Adapter.php (1)
1-205: LGTM!The abstract Adapter interface is well-designed with comprehensive documentation, consistent method signatures, and proper type annotations. The interface provides a clear contract for concrete adapter implementations.
src/Audit/Adapter/ClickHouse.php (1)
1-1129: LGTM!The ClickHouse adapter implementation is comprehensive and secure:
- Uses ClickHouse's native parameterized query mechanism to prevent SQL injection
- Implements thorough input validation (host, port, identifiers)
- Properly escapes identifiers with backticks
- Maintains consistent error handling throughout
- Well-structured with clear separation of concerns
The previous security concerns about SQL injection have been properly addressed with the parameterized query approach.
src/Audit/Adapter/Database.php (1)
1-423: LGTM!The Database adapter implementation is clean and correct:
- Properly uses authorization wrappers for all operations
- Correctly builds time-based queries with Query builders
- Implements consistent error handling across all methods
- Cleanup loop properly captures and checks the return value
- Well-structured with clear separation of concerns
All previously flagged issues have been addressed.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
README.md (2)
290-297: ClickHouse constructor example uses non-existent parameters.This example will fail at runtime—the constructor does not accept
databaseortableparameters, and there's notablesetter. This was previously flagged in code review but remains unaddressed.Apply this diff to match the actual constructor signature:
-$adapter = new ClickHouse( - host: 'localhost', - database: 'audit', - username: 'default', - password: '', - port: 8123, - table: 'audit_logs' -); +$adapter = new ClickHouse( + host: 'localhost', + username: 'default', + password: '', + port: 8123, + secure: false +); + +// Configure database/namespace as needed +$adapter->setDatabase('audit');
330-330: Custom adapter example should returnLogobjects, notDocument.The built-in adapters return
Utopia\Audit\Loginstances, but this example imports and returnsDocument. This was previously flagged but remains unresolved and will mislead developers implementing custom adapters.Apply this diff to align with the actual adapter contract:
-use Utopia\Audit\Adapter; -use Utopia\Database\Document; +use Utopia\Audit\Adapter; +use Utopia\Audit\Log; class CustomAdapter extends Adapter { // ... - public function create(array $log): Document + public function create(array $log): Log { // Store a single log entry + // return new Log($storedData); } - public function createBatch(array $logs): array + /** + * @return array<Log> + */ + public function createBatch(array $logs): array { // Store multiple log entries + // return array of Log objects }Also applies to: 344-344, 349-349
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(3 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
README.md
129-129: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
152-152: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
171-171: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
204-204: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
236-236: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Dockerfile(2 hunks)
🔇 Additional comments (2)
Dockerfile (2)
1-1: Composer version bump to 2.8 is good; verify compatibility.Updating from composer:2.0 to composer:2.8 is a significant bump (2+ years of improvements and security fixes). Ensure that all project dependencies are compatible with the newer Composer release.
15-15: Verify if additional PHP extensions are required for adapters.The PR introduces Database and ClickHouse adapters. Depending on the concrete implementation, the Database adapter may require additional PHP extensions (e.g.,
pdo_sqliteif SQLite is used for testing). ClickHouse tests run via docker-compose, so no client extension is needed here. Confirm that all necessary extensions are present for both adapter tests and CI workflows.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
Dockerfile (1)
11-11: Stage formatting issue has been fixed.The double space before
finalmentioned in prior review is now corrected to single space.tests/Audit/AuditBase.php (1)
253-260: Type mismatch issue has been fixed.The code now correctly passes
\DateTimeobjects togetLogsByUser, which matches the method signature expecting?\DateTime. The previous issue withDateTime::addSeconds()returning aDateTimeobject being passed where a string was expected is resolved.README.md (3)
103-103: Use proper heading syntax instead of bold text.Multiple sections use
**text**instead of###headings (lines 103, 129, 152, 171, 204, 236). This triggers markdownlint MD036 warnings and affects document structure.-**Get Logs By User** +### Get Logs By UserApply this pattern to all affected lines: 103, 129, 152, 171, 204, 236.
270-312: ClickHouse usage example does not match constructor signature.The example shows
database,tableparameters that don't exist in the actual constructor. The constructor signature is:public function __construct( string $host, string $username = 'default', string $password = '', int $port = self::DEFAULT_PORT, bool $secure = false )Update the example to match the actual API and include
setDatabase()call.
270-312: Missing security documentation for ClickHouse adapter.The ClickHouse section should include security considerations about:
- Default HTTP (not HTTPS) usage and risks
- Credential management best practices
- Network isolation recommendations
- HTTPS configuration example with
secure: trueThis was flagged in a previous review and remains unaddressed.
src/Audit/Audit.php (1)
234-245: Docblock correctly describes datetime parameter.The docblock now correctly states "Delete all logs older than the specified datetime" with
@param \DateTime $datetime, fixing the previous issue where it incorrectly mentioned "seconds".
🧹 Nitpick comments (6)
tests/Audit/AuditBase.php (3)
30-37: Unused variable$cleanupin setUp.The
$cleanupvariable is created and modified but never used. Either remove it or use it for the cleanup call.public function setUp(): void { $this->initializeAudit(); - $cleanup = new \DateTime(); - $cleanup = $cleanup->modify('+10 second'); $this->audit->cleanup(new \DateTime()); $this->createLogs(); }
42-47: Unused variable$cleanupin tearDown.Same issue as setUp - the variable is created but not used.
public function tearDown(): void { - $cleanup = new \DateTime(); - $cleanup = $cleanup->modify('+10 second'); $this->audit->cleanup(new \DateTime()); }
391-419: Unused variable$baseTimeat line 402.The
$baseTimevariable is assigned but never used intestRetrievalParameters. Remove it to clean up the code.public function testRetrievalParameters(): void { // ... - $baseTime = new \DateTime('2024-06-15 12:00:00'); $batchEvents = []; for ($i = 0; $i < 5; $i++) {src/Audit/Adapter.php (1)
64-79: Consider documenting the return type for getByUser in PHPDoc.The
@return array<Log>annotation is present but the detailed PHPDoc could clarify that the array is ordered by time (descending by default, ascending if$ascendingis true).src/Audit/Adapter/Database.php (2)
135-240: Query construction across get/count methods is consistent; consider a small DRY helperAll
get*andcount*methods consistently:
- Use
buildTimeQueries()for the time range.- Add the appropriate equality filters (
userId,resource,event).- Apply ordering, limit, and offset only for the
get*variants.This looks logically sound and easy to follow. Given the repetition of the query array construction, you might consider a small internal helper (e.g.
buildBaseQueries(array $filters, ?\DateTime $after, ?\DateTime $before): array) to centralize filter composition and make future changes (new filters, different sort field) less error-prone. Not mandatory, but it would reduce duplication.Also applies to: 250-368
370-389: Docblock forcleanup()has a stray nested/**The PHPDoc above
cleanup()contains two/**openings but only one closing*/, which will render oddly in generated docs:/** * Delete logs older than the specified datetime. * * @param \DateTime $datetime /** * @throws AuthorizationException|\Exception */Consider merging into a single clean block:
/** * Delete logs older than the specified datetime. * * @param \DateTime $datetime * @throws AuthorizationException|\Exception */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Dockerfile(2 hunks)README.md(3 hunks)src/Audit/Adapter.php(1 hunks)src/Audit/Adapter/ClickHouse.php(1 hunks)src/Audit/Adapter/Database.php(1 hunks)src/Audit/Audit.php(1 hunks)tests/Audit/AuditBase.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/Audit/AuditBase.php (4)
src/Audit/Audit.php (6)
Audit(12-246)cleanup(242-245)log(61-72)getLogsByUser(95-104)countLogsByUser(113-119)logBatch(82-85)src/Audit/Adapter/Database.php (2)
Database(20-424)cleanup(377-389)src/Audit/Adapter/ClickHouse.php (1)
cleanup(1092-1111)src/Audit/Log.php (1)
getId(32-36)
src/Audit/Audit.php (3)
src/Audit/Adapter.php (13)
Adapter(12-205)setup(25-25)create(43-43)createBatch(62-62)getByUser(72-79)countByUser(89-93)getByResource(103-110)countByResource(120-124)getByUserAndEvents(135-143)countByUserAndEvents(154-159)getByResourceAndEvents(170-178)countByResourceAndEvents(189-194)cleanup(204-204)src/Audit/Adapter/Database.php (13)
__construct(24-27)setup(43-61)create(70-78)createBatch(87-97)getByUser(135-160)countByUser(167-182)getByResource(191-216)countByResource(225-240)getByUserAndEvents(250-277)countByUserAndEvents(287-304)getByResourceAndEvents(314-341)countByResourceAndEvents(351-368)cleanup(377-389)src/Audit/Log.php (2)
__construct(22-25)Log(15-209)
src/Audit/Adapter/ClickHouse.php (2)
src/Audit/Adapter/SQL.php (5)
SQL(15-221)getAttributes(43-120)getColumnDefinition(202-202)getIndexes(142-166)getAttribute(184-193)src/Audit/Adapter/Database.php (2)
getColumnDefinition(403-423)Database(20-424)
🪛 GitHub Actions: CodeQL
src/Audit/Adapter/ClickHouse.php
[error] 548-548: Offset 'time' on array{userId?: string|null, event: string, resource: string, userAgent: string, ip: string, location?: string, time: string, data?: array<string, mixed>} on left side of ?? always exists and is not nullable.
🪛 markdownlint-cli2 (0.18.1)
README.md
129-129: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
152-152: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
171-171: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
204-204: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
236-236: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🪛 PHPMD (2.15.0)
tests/Audit/AuditBase.php
402-402: Avoid unused local variables such as '$baseTime'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (17)
Dockerfile (1)
1-1: Composer base image updated to 2.8.Good update to a more recent Composer version for better dependency management.
tests/Audit/AuditBase.php (1)
279-295: Unused variable$baseTime.The
$baseTimevariable on line 283 is assigned but never used. Consider removing it.public function testLargeBatchInsert(): void { // Create a large batch (50 events) $batchEvents = []; - $baseTime = DateTime::now(); + $baseTime = DateTime::now(); // Used for formatting below for ($i = 0; $i < 50; $i++) { $batchEvents[] = [ // ... 'time' => DateTime::formatTz($baseTime) ?? ''Actually,
$baseTimeis used on line 293. The static analysis hint appears to be a false positive for line 402.README.md (1)
324-352: Custom adapter example now correctly uses Log return type.The example has been updated to show
Logas the return type forcreate()and mentions returningLogobjects increateBatch(), which aligns with the actual adapter interface.src/Audit/Adapter.php (1)
12-43: Well-designed adapter interface.The abstract class provides a clean, comprehensive contract for audit storage backends with proper typing, documentation, and a logical method structure covering lifecycle, CRUD, and query operations.
src/Audit/Adapter/ClickHouse.php (6)
55-76: Good constructor design with validation.The constructor properly validates host and port, initializes the HTTP client for connection reuse, and sets up authentication headers. This addresses past concerns about input validation and client reuse.
92-98: Host validation using Hostname validator.Good implementation using
Utopia\Validator\Hostnameas suggested in a previous review.
287-325: Secure parameterized query implementation.The query method now uses ClickHouse's native parameter mechanism (multipart form data with
param_prefix), which handles escaping internally. This properly addresses the SQL injection concerns from previous reviews.
583-606: IDs correctly reused for returned documents.The implementation now stores generated IDs in
$idsarray and uses$ids[$index]when building return documents. This fixes the previously reported bug where returned documents had different IDs than inserted records.
627-632: Column count validation correctly handles sharedTables mode.The expected column count is now dynamically calculated based on
$this->sharedTables, addressing the previous bug where valid rows were incorrectly skipped in non-sharedTables mode.
316-324: Exception chaining preserves debugging context.The exception handling now preserves the original exception as the cause (
$epassed as third argument), which maintains the full stack trace for debugging. This addresses the previous feedback about lost debugging information.src/Audit/Audit.php (4)
14-24: Clean adapter-based architecture.The refactoring to inject an
Adaptervia constructor enables pluggable storage backends while maintaining a simple public API. This is a well-executed application of the adapter pattern.
61-72: Breaking change:log()now returnsLoginstead ofbool.This is a breaking API change. Existing code checking
if ($audit->log(...))will now always evaluate to true (object is truthy). Ensure this is documented in release notes with migration guidance.
82-85: Breaking change:logBatch()now returnsarray<Log>instead ofbool.Same consideration as
log()- document this breaking change for users upgrading.
95-104: API consistently uses\DateTimefor time filtering.All retrieval methods now accept
?\DateTimeparameters for$afterand$beforefilters, providing a type-safe and consistent API across the adapter interface.src/Audit/Adapter/Database.php (3)
24-61: Constructor and setup wiring look consistent with the adapter pattern
$dbis injected and stored once, andsetup()correctly checks for database existence and attempts to create the collection with attributes and indexes while swallowing the duplicate-collection case. No functional issues stand out here.
99-127: Time query helper is clear and correctly handles all combinations
buildTimeQueries()cleanly covers (after & before) viabetween, and falls back togreaterThan/lessThanwhen only one bound is provided, returning an empty array when neither is set. This keeps the call sites simple and avoids duplicated branching logic.
391-423:getColumnDefinition()implementation is reasonable and defensiveThe method correctly:
- Uses
getAttribute($id)and throws if the attribute is missing.- Extracts
typeand optionalsizeand formats a concise description string.Given it’s mainly a required implementation for the base SQL adapter and not actively used at runtime, this is sufficient and clear.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Audit/Adapter.php(1 hunks)
🔇 Additional comments (4)
src/Audit/Adapter.php (4)
1-17: LGTM!The class structure and documentation are well-designed. The abstract adapter pattern provides a clean interface for pluggable audit storage backends.
19-25: LGTM!The setup method provides a clear lifecycle hook for initializing adapter resources.
64-194: LGTM!The query methods provide a comprehensive and well-designed interface:
- Consistent patterns across all query variations
- Proper support for time-based filtering, pagination, and ordering
- Clear separation between retrieval and counting operations
- Strong typing with PHPDoc annotations
196-204: LGTM!The cleanup method provides a straightforward interface for managing log retention with a clear success/failure return value.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/Audit/Adapter/Database.php (2)
87-104: Consider preserving string timestamps increateBatchinstead of always reformatting.Right now every
timevalue is parsed into\DateTimeand re-emitted viaDateTime::format(), even when the caller already supplied a formatted string. That can subtly change the exact representation (e.g. timezone or precision) compared to what was passed in, while tests liketestLogByBatchexpect stored timestamps to match the originals.If you want behavior closer to
create()and to the tests’ expectations of “time preservation”, consider only normalizing when the input is a\DateTimeand leaving strings untouched, and providing a default whentimeis missing, e.g.:- $this->db->getAuthorization()->skip(function () use ($logs, &$created) { - foreach ($logs as $log) { - $time = $log['time'] ?? new \DateTime(); - if (is_string($time)) { - $time = new \DateTime($time); - } - assert($time instanceof \DateTime); - $log['time'] = DateTime::format($time); - $created[] = $this->db->createDocument($this->getCollectionName(), new Document($log)); - } - }); + $this->db->getAuthorization()->skip(function () use ($logs, &$created) { + foreach ($logs as $log) { + $time = $log['time'] ?? null; + + if ($time instanceof \DateTime) { + // Normalize DateTime objects to the database format + $log['time'] = DateTime::format($time); + } elseif ($time === null) { + // Match `create()` behavior: default to "now" if not provided + $log['time'] = DateTime::now(); + } + + $created[] = $this->db->createDocument( + $this->getCollectionName(), + new Document($log) + ); + } + });This keeps existing string timestamps intact, while still giving you consistent normalization for
\DateTimeinputs and a default when omitted.
377-395: Fix malformed PHPDoc forcleanup()The docblock above
cleanup()opens twice (/**on Line 377 and again on Line 381), which can confuse IDEs and doc generators.You can collapse this into a single, well-formed block:
- /** - * Delete logs older than the specified datetime. - * - * @param \DateTime $datetime - /** - * @throws AuthorizationException|\Exception - */ + /** + * Delete logs older than the specified datetime. + * + * @param \DateTime $datetime + * @throws AuthorizationException|\Exception + */The implementation itself looks good: the loop correctly uses the count from
deleteDocumentsto terminate.tests/Audit/AuditBase.php (3)
27-47: Remove or use the unused$cleanupvariable insetUp()/tearDown()In both
setUp()andtearDown()you compute a$cleanupDateTime and modify it by+10 second, but then callcleanup(new \DateTime())instead. The$cleanupvariable is effectively dead code and the+10 secondadjustment has no effect.Either delete the unused variable:
public function setUp(): void { $this->initializeAudit(); - $cleanup = new \DateTime(); - $cleanup = $cleanup->modify('+10 second'); - $this->audit->cleanup(new \DateTime()); + $this->audit->cleanup(new \DateTime()); $this->createLogs(); } public function tearDown(): void { - $cleanup = new \DateTime(); - $cleanup = $cleanup->modify('+10 second'); - $this->audit->cleanup(new \DateTime()); + $this->audit->cleanup(new \DateTime()); }or, if the intention was to clean up relative to a future time, actually pass
$cleanupintocleanup().
354-386:testCleanupis correct but relies on realsleep()delaysFunctionally the cleanup test is sound: it verifies that only logs older than the threshold are removed. However, the three
sleep(5);calls make the test relatively slow.If you find the suite getting sluggish, you could refactor this test to control
timeexplicitly via thetimefield (e.g., inserting logs with synthesized timestamps) instead of relying on wall‑clock sleeps, while keeping the current behavior as the reference.
63-151: Minor duplication in retrieval tests (optional cleanup)In
testGetLogsByUser(),testGetLogsByUserAndEvents(),testGetLogsByResourceAndEvents(), andtestGetLogsByResource(), you call the same method twice with identical parameters (logs1vslogs2,logs3vslogs4) and assert the same properties, e.g.:$logs1 = $this->audit->getLogsByUser('userId', limit: 1, offset: 1); $this->assertEquals(1, \count($logs1)); $this->assertEquals($logs1[0]->getId(), $logs[1]->getId()); $logs2 = $this->audit->getLogsByUser('userId', limit: 1, offset: 1); $this->assertEquals(1, \count($logs2)); $this->assertEquals($logs2[0]->getId(), $logs[1]->getId());If you’re not explicitly testing idempotence, you can drop the duplicate calls to keep the tests shorter and clearer. Behavior doesn’t change either way, so this is just a cleanup suggestion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Audit/Adapter/Database.php(1 hunks)tests/Audit/AuditBase.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/Audit/Adapter/Database.php (2)
src/Audit/Log.php (3)
Log(15-209)__construct(22-25)getArrayCopy(205-208)src/Audit/Adapter/SQL.php (2)
SQL(15-221)getColumnDefinition(202-202)
tests/Audit/AuditBase.php (4)
src/Audit/Audit.php (3)
Audit(12-246)cleanup(242-245)log(61-72)src/Audit/Adapter/Database.php (2)
Database(20-431)cleanup(384-396)src/Audit/Adapter/ClickHouse.php (1)
cleanup(1092-1111)src/Audit/Log.php (1)
getId(32-36)
🪛 PHPMD (2.15.0)
tests/Audit/AuditBase.php
402-402: Avoid unused local variables such as '$baseTime'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (2)
tests/Audit/AuditBase.php (2)
253-260: ConfirmgetLogsByUsernow accepts?\DateTimeforafter
testGetLogsCustomFilters()passes a\DateTimeinstance as theafterargument:$threshold = new \DateTime(); $threshold->modify('-10 seconds'); $logs = $this->audit->getLogsByUser('userId', after: $threshold);Given the adapters’
getByUsersignatures use?\DateTimeforafter/before, this is likely intentional, but it assumes theAudit::getLogsByUserfaçade (and any other adapter implementations) also accept?\DateTimerather than strings.Please double‑check that all public audit APIs and adapters agree on the type for
after/beforeto avoid type errors or silent coercions.
279-311:testLargeBatchInsert: design looks good; single-timestamp batch is fineThe large‑batch test is straightforward and effectively validates batch insert, counting, and pagination behavior. Using the same
timefor all 50 events (viaDateTime::formatTz($baseTime)) is acceptable here since the focus is on pagination, not chronological ordering.No changes needed from my side.
| // Create 5 logs with different timestamps | ||
| $baseTime = new \DateTime('2024-06-15 12:00:00'); | ||
| $batchEvents = []; | ||
| for ($i = 0; $i < 5; $i++) { | ||
| $offset = $i * 60; | ||
| $logTime = new \DateTime('2024-06-15 12:00:00'); | ||
| $logTime->modify("+{$offset} seconds"); | ||
| $timestamp = DateTime::format($logTime); | ||
| $batchEvents[] = [ | ||
| 'userId' => $userId, | ||
| 'event' => 'event_' . $i, | ||
| 'resource' => 'doc/' . $i, | ||
| 'userAgent' => $userAgent, | ||
| 'ip' => $ip, | ||
| 'location' => $location, | ||
| 'data' => ['sequence' => $i], | ||
| 'time' => $timestamp | ||
| ]; | ||
| } |
There was a problem hiding this comment.
Remove unused $baseTime in testRetrievalParameters
In testRetrievalParameters() you declare $baseTime but never use it:
$baseTime = new \DateTime('2024-06-15 12:00:00');
$batchEvents = [];
for ($i = 0; $i < 5; $i++) {
$offset = $i * 60;
$logTime = new \DateTime('2024-06-15 12:00:00');
$logTime->modify("+{$offset} seconds");
$timestamp = DateTime::format($logTime);
// ...
}Static analysis flags this as an unused local. Since you create fresh $logTime instances inside the loop, $baseTime can simply be dropped:
- $baseTime = new \DateTime('2024-06-15 12:00:00');
- $batchEvents = [];
+ $batchEvents = [];This keeps the test logic unchanged while quieting PHPMD.
🧰 Tools
🪛 PHPMD (2.15.0)
402-402: Avoid unused local variables such as '$baseTime'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
In tests/Audit/AuditBase.php around lines 401 to 419, the local variable
$baseTime is declared but never used; remove the line that creates $baseTime
(i.e. delete "$baseTime = new \DateTime('2024-06-15 12:00:00');") so the loop
creates log timestamps as before and the unused variable no longer triggers
static analysis warnings.
Summary by CodeRabbit
New Features
Documentation
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.