Realtime support for bulk api#10096
Conversation
📝 WalkthroughWalkthroughThis update introduces comprehensive support for bulk document operations and their realtime event notifications across the platform. Bulk create, update, and delete actions for database documents now queue and dispatch events with explicit bulk metadata, enabling individualized document filtering per subscriber based on permissions. The realtime messaging adapter and event dispatch logic are refactored to handle bulk payloads, including mapping documents to subscribers and sending tailored payloads per connection. End-to-end tests are expanded to cover bulk operations, including multi-client scenarios with role-based visibility. Several method signatures and constructors are updated to inject event queue dependencies and event labels, ensuring consistent event tracking for bulk actions. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/controllers/api/databases.php(9 hunks)src/Appwrite/Event/Realtime.php(1 hunks)src/Appwrite/Messaging/Adapter/Realtime.php(4 hunks)tests/e2e/Services/Realtime/RealtimeCustomClientTest.php(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ItzNotABug
PR: appwrite/appwrite#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.
Learnt from: ItzNotABug
PR: appwrite/appwrite#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.
Learnt from: ItzNotABug
PR: appwrite/appwrite#9669
File: src/Appwrite/Utopia/Request/Filters/V19.php:56-71
Timestamp: 2025-05-03T05:58:34.294Z
Learning: According to the developer, wildcard select handling in V19 request filter is only needed for `listDocuments` operations, not for `getDocument` operations, despite the doc-block mentioning both.
src/Appwrite/Messaging/Adapter/Realtime.php (2)
Learnt from: Meldiron
PR: appwrite/appwrite#8859
File: src/Appwrite/Messaging/Adapter/Realtime.php:352-352
Timestamp: 2025-04-16T10:16:04.583Z
Learning: In the Realtime adapter's fromPayload method, all switch case blocks should end with a break statement to prevent fall-through behavior.
Learnt from: ItzNotABug
PR: appwrite/appwrite#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.
app/controllers/api/databases.php (4)
undefined
<retrieved_learning>
Learnt from: ItzNotABug
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.
</retrieved_learning>
<retrieved_learning>
Learnt from: ItzNotABug
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.
</retrieved_learning>
<retrieved_learning>
Learnt from: ItzNotABug
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.
</retrieved_learning>
<retrieved_learning>
Learnt from: ItzNotABug
PR: #9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Delete.php:88-95
Timestamp: 2025-05-09T04:06:04.338Z
Learning: It's acceptable to use DATABASE_TYPE_DELETE_COLLECTION event type for both collections and tables because workers process these events based on whether setCollection() or setTable() was called, not on the event type itself.
</retrieved_learning>
🧬 Code Graph Analysis (3)
src/Appwrite/Event/Realtime.php (2)
src/Appwrite/Event/Event.php (1)
getPayload(222-225)src/Appwrite/Utopia/Response/Model/Document.php (1)
Document(8-97)
src/Appwrite/Messaging/Adapter/Realtime.php (1)
src/Appwrite/Utopia/Response/Model/Document.php (1)
Document(8-97)
tests/e2e/Services/Realtime/RealtimeCustomClientTest.php (1)
tests/e2e/Client.php (1)
Client(8-317)
🪛 PHPMD (2.15.0)
tests/e2e/Services/Realtime/RealtimeCustomClientTest.php
889-889: Avoid unused local variables such as '$documents'. (Unused Code Rules)
(UnusedLocalVariable)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: scan
🔇 Additional comments (11)
src/Appwrite/Event/Realtime.php (1)
76-85: LGTM! Payload handling correctly supports bulk operations.The updated logic properly handles both single document and bulk document payloads by checking for the presence of
documentsandtotalkeys, which aligns with the PR objectives for bulk API realtime support.src/Appwrite/Messaging/Adapter/Realtime.php (3)
252-260: Method signature correctly updated for bulk operations.The PHPDoc and method signature changes properly reflect that the payload is now an array of Document objects, maintaining consistency with the changes in
Realtime.php.
314-324: Database document handling correctly supports bulk operations.The implementation properly:
- Uses the first document to extract collection information
- Only adds document-specific channels for single document operations
- Aggregates read permissions from all documents when document security is enabled
332-333: Verify bulk operations are not expected for files and executions.The code only processes the first document for bucket files and function executions. This appears correct if bulk operations are only supported for database documents, but please confirm that bulk file uploads and bulk execution triggers are not planned features.
Also applies to: 346-347
tests/e2e/Services/Realtime/RealtimeCustomClientTest.php (1)
888-1026: Comprehensive test coverage for bulk operations realtime events.The tests thoroughly validate:
- Bulk create, update, and delete operations trigger appropriate realtime events
- Event payloads contain documents array with total count
- Channel structure excludes document-specific channels for bulk operations (2 channels instead of 3)
- Document permissions are properly preserved in the payload
Excellent test coverage!
app/controllers/api/databases.php (6)
3560-3561: LGTM: Realtime event name correctly set for bulk document creation.The event name follows the documented pattern from the PR objectives and properly enables realtime notifications for bulk document creation operations.
4715-4721: LGTM: Bulk update event queue setup correctly implemented.The event queue configuration for bulk document updates follows the correct pattern and sets the appropriate event name for realtime notifications.
4815-4820: LGTM: Bulk upsert event queue setup correctly implemented.The event queue configuration for bulk document upserts follows the correct pattern. The event name
documents.upsertis a logical extension of the documented events.
5040-5045: LGTM: Bulk delete event queue setup correctly implemented.The event queue configuration for bulk document deletes follows the correct pattern and matches the documented event name from the PR objectives.
4653-4653: Authorization.skip usage is consistent and safeI’ve verified that every
Authorization::skip(fn () => $dbForProject->getDocument(...))call—both for databases and collections—matches existing patterns in this controller. Each skipped retrieval is immediately followed by manual checks for existence (isEmpty()),enabledstatus, API-key usage, and privileged roles before proceeding or throwing an exception. This mirrors the rest of the file’s approach and does not introduce new security gaps. No changes required.
4261-4261: Upsert Document Endpoint:queueForEventsRemoval VerifiedI’ve checked the
upsertDocumentaction in app/controllers/api/databases.php (lines 4240–4300) and confirmed there are no references toqueueForEventsin the closure body. The injection and signature change won’t break existing functionality—no further updates are required.
|
@ArnabChatterjee20k shouldn't it also trigger the individual document events?
|
ArnabChatterjee20k
force-pushed
the
dat-556
branch
from
b09330c to
d9f1c08
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/e2e/Services/Realtime/RealtimeCustomClientTest.php (1)
900-900: Remove unused variable.The
$documentsvariable is assigned but never used in the test.Apply this diff to remove the unused variable:
- $documents = $this->client->call(Client::METHOD_POST, "/databases/{$databaseId}/collections/{$actorsId}/documents", array_merge([ + $this->client->call(Client::METHOD_POST, "/databases/{$databaseId}/collections/{$actorsId}/documents", array_merge([
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/Appwrite/Event/Realtime.php(0 hunks)src/Appwrite/Messaging/Adapter/Realtime.php(2 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Delete.php(4 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Update.php(5 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.php(1 hunks)tests/e2e/Services/Realtime/RealtimeCustomClientTest.php(1 hunks)
💤 Files with no reviewable changes (1)
- src/Appwrite/Event/Realtime.php
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Appwrite/Messaging/Adapter/Realtime.php
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: ItzNotABug
PR: appwrite/appwrite#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.
Learnt from: ItzNotABug
PR: appwrite/appwrite#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.
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.php (2)
Learnt from: ItzNotABug
PR: appwrite/appwrite#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.
Learnt from: ItzNotABug
PR: appwrite/appwrite#9669
File: src/Appwrite/Utopia/Request/Filters/V19.php:56-71
Timestamp: 2025-05-03T05:58:34.294Z
Learning: According to the developer, wildcard select handling in V19 request filter is only needed for `listDocuments` operations, not for `getDocument` operations, despite the doc-block mentioning both.
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Delete.php (4)
Learnt from: ItzNotABug
PR: appwrite/appwrite#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.
Learnt from: ItzNotABug
PR: appwrite/appwrite#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.
Learnt from: ItzNotABug
PR: appwrite/appwrite#9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Delete.php:88-95
Timestamp: 2025-05-09T04:06:04.338Z
Learning: It's acceptable to use DATABASE_TYPE_DELETE_COLLECTION event type for both collections and tables because workers process these events based on whether setCollection() or setTable() was called, not on the event type itself.
Learnt from: ItzNotABug
PR: appwrite/appwrite#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.
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Update.php (4)
Learnt from: ItzNotABug
PR: appwrite/appwrite#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.
Learnt from: ItzNotABug
PR: appwrite/appwrite#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.
Learnt from: ItzNotABug
PR: appwrite/appwrite#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.
Learnt from: ItzNotABug
PR: appwrite/appwrite#9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Delete.php:88-95
Timestamp: 2025-05-09T04:06:04.338Z
Learning: It's acceptable to use DATABASE_TYPE_DELETE_COLLECTION event type for both collections and tables because workers process these events based on whether setCollection() or setTable() was called, not on the event type itself.
tests/e2e/Services/Realtime/RealtimeCustomClientTest.php (1)
Learnt from: ItzNotABug
PR: appwrite/appwrite#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.
🧬 Code Graph Analysis (3)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.php (1)
src/Appwrite/Event/Event.php (1)
setEvent(117-122)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Delete.php (4)
src/Appwrite/Event/Event.php (3)
Event(10-639)setEvent(117-122)setContext(234-239)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Update.php (1)
action(81-164)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.php (1)
action(119-408)src/Appwrite/Extend/Exception.php (1)
Exception(7-442)
tests/e2e/Services/Realtime/RealtimeCustomClientTest.php (1)
tests/e2e/Client.php (1)
Client(8-317)
🪛 PHPMD (2.15.0)
tests/e2e/Services/Realtime/RealtimeCustomClientTest.php
900-900: Avoid unused local variables such as '$documents'. (Unused Code Rules)
(UnusedLocalVariable)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (17)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.php (1)
389-390: LGTM! Event queue setup is properly implemented.The event queue setup for bulk document creation follows the consistent pattern established across all bulk operations and correctly sets the event name and parameters.
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Delete.php (7)
5-5: LGTM! Event import added correctly.The Event class import is properly added to support the new event queue functionality.
20-20: LGTM! Authorization import added correctly.The Authorization import is properly added to support the skip functionality for database and collection retrieval.
72-72: LGTM! Event queue injection added correctly.The queueForEvents injection is properly added to the constructor to support event handling.
77-77: LGTM! Method signature updated correctly.The action method signature is properly updated to include the Event parameter for event queue handling.
79-79: LGTM! Authorization skip properly implemented.The Authorization::skip usage is appropriate for fetching the database document during event setup, ensuring consistent access regardless of current authorization context.
84-84: LGTM! Authorization skip properly implemented.The Authorization::skip usage is appropriate for fetching the collection document during event setup, ensuring consistent access regardless of current authorization context.
131-136: LGTM! Event queue setup is properly implemented.The event queue setup for bulk document deletion follows the consistent pattern established across all bulk operations and correctly sets:
- Event name:
databases.[databaseId].collections.[collectionId].documents.delete- Parameters:
databaseIdandcollectionId- Context:
collectionanddatabasedocumentsThis implementation is consistent with the bulk update and create operations.
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Bulk/Update.php (7)
5-5: LGTM! Event import added correctly.The Event class import is properly added to support the new event queue functionality.
21-21: LGTM! Authorization import added correctly.The Authorization import is properly added to support the skip functionality for database and collection retrieval.
76-76: LGTM! Event queue injection added correctly.The queueForEvents injection is properly added to the constructor to support event handling.
81-81: LGTM! Method signature updated correctly.The action method signature is properly updated to include the Event parameter for event queue handling.
91-91: LGTM! Authorization skip properly implemented.The Authorization::skip usage is appropriate for fetching the database document during event setup, ensuring consistent access regardless of current authorization context.
96-96: LGTM! Authorization skip properly implemented.The Authorization::skip usage is appropriate for fetching the collection document during event setup, ensuring consistent access regardless of current authorization context.
153-158: LGTM! Event queue setup is properly implemented.The event queue setup for bulk document update follows the consistent pattern established across all bulk operations and correctly sets:
- Event name:
databases.[databaseId].collections.[collectionId].documents.update- Parameters:
databaseIdandcollectionId- Context:
collectionanddatabasedocumentsThis implementation is consistent with the bulk delete and create operations.
tests/e2e/Services/Realtime/RealtimeCustomClientTest.php (2)
966-999: Well-implemented bulk update test.The test properly validates realtime event notifications for bulk document updates, including correct channel counts, event patterns, and payload verification. The assertions confirm all documents are updated with the expected data.
1000-1036: Comprehensive bulk delete test coverage.The test thoroughly validates realtime event notifications for bulk document deletions, including proper event patterns, channel verification, and detailed payload validation that confirms deleted documents retain their original structure and permissions.
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.php
Outdated
Show resolved
Hide resolved
ArnabChatterjee20k
force-pushed
the
dat-556
branch
from
c7888d3 to
a230152
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
tests/e2e/Services/Realtime/RealtimeCustomClientTest.php (1)
900-900: Remove unused variable.The
$documentsvariable is assigned but never used in the test.Apply this diff to remove the unused variable:
- $documents = $this->client->call(Client::METHOD_POST, "/databases/{$databaseId}/collections/{$actorsId}/documents", array_merge([ + $this->client->call(Client::METHOD_POST, "/databases/{$databaseId}/collections/{$actorsId}/documents", array_merge([
🧹 Nitpick comments (5)
app/realtime.php (2)
470-476: Remove commented debug code or use a debug flag.Instead of leaving commented code, either remove it entirely or control it with a proper debug flag.
if (App::isDevelopment() && !empty($receivers)) { - // var_dump($event); - // var_dump($receivers); - // Console::log("[Debug][Worker {$workerId}] Receivers: " . count($receivers)); - // Console::log("[Debug][Worker {$workerId}] Receivers Connection IDs: " . json_encode($receivers)); - // Console::log("[Debug][Worker {$workerId}] Event: " . $payload); + if (System::getEnv('_APP_REALTIME_DEBUG', false)) { + Console::log("[Debug][Worker {$workerId}] Event: " . $payload); + Console::log("[Debug][Worker {$workerId}] Receivers: " . count($receivers)); + } }
482-495: Optimize duplicate filtering logic.The current implementation uses nested loops with O(n) lookups. Consider using array functions for better performance and readability.
foreach ($connectionDocsMap as $connectionId => $docs) { - // TODO: improve the filteration step - $uniqueDocs = []; - $seenDocIds = []; - - foreach ($docs as $doc) { - $id = $doc['$id']; - - if (isset($seenDocIds[$id])) { - continue; - } - - $seenDocIds[$id] = true; - $uniqueDocs[] = $doc; - } + // Filter unique documents by ID + $uniqueDocs = array_values(array_reduce($docs, function ($carry, $doc) { + $carry[$doc['$id']] = $doc; + return $carry; + }, []));src/Appwrite/Messaging/Adapter/Realtime.php (3)
227-266: Consider performance implications for large bulk operations.The current implementation has O(N*M) complexity where N is the number of documents and M is the number of unique roles. For large bulk operations, this could become a bottleneck.
Consider adding:
- Early termination when all documents are assigned
- Caching of permission checks for identical permission sets
- Monitoring/logging for operations exceeding thresholds
350-363: Extract bulk detection logic to reduce duplication.The bulk detection logic is duplicated between
send()andfromPayload()methods.+ /** + * Check if payload represents a bulk operation + */ + private static function isBulkPayload(Document $payload): bool + { + return $payload->getAttribute('total') && ( + $payload->getAttribute('documents') !== null || + $payload->getAttribute('columns') !== null + ); + } $resource = $parts[4] ?? ''; - $isBulk = $payload->getAttribute('total') && ( - $payload->getAttribute('documents') !== null || $payload->getAttribute('columns') !== null - ); + $isBulk = self::isBulkPayload($payload);
389-401: Optimize permission merging for bulk operations.When document security is enabled, merging permissions for each document could be inefficient for large bulk operations.
Consider batching permission calculations or using a more efficient data structure:
if ($isBulk) { if ($collection->getAttribute('documentSecurity', false)) { + $collectionPerms = $collection->getPermissions(); foreach ($payload as $document) { - $roles[$document->getId()] = \array_merge($collection->getPermissions(), $document->getPermissions()); + // Only merge if document has additional permissions + $docPerms = $document->getPermissions(); + $roles[$document->getId()] = empty($docPerms) + ? $collectionPerms + : \array_merge($collectionPerms, $docPerms); } } else { $roles = $collection->getRead(); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/realtime.php(1 hunks)src/Appwrite/Messaging/Adapter/Realtime.php(6 hunks)tests/e2e/Scopes/Scope.php(1 hunks)tests/e2e/Services/Databases/Legacy/DatabasesBase.php(0 hunks)tests/e2e/Services/Realtime/RealtimeCustomClientTest.php(1 hunks)
💤 Files with no reviewable changes (1)
- tests/e2e/Services/Databases/Legacy/DatabasesBase.php
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ItzNotABug
PR: appwrite/appwrite#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.
Learnt from: ItzNotABug
PR: appwrite/appwrite#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.
src/Appwrite/Messaging/Adapter/Realtime.php (1)
Learnt from: ItzNotABug
PR: appwrite/appwrite#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.
tests/e2e/Services/Realtime/RealtimeCustomClientTest.php (1)
Learnt from: ItzNotABug
PR: appwrite/appwrite#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.
🪛 PHPMD (2.15.0)
tests/e2e/Services/Realtime/RealtimeCustomClientTest.php
900-900: Avoid unused local variables such as '$documents'. (Unused Code Rules)
(UnusedLocalVariable)
⏰ 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 (5)
tests/e2e/Scopes/Scope.php (1)
140-144: LGTM!The addition of the
$overrideparameter provides a clean way to force creation of fresh user sessions for testing scenarios. The default value offalsemaintains backward compatibility.tests/e2e/Services/Realtime/RealtimeCustomClientTest.php (4)
926-964: Excellent bulk create event testing.The bulk create test thoroughly validates the realtime event structure, including comprehensive event type checks, channel verification, and payload structure validation. The assertions correctly verify that the bulk payload contains the expected document count and structure.
966-999: Comprehensive bulk update event validation.The bulk update test properly validates the realtime event structure and ensures all documents are updated correctly. The event type assertions and payload validation are thorough and appropriate for bulk operations.
1001-1037: Thorough bulk delete event testing.The bulk delete test comprehensively validates the realtime event structure and verifies that all documents are properly included in the delete payload with correct permissions. The event assertions are complete and appropriate.
1042-1286: Excellent multi-client bulk operation testing.This new test method provides comprehensive coverage for bulk operations with multiple websocket clients and role-based permissions. The test effectively validates:
- Different permission levels (any, users, user-specific)
- Correct document filtering per client based on permissions
- Proper event delivery to each client
- Consistent payload structure across all bulk operations
The test structure is well-organized and the assertions are thorough, ensuring that the realtime system correctly handles permission-based document filtering for bulk operations.
ArnabChatterjee20k
force-pushed
the
dat-556
branch
from
9113aee to
99834a9
Compare
…coverage for bulk operations - Added event queue injections in Bulk Update and Create classes for better event handling. - Modified getUser method in Scope class to allow for user retrieval with an override option. - Expanded RealtimeCustomClientTest to include comprehensive tests for bulk create, update, and delete operations, ensuring proper event emissions for multiple clients. - Improved assertions in tests to validate event data and permissions for created, updated, and deleted documents.
ArnabChatterjee20k
force-pushed
the
dat-556
branch
from
59d0452 to
59026a1
Compare
…mize bulk document queue triggering
…meout assertions for clients
ArnabChatterjee20k
force-pushed
the
dat-556
branch
from
69bc86a to
a144d41
Compare
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/Databases/Http/Databases/Collections/Documents/Bulk/Update.php
Show resolved
Hide resolved
yes |
4 participants
What does this PR do?
-> Event working for bulk api
They'll fire on existing channels only
Clients will receive an individual event for each document
(Provide a description of what this PR does and why it's needed.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Screenshots may also be helpful.)
Related PRs and Issues
Checklist