From 9ba0080b106b1fe256e3137310996505155e2025 Mon Sep 17 00:00:00 2001 From: Sven van de Scheur Date: Thu, 12 Dec 2024 13:07:29 +0100 Subject: [PATCH] :zap: - perf: attempt to increase performance of DestructionList related API endpoints --- .../accounts/api/serializers.py | 37 +++++++--- .../openarchiefbeheer/accounts/managers.py | 53 ++++++++++++--- .../accounts/tests/test_managers.py | 68 +++++++++++++++++++ .../accounts/tests/test_user_manager.py | 22 ------ .../destruction/api/viewsets.py | 16 +---- .../openarchiefbeheer/destruction/managers.py | 39 +++++++++++ .../openarchiefbeheer/destruction/models.py | 3 + .../destruction/tests/test_managers.py | 40 +++++++++++ 8 files changed, 224 insertions(+), 54 deletions(-) create mode 100644 backend/src/openarchiefbeheer/accounts/tests/test_managers.py delete mode 100644 backend/src/openarchiefbeheer/accounts/tests/test_user_manager.py create mode 100644 backend/src/openarchiefbeheer/destruction/managers.py create mode 100644 backend/src/openarchiefbeheer/destruction/tests/test_managers.py diff --git a/backend/src/openarchiefbeheer/accounts/api/serializers.py b/backend/src/openarchiefbeheer/accounts/api/serializers.py index 7dfcf4eb6..42d6f8807 100644 --- a/backend/src/openarchiefbeheer/accounts/api/serializers.py +++ b/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 diff --git a/backend/src/openarchiefbeheer/accounts/managers.py b/backend/src/openarchiefbeheer/accounts/managers.py index de074d557..1ad83d19d 100644 --- a/backend/src/openarchiefbeheer/accounts/managers.py +++ b/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) diff --git a/backend/src/openarchiefbeheer/accounts/tests/test_managers.py b/backend/src/openarchiefbeheer/accounts/tests/test_managers.py new file mode 100644 index 000000000..eccf05313 --- /dev/null +++ b/backend/src/openarchiefbeheer/accounts/tests/test_managers.py @@ -0,0 +1,68 @@ +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", "god@heaven.com", "praisejebus") + self.assertIsNotNone(user.pk) + self.assertTrue(user.is_staff) + self.assertTrue(user.is_superuser) + self.assertEqual(user.username, "god") + self.assertEqual(user.email, "god@heaven.com") + 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()) diff --git a/backend/src/openarchiefbeheer/accounts/tests/test_user_manager.py b/backend/src/openarchiefbeheer/accounts/tests/test_user_manager.py deleted file mode 100644 index 1a6ab7d2b..000000000 --- a/backend/src/openarchiefbeheer/accounts/tests/test_user_manager.py +++ /dev/null @@ -1,22 +0,0 @@ -from django.test import TestCase - -from ..models import User - - -class UserManagerTests(TestCase): - def test_create_superuser(self): - user = User.objects.create_superuser("god", "god@heaven.com", "praisejebus") - self.assertIsNotNone(user.pk) - self.assertTrue(user.is_staff) - self.assertTrue(user.is_superuser) - self.assertEqual(user.username, "god") - self.assertEqual(user.email, "god@heaven.com") - 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()) diff --git a/backend/src/openarchiefbeheer/destruction/api/viewsets.py b/backend/src/openarchiefbeheer/destruction/api/viewsets.py index 933e7ef1b..ba66d2fe4 100644 --- a/backend/src/openarchiefbeheer/destruction/api/viewsets.py +++ b/backend/src/openarchiefbeheer/destruction/api/viewsets.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 diff --git a/backend/src/openarchiefbeheer/destruction/managers.py b/backend/src/openarchiefbeheer/destruction/managers.py new file mode 100644 index 000000000..3e9379ea2 --- /dev/null +++ b/backend/src/openarchiefbeheer/destruction/managers.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 diff --git a/backend/src/openarchiefbeheer/destruction/models.py b/backend/src/openarchiefbeheer/destruction/models.py index 778b066ac..e37d13666 100644 --- a/backend/src/openarchiefbeheer/destruction/models.py +++ b/backend/src/openarchiefbeheer/destruction/models.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") diff --git a/backend/src/openarchiefbeheer/destruction/tests/test_managers.py b/backend/src/openarchiefbeheer/destruction/tests/test_managers.py new file mode 100644 index 000000000..4c61660df --- /dev/null +++ b/backend/src/openarchiefbeheer/destruction/tests/test_managers.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)