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

Partially fixes #316

ext/java/nokogiri/XmlDocument.java

@@ -414,20 +414,13 @@ private static class DocumentBuilderFactoryHolder
     return getCachedNodeOrCreate(context.runtime, rootNode);
   }
 
-  protected IRubyObject
-  dup_implementation(Ruby runtime, boolean deep)
-  {
-    XmlDocument doc = (XmlDocument) super.dup_implementation(runtime, deep);
-    // Avoid creating a new XmlDocument since we cloned one
-    // already. Otherwise the following test will fail:
-    //
-    //   dup = doc.dup
-    //   dup.equal?(dup.children[0].document)
-    //
-    // Since `dup.children[0].document' will end up creating a new
-    // XmlDocument.  See #1060.
-    doc.resetCache();
-    return doc;
+  @JRubyMethod(visibility = Visibility.PROTECTED)
+  public IRubyObject
+  initialize_copy_with_args(ThreadContext context, IRubyObject other, IRubyObject level, IRubyObject _ignored)
+  {
+    super.initialize_copy_with_args(context, other, level, _ignored);
+    resetCache();
+    return this;
   }
 
   @JRubyMethod(name = "root=")

ext/java/nokogiri/XmlNode.java

@@ -966,45 +966,13 @@ public class XmlNode extends RubyObject
     return doc;
   }
 
+  @JRubyMethod(visibility = Visibility.PROTECTED)
   public IRubyObject
-  dup()
+  initialize_copy_with_args(ThreadContext context, IRubyObject other, IRubyObject level, IRubyObject _ignored)
   {
-    return dup_implementation(getMetaClass().getClassRuntime(), true);
-  }
-
-  @JRubyMethod
-  public IRubyObject
-  dup(ThreadContext context)
-  {
-    return dup_implementation(context, true);
-  }
-
-  @JRubyMethod
-  public IRubyObject
-  dup(ThreadContext context, IRubyObject depth)
-  {
-    boolean deep = depth instanceof RubyInteger && RubyFixnum.fix2int(depth) != 0;
-    return dup_implementation(context, deep);
-  }
-
-  protected final IRubyObject
-  dup_implementation(ThreadContext context, boolean deep)
-  {
-    return dup_implementation(context.runtime, deep);
-  }
-
-  protected IRubyObject
-  dup_implementation(Ruby runtime, boolean deep)
-  {
-    XmlNode clone;
-    try {
-      clone = (XmlNode) clone();
-    } catch (CloneNotSupportedException e) {
-      throw runtime.newRuntimeError(e.toString());
-    }
-    Node newNode = node.cloneNode(deep);
-    clone.node = newNode;
-    return clone;
+    boolean deep = level instanceof RubyInteger && RubyFixnum.fix2int(level) != 0;
+    this.node = asXmlNode(context, other).node.cloneNode(deep);
+    return this;
   }
 
   public static RubyString

ext/java/nokogiri/XsltStylesheet.java

@@ -135,7 +135,7 @@ public class XsltStylesheet extends RubyObject
     XmlDocument xmlDoc = (XmlDocument) args[0];
     ensureDocumentHasNoError(context, xmlDoc);
 
-    Document doc = ((XmlDocument) xmlDoc.dup_implementation(context, true)).getDocument();
+    Document doc = ((XmlDocument)xmlDoc.callMethod(context, "dup", runtime.newFixnum(1))).getDocument();
 
     XsltStylesheet xslt =
       (XsltStylesheet) NokogiriService.XSLT_STYLESHEET_ALLOCATOR.allocate(runtime, (RubyClass)klazz);

ext/nokogiri/xml_node.c

@@ -962,51 +962,26 @@ internal_subset(VALUE self)
   return noko_xml_node_wrap(Qnil, (xmlNodePtr)dtd);
 }
 
