From aa279725f95ef3ee7ede8ece66e9ad49ccda004f Mon Sep 17 00:00:00 2001 From: NAITOH Jun Date: Fri, 14 Aug 2026 22:37:49 +0900 Subject: [PATCH] Fix document order in REXML::XPathParser.sort REXML::XPathParser.sort keys each node on its index under each ancestor, but two different nodes could end up with the same key, and ties are then broken arbitrarily by an unstable sort. The walk stopped at the root element, so the root and everything outside it -- comments and PIs at document level -- all keyed on an empty array: //comment() | //processing-instruction() was: c1, c2, pi1, pi2 want: c1, pi1, c2, pi2 Walk up to the document instead, so those nodes are ordered against each other. An attribute borrowed the key of the element carrying it, so the element and its attributes tied: //a/@* | //a | //a/* was: @x, @y, , want: , @x, @y, Extend an attribute's key past its element's with ATTRIBUTE_POSITION. A child index is never negative, so -1 lands the attributes after and ahead of , which is where document order wants them. The attributes of one element tied with each other too, which went unnoticed because a small enough sort leaves its input alone: //a/@* -> z, m, b //a/@* -> scrambled XPath 1.0 leaves their relative order implementation dependent, but document order is a total ordering, so it has to be decided. Key them on where they were written, which is the order the small case already appeared to have. Finding that out means walking the attribute list, so index the whole list at once and remember it for the rest of the sort; asking per attribute would make sorting quadratic in the number of attributes an element carries, which is something a document gets to choose. The ancestor walk becomes a method of its own, since both branches of the key need it. It, attribute_position and ATTRIBUTE_POSITION are private rather than public names marked :nodoc:, sort being the only part callers outside the class use. --- lib/rexml/xpath_parser.rb | 60 ++++++++++++++++++------ test/xpath/test_base.rb | 96 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 143 insertions(+), 13 deletions(-) diff --git a/lib/rexml/xpath_parser.rb b/lib/rexml/xpath_parser.rb index 43571624..de8d41e3 100644 --- a/lib/rexml/xpath_parser.rb +++ b/lib/rexml/xpath_parser.rb @@ -804,6 +804,11 @@ def leave(tag, *args) trace(:leave, tag, *args) end + # Sorts before any real child index, so that the attributes of an element + # come after the element itself but before its children. + ATTRIBUTE_POSITION = -1 + private_constant :ATTRIBUTE_POSITION + # Reorders an array of nodes so that they are in document order # It tries to do this efficiently. # @@ -815,23 +820,52 @@ def leave(tag, *args) def self.sort(array_of_nodes) return array_of_nodes if array_of_nodes.size <= 1 - new_arry = [] - array_of_nodes.each { |node| - node_idx = [] - np = node.node_type == :attribute ? node.element : node - while np.parent and np.parent.node_type == :element - node_idx << np.parent.index( np ) - np = np.parent + attribute_positions = {}.compare_by_identity + array_of_nodes.sort_by do |node| + if node.node_type == :attribute + # An attribute has no place of its own in the child tree, so its key + # extends that of the element carrying it. + ancestor_indexes(node.element) << + ATTRIBUTE_POSITION << attribute_position(node, attribute_positions) + else + ancestor_indexes(node) end - new_arry << [ node_idx.reverse, node ] - } - ordered = new_arry.sort_by do |index, node| - index end - ordered.collect do |_index, node| - node + end + + # The index the node holds under each of its ancestors, outermost first. + def self.ancestor_indexes(node) + indexes = [] + # Walk all the way up to the document. Stopping at the root element + # would leave every node outside it, and the root itself, with the same + # empty key, and ties are then broken arbitrarily. + while (parent = node.parent) + indexes << parent.index(node) + node = parent + end + indexes.reverse! + end + private_class_method :ancestor_indexes + + # Where the attribute sits among the attributes of its element. XPath 1.0 + # leaves the relative order of those implementation dependent, but document + # order is a total ordering, so they do need one; this keeps them in the + # order they were written in. + def self.attribute_position(attribute, positions) + position = positions[attribute] + return position if position + + # Index the whole attribute list at once. A node set often holds every + # attribute of an element, and looking each one up on its own would make + # sorting quadratic in the number of attributes. + i = 0 + attribute.element.attributes.each_attribute do |other| + positions[other] ||= i + i += 1 end + positions[attribute] end + private_class_method :attribute_position # Scanner for descendant-or-self axis def descendant_or_self(nodeset, tester, selector) diff --git a/test/xpath/test_base.rb b/test/xpath/test_base.rb index 02b20928..a6e9655b 100644 --- a/test/xpath/test_base.rb +++ b/test/xpath/test_base.rb @@ -1,8 +1,11 @@ # frozen_string_literal: false +require "core_assertions" + module REXMLTests class TestXPathBase < Test::Unit::TestCase include Helper::Fixture + include Test::Unit::CoreAssertions include REXML SOURCE = <<-EOF @@ -1564,6 +1567,83 @@ def test_reverse_axis_function_argument_sort assert_equal(["e"], XPath.match(doc, "//e[preceding-sibling::* = '1']").map(&:name)) end + def test_document_order_top_level_nodes + # Nodes outside the root element are still ordered against each other. + doc = Document.new("") + nodes = XPath.match(doc, "//comment() | //processing-instruction()") + assert_equal(["c1", "pi1", "c2", "pi2"], stringify_nodes(nodes)) + end + + def test_document_order_descendant_or_self_from_document + doc = Document.new("") + nodes = XPath.match(doc, "/descendant-or-self::node()") + assert_equal(["DOC", "c1", "root", "a", "c2"], stringify_nodes(nodes)) + end + + # The relative order of attributes of one element is implementation + # dependent, but they must all come after the element that carries them, + # whichever way round the union is written. + def test_document_order_element_precedes_its_attributes + doc = Document.new("") + nodes = XPath.match(doc, "//a | //a/@*") + assert_equal("a", nodes.first.name) + assert_equal(["x", "y"], nodes[1..-1].collect(&:name).sort) + end + + def test_document_order_element_precedes_its_attributes_reversed_union + doc = Document.new("") + nodes = XPath.match(doc, "//a/@* | //a") + assert_equal("a", nodes.first.name) + assert_equal(["x", "y"], nodes[1..-1].collect(&:name).sort) + end + + def test_document_order_attributes_of_one_element + # XPath 1.0 leaves the relative order of the attributes of one element + # implementation dependent, but document order is a total ordering, so it + # still has to be decided: they come out in the order they were written. + names = ("a".."z").to_a.reverse + # Give each attribute its own value, so that string(), which takes the + # first node in document order, tells the order apart too. + attributes = names.collect {|name| "#{name}='#{name}'" }.join(" ") + doc = Document.new("") + assert_equal(names, XPath.match(doc, "//a/@*").collect(&:name)) + assert_equal("z", XPath.match(doc, "string(//a/@*)").first) + end + + def test_linear_performance_sort_attributes_of_one_element + # Ordering the attributes of an element must not cost more than the + # attributes themselves: one whole list is indexed per element, not one + # list per attribute. + omit("This is fragile on JRuby") if RUBY_ENGINE == "jruby" + seq = [1000, 5000, 10000, 20000, 40000] + build = ->(n) { + attributes = n.times.collect {|i| "a#{i}='1'" }.join(" ") + Document.new("") + } + assert_linear_performance(seq, rehearsal: 10, pre: build) do |doc| + XPath.match(doc, "//a/@*") + end + end + + def test_document_order_attribute_axis_across_elements + # Attributes of different elements are ordered by their owning elements. + doc = Document.new("") + assert_equal(["1", "2", "3"], XPath.match(doc, "//a/@id").collect(&:value)) + end + + def test_document_order_mixed_text_and_element_children + source = <<-XML + + before0after0 + before1after1 + + XML + doc = Document.new(source) + nodes = XPath.match(doc, "//a/node()") + assert_equal(["before0", "b", "after0", "before1", "c", "after1"], + stringify_nodes(nodes)) + end + def test_unimplemented_id_should_not_contaminate_nil doc = Document.new("") assert_equal([], XPath.match(doc, 'id("foo")')) @@ -1607,5 +1687,21 @@ def test_variables_invalid_predicates actual = (XPath.match(doc, '($x)[1<2]', nil, { 'x' => 42 }) rescue :exception) assert_includes(valid_result, actual) end + + private + + # Stringifies each node of a node set, whichever kind of node it is, so + # that document order can be asserted on a set that mixes them. + def stringify_nodes(nodes) + nodes.collect do |node| + case node.node_type + when :document then "DOC" + when :comment then node.string + when :processing_instruction then node.target + when :text then node.value + else node.name + end + end + end end end