add functionsEvents and webhooksEvents#11033
Conversation
📝 WalkthroughWalkthroughAdds a new Appwrite\Functions\EventProcessor that enumerates and caches per-project function events and enumerates webhook events. Registers EventProcessor as a public resource and injects it across dbForProject, API init/shutdown, transaction handlers, and document/table create/update/upsert/bulk flows. Updates triggerBulk and related action signatures to accept Database $dbForProject and EventProcessor $eventProcessor. Event dispatch now generates per-document events, always emits realtime events, and only enqueues functions/webhooks when generated events intersect the cached function/webhook event lists. Removes legacy immediate Publisher-based function/webhook dispatch wiring. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ 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 |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
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 (1)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php (1)
394-399: Webhook triggering logic is correct, but review contains inaccuracy about event filtering mechanism.Event::generateEvents exists and produces the expected array of event patterns. The intersection check correctly determines if any generated events match the configured webhook events. However, the code uses
getAttribute('webhookEvents', [])for the filtering, notsubQueryWebhookEvents—the latter is a database filter used during project initialization to load webhook data, not in this triggering logic.
🧹 Nitpick comments (1)
app/init/database/filters.php (1)
173-195: Webhook events aggregation logic is correct but consider performance.The filter correctly:
- Queries all webhooks for the project
- Collects events from each webhook
- Deduplicates using
array_unique()However, with many webhooks per project, this could result in:
- Multiple database round-trips if webhooks have relationships
- Large array operations for projects with many webhooks
- Repeated computation on every project read
Consider:
- Adding an index on
projectInternalIdin the webhooks table if not already present- Monitoring query performance for projects with many webhooks (check if APP_LIMIT_SUBQUERY is appropriate)
- If this becomes a bottleneck, consider caching the aggregated events or computing them during webhook create/update operations
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/config/collections/platform.phpapp/init/database/filters.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-12T04:07:12.576Z
Learnt from: abnegate
Repo: appwrite/appwrite PR: 10800
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Delete.php:153-154
Timestamp: 2025-11-12T04:07:12.576Z
Learning: In Appwrite's event queue system, `queueForRealtime`, `queueForFunctions`, and `queueForWebhooks` are copied from `queueForEvents`. Therefore, when resetting event queues in transaction staging paths, only `$queueForEvents->reset()` needs to be called in most cases. The other queues only require explicit reset when they are triggered in a loop.
Applied to files:
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.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/init/database/filters.php
🧬 Code graph analysis (1)
app/init/database/filters.php (2)
src/Appwrite/Utopia/Request.php (1)
addFilter(92-95)src/Appwrite/Utopia/Response.php (1)
addFilter(933-936)
⏰ 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/Modules/Databases/Http/Databases/Collections/Documents/Action.php (2)
381-385: Verify project context is available.The code fetches the project from
$queueForEvents->getProject()and generates events usingEvent::generateEvents(). Ensure that:
- The project document is properly set on the event queue before
triggerBulkis called- The project document includes the new
webhookEventsandfunctionEventsattributes populated by their respective filters
381-392: Critical: Function and webhook triggering will never work - events attributes are never populated.Both
functionEventsandwebhookEventsare never set anywhere in the codebase, so these conditions will always evaluate to false and triggers will never execute. Additionally, thesubQueryFunctionEventsfilter explicitly returns an empty array due to functions being stored in the project database while this check occurs in the platform database context. This architectural limitation requires a different approach to trigger functions and webhooks from database events.This code was just added (2025-12-28) and appears incomplete—the feature is non-functional as currently implemented.
⛔ Skipped due to learnings
Learnt from: abnegate Repo: appwrite/appwrite PR: 10800 File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Delete.php:153-154 Timestamp: 2025-11-12T04:07:12.576Z Learning: In Appwrite's event queue system, `queueForRealtime`, `queueForFunctions`, and `queueForWebhooks` are copied from `queueForEvents`. Therefore, when resetting event queues in transaction staging paths, only `$queueForEvents->reset()` needs to be called in most cases. The other queues only require explicit reset when they are triggered in a loop.
✨ Benchmark results
⚡ Benchmark Comparison
|
… caching for function events retrieval
…s-and-functions-events
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php (2)
464-466: Silent exception swallowing may hide issues.Catching all
\Throwableand returning an empty array silently could mask database connectivity issues or configuration problems. Consider logging the exception for observability.🔎 Suggested improvement
} catch (\Throwable $e) { + // Log error for observability while gracefully degrading + // Console::error('Failed to fetch function events: ' . $e->getMessage()); return []; }
474-498: Consider caching webhook events for consistency.Unlike
getFunctionEvents,getWebhookEventsfetches from the project document on every bulk call without caching. While webhooks are embedded in the project document (likely already in memory), consider whether cache consistency is a concern if webhooks are modified mid-operation in another request.Also, webhook cache invalidation would need to be added if caching is introduced (similar to
purgeFunctionEventsCache).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/Appwrite/Platform/Modules/Compute/Base.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Delete.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Update.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Upsert.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.phpsrc/Appwrite/Platform/Modules/Functions/Http/Functions/Delete.phpsrc/Appwrite/Platform/Modules/Functions/Http/Functions/Update.php
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-11-12T04:07:12.576Z
Learnt from: abnegate
Repo: appwrite/appwrite PR: 10800
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Delete.php:153-154
Timestamp: 2025-11-12T04:07:12.576Z
Learning: In Appwrite's event queue system, `queueForRealtime`, `queueForFunctions`, and `queueForWebhooks` are copied from `queueForEvents`. Therefore, when resetting event queues in transaction staging paths, only `$queueForEvents->reset()` needs to be called in most cases. The other queues only require explicit reset when they are triggered in a loop.
Applied to files:
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Update.phpsrc/Appwrite/Platform/Modules/Functions/Http/Functions/Update.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Delete.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Upsert.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.php
📚 Learning: 2025-12-29T10:15:21.829Z
Learnt from: fogelito
Repo: appwrite/appwrite PR: 11036
File: src/Appwrite/Platform/Workers/Functions.php:125-126
Timestamp: 2025-12-29T10:15:21.829Z
Learning: In src/Appwrite/Platform/Workers/Functions.php, the event-driven function lookup intentionally uses Query::select(['$id', 'events']) in the main query to avoid subqueries for variables relations, then re-fetches full function documents only for filtered results. This N+1 pattern is a performance optimization when filtering significantly reduces the result set.
Applied to files:
src/Appwrite/Platform/Modules/Functions/Http/Functions/Update.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.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/Action.php
📚 Learning: 2025-05-10T04:12:13.037Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Indexes/Delete.php:85-86
Timestamp: 2025-05-10T04:12:13.037Z
Learning: The class `Appwrite\Platform\Modules\Databases\Http\Databases\Collections\Indexes\Action` defines a method `getGrantParentNotFoundException()` which is used to handle cases where a parent resource (like a database collection) is not found.
Applied to files:
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.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/Modules/Databases/Http/Databases/Collections/Documents/Action.php
📚 Learning: 2025-06-19T09:20:03.312Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php:57-59
Timestamp: 2025-06-19T09:20:03.312Z
Learning: In table-related endpoints (such as `src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php`), parameter descriptions should use "table" and "row" terminology instead of "collection" and "document" for clarity and consistency.
Applied to files:
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.php
🧬 Code graph analysis (2)
src/Appwrite/Platform/Modules/Functions/Http/Functions/Update.php (1)
src/Appwrite/Platform/Modules/Compute/Base.php (1)
purgeFunctionEventsCache(346-363)
src/Appwrite/Platform/Modules/Functions/Http/Functions/Delete.php (2)
src/Appwrite/Utopia/Response/Model/Document.php (1)
Document(8-115)src/Appwrite/Platform/Modules/Compute/Base.php (1)
purgeFunctionEventsCache(346-363)
⏰ 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/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Upsert.php (1)
212-213: LGTM! Database handle correctly passed to bulk trigger.The addition of
$dbForProjectenables project-scoped caching of function and webhook events, optimizing event triggering for bulk operations.src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.php (1)
494-495: LGTM! Database handle correctly passed to bulk trigger.The addition of
$dbForProjectaligns with the caching optimization for function and webhook events across bulk document operations.src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Delete.php (1)
206-207: LGTM! Database handle correctly passed to bulk trigger.Consistent with other bulk operations, enabling project-scoped event caching.
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Update.php (1)
237-238: LGTM! Database handle correctly passed to bulk trigger.This completes the consistent pattern across all bulk operations for project-scoped event caching.
src/Appwrite/Platform/Modules/Compute/Base.php (1)
346-363: LGTM! Cache invalidation logic is correct and consistent.The cache key format matches exactly with
getFunctionEvents()inAction.php, ensuring proper cache invalidation when functions are modified or deleted. The early-return guards for empty or console projects are appropriate.src/Appwrite/Platform/Modules/Functions/Http/Functions/Delete.php (2)
62-73: LGTM! Proper dependency injection for cache invalidation.The
projectdependency is correctly injected and passed to the action method, enabling cache purge on function deletion.
101-103: LGTM! Cache purge placement is correct.The cache is purged after the function is successfully deleted from the database, ensuring stale function events are invalidated for subsequent bulk document operations.
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php (2)
355-365: LGTM! Good optimization to retrieve events once per bulk operation.The
triggerBulksignature change properly threads the database context through for event caching, and retrievingfunctionEvents/webhookEventsoutside the loop avoids redundant cache/DB lookups per document.
389-407: Good optimization: conditional event triggering reduces unnecessary queue work.The intersection check ensures functions and webhooks are only triggered when their subscribed events match the generated events for the document operation.
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php
Outdated
Show resolved
Hide resolved
src/Appwrite/Platform/Modules/Functions/Http/Functions/Update.php
Outdated
Show resolved
Hide resolved
…s; change Traefik image version in docker-compose; add debug output in Action.php
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php (1)
446-449: Address truncation of functions in event cache.This query fetches at most
APP_LIMIT_SUBQUERY(1000) functions. Projects with more functions will have incomplete event coverage, causing some functions to not trigger on document events.Consider implementing pagination or documenting this as a known limitation.
🧹 Nitpick comments (1)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php (1)
465-467: Consider logging the exception for observability.Silently returning an empty array could mask underlying issues (e.g., database connectivity problems). While this prevents document operations from failing, it makes debugging difficult when functions unexpectedly don't trigger.
Consider adding logging before returning:
🔎 Suggested improvement
} catch (\Throwable $e) { + // Consider: \Utopia\App::getResource('log')->error('Failed to fetch function events: ' . $e->getMessage()); return []; }
📜 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 (3)
composer.jsondocker-compose.ymlsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php
✅ Files skipped from review due to trivial changes (1)
- composer.json
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-12-29T10:15:21.829Z
Learnt from: fogelito
Repo: appwrite/appwrite PR: 11036
File: src/Appwrite/Platform/Workers/Functions.php:125-126
Timestamp: 2025-12-29T10:15:21.829Z
Learning: In src/Appwrite/Platform/Workers/Functions.php, the event-driven function lookup intentionally uses Query::select(['$id', 'events']) in the main query to avoid subqueries for variables relations, then re-fetches full function documents only for filtered results. This N+1 pattern is a performance optimization when filtering significantly reduces the result set.
Applied to files:
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.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/Action.php
📚 Learning: 2025-05-10T04:12:13.037Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Indexes/Delete.php:85-86
Timestamp: 2025-05-10T04:12:13.037Z
Learning: The class `Appwrite\Platform\Modules\Databases\Http\Databases\Collections\Indexes\Action` defines a method `getGrantParentNotFoundException()` which is used to handle cases where a parent resource (like a database collection) is not found.
Applied to files:
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.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/Modules/Databases/Http/Databases/Collections/Documents/Action.php
📚 Learning: 2025-11-12T04:07:12.576Z
Learnt from: abnegate
Repo: appwrite/appwrite PR: 10800
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Delete.php:153-154
Timestamp: 2025-11-12T04:07:12.576Z
Learning: In Appwrite's event queue system, `queueForRealtime`, `queueForFunctions`, and `queueForWebhooks` are copied from `queueForEvents`. Therefore, when resetting event queues in transaction staging paths, only `$queueForEvents->reset()` needs to be called in most cases. The other queues only require explicit reset when they are triggered in a loop.
Applied to files:
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php
🧬 Code graph analysis (1)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php (5)
src/Appwrite/Event/Event.php (3)
Event(10-678)generateEvents(492-604)from(614-624)src/Appwrite/Vcs/Comment.php (1)
isEmpty(58-61)src/Appwrite/Event/Audit.php (1)
getHostname(138-141)src/Appwrite/SDK/Method.php (1)
getNamespace(130-133)app/realtime.php (1)
getCache(137-157)
⏰ 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 (7)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php (6)
10-10: LGTM!Import is correctly added for the
Queryclass used in thegetFunctionEventsmethod.
352-365: LGTM!The
Database $dbForProjectparameter addition is well-documented and properly integrated. As a protected method, the signature change is safely scoped to subclasses.
374-378: LGTM!Good design: fetching function and webhook events once before the document loop avoids redundant cache lookups or database queries per document.
389-408: LGTM!The conditional triggering logic correctly uses set intersection to determine if function/webhook events match the generated events. This optimization avoids unnecessary queue triggers when no handlers are subscribed to the specific events.
Note:
Event::generateEventsis called per document, which is necessary for accurate event matching. For very large bulk operations this could have some overhead, but the benefit of avoiding unnecessary queue work outweighs this cost.
470-499: LGTM!The method correctly filters only enabled webhooks and aggregates their events. Unlike
getFunctionEvents, caching isn't needed here since webhooks are already loaded as a project attribute (no additional database query).
430-437: No action needed. The cache key format in this code matches exactly with thepurgeFunctionEventsCacheimplementation inCompute/Base.php(lines 353-360). Both use the identical format string and parameter order, ensuring proper cache invalidation when functions are updated or deleted.Likely an incorrect or invalid review comment.
docker-compose.yml (1)
15-16: Configuration is compatible with Traefik 3.6.The upgrade from Traefik 2.11 to 3.6 is safe given the current configuration. All command flags (
--providers.file.*,--providers.docker.*,--entrypoints.*,--api.insecure,--accesslog) use valid Traefik 3.x syntax, and all service labels follow the standard Traefik 3.x docker label format (traefik.http.routers.*,traefik.http.services.*). No deprecated options are present. The upgrade appears necessary for Docker API 1.52 compatibility.
| * @param Database $dbForProject | ||
| * @return array | ||
| */ | ||
| protected function getFunctionEvents(?Document $project, Database $dbForProject): array |
There was a problem hiding this comment.
Does this need to happen only on the DB controller or on a more global shutdown? Don't we face same issue also on general events and not only bulk ops? For example events from the account or users service?
There was a problem hiding this comment.
There was a problem hiding this comment.
These are all just for transactions, what about our other events?
There was a problem hiding this comment.
It looks like they all resolve here
https://github.com/appwrite/appwrite/blob/add-webhooks-and-functions-events/app/controllers/shared/api.php#L94
and here
appwrite/app/controllers/shared/api.php
Line 821 in dc85d44
if i got it right
There was a problem hiding this comment.
Yep only bulk ops go through their specific routes, only transactions and the bulk API, because they fire multiple events per request. I think we can still add this function to the base Action, even though we don't use it in other APIs now, we might in future
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php
Outdated
Show resolved
Hide resolved
| $ttl = 3600; // 1 hour cache TTL | ||
| $cachedFunctionEvents = $dbForProject->getCache()->load($cacheKey, $ttl); | ||
|
|
||
| if ($cachedFunctionEvents !== false) { |
There was a problem hiding this comment.
Would be better to store the events as a hash map for quick key based access, this will make check much faster and we don't need to access specific properties or loop through the events objects.
There was a problem hiding this comment.
You mean turn it to associative array before saving it?
There was a problem hiding this comment.
yeh, just thinking out load. Is it possible? might not work with the approach of reusing the new list cache feature.
There was a problem hiding this comment.
@eldadfux I don't think we can use list caching here, that only applies in the list documents endpoint
There was a problem hiding this comment.
Can't we apply it as a feature of utopia/dbs?
| * @param Database $dbForProject | ||
| * @return void | ||
| */ | ||
| protected function purgeFunctionEventsCache(Document $project, Database $dbForProject): void |
There was a problem hiding this comment.
Would be really neat if we could do this via event listeners or filters. We had this idea long time ago to avoid having to remember to purge on each update which is very error prone.
There was a problem hiding this comment.
I think we can add listeners to UPDATE/DELETE operations.
But it will be invoked for all collections, and we will need to add
a collection validation condition in the purge function, isn't it an overhead
if ($document->getCollection() !== 'functions') { return; }
There was a problem hiding this comment.
Check with Jake if we can have a collection specific listener, and use * to listen to all collections
There was a problem hiding this comment.
I think we can add listeners to UPDATE/DELETE operations. But it will be invoked for all collections, and we will need to add a collection validation condition in the purge function, isn't it an overhead
if ($document->getCollection() !== 'functions') { return; }
The overhead is only the event function call right? Since we'd immediately return. This approach will be much easier than reworking events to be collection-specific
| * @param Database $dbForProject | ||
| * @return void | ||
| */ | ||
| protected function purgeFunctionEventsCache(Document $project, Database $dbForProject): void |
There was a problem hiding this comment.
I think we can add listeners to UPDATE/DELETE operations. But it will be invoked for all collections, and we will need to add a collection validation condition in the purge function, isn't it an overhead
if ($document->getCollection() !== 'functions') { return; }
The overhead is only the event function call right? Since we'd immediately return. This approach will be much easier than reworking events to be collection-specific
| * @param Database $dbForProject | ||
| * @return array | ||
| */ | ||
| protected function getFunctionEvents(?Document $project, Database $dbForProject): array |
There was a problem hiding this comment.
Yep only bulk ops go through their specific routes, only transactions and the bulk API, because they fire multiple events per request. I think we can still add this function to the base Action, even though we don't use it in other APIs now, we might in future
| $ttl = 3600; // 1 hour cache TTL | ||
| $cachedFunctionEvents = $dbForProject->getCache()->load($cacheKey, $ttl); | ||
|
|
||
| if ($cachedFunctionEvents !== false) { |
There was a problem hiding this comment.
@eldadfux I don't think we can use list caching here, that only applies in the list documents endpoint
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php
Outdated
Show resolved
Hide resolved
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php
Outdated
Show resolved
Hide resolved
src/Appwrite/Platform/Modules/Functions/Http/Functions/Update.php
Outdated
Show resolved
Hide resolved
…tion events and webhooks; add cache purging on function create/update/delete events
…s-and-functions-events
…whitespace in Update class
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @app/controllers/shared/api.php:
- Line 138: Remove the debug var_dump call that prints ['purged' => $cacheKey]
in app/controllers/shared/api.php (the stray var_dump line shown) before
merging; delete that statement and, if you need traceability, replace it with a
proper logger call (e.g., use the existing logger or Cache::log/purge method)
that records the $cacheKey at an appropriate log level instead of dumping to
output.
🧹 Nitpick comments (1)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Action.php (1)
160-162: Silent exception swallowing may hide operational issues.Catching all
Throwableand returning an empty array silently could mask database connectivity issues, query errors, or other problems that operators should be aware of. Consider logging the exception before returning the empty fallback.🔎 Proposed logging addition
} catch (\Throwable $e) { + // Consider adding logging here for operational visibility + // e.g., Console::error('Failed to fetch function events: ' . $e->getMessage()); return []; }
📜 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/shared/api.phpcomposer.jsonsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Action.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Transactions/Update.phpsrc/Appwrite/Platform/Modules/Functions/Http/Functions/Update.php
✅ Files skipped from review due to trivial changes (1)
- composer.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: fogelito
Repo: appwrite/appwrite PR: 11036
File: src/Appwrite/Platform/Workers/Functions.php:125-126
Timestamp: 2025-12-29T10:15:21.829Z
Learning: In src/Appwrite/Platform/Workers/Functions.php, the event-driven function lookup intentionally uses Query::select(['$id', 'events']) in the main query to avoid subqueries for variables relations, then re-fetches full function documents only for filtered results. This N+1 pattern is a performance optimization when filtering significantly reduces the result set.
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.
Learnt from: abnegate
Repo: appwrite/appwrite PR: 10800
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Delete.php:153-154
Timestamp: 2025-11-12T04:07:12.576Z
Learning: In Appwrite's event queue system, `queueForRealtime`, `queueForFunctions`, and `queueForWebhooks` are copied from `queueForEvents`. Therefore, when resetting event queues in transaction staging paths, only `$queueForEvents->reset()` needs to be called in most cases. The other queues only require explicit reset when they are triggered in a loop.
📚 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/Transactions/Update.php
📚 Learning: 2025-12-29T10:15:21.829Z
Learnt from: fogelito
Repo: appwrite/appwrite PR: 11036
File: src/Appwrite/Platform/Workers/Functions.php:125-126
Timestamp: 2025-12-29T10:15:21.829Z
Learning: In src/Appwrite/Platform/Workers/Functions.php, the event-driven function lookup intentionally uses Query::select(['$id', 'events']) in the main query to avoid subqueries for variables relations, then re-fetches full function documents only for filtered results. This N+1 pattern is a performance optimization when filtering significantly reduces the result set.
Applied to files:
src/Appwrite/Platform/Modules/Databases/Http/Databases/Transactions/Update.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Action.phpapp/controllers/shared/api.php
📚 Learning: 2025-11-12T04:07:12.576Z
Learnt from: abnegate
Repo: appwrite/appwrite PR: 10800
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Delete.php:153-154
Timestamp: 2025-11-12T04:07:12.576Z
Learning: In Appwrite's event queue system, `queueForRealtime`, `queueForFunctions`, and `queueForWebhooks` are copied from `queueForEvents`. Therefore, when resetting event queues in transaction staging paths, only `$queueForEvents->reset()` needs to be called in most cases. The other queues only require explicit reset when they are triggered in a loop.
Applied to files:
src/Appwrite/Platform/Modules/Databases/Http/Databases/Transactions/Update.phpapp/controllers/shared/api.php
📚 Learning: 2025-06-25T06:25:45.054Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Indexes/Delete.php:40-40
Timestamp: 2025-06-25T06:25:45.054Z
Learning: In Appwrite's database module, the event label for deleting a table index intentionally uses 'update' (e.g., 'databases.[databaseId].tables.[tableId].indexes.[indexId].update') instead of 'delete', to match legacy behavior as seen in previous controller implementations.
Applied to files:
src/Appwrite/Platform/Modules/Databases/Http/Databases/Transactions/Update.php
📚 Learning: 2025-06-19T09:20:03.312Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php:57-59
Timestamp: 2025-06-19T09:20:03.312Z
Learning: In table-related endpoints (such as `src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php`), parameter descriptions should use "table" and "row" terminology instead of "collection" and "document" for clarity and consistency.
Applied to files:
src/Appwrite/Platform/Modules/Databases/Http/Databases/Transactions/Update.php
📚 Learning: 2025-05-10T04:12:13.037Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Indexes/Delete.php:85-86
Timestamp: 2025-05-10T04:12:13.037Z
Learning: The class `Appwrite\Platform\Modules\Databases\Http\Databases\Collections\Indexes\Action` defines a method `getGrantParentNotFoundException()` which is used to handle cases where a parent resource (like a database collection) is not found.
Applied to files:
src/Appwrite/Platform/Modules/Databases/Http/Databases/Action.php
🧬 Code graph analysis (1)
app/controllers/shared/api.php (1)
src/Appwrite/Event/Database.php (2)
Database(10-200)getCollection(138-141)
⏰ 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/Modules/Databases/Http/Databases/Action.php (2)
165-194: LGTM!The
getWebhooksEventsmethod correctly filters enabled webhooks, aggregates their events, and returns a deduplicated map for O(1) lookups. The implementation is clean and consistent with the caching pattern used elsewhere.
99-163: Caching implementation looks good overall.The function events caching with Redis-backed storage, pagination through functions, and O(1) lookup map using
array_flipis well designed. The use ofQuery::select(['$id', 'events'])aligns with the established N+1 optimization pattern in the codebase. Based on learnings, this approach avoids subqueries for variables relations.src/Appwrite/Platform/Modules/Databases/Http/Databases/Transactions/Update.php (3)
373-376: LGTM!Retrieving the cached event maps once before the document iteration loop is an efficient optimization that avoids redundant cache lookups per document.
388-393: Event generation approach is correct.Using
Event::generateEvents()to create the full set of event permutations for each document operation enables precise matching against configured function and webhook events.
396-414: Conditional triggering implementation is well optimized.The O(1) event matching using the pre-computed maps and early break on first match is efficient. This reduces unnecessary queue triggers for documents that don't have matching function or webhook listeners.
app/controllers/shared/api.php (3)
115-140: Cache invalidation listener is correctly implemented.The listener properly filters for the 'functions' collection and constructs a cache key that matches the
getFunctionsEventsmethod exactly, ensuring cache coherence. The early returns for non-function documents and console projects are appropriate.
680-684: Database event bindings correctly wire all function lifecycle events.The cache purge listener is bound to CREATE, UPDATE, and DELETE events for comprehensive coverage. This addresses the past review concern about cache invalidation consistency across all function operations.
539-539: LGTM!The
$functionsEventsCacheListeneris correctly captured in theuseclause alongside existing listeners.src/Appwrite/Platform/Modules/Functions/Http/Functions/Update.php (1)
287-290: Formatting change approved; cache purge mechanism is properly implemented via database listener.The blank line addition between
$queueForEvents->setParam('functionId', $function->getId());and$response->dynamic($function, Response::MODEL_FUNCTION);is a formatting-only change with no functional impact.Cache invalidation for function updates is correctly handled by the
functionsEventsCacheListener, which is wired toDatabase::EVENT_DOCUMENT_UPDATE,Database::EVENT_DOCUMENT_CREATE, andDatabase::EVENT_DOCUMENT_DELETEinapp/controllers/shared/api.php. The listener properly filters for the 'functions' collection and covers all function lifecycle operations consistently.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @app/controllers/shared/api.php:
- Line 138: Remove the stray debug var_dump call present in
app/controllers/shared/api.php (the var_dump statement at the indicated
location); delete the var_dump line or replace it with a proper logging call
(e.g., use the application's logger at debug level) or guard it behind an
environment check so no debug output is emitted in production.
- Line 120: Remove the trailing whitespace on the empty line in
app/controllers/shared/api.php that is causing the PSR-12 lint failure; open the
file (api.php), locate the stray blank line around the reported area and delete
the extra spaces so the line is truly empty (or remove the blank line if
unnecessary), then run the linter to confirm the PSR-12 error is resolved.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/controllers/shared/api.phpsrc/Appwrite/Platform/Modules/Compute/Base.php
✅ Files skipped from review due to trivial changes (1)
- src/Appwrite/Platform/Modules/Compute/Base.php
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-12T04:07:12.576Z
Learnt from: abnegate
Repo: appwrite/appwrite PR: 10800
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Delete.php:153-154
Timestamp: 2025-11-12T04:07:12.576Z
Learning: In Appwrite's event queue system, `queueForRealtime`, `queueForFunctions`, and `queueForWebhooks` are copied from `queueForEvents`. Therefore, when resetting event queues in transaction staging paths, only `$queueForEvents->reset()` needs to be called in most cases. The other queues only require explicit reset when they are triggered in a loop.
Applied to files:
app/controllers/shared/api.php
📚 Learning: 2025-12-29T10:15:21.829Z
Learnt from: fogelito
Repo: appwrite/appwrite PR: 11036
File: src/Appwrite/Platform/Workers/Functions.php:125-126
Timestamp: 2025-12-29T10:15:21.829Z
Learning: In src/Appwrite/Platform/Workers/Functions.php, the event-driven function lookup intentionally uses Query::select(['$id', 'events']) in the main query to avoid subqueries for variables relations, then re-fetches full function documents only for filtered results. This N+1 pattern is a performance optimization when filtering significantly reduces the result set.
Applied to files:
app/controllers/shared/api.php
🪛 GitHub Actions: Linter
app/controllers/shared/api.php
[error] 1-1: PSR-12 lint issue detected by Pint: statement_indentation, no_whitespace_in_bl… (lint failure reported by Laravel Pint).
⏰ 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: Benchmark
- GitHub Check: Setup & Build Appwrite Image
🔇 Additional comments (2)
app/controllers/shared/api.php (2)
681-684: LGTM! Cache purge listeners properly integrated.The function events cache purge listener is correctly wired to all relevant database mutation events (CREATE, UPDATE, DELETE). The parameters and integration look correct.
115-140: No action required. The cache key format is consistent across bothfunctionsEventsCacheListener(which purges the cache) andgetFunctionsEvents()(which reads from the cache). Both use the identical key format and parameters, ensuring proper cache invalidation without misses.
app/controllers/shared/api.php
Outdated
| */ | ||
| $functionsEventsCacheListener = function (string $event, Document $document, Document $project, Database $dbForProject) { | ||
|
|
||
|
|
There was a problem hiding this comment.
Remove trailing whitespace to fix PSR-12 lint failure.
This empty line contains trailing whitespace, which violates PSR-12 coding standards and is causing the pipeline failure.
🔎 Proposed fix
-
+
if ($document->getCollection() !== 'functions') {📝 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.
🤖 Prompt for AI Agents
In @app/controllers/shared/api.php at line 120, Remove the trailing whitespace
on the empty line in app/controllers/shared/api.php that is causing the PSR-12
lint failure; open the file (api.php), locate the stray blank line around the
reported area and delete the extra spaces so the line is truly empty (or remove
the blank line if unnecessary), then run the linter to confirm the PSR-12 error
is resolved.
app/controllers/shared/api.php
Outdated
| $dbForProject->getTenant(), | ||
| $project->getId() | ||
| ); | ||
|
|
There was a problem hiding this comment.
Remove debug var_dump statement before merging.
This debug output will pollute logs/responses in production and should be removed.
🔎 Proposed fix
- var_dump(['purged' => $cacheKey]);
$dbForProject->getCache()->purge($cacheKey);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @app/controllers/shared/api.php at line 138, Remove the stray debug var_dump
call present in app/controllers/shared/api.php (the var_dump statement at the
indicated location); delete the var_dump line or replace it with a proper
logging call (e.g., use the application's logger at debug level) or guard it
behind an environment check so no debug output is emitted in production.
app/controllers/shared/api.php
Outdated
| $queueForRealtime->from($queueForEvents) | ||
| )); | ||
| )) | ||
| ->on(Database::EVENT_DOCUMENT_CREATE, 'purge-function-events-cache', fn ($event, $document) => $functionsEventsCacheListener($event, $document, $project, $dbForProject)) |
There was a problem hiding this comment.
@abnegate as I mentioned in the engineering leads channel, this approach is really bad, we should not register events on each request. At worst it should happen inside the DI when creating the resource.
| $route = $utopia->getRoute(); | ||
| $requestParams = $route->getParamsValues(); | ||
|
|
||
| /** |
There was a problem hiding this comment.
Why did we remove this? this was just added today/yesterday.
| $queueForRealtime, | ||
| $queueForFunctions, | ||
| $queueForWebhooks | ||
| $queueForWebhooks, |
There was a problem hiding this comment.
I'm very confused about what we're doing here. This check should happen globally for all events we fire, but it seems like we're focused on solving this only on the db service scope. I understand that we have the transactions/batch operations firing event separately, but this is just part of the problem - we're firing events from account, users, storage, functions, messaging, sites etc. We need to fire events only if a function/webhook is actually listening to them.
…events; streamline event triggering in database actions
…s-and-functions-events
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/Appwrite/Functions/EventProcessor.php (2)
71-73: Consider logging errors before returning empty array.The catch-all
Throwablehandler silently returns an empty array, which might mask database connection issues, query errors, or cache failures. This could make debugging difficult.💡 Log exceptions for observability
} catch (\Throwable $e) { + // Log error for debugging while maintaining defensive behavior + error_log("EventProcessor::getFunctionsEvents failed: " . $e->getMessage()); return []; }
81-105: Inconsistent caching between functions and webhooks.
getFunctionsEventsimplements Redis caching with 1-hour TTL, butgetWebhooksEventsreads directly from the project document without caching. This inconsistency may indicate:
- Webhooks are already cached at the project document level
- Different performance characteristics justify different approaches
- An oversight in implementation
Consider documenting why the caching strategies differ, or evaluate whether webhooks would benefit from similar caching.
src/Appwrite/Platform/Modules/Databases/Http/Databases/Transactions/Update.php (1)
108-108: Consider removing or documenting the unused$userparameter.Static analysis flagged
$useras unused in this method. If it's intentionally kept for future use or API consistency, consider adding a brief comment. Otherwise, it could be removed from the signature.src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php (1)
399-399: Minor: Variable shadowing may cause confusion.The loop variable
$eventshadows the method parameter$eventon lines 399 and 410. While this works correctly, renaming the loop variable (e.g.,$generatedEvent) would improve clarity.♻️ Suggested improvement
- foreach ($generatedEvents as $event) { - if (isset($functionsEvents[$event])) { + foreach ($generatedEvents as $generatedEvent) { + if (isset($functionsEvents[$generatedEvent])) { $queueForFunctions ->from($queueForEvents) ->trigger(); break; } }Also applies to: 410-410
app/controllers/shared/api.php (1)
671-693: Consistent variable shadowing with Action.php.Same as in
Action.php, the loop variable$eventshadows the outer scope. For consistency, consider using$generatedEventhere as well if you apply the same refactor toAction.php.app/init/resources.php (1)
517-517: Remove unused injected queue dependencies.The parameters
$queueForFunctions,$queueForWebhooks, and$queueForRealtimeare injected on line 517 but immediately shadowed on lines 728-730 with new instances. Only$queueForEventsis actually used (via.from()calls). The publishers alone suffice to create the cloned queues.♻️ Proposed fix
-App::setResource('dbForProject', function (Group $pools, Database $dbForPlatform, Cache $cache, Document $project, Response $response, Publisher $publisher, Publisher $publisherFunctions, Publisher $publisherWebhooks, Event $queueForEvents, Func $queueForFunctions, Webhook $queueForWebhooks, Realtime $queueForRealtime, StatsUsage $queueForStatsUsage) { +App::setResource('dbForProject', function (Group $pools, Database $dbForPlatform, Cache $cache, Document $project, Response $response, Publisher $publisher, Publisher $publisherFunctions, Publisher $publisherWebhooks, Event $queueForEvents, StatsUsage $queueForStatsUsage) {And update the dependency list on line 756:
-}, ['pools', 'dbForPlatform', 'cache', 'project', 'response', 'publisher', 'publisherFunctions', 'publisherWebhooks', 'queueForEvents', 'queueForFunctions', 'queueForWebhooks', 'queueForRealtime', 'queueForStatsUsage']); +}, ['pools', 'dbForPlatform', 'cache', 'project', 'response', 'publisher', 'publisherFunctions', 'publisherWebhooks', 'queueForEvents', 'queueForStatsUsage']);Also applies to: 727-730, 756-756
📜 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 (14)
app/controllers/shared/api.phpapp/init/resources.phpsrc/Appwrite/Functions/EventProcessor.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Action.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Delete.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Update.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Upsert.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Transactions/Update.phpsrc/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Rows/Bulk/Delete.phpsrc/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Rows/Bulk/Update.phpsrc/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Rows/Bulk/Upsert.phpsrc/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Rows/Create.php
✅ Files skipped from review due to trivial changes (1)
- src/Appwrite/Platform/Modules/Databases/Http/Databases/Action.php
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: fogelito
Repo: appwrite/appwrite PR: 11036
File: src/Appwrite/Platform/Workers/Functions.php:125-126
Timestamp: 2025-12-29T10:15:21.829Z
Learning: In src/Appwrite/Platform/Workers/Functions.php, the event-driven function lookup intentionally uses Query::select(['$id', 'events']) in the main query to avoid subqueries for variables relations, then re-fetches full function documents only for filtered results. This N+1 pattern is a performance optimization when filtering significantly reduces the result set.
Learnt from: abnegate
Repo: appwrite/appwrite PR: 10800
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Delete.php:153-154
Timestamp: 2025-11-12T04:07:12.576Z
Learning: In Appwrite's event queue system, `queueForRealtime`, `queueForFunctions`, and `queueForWebhooks` are copied from `queueForEvents`. Therefore, when resetting event queues in transaction staging paths, only `$queueForEvents->reset()` needs to be called in most cases. The other queues only require explicit reset when they are triggered in a loop.
📚 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/Action.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Transactions/Update.php
📚 Learning: 2025-12-29T10:15:21.829Z
Learnt from: fogelito
Repo: appwrite/appwrite PR: 11036
File: src/Appwrite/Platform/Workers/Functions.php:125-126
Timestamp: 2025-12-29T10:15:21.829Z
Learning: In src/Appwrite/Platform/Workers/Functions.php, the event-driven function lookup intentionally uses Query::select(['$id', 'events']) in the main query to avoid subqueries for variables relations, then re-fetches full function documents only for filtered results. This N+1 pattern is a performance optimization when filtering significantly reduces the result set.
Applied to files:
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.phpsrc/Appwrite/Functions/EventProcessor.phpapp/controllers/shared/api.php
📚 Learning: 2025-05-10T04:12:13.037Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Indexes/Delete.php:85-86
Timestamp: 2025-05-10T04:12:13.037Z
Learning: The class `Appwrite\Platform\Modules\Databases\Http\Databases\Collections\Indexes\Action` defines a method `getGrantParentNotFoundException()` which is used to handle cases where a parent resource (like a database collection) is not found.
Applied to files:
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php
📚 Learning: 2025-11-12T04:07:12.576Z
Learnt from: abnegate
Repo: appwrite/appwrite PR: 10800
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Delete.php:153-154
Timestamp: 2025-11-12T04:07:12.576Z
Learning: In Appwrite's event queue system, `queueForRealtime`, `queueForFunctions`, and `queueForWebhooks` are copied from `queueForEvents`. Therefore, when resetting event queues in transaction staging paths, only `$queueForEvents->reset()` needs to be called in most cases. The other queues only require explicit reset when they are triggered in a loop.
Applied to files:
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Upsert.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Transactions/Update.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Update.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Delete.phpapp/controllers/shared/api.php
📚 Learning: 2025-06-25T06:25:45.054Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Indexes/Delete.php:40-40
Timestamp: 2025-06-25T06:25:45.054Z
Learning: In Appwrite's database module, the event label for deleting a table index intentionally uses 'update' (e.g., 'databases.[databaseId].tables.[tableId].indexes.[indexId].update') instead of 'delete', to match legacy behavior as seen in previous controller implementations.
Applied to files:
src/Appwrite/Platform/Modules/Databases/Http/Databases/Transactions/Update.phpsrc/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Rows/Bulk/Update.phpsrc/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Delete.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
📚 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 Appwrite's codebase, Utopia\Queue\Broker\Pool (aliased as BrokerPool) is a readonly class that implements both Publisher and Consumer interfaces, allowing it to be used directly in constructors expecting either interface type. This is extensively used throughout the codebase for queue operations.
Applied to files:
app/init/resources.phpapp/controllers/shared/api.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/init/resources.php
🧬 Code graph analysis (8)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Upsert.php (2)
src/Appwrite/Functions/EventProcessor.php (1)
EventProcessor(9-106)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Delete.php (1)
action(89-212)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Transactions/Update.php (1)
src/Appwrite/Functions/EventProcessor.php (3)
EventProcessor(9-106)getFunctionsEvents(17-74)getWebhooksEvents(81-105)
src/Appwrite/Functions/EventProcessor.php (3)
src/Appwrite/Vcs/Comment.php (1)
isEmpty(58-61)src/Appwrite/SDK/Method.php (1)
getNamespace(130-133)app/realtime.php (1)
getCache(137-157)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.php (2)
src/Appwrite/Functions/EventProcessor.php (1)
EventProcessor(9-106)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Delete.php (1)
action(89-212)
app/init/resources.php (1)
src/Appwrite/Functions/EventProcessor.php (1)
EventProcessor(9-106)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Update.php (3)
src/Appwrite/Functions/EventProcessor.php (1)
EventProcessor(9-106)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Delete.php (1)
action(89-212)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Upsert.php (1)
action(91-218)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Delete.php (1)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Upsert.php (1)
action(97-388)
app/controllers/shared/api.php (1)
src/Appwrite/Functions/EventProcessor.php (3)
EventProcessor(9-106)getFunctionsEvents(17-74)getWebhooksEvents(81-105)
🪛 PHPMD (2.15.0)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Transactions/Update.php
108-108: Avoid unused parameters such as '$user'. (undefined)
(UnusedFormalParameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (VCS)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Teams)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Tokens)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: E2E Service Test (Databases/Legacy)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: E2E General Test
- GitHub Check: Unit Test
- GitHub Check: E2E Service Test (Abuse enabled)
🔇 Additional comments (17)
src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Rows/Bulk/Update.php (1)
71-71: LGTM!Clean dependency injection wiring for
EventProcessor. The placement afterplanis consistent with other bulk operations in this PR.src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Rows/Bulk/Delete.php (1)
69-69: LGTM!Consistent dependency injection pattern for
EventProcessor.src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Rows/Create.php (1)
114-114: LGTM!EventProcessor injection follows the established pattern across bulk operations.
src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Rows/Bulk/Upsert.php (1)
71-71: LGTM!Final piece of the consistent EventProcessor dependency injection across all bulk operations.
src/Appwrite/Functions/EventProcessor.php (1)
35-40: Cache invalidation is already implemented — remove this concern.The codebase includes explicit cache invalidation in
app/init/resources.php(lines 748-750). A listener namedpurge-function-events-cacheis registered onDatabase::EVENT_DOCUMENT_CREATE,EVENT_DOCUMENT_UPDATE, andEVENT_DOCUMENT_DELETEevents for thefunctionscollection. The listener purges the function events cache immediately when functions are modified, using the same cache key asEventProcessor::getFunctionsEvents(). Function event changes are reflected instantly, not after an hour delay.Likely an incorrect or invalid review comment.
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Upsert.php (1)
8-8: LGTM - EventProcessor wiring is consistent with other bulk operations.The import, DI injection, action signature update, and
triggerBulkinvocation follow the same pattern as other bulk operations (Create, Update, Delete), ensuring consistent event processing across the codebase.Also applies to: 87-87, 91-91, 214-217
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.php (1)
8-8: LGTM - EventProcessor integration follows the established pattern.The single-document creation path correctly delegates event triggering to the API shutdown handler, while the bulk path properly passes
EventProcessortotriggerBulkfor conditional function/webhook triggering.Also applies to: 136-137, 139-139, 496-499
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Delete.php (1)
8-8: LGTM - EventProcessor wiring is correctly implemented.The implementation follows the same pattern as other bulk operations. Note that a previous reviewer raised concerns about this optimization being DB-scope only rather than global (affecting account, users, storage, etc.). That broader architectural discussion is beyond this file's scope, but the implementation here is consistent and correct.
Also applies to: 85-86, 89-89, 208-211
src/Appwrite/Platform/Modules/Databases/Http/Databases/Transactions/Update.php (1)
376-417: LGTM - Event gating logic is correctly implemented.The implementation efficiently caches function/webhook events once per transaction commit, then uses the cached data to conditionally trigger queues per document. The early
breakafter finding a matching event prevents unnecessary iterations.src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Update.php (1)
8-8: LGTM - Consistent EventProcessor integration.The changes mirror the pattern used in other bulk operations (Upsert, Delete, Create), maintaining consistency across the codebase.
Also applies to: 89-90, 93-93, 239-242
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php (1)
376-419: LGTM - Core event gating logic is well-structured.The implementation:
- Fetches cached events once before the document loop (efficient)
- Generates events per-document using
Event::generateEvents- Always triggers realtime (correct - realtime has its own filtering)
- Conditionally triggers functions/webhooks only when matching events exist
- Uses
isset()for O(1) lookups on flipped arrays- Breaks early after first match (avoids duplicate triggers)
app/controllers/shared/api.php (1)
655-693: LGTM - API shutdown event gating mirrors bulk operation logic.The implementation correctly:
- Fetches cached function/webhook events via
EventProcessor- Generates events for the current operation
- Triggers realtime unconditionally for non-console projects (correct - realtime handles its own filtering)
- Conditionally triggers functions/webhooks only when matching events exist
This provides consistent behavior between single-operation API calls and bulk operations.
app/init/resources.php (5)
157-160: LGTM!The
eventProcessorresource is correctly declared as a stateless singleton, consistent with how it's implemented in theEventProcessorclass.
553-589: LGTM!The isolated user creation event handling is well-documented and correctly addresses the gap where accounts created via anonymous, OAuth, phone flows would otherwise miss function/webhook/realtime triggers. The conditional checks for webhooks enablement and non-console projects are appropriate.
594-616: Cache invalidation logic is correct.The cache key format matches the one used in
EventProcessor::getFunctionsEvents(), ensuring proper cache coherence when functions are mutated. Minor style nit: lines 595-596 have consecutive blank lines.
618-723: LGTM!The usage metrics tracking is comprehensive, covering all relevant collection types with correct value calculations for create/delete/upsert operations. The
addReduce()calls for cascading cleanup on entity deletions follow the established pattern.
733-751: LGTM!The database event listener registration correctly wires all three listener functions to appropriate events. The
from($queueForEvents)pattern ensures cloned queues inherit the necessary state (project, user context) from the original event queue.
…s-and-functions-events
…s' into add-webhooks-and-functions-events
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @app/init/resources.php:
- Around line 563-593: Wrap the bodies of the three listeners
(eventDatabaseListener, functionsEventsCacheListener, usageDatabaseListener) in
try-catch blocks so any exception they throw does not bubble up into the
database operation; specifically, enclose the existing logic in try { ... } and
catch (\Throwable $e) { /* log the error using the project's logger or error_log
with context (listener name, project id, document id) and do not rethrow */ }
and apply the same change to the other two listeners referenced in the diff.
- Around line 598-620: The cache-key construction in the anonymous
$functionsEventsCacheListener duplicates EventProcessor logic; extract the key
generation into a single public method on EventProcessor (e.g.,
getFunctionEventsCacheKey(Document $project, Database $dbForProject): string)
that encapsulates the sprintf logic, then update $functionsEventsCacheListener
to call EventProcessor->getFunctionEventsCacheKey($project, $dbForProject)
instead of rebuilding the string; ensure EventProcessor is injected/available
where the listener is registered (or passed via the dbForProject resource) so
the listener can call the new method and then call getCache()->purge() with that
returned key.
🧹 Nitpick comments (2)
app/init/resources.php (2)
565-567: Inefficient filtering: listener called for all document creations.The
eventDatabaseListeneris registered forDatabase::EVENT_DOCUMENT_CREATE(Line 743), which fires for every document creation across all collections, but it only processes documents from theuserscollection. This means the listener is invoked unnecessarily for most document creations.Consider registering a more specific listener or filtering at the registration level if supported by the Database API.
581-585: Consider using EventProcessor to check for webhook events.Line 581 checks
!empty($project->getAttribute('webhooks'))to determine whether to trigger webhook events, but this doesn't verify if there are any enabled webhooks with relevant events. According to the EventProcessor implementation,getWebhooksEvents()filters for enabled webhooks and returns only the unique events.♻️ Refactor to use EventProcessor for webhook checking
Inject
eventProcessorinto thedbForProjectresource and use it in the listener:- /** Trigger webhooks events only if a project has them enabled */ - if (!empty($project->getAttribute('webhooks'))) { + /** Trigger webhooks events only if a project has enabled webhooks with events */ + $webhookEvents = $eventProcessor->getWebhooksEvents($project); + if (!empty($webhookEvents)) { $queueForWebhooks ->from($queueForEvents) ->trigger(); }Based on learnings, this aligns with the PR's objective to filter events using cached event lists from EventProcessor.
📜 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 (1)
app/init/resources.php
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: fogelito
Repo: appwrite/appwrite PR: 11036
File: src/Appwrite/Platform/Workers/Functions.php:125-126
Timestamp: 2025-12-29T10:15:21.829Z
Learning: In src/Appwrite/Platform/Workers/Functions.php, the event-driven function lookup intentionally uses Query::select(['$id', 'events']) in the main query to avoid subqueries for variables relations, then re-fetches full function documents only for filtered results. This N+1 pattern is a performance optimization when filtering significantly reduces the result set.
📚 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-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 Appwrite's codebase, Utopia\Queue\Broker\Pool (aliased as BrokerPool) is a readonly class that implements both Publisher and Consumer interfaces, allowing it to be used directly in constructors expecting either interface type. This is extensively used throughout the codebase for queue operations.
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/init/resources.php
🧬 Code graph analysis (1)
app/init/resources.php (2)
src/Appwrite/Functions/EventProcessor.php (1)
EventProcessor(9-106)src/Appwrite/Event/StatsUsage.php (2)
addMetric(46-54)addReduce(32-37)
⏰ 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 (7)
app/init/resources.php (7)
521-521: Resource signature significantly expanded to support event-driven architecture.The
dbForProjectresource now depends on Response and multiple queue/publisher components. This expansion is consistent with the PR's goal of wiring event-handling constructs directly into the database resource.
622-727: LGTM! Comprehensive usage metrics calculation.The
usageDatabaseListenercorrectly handles usage metrics across all resource types (teams, users, sessions, databases, collections, documents, buckets, files, functions, sites, deployments). It properly:
- Calculates positive/negative values based on event type
- Uses
addReduce()for resources that require cleanup on deletion- Tracks both project-level and resource-specific metrics (per database, per collection, etc.)
729-734: LGTM! Queue cloning prevents event lifecycle conflicts.The comment clearly explains the rationale: cloning queues ensures that events triggered by database listeners don't overwrite events that should be triggered in the shutdown hook. Each queue is properly instantiated with its corresponding publisher, and the Realtime queue is correctly created without a publisher parameter.
737-755: LGTM! Event registration properly wired.The database event hooks are correctly registered:
calculate-usagelistener on create, delete, bulk create/delete/upsert events for comprehensive usage trackingcreate-trigger-eventslistener on document create for user creation eventspurge-function-events-cachelistener on function create/update/delete for cache invalidationThe event names and listener mappings are appropriate for their respective purposes.
760-760: No concerns found with expanded dependencies.The dependency list expansion in the
dbForProjectresource is internal to the resource closure itself. Search results confirm no code directly invokes this resource closure—all usages are through Utopia's dependency injection system, which automatically resolves and injects parameters. The expanded dependencies do not affect the resource's public API or any existing calling code.
563-593: No issue found - Response::output() is safe in database listener context.The
Response::output()method is a pure data serialization function that takes a Document and model, filters the document through the model's rules, and returns an array. It has no dependencies on HTTP state (headers, status codes, or response body management). The method is completely safe to call in any context, including database listeners. The code comment explicitly states this pattern is intentional.Likely an incorrect or invalid review comment.
162-164: EventProcessor is correctly registered and used in the appropriate scope.The
eventProcessorresource is registered at lines 162-164 and is properly injected intoapp/controllers/shared/api.phpwhere it's used to retrieve cached function and webhook events. The database listeners indbForProjecthave a different responsibility: they purge the cache when functions, webhooks, or other documents change. These are separate concerns—EventProcessorreads from the cache, while the listeners maintain cache invalidation—so injectingeventProcessorinto the database listeners is not necessary.Likely an incorrect or invalid review comment.
| $eventDatabaseListener = function (Document $project, Document $document, Response $response, Event $queueForEvents, Func $queueForFunctions, Webhook $queueForWebhooks, Realtime $queueForRealtime) { | ||
| // Only trigger events for user creation with the database listener. | ||
| if ($document->getCollection() !== 'users') { | ||
| return; | ||
| } | ||
|
|
||
| $queueForEvents | ||
| ->setEvent('users.[userId].create') | ||
| ->setParam('userId', $document->getId()) | ||
| ->setPayload($response->output($document, Response::MODEL_USER)); | ||
|
|
||
| // Trigger functions, webhooks, and realtime events | ||
| $queueForFunctions | ||
| ->from($queueForEvents) | ||
| ->trigger(); | ||
|
|
||
|
|
||
| /** Trigger webhooks events only if a project has them enabled */ | ||
| if (!empty($project->getAttribute('webhooks'))) { | ||
| $queueForWebhooks | ||
| ->from($queueForEvents) | ||
| ->trigger(); | ||
| } | ||
|
|
||
| /** Trigger realtime events only for non console events */ | ||
| if ($queueForEvents->getProject()->getId() !== 'console') { | ||
| $queueForRealtime | ||
| ->from($queueForEvents) | ||
| ->trigger(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Missing error handling in database listeners.
None of the three event listeners (eventDatabaseListener, functionsEventsCacheListener, usageDatabaseListener) include try-catch blocks. If any of these listeners throw an exception, it could propagate to the database operation and cause it to fail, leading to data inconsistency or failed transactions.
🛡️ Add error handling to prevent listener exceptions from breaking database operations
Wrap each listener's body in a try-catch block:
$eventDatabaseListener = function (Document $project, Document $document, Response $response, Event $queueForEvents, Func $queueForFunctions, Webhook $queueForWebhooks, Realtime $queueForRealtime) {
+ try {
// Only trigger events for user creation with the database listener.
if ($document->getCollection() !== 'users') {
return;
}
// ... rest of the listener logic ...
+ } catch (\Throwable $e) {
+ // Log the error but don't propagate to database operation
+ Console::error('Event listener error: ' . $e->getMessage());
+ }
};Apply similar error handling to functionsEventsCacheListener and usageDatabaseListener.
Also applies to: 598-620, 622-727
🤖 Prompt for AI Agents
In @app/init/resources.php around lines 563 - 593, Wrap the bodies of the three
listeners (eventDatabaseListener, functionsEventsCacheListener,
usageDatabaseListener) in try-catch blocks so any exception they throw does not
bubble up into the database operation; specifically, enclose the existing logic
in try { ... } and catch (\Throwable $e) { /* log the error using the project's
logger or error_log with context (listener name, project id, document id) and do
not rethrow */ } and apply the same change to the other two listeners referenced
in the diff.
| $functionsEventsCacheListener = function (string $event, Document $document, Document $project, Database $dbForProject) { | ||
|
|
||
|
|
||
| if ($document->getCollection() !== 'functions') { | ||
| return; | ||
| } | ||
|
|
||
| if ($project->isEmpty() || $project->getId() === 'console') { | ||
| return; | ||
| } | ||
|
|
||
| $hostname = $dbForProject->getAdapter()->getHostname(); | ||
| $cacheKey = \sprintf( | ||
| '%s-cache-%s:%s:%s:project:%s:functions:events', | ||
| $dbForProject->getCacheName(), | ||
| $hostname ?? '', | ||
| $dbForProject->getNamespace(), | ||
| $dbForProject->getTenant(), | ||
| $project->getId() | ||
| ); | ||
|
|
||
| $dbForProject->getCache()->purge($cacheKey); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Code duplication: cache key construction duplicated from EventProcessor.
Lines 610-617 duplicate the cache key construction logic from EventProcessor.getFunctionsEvents(). If the cache key format changes, both locations must be updated, creating a maintenance burden.
♻️ Refactor to eliminate duplication
Option 1: Extract cache key construction to EventProcessor
Add a public method to EventProcessor:
// In EventProcessor.php
public function getFunctionEventsCacheKey(Document $project, Database $dbForProject): string
{
$hostname = $dbForProject->getAdapter()->getHostname();
return \sprintf(
'%s-cache-%s:%s:%s:project:%s:functions:events',
$dbForProject->getCacheName(),
$hostname ?? '',
$dbForProject->getNamespace(),
$dbForProject->getTenant(),
$project->getId()
);
}Then use it in the listener:
- $hostname = $dbForProject->getAdapter()->getHostname();
- $cacheKey = \sprintf(
- '%s-cache-%s:%s:%s:project:%s:functions:events',
- $dbForProject->getCacheName(),
- $hostname ?? '',
- $dbForProject->getNamespace(),
- $dbForProject->getTenant(),
- $project->getId()
- );
+ $cacheKey = $eventProcessor->getFunctionEventsCacheKey($project, $dbForProject);
$dbForProject->getCache()->purge($cacheKey);This requires injecting eventProcessor into the dbForProject resource.
🤖 Prompt for AI Agents
In @app/init/resources.php around lines 598 - 620, The cache-key construction in
the anonymous $functionsEventsCacheListener duplicates EventProcessor logic;
extract the key generation into a single public method on EventProcessor (e.g.,
getFunctionEventsCacheKey(Document $project, Database $dbForProject): string)
that encapsulates the sprintf logic, then update $functionsEventsCacheListener
to call EventProcessor->getFunctionEventsCacheKey($project, $dbForProject)
instead of rebuilding the string; ensure EventProcessor is injected/available
where the listener is registered (or passed via the dbForProject resource) so
the listener can call the new method and then call getCache()->purge() with that
returned key.
…s-and-functions-events # Conflicts: # app/controllers/shared/api.php # app/init/resources.php # src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.php # src/Appwrite/Platform/Modules/Databases/Http/Databases/Transactions/Update.php # src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Rows/Bulk/Delete.php # src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Rows/Bulk/Update.php # src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Rows/Bulk/Upsert.php # src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Tables/Rows/Create.php # src/Appwrite/Platform/Modules/Databases/Http/TablesDB/Transactions/Update.php
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 (2)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Transactions/Update.php (1)
85-109: Remove unused$userparameter or suppress PHPMD warning.
The action method signature includes$userbut it's never referenced in the method body. Either remove the parameter and its injection if DI allows, or suppress the PHPMDUnusedFormalParameterwarning if the signature must remain for compatibility.🔧 Suggested suppression if the parameter must stay
/** * `@param` string $transactionId * `@param` bool $commit * `@param` bool $rollback * `@param` UtopiaResponse $response * `@param` Database $dbForProject * `@param` Document $user + * `@SuppressWarnings`(PHPMD.UnusedFormalParameter) * `@param` TransactionState $transactionState * `@param` Delete $queueForDeletes * `@param` Event $queueForEvents * `@param` StatsUsage $queueForStatsUsage * `@param` Event $queueForRealtime * `@param` Event $queueForFunctions * `@param` Event $queueForWebhooks * `@param` EventProcessor $eventProcessor */app/controllers/shared/api.php (1)
3-38: Remove unused imports to satisfy Pint (PSR-12 no_unused_imports).The following imports are unused and should be removed:
use Utopia\Queue\Publisher;use Appwrite\Event\Migration;use Appwrite\Event\Webhook;
♻️ Duplicate comments (3)
app/init/resources.php (3)
603-625: Cache key construction duplicatesEventProcessorlogic.Lines 614-622 duplicate the cache key format from
EventProcessor.getFunctionsEvents(). If the key format changes, both locations must be updated, creating a maintenance burden.Consider extracting a public method in
EventProcessorto build the cache key, or passingEventProcessorto this listener.♻️ Suggested approach
Add a public method to
EventProcessor:// In EventProcessor.php public function getFunctionEventsCacheKey(Document $project, Database $dbForProject): string { $hostname = $dbForProject->getAdapter()->getHostname(); return \sprintf( '%s-cache-%s:%s:%s:project:%s:functions:events', $dbForProject->getCacheName(), $hostname ?? '', $dbForProject->getNamespace(), $dbForProject->getTenant(), $project->getId() ); }Then inject
EventProcessorand call the method in the listener.
568-598: Add error handling to prevent listener exceptions from breaking database operations.If any of the queue operations (
trigger()) throw an exception, it will propagate to the database operation and potentially cause a failed transaction or data inconsistency. Wrap the listener body in a try-catch block.🛡️ Proposed fix
$eventDatabaseListener = function (Document $project, Document $document, Response $response, Event $queueForEvents, Func $queueForFunctions, Webhook $queueForWebhooks, Realtime $queueForRealtime) { + try { // Only trigger events for user creation with the database listener. if ($document->getCollection() !== 'users') { return; } $queueForEvents ->setEvent('users.[userId].create') ->setParam('userId', $document->getId()) ->setPayload($response->output($document, Response::MODEL_USER)); // Trigger functions, webhooks, and realtime events $queueForFunctions ->from($queueForEvents) ->trigger(); /** Trigger webhooks events only if a project has them enabled */ if (!empty($project->getAttribute('webhooks'))) { $queueForWebhooks ->from($queueForEvents) ->trigger(); } /** Trigger realtime events only for non console events */ if ($queueForEvents->getProject()->getId() !== 'console') { $queueForRealtime ->from($queueForEvents) ->trigger(); } + } catch (\Throwable $e) { + Console::error('Event database listener error: ' . $e->getMessage()); + } };
627-732: Add error handling tousageDatabaseListener.Similar to
eventDatabaseListener, this listener lacks try-catch protection. An exception fromqueueForStatsUsageoperations could propagate and break the database operation.🛡️ Proposed fix
$usageDatabaseListener = function (string $event, Document $document, StatsUsage $queueForStatsUsage) { + try { $value = 1; switch ($event) { // ... existing switch logic ... } switch (true) { // ... existing switch logic ... } + } catch (\Throwable $e) { + Console::error('Usage database listener error: ' . $e->getMessage()); + } };
🧹 Nitpick comments (5)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Transactions/Update.php (1)
377-381: Avoid reloading function/webhook event maps for every operation.
These maps are project-scoped and can be computed once before the operations loop to reduce repeated cache reads.♻️ Proposed refactor
+ // Get project and function/webhook events (cached) + $project = $queueForEvents->getProject(); + $functionsEvents = $eventProcessor->getFunctionsEvents($project, $dbForProject); + $webhooksEvents = $eventProcessor->getWebhooksEvents($project); + foreach ($operations as $operation) { ... - // Get project and function/webhook events (cached) - $project = $queueForEvents->getProject(); - $functionsEvents = $eventProcessor->getFunctionsEvents($project, $dbForProject); - $webhooksEvents = $eventProcessor->getWebhooksEvents($project); ... }src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Action.php (1)
377-419: Skip event generation when no function/webhook events are configured.
This avoids unnecessary Event::generateEvents work when both caches are empty.♻️ Proposed refactor
- // Generate events for this document operation - $generatedEvents = Event::generateEvents( - $queueForEvents->getEvent(), - $queueForEvents->getParams() - ); - - if (!empty($functionsEvents)) { - foreach ($generatedEvents as $event) { - if (isset($functionsEvents[$event])) { - $queueForFunctions - ->from($queueForEvents) - ->trigger(); - break; - } - } - } - - if (!empty($webhooksEvents)) { - foreach ($generatedEvents as $event) { - if (isset($webhooksEvents[$event])) { - $queueForWebhooks - ->from($queueForEvents) - ->trigger(); - break; - } - } - } + if (!empty($functionsEvents) || !empty($webhooksEvents)) { + // Generate events for this document operation + $generatedEvents = Event::generateEvents( + $queueForEvents->getEvent(), + $queueForEvents->getParams() + ); + + if (!empty($functionsEvents)) { + foreach ($generatedEvents as $event) { + if (isset($functionsEvents[$event])) { + $queueForFunctions + ->from($queueForEvents) + ->trigger(); + break; + } + } + } + + if (!empty($webhooksEvents)) { + foreach ($generatedEvents as $event) { + if (isset($webhooksEvents[$event])) { + $queueForWebhooks + ->from($queueForEvents) + ->trigger(); + break; + } + } + } + }app/controllers/shared/api.php (1)
651-699: Avoid Event::generateEvents when no function/webhook events exist.
This shutdown hook runs on every API request; skip event generation if both caches are empty to reduce per-request overhead.♻️ Proposed refactor
- // Generate events for this operation - $generatedEvents = Event::generateEvents( - $queueForEvents->getEvent(), - $queueForEvents->getParams() - ); + $generatedEvents = []; + if (!empty($functionsEvents) || !empty($webhooksEvents)) { + // Generate events for this operation + $generatedEvents = Event::generateEvents( + $queueForEvents->getEvent(), + $queueForEvents->getParams() + ); + }app/init/resources.php (2)
524-524: Significant coupling increase indbForProjectresource.The
dbForProjectresource now depends on 14 resources (up from ~5 previously). While this consolidates event handling, it tightly couples database access with event dispatching, queue management, and response formatting. Consider whether this violates single responsibility principle.This is an architectural trade-off that may be intentional for the event-driven refactoring, but worth noting for future maintainability.
734-739: Variable shadowing: reassigning function parameters reduces clarity.Lines 737-739 reassign
$queueForFunctions,$queueForWebhooks, and$queueForRealtimewhich are function parameters. This shadowing pattern can confuse readers about which instance is being used.♻️ Suggested improvement
Use distinct variable names to make the intent clearer:
// Clone the queues, to prevent events triggered by the database listener // from overwriting the events that are supposed to be triggered in the shutdown hook. $queueForEventsClone = new Event($publisher); - $queueForFunctions = new Func($publisherFunctions); - $queueForWebhooks = new Webhook($publisherWebhooks); - $queueForRealtime = new Realtime(); + $queueForFunctionsClone = new Func($publisherFunctions); + $queueForWebhooksClone = new Webhook($publisherWebhooks); + $queueForRealtimeClone = new Realtime();Then update the listener registration to use the cloned versions explicitly.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/controllers/shared/api.php`:
- Around line 649-650: The action callback argument list has a missing space
after a comma causing a PSR-12 lint error; edit the anonymous function signature
passed to action (the line starting with ->action(function (App $utopia, Request
$request, Response $response, Document $project, User $user, Event
$queueForEvents, Audit $queueForAudits, StatsUsage $queueForStatsUsage, Delete
$queueForDeletes, EventDatabase $queueForDatabase, Build $queueForBuilds,
Messaging $queueForMessaging, Func $queueForFunctions, Event $queueForWebhooks,
Realtime $queueForRealtime, Database $dbForProject,Authorization $authorization,
callable $timelimit, EventProcessor $eventProcessor) use ($parseLabel)) and
insert the missing space after the comma (i.e., change "Database
$dbForProject,Authorization $authorization" to "Database $dbForProject,
Authorization $authorization") so all arguments have a single space after each
comma per PSR-12.
No description provided.