Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 37 additions & 3 deletions pydis_site/apps/api/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down
87 changes: 87 additions & 0 deletions pydis_site/apps/api/tests/test_deleted_messages.py
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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):
Expand Down