Merge pull request #12490 from senny/store_accessor_hstore_bug

ActiveRecord Store works with PG `hstore` columns

activerecord/CHANGELOG.md

+*   `ActiveRecord::Store` works together with PG `hstore` columns.
+    Fixes #12452.
+
+    *Yves Senn*
+
+*   Fix bug where `ActiveRecord::Store` used a global `Hash` to keep track of
+    all registered `stored_attributes`. Now every subclass of
+    `ActiveRecord::Base` has it's own `Hash`.
+
+    *Yves Senn*
+
 *   Save `has_one` association when primary key is manually set.
 
     Fixes #12302.

activerecord/lib/active_record/attribute_methods/serialization.rb

@@ -66,6 +66,10 @@ def type_cast(value)
         def type
           @column.type
         end
+
+        def accessor
+          ActiveRecord::Store::IndifferentHashAccessor
+        end
       end
 
       class Attribute < Struct.new(:coder, :value, :state) # :nodoc:

activerecord/lib/active_record/connection_adapters/postgresql/oid.rb

@@ -234,6 +234,10 @@ def type_cast(value)
 
             ConnectionAdapters::PostgreSQLColumn.string_to_hstore value
           end
+
+          def accessor
+            ActiveRecord::Store::StringKeyedHashAccessor
+          end
         end
 
         class Cidr < Type
@@ -250,6 +254,10 @@ def type_cast(value)
 
             ConnectionAdapters::PostgreSQLColumn.string_to_json value
           end
+
+          def accessor
+            ActiveRecord::Store::StringKeyedHashAccessor
+          end
         end
 
         class TypeMap

activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb

@@ -148,6 +148,10 @@ def type_cast(value)
         @oid_type.type_cast value
       end
 
+      def accessor
+        @oid_type.accessor
+      end
+
       private
 
         def has_default_function?(default_value, default)

activerecord/lib/active_record/store.rb

@@ -86,6 +86,9 @@ def store_accessor(store_attribute, *keys)
           end
         end
 
+        # assign new store attribute and create new hash to ensure that each class in the hierarchy
+        # has its own hash of stored attributes.
+        self.stored_attributes = {} if self.stored_attributes.blank?
         self.stored_attributes[store_attribute] ||= []
         self.stored_attributes[store_attribute] |= keys
       end
@@ -101,26 +104,58 @@ def _store_accessors_module
 
     protected
       def read_store_attribute(store_attribute, key)
-        attribute = initialize_store_attribute(store_attribute)
-        attribute[key]
+        accessor = store_accessor_for(store_attribute)
+        accessor.read(self, store_attribute, key)
       end
 
       def write_store_attribute(store_attribute, key, value)
-        attribute = initialize_store_attribute(store_attribute)
-        if value != attribute[key]
-          send :"#{store_attribute}_will_change!"
-          attribute[key] = value
-        end
+        accessor = store_accessor_for(store_attribute)
+        accessor.write(self, store_attribute, key, value)
       end
 
     private
-      def initialize_store_attribute(store_attribute)
-        attribute = send(store_attribute)
-        unless attribute.is_a?(ActiveSupport::HashWithIndifferentAccess)
-          attribute = IndifferentCoder.as_indifferent_hash(attribute)
-          send :"#{store_attribute}=", attribute
+      def store_accessor_for(store_attribute)
+        @column_types[store_attribute.to_s].accessor
+      end
+
+      class HashAccessor
+        def self.read(object, attribute, key)
+          prepare(object, attribute)
+          object.public_send(attribute)[key]
+        end
+
+        def self.write(object, attribute, key, value)
+          prepare(object, attribute)
+          if value != read(object, attribute, key)
+            object.public_send :"#{attribute}_will_change!"
+            object.public_send(attribute)[key] = value
+          end
+        end
+
+        def self.prepare(object, attribute)
+          object.public_send :"#{attribute}=", {} unless object.send(attribute)
+        end
+      end
+
+      class StringKeyedHashAccessor < HashAccessor
+        def self.read(object, attribute, key)
+          super object, attribute, key.to_s
+        end
+
+        def self.write(object, attribute, key, value)
+          super object, attribute, key.to_s, value
+        end
+      end
+
+      class IndifferentHashAccessor < ActiveRecord::Store::HashAccessor
+        def self.prepare(object, store_attribute)
+          attribute = object.send(store_attribute)
+          unless attribute.is_a?(ActiveSupport::HashWithIndifferentAccess)
+            attribute = IndifferentCoder.as_indifferent_hash(attribute)
+            object.send :"#{store_attribute}=", attribute
+          end
+          attribute
         end
