allow a simpler syntax for using a builder on detached nodes

[ccutrer]

rather than

node = nil
Nokogiri::XML::Builder.with(document.root) do |xml|
xml.element do |xml|
node = xml.parent
xml.parent.unlink
end
end

you can just do
node = Nokogiri::XML::Builder.detached(document) do |xml|
xml.element
end

[ccutrer]

@flavorjones is this PR reasonable? If so, I'll continue to be patient waiting for review. If not, I'll use the existing API that's just not as simple.

[flavorjones]

Hi,

Historically we've been hesitant to adopt new API synonyms or shortcuts. That said, I'd like to better understand what you're using this syntax for.

Can you explain a bit about why you want to use builder to create a detached node? Your test case simply appends the nodes back into the document, thereby not revealing your intent.

[ccutrer]

Sure. At https://github.com/instructure/ruby-saml2/blob/master/lib/saml2/assertion.rb#L12 I'm serializing just the Assertion of a SAML Response, because it gets signed (and not the whole document). By creating it detached, I'm able to do that signature cleanly, without requiring the user of the library to serialize the entire response first.

Note that for now, I've worked around the lack of this method by just creating the assertion on a separate document, and then when it's added to the entire Response, it gets put on a separate document. I've been unable to find any documentation if that is okay (both Nokogiri or libxml2).(instructure/ruby-saml2@871b0c4 is the commit that changed it from using detached to moving the node between documents). As it currently is seems to work, but the assertion's document method continues to return the old document, and I'm worried about memory management/segfault problems.

[flavorjones]

Sorry for being slow to respond. I'm afraid I still don't understand what the compelling use case is for this new method.

Builder creates a document. If you'd like to reference a node or a subtree of that document for some purpose, you can reference it directly using XPath or CSS queries (or by traversing the tree yourself). If necessary, you can also then reparent (or unparent) the node or subtree.

Does the previous paragraph make sense? Have I misunderstood the intent of this PR?

[ccutrer]

The intent is I already have a document (not a builder), and I want to use a builder to add more nodes to just a piece of that document. The current workaround is to use a builder to create a new document, and then detach it and attach it into the final document where it goes. It's just extra steps, and given nokogiri's history of memory management issues, especially having to do with reparenting of nodes, I'd like to avoid it and just build the new nodes where they belong in the first place.

[flavorjones]

@ccutrer I see what you did there with FUD around reparenting nodes. If you don't trust Nokogiri, perhaps take a look at the regression test suite we have around reparenting nodes. Just for starters:

• https://github.com/sparklemotion/nokogiri/blob/master/test/xml/test_node_reparenting.rb
• https://github.com/sparklemotion/nokogiri/blob/master/test/xml/test_unparented_node.rb

There are more tests interleaved in individual test files, as well.

It's easy to say "given the history of X" when the project in question has been around since 2008 and has had 126 distinct releases. Nokogiri's memory management is rock solid right now, and we pay immediate attention to anyone providing specifics to the contrary. So please, let's focus on your use case.

---

I'm afraid I still don't understand your use case, as what you're explaining above appears to be different from the explanation in the initial comment; and the test case is unilluminating.

If you have a Document, and you want to add nodes to it with Builder, then you can already do that, as you explained in the original comment:

#! /usr/bin/env ruby

require 'nokogiri'

doc = Nokogiri::XML "<root><containers></containers></root>"

Nokogiri::XML::Builder.with(doc.at_css("containers")) do |xml|
  xml.container "container_id" => 1
  xml.container "container_id" => 2
end

puts doc.to_xml
# => <?xml version="1.0"?>
#    <root>
#      <containers>
#        <container container_id="1"/>
#        <container container_id="2"/>
#      </containers>
#    </root>

Help me understand how the above code doesn't meet your requirement already?

[ccutrer]

Honestly, I don't remember why it was such a pain before. It's been so long, that I just worked around it with a less than ideal implementation, and forgot about this PR. And I promise I'm not trying to spread FUD. To be fair, the big pain in recent memory was the fault of https://github.com/xml4r/libxml-ruby interacting badly with Nokogiri (and we chose to convert all of our libraries to use Nokogiri, instead of libxml-ruby). But #1327 is one instance of crashes caused by Nokogiri. Given that I have no intention of going back to change my code to the old design anymore, we can just close this PR.