Skip to content

Commit

Permalink
refactor: don't use filesystem for draft aliases (#7555)
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@jennifer-richards
jennifer-richards committed Jun 18, 2024
1 parent 2a90447 commit 6338f45
Show file tree
Hide file tree
Showing 7 changed files with 139 additions and 127 deletions.
27 changes: 0 additions & 27 deletions ietf/checks.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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):
#
Expand Down
143 changes: 92 additions & 51 deletions ietf/doc/tests.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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"""
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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)
Expand All @@ -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):
Expand Down
26 changes: 24 additions & 2 deletions ietf/doc/utils.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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]:
Expand Down
61 changes: 23 additions & 38 deletions ietf/doc/views_doc.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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):

Expand Down
5 changes: 0 additions & 5 deletions ietf/settings.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
Loading

0 comments on commit 6338f45

Please sign in to comment.