Merge pull request #2252 from sparklemotion/2250-doc-frag-path-v1_11_x

fix(v1.11.x): `DocumentFragment#path` with libxml > 2.9.10

Author: flavorjones

CHANGELOG.md

@@ -4,6 +4,13 @@ Nokogiri follows [Semantic Versioning](https://semver.org/), please see the [REA
 
 ---
 
+## 1.11.6 / unreleased
+
+### Fixed
+
+* [CRuby] `DocumentFragment#xpath` checks for error case introduced in libxml > 2.9.10. In v1.11.4 and v1.11.5, calling `DocumentFragment#path` results in a segfault.
+
+
 ## 1.11.5 / 2021-05-19
 
 ### Fixed

ROADMAP.md

@@ -59,6 +59,7 @@
     - [#572](https://github.com/sparklemotion/nokogiri/issues/572)
   could we fix this by making DocumentFragment be a subclass of NodeSet?
 
+* potentially related, we should figure out a way that we can round-trip `DocumentFragment#path` through `#xpath` and get the `DocumentFragment back again - [#2250](https://github.com/sparklemotion/nokogiri/issues/2250)
 
 ## Better Syntax for custom XPath function handler
 

ext/nokogiri/xml_node.c

@@ -1292,17 +1292,25 @@ get_name(VALUE self)
  * Returns the path associated with this Node
  */
 static VALUE
-path(VALUE self)
+noko_xml_node_path(VALUE rb_node)
 {
-  xmlNodePtr node;
-  xmlChar *path ;
+  xmlNodePtr c_node;
+  xmlChar *c_path ;
   VALUE rval;
 
-  Data_Get_Struct(self, xmlNode, node);
+  Data_Get_Struct(rb_node, xmlNode, c_node);
+
+  c_path = xmlGetNodePath(c_node);
+  if (c_path == NULL) {
+    // see https://github.com/sparklemotion/nokogiri/issues/2250
+    // this behavior is clearly undesirable, but is what libxml <= 2.9.10 returned, and so we
+    // do this for now to preserve the behavior across libxml2 versions.
+    rval = NOKOGIRI_STR_NEW2("?");
+  } else {
+    rval = NOKOGIRI_STR_NEW2(c_path);
+    xmlFree(c_path);
+  }
 
-  path = xmlGetNodePath(node);
-  rval = NOKOGIRI_STR_NEW2(path);
-  xmlFree(path);
   return rval ;
 }
 
@@ -1779,7 +1787,7 @@ noko_init_xml_node()
   rb_define_method(cNokogiriXmlNode, "next_element", next_element, 0);
   rb_define_method(cNokogiriXmlNode, "previous_element", previous_element, 0);
   rb_define_method(cNokogiriXmlNode, "node_type", node_type, 0);
-  rb_define_method(cNokogiriXmlNode, "path", path, 0);
+  rb_define_method(cNokogiriXmlNode, "path", noko_xml_node_path, 0);
   rb_define_method(cNokogiriXmlNode, "key?", key_eh, 1);
   rb_define_method(cNokogiriXmlNode, "namespaced_key?", namespaced_key_eh, 2);
   rb_define_method(cNokogiriXmlNode, "blank?", blank_eh, 0);

rakelib/markdown.rake

@@ -1,2 +1,2 @@
 require "hoe/markdown"
-Hoe::Markdown::Standalone.new("nokogiri").define_markdown_tasks("CHANGELOG.md", "CONTRIBUTING.md")
+Hoe::Markdown::Standalone.new("nokogiri").define_markdown_tasks("CHANGELOG.md", "CONTRIBUTING.md", "ROADMAP.md")

test/xml/test_document.rb

@@ -1167,6 +1167,18 @@ def initialize(*args)
             assert_equal("expected Nokogiri::XML::Node but received Nokogiri::XML::NodeSet", e.message);
           end
         end
+
+        describe "#path" do
+          it "should return '/'" do
+            xml = <<~EOF
+              <root></root>
+            EOF
+
+            doc = Nokogiri::XML::Document.parse(xml)
+            assert_equal("/", doc.path)
+            assert_equal(doc, doc.at_xpath(doc.path)) # make sure we can round-trip
+          end
+        end
       end
     end
   end

test/xml/test_document_fragment.rb

@@ -363,6 +363,25 @@ def initialize(*args)
             end
           end
         end
+
+        describe "#path" do
+          it "should return '?'" do
+            # see https://github.com/sparklemotion/nokogiri/issues/2250
+            # this behavior is clearly undesirable, but is what libxml <= 2.9.10 returned, and so we
+            # do this for now to preserve the behavior across libxml2 versions.
+            xml = <<~EOF
+              <root1></root1>
+              <root2></root2>
+            EOF
+
+            frag = Nokogiri::XML::DocumentFragment.parse(xml)
+            assert_equal "?", frag.path
+
+            # # TODO: we should circle back and fix both the `#path` behavior and the `#xpath`
+            # # behavior so we can round-trip and get the DocumentFragment back again.
+            # assert_equal(frag, frag.at_xpath(doc.path)) # make sure we can round-trip
+          end
+        end
       end
     end
   end