Skip to content

Commit

Permalink
Deep duplicate in deep_merge so no references remain to the origina…
Browse files Browse the repository at this point in the history
…l hash in the result

When deep merging a Hash, first a duplicate is created. The duplicate only
does a shallow copy, so deeper level structures are not duplicated but
remain references.

This can lead to unexpected modifications of the original hash when a deeply
nested hash is merged with another hash, and the newly returned hash gets modified.
```
  x = { a: { b: "foo" } }
  y = { d: { e: "bar" } }

  z = x.deep_merge(y)
  # z => { a: { b: "foo" }, d: { e: "bar" } }

  z[:a][:b] = "baz"
  # z => { a: { b: "baz" }, d: { e: "bar" } }
  # x => { a: { b: "baz" } }
```
  • Loading branch information
MarcelEeken committed Apr 12, 2021
1 parent 3b1fae4 commit 71ab41a
Show file tree
Hide file tree
Showing 3 changed files with 15 additions and 1 deletion.
4 changes: 4 additions & 0 deletions activesupport/CHANGELOG.md
@@ -1,3 +1,7 @@
* Fix issue in `Hash#deep_merge` where it did not properly duplicate a nested `Hash`

*Marcel Eeken*

* Add `expires_at` argument to `ActiveSupport::Cache` `write` and `fetch` to set a cache entry TTL as an absolute time.

```ruby
Expand Down
Expand Up @@ -16,7 +16,7 @@ class Hash
# 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)
deep_dup.deep_merge!(other_hash, &block)
end

# Same as +deep_merge+, but modifies +self+.
Expand Down
10 changes: 10 additions & 0 deletions activesupport/test/core_ext/hash_ext_test.rb
Expand Up @@ -300,6 +300,16 @@ def test_deep_merge_with_falsey_values
assert_equal expected, hash_1
end

def test_deep_merge_with_nested_hash_returning_full_new_hash
hash_1 = { a: { b: "foo" } }
hash_2 = { d: "bar" }

new_hash = hash_1.deep_merge(hash_2)
new_hash[:a][:b] = "baz"

assert_equal("foo", hash_1[:a][:b])
end

def test_reverse_merge
defaults = { d: 0, a: "x", b: "y", c: 10 }.freeze
options = { a: 1, b: 2 }
Expand Down

0 comments on commit 71ab41a

Please sign in to comment.