feat: Reader#attribute_hash

which is now being used to implement Reader#attributes fix: Reader#namespaces on JRuby now returns an empty hash when no attributes are present (previously returned nil)
sparklemotion · Jul 21, 2022 · 12874a7 · 12874a7
1 parent 193a07d
commit 12874a7
Show file tree

Hide file tree

Showing 5 changed files with 101 additions and 17 deletions.
diff --git a/ext/java/nokogiri/XmlReader.java b/ext/java/nokogiri/XmlReader.java
@@ -144,6 +144,13 @@ public class XmlReader extends RubyObject
     return currentNode().getAttributesNodes();
   }
 
+  @JRubyMethod
+  public IRubyObject
+  attribute_hash(ThreadContext context)
+  {
+    return currentNode().getAttributes(context);
+  }
+
   @JRubyMethod(name = "attributes?")
   public IRubyObject
   attributes_p(ThreadContext context)

diff --git a/ext/java/nokogiri/internals/ReaderNode.java b/ext/java/nokogiri/internals/ReaderNode.java
@@ -112,9 +112,10 @@ public abstract class ReaderNode
   getAttributes(ThreadContext context)
   {
     final Ruby runtime = context.runtime;
-    if (attributeList == null) { return runtime.getNil(); }
     RubyHash hash = RubyHash.newHash(runtime);
+    if (attributeList == null) { return hash; }
     for (int i = 0; i < attributeList.length; i++) {
+      if (isNamespace(attributeList.names.get(i))) { continue; }
       IRubyObject k = stringOrBlank(runtime, attributeList.names.get(i));
       IRubyObject v = stringOrBlank(runtime, attributeList.values.get(i));
       hash.fastASetCheckString(runtime, k, v); // hash.op_aset(context, k, v)
@@ -150,8 +151,8 @@ public abstract class ReaderNode
   getNamespaces(ThreadContext context)
   {
     final Ruby runtime = context.runtime;
-    if (namespaces == null) { return runtime.getNil(); }
     RubyHash hash = RubyHash.newHash(runtime);
+    if (namespaces == null) { return hash; }
     for (Map.Entry<String, String> entry : namespaces.entrySet()) {
       IRubyObject k = stringOrBlank(runtime, entry.getKey());
       IRubyObject v = stringOrBlank(runtime, entry.getValue());

diff --git a/ext/nokogiri/xml_reader.c b/ext/nokogiri/xml_reader.c
@@ -31,6 +31,7 @@ has_attributes(xmlTextReaderPtr reader)
   return (0);
 }
 
+// TODO: merge this function into the `namespaces` method implementation
 static void
 Nokogiri_xml_node_namespaces(xmlNodePtr node, VALUE attr_hash)
 {
@@ -181,6 +182,47 @@ rb_xml_reader_attribute_nodes(VALUE rb_reader)
   return attr_nodes;
 }
 
+/*
+  :call-seq: attribute_hash() → Hash<String ⇒ String>
+
+  Get the attributes of the current node as a Hash of names and values.
+
+  See related: #attributes and #namespaces
+ */
+static VALUE
+rb_xml_reader_attribute_hash(VALUE rb_reader)
+{
+  VALUE rb_attributes = rb_hash_new();
+  xmlTextReaderPtr c_reader;
+  xmlNodePtr c_node;
+  xmlAttrPtr c_property;
+
+  Data_Get_Struct(rb_reader, xmlTextReader, c_reader);
+
+  if (!has_attributes(c_reader)) {
+    return rb_attributes;
+  }
+
+  c_node = xmlTextReaderExpand(c_reader);
+  c_property = c_node->properties;
+  while (c_property != NULL) {
+    VALUE rb_name = NOKOGIRI_STR_NEW2(c_property->name);
+    VALUE rb_value = Qnil;
+    xmlChar *c_value = xmlNodeGetContent((xmlNode *)c_property);
+
+    if (c_value) {
+      rb_value = NOKOGIRI_STR_NEW2(c_value);
+      xmlFree(c_value);
+    }
+
+    rb_hash_aset(rb_attributes, rb_name, rb_value);
+
+    c_property = c_property->next;
+  }
+
+  return rb_attributes;
+}
+
 /*
  * call-seq:
  *   attribute_at(index)
@@ -696,6 +738,7 @@ noko_init_xml_reader()
   rb_define_method(cNokogiriXmlReader, "attribute_at", attribute_at, 1);
   rb_define_method(cNokogiriXmlReader, "attribute_count", attribute_count, 0);
   rb_define_method(cNokogiriXmlReader, "attribute_nodes", rb_xml_reader_attribute_nodes, 0);
+  rb_define_method(cNokogiriXmlReader, "attribute_hash", rb_xml_reader_attribute_hash, 0);
   rb_define_method(cNokogiriXmlReader, "attributes?", attributes_eh, 0);
   rb_define_method(cNokogiriXmlReader, "base_uri", rb_xml_reader_base_uri, 0);
   rb_define_method(cNokogiriXmlReader, "default?", default_eh, 0);

diff --git a/lib/nokogiri/xml/reader.rb b/lib/nokogiri/xml/reader.rb
@@ -87,12 +87,7 @@ def initialize(source, url = nil, encoding = nil) # :nodoc:
       #
       # [Returns] (Hash<String, String>) Attribute names and values
       def attributes
-        attrs_hash = attribute_nodes.each_with_object({}) do |node, hash|
-          hash[node.name] = node.to_s
-        end
-        ns = namespaces
-        attrs_hash.merge!(ns) if ns
-        attrs_hash
+        attribute_hash.merge(namespaces)
       end
 
       ###

diff --git a/test/xml/test_reader.rb b/test/xml/test_reader.rb
@@ -228,23 +228,61 @@ def test_attributes?
           reader.map(&:attributes?))
       end
 
-      def test_attributes
-        reader = Nokogiri::XML::Reader.from_memory(<<-eoxml)
-        <x xmlns:tenderlove='http://tenderlovemaking.com/'
-           xmlns='http://mothership.connection.com/'
-           >
-          <tenderlove:foo awesome='true'>snuggles!</tenderlove:foo>
-        </x>
-        eoxml
+      def test_reader_attributes
+        reader = Nokogiri::XML::Reader.from_memory(<<~XML)
+          <x xmlns:tenderlove='http://tenderlovemaking.com/' xmlns='http://mothership.connection.com/'>
+            <tenderlove:foo awesome='true'>snuggles!</tenderlove:foo>
+          </x>
+        XML
         assert_empty(reader.attributes)
         assert_equal([{ "xmlns:tenderlove" => "http://tenderlovemaking.com/",
                         "xmlns" => "http://mothership.connection.com/", },
-                      {}, { "awesome" => "true" }, {}, { "awesome" => "true" }, {},
+                      {},
+                      { "awesome" => "true" },
+                      {},
+                      { "awesome" => "true" },
+                      {},
                       { "xmlns:tenderlove" => "http://tenderlovemaking.com/",
                         "xmlns" => "http://mothership.connection.com/", },],
           reader.map(&:attributes))
       end
 
+      def test_reader_attributes_hash
+        reader = Nokogiri::XML::Reader.from_memory(<<~XML)
+          <x xmlns:tenderlove='http://tenderlovemaking.com/' xmlns='http://mothership.connection.com/'>
+            <tenderlove:foo awesome='true'>snuggles!</tenderlove:foo>
+          </x>
+        XML
+        assert_empty(reader.attribute_hash)
+        assert_equal([{},
+                      {},
+                      { "awesome" => "true" },
+                      {},
+                      { "awesome" => "true" },
+                      {},
+                      {},],
+          reader.map(&:attribute_hash))
+      end
+
+      def test_reader_namespaces
+        reader = Nokogiri::XML::Reader.from_memory(<<~XML)
+          <x xmlns:tenderlove='http://tenderlovemaking.com/' xmlns='http://mothership.connection.com/'>
+            <tenderlove:foo awesome='true'>snuggles!</tenderlove:foo>
+          </x>
+        XML
+        assert_empty(reader.namespaces)
+        assert_equal([{ "xmlns:tenderlove" => "http://tenderlovemaking.com/",
+                        "xmlns" => "http://mothership.connection.com/", },
+                      {},
+                      {},
+                      {},
+                      {},
+                      {},
+                      { "xmlns:tenderlove" => "http://tenderlovemaking.com/",
+                        "xmlns" => "http://mothership.connection.com/", },],
+          reader.map(&:namespaces))
+      end
+
       def test_attribute_roundtrip
         reader = Nokogiri::XML::Reader.from_memory(<<-eoxml)
         <x xmlns:tenderlove='http://tenderlovemaking.com/'