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