Find references from submitted XML instead of rendering to text and parsing. Fixes ietf-tools#3342. Commit ready for merge.

 - Legacy-Id: 19825

Author: jennifer-richards

bin/add-old-drafts-from-archive.py

@@ -14,7 +14,7 @@
 
 from django.conf import settings
 from django.core.validators import validate_email, ValidationError
-from ietf.utils.draft import Draft
+from ietf.utils.draft import PlaintextDraft
 from ietf.submit.utils import update_authors
 
 import debug                            # pyflakes:ignore
@@ -61,7 +61,7 @@
                 except UnicodeDecodeError:
                     text = raw.decode('latin1')
                 try:
-                    draft = Draft(text, txt_file.name, name_from_source=True)
+                    draft = PlaintextDraft(text, txt_file.name, name_from_source=True)
                 except Exception as e:
                     print name, rev, "Can't parse", p,":",e
                     continue

ietf/doc/tests_utils.py

@@ -1,16 +1,22 @@
 # Copyright The IETF Trust 2020, All Rights Reserved
 import datetime
+import debug  # pyflakes:ignore
+
+from unittest.mock import patch
 
 from django.db import IntegrityError
 
 from ietf.group.factories import GroupFactory, RoleFactory
 from ietf.name.models import DocTagName
 from ietf.person.factories import PersonFactory
-from ietf.utils.test_utils import TestCase
+from ietf.utils.test_utils import TestCase, name_of_file_containing
 from ietf.person.models import Person
-from ietf.doc.factories import DocumentFactory, WgRfcFactory
+from ietf.doc.factories import DocumentFactory, WgRfcFactory, WgDraftFactory
 from ietf.doc.models import State, DocumentActionHolder, DocumentAuthor, Document
-from ietf.doc.utils import update_action_holders, add_state_change_event, update_documentauthors, fuzzy_find_documents
+from ietf.doc.utils import (update_action_holders, add_state_change_event, update_documentauthors,
+                            fuzzy_find_documents, rebuild_reference_relations)
+from ietf.utils.draft import Draft, PlaintextDraft
+from ietf.utils.xmldraft import XMLDraft
 
 
 class ActionHoldersTests(TestCase):
@@ -285,3 +291,140 @@ def test_fuzzy_find_documents(self):
         self.do_fuzzy_find_documents_rfc_test('draft-name-with-number-01')
         self.do_fuzzy_find_documents_rfc_test('draft-name-that-has-two-02-04')
         self.do_fuzzy_find_documents_rfc_test('draft-wild-01-numbers-0312')
