Merge pull request rails#46522 from fatkodima/belongs_to-avoid-queries

Avoid validating `belongs_to` association if it has not changed

Author: byroot

activerecord/CHANGELOG.md

@@ -1,3 +1,21 @@
+*   Avoid validating `belongs_to` association if it has not changed.
+
+    Previously, when updating a record, Active Record will perform an extra query to check for the presence of
+    `belongs_to` associations (if the presence is configured to be mandatory), even if that attribute hasn't changed.
+
+    Currently, only `belongs_to`-related columns are checked for presence. It is possible to have orphaned records with
+    this approach. To avoid this problem, you need to use a foreign key.
+
+    This behavior can be controlled by configuration:
+
+    ```ruby
+    config.active_record.belongs_to_required_validates_foreign_key = false
+    ```
+
+    and will be disabled by default with `load_defaults 7.1`.
+
+    *fatkodima*
+
 *   `has_one` and `belongs_to` associations now define a `reset_association` method
     on the owner model (where `association` is the name of the association). This
     method unloads the cached associate record, if any, and causes the next access

activerecord/lib/active_record.rb

@@ -271,6 +271,9 @@ def self.global_executor_concurrency # :nodoc:
   singleton_class.attr_accessor :raise_on_assign_to_attr_readonly
   self.raise_on_assign_to_attr_readonly = false
 
+  singleton_class.attr_accessor :belongs_to_required_validates_foreign_key
+  self.belongs_to_required_validates_foreign_key = true
+
   ##
   # :singleton-method:
   # Specify a threshold for the size of query result sets. If the number of

activerecord/lib/active_record/associations/builder/belongs_to.rb

@@ -123,7 +123,20 @@ def self.define_validations(model, reflection)
       super
 
       if required
-        model.validates_presence_of reflection.name, message: :required
+        if ActiveRecord.belongs_to_required_validates_foreign_key
+          model.validates_presence_of reflection.name, message: :required
+        else
+          condition = lambda { |record|
+            foreign_key = reflection.foreign_key
+            foreign_type = reflection.foreign_type
+
+            record.read_attribute(foreign_key).nil? ||
+              record.attribute_changed?(foreign_key) ||
+              (reflection.polymorphic? && (record.read_attribute(foreign_type).nil? || record.attribute_changed?(foreign_type)))
+          }
+
+          model.validates_presence_of reflection.name, message: :required, if: condition
+        end
       end
     end
 

activerecord/lib/active_record/railtie.rb

@@ -37,6 +37,7 @@ class Railtie < Rails::Railtie # :nodoc:
     config.active_record.query_log_tags_format = :legacy
     config.active_record.cache_query_log_tags = false
     config.active_record.raise_on_assign_to_attr_readonly = false
+    config.active_record.belongs_to_required_validates_foreign_key = true
 
     config.active_record.queues = ActiveSupport::InheritableOptions.new
 

activerecord/test/cases/associations/belongs_to_associations_test.rb

@@ -1665,6 +1665,61 @@ def test_multiple_counter_cache_with_after_create_update
     assert_not comment.author_changed?
     assert comment.author_previously_changed?
   end
+
+  class ShipRequired < ActiveRecord::Base
+    self.table_name = "ships"
+    belongs_to :developer, required: true
+  end
+
+  test "runs parent presence check if parent changed or nil" do
+    david = developers(:david)
+    jamis = developers(:jamis)
+
+    ship = ShipRequired.create!(name: "Medusa", developer: david)
+    assert_equal david, ship.developer
+
+    assert_queries(2) do # UPDATE and SELECT to check developer presence
+      ship.update!(developer_id: jamis.id)
+    end
+
+    ship.update_column(:developer_id, nil)
+    ship.reload
+
+    assert_queries(2) do # UPDATE and SELECT to check developer presence
+      ship.update!(developer_id: david.id)
+    end
+  end
+
+  test "skips parent presence check if parent has not changed" do
+    david = developers(:david)
+    ship = ShipRequired.create!(name: "Medusa", developer: david)
+    ship.reload # unload developer association
+
+    assert_queries(1) do # UPDATE only, no SELECT to check developer presence
+      ship.update!(name: "Leviathan")
+    end
+  end
+
+  test "runs parent presence check if parent has not changed and belongs_to_required_validates_foreign_key is set" do
+    original_value = ActiveRecord.belongs_to_required_validates_foreign_key
+    ActiveRecord.belongs_to_required_validates_foreign_key = true
+
+    model = Class.new(ActiveRecord::Base) do
+      self.table_name = "ships"
+      def self.name; "Temp"; end
+      belongs_to :developer, required: true
+    end
+
+    david = developers(:david)
+    ship = model.create!(name: "Medusa", developer: david)
+    ship.reload # unload developer association
+
+    assert_queries(2) do # UPDATE and SELECT to check developer presence
+      ship.update!(name: "Leviathan")
+    end
+  ensure
+    ActiveRecord.belongs_to_required_validates_foreign_key = original_value
+  end
 end
 
 class BelongsToWithForeignKeyTest < ActiveRecord::TestCase

