Fix n+1 queries

pydis_site/apps/api/migrations/0054_user_invalidate_unknown_role.py

@@ -3,7 +3,6 @@
 import django.contrib.postgres.fields
 import django.core.validators
 from django.db import migrations, models
-import pydis_site.apps.api.models.bot.user
 
 
 class Migration(migrations.Migration):
@@ -16,6 +15,6 @@ class Migration(migrations.Migration):
         migrations.AlterField(
             model_name='user',
             name='roles',
-            field=django.contrib.postgres.fields.ArrayField(base_field=models.BigIntegerField(validators=[django.core.validators.MinValueValidator(limit_value=0, message='Role IDs cannot be negative.'), pydis_site.apps.api.models.bot.user._validate_existing_role]), default=list, help_text='IDs of roles the user has on the server', size=None),
+            field=django.contrib.postgres.fields.ArrayField(base_field=models.BigIntegerField(validators=[django.core.validators.MinValueValidator(limit_value=0, message='Role IDs cannot be negative.')]), default=list, help_text='IDs of roles the user has on the server', size=None),
         ),
     ]

pydis_site/apps/api/migrations/0056_allow_blank_user_roles.py

@@ -3,7 +3,6 @@
 import django.contrib.postgres.fields
 import django.core.validators
 from django.db import migrations, models
-import pydis_site.apps.api.models.bot.user
 
 
 class Migration(migrations.Migration):
@@ -16,6 +15,6 @@ class Migration(migrations.Migration):
         migrations.AlterField(
             model_name='user',
             name='roles',
-            field=django.contrib.postgres.fields.ArrayField(base_field=models.BigIntegerField(validators=[django.core.validators.MinValueValidator(limit_value=0, message='Role IDs cannot be negative.'), pydis_site.apps.api.models.bot.user._validate_existing_role]), blank=True, default=list, help_text='IDs of roles the user has on the server', size=None),
+            field=django.contrib.postgres.fields.ArrayField(base_field=models.BigIntegerField(validators=[django.core.validators.MinValueValidator(limit_value=0, message='Role IDs cannot be negative.')]), blank=True, default=list, help_text='IDs of roles the user has on the server', size=None),
         ),
     ]

pydis_site/apps/api/models/bot/user.py

@@ -1,20 +1,11 @@
 from django.contrib.postgres.fields import ArrayField
-from django.core.exceptions import ValidationError
 from django.core.validators import MaxValueValidator, MinValueValidator
 from django.db import models
 
 from pydis_site.apps.api.models.bot.role import Role
 from pydis_site.apps.api.models.mixins import ModelReprMixin, ModelTimestampMixin
 
 
-def _validate_existing_role(value: int) -> None:
-    """Validate that a role exists when given in to the user model."""
-    role = Role.objects.filter(id=value)
-
-    if not role:
-        raise ValidationError(f"Role with ID {value} does not exist")
-
-
 class User(ModelReprMixin, models.Model):
     """A Discord user."""
 
@@ -54,7 +45,6 @@ class User(ModelReprMixin, models.Model):
                     limit_value=0,
                     message="Role IDs cannot be negative."
                 ),
-                _validate_existing_role
             )
         ),
         default=list,

pydis_site/apps/api/serializers.py

