(jruby) namespaced attributes (#2679)

**What problem is this PR intended to solve?**

#2677 pointed out inconsistent behavior in the JRuby implementation with
respect to namespaced attributes.

This may be an incomplete fix, but it's good enough to ship.

Closes #2677 

**Have you included adequate test coverage?**

Yes


**Does this change affect the behavior of either the C or the Java
implementations?**

This PR fixes the JRuby behavior to match the CRuby behavior.

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

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);
   }
 

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)
-          <root xmlns:tlm='http://tenderlovemaking.com/'>
-            <node tlm:foo='bar' foo='baz' />
-          </root>
-        eoxml
-
-        node = doc.at("node")
-
-        assert_equal(
-          "bar",
-          node.attribute_with_ns("foo", "http://tenderlovemaking.com/").value,
-        )
+      let(:simple_xml_with_namespaces) { <<~XML }
+        <root xmlns:tlm='http://tenderlovemaking.com/'>
+          <node tlm:foo='bar' foo='baz' />
+          <next tlm:foo='baz' />
+        </root>
+      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)
-          <root xmlns:tlm='http://tenderlovemaking.com/'>
-            <node tlm:foo='bar' foo='baz' />
-          </root>
-        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))