Skip to content

Commit 3f5d710

Browse files
committed
gh-161110: Make minidom getElementById() stable for duplicate IDs
Visit child nodes in document order and keep the first cached element for each ID, so the first element in document order is returned.
1 parent 9c0cfb1 commit 3f5d710

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)