Add extended attributes and indexes for ClickHouse adapter #102
Conversation
WalkthroughAdds a fluent query builder Utopia\Audit\Query and extends the Adapter API with getById, find, and count. Database and ClickHouse adapters implement these methods. ClickHouse adapter receives extensive changes: new attributes and indexes, dynamic Query-to-SQL parsing, tenant-aware filtering, escaped identifiers, DateTime64 handling, parameterized bindings, batch insert support, and updated result mapping. Audit service gains getLogById, find, and count and log/logBatch now accept extra attributes. Tests added/updated for Query, ClickHouse attributes/indexes, and Audit find/count/getById behaviors. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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.
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.
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.
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.
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.
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.
There was a problem hiding this comment.
Actionable comments posted: 7
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)
1020-1124:createBatch()doesn't respect attribute metadata when building column types.The table schema correctly marks required columns as non-nullable (using
getColumnDefinition()), butcreateBatch()ignores this and uses{...:Nullable(String)}for all unspecified columns. When required fields likeevent,userAgent,ip,userType,resourceType,resourceId, orprojectIdare missing from a log entry, the code defaults tonull(line 1090:$log[$column] ?? null), which violates the ClickHouse table's NOT NULL constraint.The
create()method handles this correctly by checking column type and using appropriate placeholders. AligncreateBatch()to use the same attribute metadata logic: determine nullable status from the attribute'srequiredfield and handle missing required fields by either throwing an error or providing a sensible default.
🤖 Fix all issues with AI agents
In `@src/Audit/Adapter/ClickHouse.php`:
- Around line 253-391: getAttributes adds extra fields but reads drop them
because getSelectColumns() and parseResults() still only handle the base
columns; update the read path so extended attributes round‑trip by including all
attribute columns in SELECT and dynamically mapping them in parsing. Modify
getSelectColumns() to return the full set of keys from getAttributes() (ensuring
id/tenant remain) instead of the hardcoded base list, and update parseResults()
to iterate over the attribute definitions from getAttributes() to populate each
result row (fallback to null/default when missing) so values stored by create()
are returned on subsequent reads.
- Around line 647-658: The getColumnNames() method currently returns untyped
arrays which fails static analysis; add a precise docblock return type and
ensure each $attribute['$id'] is treated as a string before pushing to the
result. Specifically, update the getColumnNames() function to include a PHPDoc
like `@return` string[] (or array<string>) and inside the loop validate/cast
$attribute['$id'] to string (and check isset or is_string) before adding to
$columns; reference getColumnNames() and getAttributes() to locate and adjust
the logic and signature.
- Around line 669-690: Update the formatDateTimeForClickHouse() signature to
include the union type hint (\DateTime|string|null) so it reads: private
function formatDateTimeForClickHouse(\DateTime|string|null $dateTime): string;
then remove the final unreachable throw (throw new Exception('DateTime must be a
DateTime object or string')) since the type system now enforces allowed types;
keep existing handling for null, \DateTime and string cases and preserve
returned string format.
- Around line 697-741: Add a PHPDoc `@param` annotation to the create method to
declare the $log parameter as array<string,mixed> to match other adapters and
improve static typing; update the docblock for public function create(array
$log): Log (method name: create in ClickHouse adapter) to include "@param
array<string,mixed> $log" and leave the existing runtime handling (calls to
formatDateTimeForClickHouse and the isset checks) unchanged.
In `@src/Audit/Adapter/Database.php`:
- Around line 112-126: The getById method in Audit\Adapter\Database currently
swallows all exceptions (catch (\Exception $e) { return null; }) which
contradicts its `@throws` docblock and hides real errors; update getById to only
return null when the retrieved $document->isEmpty() and otherwise let exceptions
from $this->db->getAuthorization()->skip, $this->db->getDocument, or Log
construction bubble up (i.e. remove the broad catch or rethrow caught
exceptions) so callers can distinguish "not found" from authorization/DB errors
while keeping the existing null-on-empty behavior.
In `@tests/Audit/AuditBase.php`:
- Around line 714-716: Remove the unused local variable $baseTime declared as
new \DateTime('2024-06-15 12:00:00') since PHPMD flags it as unused; delete that
declaration (the standalone "$baseTime = new \DateTime(...);" line near the
start of the test block that also defines $batchEvents and the for loop) and run
the tests/PHPMD to ensure nothing else references $baseTime before committing.
- Around line 601-603: Remove the unused variable $baseTime in the test: locate
the declaration of $baseTime (the new \DateTime('2024-06-15 12:00:00')) near the
loop that builds $batchEvents and delete that line so the variable is not
defined but unused, ensuring PHPMD no longer reports the unused variable in
tests/Audit/AuditBase.php.
♻️ Duplicate comments (1)
src/Audit/Adapter/ClickHouse.php (1)
905-983: Guard against attribute-name SQL injection and time-type mismatches.
$attributeis interpolated directly into SQL for filters and ORDER BY. If attributes come fromQuery::parse()(user input), this is injectable. Also,timecomparisons useStringparams which can fail againstDateTime64(3)if timezone suffixes are present.Suggest: validate
$attributeagainst an allowlist (attributes +id/tenant) and escape identifiers; useDateTime64(3)params (orformatDateTimeForClickHouse) when$attribute === 'time'.
| private function getColumnNames(): array | ||
| { | ||
| $columns = []; | ||
| foreach ($this->getAttributes() as $attribute) { | ||
| $columnName = $attribute['$id']; | ||
| // Exclude id and tenant as they're handled separately | ||
| if ($columnName !== 'id' && $columnName !== 'tenant') { | ||
| $columns[] = $columnName; | ||
| } | ||
| } | ||
| return $columns; | ||
| } |
There was a problem hiding this comment.
Tighten getColumnNames() typing to satisfy static analysis.
🔧 Proposed fix
private function getColumnNames(): array
{
$columns = [];
foreach ($this->getAttributes() as $attribute) {
- $columnName = $attribute['$id'];
+ $columnName = $attribute['$id'] ?? '';
+ if (!is_string($columnName) || $columnName === '') {
+ continue;
+ }
// Exclude id and tenant as they're handled separately
if ($columnName !== 'id' && $columnName !== 'tenant') {
$columns[] = $columnName;
}
}
return $columns;
}🧰 Tools
🪛 GitHub Actions: CodeQL
[error] 657-657: Method Utopia\Audit\Adapter\ClickHouse::getColumnNames() should return array but returns array<int<0, max>, mixed>.
🤖 Prompt for AI Agents
In `@src/Audit/Adapter/ClickHouse.php` around lines 647 - 658, The
getColumnNames() method currently returns untyped arrays which fails static
analysis; add a precise docblock return type and ensure each $attribute['$id']
is treated as a string before pushing to the result. Specifically, update the
getColumnNames() function to include a PHPDoc like `@return` string[] (or
array<string>) and inside the loop validate/cast $attribute['$id'] to string
(and check isset or is_string) before adding to $columns; reference
getColumnNames() and getAttributes() to locate and adjust the logic and
signature.
| private function formatDateTimeForClickHouse($dateTime): string | ||
| { | ||
| if ($dateTime === null) { | ||
| return (new \DateTime())->format('Y-m-d H:i:s.v'); | ||
| } | ||
|
|
||
| if ($dateTime instanceof \DateTime) { | ||
| return $dateTime->format('Y-m-d H:i:s.v'); | ||
| } | ||
|
|
||
| if (is_string($dateTime)) { | ||
| try { | ||
| // Parse the datetime string, handling ISO 8601 format with timezone | ||
| $dt = new \DateTime($dateTime); | ||
| return $dt->format('Y-m-d H:i:s.v'); | ||
| } catch (\Exception $e) { | ||
| throw new Exception("Invalid datetime string: {$dateTime}"); | ||
| } | ||
| } | ||
|
|
||
| throw new Exception('DateTime must be a DateTime object or string'); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read the method around lines 669-690
wc -l src/Audit/Adapter/ClickHouse.phpRepository: utopia-php/audit
Length of output: 97
🏁 Script executed:
# Read the specific lines mentioned in the review
sed -n '669,690p' src/Audit/Adapter/ClickHouse.phpRepository: utopia-php/audit
Length of output: 816
🏁 Script executed:
# Check the PHP version requirement from composer.json
cat composer.json | grep -A 5 '"php"'Repository: utopia-php/audit
Length of output: 322
🏁 Script executed:
# Search for calls to formatDateTimeForClickHouse in the codebase
rg "formatDateTimeForClickHouse" --type phpRepository: utopia-php/audit
Length of output: 510
🏁 Script executed:
# Get context around the method calls to understand where $log comes from
grep -B 20 "formatDateTimeForClickHouse(\$log\['time'\]" src/Audit/Adapter/ClickHouse.php | head -40Repository: utopia-php/audit
Length of output: 1319
🏁 Script executed:
# Look for the method signature that contains these calls
rg "public|private" src/Audit/Adapter/ClickHouse.php | grep -A 5 -B 5 "function" | head -60Repository: utopia-php/audit
Length of output: 2351
🏁 Script executed:
# Get a broader view - find the method that uses $log['time']
sed -n '600,700p' src/Audit/Adapter/ClickHouse.phpRepository: utopia-php/audit
Length of output: 3717
Add type hint to align formatDateTimeForClickHouse() signature with documented contract.
The docblock already documents @param \DateTime|string|null $dateTime, but the signature lacks a type hint. This causes static analysis tools to flag the final throw as unreachable. Since the project requires PHP ≥8.0, add the union type hint and remove the unreachable throw.
🔧 Proposed fix
- private function formatDateTimeForClickHouse($dateTime): string
+ private function formatDateTimeForClickHouse(\DateTime|string|null $dateTime): string
{
if ($dateTime === null) {
return (new \DateTime())->format('Y-m-d H:i:s.v');
}
if ($dateTime instanceof \DateTime) {
return $dateTime->format('Y-m-d H:i:s.v');
}
if (is_string($dateTime)) {
try {
// Parse the datetime string, handling ISO 8601 format with timezone
$dt = new \DateTime($dateTime);
return $dt->format('Y-m-d H:i:s.v');
} catch (\Exception $e) {
throw new Exception("Invalid datetime string: {$dateTime}");
}
}
-
- throw new Exception('DateTime must be a DateTime object or string');
}🧰 Tools
🪛 GitHub Actions: CodeQL
[error] 689-689: Unreachable statement - code above always terminates.
🤖 Prompt for AI Agents
In `@src/Audit/Adapter/ClickHouse.php` around lines 669 - 690, Update the
formatDateTimeForClickHouse() signature to include the union type hint
(\DateTime|string|null) so it reads: private function
formatDateTimeForClickHouse(\DateTime|string|null $dateTime): string; then
remove the final unreachable throw (throw new Exception('DateTime must be a
DateTime object or string')) since the type system now enforces allowed types;
keep existing handling for null, \DateTime and string cases and preserve
returned string format.
| $baseTime = new \DateTime('2024-06-15 12:00:00'); | ||
| $batchEvents = []; | ||
| for ($i = 0; $i < 3; $i++) { |
There was a problem hiding this comment.
Remove unused $baseTime to satisfy PHPMD.
🧹 Suggested cleanup
- $baseTime = new \DateTime('2024-06-15 12:00:00');
$batchEvents = [];📝 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.
| $baseTime = new \DateTime('2024-06-15 12:00:00'); | |
| $batchEvents = []; | |
| for ($i = 0; $i < 3; $i++) { | |
| $batchEvents = []; | |
| for ($i = 0; $i < 3; $i++) { |
🧰 Tools
🪛 PHPMD (2.15.0)
601-601: Avoid unused local variables such as '$baseTime'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
In `@tests/Audit/AuditBase.php` around lines 601 - 603, Remove the unused variable
$baseTime in the test: locate the declaration of $baseTime (the new
\DateTime('2024-06-15 12:00:00')) near the loop that builds $batchEvents and
delete that line so the variable is not defined but unused, ensuring PHPMD no
longer reports the unused variable in tests/Audit/AuditBase.php.
| $baseTime = new \DateTime('2024-06-15 12:00:00'); | ||
| $batchEvents = []; | ||
| for ($i = 0; $i < 3; $i++) { |
There was a problem hiding this comment.
Remove unused $baseTime to satisfy PHPMD.
🧹 Suggested cleanup
- $baseTime = new \DateTime('2024-06-15 12:00:00');
$batchEvents = [];📝 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.
| $baseTime = new \DateTime('2024-06-15 12:00:00'); | |
| $batchEvents = []; | |
| for ($i = 0; $i < 3; $i++) { | |
| $batchEvents = []; | |
| for ($i = 0; $i < 3; $i++) { |
🧰 Tools
🪛 PHPMD (2.15.0)
714-714: Avoid unused local variables such as '$baseTime'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
In `@tests/Audit/AuditBase.php` around lines 714 - 716, Remove the unused local
variable $baseTime declared as new \DateTime('2024-06-15 12:00:00') since PHPMD
flags it as unused; delete that declaration (the standalone "$baseTime = new
\DateTime(...);" line near the start of the test block that also defines
$batchEvents and the for loop) and run the tests/PHPMD to ensure nothing else
references $baseTime before committing.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Audit/Adapter/ClickHouse.php (2)
1580-1596: Inconsistent escaping:getByUserAndEvents()uses unescaped column names.Unlike other methods (e.g.,
getByUser,countByUser),getByUserAndEvents()still uses bareuserId,event, andtimein the SQL withoutescapeIdentifier(). This is inconsistent with the escaping applied elsewhere.🔧 Proposed fix
+ $escapedUserId = $this->escapeIdentifier('userId'); + $escapedEvent = $this->escapeIdentifier('event'); + $escapedTime = $this->escapeIdentifier('time'); + $sql = " SELECT " . $this->getSelectColumns() . " FROM {$escapedTable} - WHERE userId = {userId:String} AND event IN ({$eventList['clause']}){$tenantFilter}{$time['clause']} - ORDER BY time {$order} + WHERE {$escapedUserId} = {userId:String} AND {$escapedEvent} IN ({$eventList['clause']}){$tenantFilter}{$time['clause']} + ORDER BY {$escapedTime} {$order} LIMIT {limit:UInt64} OFFSET {offset:UInt64} FORMAT TabSeparated ";
1652-1668: Inconsistent escaping:getByResourceAndEvents()uses unescaped column names.Same issue as
getByUserAndEvents()—resource,event, andtimeare not escaped here.🔧 Proposed fix
+ $escapedResource = $this->escapeIdentifier('resource'); + $escapedEvent = $this->escapeIdentifier('event'); + $escapedTime = $this->escapeIdentifier('time'); + $sql = " SELECT " . $this->getSelectColumns() . " FROM {$escapedTable} - WHERE resource = {resource:String} AND event IN ({$eventList['clause']}){$tenantFilter}{$time['clause']} - ORDER BY time {$order} + WHERE {$escapedResource} = {resource:String} AND {$escapedEvent} IN ({$eventList['clause']}){$tenantFilter}{$time['clause']} + ORDER BY {$escapedTime} {$order} LIMIT {limit:UInt64} OFFSET {offset:UInt64} FORMAT TabSeparated ";
🤖 Fix all issues with AI agents
In `@src/Audit/Adapter/ClickHouse.php`:
- Around line 1166-1177: Add a PHPDoc type for the createBatch() parameter so
PHPStan knows the offsets may exist: add a docblock above the createBatch method
with "@param array<string,mixed> $logs" (use the actual parameter name in
createBatch) to narrow the $log type; this will remove the redundant "?? null"
warnings for offsets like 'event','ip','resource','userAgent' without changing
runtime code.
♻️ Duplicate comments (5)
src/Audit/Adapter/ClickHouse.php (5)
1203-1264: Extended attributes are not returned byparseResults().The
parseResults()method only extracts the base 9-10 columns (id, userId, event, resource, userAgent, ip, location, time, data, and optionally tenant). The new extended attributes (userType, userInternalId, resourceParent, etc.) defined ingetAttributes()are never read back from query results. This means data stored viacreate()won't round-trip throughfind()orgetById().Consider updating
getSelectColumns()to include all attribute columns andparseResults()to dynamically map results based ongetAttributes().
716-737: Fix PHPStan error: Unreachable statement and missing type hint.The pipeline reports line 736 as unreachable because
formatDateTimeForClickHouse()lacks a union type hint. Since PHP 8.0+ is required, add the type hint and remove the final throw.🔧 Proposed fix
- private function formatDateTimeForClickHouse($dateTime): string + private function formatDateTimeForClickHouse(\DateTime|string|null $dateTime): string { if ($dateTime === null) { return (new \DateTime())->format('Y-m-d H:i:s.v'); } if ($dateTime instanceof \DateTime) { return $dateTime->format('Y-m-d H:i:s.v'); } if (is_string($dateTime)) { try { $dt = new \DateTime($dateTime); return $dt->format('Y-m-d H:i:s.v'); } catch (\Exception $e) { throw new Exception("Invalid datetime string: {$dateTime}"); } } - - throw new Exception('DateTime must be a DateTime object or string'); }
649-660: Fix PHPStan error:getColumnNames()return type mismatch.The pipeline reports this method "should return array<string> but returns array<int<0, max>, mixed>". Add type assertion for the
$columnNamevalue.🔧 Proposed fix
private function getColumnNames(): array { $columns = []; foreach ($this->getAttributes() as $attribute) { - $columnName = $attribute['$id']; + /** `@var` string $columnName */ + $columnName = $attribute['$id'] ?? ''; + if ($columnName === '') { + continue; + } // Exclude id and tenant as they're handled separately if ($columnName !== 'id' && $columnName !== 'tenant') { $columns[] = $columnName; } } return $columns; }
744-788: Fix PHPStan errors: Add@paramdocblock to resolve type inference issues.Multiple pipeline errors stem from PHPStan inferring a narrow type for
$log. The method should accept an open array. Add the@paramannotation to match the Database adapter and fix the type errors.🔧 Proposed fix
/** * Create an audit log entry. * + * `@param` array<string, mixed> $log * `@throws` Exception */ public function create(array $log): LogThis will resolve the PHPStan errors on lines 747, 764, 765, 768, and 784 by widening the inferred type of
$log.
253-391: Verify:required => truewithdefault => nullis contradictory.Several attributes (userType, resourceType, resourceId, projectId, projectInternalId, teamId, teamInternalId, hostname) have
'required' => truecombined with'default' => null. This is logically inconsistent—a required field should not default to null. Either make these fields not required or provide meaningful defaults.🔧 Example fix for one attribute
[ '$id' => 'userType', 'type' => Database::VAR_STRING, 'size' => Database::LENGTH_KEY, - 'required' => true, + 'required' => false, 'default' => null, 'signed' => true, 'array' => false, 'filters' => [], ],
… and improve time handling
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Audit/Adapter/ClickHouse.php`:
- Around line 974-977: The loop in ClickHouse adapter silently skips non-Query
entries (foreach over $queries checking instanceof Query) which can widen result
sets; replace the continue with throwing a descriptive exception (e.g.,
InvalidArgumentException) when an item is not an instance of Query, including
the offending item's type or class name in the message so callers know which
element failed and can fix their input rather than silently hiding the error.
- Around line 1159-1189: The batch-insert loop in ClickHouse adapter currently
assigns Nullable(String) for missing values regardless of attribute
requirements; update the foreach that builds $paramKey/$params/$placeholders to
consult the column's attribute metadata (use the adapter's attribute schema or
metadata lookup used elsewhere) to decide nullability: for required attributes
do not emit Nullable and instead either throw a clear error or supply the
adapter's required default, for nullable attributes emit Nullable(String) (or
Nullable appropriate type for 'data'/'time'); ensure the special-handling
branches (time -> formatDateTimeForClickHouse, data JSON, and the in_array list)
use the metadata check before pushing placeholders and params so
required/nullable contract is preserved for all columns and logged when a
required value is missing.
♻️ Duplicate comments (1)
src/Audit/Adapter/ClickHouse.php (1)
1219-1307: Extended attributes are dropped on reads.
create()/createBatch write new columns, but getSelectColumns/parseResults still only cover the base 9(+tenant) fields, so find/getById never return the added attributes. Consider building SELECT columns from getAttributes() and parsing results dynamically so values round‑trip.🔧 Suggested fix
@@ - private function getSelectColumns(): string - { - $columns = [ - $this->escapeIdentifier('id'), - $this->escapeIdentifier('userId'), - $this->escapeIdentifier('event'), - $this->escapeIdentifier('resource'), - $this->escapeIdentifier('userAgent'), - $this->escapeIdentifier('ip'), - $this->escapeIdentifier('location'), - $this->escapeIdentifier('time'), - $this->escapeIdentifier('data'), - ]; + private function getSelectColumns(): string + { + $columns = [$this->escapeIdentifier('id')]; + foreach ($this->getAttributes() as $attribute) { + $id = $attribute['$id'] ?? null; + if (!is_string($id) || $id === 'id' || $id === 'tenant') { + continue; + } + $columns[] = $this->escapeIdentifier($id); + } @@ - return implode(', ', $columns); + return implode(', ', $columns); }@@ - $columns = explode("\t", $line); - // Expect 9 columns without sharedTables, 10 with sharedTables - $expectedColumns = $this->sharedTables ? 10 : 9; + $columns = explode("\t", $line); + $attributeIds = []; + $attributeMap = []; + foreach ($this->getAttributes() as $attr) { + if (isset($attr['$id'])) { + $attributeIds[] = $attr['$id']; + $attributeMap[$attr['$id']] = $attr; + } + } + $expectedColumns = 1 + count($attributeIds) + ($this->sharedTables ? 1 : 0); if (count($columns) < $expectedColumns) { continue; } - - $data = json_decode($columns[8], true) ?? []; - ... - $document = [ - '$id' => $columns[0], - 'userId' => $parseNullableString($columns[1]), - 'event' => $columns[2], - 'resource' => $columns[3], - 'userAgent' => $columns[4], - 'ip' => $columns[5], - 'location' => $parseNullableString($columns[6]), - 'time' => $time, - 'data' => $data, - ]; + $document = ['$id' => $columns[0]]; + $idx = 1; + foreach ($attributeIds as $id) { + $value = $columns[$idx++] ?? ''; + if ($id === 'data') { + $document['data'] = json_decode($value, true) ?? []; + continue; + } + if ($id === 'time') { + $time = $value; + if (strpos($time, 'T') === false) { + $time = str_replace(' ', 'T', $time) . '+00:00'; + } + $document['time'] = $time; + continue; + } + $required = (bool)($attributeMap[$id]['required'] ?? false); + $document[$id] = $required ? $value : $parseNullableString($value); + } @@ - if ($this->sharedTables && isset($columns[9])) { - $document['tenant'] = $columns[9] === '\\N' || $columns[9] === '' ? null : (int) $columns[9]; - } + if ($this->sharedTables) { + $tenantValue = $columns[$idx] ?? ''; + $document['tenant'] = $tenantValue === '\\N' || $tenantValue === '' ? null : (int) $tenantValue; + }
…fields in ClickHouse log entries
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/Audit/Adapter/ClickHouse.php (3)
1260-1321: Extended attributes not returned in query results.
parseResults()only maps the 9 base columns plus tenant. The extended attributes (userType, projectId, teamId, hostname, etc.) are stored viacreate()/createBatch()but never returned infind(),getById(), or other read methods because:
getSelectColumns()(lines 1331-1341) only lists the base columnsparseResults()only parses those same base columnsThis breaks round-trip capability for extended attributes. Consider updating both methods to include all attributes from
getAttributes().
1637-1644: Inconsistent identifier escaping in WHERE clause.Unlike
countByUserAndEvents()(lines 1671-1677) which escapesuserIdandevent, this method directly uses unescaped column names in the SQL:WHERE userId = {userId:String} AND event IN ({$eventList['clause']})For consistency and safety:
🔧 Proposed fix
+ $escapedUserId = $this->escapeIdentifier('userId'); + $escapedEvent = $this->escapeIdentifier('event'); + $sql = " SELECT " . $this->getSelectColumns() . " FROM {$escapedTable} - WHERE userId = {userId:String} AND event IN ({$eventList['clause']}){$tenantFilter}{$time['clause']} - ORDER BY time {$order} + WHERE {$escapedUserId} = {userId:String} AND {$escapedEvent} IN ({$eventList['clause']}){$tenantFilter}{$time['clause']} + ORDER BY " . $this->escapeIdentifier('time') . " {$order} LIMIT {limit:UInt64} OFFSET {offset:UInt64} FORMAT TabSeparated ";
1709-1716: Inconsistent identifier escaping (same as getByUserAndEvents).The
resource,event, andtimecolumns are not escaped here, but they are incountByResourceAndEvents(). Apply the same fix for consistency.
🤖 Fix all issues with AI agents
In `@src/Audit/Adapter/ClickHouse.php`:
- Around line 265-389: The schema lists eight attributes as required but gives
them default null (userType, resourceType, resourceId, projectId,
projectInternalId, teamId, teamInternalId, hostname) which conflicts with the
create() behavior that only inserts columns when isset($log[$column]); either
mark these fields as 'required' => false in the schema arrays for '$id' =>
'userType' etc., or add validation in ClickHouse Adapter's create() method to
enforce presence (e.g., check each required field and throw/return an error if
missing) before building the insert; pick one approach and apply consistently to
the listed identifiers and to the function create().
- Around line 786-812: In the create() method (the loop iterating
$attributeColumns, using $log, $params, $placeholders and $columns) enforce
required attributes by reading the attribute metadata for each $column and if an
attribute is marked 'required' => true and the value is missing from $log (not
set or empty as appropriate), throw/return a clear validation error (e.g.,
InvalidArgumentException) naming the missing field; otherwise proceed to
populate $params, $placeholders and $columns as before (including JSON-encoding
for 'data' and using Nullable/String vs String based on attribute nullability)
so required fields are never silently skipped.
♻️ Duplicate comments (4)
src/Audit/Adapter/ClickHouse.php (4)
649-661: PHPStan typing issue may persist.While the
@var string $columnNameannotation helps document intent, static analyzers may still flag this because$attribute['$id']returnsmixed. The past review suggested adding validation:🔧 Suggested fix for stricter typing
private function getColumnNames(): array { $columns = []; foreach ($this->getAttributes() as $attribute) { - /** `@var` string $columnName */ - $columnName = $attribute['$id']; + $columnName = $attribute['$id'] ?? ''; + if (!is_string($columnName) || $columnName === '') { + continue; + } // Exclude id and tenant as they're handled separately if ($columnName !== 'id' && $columnName !== 'tenant') { $columns[] = $columnName; } } return $columns; }
734-757: Add union type hint to method signature.The docblock documents
@param \DateTime|string|null $dateTime, but the parameter lacks a type hint. Since the project requires PHP ≥8.0, adding the union type removes the unreachable code issue:🔧 Proposed fix
- private function formatDateTimeForClickHouse($dateTime): string + private function formatDateTimeForClickHouse(\DateTime|string|null $dateTime): string { if ($dateTime === null) { return (new \DateTime())->format('Y-m-d H:i:s.v'); } if ($dateTime instanceof \DateTime) { return $dateTime->format('Y-m-d H:i:s.v'); } if (is_string($dateTime)) { try { $dt = new \DateTime($dateTime); return $dt->format('Y-m-d H:i:s.v'); } catch (\Exception $e) { throw new Exception("Invalid datetime string: {$dateTime}"); } } - - // This is unreachable code but kept for completeness - all valid types are handled above - // `@phpstan-ignore-next-line` - throw new Exception('DateTime must be a DateTime object or string'); }
991-994: Do not silently ignore invalid query types.Non-Query inputs are silently skipped with
continue, which can unexpectedly widen result sets. The Database adapter throws an exception for invalid queries—align behavior here for consistency and to prevent silent failures.🔧 Suggested fix
foreach ($queries as $query) { if (!$query instanceof Query) { - continue; + throw new Exception('Invalid query type. Expected Utopia\\Audit\\Query'); }
1329-1348: Select columns limited to base attributes.This confirms the round-trip issue: only base columns are selected. To support extended attributes, dynamically build the column list from
getAttributes():🔧 Suggested approach
private function getSelectColumns(): string { - $columns = [ - $this->escapeIdentifier('id'), - $this->escapeIdentifier('userId'), - // ... other hardcoded columns - ]; + $columns = [$this->escapeIdentifier('id')]; + + foreach ($this->getAttributes() as $attribute) { + $columnName = $attribute['$id'] ?? ''; + if (is_string($columnName) && $columnName !== '') { + $columns[] = $this->escapeIdentifier($columnName); + } + } if ($this->sharedTables) { $columns[] = $this->escapeIdentifier('tenant'); } return implode(', ', $columns); }Note:
parseResults()would also need updating to handle the additional columns.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Audit/Adapter/ClickHouse.php`:
- Around line 792-799: The new strict check in create() (using
getAttributeMetadata and $isRequired) is causing throws when a required
attribute like userType is missing; fix by relaxing validation: adjust the
attribute metadata so userType (and any fields causing test failures) is not
marked required in getAttributeMetadata or change the check in create() to avoid
throwing (e.g., treat missing required attributes as a warning/default rather
than throwing an InvalidArgumentException) and ensure any defaults are applied
before insert; reference getAttributeMetadata, the $isRequired check and the
create() method to locate the code to modify.
- Around line 1138-1153: The current loop builds $columns only from attributes
present in any $logs, so a required attribute that is missing from all logs gets
omitted and bypasses validation; modify the logic that assembles $columns (the
loops referencing $attributeColumns, $presentColumns, and $columns) to always
include attributes marked required by the attribute definitions even if they are
not found in $presentColumns/$logs, so required columns are unconditionally
appended to $columns and will be validated/handled during batch insert.
♻️ Duplicate comments (3)
src/Audit/Adapter/ClickHouse.php (3)
734-756: Add union type hint to match the documented contract.
The method doc says\DateTime|string|null, but the signature is untyped and keeps an unreachable throw. Adding the union type removes the need for the final throw and helps static analysis.♻️ Suggested change
- private function formatDateTimeForClickHouse($dateTime): string + private function formatDateTimeForClickHouse(\DateTime|string|null $dateTime): string { if ($dateTime === null) { return (new \DateTime())->format('Y-m-d H:i:s.v'); } @@ - // This is unreachable code but kept for completeness - all valid types are handled above - // `@phpstan-ignore-next-line` - throw new Exception('DateTime must be a DateTime object or string'); }
263-389: Required fields still default to null (breaks inserts/tests).
These attributes are markedrequired => truebut keepdefault => null, andcreate()now throws for missing values. This is already causing test failures (missinguserType). Align the schema with actual usage by either making these fields optional or providing a non‑null default and populating it.💡 Example fix (make new fields optional until callers populate them)
- 'required' => true, - 'default' => null, + 'required' => false, + 'default' => null,
1342-1359: Extended attributes still won’t round‑trip on reads.
SELECT uses only the base columns, and parseResults expects 9/10 columns. Any new attributes written bycreate()won’t be returned by reads. Please build the SELECT list dynamically fromgetAttributes()and updateparseResults()accordingly.🔧 Directional fix (SELECT all attributes)
- $columns = [ - $this->escapeIdentifier('id'), - $this->escapeIdentifier('userId'), - $this->escapeIdentifier('event'), - $this->escapeIdentifier('resource'), - $this->escapeIdentifier('userAgent'), - $this->escapeIdentifier('ip'), - $this->escapeIdentifier('location'), - $this->escapeIdentifier('time'), - $this->escapeIdentifier('data'), - ]; + $columns = [$this->escapeIdentifier('id')]; + foreach ($this->getAttributes() as $attribute) { + $id = $attribute['$id'] ?? null; + if (is_string($id) && $id !== 'id' && $id !== 'tenant') { + $columns[] = $this->escapeIdentifier($id); + } + }
| // Get attribute metadata to check if required | ||
| $attributeMeta = $this->getAttributeMetadata($column); | ||
| $isRequired = $attributeMeta !== null && isset($attributeMeta['required']) && $attributeMeta['required']; | ||
|
|
||
| // Check if value is missing for required attributes | ||
| if ($isRequired && (!isset($log[$column]) || $log[$column] === '')) { | ||
| throw new \InvalidArgumentException("Required attribute '{$column}' is missing or empty in log entry"); | ||
| } |
There was a problem hiding this comment.
Breaking change: required attributes now throw (tests already failing).
create() now throws when required attributes are missing; current tests/log producers don’t supply userType (pipeline failure). Either relax required flags or ensure all call sites populate these new fields.
🧰 Tools
🪛 GitHub Actions: Tests
[error] 798-798: InvalidArgumentException: Required attribute 'userType' is missing or empty in log entry. Command 'vendor/bin/phpunit --configuration phpunit.xml tests' failed with exit code 2.
🤖 Prompt for AI Agents
In `@src/Audit/Adapter/ClickHouse.php` around lines 792 - 799, The new strict
check in create() (using getAttributeMetadata and $isRequired) is causing throws
when a required attribute like userType is missing; fix by relaxing validation:
adjust the attribute metadata so userType (and any fields causing test failures)
is not marked required in getAttributeMetadata or change the check in create()
to avoid throwing (e.g., treat missing required attributes as a warning/default
rather than throwing an InvalidArgumentException) and ensure any defaults are
applied before insert; reference getAttributeMetadata, the $isRequired check and
the create() method to locate the code to modify.
| // Determine which attribute columns are present in any log | ||
| $presentColumns = []; | ||
| foreach ($logs as $log) { | ||
| foreach ($attributeColumns as $column) { | ||
| if (isset($log[$column]) && !in_array($column, $presentColumns, true)) { | ||
| $presentColumns[] = $column; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Add present columns in the order they're defined in attributes | ||
| foreach ($attributeColumns as $column) { | ||
| if (in_array($column, $presentColumns, true)) { | ||
| $columns[] = $column; | ||
| } | ||
| } |
There was a problem hiding this comment.
Batch inserts can skip required columns entirely.
Column list is built only from attributes present in any log. If a required field is missing in all logs, it never appears in $columns, so no validation happens and the column is omitted. Required fields should be enforced regardless of presence.
🔧 Suggested fix (include required columns unconditionally)
- // Determine which attribute columns are present in any log
- $presentColumns = [];
+ // Determine which attribute columns are present in any log
+ $presentColumns = [];
+ $requiredColumns = [];
+ foreach ($this->getAttributes() as $attribute) {
+ if (!empty($attribute['required']) && isset($attribute['$id'])) {
+ $requiredColumns[] = $attribute['$id'];
+ }
+ }
@@
- foreach ($attributeColumns as $column) {
- if (in_array($column, $presentColumns, true)) {
+ foreach ($attributeColumns as $column) {
+ if (in_array($column, $presentColumns, true) || in_array($column, $requiredColumns, true)) {
$columns[] = $column;
}
}🤖 Prompt for AI Agents
In `@src/Audit/Adapter/ClickHouse.php` around lines 1138 - 1153, The current loop
builds $columns only from attributes present in any $logs, so a required
attribute that is missing from all logs gets omitted and bypasses validation;
modify the logic that assembles $columns (the loops referencing
$attributeColumns, $presentColumns, and $columns) to always include attributes
marked required by the attribute definitions even if they are not found in
$presentColumns/$logs, so required columns are unconditionally appended to
$columns and will be validated/handled during batch insert.
…e batch logging functionality
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/Audit/Audit.php`:
- Around line 86-91: The logBatch method's use of array_map combined with
array_merge loses the expected typed structure for createBatch; update logBatch
to merge defaults without using array_merge (use the array union operator or an
explicit merge within the closure) so the resulting $eventsWithDefaults
preserves keys/types for $this->adapter->createBatch, e.g. replace the static fn
using array_merge with a closure that applies $defaultAttributes + $event (or an
explicit field-by-field merge) and keep the variable names logBatch,
$eventsWithDefaults, and createBatch to locate the change.
- Around line 62-75: The static analyzer complains that array_merge($baseLog,
$attributes) yields non-empty-array<string,mixed> which doesn't match
Adapter::create()'s expected type; in Audit::log, build a payload with a
compatible type before calling $this->adapter->create by asserting/casting the
merged array to the expected shape (or loosening Adapter::create()'s parameter
type). Specifically, in the log method reference $baseLog and $attributes,
perform an explicit shape/type assertion (e.g. document with a PHPDoc /** `@var`
array<string,mixed> $payload */ or equivalent cast) for the merged payload and
pass that asserted $payload to adapter->create, or update Adapter::create
signature to accept array<string,mixed>.
In `@tests/Audit/AuditBase.php`:
- Around line 249-254: The test calls applyRequiredAttributesToBatch() twice on
$batchEvents before invoking $this->audit->logBatch(), causing required
attributes to be merged twice; remove the duplicate invocation so $batchEvents =
$this->applyRequiredAttributesToBatch($batchEvents); appears only once before
calling $this->audit->logBatch($batchEvents).
- Around line 746-747: testCount() is missing a call to
applyRequiredAttributesToBatch() before invoking
$this->audit->logBatch($batchEvents), causing required fields (userType,
projectId, etc.) to be absent; update the test to mirror testFind() by calling
$this->applyRequiredAttributesToBatch($batchEvents) (or the correct method name
used in this class) immediately before $this->audit->logBatch($batchEvents) so
required attributes are applied to each event.
- Around line 814-822: The PHPDoc return type for applyRequiredAttributesToBatch
is incompatible with array_map's generic-less return; add PHPDoc generics to
preserve event shape: declare a template (e.g. `@template` T of
array<string,mixed>), annotate the parameter as `@param` array<int, T>
$batchEvents and $requiredAttributes as array<string,mixed> (from
getRequiredAttributes), then annotate the closure passed to array_map with /**
`@param` T $event `@return` T */ so array_map's inferred return becomes array<int,
T> and update the method `@return` to `@return` array<int, T> (or `@return` array<int,
array<string,mixed>> if you want to concretely collapse to that shape);
reference applyRequiredAttributesToBatch and getRequiredAttributes when making
these PHPDoc changes.
♻️ Duplicate comments (3)
tests/Audit/Adapter/ClickHouseTest.php (1)
354-374: Missing 'location' attribute in expected list.The
expectedAttributesarray is missing the'location'attribute which is defined in the parent SQL adapter schema.🐛 Add missing 'location' attribute
$expectedAttributes = [ 'userType', 'userId', 'userInternalId', 'resourceParent', 'resourceType', 'resourceId', 'resourceInternalId', 'event', 'resource', 'userAgent', 'ip', + 'location', 'country', 'time', 'data', 'projectId', 'projectInternalId', 'teamId', 'teamInternalId', 'hostname' ];tests/Audit/AuditBase.php (2)
729-729: Remove unused$baseTimevariable.Same issue as in
testFind()- this variable is declared but never used.🧹 Remove unused variable
// Create test logs with specific attributes - $baseTime = new \DateTime('2024-06-15 12:00:00'); $batchEvents = [];
614-614: Remove unused$baseTimevariable.PHPMD correctly flags this variable as unused. The loop creates a fresh
DateTimeobject on each iteration instead of using$baseTime.🧹 Remove unused variable
// Create test logs with specific attributes - $baseTime = new \DateTime('2024-06-15 12:00:00'); $batchEvents = [];
| public function log(?string $userId, string $event, string $resource, string $userAgent, string $ip, string $location, array $data = [], array $attributes = []): Log | ||
| { | ||
| return $this->adapter->create([ | ||
| $baseLog = [ | ||
| 'userId' => $userId, | ||
| 'event' => $event, | ||
| 'resource' => $resource, | ||
| 'userAgent' => $userAgent, | ||
| 'ip' => $ip, | ||
| 'location' => $location, | ||
| 'data' => $data, | ||
| ]); | ||
| ]; | ||
|
|
||
| return $this->adapter->create(array_merge($baseLog, $attributes)); | ||
| } |
There was a problem hiding this comment.
Type annotation mismatch with array_merge result.
The pipeline failure indicates that array_merge($baseLog, $attributes) returns non-empty-array<string, mixed> which doesn't satisfy the Adapter::create() expected shape. This is a static analysis limitation with array_merge.
Consider either updating the adapter's type signature to be more permissive, or using a type cast/assertion for the merged array.
♻️ Potential fix using array shape assertion
- return $this->adapter->create(array_merge($baseLog, $attributes));
+ /** `@var` array{userId?: string|null, event: string, resource: string, userAgent: string, ip: string, location?: string, data?: array<string, mixed>} $log */
+ $log = array_merge($baseLog, $attributes);
+ return $this->adapter->create($log);🧰 Tools
🪛 GitHub Actions: CodeQL
[error] 74-74: Parameter #1 $log of method Utopia\Audit\Adapter::create() expects array{userId?: string|null, event: string, resource: string, userAgent: string, ip: string, location?: string, data?: array<string, mixed>}, non-empty-array<string, mixed> given.
🤖 Prompt for AI Agents
In `@src/Audit/Audit.php` around lines 62 - 75, The static analyzer complains that
array_merge($baseLog, $attributes) yields non-empty-array<string,mixed> which
doesn't match Adapter::create()'s expected type; in Audit::log, build a payload
with a compatible type before calling $this->adapter->create by
asserting/casting the merged array to the expected shape (or loosening
Adapter::create()'s parameter type). Specifically, in the log method reference
$baseLog and $attributes, perform an explicit shape/type assertion (e.g.
document with a PHPDoc /** `@var` array<string,mixed> $payload */ or equivalent
cast) for the merged payload and pass that asserted $payload to adapter->create,
or update Adapter::create signature to accept array<string,mixed>.
| public function logBatch(array $events, array $defaultAttributes = []): bool | ||
| { | ||
| $eventsWithDefaults = array_map(static fn (array $event) => array_merge($defaultAttributes, $event), $events); | ||
|
|
||
| return $this->adapter->createBatch($eventsWithDefaults); | ||
| } |
There was a problem hiding this comment.
Same type inference issue with array_merge in logBatch().
The pipeline flags the same pattern where array_map with array_merge loses the typed structure expected by createBatch().
♻️ Potential fix
public function logBatch(array $events, array $defaultAttributes = []): bool
{
- $eventsWithDefaults = array_map(static fn (array $event) => array_merge($defaultAttributes, $event), $events);
+ /** `@var` array<array{userId?: string|null, event: string, resource: string, userAgent: string, ip: string, location?: string, time: DateTime|string|null, data?: array<string, mixed>}> $eventsWithDefaults */
+ $eventsWithDefaults = array_map(static fn (array $event) => array_merge($defaultAttributes, $event), $events);
return $this->adapter->createBatch($eventsWithDefaults);
}📝 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 logBatch(array $events, array $defaultAttributes = []): bool | |
| { | |
| $eventsWithDefaults = array_map(static fn (array $event) => array_merge($defaultAttributes, $event), $events); | |
| return $this->adapter->createBatch($eventsWithDefaults); | |
| } | |
| public function logBatch(array $events, array $defaultAttributes = []): bool | |
| { | |
| /** `@var` array<array{userId?: string|null, event: string, resource: string, userAgent: string, ip: string, location?: string, time: DateTime|string|null, data?: array<string, mixed>}> $eventsWithDefaults */ | |
| $eventsWithDefaults = array_map(static fn (array $event) => array_merge($defaultAttributes, $event), $events); | |
| return $this->adapter->createBatch($eventsWithDefaults); | |
| } |
🧰 Tools
🪛 GitHub Actions: CodeQL
[error] 90-90: Parameter #1 $logs of method Utopia\Audit\Adapter::createBatch() expects array<array{userId?: string|null, event: string, resource: string, userAgent: string, ip: string, location?: string, time: DateTime|string|null, data?: array<string, mixed>}>, array<non-empty-array<string, mixed>>> given.
🤖 Prompt for AI Agents
In `@src/Audit/Audit.php` around lines 86 - 91, The logBatch method's use of
array_map combined with array_merge loses the expected typed structure for
createBatch; update logBatch to merge defaults without using array_merge (use
the array union operator or an explicit merge within the closure) so the
resulting $eventsWithDefaults preserves keys/types for
$this->adapter->createBatch, e.g. replace the static fn using array_merge with a
closure that applies $defaultAttributes + $event (or an explicit field-by-field
merge) and keep the variable names logBatch, $eventsWithDefaults, and
createBatch to locate the change.
| $batchEvents = $this->applyRequiredAttributesToBatch($batchEvents); | ||
|
|
||
| // Test batch insertion | ||
| $batchEvents = $this->applyRequiredAttributesToBatch($batchEvents); | ||
|
|
||
| $result = $this->audit->logBatch($batchEvents); |
There was a problem hiding this comment.
Bug: Duplicate call to applyRequiredAttributesToBatch().
The applyRequiredAttributesToBatch() method is called twice (lines 249 and 252), which will merge required attributes twice. While this may not cause failures with the current implementation (since array_merge with identical keys just overwrites), it's unnecessary and indicates copy-paste error.
🐛 Remove the duplicate call
$batchEvents = $this->applyRequiredAttributesToBatch($batchEvents);
// Test batch insertion
- $batchEvents = $this->applyRequiredAttributesToBatch($batchEvents);
-
$result = $this->audit->logBatch($batchEvents);📝 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.
| $batchEvents = $this->applyRequiredAttributesToBatch($batchEvents); | |
| // Test batch insertion | |
| $batchEvents = $this->applyRequiredAttributesToBatch($batchEvents); | |
| $result = $this->audit->logBatch($batchEvents); | |
| $batchEvents = $this->applyRequiredAttributesToBatch($batchEvents); | |
| // Test batch insertion | |
| $result = $this->audit->logBatch($batchEvents); |
🧰 Tools
🪛 GitHub Actions: CodeQL
[error] 254-254: Parameter #1 $events of method Utopia\Audit\Audit::logBatch() expects array<array{userId: string|null, event: string, resource: string, userAgent: string, ip: string, location: string, time: string, data?: array<string, mixed>}>, array<int, array<string, mixed>> given.
🤖 Prompt for AI Agents
In `@tests/Audit/AuditBase.php` around lines 249 - 254, The test calls
applyRequiredAttributesToBatch() twice on $batchEvents before invoking
$this->audit->logBatch(), causing required attributes to be merged twice; remove
the duplicate invocation so $batchEvents =
$this->applyRequiredAttributesToBatch($batchEvents); appears only once before
calling $this->audit->logBatch($batchEvents).
| } | ||
| $this->audit->logBatch($batchEvents); |
There was a problem hiding this comment.
Missing applyRequiredAttributesToBatch() call before logBatch().
Unlike testFind() at line 632, testCount() is missing the required attributes application. This will cause failures when running ClickHouse tests, since required fields like userType, projectId, etc. will be missing.
🐛 Add missing required attributes application
];
}
+ $batchEvents = $this->applyRequiredAttributesToBatch($batchEvents);
+
$this->audit->logBatch($batchEvents);📝 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.
| } | |
| $this->audit->logBatch($batchEvents); | |
| } | |
| $batchEvents = $this->applyRequiredAttributesToBatch($batchEvents); | |
| $this->audit->logBatch($batchEvents); |
🤖 Prompt for AI Agents
In `@tests/Audit/AuditBase.php` around lines 746 - 747, testCount() is missing a
call to applyRequiredAttributesToBatch() before invoking
$this->audit->logBatch($batchEvents), causing required fields (userType,
projectId, etc.) to be absent; update the test to mirror testFind() by calling
$this->applyRequiredAttributesToBatch($batchEvents) (or the correct method name
used in this class) immediately before $this->audit->logBatch($batchEvents) so
required attributes are applied to each event.
| protected function applyRequiredAttributesToBatch(array $batchEvents): array | ||
| { | ||
| $requiredAttributes = $this->getRequiredAttributes(); | ||
| if ($requiredAttributes === []) { | ||
| return $batchEvents; | ||
| } | ||
|
|
||
| return array_map(static fn (array $event) => array_merge($event, $requiredAttributes), $batchEvents); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the file
find . -name "AuditBase.php" -path "*/tests/Audit/*"Repository: utopia-php/audit
Length of output: 87
🏁 Script executed:
# Once located, check the method at lines 814-822
if [ -f "tests/Audit/AuditBase.php" ]; then
wc -l tests/Audit/AuditBase.php
fiRepository: utopia-php/audit
Length of output: 89
🏁 Script executed:
# Get the actual content around those lines
if [ -f "tests/Audit/AuditBase.php" ]; then
sed -n '800,830p' tests/Audit/AuditBase.php
fiRepository: utopia-php/audit
Length of output: 1031
🏁 Script executed:
# Search for Audit::logBatch() method to understand shape expectations
rg -A 15 "function logBatch" --type phpRepository: utopia-php/audit
Length of output: 882
🏁 Script executed:
# Check for any CodeQL or type checking configurations
fd -e "\.yml" -e "\.yaml" | xargs grep -l "codeql\|phpstan\|psalm" 2>/dev/null | head -5Repository: utopia-php/audit
Length of output: 42
🏁 Script executed:
# Search for type checker configurations
find . -maxdepth 2 \( -name "phpstan.neon*" -o -name "psalm.xml*" -o -name ".phpstan.neon*" -o -name "setup.cfg" \)Repository: utopia-php/audit
Length of output: 42
🏁 Script executed:
# Look for CI configuration files that might indicate CodeQL or type checking
find . -maxdepth 2 \( -name "*.yml" -o -name "*.yaml" \) -path "*/.github/workflows/*"Repository: utopia-php/audit
Length of output: 42
🏁 Script executed:
# Search for createBatch method to understand what shape it expects
rg -A 10 "function createBatch" --type phpRepository: utopia-php/audit
Length of output: 2131
🏁 Script executed:
# Check the broader context of how applyRequiredAttributesToBatch is used
rg "applyRequiredAttributesToBatch" --type php -B 2 -A 2Repository: utopia-php/audit
Length of output: 2001
🏁 Script executed:
# Check if there are any `@phpstan-ignore` or type suppression comments in the test file
rg "@phpstan|@psalm|@suppress" tests/Audit/AuditBase.phpRepository: utopia-php/audit
Length of output: 42
🏁 Script executed:
# Final verification: check if there's any composer.json configuration for type checking
cat composer.json | grep -A 20 "require-dev\|scripts"Repository: utopia-php/audit
Length of output: 854
🏁 Script executed:
# Check root directory for any hidden config files or CI setup
ls -la | grep -E "^\." | head -20Repository: utopia-php/audit
Length of output: 42
🏁 Script executed:
# Check for phpstan configuration
find . -maxdepth 2 -name "phpstan.neon*" -o -name "phpstan.dist.neon"Repository: utopia-php/audit
Length of output: 42
🏁 Script executed:
# Try to run phpstan on the specific file to see if there are actual issues
if command -v phpstan &> /dev/null; then
phpstan analyse --level max tests/Audit/AuditBase.php 2>&1 | head -50
else
echo "PHPStan not installed in sandbox"
fiRepository: utopia-php/audit
Length of output: 819
🏁 Script executed:
# Check the actual array_map behavior and what PHPStan might infer
rg -B 5 "array_map.*array_merge" tests/Audit/AuditBase.phpRepository: utopia-php/audit
Length of output: 317
🏁 Script executed:
# Look at the return type of array_map - check if the type is being properly narrowed
cat tests/Audit/AuditBase.php | sed -n '814,822p'Repository: utopia-php/audit
Length of output: 410
Type annotation for array_map() return value is incompatible with declared return type.
The applyRequiredAttributesToBatch() method's return type is declared as array<int, array<string, mixed>> in the PHPDoc, but array_map() returns array<mixed>, causing a type mismatch that PHPStan at level max (configured in composer.json) would flag.
Use templates to preserve the array shape through array_map():
Fix type annotation
/**
* Apply adapter-specific required attributes to batch events.
*
- * `@param` array<int, array<string, mixed>> $batchEvents
- * `@return` array<int, array<string, mixed>>
+ * `@template` T of array<string, mixed>
+ * `@param` array<int, T> $batchEvents
+ * `@return` array<int, T>
*/
protected function applyRequiredAttributesToBatch(array $batchEvents): array🤖 Prompt for AI Agents
In `@tests/Audit/AuditBase.php` around lines 814 - 822, The PHPDoc return type for
applyRequiredAttributesToBatch is incompatible with array_map's generic-less
return; add PHPDoc generics to preserve event shape: declare a template (e.g.
`@template` T of array<string,mixed>), annotate the parameter as `@param` array<int,
T> $batchEvents and $requiredAttributes as array<string,mixed> (from
getRequiredAttributes), then annotate the closure passed to array_map with /**
`@param` T $event `@return` T */ so array_map's inferred return becomes array<int,
T> and update the method `@return` to `@return` array<int, T> (or `@return` array<int,
array<string,mixed>> if you want to concretely collapse to that shape);
reference applyRequiredAttributesToBatch and getRequiredAttributes when making
these PHPDoc changes.
Summary by CodeRabbit
New Features
Adapter/API
Tests
✏️ Tip: You can customize this high-level summary in your review settings.