Deprecate accepting array as an element in XPath.match, first and each

[tompng]

XPath.match, XPath.first, XPath.each, XPathParser#parse and XPathParser#match accepted nodeset as element.
This pull request changes the first parameter of these method to be an element instead of nodeset.
Passing nodeset will be deprecated.

# Documented usage. OK
REXML::XPath.match(element, xpath)

# Undocumented usage. Deprecate in this pull request
nodeset = [element]
REXML::XPath.match(nodeset, xpath)

Background

#249 will introduce a temporary cache.

def parse path, nodeset
  path_stack = @parser.parse( path )
  nodeset.first.document.send(:enable_cache) do
    match( path_stack, nodeset )
  end
end

But the signature XPathParser#match(path, nodeset) does not guarantee that all nodes in the nodeset has the same root document.
So cache does not work in the code below. It's still slow.

REXML::XPath.match(2.times.map { REXML::Document.new('<a>'*400+'</a>'*400) }, 'a//a')

The interface is holding our back, so I propose to drop accepting array as element.
This change is a backward incompatibility, but it just drops undocumented feature. I think only the test code was unintentionally using this feature.

XPath.match with array

XPath.match only traverse the first element of the array for some selectors.

nodeset = [REXML::Document.new("<a><b/></a>"), REXML::Document.new("<a><c/></a>")]

REXML::XPath.match(nodeset, "a/*")
#=> [<b/>, <c/>]

REXML::XPath.match(nodeset, "//a/*")
#=> [<b/>] # I expect [<b/>, <c/>] but the second document is ignored

It indicates that XPath.match is not designed to search inside multiple nodes/documents.

[naitoh]

But the signature XPathParser#match(path, nodeset) does not guarantee that all nodes in the nodeset has the same root document. So cache does not work in the code below. It's still slow.

In this case, I made further improvements to #249, which eliminated the slowness.

Drop accepting array as an element in XPath.match, first and each

How about instead of removing a feature, a deprecated message should be displayed if it is specified in an array?

What do you think @kou?

[kou]

I'm OK that we drop support for nodeset because we don't have enough resource to complete nodeset support.

In general, we want to keep backward compatibility as much as possible. But we can remove the feature without keeping backward compatibility because:

1. It's not documented
2. It doesn't work in some cases

But could you report a warning as @naitoh suggested something like:

diff --git a/lib/rexml/xpath_parser.rb b/lib/rexml/xpath_parser.rb
index 5eb1e5a..a2b2ef5 100644
--- a/lib/rexml/xpath_parser.rb
+++ b/lib/rexml/xpath_parser.rb
@@ -136,11 +136,12 @@ module REXML
     end
 
 
-    def match(path_stack, nodeset)
-      nodeset = nodeset.collect.with_index do |node, i|
-        position = i + 1
-        XPathNode.new(node, position: position)
+    def match(path_stack, node)
+      if node.is_a?(Array)
+        warn("REXML::XPath.XXX dropped support for nodeset...", uplevel: N)
+        node = node.first
       end
+      nodeset = [XPathNode.new(node, position: 1)]
       result = expr(path_stack, nodeset)
       case result
       when Array # nodeset

[naitoh]

LGTM
Thanks!

[naitoh]

@tompng
Can you update this PR's description?

[tompng]

@naitoh
Updated. Change the title Drop → Deprecate and mention about deprecation in the description

[naitoh]

Thanks!

[voxik]

Just FTR, this likely breaks vagrant-libvirt:

https://github.com/vagrant-libvirt/vagrant-libvirt/blob/a94ce0d7b6c90129a38435698ca97b364130313d/lib/vagrant-libvirt/action/resolve_disk_settings.rb#L53-L62

I have no idea ATM how to fix this :/

[naitoh]

@voxik

Just FTR, this likely breaks vagrant-libvirt:

I have no idea ATM how to fix this :/

We apologize for causing incompatibility with vagrant-libvirt.
To avoid the impact of this change, please implement the following countermeasures at the location where the message REXML::XPath.each, REXML::XPath.first, REXML::XPath.match dropped support for nodeset... appears.

• Please specify a single node, not an array of nodes, for REXML::XPath.match.
• When specifying an array of nodes and wanting to receive results for multiple nodes, please specify a single node using a loop with each.

https://github.com/vagrant-libvirt/vagrant-libvirt/blob/a94ce0d7b6c90129a38435698ca97b364130313d/lib/vagrant-libvirt/action/resolve_disk_settings.rb#L53-L62

Fixed example:

$ git diff -ub
diff --git a/lib/vagrant-libvirt/action/resolve_disk_settings.rb b/lib/vagrant-libvirt/action/resolve_disk_settings.rb
index f1a3f67..bf5d951 100644
--- a/lib/vagrant-libvirt/action/resolve_disk_settings.rb
+++ b/lib/vagrant-libvirt/action/resolve_disk_settings.rb
@@ -51,14 +51,16 @@ module VagrantPlugins
               xml_descr = REXML::Document.new(domain_xml)
               domain_name = xml_descr.elements['domain'].elements['name'].text
               disks_xml = REXML::XPath.match(xml_descr, '/domain/devices/disk[@device="disk"]')
-              have_aliases = !REXML::XPath.match(disks_xml, './alias[@name="ua-box-volume-0"]').first.nil?
+              have_aliases = !REXML::XPath.match(disks_xml.first, './alias[@name="ua-box-volume-0"]').first.nil?
               env[:ui].warn(I18n.t('vagrant_libvirt.domain_xml.obsolete_method')) unless have_aliases
 
               if have_aliases
-                REXML::XPath.match(disks_xml,
+                disks_xml.each do |disk_xml|
+                  REXML::XPath.match(disk_xml,
                                      './alias[contains(@name, "ua-box-volume-")]').each_with_index do |alias_xml, idx|
                     domain_volumes.push(volume_from_xml(alias_xml.parent, domain_name, idx))
                   end
+                end
               else
                 # fallback to try and infer which boxes are box images, as they are listed first
                 # as soon as there is no match, can exit

[voxik]

Fixed example:

Thanks, very useful to see real life example from experts 👍

Since I have proposed slightly different fix in vagrant-libvirt/vagrant-libvirt#1861 would you mind to check it and elaborate there what are the pros and cons of your vs my proposal?

[naitoh]

@voxik

Since I have proposed slightly different fix in vagrant-libvirt/vagrant-libvirt#1861 would you mind to check it and elaborate there what are the pros and cons of your vs my proposal?

Fixed example:

• pros

The existing logic remains unchanged, so there is no impact on performance.

• cons

Nothing.

vagrant-libvirt/vagrant-libvirt#1861

• pros

XPath specifies elements starting from the top of the XML document, making the processing easier to understand.

• cons

Each time, since we specify the XPath from the top of the XML, the number of processing steps increases.
Therefore, when the target XML is large, there may be a performance impact.

[voxik]

Each time, since we specify the XPath from the top of the XML, the number of processing steps increases. Therefore, when the target XML is large, there may be a performance impact.

That was also my worry, right. Thanks for confirming.

On top of that, I think your proposal uncovers flaw in the logic which was earlier elaborated in the original description: "XPath.match only traverse the first element of the array for some selectors.", because I does not seem to me it was intention to look only at first "disk". But I am not an author of the code, so I'll leave the decision to the upstream.

Thx for the suggestion and elaborating 👍 I appreciate that.