Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fix lookup on HashWithIndifferentAccess for array values. #3811

Closed
wants to merge 1 commit into from

3 participants

@zetter

Currently HashWithIndifferentAccess has some odd behaviour that differs from Hash.

The problem:
Accessing a HashWithIndifferentAccess does not return the the same object that is stored in the hash (i.e. equal?) causing unexpected results:

hash = HashWithIndifferentAccess.new {|h, k| h[k] = []}
hash[:a] << 1  # => [1]
hash[:a]       # => [], expected [1]

The cause:
When a block is provided to generate default values the generated values are duped if they are arrays. The duped value is stored in the hash but the original value is returned when the hash is accessed.

My fix:
The duping is there for allowing frozen arrays containing hashes to be modified. My fix restricts the duping to this case. Note that if default function generates a frozen array an error will be raised on assignment before and after my patch.

@nashby

:+1: /cc @fxn

@carlosantoniodasilva carlosantoniodasilva commented on the diff
...rt/lib/active_support/hash_with_indifferent_access.rb
@@ -153,7 +153,8 @@ def convert_value(value)
if value.is_a? Hash
value.nested_under_indifferent_access
elsif value.is_a?(Array)
- value.dup.replace(value.map { |e| convert_value(e) })
+ value = value.dup if value.frozen?
+ value.replace(value.map { |e| convert_value(e) })

I think you could just use value.map! instead?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva carlosantoniodasilva closed this pull request from a commit
@carlosantoniodasilva carlosantoniodasilva Merge branch 'hash_with_indifferent_access_fix'
The problem:
Accessing a HashWithIndifferentAccess does not return the the same object
that is stored in the hash (i.e. equal?) causing unexpected results:

    hash = HashWithIndifferentAccess.new {|h, k| h[k] = []}
    hash[:a] << 1  # => [1]
    hash[:a]       # => [], expected [1]

The cause:
When a block is provided to generate default values the generated values
are duped if they are arrays. The duped value is stored in the hash but
the original value is returned when the hash is accessed.

The fix:
The duping is there for allowing frozen arrays containing hashes to be
modified. The fix restricts the duping to this case. Note that if default
function generates a frozen array an error will be raised on assignment
before and after the patch.

Closes #3811
27411a7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 30, 2011
  1. @zetter
This page is out of date. Refresh to see the latest.
View
3  activesupport/lib/active_support/hash_with_indifferent_access.rb
@@ -153,7 +153,8 @@ def convert_value(value)
if value.is_a? Hash
value.nested_under_indifferent_access
elsif value.is_a?(Array)
- value.dup.replace(value.map { |e| convert_value(e) })
+ value = value.dup if value.frozen?
+ value.replace(value.map { |e| convert_value(e) })

I think you could just use value.map! instead?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
else
value
end
View
7 activesupport/test/core_ext/hash_ext_test.rb
@@ -268,6 +268,13 @@ def test_indifferent_to_hash
assert_equal '1234', roundtrip.default
end
+ def test_lookup_returns_the_same_object_that_is_stored_in_hash_indifferent_access
+ hash = HashWithIndifferentAccess.new {|h, k| h[k] = []}
+ hash[:a] << 1
+
+ assert_equal [1], hash[:a]
+ end
+
def test_indifferent_hash_with_array_of_hashes
hash = { "urls" => { "url" => [ { "address" => "1" }, { "address" => "2" } ] }}.with_indifferent_access
assert_equal "1", hash[:urls][:url].first[:address]
Something went wrong with that request. Please try again.