@@ -8,6 +8,7 @@
 from rest_framework.exceptions import NotFound
 from rest_framework.serializers import (
     IntegerField,
+    ListField,
     ListSerializer,
     ModelSerializer,
     PrimaryKeyRelatedField,
@@ -684,12 +685,8 @@ def to_representation(self, instance: UserAltRelationship) -> dict:
         """Add the alts of the target to the representation."""
         representation = super().to_representation(instance)
         representation['alts'] = tuple(
-            user_id
-            for (user_id,) in (
-                UserAltRelationship.objects
-                .filter(source=instance.target)
-                .values_list('target_id')
-            )
+            relationship.target_id
+            for relationship in instance.target.useraltrelationship_set.all()
         )
         return representation
 
@@ -700,6 +697,14 @@ class UserSerializer(ModelSerializer):
 
     # ID field must be explicitly set as the default id field is read-only.
     id = IntegerField(min_value=0)
+    # Declaring the field explicitly avoids the per-element existence validator
+    # on the model's `roles` ArrayField, which would issue one query per role.
+    # Existence is instead validated in bulk in `validate_roles`.
+    roles = ListField(
+        child=IntegerField(min_value=0),
+        required=False,
+        allow_empty=True,
+    )
 
     class Meta:
         """Metadata defined for the Django REST Framework."""
@@ -709,6 +714,18 @@ class Meta:
         depth = 1
         list_serializer_class = UserListSerializer
 
+    def validate_roles(self, roles: list[int]) -> list[int]:
+        """Validate that all given roles exist, using a single query."""
+        existing_role_ids = set(
+            Role.objects.filter(id__in=roles).values_list('id', flat=True)
+        )
+        unknown_roles = set(roles) - existing_role_ids
+        if unknown_roles:
+            raise ValidationError([
+                f"Role with ID {role_id} does not exist" for role_id in sorted(unknown_roles)
+            ])
+        return roles
+
     def create(self, validated_data: dict) -> User:
         """Override create method to catch IntegrityError."""
         try:
@@ -733,7 +750,7 @@ def get_alts(self, user: User) -> list[dict]:
         """Retrieve the alts with all additional data on them."""
         return [
             UserAltRelationshipSerializer(alt).data
-            for alt in user.alts.through.objects.filter(source=user)
+            for alt in user.useraltrelationship_set.all()
         ]
 
 

pydis_site/apps/api/tests/test_infractions.py

@@ -4,11 +4,13 @@
 from urllib.parse import quote
 
 from django.db import transaction
+from django.db import connection
 from django.db.utils import IntegrityError
+from django.test.utils import CaptureQueriesContext
 from django.urls import reverse
 
 from .base import AuthenticatedAPITestCase
-from pydis_site.apps.api.models import Infraction, User
+from pydis_site.apps.api.models import Infraction, User, UserAltRelationship
 from pydis_site.apps.api.serializers import InfractionSerializer
 
 
@@ -807,6 +809,34 @@ def test_list_expanded(self):
         for infraction in response_data:
             self.check_expanded_fields(infraction)
 
+    def test_list_expanded_query_count_does_not_scale_with_infractions(self):
+        """The expanded list must not run a query per infraction (N+1 regression guard)."""
+        url = reverse('api:bot:infraction-list-expanded')
+
+        # Give the user an alt so the alts serialization path is exercised.
+        alt = User.objects.create(id=6, name='jimmy', discriminator=2)
+        UserAltRelationship.objects.create(
+            source=self.user, target=alt, context='alt account', actor=self.user
+        )
+
+        with CaptureQueriesContext(connection) as ctx:
+            response = self.client.get(url)
+        self.assertEqual(response.status_code, 200)
+        baseline_queries = len(ctx.captured_queries)
+
+        # Add several more infractions for the same user.
+        for _ in range(5):
+            Infraction.objects.create(
+                user_id=self.user.id, actor_id=self.user.id, type='warning', active=False
+            )
+
+        with CaptureQueriesContext(connection) as ctx:
+            response = self.client.get(url)
+        self.assertEqual(response.status_code, 200)
+
+        # Query count must stay constant regardless of the number of infractions.
+        self.assertEqual(len(ctx.captured_queries), baseline_queries)
+
     def test_create_expanded(self):
         url = reverse('api:bot:infraction-list-expanded')
         data = {

pydis_site/apps/api/tests/test_users.py

@@ -120,6 +120,21 @@ def test_returns_400_for_unknown_role_id(self):
 
         response = self.client.post(url, data=data)
         self.assertEqual(response.status_code, 400)
+        self.assertEqual(
+            response.json(),
+            {'roles': ["Role with ID 190810291 does not exist"]}
+        )
+
+    def test_patch_returns_400_for_unknown_role_id(self):
+        url = reverse('api:bot:user-detail', args=(self.user.id,))
+        data = {'roles': [self.role.id, 190810291]}
+
+        response = self.client.patch(url, data=data)
+        self.assertEqual(response.status_code, 400)
+        self.assertEqual(
+            response.json(),
+            {'roles': ["Role with ID 190810291 does not exist"]}
+        )
 
     def test_returns_400_for_bad_data(self):
         url = reverse('api:bot:user-list')
@@ -257,6 +272,17 @@ def test_returns_404_for_not_found_user(self):
         response = self.client.patch(url, data=data)
         self.assertEqual(response.status_code, 404)
 
+    def test_bulk_patch_returns_400_for_unknown_role_id(self):
+        url = reverse("api:bot:user-bulk-patch")
+        data = [
+            {
+                "id": 1,
+                "roles": [self.role_developer.id, 190810291],
+            }
+        ]
+        response = self.client.patch(url, data=data)
+        self.assertEqual(response.status_code, 400)
+
     def test_returns_400_for_bad_data(self):
         url = reverse("api:bot:user-bulk-patch")
         data = [

pydis_site/apps/api/viewsets/bot/infraction.py

@@ -238,7 +238,11 @@ def get_queryset(self) -> QuerySet:
 
         qs = self.queryset.filter(**additional_filters)
         if self.serializer_class is ExpandedInfractionSerializer:
-            return qs.prefetch_related('actor', 'user')
+            return qs.prefetch_related(
+                'actor',
+                'user',
+                'user__useraltrelationship_set__target__useraltrelationship_set',
+            )
 
         return qs