Rails Ruby on Rails → 2.3.9

Comments and changes to this ticket

• 

  Neeraj Singh May 26th, 2010 @ 03:53 PM

  It is properly fixed in rails3. However if you need this functionality in rails 2.3.8 then a quick hack would be like this. I tested it and it is working although did not do any extensive test.

  class HashWithIndifferentAccess 
    def deep_merge(other_hash)
      orig_hash = self
      other_hash.each_pair do |key, value| 
        key = convert_key(key)
        oldval = orig_hash[key]
        newval = convert_value(value)
        
        oldval = oldval.to_hash if oldval.respond_to?(:to_hash)
        newval = newval.to_hash if newval.respond_to?(:to_hash)
  
        merged_value = oldval.kind_of?(Hash) && newval.kind_of?(Hash) ?  oldval.deep_merge(newval) : newval
        orig_hash.regular_writer(key, merged_value)
      end
      self
    end
  end
  

• 

  Santiago Pastorino May 26th, 2010 @ 05:00 PM

  • Milestone set to “2.3.9”
  • State changed from “new” to “open”

  Please do a patch file acording to the guidelines and a patch could be nice too ;).
  You have the code there :).
  Thanks.

• 

  Neeraj Singh May 26th, 2010 @ 07:10 PM

  @Santiago I will have the patch and the test soon. Did not think that the fix would be backported since it is already fixed in rails3.

• 

  Santiago Pastorino May 26th, 2010 @ 07:24 PM

  Neeraj please review the latests tickets i think this one is duplicated

• 

  Neeraj Singh May 27th, 2010 @ 02:13 AM

  I could not find any other open ticket about the same issue.

  Anyway here is a patch with test against rails 2-3-stable.

  • 4702_fix_against_rails-2-3-stable.diff 2.5 KB

• You flagged this item as spam.
  

  Georg Ledermann May 27th, 2010 @ 10:36 AM

  IMHO this patch introduces another bug, because it changes self.

  Here is a failing test (based on my test above):

  class HashTest < ActiveSupport::TestCase
    def test_deep_merge_for_hash_with_indifferent_access
      hash1 = { :k => { :v1 => 1 } }.with_indifferent_access
      hash2 = { :k => { :v2 => 2 } }
  
      # First, check the result of a deep merge => OK
      assert_equal({ "k" => { "v1" => 1, "v2" => 2 } }, hash1.deep_merge(hash2))
  
      # Second, check if the originals are unchanged =====> FAILS!
      assert_equal({ "k" => { "v1" => 1 } }, hash1)
    end 
  end
  

• 

  Neeraj Singh May 27th, 2010 @ 02:28 PM

  @Georg Good catch.

  Attached is an updated patch. I have included your test condition in the test this time. Thanks for the review.

  • 4702_fix_against_2-3-stable.diff 2.8 KB

• You flagged this item as spam.
  

  Lawrence Pit May 31st, 2010 @ 05:40 AM

  If it's properly fixed in rails 3, why can't that code be used instead of the code proposed in the patch?

• 

  Neeraj Singh May 31st, 2010 @ 05:59 AM

  In rails3 the fix is much more elegant and requires changing a few more files.

  Patch that I have attached basically does the same thing.

• 

  Santiago Pastorino May 31st, 2010 @ 10:21 PM

  Neeraj this is not related with this issue https://rails.lighthouseapp.com/projects/8994/tickets/2732-deep_mer... i didn't review the issues but seems similar, please take a look

• 

  Neeraj Singh June 1st, 2010 @ 08:40 AM

  going to class Hash from module Hash requires a lot more changes than it seems.

• 

  Santiago Pastorino June 1st, 2010 @ 09:48 AM

  • State changed from “open” to “duplicate”

  #2732