HashWithIndifferentAccess#update respects #to_hash #12550

Merged
merged 1 commit into from Mar 28, 2014

4 participants

@Peeja

Hash#update converts its argument with #to_hash before looking at its values.
This change makes HashWithIndifferentAccess do the same thing.

I'd like to write a test to go with this, but I couldn't find a test for
HashWithIndifferentAccess. Is there one?


  • Test (in test/core_ext/has_ext_test.rb)
  • Changelog entry
@robin850
Ruby on Rails member

Thanks for your patch!

I think that this will need a changelog entry. Could you add one please ?

You can write test for HashWithIndifferentAccess in test/core_ext/has_ext_test.rb. To be honest, I didn't know where it was but if you need to find something, the easiest way is to use the grep command:

$ cd activesupport/test
$ grep -Rn HashWith
@Peeja

Ah, there it is. Thanks! And I'll add a changelog entry.

@Peeja

5 months later…

Tested and Changelogged. I also extended a couple of other parts of HWIA similarly. I think that should cover it.

@arthurnn
Ruby on Rails member

I am ok with this change, not sure though if HWIA is one of the classes we dont wanna change.
anyways, squash in 1 commit, and 👍 on my side

@Peeja Peeja HashWithIndifferentAccess better respects #to_hash
In particular, `.new`, `#update`, `#merge`, `#replace` all accept
objects which respond to `#to_hash`, even if those objects are not
Hashes directly.
03f35a2
@Peeja

Squashed.

The only potential danger I see is with .new. In master, giving .new a non-Hash which responds to #to_hash will make an empty HWIA with the given object as its default value. After this change, it will convert the object to a hash and add that hash's keys and values to itself. It's possible (though I think unlikely) that client code depends on the original behavior.

The other methods, in master, raise errors if they're given non-Hashes, so I think it's safe to assume no one's depending on that behavior.

@rafaelfranca rafaelfranca commented on the diff Mar 28, 2014
...rt/lib/active_support/hash_with_indifferent_access.rb
@@ -72,6 +72,7 @@ def default(key = nil)
end
def self.new_from_hash_copying_default(hash)
+ hash = hash.to_hash
@rafaelfranca
Ruby on Rails member

Do we need to call to_hash here? Would not the new called below do the same?

@Peeja
Peeja added a note Mar 28, 2014

It does, but we also call hash.default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rafaelfranca
Ruby on Rails member

In master, giving .new a non-Hash which responds to #to_hash will make an empty HWIA with the given object as its default value. After this change, it will convert the object to a hash and add that hash's keys and values to itself. It's possible (though I think unlikely) that client code depends on the original behavior.

For this reason I'll only merge this on master. Introducing this kind of change in a stable branch can lead to problems.

@Peeja

That sounds reasonable. Is it worth preparing a version that doesn't include the .new part for the stable branch?

@rafaelfranca rafaelfranca merged commit 74d7df1 into rails:master Mar 28, 2014

1 check passed

Details default The Travis CI build passed
@rafaelfranca
Ruby on Rails member

@Peeja that could be great. Could you open a new PR against 4-1-stable?

@Peeja

PRed

@gchan gchan added a commit to gchan/rails that referenced this pull request Jul 31, 2014
@gchan gchan `HashWithIndifferentAccess.new` respects the default value or proc on…
… objects that respond to `#to_hash`.

Builds on the work of #12550 where `.new` will convert the object (that respond to `#to_hash`) to a hash and
add that hash's keys and values to itself.

This change will also make `.new` respect the default value or proc of objects that respond to `#to_hash`.
In other words, this `.new` behaves exactly like `.new_from_hash_copying_default`.

`.new_from_hash_copying_default` now simply invokes `.new` and any references to `.new_from_hash_copying_default`
are replaced with `.new`.

Added tests confirm behavior.
6e574e8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment