Implement #clone correctly with respect to singleton classes (#3117)

**What problem is this PR intended to solve?** Fixes #316 Classes this PR updates: - [x] `XML::Node` - [x] `XML::Document` - [x] `XML::NodeSet` **Have you included adequate test coverage?** Yes. **Does this change affect the behavior of either the C or the Java implementations?** The fix applies to both implementations.
sparklemotion · Feb 2, 2024 · 94a90a0 · 94a90a0
2 parents 464f8d4 + 7955303
commit 94a90a0
Show file tree

Hide file tree

Showing 14 changed files with 392 additions and 211 deletions.
diff --git a/ext/java/nokogiri/XmlDocument.java b/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)
+  {
+    super.initialize_copy_with_args(context, other, level, null);
+    resetCache();
+    return this;
   }
 
   @JRubyMethod(name = "root=")

diff --git a/ext/java/nokogiri/XmlNode.java b/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

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/java/nokogiri/XsltStylesheet.java b/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);

diff --git a/ext/nokogiri/xml_document.c b/ext/nokogiri/xml_document.c
@@ -143,6 +143,53 @@ static const rb_data_type_t noko_xml_document_data_type = {
   // .flags = RUBY_TYPED_FREE_IMMEDIATELY, // TODO see https://github.com/sparklemotion/nokogiri/issues/2822
 };
 
+static VALUE
+_xml_document_alloc(VALUE klass)
+{
+  return TypedData_Wrap_Struct(klass, &noko_xml_document_data_type, NULL);
+}
+
+static void
+_xml_document_data_ptr_set(VALUE rb_document, xmlDocPtr c_document)
+{
+  nokogiriTuplePtr tuple;
+
+  assert(DATA_PTR(rb_document) == NULL);
+  assert(c_document->_private == NULL);
+
+  DATA_PTR(rb_document) = c_document;
+
+  tuple = (nokogiriTuplePtr)ruby_xmalloc(sizeof(nokogiriTuple));
+  tuple->doc = rb_document;
+  tuple->unlinkedNodes = st_init_numtable_with_size(128);
+  tuple->node_cache = rb_ary_new();
+
+  c_document->_private = tuple ;
+
+  rb_iv_set(rb_document, "@node_cache", tuple->node_cache);
+
+  return;
+}
+
+/* :nodoc: */
+static VALUE
+rb_xml_document_initialize_copy_with_args(VALUE rb_self, VALUE rb_other, VALUE rb_level)
+{
+  xmlDocPtr c_other, c_self;
+  int c_level;
+
+  c_other = noko_xml_document_unwrap(rb_other);
+  c_level = (int)NUM2INT(rb_level);
+
+  c_self = xmlCopyDoc(c_other, c_level);
+  if (c_self == NULL) { return Qnil; }
+
+  c_self->type = c_other->type;
+  _xml_document_data_ptr_set(rb_self, c_self);
+
+  return rb_self ;
+}
+
 static void
 recursively_remove_namespaces_from_node(xmlNodePtr node)
 {
@@ -406,36 +453,6 @@ read_memory(VALUE klass,
   return document;
 }
 
-/*
- * call-seq:
- *  dup
- *
- * Copy this Document.  An optional depth may be passed in, but it defaults
- * to a deep copy.  0 is a shallow copy, 1 is a deep copy.
- */
-static VALUE
-duplicate_document(int argc, VALUE *argv, VALUE self)
-{
-  xmlDocPtr doc, dup;
-  VALUE copy;
-  VALUE level;
-
-  if (rb_scan_args(argc, argv, "01", &level) == 0) {
-    level = INT2NUM((long)1);
-  }
-
-  doc = noko_xml_document_unwrap(self);
-
-  dup = xmlCopyDoc(doc, (int)NUM2INT(level));
-
-  if (dup == NULL) { return Qnil; }
-
-  dup->type = doc->type;
-  copy = noko_xml_document_wrap(rb_obj_class(self), dup);
-  rb_iv_set(copy, "@errors", rb_iv_get(self, "@errors"));
-  return copy ;
-}
-
 /*
  * call-seq:
  *  new(version = default)
@@ -655,24 +672,16 @@ VALUE
 noko_xml_document_wrap_with_init_args(VALUE klass, xmlDocPtr c_document, int argc, VALUE *argv)
 {
   VALUE rb_document;
-  nokogiriTuplePtr tuple;
 
   if (!klass) {
     klass = cNokogiriXmlDocument;
   }
 
-  rb_document = TypedData_Wrap_Struct(klass, &noko_xml_document_data_type, c_document);
-
-  tuple = (nokogiriTuplePtr)ruby_xmalloc(sizeof(nokogiriTuple));
-  tuple->doc = rb_document;
-  tuple->unlinkedNodes = st_init_numtable_with_size(128);
-  tuple->node_cache = rb_ary_new();
-
-  c_document->_private = tuple ;
+  rb_document = _xml_document_alloc(klass);
+  _xml_document_data_ptr_set(rb_document, c_document);
 
   rb_iv_set(rb_document, "@decorators", Qnil);
   rb_iv_set(rb_document, "@errors", Qnil);
-  rb_iv_set(rb_document, "@node_cache", tuple->node_cache);
 
   rb_obj_call_init(rb_document, argc, argv);
 
@@ -760,6 +769,8 @@ noko_init_xml_document(void)
    */
   cNokogiriXmlDocument = rb_define_class_under(mNokogiriXml, "Document", cNokogiriXmlNode);
 
+  rb_define_alloc_func(cNokogiriXmlDocument, _xml_document_alloc);
+
   rb_define_singleton_method(cNokogiriXmlDocument, "read_memory", read_memory, 4);
   rb_define_singleton_method(cNokogiriXmlDocument, "read_io", read_io, 4);
   rb_define_singleton_method(cNokogiriXmlDocument, "new", new, -1);
@@ -770,8 +781,10 @@ noko_init_xml_document(void)
   rb_define_method(cNokogiriXmlDocument, "encoding=", set_encoding, 1);
   rb_define_method(cNokogiriXmlDocument, "version", version, 0);
   rb_define_method(cNokogiriXmlDocument, "canonicalize", rb_xml_document_canonicalize, -1);
-  rb_define_method(cNokogiriXmlDocument, "dup", duplicate_document, -1);
   rb_define_method(cNokogiriXmlDocument, "url", url, 0);
   rb_define_method(cNokogiriXmlDocument, "create_entity", create_entity, -1);
   rb_define_method(cNokogiriXmlDocument, "remove_namespaces!", remove_namespaces_bang, 0);
+
+  rb_define_protected_method(cNokogiriXmlDocument, "initialize_copy_with_args", rb_xml_document_initialize_copy_with_args,
+                             2);
 }