Rails Ruby on Rails

Comments and changes to this ticket

• 

  Pratik March 10th, 2009 @ 12:40 AM

  • Assigned user set to “Jeremy Kemper”
• 

  Pratik March 10th, 2009 @ 12:41 AM

  • Title changed from “[PATCH] Add Nokogiri as XmlMini backend” to “Add Nokogiri as XmlMini backend”
• 

  Michael Koziarski March 10th, 2009 @ 01:58 AM

  • Milestone cleared.

  SHould probably get this in ahead of 2.3

• 

  Jeremy Kemper March 10th, 2009 @ 06:59 PM

  • State changed from “new” to “open”

  seems to require nokogiri >= 1.1.1

• 

  Repository March 10th, 2009 @ 07:10 PM

  • State changed from “open” to “committed”

  (from [694998ee4fb8d257ba78424cab630846327a0889]) Nokogiri backend for XmlMini

  [#2190 state:committed]

  Signed-off-by: Jeremy Kemper jeremy@bitsweat.net http://github.com/rails/rails/co...

• You flagged this item as spam.
  

  Bart ten Brinke March 10th, 2009 @ 08:07 PM

  I'm getting failures if I actually enable nokogiri:

  1) Error: test_array_with_multiple_entries_from_xml(HashToXmlTest): TypeError: can't convert String into Integer

  ./test/../lib/active_support/xml_mini/nokogiri.rb:38:in `[]'
  ./test/../lib/active_support/xml_mini/nokogiri.rb:38:in `to_hash'
  ./test/../lib/active_support/xml_mini/nokogiri.rb:52:in `call'
  ./test/../lib/active_support/xml_mini/nokogiri.rb:52:in `to_hash'
  /Users/bart/.gem/ruby/1.8/gems/nokogiri-1.2.1/lib/nokogiri/xml/node_set.rb:139:in `each'
  ./test/../lib/active_support/xml_mini/nokogiri.rb:52:in `to_hash'
  ./test/../lib/active_support/xml_mini/nokogiri.rb:52:in `call'
  ./test/../lib/active_support/xml_mini/nokogiri.rb:52:in `to_hash'
  /Users/bart/.gem/ruby/1.8/gems/nokogiri-1.2.1/lib/nokogiri/xml/node_set.rb:139:in `each'
  ./test/../lib/active_support/xml_mini/nokogiri.rb:52:in `to_hash'
  ./test/../lib/active_support/xml_mini/nokogiri.rb:55:in `call'
  ./test/../lib/active_support/xml_mini/nokogiri.rb:55:in `to_hash'
  /Users/bart/.gem/ruby/1.8/gems/nokogiri-1.2.1/lib/nokogiri/xml/node_set.rb:139:in `each'
  ./test/../lib/active_support/xml_mini/nokogiri.rb:55:in `to_hash'
  ./test/../lib/active_support/xml_mini/nokogiri.rb:20:in `to_hash'
  ./test/../lib/active_support/xml_mini/nokogiri.rb:13:in `parse'
  (__DELEGATION__):2:in `__send__'
  (__DELEGATION__):2:in `parse'
  ./test/../lib/active_support/core_ext/hash/conversions.rb:153:in `from_xml'
  ./test/core_ext/hash_ext_test.rb:674:in `test_array_with_multiple_entries_from_xml'
  /Library/Ruby/Gems/1.8/gems/mocha-0.9.5/lib/mocha/test_case_adapter.rb:69:in `__send__'
  /Library/Ruby/Gems/1.8/gems/mocha-0.9.5/lib/mocha/test_case_adapter.rb:69:in `run'
  
  

  2) Error: test_file_from_xml(HashToXmlTest): NoMethodError: undefined method original_filename' for {"content_type"=>"image/png", "name"=>"logo.png", "type"=>"file"}:Hash

  ./test/core_ext/hash_ext_test.rb:689:in `test_file_from_xml'
  /Library/Ruby/Gems/1.8/gems/mocha-0.9.5/lib/mocha/test_case_adapter.rb:69:in `__send__'
  /Library/Ruby/Gems/1.8/gems/mocha-0.9.5/lib/mocha/test_case_adapter.rb:69:in `run'
  
  

  3) Error: test_file_from_xml_with_defaults(HashToXmlTest): NoMethodError: You have a nil object when you didn't expect it! The error occurred while evaluating nil.original_filename

  ./test/core_ext/hash_ext_test.rb:701:in `test_file_from_xml_with_defaults'
  /Library/Ruby/Gems/1.8/gems/mocha-0.9.5/lib/mocha/test_case_adapter.rb:69:in `__send__'
  /Library/Ruby/Gems/1.8/gems/mocha-0.9.5/lib/mocha/test_case_adapter.rb:69:in `run'
  
  

  4) Error: test_multiple_records_from_xml(HashToXmlTest): TypeError: can't convert String into Integer

  ./test/../lib/active_support/xml_mini/nokogiri.rb:44:in `[]'
  ./test/../lib/active_support/xml_mini/nokogiri.rb:44:in `to_hash'
  ./test/../lib/active_support/xml_mini/nokogiri.rb:52:in `call'
  ./test/../lib/active_support/xml_mini/nokogiri.rb:52:in `to_hash'
  /Users/bart/.gem/ruby/1.8/gems/nokogiri-1.2.1/lib/nokogiri/xml/node_set.rb:139:in `each'
  ./test/../lib/active_support/xml_mini/nokogiri.rb:52:in `to_hash'
  ./test/../lib/active_support/xml_mini/nokogiri.rb:55:in `call'
  ./test/../lib/active_support/xml_mini/nokogiri.rb:55:in `to_hash'
  /Users/bart/.gem/ruby/1.8/gems/nokogiri-1.2.1/lib/nokogiri/xml/node_set.rb:139:in `each'
  ./test/../lib/active_support/xml_mini/nokogiri.rb:55:in `to_hash'
  ./test/../lib/active_support/xml_mini/nokogiri.rb:20:in `to_hash'
  ./test/../lib/active_support/xml_mini/nokogiri.rb:13:in `parse'
  (__DELEGATION__):2:in `__send__'
  (__DELEGATION__):2:in `parse'
  ./test/../lib/active_support/core_ext/hash/conversions.rb:153:in `from_xml'
  ./test/core_ext/hash_ext_test.rb:605:in `test_multiple_records_from_xml'
  /Library/Ruby/Gems/1.8/gems/mocha-0.9.5/lib/mocha/test_case_adapter.rb:69:in `__send__'
  /Library/Ruby/Gems/1.8/gems/mocha-0.9.5/lib/mocha/test_case_adapter.rb:69:in `run'
  
  

  5) Failure: test_expansion_count_is_limited(QueryTest)

  [./test/core_ext/hash_ext_test.rb:888:in `test_expansion_count_is_limited'
   /Library/Ruby/Gems/1.8/gems/mocha-0.9.5/lib/mocha/test_case_adapter.rb:69:in `__send__'
   /Library/Ruby/Gems/1.8/gems/mocha-0.9.5/lib/mocha/test_case_adapter.rb:69:in `run']:
  
  

  exception expected but none was thrown.

  6) Failure: test_default_is_rexml(REXMLEngineTest)

  [./test/xml_mini/rexml_engine_test.rb:8:in `test_default_is_rexml'
   /Library/Ruby/Gems/1.8/gems/mocha-0.9.5/lib/mocha/test_case_adapter.rb:69:in `__send__'
   /Library/Ruby/Gems/1.8/gems/mocha-0.9.5/lib/mocha/test_case_adapter.rb:69:in `run']:
  
  

  <ActiveSupport::XmlMini_REXML> expected but was <ActiveSupport::XmlMini_Nokogiri>.

  1666 tests, 7289 assertions, 2 failures, 4 errors

• You flagged this item as spam.
  

  Bart ten Brinke March 10th, 2009 @ 08:11 PM

  If you run the entire rails test suite you'll get even more errors. Looks like the behaviour is different from Rexml. The LibXML implementation does not have this issue.

• 

  Aaron Patterson March 10th, 2009 @ 08:32 PM

  What version of nokogiri? How are you executing the tests? They all pass for me.

  Also, do you have tests for the LibXML version? I'm glad to match behavior if it is spec'd.

• You flagged this item as spam.
  

  Bart ten Brinke March 10th, 2009 @ 08:39 PM

  Well, if you enable the nokogiri gem as default:

  XmlMini.backend = 'Nokogiri'
  
  

  and then run the rails test, you'll see a lot of issues where your nokogiri implementation has a different behaviour as the default REXML and LibXML implementation. This means that enabeling nokogiri will result in a rails which will behave wrong or dangerously ( 5 Failure: test_expansion_count_is_limited(QueryTest )

• You flagged this item as spam.
  

  Bart ten Brinke March 10th, 2009 @ 08:42 PM

  If you use XmlMini.backend = 'LibXML' you will only get 1 failure: test_default_is_rexml, which is obvious.

• 

  Aaron Patterson March 10th, 2009 @ 08:46 PM

  I can fix these. But I must make it clear that it does not behave dangerously. Expansion count is limited, but nokogiri corrects the document and doesn't raise an exception. :-)

• You flagged this item as spam.
  

  Bart ten Brinke March 10th, 2009 @ 08:52 PM

  Ok, but the behavior is still different :). I didn't look into the Nokogiri implementation. I do agree with you that this test needs to be automated somehow, for future xml implementations.

  BTW: Nice work on the Nokogiri implementation :)

