⚡ - perf: attempt to increase performance of DestructionList related API endpoints

Author: svenvandescheur

backend/src/openarchiefbeheer/accounts/api/serializers.py

@@ -1,4 +1,5 @@
 from django.contrib.auth.models import Permission
+from django.db.models import Q
 from django.utils.translation import gettext_lazy as _
 
 from drf_spectacular.utils import extend_schema_field
@@ -34,15 +35,33 @@ class Meta:
 
     @extend_schema_field(RoleSerializer)
     def get_role(self, user: User) -> dict | None:
-        data = {}
-        for permission in Permission.objects.filter(user=user):
-            data[permission.codename] = True
+        """
+        Annotating a `UserQuerySet` using `annotate_permissions` (or `annotate_user_permission` on
+        `DestructionListQuerySet`) causes `user_permission_codenames` and `group_permission_codenames` to be set and
+         used for serialization. This may improve performance in cases where otherwise an n+1 issues could occur.
+        """
 
-        for group in user.groups.all():
-            for permission in group.permissions.all():
-                data[permission.codename] = True
+        # Retrieve all permission codenames.
+        permissions = []
 
-        serializer = RoleSerializer(data=data)
-        serializer.is_valid()
+        try:
+            permissions += user.user_permission_codenames
+            permissions += user.group_permission_codenames
 
-        return serializer.data
+        except AttributeError:
+            permissions = (
+                Permission.objects.filter(Q(user=user) | Q(group__user=user))
+                .distinct()
+                .values_list("codename", flat=True)
+            )
+
+        permissions_set = set(permissions)
+
+        data = {
+            "can_start_destruction": "can_start_destruction" in permissions_set,
+            "can_review_destruction": "can_review_destruction" in permissions_set,
+            "can_co_review_destruction": "can_co_review_destruction" in permissions_set,
+            "can_review_final_list": "can_review_final_list" in permissions_set,
+        }
+
+        return RoleSerializer(data).data

backend/src/openarchiefbeheer/accounts/managers.py

@@ -1,13 +1,46 @@
-from typing import TYPE_CHECKING
+from django.contrib.auth.models import BaseUserManager, Group, Permission
+from django.contrib.postgres.aggregates import ArrayAgg
+from django.db.models import Prefetch, Q, QuerySet
 
-from django.contrib.auth.models import BaseUserManager, Permission
-from django.db.models import Q, QuerySet
 
-if TYPE_CHECKING:
-    from .models import User
+class UserQuerySet(QuerySet):
+    def annotate_permissions(self) -> "UserQuerySet":
+        """
+        Adds `user_permission_codenames` and `group_permission_codenames` as `ArrayField` to the current QuerySet
+        containing the codenames of the applicable permissions. `user_permissions` and `permissions` are prefetched to
+        minimize queries.
+
+        This is used to avoid n+1 issues with nested `UserSerializer`.
+        """
+        return self.prefetch_related(
+            Prefetch(
+                "user_permissions",
+                queryset=Permission.objects.select_related("content_type").order_by(
+                    "codename"
+                ),
+            ),
+            Prefetch(
+                "groups",
+                queryset=Group.objects.prefetch_related(
+                    Prefetch(
+                        "permissions",
+                        queryset=Permission.objects.select_related(
+                            "content_type"
+                        ).order_by("codename"),
+                    )
+                ),
+            ),
+        ).annotate(
+            user_permission_codenames=ArrayAgg(
+                "user_permissions__codename", distinct=True
+            ),
+            group_permission_codenames=ArrayAgg(
+                "groups__permissions__codename", distinct=True
+            ),
+        )
 
 
-class UserManager(BaseUserManager):
+class UserManager(BaseUserManager.from_queryset(UserQuerySet)):
     use_in_migrations = True
 
     def _create_user(self, username, email, password, **extra_fields):
@@ -39,19 +72,19 @@ def create_superuser(self, username, email, password, **extra_fields):
 
         return self._create_user(username, email, password, **extra_fields)
 
