[pull] main from ietf-tools:main

ietf/doc/models.py

@@ -608,12 +608,21 @@ def all_relations_that_doc(self, relationship, related=None):
         return related
 
     def related_that(self, relationship):
+        # _cached_related_that is populated in bulk by callers that render many
+        # documents at once (see ietf.doc.utils_search.fill_in_document_relations);
+        # without it each document costs a query per relationship it displays.
+        cached = getattr(self, "_cached_related_that", None)
+        if cached is not None and relationship in cached:
+            return cached[relationship]
         return list(set([x.source for x in self.relations_that(relationship)]))
 
     def all_related_that(self, relationship, related=None):
         return list(set([x.source for x in self.all_relations_that(relationship)]))
 
     def related_that_doc(self, relationship):
+        cached = getattr(self, "_cached_related_that_doc", None)
+        if cached is not None and relationship in cached:
+            return cached[relationship]
         return list(set([x.target for x in self.relations_that_doc(relationship)]))
 
     def all_related_that_doc(self, relationship, related=None):

ietf/doc/tests.py

@@ -5,14 +5,17 @@
 import os
 import datetime
 import io
+import re
 from hashlib import sha384
 
+from django.contrib.auth.models import AnonymousUser
 from django.http import HttpRequest
 import lxml
 import bibtexparser
 from unittest import mock
 import json
 import copy
+import pickle
 import random
 
 from http.cookies import SimpleCookie
@@ -24,9 +27,13 @@
 
 from django.urls import reverse as urlreverse
 from django.conf import settings
+from django.core.cache import cache
+from django.db import connection
 from django.forms import Form
+from django.http import QueryDict
 from django.utils.html import escape
-from django.test import override_settings
+from django.test import override_settings, RequestFactory
+from django.test.utils import CaptureQueriesContext
 from django.utils import timezone
 from django.utils.text import slugify
 
@@ -48,7 +55,8 @@
                                 NewRevisionDocEventFactory,
                                 StatusChangeFactory, DocExtResourceFactory,
                                 RgDraftFactory, BcpFactory, StdFactory,
-                                FyiFactory, RfcAuthorFactory)
+                                FyiFactory, RfcAuthorFactory,
+                                TelechatDocEventFactory)
 from ietf.doc.forms import NotifyForm
 from ietf.doc.fields import SearchableDocumentsField
 from ietf.doc.utils import (
@@ -62,6 +70,7 @@
     get_doc_email_aliases,
 )
 from ietf.doc.views_doc import get_diff_revisions
+from ietf.doc.views_search import SearchForm, retrieve_search_results, _search_cache_key
 from ietf.group.models import Group, Role
 from ietf.group.factories import GroupFactory, RoleFactory
 from ietf.ipr.factories import HolderIprDisclosureFactory
@@ -77,7 +86,7 @@
 from ietf.utils.test_utils import TestCase
 from ietf.utils.text import normalize_text, texescape
 from ietf.utils.timezone import date_today, datetime_today, DEADLINE_TZINFO, RPC_TZINFO
-from ietf.doc.utils_search import AD_WORKLOAD
+from ietf.doc.utils_search import AD_WORKLOAD, fill_in_telechat_date, prepare_document_table
 
 
 class SearchTests(TestCase):
@@ -191,6 +200,239 @@ def test_search_became_rfc(self):
         self.assertEqual(r.status_code, 200)
         self.assertContains(r, rfc.title)
 
