Skip to content

Commit 3735833

Browse files
authored
gh-161110: Make minidom getElementById() stable for duplicate IDs (GH-161111)
Visit child nodes in document order and keep the first cached element for each ID, so the first element in document order is returned. ANT-2026-C52DMQ8Z
1 parent ed9cb4b commit 3735833

4 files changed

Lines changed: 53 additions & 6 deletions

File tree

‎Doc/library/xml.dom.rst‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -756,6 +756,12 @@ inherits properties from :class:`Node`.
756756
Return the element with the given ID, or ``None``.
757757
Only attributes declared as being of type ID in the DTD
758758
or by :meth:`Element.setIdAttribute` are searched.
759+
If several elements have the same ID, the first one
760+
in document order is returned.
761+
762+
.. versionchanged:: next
763+
Previously, which of the elements with the same ID was returned
764+
was unspecified and could change between calls.
759765

760766

761767
.. method:: Document.getElementsByTagName(tagName)

‎Lib/test/test_minidom.py‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2143,6 +2143,42 @@ def testSetIdAttributeNode(self):
21432143
self.confirm(e.isSameNode(doc.getElementById("w"))
21442144
and a2.isId)
21452145

2146+
def check_duplicate_id(self, doc):
2147+
# The first element in document order is returned, and lookups
2148+
# of other IDs which continue the search do not change that.
2149+
for i in range(2):
2150+
self.assertEqual(doc.getElementById("x").getAttribute("n"), "1")
2151+
self.assertEqual(doc.getElementById("y").getAttribute("n"), "0")
2152+
self.assertEqual(doc.getElementById("x").getAttribute("n"), "1")
2153+
self.assertIsNone(doc.getElementById("missing"))
2154+
self.assertEqual(doc.getElementById("x").getAttribute("n"), "1")
2155+
self.assertEqual(doc.getElementById("z").getAttribute("n"), "4")
2156+
# clear the cache and start the search again
2157+
doc.documentElement.setAttribute("a", str(i))
2158+
2159+
def testGetElementByIdDuplicate(self):
2160+
body = ("<doc>"
2161+
"<e id='y' n='0'/>"
2162+
"<f><e id='x' n='1'/></f>"
2163+
"<e id='x' n='2'><e id='x' n='3'/></e>"
2164+
"<e id='z' n='4'/>"
2165+
"</doc>")
2166+
with self.subTest("ATTLIST"):
2167+
doc = parseString("<!DOCTYPE doc [<!ATTLIST e id ID #IMPLIED>]>" +
2168+
body)
2169+
self.check_duplicate_id(doc)
2170+
with self.subTest("setIdAttribute"):
2171+
doc = parseString(body)
2172+
for e in doc.getElementsByTagName("e"):
2173+
e.setIdAttribute("id")
2174+
self.check_duplicate_id(doc)
2175+
with self.subTest("ATTLIST and setIdAttribute"):
2176+
doc = parseString("<!DOCTYPE doc [<!ATTLIST e n CDATA #IMPLIED>]>" +
2177+
body)
2178+
for e in doc.getElementsByTagName("e"):
2179+
e.setIdAttribute("id")
2180+
self.check_duplicate_id(doc)
2181+
21462182
def assert_recursive_equal(self, doc, doc2):
21472183
stack = [(doc, doc2)]
21482184
while stack:

‎Lib/xml/dom/minidom.py‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2030,8 +2030,9 @@ def getElementById(self, id):
20302030
result = None
20312031
while stack:
20322032
node = stack.pop()
2033-
# add child elements to stack for continued searching
2034-
stack.extend([child for child in node.childNodes
2033+
# add child elements to stack for continued searching,
2034+
# last child first so that they are visited in document order
2035+
stack.extend([child for child in reversed(node.childNodes)
20352036
if child.nodeType in _nodeTypes_with_children])
20362037
# check this node
20372038
info = self._get_elem_info(node)
@@ -2042,27 +2043,27 @@ def getElementById(self, id):
20422043
for attr in node.attributes.values():
20432044
if attr.namespaceURI:
20442045
if info.isIdNS(attr.namespaceURI, attr.localName):
2045-
self._id_cache[attr.value] = node
2046+
self._id_cache.setdefault(attr.value, node)
20462047
if attr.value == id:
20472048
result = node
20482049
elif not node._magic_id_nodes:
20492050
break
20502051
elif info.isId(attr.name):
2051-
self._id_cache[attr.value] = node
2052+
self._id_cache.setdefault(attr.value, node)
20522053
if attr.value == id:
20532054
result = node
20542055
elif not node._magic_id_nodes:
20552056
break
20562057
elif attr._is_id:
2057-
self._id_cache[attr.value] = node
2058+
self._id_cache.setdefault(attr.value, node)
20582059
if attr.value == id:
20592060
result = node
20602061
elif node._magic_id_nodes == 1:
20612062
break
20622063
elif node._magic_id_nodes:
20632064
for attr in node.attributes.values():
20642065
if attr._is_id:
2065-
self._id_cache[attr.value] = node
2066+
self._id_cache.setdefault(attr.value, node)
20662067
if attr.value == id:
20672068
result = node
20682069
if result is not None:
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
Fix :meth:`~xml.dom.Document.getElementById` in :mod:`xml.dom.minidom` so
2+
that it returns the first element in document order when several elements
3+
have the same ID. Previously, the result could differ between calls on an
4+
unmodified document.

0 commit comments

Comments
 (0)