Merge pull request rails#38541 from kddeisz/strict-loading-associations

eileencodes · web-flow · commit 3dff30c14fa2 · 2020-02-21T14:29:35.000-05:00
Support strict_loading on association declarations
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
@@ -1,3 +1,21 @@
+*   Add support for `strict_loading` mode on association declarations.
+
+    Raise an error if attempting to load a record from an association that has been marked as `strict_loading` unless it was explicitly eager loaded.
+
+    Usage:
+
+    ```
+    >> class Developer < ApplicationRecord
+    >>   has_many :projects, strict_loading: true
+    >> end
+    >>
+    >> dev = Developer.first
+    >> dev.projects.first
+    => ActiveRecord::StrictLoadingViolationError: The projects association is marked as strict_loading and cannot be lazily loaded.
+    ```
+
+    *Kevin Deisz*
+
 *   Add support for `strict_loading` mode to prevent lazy loading of records.
 
     Raise an error if a parent record is marked as `strict_loading` and attempts to lazily load its associations. This is useful for finding places you may want to preload an association and avoid additional queries.
diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb
@@ -1355,6 +1355,8 @@ module ClassMethods
         #   Specifies a module or array of modules that will be extended into the association object returned.
         #   Useful for defining methods on associations, especially when they should be shared between multiple
         #   association objects.
+        # [:strict_loading]
+        #   Enforces strict loading every time the associated record is loaded through this association.
         #
         # Option examples:
         #   has_many :comments, -> { order("posted_on") }
@@ -1365,6 +1367,7 @@ module ClassMethods
         #   has_many :tags, as: :taggable
         #   has_many :reports, -> { readonly }
         #   has_many :subscribers, through: :subscriptions, source: :user
+        #   has_many :comments, strict_loading: true
         def has_many(name, scope = nil, **options, &extension)
           reflection = Builder::HasMany.build(self, name, scope, options, &extension)
           Reflection.add_reflection self, name, reflection
@@ -1494,6 +1497,8 @@ def has_many(name, scope = nil, **options, &extension)
         #   When set to +true+, the association will also have its presence validated.
         #   This will validate the association itself, not the id. You can use
         #   +:inverse_of+ to avoid an extra query during validation.
+        # [:strict_loading]
+        #   Enforces strict loading every time the associated record is loaded through this association.
         #
         # Option examples:
         #   has_one :credit_card, dependent: :destroy  # destroys the associated credit card
@@ -1506,6 +1511,7 @@ def has_many(name, scope = nil, **options, &extension)
         #   has_one :club, through: :membership
         #   has_one :primary_address, -> { where(primary: true) }, through: :addressables, source: :addressable
         #   has_one :credit_card, required: true
+        #   has_one :credit_card, strict_loading: true
         def has_one(name, scope = nil, **options)
           reflection = Builder::HasOne.build(self, name, scope, options)
           Reflection.add_reflection self, name, reflection
@@ -1639,6 +1645,8 @@ def has_one(name, scope = nil, **options)
         # [:default]
         #   Provide a callable (i.e. proc or lambda) to specify that the association should
         #   be initialized with a particular record before validation.
+        # [:strict_loading]
+        #   Enforces strict loading every time the associated record is loaded through this association.
         #
         # Option examples:
         #   belongs_to :firm, foreign_key: "client_of"
@@ -1653,6 +1661,7 @@ def has_one(name, scope = nil, **options)
         #   belongs_to :company, touch: :employees_last_updated_at
         #   belongs_to :user, optional: true
         #   belongs_to :account, default: -> { company.account }
+        #   belongs_to :account, strict_loading: true
         def belongs_to(name, scope = nil, **options)
           reflection = Builder::BelongsTo.build(self, name, scope, options)
           Reflection.add_reflection self, name, reflection
