Fixed model events and transactions (#9524)

no issue

- if multiple queries run in a transaction, the model events are triggered before the txn finished
- if the txn rolls back, the events are anyway emitted
- the events are triggered too early
- solution:
  - `emitChange` needs to detect that a transaction is happening
  - it listens on a txn event to determine if events should be triggered

core/server/models/accesstoken.js

@@ -1,20 +1,21 @@
-var ghostBookshelf = require('./base'),
+'use strict';
-    Basetoken = require('./base/token'),
-    common = require('../lib/common'),
 
-    Accesstoken,
+const ghostBookshelf = require('./base'),
+    Basetoken = require('./base/token');
+
+let Accesstoken,
     Accesstokens;
 
 Accesstoken = Basetoken.extend({
     tableName: 'accesstokens',
 
-    emitChange: function emitChange(event) {
+    emitChange: function emitChange(event, options) {
-        // Event named 'token' as access and refresh token will be merged in future, see #6626
+        const eventToTrigger = 'token' + '.' + event;
-        common.events.emit('token' + '.' + event, this);
+        ghostBookshelf.Model.prototype.emitChange.bind(this)(this, eventToTrigger, options);
     },
 
-    onCreated: function onCreated(model) {
+    onCreated: function onCreated(model, attrs, options) {
-        model.emitChange('added');
+        model.emitChange('added', options);
     }
 });
 

core/server/models/base/index.js

@@ -32,6 +32,9 @@ ghostBookshelf = bookshelf(db.knex);
 // Load the Bookshelf registry plugin, which helps us avoid circular dependencies
 ghostBookshelf.plugin('registry');
 
+// Add committed/rollback events.
+ghostBookshelf.plugin(plugins.transactionEvents);
+
 // Load the Ghost filter plugin, which handles applying a 'filter' to findPage requests
 ghostBookshelf.plugin(plugins.filter);
 
@@ -98,6 +101,32 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({
         return [];
     },
 
+    emitChange: function (model, event, options) {
+        if (!options.transacting) {
+            return common.events.emit(event, model, options);
+        }
+
+        if (!model.ghostEvents) {
+            model.ghostEvents = [];
+
+            if (options.importing) {
+                options.transacting.setMaxListeners(0);
+            }
+
+            options.transacting.once('committed', (committed) => {
+                if (!committed) {
+                    return;
+                }
+
+                _.each(this.ghostEvents, (ghostEvent) => {
+                    common.events.emit(ghostEvent, model, _.omit(options, 'transacting'));
+                });
+            });
+        }
+
+        model.ghostEvents.push(event);
+    },
+
     // Bookshelf `initialize` - declare a constructor-like method for model creation
     initialize: function initialize() {
         var self = this;

core/server/models/base/listeners.js

@@ -31,9 +31,9 @@ common.events.on('user.deactivated', function (userModel, options) {
         return;
     }
 
-    models.Accesstoken.destroyByUser(_.omit(options, 'transacting'))
+    models.Accesstoken.destroyByUser(options)
         .then(function () {
-            return models.Refreshtoken.destroyByUser(_.omit(options, 'transacting'));
+            return models.Refreshtoken.destroyByUser(options);
         })
         .catch(function (err) {
             common.logging.error(new common.errors.GhostError({

core/server/models/plugins/index.js

@@ -2,5 +2,6 @@ module.exports = {
     filter: require('./filter'),
     includeCount: require('./include-count'),
     pagination: require('./pagination'),
-    collision: require('./collision')
+    collision: require('./collision'),
+    transactionEvents: require('./transaction-events')
 };

core/server/models/plugins/transaction-events.js

@@ -0,0 +1,28 @@
+'use strict';
+
+/**
+ * This is a feature request in knex for 1.0.
+ * https://github.com/tgriesser/knex/issues/1641
+ */
+module.exports = function (bookshelf) {
+    const orig1 = bookshelf.transaction;
+
+    bookshelf.transaction = function (cb) {
+        return orig1.bind(bookshelf)(function (t) {
+            const orig2 = t.commit;
+            const orig3 = t.rollback;
+
+            t.commit = function () {
+                t.emit('committed', true);
+                return orig2.apply(t, arguments);
+            };
+
+            t.rollback = function () {
+                t.emit('committed', false);
+                return orig3.apply(t, arguments);
+            };
+
+            return cb(t);
+        });
+    };
+};

core/server/models/post.js

@@ -61,15 +61,19 @@ Post = ghostBookshelf.Model.extend({
     },
 
     emitChange: function emitChange(event, options) {
-        options = options || {};
+        let eventToTrigger;
 
         var resourceType = this.get('page') ? 'page' : 'post';
 
-        if (options.usePreviousResourceType) {
+        if (options.useUpdatedAttribute) {
             resourceType = this.updated('page') ? 'page' : 'post';
+        } else if (options.usePreviousAttribute) {
+            resourceType = this.previous('page') ? 'page' : 'post';
         }
 
-        common.events.emit(resourceType + '.' + event, this, options);
+        eventToTrigger = resourceType + '.' + event;
+
+        ghostBookshelf.Model.prototype.emitChange.bind(this)(this, eventToTrigger, options);
     },
 
     /**
@@ -88,14 +92,14 @@ Post = ghostBookshelf.Model.extend({
 
         var status = model.get('status');
 
-        model.emitChange('added');
+        model.emitChange('added', options);
 
         if (['published', 'scheduled'].indexOf(status) !== -1) {
-            model.emitChange(status, {importing: options.importing});
+            model.emitChange(status, options);
         }
     },
 
-    onUpdated: function onUpdated(model) {
+    onUpdated: function onUpdated(model, attrs, options) {
         model.statusChanging = model.get('status') !== model.updated('status');
         model.isPublished = model.get('status') === 'published';
         model.isScheduled = model.get('status') === 'scheduled';
@@ -108,65 +112,65 @@ Post = ghostBookshelf.Model.extend({
         // Handle added and deleted for post -> page or page -> post
         if (model.resourceTypeChanging) {
             if (model.wasPublished) {
-                model.emitChange('unpublished', {usePreviousResourceType: true});
+                model.emitChange('unpublished', Object.assign({useUpdatedAttribute: true}, options));
             }
 
             if (model.wasScheduled) {
-                model.emitChange('unscheduled', {usePreviousResourceType: true});
+                model.emitChange('unscheduled', Object.assign({useUpdatedAttribute: true}, options));
             }
 
-            model.emitChange('deleted', {usePreviousResourceType: true});
+            model.emitChange('deleted', Object.assign({useUpdatedAttribute: true}, options));
-            model.emitChange('added');
+            model.emitChange('added', options);
 
             if (model.isPublished) {
-                model.emitChange('published');
+                model.emitChange('published', options);
             }
 
             if (model.isScheduled) {
-                model.emitChange('scheduled');
+                model.emitChange('scheduled', options);
             }
         } else {
             if (model.statusChanging) {
                 // CASE: was published before and is now e.q. draft or scheduled
                 if (model.wasPublished) {
-                    model.emitChange('unpublished');
+                    model.emitChange('unpublished', options);
                 }
 
                 // CASE: was draft or scheduled before and is now e.q. published
                 if (model.isPublished) {
-                    model.emitChange('published');
+                    model.emitChange('published', options);
                 }
 
                 // CASE: was draft or published before and is now e.q. scheduled
                 if (model.isScheduled) {
-                    model.emitChange('scheduled');
+                    model.emitChange('scheduled', options);
                 }
 
                 // CASE: from scheduled to something
                 if (model.wasScheduled && !model.isScheduled && !model.isPublished) {
-                    model.emitChange('unscheduled');
+                    model.emitChange('unscheduled', options);
                 }
             } else {
                 if (model.isPublished) {
-                    model.emitChange('published.edited');
+                    model.emitChange('published.edited', options);
                 }
 
                 if (model.needsReschedule) {
-                    model.emitChange('rescheduled');
+                    model.emitChange('rescheduled', options);
                 }
             }
 
             // Fire edited if this wasn't a change between resourceType
-            model.emitChange('edited');
+            model.emitChange('edited', options);
         }
     },
 
-    onDestroying: function onDestroying(model) {
+    onDestroyed: function onDestroyed(model, options) {
         if (model.previous('status') === 'published') {
-            model.emitChange('unpublished');
+            model.emitChange('unpublished', Object.assign({usePreviousAttribute: true}, options));
         }
 
-        model.emitChange('deleted');
+        model.emitChange('deleted', Object.assign({usePreviousAttribute: true}, options));
     },
 
     onSaving: function onSaving(model, attr, options) {

core/server/models/settings.js

@@ -1,15 +1,15 @@
-var Settings,
+'use strict';
-    Promise = require('bluebird'),
+
+const Promise = require('bluebird'),
     _ = require('lodash'),
     uuid = require('uuid'),
     crypto = require('crypto'),
     ghostBookshelf = require('./base'),
     common = require('../lib/common'),
     validation = require('../data/validation'),
+    internalContext = {context: {internal: true}};
 
-    internalContext = {context: {internal: true}},
+let Settings, defaultSettings;
-
-    defaultSettings;
 
 // For neatness, the defaults file is split into categories.
 // It's much easier for us to work with it as a single level
@@ -58,21 +58,22 @@ Settings = ghostBookshelf.Model.extend({
     },
 
     emitChange: function emitChange(event, options) {
-        common.events.emit('settings' + '.' + event, this, options);
+        const eventToTrigger = 'settings' + '.' + event;
+        ghostBookshelf.Model.prototype.emitChange.bind(this)(this, eventToTrigger, options);
     },
 
-    onDestroyed: function onDestroyed(model, response, options) {
+    onDestroyed: function onDestroyed(model, options) {
-        model.emitChange('deleted');
+        model.emitChange('deleted', options);
-        model.emitChange(model.attributes.key + '.' + 'deleted', options);
+        model.emitChange(model._previousAttributes.key + '.' + 'deleted', options);
     },
 
     onCreated: function onCreated(model, response, options) {
-        model.emitChange('added');
+        model.emitChange('added', options);
         model.emitChange(model.attributes.key + '.' + 'added', options);
     },
 
     onUpdated: function onUpdated(model, response, options) {
-        model.emitChange('edited');
+        model.emitChange('edited', options);
         model.emitChange(model.attributes.key + '.' + 'edited', options);
     },
 

core/server/models/subscriber.js

@@ -1,16 +1,18 @@
-var Promise = require('bluebird'),
+'use strict';
+
+const Promise = require('bluebird'),
     ghostBookshelf = require('./base'),
-    common = require('../lib/common'),
+    common = require('../lib/common');
-    Subscriber,
+
+let Subscriber,
     Subscribers;
 
 Subscriber = ghostBookshelf.Model.extend({
     tableName: 'subscribers',
 
     emitChange: function emitChange(event, options) {
-        options = options || {};
+        const eventToTrigger = 'subscriber' + '.' + event;
-
+        ghostBookshelf.Model.prototype.emitChange.bind(this)(this, eventToTrigger, options);
-        common.events.emit('subscriber' + '.' + event, this, options);
     },
 
     defaults: function defaults() {
@@ -27,7 +29,7 @@ Subscriber = ghostBookshelf.Model.extend({
         model.emitChange('edited', options);
     },
 
-    onDestroyed: function onDestroyed(model, response, options) {
+    onDestroyed: function onDestroyed(model, options) {
         model.emitChange('deleted', options);
     }
 }, {

core/server/models/tag.js

@@ -1,7 +1,7 @@
-var ghostBookshelf = require('./base'),
+'use strict';
-    common = require('../lib/common'),
+
-    Tag,
+const ghostBookshelf = require('./base');
-    Tags;
+let Tag, Tags;
 
 Tag = ghostBookshelf.Model.extend({
 
@@ -13,20 +13,21 @@ Tag = ghostBookshelf.Model.extend({
         };
     },
 
-    emitChange: function emitChange(event) {
+    emitChange: function emitChange(event, options) {
-        common.events.emit('tag' + '.' + event, this);
+        const eventToTrigger = 'tag' + '.' + event;
+        ghostBookshelf.Model.prototype.emitChange.bind(this)(this, eventToTrigger, options);
     },
 
-    onCreated: function onCreated(model) {
+    onCreated: function onCreated(model, attrs, options) {
-        model.emitChange('added');
+        model.emitChange('added', options);
     },
 
-    onUpdated: function onUpdated(model) {
+    onUpdated: function onUpdated(model, attrs, options) {
-        model.emitChange('edited');
+        model.emitChange('edited', options);
     },
 
-    onDestroyed: function onDestroyed(model) {
+    onDestroyed: function onDestroyed(model, options) {
-        model.emitChange('deleted');
+        model.emitChange('deleted', options);
     },
 
     onSaving: function onSaving(newTag, attr, options) {

core/server/models/user.js

@@ -1,4 +1,6 @@
-var _ = require('lodash'),
+'use strict';
+
+const _ = require('lodash'),
     Promise = require('bluebird'),
     validator = require('validator'),
     ObjectId = require('bson-objectid'),
@@ -15,9 +17,9 @@ var _ = require('lodash'),
      * locked user: imported users, they get a random passport
      */
     inactiveStates = ['inactive', 'locked'],
-    allStates = activeStates.concat(inactiveStates),
+    allStates = activeStates.concat(inactiveStates);
-    User,
+
-    Users;
+let User, Users;
 
 User = ghostBookshelf.Model.extend({
 
@@ -30,23 +32,24 @@ User = ghostBookshelf.Model.extend({
     },
 
     emitChange: function emitChange(event, options) {
-        common.events.emit('user' + '.' + event, this, options);
+        const eventToTrigger = 'user' + '.' + event;
+        ghostBookshelf.Model.prototype.emitChange.bind(this)(this, eventToTrigger, options);
     },
 
-    onDestroyed: function onDestroyed(model, response, options) {
+    onDestroyed: function onDestroyed(model, options) {
         if (_.includes(activeStates, model.previous('status'))) {
             model.emitChange('deactivated', options);
         }
 
-        model.emitChange('deleted');
+        model.emitChange('deleted', options);
     },
 
-    onCreated: function onCreated(model) {
+    onCreated: function onCreated(model, attrs, options) {
-        model.emitChange('added');
+        model.emitChange('added', options);
 
         // active is the default state, so if status isn't provided, this will be an active user
         if (!model.get('status') || _.includes(activeStates, model.get('status'))) {
-            model.emitChange('activated');
+            model.emitChange('activated', options);
         }
     },
 
@@ -58,11 +61,11 @@ User = ghostBookshelf.Model.extend({
             model.emitChange(model.isActive ? 'activated' : 'deactivated', options);
         } else {
             if (model.isActive) {
-                model.emitChange('activated.edited');
+                model.emitChange('activated.edited', options);
             }
         }
 
-        model.emitChange('edited');
+        model.emitChange('edited', options);
     },
 
     isActive: function isActive() {

core/server/models/webhook.js

@@ -1,16 +1,17 @@
-var Promise = require('bluebird'),
+'use strict';
-    ghostBookshelf = require('./base'),
+
-    common = require('../lib/common'),
+const Promise = require('bluebird'),
-    Webhook,
+    ghostBookshelf = require('./base');
+
+let Webhook,
     Webhooks;
 
 Webhook = ghostBookshelf.Model.extend({
     tableName: 'webhooks',
 
     emitChange: function emitChange(event, options) {
-        options = options || {};
+        const eventToTrigger = 'webhook' + '.' + event;
-
+        ghostBookshelf.Model.prototype.emitChange.bind(this)(this, eventToTrigger, options);
-        common.events.emit('webhook' + '.' + event, this, options);
     },
 
     onCreated: function onCreated(model, response, options) {
@@ -21,7 +22,7 @@ Webhook = ghostBookshelf.Model.extend({
         model.emitChange('edited', options);
     },
 
-    onDestroyed: function onDestroyed(model, response, options) {
+    onDestroyed: function onDestroyed(model, options) {
         model.emitChange('deleted', options);
     }
 }, {

core/test/integration/model/model_posts_spec.js

@@ -424,6 +424,48 @@ describe('Post Model', function () {
                 });
             });
 
+            it('[failure] multiple edits in one transaction', function () {
+                const options = _.cloneDeep(context),
+                    data = {
+                        status: 'published'
+                    };
+
+                return models.Base.transaction(function (txn) {
+                    options.transacting = txn;
+
+                    return models.Post.edit(data, _.merge({id: testUtils.DataGenerator.Content.posts[3].id}, options))
+                        .then(function () {
+                            return models.Post.edit(data, _.merge({id: testUtils.DataGenerator.Content.posts[5].id}, options));
+                        })
+                        .then(function () {
+                            // force rollback
+                            throw new Error();
+                        });
+                }).catch(function () {
+                    // txn was rolled back
+                    Object.keys(eventsTriggered).length.should.eql(0);
+                });
+            });
+
+            it('multiple edits in one transaction', function () {
+                const options = _.cloneDeep(context),
+                    data = {
+                        status: 'published'
+                    };
+
+                return models.Base.transaction(function (txn) {
+                    options.transacting = txn;
+
+                    return models.Post.edit(data, _.merge({id: testUtils.DataGenerator.Content.posts[3].id}, options))
+                        .then(function () {
+                            return models.Post.edit(data, _.merge({id: testUtils.DataGenerator.Content.posts[5].id}, options));
+                        });
+                }).then(function () {
+                    // txn was successful
+                    Object.keys(eventsTriggered).length.should.eql(4);
+                });
+            });
+
             it('can change title', function (done) {
                 var postId = testUtils.DataGenerator.Content.posts[0].id;
 
@@ -932,7 +974,10 @@ describe('Post Model', function () {
                     post.status.should.equal('draft');
 
                     // Test changing status and published_by at the same time
-                    return models.Post.edit({status: 'published', published_by: 4}, _.extend({}, context, {id: postId}));
+                    return models.Post.edit({
+                        status: 'published',
+                        published_by: 4
+                    }, _.extend({}, context, {id: postId}));
                 }).then(function (edited) {
                     should.exist(edited);
                     edited.attributes.status.should.equal('published');