diff --git a/lib/goldiloader/active_record_patches.rb b/lib/goldiloader/active_record_patches.rb index b205efc..497c7d1 100644 --- a/lib/goldiloader/active_record_patches.rb +++ b/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) diff --git a/spec/db/schema.rb b/spec/db/schema.rb index 8d083b8..b53b59e 100644 --- a/spec/db/schema.rb +++ b/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 diff --git a/spec/goldiloader/goldiloader_spec.rb b/spec/goldiloader/goldiloader_spec.rb index da1525e..77e0757 100644 --- a/spec/goldiloader/goldiloader_spec.rb +++ b/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