Merge pull request #6368 from Antiarchitect/store-indifferent-access

Support migrating from Hash to HashWithIndifferentAccess for accessing attributes.

2 files changed, 61 insertions, 10 deletions

diff --git a/activerecord/lib/active_record/store.rb b/activerecord/lib/active_record/store.rb
index ce2ea85ef9..fdd82b489a 100644
--- a/activerecord/lib/active_record/store.rb
+++ b/activerecord/lib/active_record/store.rb
@@ -1,3 +1,5 @@
+require 'active_support/core_ext/hash/indifferent_access'
+
 module ActiveRecord
   # Store gives you a thin wrapper around serialize for the purpose of storing hashes in a single column.
   # It's like a simple key/value store backed into your record when you don't care about being able to
@@ -13,9 +15,6 @@ module ActiveRecord
   # You can set custom coder to encode/decode your serialized attributes to/from different formats.
   # JSON, YAML, Marshal are supported out of the box. Generally it can be any wrapper that provides +load+ and +dump+.
   #
-  # String keys should be used for direct access to virtual attributes because of most of the coders do not
-  # distinguish symbols and strings as keys.
-  #
   # Examples:
   #
   #   class User < ActiveRecord::Base
@@ -23,8 +22,12 @@ module ActiveRecord
   #   end
   #
   #   u = User.new(color: 'black', homepage: '37signals.com')
-  #   u.color                           # Accessor stored attribute
-  #   u.settings['country'] = 'Denmark' # Any attribute, even if not specified with an accessor
+  #   u.color                          # Accessor stored attribute
+  #   u.settings[:country] = 'Denmark' # Any attribute, even if not specified with an accessor
+  #
+  #   # There is no difference between strings and symbols for accessing custom attributes
+  #   u.settings[:country]  # => 'Denmark'
+  #   u.settings['country'] # => 'Denmark'
   #
   #   # Add additional accessors to an existing store through store_accessor
   #   class SuperUser < User
@@ -35,24 +38,38 @@ module ActiveRecord
 
     module ClassMethods
       def store(store_attribute, options = {})
-        serialize store_attribute, options.fetch(:coder, Hash)
+        serialize store_attribute, options.fetch(:coder, ActiveSupport::HashWithIndifferentAccess)
         store_accessor(store_attribute, options[:accessors]) if options.has_key? :accessors
       end
 
       def store_accessor(store_attribute, *keys)
         keys.flatten.each do |key|
           define_method("#{key}=") do |value|
-            send("#{store_attribute}=", {}) unless send(store_attribute).is_a?(Hash)
-            send(store_attribute)[key.to_s] = value
+            initialize_store_attribute(store_attribute)
+            send(store_attribute)[key] = value
             send("#{store_attribute}_will_change!")
           end
 
           define_method(key) do
-            send("#{store_attribute}=", {}) unless send(store_attribute).is_a?(Hash)
-            send(store_attribute)[key.to_s]
+            initialize_store_attribute(store_attribute)
+            send(store_attribute)[key]
           end
         end
       end
     end
+
+    private
+      def initialize_store_attribute(store_attribute)
+        case attribute = send(store_attribute)
+        when ActiveSupport::HashWithIndifferentAccess
+          # Already initialized. Do nothing.
+        when Hash
+          # Initialized as a Hash. Convert to indifferent access.
+          send :"#{store_attribute}=", attribute.with_indifferent_access
+        else
+          # Uninitialized. Set to an indifferent hash.
+          send :"#{store_attribute}=", ActiveSupport::HashWithIndifferentAccess.new
+        end
+      end
   end
 end
diff --git a/activerecord/test/cases/store_test.rb b/activerecord/test/cases/store_test.rb
index e1d0f1f799..3a5d84df9f 100644
--- a/activerecord/test/cases/store_test.rb
+++ b/activerecord/test/cases/store_test.rb
@@ -41,6 +41,40 @@ class StoreTest < ActiveRecord::TestCase
     assert_equal false, @john.remember_login
   end
 
+  test "preserve store attributes data in HashWithIndifferentAccess format without any conversion" do
+    @john.json_data = HashWithIndifferentAccess.new(:height => 'tall', 'weight' => 'heavy')
+    @john.height = 'low'
+    assert_equal true, @john.json_data.instance_of?(HashWithIndifferentAccess)
+    assert_equal 'low', @john.json_data[:height]
+    assert_equal 'low', @john.json_data['height']
+    assert_equal 'heavy', @john.json_data[:weight]
+    assert_equal 'heavy', @john.json_data['weight']
+  end
+
+  test "convert store attributes from Hash to HashWithIndifferentAccess saving the data and access attributes indifferently" do
+    @john.json_data = { :height => 'tall', 'weight' => 'heavy' }
+    assert_equal true, @john.json_data.instance_of?(Hash)
+    assert_equal 'tall', @john.json_data[:height]
+    assert_equal nil, @john.json_data['height']
+    assert_equal nil, @john.json_data[:weight]
+    assert_equal 'heavy', @john.json_data['weight']
+    @john.height = 'low'
+    assert_equal true, @john.json_data.instance_of?(HashWithIndifferentAccess)
+    assert_equal 'low', @john.json_data[:height]
+    assert_equal 'low', @john.json_data['height']
+    assert_equal 'heavy', @john.json_data[:weight]
+    assert_equal 'heavy', @john.json_data['weight']
+  end
+
+  test "convert store attributes from any format other than Hash or HashWithIndifferent access losing the data" do
+    @john.json_data = "somedata"
+    @john.height = 'low'
+    assert_equal true, @john.json_data.instance_of?(HashWithIndifferentAccess)
+    assert_equal 'low', @john.json_data[:height]
+    assert_equal 'low', @john.json_data['height']
+    assert_equal false, @john.json_data.delete_if { |k, v| k == 'height' }.any?
+  end
+
   test "reading store attributes through accessors encoded with JSON" do
     assert_equal 'tall', @john.height
     assert_nil @john.weight