-    def _users_with_permission(self, permission: Permission) -> QuerySet["User"]:
+    def _users_with_permission(self, permission: Permission) -> UserQuerySet:
         return self.filter(
             Q(groups__permissions=permission) | Q(user_permissions=permission)
         ).distinct()
 
-    def main_reviewers(self) -> QuerySet["User"]:
+    def main_reviewers(self) -> UserQuerySet:
         permission = Permission.objects.get(codename="can_review_destruction")
         return self._users_with_permission(permission)
 
-    def archivists(self) -> QuerySet["User"]:
+    def archivists(self) -> UserQuerySet:
         permission = Permission.objects.get(codename="can_review_final_list")
         return self._users_with_permission(permission)
 
-    def co_reviewers(self) -> QuerySet["User"]:
+    def co_reviewers(self) -> UserQuerySet:
         permission = Permission.objects.get(codename="can_co_review_destruction")
         return self._users_with_permission(permission)

backend/src/openarchiefbeheer/accounts/tests/test_managers.py

@@ -0,0 +1,69 @@
+from django.contrib.auth.models import Group, Permission
+from django.contrib.contenttypes.models import ContentType
+from django.test import TestCase
+
+from openarchiefbeheer.destruction.models import DestructionList
+
+from ..models import User
+from .factories import UserFactory
+
+
+class UserQuerySetTests(TestCase):
+    def test_annotate_permissions(self):
+        content_type = ContentType.objects.get_for_model(DestructionList)
+
+        group = Group.objects.create()
+        group_permission = Permission.objects.create(
+            codename="group_permission", content_type=content_type
+        )
+        group.permissions.add(group_permission)
+
+        user_permission = Permission.objects.create(
+            codename="user_permission", content_type=content_type
+        )
+
+        users = UserFactory.create_batch(3)
+        for user in users:
+            user.user_permissions.add(user_permission)
+            user.groups.add(group)
+
+        with self.assertNumQueries(1):
+            ls = list(  # List to force QuerySet evaluation.
+                User.objects.annotate_permissions().values_list(
+                    "user_permission_codenames", flat=True
+                )
+            )
+
+            self.assertIn("user_permission", ls[0])
+            self.assertIn("user_permission", ls[1])
+            self.assertIn("user_permission", ls[2])
+
+        with self.assertNumQueries(1):
+            ls = list(  # List to force QuerySet evaluation.
+                User.objects.annotate_permissions().values_list(
+                    "group_permission_codenames", flat=True
+                )
+            )
+
+            self.assertIn("group_permission", ls[0])
+            self.assertIn("group_permission", ls[1])
+            self.assertIn("group_permission", ls[2])
+
+
+class UserManagerTests(TestCase):
+    def test_create_superuser(self):
+        user = User.objects.create_superuser("god", "[email protected]", "praisejebus")
+        self.assertIsNotNone(user.pk)
+        self.assertTrue(user.is_staff)
+        self.assertTrue(user.is_superuser)
+        self.assertEqual(user.username, "god")
+        self.assertEqual(user.email, "[email protected]")
+        self.assertTrue(user.check_password("praisejebus"))
+        self.assertNotEqual(user.password, "praisejebus")
+
+    def test_create_user(self):
+        user = User.objects.create_user("infidel")
+        self.assertIsNotNone(user.pk)
+        self.assertFalse(user.is_superuser)
+        self.assertFalse(user.is_staff)
+        self.assertFalse(user.has_usable_password())

backend/src/openarchiefbeheer/accounts/tests/test_user_manager.py

@@ -1,7 +1,7 @@
 from datetime import date, timedelta
 
 from django.db import transaction
-from django.db.models import Case, OuterRef, Prefetch, QuerySet, Subquery, Value, When
+from django.db.models import Case, OuterRef, QuerySet, Subquery, Value, When
 from django.shortcuts import get_object_or_404
 from django.utils.translation import gettext_lazy as _
 
@@ -221,18 +221,8 @@ class DestructionListViewSet(
     viewsets.GenericViewSet,
 ):
     serializer_class = DestructionListWriteSerializer