@@ -1815,13 +1824,16 @@ def belongs_to(name, scope = nil, **options)
         #
         #   Note that NestedAttributes::ClassMethods#accepts_nested_attributes_for sets
         #   <tt>:autosave</tt> to <tt>true</tt>.
+        # [:strict_loading]
+        #   Enforces strict loading every time an associated record is loaded through this association.
         #
         # Option examples:
         #   has_and_belongs_to_many :projects
         #   has_and_belongs_to_many :projects, -> { includes(:milestones, :manager) }
         #   has_and_belongs_to_many :nations, class_name: "Country"
         #   has_and_belongs_to_many :categories, join_table: "prods_cats"
         #   has_and_belongs_to_many :categories, -> { readonly }
+        #   has_and_belongs_to_many :categories, strict_loading: true
         def has_and_belongs_to_many(name, scope = nil, **options, &extension)
           habtm_reflection = ActiveRecord::Reflection::HasAndBelongsToManyReflection.new(name, scope, options, self)
 
@@ -1852,7 +1864,7 @@ def destroy_associations
           hm_options[:through] = middle_reflection.name
           hm_options[:source] = join_model.right_reflection.name
 
-          [:before_add, :after_add, :before_remove, :after_remove, :autosave, :validate, :join_table, :class_name, :extend].each do |k|
+          [:before_add, :after_add, :before_remove, :after_remove, :autosave, :validate, :join_table, :class_name, :extend, :strict_loading].each do |k|
             hm_options[k] = options[k] if options.key? k
           end
 
diff --git a/activerecord/lib/active_record/associations/association.rb b/activerecord/lib/active_record/associations/association.rb
@@ -211,6 +211,10 @@ def find_target
             raise StrictLoadingViolationError, "#{owner.class} is marked as strict_loading and #{klass} cannot be lazily loaded."
           end
 
+          if reflection.strict_loading?
+            raise StrictLoadingViolationError, "The #{reflection.name} association is marked as strict_loading and cannot be lazily loaded."
+          end
+
           scope = self.scope
           return scope.to_a if skip_statement_cache?(scope)
 
diff --git a/activerecord/lib/active_record/associations/builder/association.rb b/activerecord/lib/active_record/associations/builder/association.rb
@@ -19,7 +19,7 @@ class << self
     self.extensions = []
 
     VALID_OPTIONS = [
-      :class_name, :anonymous_class, :primary_key, :foreign_key, :dependent, :validate, :inverse_of
+      :class_name, :anonymous_class, :primary_key, :foreign_key, :dependent, :validate, :inverse_of, :strict_loading
     ].freeze # :nodoc:
 
     def self.build(model, name, scope, options, &block)
diff --git a/activerecord/lib/active_record/associations/collection_association.rb b/activerecord/lib/active_record/associations/collection_association.rb
@@ -309,6 +309,7 @@ def null_scope?
       def find_from_target?
         loaded? ||
           owner.strict_loading? ||
+          reflection.strict_loading? ||
           owner.new_record? ||
           target.any? { |record| record.new_record? || record.changed? }
       end
diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb
@@ -306,6 +306,10 @@ def join_foreign_key
         active_record_primary_key
       end
 
+      def strict_loading?
+        options[:strict_loading]
+      end
+
       protected
         def actual_source_reflection # FIXME: this is a horrible name
           self
diff --git a/activerecord/test/cases/strict_loading_test.rb b/activerecord/test/cases/strict_loading_test.rb
@@ -3,9 +3,14 @@
 require "cases/helper"
 require "models/developer"
 require "models/computer"
+require "models/mentor"
+require "models/project"
+require "models/ship"
 
 class StrictLoadingTest < ActiveRecord::TestCase
   fixtures :developers
+  fixtures :projects
+  fixtures :ships
 
   def test_strict_loading
     Developer.all.each { |d| assert_not d.strict_loading? }
@@ -67,4 +72,88 @@ def test_raises_on_unloaded_relation_methods_if_strict_loading
       dev.audit_logs.first
     end
   end