• 

  Jeremy Kemper March 10th, 2009 @ 08:56 PM

  • State changed from “committed” to “open”

  Ok, I won't revert, but reopened pending expanded test coverage. Thanks guys!

• You flagged this item as spam.
  

  Bart ten Brinke March 10th, 2009 @ 09:09 PM

  Anyone got a nice idea how to implement this in the active support rake file?

  if gem 'libxml-ruby', '=0.9.7'
    XmlMini.backend = 'LibXML'
    run_entire_testsuite
  end
  if gem 'nokogiri'
    XmlMini.backend = 'Nokogiri'
    run_entire_testsuite
  end
  
  

  This is why my initial implementation automatically chose from the available back-ends :)

  Oh yeah, could you put

     require 'nokogiri'
  
  

  at the top of you mixin? That would make it function the same as the LibXML and REXML.

• 

  Aaron Patterson March 11th, 2009 @ 03:47 AM

  Hallo! I've attached a patch that fixes the nokogiri back end so that all rails tests pass.

  • nokogiri_test_fixes.patch 5.2 KB
• 

  Repository March 11th, 2009 @ 05:53 AM

  • State changed from “open” to “resolved”

  (from [b9e021df974217b9c6ee273bd6c98b40ebde0cd3]) adding more nokogiri tests and making the main rails tests pass

  [#2190 state:resolved]

  Signed-off-by: Jeremy Kemper jeremy@bitsweat.net http://github.com/rails/rails/co...

• You flagged this item as spam.
  

  Stephen Bannasch March 15th, 2009 @ 06:24 AM

  Aaron,

  You might want to check out this benchmark I just put together comparing all three XmlMini backends.

  http://github.com/stepheneb/rail...

  The benchmark runs Hash.from_xml on a 1.7MB XML file.

  Summary: libxml-ruby was 25x faster than REXML while nokogiri was only 1.6 times faster.

  Also: JRuby ran the REXML backend 3 times faster than MRI 1.8.6.

• 

  Aaron Patterson March 15th, 2009 @ 09:43 PM

  @Stephen

  You're right. But LibXML seems to produce different results than REXML, where Nokogiri matches the REXML output.

  I would say that because of that, this speed test is not exactly valid.

  http://github.com/tenderlove/rai...

• You flagged this item as spam.
  

  Jérôme April 22nd, 2009 @ 02:11 PM

  Hello

  In which file do you set XmlMini.backend = 'Nokogiri' ?

• 

  Aaron Patterson April 22nd, 2009 @ 04:19 PM

  I suppose I would do this in my environment.rb file.

• You flagged this item as spam.
  

  Jérôme April 22nd, 2009 @ 04:27 PM

  Nope :(

  
  /opt/local/lib/ruby/gems/1.8/gems/activesupport-2.3.2/lib/active_support/dependencies.rb:443:in `load_missing_constant': uninitialized constant XmlMini (NameError)
  	from /opt/local/lib/ruby/gems/1.8/gems/activesupport-2.3.2/lib/active_support/dependencies.rb:80:in `const_missing'
  	from /opt/local/lib/ruby/gems/1.8/gems/activesupport-2.3.2/lib/active_support/dependencies.rb:92:in `const_missing'
  	from /Users/jerome/Sites/foo/config/environment.rb:45
  

• 

  Pratik April 22nd, 2009 @ 04:29 PM

  Jérôme,

  You should really be asking this question the mailing list. LH is not for general support. Also, use ActiveSupport::XmlMini.

  Thanks.

• 

  Aaron Patterson April 22nd, 2009 @ 04:29 PM

  Did you try ActiveSupport::XmlMini?

• You flagged this item as spam.
  

  Jérôme April 22nd, 2009 @ 04:33 PM

  As apache says: it works !

  Thanks.

• You flagged this item as spam.
  

  Jérôme April 22nd, 2009 @ 04:34 PM

  Thank you Pratik. However it wasn't documented, not even on the announcement on the official rails blog.

  Anyway it works, end of story. Thank you very much.