Skip to content

Avoid validating belongs_to association if it has not changed #46522

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 24, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions activerecord/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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
Expand Down
3 changes: 3 additions & 0 deletions activerecord/lib/active_record.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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? ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we could use _attribute here? I think we can assume that foreign_key and foreign_type are not aliases and are already strings. Right?

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

Expand Down
1 change: 1 addition & 0 deletions activerecord/lib/active_record/railtie.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions activerecord/test/cases/helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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|
Expand Down
12 changes: 12 additions & 0 deletions guides/source/configuring.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand Down Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions railties/lib/rails/application/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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