refactor: don't use filesystem for draft aliases (ietf-tools#7555)

* refactor: compute draft aliases on demand

n.b., very slow for full set of aliases

* refactor: simplify and cache email_aliases

The name != "" case is, as far as I can see, unused.

* chore: remove draft alias checks

* chore: remove draft alias/virtual settings

* chore: remove lint

* test: update tests

* test: better mocking

* refactor: move utility to utils

* test: add tests

Author: jennifer-richards

ietf/checks.py

@@ -56,33 +56,6 @@ def check_group_email_aliases_exists(app_configs, **kwargs):
 
     return errors
 
-@checks.register('files')
-def check_doc_email_aliases_exists(app_configs, **kwargs):
-    from ietf.doc.views_doc import check_doc_email_aliases
-    #
-    if already_ran():
-        return []
-    #
-    errors = []
-    try:
-        ok = check_doc_email_aliases()
-        if not ok:
-            errors.append(checks.Error(
-                "Found no aliases in the document email aliases file\n'%s'."%settings.DRAFT_VIRTUAL_PATH,
-                hint="These should be created by the infrastructure using ietf/bin/aliases-from-json.py.",
-                obj=None,
-                id="datatracker.E0004",
-            ))
-    except IOError as e:
-        errors.append(checks.Error(
-            "Could not read document email aliases:\n   %s" % e,
-            hint="These should be created by the infrastructure using ietf/bin/aliases-from-json.py.",
-            obj=None,
-            id="datatracker.E0005",
-        ))
-
-    return errors
-    
 @checks.register('directories')
 def check_id_submission_directories(app_configs, **kwargs):
     #

ietf/doc/tests.py

@@ -16,7 +16,6 @@
 from pathlib import Path
 from pyquery import PyQuery
 from urllib.parse import urlparse, parse_qs
-from tempfile import NamedTemporaryFile
 from collections import defaultdict
 from zoneinfo import ZoneInfo
 
@@ -51,6 +50,7 @@
     DraftAliasGenerator,
     generate_idnits2_rfc_status,
     generate_idnits2_rfcs_obsoleted,
+    get_doc_email_aliases,
 )
 from ietf.group.models import Group, Role
 from ietf.group.factories import GroupFactory, RoleFactory
@@ -2169,24 +2169,6 @@ def test_references(self):
         self.assertContains(r, doc1.name)
 
 class GenerateDraftAliasesTests(TestCase):
-    def setUp(self):
-        super().setUp()
-        self.doc_aliases_file = NamedTemporaryFile(delete=False, mode="w+")
-        self.doc_aliases_file.close()
-        self.doc_virtual_file = NamedTemporaryFile(delete=False, mode="w+")
-        self.doc_virtual_file.close()
-        self.saved_draft_aliases_path = settings.DRAFT_ALIASES_PATH
-        self.saved_draft_virtual_path = settings.DRAFT_VIRTUAL_PATH
-        settings.DRAFT_ALIASES_PATH = self.doc_aliases_file.name
-        settings.DRAFT_VIRTUAL_PATH = self.doc_virtual_file.name
-
-    def tearDown(self):
-        settings.DRAFT_ALIASES_PATH = self.saved_draft_aliases_path
-        settings.DRAFT_VIRTUAL_PATH = self.saved_draft_virtual_path
-        os.unlink(self.doc_aliases_file.name)
-        os.unlink(self.doc_virtual_file.name)
-        super().tearDown()
-
     @override_settings(TOOLS_SERVER="tools.example.org", DRAFT_ALIAS_DOMAIN="draft.example.org")
     def test_generator_class(self):
         """The DraftAliasGenerator should generate the same lists as the old mgmt cmd"""
@@ -2286,6 +2268,28 @@ def test_generator_class(self):
             {k: sorted(v) for k, v in expected_dict.items()},
         )
 
