From 66f6d97dca8950645ce37ee6e9da92e3aa75d87d Mon Sep 17 00:00:00 2001 From: ChrisLovering Date: Mon, 6 Jul 2026 16:38:53 -0400 Subject: [PATCH 1/3] Fix expanded infractions n+1 query --- pydis_site/apps/api/serializers.py | 10 ++---- pydis_site/apps/api/tests/test_infractions.py | 32 ++++++++++++++++++- .../apps/api/viewsets/bot/infraction.py | 6 +++- 3 files changed, 39 insertions(+), 9 deletions(-) diff --git a/pydis_site/apps/api/serializers.py b/pydis_site/apps/api/serializers.py index 5d87ca427..d2a24be0f 100644 --- a/pydis_site/apps/api/serializers.py +++ b/pydis_site/apps/api/serializers.py @@ -684,12 +684,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 @@ -733,7 +729,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() ] diff --git a/pydis_site/apps/api/tests/test_infractions.py b/pydis_site/apps/api/tests/test_infractions.py index ada2cd332..fc8369a3d 100644 --- a/pydis_site/apps/api/tests/test_infractions.py +++ b/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 = { diff --git a/pydis_site/apps/api/viewsets/bot/infraction.py b/pydis_site/apps/api/viewsets/bot/infraction.py index 1d16232c9..72a368ee5 100644 --- a/pydis_site/apps/api/viewsets/bot/infraction.py +++ b/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 From 978e02a489afd4fcca080fd359cdeee45e914688 Mon Sep 17 00:00:00 2001 From: ChrisLovering Date: Mon, 6 Jul 2026 16:57:35 -0400 Subject: [PATCH 2/3] Fix n+1 query in users Since the role validator was in the model, a query to the roles table had to be ran for each row returned by the user query. Moving this validation to the serialiser allowed all roles to be validated in the same query as the fetch. Since the validation is done at the serialiser level, this removed the need for the validators to be in the model at all. --- .../0054_user_invalidate_unknown_role.py | 3 +-- .../migrations/0056_allow_blank_user_roles.py | 3 +-- pydis_site/apps/api/models/bot/user.py | 10 --------- pydis_site/apps/api/serializers.py | 21 +++++++++++++++++++ 4 files changed, 23 insertions(+), 14 deletions(-) diff --git a/pydis_site/apps/api/migrations/0054_user_invalidate_unknown_role.py b/pydis_site/apps/api/migrations/0054_user_invalidate_unknown_role.py index 962300154..f1779e812 100644 --- a/pydis_site/apps/api/migrations/0054_user_invalidate_unknown_role.py +++ b/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), ), ] diff --git a/pydis_site/apps/api/migrations/0056_allow_blank_user_roles.py b/pydis_site/apps/api/migrations/0056_allow_blank_user_roles.py index 489941c7b..aac2be14f 100644 --- a/pydis_site/apps/api/migrations/0056_allow_blank_user_roles.py +++ b/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), ), ] diff --git a/pydis_site/apps/api/models/bot/user.py b/pydis_site/apps/api/models/bot/user.py index 7f49fa86a..fc4be2097 100644 --- a/pydis_site/apps/api/models/bot/user.py +++ b/pydis_site/apps/api/models/bot/user.py @@ -1,5 +1,4 @@ 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 @@ -7,14 +6,6 @@ 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, diff --git a/pydis_site/apps/api/serializers.py b/pydis_site/apps/api/serializers.py index d2a24be0f..1f1fd41f9 100644 --- a/pydis_site/apps/api/serializers.py +++ b/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, @@ -696,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.""" @@ -705,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: From f7b8d0c4f191d24658c9f2fece3c4eaa17889d09 Mon Sep 17 00:00:00 2001 From: ChrisLovering Date: Tue, 7 Jul 2026 08:50:52 -0400 Subject: [PATCH 3/3] Further test coverage for users endpoint --- pydis_site/apps/api/tests/test_users.py | 26 +++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/pydis_site/apps/api/tests/test_users.py b/pydis_site/apps/api/tests/test_users.py index 99673e537..3354724c4 100644 --- a/pydis_site/apps/api/tests/test_users.py +++ b/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 = [