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 5d87ca427..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, @@ -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() ] 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/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 = [ 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