+        # check single name
+        output = [(alias, alist) for alias, alist in DraftAliasGenerator(Document.objects.filter(name=doc1.name))]
+        alias_dict = dict(output)
+        self.assertEqual(len(alias_dict), len(output))  # no duplicate aliases
+        expected_dict = {
+            doc1.name: [author1.email_address()],
+            doc1.name + ".ad": [ad.email_address()],
+            doc1.name + ".authors": [author1.email_address()],
+            doc1.name + ".shepherd": [shepherd.email_address()],
+            doc1.name
+            + ".all": [
+                author1.email_address(),
+                ad.email_address(),
+                shepherd.email_address(),
+            ],
+        }
+        # Sort lists for comparison
+        self.assertEqual(
+            {k: sorted(v) for k, v in alias_dict.items()},
+            {k: sorted(v) for k, v in expected_dict.items()},
+        )
+
     @override_settings(TOOLS_SERVER="tools.example.org", DRAFT_ALIAS_DOMAIN="draft.example.org")
     def test_get_draft_notify_emails(self):
         ad = PersonFactory()
@@ -2336,37 +2340,20 @@ def setUp(self):
         WgDraftFactory(name='draft-ietf-mars-test',group__acronym='mars')
         WgDraftFactory(name='draft-ietf-ames-test',group__acronym='ames')
         RoleFactory(group__type_id='review', group__acronym='yangdoctors', name_id='secr')
-        self.doc_alias_file = NamedTemporaryFile(delete=False, mode='w+')
-        self.doc_alias_file.write("""# Generated by hand at 2015-02-12_16:26:45
-virtual.ietf.org anything
-draft-ietf-mars-test@ietf.org              xfilter-draft-ietf-mars-test
-expand-draft-ietf-mars-test@virtual.ietf.org  mars-author@example.com, mars-collaborator@example.com
-draft-ietf-mars-test.authors@ietf.org      xfilter-draft-ietf-mars-test.authors
-expand-draft-ietf-mars-test.authors@virtual.ietf.org  mars-author@example.mars, mars-collaborator@example.mars
-draft-ietf-mars-test.chairs@ietf.org      xfilter-draft-ietf-mars-test.chairs
-expand-draft-ietf-mars-test.chairs@virtual.ietf.org  mars-chair@example.mars
-draft-ietf-mars-test.all@ietf.org      xfilter-draft-ietf-mars-test.all
-expand-draft-ietf-mars-test.all@virtual.ietf.org  mars-author@example.mars, mars-collaborator@example.mars, mars-chair@example.mars
-draft-ietf-ames-test@ietf.org              xfilter-draft-ietf-ames-test
-expand-draft-ietf-ames-test@virtual.ietf.org  ames-author@example.com, ames-collaborator@example.com
-draft-ietf-ames-test.authors@ietf.org      xfilter-draft-ietf-ames-test.authors
-expand-draft-ietf-ames-test.authors@virtual.ietf.org  ames-author@example.ames, ames-collaborator@example.ames
-draft-ietf-ames-test.chairs@ietf.org      xfilter-draft-ietf-ames-test.chairs
-expand-draft-ietf-ames-test.chairs@virtual.ietf.org  ames-chair@example.ames
-draft-ietf-ames-test.all@ietf.org      xfilter-draft-ietf-ames-test.all
-expand-draft-ietf-ames-test.all@virtual.ietf.org  ames-author@example.ames, ames-collaborator@example.ames, ames-chair@example.ames
-
-""")
-        self.doc_alias_file.close()
-        self.saved_draft_virtual_path = settings.DRAFT_VIRTUAL_PATH
-        settings.DRAFT_VIRTUAL_PATH = self.doc_alias_file.name
-
-    def tearDown(self):
-        settings.DRAFT_VIRTUAL_PATH = self.saved_draft_virtual_path
-        os.unlink(self.doc_alias_file.name)
-        super().tearDown()
-
-    def testAliases(self):
+
+
+    @mock.patch("ietf.doc.views_doc.get_doc_email_aliases")
+    def testAliases(self, mock_get_aliases):
+        mock_get_aliases.return_value = [
+            {"doc_name": "draft-ietf-mars-test", "alias_type": "", "expansion": "mars-author@example.mars, mars-collaborator@example.mars"},
+            {"doc_name": "draft-ietf-mars-test", "alias_type": ".authors", "expansion": "mars-author@example.mars, mars-collaborator@example.mars"},
+            {"doc_name": "draft-ietf-mars-test", "alias_type": ".chairs", "expansion": "mars-chair@example.mars"},
+            {"doc_name": "draft-ietf-mars-test", "alias_type": ".all", "expansion": "mars-author@example.mars, mars-collaborator@example.mars, mars-chair@example.mars"},
+            {"doc_name": "draft-ietf-ames-test", "alias_type": "", "expansion": "ames-author@example.ames, ames-collaborator@example.ames"},
+            {"doc_name": "draft-ietf-ames-test", "alias_type": ".authors", "expansion": "ames-author@example.ames, ames-collaborator@example.ames"},
+            {"doc_name": "draft-ietf-ames-test", "alias_type": ".chairs", "expansion": "ames-chair@example.ames"},
+            {"doc_name": "draft-ietf-ames-test", "alias_type": ".all", "expansion": "ames-author@example.ames, ames-collaborator@example.ames, ames-chair@example.ames"},
+        ]
         PersonFactory(user__username='plain')
         url = urlreverse('ietf.doc.urls.redirect.document_email', kwargs=dict(name="draft-ietf-mars-test"))
         r = self.client.get(url)