activerecord/test/cases/helper.rb

@@ -31,6 +31,7 @@
 QUOTED_TYPE = ActiveRecord::Base.connection.quote_column_name("type")
 
 ActiveRecord.raise_on_assign_to_attr_readonly = true
+ActiveRecord.belongs_to_required_validates_foreign_key = false
 
 def current_adapter?(*types)
   types.any? do |type|

guides/source/configuring.md

@@ -66,6 +66,7 @@ Below are the default values associated with each target version. In cases of co
 - [`config.active_record.allow_deprecated_singular_associations_name`](#config-active-record-allow-deprecated-singular-associations-name): `false`
 - [`config.active_record.query_log_tags_format`](#config-active-record-query-log-tags-format): `:sqlcommenter`
 - [`config.active_record.raise_on_assign_to_attr_readonly`](#config-active-record-raise-on-assign-to-attr-readonly): `true`
+- [`config.active_record.belongs_to_required_validates_foreign_key`](#config-active-record-belongs-to-required-validates-foreign-key): `false`
 - [`config.active_record.run_commit_callbacks_on_first_saved_instances_in_transaction`](#config-active-record-run-commit-callbacks-on-first-saved-instances-in-transaction): `false`
 - [`config.active_record.sqlite3_adapter_strict_strings_by_default`](#config-active-record-sqlite3-adapter-strict-strings-by-default): `true`
 - [`config.active_support.default_message_encryptor_serializer`](#config-active-support-default-message-encryptor-serializer): `:json`
@@ -1002,6 +1003,17 @@ The default value depends on the `config.load_defaults` target version:
 | (original)            | `nil`                |
 | 5.0                   | `true`               |
 
+#### `config.active_record.belongs_to_required_validates_foreign_key`
+
+Enable validating only parent-related columns for presence when the parent is mandatory.
+The previous behavior was to validate the presence of the parent record, which performed an extra query
+to get the parent every time the child record was updated, even when parent has not changed.
+
+| Starting with version | The default value is |
+| --------------------- | -------------------- |
+| (original)            | `true`               |
+| 7.1                   | `false`              |
+
 #### `config.active_record.action_on_strict_loading_violation`
 
 Enables raising or logging an exception if strict_loading is set on an

railties/lib/rails/application/configuration.rb

@@ -289,6 +289,7 @@ def load_defaults(target_version)
             active_record.sqlite3_adapter_strict_strings_by_default = true
             active_record.query_log_tags_format = :sqlcommenter
             active_record.raise_on_assign_to_attr_readonly = true
+            active_record.belongs_to_required_validates_foreign_key = false
           end
 
           if respond_to?(:action_dispatch)

railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_1.rb.tt

@@ -112,3 +112,8 @@
 # behavior would allow assignment but silently not persist changes to the
 # database.
 # Rails.application.config.active_record.raise_on_assign_to_attr_readonly = true
+
+# Enable validating only parent-related columns for presence when the parent is mandatory.
+# The previous behavior was to validate the presence of the parent record, which performed an extra query
+# to get the parent every time the child record was updated, even when parent has not changed.
+# Rails.application.config.active_record.belongs_to_required_validates_foreign_key = false