✨ post update collision detection (#8328) (#8362)

closes #5599

If two users edit the same post, it can happen that they override each others content or post settings. With this change this won't happen anymore.

✨ Update collision for posts
- add a new bookshelf plugin to detect these changes
- use the `changed` object of bookshelf -> we don't have to create our own diff
- compare client and server updated_at field
- run editing posts in a transaction (see comments in code base)

🙀  update collision for tags
- `updateTags` for adding posts on `onCreated` - happens after the post was inserted
   --> it's "okay" to attach the tags afterwards on insert
   --> there is no need to add collision for inserting data
   --> it's very hard to move the updateTags call to `onCreating`, because the `updateTags` function queries the database to look up the affected post
- `updateTags` while editing posts on `onSaving` - all operations run in a transactions and are rolled back if something get's rejected

- Post model edit: if we push a transaction from outside, take this one

✨  introduce options.forUpdate
- if two queries happening in a transaction we have to signalise knex/mysql that we select for an update
- otherwise the following case happens:
  >> you fetch posts for an update
  >> a user requests comes in and updates the post (e.g. sets title to "X")
  >> you update the fetched posts, title would get overriden to the old one

use options.forUpdate and protect internal post updates: model listeners
- use a transaction for listener updates
- signalise forUpdate
- write a complex test

use options.forUpdate and protect internal post updates: scheduling
- publish endpoint runs in a transaction
- add complex test
- @TODO: right now scheduling api uses posts api, therefor we had to extend the options for api's
  >> allowed to pass transactions through it
  >> but these are only allowed if defined from outside {opts: [...]}
  >> so i think this is fine and not dirty
  >> will wait for opinions
  >> alternatively we have to re-write the scheduling endpoint to use the models directly

core/server/api/posts.js

@@ -94,7 +94,7 @@ posts = {
 
         // Push all of our tasks into a `tasks` array in the correct order
         tasks = [
-            utils.validate(docName, {attrs: attrs}),
+            utils.validate(docName, {attrs: attrs, opts: options.opts || []}),
             utils.handlePublicPermissions(docName, 'read'),
             utils.convertOptions(allowedIncludes),
             modelQuery
@@ -135,7 +135,7 @@ posts = {
 
         // Push all of our tasks into a `tasks` array in the correct order
         tasks = [
-            utils.validate(docName, {opts: utils.idDefaultOptions}),
+            utils.validate(docName, {opts: utils.idDefaultOptions.concat(options.opts || [])}),
             utils.handlePermissions(docName, 'edit'),
             utils.convertOptions(allowedIncludes),
             modelQuery

core/server/api/schedules.js

@@ -10,7 +10,11 @@ var _ = require('lodash'),
     utils = require('./utils');
 
 /**
- * publish a scheduled post
+ * Publish a scheduled post
+ *
+ * `apiPosts.read` and `apiPosts.edit` must happen in one transaction.
+ * We lock the target row on fetch by using the `forUpdate` option.
+ * Read more in models/post.js - `onFetching`
  *
  * object.force: you can force publishing a post in the past (for example if your service was down)
  */
@@ -35,21 +39,32 @@ exports.publishPost = function publishPost(object, options) {
         function (cleanOptions) {
             cleanOptions.status = 'scheduled';
 
-            return apiPosts.read(cleanOptions)
+            return dataProvider.Base.transaction(function (transacting) {
-                .then(function (result) {
+                cleanOptions.transacting = transacting;
-                    post = result.posts[0];
+                cleanOptions.forUpdate = true;
-                    publishedAtMoment = moment(post.published_at);
 
-                    if (publishedAtMoment.diff(moment(), 'minutes') > publishAPostBySchedulerToleranceInMinutes) {
+                // CASE: extend allowed options, see api/utils.js
-                        return Promise.reject(new errors.NotFoundError({message: i18n.t('errors.api.job.notFound')}));
+                cleanOptions.opts = ['forUpdate', 'transacting'];
-                    }
 
-                    if (publishedAtMoment.diff(moment(), 'minutes') < publishAPostBySchedulerToleranceInMinutes * -1 && object.force !== true) {
+                return apiPosts.read(cleanOptions)
-                        return Promise.reject(new errors.NotFoundError({message: i18n.t('errors.api.job.publishInThePast')}));
+                    .then(function (result) {
-                    }
+                        post = result.posts[0];
+                        publishedAtMoment = moment(post.published_at);
 
-                    return apiPosts.edit({posts: [{status: 'published'}]}, _.pick(cleanOptions, ['context', 'id']));
+                        if (publishedAtMoment.diff(moment(), 'minutes') > publishAPostBySchedulerToleranceInMinutes) {
-                });
+                            return Promise.reject(new errors.NotFoundError({message: i18n.t('errors.api.job.notFound')}));
+                        }
+
+                        if (publishedAtMoment.diff(moment(), 'minutes') < publishAPostBySchedulerToleranceInMinutes * -1 && object.force !== true) {
+                            return Promise.reject(new errors.NotFoundError({message: i18n.t('errors.api.job.publishInThePast')}));
+                        }
+
+                        return apiPosts.edit({
+                            posts: [{status: 'published'}]},
+                            _.pick(cleanOptions, ['context', 'id', 'transacting', 'forUpdate', 'opts'])
+                        );
+                    });
+            });
         }
     ], options);
 };

core/server/api/utils.js

@@ -122,7 +122,7 @@ utils = {
                 name: {}
             },
             // these values are sanitised/validated separately
-            noValidation = ['data', 'context', 'include', 'filter'],
+            noValidation = ['data', 'context', 'include', 'filter', 'forUpdate', 'transacting'],
             errors = [];
 
         _.each(options, function (value, key) {
@@ -262,6 +262,7 @@ utils = {
                 options.columns = utils.prepareFields(options.fields);
                 delete options.fields;
             }
+
             return options;
         };
     },

core/server/models/base/index.js

@@ -5,20 +5,20 @@
 // The models are internal to Ghost, only the API and some internal functions such as migration and import/export
 // accesses the models directly. All other parts of Ghost, including the blog frontend, admin UI, and apps are only
 // allowed to access data via the API.
-var _          = require('lodash'),
+var _ = require('lodash'),
-    bookshelf  = require('bookshelf'),
+    bookshelf = require('bookshelf'),
-    moment     = require('moment'),
+    moment = require('moment'),
-    Promise    = require('bluebird'),
+    Promise = require('bluebird'),
-    ObjectId   = require('bson-objectid'),
+    ObjectId = require('bson-objectid'),
-    config     = require('../../config'),
+    config = require('../../config'),
-    db         = require('../../data/db'),
+    db = require('../../data/db'),
-    errors     = require('../../errors'),
+    errors = require('../../errors'),
-    filters    = require('../../filters'),
+    filters = require('../../filters'),
-    schema     = require('../../data/schema'),
+    schema = require('../../data/schema'),
-    utils      = require('../../utils'),
+    utils = require('../../utils'),
     validation = require('../../data/validation'),
-    plugins    = require('../plugins'),
+    plugins = require('../plugins'),
-    i18n       = require('../../i18n'),
+    i18n = require('../../i18n'),
 
     ghostBookshelf,
     proto;
@@ -42,6 +42,9 @@ ghostBookshelf.plugin(plugins.includeCount);
 // Load the Ghost pagination plugin, which gives us the `fetchPage` method on Models
 ghostBookshelf.plugin(plugins.pagination);
 
+// Update collision plugin
+ghostBookshelf.plugin(plugins.collision);
+
 // Cache an instance of the base model prototype
 proto = ghostBookshelf.Model.prototype;
 
@@ -77,18 +80,35 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({
             this.include = _.clone(options.include);
         }
 
-        ['fetching', 'fetched', 'creating', 'created', 'updating', 'updated', 'destroying', 'destroyed', 'saved']
+        [
-            .forEach(function (eventName) {
+            'fetching',
-                var functionName = 'on' + eventName[0].toUpperCase() + eventName.slice(1);
+            'fetching:collection',
+            'fetched',
+            'creating',
+            'created',
+            'updating',
+            'updated',
+            'destroying',
+            'destroyed',
+            'saved'
+        ].forEach(function (eventName) {
+            var functionName = 'on' + eventName[0].toUpperCase() + eventName.slice(1);
 
-                if (!self[functionName]) {
+            if (functionName.indexOf(':') !== -1) {
-                    return;
+                functionName = functionName.slice(0, functionName.indexOf(':'))
-                }
+                    + functionName[functionName.indexOf(':') + 1].toUpperCase()
+                    + functionName.slice(functionName.indexOf(':') + 2);
+                functionName = functionName.replace(':', '');
+            }
 
-                self.on(eventName, function eventTriggered() {
+            if (!self[functionName]) {
-                    return this[functionName].apply(this, arguments);
+                return;
-                });
+            }
+
+            self.on(eventName, function eventTriggered() {
+                return this[functionName].apply(this, arguments);
             });
+        });
 
         this.on('saving', function onSaving() {
             var self = this,
@@ -134,8 +154,8 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({
 
         _.each(attrs, function each(value, key) {
             if (value !== null
-                    && schema.tables[self.tableName].hasOwnProperty(key)
+                && schema.tables[self.tableName].hasOwnProperty(key)
-                    && schema.tables[self.tableName][key].type === 'dateTime') {
+                && schema.tables[self.tableName][key].type === 'dateTime') {
                 attrs[key] = moment(value).format('YYYY-MM-DD HH:mm:ss');
             }
         });
@@ -172,7 +192,7 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({
         var self = this;
         _.each(attrs, function each(value, key) {
             if (schema.tables[self.tableName].hasOwnProperty(key)
-                    && schema.tables[self.tableName][key].type === 'bool') {
+                && schema.tables[self.tableName][key].type === 'bool') {
                 attrs[key] = value ? true : false;
             }
         });
@@ -360,7 +380,7 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({
      * @param {Object} options Represents options to filter in order to be passed to the Bookshelf query.
      * @param {String} methodName The name of the method to check valid options for.
      * @return {Object} The filtered results of `options`.
-    */
+     */
     filterOptions: function filterOptions(options, methodName) {
         var permittedOptions = this.permittedOptions(methodName),
             filteredOptions = _.pick(options, permittedOptions);
@@ -423,9 +443,9 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({
     findPage: function findPage(options) {
         options = options || {};
 
-        var self             = this,
+        var self = this,
-            itemCollection   = this.forge(null, {context: options.context}),
+            itemCollection = this.forge(null, {context: options.context}),
-            tableName        = _.result(this.prototype, 'tableName'),
+            tableName = _.result(this.prototype, 'tableName'),
             requestedColumns = options.columns;
 
         // Set this to true or pass ?debug=true as an API option to get output
@@ -462,7 +482,7 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({
         }
 
         return itemCollection.fetchPage(options).then(function formatResponse(response) {
-            var data   = {},
+            var data = {},
                 models = [];
 
             options.columns = requestedColumns;
@@ -496,6 +516,10 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({
     /**
      * ### Edit
      * Naive edit
+     *
+     * We always forward the `method` option to Bookshelf, see http://bookshelfjs.org/#Model-instance-save.
+     * Based on the `method` option Bookshelf and Ghost can determine if a query is an insert or an update.
+     *
      * @param {Object} data
      * @param {Object} options (optional)
      * @return {Promise(ghostBookshelf.Model)} Edited Model
@@ -514,7 +538,7 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({
 
         return model.fetch(options).then(function then(object) {
             if (object) {
-                return object.save(data, options);
+                return object.save(data, _.merge({method: 'update'}, options));
             }
         });
     },
@@ -560,13 +584,13 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({
     },
 
     /**
-    * ### Generate Slug
+     * ### Generate Slug
      * Create a string to act as the permalink for an object.
      * @param {ghostBookshelf.Model} Model Model type to generate a slug for
      * @param {String} base The string for which to generate a slug, usually a title or name
      * @param {Object} options Options to pass to findOne
      * @return {Promise(String)} Resolves to a unique slug string
-    */
+     */
     generateSlug: function generateSlug(Model, base, options) {
         var slug,
             slugTryCount = 1,

core/server/models/base/listeners.js

@@ -4,7 +4,8 @@ var config = require('../../config'),
     errors = require(config.get('paths:corePath') + '/server/errors'),
     logging = require(config.get('paths:corePath') + '/server/logging'),
     sequence = require(config.get('paths:corePath') + '/server/utils/sequence'),
-    moment = require('moment-timezone');
+    moment = require('moment-timezone'),
+    _ = require('lodash');
 
 /**
  * WHEN access token is created we will update last_seen for user.
@@ -43,54 +44,66 @@ events.on('user.deactivated', function (userModel) {
 events.on('settings.activeTimezone.edited', function (settingModel) {
     var newTimezone = settingModel.attributes.value,
         previousTimezone = settingModel._updatedAttributes.value,
-        timezoneOffsetDiff = moment.tz(previousTimezone).utcOffset() - moment.tz(newTimezone).utcOffset();
+        timezoneOffsetDiff = moment.tz(previousTimezone).utcOffset() - moment.tz(newTimezone).utcOffset(),
+        options = {context: {internal: true}};
 
     // CASE: TZ was updated, but did not change
     if (previousTimezone === newTimezone) {
         return;
     }
 
-    models.Post.findAll({filter: 'status:scheduled', context: {internal: true}})
+    /**
-        .then(function (results) {
+     * CASE:
-            if (!results.length) {
+     * `Post.findAll` and the Post.edit` must run in one single transaction.
-                return;
+     * We lock the target row on fetch by using the `forUpdate` option.
-            }
+     * Read more in models/post.js - `onFetching`
+     */
+    return models.Base.transaction(function (transacting) {
+        options.transacting = transacting;
+        options.forUpdate = true;
 
-            return sequence(results.map(function (post) {
+        return models.Post.findAll(_.merge({filter: 'status:scheduled'}, options))
-                return function reschedulePostIfPossible() {
+            .then(function (results) {
-                    var newPublishedAtMoment = moment(post.get('published_at')).add(timezoneOffsetDiff, 'minutes');
+                if (!results.length) {
-
+                    return;
-                    /**
-                     * CASE:
-                     *   - your configured TZ is GMT+01:00
-                     *   - now is 10AM +01:00 (9AM UTC)
-                     *   - your post should be published 8PM +01:00 (7PM UTC)
-                     *   - you reconfigure your blog TZ to GMT+08:00
-                     *   - now is 5PM +08:00 (9AM UTC)
-                     *   - if we don't change the published_at, 7PM + 8 hours === next day 5AM
-                     *   - so we update published_at to 7PM - 480minutes === 11AM UTC
-                     *   - 11AM UTC === 7PM +08:00
-                     */
-                    if (newPublishedAtMoment.isBefore(moment().add(5, 'minutes'))) {
-                        post.set('status', 'draft');
-                    } else {
-                        post.set('published_at', newPublishedAtMoment.toDate());
-                    }
-
-                    return models.Post.edit(post.toJSON(), {id: post.id, context: {internal: true}}).reflect();
-                };
-            })).each(function (result) {
-                if (!result.isFulfilled()) {
-                    logging.error(new errors.GhostError({
-                        err: result.reason()
-                    }));
                 }
+
+                return sequence(results.map(function (post) {
+                    return function reschedulePostIfPossible() {
+                        var newPublishedAtMoment = moment(post.get('published_at')).add(timezoneOffsetDiff, 'minutes');
+
+                        /**
+                         * CASE:
+                         *   - your configured TZ is GMT+01:00
+                         *   - now is 10AM +01:00 (9AM UTC)
+                         *   - your post should be published 8PM +01:00 (7PM UTC)
+                         *   - you reconfigure your blog TZ to GMT+08:00
+                         *   - now is 5PM +08:00 (9AM UTC)
+                         *   - if we don't change the published_at, 7PM + 8 hours === next day 5AM
+                         *   - so we update published_at to 7PM - 480minutes === 11AM UTC
+                         *   - 11AM UTC === 7PM +08:00
+                         */
+                        if (newPublishedAtMoment.isBefore(moment().add(5, 'minutes'))) {
+                            post.set('status', 'draft');
+                        } else {
+                            post.set('published_at', newPublishedAtMoment.toDate());
+                        }
+
+                        return models.Post.edit(post.toJSON(), _.merge({id: post.id}, options)).reflect();
+                    };
+                })).each(function (result) {
+                    if (!result.isFulfilled()) {
+                        logging.error(new errors.GhostError({
+                            err: result.reason()
+                        }));
+                    }
+                });
+            })
+            .catch(function (err) {
+                logging.error(new errors.GhostError({
+                    err: err,
+                    level: 'critical'
+                }));
             });
-        })
+    });
-        .catch(function (err) {
-            logging.error(new errors.GhostError({
-                err: err,
-                level: 'critical'
-            }));
-        });
 });

core/server/models/plugins/collision.js

@@ -0,0 +1,86 @@
+var moment = require('moment-timezone'),
+    Promise = require('bluebird'),
+    _ = require('lodash'),
+    errors = require('../../errors');
+
+module.exports = function (Bookshelf) {
+    var ParentModel = Bookshelf.Model,
+        Model;
+
+    Model = Bookshelf.Model.extend({
+        /**
+         * Update collision protection.
+         *
+         * IMPORTANT NOTES:
+         * The `sync` method is called for any query e.g. update, add, delete, fetch
+         *
+         * We had the option to override Bookshelf's `save` method, but hooking into the `sync` method gives us
+         * the ability to access the `changed` object. Bookshelf already knows which attributes has changed.
+         *
+         * Bookshelf's timestamp function can't be overridden, as it's synchronous, there is no way to return an Error.
+         *
+         * If we want to enable the collision plugin for other tables, the queries might need to run in a transaction.
+         * This depends on if we fetch the model before editing. Imagine two concurrent requests come in, both would fetch
+         * the same current database values and both would succeed to update and override each other.
+         */
+        sync: function timestamp(options) {
+            var parentSync = ParentModel.prototype.sync.apply(this, arguments),
+                originalUpdateSync = parentSync.update,
+                self = this;
+
+            // CASE: only enabled for posts table
+            if (this.tableName !== 'posts' ||
+                !self.serverData ||
+                ((options.method !== 'update' && options.method !== 'patch') || !options.method)
+            ) {
+                return parentSync;
+            }
+
+            /**
+             * Only hook into the update sync
+             *
+             * IMPORTANT NOTES:
+             * Even if the client sends a different `id` property, it get's ignored by bookshelf.
+             * Because you can't change the `id` of an existing post.
+             *
+             * HTML is always auto generated, ignore.
+             */
+            parentSync.update = function update() {
+                var changed = _.omit(self.changed, [
+                        'created_at', 'updated_at', 'author_id', 'id',
+                        'published_by', 'updated_by', 'html', 'plaintext'
+                    ]),
+                    clientUpdatedAt = moment(self.clientData.updated_at || self.serverData.updated_at),
+                    serverUpdatedAt = moment(self.serverData.updated_at);
+
+                if (Object.keys(changed).length) {
+                    if (clientUpdatedAt.diff(serverUpdatedAt) !== 0) {
+                        return Promise.reject(new errors.InternalServerError({
+                            message: 'Saving failed! Someone else is editing this post.',
+                            code: 'UPDATE_COLLISION'
+                        }));
+                    }
+                }
+
+                return originalUpdateSync.apply(this, arguments);
+            };
+
+            return parentSync;
+        },
+
+        /**
+         * We have to remember current server data and client data.
+         * The `sync` method has no access to it.
+         * `updated_at` is already set to "Date.now" when the overridden `sync.update` is called.
+         * See https://github.com/tgriesser/bookshelf/blob/79c526870e618748caf94e7476a0bc796ee090a6/src/model.js#L955
+         */
+        save: function save(data) {
+            this.clientData = _.cloneDeep(data) || {};
+            this.serverData = _.cloneDeep(this.attributes);
+
+            return ParentModel.prototype.save.apply(this, arguments);
+        }
+    });
+
+    Bookshelf.Model = Model;
+};

core/server/models/plugins/index.js

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

core/server/models/post.js

@@ -37,17 +37,42 @@ Post = ghostBookshelf.Model.extend({
         };
     },
 
-    onSaved: function onSaved(model, response, options) {
+    /**
-        return this.updateTags(model, response, options);
+     * We update the tags after the Post was inserted.
-    },
+     * We update the tags before the Post was updated, see `onSaving` event.
-
+     * `onCreated` is called before `onSaved`.
-    onCreated: function onCreated(model) {
+     */
+    onCreated: function onCreated(model, response, options) {
         var status = model.get('status');
+
         model.emitChange('added');
 
         if (['published', 'scheduled'].indexOf(status) !== -1) {
             model.emitChange(status);
         }
+
+        return this.updateTags(model, response, options);
+    },
+
+    /**
+     * http://knexjs.org/#Builder-forUpdate
+     * https://dev.mysql.com/doc/refman/5.7/en/innodb-locking-reads.html
+     *
+     * Lock target collection/model for further update operations.
+     * This avoids collisions and possible content override cases.
+     *
+     * `forUpdate` is only supported for posts right now
+     */
+    onFetching: function onFetching(model, columns, options) {
+        if (options.forUpdate && options.transacting) {
+            options.query.forUpdate();
+        }
+    },
+
+    onFetchingCollection: function onFetchingCollection(model, columns, options) {
+        if (options.forUpdate && options.transacting) {
+            options.query.forUpdate();
+        }
     },
 
     onUpdated: function onUpdated(model) {
@@ -152,7 +177,7 @@ Post = ghostBookshelf.Model.extend({
             publishedAt = this.get('published_at'),
             publishedAtHasChanged = this.hasDateChanged('published_at', {beforeWrite: true}),
             mobiledoc   = this.get('mobiledoc'),
-            tags = [];
+            tags = [], ops = [];
 
         // CASE: disallow published -> scheduled
         // @TODO: remove when we have versioning based on updated_at
@@ -196,6 +221,7 @@ Post = ghostBookshelf.Model.extend({
             });
 
             // keep tags for 'saved' event
+            // get('tags') will be removed after saving, because it's not a direct attribute of posts (it's a relation)
             this.tagsToSave = tags;
         }
 
@@ -243,32 +269,55 @@ Post = ghostBookshelf.Model.extend({
             }
         }
 
+        /**
+         * - `updateTags` happens before the post is saved to the database
+         * - when editing a post, it's running in a transaction, see `Post.edit`
+         * - we are using a update collision detection, we have to know if tags were updated in the client
+         *
+         * NOTE: For adding a post, updateTags happens after the post insert, see `onCreated` event
+         */
+        if (options.method === 'update' || options.method === 'patch') {
+            ops.push(function updateTags() {
+                return self.updateTags(model, attr, options);
+            });
+        }
+
         // If a title is set, not the same as the old title, a draft post, and has never been published
         if (prevTitle !== undefined && newTitle !== prevTitle && newStatus === 'draft' && !publishedAt) {
-            // Pass the new slug through the generator to strip illegal characters, detect duplicates
+            ops.push(function updateSlug() {
-            return ghostBookshelf.Model.generateSlug(Post, this.get('title'),
-                    {status: 'all', transacting: options.transacting, importing: options.importing})
-                .then(function then(slug) {
-                    // After the new slug is found, do another generate for the old title to compare it to the old slug
-                    return ghostBookshelf.Model.generateSlug(Post, prevTitle).then(function then(prevTitleSlug) {
-                        // If the old slug is the same as the slug that was generated from the old title
-                        // then set a new slug. If it is not the same, means was set by the user
-                        if (prevTitleSlug === prevSlug) {
-                            self.set({slug: slug});
-                        }
-                    });
-                });
-        } else {
-            // If any of the attributes above were false, set initial slug and check to see if slug was changed by the user
-            if (this.hasChanged('slug') || !this.get('slug')) {
                 // Pass the new slug through the generator to strip illegal characters, detect duplicates
-                return ghostBookshelf.Model.generateSlug(Post, this.get('slug') || this.get('title'),
+                return ghostBookshelf.Model.generateSlug(Post, self.get('title'),
-                        {status: 'all', transacting: options.transacting, importing: options.importing})
+                    {status: 'all', transacting: options.transacting, importing: options.importing})
                     .then(function then(slug) {
-                        self.set({slug: slug});
+                        // After the new slug is found, do another generate for the old title to compare it to the old slug
+                        return ghostBookshelf.Model.generateSlug(Post, prevTitle,
+                            {status: 'all', transacting: options.transacting, importing: options.importing}
+                        ).then(function then(prevTitleSlug) {
+                            // If the old slug is the same as the slug that was generated from the old title
+                            // then set a new slug. If it is not the same, means was set by the user
+                            if (prevTitleSlug === prevSlug) {
+                                self.set({slug: slug});
+                            }
+                        });
                     });
-            }
+            });
+        } else {
+            ops.push(function updateSlug() {
+                // If any of the attributes above were false, set initial slug and check to see if slug was changed by the user
+                if (self.hasChanged('slug') || !self.get('slug')) {
+                    // Pass the new slug through the generator to strip illegal characters, detect duplicates
+                    return ghostBookshelf.Model.generateSlug(Post, self.get('slug') || self.get('title'),
+                        {status: 'all', transacting: options.transacting, importing: options.importing})
+                        .then(function then(slug) {
+                            self.set({slug: slug});
+                        });
+                }
+
+                return Promise.resolve();
+            });
         }
+
+        return sequence(ops);
     },
 
     onCreating: function onCreating(model, attr, options) {
@@ -312,7 +361,9 @@ Post = ghostBookshelf.Model.extend({
                     tagsToRemove,
                     tagsToCreate;
 
+                // CASE: if nothing has changed, unset `tags`.
                 if (baseUtils.tagUpdate.tagSetsAreEqual(newTags, currentTags)) {
+                    savedModel.unset('tags');
                     return;
                 }
 
@@ -515,9 +566,10 @@ Post = ghostBookshelf.Model.extend({
             // whitelists for the `options` hash argument on methods, by method name.
             // these are the only options that can be passed to Bookshelf / Knex.
             validOptions = {
-                findOne: ['columns', 'importing', 'withRelated', 'require'],
+                findOne: ['columns', 'importing', 'withRelated', 'require', 'forUpdate'],
                 findPage: ['page', 'limit', 'columns', 'filter', 'order', 'status', 'staticPages'],
-                findAll: ['columns', 'filter']
+                findAll: ['columns', 'filter', 'forUpdate'],
+                edit: ['forUpdate']
             };
 
         if (validOptions[methodName]) {
@@ -626,22 +678,37 @@ Post = ghostBookshelf.Model.extend({
 
     /**
      * ### Edit
+     * Fetches and saves to Post. See model.Base.edit
+     *
      * @extends ghostBookshelf.Model.edit to handle returning the full object and manage _updatedAttributes
      * **See:** [ghostBookshelf.Model.edit](base.js.html#edit)
      */
     edit: function edit(data, options) {
-        var self = this;
+        var self = this,
+            editPost = function editPost(data, options) {
+                options.forUpdate = true;
+
+                return ghostBookshelf.Model.edit.call(self, data, options).then(function then(post) {
+                    return self.findOne({status: 'all', id: options.id}, options)
+                        .then(function then(found) {
+                            if (found) {
+                                // Pass along the updated attributes for checking status changes
+                                found._updatedAttributes = post._updatedAttributes;
+                                return found;
+                            }
+                        });
+                });
+            };
+
         options = options || {};
 
-        return ghostBookshelf.Model.edit.call(this, data, options).then(function then(post) {
+        if (options.transacting) {
-            return self.findOne({status: 'all', id: options.id}, options)
+            return editPost(data, options);
-                .then(function then(found) {
+        }
-                    if (found) {
+
-                        // Pass along the updated attributes for checking status changes
+        return ghostBookshelf.transaction(function (transacting) {
-                        found._updatedAttributes = post._updatedAttributes;
+            options.transacting = transacting;
-                        return found;
+            return editPost(data, options);
-                    }
-                });
         });
     },
 

core/test/integration/api/api_schedules_spec.js

@@ -1,13 +1,15 @@
 var should = require('should'),
-    testUtils = require('../../utils'),
+    sinon = require('sinon'),
     moment = require('moment'),
     Promise = require('bluebird'),
     ObjectId = require('bson-objectid'),
-    config = require(__dirname + '/../../../server/config'),
+    testUtils = require('../../utils'),
+    config = require('../../../server/config'),
     sequence = require(config.get('paths').corePath + '/server/utils/sequence'),
     errors = require(config.get('paths').corePath + '/server/errors'),
     api = require(config.get('paths').corePath + '/server/api'),
-    models = require(config.get('paths').corePath + '/server/models');
+    models = require(config.get('paths').corePath + '/server/models'),
+    sandbox = sinon.sandbox.create();
 
 describe('Schedules API', function () {
     var scope = {posts: []};
@@ -192,6 +194,10 @@ describe('Schedules API', function () {
             config.set('times:cannotScheduleAPostBeforeInMinutes', originalCannotScheduleAPostBeforeInMinutes);
         });
 
+        afterEach(function () {
+            sandbox.restore();
+        });
+
         describe('success', function () {
             beforeEach(function (done) {
                 scope.posts.push(testUtils.DataGenerator.forKnex.createPost({
@@ -200,6 +206,7 @@ describe('Schedules API', function () {
                     published_by: testUtils.users.ids.author,
                     published_at: moment().toDate(),
                     status: 'scheduled',
+                    title: 'title',
                     slug: 'first'
                 }));
 
@@ -281,6 +288,50 @@ describe('Schedules API', function () {
                     })
                     .catch(done);
             });
+
+            it('collision protection', function (done) {
+                var originalPostApi = api.posts.edit,
+                    postId = scope.posts[0].id, // first post is status=scheduled!
+                    requestCanComeIn = false,
+                    interval;
+
+                // this request get's blocked
+                interval = setInterval(function () {
+                    if (requestCanComeIn) {
+                        clearInterval(interval);
+
+                        // happens in a transaction, request has to wait until the scheduler api finished
+                        return models.Post.edit({title: 'Berlin'}, {id: postId, context: {internal: true}})
+                            .then(function (post) {
+                                post.id.should.eql(postId);
+                                post.get('status').should.eql('published');
+                                post.get('title').should.eql('Berlin');
+                                done();
+                            })
+                            .catch(done);
+                    }
+                }, 500);
+
+                // target post to publish was read already, simulate a client request
+                sandbox.stub(api.posts, 'edit', function () {
+                    var self = this,
+                        args = arguments;
+
+                    requestCanComeIn = true;
+                    return Promise.delay(2000)
+                        .then(function () {
+                            return originalPostApi.apply(self, args);
+                        });
+                });
+
+                api.schedules.publishPost({id: postId, context: {client: 'ghost-scheduler'}})
+                    .then(function (result) {
+                        result.posts[0].id.should.eql(postId);
+                        result.posts[0].status.should.eql('published');
+                        result.posts[0].title.should.eql('title');
+                    })
+                    .catch(done);
+            });
         });
 
         describe('error', function () {

core/test/integration/model/base/listeners_spec.js

@@ -1,6 +1,5 @@
 var should = require('should'), // jshint ignore:line
     sinon = require('sinon'),
-    testUtils = require('../../../utils'),
     Promise = require('bluebird'),
     moment = require('moment'),
     rewire = require('rewire'),
@@ -8,12 +7,15 @@ var should = require('should'), // jshint ignore:line
     config = require('../../../../server/config'),
     events = require(config.get('paths').corePath + '/server/events'),
     models = require(config.get('paths').corePath + '/server/models'),
-
+    testUtils = require(config.get('paths').corePath + '/test/utils'),
+    logging = require(config.get('paths').corePath + '/server/logging'),
+    sequence = require(config.get('paths').corePath + '/server/utils/sequence'),
     sandbox = sinon.sandbox.create();
 
 describe('Models: listeners', function () {
     var eventsToRemember = {},
         now = moment(),
+        listeners,
         scope = {
             posts: [],
             publishedAtFutureMoment1: moment().add(2, 'days').startOf('hour'),
@@ -29,10 +31,11 @@ describe('Models: listeners', function () {
             eventsToRemember[eventName] = callback;
         });
 
-        rewire(config.get('paths').corePath + '/server/models/base/listeners');
+        listeners = rewire(config.get('paths').corePath + '/server/models/base/listeners');
     });
 
     afterEach(function (done) {
+        events.on.restore();
         sandbox.restore();
         scope.posts = [];
         testUtils.teardown(done);
@@ -246,6 +249,69 @@ describe('Models: listeners', function () {
                         .catch(done);
                 })();
             });
+
+            it('collision: ensure the listener always succeeds', function (done) {
+                var timeout,
+                    interval,
+                    post1 = posts[0],
+                    listenerHasFinished = false;
+
+                sandbox.spy(logging, 'error');
+                sandbox.spy(models.Post, 'findAll');
+
+                // simulate a delay, so that the edit operation from the test here interrupts
+                // the goal here is to force that the listener has old post data, updated_at is then too old
+                // e.g. user edits while listener is active
+                listeners.__set__('sequence', function overrideSequence() {
+                    var self = this,
+                        args = arguments;
+
+                    return Promise.delay(3000)
+                        .then(function () {
+                            return sequence.apply(self, args)
+                                .finally(function () {
+                                    setTimeout(function () {
+                                        listenerHasFinished = true;
+                                    }, 500);
+                                });
+                        });
+                });
+
+                scope.timezoneOffset = -180;
+                scope.oldTimezone = 'Asia/Baghdad';
+                scope.newTimezone = 'Etc/UTC';
+
+                eventsToRemember['settings.activeTimezone.edited']({
+                    attributes: {value: scope.newTimezone},
+                    _updatedAttributes: {value: scope.oldTimezone}
+                });
+
+                models.Post.findAll.calledOnce.should.eql(false);
+
+                // set a little timeout to ensure the listener fetched posts from the database and the updated_at difference
+                // is big enough to simulate the collision scenario
+                // if you remove the transaction from the listener, this test will fail and show a collision error
+                timeout = setTimeout(function () {
+                    clearTimeout(timeout);
+
+                    // ensure findAll was called in the listener
+                    // ensure findAll was called before user's edit operation
+                    models.Post.findAll.calledOnce.should.eql(true);
+
+                    // simulate a client updates the post during the listener activity
+                    models.Post.edit({title: 'a new title, yes!'}, _.merge({id: post1.id}, testUtils.context.internal))
+                        .then(function () {
+                            interval = setInterval(function () {
+                                if (listenerHasFinished) {
+                                    clearInterval(interval);
+                                    logging.error.called.should.eql(false);
+                                    return done();
+                                }
+                            }, 1000);
+                        })
+                        .catch(done);
+                }, 2000);
+            });
         });
 
         describe('db has no scheduled posts', function () {

core/test/integration/model/model_posts_spec.js

@@ -437,7 +437,7 @@ describe('Post Model', function () {
                     should.exist(results);
                     results.attributes.html.should.match(/HTML Ipsum Presents/);
                     should.not.exist(results.attributes.plaintext);
-                    return PostModel.edit({updated_at: Date.now()}, _.extend({}, context, {id: postId}));
+                    return PostModel.edit({updated_at: results.attributes.updated_at}, _.extend({}, context, {id: postId}));
                 }).then(function (edited) {
                     should.exist(edited);
 
@@ -1455,6 +1455,110 @@ describe('Post Model', function () {
                 }).catch(done);
             });
         });
+
+        describe('Collision Protection', function () {
+            it('update post title, but updated_at is out of sync', function (done) {
+                var postToUpdate = {id: testUtils.DataGenerator.Content.posts[1].id};
+
+                PostModel.findOne({id: postToUpdate.id, status: 'all'})
+                    .then(function () {
+                        return Promise.delay(1000);
+                    })
+                    .then(function () {
+                        return PostModel.edit({
+                            title: 'New Post Title',
+                            updated_at: moment().subtract(1, 'day').format()
+                        }, _.extend({}, context, {id: postToUpdate.id}));
+                    })
+                    .then(function () {
+                        done(new Error('expected no success'));
+                    })
+                    .catch(function (err) {
+                        err.code.should.eql('UPDATE_COLLISION');
+                        done();
+                    });
+            });
+
+            it('update post tags and updated_at is out of sync', function (done) {
+                var postToUpdate = {id: testUtils.DataGenerator.Content.posts[1].id};
+
+                PostModel.findOne({id: postToUpdate.id, status: 'all'})
+                    .then(function () {
+                        return Promise.delay(1000);
+                    })
+                    .then(function () {
+                        return PostModel.edit({
+                            tags: [{name: 'new-tag-1'}],
+                            updated_at: moment().subtract(1, 'day').format()
+                        }, _.extend({}, context, {id: postToUpdate.id}));
+                    })
+                    .then(function () {
+                        done(new Error('expected no success'));
+                    })
+                    .catch(function (err) {
+                        err.code.should.eql('UPDATE_COLLISION');
+                        done();
+                    });
+            });
+
+            it('update post tags and updated_at is NOT out of sync', function (done) {
+                var postToUpdate = {id: testUtils.DataGenerator.Content.posts[1].id};
+
+                PostModel.findOne({id: postToUpdate.id, status: 'all'})
+                    .then(function () {
+                        return Promise.delay(1000);
+                    })
+                    .then(function () {
+                        return PostModel.edit({
+                            tags: [{name: 'new-tag-1'}]
+                        }, _.extend({}, context, {id: postToUpdate.id}));
+                    })
+                    .then(function () {
+                        done();
+                    })
+                    .catch(done);
+            });
+
+            it('update post with no changes, but updated_at is out of sync', function (done) {
+                var postToUpdate = {id: testUtils.DataGenerator.Content.posts[1].id};
+
+                PostModel.findOne({id: postToUpdate.id, status: 'all'})
+                    .then(function () {
+                        return Promise.delay(1000);
+                    })
+                    .then(function () {
+                        return PostModel.edit({
+                            updated_at: moment().subtract(1, 'day').format()
+                        }, _.extend({}, context, {id: postToUpdate.id}));
+                    })
+                    .then(function () {
+                        done();
+                    })
+                    .catch(done);
+            });
+
+            it('update post with old post title, but updated_at is out of sync', function (done) {
+                var postToUpdate = {
+                    id: testUtils.DataGenerator.Content.posts[1].id,
+                    title: testUtils.DataGenerator.forModel.posts[1].title
+                };
+
+                PostModel.findOne({id: postToUpdate.id, status: 'all'})
+                    .then(function () {
+                        return Promise.delay(1000);
+                    })
+                    .then(function () {
+                        return PostModel.edit({
+                            title: postToUpdate.title,
+                            updated_at: moment().subtract(1, 'day').format()
+                        }, _.extend({}, context, {id: postToUpdate.id}));
+                    })
+                    .then(function () {
+                        done();
+                    })
+                    .catch(done);
+            });
+        });
     });
 
     describe('Multiauthor Posts', function () {