refactor: relink_namespaces handles attributes properly

Previously both implementations of relink_namespaces skipped attributes if the node itself didn't have a namespace. This essentially reverts the implementation in 9fd03d8 by correcting the behavior of relink_namespaces.
sparklemotion · Aug 14, 2021 · 5e8f98a · 5e8f98a
1 parent 3ae1f38
commit 5e8f98a
Show file tree

Hide file tree

Showing 3 changed files with 38 additions and 44 deletions.
diff --git a/ext/java/nokogiri/XmlNode.java b/ext/java/nokogiri/XmlNode.java
@@ -421,24 +421,24 @@ public class XmlNode extends RubyObject
     String nsURI = e.lookupNamespaceURI(prefix);
     this.node = NokogiriHelpers.renameNode(e, nsURI, e.getNodeName());
 
-    if (nsURI == null || nsURI.isEmpty()) { return; }
-
-    String currentPrefix = e.getParentNode().lookupPrefix(nsURI);
-    String currentURI = e.getParentNode().lookupNamespaceURI(prefix);
-    boolean isDefault = e.getParentNode().isDefaultNamespace(nsURI);
-
-    // add xmlns attribute if this is a new root node or if the node's
-    // namespace isn't a default namespace in the new document
-    if (e.getParentNode().getNodeType() == Node.DOCUMENT_NODE) {
-      // this is the root node, so we must set the namespaces attributes anyway
-      e.setAttribute(prefix == null ? "xmlns" : "xmlns:" + prefix, nsURI);
-    } else if (prefix == null) {
-      // this is a default namespace but isn't the default where this node is being added
-      if (!isDefault) { e.setAttribute("xmlns", nsURI); }
-    } else if (!prefix.equals(currentPrefix) || nsURI.equals(currentURI)) {
-      // this is a prefixed namespace
-      // but doesn't have the same prefix or the prefix is set to a different URI
-      e.setAttribute("xmlns:" + prefix, nsURI);
+    if (nsURI != null && !nsURI.isEmpty()) {
+      String currentPrefix = e.getParentNode().lookupPrefix(nsURI);
+      String currentURI = e.getParentNode().lookupNamespaceURI(prefix);
+      boolean isDefault = e.getParentNode().isDefaultNamespace(nsURI);
+
+      // add xmlns attribute if this is a new root node or if the node's
+      // namespace isn't a default namespace in the new document
+      if (e.getParentNode().getNodeType() == Node.DOCUMENT_NODE) {
+        // this is the root node, so we must set the namespaces attributes anyway
+        e.setAttribute(prefix == null ? "xmlns" : "xmlns:" + prefix, nsURI);
+      } else if (prefix == null) {
+        // this is a default namespace but isn't the default where this node is being added
+        if (!isDefault) { e.setAttribute("xmlns", nsURI); }
+      } else if (!prefix.equals(currentPrefix) || nsURI.equals(currentURI)) {
+        // this is a prefixed namespace
+        // but doesn't have the same prefix or the prefix is set to a different URI
+        e.setAttribute("xmlns:" + prefix, nsURI);
+      }
     }
 
     if (e.hasAttributes()) {
@@ -473,8 +473,10 @@ public class XmlNode extends RubyObject
       }
     }
 
-    if (this.node.hasChildNodes()) {
-      relink_namespace(context, getChildren());
+    if (nsURI != null && !nsURI.isEmpty()) {
+      if (this.node.hasChildNodes()) {
+        relink_namespace(context, getChildren());
+      }
     }
   }
 

diff --git a/ext/nokogiri/xml_node.c b/ext/nokogiri/xml_node.c
@@ -115,25 +115,25 @@ relink_namespace(xmlNodePtr reparented)
     }
   }
 
-  /* Only walk all children if there actually is a namespace we need to */
-  /* reparent. */
-  if (NULL == reparented->ns) { return; }
-
-  /* When a node gets reparented, walk it's children to make sure that */
-  /* their namespaces are reparented as well. */
-  child = reparented->children;
-  while (NULL != child) {
-    relink_namespace(child);
-    child = child->next;
-  }
-
   if (reparented->type == XML_ELEMENT_NODE) {
     attr = reparented->properties;
     while (NULL != attr) {
       relink_namespace((xmlNodePtr)attr);
       attr = attr->next;
     }
   }
+
+  /* Only walk all children if there actually is a namespace we need to */
+  /* reparent. */
+  if (reparented->ns) {
+    /* When a node gets reparented, walk it's children to make sure that */
+    /* their namespaces are reparented as well. */
+    child = reparented->children;
+    while (NULL != child) {
+      relink_namespace(child);
+      child = child->next;
+    }
+  }
 }
 
 /* :nodoc: */

diff --git a/lib/nokogiri/xml/node.rb b/lib/nokogiri/xml/node.rb
@@ -128,9 +128,9 @@ def >(selector)
       def add_child(node_or_tags)
         node_or_tags = coerce(node_or_tags)
         if node_or_tags.is_a?(XML::NodeSet)
-          node_or_tags.each { |n| add_child_node_and_reparent_attrs n }
+          node_or_tags.each { |n| add_child_node n }
         else
-          add_child_node_and_reparent_attrs node_or_tags
+          add_child_node node_or_tags
         end
         node_or_tags
       end
@@ -248,9 +248,9 @@ def children=(node_or_tags)
         node_or_tags = coerce(node_or_tags)
         children.unlink
         if node_or_tags.is_a?(XML::NodeSet)
-          node_or_tags.each { |n| add_child_node_and_reparent_attrs n }
+          node_or_tags.each { |n| add_child_node n }
         else
-          add_child_node_and_reparent_attrs node_or_tags
+          add_child_node node_or_tags
         end
         node_or_tags
       end
@@ -1223,14 +1223,6 @@ def inspect_attributes
 
       # @private
       IMPLIED_XPATH_CONTEXTS = [".//".freeze].freeze
-
-      def add_child_node_and_reparent_attrs(node)
-        add_child_node node
-        node.attribute_nodes.find_all { |a| a.name =~ /:/ }.each do |attr_node|
-          attr_node.remove
-          node[attr_node.name] = attr_node.value
-        end
-      end
     end
   end
 end