fix: XML::NodeSet#clone copies the singleton class

Partially fixes #316
sparklemotion · Feb 1, 2024 · 5acd0f3 · 5acd0f3
1 parent 0e5c6c5
commit 5acd0f3
Show file tree

Hide file tree

Showing 4 changed files with 73 additions and 36 deletions.
diff --git a/ext/java/nokogiri/XmlNodeSet.java b/ext/java/nokogiri/XmlNodeSet.java
@@ -15,6 +15,7 @@
 import org.jruby.anno.JRubyClass;
 import org.jruby.anno.JRubyMethod;
 import org.jruby.runtime.ThreadContext;
+import org.jruby.runtime.Visibility;
 import org.jruby.runtime.builtin.IRubyObject;
 import org.w3c.dom.Node;
 import org.w3c.dom.NodeList;
@@ -201,7 +202,6 @@ public class XmlNodeSet extends RubyObject implements NodeList
     return context.nil;
   }
 
-  @JRubyMethod
   public IRubyObject
   dup(ThreadContext context)
   {
@@ -210,6 +210,15 @@ public class XmlNodeSet extends RubyObject implements NodeList
     return dup;
   }
 
+  @JRubyMethod(visibility = Visibility.PROTECTED)
+  public IRubyObject
+  initialize_copy(ThreadContext context, IRubyObject other)
+  {
+    setNodes(getNodes(context, other));
+    initializeFrom(context, (XmlNodeSet)other);
+    return this;
+  }
+
   @JRubyMethod(name = "include?")
   public IRubyObject
   include_p(ThreadContext context, IRubyObject node_or_namespace)

diff --git a/ext/nokogiri/xml_node_set.c b/ext/nokogiri/xml_node_set.c
@@ -83,6 +83,26 @@ xml_node_set_allocate(VALUE klass)
   return TypedData_Wrap_Struct(klass, &xml_node_set_type, xmlXPathNodeSetCreate(NULL));
 }
 
+/* :nodoc: */
+static VALUE
+rb_xml_node_set_initialize_copy(VALUE rb_self, VALUE rb_other)
+{
+  xmlNodeSetPtr c_self, c_other;
+  VALUE rb_document;
+
+  TypedData_Get_Struct(rb_self, xmlNodeSet, &xml_node_set_type, c_self);
+  TypedData_Get_Struct(rb_other, xmlNodeSet, &xml_node_set_type, c_other);
+
+  xmlXPathNodeSetMerge(c_self, c_other);
+
+  rb_document = rb_iv_get(rb_other, "@document");
+  if (!NIL_P(rb_document)) {
+    rb_iv_set(rb_self, "@document", rb_document);
+    rb_funcall(rb_document, decorate, 1, rb_self);
+  }
+
+  return rb_self;
+}
 
 static void
 xpath_node_set_del(xmlNodeSetPtr cur, xmlNodePtr val)
@@ -112,27 +132,6 @@ xpath_node_set_del(xmlNodeSetPtr cur, xmlNodePtr val)
   cur->nodeTab[cur->nodeNr] = NULL;
 }
 
