Rails Ruby on Rails → 2.3.6

Comments and changes to this ticket

• You flagged this item as spam.
  

  Jorge Dias December 23rd, 2009 @ 01:33 PM

  • Tag changed from “active_record, hash, serialization” to “active_record, active_support, hash, serialization”

• You flagged this item as spam.
  

  Serguei Filimonov December 24th, 2009 @ 09:33 PM

  • Tag changed from “active_record, active_support, hash, serialization” to “active_record, active_support, hash, serialization, yaml”

  Hey I tried this in both 2-3 stable and master branches. Can't reproduce it. Could you provide perhaps a small code snippet that fails to run because of this?

• 

  Prem Sichanugrist (sikachu) December 25th, 2009 @ 03:49 PM

  I wonder that this problem arise from using ar-extensions plugin?

  Can you try to remove the plugin first, and see if the problem still exists?

• You flagged this item as spam.
  

  Dave Ungerer December 25th, 2009 @ 07:01 PM

  I'm getting the same error, and I don't use ar-extensions. The error happens only in Rails 2.3.5, not in 2.3.4.
  

  I have an AR model with a serialized hash attribute, which itself contains some nested hashes. The error below occurs when saving the model. Hope that helps. I'll see if I can get a stand-alone code snippet to replicate this if it remains outstanding for too long.
  

  NoMethodError: You have a nil object when you didn't expect it!
  You might have expected an instance of Array.
  The error occurred while evaluating nil.each
  /opt/ruby-enterprise-1.8.7-2009.10/lib/ruby/1.8/yaml/rubytypes.rb:40:in `to_yaml'
  /opt/ruby-enterprise-1.8.7-2009.10/lib/ruby/1.8/yaml/rubytypes.rb:39:in `map'
  /opt/ruby-enterprise-1.8.7-2009.10/lib/ruby/1.8/yaml/rubytypes.rb:39:in `to_yaml'
  /opt/ruby-enterprise-1.8.7-2009.10/lib/ruby/1.8/yaml.rb:391:in `call'
  /opt/ruby-enterprise-1.8.7-2009.10/lib/ruby/1.8/yaml.rb:391:in `emit'
  /opt/ruby-enterprise-1.8.7-2009.10/lib/ruby/1.8/yaml.rb:391:in `quick_emit'
  /opt/ruby-enterprise-1.8.7-2009.10/lib/ruby/1.8/yaml/rubytypes.rb:38:in `to_yaml'
  /opt/ruby-enterprise-1.8.7-2009.10/lib/ruby/1.8/yaml/rubytypes.rb:18:in `node_export'
  /opt/ruby-enterprise-1.8.7-2009.10/lib/ruby/1.8/yaml/rubytypes.rb:18:in `add'
  /opt/ruby-enterprise-1.8.7-2009.10/lib/ruby/1.8/yaml/rubytypes.rb:18:in `to_yaml'
  /opt/ruby-enterprise-1.8.7-2009.10/lib/ruby/1.8/yaml/rubytypes.rb:17:in `each'
  /opt/ruby-enterprise-1.8.7-2009.10/lib/ruby/1.8/yaml/rubytypes.rb:17:in `to_yaml'
  /opt/ruby-enterprise-1.8.7-2009.10/lib/ruby/1.8/yaml/rubytypes.rb:16:in `map'
  /opt/ruby-enterprise-1.8.7-2009.10/lib/ruby/1.8/yaml/rubytypes.rb:16:in `to_yaml'
  /opt/ruby-enterprise-1.8.7-2009.10/lib/ruby/1.8/yaml.rb:391:in `call'
  /opt/ruby-enterprise-1.8.7-2009.10/lib/ruby/1.8/yaml.rb:391:in `emit'
  /opt/ruby-enterprise-1.8.7-2009.10/lib/ruby/1.8/yaml.rb:391:in `quick_emit'
  /opt/ruby-enterprise-1.8.7-2009.10/lib/ruby/1.8/yaml/rubytypes.rb:15:in `to_yaml'
  /opt/ruby-enterprise-1.8.7-2009.10/lib/ruby/1.8/yaml/rubytypes.rb:18:in `node_export'
  /opt/ruby-enterprise-1.8.7-2009.10/lib/ruby/1.8/yaml/rubytypes.rb:18:in `add'
  /opt/ruby-enterprise-1.8.7-2009.10/lib/ruby/1.8/yaml/rubytypes.rb:18:in `to_yaml'
  /opt/ruby-enterprise-1.8.7-2009.10/lib/ruby/1.8/yaml/rubytypes.rb:17:in `each'
  /opt/ruby-enterprise-1.8.7-2009.10/lib/ruby/1.8/yaml/rubytypes.rb:17:in `to_yaml'
  /opt/ruby-enterprise-1.8.7-2009.10/lib/ruby/1.8/yaml/rubytypes.rb:16:in `map'
  /opt/ruby-enterprise-1.8.7-2009.10/lib/ruby/1.8/yaml/rubytypes.rb:16:in `to_yaml'
  /opt/ruby-enterprise-1.8.7-2009.10/lib/ruby/1.8/yaml.rb:391:in `call'
  /opt/ruby-enterprise-1.8.7-2009.10/lib/ruby/1.8/yaml.rb:391:in `emit'
  /opt/ruby-enterprise-1.8.7-2009.10/lib/ruby/1.8/yaml.rb:391:in `quick_emit'
  /opt/ruby-enterprise-1.8.7-2009.10/lib/ruby/1.8/yaml/rubytypes.rb:15:in `to_yaml'
  /opt/ruby-enterprise-1.8.7-2009.10/lib/ruby/1.8/yaml/rubytypes.rb:41:in `node_export'
  /opt/ruby-enterprise-1.8.7-2009.10/lib/ruby/1.8/yaml/rubytypes.rb:41:in `add'
  /opt/ruby-enterprise-1.8.7-2009.10/lib/ruby/1.8/yaml/rubytypes.rb:41:in `to_yaml'
  /opt/ruby-enterprise-1.8.7-2009.10/lib/ruby/1.8/yaml/rubytypes.rb:40:in `each'
  /opt/ruby-enterprise-1.8.7-2009.10/lib/ruby/1.8/yaml/rubytypes.rb:40:in `to_yaml'
  /opt/ruby-enterprise-1.8.7-2009.10/lib/ruby/1.8/yaml/rubytypes.rb:39:in `map'
  /opt/ruby-enterprise-1.8.7-2009.10/lib/ruby/1.8/yaml/rubytypes.rb:39:in `to_yaml'
  /opt/ruby-enterprise-1.8.7-2009.10/lib/ruby/1.8/yaml.rb:391:in `call'
  /opt/ruby-enterprise-1.8.7-2009.10/lib/ruby/1.8/yaml.rb:391:in `emit'
  /opt/ruby-enterprise-1.8.7-2009.10/lib/ruby/1.8/yaml.rb:391:in `quick_emit'
  /opt/ruby-enterprise-1.8.7-2009.10/lib/ruby/1.8/yaml/rubytypes.rb:38:in `to_yaml'
  

