Fix xpath functions related to names

[tompng]

Fix and simplify name(nodesets), local-name(nodesets) and namespace-uri(nodesets) node select logic. All functions that uses a single node should use the first document-ordered node, but these functions were wrongly skipping un-named nodes.

xml = '<root>text<!-- comment --><node/></root>'
xpath = 'name(root/node())'

REXML::XPath.match(REXML::Document.new(xml), xpath)
#=> ["node"] (bug) → [""]
Nokogiri::XML.parse(xml).xpath(xpath)
#=> "" (expected)

[Copilot]

Pull request overview

This PR fixes XPath name-related functions (name(), local-name(), namespace-uri()) so they correctly use the first document-ordered node (even if it’s not a named node), rather than skipping unnamed nodes and selecting a later named node.

Changes:

• Refactors local_name, name, and namespace_uri to share a simplified “pick first document-ordered node” helper.
• Updates and expands test_local_name to avoid whitespace-text-node sensitivity and to cover unnamed-node behavior.
• Adds new test cases for attribute and non-named node inputs.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File	Description
lib/rexml/functions.rb	Simplifies node selection logic for name-related XPath functions via a new helper method.
test/functions/test_local_name.rb	Adjusts existing node-set construction and adds new cases to validate the corrected behavior.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[naitoh]

There is regression.

• before(master)

> doc = REXML::Document.new("<root xmlns:x='http://example.com/x/'><x:child/></root>")
> REXML::Functions.local_name(doc.root.elements[1]) #=> "child"
> REXML::Functions.local_name("not a node")  #=> ""

• after(this PR)

> doc = REXML::Document.new("<root xmlns:x='http://example.com/x/'><x:child/></root>")
> REXML::Functions.local_name(doc.root.elements[1]) #=> ""
> REXML::Functions.local_name("not a node")  #=> ""

Suggested change

	when nil
	node = @context[:node]
	when Array
	node = XPathParser.sort(node_set).first
	end
	when nil
	node = @context[:node]
	when Array
	node = XPathParser.sort(node_set).first
	else
	node = node_set
	end

[tompng]

Is it really a regression? I think it's a dead branch which should be cleaned up.
XPath values are: boolean, string, number, and nodeset. there is no non-nodeset-single-node.

[tompng]

If REXML::Functions.local_name(node) #=> node.local_name is considered a public API, it is better to not change the behavior. What do you think?

[naitoh]

REXML::Functions.local_name(node) #=> node.local_name is not part of the public API.

This change is not a regression.
I'm sorry.

https://docs.ruby-lang.org/ja/latest/class/REXML=3a=3aFunctions.html

内部用なのでユーザは使わないでください。

(I'm sorry for writing in Japanese.)

[naitoh]

I rewrote the code so that it doesn't use REXML::Functions.local_name(node).

• before(rexml 3.4.4)

> doc = REXML::Document.new("<root xmlns:x='http://example.com/x/'><x:child/></root>")
> child = doc.root.elements[1] #=> <x:child/>
> REXML::XPath.match(doc, "local-name($x)", nil, { "x" => child }) #=> ["child"]
> REXML::XPath.match(doc, "local-name('not a node')") #=> [""]

• after(this PR)

> doc = REXML::Document.new("<root xmlns:x='http://example.com/x/'><x:child/></root>")
> child = doc.root.elements[1] #=> <x:child/>
> REXML::XPath.match(doc, "local-name($x)", nil, { "x" => child }) #=> [""]
> REXML::XPath.match(doc, "local-name('not a node')") #=> [""]

XPath values are: boolean, string, number, and nodeset. there is no non-nodeset-single-node.

However, I agree that “single nodes do not need to be considered.”

[tompng]

The variable in the code below is invalid for now.

REXML::XPath.match(doc, "local-name($x)", nil, { "x" => child })

Example:

REXML::XPath.match(doc, "($x)/root", nil, { "x" => [doc] })
# => [<root xmlns:x='http://example.com/x/'> ... </>]
REXML::XPath.match(doc, "($x)/root", nil, { "x" => doc })
# => [<UNDEFINED> ... </>]

It will be valid in #342 (comment)

[naitoh]

It will be valid in #342 (comment)

After rebasing, I confirmed that it works as expected.
Thank you.

[naitoh]

@tompng
Could you please rebase this PR?

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

lib/rexml/functions.rb:75

• The PR changes name()/namespace-uri() selection semantics via target_named_node, but the added regression tests only cover local-name(). There’s currently no test that exercises name(node-set) and namespace-uri(node-set) when the first document-ordered node is non-named (e.g., node() returning text/comment before an element), which is the class of bug described in the PR.

Add tests similar to test_non_named that assert REXML::XPath.match(doc, 'name(root/node())') == [''] and namespace-uri(root/node()) == [''] (and an attribute case for namespace-uri()).

    def name( node_set=nil )
      target_named_node(node_set)&.expanded_name || ""
    end

[tompng]

Rebase done.
Applied the copilot suggession

- node_set = [document.root.attributes.to_a.last]
+ node_set = [document.root.attributes.get_attribute("x:attr")]

Other copilot suggestion: ignored.
test_local_name.rb should only contain local_name test unless the file is renamed.
It is actually testing common part of name/local-name/namespace: target node selection, so the basic part for other functions are already tested here, and slightly tested in other existing test (e.g. XPath tests).

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (2)

test/functions/test_local_name.rb:41

• This test only asserts local_name, but this PR also changes name() and namespace_uri() to use the first document-ordered node (including non-named nodes). Adding assertions here for attribute nodes would help prevent regressions in the other two functions.

      document = REXML::Document.new("<root xmlns:x='http://example.com/x/' x:attr='value' />")
      node_set = [document.root.attributes.get_attribute("x:attr")]
      assert_equal("attr", REXML::Functions.local_name(node_set))

test/functions/test_local_name.rb:49

• test_non_named validates local_name for text/comment nodes, but the same selection logic is now shared by name() and namespace_uri(). Adding assertions for those functions here would better cover the intended bug fix across all three functions.

      document = REXML::Document.new("<root>text<!-- comment --><a/></root>")
      children = document.root.children
      assert_equal("", REXML::Functions.local_name([children[0]]))
      assert_equal("", REXML::Functions.local_name([children[1]]))
      assert_equal("", REXML::Functions.local_name(children))

[naitoh]

Thanks!