Merge pull request #33985 from eugeneius/attribute_methods_schema_cache

Only define attribute methods from schema cache

activerecord/lib/active_record/connection_adapters/schema_cache.rb

@@ -77,6 +77,11 @@ def columns_hash(table_name)
         }]
       end
 
+      # Checks whether the columns hash is already cached for a table.
+      def columns_hash?(table_name)
+        @columns_hash.key?(table_name)
+      end
+
       # Clears out internal caches
       def clear!
         @columns.clear

activerecord/lib/active_record/core.rb

@@ -282,6 +282,10 @@ def type_caster # :nodoc:
         TypeCaster::Map.new(self)
       end
 
+      def _internal? # :nodoc:
+        false
+      end
+
       private
 
         def cached_find_by_statement(key, &block)

activerecord/lib/active_record/internal_metadata.rb

@@ -8,6 +8,10 @@ module ActiveRecord
   # as which environment migrations were run in.
   class InternalMetadata < ActiveRecord::Base # :nodoc:
     class << self
+      def _internal?
+        true
+      end
+
       def primary_key
         "key"
       end

activerecord/lib/active_record/railtie.rb

@@ -140,7 +140,19 @@ class Railtie < Rails::Railtie # :nodoc:
     initializer "active_record.define_attribute_methods" do |app|
       config.after_initialize do
         ActiveSupport.on_load(:active_record) do
-          descendants.each(&:define_attribute_methods) if app.config.eager_load
+          if app.config.eager_load
+            descendants.each do |model|
+              # SchemaMigration and InternalMetadata both override `table_exists?`
+              # to bypass the schema cache, so skip them to avoid the extra queries.
+              next if model._internal?
+
+              # If there's no connection yet, or the schema cache doesn't have the columns
+              # hash for the model cached, `define_attribute_methods` would trigger a query.
+              next unless model.connected? && model.connection.schema_cache.columns_hash?(model.table_name)
+
+              model.define_attribute_methods
+            end
+          end
         end
       end
     end

activerecord/lib/active_record/schema_migration.rb

@@ -10,6 +10,10 @@ module ActiveRecord
   # to be executed the next time.
   class SchemaMigration < ActiveRecord::Base # :nodoc:
     class << self
+      def _internal?
+        true
+      end
+
       def primary_key
         "version"
       end

activerecord/test/cases/connection_adapters/schema_cache_test.rb

@@ -91,6 +91,22 @@ def test_clear_data_source_cache
         @cache.clear_data_source_cache!("posts")
       end
 
+      test "#columns_hash? is populated by #columns_hash" do
+        assert_not @cache.columns_hash?("posts")
+
+        @cache.columns_hash("posts")
+
+        assert @cache.columns_hash?("posts")
+      end
+
+      test "#columns_hash? is not populated by #data_source_exists?" do
+        assert_not @cache.columns_hash?("posts")
+
+        @cache.data_source_exists?("posts")
+
+        assert_not @cache.columns_hash?("posts")
+      end
+
       private
 
         def schema_dump_path

railties/test/application/configuration_test.rb

@@ -331,7 +331,7 @@ class Post < ActiveRecord::Base
       assert_not_includes Post.instance_methods, :title
     end
 
-    test "eager loads attribute methods in production" do
+    test "does not eager load attribute methods in production when the schema cache is empty" do
       app_file "app/models/post.rb", <<-RUBY
         class Post < ActiveRecord::Base
         end
@@ -354,9 +354,71 @@ class Post < ActiveRecord::Base
 
       app "production"
 
+      assert_not_includes Post.instance_methods, :title
+    end
+
+    test "eager loads attribute methods in production when the schema cache is populated" do
+      app_file "app/models/post.rb", <<-RUBY
+        class Post < ActiveRecord::Base
+        end
+      RUBY
+
+      app_file "config/initializers/active_record.rb", <<-RUBY
+        ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
+        ActiveRecord::Migration.verbose = false
+        ActiveRecord::Schema.define(version: 1) do
+          create_table :posts do |t|
+            t.string :title
+          end
+        end
+      RUBY
+
+      add_to_config <<-RUBY
+        config.eager_load = true
+        config.cache_classes = true
+      RUBY
+
+      app_file "config/initializers/schema_cache.rb", <<-RUBY
+        ActiveRecord::Base.connection.schema_cache.add("posts")
+      RUBY
+
+      app "production"
+
       assert_includes Post.instance_methods, :title
     end
 
+    test "does not attempt to eager load attribute methods for models that aren't connected" do
+      app_file "app/models/post.rb", <<-RUBY
+        class Post < ActiveRecord::Base
+        end
+      RUBY
+
+      app_file "config/initializers/active_record.rb", <<-RUBY
+        ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
+        ActiveRecord::Migration.verbose = false
+        ActiveRecord::Schema.define(version: 1) do
+          create_table :posts do |t|
+            t.string :title
+          end
+        end
+      RUBY
+
+      add_to_config <<-RUBY
+        config.eager_load = true
+        config.cache_classes = true
+      RUBY
+
+      app_file "app/models/comment.rb", <<-RUBY
+        class Comment < ActiveRecord::Base
+          establish_connection(adapter: "mysql2", database: "does_not_exist")
+        end
+      RUBY
+
+      assert_nothing_raised do
+        app "production"
+      end
+    end
+
     test "initialize an eager loaded, cache classes app" do
       add_to_config <<-RUBY
         config.eager_load = true