Fix document order in REXML::XPathParser.sort (#356)

`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:

```
    <!--c1--><?pi1?><root/><!--c2--><?pi2?>
```

    //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:

```
    <root><a x="1" y="2"><b/></a></root>
```

    //a/@* | //a | //a/*
      was: @x, @y, <a>, <b>   want: <a>, @x, @y, <b>

Extend an attribute's key past its element's with ATTRIBUTE_POSITION. A
child index is never negative, so -1 lands the attributes after `<a>`
and ahead of `<b>`, 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="1" m="2" b="3"/>       //a/@*  ->  z, m, b
    <a z="1" ... 26 attributes/> //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.

Author: naitoh

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)

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
       <a id='1'>
@@ -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("<!--c1--><?pi1?><root/><!--c2--><?pi2?>")
+      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("<!--c1--><root><a/></root><!--c2-->")
+      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("<root><a x='1' y='2'/></root>")
+      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("<root><a x='1' y='2'/></root>")
+      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("<root><a #{attributes}/></root>")
+      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("<root><a #{attributes}/></root>")
+      }
+      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("<root><a id='1'/><a id='2'/><a id='3'/></root>")
+      assert_equal(["1", "2", "3"], XPath.match(doc, "//a/@id").collect(&:value))
+    end
+
+    def test_document_order_mixed_text_and_element_children
+      source = <<-XML
+<root>
+  <a>before0<b/>after0</a>
+  <a>before1<c/>after1</a>
+</root>
+      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("<root/>")
       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