From 5e8f98a98e5d010e96837fb614268e2084acc51c Mon Sep 17 00:00:00 2001 From: Mike Dalessio Date: Sat, 14 Aug 2021 11:44:13 -0400 Subject: [PATCH] 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. --- ext/java/nokogiri/XmlNode.java | 42 ++++++++++++++++++---------------- ext/nokogiri/xml_node.c | 24 +++++++++---------- lib/nokogiri/xml/node.rb | 16 ++++--------- 3 files changed, 38 insertions(+), 44 deletions(-) diff --git a/ext/java/nokogiri/XmlNode.java b/ext/java/nokogiri/XmlNode.java index d1ced80f74f..80be49f6051 100644 --- 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 index c28fb751a9b..141759bf6bc 100644 --- a/ext/nokogiri/xml_node.c +++ b/ext/nokogiri/xml_node.c @@ -115,18 +115,6 @@ 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) { @@ -134,6 +122,18 @@ relink_namespace(xmlNodePtr reparented) 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 index a06ba5fa56d..709422cafde 100644 --- 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