diff --git a/pydis_site/apps/api/serializers.py b/pydis_site/apps/api/serializers.py index 1f1fd41f9..75bf0c46b 100644 --- a/pydis_site/apps/api/serializers.py +++ b/pydis_site/apps/api/serializers.py @@ -111,9 +111,10 @@ class DeletedMessageSerializer(ModelSerializer): model for more information. """ - author = PrimaryKeyRelatedField( - queryset=User.objects.all() - ) + # Plain integers to avoid per-row DB validators (id's UniqueValidator, author's + # FK lookup) that cause N+1 queries. Validation is done in the `MessageDeletionContextSerializer` instead. + id = IntegerField() + author = IntegerField(source='author_id', allow_null=True) deletion_context = PrimaryKeyRelatedField( queryset=MessageDeletionContext.objects.all(), # This will be overridden in the `create` function @@ -146,6 +147,39 @@ class Meta: fields = ('actor', 'creation', 'id', 'deletedmessage_set') depth = 1 + def validate_deletedmessage_set(self, messages: list[dict]) -> list[dict]: + """ + Validate the deleted messages using batched queries to avoid N+1 lookups. + + Rather than letting each nested message trigger its own author and + uniqueness queries, all referenced authors and message IDs are checked + with a single query each. + """ + author_ids = { + message['author_id'] + for message in messages + if message['author_id'] is not None + } + known_author_ids = set( + User.objects.filter(id__in=author_ids).values_list('id', flat=True) + ) + unknown_author_ids = author_ids - known_author_ids + if unknown_author_ids: + raise ValidationError( + f"The following message authors do not exist: {sorted(unknown_author_ids)}" + ) + + message_ids = [message['id'] for message in messages] + existing_ids = set( + DeletedMessage.objects.filter(id__in=message_ids).values_list('id', flat=True) + ) + if existing_ids: + raise ValidationError( + f"The following message IDs already exist: {sorted(existing_ids)}" + ) + + return messages + def create(self, validated_data: dict) -> MessageDeletionContext: """ Return a `MessageDeletionContext` based on the given data. diff --git a/pydis_site/apps/api/tests/test_deleted_messages.py b/pydis_site/apps/api/tests/test_deleted_messages.py index 7ddd96861..a676f0962 100644 --- a/pydis_site/apps/api/tests/test_deleted_messages.py +++ b/pydis_site/apps/api/tests/test_deleted_messages.py @@ -1,5 +1,7 @@ from datetime import UTC, datetime +from django.db import connection +from django.test.utils import CaptureQueriesContext from django.urls import reverse from .base import AuthenticatedAPITestCase @@ -46,6 +48,91 @@ def test_accepts_valid_data(self): self.assertIsNone(context.actor) +class DeletedMessagesQueryCountTests(AuthenticatedAPITestCase): + @classmethod + def setUpTestData(cls): + cls.author = User.objects.create( + id=77, + name='Sportacus', + discriminator=77, + ) + + def _build_data(self, message_count: int) -> dict: + return { + 'actor': None, + 'creation': datetime.now(tz=UTC).isoformat(), + 'deletedmessage_set': [ + { + 'author': self.author.id, + 'id': 1000 + index, + 'channel_id': 5555, + 'content': f"Message {index}", + 'embeds': [], + 'attachments': [] + } + for index in range(message_count) + ] + } + + def test_query_count_is_independent_of_message_count(self): + """The number of queries must not grow with the size of the deletedmessage_set.""" + url = reverse('api:bot:messagedeletioncontext-list') + + with CaptureQueriesContext(connection) as few_ctx: + response = self.client.post(url, data=self._build_data(2)) + self.assertEqual(response.status_code, 201) + + MessageDeletionContext.objects.all().delete() + + with CaptureQueriesContext(connection) as many_ctx: + response = self.client.post(url, data=self._build_data(20)) + self.assertEqual(response.status_code, 201) + + self.assertEqual(len(few_ctx.captured_queries), len(many_ctx.captured_queries)) + + +class DeletedMessagesValidationTests(AuthenticatedAPITestCase): + @classmethod + def setUpTestData(cls): + cls.author = User.objects.create( + id=88, + name='Stephanie', + discriminator=88, + ) + + def _message(self, message_id: int, author_id: int) -> dict: + return { + 'author': author_id, + 'id': message_id, + 'channel_id': 5555, + 'content': "Content", + 'embeds': [], + 'attachments': [] + } + + def test_rejects_unknown_author(self): + url = reverse('api:bot:messagedeletioncontext-list') + data = { + 'actor': None, + 'creation': datetime.now(tz=UTC).isoformat(), + 'deletedmessage_set': [self._message(1, author_id=999999)], + } + response = self.client.post(url, data=data) + self.assertEqual(response.status_code, 400) + + def test_rejects_already_existing_message_id(self): + url = reverse('api:bot:messagedeletioncontext-list') + first = { + 'actor': None, + 'creation': datetime.now(tz=UTC).isoformat(), + 'deletedmessage_set': [self._message(2, author_id=self.author.id)], + } + self.assertEqual(self.client.post(url, data=first).status_code, 201) + + response = self.client.post(url, data=first) + self.assertEqual(response.status_code, 400) + + class DeletedMessagesWithActorTests(AuthenticatedAPITestCase): @classmethod def setUpTestData(cls):