Merge pull request rails#54437 from byroot/hwia-dirty-raw-set

Allow to skip HashWithIndifferentAccess value conversion

Author: byroot

activemodel/lib/active_model/attribute_mutation_tracker.rb

@@ -16,17 +16,17 @@ def changed_attribute_names
     end
 
     def changed_values
-      attr_names.each_with_object({}.with_indifferent_access) do |attr_name, result|
+      attr_names.each_with_object(ActiveSupport::HashWithIndifferentAccess.new) do |attr_name, result|
         if changed?(attr_name)
-          result[attr_name] = original_value(attr_name)
+          result.store(attr_name, original_value(attr_name), convert_value: false)
         end
       end
     end
 
     def changes
-      attr_names.each_with_object({}.with_indifferent_access) do |attr_name, result|
+      attr_names.each_with_object(ActiveSupport::HashWithIndifferentAccess.new) do |attr_name, result|
         if change = change_to_attribute(attr_name)
-          result[attr_name] = change
+          result.store(attr_name, change, convert_value: false)
         end
       end
     end

activerecord/lib/active_record/store.rb

@@ -146,37 +146,43 @@ def store_accessor(store_attribute, *keys, prefix: nil, suffix: nil)
             define_method("#{accessor_key}_changed?") do
               return false unless attribute_changed?(store_attribute)
               prev_store, new_store = changes[store_attribute]
-              prev_store&.dig(key) != new_store&.dig(key)
+              accessor = store_accessor_for(store_attribute)
+              accessor.get(prev_store, key) != accessor.get(new_store, key)
             end
 
             define_method("#{accessor_key}_change") do
               return unless attribute_changed?(store_attribute)
               prev_store, new_store = changes[store_attribute]
-              [prev_store&.dig(key), new_store&.dig(key)]
+              accessor = store_accessor_for(store_attribute)
+              [accessor.get(prev_store, key), accessor.get(new_store, key)]
             end
 
             define_method("#{accessor_key}_was") do
               return unless attribute_changed?(store_attribute)
               prev_store, _new_store = changes[store_attribute]
-              prev_store&.dig(key)
+              accessor = store_accessor_for(store_attribute)
+              accessor.get(prev_store, key)
             end
 
             define_method("saved_change_to_#{accessor_key}?") do
               return false unless saved_change_to_attribute?(store_attribute)
               prev_store, new_store = saved_changes[store_attribute]
-              prev_store&.dig(key) != new_store&.dig(key)
+              accessor = store_accessor_for(store_attribute)
+              accessor.get(prev_store, key) != accessor.get(new_store, key)
             end
 
             define_method("saved_change_to_#{accessor_key}") do
               return unless saved_change_to_attribute?(store_attribute)
               prev_store, new_store = saved_changes[store_attribute]
-              [prev_store&.dig(key), new_store&.dig(key)]
+              accessor = store_accessor_for(store_attribute)
+              [accessor.get(prev_store, key), accessor.get(new_store, key)]
             end
 
             define_method("#{accessor_key}_before_last_save") do
               return unless saved_change_to_attribute?(store_attribute)
               prev_store, _new_store = saved_changes[store_attribute]
-              prev_store&.dig(key)
+              accessor = store_accessor_for(store_attribute)
+              accessor.get(prev_store, key)
             end
           end
         end
@@ -225,39 +231,58 @@ def store_accessor_for(store_attribute)
       end
 
       class HashAccessor # :nodoc:
+        def self.get(store_object, key)
+          if store_object
+            store_object[key]
+          end
+        end
+
         def self.read(object, attribute, key)
-          prepare(object, attribute)
-          object.public_send(attribute)[key]
+          store_object = prepare(object, attribute)
+          store_object[key]
         end
 
         def self.write(object, attribute, key, value)
-          prepare(object, attribute)
-          object.public_send(attribute)[key] = value if value != read(object, attribute, key)
+          store_object = prepare(object, attribute)
+          store_object[key] = value if value != store_object[key]
         end
 
         def self.prepare(object, attribute)
-          object.public_send :"#{attribute}=", {} unless object.send(attribute)
+          store_object = object.public_send(attribute)
+
+          if store_object.nil?
+            store_object = {}
+            object.public_send(:"#{attribute}=", store_object)
+          end
+
+          store_object
         end
       end
 
       class StringKeyedHashAccessor < HashAccessor # :nodoc:
