From 977316906666b24e88b451a0d24fd9f428a7b0ee Mon Sep 17 00:00:00 2001 From: Kevin Renskers Date: Fri, 10 Oct 2025 20:10:48 +0200 Subject: [PATCH 1/4] feat: Prefetch the target generic relationship by default and clarify performance considerations --- README.md | 76 ++++++++++++++++++++++++----- generic_notifications/models.py | 2 +- pyproject.toml | 2 +- tests/test_performance.py | 84 ++++++++++++++++++++++++++++++--- 4 files changed, 144 insertions(+), 20 deletions(-) diff --git a/README.md b/README.md index 7dab1de..d9a233f 100644 --- a/README.md +++ b/README.md @@ -213,7 +213,7 @@ While the library doesn't register admin classes by default, the [example app](# ## Advanced Usage -### Required Channels +### Required channels Make certain channels mandatory for critical notifications: @@ -228,7 +228,7 @@ class SecurityAlert(NotificationType): required_channels = [EmailChannel] # Cannot be disabled ``` -### Querying Notifications +### Querying notifications ```python from generic_notifications.channels import WebsiteChannel @@ -252,7 +252,7 @@ notification.mark_as_read() mark_notifications_as_read(user=user) ``` -### Notification Grouping +### Notification grouping Prevent notification spam by grouping similar notifications together. Instead of creating multiple "You received a comment" notifications, you can update an existing notification to say "You received 3 comments". @@ -301,11 +301,18 @@ The `should_save` method is called before saving each notification. Return `Fals ### Accessing `notification.target` -While you can store any object into a notification's `target` field, it's usually not a great idea to use this field to dynamically create the notification's subject and text, as the `target` generic relationship can't be prefetched more than one level deep. +The `target` field is a GenericForeignKey that can point to any Django model instance. While convenient, accessing targets requires careful consideration for performance. -In other words, something like this will cause an N+1 query problem when you show a list of notifications in a table, for example: +**Target Prefetching**: This library requires Django 5.0+ and automatically includes `.prefetch_related("target")` when using the standard query methods. This efficiently fetches target objects, but only the direct targets - accessing relationships *through* the target will still cause additional queries. + +Consider this problematic example that will cause N+1 queries: ```python +class Article(models.Model): + title = models.CharField(max_length=255) + text = models.TextField() + + class Comment(models.Model): article = models.ForeignKey(Article, on_delete=models.CASCADE) user = models.ForeignKey(User, on_delete=models.CASCADE) @@ -319,20 +326,65 @@ class CommentNotificationType(NotificationType): description = "You received a comment" def get_text(self, notification): - actor_name = notification.actor.full_name - article = notification.target.article - comment_text = notification.target.comment.comment_text - return f'{actor_name} commented on your article "{article.title}": "{comment_text}"' + actor_name = notification.actor.full_name + article = notification.target.article + comment_text = notification.target.comment_text + + # This causes an extra query per notification! + return f'{actor_name} commented on your article "{article.title}": "{comment_text}"' +``` + +When displaying a list of 10 notifications, this will execute: +- 1 query for the notifications +- 1 query for the target comments +- 10 queries for the articles (N+1 problem!) + +#### Solution 1: store data in the notification + +The best approach is to store the needed data directly in the notification: + +```python +send_notification( + recipient=article.author, + notification_type=CommentNotificationType, + actor=commenter, + target=comment, + subject=f"New comment on {article.title}", + text=f'{commenter.full_name} commented on your article "{article.title}": "{comment.comment_text}"', + url=article.get_absolute_url() +) ``` -The problem is `target.article`, which cannot be prefetched and thus causes another query for every notification. This is why it’s better to store the subject, text and url in the notification itself, rather than relying on `target` dynamically. +#### Solution 2: prefetch deeper relationships + +If you must access relationships through the target, you can prefetch them: + +```python +# The library already prefetches targets, but you need to add deeper relationships +notifications = get_notifications(user).prefetch_related( + "target__article" # This prevents the N+1 problem +) +``` + +**Note**: This approach has limitations: +- You need to know the target's type and relationships in advance +- It won't work efficiently with heterogeneous targets (different model types) +- Each additional relationship level requires explicit prefetching + +#### Recommended approach + +For best performance: + +1. If possible, store all display data directly in the notification (subject, text, url) +2. If you need dynamic text, prefer accessing only direct fields on the target +3. Otherwise, make sure you prefetch the right relationships ### Non-blocking email sending The email channel (EmailChannel) will send real-time emails using Django’s built-in `send_mail` method. This is a blocking function call, meaning that while a connection with the SMTP server is made and the email is sent off, the process that’s sending the notification has to wait. This is not ideal, but easily solved by using something like [django-mailer](https://github.com/pinax/django-mailer/), which provides a queueing backend for `send_mail`. This means that sending email is no longer a blocking action. -## Example app +## Example App An example app is provided, which shows how to create a custom notification type, how to send a notification, it has a nice looking notification center with unread notifications as well as an archive of all read notifications, plus a settings view where you can manage notification preferences. @@ -361,7 +413,7 @@ cd django-generic-notifications uv run pytest ``` -### Code Quality +### Code quality ```bash # Type checking diff --git a/generic_notifications/models.py b/generic_notifications/models.py index 3256316..491914c 100644 --- a/generic_notifications/models.py +++ b/generic_notifications/models.py @@ -18,7 +18,7 @@ class NotificationQuerySet(models.QuerySet): def prefetch(self): """Prefetch related objects""" - return self.select_related("recipient", "actor") + return self.select_related("recipient", "actor").prefetch_related("target") def for_channel(self, channel: type[NotificationChannel] = WebsiteChannel): """Filter notifications by channel""" diff --git a/pyproject.toml b/pyproject.toml index bd2ea03..71c3e75 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -10,7 +10,7 @@ license-files = [ "LICENSE" ] readme = "README.md" requires-python = ">=3.10" dependencies = [ - "django>=4.2.0", + "django>=5.0.0", ] keywords = [ "django", diff --git a/tests/test_performance.py b/tests/test_performance.py index 255b9f1..1ceffb8 100644 --- a/tests/test_performance.py +++ b/tests/test_performance.py @@ -59,9 +59,8 @@ def test_notification_template_rendering_queries(self): _ = notification.id def test_notification_target_access_queries(self): - """Test queries when accessing notification.target in template""" + """Test queries when accessing notification.target without explicit prefetching""" # Create notifications with targets - Notification.objects.all().delete() for i in range(5): Notification.objects.create( recipient=self.user, @@ -74,11 +73,84 @@ def test_notification_target_access_queries(self): ) # First, evaluate the queryset - notifications = get_notifications(self.user) - notifications_list = list(notifications) + with self.assertNumQueries(2): # 1 for notifications + 1 for targets + notifications = get_notifications(self.user) + notifications_list = list(notifications) - # Test accessing target - this will cause queries - with self.assertNumQueries(5): # Expecting 5 queries + # Test accessing target - should be 0 queries since we prefetch target + with self.assertNumQueries(0): for notification in notifications_list: if notification.target and hasattr(notification.target, "email"): _ = notification.target.email + + def test_notification_target_relationship_access(self): + """Test that accessing relationships through target causes additional queries""" + # Create notifications where each has a different notification as its target + for i in range(5): + # Create the target notification + target_notification = Notification.objects.create( + recipient=self.actor, + notification_type="target_notification", + subject=f"Target notification {i}", + text=f"Target text {i}", + channels=[WebsiteChannel.key], + ) + + # Create notification pointing to it + Notification.objects.create( + recipient=self.user, + actor=self.actor, + notification_type="test_notification", + subject=f"Test notification {i}", + text=f"This is test notification {i}", + channels=[WebsiteChannel.key], + target=target_notification, + ) + + # First, evaluate the queryset + with self.assertNumQueries(2): # 1 for notifications + 1 for targets + notifications = get_notifications(self.user) + notifications_list = list(notifications) + + # Test accessing target.recipient - this WILL cause N+1 queries + # because we didn't prefetch the recipient relationship on the target notifications + with self.assertNumQueries(5): # 5 queries for recipient access + for notification in notifications_list: + if notification.target and hasattr(notification.target, "recipient"): + _ = notification.target.recipient.email + + def test_notification_target_relationship_preselect_access(self): + """Test that accessing relationships through target causes additional queries""" + # Create notifications where each has a different notification as its target + for i in range(5): + # Create the target notification + target_notification = Notification.objects.create( + recipient=self.actor, + notification_type="target_notification", + subject=f"Target notification {i}", + text=f"Target text {i}", + channels=[WebsiteChannel.key], + ) + + # Create notification pointing to it + Notification.objects.create( + recipient=self.user, + actor=self.actor, + notification_type="test_notification", + subject=f"Test notification {i}", + text=f"This is test notification {i}", + channels=[WebsiteChannel.key], + target=target_notification, + ) + + # First, evaluate the queryset + with self.assertNumQueries(3): # 1 for notifications + 1 for targets + 1 for target recipients + notifications = get_notifications(self.user).prefetch_related("target__recipient") + notifications_list = list(notifications) + + # Test accessing target.recipient - this WILL cause N+1 queries + # because we didn't prefetch the recipient relationship on the target notifications + with self.assertNumQueries(0): # 5 queries for recipient access + for notification in notifications_list: + if notification.target and hasattr(notification.target, "recipient"): + _ = notification.target.recipient.email From a729b571fc38e01aaba95b89eab9929bb8df605a Mon Sep 17 00:00:00 2001 From: Kevin Renskers Date: Fri, 10 Oct 2025 20:22:02 +0200 Subject: [PATCH 2/4] Keep Django 4.2 support --- README.md | 9 ++++++--- generic_notifications/models.py | 9 ++++++++- pyproject.toml | 2 +- 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index d9a233f..7d3524d 100644 --- a/README.md +++ b/README.md @@ -303,7 +303,9 @@ The `should_save` method is called before saving each notification. Return `Fals The `target` field is a GenericForeignKey that can point to any Django model instance. While convenient, accessing targets requires careful consideration for performance. -**Target Prefetching**: This library requires Django 5.0+ and automatically includes `.prefetch_related("target")` when using the standard query methods. This efficiently fetches target objects, but only the direct targets - accessing relationships *through* the target will still cause additional queries. +When using Django 5.0+, this library automatically includes `.prefetch_related("target")` when using the standard query methods. This efficiently fetches target objects, but only the *direct* targets - accessing relationships *through* the target will still cause additional queries. + +*On Django 4.2, you'll need to manually deal with prefetching the `target` relationship.* Consider this problematic example that will cause N+1 queries: @@ -337,7 +339,7 @@ class CommentNotificationType(NotificationType): When displaying a list of 10 notifications, this will execute: - 1 query for the notifications -- 1 query for the target comments +- 1 query for the target comments (Django 5.0+ only, automatically prefetched) - 10 queries for the articles (N+1 problem!) #### Solution 1: store data in the notification @@ -361,7 +363,8 @@ send_notification( If you must access relationships through the target, you can prefetch them: ```python -# The library already prefetches targets, but you need to add deeper relationships +# On Django 5.0+ the library already prefetches targets, +# but you need to add deeper relationships yourself notifications = get_notifications(user).prefetch_related( "target__article" # This prevents the N+1 problem ) diff --git a/generic_notifications/models.py b/generic_notifications/models.py index 491914c..0a128f7 100644 --- a/generic_notifications/models.py +++ b/generic_notifications/models.py @@ -1,3 +1,4 @@ +import django from django.conf import settings from django.contrib.auth import get_user_model from django.contrib.contenttypes.fields import GenericForeignKey @@ -18,7 +19,13 @@ class NotificationQuerySet(models.QuerySet): def prefetch(self): """Prefetch related objects""" - return self.select_related("recipient", "actor").prefetch_related("target") + qs = self.select_related("recipient", "actor") + + # Only add target prefetching on Django 5.0+ due to GenericForeignKey limitations + if django.VERSION >= (5, 0): + qs = qs.prefetch_related("target") + + return qs def for_channel(self, channel: type[NotificationChannel] = WebsiteChannel): """Filter notifications by channel""" diff --git a/pyproject.toml b/pyproject.toml index 71c3e75..bd2ea03 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -10,7 +10,7 @@ license-files = [ "LICENSE" ] readme = "README.md" requires-python = ">=3.10" dependencies = [ - "django>=5.0.0", + "django>=4.2.0", ] keywords = [ "django", From 50d5d291199b7bd46c2a71eb66703d782efd69df Mon Sep 17 00:00:00 2001 From: Kevin Renskers Date: Fri, 10 Oct 2025 20:32:01 +0200 Subject: [PATCH 3/4] Improve README --- README.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 7d3524d..f675be2 100644 --- a/README.md +++ b/README.md @@ -344,7 +344,7 @@ When displaying a list of 10 notifications, this will execute: #### Solution 1: store data in the notification -The best approach is to store the needed data directly in the notification: +The simplest approach is to store the needed data directly in the notification: ```python send_notification( @@ -358,6 +358,8 @@ send_notification( ) ``` +However, this only works if you don’t need to dynamically generate the text - for example to make sure the text is always up to date, or to deal with internationalization. + #### Solution 2: prefetch deeper relationships If you must access relationships through the target, you can prefetch them: From da6ccb63ba4d646e83a47f3f8a736494bf2361b4 Mon Sep 17 00:00:00 2001 From: Kevin Renskers Date: Fri, 10 Oct 2025 20:37:54 +0200 Subject: [PATCH 4/4] Fix comments, bad copy paste --- tests/test_performance.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/test_performance.py b/tests/test_performance.py index 1ceffb8..99a8117 100644 --- a/tests/test_performance.py +++ b/tests/test_performance.py @@ -59,7 +59,7 @@ def test_notification_template_rendering_queries(self): _ = notification.id def test_notification_target_access_queries(self): - """Test queries when accessing notification.target without explicit prefetching""" + """Test queries when accessing notification.target in template""" # Create notifications with targets for i in range(5): Notification.objects.create( @@ -113,7 +113,7 @@ def test_notification_target_relationship_access(self): notifications_list = list(notifications) # Test accessing target.recipient - this WILL cause N+1 queries - # because we didn't prefetch the recipient relationship on the target notifications + # because we didn't prefetch the target__recipient relationship with self.assertNumQueries(5): # 5 queries for recipient access for notification in notifications_list: if notification.target and hasattr(notification.target, "recipient"): @@ -148,9 +148,9 @@ def test_notification_target_relationship_preselect_access(self): notifications = get_notifications(self.user).prefetch_related("target__recipient") notifications_list = list(notifications) - # Test accessing target.recipient - this WILL cause N+1 queries - # because we didn't prefetch the recipient relationship on the target notifications - with self.assertNumQueries(0): # 5 queries for recipient access + # Test accessing target.recipient - this won't cause N+1 queries + # because we now prefetch the target__recipient relationship + with self.assertNumQueries(0): for notification in notifications_list: if notification.target and hasattr(notification.target, "recipient"): _ = notification.target.recipient.email