diff --git a/ietf/doc/models.py b/ietf/doc/models.py index ff879dd71c6..708bbb2ede9 100644 --- a/ietf/doc/models.py +++ b/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): diff --git a/ietf/doc/tests.py b/ietf/doc/tests.py index 0b4818ab788..95bee78ae5f 100644 --- a/ietf/doc/tests.py +++ b/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")) diff --git a/ietf/doc/utils_search.py b/ietf/doc/utils_search.py index a5f461f9bb7..267c51aeee7 100644 --- a/ietf/doc/utils_search.py +++ b/ietf/doc/utils_search.py @@ -5,6 +5,7 @@ import datetime import debug # pyflakes:ignore +from collections import defaultdict from zoneinfo import ZoneInfo from django.conf import settings @@ -13,12 +14,198 @@ from ietf.doc.expire import expirable_drafts from ietf.doc.utils import augment_docs_and_person_with_person_info from ietf.meeting.models import SessionPresentation, Meeting, Session +from ietf.person.models import Alias from ietf.review.utils import review_assignments_to_list_for_docs from ietf.utils.timezone import date_today -def wrap_value(v): - return lambda: v +class wrap_value: + """Callable stand-in for a no-argument method whose value was computed in bulk. + + Assigned over the method on the instance, so templates and code can keep calling + doc.telechat_date() without hitting the database. A class rather than a closure + because documents carrying one of these get pickled into the caches behind + ietf.doc.views_search.recent_drafts, and a lambda is not picklable. + """ + + def __init__(self, value): + self.value = value + + def __call__(self): + return self.value + + def __eq__(self, other): + return isinstance(other, wrap_value) and self.value == other.value + + def __hash__(self): + # Defining __eq__ without this would set __hash__ to None and make instances + # unhashable, which a method they stand in for is not. + return hash(self.value) + + def __repr__(self): + return f"wrap_value({self.value!r})" + + +# Relationships the document table renders for each row. RELATED_THAT holds the ones +# read in the "documents pointing at this one" direction (Document.related_that), +# RELATED_THAT_DOC the ones read in the "documents this one points at" direction +# (Document.related_that_doc). +RELATED_THAT = ("replaces", "contains") +RELATED_THAT_DOC = ("became_rfc", "replaces") + +# Followed transitively to find the documents whose IPR disclosures count as related. +IPR_RELATED = ("obs", "replaces") + + +def fill_in_document_relations(docs, doc_dict, doc_ids): + """Seed each document's relation caches from two queries. + + Document.related_that/related_that_doc otherwise run one query per document per + relationship, and the table reads several of them for every row (friendly_state, + part_of, replaces, became_rfc). + """ + for d in docs: + d._cached_related_that = {name: [] for name in RELATED_THAT} + d._cached_related_that_doc = {name: [] for name in RELATED_THAT_DOC} + + for rel in RelatedDocument.objects.filter( + target_id__in=doc_ids, relationship__in=RELATED_THAT + ).select_related("source"): + doc_dict[rel.target_id]._cached_related_that[rel.relationship_id].append(rel.source) + + for rel in RelatedDocument.objects.filter( + source_id__in=doc_ids, relationship__in=RELATED_THAT_DOC + ).select_related("target"): + doc_dict[rel.source_id]._cached_related_that_doc[rel.relationship_id].append(rel.target) + + for d in docs: + # related_that/related_that_doc deduplicate; match that. + for cache in (d._cached_related_that, d._cached_related_that_doc): + for name, related in cache.items(): + cache[name] = list({r.pk: r for r in related}.values()) + d._cached_became_rfc = next(iter(d._cached_related_that_doc["became_rfc"]), None) + + # For each subseries document a row is part of, the table also reads what that + # subseries contains. Those documents are not in `docs`, so seed them here rather + # than leaving a query per subseries membership. + subseries = defaultdict(list) + for d in docs: + for sub in d._cached_related_that["contains"]: + subseries[sub.pk].append(sub) + if subseries: + contains = defaultdict(list) + for rel in RelatedDocument.objects.filter( + source_id__in=subseries, relationship_id="contains" + ).select_related("target"): + contains[rel.source_id].append(rel.target) + for pk, instances in subseries.items(): + targets = list({r.pk: r for r in contains[pk]}.values()) + for sub in instances: + sub._cached_related_that_doc = {"contains": targets} + + +def fill_in_related_ipr(docs, doc_dict, doc_ids): + """Attach the related IPR disclosure ids to each document. + + Document.related_ipr walks the obs/replaces graph with Document.all_relations_that_doc, + which issues a query per node it visits, per document. Here the graph is walked once + for the whole result set -- one query per level of depth -- and the disclosures are + fetched in a single query. + """ + from ietf.ipr.models import IprDocRel + + edges = defaultdict(set) + seen = set(doc_ids) + front = set(doc_ids) + while front: + next_front = set() + for source_id, target_id in RelatedDocument.objects.filter( + source_id__in=front, relationship__in=IPR_RELATED + ).values_list("source_id", "target_id"): + edges[source_id].add(target_id) + if target_id not in seen: + seen.add(target_id) + next_front.add(target_id) + front = next_front + + def reachable_from(start): + """start plus every document it directly or indirectly obsoletes or replaces.""" + found = {start} + stack = [start] + while stack: + for target_id in edges[stack.pop()]: + if target_id not in found: + found.add(target_id) + stack.append(target_id) + return found + + reachable = {pk: reachable_from(pk) for pk in doc_ids} + + disclosures = defaultdict(set) + involved = set().union(*reachable.values()) if reachable else set() + for document_id, disclosure_id in IprDocRel.objects.filter( + document_id__in=involved, disclosure__state__in=settings.PUBLISH_IPR_STATES + ).values_list("document_id", "disclosure_id"): + disclosures[document_id].add(disclosure_id) + + for d in docs: + related = set() + for pk in reachable[d.pk]: + related |= disclosures[pk] + # Wrapped rather than assigned bare so that the attribute stays callable, like + # the Document.related_ipr method it shadows. Templates auto-call either way, + # but a bare list would turn doc.related_ipr() into a TypeError for any Python + # caller handed a prepared document. + d.related_ipr = wrap_value(sorted(related)) + + +def fill_in_person_caches(docs): + """Seed the per-instance caches person_link and email_person_link read. + + select_related hands every row its own Person instance, so Person.email() and + Person.has_alias_for_name() each cost a query per person the table names -- the AD, + the shepherd, and any action holders. The emails come from the prefetches set up in + prepare_document_table; the aliases need one query between all of them. + """ + # People whose address the table renders via Person.email(). Their email_set is + # prefetched in prepare_document_table. Action holders are only read when the + # document has them enabled, which is the same condition the template applies -- so + # this costs nothing extra for callers that skipped the prefetch. + with_email = [d.ad for d in docs if d.ad_id] + for d in docs: + if d.action_holders_enabled(): + with_email.extend(holder.person for holder in d.documentactionholder_set.all()) + # The shepherd column renders the address it already holds, but still needs an alias. + shepherds = [d.shepherd.person for d in docs if d.shepherd_id and d.shepherd.person_id] + + people = with_email + shepherds + if not people: + return + + aliased = set( + Alias.objects.filter(person__in={p.pk for p in people}).values_list("person_id", "name") + ) + for person in people: + person._cached_has_alias_for_name = (person.pk, person.name) in aliased + + for person in with_email: + if hasattr(person, "_cached_email"): + continue + emails = list(person.email_set.all()) + # Mirror Person.email(): a primary address if there is one -- lowest by address, + # which is the pk an unordered first() would have ordered by -- and otherwise + # the most recent active one. Email.address is a CICharField, so the database + # orders it case-insensitively; casefold the key to match. + primary = sorted((e for e in emails if e.primary), key=lambda e: e.address.lower()) + if primary: + person._cached_email = primary[0] + else: + active = sorted( + (e for e in emails if e.active), + key=lambda e: (e.time, e.address.lower()), + reverse=True, + ) + person._cached_email = active[0] if active else None def fill_in_telechat_date(docs, doc_dict=None, doc_ids=None): @@ -29,12 +216,19 @@ def fill_in_telechat_date(docs, doc_dict=None, doc_ids=None): doc_ids = list(doc_dict.keys()) seen = set() - for e in TelechatDocEvent.objects.filter(doc__id__in=doc_ids, type="scheduled_for_telechat").order_by('-time'): + for e in TelechatDocEvent.objects.filter(doc__id__in=doc_ids, type="scheduled_for_telechat").order_by('-time', '-id'): if e.doc_id not in seen: - #d = doc_dict[e.doc_id] - #d.telechat_date = wrap_value(d.telechat_date(e)) + d = doc_dict[e.doc_id] + # Shadow Document.telechat_date with a callable returning the precomputed + # value, so templates can keep calling doc.telechat_date without each row + # issuing its own latest_event() query. + d.telechat_date = wrap_value(d.telechat_date(e)) seen.add(e.doc_id) + for pk, d in doc_dict.items(): + if pk not in seen: + d.telechat_date = wrap_value(None) + def fill_in_document_sessions(docs, doc_dict, doc_ids): today = date_today() beg_date = today-datetime.timedelta(days=7) @@ -72,15 +266,30 @@ def fill_in_document_table_attributes(docs, have_telechat_date=False): for e in event_types: d.latest_event_cache[e] = None - for e in DocEvent.objects.filter(doc__id__in=doc_ids, type__in=event_types).order_by('time'): + # DISTINCT ON fetches only the newest event of each (doc, type) pair. Ordering + # ascending and letting later rows overwrite earlier ones pulls back every matching + # event, and a draft routinely has dozens of new_revision events. + for e in (DocEvent.objects + .filter(doc__id__in=doc_ids, type__in=event_types) + .order_by('doc_id', 'type', '-time', '-id') + .distinct('doc_id', 'type')): doc_dict[e.doc_id].latest_event_cache[e.type] = e + # Default to None so that ballot_icon finds the attribute for documents with no + # ballot event at all. Otherwise it falls back to doc.active_ballot(), which costs a + # query per such row. + for d in docs: + d.ballot = None seen = set() for e in BallotDocEvent.objects.filter(doc__id__in=doc_ids, type__in=('created_ballot', 'closed_ballot')).order_by('-time','-id'): if not e.doc_id in seen: doc_dict[e.doc_id].ballot = e if e.type == 'created_ballot' else None seen.add(e.doc_id) + fill_in_document_relations(docs, doc_dict, doc_ids) + fill_in_related_ipr(docs, doc_dict, doc_ids) + fill_in_person_caches(docs) + if not have_telechat_date: fill_in_telechat_date(docs, doc_dict, doc_ids) @@ -90,6 +299,13 @@ def fill_in_document_table_attributes(docs, have_telechat_date=False): # misc expirable_pks = expirable_drafts(Document.objects.filter(pk__in=doc_ids)).values_list('pk', flat=True) + + # Look up review assignments for every draft at once. Calling this per document, as + # the loop below used to, repeats a breadth-first walk of the replaces graph and an + # assignment query for each row. + review_docs = [d for d in docs if d.type_id == "draft" and d.get_state_slug() != "rfc"] + review_assignments = review_assignments_to_list_for_docs(review_docs) if review_docs else {} + for d in docs: if d.type_id == "rfc" and d.latest_event_cache["published_rfc"]: @@ -121,8 +337,10 @@ def fill_in_document_table_attributes(docs, have_telechat_date=False): d.expirable = False if d.type_id == "draft" and d.get_state_slug() != "rfc": - d.milestones = [ m for (t, s, v, m) in sorted(((m.time, m.state.slug, m.desc, m) for m in d.groupmilestone_set.all() if m.state_id == "active")) ] - d.review_assignments = review_assignments_to_list_for_docs([d]).get(d.name, []) + # m.state_id is the state slug; reading m.state.slug instead costs a query + # per milestone because the prefetch does not cover it. + d.milestones = [ m for (t, s, v, m) in sorted(((m.time, m.state_id, m.desc, m) for m in d.groupmilestone_set.all() if m.state_id == "active")) ] + d.review_assignments = review_assignments.get(d.name, []) e = d.latest_event_cache.get('started_iesg_process', None) d.balloting_started = e.time if e else datetime.datetime.min @@ -147,7 +365,7 @@ def fill_in_document_table_attributes(docs, have_telechat_date=False): RelatedDocument.objects.filter( target__name__in=list(rfcs.values()), relationship__in=("obs", "updates"), - ).select_related("target") + ).select_related("target", "source") ) # TODO - this likely reduces to something even simpler rel_rfcs = { @@ -193,9 +411,14 @@ def prepare_document_table(request, docs, query=None, max_results=200, show_ad_a if not isinstance(docs, list): # evaluate and fill in attribute results immediately to decrease # the number of queries - docs = docs.select_related("ad", "std_level", "intended_std_level", "group", "stream", "shepherd", ) + # "type" is here because fill_in_document_table_attributes renders it into + # search_heading for every non-draft row. "iprdocrel_set" is not: the table shows + # doc.related_ipr, which is precomputed in fill_in_document_table_attributes and + # never touches that relation. + docs = docs.select_related("ad", "std_level", "intended_std_level", "group", "stream", + "shepherd__person", "type", ) docs = docs.prefetch_related("states__type", "tags", "groupmilestone_set__group", "reviewrequest_set__team", - "ad__email_set", "iprdocrel_set") + "ad__email_set", "documentactionholder_set__person__email_set") docs = docs[:max_results] # <- that is still a queryset, but with a LIMIT now docs = list(docs) else: diff --git a/ietf/doc/views_search.py b/ietf/doc/views_search.py index 5d59adb349f..7cf84b52768 100644 --- a/ietf/doc/views_search.py +++ b/ietf/doc/views_search.py @@ -41,13 +41,14 @@ import operator from collections import defaultdict +from django_stubs_ext import QuerySetAny from functools import reduce from django import forms from django.conf import settings from django.core.cache import cache, caches from django.urls import reverse as urlreverse -from django.db.models import Q +from django.db.models import Model, Q from django.http import Http404, HttpResponseBadRequest, HttpResponse, HttpResponseRedirect, QueryDict from django.shortcuts import render from django.utils import timezone @@ -55,11 +56,10 @@ from django.utils.cache import _generate_cache_key # type: ignore from django.utils.text import slugify - import debug # pyflakes:ignore -from ietf.doc.models import ( Document, DocHistory, State, - NewRevisionDocEvent, IESG_SUBSTATE_TAGS, +from ietf.doc.models import ( Document, DocHistory, DocumentAuthor, RelatedDocument, + RfcAuthor, State, NewRevisionDocEvent, IESG_SUBSTATE_TAGS, IESG_BALLOT_ACTIVE_STATES, IESG_STATCHG_CONFLREV_ACTIVE_STATES, IESG_CHARTER_ACTIVE_STATES ) from ietf.doc.fields import select2_id_doc_name_json @@ -196,27 +196,38 @@ def retrieve_search_results(form, all_types=False): Q(title__icontains=singlespace) ]) + # Matches against a related document are expressed as a subquery on pk rather + # than by following the multi-valued `targets_related` relation. A join here + # would cross-multiply with any other multi-valued relation ORed into the same + # filter (notably the author search below), and the intermediate result explodes. + def related_source_matches(relationship, **source_lookup): + return Q( + pk__in=RelatedDocument.objects.filter( + relationship_id=relationship, **source_lookup + ).values("target_id") + ) + # Do a similar thing if the search is just for a subseries doc, like a bcp. if look_for.lower()[:3] in ["bcp", "fyi", "std"] and look_for[3:].strip().isdigit() and query["rfcs"]: # Also look for rfcs contained in the subseries. queries.extend([ - Q(targets_related__source__name__icontains=look_for, targets_related__relationship_id="contains"), - Q(targets_related__source__title__icontains=look_for, targets_related__relationship_id="contains"), + related_source_matches("contains", source__name__icontains=look_for), + related_source_matches("contains", source__title__icontains=look_for), ]) spaceless = look_for.lower()[:3]+look_for[3:].strip() if spaceless != look_for: queries.extend([ - Q(targets_related__source__name__icontains=spaceless, targets_related__relationship_id="contains"), - Q(targets_related__source__title__icontains=spaceless, targets_related__relationship_id="contains"), + related_source_matches("contains", source__name__icontains=spaceless), + related_source_matches("contains", source__title__icontains=spaceless), ]) singlespace = look_for.lower()[:3]+" "+look_for[3:].strip() if singlespace != look_for: queries.extend([ - Q(targets_related__source__name__icontains=singlespace, targets_related__relationship_id="contains"), - Q(targets_related__source__title__icontains=singlespace, targets_related__relationship_id="contains"), + related_source_matches("contains", source__name__icontains=singlespace), + related_source_matches("contains", source__title__icontains=singlespace), ]) if query["rfcs"]: - queries.extend([Q(targets_related__source__name__icontains=look_for, targets_related__relationship_id="became_rfc")]) + queries.append(related_source_matches("became_rfc", source__name__icontains=look_for)) combined_query = reduce(operator.or_, queries) docs = docs.filter(combined_query) @@ -228,18 +239,41 @@ def retrieve_search_results(form, all_types=False): if query["olddrafts"]: allowed_draft_states.extend(['repl', 'expired', 'auth-rm', 'ietf-rm']) - docs = docs.filter(Q(states__slug__in=allowed_draft_states) | - ~Q(type__slug='draft')) + if allowed_draft_states: + # Subquery rather than a join on `states`: a document has several state rows, so + # joining here duplicates every document that passes the ~Q(type__slug='draft') + # half of the OR, which is what forced the distinct() this function used to end + # with. See the comment before the return. + docs = docs.filter( + ~Q(type__slug='draft') + | Q(pk__in=Document.states.through.objects.filter( + state__slug__in=allowed_draft_states + ).values("document_id")) + ) + elif all_types: + # No draft state is allowed, so no draft can match. Only the all_types path can + # still be holding drafts at this point -- when types drives the queryset above, + # "draft" is in it only if at least one draft state is allowed. + docs = docs.exclude(type__slug='draft') # radio choices by = query["by"] if by == "author": + # Resolve the name or address to people first and match documents against those + # people by primary key. Expressed as ORed joins, the documentauthor and + # rfcauthor paths (each reaching person -> alias and person -> email) cross- + # multiply into an enormous intermediate result. + author = query["author"] + person_ids = Person.objects.filter( + Q(alias__name__icontains=author) | Q(email__address__icontains=author) + ).values("pk") docs = docs.filter( - Q(documentauthor__person__alias__name__icontains=query["author"]) | - Q(documentauthor__person__email__address__icontains=query["author"]) | - Q(rfcauthor__person__alias__name__icontains=query["author"]) | - Q(rfcauthor__person__email__address__icontains=query["author"]) | - Q(rfcauthor__titlepage_name__icontains=query["author"]) + Q(pk__in=DocumentAuthor.objects.filter( + person__in=person_ids + ).values("document_id")) + | Q(pk__in=RfcAuthor.objects.filter( + Q(person__in=person_ids) | Q(titlepage_name__icontains=author) + ).values("document_id")) ) elif by == "group": docs = docs.filter(group__acronym__iexact=query["group"]) @@ -258,22 +292,53 @@ def retrieve_search_results(form, all_types=False): elif by == "stream": docs = docs.filter(stream=query["stream"]) - docs=docs.distinct() + # No distinct() here: every filter above matches a document at most once. The + # multi-valued relations (documentauthor, rfcauthor, targets_related, states) are all + # reached through pk subqueries, and the remaining `states`/`tags` filters match a + # single specific row. A distinct() would be applied to the full column list -- which + # prepare_document_table then widens further with select_related() -- and force a sort + # over every selected column, including abstract, biography and group description. + # If a multi-valued join is ever added back above, restore the distinct() with it. # order by time here to retain the most recent documents in case we - # find too many and have to chop the results list in prepare_document_table - docs = docs.order_by('-time') + # find too many and have to chop the results list in prepare_document_table. + # `time` alone is not a total order -- documents sharing a timestamp to the second + # are common -- so tie-break on pk. Without it the set of documents kept by that + # truncation, and the order prepare_document_table's stable sort falls back to for + # equal sort keys, both vary from one execution to the next. + docs = docs.order_by('-time', 'pk') return docs -def search(request): - def _get_cache_key(params): - fields = set(SearchForm.base_fields) - {'sort'} - kwargs = dict([(k, v) for (k, v) in list(params.items()) if k in fields]) - key = "doc:document:search:" + hashlib.sha512(json.dumps(kwargs, sort_keys=True).encode('utf-8')).hexdigest() - return key +def _search_cache_key(form): + """Cache key for a validated SearchForm. + + Derived from cleaned_data rather than the raw query string. The raw parameters vary + in ways that do not change the search ("on" vs "1" for a checkbox), and + QueryDict.items() returns only the last value of a multi-valued field, so + ?doctypes=charter&doctypes=statchg and ?doctypes=statchg used to share a key and + serve each other's results. + + 'sort' is excluded deliberately: the cached value is the list of matching document + ids, and prepare_document_table applies the requested sort on every request, so one + entry serves every sort order. + """ + def normalize(value): + if isinstance(value, QuerySetAny): # ModelMultipleChoiceField, e.g. doctypes + return sorted(str(obj.pk) for obj in value) + if isinstance(value, Model): # ModelChoiceField, e.g. area, state + return str(value.pk) + return value + + kwargs = {k: normalize(v) for k, v in form.cleaned_data.items() if k != "sort"} + digest = hashlib.sha512( + json.dumps(kwargs, sort_keys=True, default=str).encode("utf-8") + ).hexdigest() + return "doc:document:search:" + digest + +def search(request): if request.GET: # backwards compatibility get_params = request.GET.copy() @@ -288,15 +353,31 @@ def _get_cache_key(params): if not form.is_valid(): return HttpResponseBadRequest("form not valid: %s" % form.errors) - cache_key = _get_cache_key(get_params) - cached_val = cache.get(cache_key) - if cached_val: - [results, meta] = cached_val - else: + # Cache the ids of the matching documents, not the prepared Document objects. + # Pickling the prepared objects produced payloads over memcached's 1MB item + # limit for large result sets (measured at 1.28MB for 200 rows), which + # LenientMemcacheCache silently discards -- so the most expensive searches were + # never cached at all. It also meant a cached result carried the sort order it + # was first computed with, since 'sort' is not part of the key. + cache_key = _search_cache_key(form) + cached_pks = cache.get(cache_key) + if cached_pks is None: results = retrieve_search_results(form) results, meta = prepare_document_table(request, results, get_params) - cache.set(cache_key, [results, meta]) # for settings.CACHE_MIDDLEWARE_SECONDS + cache.set( # for settings.CACHE_MIDDLEWARE_SECONDS + cache_key, [doc.pk for doc in results] + ) log(f"Search results computed for {get_params}") + else: + # Same ordering as retrieve_search_results, so a hit hands + # prepare_document_table the rows in the order the miss did. Its sort is + # stable and several sort keys tie heavily (ipr, status, ad), so an + # unordered pk__in here would reorder those pages between requests. + results, meta = prepare_document_table( + request, + Document.objects.filter(pk__in=cached_pks).order_by('-time', 'pk'), + get_params, + ) meta['searching'] = True else: form = SearchForm() diff --git a/ietf/person/models.py b/ietf/person/models.py index 3ab89289a65..bb6b4ffc737 100644 --- a/ietf/person/models.py +++ b/ietf/person/models.py @@ -158,6 +158,15 @@ def email(self): e = self.email_set.filter(active=True).order_by("-time").first() self._cached_email = e return self._cached_email + def has_alias_for_name(self): + """Is self.name recorded as one of this person's aliases? + + Cached per instance so that callers rendering many people at once can seed it + in bulk instead of paying a query apiece. + """ + if not hasattr(self, '_cached_has_alias_for_name'): + self._cached_has_alias_for_name = self.alias_set.filter(name=self.name).exists() + return self._cached_has_alias_for_name def email_allowing_inactive(self): if not hasattr(self, "_cached_email_allowing_inactive"): e = self.email() @@ -254,6 +263,8 @@ def save(self, *args, **kwargs): if self.ascii and self.name != self.ascii: if not self.ascii in [ a.name for a in self.alias_set.filter(name=self.ascii) ]: self.alias_set.create(name=self.ascii) + # The aliases just changed; drop what has_alias_for_name() memoized about them. + self.__dict__.pop('_cached_has_alias_for_name', None) #this variable, if not None, may be used by url() to keep the sitefqdn. default_hostscheme = None diff --git a/ietf/person/templatetags/person_filters.py b/ietf/person/templatetags/person_filters.py index a7a6e8193a0..715d154a949 100644 --- a/ietf/person/templatetags/person_filters.py +++ b/ietf/person/templatetags/person_filters.py @@ -53,11 +53,7 @@ def person_link(person, **kwargs): titlepage_name = kwargs.get("titlepage_name", None) if person is not None: plain_name = person.plain_name() - name = ( - person.name - if person.alias_set.filter(name=person.name).exists() - else plain_name - ) + name = person.name if person.has_alias_for_name() else plain_name email = person.email_address() return { "name": name, @@ -78,11 +74,7 @@ def email_person_link(email, **kwargs): cls = kwargs.get("class", "") with_email = kwargs.get("with_email", True) plain_name = email.person.plain_name() - name = ( - email.person.name - if email.person.alias_set.filter(name=email.person.name).exists() - else plain_name - ) + name = email.person.name if email.person.has_alias_for_name() else plain_name email = email.address return { "name": name, diff --git a/ietf/review/utils.py b/ietf/review/utils.py index 61494738d35..cf5482a0c1a 100644 --- a/ietf/review/utils.py +++ b/ietf/review/utils.py @@ -73,8 +73,13 @@ def can_access_review_stats_for_team(user, team): or has_role(user, ["Secretariat", "Area Director"])) def review_assignments_to_list_for_docs(docs): + # The document table renders the request's doc, team and type, and links to the + # review itself, for every assignment it shows -- fetch them with the assignment + # rather than one query per attribute per row. assignment_qs = ReviewAssignment.objects.filter( state__in=["assigned", "accepted", "part-completed", "completed"], + ).select_related( + "review_request__doc", "review_request__team", "review_request__type", "review" ).prefetch_related("result") doc_names = [d.name for d in docs]