[bug] Reparenting of nodes can get wrong namespaces when prefixes are reused

[TreyE]

If I reuse namespace prefixes for different nodes (such as when I use a blank XML namespace, i.e. xmlns=), I can get the wrong namespace when the node is reparented.

Code that Causes the Error

#! /usr/bin/env ruby

require 'nokogiri'

doc = Nokogiri::XML("<root xmlns=\"urn:something\"><child1 xmlns=\"urn:something_else\"/><child2/></root>")

node = doc.at_xpath("//ns:child2",{"ns" => "urn:something"})
new_parent = doc.at_xpath("//ns:child1",{"ns" => "urn:something_else"})
new_parent.add_child(node)

puts doc.to_xml

Note that this will now output incorrect namespaces on the "child2" node, like the following:

<?xml version="1.0"?>
<root xmlns="urn:something">
  <child1 xmlns="urn:something_else">
    <child2/>
  </child1>
</root>

Note that the "child2" node has now been incorrectly 'moved' into the "urn:something_else" namespace.

A more correct output would be something similar to:

<?xml version="1.0"?>
<root xmlns="urn:something">
  <child1 xmlns="urn:something_else">
    <child2 xmlns="urn:something"/>
  </child1>
</root>

Source of the Behaviour

I've tracked this down to static void relink_namespace(xmlNodePtr reparented) in ext/nokogiri/xml_node.c.
It seems this code doesn't check for nodes above the current node possibly 'squatting' on a namespace, for example:

1. The node being relinked has an ancestor node that has a matching, 'correct', namespace definition.
2. The node being relinked also has an ancestor node that has a namespace definition that matches by prefix, but NOT by href.
3. The node with the with the incorrect definition is not only an ancestor of the node being relinked, it is also a descendent of the node with the 'correct' definition - meaning that traversing up the tree you will hit the node with the incorrect definition first.

Tests and PRs

A test which reproduces this issue an an associated correction are provided in PR #2495.

[flavorjones]

Thank you for opening this as well as submitting a PR for it!

I want to spend some time understanding the bug report and re-building context around the reparenting code, which has always been complex and tricky to get right. Please give me a day or two! Thanks.

[TreyE]

@flavorjones NP! Thanks for getting back to me. This is my first major C submission here and understand I might not have gotten everything exactly right - so take as much time as you need.

[flavorjones]

For whatever it's worth, I think the behavior we should be striving for is described in #1200 (comment)

And I think we probably need to make whatever fix we adopt be in v2.0 and call it a breaking change (which it will be for some use cases).