diff --git a/src/bpp/admin/autor.py b/src/bpp/admin/autor.py index 29aeb6f2f..4ead588d4 100644 --- a/src/bpp/admin/autor.py +++ b/src/bpp/admin/autor.py @@ -28,6 +28,7 @@ OrcidObecnyFilter, PBN_UID_IDObecnyFilter, PBNIDObecnyFilter, + WydzialAutoraFilter, ) from .helpers.fieldsets import ADNOTACJE_FIELDSET, ZapiszZAdnotacjaMixin from .helpers.site_filtered import SiteFilteredAdminMixin @@ -335,7 +336,7 @@ def has_delete_permission(self, request, obj=None): ] list_filter = [ JednostkaFilter, - "aktualna_jednostka__wydzial", + WydzialAutoraFilter, "tytul", PBNIDObecnyFilter, OrcidObecnyFilter, diff --git a/src/bpp/admin/core.py b/src/bpp/admin/core.py index 3ceef8e41..0e1fb363d 100644 --- a/src/bpp/admin/core.py +++ b/src/bpp/admin/core.py @@ -8,6 +8,7 @@ from django.conf import settings from django.contrib import admin from django.core.cache import cache +from django.db.models import FETCH_PEERS from django.db.models.fields import BLANK_CHOICE_DASH from django.forms import NullBooleanField from django.forms.widgets import HiddenInput @@ -117,6 +118,41 @@ class BaseBppAdminMixin(DynamicAdminFilterMixin): # ograniczenie wielkosci listy list_per_page = 50 + def get_queryset(self, request): + """Włącz ``FETCH_PEERS`` (Django 6.1) dla querysetów tego admina. + + Changelisty admina to najgęstsze w BPP skupisko N+1: ``list_display`` + i ``__str__`` modeli sięgają po FK, których nikt nie zadeklarował + w ``list_select_related``, a każde takie dotknięcie to osobny SELECT + per wiersz. ``FETCH_PEERS`` sprawia, że PIERWSZE leniwe dotknięcie + relacji (albo pola odroczonego) dociąga ją HURTEM dla całego + rodzeństwa z tego samego pobrania — N+1 zamienia się w 2 zapytania, + bez zgadywania z góry, które FK dotknie szablon. + + Zmierzone na kopii bazy produkcyjnej (z produkcyjnymi regułami + ``CACHEOPS``) — zapytania i mediana czasu na request: + + * changelist jednostek 66 → 17 zapytań, 185 → 120 ms + * changelist wyd. zwartych 220 → 40 zapytań, 227 → 184 ms + * changelist wyd. ciągłych 190 → 38 zapytań, 221 → 190 ms + + Dlaczego TU, a nie globalnie (podstawienie ``DEFAULT_FETCH_MODE``): + ``track_peers`` trzyma ``weakref`` do każdej instancji z pobrania, + więc koszt ponosiłby KAŻDY queryset w aplikacji, a zysk jest + skoncentrowany w adminie. Samo ``get_queryset`` wystarcza, bo + ``QuerySet._clone()`` przenosi ``_fetch_mode`` — tryb przeżywa + filtry, sortowanie i slicing dokładane przez dalsze mixiny + i przez sam ``ChangeList``. + + Semantyka się NIE zmienia: ``fetch_one`` i ``fetch_many`` idą tą samą + ścieżką managera (``_base_manager``), więc tryb nie zaczyna nagle + odfiltrowywać rekordów — istotne przy soft-delete. + + WYMAGA Django >= 6.1 (``QuerySet.fetch_mode``) — dlatego ta zmiana + celuje w gałąź ``django-6.1``, a nie w ``dev``. + """ + return super().get_queryset(request).fetch_mode(FETCH_PEERS) + def save_related(self, request, form, formsets, change): """ Przebuduj cache punktacji PO zapisaniu wszystkich inlines (autorzy/dyscypliny). diff --git a/src/bpp/admin/filters.py b/src/bpp/admin/filters.py index 058845a5b..4146e0b0c 100644 --- a/src/bpp/admin/filters.py +++ b/src/bpp/admin/filters.py @@ -326,6 +326,40 @@ def queryset(self, request, queryset): return queryset +class WydzialAutoraFilter(WydzialFilter): + """``WydzialFilter`` dla changelisty ``AutorAdmin`` (#438, domknięcie). + + Ten sam problem co w ``JednostkaAdmin``, tylko o jeden hop dalej: goły + ``list_filter = ("aktualna_jednostka__wydzial", ...)`` kazał Django + zbudować ``RelatedFieldListFilter``, a ten woła ``field.get_choices()``. + Skoro po Fazie B denorm ``Jednostka.wydzial`` jest self-FK na + ``Jednostka``, ``get_choices()`` enumerowało CAŁĄ tabelę jednostek -- + produkcyjnie 504 pozycje zamiast 7 wydziałów, plus 504 zapytania na + request (``Jednostka.__str__`` czyta ``self.uczelnia``). + + Z klasy bazowej dziedziczymy BEZ zmian ``lookups`` (tylko korzenie, + zawężone do uczelni z requestu) i ``has_output`` (bramka + ``uzywaj_wydzialow``) -- lista „wydziałów" jest ta sama niezależnie od + tego, co filtrujemy. Nadpisujemy wyłącznie ``queryset``, bo tu + zawężamy ``Autor``, a nie ``Jednostka``: do korzenia trzeba dojść przez + ``aktualna_jednostka__``. + """ + + def queryset(self, request, queryset): + v = self.value() + if v: + # Odpowiednik `Q(wydzial_id=v) | Q(pk=v)` z klasy bazowej, + # przetraversowany o jeden FK dalej: autorzy z jednostek + # niosących denorm ``wydzial=korzeń`` PLUS autorzy przypisani + # wprost do samego korzenia (korzeń nie wskazuje na siebie). + # Oba warunki to forward-FK (bez multi-valued join), więc unia + # nie duplikuje wierszy i nie potrzebuje ``distinct()``. + return queryset.filter( + Q(aktualna_jednostka__wydzial_id=v) | Q(aktualna_jednostka_id=v) + ) + return queryset + + class JednostkaFilter(SimpleListFilter): title = "Jednostka" parameter_name = "jednostka" @@ -337,8 +371,18 @@ def queryset(self, request, queryset): return queryset def lookups(self, request, model_admin): + # ``uczelnia`` w select_related, NIE tylko ``wydzial``: + # ``Jednostka.__str__`` czyta OBA FK — ``self.uczelnia`` (bramka + # ``uzywaj_wydzialow``) i ``self.wydzial`` (skrót w nawiasie). Bez + # ``uczelnia`` każda opcja listy kosztowała osobny SELECT: na bazie + # produkcyjnej 504 jednostki = 504 zapytania na KAŻDE wejście na + # changelistę autorów. Cacheops zamieniał je na trafienia w Redis + # (``bpp.uczelnia`` jest w regułach), więc licznik zapytań SQL tego + # nie pokazywał — ale 504 round-tripy do Redisa nadal kosztowały + # ~200 ms na request. return ( - (x.pk, str(x)) for x in Jednostka.objects.all().select_related("wydzial") + (x.pk, str(x)) + for x in Jednostka.objects.all().select_related("wydzial", "uczelnia") ) @@ -358,11 +402,22 @@ def logentries(self): ) def lookups(self, request, model_admin): + # ``only()`` MUSI wymieniać wszystkie pola, które czyta + # ``BppUser.__str__`` (``last_name``, ``first_name`` — patrz + # ``bpp/models/profile.py``), inaczej „optymalizacja" kosztuje + # zamiast oszczędzać: każde pole odroczone to osobny + # ``refresh_from_db()`` per użytkownik, czyli DWA dodatkowe SELECT-y + # na wiersz. Na bazie produkcyjnej to było 156 zapytań na wejście + # na changelistę wydawnictw ciągłych (78 użytkowników × 2 pola) — + # i w przeciwieństwie do słowników ``bpp.bppuser`` NIE jest + # cache'owany przez cacheops, więc szły wprost do PostgreSQL. + # + # Jeśli dokładasz pole do ``__str__``, dołóż je też tutaj. return ( (x.pk, str(x)) for x in BppUser.objects.filter( pk__in=self.logentries().values_list("user_id") - ).only("pk", "username") + ).only("pk", "username", "last_name", "first_name") ) diff --git a/src/bpp/newsfragments/autor-filtr-wydzial.bugfix.rst b/src/bpp/newsfragments/autor-filtr-wydzial.bugfix.rst new file mode 100644 index 000000000..aafdf2dbc --- /dev/null +++ b/src/bpp/newsfragments/autor-filtr-wydzial.bugfix.rst @@ -0,0 +1,4 @@ +Filtr „Wydział" na liście autorów w panelu admina listował wszystkie jednostki +(na produkcji 504 pozycje) zamiast samych wydziałów — teraz pokazuje wyłącznie +jednostki-korzenie i filtruje po całym poddrzewie wydziału, co dodatkowo +usuwa setki zbędnych zapytań przy każdym wyświetleniu listy. diff --git a/src/bpp/newsfragments/fetch-peers-admin.feature.rst b/src/bpp/newsfragments/fetch-peers-admin.feature.rst new file mode 100644 index 000000000..9c049106d --- /dev/null +++ b/src/bpp/newsfragments/fetch-peers-admin.feature.rst @@ -0,0 +1,5 @@ +Listy w panelu administracyjnym korzystają z nowego trybu pobierania relacji +z Django 6.1 (``FETCH_PEERS``): powiązane obiekty potrzebne do wyświetlenia +wiersza dociągane są hurtem dla całej strony, a nie osobno dla każdego +wiersza. Na dużej bazie skraca to czas otwarcia list o kilkanaście do +kilkudziesięciu procent, zależnie od listy. diff --git a/src/bpp/newsfragments/wydajnosc-filtry-admina.bugfix.rst b/src/bpp/newsfragments/wydajnosc-filtry-admina.bugfix.rst new file mode 100644 index 000000000..cb34fdfc4 --- /dev/null +++ b/src/bpp/newsfragments/wydajnosc-filtry-admina.bugfix.rst @@ -0,0 +1,6 @@ +Przyspieszono listy rozwijane filtrów w panelu administracyjnym. Filtr +„Jednostka" dociągał uczelnię osobnym zapytaniem dla każdej pozycji listy +(na dużej bazie ponad 500 zapytań na każde wejście na listę autorów), +a filtry „Utworzone przez" i „Ostatnio zmienione przez" pobierały imię +i nazwisko użytkownika dwoma dodatkowymi zapytaniami na pozycję. Teraz +każda z tych list powstaje jednym zapytaniem. diff --git a/src/bpp/newsfragments/wydajnosc-indeks-jednostek.bugfix.rst b/src/bpp/newsfragments/wydajnosc-indeks-jednostek.bugfix.rst new file mode 100644 index 000000000..0614d0939 --- /dev/null +++ b/src/bpp/newsfragments/wydajnosc-indeks-jednostek.bugfix.rst @@ -0,0 +1,4 @@ +Przyspieszono indeks jednostek w części publicznej. Liczba autorów przy +każdej jednostce była liczona osobnym zapytaniem, i to kilkukrotnie na +wiersz — na dużej bazie dawało to ponad 500 zapytań na jedno wyświetlenie +strony. Teraz liczby przychodzą jednym zapytaniem razem z listą jednostek. diff --git a/src/bpp/templates/browse/jednostki.html b/src/bpp/templates/browse/jednostki.html index fa0de4575..1ea415df4 100644 --- a/src/bpp/templates/browse/jednostki.html +++ b/src/bpp/templates/browse/jednostki.html @@ -125,10 +125,10 @@

{% endif %} - {% if item.aktualna_jednostka.count > 0 %} + {% if item.liczba_autorow > 0 %}
- {{ item.aktualna_jednostka.count }} {% if item.aktualna_jednostka.count == 1 %}autor{% elif item.aktualna_jednostka.count < 5 %}autorów{% else %}autorów{% endif %} + {{ item.liczba_autorow }} {% if item.liczba_autorow == 1 %}autor{% elif item.liczba_autorow < 5 %}autorów{% else %}autorów{% endif %}
{% endif %} diff --git a/src/bpp/tests/test_admin/test_autor_wydzial_filter.py b/src/bpp/tests/test_admin/test_autor_wydzial_filter.py new file mode 100644 index 000000000..1fa69c566 --- /dev/null +++ b/src/bpp/tests/test_admin/test_autor_wydzial_filter.py @@ -0,0 +1,146 @@ +"""Testy filtra „Wydział" na changeliście ``AutorAdmin`` (#438, domknięcie). + +Faza B (#438) zamieniła goły ``list_filter = ("wydzial", ...)`` na +``WydzialFilter`` w ``JednostkaAdmin``, ale ``AutorAdmin`` został z gołym +stringiem ``"aktualna_jednostka__wydzial"``. Ponieważ denorm +``Jednostka.wydzial`` jest self-FK na ``Jednostka``, ``RelatedFieldListFilter`` +enumerował CAŁĄ tabelę jednostek (produkcyjnie: 504 pozycje zamiast +7 wydziałów). +""" + +import pytest +from django.urls import reverse +from model_bakery import baker + +from bpp.admin.filters import WydzialAutoraFilter +from bpp.models import Autor, Jednostka, Uczelnia + + +def _spec_wydzial(response): + """Zwraca FilterSpec o tytule „Wydział" z wyrenderowanej changelisty.""" + for spec in response.context["cl"].filter_specs: + if str(spec.title) == "Wydział": + return spec + return None + + +@pytest.mark.django_db +def test_AutorAdmin_WydzialFilter_opcje_tylko_korzenie( + admin_client, + uczelnia: Uczelnia, + jednostka: Jednostka, + jednostka_podrzedna: Jednostka, + druga_jednostka: Jednostka, +): + # ISTOTA BUGA: opcji filtra ma być tyle, ile jednostek-korzeni („wydziałów"), + # a nie tyle, ile jednostek w bazie. Tu drzewo to 1 korzeń (fixture + # `wydzial`) + 3 węzły niżej — więc dokładnie 1 opcja, nie 4. + uczelnia.uzywaj_wydzialow = True + uczelnia.save() + + response = admin_client.get(reverse("admin:bpp_autor_changelist")) + assert response.status_code == 200 + + spec = _spec_wydzial(response) + assert spec is not None, "changelist nie renderuje filtra po wydziale" + + korzenie = Jednostka.objects.filter(parent__isnull=True, widoczna=True) + lookup_pks = {pk for pk, _ in spec.lookup_choices} + assert lookup_pks == {j.pk for j in korzenie} + assert len(lookup_pks) == korzenie.count() + # Węzły spod korzenia NIE mogą trafiać na listę wyboru „wydziału". + assert jednostka.pk not in lookup_pks + assert jednostka_podrzedna.pk not in lookup_pks + assert druga_jednostka.pk not in lookup_pks + + +@pytest.mark.django_db +def test_AutorAdmin_WydzialFilter_filtruje_poddrzewo( + admin_client, + jednostka: Jednostka, + jednostka_podrzedna: Jednostka, + druga_jednostka: Jednostka, +): + # Wybór korzenia zawęża do autorów z CAŁEGO poddrzewa: bezpośrednich + # dzieci, wnuków ORAZ przypisanych wprost do korzenia (ten ostatni + # przypadek obsługuje `| Q(aktualna_jednostka_id=v)`). + root = jednostka.parent + obcy_root = baker.make(Jednostka, parent=None, uczelnia=jednostka.uczelnia) + obca = baker.make(Jednostka, parent=obcy_root, uczelnia=jednostka.uczelnia) + + a_dziecko = baker.make(Autor, aktualna_jednostka=jednostka) + a_wnuk = baker.make(Autor, aktualna_jednostka=jednostka_podrzedna) + a_drugie_dziecko = baker.make(Autor, aktualna_jednostka=druga_jednostka) + a_korzen = baker.make(Autor, aktualna_jednostka=root) + a_obcy = baker.make(Autor, aktualna_jednostka=obca) + a_bez_jednostki = baker.make(Autor, aktualna_jednostka=None) + + response = admin_client.get( + reverse("admin:bpp_autor_changelist"), {"wydzial": root.pk} + ) + assert response.status_code == 200 + + result_list = list(response.context["cl"].result_list) + assert a_dziecko in result_list + assert a_wnuk in result_list + assert a_drugie_dziecko in result_list + assert a_korzen in result_list + assert a_obcy not in result_list + assert a_bez_jednostki not in result_list + + +@pytest.mark.django_db +def test_AutorAdmin_WydzialFilter_ukryty_gdy_uczelnia_bez_wydzialow( + admin_client, uczelnia: Uczelnia, jednostka: Jednostka +): + # Bramka `uzywaj_wydzialow` (dziedziczona z WydzialFilter): instytucja + # 1-progowa nie ma czego filtrować po wydziale. + uczelnia.uzywaj_wydzialow = False + uczelnia.save() + + response = admin_client.get(reverse("admin:bpp_autor_changelist")) + assert response.status_code == 200 + assert not any( + isinstance(spec, WydzialAutoraFilter) + for spec in response.context["cl"].filter_specs + ) + + +@pytest.mark.django_db +def test_AutorAdmin_WydzialFilter_widoczny_gdy_uczelnia_z_wydzialami( + admin_client, uczelnia: Uczelnia, jednostka: Jednostka +): + uczelnia.uzywaj_wydzialow = True + uczelnia.save() + + response = admin_client.get(reverse("admin:bpp_autor_changelist")) + assert response.status_code == 200 + assert any( + isinstance(spec, WydzialAutoraFilter) + for spec in response.context["cl"].filter_specs + ) + + +@pytest.mark.django_db +def test_AutorAdmin_stary_querystring_wydzialu_daje_400( + admin_client, jednostka: Jednostka +): + # Świadoma zmiana kontraktu URL: filtr zmienił parametr z + # `?aktualna_jednostka__wydzial__id__exact=` na `?wydzial=`. + # Filtry admina to ulotny stan UI (a nie trwałe linki), więc zerwanie + # starych URL-i akceptujemy — ale musi degradować się przewidywalnie, + # bez 500. + # + # ZMIERZONE zachowanie (nie założone): skoro pola nie ma już w + # `list_filter`, `ChangeList.get_filters` odrzuca lookup jako + # `DisallowedModelAdminLookup`. To podklasa `SuspiciousOperation`, więc + # handler wyjątków Django zamienia ją na **400 Bad Request** i loguje w + # kanale `django.security` — NIE jest to 500 ani redirect na `?e=1` + # (`?e=1` dostajemy tylko dla `IncorrectLookupParameters`, czyli dla + # DOZWOLONEGO pola z niepoprawną wartością). + root = jednostka.parent + response = admin_client.get( + reverse("admin:bpp_autor_changelist"), + {"aktualna_jednostka__wydzial__id__exact": root.pk}, + ) + assert response.status_code == 400 diff --git a/src/bpp/tests/test_admin/test_fetch_peers.py b/src/bpp/tests/test_admin/test_fetch_peers.py new file mode 100644 index 000000000..d5b6a1a45 --- /dev/null +++ b/src/bpp/tests/test_admin/test_fetch_peers.py @@ -0,0 +1,61 @@ +"""Kontrakt: adminy BPP pobierają dane w trybie ``FETCH_PEERS`` (Django 6.1). + +``BaseBppAdminMixin.get_queryset`` włącza ``FETCH_PEERS``, żeby pierwsze +leniwe dotknięcie relacji (albo pola odroczonego) dociągało ją hurtem dla +całego rodzeństwa z tego samego pobrania. To zamienia N+1 na changelistach +admina na 2 zapytania — bez zgadywania z góry, które FK dotknie szablon. + +Testy tutaj pilnują dwóch rzeczy, które łatwo zepsuć niechcący: + +1. tryb jest w ogóle ustawiany (ktoś mógłby nadpisać ``get_queryset`` + w podklasie i zapomnieć o ``super()``), +2. tryb PRZEŻYWA łańcuch ``.filter()/.order_by()/[slice]`` — to jest cała + przesłanka, dla której wystarcza jedno wywołanie w ``get_queryset``, + a nie łatanie każdego miejsca osobno. +""" + +import pytest +from django.contrib import admin as dj_admin +from django.db.models import FETCH_PEERS + +from bpp.admin.autor import AutorAdmin +from bpp.admin.jednostka import JednostkaAdmin +from bpp.admin.wydawnictwo_ciagle import Wydawnictwo_CiagleAdmin +from bpp.models import Autor, Jednostka, Wydawnictwo_Ciagle + + +def _queryset_admina(klasa_admina, model, rf, admin_user): + request = rf.get("/admin/") + request.user = admin_user + return klasa_admina(model, dj_admin.site).get_queryset(request) + + +@pytest.mark.django_db +@pytest.mark.parametrize( + "klasa_admina,model", + [ + (AutorAdmin, Autor), + (JednostkaAdmin, Jednostka), + (Wydawnictwo_CiagleAdmin, Wydawnictwo_Ciagle), + ], +) +def test_admin_pobiera_w_trybie_fetch_peers(klasa_admina, model, rf, admin_user): + queryset = _queryset_admina(klasa_admina, model, rf, admin_user) + assert queryset._fetch_mode is FETCH_PEERS, klasa_admina.__name__ + + +@pytest.mark.django_db +def test_fetch_peers_przezywa_lancuch_querysetu(rf, admin_user): + """Tryb przenosi się przez ``_clone()`` — filtr, sortowanie i slicing. + + ``ChangeList`` i dalsze mixiny dokładają do querysetu własne ``filter``, + ``order_by`` i wycinek strony. Gdyby tryb ginął przy klonowaniu, + ustawienie go w ``get_queryset`` nic by nie dawało. + """ + queryset = _queryset_admina(AutorAdmin, Autor, rf, admin_user) + + assert queryset.filter(pokazuj=True)._fetch_mode is FETCH_PEERS + assert queryset.order_by("nazwisko")._fetch_mode is FETCH_PEERS + assert queryset.filter(pokazuj=True).order_by("nazwisko")[:10]._fetch_mode is ( + FETCH_PEERS + ) diff --git a/src/bpp/tests/test_admin/test_filters.py b/src/bpp/tests/test_admin/test_filters.py index 7a4813c21..1e4c16167 100644 --- a/src/bpp/tests/test_admin/test_filters.py +++ b/src/bpp/tests/test_admin/test_filters.py @@ -1,11 +1,14 @@ import pytest -from model_bakery import baker - from django.contrib.admin.models import ADDITION, CHANGE, LogEntry from django.contrib.contenttypes.models import ContentType +from model_bakery import baker -from bpp.admin.filters import OstatnioZmienionePrzezFilter, UtworzonePrzezFilter -from bpp.models import Wydawnictwo_Zwarte +from bpp.admin.filters import ( + JednostkaFilter, + OstatnioZmienionePrzezFilter, + UtworzonePrzezFilter, +) +from bpp.models import Autor, BppUser, Wydawnictwo_Zwarte @pytest.mark.django_db @@ -57,3 +60,64 @@ def test_UtworzonePrzeZFilter(wydawnictwo_zwarte, admin_user, normal_django_user user_ids = [x[0] for x in f.lookups(None, None)] assert admin_user.pk in user_ids + + +@pytest.mark.django_db +def test_JednostkaFilter_lookups_jednym_zapytaniem(uczelnia, django_assert_num_queries): + """Opcje filtra „Jednostka" powstają JEDNYM zapytaniem. + + Regresja: ``lookups()`` woła ``str(x)`` na każdej jednostce, a + ``Jednostka.__str__`` czyta DWA FK — ``self.uczelnia`` (bramka + ``uzywaj_wydzialow``) i ``self.wydzial`` (skrót w nawiasie). + ``select_related`` pokrywał tylko ``wydzial``, więc każda opcja listy + kosztowała osobny SELECT po uczelnię: na bazie produkcyjnej 504 jednostki + = 504 zapytania na KAŻDE wejście na changelistę autorów. + """ + from bpp.tests.util import any_jednostka + + for numer in range(4): + any_jednostka(nazwa=f"Jednostka Filtrowa {numer}", uczelnia=uczelnia) + + filtr = JednostkaFilter(None, {}, Autor, None) + + with django_assert_num_queries(1): + etykiety = [etykieta for _pk, etykieta in filtr.lookups(None, None)] + + assert len([e for e in etykiety if "Jednostka Filtrowa" in e]) == 4 + + +@pytest.mark.django_db +def test_UtworzonePrzezFilter_lookups_jednym_zapytaniem( + wydawnictwo_zwarte, django_assert_num_queries +): + """Opcje filtra „Utworzone przez" powstają JEDNYM zapytaniem. + + Regresja: ``lookups()`` zawężał queryset przez ``.only("pk", "username")``, + ale ``BppUser.__str__`` czyta też ``last_name`` i ``first_name``. Każde + pole odroczone to osobny ``refresh_from_db()`` per użytkownik, czyli DWA + dodatkowe SELECT-y na wiersz — „optymalizacja", która kosztowała zamiast + oszczędzać (na produkcji 156 zapytań na wejście na changelistę wydawnictw + ciągłych). Test pilnuje, żeby ``only()`` nadążało za ``__str__``. + """ + content_type_id = ContentType.objects.get_for_model(wydawnictwo_zwarte).pk + + for numer in range(3): + LogEntry.objects.create( + action_flag=ADDITION, + object_id=wydawnictwo_zwarte.pk, + user=baker.make( + BppUser, first_name=f"Imie{numer}", last_name=f"Nazwisko{numer}" + ), + content_type_id=content_type_id, + ) + + filtr = UtworzonePrzezFilter(None, {}, Wydawnictwo_Zwarte, None) + + with django_assert_num_queries(1): + etykiety = [etykieta for _pk, etykieta in filtr.lookups(None, None)] + + assert len(etykiety) == 3 + # Nazwisko i imię MUSZĄ się pojawić — inaczej ``only()`` znów je pominęło, + # a ``__str__`` po cichu degradowałby do samego ``username``. + assert all("Nazwisko" in etykieta for etykieta in etykiety), etykiety + assert all("Imie" in etykieta for etykieta in etykiety), etykiety diff --git a/src/bpp/tests/test_views/test_browse/test_jednostka_jednostki.py b/src/bpp/tests/test_views/test_browse/test_jednostka_jednostki.py index 70f2c692a..0195db59f 100644 --- a/src/bpp/tests/test_views/test_browse/test_jednostka_jednostki.py +++ b/src/bpp/tests/test_views/test_browse/test_jednostka_jednostki.py @@ -349,3 +349,62 @@ def test_jednostka_aktualni_pracownicy( html = normalize_html(res.rendered_content) assert "Obecni pracownicy" in html assert "Byli pracownicy" in html + + +def test_JednostkiView_liczba_autorow_jednym_zapytaniem( + uczelnia, rf, django_assert_num_queries +): + """Liczba autorów per jednostka przychodzi z adnotacji, nie z pętli. + + Regresja: szablon ``browse/jednostki.html`` wołał + ``item.aktualna_jednostka.count`` — relacja ODWROTNA (Autor → Jednostka), + więc każde użycie to osobny ``COUNT(*)``, a szablon używał go 3–4 razy na + wiersz. Na bazie produkcyjnej (150 jednostek na stronę) dawało to ~514 + zapytań na jedno wejście na indeks jednostek. + + Test pilnuje SEDNA: przejście po całym querysecie i odczytanie + ``liczba_autorow`` dla każdego wiersza to JEDNO zapytanie, niezależnie od + liczby jednostek. Sam queryset budujemy poza pomiarem, bo + ``Uczelnia.objects.get_for_request`` też odpytuje bazę, a nie o nim tu mowa. + """ + from bpp.tests.util import any_jednostka + + z_autorami = any_jednostka(nazwa="Jednostka Z Autorami", uczelnia=uczelnia) + bez_autorow = any_jednostka(nazwa="Jednostka Bez Autorow", uczelnia=uczelnia) + for _ in range(3): + baker.make(Autor, aktualna_jednostka=z_autorami) + + widok = JednostkiView() + widok.request = rf.get(reverse("bpp:browse_jednostki")) + widok.kwargs = {} + queryset = widok.get_queryset() + + with django_assert_num_queries(1): + liczby = {j.pk: j.liczba_autorow for j in queryset} + + assert liczby[z_autorami.pk] == 3 + assert liczby[bez_autorow.pk] == 0 + + +def test_browse_jednostki_pokazuje_liczbe_autorow(uczelnia, client): + """Liczba autorów jest widoczna na stronie — nie zniknęła z szablonu. + + Odrębny test od powyższego, bo optymalizacja przeniosła źródło liczby + z ``item.aktualna_jednostka.count`` na adnotację ``item.liczba_autorow``. + Literówka w nazwie adnotacji NIE wywaliłaby żadnego wyjątku — Django + renderuje nieistniejącą zmienną jako pusty łańcuch — więc strona po cichu + przestałaby pokazywać liczby. Ten test to wyłapuje. + """ + from bpp.tests.util import any_jednostka + + jednostka_z_autorami = any_jednostka( + nazwa="Jednostka Licznikowa", uczelnia=uczelnia + ) + for _ in range(2): + baker.make(Autor, aktualna_jednostka=jednostka_z_autorami) + + res = client.get(reverse("bpp:browse_jednostki")) + tresc = normalize_html(res.rendered_content) + + assert "Jednostka Licznikowa" in tresc + assert "2 autorów" in tresc diff --git a/src/bpp/views/browse.py b/src/bpp/views/browse.py index dd7db3377..27d9fed07 100644 --- a/src/bpp/views/browse.py +++ b/src/bpp/views/browse.py @@ -735,7 +735,30 @@ def get_queryset(self): if uczelnia.pokazuj_tylko_jednostki_nadrzedne: qry = qry.filter(parent=None) - ret = qry.only("nazwa", "slug", "wydzial").select_related("wydzial") + # ``liczba_autorow`` liczona JEDNYM agregatem, a nie per wiersz w + # szablonie. Wcześniej `browse/jednostki.html` wołał + # ``item.aktualna_jednostka.count`` — relacja ODWROTNA (Autor → + # Jednostka), więc każde użycie to osobny ``COUNT(*)``, a szablon + # używał go 3–4 razy na wiersz. Przy 150 jednostkach na stronę dawało + # to ~514 zapytań na jedno wejście na indeks jednostek (zmierzone na + # bazie produkcyjnej: 514 → 3 zapytania, 137 ms → 20 ms). + # + # Ani cacheops, ani fetch modes z Django 6.1 tego NIE łapały: + # queryset dotyczy ``bpp.autor``, którego nie ma w regułach + # ``CACHEOPS``, a ``FETCH_PEERS`` obsługuje leniwe FK i pola + # odroczone — nie ``RelatedManager.count()``. + # + # Adnotacja jest tu bezpieczna (nie zawyża liczb przez zdublowane + # wiersze), bo — jak dokumentuje komentarz w ``Browser.get_queryset`` + # — żadna ścieżka filtrowania ``JednostkiView`` nie mnoży wierszy: + # filtr literki to ``istartswith`` na własnej kolumnie, fulltext dla + # ``Jednostka`` to predykat na jednokolumnowym tsvectorze bez JOIN-a, + # a ``scope_jednostki_do_uczelni`` porównuje skalarny FK. + ret = ( + qry.only("nazwa", "slug", "wydzial") + .select_related("wydzial") + .annotate(liczba_autorow=Count("aktualna_jednostka")) + ) if ordering: ret = ret.order_by(*ordering)