+    def test_search_by_author(self):
+        """The author search covers both DocumentAuthor and RfcAuthor"""
+        base_url = urlreverse('ietf.doc.views_search.search')
+
+        person = PersonFactory(name="Ford Prefect")
+        draft = WgDraftFactory(authors=[person])
+        rfc = WgRfcFactory()
+        RfcAuthorFactory(document=rfc, person=person, titlepage_name="F. Prefect")
+        # an RFC whose title page credits someone the datatracker has no Person for
+        anonymous_rfc = WgRfcFactory()
+        RfcAuthorFactory(document=anonymous_rfc, person=None, titlepage_name="Zaphod Beeblebrox")
+
+        def search(author):
+            r = self.client.get(base_url + f"?activedrafts=on&rfcs=on&by=author&author={author}")
+            self.assertEqual(r.status_code, 200)
+            return r
+
+        # by alias
+        r = search("Prefect")
+        self.assertContains(r, draft.title)
+        self.assertContains(r, rfc.title)
+        self.assertNotContains(r, anonymous_rfc.title)
+
+        # by email address
+        r = search(person.email().address)
+        self.assertContains(r, draft.title)
+        self.assertContains(r, rfc.title)
+
+        # by title page name only
+        r = search("Beeblebrox")
+        self.assertContains(r, anonymous_rfc.title)
+        self.assertNotContains(r, draft.title)
+
+    def test_search_results_are_not_duplicated(self):
+        """retrieve_search_results must match each document at most once.
+
+        It does not apply distinct(): doing so forces a sort over every selected column
+        (including abstract and, once prepare_document_table adds its select_related,
+        group description and person biography). Any filter that can match a document
+        twice has to be expressed as a subquery instead.
+        """
+        person = PersonFactory()
+        rfc = WgRfcFactory()
+        # several ways for one document to match one author search
+        RfcAuthorFactory(document=rfc, person=person)
+        RfcAuthorFactory(document=rfc, person=person)
+        EmailFactory(person=person)
+        draft = WgDraftFactory(authors=[person, person])
+        draft.set_state(State.objects.get(type="draft", slug="active"))
+
+        for query in (
+            f"activedrafts=on&olddrafts=on&rfcs=on&by=author&author={person.name}",
+            "activedrafts=on&olddrafts=on&rfcs=on",
+            f"activedrafts=on&rfcs=on&by=group&group={draft.group.acronym}",
+        ):
+            form = SearchForm(QueryDict(query))
+            self.assertTrue(form.is_valid(), form.errors)
+            pks = list(retrieve_search_results(form).values_list("pk", flat=True))
+            self.assertEqual(len(pks), len(set(pks)), f"duplicate rows for ?{query}")
+
+    def test_search_does_not_join_multivalued_relations(self):
+        """The main search query must not join any multi-valued relation.
+
+        ORing lookups across documentauthor, rfcauthor and targets_related into a single
+        filter() makes those paths cross-multiply. Against the production data set the
+        search this guards produced a 126M-row intermediate result to return 32
+        documents; the joins have to stay out of the outer query.
+        """
+        form = SearchForm(QueryDict("by=author&author=Beeblebrox&name=rfc&rfcs=on"))
+        self.assertTrue(form.is_valid(), form.errors)
+        query = retrieve_search_results(form).query
+
+        # Only the outer query matters: the relations are reached through subqueries,
+        # which do contain joins of their own but are each evaluated once.
+        sql = str(query)
+        from_clause = sql[sql.index(" FROM ") : sql.index(" WHERE ")]
+        self.assertNotIn("JOIN", from_clause, f"main search query joins: {from_clause}")
+        self.assertFalse(query.distinct, "distinct() over the full column list is expensive")
+
+    def test_search_cache_key(self):
+        def key(query):
+            form = SearchForm(QueryDict(query))
+            self.assertTrue(form.is_valid(), form.errors)
+            return _search_cache_key(form)
+
+        # A multi-valued field must not collapse to its last value -- these are
+        # different searches and must not share an entry.
+        self.assertNotEqual(
+            key("doctypes=charter&doctypes=statchg"), key("doctypes=statchg")
+        )
+        # ...but the order the values arrive in does not change the search
+        self.assertEqual(
+            key("doctypes=statchg&doctypes=charter"), key("doctypes=charter&doctypes=statchg")
+        )
+        # sort is applied on every request, so it must not split the cache
+        self.assertEqual(key("rfcs=on&sort=title"), key("rfcs=on&sort=-date"))
+        # equivalent spellings of a checkbox are one search
+        self.assertEqual(key("rfcs=on&name=foo"), key("rfcs=1&name=foo"))
+        # different searches stay apart
+        self.assertNotEqual(key("rfcs=on&name=foo"), key("rfcs=on&name=bar"))
+
+    def test_search_query_count_does_not_grow_with_results(self):
+        """Rendering the document table must not cost queries per row.
+
+        Every attribute the table shows is filled in for the whole result set at once,
+        so doubling the number of rows must not change the number of queries. A lookup
+        that slipped back into the per-row path shows up here as a count that grows.
+
+        Mind the blind spots: these documents have no IESG state, ballot, last call,
+        action holders, telechat or obsoleting RFCs, so the per-row work the columns
+        driven by those still do is not covered. Widen the fixtures rather than reading
+        a pass here as "the table does no per-row queries".
+        """
+        group = GroupFactory(type_id="wg")
+        url = urlreverse('ietf.doc.views_search.search') + (
+            f"?activedrafts=on&olddrafts=on&rfcs=on&by=group&group={group.acronym}"
+        )
+
+        def add_documents(count):
+            for _ in range(count):
+                WgDraftFactory(group=group, authors=[PersonFactory()], ad=PersonFactory(),
+                               shepherd=EmailFactory())
+                WgRfcFactory(group=group)
+
+        def count_queries():
+            with CaptureQueriesContext(connection) as context:
+                r = self.client.get(url)
+            self.assertEqual(r.status_code, 200)
+            return len(context.captured_queries)
+
+        add_documents(2)
+        baseline = count_queries()
+        add_documents(4)
+        doubled = count_queries()
+
+        # A per-row lookup would add at least one query for each of the 8 new documents.
+        self.assertLessEqual(
+            doubled, baseline + 2,
+            f"query count grew from {baseline} to {doubled} when the result set tripled",
+        )
+
+    @override_settings(CACHES={"default": {
+        "BACKEND": "django.core.cache.backends.locmem.LocMemCache",
+        "LOCATION": "test_search_cache_hit_preserves_row_order",
+    }})
+    def test_search_cache_hit_preserves_row_order(self):
+        """A cached search must render its rows in the same order as an uncached one.
+
+        The view caches document ids and re-prepares them on a hit, so the rows arrive
+        in whatever order the pk lookup returns. prepare_document_table sorts stably and
+        several sort keys tie heavily -- ipr, status and ad -- so without a total
+        ordering the same URL renders differently depending on whether it hit the cache.
+        Dev and test normally configure a dummy cache, hence the override.
+        """
+        group = GroupFactory(type_id="wg")
+        # Documents sharing a timestamp, which is what makes the ordering ambiguous.
+        shared_time = timezone.now() - datetime.timedelta(days=30)
+        for _ in range(6):
+            draft = WgDraftFactory(group=group, authors=[PersonFactory()])
+            Document.objects.filter(pk=draft.pk).update(time=shared_time)
+        base = urlreverse('ietf.doc.views_search.search')
+
+        for sort in ("ipr", "status", "ad", ""):
+            with self.subTest(sort=sort):
+                cache.clear()
+                url = f"{base}?activedrafts=on&rfcs=on&by=group&group={group.acronym}&sort={sort}"
+                miss = self.client.get(url)
+                hit = self.client.get(url)
+                self.assertEqual(miss.status_code, 200)
+                self.assertEqual(hit.status_code, 200)
+                rows = lambda r: re.findall(  # noqa: E731
+                    rb'href="(/doc/[^"]+)"', r.content
+                )
+                self.assertEqual(rows(miss), rows(hit))
+
+    def test_prepared_documents_are_picklable(self):
+        """ietf.doc.views_search.recent_drafts pickles prepared documents into a cache.
+
+        In production the slowpages cache is file-based, so everything attached to a
+        document by prepare_document_table has to survive pickling. Dev and test both
+        use a dummy cache, which accepts anything without serializing it, so nothing
+        else in the suite would notice a regression here.
+        """
+        draft = WgDraftFactory(authors=[PersonFactory()], ad=PersonFactory(),
+                               shepherd=EmailFactory())
+        TelechatDocEventFactory(doc=draft)
+        WgRfcFactory()
+
+        request = RequestFactory().get("/doc/recent/")
+        request.user = AnonymousUser()
+        results, meta = prepare_document_table(request, Document.objects.all())
+        self.assertTrue(results)
+
+        restored, _ = pickle.loads(pickle.dumps([results, meta]))
+        for before, after in zip(results, restored):
+            self.assertEqual(after.telechat_date(), before.telechat_date())
+
+    def test_fill_in_telechat_date_matches_the_method(self):
+        """The precomputed value has to equal what Document.telechat_date() returns.
+
+        The IESG agenda views call fill_in_telechat_date() over a queryset and then
+        filter on doc.telechat_date(), so a mismatch silently drops documents off a
+        telechat agenda.
+        """
+        future = WgDraftFactory()
+        TelechatDocEventFactory(doc=future,
+                                telechat_date=timezone.now() + datetime.timedelta(days=14))
+        past = WgDraftFactory()
+        TelechatDocEventFactory(doc=past,
+                                telechat_date=timezone.now() - datetime.timedelta(days=14))
+        rescheduled = WgDraftFactory()
+        TelechatDocEventFactory(doc=rescheduled,
+                                telechat_date=timezone.now() - datetime.timedelta(days=7))
+        TelechatDocEventFactory(doc=rescheduled,
+                                telechat_date=timezone.now() + datetime.timedelta(days=7))
+        none_scheduled = WgDraftFactory()
+
+        expected = {
+            d.name: Document.objects.get(pk=d.pk).telechat_date()
+            for d in (future, past, rescheduled, none_scheduled)
+        }
+
+        docs = list(Document.objects.filter(
+            name__in=[d.name for d in (future, past, rescheduled, none_scheduled)]
+        ))
+        fill_in_telechat_date(docs)
+
+        for doc in docs:
+            self.assertEqual(doc.telechat_date(), expected[doc.name], doc.name)
+        # the two ends of the range, so the test would fail if everything came back None
+        self.assertIsNotNone(expected[future.name])
+        self.assertIsNone(expected[past.name])
+
     def test_search_for_name(self):
         draft = WgDraftFactory(name='draft-ietf-mars-test',group=GroupFactory(acronym='mars',parent=Group.objects.get(acronym='farfut')),authors=[PersonFactory()],ad=PersonFactory())
         draft.set_state(State.objects.get(used=True, type="draft-iesg", slug="pub-req"))