• You flagged this item as spam.
  

  Dave Ungerer January 15th, 2010 @ 11:53 AM

  I can now confirm that this bug doesn't happen under Ruby 1.9.1. That leaves 2 possibilities that I can see:

  1. It's a bug in Ruby Enterprise Edition (since both bug reports used it)
     
  2. It's a general 1.8 incompatibility introduced when fixing 1.9 compatibility. (since the bug was introduced in Rails 2.3.5)

  I'm not going to bother installing and testing with MRI 1.8.7 to narrow down the possiblities, because it's not a deployment option for me. Will just upgrade to Ruby 1.9.1 instead - been needing an excuse.

• You flagged this item as spam.
  

  Gregor Schmidt January 25th, 2010 @ 06:55 PM

  I am seeing this bug as well. The test case on http://gist.github.com/286087 is the shortest one, I could come up with.

  It fails with activesupport -v 2.3.5 on

  • Ruby 1.8.6-p287
  • Ruby 1.8.7-p248
  • JRuby 1.4
  • JRuby HEAD

  As far as I can tell, there's no direct relation to AR - it just uses the broken functionality of AS.

• 

  Martin Gamsjaeger (snusnu) January 26th, 2010 @ 03:17 AM

  • Tag changed from “active_record, active_support, hash, serialization, yaml” to “active_record, active_support, datamapper, hash, serialization, yaml”

  I can confirm this too. I don't use AR, I'm a datamapper user, but it happens when I run specs for dm-serializer from branches of dm-core and dm-more that work with active_support

  http://github.com/snusnu/dm-core/tree/active_support
  http://github.com/snusnu/dm-more/tree/active_support

  Now the cool thing is that this is the only failing spec for the whole of dm-core and dm-more specs, when running with active_support instead of extlib.

  I would really appreciate a fix for this, since that would mean that datamapper is 100% compatible with active_support and my patches could safely be pulled into dm-core and dm-more repositories. This, in combination with http://github.com/dkubb/rails3_datamapper would mean that datamapper is fully usable with current rails master.

  Here's what I get:

  undefined method `each' for nil:NilClass
  /Users/snusnu/.rvm/gems/ree/1.8.7%rails3/gems/activesupport-3.0.pre/lib/active_support/ordered_hash.rb:97:in `each'
  

