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

nokogiri Jruby does not let you move nodes from one document to another, MRI does #1773

Closed
jrochkind opened this issue Jun 26, 2018 · 4 comments
Milestone

Comments

@jrochkind
Copy link
Contributor

MRI nokogiri version
$ nokogiri -v
# Nokogiri (1.8.3)
    ---
    warnings: []
    nokogiri: 1.8.3
    ruby:
      version: 2.5.1
      platform: x86_64-darwin16
      description: ruby 2.5.1p57 (2018-03-29 revision 63029) [x86_64-darwin16]
      engine: ruby
    libxml:
      binding: extension
      source: packaged
      libxml2_path: "/Users/jrochkind/.gem/ruby/2.5.1/gems/nokogiri-1.8.3/ports/x86_64-apple-darwin16.7.0/libxml2/2.9.8"
      libxslt_path: "/Users/jrochkind/.gem/ruby/2.5.1/gems/nokogiri-1.8.3/ports/x86_64-apple-darwin16.7.0/libxslt/1.1.32"
      libxml2_patches:
      - 0001-Revert-Do-not-URI-escape-in-server-side-includes.patch
      libxslt_patches: []
      compiled: 2.9.8
      loaded: 2.9.8
Jruby nokogiri version
$ nokogiri -v
Ignoring jruby-launcher-1.1.5-java because its extensions are not built. Try: gem pristine jruby-launcher --version 1.1.5
# Nokogiri (1.8.3)
    ---
    warnings: []
    nokogiri: 1.8.3
    ruby:
      version: 2.5.0
      platform: java
      description: jruby 9.2.0.0 (2.5.0) 2018-05-24 81156a8 Java HotSpot(TM) 64-Bit Server
        VM 25.121-b13 on 1.8.0_121-b13 +jit [darwin-x86_64]
      engine: jruby
      jruby: 9.2.0.0
    xerces: Xerces-J 2.11.0
    nekohtml: NekoHTML 1.9.21

Reproduction:

require 'nokogiri'

xml_src = "<record><field><value>3</value></field></record>"

doc1 = Nokogiri::XML.parse(xml_src)

doc2 = Nokogiri::XML::Document.new

node = doc1.at_xpath("//field")
node.remove

doc2.add_child(node)

puts "===="
puts doc2.to_xml

In MRI nokogiri, this works fine, and prints what you'd expect:

<?xml version="1.0"?>
<field>
  <value>3</value>
</field>

In JRuby nokogiri, it raises:

RuntimeError: org.w3c.dom.DOMException: WRONG_DOCUMENT_ERR: A node is used in a different document than the one that created it.
                     add_child_node at nokogiri/XmlNode.java:1701
  add_child_node_and_reparent_attrs at /Users/jrochkind/.gem/jruby/2.5.0/gems/nokogiri-1.8.3-java/lib/nokogiri/xml/node.rb:882
                          add_child at /Users/jrochkind/.gem/jruby/2.5.0/gems/nokogiri-1.8.3-java/lib/nokogiri/xml/node.rb:142
                          add_child at /Users/jrochkind/.gem/jruby/2.5.0/gems/nokogiri-1.8.3-java/lib/nokogiri/xml/document.rb:249
                             <main> at test_ns.rb:12

Surprisingly, doc2.root = node does seem to work without raising, JRuby nokogiri can move a node from one doc to another using root=, at least.

@jrochkind
Copy link
Contributor Author

(oops, doc2.root = node does sort of work, but if there are namespaces involved it doesn't do the same thing with them as in MRI nokogiri, where I consider MRI's behavior correct. But namespace stuff has a variety of other inconsistencies between MRI and JRuby nokogiri, and is so confusing, that other than mentioning it's a thing, I won't go into details in this issue).

@flavorjones
Copy link
Member

I'll take a look -- we've got pretty thorough tests for reparenting nodes, but it's totally possible we've missed an edge case that affects JRuby.

@jrochkind
Copy link
Contributor Author

Cool, thanks, the repro is above which you can make into a test if you like. If somehow you can't repro with that script that raises for me... that'd be odd.

@jvshahid
Copy link
Member

jvshahid commented Jul 8, 2018

pushed a fix in #1777. @flavorjones can you take a look at the PR.

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

3 participants