feat: Give better errors when docName is missing (#7083)

* feat: Give better error when docName is missing * refactor: Make method static for easier testing * test: Add test of XMLDraft.parse_docname()
ietf-tools · Feb 21, 2024 · ecc823e · ecc823e
1 parent 89d2a0c
commit ecc823e
Show file tree

Hide file tree

Showing 2 changed files with 42 additions and 3 deletions.
diff --git a/ietf/utils/tests.py b/ietf/utils/tests.py
@@ -5,6 +5,7 @@
 import datetime
 import io
 import json
+import lxml.etree
 import os.path
 import pytz
 import shutil
@@ -450,6 +451,41 @@ def test_parse_creation_date(self):
                 datetime.date(today.year, 1 if today.month != 1 else 2, 15),
             )
 
+    def test_parse_docname(self):
+        with self.assertRaises(ValueError) as cm:
+            XMLDraft.parse_docname(lxml.etree.Element("xml"))  # no docName
+        self.assertIn("Missing docName attribute", str(cm.exception))
+
+        # There to be more invalid docNames, but we use XMLDraft in places where we don't
+        # actually care about the validation, so for now just test what has long been the
+        # implementation. 
+        with self.assertRaises(ValueError) as cm:
+            XMLDraft.parse_docname(lxml.etree.Element("xml", docName=""))  # not a valid docName
+        self.assertIn("Unable to parse docName", str(cm.exception))
+
+        self.assertEqual(
+            XMLDraft.parse_docname(lxml.etree.Element("xml", docName="draft-foo-bar-baz-01")),
+            ("draft-foo-bar-baz", "01"),
+        )
+
+        self.assertEqual(
+            XMLDraft.parse_docname(lxml.etree.Element("xml", docName="draft-foo-bar-baz")),
+            ("draft-foo-bar-baz", None),
+        )
+
+        self.assertEqual(
+            XMLDraft.parse_docname(lxml.etree.Element("xml", docName="draft-foo-bar-baz-")),
+            ("draft-foo-bar-baz-", None),
+        )
+
+        # This is awful, but is how we've been running for some time. The missing rev will trigger
+        # validation errors for submissions, so we're at least somewhat guarded against this
+        # property.
+        self.assertEqual(
+            XMLDraft.parse_docname(lxml.etree.Element("xml", docName="-01")),
+            ("-01", None),
+        )
+
 
 class NameTests(TestCase):
 

diff --git a/ietf/utils/xmldraft.py b/ietf/utils/xmldraft.py
@@ -29,7 +29,7 @@ def __init__(self, xml_file):
         # cast xml_file to str so, e.g., this will work with a Path
         self.xmltree, self.xml_version = self.parse_xml(str(xml_file))
         self.xmlroot = self.xmltree.getroot()
-        self.filename, self.revision = self._parse_docname()
+        self.filename, self.revision = self.parse_docname(self.xmlroot)
 
     @staticmethod
     def parse_xml(filename):
@@ -125,8 +125,11 @@ def _reference_section_name(self, section_elt):
             section_name = section_elt.get('title')  # fall back to title if we have it
         return section_name
 
-    def _parse_docname(self):
-        docname = self.xmlroot.attrib.get('docName')
+    @staticmethod
+    def parse_docname(xmlroot):
+        docname = xmlroot.attrib.get('docName')
+        if docname is None:
+            raise ValueError("Missing docName attribute in the XML root element")
         revmatch = re.match(
             r'^(?P<filename>.+?)(?:-(?P<rev>[0-9][0-9]))?$',
             docname,