Add extended attributes and indexes for ClickHouse adapter #102
Conversation
WalkthroughAdds a fluent Query builder (Utopia\Audit\Query) and extends audit adapters and API to support indexed, attribute-rich ClickHouse queries. New adapter methods: getById(string): ?Log, find(array): array, count(array): int are declared in Adapter and implemented in Database and ClickHouse (with SQL/HTTP query execution, parameter formatting, and query parsing). Audit exposes getLogById, find, and count delegating to the adapter. Tests added for Query, ClickHouse adapter attributes/indexes, and extended Audit find/count/getById behavior. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
…s and parent naming conventions
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @tests/Audit/Adapter/ClickHouseTest.php:
- Around line 323-360: The test testClickHouseAdapterAttributes is missing the
'location' attribute in its $expectedAttributes list and the adapter's creation
path doesn't accept/populate it; add 'location' to the $expectedAttributes array
in testClickHouseAdapterAttributes to assert its presence, and update the
ClickHouse create() (or the parent SQL adapter create() method that handles
audit fields) to accept a 'location' input and populate the corresponding
schema/attribute so the attribute can actually be stored and retrieved via
ClickHouse::getAttributes().
- Around line 365-393: The test testClickHouseAdapterIndexes expects parent
index IDs with `_key_*` names but the parent SQL adapter (class SQL) actually
uses `idx_*` names; update the expectedIndexes array in the test to match the
real parent IDs (replace `_key_event` -> `idx_event`,
`_key_user_internal_and_event` -> `idx_userId_event`, `_key_resource_and_event`
-> `idx_resource_event`, `_key_time` -> `idx_time_desc`) so the assertions
compare against the actual index IDs returned by ClickHouse adapter.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Audit/Adapter/ClickHouse.phptests/Audit/Adapter/ClickHouseTest.php
🧰 Additional context used
🧬 Code graph analysis (2)
src/Audit/Adapter/ClickHouse.php (1)
src/Audit/Adapter/SQL.php (2)
getAttributes(43-120)getIndexes(142-166)
tests/Audit/Adapter/ClickHouseTest.php (1)
src/Audit/Adapter/ClickHouse.php (1)
ClickHouse(17-1295)
🪛 GitHub Actions: Tests
tests/Audit/Adapter/ClickHouseTest.php
[error] 391-391: Command failed: docker compose exec tests vendor/bin/phpunit --configuration phpunit.xml tests. PHPUnit failure: 'Index '_key_event' not found in ClickHouse adapter' at tests/Audit/Adapter/ClickHouseTest.php:391.
🔇 Additional comments (2)
src/Audit/Adapter/ClickHouse.php (2)
7-7: LGTM - Required import for attribute and index constants.The
Databaseclass import is correctly added to support the new attribute and index definitions.
398-455: Verify indexing strategy for resourceType, resourceId, resourceParent, and resourceInternalId attributes.These four attributes are defined but not indexed, while other new attributes have dedicated indexes. If these attributes are frequently used in WHERE clauses or JOIN conditions, they may benefit from indexes. Consider reviewing query patterns to confirm the current indexing strategy is complete.
| public function getAttributes(): array | ||
| { | ||
| $parentAttributes = parent::getAttributes(); | ||
|
|
||
| return [ | ||
| ...$parentAttributes, | ||
| [ | ||
| '$id' => 'userType', | ||
| 'type' => Database::VAR_STRING, | ||
| 'size' => Database::LENGTH_KEY, | ||
| 'required' => true, | ||
| 'default' => null, | ||
| 'signed' => true, | ||
| 'array' => false, | ||
| 'filters' => [], | ||
| ], | ||
| [ | ||
| '$id' => 'userInternalId', | ||
| 'type' => Database::VAR_STRING, | ||
| 'size' => Database::LENGTH_KEY, | ||
| 'required' => false, | ||
| 'default' => null, | ||
| 'signed' => true, | ||
| 'array' => false, | ||
| 'filters' => [], | ||
| ], | ||
| [ | ||
| '$id' => 'resourceParent', | ||
| 'type' => Database::VAR_STRING, | ||
| 'size' => Database::LENGTH_KEY, | ||
| 'required' => false, | ||
| 'default' => null, | ||
| 'signed' => true, | ||
| 'array' => false, | ||
| 'filters' => [], | ||
| ], | ||
| [ | ||
| '$id' => 'resourceType', | ||
| 'type' => Database::VAR_STRING, | ||
| 'size' => Database::LENGTH_KEY, | ||
| 'required' => true, | ||
| 'default' => null, | ||
| 'signed' => true, | ||
| 'array' => false, | ||
| 'filters' => [], | ||
| ], | ||
| [ | ||
| '$id' => 'resourceId', | ||
| 'type' => Database::VAR_STRING, | ||
| 'size' => Database::LENGTH_KEY, | ||
| 'required' => true, | ||
| 'default' => null, | ||
| 'signed' => true, | ||
| 'array' => false, | ||
| 'filters' => [], | ||
| ], | ||
| [ | ||
| '$id' => 'resourceInternalId', | ||
| 'type' => Database::VAR_STRING, | ||
| 'size' => Database::LENGTH_KEY, | ||
| 'required' => false, | ||
| 'default' => null, | ||
| 'signed' => true, | ||
| 'array' => false, | ||
| 'filters' => [], | ||
| ], | ||
| [ | ||
| '$id' => 'country', | ||
| 'type' => Database::VAR_STRING, | ||
| 'size' => Database::LENGTH_KEY, | ||
| 'required' => false, | ||
| 'default' => null, | ||
| 'signed' => true, | ||
| 'array' => false, | ||
| 'filters' => [], | ||
| ], | ||
| [ | ||
| '$id' => 'projectId', | ||
| 'type' => Database::VAR_STRING, | ||
| 'format' => '', | ||
| 'size' => Database::LENGTH_KEY, | ||
| 'signed' => true, | ||
| 'required' => true, | ||
| 'default' => null, | ||
| 'array' => false, | ||
| 'filters' => [], | ||
| ], | ||
| [ | ||
| '$id' => 'projectInternalId', | ||
| 'type' => Database::VAR_STRING, | ||
| 'format' => '', | ||
| 'size' => Database::LENGTH_KEY, | ||
| 'signed' => true, | ||
| 'required' => true, | ||
| 'default' => null, | ||
| 'array' => false, | ||
| 'filters' => [], | ||
| ], | ||
| [ | ||
| '$id' => 'teamId', | ||
| 'type' => Database::VAR_STRING, | ||
| 'format' => '', | ||
| 'size' => Database::LENGTH_KEY, | ||
| 'signed' => true, | ||
| 'required' => true, | ||
| 'default' => null, | ||
| 'array' => false, | ||
| 'filters' => [], | ||
| ], | ||
| [ | ||
| '$id' => 'teamInternalId', | ||
| 'type' => Database::VAR_STRING, | ||
| 'format' => '', | ||
| 'size' => Database::LENGTH_KEY, | ||
| 'signed' => true, | ||
| 'required' => true, | ||
| 'default' => null, | ||
| 'array' => false, | ||
| 'filters' => [], | ||
| ], | ||
| [ | ||
| '$id' => 'hostname', | ||
| 'type' => Database::VAR_STRING, | ||
| 'format' => '', | ||
| 'size' => Database::LENGTH_KEY, | ||
| 'signed' => true, | ||
| 'required' => true, | ||
| 'default' => null, | ||
| 'array' => false, | ||
| 'filters' => [], | ||
| ], | ||
| ]; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l src/Audit/Adapter/ClickHouse.phpRepository: utopia-php/audit
Length of output: 97
🏁 Script executed:
# First, let's find the file and examine its structure
git ls-files src/Audit/Adapter/ClickHouse.phpRepository: utopia-php/audit
Length of output: 92
🏁 Script executed:
# Get the create() method to see what it populates
sed -n '645,702p' src/Audit/Adapter/ClickHouse.phpRepository: utopia-php/audit
Length of output: 2128
🏁 Script executed:
# Get the createBatch() method
sed -n '709,789p' src/Audit/Adapter/ClickHouse.phpRepository: utopia-php/audit
Length of output: 3190
🏁 Script executed:
# Also check the full getAttributes() method to see all attribute definitions
sed -n '258,390p' src/Audit/Adapter/ClickHouse.phpRepository: utopia-php/audit
Length of output: 4443
🏁 Script executed:
# Verify if these new attributes are referenced elsewhere in the file
rg -n 'userType|resourceType|resourceId|projectId|teamId|hostname' src/Audit/Adapter/ClickHouse.php | head -20Repository: utopia-php/audit
Length of output: 549
🏁 Script executed:
# Check if there's a parent getAttributes() method that might define these
rg -B5 -A20 'public function getAttributes' src/Audit/Adapter/ClickHouse.php | head -50Repository: utopia-php/audit
Length of output: 952
🏁 Script executed:
# Check the context around lines 436 and 448-450 to see how these attributes are used
sed -n '430,460p' src/Audit/Adapter/ClickHouse.phpRepository: utopia-php/audit
Length of output: 987
🏁 Script executed:
# Search for any INSERT or UPDATE statements that might use these new attributes
rg -n "INSERT|UPDATE" src/Audit/Adapter/ClickHouse.phpRepository: utopia-php/audit
Length of output: 283
🏁 Script executed:
# Check if there's a schema creation method that uses these attributes
rg -B2 -A10 'CREATE TABLE|getSchema' src/Audit/Adapter/ClickHouse.phpRepository: utopia-php/audit
Length of output: 572
🏁 Script executed:
# Check if the parent class method has any of these attributes
rg -n "extends\|class" src/Audit/Adapter/ClickHouse.php | head -5Repository: utopia-php/audit
Length of output: 42
🏁 Script executed:
# Check parent class and look for Log constructor to understand data flow
rg -n "new Log" src/Audit/Adapter/ClickHouse.phpRepository: utopia-php/audit
Length of output: 147
🏁 Script executed:
# Look at the full file structure to find parent class
head -30 src/Audit/Adapter/ClickHouse.phpRepository: utopia-php/audit
Length of output: 702
🏁 Script executed:
# Find the parent SQL class to understand the parent::getAttributes()
find . -name "SQL.php" -type f | head -5Repository: utopia-php/audit
Length of output: 87
🏁 Script executed:
# Look at the Log class to see what it does with attributes
fd -e php "Log.php" | grep -v node_modulesRepository: utopia-php/audit
Length of output: 77
🏁 Script executed:
# Check if there are any comments or TODOs about these new attributes
rg -n "TODO|FIXME|NOTE|XXX" src/Audit/Adapter/ClickHouse.phpRepository: utopia-php/audit
Length of output: 42
🏁 Script executed:
# Check the parent SQL class getAttributes method
rg -B5 -A50 "public function getAttributes" src/Audit/Adapter/SQL.phpRepository: utopia-php/audit
Length of output: 1760
🏁 Script executed:
# Look at the Log class to understand its structure
cat src/Audit/Log.phpRepository: utopia-php/audit
Length of output: 4329
🏁 Script executed:
# Check if the new attributes are ever populated anywhere in the SQL adapter
rg -n "userType|resourceType|resourceId|projectId|teamId" src/Audit/Adapter/SQL.phpRepository: utopia-php/audit
Length of output: 42
🏁 Script executed:
# Check if there are any tests or examples showing how these new attributes should be populated
fd -e php "test\|example\|spec" | head -10Repository: utopia-php/audit
Length of output: 42
🏁 Script executed:
# Search across the entire repo for usage of these new attributes
rg -n "userType|resourceType|resourceId|projectId|teamId" --type=php | head -20Repository: utopia-php/audit
Length of output: 857
🏁 Script executed:
# Check the create signature and surrounding context to see if there are any clues
sed -n '640,650p' src/Audit/Adapter/ClickHouse.phpRepository: utopia-php/audit
Length of output: 328
🏁 Script executed:
# Check the test file to understand how these attributes are expected to be used
sed -n '330,360p' tests/Audit/Adapter/ClickHouseTest.phpRepository: utopia-php/audit
Length of output: 945
🏁 Script executed:
# Look more broadly at the test to see the full context
sed -n '320,380p' tests/Audit/Adapter/ClickHouseTest.phpRepository: utopia-php/audit
Length of output: 1819
🏁 Script executed:
# Check if there are any other create methods or overrides we might have missed
rg -n "function create|function insert" src/Audit/Adapter/ClickHouse.phpRepository: utopia-php/audit
Length of output: 162
🏁 Script executed:
# Look for any comments or documentation about when these attributes are supposed to be populated
sed -n '250,265p' src/Audit/Adapter/ClickHouse.phpRepository: utopia-php/audit
Length of output: 494
🏁 Script executed:
# Check the entire indexes definition to understand if these are truly required
sed -n '408,460p' src/Audit/Adapter/ClickHouse.phpRepository: utopia-php/audit
Length of output: 1783
Critical: New attributes are never populated in log creation methods.
The new attributes (userType, userInternalId, resourceParent, resourceType, resourceId, resourceInternalId, country, projectId, projectInternalId, teamId, teamInternalId, hostname) are defined in the schema with indexes, but are never populated in the create() (lines 645-702) or createBatch() (lines 709-789) methods. These fields will always remain NULL/empty when audit logs are created, making the indexes on these columns ineffective.
Additionally, 8 attributes have conflicting configuration: 'required' => true combined with 'default' => null. If a field is required, it cannot default to null. This applies to: userType, resourceType, resourceId, projectId, projectInternalId, teamId, teamInternalId, and hostname.
The create() method must be updated to accept and populate these new attributes, and the 'required' flags should be corrected to match the actual schema constraints.
| public function testClickHouseAdapterAttributes(): void | ||
| { | ||
| $adapter = new ClickHouse( | ||
| host: 'clickhouse', | ||
| username: 'default', | ||
| password: 'clickhouse' | ||
| ); | ||
|
|
||
| $attributes = $adapter->getAttributes(); | ||
| $attributeIds = array_map(fn ($attr) => $attr['$id'], $attributes); | ||
|
|
||
| // Verify all expected attributes exist | ||
| $expectedAttributes = [ | ||
| 'userType', | ||
| 'userId', | ||
| 'userInternalId', | ||
| 'resourceParent', | ||
| 'resourceType', | ||
| 'resourceId', | ||
| 'resourceInternalId', | ||
| 'event', | ||
| 'resource', | ||
| 'userAgent', | ||
| 'ip', | ||
| 'country', | ||
| 'time', | ||
| 'data', | ||
| 'projectId', | ||
| 'projectInternalId', | ||
| 'teamId', | ||
| 'teamInternalId', | ||
| 'hostname' | ||
| ]; | ||
|
|
||
| foreach ($expectedAttributes as $expected) { | ||
| $this->assertContains($expected, $attributeIds, "Attribute '{$expected}' not found in ClickHouse adapter"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing 'location' attribute in expected list.
The test validates attribute presence but is missing 'location' from the expected attributes list (line 335-355). The parent SQL adapter defines a 'location' attribute that should be included in the validation.
Additionally, note that this test only validates schema presence, not that these attributes can actually be populated. The underlying issue is that the create() method doesn't accept or populate these new attributes.
✅ Add missing 'location' attribute to expected list
$expectedAttributes = [
'userType',
'userId',
'userInternalId',
'resourceParent',
'resourceType',
'resourceId',
'resourceInternalId',
'event',
'resource',
'userAgent',
'ip',
+ 'location',
'country',
'time',
'data',
'projectId',
'projectInternalId',
'teamId',
'teamInternalId',
'hostname'
];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function testClickHouseAdapterAttributes(): void | |
| { | |
| $adapter = new ClickHouse( | |
| host: 'clickhouse', | |
| username: 'default', | |
| password: 'clickhouse' | |
| ); | |
| $attributes = $adapter->getAttributes(); | |
| $attributeIds = array_map(fn ($attr) => $attr['$id'], $attributes); | |
| // Verify all expected attributes exist | |
| $expectedAttributes = [ | |
| 'userType', | |
| 'userId', | |
| 'userInternalId', | |
| 'resourceParent', | |
| 'resourceType', | |
| 'resourceId', | |
| 'resourceInternalId', | |
| 'event', | |
| 'resource', | |
| 'userAgent', | |
| 'ip', | |
| 'country', | |
| 'time', | |
| 'data', | |
| 'projectId', | |
| 'projectInternalId', | |
| 'teamId', | |
| 'teamInternalId', | |
| 'hostname' | |
| ]; | |
| foreach ($expectedAttributes as $expected) { | |
| $this->assertContains($expected, $attributeIds, "Attribute '{$expected}' not found in ClickHouse adapter"); | |
| } | |
| } | |
| public function testClickHouseAdapterAttributes(): void | |
| { | |
| $adapter = new ClickHouse( | |
| host: 'clickhouse', | |
| username: 'default', | |
| password: 'clickhouse' | |
| ); | |
| $attributes = $adapter->getAttributes(); | |
| $attributeIds = array_map(fn ($attr) => $attr['$id'], $attributes); | |
| // Verify all expected attributes exist | |
| $expectedAttributes = [ | |
| 'userType', | |
| 'userId', | |
| 'userInternalId', | |
| 'resourceParent', | |
| 'resourceType', | |
| 'resourceId', | |
| 'resourceInternalId', | |
| 'event', | |
| 'resource', | |
| 'userAgent', | |
| 'ip', | |
| 'location', | |
| 'country', | |
| 'time', | |
| 'data', | |
| 'projectId', | |
| 'projectInternalId', | |
| 'teamId', | |
| 'teamInternalId', | |
| 'hostname' | |
| ]; | |
| foreach ($expectedAttributes as $expected) { | |
| $this->assertContains($expected, $attributeIds, "Attribute '{$expected}' not found in ClickHouse adapter"); | |
| } | |
| } |
🤖 Prompt for AI Agents
In @tests/Audit/Adapter/ClickHouseTest.php around lines 323 - 360, The test
testClickHouseAdapterAttributes is missing the 'location' attribute in its
$expectedAttributes list and the adapter's creation path doesn't accept/populate
it; add 'location' to the $expectedAttributes array in
testClickHouseAdapterAttributes to assert its presence, and update the
ClickHouse create() (or the parent SQL adapter create() method that handles
audit fields) to accept a 'location' input and populate the corresponding
schema/attribute so the attribute can actually be stored and retrieved via
ClickHouse::getAttributes().
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/Audit/Adapter/ClickHouseTest.php (2)
323-360: Consider asserting “missing attributes” in one shot (clearer failures, less noise).Current loop works, but a single
array_diff-based assertion typically produces cleaner failure output and fewer assertions; alsoarray_column($attributes, '$id')is simpler thanarray_map.Proposed tweak
- $attributes = $adapter->getAttributes(); - $attributeIds = array_map(fn ($attr) => $attr['$id'], $attributes); + $attributes = $adapter->getAttributes(); + $attributeIds = array_column($attributes, '$id'); // Verify all expected attributes exist $expectedAttributes = [ 'userType', 'userId', 'userInternalId', 'resourceParent', 'resourceType', 'resourceId', 'resourceInternalId', 'event', 'resource', 'userAgent', 'ip', 'country', 'time', 'data', 'projectId', 'projectInternalId', 'teamId', 'teamInternalId', 'hostname' ]; - foreach ($expectedAttributes as $expected) { - $this->assertContains($expected, $attributeIds, "Attribute '{$expected}' not found in ClickHouse adapter"); - } + $missing = array_diff($expectedAttributes, $attributeIds); + $this->assertEmpty($missing, 'Missing attributes: ' . implode(', ', $missing));
365-396: Index coverage looks good; same “single missing set” assertion could simplify (and note: parent index IDs are hard-coded).The ClickHouse-specific index assertions align with the PR objective. If the parent
idx_*IDs aren’t a stable contract, this portion may be the first to become brittle on upstream schema changes.Proposed tweak (also uses
array_column)- $indexes = $adapter->getIndexes(); - $indexIds = array_map(fn ($idx) => $idx['$id'], $indexes); + $indexes = $adapter->getIndexes(); + $indexIds = array_column($indexes, '$id'); // Verify all ClickHouse-specific indexes exist $expectedClickHouseIndexes = [ '_key_user_internal_and_event', '_key_project_internal_id', '_key_team_internal_id', '_key_user_internal_id', '_key_user_type', '_key_country', '_key_hostname' ]; - foreach ($expectedClickHouseIndexes as $expected) { - $this->assertContains($expected, $indexIds, "ClickHouse index '{$expected}' not found in ClickHouse adapter"); - } + $missingClickHouse = array_diff($expectedClickHouseIndexes, $indexIds); + $this->assertEmpty($missingClickHouse, 'Missing ClickHouse indexes: ' . implode(', ', $missingClickHouse)); // Verify parent indexes are also included (with parent naming convention) $parentExpectedIndexes = ['idx_event', 'idx_userId_event', 'idx_resource_event', 'idx_time_desc']; - foreach ($parentExpectedIndexes as $expected) { - $this->assertContains($expected, $indexIds, "Parent index '{$expected}' not found in ClickHouse adapter"); - } + $missingParent = array_diff($parentExpectedIndexes, $indexIds); + $this->assertEmpty($missingParent, 'Missing parent indexes: ' . implode(', ', $missingParent));
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/Audit/Adapter/ClickHouseTest.php
🧰 Additional context used
🧬 Code graph analysis (1)
tests/Audit/Adapter/ClickHouseTest.php (1)
src/Audit/Adapter/ClickHouse.php (1)
ClickHouse(17-1295)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Audit/Adapter.phpsrc/Audit/Adapter/ClickHouse.phpsrc/Audit/Adapter/Database.phpsrc/Audit/Audit.phptests/Audit/AuditBase.php
🧰 Additional context used
🧬 Code graph analysis (5)
src/Audit/Audit.php (4)
src/Audit/Log.php (1)
Log(15-209)src/Audit/Adapter.php (1)
getById(35-35)src/Audit/Adapter/ClickHouse.php (1)
getById(711-729)src/Audit/Adapter/Database.php (1)
getById(112-127)
tests/Audit/AuditBase.php (2)
src/Audit/Audit.php (2)
log(61-72)getLogById(95-98)src/Audit/Log.php (1)
getId(32-36)
src/Audit/Adapter.php (3)
src/Audit/Adapter/ClickHouse.php (1)
getById(711-729)src/Audit/Adapter/Database.php (1)
getById(112-127)src/Audit/Log.php (1)
Log(15-209)
src/Audit/Adapter/Database.php (3)
src/Audit/Adapter.php (1)
getById(35-35)src/Audit/Adapter/ClickHouse.php (1)
getById(711-729)src/Audit/Log.php (2)
Log(15-209)getArrayCopy(205-208)
src/Audit/Adapter/ClickHouse.php (4)
src/Audit/Adapter/Database.php (2)
Database(20-454)getById(112-127)src/Audit/Adapter/SQL.php (2)
getAttributes(43-120)getIndexes(142-166)src/Audit/Adapter.php (1)
getById(35-35)src/Audit/Log.php (1)
Log(15-209)
🔇 Additional comments (7)
src/Audit/Audit.php (1)
87-98: LGTM!The delegation to the adapter is correct and the method signature aligns with the adapter contract.
src/Audit/Adapter.php (1)
27-35: LGTM!The abstract method declaration establishes a clear contract for retrieving a single log by ID across all adapter implementations.
src/Audit/Adapter/Database.php (1)
105-127: LGTM!The implementation follows the established patterns in this adapter: authorization skip, empty document handling, and consistent exception handling that returns null.
tests/Audit/AuditBase.php (1)
158-186: LGTM!The test provides comprehensive coverage of the getLogById functionality, including validation of all log fields and proper handling of non-existent IDs.
src/Audit/Adapter/ClickHouse.php (3)
7-7: LGTM!The Database import is necessary for the constants used in getAttributes() and getIndexes() methods.
392-455: LGTM!The extended indexes are well-structured for query optimization. The composite index on userInternalId and event, along with single-column indexes on the new attributes, will support efficient filtering and retrieval.
704-729: LGTM!The getById implementation correctly follows the ClickHouse adapter patterns: parameterized queries for safety, tenant filtering for multi-tenancy support, and appropriate result parsing.
| /** | ||
| * Override getAttributes to provide extended attributes for ClickHouse. | ||
| * Includes existing attributes from parent and adds new missing ones. | ||
| * | ||
| * @return array<int, array<string, mixed>> | ||
| */ | ||
| public function getAttributes(): array | ||
| { | ||
| $parentAttributes = parent::getAttributes(); | ||
|
|
||
| return [ | ||
| ...$parentAttributes, | ||
| [ | ||
| '$id' => 'userType', | ||
| 'type' => Database::VAR_STRING, | ||
| 'size' => Database::LENGTH_KEY, | ||
| 'required' => true, | ||
| 'default' => null, | ||
| 'signed' => true, | ||
| 'array' => false, | ||
| 'filters' => [], | ||
| ], | ||
| [ | ||
| '$id' => 'userInternalId', | ||
| 'type' => Database::VAR_STRING, | ||
| 'size' => Database::LENGTH_KEY, | ||
| 'required' => false, | ||
| 'default' => null, | ||
| 'signed' => true, | ||
| 'array' => false, | ||
| 'filters' => [], | ||
| ], | ||
| [ | ||
| '$id' => 'resourceParent', | ||
| 'type' => Database::VAR_STRING, | ||
| 'size' => Database::LENGTH_KEY, | ||
| 'required' => false, | ||
| 'default' => null, | ||
| 'signed' => true, | ||
| 'array' => false, | ||
| 'filters' => [], | ||
| ], | ||
| [ | ||
| '$id' => 'resourceType', | ||
| 'type' => Database::VAR_STRING, | ||
| 'size' => Database::LENGTH_KEY, | ||
| 'required' => true, | ||
| 'default' => null, | ||
| 'signed' => true, | ||
| 'array' => false, | ||
| 'filters' => [], | ||
| ], | ||
| [ | ||
| '$id' => 'resourceId', | ||
| 'type' => Database::VAR_STRING, | ||
| 'size' => Database::LENGTH_KEY, | ||
| 'required' => true, | ||
| 'default' => null, | ||
| 'signed' => true, | ||
| 'array' => false, | ||
| 'filters' => [], | ||
| ], | ||
| [ | ||
| '$id' => 'resourceInternalId', | ||
| 'type' => Database::VAR_STRING, | ||
| 'size' => Database::LENGTH_KEY, | ||
| 'required' => false, | ||
| 'default' => null, | ||
| 'signed' => true, | ||
| 'array' => false, | ||
| 'filters' => [], | ||
| ], | ||
| [ | ||
| '$id' => 'country', | ||
| 'type' => Database::VAR_STRING, | ||
| 'size' => Database::LENGTH_KEY, | ||
| 'required' => false, | ||
| 'default' => null, | ||
| 'signed' => true, | ||
| 'array' => false, | ||
| 'filters' => [], | ||
| ], | ||
| [ | ||
| '$id' => 'projectId', | ||
| 'type' => Database::VAR_STRING, | ||
| 'format' => '', | ||
| 'size' => Database::LENGTH_KEY, | ||
| 'signed' => true, | ||
| 'required' => true, | ||
| 'default' => null, | ||
| 'array' => false, | ||
| 'filters' => [], | ||
| ], | ||
| [ | ||
| '$id' => 'projectInternalId', | ||
| 'type' => Database::VAR_STRING, | ||
| 'format' => '', | ||
| 'size' => Database::LENGTH_KEY, | ||
| 'signed' => true, | ||
| 'required' => true, | ||
| 'default' => null, | ||
| 'array' => false, | ||
| 'filters' => [], | ||
| ], | ||
| [ | ||
| '$id' => 'teamId', | ||
| 'type' => Database::VAR_STRING, | ||
| 'format' => '', | ||
| 'size' => Database::LENGTH_KEY, | ||
| 'signed' => true, | ||
| 'required' => true, | ||
| 'default' => null, | ||
| 'array' => false, | ||
| 'filters' => [], | ||
| ], | ||
| [ | ||
| '$id' => 'teamInternalId', | ||
| 'type' => Database::VAR_STRING, | ||
| 'format' => '', | ||
| 'size' => Database::LENGTH_KEY, | ||
| 'signed' => true, | ||
| 'required' => true, | ||
| 'default' => null, | ||
| 'array' => false, | ||
| 'filters' => [], | ||
| ], | ||
| [ | ||
| '$id' => 'hostname', | ||
| 'type' => Database::VAR_STRING, | ||
| 'format' => '', | ||
| 'size' => Database::LENGTH_KEY, | ||
| 'signed' => true, | ||
| 'required' => true, | ||
| 'default' => null, | ||
| 'array' => false, | ||
| 'filters' => [], | ||
| ], | ||
| ]; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for usage of the new attribute fields
echo "=== Searching for usage of new attribute fields ==="
for field in userType userInternalId resourceParent resourceType resourceId resourceInternalId country projectId projectInternalId teamId teamInternalId hostname; do
echo "--- Searching for: $field ---"
rg -n -C2 "\b$field\b" --type=php -g '!Test.php' | head -20
doneRepository: utopia-php/audit
Length of output: 11609
🏁 Script executed:
#!/bin/bash
# Examine the create() and createBatch() methods in the ClickHouse adapter
echo "=== Examining create() and createBatch() methods ==="
rg -n -A30 "public function (create|createBatch)" src/Audit/Adapter/ClickHouse.phpRepository: utopia-php/audit
Length of output: 2753
🏁 Script executed:
#!/bin/bash
# Check parent class getAttributes() method
echo "=== Searching for parent class and its getAttributes() ==="
rg -n "class ClickHouse" src/Audit/Adapter/ClickHouse.php | head -5Repository: utopia-php/audit
Length of output: 150
🏁 Script executed:
#!/bin/bash
# Look for the parent class definition
rg -n "extends" src/Audit/Adapter/ClickHouse.phpRepository: utopia-php/audit
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Search for where these attributes might be set during initialization
echo "=== Searching for setAttribute or attribute population ==="
rg -n "(setAttribute|setUserType|setResourceType|setProjectId|setTeamId)" src/Audit/Adapter/ClickHouse.phpRepository: utopia-php/audit
Length of output: 118
Fix contradictory field definitions and populate required attributes in create methods.
Multiple attributes are marked as required: true but have default: null, which is contradictory. Required fields should not have null defaults. Additionally, these 12 new attributes (userType, userInternalId, resourceParent, resourceType, resourceId, resourceInternalId, country, projectId, projectInternalId, teamId, teamInternalId, hostname) are never populated in the create() or createBatch() methods, causing ClickHouse INSERT operations to fail since required fields have no values.
Either:
- Change
requiredtofalsefor optional attributes and provide appropriate defaults for ClickHouse - If these fields are required, populate them in
create()andcreateBatch()from the caller's input or application context
The attributes are already indexed, indicating they should be populated and available for queries.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @src/Audit/Adapter/ClickHouse.php:
- Around line 744-759: The code accesses $parsed['select'] (which parseQueries
never returns) and uses null-coalescing on $parsed['filters'] even though
filters always exists; modify the logic in the ClickHouse adapter to always use
$this->getSelectColumns() instead of $parsed['select'] and read filters directly
from $parsed['filters'] (e.g., $conditions = $parsed['filters'];) before
appending the tenant filter from $this->getTenantFilter(); if you plan to
support selectable columns later, update parseQueries to include a 'select'
return entry and the query TYPE_SELECT handling instead of referencing 'select'
here.
- Around line 851-859: Query::TYPE_LIMIT and Query::TYPE_OFFSET handlers cast
$values[0] (a mixed) directly to int which PHPStan flags; update both branches
to safely extract and validate the value before casting: check isset($values[0])
and that it is numeric (or use is_int/is_string checks), then cast to int,
otherwise use a sensible default (e.g. 0), and assign to $params['limit'] and
$params['offset'] respectively; reference the Query::TYPE_LIMIT and
Query::TYPE_OFFSET cases and the $values/$params variables when making the
change.
🧹 Nitpick comments (2)
src/Audit/Query.php (2)
235-254: Consider validating method against known query types.The
parseQuerymethod validates thatmethod,attribute, andvalueshave correct types, but doesn't verify thatmethodis one of the definedTYPE_*constants. While the ClickHouse adapter silently skips unknown methods, explicit validation here would fail fast and help debug malformed queries.♻️ Optional improvement
protected static function parseQuery(array $query): self { $method = $query['method'] ?? ''; $attribute = $query['attribute'] ?? ''; $values = $query['values'] ?? []; if (!\is_string($method)) { throw new \Exception('Invalid query method. Must be a string, got ' . \gettype($method)); } + $validMethods = [ + self::TYPE_EQUAL, + self::TYPE_GREATER, + self::TYPE_LESSER, + self::TYPE_BETWEEN, + self::TYPE_IN, + self::TYPE_ORDER_DESC, + self::TYPE_ORDER_ASC, + self::TYPE_LIMIT, + self::TYPE_OFFSET, + ]; + + if (!in_array($method, $validMethods, true)) { + throw new \Exception('Unknown query method: ' . $method); + } + if (!\is_string($attribute)) { throw new \Exception('Invalid query attribute. Must be a string, got ' . \gettype($attribute)); }
5-10: Consider updating class docblock for broader applicability.The docblock states this is "for ClickHouse Audit adapter", but the class is generic and could be used by any adapter implementing query parsing. Consider updating the description.
/** - * Query class for ClickHouse Audit adapter + * Query class for Audit adapters * - * Provides a fluent interface for building ClickHouse queries. + * Provides a fluent interface for building audit log queries. * Contains only methods needed for audit log operations. */
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Audit/Adapter/ClickHouse.phpsrc/Audit/Query.php
🧰 Additional context used
🧬 Code graph analysis (1)
src/Audit/Query.php (1)
src/Audit/Adapter/ClickHouse.php (3)
__construct(57-78)query(494-532)parseQueries(788-881)
🪛 GitHub Actions: CodeQL
src/Audit/Adapter/ClickHouse.php
[error] 748-748: PHPStan: Offset 'select' on a non-existent offset in the array structure. See log line for details.
[error] 754-754: PHPStan: Offset 'filters' on array{filters: array, params: array<string, mixed>, orderBy?: array, limit?: int, offset?: int} on left side of ?? always exists and is not nullable.
[error] 852-852: PHPStan: Cannot cast mixed to int.
[error] 857-857: PHPStan: Cannot cast mixed to int.
🔇 Additional comments (8)
src/Audit/Adapter/ClickHouse.php (4)
253-391: LGTM - Extended attributes are well-defined.The attribute definitions follow the established pattern from the parent class, with appropriate type definitions and
requiredflags. The spread operator usage to merge with parent attributes is clean.
393-456: LGTM - Index definitions are consistent.The new indexes align with the extended attributes and follow the same structure as parent indexes. Using
bloom_filteris appropriate for ClickHouse string-based filtering.
705-730: LGTM -getByIdimplementation is correct.The method properly applies tenant filtering, uses parameterized queries to prevent SQL injection, and returns
nullwhen no result is found.
7-8: LGTM - Imports are appropriate.The new imports for
QueryandDatabaseare correctly added for the extended functionality.src/Audit/Query.php (4)
11-48: LGTM - Class structure and constructor are well-designed.The Query class follows a clean builder pattern with proper encapsulation. The constructor accepts flexible parameters for different query types.
50-81: LGTM - Accessor methods are straightforward.The getters provide appropriate access to internal state.
83-186: LGTM - Factory methods provide a clean fluent API.The static factory methods (
equal,lessThan,greaterThan,between,in,orderDesc,orderAsc,limit,offset) offer a type-safe way to construct queries, reducing the chance of typos in method names.
256-287: LGTM - Serialization methods are correct.The
toArrayandtoStringmethods properly handle JSON encoding with error handling viaJSON_THROW_ON_ERROR.
| switch ($method) { | ||
| case Query::TYPE_EQUAL: | ||
| $paramName = 'param_' . $paramCounter++; | ||
| $filters[] = "{$attribute} = {{$paramName}:String}"; | ||
| $params[$paramName] = $this->formatParamValue($values[0]); | ||
| break; | ||
|
|
||
| case Query::TYPE_LESSER: | ||
| $paramName = 'param_' . $paramCounter++; | ||
| $filters[] = "{$attribute} < {{$paramName}:String}"; | ||
| $params[$paramName] = $this->formatParamValue($values[0]); | ||
| break; | ||
|
|
||
| case Query::TYPE_GREATER: | ||
| $paramName = 'param_' . $paramCounter++; | ||
| $filters[] = "{$attribute} > {{$paramName}:String}"; | ||
| $params[$paramName] = $this->formatParamValue($values[0]); | ||
| break; | ||
|
|
||
| case Query::TYPE_BETWEEN: | ||
| $paramName1 = 'param_' . $paramCounter++; | ||
| $paramName2 = 'param_' . $paramCounter++; | ||
| $filters[] = "{$attribute} BETWEEN {{$paramName1}:String} AND {{$paramName2}:String}"; | ||
| $params[$paramName1] = $this->formatParamValue($values[0]); | ||
| $params[$paramName2] = $this->formatParamValue($values[1]); | ||
| break; | ||
|
|
||
| case Query::TYPE_IN: | ||
| $inParams = []; | ||
| foreach ($values as $value) { | ||
| $paramName = 'param_' . $paramCounter++; | ||
| $inParams[] = "{{$paramName}:String}"; | ||
| $params[$paramName] = $this->formatParamValue($value); | ||
| } | ||
| $filters[] = "{$attribute} IN (" . implode(', ', $inParams) . ")"; | ||
| break; | ||
|
|
||
| case Query::TYPE_ORDER_DESC: | ||
| $orderBy[] = "{$attribute} DESC"; | ||
| break; | ||
|
|
||
| case Query::TYPE_ORDER_ASC: | ||
| $orderBy[] = "{$attribute} ASC"; | ||
| break; |
There was a problem hiding this comment.
SQL injection risk: Attribute names are interpolated without validation.
The $attribute value from Query::getAttribute() is directly interpolated into SQL fragments (e.g., "{$attribute} = {{$paramName}:String}"). While parameter values use ClickHouse's safe parameter binding, column/attribute names are not validated or escaped.
If user input can influence the attribute name (e.g., via Query::parse() from external JSON), this could allow SQL injection.
🔧 Suggested mitigation
Either validate against an allowlist of known attributes:
private function validateAttribute(string $attribute): void
{
$validAttributes = array_column($this->getAttributes(), '$id');
$validAttributes[] = 'id'; // Add 'id' column
if (!in_array($attribute, $validAttributes, true)) {
throw new Exception("Invalid attribute: {$attribute}");
}
}Or escape the identifier before interpolation:
case Query::TYPE_EQUAL:
$paramName = 'param_' . $paramCounter++;
- $filters[] = "{$attribute} = {{$paramName}:String}";
+ $escapedAttr = $this->escapeIdentifier($attribute);
+ $filters[] = "{$escapedAttr} = {{$paramName}:String}";
$params[$paramName] = $this->formatParamValue($values[0]);
break;Apply similar escaping to all cases that use $attribute.
| case Query::TYPE_ORDER_DESC: | ||
| $orderBy[] = "{$attribute} DESC"; | ||
| break; | ||
|
|
||
| case Query::TYPE_ORDER_ASC: | ||
| $orderBy[] = "{$attribute} ASC"; | ||
| break; |
There was a problem hiding this comment.
ORDER BY clause also susceptible to attribute injection.
Same concern as the filter attributes - $attribute is interpolated directly into the ORDER BY clause without validation or escaping.
Apply the same mitigation (allowlist validation or identifier escaping) as suggested for filter attributes.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @tests/Audit/QueryTest.php:
- Around line 105-123: The test testQueryParseQueries uses a JSON query with
"method":"greaterThan" which doesn't match the Query::TYPE_GREATER constant
expected by assertions; update the input JSON in testQueryParseQueries to use
the canonical method name ("greater") or adjust Query::parseQueries to map
"greaterThan" to Query::TYPE_GREATER; specifically, change the JSON entry in the
$queries array from "method":"greaterThan" to "method":"greater" (or add mapping
logic in Query::parseQueries) so the parsed object's getMethod() equals
Query::TYPE_GREATER.
🧹 Nitpick comments (1)
tests/Audit/Adapter/ClickHouseTest.php (1)
399-613: Comprehensive query test coverage.These tests effectively validate the find method with various query types (equal, multiple filters, IN, ordering, limit/offset). The test data is well-isolated and assertions are appropriate.
Note: Line 537 uses
sleep(1)to ensure different timestamps. While this works, it slows down tests. Consider using microsecond precision or mocking time if test performance becomes a concern.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/Audit/Adapter/ClickHouseTest.phptests/Audit/QueryTest.php
🧰 Additional context used
🧬 Code graph analysis (1)
tests/Audit/QueryTest.php (1)
src/Audit/Query.php (16)
Query(11-288)equal(90-93)getMethod(53-56)getValues(69-72)lessThan(102-105)greaterThan(114-117)between(127-130)in(139-142)orderDesc(150-153)orderAsc(161-164)limit(172-175)offset(183-186)parse(195-208)toString(280-287)toArray(261-272)getValue(78-81)
🪛 GitHub Actions: Tests
tests/Audit/QueryTest.php
[error] 121-121: Failed asserting that two strings are equal. Expected 'greater', Actual 'greaterThan'.
🔇 Additional comments (8)
tests/Audit/Adapter/ClickHouseTest.php (3)
9-9: LGTM!The import of the
Queryclass is necessary for the new find test methods that use Query objects for building queries.
321-397: LGTM!These tests provide good coverage for verifying that the ClickHouse adapter exposes the correct attributes and indexes. The use of descriptive error messages in assertions will help diagnose issues if the tests fail.
615-669: LGTM!Good use of
logBatchwith explicit timestamps to test the between query functionality. The test correctly validates time-based filtering.tests/Audit/QueryTest.php (5)
13-68: LGTM!Comprehensive test coverage of all Query factory methods. The assertions correctly verify the method, attribute, and values for each query type.
73-100: LGTM!Good test coverage for serialization and deserialization. The round-trip testing (create → toString → parse → verify) ensures consistency.
128-154: LGTM!Good coverage of the
getValuemethod, including testing with default values and queries that have empty values.
159-209: LGTM!Excellent coverage of error scenarios with appropriate exception messages. These tests ensure that invalid input is properly rejected with clear error messages.
214-224: LGTM!Good test for complex values with multiple elements. The round-trip verification ensures that multi-value queries (like between) serialize and deserialize correctly.
| public function testQueryParseQueries(): void | ||
| { | ||
| $queries = [ | ||
| '{"method":"equal","attribute":"userId","values":["123"]}', | ||
| '{"method":"greaterThan","attribute":"time","values":["2023-01-01"]}', | ||
| '{"method":"limit","values":[10]}' | ||
| ]; | ||
|
|
||
| $parsed = Query::parseQueries($queries); | ||
|
|
||
| $this->assertCount(3, $parsed); | ||
| $this->assertInstanceOf(Query::class, $parsed[0]); | ||
| $this->assertInstanceOf(Query::class, $parsed[1]); | ||
| $this->assertInstanceOf(Query::class, $parsed[2]); | ||
|
|
||
| $this->assertEquals(Query::TYPE_EQUAL, $parsed[0]->getMethod()); | ||
| $this->assertEquals(Query::TYPE_GREATER, $parsed[1]->getMethod()); | ||
| $this->assertEquals(Query::TYPE_LIMIT, $parsed[2]->getMethod()); | ||
| } |
There was a problem hiding this comment.
Critical: Fix method name mismatch causing pipeline failure.
The pipeline failure indicates that line 121 expects 'greater' but gets 'greaterThan'. The issue is at line 109 where the JSON contains "method":"greaterThan", but the Query::TYPE_GREATER constant is likely defined as 'greater'.
🐛 Proposed fix
$queries = [
'{"method":"equal","attribute":"userId","values":["123"]}',
- '{"method":"greaterThan","attribute":"time","values":["2023-01-01"]}',
+ '{"method":"greater","attribute":"time","values":["2023-01-01"]}',
'{"method":"limit","values":[10]}'
];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function testQueryParseQueries(): void | |
| { | |
| $queries = [ | |
| '{"method":"equal","attribute":"userId","values":["123"]}', | |
| '{"method":"greaterThan","attribute":"time","values":["2023-01-01"]}', | |
| '{"method":"limit","values":[10]}' | |
| ]; | |
| $parsed = Query::parseQueries($queries); | |
| $this->assertCount(3, $parsed); | |
| $this->assertInstanceOf(Query::class, $parsed[0]); | |
| $this->assertInstanceOf(Query::class, $parsed[1]); | |
| $this->assertInstanceOf(Query::class, $parsed[2]); | |
| $this->assertEquals(Query::TYPE_EQUAL, $parsed[0]->getMethod()); | |
| $this->assertEquals(Query::TYPE_GREATER, $parsed[1]->getMethod()); | |
| $this->assertEquals(Query::TYPE_LIMIT, $parsed[2]->getMethod()); | |
| } | |
| public function testQueryParseQueries(): void | |
| { | |
| $queries = [ | |
| '{"method":"equal","attribute":"userId","values":["123"]}', | |
| '{"method":"greater","attribute":"time","values":["2023-01-01"]}', | |
| '{"method":"limit","values":[10]}' | |
| ]; | |
| $parsed = Query::parseQueries($queries); | |
| $this->assertCount(3, $parsed); | |
| $this->assertInstanceOf(Query::class, $parsed[0]); | |
| $this->assertInstanceOf(Query::class, $parsed[1]); | |
| $this->assertInstanceOf(Query::class, $parsed[2]); | |
| $this->assertEquals(Query::TYPE_EQUAL, $parsed[0]->getMethod()); | |
| $this->assertEquals(Query::TYPE_GREATER, $parsed[1]->getMethod()); | |
| $this->assertEquals(Query::TYPE_LIMIT, $parsed[2]->getMethod()); | |
| } |
🧰 Tools
🪛 GitHub Actions: Tests
[error] 121-121: Failed asserting that two strings are equal. Expected 'greater', Actual 'greaterThan'.
🤖 Prompt for AI Agents
In @tests/Audit/QueryTest.php around lines 105 - 123, The test
testQueryParseQueries uses a JSON query with "method":"greaterThan" which
doesn't match the Query::TYPE_GREATER constant expected by assertions; update
the input JSON in testQueryParseQueries to use the canonical method name
("greater") or adjust Query::parseQueries to map "greaterThan" to
Query::TYPE_GREATER; specifically, change the JSON entry in the $queries array
from "method":"greaterThan" to "method":"greater" (or add mapping logic in
Query::parseQueries) so the parsed object's getMethod() equals
Query::TYPE_GREATER.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Audit/Adapter/ClickHouse.php (1)
646-702: Missing required fields will cause INSERT failures:userType,resourceType,resourceId,projectId,projectInternalId,teamId,teamInternalId, andhostnameare declared asrequired=trueingetAttributes(), which creates NOT NULL columns in the table schema, butcreate()never populates them.The
setup()method builds columns fromgetAttributes()and applies NOT NULL constraints to allrequired=truefields (line 1208). However,create()only inserts the 7 base fields (userId, event, resource, userAgent, ip, location, data) plus id and time. ClickHouse will reject these INSERTs due to missing required columns.Either:
- Add these fields to the
create()method and accept them in the public API- Change these attributes to
required=false- Provide default values in the column definition
🤖 Fix all issues with AI agents
In @src/Audit/Adapter/ClickHouse.php:
- Around line 893-901: The direct casts of $values[0] to int in the
Query::TYPE_LIMIT and Query::TYPE_OFFSET branches are unsafe because $values is
mixed; validate and normalize the value before assigning to $params: check that
isset($values[0]) and that it is an int or numeric string (use
is_int/is_numeric), then cast to int; if validation fails, either set a safe
default (e.g., 0) or throw a clear exception. Update the logic around the LIMIT
and OFFSET case blocks (referencing Query::TYPE_LIMIT, Query::TYPE_OFFSET,
$values, $limit, $offset, and $params) to perform the validation/coercion and
handle invalid input accordingly.
- Around line 746-758: The code reads $parsed['select'] and uses
$parsed['filters'] with a redundant null-coalescing despite parseQueries() never
populating 'select' and always returning 'filters' => []; update the logic in
the SELECT/WHERE build in ClickHouse.php to stop relying on $parsed['select']
(use getSelectColumns() exclusively or implement select support inside
parseQueries()), and simplify the filters handling by assigning $conditions =
$parsed['filters'] (remove the ?? []), keeping the tenant merge via
getTenantFilter() and the final implode(' AND ', $conditions) as-is.
- Around line 798-804: The code uses a redundant null-coalescing operator for
$parsed['filters'] even though parseQueries() always defines that key; in the
block that builds $conditions (inside the method referencing getTenantFilter()),
replace the usage of $parsed['filters'] ?? [] with the guaranteed
$parsed['filters'] (e.g., set $conditions = $parsed['filters']), keep the
tenantFilter append logic as-is, and ensure the resulting $whereClause
construction remains unchanged; mirror the same fix you applied in find() so
there’s no unnecessary ?? operator.
In @tests/Audit/Adapter/ClickHouseTest.php:
- Around line 865-916: The timestamps in testCountWithBetweenQuery include a
timezone suffix (+00:00) which is incompatible with ClickHouse DateTime64(3);
update the three variables ($time1, $time2, $time3) and the Query::between call
to use timestamps without the timezone suffix (e.g. '2023-01-01 00:00:00',
'2023-06-01 00:00:00', '2023-12-31 23:59:59' and between('time', '2023-05-01
00:00:00', '2023-12-31 00:00:00')), leaving the rest of
testCountWithBetweenQuery and logBatch data unchanged so ClickHouse accepts the
DateTime64(3) values.
- Around line 618-669: The test testFindWithBetweenQuery is using timestamps
with a +00:00 timezone suffix which ClickHouse DateTime64(3) cannot parse;
update the three $time variables passed to logBatch and the Query::between
bounds to use ClickHouse-compatible format (e.g., "YYYY-MM-DD HH:MM:SS" or
"YYYY-MM-DD HH:MM:SS.mmm" without any timezone suffix) so ClickHouse can ingest
and query them; modify the timestamps in testFindWithBetweenQuery (and the
Query::between call) to remove "+00:00" and optionally include milliseconds like
".000" to match DateTime64(3).
🧹 Nitpick comments (1)
tests/Audit/Adapter/ClickHouseTest.php (1)
537-537: Consider using explicit timestamps instead ofsleep(1)for faster tests.Using
sleep(1)slows down the test suite. Consider usinglogBatchwith explicit timestamps for deterministic ordering without delay.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Audit/Adapter/ClickHouse.phptests/Audit/Adapter/ClickHouseTest.php
🧰 Additional context used
🧬 Code graph analysis (2)
tests/Audit/Adapter/ClickHouseTest.php (3)
src/Audit/Audit.php (4)
Audit(12-259)log(61-72)getAdapter(31-34)logBatch(82-85)src/Audit/Query.php (8)
Query(11-288)equal(90-93)in(139-142)orderAsc(161-164)orderDesc(150-153)limit(172-175)offset(183-186)between(127-130)src/Audit/Adapter/ClickHouse.php (5)
ClickHouse(18-1516)getAttributes(259-391)getIndexes(399-456)find(739-779)count(788-821)
src/Audit/Adapter/ClickHouse.php (4)
src/Audit/Query.php (7)
Query(11-288)parseQueries(217-226)limit(172-175)offset(183-186)getMethod(53-56)getAttribute(61-64)getValues(69-72)src/Audit/Adapter/SQL.php (3)
getAttributes(43-120)getIndexes(142-166)getAttribute(184-193)src/Audit/Adapter.php (1)
getById(35-35)src/Audit/Log.php (2)
Log(15-209)getAttribute(157-160)
🪛 GitHub Actions: CodeQL
src/Audit/Adapter/ClickHouse.php
[error] 748-748: Offset 'select' on array{filters: array, params: array<string, mixed>, orderBy?: array, limit?: int, offset?: int} on left side of ?? does not exist.
[error] 754-754: Offset 'filters' on array{filters: array, params: array<string, mixed>, orderBy?: array, limit?: int, offset?: int} on left side of ?? always exists and is not nullable.
[error] 800-800: Offset 'filters' on array{filters: array, params: array<string, mixed>, orderBy?: array, limit?: int, offset?: int} on left side of ?? always exists and is not nullable.
[error] 894-894: Cannot cast mixed to int.
[error] 899-899: Cannot cast mixed to int.
🪛 GitHub Actions: Tests
tests/Audit/Adapter/ClickHouseTest.php
[error] 662-662: Caused by: ClickHouse query failed with HTTP 400: TYPE_MISMATCH due to converting '+00:00' to DateTime64(3).
[error] 909-909: Caused by: ClickHouse query failed with HTTP 400: TYPE_MISMATCH due to converting '+00:00' to DateTime64(3).
src/Audit/Adapter/ClickHouse.php
[error] 526-526: ClickHouse query execution failed: Cannot convert string '+00:00' to DateTime64(3) during greaterOrEquals comparison. (TYPE_MISMATCH)
[error] 518-518: ClickHouse query execution failed: Cannot convert string '+00:00' to DateTime64(3) during greaterOrEquals comparison. (TYPE_MISMATCH)
🔇 Additional comments (6)
tests/Audit/Adapter/ClickHouseTest.php (2)
324-361: LGTM!The attribute verification test is well-structured, checking that all extended attributes from the ClickHouse adapter are present.
366-397: LGTM!The index verification test correctly validates both ClickHouse-specific indexes and inherited parent indexes.
src/Audit/Adapter/ClickHouse.php (4)
7-8: LGTM!Imports are correctly added for the new Query class and Database constants used in attribute/index definitions.
259-391: LGTM!The extended attribute definitions follow the parent schema pattern correctly and add all necessary ClickHouse-specific fields.
399-456: LGTM!Index definitions properly extend parent indexes and add appropriate single and composite indexes for the new attributes.
712-730: LGTM!The
getByIdmethod correctly uses parameterized queries and handles tenant filtering.
|
|
||
| // Build SELECT clause | ||
| $selectColumns = $parsed['select'] ?? $this->getSelectColumns(); | ||
|
|
||
| // Build WHERE clause | ||
| $whereClause = ''; | ||
| $tenantFilter = $this->getTenantFilter(); | ||
| if (!empty($parsed['filters']) || $tenantFilter) { | ||
| $conditions = $parsed['filters'] ?? []; | ||
| if ($tenantFilter) { | ||
| $conditions[] = ltrim($tenantFilter, ' AND'); | ||
| } | ||
| $whereClause = ' WHERE ' . implode(' AND ', $conditions); |
There was a problem hiding this comment.
Fix static analysis issues in query parsing.
CodeQL reports two issues:
$parsed['select'](line 748) is accessed but never populated byparseQueries()- either remove this or addselectsupport toparseQueries().$parsed['filters'] ?? [](line 754) is redundant sinceparseQueries()always returns'filters' => [].
🔧 Proposed fix
// Parse queries
$parsed = $this->parseQueries($queries);
// Build SELECT clause
- $selectColumns = $parsed['select'] ?? $this->getSelectColumns();
+ $selectColumns = $this->getSelectColumns();
// Build WHERE clause
$whereClause = '';
$tenantFilter = $this->getTenantFilter();
if (!empty($parsed['filters']) || $tenantFilter) {
- $conditions = $parsed['filters'] ?? [];
+ $conditions = $parsed['filters'];
if ($tenantFilter) {
$conditions[] = ltrim($tenantFilter, ' AND');
}
$whereClause = ' WHERE ' . implode(' AND ', $conditions);
}🧰 Tools
🪛 GitHub Actions: CodeQL
[error] 748-748: Offset 'select' on array{filters: array, params: array<string, mixed>, orderBy?: array, limit?: int, offset?: int} on left side of ?? does not exist.
[error] 754-754: Offset 'filters' on array{filters: array, params: array<string, mixed>, orderBy?: array, limit?: int, offset?: int} on left side of ?? always exists and is not nullable.
🤖 Prompt for AI Agents
In @src/Audit/Adapter/ClickHouse.php around lines 746 - 758, The code reads
$parsed['select'] and uses $parsed['filters'] with a redundant null-coalescing
despite parseQueries() never populating 'select' and always returning 'filters'
=> []; update the logic in the SELECT/WHERE build in ClickHouse.php to stop
relying on $parsed['select'] (use getSelectColumns() exclusively or implement
select support inside parseQueries()), and simplify the filters handling by
assigning $conditions = $parsed['filters'] (remove the ?? []), keeping the
tenant merge via getTenantFilter() and the final implode(' AND ', $conditions)
as-is.
| $tenantFilter = $this->getTenantFilter(); | ||
| if (!empty($parsed['filters']) || $tenantFilter) { | ||
| $conditions = $parsed['filters'] ?? []; | ||
| if ($tenantFilter) { | ||
| $conditions[] = ltrim($tenantFilter, ' AND'); | ||
| } | ||
| $whereClause = ' WHERE ' . implode(' AND ', $conditions); |
There was a problem hiding this comment.
Redundant null-coalescing operator.
Same issue as in find() – $parsed['filters'] is always defined by parseQueries().
🔧 Proposed fix
if (!empty($parsed['filters']) || $tenantFilter) {
- $conditions = $parsed['filters'] ?? [];
+ $conditions = $parsed['filters'];
if ($tenantFilter) {
$conditions[] = ltrim($tenantFilter, ' AND');
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $tenantFilter = $this->getTenantFilter(); | |
| if (!empty($parsed['filters']) || $tenantFilter) { | |
| $conditions = $parsed['filters'] ?? []; | |
| if ($tenantFilter) { | |
| $conditions[] = ltrim($tenantFilter, ' AND'); | |
| } | |
| $whereClause = ' WHERE ' . implode(' AND ', $conditions); | |
| $tenantFilter = $this->getTenantFilter(); | |
| if (!empty($parsed['filters']) || $tenantFilter) { | |
| $conditions = $parsed['filters']; | |
| if ($tenantFilter) { | |
| $conditions[] = ltrim($tenantFilter, ' AND'); | |
| } | |
| $whereClause = ' WHERE ' . implode(' AND ', $conditions); |
🧰 Tools
🪛 GitHub Actions: CodeQL
[error] 800-800: Offset 'filters' on array{filters: array, params: array<string, mixed>, orderBy?: array, limit?: int, offset?: int} on left side of ?? always exists and is not nullable.
🤖 Prompt for AI Agents
In @src/Audit/Adapter/ClickHouse.php around lines 798 - 804, The code uses a
redundant null-coalescing operator for $parsed['filters'] even though
parseQueries() always defines that key; in the block that builds $conditions
(inside the method referencing getTenantFilter()), replace the usage of
$parsed['filters'] ?? [] with the guaranteed $parsed['filters'] (e.g., set
$conditions = $parsed['filters']), keep the tenantFilter append logic as-is, and
ensure the resulting $whereClause construction remains unchanged; mirror the
same fix you applied in find() so there’s no unnecessary ?? operator.
| case Query::TYPE_BETWEEN: | ||
| $paramName1 = 'param_' . $paramCounter++; | ||
| $paramName2 = 'param_' . $paramCounter++; | ||
| $filters[] = "{$attribute} BETWEEN {{$paramName1}:String} AND {{$paramName2}:String}"; | ||
| $params[$paramName1] = $this->formatParamValue($values[0]); | ||
| $params[$paramName2] = $this->formatParamValue($values[1]); | ||
| break; |
There was a problem hiding this comment.
DateTime64 type mismatch in BETWEEN queries.
The pipeline fails when comparing the time column (DateTime64) with String-typed parameters containing timezone suffixes. ClickHouse cannot implicitly convert '+00:00' suffixed strings to DateTime64.
Consider using explicit DateTime64 casting for the time attribute or stripping timezone suffixes before comparison.
🐛 Proposed fix: Use DateTime64 type for time attribute
case Query::TYPE_BETWEEN:
$paramName1 = 'param_' . $paramCounter++;
$paramName2 = 'param_' . $paramCounter++;
- $filters[] = "{$attribute} BETWEEN {{$paramName1}:String} AND {{$paramName2}:String}";
- $params[$paramName1] = $this->formatParamValue($values[0]);
- $params[$paramName2] = $this->formatParamValue($values[1]);
+ // Use DateTime64 type for time column, String for others
+ $paramType = ($attribute === 'time') ? 'DateTime64(3)' : 'String';
+ $filters[] = "{$attribute} BETWEEN {{$paramName1}:{$paramType}} AND {{$paramName2}:{$paramType}}";
+ $params[$paramName1] = $this->formatDateTimeParam($values[0]);
+ $params[$paramName2] = $this->formatDateTimeParam($values[1]);
break;Add helper method to format datetime values:
private function formatDateTimeParam(mixed $value): string
{
$strValue = $this->formatParamValue($value);
// Remove timezone suffix if present (e.g., +00:00)
return preg_replace('/[+-]\d{2}:\d{2}$/', '', $strValue) ?? $strValue;
}| case Query::TYPE_LIMIT: | ||
| $limit = (int) $values[0]; | ||
| $params['limit'] = $limit; | ||
| break; | ||
|
|
||
| case Query::TYPE_OFFSET: | ||
| $offset = (int) $values[0]; | ||
| $params['offset'] = $offset; | ||
| break; |
There was a problem hiding this comment.
Unsafe cast from mixed to int.
CodeQL reports that casting $values[0] (type mixed) directly to int is not type-safe. Add validation or explicit type coercion.
🔧 Proposed fix
case Query::TYPE_LIMIT:
- $limit = (int) $values[0];
+ $limit = isset($values[0]) && is_numeric($values[0]) ? (int) $values[0] : 25;
$params['limit'] = $limit;
break;
case Query::TYPE_OFFSET:
- $offset = (int) $values[0];
+ $offset = isset($values[0]) && is_numeric($values[0]) ? (int) $values[0] : 0;
$params['offset'] = $offset;
break;🧰 Tools
🪛 GitHub Actions: CodeQL
[error] 894-894: Cannot cast mixed to int.
[error] 899-899: Cannot cast mixed to int.
🤖 Prompt for AI Agents
In @src/Audit/Adapter/ClickHouse.php around lines 893 - 901, The direct casts of
$values[0] to int in the Query::TYPE_LIMIT and Query::TYPE_OFFSET branches are
unsafe because $values is mixed; validate and normalize the value before
assigning to $params: check that isset($values[0]) and that it is an int or
numeric string (use is_int/is_numeric), then cast to int; if validation fails,
either set a safe default (e.g., 0) or throw a clear exception. Update the logic
around the LIMIT and OFFSET case blocks (referencing Query::TYPE_LIMIT,
Query::TYPE_OFFSET, $values, $limit, $offset, and $params) to perform the
validation/coercion and handle invalid input accordingly.
| public function testFindWithBetweenQuery(): void | ||
| { | ||
| $time1 = '2023-01-01 00:00:00+00:00'; | ||
| $time2 = '2023-06-01 00:00:00+00:00'; | ||
| $time3 = '2023-12-31 23:59:59+00:00'; | ||
|
|
||
| // Create test data with different times using logBatch | ||
| $this->audit->logBatch([ | ||
| [ | ||
| 'userId' => 'betweenUser', | ||
| 'event' => 'event1', | ||
| 'resource' => 'test', | ||
| 'userAgent' => 'Test Agent', | ||
| 'ip' => '127.0.0.1', | ||
| 'location' => 'US', | ||
| 'data' => [], | ||
| 'time' => $time1 | ||
| ], | ||
| [ | ||
| 'userId' => 'betweenUser', | ||
| 'event' => 'event2', | ||
| 'resource' => 'test', | ||
| 'userAgent' => 'Test Agent', | ||
| 'ip' => '127.0.0.1', | ||
| 'location' => 'US', | ||
| 'data' => [], | ||
| 'time' => $time2 | ||
| ], | ||
| [ | ||
| 'userId' => 'betweenUser', | ||
| 'event' => 'event3', | ||
| 'resource' => 'test', | ||
| 'userAgent' => 'Test Agent', | ||
| 'ip' => '127.0.0.1', | ||
| 'location' => 'US', | ||
| 'data' => [], | ||
| 'time' => $time3 | ||
| ] | ||
| ]); | ||
|
|
||
| /** @var ClickHouse $adapter */ | ||
| $adapter = $this->audit->getAdapter(); | ||
|
|
||
| // Test between query | ||
| $logs = $adapter->find([ | ||
| Query::equal('userId', 'betweenUser'), | ||
| Query::between('time', '2023-05-01 00:00:00+00:00', '2023-12-31 00:00:00+00:00') | ||
| ]); | ||
|
|
||
| $this->assertIsArray($logs); | ||
| $this->assertGreaterThan(0, count($logs)); | ||
| } |
There was a problem hiding this comment.
Fix datetime format for ClickHouse compatibility.
The pipeline is failing because ClickHouse's DateTime64(3) type cannot parse the +00:00 timezone suffix. ClickHouse expects timestamps in 'YYYY-MM-DD HH:MM:SS.mmm' format without timezone suffix, as the column metadata handles timezone.
🐛 Proposed fix: Remove timezone suffix from test timestamps
public function testFindWithBetweenQuery(): void
{
- $time1 = '2023-01-01 00:00:00+00:00';
- $time2 = '2023-06-01 00:00:00+00:00';
- $time3 = '2023-12-31 23:59:59+00:00';
+ $time1 = '2023-01-01 00:00:00.000';
+ $time2 = '2023-06-01 00:00:00.000';
+ $time3 = '2023-12-31 23:59:59.999';
// ... rest of test setup ...
// Test between query
$logs = $adapter->find([
Query::equal('userId', 'betweenUser'),
- Query::between('time', '2023-05-01 00:00:00+00:00', '2023-12-31 00:00:00+00:00')
+ Query::between('time', '2023-05-01 00:00:00.000', '2023-12-31 00:00:00.000')
]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function testFindWithBetweenQuery(): void | |
| { | |
| $time1 = '2023-01-01 00:00:00+00:00'; | |
| $time2 = '2023-06-01 00:00:00+00:00'; | |
| $time3 = '2023-12-31 23:59:59+00:00'; | |
| // Create test data with different times using logBatch | |
| $this->audit->logBatch([ | |
| [ | |
| 'userId' => 'betweenUser', | |
| 'event' => 'event1', | |
| 'resource' => 'test', | |
| 'userAgent' => 'Test Agent', | |
| 'ip' => '127.0.0.1', | |
| 'location' => 'US', | |
| 'data' => [], | |
| 'time' => $time1 | |
| ], | |
| [ | |
| 'userId' => 'betweenUser', | |
| 'event' => 'event2', | |
| 'resource' => 'test', | |
| 'userAgent' => 'Test Agent', | |
| 'ip' => '127.0.0.1', | |
| 'location' => 'US', | |
| 'data' => [], | |
| 'time' => $time2 | |
| ], | |
| [ | |
| 'userId' => 'betweenUser', | |
| 'event' => 'event3', | |
| 'resource' => 'test', | |
| 'userAgent' => 'Test Agent', | |
| 'ip' => '127.0.0.1', | |
| 'location' => 'US', | |
| 'data' => [], | |
| 'time' => $time3 | |
| ] | |
| ]); | |
| /** @var ClickHouse $adapter */ | |
| $adapter = $this->audit->getAdapter(); | |
| // Test between query | |
| $logs = $adapter->find([ | |
| Query::equal('userId', 'betweenUser'), | |
| Query::between('time', '2023-05-01 00:00:00+00:00', '2023-12-31 00:00:00+00:00') | |
| ]); | |
| $this->assertIsArray($logs); | |
| $this->assertGreaterThan(0, count($logs)); | |
| } | |
| public function testFindWithBetweenQuery(): void | |
| { | |
| $time1 = '2023-01-01 00:00:00.000'; | |
| $time2 = '2023-06-01 00:00:00.000'; | |
| $time3 = '2023-12-31 23:59:59.999'; | |
| // Create test data with different times using logBatch | |
| $this->audit->logBatch([ | |
| [ | |
| 'userId' => 'betweenUser', | |
| 'event' => 'event1', | |
| 'resource' => 'test', | |
| 'userAgent' => 'Test Agent', | |
| 'ip' => '127.0.0.1', | |
| 'location' => 'US', | |
| 'data' => [], | |
| 'time' => $time1 | |
| ], | |
| [ | |
| 'userId' => 'betweenUser', | |
| 'event' => 'event2', | |
| 'resource' => 'test', | |
| 'userAgent' => 'Test Agent', | |
| 'ip' => '127.0.0.1', | |
| 'location' => 'US', | |
| 'data' => [], | |
| 'time' => $time2 | |
| ], | |
| [ | |
| 'userId' => 'betweenUser', | |
| 'event' => 'event3', | |
| 'resource' => 'test', | |
| 'userAgent' => 'Test Agent', | |
| 'ip' => '127.0.0.1', | |
| 'location' => 'US', | |
| 'data' => [], | |
| 'time' => $time3 | |
| ] | |
| ]); | |
| /** @var ClickHouse $adapter */ | |
| $adapter = $this->audit->getAdapter(); | |
| // Test between query | |
| $logs = $adapter->find([ | |
| Query::equal('userId', 'betweenUser'), | |
| Query::between('time', '2023-05-01 00:00:00.000', '2023-12-31 00:00:00.000') | |
| ]); | |
| $this->assertIsArray($logs); | |
| $this->assertGreaterThan(0, count($logs)); | |
| } |
🧰 Tools
🪛 GitHub Actions: Tests
[error] 662-662: Caused by: ClickHouse query failed with HTTP 400: TYPE_MISMATCH due to converting '+00:00' to DateTime64(3).
🤖 Prompt for AI Agents
In @tests/Audit/Adapter/ClickHouseTest.php around lines 618 - 669, The test
testFindWithBetweenQuery is using timestamps with a +00:00 timezone suffix which
ClickHouse DateTime64(3) cannot parse; update the three $time variables passed
to logBatch and the Query::between bounds to use ClickHouse-compatible format
(e.g., "YYYY-MM-DD HH:MM:SS" or "YYYY-MM-DD HH:MM:SS.mmm" without any timezone
suffix) so ClickHouse can ingest and query them; modify the timestamps in
testFindWithBetweenQuery (and the Query::between call) to remove "+00:00" and
optionally include milliseconds like ".000" to match DateTime64(3).
| public function testCountWithBetweenQuery(): void | ||
| { | ||
| $time1 = '2023-01-01 00:00:00+00:00'; | ||
| $time2 = '2023-06-01 00:00:00+00:00'; | ||
| $time3 = '2023-12-31 23:59:59+00:00'; | ||
|
|
||
| // Create test data with different times using logBatch | ||
| $this->audit->logBatch([ | ||
| [ | ||
| 'userId' => 'betweenCountUser', | ||
| 'event' => 'event1', | ||
| 'resource' => 'test', | ||
| 'userAgent' => 'Test Agent', | ||
| 'ip' => '127.0.0.1', | ||
| 'location' => 'US', | ||
| 'data' => [], | ||
| 'time' => $time1 | ||
| ], | ||
| [ | ||
| 'userId' => 'betweenCountUser', | ||
| 'event' => 'event2', | ||
| 'resource' => 'test', | ||
| 'userAgent' => 'Test Agent', | ||
| 'ip' => '127.0.0.1', | ||
| 'location' => 'US', | ||
| 'data' => [], | ||
| 'time' => $time2 | ||
| ], | ||
| [ | ||
| 'userId' => 'betweenCountUser', | ||
| 'event' => 'event3', | ||
| 'resource' => 'test', | ||
| 'userAgent' => 'Test Agent', | ||
| 'ip' => '127.0.0.1', | ||
| 'location' => 'US', | ||
| 'data' => [], | ||
| 'time' => $time3 | ||
| ] | ||
| ]); | ||
|
|
||
| /** @var ClickHouse $adapter */ | ||
| $adapter = $this->audit->getAdapter(); | ||
|
|
||
| // Count with between query | ||
| $count = $adapter->count([ | ||
| Query::equal('userId', 'betweenCountUser'), | ||
| Query::between('time', '2023-05-01 00:00:00+00:00', '2023-12-31 00:00:00+00:00') | ||
| ]); | ||
|
|
||
| $this->assertIsInt($count); | ||
| $this->assertGreaterThan(0, $count); | ||
| } |
There was a problem hiding this comment.
Fix datetime format for ClickHouse compatibility.
Same issue as testFindWithBetweenQuery – the timezone suffix +00:00 is incompatible with ClickHouse's DateTime64(3).
🐛 Proposed fix: Remove timezone suffix from test timestamps
public function testCountWithBetweenQuery(): void
{
- $time1 = '2023-01-01 00:00:00+00:00';
- $time2 = '2023-06-01 00:00:00+00:00';
- $time3 = '2023-12-31 23:59:59+00:00';
+ $time1 = '2023-01-01 00:00:00.000';
+ $time2 = '2023-06-01 00:00:00.000';
+ $time3 = '2023-12-31 23:59:59.999';
// ... rest of test setup ...
// Count with between query
$count = $adapter->count([
Query::equal('userId', 'betweenCountUser'),
- Query::between('time', '2023-05-01 00:00:00+00:00', '2023-12-31 00:00:00+00:00')
+ Query::between('time', '2023-05-01 00:00:00.000', '2023-12-31 00:00:00.000')
]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function testCountWithBetweenQuery(): void | |
| { | |
| $time1 = '2023-01-01 00:00:00+00:00'; | |
| $time2 = '2023-06-01 00:00:00+00:00'; | |
| $time3 = '2023-12-31 23:59:59+00:00'; | |
| // Create test data with different times using logBatch | |
| $this->audit->logBatch([ | |
| [ | |
| 'userId' => 'betweenCountUser', | |
| 'event' => 'event1', | |
| 'resource' => 'test', | |
| 'userAgent' => 'Test Agent', | |
| 'ip' => '127.0.0.1', | |
| 'location' => 'US', | |
| 'data' => [], | |
| 'time' => $time1 | |
| ], | |
| [ | |
| 'userId' => 'betweenCountUser', | |
| 'event' => 'event2', | |
| 'resource' => 'test', | |
| 'userAgent' => 'Test Agent', | |
| 'ip' => '127.0.0.1', | |
| 'location' => 'US', | |
| 'data' => [], | |
| 'time' => $time2 | |
| ], | |
| [ | |
| 'userId' => 'betweenCountUser', | |
| 'event' => 'event3', | |
| 'resource' => 'test', | |
| 'userAgent' => 'Test Agent', | |
| 'ip' => '127.0.0.1', | |
| 'location' => 'US', | |
| 'data' => [], | |
| 'time' => $time3 | |
| ] | |
| ]); | |
| /** @var ClickHouse $adapter */ | |
| $adapter = $this->audit->getAdapter(); | |
| // Count with between query | |
| $count = $adapter->count([ | |
| Query::equal('userId', 'betweenCountUser'), | |
| Query::between('time', '2023-05-01 00:00:00+00:00', '2023-12-31 00:00:00+00:00') | |
| ]); | |
| $this->assertIsInt($count); | |
| $this->assertGreaterThan(0, $count); | |
| } | |
| public function testCountWithBetweenQuery(): void | |
| { | |
| $time1 = '2023-01-01 00:00:00.000'; | |
| $time2 = '2023-06-01 00:00:00.000'; | |
| $time3 = '2023-12-31 23:59:59.999'; | |
| // Create test data with different times using logBatch | |
| $this->audit->logBatch([ | |
| [ | |
| 'userId' => 'betweenCountUser', | |
| 'event' => 'event1', | |
| 'resource' => 'test', | |
| 'userAgent' => 'Test Agent', | |
| 'ip' => '127.0.0.1', | |
| 'location' => 'US', | |
| 'data' => [], | |
| 'time' => $time1 | |
| ], | |
| [ | |
| 'userId' => 'betweenCountUser', | |
| 'event' => 'event2', | |
| 'resource' => 'test', | |
| 'userAgent' => 'Test Agent', | |
| 'ip' => '127.0.0.1', | |
| 'location' => 'US', | |
| 'data' => [], | |
| 'time' => $time2 | |
| ], | |
| [ | |
| 'userId' => 'betweenCountUser', | |
| 'event' => 'event3', | |
| 'resource' => 'test', | |
| 'userAgent' => 'Test Agent', | |
| 'ip' => '127.0.0.1', | |
| 'location' => 'US', | |
| 'data' => [], | |
| 'time' => $time3 | |
| ] | |
| ]); | |
| /** @var ClickHouse $adapter */ | |
| $adapter = $this->audit->getAdapter(); | |
| // Count with between query | |
| $count = $adapter->count([ | |
| Query::equal('userId', 'betweenCountUser'), | |
| Query::between('time', '2023-05-01 00:00:00.000', '2023-12-31 00:00:00.000') | |
| ]); | |
| $this->assertIsInt($count); | |
| $this->assertGreaterThan(0, $count); | |
| } |
🧰 Tools
🪛 GitHub Actions: Tests
[error] 909-909: Caused by: ClickHouse query failed with HTTP 400: TYPE_MISMATCH due to converting '+00:00' to DateTime64(3).
🤖 Prompt for AI Agents
In @tests/Audit/Adapter/ClickHouseTest.php around lines 865 - 916, The
timestamps in testCountWithBetweenQuery include a timezone suffix (+00:00) which
is incompatible with ClickHouse DateTime64(3); update the three variables
($time1, $time2, $time3) and the Query::between call to use timestamps without
the timezone suffix (e.g. '2023-01-01 00:00:00', '2023-06-01 00:00:00',
'2023-12-31 23:59:59' and between('time', '2023-05-01 00:00:00', '2023-12-31
00:00:00')), leaving the rest of testCountWithBetweenQuery and logBatch data
unchanged so ClickHouse accepts the DateTime64(3) values.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @tests/Audit/AuditBase.php:
- Around line 590-674: testFind() assumes data that createLogs() doesn't create
(expects 5 logs for userId 'userId', and resources/events like 'doc/0',
'event_0', 'event_1' that are created in testRetrievalParameters()), causing
failures because setUp() calls cleanup() then createLogs() before each test; fix
by either updating assertions in testFind() to match createLogs() (expect 3 logs
and use existing resource/event values like 'database/document/1', 'update',
'delete'), or add explicit setup inside testFind() to create the missing
logs/resources/events (call the same creation logic used in createLogs() or
replicate the entries) so the filters (equal('userId', ...),
equal('resource','doc/0'), in('event', [...]), between/greaterThan/lessThan on
'time') operate on known data.
🧹 Nitpick comments (2)
src/Audit/Adapter/Database.php (2)
105-127: Docblock claims@throwsbut implementation swallows all exceptions.The method catches all exceptions and returns
null, but the docblock still declares@throws AuthorizationException|\Exception. This is misleading for callers. Consider either:
- Removing the
@throwsannotation since exceptions are never propagated, or- Re-throwing specific exceptions that callers should handle
♻️ Suggested fix: Remove throws annotation
/** * Get a single log by its ID. * * @param string $id * @return Log|null The log entry or null if not found - * @throws AuthorizationException|\Exception */ public function getById(string $id): ?Log
498-524: Consider also skipping order queries in count operations.The implementation correctly skips
limitandoffsetqueries since they don't affect count results. For consistency and potential performance optimization, consider also skippingorderAscandorderDescqueries:♻️ Suggested improvement
// Skip limit and offset for count queries $method = $query->getMethod(); - if ($method === \Utopia\Audit\Query::TYPE_LIMIT || $method === \Utopia\Audit\Query::TYPE_OFFSET) { + if (in_array($method, [ + \Utopia\Audit\Query::TYPE_LIMIT, + \Utopia\Audit\Query::TYPE_OFFSET, + \Utopia\Audit\Query::TYPE_ORDER_ASC, + \Utopia\Audit\Query::TYPE_ORDER_DESC, + ], true)) { continue; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Audit/Adapter.phpsrc/Audit/Adapter/Database.phpsrc/Audit/Audit.phptests/Audit/Adapter/ClickHouseTest.phptests/Audit/AuditBase.php
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Audit/Adapter/ClickHouseTest.php
🧰 Additional context used
🧬 Code graph analysis (3)
src/Audit/Audit.php (3)
src/Audit/Adapter.php (3)
getById(35-35)find(224-224)count(234-234)src/Audit/Adapter/Database.php (3)
getById(112-127)find(464-486)count(498-524)src/Audit/Adapter/ClickHouse.php (3)
getById(712-730)find(739-779)count(788-821)
src/Audit/Adapter.php (3)
src/Audit/Adapter/Database.php (3)
getById(112-127)find(464-486)count(498-524)src/Audit/Adapter/ClickHouse.php (3)
getById(712-730)find(739-779)count(788-821)src/Audit/Audit.php (2)
find(268-271)count(281-284)
src/Audit/Adapter/Database.php (5)
src/Audit/Adapter.php (3)
getById(35-35)find(224-224)count(234-234)src/Audit/Adapter/ClickHouse.php (4)
getById(712-730)find(739-779)query(494-532)count(788-821)src/Audit/Log.php (2)
Log(15-209)getArrayCopy(205-208)src/Audit/Audit.php (3)
find(268-271)Audit(12-285)count(281-284)src/Audit/Query.php (3)
Query(11-288)parseQuery(235-254)toArray(261-272)
🪛 GitHub Actions: Tests
tests/Audit/AuditBase.php
[error] 598-598: Test failed: Utopia\Tests\Audit\Adapter\ClickHouseTest::testFind - Failed asserting that 3 matches expected 5. Step: docker compose exec tests vendor/bin/phpunit --configuration phpunit.xml tests
[error] 684-684: Test failed: Utopia\Tests\Audit\Adapter\ClickHouseTest::testCount - Failed asserting that 3 matches expected 5. Step: docker compose exec tests vendor/bin/phpunit --configuration phpunit.xml tests
[error] 598-598: Test failed: Utopia\Tests\Audit\Adapter\DatabaseTest::testFind - Failed asserting that 3 matches expected 5. Step: docker compose exec tests vendor/bin/phpunit --configuration phpunit.xml tests
[error] 684-684: Test failed: Utopia\Tests\Audit\Adapter\DatabaseTest::testCount - Failed asserting that 3 matches expected 5. Step: docker compose exec tests vendor/bin/phpunit --configuration phpunit.xml tests
🔇 Additional comments (6)
src/Audit/Adapter.php (2)
27-35: LGTM!The
getByIdabstract method is well-defined with appropriate return type and exception documentation. This aligns with the concrete implementations in Database and ClickHouse adapters.
216-234: LGTM!The
findandcountabstract methods are properly declared with typed docblocks specifyingarray<\Utopia\Audit\Query>as the parameter type. The contract is clear and aligns with the adapter implementations.src/Audit/Adapter/Database.php (1)
454-486: LGTM!The
findimplementation correctly validates query types and convertsAudit\Queryobjects toDatabase\Queryobjects using the shared array structure. The use of authorization skip ensures consistent behavior with other adapter methods.src/Audit/Audit.php (2)
87-98: LGTM!Clean delegation to the adapter with proper documentation.
259-284: LGTM!The
findandcountmethods correctly delegate to the adapter with proper type annotations in the docblocks.tests/Audit/AuditBase.php (1)
158-186: LGTM!The
testGetLogByIdtest properly validates:
- Creating a log and retrieving it by ID
- Verifying all attributes are correctly persisted
- Testing the null return for non-existent IDs
Good comprehensive coverage of the new method.
| public function testFind(): void | ||
| { | ||
| $userId = 'userId'; | ||
|
|
||
| // Test 1: Find with equal filter | ||
| $logs = $this->audit->find([ | ||
| \Utopia\Audit\Query::equal('userId', $userId), | ||
| ]); | ||
| $this->assertEquals(5, \count($logs)); | ||
|
|
||
| // Test 2: Find with equal and limit | ||
| $logs = $this->audit->find([ | ||
| \Utopia\Audit\Query::equal('userId', $userId), | ||
| \Utopia\Audit\Query::limit(2), | ||
| ]); | ||
| $this->assertEquals(2, \count($logs)); | ||
|
|
||
| // Test 3: Find with equal, limit and offset | ||
| $logs = $this->audit->find([ | ||
| \Utopia\Audit\Query::equal('userId', $userId), | ||
| \Utopia\Audit\Query::limit(2), | ||
| \Utopia\Audit\Query::offset(1), | ||
| ]); | ||
| $this->assertEquals(2, \count($logs)); | ||
|
|
||
| // Test 4: Find with multiple filters | ||
| $logs = $this->audit->find([ | ||
| \Utopia\Audit\Query::equal('userId', $userId), | ||
| \Utopia\Audit\Query::equal('resource', 'doc/0'), | ||
| ]); | ||
| $this->assertEquals(1, \count($logs)); | ||
|
|
||
| // Test 5: Find with ordering | ||
| $logsDesc = $this->audit->find([ | ||
| \Utopia\Audit\Query::equal('userId', $userId), | ||
| \Utopia\Audit\Query::orderDesc('time'), | ||
| ]); | ||
| $logsAsc = $this->audit->find([ | ||
| \Utopia\Audit\Query::equal('userId', $userId), | ||
| \Utopia\Audit\Query::orderAsc('time'), | ||
| ]); | ||
| $this->assertEquals(5, \count($logsDesc)); | ||
| $this->assertEquals(5, \count($logsAsc)); | ||
|
|
||
| // Verify order is reversed | ||
| if (\count($logsDesc) === \count($logsAsc)) { | ||
| for ($i = 0; $i < \count($logsDesc); $i++) { | ||
| $this->assertEquals( | ||
| $logsDesc[$i]->getId(), | ||
| $logsAsc[\count($logsAsc) - 1 - $i]->getId() | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| // Test 6: Find with IN filter | ||
| $logs = $this->audit->find([ | ||
| \Utopia\Audit\Query::in('event', ['event_0', 'event_1']), | ||
| ]); | ||
| $this->assertGreaterThanOrEqual(2, \count($logs)); | ||
|
|
||
| // Test 7: Find with between query for time range | ||
| $afterTime = new \DateTime('2024-06-15 12:01:00'); | ||
| $beforeTime = new \DateTime('2024-06-15 12:04:00'); | ||
| $logs = $this->audit->find([ | ||
| \Utopia\Audit\Query::equal('userId', $userId), | ||
| \Utopia\Audit\Query::between('time', DateTime::format($afterTime), DateTime::format($beforeTime)), | ||
| ]); | ||
| $this->assertGreaterThanOrEqual(0, \count($logs)); | ||
|
|
||
| // Test 8: Find with greater than | ||
| $afterTime = new \DateTime('2024-06-15 12:02:00'); | ||
| $logs = $this->audit->find([ | ||
| \Utopia\Audit\Query::equal('userId', $userId), | ||
| \Utopia\Audit\Query::greaterThan('time', DateTime::format($afterTime)), | ||
| ]); | ||
| $this->assertGreaterThanOrEqual(0, \count($logs)); | ||
|
|
||
| // Test 9: Find with less than | ||
| $beforeTime = new \DateTime('2024-06-15 12:03:00'); | ||
| $logs = $this->audit->find([ | ||
| \Utopia\Audit\Query::equal('userId', $userId), | ||
| \Utopia\Audit\Query::lessThan('time', DateTime::format($beforeTime)), | ||
| ]); | ||
| $this->assertGreaterThanOrEqual(0, \count($logs)); | ||
| } |
There was a problem hiding this comment.
Test data mismatch causes pipeline failures.
The test expects 5 logs for userId='userId' at line 598, but createLogs() only creates 3 logs for that user. Similarly, the test references resources like 'doc/0' (line 618-619) and events like 'event_0', 'event_1' (line 646) which are created in testRetrievalParameters(), not in createLogs().
Since setUp() runs before each test method—calling cleanup() and createLogs()—data from other tests doesn't persist.
Options to fix:
- Update expected counts to match
createLogs()output (3 logs for 'userId') - Add the required test data setup within
testFind()itself - Use resources/events that actually exist in
createLogs()(e.g.,'database/document/1','update','delete')
🐛 Suggested fix: Add test data setup or fix expected values
public function testFind(): void
{
$userId = 'userId';
+
+ // Setup additional test data for find tests
+ $userAgent = 'Mozilla/5.0';
+ $ip = '127.0.0.1';
+ $location = 'US';
+ $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");
+ $batchEvents[] = [
+ 'userId' => $userId,
+ 'event' => 'event_' . $i,
+ 'resource' => 'doc/' . $i,
+ 'userAgent' => $userAgent,
+ 'ip' => $ip,
+ 'location' => $location,
+ 'data' => ['sequence' => $i],
+ 'time' => DateTime::format($logTime)
+ ];
+ }
+ $this->audit->logBatch($batchEvents);
// Test 1: Find with equal filter
$logs = $this->audit->find([
\Utopia\Audit\Query::equal('userId', $userId),
]);
- $this->assertEquals(5, \count($logs));
+ $this->assertEquals(8, \count($logs)); // 3 from createLogs() + 5 from batchAlternatively, update to match existing data:
// Test 1: Find with equal filter
$logs = $this->audit->find([
\Utopia\Audit\Query::equal('userId', $userId),
]);
- $this->assertEquals(5, \count($logs));
+ $this->assertEquals(3, \count($logs)); // createLogs() creates 3 logs for userId
// ...
// Test 4: Find with multiple filters
$logs = $this->audit->find([
\Utopia\Audit\Query::equal('userId', $userId),
- \Utopia\Audit\Query::equal('resource', 'doc/0'),
+ \Utopia\Audit\Query::equal('resource', 'database/document/1'),
]);
$this->assertEquals(1, \count($logs));🧰 Tools
🪛 GitHub Actions: Tests
[error] 598-598: Test failed: Utopia\Tests\Audit\Adapter\ClickHouseTest::testFind - Failed asserting that 3 matches expected 5. Step: docker compose exec tests vendor/bin/phpunit --configuration phpunit.xml tests
[error] 598-598: Test failed: Utopia\Tests\Audit\Adapter\DatabaseTest::testFind - Failed asserting that 3 matches expected 5. Step: docker compose exec tests vendor/bin/phpunit --configuration phpunit.xml tests
🤖 Prompt for AI Agents
In @tests/Audit/AuditBase.php around lines 590 - 674, testFind() assumes data
that createLogs() doesn't create (expects 5 logs for userId 'userId', and
resources/events like 'doc/0', 'event_0', 'event_1' that are created in
testRetrievalParameters()), causing failures because setUp() calls cleanup()
then createLogs() before each test; fix by either updating assertions in
testFind() to match createLogs() (expect 3 logs and use existing resource/event
values like 'database/document/1', 'update', 'delete'), or add explicit setup
inside testFind() to create the missing logs/resources/events (call the same
creation logic used in createLogs() or replicate the entries) so the filters
(equal('userId', ...), equal('resource','doc/0'), in('event', [...]),
between/greaterThan/lessThan on 'time') operate on known data.
| public function testCount(): void | ||
| { | ||
| $userId = 'userId'; | ||
|
|
||
| // Test 1: Count with simple filter | ||
| $count = $this->audit->count([ | ||
| \Utopia\Audit\Query::equal('userId', $userId), | ||
| ]); | ||
| $this->assertEquals(5, $count); | ||
|
|
||
| // Test 2: Count with multiple filters | ||
| $count = $this->audit->count([ | ||
| \Utopia\Audit\Query::equal('userId', $userId), | ||
| \Utopia\Audit\Query::equal('resource', 'doc/0'), | ||
| ]); | ||
| $this->assertEquals(1, $count); | ||
|
|
||
| // Test 3: Count with IN filter | ||
| $count = $this->audit->count([ | ||
| \Utopia\Audit\Query::in('event', ['event_0', 'event_1']), | ||
| ]); | ||
| $this->assertGreaterThanOrEqual(2, $count); | ||
|
|
||
| // Test 4: Count ignores limit and offset | ||
| $count = $this->audit->count([ | ||
| \Utopia\Audit\Query::equal('userId', $userId), | ||
| \Utopia\Audit\Query::limit(2), | ||
| \Utopia\Audit\Query::offset(1), | ||
| ]); | ||
| $this->assertEquals(5, $count); // Should count all 5, not affected by limit/offset | ||
|
|
||
| // Test 5: Count with between query | ||
| $afterTime = new \DateTime('2024-06-15 12:01:00'); | ||
| $beforeTime = new \DateTime('2024-06-15 12:04:00'); | ||
| $count = $this->audit->count([ | ||
| \Utopia\Audit\Query::equal('userId', $userId), | ||
| \Utopia\Audit\Query::between('time', DateTime::format($afterTime), DateTime::format($beforeTime)), | ||
| ]); | ||
| $this->assertGreaterThanOrEqual(0, $count); | ||
|
|
||
| // Test 6: Count with greater than | ||
| $afterTime = new \DateTime('2024-06-15 12:02:00'); | ||
| $count = $this->audit->count([ | ||
| \Utopia\Audit\Query::equal('userId', $userId), | ||
| \Utopia\Audit\Query::greaterThan('time', DateTime::format($afterTime)), | ||
| ]); | ||
| $this->assertGreaterThanOrEqual(0, $count); | ||
|
|
||
| // Test 7: Count with less than | ||
| $beforeTime = new \DateTime('2024-06-15 12:03:00'); | ||
| $count = $this->audit->count([ | ||
| \Utopia\Audit\Query::equal('userId', $userId), | ||
| \Utopia\Audit\Query::lessThan('time', DateTime::format($beforeTime)), | ||
| ]); | ||
| $this->assertGreaterThanOrEqual(0, $count); | ||
|
|
||
| // Test 8: Count returns zero for no matches | ||
| $count = $this->audit->count([ | ||
| \Utopia\Audit\Query::equal('userId', 'nonExistentUser'), | ||
| ]); | ||
| $this->assertEquals(0, $count); | ||
| } |
There was a problem hiding this comment.
Same test data mismatch as testFind() causes pipeline failure.
Line 684 expects count = 5 for userId='userId', but createLogs() only creates 3 logs for that user. The same resources ('doc/0') and events ('event_0', 'event_1') referenced here don't exist in the test fixture.
Apply the same fix as testFind() - either add test data setup at the beginning of this test or update expected values to match createLogs() output.
🐛 Suggested fix: Update expected count to match test data
// Test 1: Count with simple filter
$count = $this->audit->count([
\Utopia\Audit\Query::equal('userId', $userId),
]);
- $this->assertEquals(5, $count);
+ $this->assertEquals(3, $count); // createLogs() creates 3 logs for userId
// Test 2: Count with multiple filters
$count = $this->audit->count([
\Utopia\Audit\Query::equal('userId', $userId),
- \Utopia\Audit\Query::equal('resource', 'doc/0'),
+ \Utopia\Audit\Query::equal('resource', 'database/document/1'),
]);
$this->assertEquals(1, $count);
// Test 3: Count with IN filter
$count = $this->audit->count([
- \Utopia\Audit\Query::in('event', ['event_0', 'event_1']),
+ \Utopia\Audit\Query::in('event', ['update', 'delete']),
]);
- $this->assertGreaterThanOrEqual(2, $count);
+ $this->assertEquals(3, $count); // 2 updates + 1 delete
// Test 4: Count ignores limit and offset
$count = $this->audit->count([
\Utopia\Audit\Query::equal('userId', $userId),
\Utopia\Audit\Query::limit(2),
\Utopia\Audit\Query::offset(1),
]);
- $this->assertEquals(5, $count);
+ $this->assertEquals(3, $count); // Should count all 3, not affected by limit/offset🧰 Tools
🪛 GitHub Actions: Tests
[error] 684-684: Test failed: Utopia\Tests\Audit\Adapter\ClickHouseTest::testCount - Failed asserting that 3 matches expected 5. Step: docker compose exec tests vendor/bin/phpunit --configuration phpunit.xml tests
[error] 684-684: Test failed: Utopia\Tests\Audit\Adapter\DatabaseTest::testCount - Failed asserting that 3 matches expected 5. Step: docker compose exec tests vendor/bin/phpunit --configuration phpunit.xml tests
Summary by CodeRabbit
New Features
Adapter/API
Tests
✏️ Tip: You can customize this high-level summary in your review settings.