make sure NodeSet's @document is set even if children empty

resolves #1319 (test borrowed from @jkraemer) adjusted test for #1320 to be a little more complex
sparklemotion · Feb 12, 2017 · 5165f24 · 5165f24
1 parent 099a950
commit 5165f24
Show file tree

Hide file tree

Showing 3 changed files with 31 additions and 9 deletions.
diff --git a/ext/java/nokogiri/XmlNode.java b/ext/java/nokogiri/XmlNode.java
@@ -713,8 +713,16 @@ public IRubyObject child(ThreadContext context) {
 
     @JRubyMethod
     public IRubyObject children(ThreadContext context) {
-        XmlNodeSet xmlNodeSet = (XmlNodeSet) NokogiriService.XML_NODESET_ALLOCATOR.allocate(context.getRuntime(), getNokogiriClass(context.getRuntime(), "Nokogiri::XML::NodeSet"));
-        xmlNodeSet.setNodeList(node.getChildNodes());
+        XmlNodeSet xmlNodeSet = XmlNodeSet.create(context.runtime);
+
+        NodeList nodeList = node.getChildNodes();
+        if (nodeList.getLength() > 0) {
+            xmlNodeSet.setNodeList(nodeList); // initializes @document from first node
+        }
+        else { // TODO this is very ripe for refactoring
+            setDocumentAndDecorate(context, xmlNodeSet, doc);
+        }
+
         return xmlNodeSet;
     }
 
@@ -1430,9 +1438,10 @@ public IRubyObject set_namespace(ThreadContext context, IRubyObject namespace) {
 
     @JRubyMethod(name = {"unlink", "remove"})
     public IRubyObject unlink(ThreadContext context) {
-        if (node.getParentNode() != null) {
-            clearXpathContext(node.getParentNode());
-            node.getParentNode().removeChild(node);
+        final Node parent = node.getParentNode();
+        if (parent != null) {
+            parent.removeChild(node);
+            clearXpathContext(parent);
         }
         return this;
     }

diff --git a/lib/nokogiri/xml/node_set.rb b/lib/nokogiri/xml/node_set.rb
@@ -293,7 +293,11 @@ def == other
       # Returns a new NodeSet containing all the children of all the nodes in
       # the NodeSet
       def children
-        inject(NodeSet.new(document)) { |set, node| set += node.children }
+        node_set = NodeSet.new(document)
+        each do |node|
+          node.children.each { |n| node_set.push(n) }
+        end
+        node_set
       end
 
       ###

diff --git a/test/xml/test_document_fragment.rb b/test/xml/test_document_fragment.rb
@@ -141,16 +141,15 @@ def test_fragment_css_search_with_whitespace_and_node_removal
         # does not expose the error. Putting both nodes on the same line
         # instead also fixes the crash.
         fragment = Nokogiri::XML::DocumentFragment.parse <<-EOXML
-          <p id="content">hi</p>
-<p>another paragraph</p>
+          <p id="content">hi</p> x <!--y--> <p>another paragraph</p>
         EOXML
         children = fragment.css('p')
         assert_equal 2, children.length
         # removing the last node instead does not yield the error. Probably the
         # node removal leaves around two consecutive text nodes which make the
         # css search crash?
         children.first.remove
-        # using xpath('/p') instead works as expected
+        assert_equal 1, fragment.xpath('.//p | self::p').length
         assert_equal 1, fragment.css('p').length
       end
 
@@ -237,6 +236,16 @@ def awesome!
         assert fragment.children.respond_to?(:awesome!), fragment.children.class
       end
 
+      def test_decorator_is_applied_to_empty_set
+        x = Module.new do
+          def awesome!
+          end
+        end
+        util_decorate(@xml, x)
+        fragment = Nokogiri::XML::DocumentFragment.new(@xml, "")
+        assert fragment.children.respond_to?(:awesome!), fragment.children.class
+      end
+
       def test_add_node_to_doc_fragment_segfault
         frag = Nokogiri::XML::DocumentFragment.new(@xml, '<p>hello world</p>')
         Nokogiri::XML::Comment.new(frag,'moo')