Move embedded many concats out into it's own method.

- This addresses the after_* callbacks firing on individual pushes
  before the documents actually get saved to the database.
- Batch pushing will still contain this issue until active support
  provides a means to execute the after_* callbacks individually.
- Fixes #1370.

CHANGELOG.md

@@ -118,6 +118,9 @@ For instructions on upgrading to newer versions, visit
 * \#1381, \#1371 The identity map now functions properly with inherited
   documents. (Paul Canavese)
 
+* \#1370 Split concat on embedded arrays into its own method to handle the
+  batch processing due to after callback run execution issues.
+
 * \#1366 Array and hash values now get deep copied for dirty tracking.
 
 * \#1359 Provide ability to not have default scope applied to all named

lib/mongoid/relations/embedded/many.rb

@@ -17,21 +17,42 @@ class Many < Relations::Many
         # @example Push a document.
         #   person.addresses.push(address)
         #
+        # @param [ Document, Array<Document> ] *args Any number of documents.
+        def <<(*args)
+          docs = args.flatten
+          return concat(docs) if docs.size > 1
+          if doc = docs.first
+            append(doc)
+            doc.save if persistable? && !_assigning?
+          end
+        end
+        alias :push :<<
+
+        # Appends an array of documents to the relation. Performs a batch
+        # insert of the documents instead of persisting one at a time.
+        #
+        # @note When performing batch inserts the *after* callbacks will get
+        #   executed before the documents have actually been persisted to the
+        #   database due to an issue with Active Support's callback system - we
+        #   cannot explicitly fire the after callbacks by themselves.
+        #
         # @example Concat with other documents.
         #   person.addresses.concat([ address_one, address_two ])
         #
-        # @param [ Document, Array<Document> ] *args Any number of documents.
-        def <<(*args)
+        # @param [ Array<Document> ] documents The docs to add.
+        #
+        # @return [ Array<Document> ] The documents.
+        #
+        # @since 2.4.0
+        def concat(documents)
           atomically(:$pushAll) do
-            args.flatten.each do |doc|
+            documents.each do |doc|
               next unless doc
               append(doc)
-              doc.save if persistable? && !_assigning?
+              doc.save if persistable?
             end
           end
         end
-        alias :concat :<<
-        alias :push :<<
 
         # Builds a new document in the relation and appends it to the target.
         # Takes an optional type if you want to specify a subclass.

spec/functional/mongoid/relations/embedded/many_spec.rb

@@ -7,7 +7,7 @@
       Quiz, Role, Patient, Product, Purchase ].map(&:delete_all)
   end
 
-  [ :<<, :push, :concat ].each do |method|
+  [ :<<, :push ].each do |method|
 
     describe "##{method}" do
 
@@ -887,6 +887,163 @@
     end
   end
 
+  describe "#concat" do
+
+    context "when the parent is a new record" do
+
+      let(:person) do
+        Person.new
+      end
+
+      let(:address) do
+        Address.new
+      end
+
+      before do
+        person.addresses.concat([ address ])
+      end
+
+      it "appends to the target" do
+        person.addresses.should == [ address ]
+      end
+
+      it "sets the base on the inverse relation" do
+        address.addressable.should == person
+      end
+
+      it "sets the same instance on the inverse relation" do
+        address.addressable.should eql(person)
+      end
+
+      it "does not save the new document" do
+        address.should_not be_persisted
+      end
+
+      it "sets the parent on the child" do
+        address._parent.should == person
+      end
+
+      it "sets the metadata on the child" do
+        address.metadata.should_not be_nil
+      end
+
+      it "sets the index on the child" do
+        address._index.should == 0
+      end
+    end
+
+    context "when the parent is not a new record" do
+
+      let(:person) do
+        Person.create(:ssn => "234-44-4432")
+      end
+
+      let(:address) do
+        Address.new
+      end
+
+      before do
+        person.addresses.concat([ address ])
+      end
+
+      it "saves the new document" do
+        address.should be_persisted
+      end
+    end
+
+    context "when appending more than one document at once" do
+
+      let(:person) do
+        Person.create(:ssn => "234-44-4432")
+      end
+
+      let(:address_one) do
+        Address.new
+      end
+
+      let(:address_two) do
+        Address.new
+      end
+
+      before do
+        person.addresses.concat([ address_one, address_two ])
+      end
+
+      it "saves the first document" do
+        address_one.should be_persisted
+      end
+
+      it "saves the second document" do
+        address_two.should be_persisted
+      end
+    end
+
+    context "when the parent and child have a cyclic relation" do
+
+      context "when the parent is a new record" do
+
+        let(:parent_role) do
+          Role.new
+        end
+
+        let(:child_role) do
+          Role.new
+        end
+
+        before do
+          parent_role.child_roles.concat([ child_role ])
+        end
+
+        it "appends to the target" do
+          parent_role.child_roles.should == [ child_role ]
+        end
+
+        it "sets the base on the inverse relation" do
+          child_role.parent_role.should == parent_role
+        end
+
+        it "sets the same instance on the inverse relation" do
+          child_role.parent_role.should eql(parent_role)
+        end
+
+        it "does not save the new document" do
+          child_role.should_not be_persisted
+        end
+
+        it "sets the parent on the child" do
+          child_role._parent.should == parent_role
+        end
+
+        it "sets the metadata on the child" do
+          child_role.metadata.should_not be_nil
+        end
+
+        it "sets the index on the child" do
+          child_role._index.should == 0
+        end
+      end
+
+      context "when the parent is not a new record" do
+
+        let(:parent_role) do
+          Role.create(:name => "CEO")
+        end
+
+        let(:child_role) do
+          Role.new(:name => "COO")
+        end
+
+        before do
+          parent_role.child_roles.concat([ child_role ])
+        end
+
+        it "saves the new document" do
+          child_role.should be_persisted
+        end
+      end
+    end
+  end
+
   describe "#count" do
 
     let(:person) do

spec/unit/mongoid/relations/embedded/many_spec.rb

@@ -34,7 +34,7 @@
     Person.relations["addresses"]
   end
 
-  [ :<<, :push, :concat ].each do |method|
+  [ :<<, :push ].each do |method|
 
     describe "##{method}" do
 
@@ -170,6 +170,52 @@
     end
   end
 
+  describe "#concat" do
+
+    let(:relation) do
+      described_class.new(base, target, metadata)
+    end
+
+    let(:document) do
+      Address.new(:street => "Bond St")
+    end
+
+    before do
+      relation.loaded = true
+      binding_klass.expects(:new).returns(binding)
+      binding.expects(:bind_one)
+    end
+
+    context "when the base is persisted" do
+
+      before do
+        base.expects(:persisted?).returns(true)
+        document.expects(:save).returns(true)
+        relation.concat([ document ])
+      end
+
+      it "appends the document to the target" do
+        relation.target.size.should == 2
+      end
+
+      it "adds the metadata to the target" do
+        document.metadata.should == metadata
+      end
+
+      it "indexes the target" do
+        document._index.should == 1
+      end
+    end
+
+    context "when the base is not persisted" do
+
+      it "does not save the target" do
+        document.expects(:save).never
+        relation.concat([ document ])
+      end
+    end
+  end
+
   describe "#create" do
 
     let(:relation) do