From 5acd0f3d2c689f9199121cb839daf4de97a99631 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Thu, 1 Feb 2024 12:32:07 -0500 Subject: [PATCH] fix: XML::NodeSet#clone copies the singleton class Partially fixes #316 --- ext/java/nokogiri/XmlNodeSet.java | 11 +++++- ext/nokogiri/xml_node_set.c | 60 +++++++++++++++---------------- lib/nokogiri/xml/node_set.rb | 12 ++++--- test/xml/test_node_set.rb | 26 ++++++++++++++ 4 files changed, 73 insertions(+), 36 deletions(-) diff --git a/ext/java/nokogiri/XmlNodeSet.java b/ext/java/nokogiri/XmlNodeSet.java index f7390f832aa..bdb5a06e0eb 100644 --- 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 index ec069e9a1ae..92c8d1bdf66 100644 --- 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 index ab21e889f2e..a24e7a5b450 100644 --- a/lib/nokogiri/xml/node_set.rb +++ b/lib/nokogiri/xml/node_set.rb @@ -4,9 +4,13 @@ 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 @@ -14,8 +18,6 @@ class NodeSet # 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 index 9ca385312ca..2e2e6c7cdeb 100644 --- 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("").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("").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