[fix] Optimized SQL queries for notification storm prevention #383#399
[fix] Optimized SQL queries for notification storm prevention #383#399Eeshu-Yadav wants to merge 2 commits intoopenwisp:masterfrom
Conversation
Eeshu-Yadav
force-pushed
the
issues/383-optimize-notification-sql-queries
branch
2 times, most recently
from
480df32 to
f50f875
Compare
Eeshu-Yadav
force-pushed
the
issues/383-optimize-notification-sql-queries
branch
from
6892077 to
42015be
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughNotification creation now materializes recipients into a list, builds Notification instances into Sequence Diagram(s)sequenceDiagram
participant Creator as Notification Handler
participant DB as Database
participant Cache as Cache Layer
participant Email as Email Sender
participant WS as WebSocket Handler
rect rgba(150, 200, 100, 0.5)
Note over Creator,DB: Bulk creation flow
Creator->>Creator: materialize recipients, build notifications_to_create
Creator->>DB: bulk_create(notifications_to_create)
DB-->>Creator: created notification instances
Creator->>Email: send_email_notification(...) for each created
Creator->>Cache: invalidate unread caches per recipient
Creator->>WS: bulk_notification_update_handler(recipients, reload_widget, notification_map)
end
sequenceDiagram
participant WS as WebSocket Handler
participant Cache as Cache Layer
participant DB as Database
participant Client as Recipient WS Client
rect rgba(100, 150, 200, 0.5)
Note over WS,DB: Bulk websocket update flow
WS->>Cache: check cached storm/unread for recipients
alt uncached recipients exist
WS->>DB: aggregate unread counts & recent notifications
DB-->>WS: unread_count per recipient, storm determination
WS->>Cache: store storm flags / counts
end
WS->>Client: send update per recipient (optional notification payload, in_storm, notification_count)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can customize the high-level summary generated by CodeRabbit.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@openwisp_notifications/handlers.py`:
- Around line 181-186: The handler currently picks first_notification from
notification_list and passes it into
ws_handlers.bulk_notification_update_handler which causes
NotificationListSerializer to broadcast recipient-specific fields (like unread)
identically to all recipients; instead, change bulk_notification_update_handler
usage to avoid serializing a single notification for all recipients—either pass
the full notification_list and have bulk_notification_update_handler iterate and
serialize per-recipient items (respecting each Notification.recipient and
unread), or remove recipient-sensitive fields from the payload used for
broadcast and send per-recipient updates (e.g., call a per-recipient send path
or build a list of serialized notifications keyed by recipient). Update
references: notification_list, first_notification, recipients_list,
bulk_notification_update_handler, and NotificationListSerializer accordingly so
unread is computed/serialized per recipient rather than reused from the first
item.
In `@openwisp_notifications/websockets/handlers.py`:
- Around line 20-21: recipient_ids is built as strings but later used to filter
by recipient_id and to key cache lookups, causing type mismatches; change the
code to preserve native PK types (use recipient.pk without str() when building
recipient_ids and cache key list) and normalize keys when reading DB results
(e.g., cast DB result recipient_id to str only when accessing string-cache keys)
so filters, cache.get_many keys (cached_storm_data), and result dictionary
lookups all use the same type; update references to recipient_ids,
cached_storm_data, and any places reading recipient_id from query results to use
the consistent type.
- Around line 42-54: The QuerySet stored in storm_and_unread_data is consumed
multiple times (iterated at the first loop and later used in the list
comprehension checking recipient.pk), causing empty results on the second
access; materialize the QuerySet immediately (e.g., assign
list(storm_and_unread_data)) so subsequent uses of storm_and_unread_data refer
to an in-memory list, and update uses in the surrounding logic that references
recipient_ids / recipient.pk to operate on that materialized collection.
🧹 Nitpick comments (5)
openwisp_notifications/handlers.py (3)
170-193: Potential IndexError whenrecipients_listis empty.If
recipients_listis empty,notification_listwill be empty afterbulk_create, and Line 181 will accessnotification_list[0]which raises anIndexError. The conditionif notification_list else Nonehandles this, but the code still proceeds to iterate over an empty list, which is fine. However, Line 195 returnsnotification_listwhich may be referenced before assignment if an exception occurs before Line 173.More critically, the signal is disconnected even when there are no notifications to create, which is unnecessary overhead.
♻️ Suggested improvement to handle empty recipients early
notifications_to_create = [] recipients_list = list(recipients) + if not recipients_list: + return [] + for recipient in recipients_list:
175-176: Email notifications bypass batching optimization for bulk creation.The loop at Lines 175-176 calls
send_email_notification()individually for each notification after bulk creation. Sincesend_email_notificationis also apost_savesignal handler (Line 198), calling it directly here is intentional to replace the signal behavior. However, this still results in N iterations and potential N database queries for email verification checks (Line 204-206 insend_email_notification).Consider whether email address verification data could also be prefetched in bulk to further reduce queries, aligning with the PR's objective to minimize per-recipient queries.
189-193: Signal reconnection uses explicitdispatch_uidbut disconnection does not.The
post_save.disconnect()at Line 170 does not specifydispatch_uid, while the reconnection at Lines 189-193 specifiesdispatch_uid="clear_notification_cache_saved". Django's signal disconnect withoutdispatch_uiddisconnects based on receiver function identity, which should work, but for consistency and robustness, consider specifying thedispatch_uidin the disconnect call as well.♻️ Suggested fix for consistency
- post_save.disconnect(clear_notification_cache, sender=Notification) + post_save.disconnect( + clear_notification_cache, + sender=Notification, + dispatch_uid="clear_notification_cache_saved", + )openwisp_notifications/websockets/handlers.py (2)
79-83: Inefficient O(n×m) lookup for recipients not in query results.The check at Lines 80-82 creates a new list comprehension on every iteration of the loop, resulting in O(n×m) complexity where n is the number of uncached recipients and m is the number of query results. This could be optimized using a set.
♻️ Suggested optimization
+ returned_recipient_ids = {data["recipient_id"] for data in storm_and_unread_data} + for recipient in uncached_recipients: - if recipient.pk not in [ - data["recipient_id"] for data in storm_and_unread_data - ]: + if recipient.pk not in returned_recipient_ids: results[recipient.pk] = [False, 0]Note: You'll need to convert
storm_and_unread_datato a list first since it's a QuerySet that gets consumed during iteration at Line 57.
85-100: Secondary query for unread counts duplicates work already done.The unread count is already computed in the first aggregation query at Line 52 (
unread_count=Count("id", filter=Q(unread=True))). The secondary query at Lines 87-93 is only needed for recipients whose storm status was cached (and thus skipped the first query). However, the condition at Line 85 checks forNoneunread counts, which includes both cached recipients and recipients not returned by the first query (due to having no notifications).This logic is correct but could be clearer. Consider adding a comment explaining that this secondary query is specifically for recipients with cached storm status who need fresh unread counts.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
openwisp_notifications/handlers.pyopenwisp_notifications/websockets/handlers.py
🧰 Additional context used
🧬 Code graph analysis (2)
openwisp_notifications/handlers.py (3)
openwisp_notifications/models.py (1)
Notification(11-15)openwisp_notifications/base/models.py (1)
invalidate_unread_cache(135-139)openwisp_notifications/websockets/handlers.py (1)
bulk_notification_update_handler(145-175)
openwisp_notifications/websockets/handlers.py (4)
openwisp_notifications/tests/test_admin.py (1)
pk(35-36)tests/openwisp2/sample_notifications/models.py (1)
Notification(32-35)openwisp_notifications/base/notifications.py (1)
unread(22-24)openwisp_notifications/utils.py (1)
normalize_unread_count(41-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: CodeQL analysis (python)
- GitHub Check: Agent
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=4.2.0
🔇 Additional comments (3)
openwisp_notifications/websockets/handlers.py (2)
145-175: LGTM - Bulk notification update handler is well-structured.The function correctly:
- Handles empty recipients early (Lines 148-149)
- Serializes notification once and reuses it (Lines 153-158)
- Fetches bulk data in a single call (Line 160)
- Gracefully handles missing recipients in bulk_data with default (Line 163)
178-181: LGTM - Clean delegation to bulk handler.The refactored
notification_update_handlerproperly delegates to the bulk handler with a single-element list, maintaining backward compatibility while reusing the optimized code path.openwisp_notifications/handlers.py (1)
166-167: The code design is intentional and safe—remove the EXTRA_DATA flag reference.The
notify_handlerfunction explicitly extracts all known parameters using.pop()before assigning any remainingkwargstonotification.data. The parameters extracted are:signal,sender,public,description,timestamp,recipient,type,level,verb,target, andaction_object. Any leftoverkwargsthat remain after these explicit extractions are intentionally stored innotification.data.All actual callers in the codebase (tests and signal handlers) pass only the expected parameters. There is no evidence of the "EXTRA_DATA flag" mentioned in the original comment—no such flag exists in the codebase. The concern about unintended data storage is not supported by the code structure or usage patterns; the design safely isolates known parameters before storing extras.
Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
@Eeshu-Yadav too many blank lines as usual.
There was a problem hiding this comment.
Pull request overview
This PR optimizes notification creation and websocket updates to reduce the number of SQL queries and improve performance, especially when sending notifications to multiple recipients and enforcing storm-prevention logic.
Changes:
- Reworked
notify_handlerto bulk-create notifications, manually trigger email sending, and invalidate unread-count caches while batching websocket updates. - Introduced
bulk_check_notification_storm_and_unread_countto compute storm-prevention flags and unread counts for multiple recipients in a small number of aggregated queries. - Added
bulk_notification_update_handlerand updated the websocket notification flow to use bulk storm/unread data and shared serialization work.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
openwisp_notifications/handlers.py |
Switches notification creation to bulk_create, centralizes email triggering and unread-cache invalidation, and introduces a bulk websocket update call for all affected recipients. |
openwisp_notifications/websockets/handlers.py |
Adds a bulk storm+unread computation helper and a bulk websocket update handler so that storm detection and unread counts are fetched with aggregated queries instead of per-recipient calls. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def bulk_notification_update_handler( | ||
| recipients, reload_widget=False, notification=None | ||
| ): | ||
| if not recipients: | ||
| return | ||
|
|
||
| channel_layer = layers.get_channel_layer() | ||
|
|
||
| serialized_notification = None | ||
| try: | ||
| assert notification is not None | ||
| notification = NotificationListSerializer(notification).data | ||
| if notification is not None: | ||
| serialized_notification = NotificationListSerializer(notification).data | ||
| except (NotFound, AssertionError): | ||
| pass | ||
| async_to_sync(channel_layer.group_send)( | ||
| f"ow-notification-{recipient.pk}", | ||
| { | ||
| "type": "send.updates", | ||
| "reload_widget": reload_widget, | ||
| "notification": notification, | ||
| "recipient": str(recipient.pk), | ||
| "in_notification_storm": user_in_notification_storm(recipient), | ||
| "notification_count": normalize_unread_count( | ||
| recipient.notifications.unread().count() | ||
| ), | ||
| }, | ||
| ) | ||
|
|
||
| bulk_data = bulk_check_notification_storm_and_unread_count(recipients) | ||
|
|
||
| for recipient in recipients: | ||
| in_storm, unread_count = bulk_data.get(recipient.pk, (False, 0)) | ||
|
|
||
| async_to_sync(channel_layer.group_send)( | ||
| f"ow-notification-{recipient.pk}", | ||
| { | ||
| "type": "send.updates", | ||
| "reload_widget": reload_widget, | ||
| "notification": serialized_notification, | ||
| "recipient": str(recipient.pk), | ||
| "in_notification_storm": in_storm, | ||
| "notification_count": normalize_unread_count(unread_count), | ||
| }, | ||
| ) |
There was a problem hiding this comment.
bulk_notification_update_handler serializes a single notification instance and reuses the same payload for all recipients, which means that for multi-recipient notifications the id field in the serialized data will not match the Notification row actually created for most recipients. If any client logic relies on this id to reference or operate on the recipient's own notification (for example via the REST API), it will be incorrect; consider passing a per-recipient notification instance or otherwise ensuring that the payload's identifier corresponds to the notification for that recipient.
|
Eeshu-Yadav
force-pushed
the
issues/383-optimize-notification-sql-queries
branch
2 times, most recently
from
2c59edc to
e1298b6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openwisp_notifications/websockets/handlers.py`:
- Around line 149-152: The notification parameter in notification_update_handler
is ignored, dropping notification data from callers like
clear_notification_cache; update notification_update_handler to forward the
notification to bulk_notification_update_handler (call
bulk_notification_update_handler([recipient], reload_widget,
notification=notification) or equivalent signature change) or, if deprecating
the parameter, remove notification from the function signature and update
clear_notification_cache to stop passing notification; ensure you adjust the
bulk_notification_update_handler signature/usage accordingly and keep
clear_notification_cache, notification_update_handler, and
bulk_notification_update_handler consistent.
🧹 Nitpick comments (2)
openwisp_notifications/handlers.py (2)
170-171: Consider batching email-related queries for further optimization.While
bulk_createreduces notification inserts to a single query, the email notification loop still performs per-recipient database queries (email verification check at line 187-189, user preferences). For large recipient lists, this could be optimized by:
- Pre-fetching verified emails for all recipients in one query
- Batch-loading user email preferences
This is a recommended follow-up optimization rather than a blocker.
172-173: Consider usingcache.delete_many()for batch cache invalidation.The loop calls
invalidate_unread_cacheper recipient, which issues individual cache delete operations. For better efficiency with multiple recipients:♻️ Proposed optimization
- for recipient in recipients_list: - Notification.invalidate_unread_cache(recipient) + cache_keys = [Notification.count_cache_key(r.pk) for r in recipients_list] + cache.delete_many(cache_keys)This requires importing
cacheand potentially exposingcount_cache_keyas a static method, but reduces cache operations from N to 1.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
openwisp_notifications/handlers.pyopenwisp_notifications/websockets/handlers.py
🧰 Additional context used
🧬 Code graph analysis (2)
openwisp_notifications/handlers.py (2)
openwisp_notifications/base/models.py (1)
invalidate_unread_cache(135-139)openwisp_notifications/websockets/handlers.py (1)
bulk_notification_update_handler(129-146)
openwisp_notifications/websockets/handlers.py (7)
openwisp_notifications/tests/test_admin.py (1)
pk(35-36)openwisp_notifications/models.py (1)
Notification(11-15)tests/openwisp2/sample_notifications/models.py (1)
Notification(32-35)openwisp_notifications/base/notifications.py (1)
unread(22-24)openwisp_notifications/api/serializers.py (1)
data(51-56)openwisp_notifications/api/views.py (4)
get(72-73)get(94-97)get(141-142)get(164-165)openwisp_notifications/utils.py (1)
normalize_unread_count(41-45)
🪛 Ruff (0.14.14)
openwisp_notifications/websockets/handlers.py
[warning] 149-149: Unused function argument: notification
(ARG001)
⏰ 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). (14)
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=4.2.0
🔇 Additional comments (8)
openwisp_notifications/websockets/handlers.py (4)
15-28: LGTM on cache lookup and initialization logic.The function correctly handles empty recipients, uses native PK types (addressing the past type inconsistency concern), and properly initializes results with cached storm data while tracking uncached recipients.
37-71: LGTM on aggregated query and storm detection logic.The QuerySet is properly materialized with
list()(addressing the past review concern), and the aggregation efficiently computes short/long-term counts and unread counts in a single query. The storm detection logic and cache batching withset_many()are well-implemented.
72-86: LGTM on unread count fallback handling.The logic correctly handles recipients with cached storm data by fetching their unread counts in a separate query. The return format as
(storm, unread)tuples is consistent with the unpacking at the call site.
129-146: LGTM on bulk notification update handler.The implementation correctly uses the bulk data lookup and properly handles the WebSocket broadcast per recipient. Setting
notification: Noneis appropriate here since individual notification data is not relevant for bulk count/storm updates.openwisp_notifications/handlers.py (4)
142-145: LGTM on initialization and recipient materialization.Materializing recipients into a list ensures consistent iteration and avoids QuerySet re-evaluation issues.
146-168: LGTM on notification object construction.The notification instances are correctly built with all required fields and optional objects. The simplified condition at line 166 for setting
notification.datawhenkwargsis truthy is cleaner.
174-178: LGTM on bulk WebSocket update integration.The call to
bulk_notification_update_handlerwithreload_widget=Truecorrectly triggers efficient WebSocket updates for all recipients after bulk creation.
169-171: This code is safe for the project's supported versions. Django 4.2+ (the minimum version tested in CI) guarantees thatbulk_create()returns objects with primary keys set when using SQLite 3.35+. Thebulk_update()call insidesend_email()(line 369) will work correctly with the notification objects returned frombulk_create()at line 169. This concern only applies to unsupported Django versions < 4.0 or SQLite versions < 3.35.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Eeshu-Yadav
force-pushed
the
issues/383-optimize-notification-sql-queries
branch
from
e1298b6 to
6f9e663
Compare
|
@nemesifier trimmed the extra spaces ,let me know if any further changes required |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openwisp_notifications/handlers.py`:
- Around line 169-174: bulk_create may return objects without PKs on some
DB/Django combos; update the post-create logic in the block using Notification,
notifications_to_create, notification_list and recipients_list to ensure IDs
exist before email batching and WebSocket updates: after
Notification.objects.bulk_create(notifications_to_create) check whether returned
notification_list items have .id populated and if not re-query the DB for the
created rows (e.g. by filtering on unique fields you set on each Notification or
using created timestamps) to build a list of persisted Notification IDs, then
use that re-fetched list for send_email_notification,
Notification.invalidate_unread_cache, notification_map and any code that appends
instance.id to batched_notifications so you never rely on None IDs;
alternatively add a test/assertion to fail fast if PKs are missing when you
require DB guarantees.
🧹 Nitpick comments (1)
openwisp_notifications/handlers.py (1)
174-174: Make zip strict to satisfy Ruff B905Line 174 uses
zip(...)withoutstrict=. If the project targets Python ≥3.10, passstrict=Trueto fail fast on mismatches (or add an explicit length guard).🔧 Suggested change
- notification_map = dict(zip(recipients_list, notification_list)) + notification_map = dict(zip(recipients_list, notification_list, strict=True))
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
openwisp_notifications/handlers.pyopenwisp_notifications/websockets/handlers.py
🧰 Additional context used
🧬 Code graph analysis (2)
openwisp_notifications/websockets/handlers.py (4)
openwisp_notifications/models.py (1)
Notification(11-15)openwisp_notifications/base/notifications.py (1)
unread(21-23)openwisp_notifications/api/serializers.py (2)
data(51-56)NotificationListSerializer(59-74)openwisp_notifications/utils.py (1)
normalize_unread_count(41-45)
openwisp_notifications/handlers.py (4)
openwisp_notifications/models.py (1)
Notification(11-15)tests/openwisp2/sample_notifications/models.py (1)
Notification(32-35)openwisp_notifications/base/models.py (1)
invalidate_unread_cache(135-139)openwisp_notifications/websockets/handlers.py (1)
bulk_notification_update_handler(129-160)
🪛 Ruff (0.14.14)
openwisp_notifications/websockets/handlers.py
[error] 148-149: try-except-pass detected, consider logging the exception
(S110)
[warning] 148-148: Do not catch blind exception: Exception
(BLE001)
openwisp_notifications/handlers.py
[warning] 174-174: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
⏰ 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). (8)
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
🔇 Additional comments (2)
openwisp_notifications/websockets/handlers.py (2)
4-87: LGTM: bulk storm/unread aggregation is consistentThe cached/uncached paths and unread-count defaults look coherent.
144-149: Catch only the expectedNotFoundexception instead of swallowing all exceptionsLines 144-149 use
except Exception: pass, which hides failures and triggers Ruff S110/BLE001. TheNotificationListSerializer.dataproperty explicitly raisesNotFound(converted fromNotificationRenderException), so catch only that exception and log it; let unexpected errors surface.🔧 Suggested change
+import logging +from rest_framework.exceptions import NotFound @@ +logger = logging.getLogger(__name__) @@ - try: - serialized_notification = NotificationListSerializer( - notification_map[recipient] - ).data - except Exception: - pass + try: + serialized_notification = NotificationListSerializer( + notification_map[recipient] + ).data + except NotFound: + logger.warning( + "Failed to serialize notification for recipient %s", + recipient.pk, + )
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Eeshu-Yadav
force-pushed
the
issues/383-optimize-notification-sql-queries
branch
from
6f9e663 to
08e710b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openwisp_notifications/websockets/handlers.py`:
- Around line 139-149: The current except Exception: pass silently swallows
serialization errors for NotificationListSerializer when handling
notification_map[recipient]; change this to catch and log the failure (e.g.,
except Exception as e: logger.exception(...) or logger.error(...,
exc_info=True)) while preserving the existing control flow, and consider
narrowing the exception type if appropriate; reference
NotificationListSerializer, notification_map[recipient], recipient and
serialized_notification to locate where to add the logging and include
contextual details (recipient id, notification id or repr) in the log.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
openwisp_notifications/handlers.pyopenwisp_notifications/websockets/handlers.py
🧰 Additional context used
🧬 Code graph analysis (2)
openwisp_notifications/handlers.py (3)
openwisp_notifications/models.py (1)
Notification(11-15)openwisp_notifications/base/models.py (1)
invalidate_unread_cache(135-139)openwisp_notifications/websockets/handlers.py (1)
bulk_notification_update_handler(129-160)
openwisp_notifications/websockets/handlers.py (3)
openwisp_notifications/base/notifications.py (1)
unread(22-24)openwisp_notifications/api/serializers.py (1)
NotificationListSerializer(59-74)openwisp_notifications/utils.py (1)
normalize_unread_count(41-45)
🪛 Ruff (0.14.14)
openwisp_notifications/handlers.py
[warning] 174-174: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
openwisp_notifications/websockets/handlers.py
[error] 148-149: try-except-pass detected, consider logging the exception
(S110)
[warning] 148-148: Do not catch blind exception: Exception
(BLE001)
⏰ 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). (14)
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
🔇 Additional comments (2)
openwisp_notifications/websockets/handlers.py (2)
15-86: Bulk storm/unread aggregation looks solid.
Batching and cache usage are coherent and reduce N+1 queries while preserving behavior.
163-167: LGTM wrapper delegation.
The single-recipient wrapper correctly delegates to the bulk path.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
The PR says it reduced the number of queries, but there's no assertion in tests which backs this claim. Moreover, without test assertions, future changes may causes regression on this aspect without us noticing it. See openwisp_utils.tests.AssertNumQueriesSubTestMixin, we did some work on this aspect.
The test coverage is also decreasing BTW.
See my comments below.
nemesifier
changed the title
…#383 - Implemented bulk notification creation using Django's bulk_create() - Added bulk_check_notification_storm_and_unread_count() function to perform aggregated queries instead of individual COUNT queries per recipient - Added bulk_notification_update_handler() for efficient websocket updates - Reduced SQL queries from 3*N to 1-2 total queries for N recipients - Preserved all existing functionality including email notifications - All tests pass with significant performance improvement Closes openwisp#383
Eeshu-Yadav
force-pushed
the
issues/383-optimize-notification-sql-queries
branch
from
08e710b to
7170605
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
openwisp_notifications/handlers.py (1)
142-145:⚠️ Potential issue | 🟠 MajorDon't materialize the full recipient set here.
list(recipients)eagerly loads every matched user before the first insert, and this path then keepsrecipients_list,notifications_to_create, andnotification_listalive at the same time. For large fan-outs that is still O(N) memory and can exhaust the worker. Stream querysets with.iterator()and flush bulk inserts / follow-up work in bounded chunks instead of building one giant list.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_notifications/handlers.py` around lines 142 - 145, The code is eagerly materializing recipients via recipients_list = list(recipients), which can OOM for large fan-outs; change the loop to stream recipients using recipients.iterator() (or an iterator wrapper) and accumulate notifications_to_create in bounded chunks (e.g., batch of N), call Model.objects.bulk_create(notification_list) for each full batch, then clear the batch lists after each insert to free memory; update any references to recipients_list/notification_list in the handler to work with the streaming iterator and chunked flush logic (keep the variable names notifications_to_create, recipients, notification_list as anchors to locate the change).openwisp_notifications/websockets/handlers.py (1)
144-149:⚠️ Potential issue | 🟡 MinorStop swallowing serializer failures.
If
NotificationListSerializerbreaks here, the recipient silently loses the toast payload and you lose the only signal that the payload is malformed. Log the exception with recipient / notification context, then fall back tonotification=None.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_notifications/websockets/handlers.py` around lines 144 - 149, The current try/except around NotificationListSerializer silently swallows errors; change it to catch Exception as e, log the exception with context (include recipient and the failing payload from notification_map[recipient] or relevant notification id) using the module logger (e.g., logger.exception or logger.error with the exception), and then set serialized_notification = None as the fallback; keep the surrounding code path that uses serialized_notification so recipients still receive a null payload instead of silently losing the toast.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_notifications/handlers.py`:
- Around line 170-171: The loop calling
send_email_notification(notification_list) makes N queries to check
instance.recipient.emailaddress_set.filter(...).exists(); precompute
verified-email state for all recipients before the loop (e.g., query
EmailAddress objects for recipients in notification_list and build a map keyed
by recipient id or (recipient id, email) indicating verified=True), then change
the loop to pass that map (or attach a cached property) into
send_email_notification(Notification, notification, created=True,
verified_map=...) or have send_email_notification read from the cached map
instead of querying the DB; update send_email_notification (and its callers) to
accept and use the pre-fetched verified lookup (referencing
send_email_notification and the emailaddress_set.filter(...).exists() check) so
the per-notification DB query is eliminated.
- Line 169: The code uses
Notification.objects.bulk_create(notifications_to_create) which bypasses
model.save() and pre/post_save signals and thus breaks custom logic in swapped
Notification models; replace the bulk_create call with iterating over
notifications_to_create and calling notif.save() for each (optionally wrapped in
transaction.atomic() for performance/atomicity), then call
send_email_notification() and invalidate_unread_cache() after saves (or keep
their current per-instance calls if they need to run per save); ensure you
update the Notification.save path referenced and remove reliance on bulk_create
so model-level save() and signal handlers run.
---
Duplicate comments:
In `@openwisp_notifications/handlers.py`:
- Around line 142-145: The code is eagerly materializing recipients via
recipients_list = list(recipients), which can OOM for large fan-outs; change the
loop to stream recipients using recipients.iterator() (or an iterator wrapper)
and accumulate notifications_to_create in bounded chunks (e.g., batch of N),
call Model.objects.bulk_create(notification_list) for each full batch, then
clear the batch lists after each insert to free memory; update any references to
recipients_list/notification_list in the handler to work with the streaming
iterator and chunked flush logic (keep the variable names
notifications_to_create, recipients, notification_list as anchors to locate the
change).
In `@openwisp_notifications/websockets/handlers.py`:
- Around line 144-149: The current try/except around NotificationListSerializer
silently swallows errors; change it to catch Exception as e, log the exception
with context (include recipient and the failing payload from
notification_map[recipient] or relevant notification id) using the module logger
(e.g., logger.exception or logger.error with the exception), and then set
serialized_notification = None as the fallback; keep the surrounding code path
that uses serialized_notification so recipients still receive a null payload
instead of silently losing the toast.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c08b4d14-a5ff-40d4-bea5-ae6524490a34
📒 Files selected for processing (2)
openwisp_notifications/handlers.pyopenwisp_notifications/websockets/handlers.py
📜 Review details
⏰ 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). (14)
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.2.0
Eeshu-Yadav
force-pushed
the
issues/383-optimize-notification-sql-queries
branch
from
b851d2c to
a14058d
Compare
Checklist
Reference to Existing Issue
Closes #383.
Description of Changes
This PR optimizes SQL queries in the notification system to address performance issues when sending notifications to multiple recipients. The main improvements include:
Performance Optimizations:
bulk_create()instead of individual savesCount()withQ()filters instead of separate COUNT queries per recipientTechnical Changes:
notify_handler()inhandlers.pyto use bulk creation with proper signal managementbulk_check_notification_storm_and_unread_count()function for efficient multi-recipient storm checkingbulk_notification_update_handler()for optimized websocket updates