Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deep duplicate in deep_merge so no references remain to the original hash in the result #41931

Merged

Conversation

MarcelEeken
Copy link
Contributor

@MarcelEeken MarcelEeken commented Apr 12, 2021

Summary

The documentation for deep_merge says that a new hash is returned. But when deep merging a Hash with a nested Hash, the nested Hash is actually a reference to the original nested Hash, only a shallow copy is created.

This can lead to unexpected modifications when the nested Hash of the merged result 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" } }

Other Information

Reproduce case:

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  # Activate the gem you are reporting the issue against.
  # gem "activesupport", "6.1.0"
  gem "rails", github: "rails/rails", branch: "main"
end

require "active_support"
require "active_support/core_ext/object/blank"
require "minitest/autorun"

class BugTest < Minitest::Test
  def test_stuff
    x = { a: { b: "foo" } }
    y = { d: { e: "bar" } }

    z = x.deep_merge(y)
    z[:a][:b] = "baz"

    assert_equal "foo", x[:a][:b], "Changing 'z' should not change 'x'"
  end
end

…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" } }
```
@MarcelEeken MarcelEeken force-pushed the deep-merge-changing-original-hash branch from 8ce559d to 71ab41a Compare April 12, 2021 13:04
@rafaelfranca rafaelfranca merged commit 483d36a into rails:main Apr 12, 2021
rafaelfranca added a commit that referenced this pull request Apr 12, 2021
…al-hash

Deep duplicate in `deep_merge` so no references remain to the original hash in the result
rafaelfranca added a commit that referenced this pull request Apr 12, 2021
…al-hash

Deep duplicate in `deep_merge` so no references remain to the original hash in the result
@casperisfine
Copy link
Contributor

Hum, this is breaking things in our test suite:

>> {foo: Object}.deep_merge({})
=> {:foo=>#<Class:0x00007f8f96a66da0>}

I think we should only dup hashes, not their content. I'll submit a PR.

casperisfine pushed a commit to Shopify/rails that referenced this pull request Apr 13, 2021
Module and Class are technically duplicable,
however the copy will be anonymous which can have consequences.

Also improve rails#41931
to also protect the merged hash from mutation.
byroot added a commit that referenced this pull request Apr 13, 2021
…g-original-hash"

This reverts commit 483d36a, reversing
changes made to b171b84.
byroot added a commit that referenced this pull request Apr 13, 2021
byroot added a commit that referenced this pull request Apr 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants