Fix potential scope leakage in ThroughAssociationPatch

lib/goldiloader/active_record_patches.rb

@@ -189,13 +189,90 @@ def auto_include?
       # If the through association can just be auto-included we're good
       return true if through_association.auto_include?
 
-      # If the through association was already loaded and does not contain new, changed, or destroyed records
-      # we are also able to auto-include the association. It means it has only already been read or changes are
-      # already persisted.
+      # The original logic allowed auto-including when the through association was already loaded
+      # and didn't contain new, changed, or destroyed records. However, this can cause scope leakage
+      # issues in Rails' Preloader where scopes from one association leak into queries for other
+      # associations when multiple through associations share the same through model.
+      #
+      # Specifically, when the owner model has a default_scope AND there are multiple scoped through
+      # associations using the same join table, Rails' Preloader may incorrectly apply scopes/orders
+      # from one association to another association's query.
+      #
+      # To prevent this, we disable the optimization if:
+      # 1. The owner model has a default_scope with ordering, AND
+      # 2. There are other scoped through associations using the same through association
+      return false if has_scope_leakage_risk?
+
       through_association.loaded? && Array.wrap(through_association.target).none? do |record|
         record.new_record? || record.changed? || record.destroyed?
       end
     end
+
+    private
+
+    def has_scope_leakage_risk?
+      check_class = resolve_check_class
+
+      # Only check for risk if owner has a default_scope with ordering
+      return false unless class_has_order_default_scope?(check_class)
+
+      # Check if there are other scoped through associations via the same join
+      has_other_scoped_through_associations?(check_class)
+    end
+
+    def resolve_check_class
+      # For STI subclasses, use the base class since that's where associations and default_scopes are defined
+      owner_class = owner.class
+      owner_class.respond_to?(:base_class) ? owner_class.base_class : owner_class
+    end
+
+    def class_has_order_default_scope?(check_class)
+      return false unless check_class.respond_to?(:default_scopes)
+
+      # Cache the result per class since default_scopes don't change at runtime
+      cache_key = check_class
+      @order_scope_cache ||= {}
+      return @order_scope_cache[cache_key] if @order_scope_cache.key?(cache_key)
+
+      @order_scope_cache[cache_key] = check_class.default_scopes.any? do |scope|
+        # Handle both Proc and ActiveRecord::Scoping::DefaultScope
+        scope_proc = scope.is_a?(Proc) ? scope : (scope.respond_to?(:scope) ? scope.scope : nil)
+        next false unless scope_proc
+
+        # Evaluate the scope to check if it contains ordering
+        begin
+          scope_relation = check_class.unscoped.instance_exec(&scope_proc)
+          scope_relation.order_values.present?
+        rescue StandardError => e
+          # If we can't evaluate the scope, be conservative and assume it has ordering
+          true
+        end
+      end
+    end
+
+    def has_other_scoped_through_associations?(check_class)
+      # Check if there are other through associations that:
+      # 1. Use the same through reflection
+      # 2. Have their own scopes (not just relying on the target model's default_scope)
+      through_name = through_reflection.name
+      current_assoc_name = reflection.name
+
+      # Cache the scoped through associations per class+through_name since they don't change at runtime
+      cache_key = [check_class, through_name]
+      @scoped_through_cache ||= {}
+
+      unless @scoped_through_cache.key?(cache_key)
+        @scoped_through_cache[cache_key] = check_class.reflect_on_all_associations.select do |assoc|
+          assoc.is_a?(ActiveRecord::Reflection::ThroughReflection) &&
+            assoc.through_reflection.name == through_name &&
+            assoc.scope.present?
+        end.map(&:name).to_set
+      end
+
+      # Check if there are any other scoped associations besides the current one
+      scoped_assocs = @scoped_through_cache[cache_key]
+      scoped_assocs.size > 1 || (scoped_assocs.size == 1 && !scoped_assocs.include?(current_assoc_name))
+    end
   end
   ::ActiveRecord::Associations::HasManyThroughAssociation.prepend(::Goldiloader::ThroughAssociationPatch)
   ::ActiveRecord::Associations::HasOneThroughAssociation.prepend(::Goldiloader::ThroughAssociationPatch)

spec/db/schema.rb

@@ -64,6 +64,24 @@
             unique: true
     t.foreign_key :active_storage_blobs, column: :blob_id
   end
+
+  # Models for testing scope leakage fix
+  create_table(:sections, force: true) do |t|
+    t.integer :position
+    t.string :name
+  end
+
+  create_table(:section_contents, force: true) do |t|
+    t.integer :section_id
+    t.integer :content_id
+    t.string :content_type
+    t.integer :position
+  end
+
+  create_table(:articles, force: true) do |t|
+    t.string :title
+    t.string :status
+  end
 end
 
 class Tag < ActiveRecord::Base
@@ -195,3 +213,25 @@ class ScopedAddress < ActiveRecord::Base
 class Group < ActiveRecord::Base
   has_many :tags, as: :owner
 end