• You flagged this item as spam.
  

  Dan Kubb (dkubb) January 26th, 2010 @ 06:45 AM

  Based on Gregor's test case reduction, it looks like YAML.load is creating the OrderedHash object without calling #initialize (or for that matter the allocate or new class methods).

  Here's a simple work-around I found that causes Gregor's test to pass:

    class ActiveSupport::OrderedHash
      def yaml_initialize(*)
        initialize()  # initialize @keys
        super
      end
    end
  

  I would submit a patch, but I wanted to understand why YAML is doing this. Is it a bug, or was it on purpose? I mean, I can understand why it might not call new directly, but why is it not calling allocate? (and how is it doing that... is there some part of the MRI C API that allows it to allocate a Ruby object without calling any of the normal methods?) I can't tell if it's bypassing things on purpose, with a valid reason, or not.

  I also couldn't really find any documentation on yaml_initialize. I found it by looking through the source. It seems to be reasonably well used in other parts of YAML, but I was cautious to recommend using it unless my usage is valid.

• You flagged this item as spam.
  

  Gregor Schmidt January 26th, 2010 @ 09:17 AM

  Okay, thanks for the work-around. It already solves my immediate problem. But unfortunately it does not restore the key ordering within the hash, which would be the main feature of this class, I guess.

  I have attached a patch against 2-3-stable containing two failing test cases. One of them is fixed by the above patch, the other is still broken.

  Regarding your concerns about the usage of hash_initialize: I guess it is save to use this feature. It's used all over the place in YAML so I would not expect it to disappear.

  • test_cases_testing_each_and_ordering.diff 885 Bytes

• You flagged this item as spam.
  

  Gregor Schmidt January 26th, 2010 @ 11:07 AM

  So, I finally got a little closer.

  First: the above test cases fail both with Dan's workaround.

  Second: I managed to find two alternative fixes for the underlying problem. One uses the "yaml_initialize" method, the other uses "add_domain_type" which seems to be the official way to serialize and deserialize custom objects with syck.

  I have attached complete patches against 2-3-stable for both implementations.

  • patch_using_yaml_add_domain_type.diff 1.9 KB
  • patch_using_yaml_initialize.diff 1.7 KB
• 

  Martin Gamsjaeger (snusnu) January 26th, 2010 @ 02:55 PM

  • State changed from “new” to “verified”

  Thx a lot Gregor! I tried both your patches, both of them apply cleanly against current rails master too, and both of them solve the failing spec in dm-more/dm-serializer. Now if only I knew which one is better suited for being applied, I would vote for one of them. Since I don't know it, I'll leave that up for others to decide :)

  Again, thx Gregor for taking the time to investigate this!

  PS: I'm not sure which milestone to assign, since I guess we definitely want to have that in 3.0 but in 2.x too. I hope changing the Ticket state to verified is ok, since I can definitely verify the original bug and the fix, but I'm new to the rules of rails' ticket handling (probably that's also the reason why I left Priority to be low for the time being :)

• You flagged this item as spam.
  

  Dan Kubb (dkubb) January 26th, 2010 @ 06:07 PM

  I should also note that the YAML spec has defined an ordered map type that may fit this use case more closely. It's defined as an "Ordered sequence of key:value pairs without duplicates".

