From 2920cf454189ce2b88ebd53b3a3fd68482731092 Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Mon, 22 Mar 2021 16:05:27 -0400 Subject: [PATCH] prefactor: cleans up Document#root= tests and C code --- ext/nokogiri/xml_document.c | 67 +++++++++++++++---------------- test/xml/test_document.rb | 79 +++++++++++++++++++------------------ 2 files changed, 72 insertions(+), 74 deletions(-) diff --git a/ext/nokogiri/xml_document.c b/ext/nokogiri/xml_document.c index 0f05719d69..4b0bc69fe5 100644 --- a/ext/nokogiri/xml_document.c +++ b/ext/nokogiri/xml_document.c @@ -141,42 +141,35 @@ url(VALUE self) * Set the root element on this document */ static VALUE -set_root(VALUE self, VALUE root) +rb_xml_document_root_set(VALUE self, VALUE rb_new_root) { - xmlDocPtr doc; - xmlNodePtr new_root; - xmlNodePtr old_root; - - Data_Get_Struct(self, xmlDoc, doc); + xmlDocPtr c_document; + xmlNodePtr c_new_root = NULL, c_current_root; - old_root = NULL; - - if (NIL_P(root)) { - old_root = xmlDocGetRootElement(doc); - - if (old_root) { - xmlUnlinkNode(old_root); - noko_xml_document_pin_node(old_root); - } + Data_Get_Struct(self, xmlDoc, c_document); - return root; + c_current_root = xmlDocGetRootElement(c_document); + if (c_current_root) { + xmlUnlinkNode(c_current_root); + noko_xml_document_pin_node(c_current_root); } - Data_Get_Struct(root, xmlNode, new_root); + if (!NIL_P(rb_new_root)) { + Data_Get_Struct(rb_new_root, xmlNode, c_new_root); - - /* If the new root's document is not the same as the current document, - * then we need to dup the node in to this document. */ - if (new_root->doc != doc) { - old_root = xmlDocGetRootElement(doc); - if (!(new_root = xmlDocCopyNode(new_root, doc, 1))) { - rb_raise(rb_eRuntimeError, "Could not reparent node (xmlDocCopyNode)"); + /* If the new root's document is not the same as the current document, + * then we need to dup the node in to this document. */ + if (c_new_root->doc != c_document) { + c_new_root = xmlDocCopyNode(c_new_root, c_document, 1); + if (!c_new_root) { + rb_raise(rb_eRuntimeError, "Could not reparent node (xmlDocCopyNode)"); + } } } - xmlDocSetRootElement(doc, new_root); - if (old_root) { noko_xml_document_pin_node(old_root); } - return root; + xmlDocSetRootElement(c_document, c_new_root); + + return rb_new_root; } /* @@ -186,17 +179,19 @@ set_root(VALUE self, VALUE root) * Get the root node for this document. */ static VALUE -root(VALUE self) +rb_xml_document_root(VALUE self) { - xmlDocPtr doc; - xmlNodePtr root; + xmlDocPtr c_document; + xmlNodePtr c_root; - Data_Get_Struct(self, xmlDoc, doc); + Data_Get_Struct(self, xmlDoc, c_document); - root = xmlDocGetRootElement(doc); + c_root = xmlDocGetRootElement(c_document); + if (!c_root) { + return Qnil; + } - if (!root) { return Qnil; } - return noko_xml_node_wrap(Qnil, root) ; + return noko_xml_node_wrap(Qnil, c_root) ; } /* @@ -666,8 +661,8 @@ noko_init_xml_document() rb_define_singleton_method(cNokogiriXmlDocument, "read_io", read_io, 4); rb_define_singleton_method(cNokogiriXmlDocument, "new", new, -1); - rb_define_method(cNokogiriXmlDocument, "root", root, 0); - rb_define_method(cNokogiriXmlDocument, "root=", set_root, 1); + rb_define_method(cNokogiriXmlDocument, "root", rb_xml_document_root, 0); + rb_define_method(cNokogiriXmlDocument, "root=", rb_xml_document_root_set, 1); rb_define_method(cNokogiriXmlDocument, "encoding", encoding, 0); rb_define_method(cNokogiriXmlDocument, "encoding=", set_encoding, 1); rb_define_method(cNokogiriXmlDocument, "version", version, 0); diff --git a/test/xml/test_document.rb b/test/xml/test_document.rb index 666616c4b7..d3cfc4dc66 100644 --- a/test/xml/test_document.rb +++ b/test/xml/test_document.rb @@ -77,11 +77,6 @@ def test_document_with_initial_space assert_equal(2, doc.children.size) end - def test_root_set_to_nil - xml.root = nil - assert_nil(xml.root) - end - def test_million_laugh_attach doc = Nokogiri::XML(<<~EOF) @@ -146,7 +141,7 @@ def test_collect_namespaces end def test_subclass_initialize_modify # testing a segv - Class.new(Nokogiri::XML::Document) do + doc = Class.new(Nokogiri::XML::Document) do def initialize super body_node = Nokogiri::XML::Node.new("body", self) @@ -154,6 +149,7 @@ def initialize self.root = body_node end end.new + assert_equal("stuff", doc.root.content) end def test_create_text_node @@ -419,18 +415,6 @@ def test_prepend_child_with_string assert_equal("quack!", doc.root.children.first.content) end - def test_move_root_to_document_with_no_root - sender = Nokogiri::XML("foo") - newdoc = Nokogiri::XML::Document.new - newdoc.root = sender.root - end - - def test_move_root_with_existing_root_gets_gcd - doc = Nokogiri::XML("test") - doc2 = Nokogiri::XML("#{'x' * 5000000}") - doc2.root = doc.root - end - def test_validate if Nokogiri.uses_libxml? assert_equal(45, xml.validate.length) @@ -852,20 +836,6 @@ def test_new assert_nil(doc.root) end - def test_set_root - doc = nil - doc = Nokogiri::XML::Document.new - assert(doc) - assert(doc.xml?) - assert_nil(doc.root) - node = Nokogiri::XML::Node.new("b", doc) do |n| - n.content = "hello world" - end - assert_equal("hello world", node.content) - doc.root = node - assert_equal(node, doc.root) - end - def test_remove_namespaces doc = Nokogiri::XML(<<~EOX) @@ -958,6 +928,12 @@ def awesome! assert(xml.children.respond_to?(:awesome!)) end + def test_can_be_closed + f = File.open(XML_FILE) + Nokogiri::XML(f) + f.close + end + describe "JRuby-only methods" do describe "Document.wrap" do if Nokogiri.jruby? @@ -1034,12 +1010,6 @@ def awesome! end end - def test_can_be_closed - f = File.open(XML_FILE) - Nokogiri::XML(f) - f.close - end - describe "XML::Document.parse" do # establish baseline behavior for HTML document behavior in # https://github.com/sparklemotion/nokogiri/issues/2130 @@ -1148,6 +1118,39 @@ def initialize(*args) end end end + + describe "#root=" do + it "assigns a root node" do + doc = Nokogiri::XML::Document.new + node = doc.create_element("div") + assert_nil(doc.root) + + doc.root = node + + assert_equal(node, doc.root) + end + + it "allows removing the root node by setting to nil" do + assert_kind_of(Nokogiri::XML::Node, xml.root) + assert_nil(xml.root = nil) + assert_nil(xml.root) + end + + it "supports moving nodes across documents" do + source = Nokogiri::XML("foo") + target = Nokogiri::XML::Document.new + assert_nil(target.root) + target.root = source.root + assert_equal("foo", target.root.content) + end + + it "doesn't leak the replaced node" do + skip("only run if NOKOGIRI_GC is set") unless ENV['NOKOGIRI_GC'] + doc = Nokogiri::XML("test") + doc2 = Nokogiri::XML("#{'x' * 5000000}") + doc2.root = doc.root + end + end end end end