+
+class Section < ActiveRecord::Base
+  default_scope { order(position: :asc) }
+
+  has_many :section_contents, dependent: :destroy
+
+  # These through associations reproduce the scope leakage bug pattern
+  # Some have scopes, some don't - this triggers the bug
+  has_many :published_articles, -> { where(status: 'published') }, through: :section_contents, source: :content, source_type: 'Article'
+  has_many :draft_articles, -> { where(status: 'draft') }, through: :section_contents, source: :content, source_type: 'Article'
+  has_many :all_articles, through: :section_contents, source: :content, source_type: 'Article'
+end
+
+class SectionContent < ActiveRecord::Base
+  default_scope { order(position: :asc) }
+
+  belongs_to :section
+  belongs_to :content, polymorphic: true
+end
+
+class Article < ActiveRecord::Base
+end

spec/goldiloader/goldiloader_spec.rb

@@ -1224,4 +1224,107 @@ def create_attachment(owner:, name:)
       )
     end
   end
+
+  context "with default_scope and multiple scoped through associations" do
+    # Tests for https://github.com/salsify/goldiloader/issues/166
+    # Bug: When a model has default_scope with ORDER BY and multiple has_many :through associations
+    # via the same join table, scopes from one association would leak into others during eager loading
+
+    let!(:section1) do
+      section = Section.create!(name: 'section1', position: 1)
+
+      section.section_contents.create!(content: Article.create!(title: 'section1-published1', status: 'published'), position: 1)
+      section.section_contents.create!(content: Article.create!(title: 'section1-published2', status: 'published'), position: 2)
+      section.section_contents.create!(content: Article.create!(title: 'section1-draft1', status: 'draft'), position: 3)
+
+      section
+    end
+
+    let!(:section2) do
+      section = Section.create!(name: 'section2', position: 2)
+
+      section.section_contents.create!(content: Article.create!(title: 'section2-published1', status: 'published'), position: 1)
+      section.section_contents.create!(content: Article.create!(title: 'section2-draft1', status: 'draft'), position: 2)
+      section.section_contents.create!(content: Article.create!(title: 'section2-draft2', status: 'draft'), position: 3)
+
+      section
+    end
+
+    let(:sections) { Section.order(:name).to_a }
+
+    context "when the through association has already been loaded" do
+      before do
+        sections.first.section_contents.to_a
+      end
+
+      it "doesn't leak scopes between through associations" do
+        # Access the scoped association first
+        sections.first.published_articles.to_a
+
+        # Now access the unscoped association
+        # Without the fix, the where(status: 'published') scope would leak here
+        all_articles = sections.first.all_articles.to_a
+
+        expect(all_articles.size).to eq 3
+        expect(all_articles.map(&:title)).to match_array(['section1-published1', 'section1-published2', 'section1-draft1'])
+      end
+
+      it "doesn't auto-include through associations when scope leakage risk exists" do
+        sections.first.published_articles.to_a
+
+        # The optimization should be disabled due to scope leakage risk
+        expect(sections.second.association(:published_articles)).not_to be_loaded
+      end
+
+      it "returns correct results for scoped through associations" do
+        published = sections.first.published_articles.to_a
+        drafts = sections.first.draft_articles.to_a
+
+        expect(published.map(&:title)).to match_array(['section1-published1', 'section1-published2'])
+        expect(drafts.map(&:title)).to eq(['section1-draft1'])
+      end
+
+      it "returns correct results when accessing multiple scoped associations" do
+        # Access published_articles first
+        published = sections.first.published_articles.to_a
+        expect(published.map(&:title)).to match_array(['section1-published1', 'section1-published2'])
+
+        # Then access draft_articles - should not include published scope
+        drafts = sections.first.draft_articles.to_a
+        expect(drafts.map(&:title)).to eq(['section1-draft1'])
+
+        # Then access all_articles - should not include any scope
+        all_articles = sections.first.all_articles.to_a
+        expect(all_articles.size).to eq 3
+        expect(all_articles.map(&:title)).to match_array(['section1-published1', 'section1-published2', 'section1-draft1'])
+      end
+
+      it "returns correct results when accessing unscoped before scoped associations" do
+        # Access unscoped association first
+        all_articles = sections.first.all_articles.to_a
+        expect(all_articles.size).to eq 3
+
+        # Then access scoped association
+        published = sections.first.published_articles.to_a
+        expect(published.size).to eq 2
+
+        # Unscoped should still return all records
+        all_articles_again = sections.first.all_articles.to_a
+        expect(all_articles_again.size).to eq 3
+      end
+    end
+
+    it "returns correct results for both sections" do
+      sections.first.section_contents.to_a
+
+      section1_articles = sections.first.all_articles.to_a
+      section2_articles = sections.second.all_articles.to_a
+
+      expect(section1_articles.size).to eq 3
+      expect(section2_articles.size).to eq 3
+
+      expect(section1_articles.map(&:title)).to match_array(['section1-published1', 'section1-published2', 'section1-draft1'])
+      expect(section2_articles.map(&:title)).to match_array(['section2-published1', 'section2-draft1', 'section2-draft2'])
+    end
+  end
 end