Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Only convert direct hash instances in hash with indifferent access.

  • Loading branch information...
commit ce9456eca0c4ea77a42aaad5e8080842c1c01422 1 parent 9332cc5
@josevalim josevalim authored
View
7 activesupport/lib/active_support/hash_with_indifferent_access.rb
@@ -140,11 +140,10 @@ def convert_key(key)
end
def convert_value(value)
- case value
- when Hash
+ if value.class == Hash
self.class.new_from_hash_copying_default(value)
- when Array
- value.dup.replace(value.collect { |e| e.is_a?(Hash) ? self.class.new_from_hash_copying_default(e) : e })
+ elsif value.is_a?(Array)
+ value.dup.replace(value.map { |e| convert_value(e) })
else
value
end
View
8 activesupport/test/core_ext/hash_ext_test.rb
@@ -12,6 +12,9 @@ class IndifferentHash < HashWithIndifferentAccess
class SubclassingArray < Array
end
+ class SubclassingHash < Hash
+ end
+
def setup
@strings = { 'a' => 1, 'b' => 2 }
@symbols = { :a => 1, :b => 2 }
@@ -105,6 +108,11 @@ def test_stringify_keys_bang_for_hash_with_indifferent_access
assert_equal @strings, @mixed.with_indifferent_access.dup.stringify_keys!
end
+ def test_hash_subclass
+ flash = { "foo" => SubclassingHash.new.tap { |h| h["bar"] = "baz" } }.with_indifferent_access
+ assert_kind_of SubclassingHash, flash["foo"]
+ end
+
def test_indifferent_assorted
@strings = @strings.with_indifferent_access
@symbols = @symbols.with_indifferent_access

14 comments on commit ce9456e

@dlee

@josevalim What was the rationale for this change? It breaks nested conversion from BSON::OrderedHash.

@josevalim
Owner

afaik, It was converting ordered hashes to hash with indifferent access not preserving the order. @spastorino, can you confirm?

@spastorino
Owner

should I know about this? I don't remember :P.

@dlee

Just to clarify, the way the change breaks BSON::OrderedHash is that it won't convert BSON::OrderedHash objects that are values within the top-level hash.

From what I can tell, under current and previous code, using Ruby-1.9 should maintain the order, while using 1.8 is undefined. The only difference is whether or not nested objects of a subclass of Hash will also get converted.

@josevalim
Owner

Exactly. If you are inheriting from HWIA, you can override convert_to_value so it also converts BSON stuff and call super. If not, you can provide a patch that to this convert_values that uses duck typing and asks the object if it should be converted or not.

@dlee

What do you say to just allowing all nested objects of subclasses of Hash to be converted, like the old behavior?

Otherwise, we can check every object if it responds to #with_indifferent_access and call that.

The second option would have a slight performance impact compared to the old behavior, but it looks to be the correct behavior.

@josevalim
Owner

It was changed due to a bug. Although I do not recall it. I certainy wouldn't change this class otherwise. And we cannot use .with_indifferent_access because it is implemented in Hash.

@dlee

Doh. We can go with option 1. Would the bug be uncovered by the Rails tests if we were to revert the behavior?

@josevalim
Owner

I don't think so. But I think the issue is that one Flash/Cookies/Session are a hash with indifferent access and that was causing them to modify OrderedHash if any was stored. In any case, I do think that modifying all subclasses of hashes will potentially change their behavior.

@dlee

I think the proper solution is to use .with_indifferent_access for conversion of nested values. A subclass of Hash must then override .with_indifferent_access to return an indifferent-access version of itself. Otherwise, it will be converted into a HashWithIndifferentAccess.

for example:

{a: OrdererdHash.new(b: 'c')}.with_indifferent_access['a'].class # => HashWithIndifferentAccess

class OrderedHashWithIndifferentAccess < OrderedHash
  ...
end

class OrderedHash
  def with_indifferent_access
    OrderedHashWithIndifferentAccess.new self
  end
end

{a: OrdererdHash.new(b: 'c')}.with_indifferent_access['a'].class # => OrderedHashWithIndifferentAccess

I think this follows proper expectations of users as well as obeying class hierarchy best practices in OOP.

What do you think?

@josevalim
Owner

It makes me sense. It should not be hard to extract HashWithIndifferentAccess methods to a module and create an OrderedHashWithIndifferentAccess on top of an OrderedHash. It is just a pity to have to do all those things because of Ruby 1.8.x.

@dlee

More options:

Option 1: Nest indifferent access by default

class HashWithIndifferentAccess
  def convert_value(value)
    if value.respond_to? :with_indifferent_access
      value.with_indifferent_access
    elsif value.is_a?(Array)
      value.dup.replace(value.map { |e| convert_value(e) })
    else
      value
    end
  end
end 

class OrderedHash
  # specifically opt-out of nested indifferent access
  undef with_indifferent_access
end

Option 2: Opt-in to nested indifferent access

class Hash
  def can_indifferent_access?
    self.class == Hash
  end
end

class HashWithIndifferentAccess
  def convert_value(value)
    if value.is_a?(Hash) && value.can_indfferent_access?
      value.with_indifferent_access
    elsif value.is_a?(Array)
      value.dup.replace(value.map { |e| convert_value(e) })
    else
      value
    end
  end
end 

class OrderedHash
  # specifically opt-in for indifferent access
  def can_indifferent_access?
    true
  end
end
@josevalim
Owner

with_indifferent_access() is public API and if someone wants to explicitly convert an OrderedHash to one with indifferent access, he should be able to. So we should go with option 2 but instead of explicitly opt-in, we could make explicit opt-out for backwards compatibility. It can be something like: hash.respond_to?(:skip_implicit_indifferent_access_conversion) that would be implemented only in OrderedHash at first.

@dlee

I just sent pull request #454.

Please sign in to comment.
Something went wrong with that request. Please try again.