-/*
- * :call-seq:
- *   dup → Nokogiri::XML::Node
- *   dup(depth) → Nokogiri::XML::Node
- *   dup(depth, new_parent_doc) → Nokogiri::XML::Node
- *
- * Copy this node.
- *
- * [Parameters]
- * - +depth+ 0 is a shallow copy, 1 (the default) is a deep copy.
- * - +new_parent_doc+
- *   The new node's parent Document. Defaults to the this node's document.
- *
- * [Returns] The new Nokogiri::XML::Node
- */
+/* :nodoc: */
 static VALUE
-duplicate_node(int argc, VALUE *argv, VALUE self)
+rb_xml_node_initialize_copy_with_args(VALUE rb_self, VALUE rb_other, VALUE rb_level, VALUE rb_new_parent_doc)
 {
-  VALUE r_level, r_new_parent_doc;
-  int level;
-  int n_args;
-  xmlDocPtr new_parent_doc;
-  xmlNodePtr node, dup;
+  xmlNodePtr c_self, c_other;
+  int c_level;
+  xmlDocPtr c_new_parent_doc;
 
-  Noko_Node_Get_Struct(self, xmlNode, node);
-
-  n_args = rb_scan_args(argc, argv, "02", &r_level, &r_new_parent_doc);
-
-  if (n_args < 1) {
-    r_level = INT2NUM((long)1);
-  }
-  level = (int)NUM2INT(r_level);
+  Noko_Node_Get_Struct(rb_self, xmlNode, c_self);
+  Noko_Node_Get_Struct(rb_other, xmlNode, c_other);
+  c_level = (int)NUM2INT(rb_level);
+  c_new_parent_doc = noko_xml_document_unwrap(rb_new_parent_doc);
 
-  if (n_args < 2) {
-    new_parent_doc = node->doc;
-  } else {
-    new_parent_doc = noko_xml_document_unwrap(r_new_parent_doc);
-  }
+  c_self = xmlDocCopyNode(c_other, c_new_parent_doc, c_level);
+  if (c_self == NULL) { return Qnil; }
 
-  dup = xmlDocCopyNode(node, new_parent_doc, level);
-  if (dup == NULL) { return Qnil; }
+  _xml_node_data_ptr_set(rb_self, c_self);
+  noko_xml_document_pin_node(c_self);
 
-  noko_xml_document_pin_node(dup);
-
-  return noko_xml_node_wrap(rb_obj_class(self), dup);
+  return rb_self;
 }
 
 /*
@@ -2384,7 +2359,7 @@ noko_init_xml_node(void)
 {
   cNokogiriXmlNode = rb_define_class_under(mNokogiriXml, "Node", rb_cObject);
 
-  rb_undef_alloc_func(cNokogiriXmlNode);
+  rb_define_alloc_func(cNokogiriXmlNode, _xml_node_alloc);
 
   rb_define_singleton_method(cNokogiriXmlNode, "new", rb_xml_node_new, -1);
 
@@ -2399,7 +2374,6 @@ noko_init_xml_node(void)
   rb_define_method(cNokogiriXmlNode, "create_external_subset", create_external_subset, 3);
   rb_define_method(cNokogiriXmlNode, "create_internal_subset", create_internal_subset, 3);
   rb_define_method(cNokogiriXmlNode, "document", rb_xml_node_document, 0);
-  rb_define_method(cNokogiriXmlNode, "dup", duplicate_node, -1);
   rb_define_method(cNokogiriXmlNode, "element_children", rb_xml_node_element_children, 0);
   rb_define_method(cNokogiriXmlNode, "encode_special_chars", encode_special_chars, 1);
   rb_define_method(cNokogiriXmlNode, "external_subset", external_subset, 0);
@@ -2428,6 +2402,8 @@ noko_init_xml_node(void)
   rb_define_method(cNokogiriXmlNode, "previous_sibling", previous_sibling, 0);
   rb_define_method(cNokogiriXmlNode, "unlink", unlink_node, 0);
 
+  rb_define_protected_method(cNokogiriXmlNode, "initialize_copy_with_args", rb_xml_node_initialize_copy_with_args, 3);
+
   rb_define_private_method(cNokogiriXmlNode, "add_child_node", add_child, 1);
   rb_define_private_method(cNokogiriXmlNode, "add_next_sibling_node", add_next_sibling, 1);
   rb_define_private_method(cNokogiriXmlNode, "add_previous_sibling_node", add_previous_sibling, 1);

lib/nokogiri/xml/node.rb

@@ -127,6 +127,42 @@ def initialize(name, document)
         # This is intentionally empty, and sets the method signature for subclasses.
       end
 
+      #
+      # :call-seq:
+      #   dup → Nokogiri::XML::Node
+      #   dup(level) → Nokogiri::XML::Node
+      #   dup(level, new_parent_doc) → Nokogiri::XML::Node
+      #
+      # Duplicate this node.
+      #
+      # [Parameters]
+      # - +level+ (optional Integer). 0 is a shallow copy, 1 (the default) is a deep copy.
+      # - +new_parent_doc+ (optional Nokogiri::XML::Document)
+      #   The new node's parent Document. Defaults to the the Document of the current node.
+      # [Returns] The new Nokogiri::XML::Node
+      #
+      def dup(level = 1, new_parent_doc = document)
+        super().initialize_copy_with_args(self, level, new_parent_doc)
+      end
+
+      #
+      # :call-seq:
+      #   clone → Nokogiri::XML::Node
+      #   clone(level) → Nokogiri::XML::Node
+      #   clone(level, new_parent_doc) → Nokogiri::XML::Node
+      #
+      # Clone this node.
+      #
+      # [Parameters]
+      # - +level+ (optional Integer). 0 is a shallow copy, 1 (the default) is a deep copy.
+      # - +new_parent_doc+
+      #   The new node's parent Document. Defaults to the the Document of the current node.
+      # [Returns] The new Nokogiri::XML::Node
+      #
+      def clone(level = 1, new_parent_doc = document)
+        super().initialize_copy_with_args(self, level, new_parent_doc)
+      end
+
       ###
       # Decorate this node with the decorators set up in this node's Document
       def decorate!
@@ -474,7 +510,6 @@ def do_xinclude(options = XML::ParseOptions::DEFAULT_XML)
       alias_method :to_str, :content
       alias_method :name, :node_name
       alias_method :type, :node_type
-      alias_method :clone, :dup
       alias_method :elements, :element_children
 
       # :section: Working With Node Attributes

test/xml/test_node.rb

@@ -478,13 +478,41 @@ def test_duplicate_node_removes_namespace
           end
         end
 
+        def test_dup_should_not_copy_singleton_class
+          # https://github.com/sparklemotion/nokogiri/issues/316
+          m = Module.new do
+            def foo; end
+          end
+
+          node = Nokogiri::XML::Document.parse("<root/>").root
+          node.extend(m)
+
+          assert_respond_to(node, :foo)
+          refute_respond_to(node.dup, :foo)
+        end
+
+        def test_clone_should_copy_singleton_class
+          # https://github.com/sparklemotion/nokogiri/issues/316
+          m = Module.new do
+            def foo; end
+          end
+
+          node = Nokogiri::XML::Document.parse("<root/>").root
+          node.extend(m)
+
+          assert_respond_to(node, :foo)
+          assert_respond_to(node.clone, :foo)
+        end
+
         def test_inspect_object_with_no_data_ptr
           # test for the edge case when an exception is thrown during object construction/copy
           node = Nokogiri::XML("<root>").root
           refute_includes(node.inspect, "(no data)")
 
-          node.stub :data_ptr?, false do
-            assert_includes(node.inspect, "(no data)")
+          if node.respond_to?(:data_ptr?)
+            node.stub(:data_ptr?, false) do
+              assert_includes(node.inspect, "(no data)")
+            end
           end
         end