Cached columns_hash fields should be excluded from ResultSet#column_types

DmitryTsepelev · DmitryTsepelev · commit d34c1fc3d64e · 2018-11-27T16:30:26.000+03:00
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
@@ -1,3 +1,32 @@
+*   Cached columns_hash fields should be excluded from ResultSet#column_types
+
+    PR #34528 addresses the inconsistent behaviour when attribute is defined for an ignored column. The following test
+    was passing for SQLite and MySQL, but failed for PostgreSQL:
+
+    ```ruby
+    class DeveloperName < ActiveRecord::Type::String
+      def deserialize(value)
+        "Developer: #{value}"
+      end
+    end
+
+    class AttributedDeveloper < ActiveRecord::Base
+      self.table_name = "developers"
+
+      attribute :name, DeveloperName.new
+
+      self.ignored_columns += ["name"]
+    end
+
+    developer = AttributedDeveloper.create
+    developer.update_column :name, "name"
+
+    loaded_developer = AttributedDeveloper.where(id: developer.id).select("*").first
+    puts loaded_developer.name # should be "Developer: name" but it's just "name"
+    ```
+
+    *Dmitry Tsepelev*
+
 *   Make the implicit order column configurable.
 
     When calling ordered finder methods such as +first+ or +last+ without an
diff --git a/activerecord/lib/active_record/querying.rb b/activerecord/lib/active_record/querying.rb
@@ -40,7 +40,8 @@ module Querying
     def find_by_sql(sql, binds = [], preparable: nil, &block)
       result_set = connection.select_all(sanitize_sql(sql), "#{name} Load", binds, preparable: preparable)
       column_types = result_set.column_types.dup
-      columns_hash.each_key { |k| column_types.delete k }
+      cached_columns_hash = connection.schema_cache.columns_hash(table_name)
+      cached_columns_hash.each_key { |k| column_types.delete k }
       message_bus = ActiveSupport::Notifications.instrumenter
 
       payload = {
diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb
@@ -1447,6 +1447,14 @@ def test_default_values_are_deeply_dupped
     assert_not_respond_to developer, :first_name=
   end
 
+  test "when ignored attribute is loaded, cast type should be preferred over DB type" do
+    developer = AttributedDeveloper.create
+    developer.update_column :name, "name"
+
+    loaded_developer = AttributedDeveloper.where(id: developer.id).select("*").first
+    assert_equal "Developer: name", loaded_developer.name
+  end
+
   test "ignored columns not included in SELECT" do
     query = Developer.all.to_sql.downcase
 
diff --git a/activerecord/test/cases/instrumentation_test.rb b/activerecord/test/cases/instrumentation_test.rb
@@ -5,6 +5,10 @@
 
 module ActiveRecord
   class InstrumentationTest < ActiveRecord::TestCase
+    def setup
+      ActiveRecord::Base.connection.schema_cache.add(Book.table_name)
+    end
+
     def test_payload_name_on_load
       Book.create(name: "test book")
       subscriber = ActiveSupport::Notifications.subscribe("sql.active_record") do |*args|
diff --git a/activerecord/test/models/developer.rb b/activerecord/test/models/developer.rb
@@ -279,3 +279,17 @@ class DeveloperWithIncorrectlyOrderedHasManyThrough < ActiveRecord::Base
   has_many :companies, through: :contracts
   has_many :contracts, foreign_key: :developer_id
 end
+
+class DeveloperName < ActiveRecord::Type::String
+  def deserialize(value)
+    "Developer: #{value}"
+  end
+end
+
+class AttributedDeveloper < ActiveRecord::Base
+  self.table_name = "developers"
+
+  attribute :name, DeveloperName.new
+
+  self.ignored_columns += ["name"]
+end
