Merge pull request #12490 from senny/store_accessor_hstore_bug

ActiveRecord Store works with PG `hstore` columns

8 files changed, 127 insertions, 14 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 25b905891f..7062d59a3d 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,14 @@
+*   `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.
diff --git a/activerecord/lib/active_record/attribute_methods/serialization.rb b/activerecord/lib/active_record/attribute_methods/serialization.rb
index 1287de0d0d..5701804168 100644
--- a/activerecord/lib/active_record/attribute_methods/serialization.rb
+++ b/activerecord/lib/active_record/attribute_methods/serialization.rb
@@ -66,6 +66,10 @@ module ActiveRecord
         def type
           @column.type
         end
+
+        def accessor
+          ActiveRecord::Store::IndifferentHashAccessor
+        end
       end
 
       class Attribute < Struct.new(:coder, :value, :state) # :nodoc:
diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/oid.rb b/activerecord/lib/active_record/connection_adapters/postgresql/oid.rb
index dab876af14..4babee07b4 100644
--- a/activerecord/lib/active_record/connection_adapters/postgresql/oid.rb
+++ b/activerecord/lib/active_record/connection_adapters/postgresql/oid.rb
@@ -234,6 +234,10 @@ module ActiveRecord
 
             ConnectionAdapters::PostgreSQLColumn.string_to_hstore value
           end
+
+          def accessor
+            ActiveRecord::Store::StringKeyedHashAccessor
+          end
         end
 
         class Cidr < Type
@@ -250,6 +254,10 @@ module ActiveRecord
 
             ConnectionAdapters::PostgreSQLColumn.string_to_json value
           end
+
+          def accessor
+            ActiveRecord::Store::StringKeyedHashAccessor
+          end
         end
 
         class TypeMap
diff --git a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
index c1cc905606..3668aecd4b 100644
--- a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
@@ -148,6 +148,10 @@ module ActiveRecord
         @oid_type.type_cast value
       end
 
+      def accessor
+        @oid_type.accessor
+      end
+
       private
 
         def has_default_function?(default_value, default)
diff --git a/activerecord/lib/active_record/store.rb b/activerecord/lib/active_record/store.rb
index a610f479f2..b841b977fc 100644
--- a/activerecord/lib/active_record/store.rb
+++ b/activerecord/lib/active_record/store.rb
@@ -86,6 +86,9 @@ module ActiveRecord
           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 @@ module ActiveRecord
 
     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 @@ module ActiveRecord
       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)
diff --git a/activerecord/test/cases/adapters/postgresql/hstore_test.rb b/activerecord/test/cases/adapters/postgresql/hstore_test.rb
index f61f196c71..de724486c2 100644
--- a/activerecord/test/cases/adapters/postgresql/hstore_test.rb
+++ b/activerecord/test/cases/adapters/postgresql/hstore_test.rb
@@ -7,6 +7,8 @@ require 'active_record/connection_adapters/postgresql_adapter'
 class PostgresqlHstoreTest < ActiveRecord::TestCase
   class Hstore < ActiveRecord::Base
     self.table_name = 'hstores'
+
+    store_accessor :settings, :language, :timezone
   end
 
   def setup
@@ -26,6 +28,7 @@ class PostgresqlHstoreTest < ActiveRecord::TestCase
     @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 @@ class PostgresqlHstoreTest < ActiveRecord::TestCase
     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
diff --git a/activerecord/test/cases/adapters/postgresql/json_test.rb b/activerecord/test/cases/adapters/postgresql/json_test.rb
index adac1d3c13..f7609404ce 100644
--- a/activerecord/test/cases/adapters/postgresql/json_test.rb
+++ b/activerecord/test/cases/adapters/postgresql/json_test.rb
@@ -7,6 +7,8 @@ require 'active_record/connection_adapters/postgresql_adapter'
 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 @@ class PostgresqlJSONTest < ActiveRecord::TestCase
       @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 @@ class PostgresqlJSONTest < ActiveRecord::TestCase
     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
diff --git a/activerecord/test/cases/store_test.rb b/activerecord/test/cases/store_test.rb
index c2c56abacd..0c9f7ccd55 100644
--- a/activerecord/test/cases/store_test.rb
+++ b/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