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

Partially fixes #316
sparklemotion · Feb 1, 2024 · 3a98dd4 · 3a98dd4
1 parent 8c11241
commit 3a98dd4
Show file tree

Hide file tree

Showing 3 changed files with 63 additions and 35 deletions.
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