+
+
+class RebuildReferenceRelationsTests(TestCase):
+    def setUp(self):
+        super().setUp()
+        self.doc = WgDraftFactory()  # document under test
+        # Other documents that should be found by rebuild_reference_relations
+        self.normative, self.informative, self.unknown = WgRfcFactory.create_batch(3)
+        for relationship in ['refnorm', 'refinfo', 'refunk', 'refold']:
+            self.doc.relateddocument_set.create(
+                target=WgRfcFactory().docalias.first(),
+                relationship_id=relationship,
+            )
+        self.updated = WgRfcFactory()  # related document that should be left alone
+        self.doc.relateddocument_set.create(target=self.updated.docalias.first(), relationship_id='updates')
+        self.assertCountEqual(self.doc.relateddocument_set.values_list('relationship__slug', flat=True),
+                              ['refnorm', 'refinfo', 'refold', 'refunk', 'updates'],
+                              'Test conditions set up incorrectly: wrong prior document relationships')
+        for other_doc in [self.normative, self.informative, self.unknown]:
+            self.assertEqual(
+                self.doc.relateddocument_set.filter(target__name=other_doc.canonical_name()).count(),
+                0,
+                'Test conditions set up incorrectly: new documents already related',
+            )
+
+    def _get_refs_return_value(self):
+        return {
+            self.normative.canonical_name(): Draft.REF_TYPE_NORMATIVE,
+            self.informative.canonical_name(): Draft.REF_TYPE_INFORMATIVE,
+            self.unknown.canonical_name(): Draft.REF_TYPE_UNKNOWN,
+            'draft-not-found': Draft.REF_TYPE_NORMATIVE,
+        }
+
+    def test_requires_txt_or_xml(self):
+        result = rebuild_reference_relations(self.doc, {})
+        self.assertCountEqual(result.keys(), ['errors'])
+        self.assertEqual(len(result['errors']), 1)
+        self.assertIn('No draft text available', result['errors'][0],
+                      'Error should be reported if no draft file is given')
+
+        result = rebuild_reference_relations(self.doc, {'md': 'cant-do-this.md'})
+        self.assertCountEqual(result.keys(), ['errors'])
+        self.assertEqual(len(result['errors']), 1)
+        self.assertIn('No draft text available', result['errors'][0],
+                      'Error should be reported if no XML or plaintext file is given')
+
+    @patch.object(XMLDraft, 'get_refs')
+    @patch.object(XMLDraft, '__init__', return_value=None)
+    def test_xml(self, mock_init, mock_get_refs):
+        """Should build reference relations with only XML"""
+        mock_get_refs.return_value = self._get_refs_return_value()
+
+        result = rebuild_reference_relations(self.doc, {'xml': 'file.xml'})
+
+        # if the method of calling the XMLDraft() constructor changes, this will need to be updated
+        xmldraft_init_args, _ = mock_init.call_args
+        self.assertEqual(xmldraft_init_args, ('file.xml',), 'XMLDraft initialized with unexpected arguments')
+        self.assertEqual(
+            result,
+            {
+                'warnings': ['There were 1 references with no matching DocAlias'],
+                'unfound': ['draft-not-found'],
+            }
+        )
+
+        self.assertCountEqual(
+            self.doc.relateddocument_set.values_list('target__name', 'relationship__slug'),
+            [
+                (self.normative.canonical_name(), 'refnorm'),
+                (self.informative.canonical_name(), 'refinfo'),
+                (self.unknown.canonical_name(), 'refunk'),
+                (self.updated.docalias.first().name, 'updates'),
+            ]
+        )
+
+    @patch.object(PlaintextDraft, 'get_refs')
+    @patch.object(PlaintextDraft, '__init__', return_value=None)
+    def test_plaintext(self, mock_init, mock_get_refs):
+        """Should build reference relations with only plaintext"""
+        mock_get_refs.return_value = self._get_refs_return_value()
+
+        with name_of_file_containing('contents') as temp_file_name:
+            result = rebuild_reference_relations(self.doc, {'txt': temp_file_name})
+
+        # if the method of calling the PlaintextDraft() constructor changes, this test will need to be updated
+        _, mock_init_kwargs = mock_init.call_args
+        self.assertEqual(mock_init_kwargs, {'text': 'contents', 'source': temp_file_name},
+                         'PlaintextDraft initialized with unexpected arguments')
+        self.assertEqual(
+            result,
+            {
+                'warnings': ['There were 1 references with no matching DocAlias'],
+                'unfound': ['draft-not-found'],
+            }
+        )
+
+        self.assertCountEqual(
+            self.doc.relateddocument_set.values_list('target__name', 'relationship__slug'),
+            [
+                (self.normative.canonical_name(), 'refnorm'),
+                (self.informative.canonical_name(), 'refinfo'),
+                (self.unknown.canonical_name(), 'refunk'),
+                (self.updated.docalias.first().name, 'updates'),
+            ]
+        )
+
+    @patch.object(PlaintextDraft, '__init__')
+    @patch.object(XMLDraft, 'get_refs')
+    @patch.object(XMLDraft, '__init__', return_value=None)
+    def test_xml_and_plaintext(self, mock_init, mock_get_refs, mock_plaintext_init):
+        """Should build reference relations with XML when plaintext also available"""
+        mock_get_refs.return_value = self._get_refs_return_value()
+
+        result = rebuild_reference_relations(self.doc, {'txt': 'file.txt', 'xml': 'file.xml'})
+
+        self.assertFalse(mock_plaintext_init.called, 'PlaintextDraft should not be used when XML is available')
+
+        # if the method of calling the XMLDraft() constructor changes, this will need to be updated
+        xmldraft_init_args, _ = mock_init.call_args
+        self.assertEqual(xmldraft_init_args, ('file.xml',), 'XMLDraft initialized with unexpected arguments')
+        self.assertEqual(
+            result,
+            {
+                'warnings': ['There were 1 references with no matching DocAlias'],
+                'unfound': ['draft-not-found'],
+            }
+        )
+
+        self.assertCountEqual(
+            self.doc.relateddocument_set.values_list('target__name', 'relationship__slug'),
+            [
+                (self.normative.canonical_name(), 'refnorm'),
+                (self.informative.canonical_name(), 'refinfo'),
+                (self.unknown.canonical_name(), 'refunk'),
+                (self.updated.docalias.first().name, 'updates'),
+            ]
+        )

ietf/doc/utils.py

@@ -39,6 +39,8 @@
 from ietf.utils.mail import send_mail
 from ietf.mailtrigger.utils import gather_address_lists
 from ietf.utils import log
