Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding namespace-less node to namespaced parent attaches the parent namespace to the child #425

Closed
mbklein opened this issue Feb 19, 2011 · 12 comments

Comments

@mbklein
Copy link
Contributor

mbklein commented Feb 19, 2011

Start with

doc = Nokogiri::XML(%{
<my:document xmlns:my="foo:bar">
  <my:first-child>Some Text!</my:first-child>
</my:document>
})

Then

doc.root.add_child(%{<second-child/>})

or

doc.root.add_child(%{<third-child xmlns=""/>})

or even

new_child = Nokogiri::XML(%{<fourth-child/>}).root
doc.root.add_child(new_child)

Do it all, and you end up with

<?xml version="1.0"?>
<my:document xmlns:my="foo:bar">
  <my:first-child>Some Text!</my:first-child>
  <my:second-child/>
  <my:third-child xmlns=""/>
  <my:fourth-child/>
</my:document>

None of which is probably what you meant.

The only way I've been able to get the new element (and its children) to end up with no namespace is to traverse it after adding and explicitly set node.namespace = nil on everything.

@flavorjones
Copy link
Member

Confirmed. This is probably a bug. Test to reproduce:

          describe "given a parent node with a non-default namespace" do
            before do
              @doc = Nokogiri::XML(<<-eoxml)
                <root xmlns="http://tenderlovemaking.com/" xmlns:foo="http://flavorjon.es/">
                  <first>
                  </first>
                </root>
              eoxml
            end

            it "inserts a node with no namespace" do
              assert node = @doc.at('//xmlns:first')
              child = Nokogiri::XML::Node.new('second', @doc)
              node.add_child(child)
              assert @doc.at('//second')
            end

@tenderlove - do you agree that this is unintended behavior?

@tenderlove
Copy link
Member

I think someone wanted the current behavior. IIRC, we have tests that ensure this behavior. But it could be that those tests were only in the context of the XML builder. When you're hand building a tree like this, I agree, it shouldn't inherit namespaces like that.

@flavorjones
Copy link
Member

We actually only have tests around adding children of nodes with a
default namespace. We have no coverage on non-default namespaces. And
non-default namespace behavior is, I think, the question being asked.
On Feb 21, 2012 11:46 AM, "Aaron Patterson" <
reply@reply.github.com>
wrote:

I think someone wanted the current behavior. IIRC, we have tests that
ensure this behavior. But it could be that those tests were only in the
context of the XML builder. When you're hand building a tree like this, I
agree, it shouldn't inherit namespaces like that.


Reply to this email directly or view it on GitHub:
https://github.com/tenderlove/nokogiri/issues/425#issuecomment-4078569

@amitavmohanty01
Copy link

I guess we need a way of having this inheritance of namespaces configurable. Is this issue resolved? Is there any by-pass for achieving this?

@jesusabarca
Copy link

Hi. Is this resolved?

@pgericson
Copy link

This does not seem to be resolved, I'm on version 1.8.0.

@mmustala
Copy link

What a surprising and annoying feature. I would have expected node.replace(xml_string) to just change the content of the node, but no, it messes up the namespaces. I guess I need to write a regex to get the node replaced properly.

@jdongelmans
Copy link

FYI: My downstream gem broke because of a now missing namespace and I stumbled on this old issue because with the upgrade to version >= 1.12.0 (and thus when libxml version >= 2.9.12, ref: 889ee2a) this has been fixed.

jdongelmans added a commit to digidentity/xmlmapper that referenced this issue Aug 12, 2021
Newer versions of Nokogiri (>= 1.12.0) can use libxml >= 2.9.12 which breaks the
automatic addition of the parent namespace when adding a child.
This was already the case for JRuby, so added the same workaround
for when nokogiri is using this newer version of libxml.

Ref: sparklemotion/nokogiri#425
@flavorjones
Copy link
Member

flavorjones commented Aug 12, 2021

@jdongelmans Thanks for the note. I'd like to clarify, though: 889ee2a noted a change in the HTML parser, but this issue is about XML parser behavior.

That said, this behavior has also changed in the XML parser (for some of the cases described), and I had a mental note to circle back to this issue to verify that and note it here. I think the behavior changed in #2230 (specifically 1f483f0) but will verify that. Also see related #1712 fixed by that commit. This was noted in the CHANGELOG for v1.12.0 as well.

@jdongelmans
Copy link

@flavorjones Thanks for the clarification. Indeed, you are right. I shortcutted my own thinking because of an already present comment in our code directing to this issue.

@flavorjones
Copy link
Member

OK, after a bit of git-bisecting ...

The "third child" example already worked as expected in v1.6.0 which is as far back as I can easily go on my dev machine.

But the "second child" and "fourth child" cases were (as suspected) fixed by 1f483f0 which first appeared in v1.12.0 and was part of #2230 which fixed #1712 and #2228. See also #2246 which updated some tests to reflect this behavioral change.

So: I'm closing this one out as "fixed in v1.12.0". Sorry for the delay and the confusion caused by leaving this open.

@flavorjones
Copy link
Member

Please see #2320 for more discussion about this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants