Merge pull request #48793 from Shopify/define-attributes-initializer

Never connect to the database in define_attribute_methods initializer

activerecord/lib/active_record/railtie.rb

@@ -186,20 +186,18 @@ class Railtie < Rails::Railtie # :nodoc:
         ActiveSupport.on_load(:active_record) do
           # In development and test we shouldn't eagerly define attribute methods because
           # db:test:prepare will trigger later and might change the schema.
-          if app.config.eager_load && !Rails.env.local?
+          #
+          # Additionally if `check_schema_cache_dump_version` is enabled (which is the default),
+          # loading the schema cache dump trigger a database connection to compare the schema
+          # versions.
+          # This means the attribute methods will be lazily defined whent the model is accessed,
+          # likely as part of the first few requests or jobs. This isn't good for performance
+          # but we unfortunately have to arbitrate between resiliency and performance, and chose
+          # resiliency.
+          if !check_schema_cache_dump_version && app.config.eager_load && !Rails.env.local?
             begin
               descendants.each do |model|
-                # If the schema cache doesn't have the columns for this model,
-                # we avoid calling `define_attribute_methods` as it would trigger a query.
-                #
-                # However if we're already connected to the database, it's too late so we might
-                # as well eagerly define the attributes and hope the database timeout is strict enough.
-                #
-                # Additionally if `check_schema_cache_dump_version` is enabled, we have to connect to the
-                # database anyway to load the schema cache dump, so we might as well do it during boot to
-                # save memory in pre-forking setups and avoid slowness during the first requests post deploy.
-                schema_reflection = model.connection_pool.schema_reflection
-                if check_schema_cache_dump_version || schema_reflection.cached?(model.table_name) || model.connected?
+                if model.connection_pool.schema_reflection.cached?(model.table_name)
                   model.define_attribute_methods
                 end
               end

railties/test/application/configuration_test.rb

@@ -406,7 +406,7 @@ class Post < ActiveRecord::Base
       assert_not_includes Post.instance_methods, :title
     end
 
-    test "does not eager load attribute methods in production when the schema cache is empty and the database not connected" do
+    test "does not eager load attribute methods in production when the schema cache is empty and and check_schema_cache_dump_version=false" do
       app_file "app/models/post.rb", <<-RUBY
         class Post < ActiveRecord::Base
         end
@@ -423,7 +423,7 @@ class Post < ActiveRecord::Base
       assert_not_includes (Post.instance_methods - ActiveRecord::Base.instance_methods), :title
     end
 
-    test "eager loads attribute methods in production when the schema cache is populated" do
+    test "eager loads attribute methods in production when the schema cache is populated and check_schema_cache_dump_version=false" do
       app_file "app/models/post.rb", <<-RUBY
         class Post < ActiveRecord::Base
         end
@@ -442,18 +442,19 @@ class Post < ActiveRecord::Base
       add_to_config <<-RUBY
         config.enable_reloading = false
         config.eager_load = true
+        config.active_record.check_schema_cache_dump_version = false
       RUBY
 
       app_file "config/initializers/schema_cache.rb", <<-RUBY
-        ActiveRecord::Base.connection.schema_cache.add("posts")
+      ActiveRecord::Base.connection.schema_cache.add("posts")
       RUBY
 
       app "production"
 
-      assert_includes Post.instance_methods, :title
+      assert_includes (Post.instance_methods - ActiveRecord::Base.instance_methods), :title
     end
 
-    test "does not attempt to eager load attribute methods for models that aren't connected" do
+    test "does not eager loads attribute methods in production when the schema cache is populated and check_schema_cache_dump_version=true" do
       app_file "app/models/post.rb", <<-RUBY
         class Post < ActiveRecord::Base
         end
@@ -472,17 +473,16 @@ class Post < ActiveRecord::Base
       add_to_config <<-RUBY
         config.enable_reloading = false
         config.eager_load = true
+        config.active_record.check_schema_cache_dump_version = true
       RUBY
 
-      app_file "app/models/comment.rb", <<-RUBY
-        class Comment < ActiveRecord::Base
-          establish_connection(adapter: "mysql2", database: "does_not_exist")
-        end
+      app_file "config/initializers/schema_cache.rb", <<-RUBY
+      ActiveRecord::Base.connection.schema_cache.add("posts")
       RUBY
 
-      assert_nothing_raised do
-        app "production"
-      end
+      app "production"
+
+      assert_not_includes (Post.instance_methods - ActiveRecord::Base.instance_methods), :title
     end
 
     test "application is always added to eager_load namespaces" do