-    queryset = (
-        DestructionList.objects.all()
-        .select_related("author", "assignee")
-        .prefetch_related(
-            Prefetch(
-                "assignees",
-                queryset=DestructionListAssignee.objects.prefetch_related(
-                    "user__user_permissions"
-                ).order_by("pk"),
-            )
-        )
-    )
+    queryset = DestructionList.objects.annotate_user_permissions()
+
     lookup_field = "uuid"
     filter_backends = (DjangoFilterBackend,)
     filterset_class = DestructionListFilterset

backend/src/openarchiefbeheer/destruction/api/viewsets.py

@@ -0,0 +1,39 @@
+from django.db.models import Manager, Prefetch, QuerySet
+
+from openarchiefbeheer.accounts.models import User
+
+
+class DestructionListQuerySet(QuerySet):
+    def annotate_user_permissions(self):
+        """
+        Adds `user_permission_codenames` and `group_permission_codenames` as `ArrayField` to the `author`,
+        `assignee`, and `assignees__user` in the current QuerySet containing the codenames of the applicable
+        permissions. `user_permissions` and `permissions` are prefetched to minimize queries.
+
+        This is used to avoid n+1 issues with nested `UserSerializer`.
+        """
+        from openarchiefbeheer.destruction.models import DestructionListAssignee
+
+        return self.prefetch_related(
+            Prefetch(
+                "author",
+                queryset=User.objects.annotate_permissions(),
+            ),
+            Prefetch(
+                "assignee",
+                queryset=User.objects.annotate_permissions(),
+            ),
+            Prefetch(
+                "assignees",
+                queryset=DestructionListAssignee.objects.prefetch_related(
+                    Prefetch(
+                        "user",
+                        queryset=User.objects.annotate_permissions(),
+                    )
+                ).order_by("pk"),
+            ),
+        )
+
+
+class DestructionListManager(Manager.from_queryset(DestructionListQuerySet)):
+    pass

backend/src/openarchiefbeheer/destruction/managers.py

@@ -35,6 +35,7 @@
     ZaakActionType,
 )
 from .exceptions import ZaakArchiefactiedatumInFuture, ZaakNotFound
+from .managers import DestructionListManager
 
 if TYPE_CHECKING:
     from openarchiefbeheer.zaken.models import Zaak
@@ -137,6 +138,8 @@ class DestructionList(models.Model):
 
     logs = GenericRelation(TimelineLog, related_query_name="destruction_list")
 
+    objects = DestructionListManager()
+
     class Meta:
         verbose_name = _("destruction list")
         verbose_name_plural = _("destruction lists")

backend/src/openarchiefbeheer/destruction/models.py

@@ -0,0 +1,40 @@
+from django.contrib.auth.models import Group, Permission
+from django.contrib.contenttypes.models import ContentType
+from django.test import TestCase
+
+from openarchiefbeheer.destruction.models import DestructionList
+from openarchiefbeheer.destruction.tests.factories import (
+    DestructionListAssigneeFactory,
+    DestructionListFactory,
+)
+
+
+class DestructionListQuerySetTests(TestCase):
+    def test_annotate_user_permissions(self):
+        content_type = ContentType.objects.get_for_model(DestructionList)
+
+        group = Group.objects.create()
+        group_permission = Permission.objects.create(
+            codename="group_permission", content_type=content_type
+        )
+        group.permissions.add(group_permission)
+
+        user_permission = Permission.objects.create(
+            codename="user_permission", content_type=content_type
+        )
+
+        assignees = DestructionListAssigneeFactory.create_batch(3)
+        author = assignees[0].user
+        author.user_permissions.add(user_permission)
+        destruction_lists = DestructionListFactory.create_batch(100, author=author)
+        for destruction_list in destruction_lists:
+            destruction_list.assignees.set(assignees)
+
+        with self.assertNumQueries(11):
+            ls = list(  # List to force QuerySet evaluation.
+                DestructionList.objects.filter(
+                    author=author
+                ).annotate_user_permissions()
+            )
+            for i in range(0, len(ls)):
+                self.assertIn("user_permission", ls[i].author.user_permission_codenames)