From 3ccd9ac1fc38a5c1c48d8e7adafdf72513247f2a Mon Sep 17 00:00:00 2001 From: Jamis Buck Date: Fri, 18 Jul 2025 09:59:48 -0600 Subject: [PATCH] MONGOID-4889 Optimize batch assignment of embedded documents (#6008) * MONGOID-4889 Optimize batch assignment of embedded documents * rubocop appeasement * simplify some refactoring artifacts * improve the name of the extracted method --- .rubocop.yml | 2 +- lib/mongoid/association/embedded/batchable.rb | 21 ++++--- .../association/embedded/embeds_many/proxy.rb | 61 +++++++++++++++++++ .../association/referenced/has_many/proxy.rb | 4 -- .../associations/embeds_many_spec.rb | 41 +++++++++++++ 5 files changed, 114 insertions(+), 15 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index f68dd0d561..a4343f419e 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -64,7 +64,7 @@ Layout/SpaceInsidePercentLiteralDelimiters: Enabled: false Metrics/ClassLength: - Max: 200 + Enabled: false Metrics/ModuleLength: Enabled: false diff --git a/lib/mongoid/association/embedded/batchable.rb b/lib/mongoid/association/embedded/batchable.rb index ef95358972..a3fe04e3b3 100644 --- a/lib/mongoid/association/embedded/batchable.rb +++ b/lib/mongoid/association/embedded/batchable.rb @@ -313,18 +313,19 @@ def selector # # @return [ Array ] The documents as an array of hashes. def pre_process_batch_insert(docs) - docs.map do |doc| - next unless doc - append(doc) - if persistable? && !_assigning? - self.path = doc.atomic_path unless path - if doc.valid?(:create) - doc.run_before_callbacks(:save, :create) - else - self.inserts_valid = false + [].tap do |results| + append_many(docs) do |doc| + if persistable? && !_assigning? + self.path = doc.atomic_path unless path + if doc.valid?(:create) + doc.run_before_callbacks(:save, :create) + else + self.inserts_valid = false + end end + + results << doc.send(:as_attributes) end - doc.send(:as_attributes) end end diff --git a/lib/mongoid/association/embedded/embeds_many/proxy.rb b/lib/mongoid/association/embedded/embeds_many/proxy.rb index a2d14a035e..aab5881250 100644 --- a/lib/mongoid/association/embedded/embeds_many/proxy.rb +++ b/lib/mongoid/association/embedded/embeds_many/proxy.rb @@ -443,6 +443,67 @@ def append(document) execute_callback :after_add, document end + # Returns a unique id for the document, which is either + # its _id or its object_id. + def id_of(doc) + doc._id || doc.object_id + end + + # Optimized version of #append that handles multiple documents + # in a more efficient way. + # + # @param [ Array ] documents The documents to append. + # + # @return [ EmbedsMany::Proxy ] This proxy instance. + def append_many(documents, &block) + unique_set = process_incoming_docs(documents, &block) + + _unscoped.concat(unique_set) + _target.push(*scope(unique_set)) + update_attributes_hash + + unique_set.each { |doc| execute_callback :after_add, doc } + + self + end + + # Processes the list of documents, building a list of those + # that are not already in the association, and preparing + # each unique document to be integrated into the association. + # + # The :before_add callback is executed for each unique document + # as part of this step. + # + # @param [ Array ] documents The incoming documents to + # process. + # + # @yield [ Document ] Optional block to call for each unique + # document. + # + # @return [ Array ] The list of unique documents that + # do not yet exist in the association. + def process_incoming_docs(documents, &block) + visited_docs = Set.new(_target.map { |doc| id_of(doc) }) + next_index = _unscoped.size + + documents.select do |doc| + next unless doc + + id = id_of(doc) + next if visited_docs.include?(id) + + execute_callback :before_add, doc + + visited_docs.add(id) + integrate(doc) + + doc._index = next_index + next_index += 1 + + block&.call(doc) || true + end + end + # Instantiate the binding associated with this association. # # @example Create the binding. diff --git a/lib/mongoid/association/referenced/has_many/proxy.rb b/lib/mongoid/association/referenced/has_many/proxy.rb index ef0bd2bd08..75453a160a 100644 --- a/lib/mongoid/association/referenced/has_many/proxy.rb +++ b/lib/mongoid/association/referenced/has_many/proxy.rb @@ -1,8 +1,5 @@ # frozen_string_literal: true -# TODO: consider refactoring this Proxy class, to satisfy the following -# cops... -# rubocop:disable Metrics/ClassLength module Mongoid module Association module Referenced @@ -588,4 +585,3 @@ def save_or_delay(doc, docs, inserts) end end end -# rubocop:enable Metrics/ClassLength diff --git a/spec/integration/associations/embeds_many_spec.rb b/spec/integration/associations/embeds_many_spec.rb index 45e2a7fe39..7a630fac0b 100644 --- a/spec/integration/associations/embeds_many_spec.rb +++ b/spec/integration/associations/embeds_many_spec.rb @@ -201,6 +201,47 @@ include_examples 'persists correctly' end end + + context 'including duplicates in the assignment' do + let(:canvas) do + Canvas.create!(shapes: [Shape.new]) + end + + shared_examples 'persists correctly' do + it 'persists correctly' do + canvas.shapes.length.should eq 2 + _canvas = Canvas.find(canvas.id) + _canvas.shapes.length.should eq 2 + end + end + + context 'via assignment operator' do + before do + canvas.shapes = [ canvas.shapes.first, Shape.new, canvas.shapes.first ] + canvas.save! + end + + include_examples 'persists correctly' + end + + context 'via attributes=' do + before do + canvas.attributes = { shapes: [ canvas.shapes.first, Shape.new, canvas.shapes.first ] } + canvas.save! + end + + include_examples 'persists correctly' + end + + context 'via assign_attributes' do + before do + canvas.assign_attributes(shapes: [ canvas.shapes.first, Shape.new, canvas.shapes.first ]) + canvas.save! + end + + include_examples 'persists correctly' + end + end end context 'when an anonymous class defines an embeds_many association' do