+        def self.get(store_object, key)
+          super store_object, Symbol === key ? key.name : key.to_s
+        end
+
         def self.read(object, attribute, key)
-          super object, attribute, key.to_s
+          super object, attribute, Symbol === key ? key.name : key.to_s
         end
 
         def self.write(object, attribute, key, value)
-          super object, attribute, key.to_s, value
+          super object, attribute, Symbol === key ? key.name : key.to_s, value
         end
       end
 
       class IndifferentHashAccessor < ActiveRecord::Store::HashAccessor # :nodoc:
-        def self.prepare(object, store_attribute)
-          attribute = object.send(store_attribute)
-          unless attribute.is_a?(ActiveSupport::HashWithIndifferentAccess)
-            attribute = IndifferentCoder.as_indifferent_hash(attribute)
-            object.public_send :"#{store_attribute}=", attribute
+        def self.prepare(object, attribute)
+          store_object = object.public_send(attribute)
+
+          unless store_object.is_a?(ActiveSupport::HashWithIndifferentAccess)
+            store_object = IndifferentCoder.as_indifferent_hash(store_object)
+            object.public_send :"#{attribute}=", store_object
           end
-          attribute
+
+          store_object
         end
       end
 

activerecord/test/cases/store_test.rb

@@ -31,6 +31,14 @@ class StoreTest < ActiveRecord::TestCase
     assert_equal "37signals.com", @john.homepage
   end
 
+  test "reading store attributes with indifferent access" do
+    @john.preferences = { "remember_login" => "yes" }
+    assert_equal "yes", @john.remember_login
+
+    @john.preferences = { remember_login: "no" }
+    assert_equal "no", @john.remember_login
+  end
+
   test "writing store attributes does not update unchanged value" do
     admin_user = Admin::User.new(homepage: nil)
     admin_user.homepage = nil

activerecord/test/models/admin/user.rb

@@ -16,18 +16,28 @@ def load(s)
   end
 
   belongs_to :account
+
   store :params, accessors: [ :token ], coder: YAML
+
   store :settings, accessors: [ :color, :homepage ]
   store_accessor :settings, :favorite_food
+
   store :parent, accessors: [:birthday, :name], prefix: true
+
   store :spouse, accessors: [:birthday], prefix: :partner
   store_accessor :spouse, :name, prefix: :partner
+
   store :configs, accessors: [ :secret_question ]
   store :configs, accessors: [ :two_factor_auth ], suffix: true
   store_accessor :configs, :login_retry, suffix: :config
-  store :preferences, accessors: [ :remember_login ]
+
+  serialize :preferences, type: Hash, coder: Coder.new
+  store_accessor :preferences, :remember_login
+
   store :json_data, accessors: [ :height, :weight ], coder: Coder.new
+
   store :json_data_empty, accessors: [ :is_a_good_guy ], coder: Coder.new
+
   store_accessor :json_options, :enable_friend_requests
 
   def phone_number

activesupport/lib/active_support/hash_with_indifferent_access.rb

@@ -95,11 +95,27 @@ def self.[](*args)
     #   hash[:key] = 'value'
     #
     # This value can be later fetched using either +:key+ or <tt>'key'</tt>.
+    #
+    # If the value is a Hash or contains one or multiple Hashes, they will be
+    # converted to +HashWithIndifferentAccess+.
     def []=(key, value)
       regular_writer(convert_key(key), convert_value(value, conversion: :assignment))
     end
 
-    alias_method :store, :[]=
+    # Assigns a new value to the hash:
+    #
+    #   hash = ActiveSupport::HashWithIndifferentAccess.new
+    #   hash[:key] = 'value'
+    #
+    # This value can be later fetched using either +:key+ or <tt>'key'</tt>.
+    #
+    # If the value is a Hash or contains one or multiple Hashes, they will be
+    # converted to +HashWithIndifferentAccess+. unless `convert_value: false`
+    # is set.
+    def store(key, value, convert_value: true)
+      value = convert_value(value, conversion: :assignment) if convert_value
+      regular_writer(convert_key(key), value)
+    end
 
     # Updates the receiver in-place, merging in the hashes passed as arguments:
     #