deprecate: Reader#attribute_nodes

This method will be removed in a future version
sparklemotion · Jul 21, 2022 · 80e888c · 80e888c
1 parent 8d0c4ea
commit 80e888c
Show file tree

Hide file tree

Showing 4 changed files with 31 additions and 12 deletions.
diff --git a/ext/java/nokogiri/XmlReader.java b/ext/java/nokogiri/XmlReader.java
@@ -141,6 +141,7 @@ public class XmlReader extends RubyObject
   public IRubyObject
   attribute_nodes(ThreadContext context)
   {
+    context.runtime.getWarnings().warn("Reader#attribute_nodes is deprecated and will be removed in a future version of Nokogiri. Please use Reader#attribute_hash instead.");
     return currentNode().getAttributesNodes();
   }
 

diff --git a/ext/nokogiri/xml_reader.c b/ext/nokogiri/xml_reader.c
@@ -151,7 +151,11 @@ namespaces(VALUE self)
 /*
   :call-seq: attribute_nodes() → Array<Nokogiri::XML::Attr>
 
-  Get the attributes of the current node as an Array of Attr
+  Get the attributes of the current node as an Array of XML:Attr
+
+  ⚠ This method is deprecated and unsafe to use. It will be removed in a future version of Nokogiri.
+
+  See related: #attribute_hash, #attributes
  */
 static VALUE
 rb_xml_reader_attribute_nodes(VALUE rb_reader)
@@ -161,6 +165,10 @@ rb_xml_reader_attribute_nodes(VALUE rb_reader)
   VALUE attr_nodes;
   int j;
 
+  // TODO: deprecated, remove in Nokogiri v1.15, see https://github.com/sparklemotion/nokogiri/issues/2598
+  // After removal, we can also remove all the "node_has_a_document" special handling from xml_node.c
+  NOKO_WARN_DEPRECATION("Reader#attribute_nodes is deprecated and will be removed in a future version of Nokogiri. Please use Reader#attribute_hash instead.");
+
   Data_Get_Struct(rb_reader, xmlTextReader, c_reader);
 
   if (! has_attributes(c_reader)) {

diff --git a/lib/nokogiri/xml/reader.rb b/lib/nokogiri/xml/reader.rb
@@ -83,9 +83,12 @@ def initialize(source, url = nil, encoding = nil) # :nodoc:
       end
       private :initialize
 
-      # Get the attributes of the current node as a Hash
+      # Get the attributes and namespaces of the current node as a Hash.
       #
-      # [Returns] (Hash<String, String>) Attribute names and values
+      # This is the union of Reader#attribute_hash and Reader#namespaces
+      #
+      # [Returns]
+      #   (Hash<String, String>) Attribute names and values, and namespace prefixes and hrefs.
       def attributes
         attribute_hash.merge(namespaces)
       end

diff --git a/test/xml/test_reader.rb b/test/xml/test_reader.rb
@@ -472,7 +472,9 @@ def test_reader_node_attributes_keep_a_reference_to_the_reader
 
           reader = Nokogiri::XML::Reader.from_memory(xml)
           reader.each do |element|
-            attribute_nodes += element.attribute_nodes
+            assert_output(nil, /Reader#attribute_nodes is deprecated/) do
+              attribute_nodes += element.attribute_nodes
+            end
           end
         end
 
@@ -489,7 +491,9 @@ def test_namespaced_attributes
         eoxml
         attr_ns = []
         while reader.read
-          if reader.node_type == Nokogiri::XML::Node::ELEMENT_NODE
+          next unless reader.node_type == Nokogiri::XML::Node::ELEMENT_NODE
+
+          assert_output(nil, /Reader#attribute_nodes is deprecated/) do
             reader.attribute_nodes.each { |attr| attr_ns << (attr.namespace.nil? ? nil : attr.namespace.prefix) }
           end
         end
@@ -593,14 +597,17 @@ def test_read_from_memory
       end
 
       def test_large_document_smoke_test
-        #  simply run on a large document to verify that there no GC issues
-        xml = []
-        xml << "<elements>"
-        10000.times { |j| xml << "<element id=\"#{j}\"/>" }
-        xml << "</elements>"
-        xml = xml.join("\n")
+        skip_unless_libxml2("valgrind tests should only run with libxml2")
 
-        Nokogiri::XML::Reader.from_memory(xml).each(&:attributes)
+        refute_valgrind_errors do
+          xml = []
+          xml << "<elements>"
+          10000.times { |j| xml << "<element id=\"#{j}\"/>" }
+          xml << "</elements>"
+          xml = xml.join("\n")
+
+          Nokogiri::XML::Reader.from_memory(xml).each(&:attributes)
+        end
       end
 
       def test_correct_outer_xml_inclusion