From d20cd17d55a6e1540d624979875ae4bd9e08dfef Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Fri, 21 Oct 2022 16:34:30 -0400 Subject: [PATCH 1/2] test: add coverage for namespaced attributes including a failing test for JRuby round-trip of an attribute --- test/xml/test_node_attributes.rb | 87 ++++++++++++++++++++++++-------- 1 file changed, 67 insertions(+), 20 deletions(-) diff --git a/test/xml/test_node_attributes.rb b/test/xml/test_node_attributes.rb index a974273fea..3a3e465c0b 100644 --- a/test/xml/test_node_attributes.rb +++ b/test/xml/test_node_attributes.rb @@ -5,19 +5,71 @@ module Nokogiri module XML class TestNodeAttributes < Nokogiri::TestCase - def test_attribute_with_ns - doc = Nokogiri::XML(<<-eoxml) - - - - eoxml - - node = doc.at("node") - - assert_equal( - "bar", - node.attribute_with_ns("foo", "http://tenderlovemaking.com/").value, - ) + let(:simple_xml_with_namespaces) { <<~XML } + + + + + XML + + describe "#attribute" do + it "returns an attribute that matches the local name" do + doc = Nokogiri.XML(simple_xml_with_namespaces) + node = doc.at_css("next") + refute_nil(attr = node.attribute("foo")) + + # NOTE: that we don't make any claim over _which_ attribute should be returned. + # this situation is ambiguous and we make no guarantees. + assert_equal("foo", attr.name) + end + + it "does not return an attribute that matches the full name" do + doc = Nokogiri.XML(simple_xml_with_namespaces) + node = doc.at_css("next") + assert_nil(node.attribute("tlm:foo")) + end + end + + describe "#attribute_with_ns" do + it "returns the attribute that matches the name and namespace" do + doc = Nokogiri.XML(simple_xml_with_namespaces) + node = doc.at_css("node") + + refute_nil(attr = node.attribute_with_ns("foo", "http://tenderlovemaking.com/")) + assert_equal("bar", attr.value) + end + + it "returns the attribute that matches the name and nil namespace" do + doc = Nokogiri.XML(simple_xml_with_namespaces) + node = doc.at_css("node") + + refute_nil(attr = node.attribute_with_ns("foo", nil)) + assert_equal("baz", attr.value) + end + + it "does not return a attribute that matches name but not namespace" do + doc = Nokogiri.XML(simple_xml_with_namespaces) + node = doc.at_css("node") + + assert_nil(node.attribute_with_ns("foo", "http://nokogiri.org/")) + end + + it "does not return a attribute that matches namespace but not name" do + doc = Nokogiri.XML(simple_xml_with_namespaces) + node = doc.at_css("node") + + assert_nil(node.attribute_with_ns("not-present", "http://tenderlovemaking.com/")) + end + end + + describe "#set_attribute" do + it "round trips" do + doc = Nokogiri.XML(simple_xml_with_namespaces) + node = doc.at_css("node") + node["xxx"] = "yyy" + refute_nil(node.attribute("xxx")) + assert_equal("yyy", node.attribute("xxx").value) + end end def test_prefixed_attributes @@ -84,13 +136,8 @@ def test_append_child_element_with_prefixed_attributes end def test_namespace_key? - doc = Nokogiri::XML(<<-eoxml) - - - - eoxml - - node = doc.at("node") + doc = Nokogiri.XML(simple_xml_with_namespaces) + node = doc.at_css("node") assert(node.namespaced_key?("foo", "http://tenderlovemaking.com/")) assert(node.namespaced_key?("foo", nil)) From c142785763274e62b836d9d6ac893aba548d4edc Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Fri, 21 Oct 2022 17:24:45 -0400 Subject: [PATCH 2/2] fix(jruby): Node#attribute correctly matches against localName Previously, it was matching against the full name. Also, make sure that we create new attributes with setAttributeNS (even if the URI is null) so that we can retrieve with `localName`. Note that we need additional logic to handle unknown namespaces. --- CHANGELOG.md | 1 + ext/java/nokogiri/XmlNode.java | 27 +++++++++++++++++++-------- 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4d723481f2..c6d7e39861 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,7 @@ Nokogiri follows [Semantic Versioning](https://semver.org/), please see the [REA * [CRuby] libgumbo (the HTML5 parser) treats reaching max-depth as EOF. This addresses a class of issues when the parser is interrupted in this way. [#3121] @stevecheckoway * [CRuby] Update node GC lifecycle to avoid a potential memory leak with fragments in libxml 2.13.0 caused by changes in `xmlAddChild`. [#3156] @flavorjones * [CRuby] libgumbo correctly prints nonstandard element names in error messages. [#3219] @stevecheckoway +* [JRuby] Fixed some bugs in how `Node#attributes` handles attributes with namespaces. [#2677, #2679] @flavorjones ### Changed diff --git a/ext/java/nokogiri/XmlNode.java b/ext/java/nokogiri/XmlNode.java index 4ecc957954..70440b5351 100644 --- a/ext/java/nokogiri/XmlNode.java +++ b/ext/java/nokogiri/XmlNode.java @@ -643,12 +643,22 @@ public class XmlNode extends RubyObject @JRubyMethod(name = {"attribute", "attr"}) public IRubyObject - attribute(ThreadContext context, IRubyObject name) + attribute(ThreadContext context, IRubyObject rbName) { - NamedNodeMap attrs = this.node.getAttributes(); - Node attr = attrs.getNamedItem(rubyStringToString(name)); - if (attr == null) { return context.nil; } - return getCachedNodeOrCreate(context.runtime, attr); + NamedNodeMap attributes = this.node.getAttributes(); + String name = rubyStringToString(rbName); + + for (int j = 0 ; j < attributes.getLength() ; j++) { + Node attribute = attributes.item(j); + String localName = attribute.getLocalName(); + if (localName == null) { + continue; + } + if (localName.equals(name)) { + return getCachedNodeOrCreate(context.runtime, attribute); + } + } + return context.nil; } @JRubyMethod @@ -1415,11 +1425,12 @@ public class XmlNode extends RubyObject } } - if (uri != null) { - element.setAttributeNS(uri, key, val); - } else { + if (colonIndex > 0 && uri == null) { element.setAttribute(key, val); + } else { + element.setAttributeNS(uri, key, val); } + clearXpathContext(node); }