From c93f03b87e122edb62cc9f780926d640555bd5b4 Mon Sep 17 00:00:00 2001 From: Katharina Irrgang Date: Wed, 19 Apr 2017 15:53:23 +0200 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8=20=20post=20update=20collision=20dete?= =?UTF-8?q?ction=20(#8328)=20(#8362)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 4 +- core/server/api/schedules.js | 41 +++-- core/server/api/utils.js | 3 +- core/server/models/base/index.js | 90 +++++++---- core/server/models/base/listeners.js | 95 +++++++----- core/server/models/plugins/collision.js | 86 +++++++++++ core/server/models/plugins/index.js | 3 +- core/server/models/post.js | 141 +++++++++++++----- .../integration/api/api_schedules_spec.js | 57 ++++++- .../integration/model/base/listeners_spec.js | 72 ++++++++- .../integration/model/model_posts_spec.js | 106 ++++++++++++- 11 files changed, 563 insertions(+), 135 deletions(-) create mode 100644 core/server/models/plugins/collision.js diff --git a/core/server/api/posts.js b/core/server/api/posts.js index ef4bc69bf82..671ccada122 100644 --- a/core/server/api/posts.js +++ b/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 diff --git a/core/server/api/schedules.js b/core/server/api/schedules.js index 1bfe83d63fd..912ed1ab1f1 100644 --- a/core/server/api/schedules.js +++ b/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) - .then(function (result) { - post = result.posts[0]; - publishedAtMoment = moment(post.published_at); + return dataProvider.Base.transaction(function (transacting) { + cleanOptions.transacting = transacting; + cleanOptions.forUpdate = true; - if (publishedAtMoment.diff(moment(), 'minutes') > publishAPostBySchedulerToleranceInMinutes) { - return Promise.reject(new errors.NotFoundError({message: i18n.t('errors.api.job.notFound')})); - } + // CASE: extend allowed options, see api/utils.js + cleanOptions.opts = ['forUpdate', 'transacting']; - 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.read(cleanOptions) + .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); }; diff --git a/core/server/api/utils.js b/core/server/api/utils.js index 34ff2140223..521626f178a 100644 --- a/core/server/api/utils.js +++ b/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; }; }, diff --git a/core/server/models/base/index.js b/core/server/models/base/index.js index e8b68a2a2d8..6206f5286b1 100644 --- a/core/server/models/base/index.js +++ b/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'), - bookshelf = require('bookshelf'), - moment = require('moment'), - Promise = require('bluebird'), - ObjectId = require('bson-objectid'), - config = require('../../config'), - db = require('../../data/db'), - errors = require('../../errors'), - filters = require('../../filters'), - schema = require('../../data/schema'), - utils = require('../../utils'), +var _ = require('lodash'), + bookshelf = require('bookshelf'), + moment = require('moment'), + Promise = require('bluebird'), + ObjectId = require('bson-objectid'), + config = require('../../config'), + db = require('../../data/db'), + errors = require('../../errors'), + filters = require('../../filters'), + schema = require('../../data/schema'), + utils = require('../../utils'), validation = require('../../data/validation'), - plugins = require('../plugins'), - i18n = require('../../i18n'), + plugins = require('../plugins'), + 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) { - var functionName = 'on' + eventName[0].toUpperCase() + eventName.slice(1); + [ + 'fetching', + 'fetching:collection', + 'fetched', + 'creating', + 'created', + 'updating', + 'updated', + 'destroying', + 'destroyed', + 'saved' + ].forEach(function (eventName) { + var functionName = 'on' + eventName[0].toUpperCase() + eventName.slice(1); + + if (functionName.indexOf(':') !== -1) { + functionName = functionName.slice(0, functionName.indexOf(':')) + + functionName[functionName.indexOf(':') + 1].toUpperCase() + + functionName.slice(functionName.indexOf(':') + 2); + functionName = functionName.replace(':', ''); + } - if (!self[functionName]) { - return; - } + if (!self[functionName]) { + return; + } - self.on(eventName, function eventTriggered() { - return this[functionName].apply(this, arguments); - }); + 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][key].type === 'dateTime') { + && schema.tables[self.tableName].hasOwnProperty(key) + && 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, - itemCollection = this.forge(null, {context: options.context}), - tableName = _.result(this.prototype, 'tableName'), + var self = this, + itemCollection = this.forge(null, {context: options.context}), + 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, diff --git a/core/server/models/base/listeners.js b/core/server/models/base/listeners.js index 51e26073ef8..a86c613579d 100644 --- a/core/server/models/base/listeners.js +++ b/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) { - if (!results.length) { - return; - } + /** + * CASE: + * `Post.findAll` and the Post.edit` must run in one single transaction. + * 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 function reschedulePostIfPossible() { - var newPublishedAtMoment = moment(post.get('published_at')).add(timezoneOffsetDiff, 'minutes'); + return models.Post.findAll(_.merge({filter: 'status:scheduled'}, options)) + .then(function (results) { + 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 sequence(results.map(function (post) { + return function reschedulePostIfPossible() { + var newPublishedAtMoment = moment(post.get('published_at')).add(timezoneOffsetDiff, 'minutes'); - 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() - })); - } + /** + * 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' - })); - }); + }); }); diff --git a/core/server/models/plugins/collision.js b/core/server/models/plugins/collision.js new file mode 100644 index 00000000000..4a1bec0c97d --- /dev/null +++ b/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; +}; diff --git a/core/server/models/plugins/index.js b/core/server/models/plugins/index.js index 0246ff98508..c168a4449f2 100644 --- a/core/server/models/plugins/index.js +++ b/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') }; diff --git a/core/server/models/post.js b/core/server/models/post.js index d924d5d26b5..52a4876e222 100644 --- a/core/server/models/post.js +++ b/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); - }, - - onCreated: function onCreated(model) { + /** + * 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, 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 - return ghostBookshelf.Model.generateSlug(Post, this.get('title'), + ops.push(function updateSlug() { + // Pass the new slug through the generator to strip illegal characters, detect duplicates + return ghostBookshelf.Model.generateSlug(Post, self.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}); - } + .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, + {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 { - // 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'), + 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}); - }); - } + .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) { - 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; - } - }); + if (options.transacting) { + return editPost(data, options); + } + + return ghostBookshelf.transaction(function (transacting) { + options.transacting = transacting; + return editPost(data, options); }); }, diff --git a/core/test/integration/api/api_schedules_spec.js b/core/test/integration/api/api_schedules_spec.js index baaab84f6e7..fbd883c08a6 100644 --- a/core/test/integration/api/api_schedules_spec.js +++ b/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 () { diff --git a/core/test/integration/model/base/listeners_spec.js b/core/test/integration/model/base/listeners_spec.js index 7d6b63e38ca..238cf74d2d7 100644 --- a/core/test/integration/model/base/listeners_spec.js +++ b/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 () { diff --git a/core/test/integration/model/model_posts_spec.js b/core/test/integration/model/model_posts_spec.js index 1d404b2f86a..1475320dab2 100644 --- a/core/test/integration/model/model_posts_spec.js +++ b/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 () {