From 854de864502be67e9da779bbeb1aca968f458f94 Mon Sep 17 00:00:00 2001 From: Jamis Buck Date: Mon, 4 May 2026 13:12:01 -0600 Subject: [PATCH 01/19] MONGOID-5057 Add ReadonlyAssociation error class --- lib/config/locales/en.yml | 4 +++ lib/mongoid/errors.rb | 1 + lib/mongoid/errors/readonly_association.rb | 22 ++++++++++++ .../errors/readonly_association_spec.rb | 34 +++++++++++++++++++ 4 files changed, 61 insertions(+) create mode 100644 lib/mongoid/errors/readonly_association.rb create mode 100644 spec/mongoid/errors/readonly_association_spec.rb diff --git a/lib/config/locales/en.yml b/lib/config/locales/en.yml index b6f6884ca1..640c934b98 100644 --- a/lib/config/locales/en.yml +++ b/lib/config/locales/en.yml @@ -618,6 +618,10 @@ en: can only have values set when the document is a new record." resolution: "Don't define '%{name}' as readonly, or do not attempt to update its value after the document is persisted." + readonly_association: + message: "Attempted to modify the read-only association '%{name}' on '%{klass}'." + summary: "The :%{name} association is defined with :through and is read-only." + resolution: "To modify the association, update :%{through} directly." readonly_document: message: "Attempted to persist a readonly document of class '%{klass}'." summary: "Documents that are marked readonly cannot be persisted." diff --git a/lib/mongoid/errors.rb b/lib/mongoid/errors.rb index c3e7391a48..669e2fb821 100644 --- a/lib/mongoid/errors.rb +++ b/lib/mongoid/errors.rb @@ -60,6 +60,7 @@ require 'mongoid/errors/no_client_hosts' require 'mongoid/errors/readonly_attribute' require 'mongoid/errors/readonly_document' +require 'mongoid/errors/readonly_association' require 'mongoid/errors/rollback' require 'mongoid/errors/sessions_not_supported' require 'mongoid/errors/scope_overwrite' diff --git a/lib/mongoid/errors/readonly_association.rb b/lib/mongoid/errors/readonly_association.rb new file mode 100644 index 0000000000..489563f098 --- /dev/null +++ b/lib/mongoid/errors/readonly_association.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +module Mongoid + module Errors + # Raised when attempting to write to a :through association, which is + # read-only. + class ReadonlyAssociation < MongoidError + def initialize(klass, association) + super( + compose_message( + 'readonly_association', + { + klass: klass, + name: association.name, + through: association.options[:through] + } + ) + ) + end + end + end +end diff --git a/spec/mongoid/errors/readonly_association_spec.rb b/spec/mongoid/errors/readonly_association_spec.rb new file mode 100644 index 0000000000..36f5d64ed0 --- /dev/null +++ b/spec/mongoid/errors/readonly_association_spec.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Mongoid::Errors::ReadonlyAssociation do + let(:owner_class) do + Class.new do + def self.to_s + 'SomeOwner' + end + end + end + + let(:association) do + double(name: :patients, options: { through: :appointments }) + end + + describe '#message' do + it 'names the association' do + error = described_class.new(owner_class, association) + expect(error.message).to include(':patients') + end + + it 'names the owner class' do + error = described_class.new(owner_class, association) + expect(error.message).to include('SomeOwner') + end + + it 'names the intermediate association' do + error = described_class.new(owner_class, association) + expect(error.message).to include(':appointments') + end + end +end From cf23a93f9a39507de47b685edf09ba9b2037c5d7 Mon Sep 17 00:00:00 2001 From: Jamis Buck Date: Mon, 4 May 2026 13:18:27 -0600 Subject: [PATCH 02/19] MONGOID-5057 Add YARD docs to ReadonlyAssociation#initialize --- lib/mongoid/errors/readonly_association.rb | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lib/mongoid/errors/readonly_association.rb b/lib/mongoid/errors/readonly_association.rb index 489563f098..1b827e82a9 100644 --- a/lib/mongoid/errors/readonly_association.rb +++ b/lib/mongoid/errors/readonly_association.rb @@ -5,6 +5,13 @@ module Errors # Raised when attempting to write to a :through association, which is # read-only. class ReadonlyAssociation < MongoidError + # Instantiate the exception. + # + # @example Create the error. + # ReadonlyAssociation.new(Physician, association) + # + # @param [ Class ] klass The owner class. + # @param [ Mongoid::Association::Relatable ] association The through association. def initialize(klass, association) super( compose_message( From 43e0ba1968788eca41a7b4454302eb0219c02664 Mon Sep 17 00:00:00 2001 From: Jamis Buck Date: Mon, 4 May 2026 13:20:51 -0600 Subject: [PATCH 03/19] MONGOID-5057 Add THROUGH_MACRO_MAPPING and :through routing in define_association! --- lib/mongoid/association.rb | 7 +++++ lib/mongoid/association/macros.rb | 14 ++++++++- lib/mongoid/association/referenced.rb | 2 ++ .../referenced/has_many_through.rb | 30 +++++++++++++++++++ .../association/referenced/has_one_through.rb | 30 +++++++++++++++++++ spec/mongoid/association/macros_spec.rb | 26 ++++++++++++++++ 6 files changed, 108 insertions(+), 1 deletion(-) create mode 100644 lib/mongoid/association/referenced/has_many_through.rb create mode 100644 lib/mongoid/association/referenced/has_one_through.rb diff --git a/lib/mongoid/association.rb b/lib/mongoid/association.rb index a40ac97809..dfad121f37 100644 --- a/lib/mongoid/association.rb +++ b/lib/mongoid/association.rb @@ -46,6 +46,13 @@ module Association belongs_to: Association::Referenced::BelongsTo }.freeze + # Internal mapping used when :through option is present. Not exposed as + # callable macros. + THROUGH_MACRO_MAPPING = { + has_one: Association::Referenced::HasOneThrough, + has_many: Association::Referenced::HasManyThrough + }.freeze + attr_accessor :_association included do diff --git a/lib/mongoid/association/macros.rb b/lib/mongoid/association/macros.rb index f826bd0d07..518cd9ab30 100644 --- a/lib/mongoid/association/macros.rb +++ b/lib/mongoid/association/macros.rb @@ -218,7 +218,19 @@ def has_one(name, options = {}, &block) # rubocop:disable Naming/PredicatePrefix private def define_association!(macro_name, name, options = {}, &block) - Association::MACRO_MAPPING[macro_name].new(self, name, options, &block).tap do |assoc| + klass = if options[:through] + Association::THROUGH_MACRO_MAPPING[macro_name] || + raise( + Errors::InvalidRelationOption.new( + self, name, :through, + Association::THROUGH_MACRO_MAPPING.keys + ) + ) + else + Association::MACRO_MAPPING[macro_name] + end + + klass.new(self, name, options, &block).tap do |assoc| assoc.setup! self.relations = relations.merge(name => assoc) if assoc.embedded? && assoc.respond_to?(:store_as) && assoc.store_as != name diff --git a/lib/mongoid/association/referenced.rb b/lib/mongoid/association/referenced.rb index a726f2d7a2..0cb04fc2dd 100644 --- a/lib/mongoid/association/referenced.rb +++ b/lib/mongoid/association/referenced.rb @@ -7,3 +7,5 @@ require 'mongoid/association/referenced/has_many' require 'mongoid/association/referenced/has_and_belongs_to_many' require 'mongoid/association/referenced/has_one' +require 'mongoid/association/referenced/has_one_through' +require 'mongoid/association/referenced/has_many_through' diff --git a/lib/mongoid/association/referenced/has_many_through.rb b/lib/mongoid/association/referenced/has_many_through.rb new file mode 100644 index 0000000000..f263255387 --- /dev/null +++ b/lib/mongoid/association/referenced/has_many_through.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +module Mongoid + module Association + module Referenced + class HasManyThrough + include Relatable + + ASSOCIATION_OPTIONS = %i[class_name source through scope].freeze + VALID_OPTIONS = (ASSOCIATION_OPTIONS + SHARED_OPTIONS).freeze + + def embedded? + false + end + + def setup! + self + end + + def relation + Proxy + end + + # Placeholder proxy class; replaced in a later task. + class Proxy # rubocop:disable Lint/EmptyClass + end + end + end + end +end diff --git a/lib/mongoid/association/referenced/has_one_through.rb b/lib/mongoid/association/referenced/has_one_through.rb new file mode 100644 index 0000000000..ec329386ed --- /dev/null +++ b/lib/mongoid/association/referenced/has_one_through.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +module Mongoid + module Association + module Referenced + class HasOneThrough + include Relatable + + ASSOCIATION_OPTIONS = %i[class_name source through scope].freeze + VALID_OPTIONS = (ASSOCIATION_OPTIONS + SHARED_OPTIONS).freeze + + def embedded? + false + end + + def setup! + self + end + + def relation + Proxy + end + + # Placeholder proxy class; replaced in a later task. + class Proxy # rubocop:disable Lint/EmptyClass + end + end + end + end +end diff --git a/spec/mongoid/association/macros_spec.rb b/spec/mongoid/association/macros_spec.rb index d56ae738a5..4fc8a8bee2 100644 --- a/spec/mongoid/association/macros_spec.rb +++ b/spec/mongoid/association/macros_spec.rb @@ -1027,4 +1027,30 @@ def full_name end end end + + context 'with :through option' do + let(:klass) do + Class.new do + include Mongoid::Document + end + end + + it 'creates a HasManyThrough for has_many with :through' do + klass.has_many(:patients, through: :appointments) + assoc = klass.relations['patients'] + expect(assoc).to be_a(Mongoid::Association::Referenced::HasManyThrough) + end + + it 'creates a HasOneThrough for has_one with :through' do + klass.has_one(:store, through: :franchise) + assoc = klass.relations['store'] + expect(assoc).to be_a(Mongoid::Association::Referenced::HasOneThrough) + end + + it 'raises when :through is used with belongs_to' do + expect do + klass.belongs_to(:something, through: :other) + end.to raise_error(Mongoid::Errors::InvalidRelationOption) + end + end end From 1832ade14b7637c8570d5dde9a48c478fa3a5ecc Mon Sep 17 00:00:00 2001 From: Jamis Buck Date: Mon, 4 May 2026 13:23:51 -0600 Subject: [PATCH 04/19] MONGOID-5057 Fix ASSOCIATION_OPTIONS and add validation_default in through stubs --- lib/mongoid/association/referenced/has_many_through.rb | 6 +++++- lib/mongoid/association/referenced/has_one_through.rb | 6 +++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/lib/mongoid/association/referenced/has_many_through.rb b/lib/mongoid/association/referenced/has_many_through.rb index f263255387..93805b4951 100644 --- a/lib/mongoid/association/referenced/has_many_through.rb +++ b/lib/mongoid/association/referenced/has_many_through.rb @@ -6,7 +6,7 @@ module Referenced class HasManyThrough include Relatable - ASSOCIATION_OPTIONS = %i[class_name source through scope].freeze + ASSOCIATION_OPTIONS = %i[source through scope].freeze VALID_OPTIONS = (ASSOCIATION_OPTIONS + SHARED_OPTIONS).freeze def embedded? @@ -21,6 +21,10 @@ def relation Proxy end + def validation_default + false + end + # Placeholder proxy class; replaced in a later task. class Proxy # rubocop:disable Lint/EmptyClass end diff --git a/lib/mongoid/association/referenced/has_one_through.rb b/lib/mongoid/association/referenced/has_one_through.rb index ec329386ed..2ed276f1a6 100644 --- a/lib/mongoid/association/referenced/has_one_through.rb +++ b/lib/mongoid/association/referenced/has_one_through.rb @@ -6,7 +6,7 @@ module Referenced class HasOneThrough include Relatable - ASSOCIATION_OPTIONS = %i[class_name source through scope].freeze + ASSOCIATION_OPTIONS = %i[source through scope].freeze VALID_OPTIONS = (ASSOCIATION_OPTIONS + SHARED_OPTIONS).freeze def embedded? @@ -21,6 +21,10 @@ def relation Proxy end + def validation_default + false + end + # Placeholder proxy class; replaced in a later task. class Proxy # rubocop:disable Lint/EmptyClass end From 485ba7b277ff7d9120787f85d9c5abb0b6169833 Mon Sep 17 00:00:00 2001 From: Jamis Buck Date: Mon, 4 May 2026 13:27:28 -0600 Subject: [PATCH 05/19] MONGOID-5057 Implement HasOneThrough metadata, proxy, and eager loader --- .../association/referenced/has_one_through.rb | 123 +++++++++++++++++- .../referenced/has_one_through/eager.rb | 60 +++++++++ .../referenced/has_one_through/proxy.rb | 30 +++++ .../referenced/has_one_through/proxy_spec.rb | 49 +++++++ .../referenced/has_one_through_spec.rb | 88 +++++++++++++ 5 files changed, 346 insertions(+), 4 deletions(-) create mode 100644 lib/mongoid/association/referenced/has_one_through/eager.rb create mode 100644 lib/mongoid/association/referenced/has_one_through/proxy.rb create mode 100644 spec/mongoid/association/referenced/has_one_through/proxy_spec.rb create mode 100644 spec/mongoid/association/referenced/has_one_through_spec.rb diff --git a/lib/mongoid/association/referenced/has_one_through.rb b/lib/mongoid/association/referenced/has_one_through.rb index 2ed276f1a6..6c446e0d32 100644 --- a/lib/mongoid/association/referenced/has_one_through.rb +++ b/lib/mongoid/association/referenced/has_one_through.rb @@ -1,32 +1,147 @@ # frozen_string_literal: true +require 'mongoid/association/referenced/has_one_through/proxy' +require 'mongoid/association/referenced/has_one_through/eager' + module Mongoid module Association module Referenced + # Metadata class for has_one :through associations. class HasOneThrough include Relatable + # The options available for this type of association, in addition to the + # common ones. + # + # @return [ Array ] The extra valid options. ASSOCIATION_OPTIONS = %i[source through scope].freeze + + # The complete list of valid options for this association, including + # the shared ones. + # + # @return [ Array ] The valid options. VALID_OPTIONS = (ASSOCIATION_OPTIONS + SHARED_OPTIONS).freeze - def embedded? - false + # The list of association complements. + # + # @return [ Array ] + def relation_complements + [].freeze end + # Setup instance methods on the owner class. + # + # @return [ self ] def setup! + setup_instance_methods! self end + # Is this association embedded? + # + # @return [ false ] + def embedded? + false + end + + # The proxy class for this association type. + # + # @return [ Class ] def relation Proxy end + # The intermediate association metadata on the owner class. + # Resolved lazily to allow forward references. + # + # @return [ Mongoid::Association::Relatable ] + def through_association + @through_association ||= begin + assoc = @owner_class.relations[@options[:through].to_s] || + raise( + Errors::InvalidRelationOption.new( + @owner_class, name, :through, @options[:through] + ) + ) + if assoc.embedded? + raise( + Errors::InvalidRelationOption.new( + @owner_class, name, :through, + 'through association must be a referenced association, not embedded' + ) + ) + end + assoc + end + end + + # The source association metadata on the intermediate class. + # Resolved lazily to allow forward references. + # + # @return [ Mongoid::Association::Relatable ] + def source_association + @source_association ||= begin + source_name = (@options[:source] || name).to_s + through_association.klass.relations[source_name] || + raise( + Errors::InvalidRelationOption.new( + @owner_class, name, :source, source_name + ) + ) + end + end + + # Build the target by delegating through the intermediate proxy. + # Returns the source document or nil. + # + # @param [ Document ] base The owner document. + # + # @return [ Document | nil ] + def criteria(base) + through_target = base.public_send(through_association.name) + return nil if through_target.nil? + + through_target.public_send(source_association.name) + end + + # The default for validating the association object. + # + # @return [ false ] def validation_default false end - # Placeholder proxy class; replaced in a later task. - class Proxy # rubocop:disable Lint/EmptyClass + private + + def setup_instance_methods! + define_through_getter! + define_readonly_setter! + define_existence_check! + self + end + + def define_through_getter! + assoc = self + assoc_name = name + @owner_class.re_define_method(assoc_name) do |reload = false| + if reload || !instance_variable_defined?("@_#{assoc_name}") + doc = assoc.criteria(self) + proxy = doc ? HasOneThrough::Proxy.new(self, doc, assoc) : nil + set_relation(assoc_name, proxy) + end + instance_variable_get("@_#{assoc_name}") + end + end + + def define_readonly_setter! + assoc = self + @owner_class.re_define_method("#{name}=") do |_object| + raise Mongoid::Errors::ReadonlyAssociation.new(self.class, assoc) + end + end + + def default_primary_key + PRIMARY_KEY_DEFAULT end end end diff --git a/lib/mongoid/association/referenced/has_one_through/eager.rb b/lib/mongoid/association/referenced/has_one_through/eager.rb new file mode 100644 index 0000000000..c0c6a5c6d1 --- /dev/null +++ b/lib/mongoid/association/referenced/has_one_through/eager.rb @@ -0,0 +1,60 @@ +# frozen_string_literal: true + +module Mongoid + module Association + module Referenced + class HasOneThrough + # Two-query eager preloader for has_one :through associations. + class Eager < Association::Eager + private + + def preload + @docs.each { |d| set_relation(d, nil) } + + through_assoc = @association.through_association + source_assoc = @association.source_association + + # Step 1: load all intermediate records keyed by their FK to owner + owner_pk = through_assoc.primary_key + through_fk = through_assoc.foreign_key + + owner_ids = @docs.filter_map { |d| d.public_send(owner_pk) }.uniq + return if owner_ids.empty? + + intermediates = through_assoc.klass.where(through_fk => { '$in' => owner_ids }).to_a + + # Step 2: load all source records + if source_assoc.stores_foreign_key? + # FK is on the intermediate (e.g. belongs_to :store => intermediate.store_id) + source_fk_values = intermediates.filter_map { |i| i.public_send(source_assoc.foreign_key) }.uniq + targets = source_assoc.klass.where(source_assoc.primary_key => { '$in' => source_fk_values }).to_a + targets_by_key = targets.index_by { |t| t.public_send(source_assoc.primary_key) } + intermediate_to_target = intermediates.to_h do |i| + [ i.public_send(through_fk), targets_by_key[i.public_send(source_assoc.foreign_key)] ] + end + else + # FK is on the source (e.g. has_one :store => store.franchise_id) + intermediate_pks = intermediates.filter_map { |i| i.public_send(through_assoc.primary_key) }.uniq + targets = source_assoc.klass.where(source_assoc.foreign_key => { '$in' => intermediate_pks }).to_a + targets_by_fk = targets.index_by { |t| t.public_send(source_assoc.foreign_key) } + intermediate_by_owner_fk = intermediates.index_by { |i| i.public_send(through_fk) } + intermediate_to_target = intermediate_by_owner_fk.transform_values do |i| + targets_by_fk[i.public_send(through_assoc.primary_key)] + end + end + + # Step 3: set relation on each owner doc + @docs.each do |doc| + owner_key_val = doc.public_send(owner_pk) + set_relation(doc, intermediate_to_target[owner_key_val]) + end + end + + def group_by_key + @association.through_association.primary_key + end + end + end + end + end +end diff --git a/lib/mongoid/association/referenced/has_one_through/proxy.rb b/lib/mongoid/association/referenced/has_one_through/proxy.rb new file mode 100644 index 0000000000..e6899ff0b5 --- /dev/null +++ b/lib/mongoid/association/referenced/has_one_through/proxy.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +module Mongoid + module Association + module Referenced + class HasOneThrough + # Read-only proxy for has_one :through associations. + # Instances are returned by the association getter. Write attempts raise + # ReadonlyAssociation. + class Proxy < Association::One + module ClassMethods + def eager_loader(association, docs) + Eager.new(association, docs) + end + + # Returns true if the association is an embedded one. In this case + # always false. + # + # @return [ false ] Always false. + def embedded? + false + end + end + + extend ClassMethods + end + end + end + end +end diff --git a/spec/mongoid/association/referenced/has_one_through/proxy_spec.rb b/spec/mongoid/association/referenced/has_one_through/proxy_spec.rb new file mode 100644 index 0000000000..f50084d01a --- /dev/null +++ b/spec/mongoid/association/referenced/has_one_through/proxy_spec.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Mongoid::Association::Referenced::HasOneThrough::Proxy do + before(:all) do + Object.const_set(:PxHotStore, Class.new { include Mongoid::Document }) + Object.const_set(:PxHotFranchise, Class.new do + include Mongoid::Document + + has_one :px_hot_store, class_name: 'PxHotStore', inverse_of: :px_hot_franchise + end) + Object.const_set(:PxHotCustomer, Class.new do + include Mongoid::Document + + has_one :px_hot_franchise, class_name: 'PxHotFranchise', + inverse_of: :px_hot_customer + has_one :px_hot_store, through: :px_hot_franchise, class_name: 'PxHotStore', + source: :px_hot_store + end) + end + + after(:all) do + %w[PxHotCustomer PxHotFranchise PxHotStore].each { |c| Object.send(:remove_const, c) } + end + + let(:customer) { PxHotCustomer.new } + + describe 'setter' do + it 'raises ReadonlyAssociation' do + expect { customer.px_hot_store = PxHotStore.new }.to \ + raise_error(Mongoid::Errors::ReadonlyAssociation) + end + end + + describe '.eager_loader' do + it 'returns a HasOneThrough::Eager' do + assoc = PxHotCustomer.relations['px_hot_store'] + result = described_class.eager_loader([ assoc ], []) + expect(result).to be_a(Mongoid::Association::Referenced::HasOneThrough::Eager) + end + end + + describe '.embedded?' do + it 'returns false' do + expect(described_class.embedded?).to be false + end + end +end diff --git a/spec/mongoid/association/referenced/has_one_through_spec.rb b/spec/mongoid/association/referenced/has_one_through_spec.rb new file mode 100644 index 0000000000..2547a34c03 --- /dev/null +++ b/spec/mongoid/association/referenced/has_one_through_spec.rb @@ -0,0 +1,88 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Mongoid::Association::Referenced::HasOneThrough do + # Model setup: + # HotCustomer has_one :hot_franchise + # HotCustomer has_one :hot_store, through: :hot_franchise, source: :depot + # HotFranchise has_one :depot (class HotStore) + + before(:all) do + Object.const_set(:HotStore, Class.new do + include Mongoid::Document + + field :hot_franchise_id, type: BSON::ObjectId + end) + Object.const_set(:HotFranchise, Class.new do + include Mongoid::Document + + field :hot_customer_id, type: BSON::ObjectId + has_one :depot, class_name: 'HotStore', inverse_of: :hot_franchise + end) + Object.const_set(:HotCustomer, Class.new do + include Mongoid::Document + + has_one :hot_franchise, class_name: 'HotFranchise', inverse_of: :hot_customer + has_one :hot_store, through: :hot_franchise, + class_name: 'HotStore', source: :depot + end) + end + + after(:all) do + %w[HotCustomer HotFranchise HotStore].each { |c| Object.send(:remove_const, c) } + end + + let(:assoc) { HotCustomer.relations['hot_store'] } + + describe '#through_association' do + it 'returns the intermediate association metadata' do + expect(assoc.through_association).to eq(HotCustomer.relations['hot_franchise']) + end + end + + describe '#source_association' do + it 'returns the :depot association on HotFranchise' do + expect(assoc.source_association).to eq(HotFranchise.relations['depot']) + end + end + + describe '#embedded?' do + it 'returns false' do + expect(assoc.embedded?).to be false + end + end + + describe 'VALID_OPTIONS' do + it 'accepts :through' do + expect { HotCustomer.has_one(:foo, through: :hot_franchise) }.not_to raise_error + end + + it 'accepts :source' do + expect { HotCustomer.has_one(:bar, through: :hot_franchise, source: :depot) }.not_to raise_error + end + + it 'rejects unknown options at definition time' do + expect do + HotCustomer.has_one(:baz, through: :hot_franchise, bogus_option: true) + end.to raise_error(Mongoid::Errors::InvalidRelationOption) + end + end + + describe '#criteria' do + it 'returns nil when the intermediate is nil' do + customer = HotCustomer.new + allow(customer).to receive(:hot_franchise).and_return(nil) + expect(assoc.criteria(customer)).to be_nil + end + + it 'delegates to the source association on the intermediate' do + franchise = HotFranchise.new + store = HotStore.new + customer = HotCustomer.new + allow(customer).to receive(:hot_franchise).and_return(franchise) + allow(franchise).to receive(:depot).and_return(store) + expect(assoc.criteria(customer)).to eq(store) + end + end +end From 1793887c0ba06426e445e0f727ced226cba4092b Mon Sep 17 00:00:00 2001 From: Jamis Buck Date: Mon, 4 May 2026 13:42:53 -0600 Subject: [PATCH 06/19] MONGOID-5057 Add stores_foreign_key? and clarify criteria/group_by_key in HasOneThrough --- .../association/referenced/has_one_through.rb | 13 +++++++++++-- .../association/referenced/has_one_through/eager.rb | 3 +++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/lib/mongoid/association/referenced/has_one_through.rb b/lib/mongoid/association/referenced/has_one_through.rb index 6c446e0d32..f9a3dc28f1 100644 --- a/lib/mongoid/association/referenced/has_one_through.rb +++ b/lib/mongoid/association/referenced/has_one_through.rb @@ -91,8 +91,17 @@ def source_association end end - # Build the target by delegating through the intermediate proxy. - # Returns the source document or nil. + # Through associations never store a foreign key on the owner document. + # + # @return [ false ] + def stores_foreign_key? + false + end + + # Resolve the target by delegating through the intermediate proxy. + # Unlike other association types, this returns a document (or nil) + # directly rather than a Mongoid::Criteria, because the two-hop + # traversal is performed eagerly via the existing association proxy. # # @param [ Document ] base The owner document. # diff --git a/lib/mongoid/association/referenced/has_one_through/eager.rb b/lib/mongoid/association/referenced/has_one_through/eager.rb index c0c6a5c6d1..c4007557d1 100644 --- a/lib/mongoid/association/referenced/has_one_through/eager.rb +++ b/lib/mongoid/association/referenced/has_one_through/eager.rb @@ -50,6 +50,9 @@ def preload end end + # Required by the base class contract. Not called by this preloader + # because preload manages document traversal directly without using + # the grouped_docs / keys_from_docs machinery from the base class. def group_by_key @association.through_association.primary_key end From 40fb663e1d1fc224b62514d21897cebe6fe12e0c Mon Sep 17 00:00:00 2001 From: Jamis Buck Date: Mon, 4 May 2026 13:44:13 -0600 Subject: [PATCH 07/19] MONGOID-5057 Add has_one :through integration tests and embedded guard spec --- .../referenced/has_one_through_spec.rb | 81 +++++++++++++++++++ 1 file changed, 81 insertions(+) diff --git a/spec/mongoid/association/referenced/has_one_through_spec.rb b/spec/mongoid/association/referenced/has_one_through_spec.rb index 2547a34c03..e2547ca8e8 100644 --- a/spec/mongoid/association/referenced/has_one_through_spec.rb +++ b/spec/mongoid/association/referenced/has_one_through_spec.rb @@ -85,4 +85,85 @@ expect(assoc.criteria(customer)).to eq(store) end end + + describe ':through on an embedded association' do + it 'raises InvalidRelationOption when through association is embedded' do + embedded_owner = Class.new do + include Mongoid::Document + + embeds_one :address + end + embedded_owner.has_one(:city, through: :address) + expect do + embedded_owner.relations['city'].through_association # triggers lazy validation + end.to raise_error(Mongoid::Errors::InvalidRelationOption) + end + end + + context 'integration', :integration do + before(:all) do + Object.const_set(:IntCustomer, Class.new do + include Mongoid::Document + + store_in collection: 'int_customers' + has_one :int_franchise, class_name: 'IntFranchise', inverse_of: :int_customer + has_one :int_store, through: :int_franchise, class_name: 'IntStore' + end) + + Object.const_set(:IntFranchise, Class.new do + include Mongoid::Document + + store_in collection: 'int_franchises' + field :int_customer_id, type: BSON::ObjectId + belongs_to :int_customer, class_name: 'IntCustomer' + has_one :int_store, class_name: 'IntStore', inverse_of: :int_franchise + end) + + Object.const_set(:IntStore, Class.new do + include Mongoid::Document + + store_in collection: 'int_stores' + field :int_franchise_id, type: BSON::ObjectId + belongs_to :int_franchise, class_name: 'IntFranchise' + end) + end + + after(:all) do + %w[IntCustomer IntFranchise IntStore].each { |c| Object.send(:remove_const, c) } + end + + before { [ IntCustomer, IntFranchise, IntStore ].each(&:delete_all) } + + let!(:customer) { IntCustomer.create! } + let!(:franchise) { IntFranchise.create!(int_customer: customer) } + let!(:store) { IntStore.create!(int_franchise: franchise) } + + describe 'getter' do + it 'returns the store via the franchise' do + expect(customer.int_store).to eq(store) + end + + it 'returns nil when the franchise is absent' do + lone = IntCustomer.create! + expect(lone.int_store).to be_nil + end + + it 'reloads the association when reload: true' do + customer.int_store # prime cache + new_store = IntStore.create!(int_franchise: franchise) + franchise.int_store = new_store + franchise.save! + + # Without reload: stale cached value + expect(customer.int_store(true)).to eq(new_store) + end + end + + describe 'setter' do + it 'raises ReadonlyAssociation' do + expect { customer.int_store = IntStore.new }.to \ + raise_error(Mongoid::Errors::ReadonlyAssociation) + end + end + end end From 8ccdc6ee61e09a600cc62c5c663e4de06bdff9b1 Mon Sep 17 00:00:00 2001 From: Jamis Buck Date: Mon, 4 May 2026 13:46:25 -0600 Subject: [PATCH 08/19] MONGOID-5057 Strengthen has_one :through integration tests (source option, reload) --- .../referenced/has_one_through_spec.rb | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/spec/mongoid/association/referenced/has_one_through_spec.rb b/spec/mongoid/association/referenced/has_one_through_spec.rb index e2547ca8e8..8ab64fd497 100644 --- a/spec/mongoid/association/referenced/has_one_through_spec.rb +++ b/spec/mongoid/association/referenced/has_one_through_spec.rb @@ -107,7 +107,8 @@ store_in collection: 'int_customers' has_one :int_franchise, class_name: 'IntFranchise', inverse_of: :int_customer - has_one :int_store, through: :int_franchise, class_name: 'IntStore' + # source: :depot exercises the :source option end-to-end + has_one :int_store, through: :int_franchise, class_name: 'IntStore', source: :depot end) Object.const_set(:IntFranchise, Class.new do @@ -116,7 +117,7 @@ store_in collection: 'int_franchises' field :int_customer_id, type: BSON::ObjectId belongs_to :int_customer, class_name: 'IntCustomer' - has_one :int_store, class_name: 'IntStore', inverse_of: :int_franchise + has_one :depot, class_name: 'IntStore', inverse_of: :int_franchise end) Object.const_set(:IntStore, Class.new do @@ -148,14 +149,16 @@ expect(lone.int_store).to be_nil end - it 'reloads the association when reload: true' do - customer.int_store # prime cache - new_store = IntStore.create!(int_franchise: franchise) - franchise.int_store = new_store + it 'returns cached value before reload and fresh value after' do + first_store = customer.int_store # prime cache + expect(first_store).to eq(store) + + new_store = IntStore.create! + franchise.depot = new_store franchise.save! - # Without reload: stale cached value - expect(customer.int_store(true)).to eq(new_store) + expect(customer.int_store).to eq(store) # cached — still old + expect(customer.int_store(true)).to eq(new_store) # reloaded end end From f0768d41a7b6df246074fed6a52a5d22ac6901da Mon Sep 17 00:00:00 2001 From: Jamis Buck Date: Mon, 4 May 2026 13:51:45 -0600 Subject: [PATCH 09/19] MONGOID-5057 Implement HasManyThrough metadata, proxy, and eager loader --- .../referenced/has_many_through.rb | 163 +++++++++++++++++- .../referenced/has_many_through/eager.rb | 95 ++++++++++ .../referenced/has_many_through/proxy.rb | 56 ++++++ .../referenced/has_many_through/proxy_spec.rb | 62 +++++++ .../referenced/has_many_through_spec.rb | 68 ++++++++ 5 files changed, 440 insertions(+), 4 deletions(-) create mode 100644 lib/mongoid/association/referenced/has_many_through/eager.rb create mode 100644 lib/mongoid/association/referenced/has_many_through/proxy.rb create mode 100644 spec/mongoid/association/referenced/has_many_through/proxy_spec.rb create mode 100644 spec/mongoid/association/referenced/has_many_through_spec.rb diff --git a/lib/mongoid/association/referenced/has_many_through.rb b/lib/mongoid/association/referenced/has_many_through.rb index 93805b4951..4968cc0f94 100644 --- a/lib/mongoid/association/referenced/has_many_through.rb +++ b/lib/mongoid/association/referenced/has_many_through.rb @@ -1,32 +1,187 @@ # frozen_string_literal: true +require 'mongoid/association/referenced/has_many_through/proxy' +require 'mongoid/association/referenced/has_many_through/eager' + module Mongoid module Association module Referenced + # Metadata class for has_many :through associations. class HasManyThrough include Relatable + # The options available for this type of association, in addition to the + # common ones. + # + # @return [ Array ] The extra valid options. ASSOCIATION_OPTIONS = %i[source through scope].freeze + + # The complete list of valid options for this association, including + # the shared ones. + # + # @return [ Array ] The valid options. VALID_OPTIONS = (ASSOCIATION_OPTIONS + SHARED_OPTIONS).freeze - def embedded? - false + # The list of association complements. + # + # @return [ Array ] + def relation_complements + [].freeze end + # Setup instance methods on the owner class. + # + # @return [ self ] def setup! + setup_instance_methods! self end + # Is this association embedded? + # + # @return [ false ] + def embedded? + false + end + + # The proxy class for this association type. + # + # @return [ Class ] def relation Proxy end + # Through associations never store a foreign key on the owner document. + # + # @return [ false ] + def stores_foreign_key? + false + end + + # The intermediate association metadata on the owner class. + # Resolved lazily to allow forward references. + # + # @return [ Mongoid::Association::Relatable ] + def through_association + @through_association ||= begin + assoc = @owner_class.relations[@options[:through].to_s] || + raise( + Errors::InvalidRelationOption.new( + @owner_class, name, :through, @options[:through] + ) + ) + if assoc.embedded? + raise( + Errors::InvalidRelationOption.new( + @owner_class, name, :through, + 'through association must be a referenced association, not embedded' + ) + ) + end + assoc + end + end + + # The source association metadata on the intermediate class. + # Resolved lazily to allow forward references. + # + # @return [ Mongoid::Association::Relatable ] + def source_association + @source_association ||= begin + source_name = (@options[:source] || name.to_s.singularize).to_s + through_association.klass.relations[source_name] || + raise( + Errors::InvalidRelationOption.new( + @owner_class, name, :source, source_name + ) + ) + end + end + + # Return a base criteria for the source class. The actual two-query + # resolution is deferred until the proxy enumerates the results (see + # resolve). This method is used by callers that only need the target + # klass (e.g., eager loaders bootstrapping the association type). + # + # @param [ Document ] _base The owner document (unused here). + # + # @return [ Mongoid::Criteria ] + def criteria(_base = nil) + source_association.klass.criteria + end + + # Execute the two-query through-join and return a Criteria scoped to + # the matching target documents. Called lazily by the proxy on first + # enumeration. + # + # @param [ Document ] base The owner document. + # + # @return [ Mongoid::Criteria ] + def resolve(base) + through_crit = through_association.criteria(base) + + if source_association.stores_foreign_key? + # FK is on the intermediate (e.g. appointment.patient_id -> belongs_to :patient) + target_pk = source_association.primary_key # '_id' on Patient + source_fk = source_association.foreign_key # 'patient_id' on Appointment + source_association.klass.where( + target_pk => { '$in' => through_crit.pluck(source_fk) } + ) + else + # FK is on the source (e.g. reader.book_id -> has_many :readers on Book) + through_pk = through_association.primary_key # '_id' on Book + source_fk = source_association.foreign_key # 'book_id' on Reader + source_association.klass.where( + source_fk => { '$in' => through_crit.pluck(through_pk) } + ) + end + end + + # The default for validating the association object. + # + # @return [ false ] def validation_default false end - # Placeholder proxy class; replaced in a later task. - class Proxy # rubocop:disable Lint/EmptyClass + private + + def setup_instance_methods! + define_through_getter! + define_through_ids_getter! + define_readonly_setter! + define_existence_check! + self + end + + def define_through_getter! + assoc = self + assoc_name = name + @owner_class.re_define_method(assoc_name) do |reload = false| + if reload || !instance_variable_defined?("@_#{assoc_name}") + set_relation(assoc_name, HasManyThrough::Proxy.new(self, assoc)) + end + instance_variable_get("@_#{assoc_name}") + end + end + + def define_through_ids_getter! + assoc_name = name + ids_method = :"#{assoc_name.to_s.singularize}_ids" + @owner_class.re_define_method(ids_method) do + send(assoc_name).pluck(:_id) + end + end + + def define_readonly_setter! + assoc = self + @owner_class.re_define_method(:"#{name}=") do |_object| + raise Mongoid::Errors::ReadonlyAssociation.new(self.class, assoc) + end + end + + def default_primary_key + PRIMARY_KEY_DEFAULT end end end diff --git a/lib/mongoid/association/referenced/has_many_through/eager.rb b/lib/mongoid/association/referenced/has_many_through/eager.rb new file mode 100644 index 0000000000..45bf235533 --- /dev/null +++ b/lib/mongoid/association/referenced/has_many_through/eager.rb @@ -0,0 +1,95 @@ +# frozen_string_literal: true + +module Mongoid + module Association + module Referenced + class HasManyThrough + # Two-query eager preloader for has_many :through associations. + class Eager < Association::Eager + private + + def preload + @docs.each { |d| set_relation(d, []) } + + through_assoc = @association.through_association + source_assoc = @association.source_association + + owner_pk = through_assoc.primary_key + through_fk = through_assoc.foreign_key + + owner_ids = @docs.filter_map { |d| d.public_send(owner_pk) }.uniq + return if owner_ids.empty? + + # Step 1: load all intermediate records + intermediates = through_assoc.klass + .where(through_fk => { '$in' => owner_ids }) + .to_a + + # Step 2: map owner FK values to arrays of target docs + targets_by_owner_fk = build_targets_map(intermediates, through_fk, source_assoc, owner_pk) + + # Step 3: set relation on each owner doc + @docs.each do |doc| + key_val = doc.public_send(owner_pk) + set_relation(doc, targets_by_owner_fk[key_val] || []) + end + end + + # Build a Hash mapping each owner FK value to an array of target docs. + # Uses two different strategies depending on where the FK lives. + def build_targets_map(intermediates, through_fk, source_assoc, owner_pk) + if source_assoc.stores_foreign_key? + fk_on_intermediate_targets_map(intermediates, through_fk, source_assoc) + else + fk_on_source_targets_map(intermediates, through_fk, source_assoc, owner_pk) + end + end + + # FK is on the intermediate (e.g. appointment.patient_id -> belongs_to :patient). + def fk_on_intermediate_targets_map(intermediates, through_fk, source_assoc) + source_fk_vals = intermediates.filter_map { |i| i.public_send(source_assoc.foreign_key) }.uniq + targets = source_assoc.klass.where( + source_assoc.primary_key => { '$in' => source_fk_vals } + ).to_a + targets_by_pk = targets.group_by { |t| t.public_send(source_assoc.primary_key) } + + result = Hash.new { |h, k| h[k] = [] } + intermediates.each do |i| + owner_fk_val = i.public_send(through_fk) + matched = targets_by_pk[i.public_send(source_assoc.foreign_key)] || [] + result[owner_fk_val].concat(matched) + end + result + end + + # FK is on the source (e.g. reader.book_id -> has_many :readers on Book). + def fk_on_source_targets_map(intermediates, through_fk, source_assoc, owner_pk) + intermediate_pks = intermediates.filter_map { |i| i.public_send(owner_pk) }.uniq + targets = source_assoc.klass.where( + source_assoc.foreign_key => { '$in' => intermediate_pks } + ).to_a + targets_by_source_fk = targets.group_by { |t| t.public_send(source_assoc.foreign_key) } + + result = Hash.new { |h, k| h[k] = [] } + intermediates.each do |i| + owner_fk_val = i.public_send(through_fk) + matched = targets_by_source_fk[i.public_send(owner_pk)] || [] + result[owner_fk_val].concat(matched) + end + result + end + + def set_relation(doc, element) + doc.set_relation(@association.name, element) unless doc.blank? + end + + # Required by base class contract. Not called from preload since this + # class manages its own two-query traversal directly. + def group_by_key + @association.through_association.primary_key + end + end + end + end + end +end diff --git a/lib/mongoid/association/referenced/has_many_through/proxy.rb b/lib/mongoid/association/referenced/has_many_through/proxy.rb new file mode 100644 index 0000000000..28ab1ae8b3 --- /dev/null +++ b/lib/mongoid/association/referenced/has_many_through/proxy.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +module Mongoid + module Association + module Referenced + class HasManyThrough + # Read-only proxy for has_many :through associations. Wraps the lazy + # Criteria returned by the association and raises ReadonlyAssociation on + # any write attempt. + class Proxy + extend Forwardable + include Enumerable + + module ClassMethods + def eager_loader(association, docs) + Eager.new(association, docs) + end + + def embedded? + false + end + end + + extend ClassMethods + + def_delegators :criteria, + :each, :to_a, :first, :last, :count, :size, :length, + :where, :pluck, :exists?, :any?, :none?, :empty?, + :limit, :skip, :order_by, :only, :without + + READONLY_METHODS = %i[ + << push concat substitute build new create create! + delete delete_one delete_all destroy_all clear nullify + ].freeze + + def initialize(base, association) + @_base = base + @_association = association + end + + # Lazily compute the resolved criteria when first needed. + # Triggers the two-query through-join on first access. + def criteria + @criteria ||= @_association.resolve(@_base) + end + + READONLY_METHODS.each do |meth| + define_method(meth) do |*| + raise Mongoid::Errors::ReadonlyAssociation.new(@_base.class, @_association) + end + end + end + end + end + end +end diff --git a/spec/mongoid/association/referenced/has_many_through/proxy_spec.rb b/spec/mongoid/association/referenced/has_many_through/proxy_spec.rb new file mode 100644 index 0000000000..3eecbb84b7 --- /dev/null +++ b/spec/mongoid/association/referenced/has_many_through/proxy_spec.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Mongoid::Association::Referenced::HasManyThrough::Proxy do + before(:all) do + Object.const_set(:PxAppointment, Class.new do + include Mongoid::Document + + field :physician_id, type: BSON::ObjectId + field :patient_id, type: BSON::ObjectId + belongs_to :px_physician, class_name: 'PxPhysician' + belongs_to :px_patient, class_name: 'PxPatient' + end) + Object.const_set(:PxPatient, Class.new { include Mongoid::Document }) + Object.const_set(:PxPhysician, Class.new do + include Mongoid::Document + + has_many :px_appointments, class_name: 'PxAppointment', inverse_of: :px_physician + has_many :px_patients, through: :px_appointments, class_name: 'PxPatient', + source: :px_patient + end) + end + + after(:all) do + %w[PxPhysician PxAppointment PxPatient].each { |c| Object.send(:remove_const, c) } + end + + let(:physician) { PxPhysician.new } + + describe 'mutation methods' do + %i[<< push concat build create create! delete delete_all + destroy_all clear nullify substitute].each do |method| + it "raises ReadonlyAssociation on ##{method}" do + expect { physician.px_patients.public_send(method) }.to \ + raise_error(Mongoid::Errors::ReadonlyAssociation) + end + end + end + + describe 'read methods' do + it 'responds to #each' do + expect(physician.px_patients).to respond_to(:each) + end + + it 'responds to #to_a' do + expect(physician.px_patients).to respond_to(:to_a) + end + + it 'responds to #count' do + expect(physician.px_patients).to respond_to(:count) + end + end + + describe '.eager_loader' do + it 'returns a HasManyThrough::Eager' do + assoc = PxPhysician.relations['px_patients'] + result = described_class.eager_loader([ assoc ], []) + expect(result).to be_a(Mongoid::Association::Referenced::HasManyThrough::Eager) + end + end +end diff --git a/spec/mongoid/association/referenced/has_many_through_spec.rb b/spec/mongoid/association/referenced/has_many_through_spec.rb new file mode 100644 index 0000000000..c8a446168a --- /dev/null +++ b/spec/mongoid/association/referenced/has_many_through_spec.rb @@ -0,0 +1,68 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Mongoid::Association::Referenced::HasManyThrough do + before(:all) do + Object.const_set(:Appointment, Class.new do + include Mongoid::Document + + field :physician_id, type: BSON::ObjectId + field :patient_id, type: BSON::ObjectId + belongs_to :physician + belongs_to :patient + end) + + Object.const_set(:Patient, Class.new do + include Mongoid::Document + end) + + Object.const_set(:Physician, Class.new do + include Mongoid::Document + + has_many :appointments + has_many :patients, through: :appointments + end) + end + + after(:all) do + %w[Physician Appointment Patient].each { |c| Object.send(:remove_const, c) } + end + + let(:assoc) { Physician.relations['patients'] } + + describe '#through_association' do + it 'returns the appointments metadata' do + expect(assoc.through_association).to eq(Physician.relations['appointments']) + end + end + + describe '#source_association' do + it 'returns the patient belongs_to on Appointment' do + expect(assoc.source_association).to eq(Appointment.relations['patient']) + end + end + + describe '#embedded?' do + it 'returns false' do + expect(assoc.embedded?).to be false + end + end + + describe 'VALID_OPTIONS' do + it 'rejects unknown options' do + expect do + Physician.has_many(:foos, through: :appointments, bad_opt: true) + end.to raise_error(Mongoid::Errors::InvalidRelationOption) + end + end + + describe '#criteria' do + it 'returns a Criteria for the source class' do + physician = Physician.new + crit = assoc.criteria(physician) + expect(crit).to be_a(Mongoid::Criteria) + expect(crit.klass).to eq(Patient) + end + end +end From c26661221375d99757e2dccaec1fa2fce85a0380 Mon Sep 17 00:00:00 2001 From: Jamis Buck Date: Mon, 4 May 2026 13:54:40 -0600 Subject: [PATCH 10/19] MONGOID-5057 Clarify criteria vs resolve on HasManyThrough --- .../association/referenced/has_many_through.rb | 11 ++++------- .../association/referenced/has_many_through_spec.rb | 5 ++--- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/lib/mongoid/association/referenced/has_many_through.rb b/lib/mongoid/association/referenced/has_many_through.rb index 4968cc0f94..bc0580dd49 100644 --- a/lib/mongoid/association/referenced/has_many_through.rb +++ b/lib/mongoid/association/referenced/has_many_through.rb @@ -98,15 +98,12 @@ def source_association end end - # Return a base criteria for the source class. The actual two-query - # resolution is deferred until the proxy enumerates the results (see - # resolve). This method is used by callers that only need the target - # klass (e.g., eager loaders bootstrapping the association type). - # - # @param [ Document ] _base The owner document (unused here). + # Return an unscoped criteria for the source class. Used by the base + # infrastructure to determine the target class; the actual two-query + # resolution is performed by resolve(base) when the proxy enumerates. # # @return [ Mongoid::Criteria ] - def criteria(_base = nil) + def criteria source_association.klass.criteria end diff --git a/spec/mongoid/association/referenced/has_many_through_spec.rb b/spec/mongoid/association/referenced/has_many_through_spec.rb index c8a446168a..cfaaa8d115 100644 --- a/spec/mongoid/association/referenced/has_many_through_spec.rb +++ b/spec/mongoid/association/referenced/has_many_through_spec.rb @@ -58,9 +58,8 @@ end describe '#criteria' do - it 'returns a Criteria for the source class' do - physician = Physician.new - crit = assoc.criteria(physician) + it 'returns an unscoped Criteria for the source class' do + crit = assoc.criteria expect(crit).to be_a(Mongoid::Criteria) expect(crit.klass).to eq(Patient) end From f858297d79a49cab1c10aa0a7a1c44d429b76476 Mon Sep 17 00:00:00 2001 From: Jamis Buck Date: Mon, 4 May 2026 13:56:03 -0600 Subject: [PATCH 11/19] MONGOID-5057 Add has_many :through integration tests --- .../referenced/has_many_through_spec.rb | 113 ++++++++++++++++++ 1 file changed, 113 insertions(+) diff --git a/spec/mongoid/association/referenced/has_many_through_spec.rb b/spec/mongoid/association/referenced/has_many_through_spec.rb index cfaaa8d115..35b0d4bc63 100644 --- a/spec/mongoid/association/referenced/has_many_through_spec.rb +++ b/spec/mongoid/association/referenced/has_many_through_spec.rb @@ -64,4 +64,117 @@ expect(crit.klass).to eq(Patient) end end + + context 'integration — belongs_to source (join table)', :integration do + before(:all) do + Object.const_set(:JtPhysician, Class.new do + include Mongoid::Document + + store_in collection: 'jt_physicians' + has_many :jt_appointments, class_name: 'JtAppointment', inverse_of: :jt_physician + has_many :jt_patients, through: :jt_appointments, + class_name: 'JtPatient', source: :jt_patient + end) + Object.const_set(:JtAppointment, Class.new do + include Mongoid::Document + + store_in collection: 'jt_appointments' + belongs_to :jt_physician, class_name: 'JtPhysician' + belongs_to :jt_patient, class_name: 'JtPatient' + end) + Object.const_set(:JtPatient, Class.new do + include Mongoid::Document + + store_in collection: 'jt_patients' + end) + end + + after(:all) do + %w[JtPhysician JtAppointment JtPatient].each { |c| Object.send(:remove_const, c) } + end + + before { [ JtPhysician, JtAppointment, JtPatient ].each(&:delete_all) } + + let!(:physician) { JtPhysician.create! } + let!(:patient1) { JtPatient.create! } + let!(:patient2) { JtPatient.create! } + let!(:_appt1) { JtAppointment.create!(jt_physician: physician, jt_patient: patient1) } + let!(:_appt2) { JtAppointment.create!(jt_physician: physician, jt_patient: patient2) } + + it 'returns all patients through appointments' do + expect(physician.jt_patients.to_a).to contain_exactly(patient1, patient2) + end + + it 'does not include patients of other physicians' do + other = JtPhysician.create! + other_patient = JtPatient.create! + JtAppointment.create!(jt_physician: other, jt_patient: other_patient) + expect(physician.jt_patients.to_a).not_to include(other_patient) + end + + it 'returns an empty result when no appointments exist' do + lone = JtPhysician.create! + expect(lone.jt_patients.to_a).to eq([]) + end + + it 'reloads on demand' do + physician.jt_patients.to_a # prime cache + new_patient = JtPatient.create! + JtAppointment.create!(jt_physician: physician, jt_patient: new_patient) + expect(physician.jt_patients(true).to_a).to include(new_patient) + end + + it 'raises ReadonlyAssociation on <<' do + expect { physician.jt_patients << JtPatient.new }.to \ + raise_error(Mongoid::Errors::ReadonlyAssociation) + end + + it 'exposes a singular_ids getter' do + ids = physician.jt_patient_ids + expect(ids).to contain_exactly(patient1.id, patient2.id) + end + end + + context 'integration — has_many source', :integration do + before(:all) do + Object.const_set(:HmAuthor, Class.new do + include Mongoid::Document + + store_in collection: 'hm_authors' + has_many :hm_books, class_name: 'HmBook', inverse_of: :hm_author + has_many :hm_readers, through: :hm_books, + class_name: 'HmReader', source: :hm_readers + end) + Object.const_set(:HmBook, Class.new do + include Mongoid::Document + + store_in collection: 'hm_books' + belongs_to :hm_author, class_name: 'HmAuthor' + has_many :hm_readers, class_name: 'HmReader', inverse_of: :hm_book + end) + Object.const_set(:HmReader, Class.new do + include Mongoid::Document + + store_in collection: 'hm_readers' + belongs_to :hm_book, class_name: 'HmBook' + end) + end + + after(:all) do + %w[HmAuthor HmBook HmReader].each { |c| Object.send(:remove_const, c) } + end + + before { [ HmAuthor, HmBook, HmReader ].each(&:delete_all) } + + let!(:author) { HmAuthor.create! } + let!(:book1) { HmBook.create!(hm_author: author) } + let!(:book2) { HmBook.create!(hm_author: author) } + let!(:r1) { HmReader.create!(hm_book: book1) } + let!(:r2) { HmReader.create!(hm_book: book1) } + let!(:r3) { HmReader.create!(hm_book: book2) } + + it 'returns readers across all books' do + expect(author.hm_readers.to_a).to contain_exactly(r1, r2, r3) + end + end end From 15378d83def4cf5ea1f02e6efda86cb2357ecfe4 Mon Sep 17 00:00:00 2001 From: Jamis Buck Date: Mon, 4 May 2026 13:58:20 -0600 Subject: [PATCH 12/19] MONGOID-5057 Fall back to preload in eager_load for :through associations When eager_load is called with a HasOneThrough or HasManyThrough association, log a warning naming the through associations and fall back to the includes-style preload path, since $lookup does not support two-hop through queries. --- lib/mongoid/association/eager_loadable.rb | 14 ++++++ .../association/eager_loadable_spec.rb | 47 +++++++++++++++++++ 2 files changed, 61 insertions(+) create mode 100644 spec/mongoid/association/eager_loadable_spec.rb diff --git a/lib/mongoid/association/eager_loadable.rb b/lib/mongoid/association/eager_loadable.rb index 5e82d0e6cd..ece83b2944 100644 --- a/lib/mongoid/association/eager_loadable.rb +++ b/lib/mongoid/association/eager_loadable.rb @@ -44,6 +44,20 @@ def eager_load_with_lookup return eager_load(docs_for_lookup_fallback) end + through_inclusions = criteria.inclusions.select do |assoc| + assoc.is_a?(Association::Referenced::HasOneThrough) || + assoc.is_a?(Association::Referenced::HasManyThrough) + end + + if through_inclusions.any? + names = through_inclusions.map { |a| ":#{a.name}" }.join(', ') + Mongoid.logger.warn( + 'The following :through associations do not support $lookup-based eager loading ' \ + "and will be preloaded using separate queries: #{names}." + ) + return eager_load(docs_for_lookup_fallback) + end + preload_for_lookup(criteria) end diff --git a/spec/mongoid/association/eager_loadable_spec.rb b/spec/mongoid/association/eager_loadable_spec.rb new file mode 100644 index 0000000000..b0cbacdb90 --- /dev/null +++ b/spec/mongoid/association/eager_loadable_spec.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Mongoid::Association::EagerLoadable do + context 'with through associations' do + before(:all) do + Object.const_set(:ELPhysician, Class.new do + include Mongoid::Document + + store_in collection: 'el_physicians' + has_many :el_appointments, class_name: 'ELAppointment', inverse_of: :el_physician + has_many :el_patients, through: :el_appointments, + class_name: 'ELPatient', source: :el_patient + end) + Object.const_set(:ELAppointment, Class.new do + include Mongoid::Document + + store_in collection: 'el_appointments' + belongs_to :el_physician, class_name: 'ELPhysician' + belongs_to :el_patient, class_name: 'ELPatient' + end) + Object.const_set(:ELPatient, Class.new do + include Mongoid::Document + + store_in collection: 'el_patients' + end) + end + + after(:all) do + %w[ELPhysician ELAppointment ELPatient].each { |c| Object.send(:remove_const, c) } + end + + before { [ ELPhysician, ELAppointment, ELPatient ].each(&:delete_all) } + + it 'logs a warning and falls back to preload when eager_load is used with a through association' do + physician = ELPhysician.create! + patient = ELPatient.create! + ELAppointment.create!(el_physician: physician, el_patient: patient) + + expect(Mongoid.logger).to receive(:warn).with(a_string_including('through')) + + docs = ELPhysician.eager_load(:el_patients).to_a + expect(docs.first.el_patients.to_a).to include(patient) + end + end +end From 29ed3966268f3dcb2f8023ea3726d5b931178d24 Mon Sep 17 00:00:00 2001 From: Jamis Buck Date: Mon, 4 May 2026 13:59:53 -0600 Subject: [PATCH 13/19] MONGOID-5057 Clarify eager_load fallback warning when mixed inclusions are present --- lib/mongoid/association/eager_loadable.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/mongoid/association/eager_loadable.rb b/lib/mongoid/association/eager_loadable.rb index ece83b2944..d46d4c3f44 100644 --- a/lib/mongoid/association/eager_loadable.rb +++ b/lib/mongoid/association/eager_loadable.rb @@ -52,8 +52,8 @@ def eager_load_with_lookup if through_inclusions.any? names = through_inclusions.map { |a| ":#{a.name}" }.join(', ') Mongoid.logger.warn( - 'The following :through associations do not support $lookup-based eager loading ' \ - "and will be preloaded using separate queries: #{names}." + "#{names} are :through associations and do not support $lookup-based eager " \ + 'loading. All inclusions for this query will be preloaded using separate queries.' ) return eager_load(docs_for_lookup_fallback) end From 2e66baa9f79303b430fe53ee978c2278f373aef5 Mon Sep 17 00:00:00 2001 From: Jamis Buck Date: Mon, 4 May 2026 14:01:16 -0600 Subject: [PATCH 14/19] MONGOID-5057 Add eager loading integration tests for :through associations --- .../referenced/has_many_through/eager_spec.rb | 58 +++++++++++++++++++ .../referenced/has_one_through/eager_spec.rb | 58 +++++++++++++++++++ 2 files changed, 116 insertions(+) create mode 100644 spec/mongoid/association/referenced/has_many_through/eager_spec.rb create mode 100644 spec/mongoid/association/referenced/has_one_through/eager_spec.rb diff --git a/spec/mongoid/association/referenced/has_many_through/eager_spec.rb b/spec/mongoid/association/referenced/has_many_through/eager_spec.rb new file mode 100644 index 0000000000..bcb70c2209 --- /dev/null +++ b/spec/mongoid/association/referenced/has_many_through/eager_spec.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Mongoid::Association::Referenced::HasManyThrough::Eager do + before(:all) do + Object.const_set(:EgPhysician, Class.new do + include Mongoid::Document + + store_in collection: 'eg_physicians' + has_many :eg_appointments, class_name: 'EgAppointment', inverse_of: :eg_physician + has_many :eg_patients, through: :eg_appointments, + class_name: 'EgPatient', source: :eg_patient + end) + Object.const_set(:EgAppointment, Class.new do + include Mongoid::Document + + store_in collection: 'eg_appointments' + belongs_to :eg_physician, class_name: 'EgPhysician' + belongs_to :eg_patient, class_name: 'EgPatient' + end) + Object.const_set(:EgPatient, Class.new do + include Mongoid::Document + + store_in collection: 'eg_patients' + end) + end + + after(:all) do + %w[EgPhysician EgAppointment EgPatient].each { |c| Object.send(:remove_const, c) } + end + + before { [ EgPhysician, EgAppointment, EgPatient ].each(&:delete_all) } + + it 'preloads patients for multiple physicians' do + p1 = EgPhysician.create! + p2 = EgPhysician.create! + pat1 = EgPatient.create! + pat2 = EgPatient.create! + pat3 = EgPatient.create! + EgAppointment.create!(eg_physician: p1, eg_patient: pat1) + EgAppointment.create!(eg_physician: p1, eg_patient: pat2) + EgAppointment.create!(eg_physician: p2, eg_patient: pat3) + + physicians = EgPhysician.includes(:eg_patients).to_a + by_id = physicians.index_by(&:id) + + expect(by_id[p1.id].eg_patients.to_a).to contain_exactly(pat1, pat2) + expect(by_id[p2.id].eg_patients.to_a).to contain_exactly(pat3) + end + + it 'sets [] on physicians with no appointments' do + lone = EgPhysician.create! + physicians = EgPhysician.includes(:eg_patients).to_a + loaded = physicians.find { |p| p.id == lone.id } + expect(loaded.eg_patients.to_a).to eq([]) + end +end diff --git a/spec/mongoid/association/referenced/has_one_through/eager_spec.rb b/spec/mongoid/association/referenced/has_one_through/eager_spec.rb new file mode 100644 index 0000000000..8c02518aca --- /dev/null +++ b/spec/mongoid/association/referenced/has_one_through/eager_spec.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Mongoid::Association::Referenced::HasOneThrough::Eager do + before(:all) do + Object.const_set(:EgCustomer, Class.new do + include Mongoid::Document + + store_in collection: 'eg_customers' + has_one :eg_franchise, class_name: 'EgFranchise', inverse_of: :eg_customer + has_one :eg_store, through: :eg_franchise, class_name: 'EgStore' + end) + Object.const_set(:EgFranchise, Class.new do + include Mongoid::Document + + store_in collection: 'eg_franchises' + belongs_to :eg_customer, class_name: 'EgCustomer' + has_one :eg_store, class_name: 'EgStore', inverse_of: :eg_franchise + end) + Object.const_set(:EgStore, Class.new do + include Mongoid::Document + + store_in collection: 'eg_stores' + belongs_to :eg_franchise, class_name: 'EgFranchise' + end) + end + + after(:all) do + %w[EgCustomer EgFranchise EgStore].each { |c| Object.send(:remove_const, c) } + end + + before { [ EgCustomer, EgFranchise, EgStore ].each(&:delete_all) } + + it 'preloads the through association for multiple owners' do + c1 = EgCustomer.create! + c2 = EgCustomer.create! + f1 = EgFranchise.create!(eg_customer: c1) + f2 = EgFranchise.create!(eg_customer: c2) + s1 = EgStore.create!(eg_franchise: f1) + s2 = EgStore.create!(eg_franchise: f2) + + customers = EgCustomer.includes(:eg_store).to_a + by_id = customers.index_by(&:id) + + expect(by_id[c1.id].eg_store).to eq(s1) + expect(by_id[c2.id].eg_store).to eq(s2) + end + + it 'sets nil on owners without a store' do + lone = EgCustomer.create! + EgFranchise.create!(eg_customer: lone) # franchise but no store + + customers = EgCustomer.includes(:eg_store).to_a + loaded = customers.find { |c| c.id == lone.id } + expect(loaded.eg_store).to be_nil + end +end From 298ca1f906404b56e391721a672e51e83ded2974 Mon Sep 17 00:00:00 2001 From: Jamis Buck Date: Mon, 4 May 2026 14:19:30 -0600 Subject: [PATCH 15/19] MONGOID-5057 Add :source option integration test --- .../referenced/has_one_through_spec.rb | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/spec/mongoid/association/referenced/has_one_through_spec.rb b/spec/mongoid/association/referenced/has_one_through_spec.rb index 8ab64fd497..0bdb6f42b6 100644 --- a/spec/mongoid/association/referenced/has_one_through_spec.rb +++ b/spec/mongoid/association/referenced/has_one_through_spec.rb @@ -169,4 +169,43 @@ end end end + + context 'with :source option resolving a differently-named association', :integration do + before(:all) do + Object.const_set(:SrcOrg, Class.new do + include Mongoid::Document + + store_in collection: 'src_orgs' + has_one :src_location, class_name: 'SrcLocation', inverse_of: :src_org + has_one :src_hq, through: :src_location, + class_name: 'SrcBuilding', source: :src_main_building + end) + Object.const_set(:SrcLocation, Class.new do + include Mongoid::Document + + store_in collection: 'src_locations' + belongs_to :src_org, class_name: 'SrcOrg' + has_one :src_main_building, class_name: 'SrcBuilding', inverse_of: :src_location + end) + Object.const_set(:SrcBuilding, Class.new do + include Mongoid::Document + + store_in collection: 'src_buildings' + belongs_to :src_location, class_name: 'SrcLocation' + end) + end + + after(:all) do + %w[SrcOrg SrcLocation SrcBuilding].each { |c| Object.send(:remove_const, c) } + end + + before { [ SrcOrg, SrcLocation, SrcBuilding ].each(&:delete_all) } + + it 'resolves the :source association on the intermediate' do + org = SrcOrg.create! + location = SrcLocation.create!(src_org: org) + building = SrcBuilding.create!(src_location: location) + expect(org.src_hq).to eq(building) + end + end end From 41697e12d7b8950cc2cffbcfaa4fd9eac9e16916 Mon Sep 17 00:00:00 2001 From: Jamis Buck Date: Mon, 4 May 2026 16:55:46 -0600 Subject: [PATCH 16/19] MONGOID-5057 Fix HasManyThrough#criteria to accept base and return scoped criteria The metadata-level criteria(base) method must mirror the contract established by HasMany and other associations: it takes the owner document and returns a Criteria scoped to that owner's related records. The previous split into an unscoped criteria/0 and a resolve(base) method meant that any caller using the standard Relatable interface (e.g. eager loaders) would receive an unscoped result. Also add sum/avg/min/max to the proxy's delegator list so that aggregation methods work consistently with other has_many proxies. --- .../association/referenced/has_many_through.rb | 17 ++++------------- .../referenced/has_many_through/proxy.rb | 7 +++---- .../referenced/has_many_through_spec.rb | 7 +++++-- 3 files changed, 12 insertions(+), 19 deletions(-) diff --git a/lib/mongoid/association/referenced/has_many_through.rb b/lib/mongoid/association/referenced/has_many_through.rb index bc0580dd49..80eff67ed3 100644 --- a/lib/mongoid/association/referenced/has_many_through.rb +++ b/lib/mongoid/association/referenced/has_many_through.rb @@ -98,23 +98,14 @@ def source_association end end - # Return an unscoped criteria for the source class. Used by the base - # infrastructure to determine the target class; the actual two-query - # resolution is performed by resolve(base) when the proxy enumerates. - # - # @return [ Mongoid::Criteria ] - def criteria - source_association.klass.criteria - end - - # Execute the two-query through-join and return a Criteria scoped to - # the matching target documents. Called lazily by the proxy on first - # enumeration. + # Return a Criteria scoped to the target documents reachable from base + # via the through association. Performs two queries: one against the + # intermediate collection, one against the source collection. # # @param [ Document ] base The owner document. # # @return [ Mongoid::Criteria ] - def resolve(base) + def criteria(base) through_crit = through_association.criteria(base) if source_association.stores_foreign_key? diff --git a/lib/mongoid/association/referenced/has_many_through/proxy.rb b/lib/mongoid/association/referenced/has_many_through/proxy.rb index 28ab1ae8b3..abae4629f1 100644 --- a/lib/mongoid/association/referenced/has_many_through/proxy.rb +++ b/lib/mongoid/association/referenced/has_many_through/proxy.rb @@ -26,7 +26,8 @@ def embedded? def_delegators :criteria, :each, :to_a, :first, :last, :count, :size, :length, :where, :pluck, :exists?, :any?, :none?, :empty?, - :limit, :skip, :order_by, :only, :without + :limit, :skip, :order_by, :only, :without, + :sum, :avg, :min, :max READONLY_METHODS = %i[ << push concat substitute build new create create! @@ -38,10 +39,8 @@ def initialize(base, association) @_association = association end - # Lazily compute the resolved criteria when first needed. - # Triggers the two-query through-join on first access. def criteria - @criteria ||= @_association.resolve(@_base) + @criteria ||= @_association.criteria(@_base) end READONLY_METHODS.each do |meth| diff --git a/spec/mongoid/association/referenced/has_many_through_spec.rb b/spec/mongoid/association/referenced/has_many_through_spec.rb index 35b0d4bc63..6ed1971e8b 100644 --- a/spec/mongoid/association/referenced/has_many_through_spec.rb +++ b/spec/mongoid/association/referenced/has_many_through_spec.rb @@ -58,8 +58,11 @@ end describe '#criteria' do - it 'returns an unscoped Criteria for the source class' do - crit = assoc.criteria + it 'returns a scoped Criteria for the source class' do + physician = Physician.new + appointments_crit = instance_double(Mongoid::Criteria, pluck: []) + allow(assoc.through_association).to receive(:criteria).with(physician).and_return(appointments_crit) + crit = assoc.criteria(physician) expect(crit).to be_a(Mongoid::Criteria) expect(crit.klass).to eq(Patient) end From 882e793f22dbe40f87ae37c00024d41448bbd61a Mon Sep 17 00:00:00 2001 From: Jamis Buck Date: Tue, 26 May 2026 10:28:17 -0600 Subject: [PATCH 17/19] Add :order option, and disallow HABTM as a source for has_any :through --- .../referenced/has_many_through.rb | 46 ++++++----- .../referenced/has_many_through_spec.rb | 79 +++++++++++++++++++ 2 files changed, 107 insertions(+), 18 deletions(-) diff --git a/lib/mongoid/association/referenced/has_many_through.rb b/lib/mongoid/association/referenced/has_many_through.rb index 80eff67ed3..29117db3b0 100644 --- a/lib/mongoid/association/referenced/has_many_through.rb +++ b/lib/mongoid/association/referenced/has_many_through.rb @@ -14,7 +14,7 @@ class HasManyThrough # common ones. # # @return [ Array ] The extra valid options. - ASSOCIATION_OPTIONS = %i[source through scope].freeze + ASSOCIATION_OPTIONS = %i[order source through scope].freeze # The complete list of valid options for this association, including # the shared ones. @@ -89,12 +89,21 @@ def through_association def source_association @source_association ||= begin source_name = (@options[:source] || name.to_s.singularize).to_s - through_association.klass.relations[source_name] || + assoc = through_association.klass.relations[source_name] || + raise( + Errors::InvalidRelationOption.new( + @owner_class, name, :source, source_name + ) + ) + if assoc.is_a?(Referenced::HasAndBelongsToMany) raise( Errors::InvalidRelationOption.new( - @owner_class, name, :source, source_name + @owner_class, name, :source, + 'has_and_belongs_to_many is not supported as a :through source' ) ) + end + assoc end end @@ -108,21 +117,22 @@ def source_association def criteria(base) through_crit = through_association.criteria(base) - if source_association.stores_foreign_key? - # FK is on the intermediate (e.g. appointment.patient_id -> belongs_to :patient) - target_pk = source_association.primary_key # '_id' on Patient - source_fk = source_association.foreign_key # 'patient_id' on Appointment - source_association.klass.where( - target_pk => { '$in' => through_crit.pluck(source_fk) } - ) - else - # FK is on the source (e.g. reader.book_id -> has_many :readers on Book) - through_pk = through_association.primary_key # '_id' on Book - source_fk = source_association.foreign_key # 'book_id' on Reader - source_association.klass.where( - source_fk => { '$in' => through_crit.pluck(through_pk) } - ) - end + crit = if source_association.stores_foreign_key? + # FK is on the intermediate (e.g. appointment.patient_id -> belongs_to :patient) + target_pk = source_association.primary_key # '_id' on Patient + source_fk = source_association.foreign_key # 'patient_id' on Appointment + source_association.klass.where( + target_pk => { '$in' => through_crit.pluck(source_fk) } + ) + else + # FK is on the source (e.g. reader.book_id -> has_many :readers on Book) + through_pk = through_association.primary_key # '_id' on Book + source_fk = source_association.foreign_key # 'book_id' on Reader + source_association.klass.where( + source_fk => { '$in' => through_crit.pluck(through_pk) } + ) + end + order ? crit.order_by(order) : crit end # The default for validating the association object. diff --git a/spec/mongoid/association/referenced/has_many_through_spec.rb b/spec/mongoid/association/referenced/has_many_through_spec.rb index 6ed1971e8b..b24ca40c30 100644 --- a/spec/mongoid/association/referenced/has_many_through_spec.rb +++ b/spec/mongoid/association/referenced/has_many_through_spec.rb @@ -41,6 +41,33 @@ it 'returns the patient belongs_to on Appointment' do expect(assoc.source_association).to eq(Appointment.relations['patient']) end + + context 'when the source association is has_and_belongs_to_many' do + before(:all) do + Object.const_set(:HabtmTag, Class.new { include Mongoid::Document }) + Object.const_set(:HabtmPost, Class.new do + include Mongoid::Document + + has_and_belongs_to_many :habtm_tags, class_name: 'HabtmTag' + end) + Object.const_set(:HabtmBlog, Class.new do + include Mongoid::Document + + has_many :habtm_posts, class_name: 'HabtmPost', inverse_of: :habtm_blog + has_many :habtm_tags, through: :habtm_posts, class_name: 'HabtmTag' + end) + end + + after(:all) do + %w[HabtmBlog HabtmPost HabtmTag].each { |c| Object.send(:remove_const, c) } + end + + it 'raises InvalidRelationOption' do + blog_assoc = HabtmBlog.relations['habtm_tags'] + expect { blog_assoc.source_association }.to \ + raise_error(Mongoid::Errors::InvalidRelationOption) + end + end end describe '#embedded?' do @@ -55,6 +82,14 @@ Physician.has_many(:foos, through: :appointments, bad_opt: true) end.to raise_error(Mongoid::Errors::InvalidRelationOption) end + + it 'accepts :order' do + expect do + Physician.has_many(:ordered_patients, through: :appointments, + class_name: 'Patient', source: :patient, + order: { _id: 1 }) + end.not_to raise_error + end end describe '#criteria' do @@ -180,4 +215,48 @@ expect(author.hm_readers.to_a).to contain_exactly(r1, r2, r3) end end + + context 'integration — :order option', :integration do + before(:all) do + Object.const_set(:OrdPhysician, Class.new do + include Mongoid::Document + + store_in collection: 'ord_physicians' + has_many :ord_appointments, class_name: 'OrdAppointment', inverse_of: :ord_physician + has_many :ord_patients, through: :ord_appointments, + class_name: 'OrdPatient', source: :ord_patient, + order: { name: 1 } + end) + Object.const_set(:OrdAppointment, Class.new do + include Mongoid::Document + + store_in collection: 'ord_appointments' + belongs_to :ord_physician, class_name: 'OrdPhysician' + belongs_to :ord_patient, class_name: 'OrdPatient' + end) + Object.const_set(:OrdPatient, Class.new do + include Mongoid::Document + + store_in collection: 'ord_patients' + field :name, type: String + end) + end + + after(:all) do + %w[OrdPhysician OrdAppointment OrdPatient].each { |c| Object.send(:remove_const, c) } + end + + before { [ OrdPhysician, OrdAppointment, OrdPatient ].each(&:delete_all) } + + it 'returns patients in the declared order' do + physician = OrdPhysician.create! + charlie = OrdPatient.create!(name: 'Charlie') + alice = OrdPatient.create!(name: 'Alice') + bob = OrdPatient.create!(name: 'Bob') + [ charlie, alice, bob ].each do |p| + OrdAppointment.create!(ord_physician: physician, ord_patient: p) + end + expect(physician.ord_patients.to_a).to eq([ alice, bob, charlie ]) + end + end end From 0ff8a0e6362568666353f3fb7d05f87af44fed4d Mon Sep 17 00:00:00 2001 From: Jamis Buck Date: Tue, 26 May 2026 11:23:51 -0600 Subject: [PATCH 18/19] MONGOID-5057 Address code review feedback on through associations - Remove :scope from ASSOCIATION_OPTIONS on both through types; it was listed as valid but never applied, causing silent no-ops - Add HABTM guard to HasOneThrough#source_association to match the existing guard in HasManyThrough - Fix reverse-FK eager loaders (has_one and has_many) to use source_assoc.primary_key when collecting intermediate PKs and joining back to targets; through_assoc.primary_key is the owner's key and gives wrong results when a custom :primary_key is configured - Fix HasManyThrough#criteria reverse-FK branch with the same correction - Eager loaders now store a Proxy rather than a raw document/array, so the cached type after includes() matches the non-eager getter - HasManyThrough::Proxy accepts a preloaded: kwarg; enumerable methods (each, to_a, first, etc.) use the preloaded array when set, while query methods (where, pluck, etc.) always build a fresh Criteria --- .../referenced/has_many_through.rb | 8 +++--- .../referenced/has_many_through/eager.rb | 18 +++++++----- .../referenced/has_many_through/proxy.rb | 21 +++++++++++--- .../association/referenced/has_one_through.rb | 15 ++++++++-- .../referenced/has_one_through/eager.rb | 28 ++++++++++--------- 5 files changed, 59 insertions(+), 31 deletions(-) diff --git a/lib/mongoid/association/referenced/has_many_through.rb b/lib/mongoid/association/referenced/has_many_through.rb index 29117db3b0..283cab2dd2 100644 --- a/lib/mongoid/association/referenced/has_many_through.rb +++ b/lib/mongoid/association/referenced/has_many_through.rb @@ -14,7 +14,7 @@ class HasManyThrough # common ones. # # @return [ Array ] The extra valid options. - ASSOCIATION_OPTIONS = %i[order source through scope].freeze + ASSOCIATION_OPTIONS = %i[order source through].freeze # The complete list of valid options for this association, including # the shared ones. @@ -126,10 +126,10 @@ def criteria(base) ) else # FK is on the source (e.g. reader.book_id -> has_many :readers on Book) - through_pk = through_association.primary_key # '_id' on Book - source_fk = source_association.foreign_key # 'book_id' on Reader + source_pk = source_association.primary_key # '_id' on intermediate (Book) + source_fk = source_association.foreign_key # 'book_id' on Reader source_association.klass.where( - source_fk => { '$in' => through_crit.pluck(through_pk) } + source_fk => { '$in' => through_crit.pluck(source_pk) } ) end order ? crit.order_by(order) : crit diff --git a/lib/mongoid/association/referenced/has_many_through/eager.rb b/lib/mongoid/association/referenced/has_many_through/eager.rb index 45bf235533..930cad2ed2 100644 --- a/lib/mongoid/association/referenced/has_many_through/eager.rb +++ b/lib/mongoid/association/referenced/has_many_through/eager.rb @@ -26,7 +26,7 @@ def preload .to_a # Step 2: map owner FK values to arrays of target docs - targets_by_owner_fk = build_targets_map(intermediates, through_fk, source_assoc, owner_pk) + targets_by_owner_fk = build_targets_map(intermediates, through_fk, source_assoc) # Step 3: set relation on each owner doc @docs.each do |doc| @@ -37,11 +37,11 @@ def preload # Build a Hash mapping each owner FK value to an array of target docs. # Uses two different strategies depending on where the FK lives. - def build_targets_map(intermediates, through_fk, source_assoc, owner_pk) + def build_targets_map(intermediates, through_fk, source_assoc) if source_assoc.stores_foreign_key? fk_on_intermediate_targets_map(intermediates, through_fk, source_assoc) else - fk_on_source_targets_map(intermediates, through_fk, source_assoc, owner_pk) + fk_on_source_targets_map(intermediates, through_fk, source_assoc) end end @@ -63,8 +63,9 @@ def fk_on_intermediate_targets_map(intermediates, through_fk, source_assoc) end # FK is on the source (e.g. reader.book_id -> has_many :readers on Book). - def fk_on_source_targets_map(intermediates, through_fk, source_assoc, owner_pk) - intermediate_pks = intermediates.filter_map { |i| i.public_send(owner_pk) }.uniq + def fk_on_source_targets_map(intermediates, through_fk, source_assoc) + source_pk = source_assoc.primary_key + intermediate_pks = intermediates.filter_map { |i| i.public_send(source_pk) }.uniq targets = source_assoc.klass.where( source_assoc.foreign_key => { '$in' => intermediate_pks } ).to_a @@ -73,14 +74,17 @@ def fk_on_source_targets_map(intermediates, through_fk, source_assoc, owner_pk) result = Hash.new { |h, k| h[k] = [] } intermediates.each do |i| owner_fk_val = i.public_send(through_fk) - matched = targets_by_source_fk[i.public_send(owner_pk)] || [] + matched = targets_by_source_fk[i.public_send(source_pk)] || [] result[owner_fk_val].concat(matched) end result end def set_relation(doc, element) - doc.set_relation(@association.name, element) unless doc.blank? + return if doc.blank? + + proxy = HasManyThrough::Proxy.new(doc, @association, preloaded: element) + doc.set_relation(@association.name, proxy) end # Required by base class contract. Not called from preload since this diff --git a/lib/mongoid/association/referenced/has_many_through/proxy.rb b/lib/mongoid/association/referenced/has_many_through/proxy.rb index abae4629f1..e3308aacb1 100644 --- a/lib/mongoid/association/referenced/has_many_through/proxy.rb +++ b/lib/mongoid/association/referenced/has_many_through/proxy.rb @@ -23,10 +23,14 @@ def embedded? extend ClassMethods - def_delegators :criteria, + # Enumerable methods use the preloaded array when available; query + # methods always delegate to criteria so callers get a Criteria back. + def_delegators :_source, :each, :to_a, :first, :last, :count, :size, :length, - :where, :pluck, :exists?, :any?, :none?, :empty?, - :limit, :skip, :order_by, :only, :without, + :exists?, :any?, :none?, :empty? + + def_delegators :criteria, + :where, :pluck, :limit, :skip, :order_by, :only, :without, :sum, :avg, :min, :max READONLY_METHODS = %i[ @@ -34,15 +38,24 @@ def embedded? delete delete_one delete_all destroy_all clear nullify ].freeze - def initialize(base, association) + def initialize(base, association, preloaded: nil) @_base = base @_association = association + @preloaded = preloaded end def criteria @criteria ||= @_association.criteria(@_base) end + private + + def _source + @preloaded || criteria + end + + public + READONLY_METHODS.each do |meth| define_method(meth) do |*| raise Mongoid::Errors::ReadonlyAssociation.new(@_base.class, @_association) diff --git a/lib/mongoid/association/referenced/has_one_through.rb b/lib/mongoid/association/referenced/has_one_through.rb index f9a3dc28f1..5f069830c4 100644 --- a/lib/mongoid/association/referenced/has_one_through.rb +++ b/lib/mongoid/association/referenced/has_one_through.rb @@ -14,7 +14,7 @@ class HasOneThrough # common ones. # # @return [ Array ] The extra valid options. - ASSOCIATION_OPTIONS = %i[source through scope].freeze + ASSOCIATION_OPTIONS = %i[source through].freeze # The complete list of valid options for this association, including # the shared ones. @@ -82,12 +82,21 @@ def through_association def source_association @source_association ||= begin source_name = (@options[:source] || name).to_s - through_association.klass.relations[source_name] || + assoc = through_association.klass.relations[source_name] || + raise( + Errors::InvalidRelationOption.new( + @owner_class, name, :source, source_name + ) + ) + if assoc.is_a?(Referenced::HasAndBelongsToMany) raise( Errors::InvalidRelationOption.new( - @owner_class, name, :source, source_name + @owner_class, name, :source, + 'has_and_belongs_to_many is not supported as a :through source' ) ) + end + assoc end end diff --git a/lib/mongoid/association/referenced/has_one_through/eager.rb b/lib/mongoid/association/referenced/has_one_through/eager.rb index c4007557d1..b72e5b81c8 100644 --- a/lib/mongoid/association/referenced/has_one_through/eager.rb +++ b/lib/mongoid/association/referenced/has_one_through/eager.rb @@ -14,7 +14,6 @@ def preload through_assoc = @association.through_association source_assoc = @association.source_association - # Step 1: load all intermediate records keyed by their FK to owner owner_pk = through_assoc.primary_key through_fk = through_assoc.foreign_key @@ -22,32 +21,35 @@ def preload return if owner_ids.empty? intermediates = through_assoc.klass.where(through_fk => { '$in' => owner_ids }).to_a + intermediate_to_target = build_intermediate_to_target(intermediates, through_fk, source_assoc) - # Step 2: load all source records + @docs.each do |doc| + owner_key_val = doc.public_send(owner_pk) + target = intermediate_to_target[owner_key_val] + proxy = target ? HasOneThrough::Proxy.new(doc, target, @association) : nil + doc.set_relation(@association.name, proxy) unless doc.blank? + end + end + + def build_intermediate_to_target(intermediates, through_fk, source_assoc) if source_assoc.stores_foreign_key? # FK is on the intermediate (e.g. belongs_to :store => intermediate.store_id) source_fk_values = intermediates.filter_map { |i| i.public_send(source_assoc.foreign_key) }.uniq targets = source_assoc.klass.where(source_assoc.primary_key => { '$in' => source_fk_values }).to_a targets_by_key = targets.index_by { |t| t.public_send(source_assoc.primary_key) } - intermediate_to_target = intermediates.to_h do |i| + intermediates.to_h do |i| [ i.public_send(through_fk), targets_by_key[i.public_send(source_assoc.foreign_key)] ] end else # FK is on the source (e.g. has_one :store => store.franchise_id) - intermediate_pks = intermediates.filter_map { |i| i.public_send(through_assoc.primary_key) }.uniq + source_pk = source_assoc.primary_key + intermediate_pks = intermediates.filter_map { |i| i.public_send(source_pk) }.uniq targets = source_assoc.klass.where(source_assoc.foreign_key => { '$in' => intermediate_pks }).to_a targets_by_fk = targets.index_by { |t| t.public_send(source_assoc.foreign_key) } - intermediate_by_owner_fk = intermediates.index_by { |i| i.public_send(through_fk) } - intermediate_to_target = intermediate_by_owner_fk.transform_values do |i| - targets_by_fk[i.public_send(through_assoc.primary_key)] + intermediates.index_by { |i| i.public_send(through_fk) }.transform_values do |i| + targets_by_fk[i.public_send(source_pk)] end end - - # Step 3: set relation on each owner doc - @docs.each do |doc| - owner_key_val = doc.public_send(owner_pk) - set_relation(doc, intermediate_to_target[owner_key_val]) - end end # Required by the base class contract. Not called by this preloader From 178cbc427da4fe325498911350d921f27df18289 Mon Sep 17 00:00:00 2001 From: Jamis Buck Date: Tue, 26 May 2026 13:09:05 -0600 Subject: [PATCH 19/19] MONGOID-5057 Replace em-dashes in context descriptions with ASCII hyphens RSpec embeds context description strings in the JSON output. Em-dash characters (U+2014, encoded as \xE2\x80\x94 in UTF-8) cause Encoding::InvalidByteSequenceError when CI reads tmp/rspec.json under LANG=C, where Encoding.default_external is US-ASCII. --- .../mongoid/association/referenced/has_many_through_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/mongoid/association/referenced/has_many_through_spec.rb b/spec/mongoid/association/referenced/has_many_through_spec.rb index b24ca40c30..e6cc312ce2 100644 --- a/spec/mongoid/association/referenced/has_many_through_spec.rb +++ b/spec/mongoid/association/referenced/has_many_through_spec.rb @@ -103,7 +103,7 @@ end end - context 'integration — belongs_to source (join table)', :integration do + context 'integration - belongs_to source (join table)', :integration do before(:all) do Object.const_set(:JtPhysician, Class.new do include Mongoid::Document @@ -173,7 +173,7 @@ end end - context 'integration — has_many source', :integration do + context 'integration - has_many source', :integration do before(:all) do Object.const_set(:HmAuthor, Class.new do include Mongoid::Document @@ -216,7 +216,7 @@ end end - context 'integration — :order option', :integration do + context 'integration - :order option', :integration do before(:all) do Object.const_set(:OrdPhysician, Class.new do include Mongoid::Document