-
-/*
- * call-seq:
- *  dup
- *
- * Duplicate this NodeSet. Note that the Nodes contained in the NodeSet are not
- * duplicated (similar to how Array and other Enumerable classes work).
- */
-static VALUE
-duplicate(VALUE rb_self)
-{
-  xmlNodeSetPtr c_self;
-  xmlNodeSetPtr dupl;
-
-  TypedData_Get_Struct(rb_self, xmlNodeSet, &xml_node_set_type, c_self);
-
-  dupl = xmlXPathNodeSetMerge(NULL, c_self);
-
-  return noko_xml_node_set_wrap(dupl, rb_iv_get(rb_self, "@document"));
-}
-
 /*
  * call-seq:
  *  length
@@ -501,18 +500,19 @@ noko_init_xml_node_set(void)
 
   rb_define_alloc_func(cNokogiriXmlNodeSet, xml_node_set_allocate);
 
-  rb_define_method(cNokogiriXmlNodeSet, "length", length, 0);
-  rb_define_method(cNokogiriXmlNodeSet, "[]", slice, -1);
-  rb_define_method(cNokogiriXmlNodeSet, "slice", slice, -1);
-  rb_define_method(cNokogiriXmlNodeSet, "push", push, 1);
-  rb_define_method(cNokogiriXmlNodeSet, "|", rb_xml_node_set_union, 1);
+  rb_define_method(cNokogiriXmlNodeSet, "&", intersection, 1);
   rb_define_method(cNokogiriXmlNodeSet, "-", minus, 1);
-  rb_define_method(cNokogiriXmlNodeSet, "unlink", unlink_nodeset, 0);
-  rb_define_method(cNokogiriXmlNodeSet, "to_a", to_array, 0);
-  rb_define_method(cNokogiriXmlNodeSet, "dup", duplicate, 0);
+  rb_define_method(cNokogiriXmlNodeSet, "[]", slice, -1);
   rb_define_method(cNokogiriXmlNodeSet, "delete", delete, 1);
-  rb_define_method(cNokogiriXmlNodeSet, "&", intersection, 1);
   rb_define_method(cNokogiriXmlNodeSet, "include?", include_eh, 1);
+  rb_define_method(cNokogiriXmlNodeSet, "length", length, 0);
+  rb_define_method(cNokogiriXmlNodeSet, "push", push, 1);
+  rb_define_method(cNokogiriXmlNodeSet, "slice", slice, -1);
+  rb_define_method(cNokogiriXmlNodeSet, "to_a", to_array, 0);
+  rb_define_method(cNokogiriXmlNodeSet, "unlink", unlink_nodeset, 0);
+  rb_define_method(cNokogiriXmlNodeSet, "|", rb_xml_node_set_union, 1);
+
+  rb_define_private_method(cNokogiriXmlNodeSet, "initialize_copy", rb_xml_node_set_initialize_copy, 1);
 
   decorate = rb_intern("decorate");
 }
diff --git a/lib/nokogiri/xml/node_set.rb b/lib/nokogiri/xml/node_set.rb
@@ -4,18 +4,20 @@
 module Nokogiri
   module XML
     ####
-    # A NodeSet contains a list of Nokogiri::XML::Node objects.  Typically
-    # a NodeSet is return as a result of searching a Document via
-    # Nokogiri::XML::Searchable#css or Nokogiri::XML::Searchable#xpath
+    # A NodeSet is an Enumerable that contains a list of Nokogiri::XML::Node objects.
+    #
+    # Typically a NodeSet is returned as a result of searching a Document via
+    # Nokogiri::XML::Searchable#css or Nokogiri::XML::Searchable#xpath.
+    #
+    # Note that the `#dup` and `#clone` methods perform shallow copies; these methods do not copy
+    # the Nodes contained in the NodeSet (similar to how Array and other Enumerable classes work).
     class NodeSet
       include Nokogiri::XML::Searchable
       include Enumerable
 
       # The Document this NodeSet is associated with
       attr_accessor :document
 
-      alias_method :clone, :dup
-
       # Create a NodeSet with +document+ defaulting to +list+
       def initialize(document, list = [])
         @document = document

diff --git a/test/xml/test_node_set.rb b/test/xml/test_node_set.rb
@@ -368,6 +368,32 @@ def awesome(ns)
           end
         end
 
+        specify "test_dup_should_not_copy_singleton_class" do
+          # https://github.com/sparklemotion/nokogiri/issues/316
+          m = Module.new do
+            def foo; end
+          end
+
+          set = Nokogiri::XML::Document.parse("<root/>").css("root")
+          set.extend(m)
+
+          assert_respond_to(set, :foo)
+          refute_respond_to(set.dup, :foo)
+        end
+
+        specify "test_clone_should_copy_singleton_class" do
+          # https://github.com/sparklemotion/nokogiri/issues/316
+          m = Module.new do
+            def foo; end
+          end
+
+          set = Nokogiri::XML::Document.parse("<root/>").css("root")
+          set.extend(m)
+
+          assert_respond_to(set, :foo)
+          assert_respond_to(set.clone, :foo)
+        end
+
         specify "#dup on empty set" do
           empty_set = Nokogiri::XML::NodeSet.new(xml, [])
           assert_equal(0, empty_set.dup.length) # this shouldn't raise null pointer exception