+from ietf.utils.xmldraft import XMLDraft
+
 
 def save_document_in_history(doc):
     """Save a snapshot of document and related objects in the database."""
@@ -742,28 +744,33 @@ def update_telechat(request, doc, by, new_telechat_date, new_returning_item=None
 
     return e
 
-def rebuild_reference_relations(doc,filename=None):
+def rebuild_reference_relations(doc, filenames):
+    """Rebuild reference relations for a document
+
+    filenames should be a dict mapping file ext (i.e., type) to the full path of each file.
+    """
     if doc.type.slug != 'draft':
         return None
 
-    if not filename:
-        if doc.get_state_slug() == 'rfc':
-            filename=os.path.join(settings.RFC_PATH,doc.canonical_name()+".txt")
-        else:
-            filename=os.path.join(settings.INTERNET_DRAFT_PATH,doc.filename_with_rev())
-
-    try:
-        with io.open(filename, 'rb') as file:
-            refs = draft.Draft(file.read().decode('utf8'), filename).get_refs()
-    except IOError as e:
-        return { 'errors': ["%s :%s" %  (e.strerror, filename)] }
+    # try XML first
+    if 'xml' in filenames:
+        refs = XMLDraft(filenames['xml']).get_refs()
+    elif 'txt' in filenames:
+        filename = filenames['txt']
+        try:
+            refs = draft.PlaintextDraft.from_file(filename).get_refs()
+        except IOError as e:
+            return { 'errors': ["%s :%s" %  (e.strerror, filename)] }
+    else:
+        return {'errors': ['No draft text available for rebuilding reference relations. Need XML or plaintext.']}
 
     doc.relateddocument_set.filter(relationship__slug__in=['refnorm','refinfo','refold','refunk']).delete()
 
     warnings = []
     errors = []
     unfound = set()
     for ( ref, refType ) in refs.items():
+        # As of Dec 2021, DocAlias has a unique constraint on the name field, so count > 1 should not occur
         refdoc = DocAlias.objects.filter( name=ref )
         count = refdoc.count()
         if count == 0:

ietf/doc/views_doc.py

@@ -80,7 +80,7 @@
 from ietf.review.utils import can_request_review_of_doc, review_assignments_to_list_for_docs
 from ietf.review.utils import no_review_from_teams_on_doc
 from ietf.utils import markup_txt, log, markdown
-from ietf.utils.draft import Draft
+from ietf.utils.draft import PlaintextDraft
 from ietf.utils.response import permission_denied
 from ietf.utils.text import maybe_split
 
@@ -1842,7 +1842,7 @@ def idnits2_state(request, name, rev=None):
     else:
         text = doc.text()
         if text:
-            parsed_draft = Draft(text=doc.text(), source=name, name_from_source=False)
+            parsed_draft = PlaintextDraft(text=doc.text(), source=name, name_from_source=False)
             doc.deststatus = parsed_draft.get_status()
         else:
             doc.deststatus="Unknown"

ietf/stats/backfill_data.py

@@ -29,7 +29,7 @@
 
 from ietf.doc.models import Document
 from ietf.name.models import FormalLanguageName
-from ietf.utils.draft import Draft
+from ietf.utils.draft import PlaintextDraft
 
 parser = argparse.ArgumentParser()
 parser.add_argument("--document", help="specific document name")
@@ -89,7 +89,7 @@ def unicode(text):
     with io.open(path, 'rb') as f:
         say("\nProcessing %s" % doc.name)
         sys.stdout.flush()
-        d = Draft(unicode(f.read()), path)
+        d = PlaintextDraft(unicode(f.read()), path)
 
         updated = False
 

ietf/submit/forms.py

@@ -38,7 +38,7 @@
 from ietf.submit.parsers.plain_parser import PlainParser
 from ietf.submit.parsers.xml_parser import XMLParser
 from ietf.utils import log
-from ietf.utils.draft import Draft
+from ietf.utils.draft import PlaintextDraft
 from ietf.utils.text import normalize_text
 
 class SubmissionBaseUploadForm(forms.Form):
@@ -302,7 +302,7 @@ def cleanup():  # called when context exited, even in case of exception
             try:
                 text = bytes.decode(self.file_info['txt'].charset)
             #
-                self.parsed_draft = Draft(text, txt_file.name)
+                self.parsed_draft = PlaintextDraft(text, txt_file.name)
                 if self.filename == None:
                     self.filename = self.parsed_draft.filename
                 elif self.filename != self.parsed_draft.filename:

ietf/submit/tests.py

@@ -13,14 +13,19 @@
 from io import StringIO
 from pyquery import PyQuery
 
+from pathlib import Path
+
 from django.conf import settings
+from django.test import override_settings
+from django.test.client import RequestFactory
 from django.urls import reverse as urlreverse
 from django.utils.encoding import force_str, force_text
 
 import debug                            # pyflakes:ignore
 
-from ietf.submit.utils import expirable_submissions, expire_submission
-from ietf.doc.factories import DocumentFactory, WgDraftFactory, IndividualDraftFactory
+from ietf.submit.utils import (expirable_submissions, expire_submission, find_submission_filenames,
+                               post_submission)
+from ietf.doc.factories import DocumentFactory, WgDraftFactory, IndividualDraftFactory, IndividualRfcFactory
 from ietf.doc.models import ( Document, DocAlias, DocEvent, State,
     BallotPositionDocEvent, DocumentAuthor, SubmissionDocEvent )
 from ietf.doc.utils import create_ballot_if_not_open, can_edit_docextresources, update_action_holders
@@ -40,7 +45,7 @@
 from ietf.utils.mail import outbox, empty_outbox, get_payload_text
 from ietf.utils.models import VersionInfo
 from ietf.utils.test_utils import login_testing_unauthorized, TestCase
-from ietf.utils.draft import Draft
+from ietf.utils.draft import PlaintextDraft
 
 
 class BaseSubmitTestCase(TestCase):
@@ -2860,10 +2865,62 @@ def test_draft_refs_identification(self):
 
         group = None
         file, __ = submission_file('draft-some-subject', '00', group, 'txt', "test_submission.txt", )
-        draft = Draft(file.read(), file.name)
+        draft = PlaintextDraft(file.read(), file.name)
         refs = draft.get_refs()
         self.assertEqual(refs['rfc2119'], 'norm')
         self.assertEqual(refs['rfc8174'], 'norm')
         self.assertEqual(refs['rfc8126'], 'info')
         self.assertEqual(refs['rfc8175'], 'info')
-        
+
+
+class PostSubmissionTests(BaseSubmitTestCase):
+    @override_settings(RFC_FILE_TYPES=('txt', 'xml'), IDSUBMIT_FILE_TYPES=('pdf', 'md'))
+    def test_find_submission_filenames_rfc(self):
+        """Posting an RFC submission should use RFC_FILE_TYPES"""
+        rfc = IndividualRfcFactory()
+        path = Path(self.staging_dir)
+        for ext in ['txt', 'xml', 'pdf', 'md']:
+            (path / f'{rfc.name}-{rfc.rev}.{ext}').touch()
+        files = find_submission_filenames(rfc)
+        self.assertCountEqual(
+            files,
+            {
+                'txt': f'{path}/{rfc.name}-{rfc.rev}.txt',
+                'xml': f'{path}/{rfc.name}-{rfc.rev}.xml',
+                # should NOT find the pdf or md
+            }
+        )
+
+    @override_settings(RFC_FILE_TYPES=('txt', 'xml'), IDSUBMIT_FILE_TYPES=('pdf', 'md'))
+    def test_find_submission_filenames_draft(self):
+        """Posting an I-D submission should use IDSUBMIT_FILE_TYPES"""
+        draft = WgDraftFactory()
+        path = Path(self.staging_dir)
+        for ext in ['txt', 'xml', 'pdf', 'md']:
+            (path / f'{draft.name}-{draft.rev}.{ext}').touch()
+        files = find_submission_filenames(draft)
+        self.assertCountEqual(
+            files,
+            {
+                'pdf': f'{path}/{draft.name}-{draft.rev}.pdf',
+                'md': f'{path}/{draft.name}-{draft.rev}.md',
+                # should NOT find the txt or xml
+            }
+        )
+
+    @mock.patch('ietf.submit.utils.rebuild_reference_relations')
+    @mock.patch('ietf.submit.utils.find_submission_filenames')
+    def test_post_submission_rebuilds_ref_relations(self, mock_find_filenames, mock_rebuild_reference_relations):
+        """The post_submission method should rebuild reference relations from correct files
+
+        This tests that the post_submission() utility function gets the list of files to handle from the
+        find_submission_filenames() method and passes them along to rebuild_reference_relations().
+        """
+        submission = SubmissionFactory()
+        mock_find_filenames.return_value = {'xml': f'{self.staging_dir}/{submission.name}.xml'}
+        request = RequestFactory()
+        request.user = PersonFactory().user
+        post_submission(request, submission, 'doc_desc', 'subm_desc')
+        args, kwargs = mock_rebuild_reference_relations.call_args
+        self.assertEqual(args[1], mock_find_filenames.return_value)
+