fix: ensure namespaced attributes are reparented properly

Previously, the namespace was dropped from the attribute. Fixes #2228 Co-authored-by: Nikolaj Potashnikoff <34554890+fiddlededee@users.noreply.github.com>
sparklemotion · May 11, 2021 · cd6941f · cd6941f
1 parent 1f483f0
commit cd6941f
Show file tree

Hide file tree

Showing 3 changed files with 33 additions and 2 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -23,6 +23,11 @@ Many thanks to Sam Ruby, Steve Checkoway, and Craig Barnes for creating and main
 * `Nokogiri.XSLT` parses stylesheets using `ParseOptions::DEFAULT_XSLT`, which should make some edge-case XSL transformations match libxslt's default behavior. [[#1940](https://github.com/sparklemotion/nokogiri/issues/1940)]
 
 
+### Fixed
+
+* [CRuby] Namespaced attributes are handled properly when their parent node is reparented into another document. Previously, the namespace may have gotten dropped. [[#2228](https://github.com/sparklemotion/nokogiri/issues/2228)]
+
+
 ### Improved
 
 * [CRuby] Speed up (slightly) the compile time of packaged libraries `libiconv`, `libxml2`, and `libxslt` by using autoconf's `--disable-dependency-tracking` option. ("ruby" platform gem only.)

diff --git a/ext/nokogiri/xml_node.c b/ext/nokogiri/xml_node.c
@@ -166,7 +166,7 @@ static VALUE
 reparent_node_with(VALUE pivot_obj, VALUE reparentee_obj, pivot_reparentee_func prf)
 {
   VALUE reparented_obj ;
-  xmlNodePtr reparentee, pivot, reparented, next_text, new_next_text, parent ;
+  xmlNodePtr reparentee, original_reparentee, pivot, reparented, next_text, new_next_text, parent ;
   int original_ns_prefix_is_default = 0 ;
 
   if (!rb_obj_is_kind_of(reparentee_obj, cNokogiriXmlNode)) {
@@ -252,7 +252,7 @@ reparent_node_with(VALUE pivot_obj, VALUE reparentee_obj, pivot_reparentee_func
   }
 
 ok:
-  xmlUnlinkNode(reparentee);
+  original_reparentee = reparentee;
 
   if (reparentee->doc != pivot->doc || reparentee->type == XML_TEXT_NODE) {
     /*
@@ -309,6 +309,8 @@ reparent_node_with(VALUE pivot_obj, VALUE reparentee_obj, pivot_reparentee_func
     }
   }
 
+  xmlUnlinkNode(original_reparentee);
+
   if (prf != xmlAddPrevSibling && prf != xmlAddNextSibling
       && reparentee->type == XML_TEXT_NODE && pivot->next && pivot->next->type == XML_TEXT_NODE) {
     /*

diff --git a/test/xml/test_node_reparenting.rb b/test/xml/test_node_reparenting.rb
@@ -360,6 +360,30 @@ def coerce(data)
                 assert_match(%r{<first xmlns=\"http://tenderlovemaking.com/\">}, @doc.to_xml)
               end
             end
+
+            describe "and a child node has a namespaced attribute" do
+              # https://github.com/sparklemotion/nokogiri/issues/2228
+              it "should not lose attribute namespace" do
+                source_doc = Nokogiri::XML::Document.parse(<<~EOXML)
+                  <pre1:root xmlns:pre1="ns1" xmlns:pre2="ns2">
+                    <pre1:child pre2:attr="attrval">
+                  </pre1:root>
+                EOXML
+                assert(source_node = source_doc.at_xpath("//pre1:child", {"pre1" => "ns1"}))
+                assert_equal("attrval", source_node.attribute_with_ns("attr", "ns2")&.value)
+
+                dest_doc = Nokogiri::XML::Document.parse(<<~EOXML)
+                  <pre1:root xmlns:pre1="ns1" xmlns:pre2="ns2">
+                  </pre1:root>
+                EOXML
+                assert(dest_node = dest_doc.at_xpath("//pre1:root", {"pre1" => "ns1"}))
+
+                inserted = dest_node.add_child(source_node)
+
+                assert_equal("attrval", inserted.attribute_with_ns("attr", "ns2")&.value,
+                             "inserted node attribute should be namespaced")
+              end
+            end
           end
 
           describe "given a parent node with a default and non-default namespace" do