From 656c6faf7891ad6068155d1461e8f8aa64131afa Mon Sep 17 00:00:00 2001 From: eileencodes Date: Fri, 30 Oct 2020 10:40:35 -0400 Subject: [PATCH] Allow applications to change the behavior for a strict loading violation Originally strict loading violations would always raise an error if turned on for an association. This change allows for an application to optionally log instead of raise. The behavior here is similar to how strong parameters work. By default all environments will raise. An application may want to use log in production while working on finding all lazily loaded associations. Set `config.active_record.action_on_strict_loading_violation` to `:log` in your application to log instead of raise. --- activerecord/CHANGELOG.md | 8 ++++ .../active_record/associations/association.rb | 4 +- activerecord/lib/active_record/core.rb | 17 +++++++ .../lib/active_record/log_subscriber.rb | 9 ++++ .../test/cases/strict_loading_test.rb | 47 +++++++++++++++++++ guides/source/configuring.md | 5 ++ 6 files changed, 88 insertions(+), 2 deletions(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index b56eedf591..41dd28987d 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,11 @@ +* Add option to raise or log for `ActiveRecord::StrictLoadingViolationError`. + + Some applications may not want to raise an error in production if using `strict_loading`. This would allow an application to set strict loading to log for the production environment while still raising in development and test environments. + + Set `config.active_record.action_on_strict_loading_violation` to `:log` errors instead of raising. + + *Eileen M. Uchitelle* + * Allow the inverse of a `has_one` association that was previously autosaved to be loaded. Fixes #34255. diff --git a/activerecord/lib/active_record/associations/association.rb b/activerecord/lib/active_record/associations/association.rb index 3ec69ac75e..8a80c30374 100644 --- a/activerecord/lib/active_record/associations/association.rb +++ b/activerecord/lib/active_record/associations/association.rb @@ -208,11 +208,11 @@ module ActiveRecord private def find_target if owner.strict_loading? - raise StrictLoadingViolationError, "#{owner.class} is marked as strict_loading and #{klass} cannot be lazily loaded." + Base.strict_loading_violation!(owner: owner.class, association: klass) end if reflection.strict_loading? - raise StrictLoadingViolationError, "The #{reflection.name} association is marked as strict_loading and cannot be lazily loaded." + Base.strict_loading_violation!(owner: owner.class, association: reflection.name) end scope = self.scope diff --git a/activerecord/lib/active_record/core.rb b/activerecord/lib/active_record/core.rb index dcd6d100ea..543721da1e 100644 --- a/activerecord/lib/active_record/core.rb +++ b/activerecord/lib/active_record/core.rb @@ -133,6 +133,12 @@ module ActiveRecord class_attribute :belongs_to_required_by_default, instance_accessor: false + ## + # :singleton-method: + # Set the application to log or raise when an association violates strict loading. + # Defaults to :raise. + mattr_accessor :action_on_strict_loading_violation, instance_accessor: false, default: :raise + class_attribute :strict_loading_by_default, instance_accessor: false, default: false mattr_accessor :writing_role, instance_accessor: false, default: :writing @@ -263,6 +269,17 @@ module ActiveRecord self.default_connection_handler = ConnectionAdapters::ConnectionHandler.new self.default_role = writing_role self.default_shard = :default + + def self.strict_loading_violation!(owner:, association:) # :nodoc: + case action_on_strict_loading_violation + when :raise + message = "`#{association}` called on `#{owner}` is marked for strict_loading and cannot be lazily loaded." + raise ActiveRecord::StrictLoadingViolationError.new(message) + when :log + name = "strict_loading_violation.active_record" + ActiveSupport::Notifications.instrument(name, owner: owner, association: association) + end + end end module ClassMethods diff --git a/activerecord/lib/active_record/log_subscriber.rb b/activerecord/lib/active_record/log_subscriber.rb index 5fc1749640..af6ad919f2 100644 --- a/activerecord/lib/active_record/log_subscriber.rb +++ b/activerecord/lib/active_record/log_subscriber.rb @@ -19,6 +19,15 @@ module ActiveRecord rt end + def strict_loading_violation(event) + debug do + owner = event.payload[:owner] + association = event.payload[:association] + + color("Strict loading violation: #{association} lazily loaded on #{owner}.", RED) + end + end + def sql(event) self.class.runtime += event.duration return unless logger.debug? diff --git a/activerecord/test/cases/strict_loading_test.rb b/activerecord/test/cases/strict_loading_test.rb index 8e7d0795cf..c730fbe588 100644 --- a/activerecord/test/cases/strict_loading_test.rb +++ b/activerecord/test/cases/strict_loading_test.rb @@ -359,6 +359,38 @@ class StrictLoadingTest < ActiveRecord::TestCase end end + def test_strict_loading_violation_raises_by_default + assert_equal :raise, ActiveRecord::Base.action_on_strict_loading_violation + + developer = Developer.first + assert_not_predicate developer, :strict_loading? + + developer.strict_loading! + assert_predicate developer, :strict_loading? + + assert_raises ActiveRecord::StrictLoadingViolationError do + developer.audit_logs.to_a + end + end + + def test_strict_loading_violation_can_log_instead_of_raise + old_value = ActiveRecord::Base.action_on_strict_loading_violation + ActiveRecord::Base.action_on_strict_loading_violation = :log + assert_equal :log, ActiveRecord::Base.action_on_strict_loading_violation + + developer = Developer.first + assert_not_predicate developer, :strict_loading? + + developer.strict_loading! + assert_predicate developer, :strict_loading? + + assert_logged("Strict loading violation: AuditLog lazily loaded on Developer.") do + developer.audit_logs.to_a + end + ensure + ActiveRecord::Base.action_on_strict_loading_violation = old_value + end + private def with_strict_loading_by_default(model) previous_strict_loading_by_default = model.strict_loading_by_default @@ -369,4 +401,19 @@ class StrictLoadingTest < ActiveRecord::TestCase ensure model.strict_loading_by_default = previous_strict_loading_by_default end + + def assert_logged(message) + old_logger = ActiveRecord::Base.logger + log = StringIO.new + ActiveRecord::Base.logger = Logger.new(log) + + begin + yield + + log.rewind + assert_match message, log.read + ensure + ActiveRecord::Base.logger = old_logger + end + end end diff --git a/guides/source/configuring.md b/guides/source/configuring.md index c9de29fc41..36a92a2aa2 100644 --- a/guides/source/configuring.md +++ b/guides/source/configuring.md @@ -436,6 +436,11 @@ in controllers and views. This defaults to `false`. controls whether a record fails validation if `belongs_to` association is not present. +* `config.active_record.action_on_strict_loading_violation` enables raising or + logging an exception if strict_loading is set on an association. The default + value is `:raise` in all environments. It can be changed to `:log` to send + violations to the logger instead of raising. + * `config.active_record.strict_loading_by_default` is a boolean value that either enables or disables strict_loading mode by default. Defaults to `false`.