From ecc823eae5c213bbfa6fc6638e3585079960cb09 Mon Sep 17 00:00:00 2001
From: Jennifer Richards <jennifer@staff.ietf.org>
Date: Wed, 21 Feb 2024 13:59:07 -0400
Subject: [PATCH] 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/utils/tests.py    | 36 ++++++++++++++++++++++++++++++++++++
 ietf/utils/xmldraft.py |  9 ++++++---
 2 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/ietf/utils/tests.py b/ietf/utils/tests.py
index 499b87488..4ac2732ed 100644
--- 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 @@ class XMLDraftTests(TestCase):
                 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
index 5a0abb613..3a9ac02b1 100644
--- a/ietf/utils/xmldraft.py
+++ b/ietf/utils/xmldraft.py
@@ -29,7 +29,7 @@ class XMLDraft(Draft):
         # 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 @@ class XMLDraft(Draft):
             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,