Merge pull request #12746 from coreyward/master

Fix Hash#deep_merge bug and improve documentation — resolves #12738

2 files changed, 36 insertions, 15 deletions

diff --git a/activesupport/lib/active_support/core_ext/hash/deep_merge.rb b/activesupport/lib/active_support/core_ext/hash/deep_merge.rb
index dc86c92003..763d563231 100644
--- a/activesupport/lib/active_support/core_ext/hash/deep_merge.rb
+++ b/activesupport/lib/active_support/core_ext/hash/deep_merge.rb
@@ -1,27 +1,38 @@
 class Hash
   # Returns a new hash with +self+ and +other_hash+ merged recursively.
   #
-  #   h1 = { x: { y: [4, 5, 6] }, z: [7, 8, 9] }
-  #   h2 = { x: { y: [7, 8, 9] }, z: 'xyz' }
+  #   h1 = { a: true, b: { c: [1, 2, 3] } }
+  #   h2 = { a: false, b: { x: [3, 4, 5] } }
   #
-  #   h1.deep_merge(h2) # => {x: {y: [7, 8, 9]}, z: "xyz"}
-  #   h2.deep_merge(h1) # => {x: {y: [4, 5, 6]}, z: [7, 8, 9]}
-  #   h1.deep_merge(h2) { |key, old, new| Array.wrap(old) + Array.wrap(new) }
-  #   # => {:x=>{:y=>[4, 5, 6, 7, 8, 9]}, :z=>[7, 8, 9, "xyz"]}
+  #   h1.deep_merge(h2) #=> { a: false, b: { c: [1, 2, 3], x: [3, 4, 5] } }
+  #
+  # Like with Hash#merge in the standard library, a block can be provided
+  # to merge values:
+  #
+  #   h1 = { a: 100, b: 200, c: { c1: 100 } }
+  #   h2 = { b: 250, c: { c1: 200 } }
+  #   h1.deep_merge(h2) { |key, this_val, other_val| this_val + other_val }
+  #   # => { a: 100, b: 450, c: { c1: 300 } }
   def deep_merge(other_hash, &block)
     dup.deep_merge!(other_hash, &block)
   end
 
   # Same as +deep_merge+, but modifies +self+.
   def deep_merge!(other_hash, &block)
-    other_hash.each_pair do |k,v|
-      tv = self[k]
-      if tv.is_a?(Hash) && v.is_a?(Hash)
-        self[k] = tv.deep_merge(v, &block)
+    other_hash.each_pair do |current_key, other_value|
+      this_value = self[current_key]
+
+      self[current_key] = if this_value.is_a?(Hash) && other_value.is_a?(Hash)
+        this_value.deep_merge(other_value, &block)
       else
-        self[k] = block && tv ? block.call(k, tv, v) : v
+        if block_given? && key?(current_key)
+          block.call(current_key, this_value, other_value)
+        else
+          other_value
+        end
       end
     end
+
     self
   end
 end
diff --git a/activesupport/test/core_ext/hash_ext_test.rb b/activesupport/test/core_ext/hash_ext_test.rb
index d824a16e98..ad354a4c30 100644
--- a/activesupport/test/core_ext/hash_ext_test.rb
+++ b/activesupport/test/core_ext/hash_ext_test.rb
@@ -697,6 +697,16 @@ class HashExtTest < ActiveSupport::TestCase
     assert_equal expected, hash_1
   end
 
+  def test_deep_merge_with_falsey_values
+    hash_1 = { e: false }
+    hash_2 = { e: 'e' }
+    expected = { e: [:e, false, 'e'] }
+    assert_equal(expected, hash_1.deep_merge(hash_2) { |k, o, n| [k, o, n] })
+
+    hash_1.deep_merge!(hash_2) { |k, o, n| [k, o, n] }
+    assert_equal expected, hash_1
+  end
+
   def test_deep_merge_on_indifferent_access
     hash_1 = HashWithIndifferentAccess.new({ :a => "a", :b => "b", :c => { :c1 => "c1", :c2 => "c2", :c3 => { :d1 => "d1" } } })
     hash_2 = HashWithIndifferentAccess.new({ :a => 1, :c => { :c1 => 2, :c3 => { :d2 => "d2" } } })
@@ -905,11 +915,11 @@ class HashExtTest < ActiveSupport::TestCase
   def test_compact
     hash_contain_nil_value = @symbols.merge(z: nil)
     hash_with_only_nil_values = { a: nil, b: nil }
-    
+
     h = hash_contain_nil_value.dup
     assert_equal(@symbols, h.compact)
     assert_equal(hash_contain_nil_value, h)
-    
+
     h = hash_with_only_nil_values.dup
     assert_equal({}, h.compact)
     assert_equal(hash_with_only_nil_values, h)
@@ -918,11 +928,11 @@ class HashExtTest < ActiveSupport::TestCase
   def test_compact!
     hash_contain_nil_value = @symbols.merge(z: nil)
     hash_with_only_nil_values = { a: nil, b: nil }
-    
+
     h = hash_contain_nil_value.dup
     assert_equal(@symbols, h.compact!)
     assert_equal(@symbols, h)
-    
+
     h = hash_with_only_nil_values.dup
     assert_equal({}, h.compact!)
     assert_equal({}, h)