Pre-save middleware fires on subdocs prior to their parent

Automattic · Jul 31, 2015 · f691753 · f691753
1 parent 4953963
commit f691753
Show file tree

Hide file tree

Showing 5 changed files with 136 additions and 54 deletions.
diff --git a/lib/model.js b/lib/model.js
@@ -222,23 +222,8 @@ Model.prototype.save = function (options, fn) {
   }
 
   var _this = this;
-  var promise = new Promise.ES6(function(resolve, reject) {
-    async.each(
-      _this.$__getAllSubdocs(),
-      function(subdoc, cb) {
-        subdoc.save(cb);
-      },
-      function(error) {
-        if (error) {
-          reject(error);
-          return;
-        }
-        resolve();
-      });
-  });
 
-  return promise.
-    then(this.$__handleSave.bind(this, options)).
+  return _this.$__handleSave(options).
     then(
       function(result) {
         _this.$__reset();

diff --git a/lib/schema.js b/lib/schema.js
@@ -2,12 +2,15 @@
  * Module dependencies.
  */
 
-var readPref = require('./drivers').ReadPreference;
-var EventEmitter = require('events').EventEmitter;
-var VirtualType = require('./virtualtype');
-var utils = require('./utils');
-var MongooseTypes;
-var Kareem = require('kareem');
+var readPref = require('./drivers').ReadPreference
+  , EventEmitter = require('events').EventEmitter
+  , VirtualType = require('./virtualtype')
+  , utils = require('./utils')
+  , MongooseTypes
+  , Kareem = require('kareem')
+  , async = require('async')
+  , PromiseProvider = require('./promise_provider');
+
 
 var IS_QUERY_HOOK = {
   count: true,
@@ -106,7 +109,7 @@ function Schema (obj, options) {
 
   for (var i = 0; i < this._defaultMiddleware.length; ++i) {
     var m = this._defaultMiddleware[i];
-    this[m.kind](m.hook, m.fn);
+    this[m.kind](m.hook, !!m.isAsync, m.fn);
   }
 
   // adds updatedAt and createdAt timestamps to documents if enabled
@@ -173,25 +176,65 @@ Object.defineProperty(Schema.prototype, '_defaultMiddleware', {
   configurable: false,
   enumerable: false,
   writable: false,
-  value: [
-    {
-      kind: 'pre',
-      hook: 'save',
-      fn: function(next) {
-        // Nested docs have their own presave
-        if (this.ownerDocument) {
-          return next();
-        }
+  value: [{
+    kind: 'pre',
+    hook: 'save',
+    fn: function(next) {
+      // Nested docs have their own presave
+      if (this.ownerDocument) {
+        return next();
+      }
 
-        // Validate
-        if (this.schema.options.validateBeforeSave) {
-          this.validate().then(next, next);
-        } else {
-          next();
-        }
+      // Validate
+      if (this.schema.options.validateBeforeSave) {
+        this.validate().then(next, next);
+      } else {
+        next();
       }
     }
-  ]
+  }, {
+    kind: 'pre',
+    hook: 'save',
+    isAsync: true,
+    fn: function(next, done) {
+      var Promise = PromiseProvider.get(),
+          subdocs = this.$__getAllSubdocs(),
+          _this = this;
+
+      // Calling `save` on a nested subdoc calls `save` with
+      // the scope of its direct parent. That means in the
+      // case of a nested subdoc, `subdocs` here will include
+      // this subdoc itself leading to an infinite loop.
+      subdocs = subdocs.filter(function(subdoc) {
+        return !subdoc.$__preSavingFromParent;
+      });
+
+      if (!subdocs.length) {
+        done();
+        next();
+        return;
+      }
+
+      new Promise.ES6(function(resolve, reject) {
+        async.each(subdocs, function(subdoc, cb) {
+          subdoc.$__preSavingFromParent = true;
+          subdoc.save(function(err) {
+            delete subdoc.$__preSavingFromParent;
+            cb(err);
+          });
+        }, function(error) {
+          if (error) {
+            reject(error);
+            return;
+          }
+          resolve();
+        });
+      }).then(function() {
+        next();
+        done();
+      }, done);
+    }
+  }]
 });
 
 /**

diff --git a/test/document.hooks.test.js b/test/document.hooks.test.js
@@ -338,10 +338,16 @@ describe('document: hooks:', function () {
     var S = db.model('docArrayWithHookedSave', schema);
     var s = new S({ name: 'hi', e: [{}] });
     s.save(function (err) {
-      assert.ok(err);
-      assert.ok(err.errors['e.0.text']);
-      assert.equal(false, presave);
-      db.close(done);
+      db.close();
+
+      try {
+        assert.ok(err);
+        assert.ok(err.errors['e.0.text']);
+        assert.equal(false, presave);
+        done();
+      } catch (e) {
+        done(e);
+      }
     });
   });
 
@@ -500,7 +506,7 @@ describe('document: hooks:', function () {
       setTimeout(function () {
         count++;
         next();
-        if (count == 3) {
+        if (count === 3) {
           done(new Error("gaga"));
         } else {
           done();
@@ -530,9 +536,15 @@ describe('document: hooks:', function () {
     });
 
     m.save(function (err) {
-      assert.equal(err.message, "gaga");
-      assert.equal(count, 4);
-      db.close(done);
+      db.close();
+
+      try {
+        assert.equal(err.message, "gaga");
+        assert.equal(count, 4);
+        done();
+      } catch (e) {
+        done(e);
+      }
     });
   });
 
@@ -639,4 +651,45 @@ describe('document: hooks:', function () {
       });
     });
   });
+
+  it('pre-save hooks fire on subdocs before their parent doc', function(done) {
+    var childSchema = Schema({ name: String, count: Number });
+
+    childSchema.pre('save', function(next) {
+      ++this.count;
+      next();
+      // On subdocuments, you have to return `this`
+      return this;
+    });
+
+    var parentSchema = Schema({
+      cumulativeCount: Number,
+      children: [childSchema]
+    });
+
+    parentSchema.pre('save', function(next) {
+      this.cumulativeCount = this.children.reduce(function (seed, child) {
+        return seed += child.count;
+      }, 0)
+      next();
+    });
+
+    var db = start(),
+        Parent = db.model('ParentWithChildren', parentSchema),
+        doc = new Parent({ children: [{ count: 0, name: 'a' }, { count: 1, name: 'b' }] });
+
+    doc.save(function(err, doc){
+      db.close();
+
+      try {
+        assert.strictEqual(doc.children[0].count, 1);
+        assert.strictEqual(doc.children[1].count, 2);
+        assert.strictEqual(doc.cumulativeCount, 3);
+      } catch (e) {
+        return done(e);
+      }
+
+      done();
+    });
+  });
 });
diff --git a/test/model.discriminator.test.js b/test/model.discriminator.test.js
@@ -307,14 +307,15 @@ describe('model', function() {
       });
 
       it('merges callQueue with base queue defined before discriminator types callQueue', function(done) {
-        assert.equal(Employee.schema.callQueue.length, 3);
+        assert.equal(Employee.schema.callQueue.length, 4);
         // PersonSchema.post('save')
         assert.strictEqual(Employee.schema.callQueue[0], Person.schema.callQueue[0]);
 
         // EmployeeSchema.pre('save')
-        assert.strictEqual(Employee.schema.callQueue[2][0], 'pre');
-        assert.strictEqual(Employee.schema.callQueue[2][1]['0'], 'save');
-        assert.strictEqual(Employee.schema.callQueue[2][1]['1'], employeeSchemaPreSaveFn);
+        var queueIndex = Employee.schema.callQueue.length - 1;
+        assert.strictEqual(Employee.schema.callQueue[queueIndex][0], 'pre');
+        assert.strictEqual(Employee.schema.callQueue[queueIndex][1]['0'], 'save');
+        assert.strictEqual(Employee.schema.callQueue[queueIndex][1]['1'], employeeSchemaPreSaveFn);
         done();
       });
 

diff --git a/test/schema.test.js b/test/schema.test.js
@@ -728,13 +728,13 @@ describe('schema', function(){
       var Tobi = new Schema();
 
       Tobi.pre('save', function(){});
-      assert.equal(2, Tobi.callQueue.length);
+      assert.equal(Tobi.callQueue.length, 3);
 
       Tobi.post('save', function(){});
-      assert.equal(3, Tobi.callQueue.length);
+      assert.equal(Tobi.callQueue.length, 4);
 
       Tobi.pre('save', function(){});
-      assert.equal(4, Tobi.callQueue.length);
+      assert.equal(Tobi.callQueue.length, 5);
       done();
     });
   });