• You flagged this item as spam.
  

  Dan Kubb (dkubb) January 26th, 2010 @ 06:12 PM

  Another consideration that I had is should we make it so that this works the same between Ruby 1.8 and 1.9? (sorry for the extra post, I can't edit the comment I just submitted)

  From the way AS::OrderedHash is defined, it looks like it delegates straight to the Hash if Ruby 1.9 is being used. That would bypass the patched code Gregor submitted and just rely on whatever defaults the Ruby 1.9 Hash has. Depending on which approach is used, I think you'd want it to work the same between Ruby 1.8 and 1.9, so that if something was serialized with 1.8 it would be readable with someone using AS::OrderedHash with 1.9, and vice versa.

• You flagged this item as spam.
  

  Gregor Schmidt January 27th, 2010 @ 10:52 AM

  First of all, thanks for the reviews, Martin and Dan.

  Dan's remarks made me think again about the implementation strategy. As far as I can tell, there is only one way to allow an OrderedHash serialized in 1.9 to be correctly deserialized in 1.8 - we have to tag it specifically to be an ordered one. Therefore I now also subclassed Hash in 1.9 and - for all ruby version - added a custom serialization matching YAML's built-in ordered map. This way, anything expicitly instantiated as AS::OrderedHash will be serialized as !!omap and is loaded into an OrderedHash on the other side - full round trip. I think, this would not be possible using the "yaml_initialize"-strategy - therefore I dropped this implementation.

  The only problem, that may arise is, if other libraries start defining custom deserializations for !!omap - this would be a reason to not use "add_builtin_type" but "add_domain_type" with something like "ActiveSupport,2010/orderedHash".

  As far as I can tell, the attached patch passes on

  • 1.8.5
  • 1.8.6
  • 1.8.7
  • 1.9.1
  • JRuby 1.4.0

  I would welcome any feedback.

  • patch_using_yaml_add_domain_type.diff 2.3 KB
• 

  Martin Gamsjaeger (snusnu) January 27th, 2010 @ 05:26 PM

  • State changed from “verified” to “incomplete”

  Gregor,

  Again, thx for doing the work on this. I just tried your new patch - which btw looks like a good way of solving the problem to me - but unfortunately I got (different) failures in dm-more/dm-serializer specs. Here's what I get now:

  expected: {"name"=>["Name must be at least 2 characters long"], "solar_system_id"=>["Solar system must not be blank"]},
       got: {"name"=>"Name must be at least 2 characters long", "solar_system_id"=>"Solar system must not be blank"} (using ==)
  

  So it seems there's some problem with serializing (array) values? Oh and btw, active_support tests all pass with your patch.

• 

  Jeremy Kemper January 27th, 2010 @ 09:50 PM

  • Assigned user set to “Jeremy Kemper”
  • Milestone set to “2.3.6”

  +    ActiveSupport::OrderedHash[*val.map(&:to_a).flatten]
  

  Note that flatten is recursive. This will over-flatten array values.

• You flagged this item as spam.
  

  Gregor Schmidt January 27th, 2010 @ 10:37 PM

  Thanks for the hints. I hope, I've got it right this time.

  I've added a test with a nested array. It was failing with my latest patch. I then fixed the "flatten" problem. Please have a look at the newly attached patch.

• You flagged this item as spam.
  

  Gregor Schmidt January 27th, 2010 @ 10:41 PM

  • no changes were found...

  • patch_using_yaml_add_domain_type_v3.diff 2.7 KB
• 

  Martin Gamsjaeger (snusnu) January 27th, 2010 @ 11:04 PM

  • State changed from “incomplete” to “verified”

  Yes! Now all dm-core and dm-more specs pass with active_support :) Also, active_support tests pass for me. I should probably add that I haven't tested the scenario of serializing with 1.8 and deserializing with 1.9 (and vice versa), but I can confirm that the issues I previously had are now fixed.

  Thx for taking the time guys!

• 

  Repository January 27th, 2010 @ 11:31 PM

  • State changed from “verified” to “committed”

  (from [57337cd74db0596a1e9f8eb0e6df0bb8ccf53dd2]) Adding custom yaml (de-)serialization for OrderedHash

  [#3608 state:committed]

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

• 

  Repository January 27th, 2010 @ 11:31 PM

  (from [7b8c6472ffb6df4d341b0657f82e63c19e9a429c]) Adding custom yaml (de-)serialization for OrderedHash

  [#3608 state:committed]

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

• 

  Martin Gamsjaeger (snusnu) January 28th, 2010 @ 01:10 AM

  • State changed from “committed” to “incomplete”

  Jeremy,

  OrderedHash now requires yaml, and in order to be properly cherry pickable, ordered_hash.rb should require 'yaml'. (My datamapper sample app won't boot otherwise). The obviously trivial patch can be found at:

  http://github.com/snusnu/rails/commit/7e5379189e4398d91c77fb5eea119...

• 

  Jeremy Kemper January 28th, 2010 @ 02:14 AM

  • State changed from “incomplete” to “resolved”

  Thanks Martin!