Conversation
📝 WalkthroughWalkthroughAudit integration was refactored to use dependency injection and an adapter layer. Multiple controllers and platform modules now accept an injected Audit instance (replacing inline Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)src/Appwrite/Platform/Workers/Deletes.php (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (2)
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 |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (3)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Logs/XList.php (2)
5-27: Fix Pintordered_importsfailure in this file before merge.
The linter is already failing on PSR-12 import ordering; please run Pint (or apply the repo’s import ordering rules) and commit the result.
89-90: Fix method calls toAudit::getLogsByResource()andAudit::countLogsByResource(): these v2.0.0 methods do not accept a$queriesarray.Per utopia-php/audit v2.0.0, the correct signatures are:
getLogsByResource(resource: string, events: array|null = null, after: \DateTime|null = null, before: \DateTime|null = null, limit: int|null = null, offset: int|null = null, ascending: bool|null = null): arraycountLogsByResource(resource: string, events: array|null = null, after: \DateTime|null = null, before: \DateTime|null = null): intThe calls at lines 89 and 136 pass
$queriesas a second positional argument, which these methods do not accept. Additionally,countLogsByResourcedoes not supportlimit/offsetparameters at all. Convert$queriesto individual named parameters matching the method signatures.app/controllers/api/messaging.php (1)
1148-1167:Query::groupByType()method does not exist — this will cause a fatal runtime error.The code calls
Query::groupByType($queries)at line 1162, but this method does not exist in either utopia-php/database or Appwrite's codebase. The method call will throw a fatal "Call to undefined method" error when executed.Check the correct query parsing/extraction method in Appwrite's Query class or utopia-php/database to properly extract limit and offset values.
🧹 Nitpick comments (2)
app/init/resources.php (1)
657-660: Type-hint the injected DB (and consider a return type) for the newauditresource.
Right now$dbForProjectis untyped, which makes DI errors harder to spot and static analysis weaker.-App::setResource('audit', function ($dbForProject) { +App::setResource('audit', function (Database $dbForProject): Audit { $adapter = new AdapterDatabase($dbForProject); return new Audit($adapter); }, ['dbForProject']);app/controllers/api/messaging.php (1)
1162-1167: Extract filters-only for count or pass full queries consistently to both methods.The provider logs endpoint extracts
offsetandlimitfrom queries to pass togetLogsByResource(), but passes the full$queriesarray (containing offset/limit) tocountLogsByResource(). This parameter misalignment can cause incorrect totals ifcountLogsByResource()ignores pagination parameters. Align by either: (1) extracting filters-only and passing to both methods, or (2) passing the full$queriesto both (consistent with other endpoints like teams.php). Also verify Audit v2'scountLogsByResource()behavior with limit/offset in the query array.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
app/controllers/api/messaging.php(5 hunks)app/controllers/api/projects.php(1 hunks)app/controllers/api/teams.php(1 hunks)app/controllers/api/users.php(1 hunks)app/http.php(3 hunks)app/init/resources.php(4 hunks)composer.json(2 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Logs/XList.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Logs/XList.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Logs/XList.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Logs/XList.php(1 hunks)src/Appwrite/Platform/Workers/Audits.php(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-08-11T10:32:38.142Z
Learnt from: abnegate
Repo: appwrite/appwrite PR: 10297
File: app/controllers/api/users.php:649-657
Timestamp: 2025-08-11T10:32:38.142Z
Learning: In Appwrite's utopia-php/database (version 0.77.* and later), the database layer internally handles filter separation for count operations. When using `$dbForProject->count()` within a `skipFilters` closure, there's no need to manually separate filter queries using `Query::groupByType($queries)['filters']` as the database handles this automatically.
Applied to files:
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Logs/XList.phpsrc/Appwrite/Platform/Modules/Databases/Http/TablesDB/Logs/XList.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Logs/XList.phpapp/controllers/api/messaging.phpapp/controllers/api/users.php
📚 Learning: 2025-10-02T10:16:48.075Z
Learnt from: abnegate
Repo: appwrite/appwrite PR: 10023
File: src/Appwrite/Databases/TransactionState.php:174-219
Timestamp: 2025-10-02T10:16:48.075Z
Learning: In `src/Appwrite/Databases/TransactionState.php`, the `listDocuments` method intentionally applies queries only to the initial database fetch of committed documents. Transaction state changes (creates, updates, deletes) are then overlaid on top of the filtered committed set without re-evaluating the queries against the final merged result. This is the intended behavior for transaction-aware document listing.
Applied to files:
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Logs/XList.php
📚 Learning: 2025-08-22T06:23:32.026Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 10348
File: app/controllers/shared/api.php:546-546
Timestamp: 2025-08-22T06:23:32.026Z
Learning: The Utopia\Queue\Broker\Pool class (aliased as BrokerPool) implements both Publisher and Consumer interfaces, making it directly compatible with constructors expecting Publisher parameters like Func::__construct(protected Publisher $publisher). No wrapping is needed.
Applied to files:
app/init/resources.php
📚 Learning: 2025-08-22T06:23:32.026Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 10348
File: app/controllers/shared/api.php:546-546
Timestamp: 2025-08-22T06:23:32.026Z
Learning: In the Utopia Queue system, the Pool class implements both Publisher and Consumer interfaces, so BrokerPool (Pool) can be passed directly to constructors expecting Publisher parameters like Func::__construct(protected Publisher $publisher).
Applied to files:
app/init/resources.php
🧬 Code graph analysis (5)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Logs/XList.php (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Logs/XList.php (1)
action(74-139)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Logs/XList.php (1)
action(69-134)
app/controllers/api/teams.php (1)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Logs/XList.php (1)
action(74-139)
src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Logs/XList.php (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Logs/XList.php (1)
action(74-139)src/Appwrite/Platform/Workers/Audits.php (1)
action(64-154)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Logs/XList.php (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Logs/XList.php (1)
action(78-152)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Logs/XList.php (1)
action(69-134)
app/controllers/api/users.php (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Logs/XList.php (1)
action(74-139)src/Appwrite/Platform/Workers/Audits.php (1)
action(64-154)
🪛 GitHub Actions: Linter
src/Appwrite/Platform/Workers/Audits.php
[error] 1-1: PSR-12: Coding standard issue detected by Pint (ordered_imports).
src/Appwrite/Platform/Modules/Databases/Http/Databases/Logs/XList.php
[error] 1-1: PSR-12: Coding standard issue detected by Pint (ordered_imports).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (10)
app/controllers/api/users.php (1)
948-1005: InjectedAuditusage is correctly scoped todbForProject.The audit resource definition (
app/init/resources.php:657-659) explicitly declaresdbForProjectas its dependency, wrapping it inAdapterDatabasebefore passing to theAuditclass. This ensures audit logs are fetched from the current project's database, not a platform-level store. The DI scoping correctly implements tenant isolation.app/controllers/api/projects.php (1)
249-253: Avoid$adaptershadowing + verify adapter class namespace.Variable shadowing is valid:
$adapteris reused after being declared earlier at Line 226 forDatabasePool, making the code harder to follow. Rename to$auditAdapterfor clarity.However, the adapter namespace
\Utopia\Audit\Adapters\Databaserequires verification. Official Utopia framework documentation shows thatAudittypically accepts\Utopia\Database\Databaseinstances directly, not a separate\Utopia\Audit\Adapters\Databaseclass. Confirm that the namespace in the proposed fix matches what Audit v2 actually expects before applying the change.- $adapter = new \Utopia\Audit\Adapters\Database($dbForProject); - $audit = new Audit($adapter); + $auditAdapter = new \Utopia\Audit\Adapters\Database($dbForProject); + $audit = new Audit($auditAdapter); $audit->setup();src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Logs/XList.php (1)
75-76: Verify audit DI wiring pattern consistency across document log endpoints.Unable to verify the claim that the audit DI wiring mirrors the collection logs endpoint due to the file being new/uncommitted in this PR. Confirm that all document-level log endpoints (line 75-76 and line 79) use consistent DI injection patterns with the collection-level logs endpoint, following the standard
->inject('audit')pattern without local instantiation.src/Appwrite/Platform/Workers/Audits.php (1)
8-8: Verify adapter namespace and import order against Audit v2 documentation.The review raises concerns about ordered imports failing Pint linting and inconsistent adapter namespaces. Based on Audit v2 package structure, adapters should come from
Utopia\Database\Adapter\*(notUtopia\Audit\Adapters\*orUtopia\Audit\Adapter\*). Confirm the correct import path and ensure all imports are sorted per Pint configuration.app/http.php (1)
15-16: Verify namespace usage across changed files. The import statement usesUtopia\Audit\Adapter\Databasebut the review claims other files in this PR useUtopia\Audit\Adapters\Database(plural). Confirm which namespace is correct according to utopia-php/audit documentation and ensure all imports are aligned.src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Logs/XList.php (1)
61-66: Audit DI update matches the other logs endpoints. InjectedAudit $auditusage forgetLogsByResource()andcountLogsByResource()is consistent and removes duplicated instantiation across all log endpoint implementations.composer.json (1)
50-56: No action needed—Audit v2 and database v3.5.0 are compatible.The codebase already successfully uses
Utopia\Audit\Adapters\Databasewith database v3.5.0 (aliased as 4.0.0), as shown insrc/Appwrite/Platform/Workers/Audits.php. The recent "Audits upgrade" commit (Dec 14, 2025) confirms this combination is intentional. The "2." constraint for Audit is consistent with how other utopia-php packages are versioned in this project (e.g., "1.", "0.15.*"). Database v3.5.0 (released Nov 18, 2025) contains no breaking changes for Audit compatibility.app/init/resources.php (1)
150-152:queueForAuditsnow returnsAuditEvent— double-check downstream expectations.
If any consumers expected aUtopia\Audit\Auditinstance fromqueueForAudits, this will be a runtime break; the naming (queueForAudits) also reads “audit service” vs “audit event publisher,” so consider a follow-up rename for clarity.src/Appwrite/Platform/Modules/Databases/Http/Databases/Logs/XList.php (1)
66-71: DI wiring forAuditlooks consistent with the broader refactor.
Good move removing local instantiation and injectingauditinto the handler.Also applies to: 74-74
app/controllers/api/teams.php (1)
1467-1469: Audit DI for/v1/teams/:teamId/logslooks good.
This matches the pattern used in other updated log endpoints and removes per-request construction.
✨ Benchmark results
⚡ Benchmark Comparison
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
app/http.php (1)
395-399: Consider renaming the adapter variable for clarity.The variable
$adapteron line 396 shadows the$adaptervariable defined on line 381. While functionally correct (the original adapter is no longer needed), using a more descriptive name like$auditAdapterwould improve code clarity and prevent potential confusion during future maintenance.Apply this diff to improve variable naming:
if ($dbForProject->getCollection(AuditAdapterSQL::COLLECTION)->isEmpty()) { - $adapter = new AdapterDatabase($dbForProject); - $audit = new Audit($adapter); + $auditAdapter = new AdapterDatabase($dbForProject); + $audit = new Audit($auditAdapter); $audit->setup(); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
app/http.php(3 hunks)composer.json(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (1)
app/http.php (1)
15-16: LGTM!The import aliases are well-chosen to avoid naming conflicts with existing Database imports, and both are properly utilized in the adapter-based Audit construction below.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/controllers/api/messaging.php (1)
1148-1168: Fix undefined$filterQueriesin provider logs and remove invalid parameters fromcountLogsByResource()callsThe provider logs endpoint on line ~1221 calls
$audit->countLogsByResource($resource, $filterQueries)where$filterQueriesis never defined, causing a fatal error. Additionally, theAudit::countLogsByResource()method signature only acceptsresource,after, andbeforeparameters—it does not accept filter or query parameters. Both provider logs and message logs endpoints are passing invalid parameters to this method.Provider logs fix:
- 'total' => $includeTotal ? $audit->countLogsByResource($resource, $filterQueries) : 0, + 'total' => $includeTotal ? $audit->countLogsByResource($resource) : 0,Message logs fix (line ~1168):
- 'total' => $includeTotal ? $audit->countLogsByResource($resource, $queries) : 0, + 'total' => $includeTotal ? $audit->countLogsByResource($resource) : 0,Also remove the unused
$filterQueries = $grouped['filters'] ?? [];line from the message logs handler.
🧹 Nitpick comments (3)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Logs/XList.php (1)
106-109: LGTM! Pagination logic correctly implemented.The query grouping and explicit offset/limit extraction follows the established pattern across all log endpoints. The default values (limit: 25, offset: 0) are consistent with other log handlers in the codebase.
Consider extracting the pagination defaults to class constants for easier maintenance:
private const DEFAULT_LIMIT = 25; private const DEFAULT_OFFSET = 0;Then use:
-$limit = $grouped['limit'] ?? 25; -$offset = $grouped['offset'] ?? 0; +$limit = $grouped['limit'] ?? self::DEFAULT_LIMIT; +$offset = $grouped['offset'] ?? self::DEFAULT_OFFSET;This would improve consistency if defaults need to change across multiple log endpoints in the future.
app/controllers/api/account.php (1)
2974-2977: Optional: dropdbForProjectinjection if it’s no longer used in this route.
The updated handler doesn’t reference$dbForProject; consider removing the injection/parameter to keep the route signature minimal (unless DI requires it for per-project Audit wiring).app/controllers/api/messaging.php (1)
1170-1211: Avoidforeach (... as &$log)withoutunset($log)
Using a by-reference loop variable can leak a reference to the last element into later code paths. If you don’t need by-ref, drop&. If you do,unset($log)after the loop.- foreach ($logs as $i => &$log) { + foreach ($logs as $i => $log) { $log['userAgent'] = (!empty($log['userAgent'])) ? $log['userAgent'] : 'UNKNOWN'; @@ - } + }Also applies to: 2578-2619, 2998-3039, 3797-3838
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/controllers/api/account.php(1 hunks)app/controllers/api/messaging.php(11 hunks)app/controllers/api/teams.php(3 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Logs/XList.php(3 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Logs/XList.php(3 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Logs/XList.php(3 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Logs/XList.php(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-11T10:32:38.142Z
Learnt from: abnegate
Repo: appwrite/appwrite PR: 10297
File: app/controllers/api/users.php:649-657
Timestamp: 2025-08-11T10:32:38.142Z
Learning: In Appwrite's utopia-php/database (version 0.77.* and later), the database layer internally handles filter separation for count operations. When using `$dbForProject->count()` within a `skipFilters` closure, there's no need to manually separate filter queries using `Query::groupByType($queries)['filters']` as the database handles this automatically.
Applied to files:
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Logs/XList.phpapp/controllers/api/messaging.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Logs/XList.phpsrc/Appwrite/Platform/Modules/Databases/Http/TablesDB/Logs/XList.php
🧬 Code graph analysis (5)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Logs/XList.php (1)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Logs/XList.php (1)
action(79-160)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Logs/XList.php (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Logs/XList.php (1)
action(78-155)src/Appwrite/Platform/Modules/Databases/Http/Databases/Logs/XList.php (1)
action(74-142)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Logs/XList.php (3)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Logs/XList.php (1)
action(78-155)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Logs/XList.php (1)
action(69-137)src/Appwrite/Platform/Workers/Audits.php (1)
action(64-154)
src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Logs/XList.php (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Logs/XList.php (1)
action(74-142)src/Appwrite/Platform/Workers/Audits.php (1)
action(64-154)
app/controllers/api/account.php (1)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Logs/XList.php (1)
action(74-142)
🪛 GitHub Actions: Linter
app/controllers/api/account.php
[error] 1-1: PSR-12: no_unused_imports. Remove unused imports in this file.
[error] 1-1: Pint lint failed. No_unused_imports issue detected by PSR-12 linter. Command: vendor/bin/pint --test --config pint.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: E2E Service Test (Users)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (Migrations)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Shared Mode Service Test (Dev Keys) (Shared V2)
- GitHub Check: E2E Shared Mode Service Test (Dev Keys) (Shared V1)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: E2E General Test
- GitHub Check: Unit Test
- GitHub Check: E2E Shared Mode Service Test (Site Screenshots) (Shared V2)
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: E2E Shared Mode Service Test (Site Screenshots) (Shared V1)
- GitHub Check: Benchmark
- GitHub Check: scan
🔇 Additional comments (10)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Logs/XList.php (4)
74-74: LGTM! Proper dependency injection.The Audit service is correctly injected into the dependency chain, aligning with the upgrade to Audit v2.0.0 and the shift to dependency-injected services across the platform.
78-78: LGTM! Correct method signature update.The
Auditparameter correctly receives the injected dependency, replacing the previous pattern of local Audit instantiation.
101-104: LGTM! Correct adaptation to Audit v2 API.The query handling properly:
- Groups queries to extract pagination parameters
- Applies sensible defaults (limit=25, offset=0)
- Passes offset and limit as named parameters to
getLogsByResourceThis pattern is consistent with the updated Audit v2.0.0 API and matches the implementation in related files.
153-153: LGTM! Correct usage of updated count API.The
countLogsByResourcecall correctly passes only the resource parameter, aligning with the Audit v2.0.0 API where count operations no longer require query parameters.src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Logs/XList.php (2)
75-75: LGTM! Audit service properly injected.The dependency injection is correctly wired in both the constructor and the action method signature, aligning with the repository-wide audit v2.0.0 upgrade pattern.
Also applies to: 79-79
158-158: LGTM! Count method correctly updated.The
countLogsByResourcecall properly uses the new v2.0.0 API signature without the queries parameter, consistent with the audit upgrade across the codebase.src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Logs/XList.php (1)
65-66: Audit DI injection and method calls are correct. TheAudit::getLogsByResource($resource, offset: ..., limit: ...)andAudit::countLogsByResource($resource)signatures match the usage in lines 87 and 134. Pagination logic is sound—extracting offset/limit viaQuery::groupByType()and passing them as named parameters togetLogsByResourcewhile callingcountLogsByResourcewithout pagination parameters avoids double-counting.app/controllers/api/teams.php (1)
1467-1469: Audit DI and pagination implementation verified as correct.The Audit dependency injection and API usage are properly implemented:
getLogsByResource($resource, offset: $offset, limit: $limit)uses named arguments compatible with the API signaturecountLogsByResource($resource)correctly excludes queries (pagination only affects fetching, not counting)- Implementation is consistent across all team log endpoints
No action needed.
app/controllers/api/messaging.php (2)
2991-2995: Same limit/offset typing concern (subscriber logs)
Same pattern as topic logs; please ensure$limitand$offsetare ints derived from query values.Also applies to: 3041-3043
2571-2575: No action needed. The code correctly usesQuery::groupByType()to extract pagination parameters. The method returns an associative array wherelimitandoffsetare integers (or null), not Query objects. The null coalescing operators (?? 25and?? 0) properly convert null values to integer defaults, which are then correctly passed as integer parameters togetLogsByResource(). This implementation pattern is used consistently throughout the codebase and functions correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
app/controllers/api/account.php(3 hunks)src/Appwrite/Platform/Workers/Audits.php(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/controllers/api/account.php
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (2)
src/Appwrite/Platform/Workers/Audits.php (2)
7-7: LGTM!The import correctly uses the singular
Utopia\Audit\Adapternamespace, consistent with v2.0.0 and other usages across the codebase.
140-142: LGTM!The adapter-based Audit construction correctly follows the v2.0.0 pattern: wrapping the database connection with
AdapterDatabaseand passing it to theAuditconstructor. This aligns with the dependency injection approach across the codebase.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/controllers/api/teams.php (1)
1463-1468: Team logs: adjust Audit calls to use limit/offset, not$queriesThe injected
Utopia\Audit\Auditinstance expects scalar pagination (intlimit/offset) and optional DateTime filters; passing$queries(array of Query objects) intogetLogsByResource()andcountLogsByResource()will cause a runtime TypeError.Extract limit/offset from the parsed queries using the standard pattern already used in other endpoints:
try { $queries = Query::parseQueries($queries); } catch (QueryException $e) { throw new Exception(Exception::GENERAL_QUERY_INVALID, $e->getMessage()); } + $grouped = Query::groupByType($queries); + $limit = $grouped['limit'] ?? 25; + $offset = $grouped['offset'] ?? 0; + $resource = 'team/' . $team->getId(); - $logs = $audit->getLogsByResource($resource, $queries); + $logs = $audit->getLogsByResource( + resource: $resource, + limit: $limit, + offset: $offset, + );And update the count call:
- 'total' => $includeTotal ? $audit->countLogsByResource($resource, $queries) : 0, + 'total' => $includeTotal ? $audit->countLogsByResource($resource) : 0,
♻️ Duplicate comments (2)
app/controllers/api/messaging.php (2)
1148-1149: Providers logs: keep offset/limit, fixcountLogsByResourcesignatureInjecting
Auditand deriving$limit/$offsetviaQuery::groupByTypeis the right direction, but:$response->dynamic(new Document([ 'total' => $includeTotal ? $audit->countLogsByResource($resource, $queries) : 0,still passes
$queriesintocountLogsByResource, which expects only the resource plus optional DateTime bounds, not aQuery[]array. This will raise a type error. (github.com)You can keep pagination behavior and fix the signature with:
- $grouped = Query::groupByType($queries); - $limit = $grouped['limit'] ?? 25; - $offset = $grouped['offset'] ?? 0; + $grouped = Query::groupByType($queries); + $limit = $grouped['limit'] ?? 25; + $offset = $grouped['offset'] ?? 0; @@ - $logs = $audit->getLogsByResource($resource, offset: $offset, limit: $limit); + $logs = $audit->getLogsByResource( + resource: $resource, + limit: $limit, + offset: $offset, + ); @@ - $response->dynamic(new Document([ - 'total' => $includeTotal ? $audit->countLogsByResource($resource, $queries) : 0, - 'logs' => $output, - ]), Response::MODEL_LOG_LIST); + $response->dynamic(new Document([ + 'total' => $includeTotal ? $audit->countLogsByResource($resource) : 0, + 'logs' => $output, + ]), Response::MODEL_LOG_LIST);Please confirm against your local
Audit::countLogsByResourcesignature that only resource / DateTime parameters are accepted, and adjust if it has been extended.Also applies to: 1162-1167, 1213-1216
2556-2557: Topic/subscriber/message logs: don’t pass$queriesintogetLogsByResource/countLogsByResourceAcross the topic, subscriber, and message log endpoints the newly injected
Auditis used like:$logs = $audit->getLogsByResource($resource, $queries); ... 'total' => $includeTotal ? $audit->countLogsByResource($resource, $queries) : 0,However,
Utopia\Audit\AuditexpectsgetLogsByResource(string $resource, ?\DateTime $after = null, ?\DateTime $before = null, int $limit = 25, int $offset = 0, bool $ascending = false)andcountLogsByResource(string $resource, ?\DateTime $after = null, ?\DateTime $before = null). Passing aQuery[]array as the second argument will cause aTypeError. (github.com)To preserve the existing API while using Audit correctly, follow the same pattern as in the providers logs endpoint:
@@ App::get('/v1/messaging/topics/:topicId/logs') - try { - $queries = Query::parseQueries($queries); - } catch (QueryException $e) { - throw new Exception(Exception::GENERAL_QUERY_INVALID, $e->getMessage()); - } - - $resource = 'topic/' . $topicId; - $logs = $audit->getLogsByResource($resource, $queries); + try { + $queries = Query::parseQueries($queries); + } catch (QueryException $e) { + throw new Exception(Exception::GENERAL_QUERY_INVALID, $e->getMessage()); + } + + $grouped = Query::groupByType($queries); + $limit = $grouped['limit'] ?? 25; + $offset = $grouped['offset'] ?? 0; + + $resource = 'topic/' . $topicId; + $logs = $audit->getLogsByResource( + resource: $resource, + limit: $limit, + offset: $offset, + ); @@ - $response->dynamic(new Document([ - 'total' => $includeTotal ? $audit->countLogsByResource($resource, $queries) : 0, - 'logs' => $output, - ]), Response::MODEL_LOG_LIST); + $response->dynamic(new Document([ + 'total' => $includeTotal ? $audit->countLogsByResource($resource) : 0, + 'logs' => $output, + ]), Response::MODEL_LOG_LIST);Apply the same pattern for:
- Subscriber logs (
/v1/messaging/subscribers/:subscriberId/logs)- Message logs (
/v1/messaging/messages/:messageId/logs)i.e., parse queries, extract
limit/offsetviaQuery::groupByType, pass them via named args togetLogsByResource, and remove$queriesfromcountLogsByResource.Verify your installed
utopia-php/auditversion’s method signatures (especiallygetLogsByResourceandcountLogsByResource) to ensure they match the README examples and that no additional query-array overload was introduced.Also applies to: 2570-2572, 2618-2620, 2973-2974, 2987-2989, 3035-3037, 3768-3769, 3782-3784, 3830-3832
🧹 Nitpick comments (2)
src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Logs/XList.php (2)
59-66: Minor indentation inconsistency in method chaining.The
->inject()calls (lines 61-65) have extra indentation compared to the->param()calls above them. This is a stylistic nit that doesn't affect functionality but could be aligned for consistency.->param('databaseId', '', new UID(), 'Database ID.') ->param('queries', [], new Queries([new Limit(), new Offset()]), 'Array of query strings generated using the Query class provided by the SDK. [Learn more about queries](https://appwrite.io/docs/queries). Only supported methods are limit and offset', true) - ->inject('response') - ->inject('dbForProject') - ->inject('locale') - ->inject('geodb') - ->inject('audit') + ->inject('response') + ->inject('dbForProject') + ->inject('locale') + ->inject('geodb') + ->inject('audit') ->callback($this->action(...));
88-89: Unnecessary reference in foreach loop.The
&$logreference modifies$log['userAgent']on line 89, but this modification isn't used since$output[$i]is populated independently. The reference can be removed for clarity.- foreach ($logs as $i => &$log) { - $log['userAgent'] = $log['userAgent'] ?: 'UNKNOWN'; + foreach ($logs as $i => $log) { + $userAgent = $log['userAgent'] ?: 'UNKNOWN'; - $detector = new Detector($log['userAgent']); + $detector = new Detector($userAgent);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
app/controllers/api/messaging.php(5 hunks)app/controllers/api/projects.php(1 hunks)app/controllers/api/teams.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Logs/XList.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Logs/XList.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Logs/XList.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Logs/XList.php(1 hunks)src/Appwrite/Platform/Workers/Audits.php(2 hunks)tests/e2e/Services/Account/AccountCustomClientTest.php(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/e2e/Services/Account/AccountCustomClientTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- app/controllers/api/projects.php
- src/Appwrite/Platform/Workers/Audits.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-11T10:32:38.142Z
Learnt from: abnegate
Repo: appwrite/appwrite PR: 10297
File: app/controllers/api/users.php:649-657
Timestamp: 2025-08-11T10:32:38.142Z
Learning: In Appwrite's utopia-php/database (version 0.77.* and later), the database layer internally handles filter separation for count operations. When using `$dbForProject->count()` within a `skipFilters` closure, there's no need to manually separate filter queries using `Query::groupByType($queries)['filters']` as the database handles this automatically.
Applied to files:
src/Appwrite/Platform/Modules/Databases/Http/Databases/Logs/XList.phpsrc/Appwrite/Platform/Modules/Databases/Http/TablesDB/Logs/XList.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Logs/XList.phpapp/controllers/api/messaging.php
🧬 Code graph analysis (5)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Logs/XList.php (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Logs/XList.php (1)
action(78-152)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Logs/XList.php (1)
action(69-134)
app/controllers/api/teams.php (3)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Logs/XList.php (1)
action(78-152)src/Appwrite/Platform/Modules/Databases/Http/Databases/Logs/XList.php (1)
action(74-139)src/Appwrite/Platform/Workers/Audits.php (1)
action(64-154)
src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Logs/XList.php (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Logs/XList.php (1)
action(74-139)src/Appwrite/Platform/Workers/Audits.php (1)
action(64-154)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Logs/XList.php (1)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Logs/XList.php (1)
action(74-139)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Logs/XList.php (2)
src/Appwrite/Platform/Workers/Audits.php (1)
action(64-154)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/XList.php (1)
action(80-177)
🪛 GitHub Actions: Linter
src/Appwrite/Platform/Modules/Databases/Http/Databases/Logs/XList.php
[error] 1-1: PSR-12 linting failed. Pint reported issues in 890 files total with 2 style issues; file shows a lint issue in this path.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (Users)
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (Messaging)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E General Test
- GitHub Check: E2E Shared Mode Service Test (Dev Keys) (Shared V2)
- GitHub Check: E2E Shared Mode Service Test (Site Screenshots) (Shared V1)
- GitHub Check: E2E Shared Mode Service Test (Dev Keys) (Shared V1)
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: E2E Shared Mode Service Test (Site Screenshots) (Shared V2)
- GitHub Check: Unit Test
- GitHub Check: scan
🔇 Additional comments (3)
src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Logs/XList.php (2)
69-69: LGTM!The action method signature correctly receives the injected
Audit $auditparameter, aligning with the new DI pattern for the audit upgrade.
83-84: LGTM!The audit usage correctly follows the new DI pattern:
getLogsByResourcefor fetching logs with query supportcountLogsByResourcefor total count with the same queriesThis is consistent with the established pattern in other log endpoints. Based on learnings, the database layer internally handles filter separation for count operations, so passing queries directly is correct.
Also applies to: 130-131
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Logs/XList.php (1)
70-74: LGTM: Audit dependency injection properly configured.The addition of the
auditinjection follows the established DI pattern and is correctly positioned after other dependencies.
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Logs/XList.php
Show resolved
Hide resolved
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Logs/XList.php
Show resolved
Hide resolved
src/Appwrite/Platform/Modules/Databases/Http/Databases/Logs/XList.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/controllers/api/teams.php (1)
1467-1469: Team logs: Audit usage and pagination are correct; consider small consistency/refactor tweaksThe team logs route cleanly injects Audit, derives limit/offset from grouped queries, and uses
getLogsByResource()pluscountLogsByResource()in line with the other audit-backed log endpoints—behavior looks correct. For readability and future maintenance, you might (a) match the other handlers by ordering the named arguments aslimitthenoffsetin thegetLogsByResource()call, and (b) consider a shared helper for the repeated “audit log →Document+ geodb country lookup” transformation used across the various log endpoints.Also applies to: 1482-1488, 1534-1535
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/controllers/api/messaging.php(12 hunks)app/controllers/api/teams.php(3 hunks)app/controllers/api/users.php(3 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Logs/XList.php(3 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Logs/XList.php(3 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Logs/XList.php(3 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Logs/XList.php(3 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Logs/XList.php(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Rows/Logs/XList.php(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-11T10:32:38.142Z
Learnt from: abnegate
Repo: appwrite/appwrite PR: 10297
File: app/controllers/api/users.php:649-657
Timestamp: 2025-08-11T10:32:38.142Z
Learning: In Appwrite's utopia-php/database (version 0.77.* and later), the database layer internally handles filter separation for count operations. When using `$dbForProject->count()` within a `skipFilters` closure, there's no need to manually separate filter queries using `Query::groupByType($queries)['filters']` as the database handles this automatically.
Applied to files:
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Logs/XList.phpsrc/Appwrite/Platform/Modules/Databases/Http/TablesDB/Logs/XList.phpapp/controllers/api/messaging.phpapp/controllers/api/users.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Logs/XList.php
🧬 Code graph analysis (3)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Logs/XList.php (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Logs/XList.php (1)
action(74-144)src/Appwrite/Platform/Workers/Audits.php (1)
action(64-154)
app/controllers/api/teams.php (3)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Logs/XList.php (1)
action(79-162)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Logs/XList.php (1)
action(78-156)src/Appwrite/Platform/Modules/Databases/Http/Databases/Logs/XList.php (1)
action(74-144)
app/controllers/api/users.php (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Logs/XList.php (1)
action(78-156)src/Appwrite/Platform/Modules/Databases/Http/Databases/Logs/XList.php (1)
action(74-144)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: E2E Shared Mode Service Test (Site Screenshots) (Shared V2)
- GitHub Check: Unit Test
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: E2E Shared Mode Service Test (Dev Keys) (Shared V2)
- GitHub Check: Benchmark
- GitHub Check: scan
🔇 Additional comments (13)
src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Rows/Logs/XList.php (1)
54-55: Parent class signature verified. The Audit service injection is correctly positioned in the DI chain. The parent classDocumentLogXList::action()signature confirms theAudit $auditparameter is properly placed afterReader $geodb, matching the injection order in lines 50-54.src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Logs/XList.php (1)
53-54: LGTM! The Audit service injection is correctly positioned in the DI chain, and the parent classCollectionLogXList::action()method signature properly includes theAudit $auditparameter at the correct position (afterReader $geodb).src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Logs/XList.php (5)
75-75: LGTM! Audit service injection follows DI pattern.The addition of the Audit service via dependency injection is consistent with the refactoring described in the PR summary and aligns with similar patterns in related endpoints.
79-79: LGTM! Method signature correctly accepts injected Audit service.The Audit parameter is properly aligned with the injection declaration and follows the established parameter ordering convention.
106-111: LGTM! Pagination logic correctly implemented.The code properly extracts
limitandoffsetfrom grouped queries and passes them as named parameters togetLogsByResource. This correctly addresses the method signature requirements and avoids theTypeErrorthat would result from passing the$queriesarray directly.Based on learnings from past review, this fix was previously identified and has been correctly applied here.
160-160: LGTM! Count method call is correct.The
countLogsByResourcemethod is correctly called with only the$resourceparameter, matching its signature and avoiding the invalid$queriesargument.Based on learnings from past review, this fix was previously identified and has been correctly applied here.
75-79: The audit library dependency in composer.json is specified as"utopia-php/audit": "dev-feat-db-3.x", not version 2.0.0 as referenced in the PR description. Verify that this development branch version is intentional and aligns with the actual audit API changes implemented in this PR.Likely an incorrect or invalid review comment.
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Logs/XList.php (2)
69-78: Audit DI injection and action signature alignment look goodInjecting
'audit'and appendingAudit $auditat the end of the action parameters matches the inject chain order (response,dbForProject,locale,geodb,audit) and is consistent with the broader DI pattern in this PR. I don't see wiring or type-hint issues here.
93-106: Audit 2.x pagination + counting usage looks correct; just confirm API and total semanticsParsing queries, grouping with
Query::groupByType(), and defaultinglimit/offsetis appropriate given the validator only allowsLimitandOffset. Passing them as named args togetLogsByResource($resource, limit: $limit, offset: $offset)matches the new Audit API style (see README examples using namedlimit/offset/after/before/ascendingparameters for getters), and avoids the earlier incompatibility of passing rawQuery[]. (github.com)Using
countLogsByResource($resource)for'total'delegates counting to the Audit adapter, which is a good consolidation, but it intentionally ignores pagination/time filters — the total returned here is for all logs on that resource, not just the current page.Please double‑check that:
- The
utopia-php/auditversion wired into this branch exposesgetLogsByResource(string $resource, ?DateTime $after = null, ?DateTime $before = null, int $limit = 25, int $offset = 0, bool $ascending = false)(or equivalent) so thelimit/offsetnamed arguments resolve correctly, and- The client/SDK for this endpoint indeed expects
totalto be the overall resource log count, not a page-scoped count.Also applies to: 152-155
src/Appwrite/Platform/Modules/Databases/Http/Databases/Logs/XList.php (1)
70-71: Audit DI and pagination for database logs look correctAudit is cleanly injected, the action signature matches the param/inject order, and limit/offset are correctly derived via
Query::groupByType()before callinggetLogsByResource(). UsingcountLogsByResource($resource)fortotalaligns with the other audit log handlers; no issues spotted here.Also applies to: 74-75, 89-95, 140-142
src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Logs/XList.php (1)
65-66: TablesDB database logs endpoint correctly migrated to injected AuditThe TablesDB logs route mirrors the database logs handler: Audit is injected, queries are parsed and grouped to get limit/offset, and both retrieval and total counting use the injected Audit instance without leaking the raw
$queries. Implementation looks consistent and sound.Also applies to: 69-70, 83-89, 135-136
app/controllers/api/users.php (1)
948-949: User logs endpoint: Audit injection and total handling look goodAudit is properly injected, queries are parsed and grouped to derive limit/offset, and
getLogsByUser()is called with those pagination values. Thetotalfield correctly honours thetotalflag while returning the overall user-log count rather than just the current page. No issues from a correctness perspective.Also applies to: 962-967, 1005-1007
app/controllers/api/messaging.php (1)
1148-1149: Audit DI and pagination for messaging log endpoints look goodInjecting
Auditand switching the provider/topic/subscriber/message log endpoints to useQuery::groupByTypeforlimit/offset,getLogsByResource($resource, ...)for retrieval, andcountLogsByResource($resource)for totals is consistent and aligns with the documented "limit + offset only" query support on these routes. This also resolves the earlier incorrect extra-argument usage oncountLogsByResource. I don't see any functional or correctness issues in these changes.Also applies to: 1162-1164, 1167-1167, 1214-1214, 2556-2557, 2570-2572, 2575-2575, 2623-2623, 2977-2978, 2991-2993, 2996-2996, 3044-3044, 3776-3777, 3790-3792, 3795-3795, 3843-3843
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/controllers/api/teams.php (1)
30-30: Team logs Audit integration and pagination are correctly wired.
- Importing
Audit, injectingaudit, and type-hintingAudit $auditin/v1/teams/:teamId/logsare consistent with the new DI pattern.- Parsing queries, using
Query::groupByTypeto computelimit/offset, then calling$audit->getLogsByResource($resource, offset: $offset, limit: $limit)and$audit->countLogsByResource($resource)matches the expected usage and keeps total independent of pagination.As a minor future clean-up, the user-agent + geo-IP → log DTO transformation here is nearly identical to other log endpoints (databases, collections, account); extracting a shared helper could reduce duplication later on.
Also applies to: 1467-1468, 1482-1485, 1487-1487, 1534-1535
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
app/controllers/api/account.php(3 hunks)app/controllers/api/messaging.php(12 hunks)app/controllers/api/teams.php(3 hunks)app/controllers/api/users.php(3 hunks)app/init/resources.php(4 hunks)src/Appwrite/Platform/Workers/Deletes.php(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/controllers/api/messaging.php
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-08-11T10:32:38.142Z
Learnt from: abnegate
Repo: appwrite/appwrite PR: 10297
File: app/controllers/api/users.php:649-657
Timestamp: 2025-08-11T10:32:38.142Z
Learning: In Appwrite's utopia-php/database (version 0.77.* and later), the database layer internally handles filter separation for count operations. When using `$dbForProject->count()` within a `skipFilters` closure, there's no need to manually separate filter queries using `Query::groupByType($queries)['filters']` as the database handles this automatically.
Applied to files:
app/controllers/api/users.phpsrc/Appwrite/Platform/Workers/Deletes.php
📚 Learning: 2025-10-23T08:06:38.889Z
Learnt from: abnegate
Repo: appwrite/appwrite PR: 10546
File: src/Appwrite/Platform/Workers/Migrations.php:144-148
Timestamp: 2025-10-23T08:06:38.889Z
Learning: In the Appwrite codebase, migration workers receive already-validated data from queued jobs. Query validation using Query::parseQueries() happens at the API endpoint level (with try-catch for QueryException) before jobs are queued, so workers in src/Appwrite/Platform/Workers/Migrations.php don't need redundant validation.
Applied to files:
src/Appwrite/Platform/Workers/Deletes.php
📚 Learning: 2025-08-22T06:23:32.026Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 10348
File: app/controllers/shared/api.php:546-546
Timestamp: 2025-08-22T06:23:32.026Z
Learning: The Utopia\Queue\Broker\Pool class (aliased as BrokerPool) implements both Publisher and Consumer interfaces, making it directly compatible with constructors expecting Publisher parameters like Func::__construct(protected Publisher $publisher). No wrapping is needed.
Applied to files:
app/init/resources.php
📚 Learning: 2025-08-22T06:23:32.026Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 10348
File: app/controllers/shared/api.php:546-546
Timestamp: 2025-08-22T06:23:32.026Z
Learning: In the Utopia Queue system, the Pool class implements both Publisher and Consumer interfaces, so BrokerPool (Pool) can be passed directly to constructors expecting Publisher parameters like Func::__construct(protected Publisher $publisher).
Applied to files:
app/init/resources.php
🧬 Code graph analysis (3)
app/controllers/api/users.php (1)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Logs/XList.php (1)
action(74-144)
app/controllers/api/teams.php (3)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Logs/XList.php (1)
action(74-144)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Logs/XList.php (1)
action(78-156)src/Appwrite/Platform/Workers/Audits.php (1)
action(64-154)
app/controllers/api/account.php (1)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Logs/XList.php (1)
action(74-144)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (9)
src/Appwrite/Platform/Workers/Deletes.php (2)
12-12: LGTM! Audit adapter import updated.The import change from
Utopia\Audit\AudittoUtopia\Audit\Adapter\SQLaligns with the audit system upgrade to version 2.0.0.
520-520: Verify that SQL::COLLECTION constant is correctly defined.The code references
SQL::COLLECTIONinstead ofAudit::COLLECTION. Ensure this constant is properly defined in theUtopia\Audit\Adapter\SQLclass and matches the expected audit collection name in the database schema.Applies to: Lines 520 and 789-794
app/init/resources.php (3)
7-7: LGTM! Audit-related imports added.The new imports support the audit system upgrade:
AuditEventfor the audit event queueAdapterDatabaseandAuditfor the new dependency-injected audit resourceAlso applies to: 33-34
150-152: Verify AuditEvent constructor signature.The
queueForAuditsresource now returnsnew AuditEvent($publisher)instead ofnew Audit($publisher). Based on the learnings,BrokerPool(aliased asPublisher) should be compatible with event queue constructors. However, verify that theAuditEventclass constructor accepts aPublisherparameter.#!/bin/bash # Description: Verify AuditEvent constructor accepts Publisher parameter # Find AuditEvent class constructor ast-grep --pattern $'class AuditEvent { $$$ public function __construct($$$) { $$$ } $$$ }' # Also check parent class if it extends Event rg -nP 'class\s+AuditEvent\s+extends' --type=php
645-648: Theauditresource initialization properly wraps thedbForProjectdatabase in anAdapterDatabaseadapter before passing it to theAuditconstructor. Constructor signatures are compatible:
AdapterDatabase::__construct(DatabaseClient $db)accepts aUtopia\Database\DatabaseinstanceAudit::__construct(Adapter $adapter)accepts adapter instances, andAdapterDatabaseproperly extends theAdapterbase classThis adapter pattern implementation is correct and aligns with the utopia-php/audit library design.
app/controllers/api/users.php (3)
948-949: LGTM! Audit dependency injection implemented.The route now injects the
auditresource (registered inapp/init/resources.php) instead of instantiatingAuditlocally. This follows a cleaner dependency injection pattern and aligns with similar changes across other controllers in this PR.
962-964: LGTM! Pagination logic follows established pattern.The pagination parameters are now extracted using
Query::groupByType($queries)with sensible defaults (limit: 25, offset: 0). This pattern is consistent with other log listing endpoints in the codebase (e.g.,src/Appwrite/Platform/Modules/Databases/Http/Databases/Logs/XList.php).
966-966: Verify that$user->getSequence()(integer) is the correct parameter type forgetLogsByUser()andcountLogsByUser().The Audit library's public documentation shows
countLogsByUserand similar retrieval methods accept a user identifier as the first parameter, with pagination and filtering as named parameters. However, the code uses$user->getSequence()(an integer) where documentation examples show string userId values. Confirm whether the Audit library v2.0.0 accepts integer sequence IDs or expects string user IDs for these methods.app/controllers/api/account.php (1)
40-40: Audit DI and account logs pagination look correct and consistent.
- Importing
Utopia\Audit\Auditand injectingauditinto/v1/account/logsaligns the handler with the new DI-based audit setup.- Using
Query::groupByType($queries)to derivelimit/offset, and then passing them via named args to$audit->getLogsByUser(...), withcountLogsByUsercalled without pagination params, matches the intended API and avoids the previous incorrect$queriesargument.No functional issues spotted here.
Also applies to: 2984-2986, 2993-2997, 3025-3025
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/http.php (1)
265-269: Verify complete migration ofAudit::COLLECTIONreferences across the codebase.The adapter-based initialization pattern here is correct. However, as flagged in a previous review, there may be remaining references to
Audit::COLLECTIONin other files (e.g.,src/Appwrite/Platform/Workers/Deletes.php) that need to be updated toAuditAdapterSQL::COLLECTIONfor consistency.#!/bin/bash # Verify all Audit::COLLECTION and AuditAdapterSQL::COLLECTION references echo "=== Searching for Audit::COLLECTION references ===" rg 'Audit::COLLECTION' --type=php -n echo -e "\n=== Searching for AuditAdapterSQL::COLLECTION references ===" rg 'AuditAdapterSQL::COLLECTION' --type=php -n
🧹 Nitpick comments (2)
app/controllers/api/account.php (1)
40-40: Audit injection and pagination look good; consider normalizing log shape before mergingThe
/v1/account/logschanges are aligned with the new audit API:
- Importing
Utopia\Audit\Auditand injectingauditmatches the newApp::setResource('audit', ...).- Using
Query::groupByType($queries)to derive$limit/$offset, and passing them as named arguments intogetLogsByUser(...), follows the v2.0.0 README patterns.- Total is now computed via
countLogsByUser($user->getSequence()), which matches the count API (time filters optional, queries not accepted).One robustness concern:
- Inside the loop,
$logis treated both as an array ($log['userAgent'],$log['data'],$log['ip']) and as an object ($log->getArrayCopy()). Other handlers in this PR (e.g. database/collection log XList endpoints) only use array-style access and don’t callgetArrayCopy(), which suggests adapters may legitimately return plain arrays.To make this endpoint resilient regardless of whether the adapter returns
Logobjects or plain arrays, you can normalize to an array once and merge from there:Proposed safer handling for log normalization
- foreach ($logs as $i => &$log) { - $log['userAgent'] = (!empty($log['userAgent'])) ? $log['userAgent'] : 'UNKNOWN'; - - $detector = new Detector($log['userAgent']); - - $output[$i] = new Document(array_merge( - $log->getArrayCopy(), - $log['data'], - $detector->getOS(), - $detector->getClient(), - $detector->getDevice() - )); + foreach ($logs as $i => &$log) { + // Normalize to array for both Log objects and plain arrays + if (\is_object($log) && \method_exists($log, 'getArrayCopy')) { + $logArray = $log->getArrayCopy(); + } else { + $logArray = (array) $log; + } + + $logArray['userAgent'] = !empty($logArray['userAgent']) ? $logArray['userAgent'] : 'UNKNOWN'; + + $detector = new Detector($logArray['userAgent']); + + $output[$i] = new Document(array_merge( + $logArray, + $logArray['data'] ?? [], + $detector->getOS(), + $detector->getClient(), + $detector->getDevice() + )); - $record = $geodb->get($log['ip']); + $record = $geodb->get($logArray['ip'] ?? null);This keeps the response model the same while avoiding a potential fatal error if the adapter returns arrays instead of objects.
Please confirm against the concrete return type from your configured
Utopia\Audit\Adapter(Database/ClickHouse) that$logsis always an object withgetArrayCopy(). If there is any chance of plain arrays, adopting the normalization above (or an equivalent check) will prevent runtime errors. You can verify quickly by dumpingget_class($logs[0])in a test environment or by asserting the type in a unit test for/v1/account/logs.Also applies to: 3043-3056, 3084-3084
app/controllers/api/messaging.php (1)
1169-1169: Consider consistent parameter order for maintainability.Provider and topic log endpoints use
offset: $offset, limit: $limit, while subscriber and message log endpoints uselimit: $limit, offset: $offset. Although named parameters make this functionally equivalent, aligning all calls to uselimit: $limit, offset: $offset(matching the pattern in the database logs modules) would improve code consistency.🔎 Suggested alignment
- $logs = $audit->getLogsByResource($resource, offset: $offset, limit: $limit); + $logs = $audit->getLogsByResource($resource, limit: $limit, offset: $offset);Apply the same change at line 2578 for topic logs.
Also applies to: 2578-2578
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
app/controllers/api/account.php(3 hunks)app/controllers/api/messaging.php(12 hunks)app/controllers/api/projects.php(1 hunks)app/controllers/api/teams.php(3 hunks)app/controllers/api/users.php(3 hunks)app/http.php(3 hunks)app/init/resources.php(4 hunks)composer.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- composer.json
- app/controllers/api/projects.php
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-22T06:23:32.026Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 10348
File: app/controllers/shared/api.php:546-546
Timestamp: 2025-08-22T06:23:32.026Z
Learning: The Utopia\Queue\Broker\Pool class (aliased as BrokerPool) implements both Publisher and Consumer interfaces, making it directly compatible with constructors expecting Publisher parameters like Func::__construct(protected Publisher $publisher). No wrapping is needed.
Applied to files:
app/init/resources.php
📚 Learning: 2025-08-22T06:23:32.026Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 10348
File: app/controllers/shared/api.php:546-546
Timestamp: 2025-08-22T06:23:32.026Z
Learning: In the Utopia Queue system, the Pool class implements both Publisher and Consumer interfaces, so BrokerPool (Pool) can be passed directly to constructors expecting Publisher parameters like Func::__construct(protected Publisher $publisher).
Applied to files:
app/init/resources.php
📚 Learning: 2025-08-11T10:32:38.142Z
Learnt from: abnegate
Repo: appwrite/appwrite PR: 10297
File: app/controllers/api/users.php:649-657
Timestamp: 2025-08-11T10:32:38.142Z
Learning: In Appwrite's utopia-php/database (version 0.77.* and later), the database layer internally handles filter separation for count operations. When using `$dbForProject->count()` within a `skipFilters` closure, there's no need to manually separate filter queries using `Query::groupByType($queries)['filters']` as the database handles this automatically.
Applied to files:
app/controllers/api/users.phpapp/controllers/api/messaging.php
🧬 Code graph analysis (5)
app/controllers/api/users.php (1)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Logs/XList.php (1)
action(74-144)
app/controllers/api/account.php (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Logs/XList.php (1)
action(79-157)src/Appwrite/Platform/Modules/Databases/Http/Databases/Logs/XList.php (1)
action(74-144)
app/controllers/api/teams.php (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Logs/XList.php (1)
action(79-157)src/Appwrite/Platform/Modules/Databases/Http/Databases/Logs/XList.php (1)
action(74-144)
app/controllers/api/messaging.php (3)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Logs/XList.php (1)
action(79-157)src/Appwrite/Platform/Modules/Databases/Http/Databases/Logs/XList.php (1)
action(74-144)src/Appwrite/Platform/Workers/Audits.php (1)
action(64-154)
app/http.php (2)
src/Appwrite/Event/Database.php (1)
getCollection(137-140)src/Appwrite/Vcs/Comment.php (1)
isEmpty(58-61)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (9)
app/init/resources.php (1)
7-7: Audit DI wiring and aliasing look correctAlias for
Appwrite\Event\Auditand the newauditresource usingAdapterDatabase+Auditare consistent with the updated utopia-php/audit adapter-based API and avoid class name collisions. No issues from a wiring/correctness perspective.Please run the audit-related tests (account logs, database/collection logs, and workers) against a project with existing audit data to confirm that logs are written and read correctly after the adapter upgrade.
Also applies to: 33-35, 150-152, 656-659
app/controllers/api/account.php (1)
1277-1277: Authorization injection is now explicit and consistent across flowsThe added
->inject('authorization')calls for the token-based session, email update, phone update, recovery confirmation, and phone verification endpoints make existing$authorizationusage (e.g.skip()and role changes) explicit and DI-driven. This improves clarity without changing behavior.If you have API tests around these flows, rerun them to confirm there are no regressions in auth scoping (especially around
skip()blocks and role additions) after the injection change.Also applies to: 3250-3250, 3360-3360, 3738-3738, 4160-4160
app/controllers/api/users.php (2)
948-966: LGTM - Audit DI and pagination pattern correctly applied.The changes properly inject
Auditvia dependency injection, extract pagination parameters usingQuery::groupByType($queries), and use the new API methods with named parameters. The default limit of 25 and offset of 0 are consistent with other log endpoints in the codebase.
1006-1006: Count method usage is correct.The
countLogsByUsermethod is called without forwarding queries, which aligns with the new Audit API where count methods take only the resource/user identifier. Based on learnings, the database layer internally handles filter separation for count operations.app/controllers/api/teams.php (2)
1474-1494: LGTM - Teams logs endpoint correctly updated for Audit upgrade.The dependency injection, pagination extraction, and method calls follow the same pattern applied across other log endpoints. The implementation is consistent with the relevant code snippets from
XList.phphandlers.
1541-1541: Count method usage is consistent with the new Audit API.The
countLogsByResourcecall correctly omits the queries parameter, matching the pattern used in other log endpoints.app/http.php (2)
15-16: LGTM - Adapter imports correctly added.The imports for
AdapterDatabaseandAuditAdapterSQLproperly support the new adapter-based Audit initialization pattern required by utopia-php/audit 2.0.0.
395-399: LGTM - Project database audit setup follows the same pattern.The adapter-based Audit initialization for shared project tables correctly mirrors the platform database setup, ensuring consistency across both initialization paths.
app/controllers/api/messaging.php (1)
1150-1151: LGTM – Audit DI and pagination correctly implemented.The dependency injection of
Audit, extraction of pagination parameters viaQuery::groupByType($queries), and method calls togetLogsByResourceandcountLogsByResourceare all correctly implemented and match the expected pattern for the Audit 2.0.0 upgrade.Also applies to: 1164-1169, 1216-1216
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Appwrite/Platform/Workers/Audits.php (1)
52-63: PHPDoc parameter is out of sync with the method signature.Line 54 still references
@param callable $getProjectDBbut this parameter no longer exists in the method signature on line 63. The PHPDoc should be updated to reflect the current parameters.🔎 Proposed fix
/** * @param Message $message - * @param callable $getProjectDB * @param Document $project * @param callable $getAudit * @return Commit|NoCommit
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/worker.php(2 hunks)src/Appwrite/Platform/Workers/Audits.php(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/worker.php (1)
app/realtime.php (1)
getProjectDB(79-131)
src/Appwrite/Platform/Workers/Audits.php (1)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Logs/XList.php (1)
action(74-144)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: E2E Service Test (VCS)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Migrations)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (Tokens)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: Unit Test
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: E2E General Test
- GitHub Check: scan
🔇 Additional comments (4)
src/Appwrite/Platform/Workers/Audits.php (2)
105-105: Field rename from 'timestamp' to 'time' is correct.Per the past discussion, this aligns with the audit collection's actual attribute name. The audit library was previously mapping 'timestamp' to 'time' internally, so this change eliminates the inconsistency.
138-139: DI-based Audit instantiation looks good.The change from direct instantiation to using the
$getAuditcallable aligns with the broader refactoring pattern across the codebase, ensuring consistent adapter-based Audit creation.app/worker.php (2)
24-25: Import namespaces are correct for utopia-php/audit v2.0.0.The imports use the correct singular
Adapternamespace (Utopia\Audit\Adapter\Database) which aligns with the v2.0.0 API.
471-482: Well-structuredgetAuditresource factory.The implementation correctly:
- Returns a factory closure for project-aware Audit instantiation
- Handles console/empty projects using
dbForPlatform- Uses
getProjectDBfor project-specific databases- Follows the same DI pattern as other resources in this file
…t-audits-upgrade
lohanidamodar
force-pushed
the
feat-audits-upgrade
branch
from
cf991bc to
33b9f33
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
composer.json (2)
50-50: Dev branch dependency already flagged in previous review.The use of
dev-feat-db-3xinstead of a stable release was already identified. Ensure this is resolved before merging to a stable branch.
112-112: Missing trailing newline already flagged.src/Appwrite/Platform/Workers/Audits.php (1)
105-105: Field rename from 'timestamp' to 'time' already discussed and confirmed.Per the author's comments, this aligns the field name with the audit collection attribute.
🧹 Nitpick comments (1)
app/worker.php (1)
455-466: NewgetAuditresource follows established patterns and is correctly implemented.The resource correctly handles platform vs. per-project Audit instance creation using dependency injection. Consider a minor simplification to reduce duplication:
🔎 Optional: Reduce duplication
Server::setResource('getAudit', function (Database $dbForPlatform, callable $getProjectDB) { return function (Document $project) use ($dbForPlatform, $getProjectDB) { - if ($project->isEmpty() || $project->getId() === 'console') { - $adapter = new AdapterDatabase($dbForPlatform); - return new UtopiaAudit($adapter); - } - - $dbForProject = $getProjectDB($project); - $adapter = new AdapterDatabase($dbForProject); - return new UtopiaAudit($adapter); + $db = ($project->isEmpty() || $project->getId() === 'console') + ? $dbForPlatform + : $getProjectDB($project); + return new UtopiaAudit(new AdapterDatabase($db)); }; }, ['dbForPlatform', 'getProjectDB']);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
app/controllers/api/projects.phpapp/worker.phpcomposer.jsonsrc/Appwrite/Platform/Workers/Audits.php
🚧 Files skipped from review as they are similar to previous changes (1)
- app/controllers/api/projects.php
🧰 Additional context used
🧬 Code graph analysis (2)
src/Appwrite/Platform/Workers/Audits.php (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Logs/XList.php (1)
action(78-156)src/Appwrite/Platform/Modules/Databases/Http/Databases/Logs/XList.php (1)
action(74-144)
app/worker.php (3)
src/Appwrite/Event/Audit.php (1)
Audit(8-162)app/realtime.php (1)
getProjectDB(80-132)src/Appwrite/Utopia/Response/Model/Document.php (1)
Document(8-115)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: E2E Service Test (Users)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Shared Mode Service Test (Dev Keys) (Shared V2)
- GitHub Check: E2E Shared Mode Service Test (Dev Keys) (Shared V1)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: E2E Shared Mode Service Test (Site Screenshots) (Shared V1)
- GitHub Check: E2E Shared Mode Service Test (Site Screenshots) (Shared V2)
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: E2E General Test
- GitHub Check: Benchmark
🔇 Additional comments (5)
composer.json (1)
55-55: Version constraint change is acceptable.The change from
"3.*"to"3.*.*"is functionally equivalent for Composer's semver resolution and has no practical impact.app/worker.php (1)
24-25: Imports are correctly namespaced for the audit v2 adapter pattern.The singular
Adapternamespace aligns with the utopia-php/audit v2 API and matches usage across other files in this PR.src/Appwrite/Platform/Workers/Audits.php (3)
45-46: Injection updated correctly to use the newgetAuditresource.The change from
getProjectDBtogetAuditaligns with the new resource pattern introduced inapp/worker.php.
63-63: Action method signature correctly updated.The parameter order matches the injection order in the constructor, and the
$getAuditcallable is properly typed.
133-147: Batch processing correctly uses the injected Audit factory.The loop properly retrieves the Audit instance per project using
$getAudit($projectDocument)and handles errors with appropriate try/catch/finally cleanup.
| * @param callable $getProjectDB | ||
| * @param Document $project | ||
| * @param callable $getAudit |
There was a problem hiding this comment.
Stale docblock parameter reference.
Line 54 still references $getProjectDB which no longer exists in the method signature. This appears to be a leftover from the refactor.
🔎 Proposed fix
/**
* @param Message $message
- * @param callable $getProjectDB
* @param Document $project
* @param callable $getAudit
* @return Commit|NoCommit📝 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.
| * @param callable $getProjectDB | |
| * @param Document $project | |
| * @param callable $getAudit | |
| /** | |
| * @param Message $message | |
| * @param Document $project | |
| * @param callable $getAudit | |
| * @return Commit|NoCommit |
🤖 Prompt for AI Agents
In src/Appwrite/Platform/Workers/Audits.php around lines 54 to 56, the docblock
still references a parameter named $getProjectDB that no longer exists in the
method signature; update the docblock to match the current signature by removing
the stale @param entry for $getProjectDB (or renaming it if the parameter was
renamed) and ensure the remaining @param tags and types correspond exactly to
the actual method parameters and types.
|
|
||
| try { | ||
| $this->deleteByGroup(Audit::COLLECTION, [ | ||
| $this->deleteByGroup(SQL::COLLECTION, [ |
There was a problem hiding this comment.
Why do we need this constant? can we abstract it behind the delete method? we already Iintialize the adapter so we should be able to know this internally.
| } | ||
|
|
||
| if ($create || $projectTables) { | ||
| $audit = new Audit($dbForProject); |
There was a problem hiding this comment.
Can we remove all audit related stuff from the project creation once we complete the switch?
|
|
||
| Server::setResource('executor', fn () => new Executor()); | ||
|
|
||
| Server::setResource('getAudit', function (Database $dbForPlatform, callable $getProjectDB) { |
There was a problem hiding this comment.
For future we need to avoid using DI to return callbacks, this can be done by standardizing the way we pass project / user to message jobs which will allow us to auto create needed dependencies without needing to pass params to every dependency.
There was a problem hiding this comment.
Yes thi will need a proper refactor.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Appwrite/Platform/Workers/Deletes.php (1)
190-190: Fix incorrect parameters in deleteAuditLogs call.The call to
deleteAuditLogspasses incorrect parameters. The method signature at line 787 expects(Document $project, string $auditRetention, callable $getAudit), but this call provides($project, $getProjectDB, $auditRetention). This will cause a type error at runtime whenDELETE_TYPE_MAINTENANCEis triggered.🔎 Proposed fix
- $this->deleteAuditLogs($project, $getProjectDB, $auditRetention); + $this->deleteAuditLogs($project, $auditRetention, $getAudit);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Appwrite/Platform/Workers/Deletes.php
🧰 Additional context used
🧬 Code graph analysis (1)
src/Appwrite/Platform/Workers/Deletes.php (2)
src/Appwrite/Event/Audit.php (1)
Audit(8-162)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Create.php (1)
cleanup(393-410)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (2)
src/Appwrite/Platform/Workers/Deletes.php (2)
523-523: SQL::COLLECTION usage aligns with audit 2.0.0 upgrade.The change from
Audit::COLLECTIONtoSQL::COLLECTIONis part of the audit library upgrade to version 2.0.0. A previous review comment questioned whether this constant should be abstracted behind the delete method. While that architectural improvement remains valid, this change correctly reflects the new audit library's adapter pattern.Based on learnings, the previous reviewer suggested abstracting the collection constant.
787-798: The implementation correctly uses the audit library'scleanup()method. The method signature accepts\DateTimeas documented, and catchingThrowableis appropriate since the underlying adapters may throw various exception types.
3 participants
What does this PR do?
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Screenshots may also be helpful.)
Related PRs and Issues
Checklist