@@ -2376,16 +2363,70 @@ def testAliases(self):
         login_testing_unauthorized(self, "plain", url)
         r = self.client.get(url)
         self.assertEqual(r.status_code, 200)
+        self.assertEqual(mock_get_aliases.call_args, mock.call())
         self.assertTrue(all([x in unicontent(r) for x in ['mars-test@','mars-test.authors@','mars-test.chairs@']]))
         self.assertTrue(all([x in unicontent(r) for x in ['ames-test@','ames-test.authors@','ames-test.chairs@']]))
 
-    def testExpansions(self):
+
+    @mock.patch("ietf.doc.views_doc.get_doc_email_aliases")
+    def testExpansions(self, mock_get_aliases):
+        mock_get_aliases.return_value = [
+            {"doc_name": "draft-ietf-mars-test", "alias_type": "", "expansion": "mars-author@example.mars, mars-collaborator@example.mars"},
+            {"doc_name": "draft-ietf-mars-test", "alias_type": ".authors", "expansion": "mars-author@example.mars, mars-collaborator@example.mars"},
+            {"doc_name": "draft-ietf-mars-test", "alias_type": ".chairs", "expansion": "mars-chair@example.mars"},
+            {"doc_name": "draft-ietf-mars-test", "alias_type": ".all", "expansion": "mars-author@example.mars, mars-collaborator@example.mars, mars-chair@example.mars"},
+        ]
         url = urlreverse('ietf.doc.views_doc.document_email', kwargs=dict(name="draft-ietf-mars-test"))
         r = self.client.get(url)
+        self.assertEqual(mock_get_aliases.call_args, mock.call("draft-ietf-mars-test"))
         self.assertEqual(r.status_code, 200)
         self.assertContains(r, 'draft-ietf-mars-test.all@ietf.org')
         self.assertContains(r, 'iesg_ballot_saved')
+    
+    @mock.patch("ietf.doc.utils.DraftAliasGenerator")
+    def test_get_doc_email_aliases(self, mock_alias_gen_cls):
+        mock_alias_gen_cls.return_value = [
+            ("draft-something-or-other.some-type", ["somebody@example.com"]),
+            ("draft-something-or-other", ["somebody@example.com"]),
+            ("draft-nothing-at-all", ["nobody@example.com"]),
+            ("draft-nothing-at-all.some-type", ["nobody@example.com"]),
+        ]
+        # order is important in the response - should be sorted by doc name and otherwise left
+        # in order
+        self.assertEqual(
+            get_doc_email_aliases(),
+            [
+                {
+                    "doc_name": "draft-nothing-at-all",
+                    "alias_type": "",
+                    "expansion": "nobody@example.com",
+                },
+                {
+                    "doc_name": "draft-nothing-at-all",
+                    "alias_type": ".some-type",
+                    "expansion": "nobody@example.com",
+                },
+                {
+                    "doc_name": "draft-something-or-other",
+                    "alias_type": ".some-type",
+                    "expansion": "somebody@example.com",
+                },
+                {
+                    "doc_name": "draft-something-or-other",
+                    "alias_type": "",
+                    "expansion": "somebody@example.com",
+                },
+            ],
+        )
+        self.assertEqual(mock_alias_gen_cls.call_args, mock.call(None))
 