-        attribute
       end
 
     class IndifferentCoder # :nodoc:
@@ -138,7 +173,7 @@ def dump(obj)
       end
 
       def load(yaml)
-        self.class.as_indifferent_hash @coder.load(yaml)
+        self.class.as_indifferent_hash(@coder.load(yaml))
       end
 
       def self.as_indifferent_hash(obj)

activerecord/test/cases/adapters/postgresql/hstore_test.rb

@@ -7,6 +7,8 @@
 class PostgresqlHstoreTest < ActiveRecord::TestCase
   class Hstore < ActiveRecord::Base
     self.table_name = 'hstores'
+
+    store_accessor :settings, :language, :timezone
   end
 
   def setup
@@ -26,6 +28,7 @@ def setup
     @connection.transaction do
       @connection.create_table('hstores') do |t|
         t.hstore 'tags', :default => ''
+        t.hstore 'settings'
       end
     end
     @column = Hstore.columns.find { |c| c.name == 'tags' }
@@ -90,6 +93,24 @@ def test_type_cast_hstore
     assert_equal({'c'=>'}','"a"'=>'b "a b'}, @column.type_cast(%q(c=>"}", "\"a\""=>"b \"a b")))
   end
 
+  def test_with_store_accessors
+    x = Hstore.new(language: "fr", timezone: "GMT")
+    assert_equal "fr", x.language
+    assert_equal "GMT", x.timezone
+
+    x.save!
+    x = Hstore.first
+    assert_equal "fr", x.language
+    assert_equal "GMT", x.timezone
+
+    x.language = "de"
+    x.save!
+
+    x = Hstore.first
+    assert_equal "de", x.language
+    assert_equal "GMT", x.timezone
+  end
+
   def test_gen1
     assert_equal(%q(" "=>""), @column.class.hstore_to_string({' '=>''}))
   end

activerecord/test/cases/adapters/postgresql/json_test.rb

@@ -7,6 +7,8 @@
 class PostgresqlJSONTest < ActiveRecord::TestCase
   class JsonDataType < ActiveRecord::Base
     self.table_name = 'json_data_type'
+
+    store_accessor :settings, :resolution
   end
 
   def setup
@@ -15,6 +17,7 @@ def setup
       @connection.transaction do
         @connection.create_table('json_data_type') do |t|
           t.json 'payload', :default => {}
+          t.json 'settings'
         end
       end
     rescue ActiveRecord::StatementInvalid
@@ -96,4 +99,19 @@ def test_rewrite_array_json_value
     x.payload = ['v1', {'k2' => 'v2'}, 'v3']
     assert x.save!
   end
+
+  def test_with_store_accessors
+    x = JsonDataType.new(resolution: "320×480")
+    assert_equal "320×480", x.resolution
+
+    x.save!
+    x = JsonDataType.first
+    assert_equal "320×480", x.resolution
+
+    x.resolution = "640×1136"
+    x.save!
+
+    x = JsonDataType.first
+    assert_equal "640×1136", x.resolution
+  end
 end

activerecord/test/cases/store_test.rb

@@ -150,4 +150,16 @@ class StoreTest < ActiveRecord::TestCase
   test "all stored attributes are returned" do
     assert_equal [:color, :homepage, :favorite_food], Admin::User.stored_attributes[:settings]
   end
+
+  test "stored_attributes are tracked per class" do
+    first_model = Class.new(ActiveRecord::Base) do
+      store_accessor :data, :color
+    end
+    second_model = Class.new(ActiveRecord::Base) do
+      store_accessor :data, :width, :height
+    end
+
+    assert_equal [:color], first_model.stored_attributes[:data]
+    assert_equal [:width, :height], second_model.stored_attributes[:data]
+  end
 end