Fix issue #383: Optimize SQL queries for notification storm prevention #399
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
📝 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 (Count, Q)
DB-->>WS: unread_count per recipient, storm determination
WS->>Cache: store storm flags / counts
end
WS->>Client: send update per recipient (notification payload optional, in_notification_storm, notification_count)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![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.
| recipient_ids = [str(recipient.pk) for recipient in recipients] | ||
| cached_storm_data = cache.get_many([f"ow-noti-storm-{pk}" for pk in recipient_ids]) | ||
|
|
||
| results = {} | ||
| uncached_recipients = [] | ||
|
|
||
| for recipient in recipients: | ||
| cache_key = f"ow-noti-storm-{recipient.pk}" | ||
| if cache_key in cached_storm_data: | ||
| results[recipient.pk] = [cached_storm_data[cache_key], None] | ||
| else: | ||
| uncached_recipients.append(recipient) | ||
| results[recipient.pk] = [False, None] | ||
|
|
||
| if uncached_recipients: | ||
| short_term_threshold = now() - timedelta( | ||
| seconds=app_settings.NOTIFICATION_STORM_PREVENTION["short_term_time_period"] | ||
| ) | ||
| long_term_threshold = now() - timedelta( | ||
| seconds=app_settings.NOTIFICATION_STORM_PREVENTION["long_term_time_period"] | ||
| ) | ||
|
|
||
| storm_and_unread_data = ( | ||
| Notification.objects.filter(recipient_id__in=recipient_ids) | ||
| .values("recipient_id") | ||
| .annotate( | ||
| short_term_count=Count( | ||
| "id", filter=Q(timestamp__gte=short_term_threshold) | ||
| ), | ||
| long_term_count=Count( | ||
| "id", filter=Q(timestamp__gte=long_term_threshold) | ||
| ), | ||
| unread_count=Count("id", filter=Q(unread=True)), | ||
| ) | ||
| ) |
There was a problem hiding this comment.
In bulk_check_notification_storm_and_unread_count, recipient_ids is computed from all recipients but then used in the aggregated query even when most of them are already covered by cached_storm_data, which leads to unnecessary database work in high-fanout scenarios where many users are in the storm cache. You could reduce this overhead by restricting the aggregation query to uncached_recipients (and computing their IDs once), while still running the separate unread-count query for cached recipients as you already do.
openwisp_notifications/handlers.py
Outdated
| post_save.disconnect(clear_notification_cache, sender=Notification) | ||
|
|
||
| try: | ||
| notification_list = Notification.objects.bulk_create(notifications_to_create) | ||
|
|
||
| for notification in notification_list: | ||
| send_email_notification(Notification, notification, created=True) | ||
|
|
||
| for recipient in recipients_list: | ||
| Notification.invalidate_unread_cache(recipient) | ||
|
|
||
| first_notification = notification_list[0] if notification_list else None | ||
| ws_handlers.bulk_notification_update_handler( | ||
| recipients=recipients_list, | ||
| reload_widget=True, | ||
| notification=first_notification, | ||
| ) | ||
|
|
||
| finally: | ||
| post_save.connect( | ||
| clear_notification_cache, | ||
| sender=Notification, | ||
| dispatch_uid="clear_notification_cache_saved", | ||
| ) | ||
|
|
There was a problem hiding this comment.
Temporarily disconnecting the global clear_notification_cache post_save receiver around bulk_create introduces a race condition: any Notification instances saved by other code paths while this block is executing will not trigger cache invalidation or websocket updates. Since bulk_create does not emit post_save signals, the disconnect/reconnect here is unnecessary; removing it would avoid global side effects while preserving the intended behavior of manually invalidating the unread cache and sending websocket updates.
| post_save.disconnect(clear_notification_cache, sender=Notification) | |
| try: | |
| notification_list = Notification.objects.bulk_create(notifications_to_create) | |
| for notification in notification_list: | |
| send_email_notification(Notification, notification, created=True) | |
| for recipient in recipients_list: | |
| Notification.invalidate_unread_cache(recipient) | |
| first_notification = notification_list[0] if notification_list else None | |
| ws_handlers.bulk_notification_update_handler( | |
| recipients=recipients_list, | |
| reload_widget=True, | |
| notification=first_notification, | |
| ) | |
| finally: | |
| post_save.connect( | |
| clear_notification_cache, | |
| sender=Notification, | |
| dispatch_uid="clear_notification_cache_saved", | |
| ) | |
| notification_list = Notification.objects.bulk_create(notifications_to_create) | |
| for notification in notification_list: | |
| send_email_notification(Notification, notification, created=True) | |
| for recipient in recipients_list: | |
| Notification.invalidate_unread_cache(recipient) | |
| first_notification = notification_list[0] if notification_list else None | |
| ws_handlers.bulk_notification_update_handler( | |
| recipients=recipients_list, | |
| reload_widget=True, | |
| notification=first_notification, | |
| ) |
| 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.
…#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
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.
| notification_list = Notification.objects.bulk_create(notifications_to_create) | ||
| for notification in notification_list: | ||
| send_email_notification(Notification, notification, created=True) | ||
| for recipient in recipients_list: | ||
| Notification.invalidate_unread_cache(recipient) | ||
| notification_map = dict(zip(recipients_list, notification_list)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.py" | grep -E "(handlers|settings|config)" | head -20Repository: openwisp/openwisp-notifications
Length of output: 389
🏁 Script executed:
git ls-files | head -30Repository: openwisp/openwisp-notifications
Length of output: 890
🏁 Script executed:
wc -l openwisp_notifications/handlers.pyRepository: openwisp/openwisp-notifications
Length of output: 113
🏁 Script executed:
sed -n '160,185p' openwisp_notifications/handlers.pyRepository: openwisp/openwisp-notifications
Length of output: 1188
🏁 Script executed:
sed -n '1,50p' openwisp_notifications/handlers.pyRepository: openwisp/openwisp-notifications
Length of output: 1999
🏁 Script executed:
rg -n "send_email_notification" openwisp_notifications/handlers.py -B 2 -A 10Repository: openwisp/openwisp-notifications
Length of output: 1232
🏁 Script executed:
sed -n '183,210p' openwisp_notifications/handlers.pyRepository: openwisp/openwisp-notifications
Length of output: 1205
🏁 Script executed:
sed -n '210,240p' openwisp_notifications/handlers.pyRepository: openwisp/openwisp-notifications
Length of output: 1255
🏁 Script executed:
cat openwisp_notifications/websockets/handlers.pyRepository: openwisp/openwisp-notifications
Length of output: 6626
🏁 Script executed:
rg -n "bulk_create" openwisp_notifications/ -B 2 -A 2Repository: openwisp/openwisp-notifications
Length of output: 2108
🏁 Script executed:
grep -r "DATABASE" tests/openwisp2/settings.py openwisp_notifications/settings.pyRepository: openwisp/openwisp-notifications
Length of output: 116
🏁 Script executed:
cat tests/openwisp2/settings.py | grep -A 10 "DATABASES"Repository: openwisp/openwisp-notifications
Length of output: 331
🏁 Script executed:
find . -name "setup.py" -o -name "setup.cfg" -o -name "pyproject.toml" -o -name "requirements*.txt" | xargs grep -l "django\|Django" 2>/dev/nullRepository: openwisp/openwisp-notifications
Length of output: 126
🏁 Script executed:
cat setup.py | grep -A 20 "install_requires\|Django"Repository: openwisp/openwisp-notifications
Length of output: 1160
🏁 Script executed:
rg -n "bulk_create.*populate_pk|return_pk" openwisp_notifications/ tests/Repository: openwisp/openwisp-notifications
Length of output: 57
🏁 Script executed:
cat requirements.txtRepository: openwisp/openwisp-notifications
Length of output: 290
🏁 Script executed:
rg -n "\.id" openwisp_notifications/handlers.py | grep -E "(notification|batched_notifications)"Repository: openwisp/openwisp-notifications
Length of output: 124
🏁 Script executed:
rg -n "notification.id" openwisp_notifications/handlers.py -B 2 -A 2Repository: openwisp/openwisp-notifications
Length of output: 57
🏁 Script executed:
sed -n '215,230p' openwisp_notifications/handlers.pyRepository: openwisp/openwisp-notifications
Length of output: 689
🏁 Script executed:
rg -n "instance\.id\|notification\.id" openwisp_notifications/handlers.pyRepository: openwisp/openwisp-notifications
Length of output: 57
🏁 Script executed:
python3 << 'EOF'
# Check Django bulk_create behavior
print("Django bulk_create() behavior:")
print("- For Django 3.1+: bulk_create() always populates PKs by default")
print("- For SQLite: PKs are populated (uses lastrowid)")
print("- Exceptions: Custom primary keys, certain edge cases")
print("\nKey point: batch_start_time on line 220 uses instance.id")
print("If instance.id is None, batching could fail silently")
EOFRepository: openwisp/openwisp-notifications
Length of output: 384
🏁 Script executed:
cat requirements.txt | grep -i djangoRepository: openwisp/openwisp-notifications
Length of output: 57
🏁 Script executed:
cat setup.py | grep -E "openwisp-users|openwisp-utils"Repository: openwisp/openwisp-notifications
Length of output: 57
🏁 Script executed:
rg -n "return_pk\|populate_pk" . --type pyRepository: openwisp/openwisp-notifications
Length of output: 57
🏁 Script executed:
rg -n "test.*bulk_create\|bulk_create.*test" openwisp_notifications/tests/ -B 3 -A 5 | head -40Repository: openwisp/openwisp-notifications
Length of output: 57
🏁 Script executed:
rg -n "def test.*notify" openwisp_notifications/tests/ -B 1 -A 8 | head -60Repository: openwisp/openwisp-notifications
Length of output: 57
🏁 Script executed:
fd "test_.*\.py" openwisp_notifications/tests/ --type fRepository: openwisp/openwisp-notifications
Length of output: 725
🏁 Script executed:
rg -n "send_email_notification\|bulk_create" openwisp_notifications/tests/test_handlers.py -B 2 -A 5 | head -80Repository: openwisp/openwisp-notifications
Length of output: 160
🏁 Script executed:
rg -n "notify_handler\|send_email_notification\|bulk_create" openwisp_notifications/tests/test_notifications.py -B 2 -A 5 | head -100Repository: openwisp/openwisp-notifications
Length of output: 57
🏁 Script executed:
rg -n "class.*Test.*Notif" openwisp_notifications/tests/test_notifications.pyRepository: openwisp/openwisp-notifications
Length of output: 231
🏁 Script executed:
sed -n '1,100p' openwisp_notifications/tests/test_notifications.pyRepository: openwisp/openwisp-notifications
Length of output: 3707
🏁 Script executed:
rg -n "batched_notifications\|batch.*email" openwisp_notifications/tests/test_notifications.py -B 3 -A 8 | head -150Repository: openwisp/openwisp-notifications
Length of output: 57
🏁 Script executed:
rg -n "def test.*batch" openwisp_notifications/tests/test_notifications.pyRepository: openwisp/openwisp-notifications
Length of output: 689
🏁 Script executed:
sed -n '1500,1550p' openwisp_notifications/tests/test_notifications.pyRepository: openwisp/openwisp-notifications
Length of output: 2196
🏁 Script executed:
sed -n '1098,1135p' openwisp_notifications/tests/test_notifications.pyRepository: openwisp/openwisp-notifications
Length of output: 1867
🏁 Script executed:
sed -n '1136,1203p' openwisp_notifications/tests/test_notifications.pyRepository: openwisp/openwisp-notifications
Length of output: 3107
🏁 Script executed:
web_search
Django bulk_create PK population SQLite behaviorRepository: openwisp/openwisp-notifications
Length of output: 170
🌐 Web query:
Django bulk_create PK population SQLite backend
💡 Result:
With Django’s bulk_create(), primary keys (AutoField/BigAutoField) are only populated on the in-memory instances if the DB backend supports returning rows from a bulk insert (i.e., Django can use INSERT … RETURNING …). Django’s docs explicitly list SQLite 3.35+ as supported (along with PostgreSQL and MariaDB 10.5+); otherwise, the objects are inserted but their .pk stays None. [1]
For SQLite, PK population after bulk_create() therefore requires:
- SQLite library ≥ 3.35 (the version your Python
sqlite3module is linked against), and - a Django version where this support exists (added in Django 4.0). [1]
Common reasons you still see pk=None on SQLite:
- Your runtime SQLite is < 3.35 (very common on older OS/Python builds). Check:
import sqlite3 sqlite3.sqlite_version
- You used
ignore_conflicts=True, which disables PK-setting even on DBs that otherwise support it. [1] - You’re on a backend/path where
can_return_rows_from_bulk_insertis false, in which case Django won’t set autoincrement PKs duringbulk_create(). [2]
Workarounds if you can’t upgrade SQLite:
- Don’t rely on returned PKs (query again), or
- Use non-autoincrement PKs you set yourself (e.g., UUID primary keys) if you must have IDs immediately.
Sources: Django QuerySet bulk_create() docs (PK support + SQLite 3.35+ note, Django 4.0 change) [1]; Django ORM implementation notes on PK setting depending on can_return_rows_from_bulk_insert [2].
[1] (docs.djangoproject.com)
[2] (docs.djangoproject.com)
Citations:
- 1: https://docs.djangoproject.com/en/4.0/ref/models/querysets/?utm_source=openai
- 2: https://docs.djangoproject.com/en/5.0/_modules/django/db/models/query/?utm_source=openai
🏁 Script executed:
rg -n "\.id.*None\|None.*\.id" openwisp_notifications/handlers.pyRepository: openwisp/openwisp-notifications
Length of output: 57
🏁 Script executed:
rg -n "class Notification" openwisp_notifications/ --type py -A 20 | head -60Repository: openwisp/openwisp-notifications
Length of output: 4677
🏁 Script executed:
sed -n '183,250p' openwisp_notifications/handlers.pyRepository: openwisp/openwisp-notifications
Length of output: 2709
🏁 Script executed:
grep -n "populate_pk\|return_pk\|can_return_rows" openwisp_notifications/ -rRepository: openwisp/openwisp-notifications
Length of output: 57
🏁 Script executed:
rg -n "ignore_conflicts" openwisp_notifications/ --type pyRepository: openwisp/openwisp-notifications
Length of output: 181
🏁 Script executed:
rg -n "post_save.*Notification\|receiver.*send_email" openwisp_notifications/handlers.py -B 2 -A 2Repository: openwisp/openwisp-notifications
Length of output: 57
🏁 Script executed:
sed -n '165,181p' openwisp_notifications/handlers.pyRepository: openwisp/openwisp-notifications
Length of output: 758
🏁 Script executed:
rg -n "dispatch_uid.*send_email" openwisp_notifications/ --type pyRepository: openwisp/openwisp-notifications
Length of output: 195
🏁 Script executed:
rg -n "def send_email" openwisp_notifications/models.py -A 15 | head -40Repository: openwisp/openwisp-notifications
Length of output: 57
🏁 Script executed:
rg -n "Django\|django" setup.py requirements.txt pyproject.toml | grep -i versionRepository: openwisp/openwisp-notifications
Length of output: 57
🏁 Script executed:
sed -n '100,150p' openwisp_notifications/handlers.pyRepository: openwisp/openwisp-notifications
Length of output: 1814
Ensure bulk_create() returns populated PKs before using notifications for email batching and WebSocket updates
Line 169's bulk_create() may not populate PKs depending on the database backend and Django version. SQLite requires version 3.35+ and Django 4.0+; older environments will return objects with id=None. Line 220 then appends instance.id to batched_notifications, and if id is None, this silently corrupts the batch tracking list. The WebSocket serialization at line 135 also depends on valid notification IDs.
Explicitly handle the case where PKs are unpopulated, either by:
- Re-fetching notifications after insertion with
values_list('id', flat=True)or similar, or - Confirming your deployment guarantees SQLite 3.35+ and Django 4.0+, then verify with a test assertion.
🧰 Tools
🪛 Ruff (0.14.14)
[warning] 174-174: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
🤖 Prompt for AI Agents
In `@openwisp_notifications/handlers.py` around lines 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.
2 participants
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