Fix Element#attribute when a namespace URI is bound to multiple prefixes - #357
Fix Element#attribute when a namespace URI is bound to multiple prefixes#357sjh9714 wants to merge 2 commits into
Conversation
Element#attribute used namespaces.key(namespace), which returns only the first prefix bound to that URI, so an attribute using any other prefix for the same URI was not found. Try every prefix bound to the URI instead, keeping the unprefixed attribute preferred when the URI is the default namespace.
There was a problem hiding this comment.
Pull request overview
This pull request fixes REXML::Element#attribute lookups when the same namespace URI is bound to multiple prefixes, ensuring attributes using any of those prefixes can be found (regression noted in #346, with context from prior refactor in #335).
Changes:
- Update
Element#attributeto try all prefixes bound to the requested namespace URI (while preserving the existing “default namespace prefers unprefixed attribute” behavior). - Add a regression test covering duplicated namespace-URI bindings and expected attribute resolution.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lib/rexml/element.rb | Adjusts Element#attribute to consider multiple prefixes mapped to the same namespace URI. |
| test/test_element.rb | Adds a regression test reproducing the duplicated-URI/prefix lookup failure from #346. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| return ret_val unless ret_val.nil? | ||
| return nil if prefix.nil? | ||
| # An unprefixed attribute is used for the default namespace. |
| @@ -1269,20 +1269,22 @@ def [](name_or_index) | |||
| # document.root.attribute("x", "a") # => a:x='a:x' | |||
| # | |||
| def attribute( name, namespace=nil ) | |||
There was a problem hiding this comment.
For this XML:
root1 = REXML::Document.new("<root xmlns:ns1='url1' xmlns='url1' a='A' ns1:a='NS1A'/>").root
root2 = REXML::Document.new("<root a='A' xmlns:ns1='url1' xmlns='url1' ns1:a='NS1A'/>").root
root3 = REXML::Document.new("<root xmlns:ns1='url1' xmlns='url1' ns1:a='NS1A' a='A'/>").root
root4 = REXML::Document.new("<root xmlns='url1' xmlns:ns1='url1' ns1:a='NS1A' a='A'/>").rootroot.attributes.get_attribute_ns('url1', 'a').expanded_name always returns "ns1:a".
But attribute('a', 'url1') is different.
[root1, root2, root3, root4].map do |root|
root.attribute('a', 'url1').expanded_name
end
# Master: ["ns1:a", "ns1:a", "ns1:a", "a"]
# (the result depends on the declaration order of xmlns / xmlns:ns1 — this is a bug)
# This PR: ["a", "a", "a", "a"]The behavior is changed, and it is changed to the opposite way from get_attribute_ns. Note that the PR description says "No lookup that previously returned an attribute returns a different one", but root1–root3 are counterexamples. It's worth describing this change in the PR description.
Though the expected behavior should be decided by the maintainer (@naitoh), I think it may be nice to align with get_attribute_ns.
If we're going to mainly align with get_attribute_ns, how about first calling get_attribute_ns, and falling back to a custom logic if nothing is found? The compatibility-oriented loose behaviors (matching an unprefixed attribute for the default namespace or for an undeclared namespace URI) can live in the fallback, and it would make it much easier to see how this method differs from get_attribute_ns.
There was a problem hiding this comment.
-
Since Handle unprefixed attributes in
Element#attributecorrectly #151 caused incompatibility with existing test code, we prioritized compatibility by only adding documentation in Document how Element#attribute matches a namespace #360; however, ac3e8b8 does not affect existing test code. -
Also,
root1,root2,root3, androot4should be fixed so that they return the same result. The response forns1:ain ac3e8b8 (which is the same as get_attribute_ns) seems reasonable—what do you think?
Per review, look up the attribute via get_attribute_ns first, so a prefixed attribute wins regardless of xmlns declaration order, and keep the compatibility-oriented unprefixed fallback (default namespace or an undeclared namespace URI) in one place.
|
Restructured in ac3e8b8 as you suggested. Your four examples now all return On the description's compatibility claim, you're right, root1-root3 were counterexamples. With this revision the remaining difference from master is narrower. A lookup can change only when the requested URI is the default namespace and both an unprefixed and a prefixed attribute are present. Master returned one or the other depending on xmlns declaration order, and this branch now always returns the prefixed one, like Ran |
| @@ -1269,20 +1269,22 @@ def [](name_or_index) | |||
| # document.root.attribute("x", "a") # => a:x='a:x' | |||
| # | |||
| def attribute( name, namespace=nil ) | |||
|
Updated the PR description to match |
Fixes #346.
Motivation
Element#attributecan miss a namespaced attribute when the same namespace URI is bound to multiple prefixes. Lookups with both unprefixed and prefixed attributes could also depend on declaration order.Attributes#get_attribute_nsresolves the namespaced match consistently. This change makesElement#attributeuse that result first.Changes
get_attributelookup when no namespace is supplied.get_attribute_ns(namespace, name)first.A matching result from
get_attribute_nsnow wins when both prefixed and unprefixed attributes exist.Tests
799 tests, 0 failures.