+        # Repeat with a name, no need to re-test that the alias list is actually passed through, just
+        # check that the DraftAliasGenerator is called correctly
+        draft = WgDraftFactory()
+        get_doc_email_aliases(draft.name)
+        self.assertQuerySetEqual(mock_alias_gen_cls.call_args[0][0], Document.objects.filter(pk=draft.pk))
+        
+        
 class DocumentMeetingTests(TestCase):
 
     def setUp(self):

ietf/doc/utils.py

@@ -14,7 +14,7 @@
 from collections import defaultdict, namedtuple, Counter
 from dataclasses import dataclass
 from pathlib import Path
-from typing import Iterator, Union
+from typing import Iterator, Optional, Union
 from zoneinfo import ZoneInfo
 
 from django.conf import settings
@@ -1265,6 +1265,12 @@ def bibxml_for_draft(doc, rev=None):
 class DraftAliasGenerator:
     days = 2 * 365
 
+    def __init__(self, draft_queryset=None):
+        if draft_queryset is not None:
+            self.draft_queryset = draft_queryset.filter(type_id="draft")  # only drafts allowed
+        else:
+            self.draft_queryset = Document.objects.filter(type_id="draft")
+
     def get_draft_ad_emails(self, doc):
         """Get AD email addresses for the given draft, if any."""
         from ietf.group.utils import get_group_ad_emails  # avoid circular import
@@ -1333,7 +1339,7 @@ def get_draft_notify_emails(self, doc):
     def __iter__(self) -> Iterator[tuple[str, list[str]]]:
         # Internet-Drafts with active status or expired within self.days
         show_since = timezone.now() - datetime.timedelta(days=self.days)
-        drafts = Document.objects.filter(type_id="draft")
+        drafts = self.draft_queryset
         active_drafts = drafts.filter(states__slug='active')
         inactive_recent_drafts = drafts.exclude(states__slug='active').filter(expires__gte=show_since)
         interesting_drafts = active_drafts | inactive_recent_drafts
@@ -1384,6 +1390,22 @@ def __iter__(self) -> Iterator[tuple[str, list[str]]]:
             if all:
                 yield alias + ".all", list(all)
 
+
+def get_doc_email_aliases(name: Optional[str] = None):
+    aliases = []
+    for (alias, alist) in DraftAliasGenerator(
+        Document.objects.filter(type_id="draft", name=name) if name else None
+    ):
+        # alias is draft-name.alias_type
+        doc_name, _dot, alias_type = alias.partition(".")
+        aliases.append({
+            "doc_name": doc_name,
+            "alias_type": f".{alias_type}" if alias_type else "",
+            "expansion": ", ".join(sorted(alist)),
+        })
+    return sorted(aliases, key=lambda a: (a["doc_name"]))
+
+
 def investigate_fragment(name_fragment):
     can_verify = set()
     for root in [settings.INTERNET_DRAFT_PATH, settings.INTERNET_DRAFT_ARCHIVE_DIR]:

ietf/doc/views_doc.py

@@ -35,20 +35,21 @@
 
 
 import glob
-import io
 import json
 import os
 import re
 
 from pathlib import Path
 
+from django.core.cache import caches
 from django.db.models import Max
 from django.http import HttpResponse, Http404
 from django.shortcuts import render, get_object_or_404, redirect
 from django.template.loader import render_to_string
 from django.urls import reverse as urlreverse
 from django.conf import settings
 from django import forms
+from django.contrib.auth.decorators import login_required
 from django.contrib.staticfiles import finders
 
 import debug                            # pyflakes:ignore
