From f0969cf7cff8f24b3ce22265c6024f104185fb0d Mon Sep 17 00:00:00 2001 From: Jon Froehlich Date: Thu, 18 Jun 2026 19:31:25 -0700 Subject: [PATCH] perf(admin): prefetch the artifact-family changelists (#1346, Phase 3) The Publication/Talk/Poster/Video/Grant/Award list pages each rendered M2M/FK callable columns (author & speaker lists, projects, sponsor, recipients) with no prefetch, firing 1-3 queries per row. - get_queryset() now prefetch_related()s the relations each changelist renders: Publication (authors, projects), Talk (authors), Poster (authors), Video (projects), Grant (authors), Award (recipients, projects). Grant's sponsor FK uses list_select_related. - Artifact.get_first_author_last_name rewritten to read list(self.authors.all()) instead of .exists()/.first() (which bypass a prefetch cache); SortedManyToManyField preserves order so [0] is still the first author. PublicationAdmin.display_authors likewise reads the cached list instead of a [:5] queryset slice + .count(). Note: Django has no `list_prefetch_related` ModelAdmin option (only list_select_related), so prefetching must go through get_queryset(). Tests (test_admin_perf_artifacts.py): correctness of the two rewritten methods, plus a steady-state guard that each of the five changelists stays flat as rows grow 3 -> 6. Full suite green (401). Co-Authored-By: Claude Opus 4.8 (1M context) --- website/admin/award_admin.py | 5 + website/admin/grant_admin.py | 8 ++ website/admin/poster_admin.py | 6 ++ website/admin/publication_admin.py | 18 +++- website/admin/talk_admin.py | 4 + website/admin/video_admin.py | 4 + website/models/artifact.py | 16 ++- website/tests/test_admin_perf_artifacts.py | 113 +++++++++++++++++++++ 8 files changed, 164 insertions(+), 10 deletions(-) create mode 100644 website/tests/test_admin_perf_artifacts.py diff --git a/website/admin/award_admin.py b/website/admin/award_admin.py index 162c87c4..1de1fc4d 100644 --- a/website/admin/award_admin.py +++ b/website/admin/award_admin.py @@ -46,6 +46,11 @@ class AwardAdmin(admin.ModelAdmin): date_hierarchy = 'date' # Year/month/day drill-down (awards are browsed by year) + # Prefetch the M2M relations get_recipient_names / get_project_names walk, so + # they don't fire two queries per award on the changelist (#1346). + def get_queryset(self, request): + return super().get_queryset(request).prefetch_related('recipients', 'projects') + def get_fieldsets(self, request, obj=None): # Built at request time so reverse() can resolve the Publications admin URL. publications_url = reverse('admin:website_publication_changelist') diff --git a/website/admin/grant_admin.py b/website/admin/grant_admin.py index 0b249516..7d6f8f93 100644 --- a/website/admin/grant_admin.py +++ b/website/admin/grant_admin.py @@ -26,6 +26,14 @@ class GrantAdmin(ArtifactAdmin): ordering = ('-date',) # sort by date, most recent first date_hierarchy = 'date' # Year/month/day drill-down by grant start date + # sponsor is a FK column -> list_select_related (Django applies it to the + # changelist query); the get_first_author_last_name column walks authors -> + # prefetch in get_queryset. Together these drop the per-row queries (#1346). + list_select_related = ('sponsor',) + + def get_queryset(self, request): + return super().get_queryset(request).prefetch_related('authors') + fieldsets = [ (None, {'fields': ['title', 'authors']}), ('Grant Info', {'fields': ['date', 'end_date', 'sponsor', 'funding_amount', 'forum_url', 'grant_id']}), diff --git a/website/admin/poster_admin.py b/website/admin/poster_admin.py index fb88cf28..a58f02de 100644 --- a/website/admin/poster_admin.py +++ b/website/admin/poster_admin.py @@ -20,6 +20,12 @@ class PosterAdmin(ArtifactAdmin): ordering = ('-date',) # Poster had no default sort; newest first like its siblings date_hierarchy = 'date' # Year/month/day drill-down + # Prefetch authors so the inherited get_first_author_last_name column resolves + # from cache instead of querying authors per row (#1346). (Artifact.get_first_ + # author_last_name reads list(self.authors.all()) so it uses this prefetch.) + def get_queryset(self, request): + return super().get_queryset(request).prefetch_related('authors') + def get_changeform_initial_data(self, request): """ Pre-fills the poster form with data from a related publication. diff --git a/website/admin/publication_admin.py b/website/admin/publication_admin.py index 69c00508..e08491c9 100644 --- a/website/admin/publication_admin.py +++ b/website/admin/publication_admin.py @@ -56,7 +56,12 @@ class Media: list_filter = (PubVenueTypeListFilter, PubVenueListFilter) - # add in auto-complete fields + # Prefetch the M2M relations the changelist renders per row (display_authors, + # display_projects) so they don't fire 2-3 queries each per publication (#1346). + def get_queryset(self, request): + return super().get_queryset(request).prefetch_related('authors', 'projects') + + # add in auto-complete fields # this addresses: https://github.com/jonfroehlich/makeabilitylabwebsite/issues/553 # # autocomplete_fields is a list of ForeignKey and/or ManyToManyField fields you would like @@ -77,10 +82,13 @@ def display_projects(self, obj): display_projects.short_description = 'Projects' def display_authors(self, obj): - authors = [author.get_full_name() for author in obj.authors.all()[:5]] - if obj.authors.count() > 5: - authors.append("...") - return ", ".join(authors) if authors else 'No authors' + # list(...) reuses the list_prefetch_related('authors') cache; slicing the + # queryset ([:5]) or calling .count() would each re-query per row instead. + authors = list(obj.authors.all()) + names = [author.get_full_name() for author in authors[:5]] + if len(authors) > 5: + names.append("...") + return ", ".join(names) if names else 'No authors' display_authors.short_description = 'Authors (First 5)' def get_display_thumbnail(self, obj): diff --git a/website/admin/talk_admin.py b/website/admin/talk_admin.py index fb774842..a3d14fbc 100644 --- a/website/admin/talk_admin.py +++ b/website/admin/talk_admin.py @@ -43,6 +43,10 @@ class TalkAdmin(ArtifactAdmin): ordering = ('-date',) # Sort talks by date in descending order list_filter = ('talk_type',) # Add a filter for the talk type + + # Prefetch speakers so get_speakers_as_csv doesn't query authors per row (#1346). + def get_queryset(self, request): + return super().get_queryset(request).prefetch_related('authors') date_hierarchy = 'date' # Year/month/day drill-down at the top of the list def get_changeform_initial_data(self, request): diff --git a/website/admin/video_admin.py b/website/admin/video_admin.py index c93d6eb0..baeaf59c 100644 --- a/website/admin/video_admin.py +++ b/website/admin/video_admin.py @@ -23,6 +23,10 @@ class VideoAdmin(admin.ModelAdmin): ordering = ('-date',) date_hierarchy = 'date' # Year/month/day drill-down + # Prefetch projects so display_projects doesn't query them per row (#1346). + def get_queryset(self, request): + return super().get_queryset(request).prefetch_related('projects') + def display_projects(self, obj): return ", ".join([project.name for project in obj.projects.all()]) diff --git a/website/models/artifact.py b/website/models/artifact.py index 34f2ed4e..1a2f9181 100644 --- a/website/models/artifact.py +++ b/website/models/artifact.py @@ -63,14 +63,20 @@ def get_upload_thumbnail_dir(self, filename): def get_first_author_last_name(self): """ Returns the last name of the first author of the artifact. - + If the artifact has an ID and at least one author, it returns the last name of the first author. Otherwise, it returns "Unknown". + + Reads ``list(self.authors.all())`` rather than ``.exists()`` + ``.first()`` so an + admin changelist with ``list_prefetch_related('authors')`` resolves it from the + prefetch cache (no per-row query). ``authors`` is a SortedManyToManyField, so + ``.all()`` preserves the editor-defined order — ``[0]`` is still the first author. """ - if self.id and self.authors.exists(): - return self.authors.first().last_name - else: - return "Unknown" + if self.id: + authors = list(self.authors.all()) + if authors: + return authors[0].last_name + return "Unknown" get_first_author_last_name.short_description = 'First Author (Last Name)' # used in the admin display for column header diff --git a/website/tests/test_admin_perf_artifacts.py b/website/tests/test_admin_perf_artifacts.py new file mode 100644 index 00000000..c1e76f8a --- /dev/null +++ b/website/tests/test_admin_perf_artifacts.py @@ -0,0 +1,113 @@ +""" +Phase 3 of the admin changelist audit (#1346): prefetch the artifact-family +changelists so their per-row callable columns (author/speaker lists, sponsor, +recipients, projects) stop issuing per-row queries. + +Coverage mirrors Phase 2: + 1. Correctness — the rewritten ``Artifact.get_first_author_last_name`` (now + reads ``list(self.authors.all())`` so a prefetch resolves it) and the + rewritten ``PublicationAdmin.display_authors`` are behavior-preserving. + 2. Regression — each changelist's steady-state query count stays flat as rows + grow (3 -> 6). Before this phase, columns like ``display_authors`` (a + ``[:5]`` slice + ``.count()``) or ``get_speakers_as_csv`` fired 1-3 queries + per row. +""" + +from website.models import Publication, Poster +from website.admin.admin_site import ml_admin_site +from website.admin.publication_admin import PublicationAdmin +from website.admin.talk_admin import TalkAdmin +from website.admin.poster_admin import PosterAdmin +from website.admin.video_admin import VideoAdmin +from website.admin.award_admin import AwardAdmin +from website.tests.factories import (PersonFactory, ProjectFactory, + PublicationFactory, TalkFactory, + PosterFactory, VideoFactory, AwardFactory) +from website.tests.test_admin_perf import _AdminPerfBase +from website.tests.base import DatabaseTestCase + + +class ArtifactFirstAuthorTests(DatabaseTestCase): + """get_first_author_last_name (rewritten to use list(self.authors.all())).""" + + def test_returns_first_author_in_sorted_order(self): + first = PersonFactory(last_name="Aaa") + second = PersonFactory(last_name="Bbb") + poster = PosterFactory(authors=[first, second]) + self.assertEqual(poster.get_first_author_last_name(), "Aaa") + + def test_unknown_when_no_authors(self): + poster = PosterFactory() + self.assertEqual(poster.get_first_author_last_name(), "Unknown") + + +class PublicationDisplayAuthorsTests(DatabaseTestCase): + """PublicationAdmin.display_authors caps at five names and appends an ellipsis.""" + + def setUp(self): + self.admin = PublicationAdmin(Publication, ml_admin_site) + + def test_six_authors_shows_five_plus_ellipsis(self): + people = [PersonFactory(first_name=f"A{i}", last_name=f"L{i}") for i in range(6)] + pub = PublicationFactory(authors=people) + rendered = self.admin.display_authors(pub) + self.assertIn("...", rendered) + self.assertEqual(rendered.count(","), 5) # 5 names + "..." => 5 commas + + def test_no_authors(self): + pub = PublicationFactory() + self.assertEqual(self.admin.display_authors(pub), "No authors") + + +class ArtifactChangelistPerfTests(_AdminPerfBase): + """Each artifact changelist's query count stays flat as rows grow (3 -> 6).""" + + def _assert_flat(self, model_admin, params, make_fn): + make_fn(0, 3) + n1 = self._steady_state_query_count(model_admin, params) + make_fn(3, 6) + n2 = self._steady_state_query_count(model_admin, params) + self.assertEqual( + n1, n2, + f"{type(model_admin).__name__} N+1: {n1} -> {n2} queries as rows grew 3 -> 6") + + def test_publications_flat(self): + def make(start, end): + for i in range(start, end): + pub = PublicationFactory( + title=f"Pub {i}", + authors=[PersonFactory(), PersonFactory()]) + pub.projects.set([ProjectFactory()]) + self._assert_flat(PublicationAdmin(Publication, ml_admin_site), {}, make) + + def test_talks_flat(self): + from website.models import Talk + def make(start, end): + for i in range(start, end): + TalkFactory(title=f"Talk {i}", + authors=[PersonFactory(), PersonFactory()]) + self._assert_flat(TalkAdmin(Talk, ml_admin_site), {}, make) + + def test_posters_flat(self): + def make(start, end): + for i in range(start, end): + PosterFactory(title=f"Poster {i}", + authors=[PersonFactory(), PersonFactory()]) + self._assert_flat(PosterAdmin(Poster, ml_admin_site), {}, make) + + def test_videos_flat(self): + from website.models import Video + def make(start, end): + for i in range(start, end): + video = VideoFactory(title=f"Video {i}") + video.projects.set([ProjectFactory(), ProjectFactory()]) + self._assert_flat(VideoAdmin(Video, ml_admin_site), {}, make) + + def test_awards_flat(self): + from website.models import Award + def make(start, end): + for i in range(start, end): + award = AwardFactory(title=f"Award {i}", + recipients=[PersonFactory(), PersonFactory()]) + award.projects.set([ProjectFactory()]) + self._assert_flat(AwardAdmin(Award, ml_admin_site), {}, make)