+
+  def test_raises_on_lazy_loading_a_strict_loading_belongs_to_relation
+    mentor = Mentor.create!(name: "Mentor")
+
+    developer = Developer.first
+    developer.update_column(:mentor_id, mentor.id)
+
+    assert_raises ActiveRecord::StrictLoadingViolationError do
+      developer.strict_loading_mentor
+    end
+  end
+
+  def test_does_not_raise_on_eager_loading_a_strict_loading_belongs_to_relation
+    mentor = Mentor.create!(name: "Mentor")
+
+    Developer.first.update_column(:mentor_id, mentor.id)
+    developer = Developer.includes(:strict_loading_mentor).first
+
+    assert_nothing_raised { developer.strict_loading_mentor }
+  end
+
+  def test_raises_on_lazy_loading_a_strict_loading_has_one_relation
+    developer = Developer.first
+    ship = Ship.first
+
+    ship.update_column(:developer_id, developer.id)
+
+    assert_raises ActiveRecord::StrictLoadingViolationError do
+      developer.strict_loading_ship
+    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
+
+    assert_nothing_raised { developer.strict_loading_ship }
+  end
+
+  def test_raises_on_lazy_loading_a_strict_loading_has_many_relation
+    developer = Developer.first
+
+    AuditLog.create(
+      3.times.map do
+        { developer_id: developer.id, message: "I am message" }
+      end
+    )
+
+    assert_raises ActiveRecord::StrictLoadingViolationError do
+      developer.strict_loading_opt_audit_logs.first
+    end
+  end
+
+  def test_does_not_raise_on_eager_loading_a_strict_loading_has_many_relation
+    developer = Developer.first
+
+    AuditLog.create(
+      3.times.map do
+        { developer_id: developer.id, message: "I am message" }
+      end
+    )
+
+    developer = Developer.includes(:strict_loading_opt_audit_logs).first
+
+    assert_nothing_raised { developer.strict_loading_opt_audit_logs.first }
+  end
+
+  def test_raises_on_lazy_loading_a_strict_loading_habtm_relation
+    developer = Developer.first
+    developer.projects << Project.first
+
+    assert_not developer.strict_loading_projects.loaded?
+
+    assert_raises ActiveRecord::StrictLoadingViolationError do
+      developer.strict_loading_projects.first
+    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
 end
diff --git a/activerecord/test/models/developer.rb b/activerecord/test/models/developer.rb
@@ -18,6 +18,7 @@ def find_most_recent
   end
 
   belongs_to :mentor
+  belongs_to :strict_loading_mentor, strict_loading: true, foreign_key: :mentor_id, class_name: "Mentor"
 
   accepts_nested_attributes_for :projects
 
@@ -45,6 +46,12 @@ def find_least_recent
         end
       end
 
+  has_and_belongs_to_many :strict_loading_projects,
+                          join_table: :developers_projects,
+                          association_foreign_key: :project_id,
+                          class_name: "Project",
+                          strict_loading: true
+
   has_and_belongs_to_many :special_projects, join_table: "developers_projects", association_foreign_key: "project_id"
   has_and_belongs_to_many :sym_special_projects,
                           join_table: :developers_projects,
@@ -53,11 +60,14 @@ def find_least_recent
 
   has_many :audit_logs
   has_many :strict_loading_audit_logs, -> { strict_loading }, class_name: "AuditLog"
+  has_many :strict_loading_opt_audit_logs, strict_loading: true, class_name: "AuditLog"
   has_many :contracts
   has_many :firms, through: :contracts, source: :firm
   has_many :comments, ->(developer) { where(body: "I'm #{developer.name}") }
   has_many :ratings, through: :comments
+
   has_one :ship, dependent: :nullify
+  has_one :strict_loading_ship, strict_loading: true, class_name: "Ship"
 
   belongs_to :firm
   has_many :contracted_projects, class_name: "Project"