@@ -64,7 +65,7 @@
     add_events_message_info, get_unicode_document_content,
     augment_docs_and_person_with_person_info, irsg_needed_ballot_positions, add_action_holder_change_event,
     build_file_urls, update_documentauthors, fuzzy_find_documents,
-    bibxml_for_draft)
+    bibxml_for_draft, get_doc_email_aliases)
 from ietf.doc.utils_bofreq import bofreq_editors, bofreq_responsible
 from ietf.group.models import Role, Group
 from ietf.group.utils import can_manage_all_groups_of_type, can_manage_materials, group_features_role_filter
@@ -1071,32 +1072,6 @@ def document_pdfized(request, name, rev=None, ext=None):
     else:
         raise Http404
 
-def check_doc_email_aliases():
-    pattern = re.compile(r'^expand-(.*?)(\..*?)?@.*? +(.*)$')
-    good_count = 0
-    tot_count = 0
-    with io.open(settings.DRAFT_VIRTUAL_PATH,"r") as virtual_file:
-        for line in virtual_file.readlines():
-            m = pattern.match(line)
-            tot_count += 1
-            if m:
-                good_count += 1
-            if good_count > 50 and tot_count < 3*good_count:
-                return True
-    return False
-
-def get_doc_email_aliases(name):
-    if name:
-        pattern = re.compile(r'^expand-(%s)(\..*?)?@.*? +(.*)$'%name)
-    else:
-        pattern = re.compile(r'^expand-(.*?)(\..*?)?@.*? +(.*)$')
-    aliases = []
-    with io.open(settings.DRAFT_VIRTUAL_PATH,"r") as virtual_file:
-        for line in virtual_file.readlines():
-            m = pattern.match(line)
-            if m:
-                aliases.append({'doc_name':m.group(1),'alias_type':m.group(2),'expansion':m.group(3)})
-    return aliases
 
 def document_email(request,name):
     doc = get_object_or_404(Document, name=name)
@@ -2021,16 +1996,26 @@ def remind_action_holders(request, name):
     )
 
 
-def email_aliases(request,name=''):
-    doc = get_object_or_404(Document, name=name) if name else None
-    if not name:
-        # require login for the overview page, but not for the
-        # document-specific pages 
-        if not request.user.is_authenticated:
-                return redirect('%s?next=%s' % (settings.LOGIN_URL, request.path))
-    aliases = get_doc_email_aliases(name)
-
-    return render(request,'doc/email_aliases.html',{'aliases':aliases,'ietf_domain':settings.IETF_DOMAIN,'doc':doc})
+@login_required
+def email_aliases(request):
+    """List of all email aliases
+    
+    This is currently slow except when cached
+    """
+    slowcache = caches["slowpages"]
+    cache_key = "emailaliasesview"
+    aliases = slowcache.get(cache_key)
+    if not aliases:
+        aliases = get_doc_email_aliases()  # gets all aliases
+        slowcache.set(cache_key, aliases, 3600)
+    return render(
+        request,
+        "doc/email_aliases.html",
+        {
+            "aliases": aliases,
+            "ietf_domain": settings.IETF_DOMAIN,
+        },
+    )
 
 class VersionForm(forms.Form):
 

ietf/settings.py

@@ -1065,11 +1065,6 @@ def skip_unreadable_post(record):
 
 TEST_DATA_DIR = os.path.abspath(BASE_DIR + "/../test/data")
 
-# Path to the email alias lists.  Used by ietf.utils.aliases
-DRAFT_ALIASES_PATH = os.path.join(TEST_DATA_DIR, "draft-aliases")
-DRAFT_VIRTUAL_PATH = os.path.join(TEST_DATA_DIR, "draft-virtual")
-DRAFT_VIRTUAL_DOMAIN = "virtual.ietf.org"
-
 GROUP_ALIASES_PATH = os.path.join(TEST_DATA_DIR, "group-aliases")
 GROUP_VIRTUAL_PATH = os.path.join(TEST_DATA_DIR, "group-virtual")
 GROUP_VIRTUAL_DOMAIN = "virtual.ietf.org"