From 69bf870c50d14bb8c18142242cf93f4102cc13dc Mon Sep 17 00:00:00 2001 From: bogdanvlviv Date: Sat, 30 May 2020 15:09:28 +0300 Subject: [PATCH] Add `ActiveRecord::Base.strict_loading_by_default` and `ActiveRecord::Base.strict_loading_by_default=`. This will allow to enable/disable strict_loading mode by default for a model. The configuration's value is inheritable by subclasses, but they can override that value and it will not impact parent class: ```ruby class Developer < ApplicationRecord self.strict_loading_by_default = true has_many :projects end dev = Developer.first dev.projects.first \# => ActiveRecord::StrictLoadingViolationError Exception: Developer is marked as strict_loading and Project cannot be lazily loaded. ``` What is great about this feature that it could help users to nip N+1 queries in the bud, especially for fresh applications, by setting `ActiveRecord::Base.strict_loading_by_default = true` / `config.active_record.strict_loading_by_default = true`. That is also a great way to prevent new N+1 queries in the existing applications after all the N+1 queries are eliminated. (See https://guides.rubyonrails.org/v6.0/active_record_querying.html#eager-loading-associations, https://github.com/seejohnrun/prelude for details on how to fight against N+1 queries). Related to https://github.com/rails/rails/pull/37400, https://github.com/rails/rails/pull/38541 --- activerecord/CHANGELOG.md | 20 ++ activerecord/lib/active_record/core.rb | 4 +- .../test/cases/strict_loading_test.rb | 201 ++++++++++++++++++ guides/source/configuring.md | 4 + 4 files changed, 228 insertions(+), 1 deletion(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index a669bcc494..3a7001b1ed 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,23 @@ +* Add `ActiveRecord::Base.strict_loading_by_default` and `ActiveRecord::Base.strict_loading_by_default=` + to enable/disable strict_loading mode by default for a model. The configuration's value is + inheritable by subclasses, but they can override that value and it will not impact parent class. + + Usage: + + ```ruby + class Developer < ApplicationRecord + self.strict_loading_by_default = true + + has_many :projects + end + + dev = Developer.first + dev.projects.first + # => ActiveRecord::StrictLoadingViolationError Exception: Developer is marked as strict_loading and Project cannot be lazily loaded. + ``` + + *bogdanvlviv* + * Deprecate passing an Active Record object to `quote`/`type_cast` directly. *Ryuta Kamizono* diff --git a/activerecord/lib/active_record/core.rb b/activerecord/lib/active_record/core.rb index d1532a6c44..615ecaa2b4 100644 --- a/activerecord/lib/active_record/core.rb +++ b/activerecord/lib/active_record/core.rb @@ -123,6 +123,8 @@ def self.configurations class_attribute :belongs_to_required_by_default, instance_accessor: false + class_attribute :strict_loading_by_default, instance_accessor: false, default: false + mattr_accessor :connection_handlers, instance_accessor: false, default: {} mattr_accessor :writing_role, instance_accessor: false, default: :writing @@ -600,7 +602,7 @@ def init_internals @marked_for_destruction = false @destroyed_by_association = nil @_start_transaction_state = nil - @strict_loading = false + @strict_loading = self.class.strict_loading_by_default self.class.define_attribute_methods end diff --git a/activerecord/test/cases/strict_loading_test.rb b/activerecord/test/cases/strict_loading_test.rb index 4c59ebe947..8e7d0795cf 100644 --- a/activerecord/test/cases/strict_loading_test.rb +++ b/activerecord/test/cases/strict_loading_test.rb @@ -29,6 +29,43 @@ def test_strict_loading Developer.strict_loading.each { |d| assert d.strict_loading? } end + def test_strict_loading_by_default + with_strict_loading_by_default(Developer) do + Developer.all.each { |d| assert d.strict_loading? } + end + end + + def test_strict_loading_by_default_can_be_set_per_model + model1 = Class.new(ActiveRecord::Base) do + self.table_name = "developers" + self.strict_loading_by_default = true + end.new + + model2 = Class.new(ActiveRecord::Base) do + self.table_name = "developers" + self.strict_loading_by_default = false + end.new + + assert_predicate model1, :strict_loading? + assert_not_predicate model2, :strict_loading? + end + + def test_strict_loading_by_default_is_inheritable + with_strict_loading_by_default(ActiveRecord::Base) do + model1 = Class.new(ActiveRecord::Base) do + self.table_name = "developers" + end.new + + model2 = Class.new(ActiveRecord::Base) do + self.table_name = "developers" + self.strict_loading_by_default = false + end.new + + assert_predicate model1, :strict_loading? + assert_not_predicate model2, :strict_loading? + end + end + def test_raises_if_strict_loading_and_lazy_loading dev = Developer.strict_loading.first assert_predicate dev, :strict_loading? @@ -38,6 +75,17 @@ def test_raises_if_strict_loading_and_lazy_loading end end + def test_raises_if_strict_loading_by_default_and_lazy_loading + with_strict_loading_by_default(Developer) do + dev = Developer.first + assert_predicate dev, :strict_loading? + + assert_raises ActiveRecord::StrictLoadingViolationError do + dev.audit_logs.to_a + end + end + end + def test_preload_audit_logs_are_strict_loading_because_parent_is_strict_loading developer = Developer.first @@ -51,6 +99,21 @@ def test_preload_audit_logs_are_strict_loading_because_parent_is_strict_loading assert dev.audit_logs.all?(&:strict_loading?), "Expected all audit logs to be strict_loading" end + def test_preload_audit_logs_are_strict_loading_because_it_is_strict_loading_by_default + with_strict_loading_by_default(AuditLog) do + developer = Developer.first + + 3.times do + AuditLog.create(developer: developer, message: "I am message") + end + + dev = Developer.includes(:audit_logs).first + + assert_not_predicate dev, :strict_loading? + assert dev.audit_logs.all?(&:strict_loading?), "Expected all audit logs to be strict_loading" + end + end + def test_eager_load_audit_logs_are_strict_loading_because_parent_is_strict_loading_in_hm_relation developer = Developer.first @@ -76,6 +139,22 @@ def test_eager_load_audit_logs_are_strict_loading_because_parent_is_strict_loadi assert dev.audit_logs.all?(&:strict_loading?), "Expected all audit logs to be strict_loading" end + def test_eager_load_audit_logs_are_strict_loading_because_it_is_strict_loading_by_default + with_strict_loading_by_default(AuditLog) do + developer = Developer.first + + 3.times do + AuditLog.create(developer: developer, message: "I am message") + end + + dev = Developer.eager_load(:audit_logs).first + + assert_not_predicate dev, :strict_loading? + assert_predicate AuditLog.last, :strict_loading? + assert dev.audit_logs.all?(&:strict_loading?), "Expected all audit logs to be strict_loading" + end + end + def test_raises_on_unloaded_relation_methods_if_strict_loading dev = Developer.strict_loading.first assert_predicate dev, :strict_loading? @@ -85,6 +164,17 @@ def test_raises_on_unloaded_relation_methods_if_strict_loading end end + def test_raises_on_unloaded_relation_methods_if_strict_loading_by_default + with_strict_loading_by_default(Developer) do + dev = Developer.first + assert_predicate dev, :strict_loading? + + assert_raises ActiveRecord::StrictLoadingViolationError do + dev.audit_logs.first + end + end + end + def test_raises_on_lazy_loading_a_strict_loading_belongs_to_relation mentor = Mentor.create!(name: "Mentor") @@ -96,6 +186,19 @@ def test_raises_on_lazy_loading_a_strict_loading_belongs_to_relation end end + def test_raises_on_lazy_loading_a_belongs_to_relation_if_strict_loading_by_default + with_strict_loading_by_default(Developer) do + mentor = Mentor.create!(name: "Mentor") + + developer = Developer.first + developer.update_column(:mentor_id, mentor.id) + + assert_raises ActiveRecord::StrictLoadingViolationError do + developer.mentor + end + end + end + def test_does_not_raise_on_eager_loading_a_strict_loading_belongs_to_relation mentor = Mentor.create!(name: "Mentor") @@ -105,6 +208,17 @@ def test_does_not_raise_on_eager_loading_a_strict_loading_belongs_to_relation assert_nothing_raised { developer.strict_loading_mentor } end + def test_does_not_raise_on_eager_loading_a_belongs_to_relation_if_strict_loading_by_default + with_strict_loading_by_default(Developer) do + mentor = Mentor.create!(name: "Mentor") + + Developer.first.update_column(:mentor_id, mentor.id) + developer = Developer.includes(:mentor).first + + assert_nothing_raised { developer.mentor } + end + end + def test_raises_on_lazy_loading_a_strict_loading_has_one_relation developer = Developer.first ship = Ship.first @@ -116,6 +230,19 @@ def test_raises_on_lazy_loading_a_strict_loading_has_one_relation end end + def test_raises_on_lazy_loading_a_has_one_relation_if_strict_loading_by_default + with_strict_loading_by_default(Developer) do + developer = Developer.first + ship = Ship.first + + ship.update_column(:developer_id, developer.id) + + assert_raises ActiveRecord::StrictLoadingViolationError do + developer.ship + end + end + end + def test_does_not_raise_on_eager_loading_a_strict_loading_has_one_relation Ship.first.update_column(:developer_id, Developer.first.id) developer = Developer.includes(:strict_loading_ship).first @@ -123,6 +250,15 @@ def test_does_not_raise_on_eager_loading_a_strict_loading_has_one_relation assert_nothing_raised { developer.strict_loading_ship } end + def test_does_not_raise_on_eager_loading_a_has_one_relation_if_strict_loading_by_default + with_strict_loading_by_default(Developer) do + Ship.first.update_column(:developer_id, Developer.first.id) + developer = Developer.includes(:ship).first + + assert_nothing_raised { developer.ship } + end + end + def test_raises_on_lazy_loading_a_strict_loading_has_many_relation developer = Developer.first @@ -137,6 +273,22 @@ def test_raises_on_lazy_loading_a_strict_loading_has_many_relation end end + def test_raises_on_lazy_loading_a_has_many_relation_if_strict_loading_by_default + with_strict_loading_by_default(Developer) do + developer = Developer.first + + AuditLog.create( + 3.times.map do + { developer_id: developer.id, message: "I am message" } + end + ) + + assert_raises ActiveRecord::StrictLoadingViolationError do + developer.audit_logs.first + end + end + end + def test_does_not_raise_on_eager_loading_a_strict_loading_has_many_relation developer = Developer.first @@ -151,6 +303,22 @@ def test_does_not_raise_on_eager_loading_a_strict_loading_has_many_relation assert_nothing_raised { developer.strict_loading_opt_audit_logs.first } end + def test_does_not_raise_on_eager_loading_a_has_many_relation_if_strict_loading_by_default + with_strict_loading_by_default(Developer) do + developer = Developer.first + + AuditLog.create( + 3.times.map do + { developer_id: developer.id, message: "I am message" } + end + ) + + developer = Developer.includes(:audit_logs).first + + assert_nothing_raised { developer.audit_logs.first } + end + end + def test_raises_on_lazy_loading_a_strict_loading_habtm_relation developer = Developer.first developer.projects << Project.first @@ -162,10 +330,43 @@ def test_raises_on_lazy_loading_a_strict_loading_habtm_relation end end + def test_raises_on_lazy_loading_a_habtm_relation_if_strict_loading_by_default + with_strict_loading_by_default(Developer) do + developer = Developer.first + developer.projects << Project.first + + assert_not developer.projects.loaded? + + assert_raises ActiveRecord::StrictLoadingViolationError do + developer.projects.first + end + end + end + def test_does_not_raise_on_eager_loading_a_strict_loading_habtm_relation Developer.first.projects << Project.first developer = Developer.includes(:strict_loading_projects).first assert_nothing_raised { developer.strict_loading_projects.first } end + + def test_does_not_raise_on_eager_loading_a_habtm_relation_if_strict_loading_by_default + with_strict_loading_by_default(Developer) do + Developer.first.projects << Project.first + developer = Developer.includes(:projects).first + + assert_nothing_raised { developer.projects.first } + end + end + + private + def with_strict_loading_by_default(model) + previous_strict_loading_by_default = model.strict_loading_by_default + + model.strict_loading_by_default = true + + yield + ensure + model.strict_loading_by_default = previous_strict_loading_by_default + end end diff --git a/guides/source/configuring.md b/guides/source/configuring.md index 593f7245da..f90d3c5091 100644 --- a/guides/source/configuring.md +++ b/guides/source/configuring.md @@ -432,6 +432,10 @@ in controllers and views. This defaults to `false`. controls whether a record fails validation if `belongs_to` association is not present. +* `config.active_record.strict_loading_by_default` is a boolean value + that either enables or disables strict_loading mode by default. + Defaults to `false`. + * `config.active_record.warn_on_records_fetched_greater_than` allows setting a warning threshold for query result size. If the number of records returned by a query exceeds the threshold, a warning is logged. This can be used to -- GitLab