From 3289dc76191ce90c3d1149610482134110171164 Mon Sep 17 00:00:00 2001 From: kirrg001 Date: Sat, 2 Feb 2019 10:24:53 +0100 Subject: [PATCH] Introduced model._changed refs #9248 - Bookshelf gives access to ".changed" before the update - Discussion: https://github.com/bookshelf/bookshelf/issues/1943 - We also need to know what has changed after the update to be able to decide if we should trigger events - Furthermore: Bookshelf cannot handle relation updates, it always marks relations as changed even though they did not change - Bumped bookshelf-relations to be able to - know if relations were updated - ensure we unset relations on bookshelf's ".changed" --- core/server/models/base/index.js | 30 ++++++++++------ core/server/models/plugins/collision.js | 48 +++++++++++++------------ core/server/models/relations/authors.js | 1 - package.json | 2 +- yarn.lock | 12 +++---- 5 files changed, 52 insertions(+), 41 deletions(-) diff --git a/core/server/models/base/index.js b/core/server/models/base/index.js index 679579cb31..0054405549 100644 --- a/core/server/models/base/index.js +++ b/core/server/models/base/index.js @@ -53,6 +53,7 @@ ghostBookshelf.plugin(plugins.hasPosts); ghostBookshelf.plugin('bookshelf-relations', { allowedOptions: ['context', 'importing', 'migrating'], unsetRelations: true, + extendChanged: '_changed', hooks: { belongsToMany: { after: function (existing, targets, options) { @@ -191,7 +192,7 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({ self.on(eventName, self[functionName]); }); - // NOTE: Please keep here. If we don't initialize the parent, bookshelf-relations won't work. + // @NOTE: Please keep here. If we don't initialize the parent, bookshelf-relations won't work. proto.initialize.call(this); }, @@ -279,23 +280,27 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({ * * @deprecated: x_by fields (https://github.com/TryGhost/Ghost/issues/10286) */ - onUpdating: function onUpdating(newObj, attr, options) { + onUpdating: function onUpdating(model, attr, options) { + if (this.relationships) { + model.changed = _.omit(model.changed, this.relationships); + } + if (schema.tables[this.tableName].hasOwnProperty('updated_by')) { if (!options.importing && !options.migrating) { this.set('updated_by', this.contextUser(options)); } } - if (options && options.context && !options.internal && !options.importing) { + if (options && options.context && !options.context.internal && !options.importing) { if (schema.tables[this.tableName].hasOwnProperty('created_at')) { - if (newObj.hasDateChanged('created_at', {beforeWrite: true})) { - newObj.set('created_at', this.previous('created_at')); + if (model.hasDateChanged('created_at', {beforeWrite: true})) { + model.set('created_at', this.previous('created_at')); } } if (schema.tables[this.tableName].hasOwnProperty('created_by')) { - if (newObj.hasChanged('created_by')) { - newObj.set('created_by', this.previous('created_by')); + if (model.hasChanged('created_by')) { + model.set('created_by', this.previous('created_by')); } } } @@ -303,13 +308,16 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({ // CASE: do not allow setting only the `updated_at` field, exception: importing if (schema.tables[this.tableName].hasOwnProperty('updated_at') && !options.importing) { if (options.migrating) { - newObj.set('updated_at', newObj.previous('updated_at')); - } else if (newObj.hasChanged() && Object.keys(newObj.changed).length === 1 && newObj.changed.updated_at) { - newObj.set('updated_at', newObj.previous('updated_at')); + model.set('updated_at', model.previous('updated_at')); + } else if (Object.keys(model.changed).length === 1 && model.changed.updated_at) { + model.set('updated_at', model.previous('updated_at')); + delete model.changed.updated_at; } } - return Promise.resolve(this.onValidate(newObj, attr, options)); + model._changed = _.cloneDeep(model.changed); + + return Promise.resolve(this.onValidate(model, attr, options)); }, /** diff --git a/core/server/models/plugins/collision.js b/core/server/models/plugins/collision.js index af2260ad7d..6d401867ba 100644 --- a/core/server/models/plugins/collision.js +++ b/core/server/models/plugins/collision.js @@ -46,30 +46,34 @@ module.exports = function (Bookshelf) { * 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 || new Date()), - serverUpdatedAt = moment(self.serverData.updated_at || clientUpdatedAt); + return originalUpdateSync.apply(this, arguments) + .then((response) => { + const changed = _.omit(self._changed, [ + 'created_at', 'updated_at', 'author_id', 'id', + 'published_by', 'updated_by', 'html', 'plaintext' + ]); - if (Object.keys(changed).length) { - if (clientUpdatedAt.diff(serverUpdatedAt) !== 0) { - // @TODO: Remove later. We want to figure out how many people experience the error in which context. - return Promise.reject(new common.errors.UpdateCollisionError({ - message: 'Saving failed! Someone else is editing this post.', - code: 'UPDATE_COLLISION', - level: 'critical', - context: JSON.stringify({ - clientUpdatedAt: self.clientData.updated_at, - serverUpdatedAt: self.serverData.updated_at, - changed: changed - }) - })); - } - } + const clientUpdatedAt = moment(self.clientData.updated_at || self.serverData.updated_at || new Date()); + const serverUpdatedAt = moment(self.serverData.updated_at || clientUpdatedAt); - return originalUpdateSync.apply(this, arguments); + if (Object.keys(changed).length) { + if (clientUpdatedAt.diff(serverUpdatedAt) !== 0) { + // @NOTE: This will rollback the update. We cannot know if relations were updated before doing the update. + return Promise.reject(new common.errors.UpdateCollisionError({ + message: 'Saving failed! Someone else is editing this post.', + code: 'UPDATE_COLLISION', + level: 'critical', + context: JSON.stringify({ + clientUpdatedAt: self.clientData.updated_at, + serverUpdatedAt: self.serverData.updated_at, + changed: changed + }) + })); + } + } + + return response; + }); }; return parentSync; diff --git a/core/server/models/relations/authors.js b/core/server/models/relations/authors.js index 89130512fd..692224aaa5 100644 --- a/core/server/models/relations/authors.js +++ b/core/server/models/relations/authors.js @@ -157,7 +157,6 @@ module.exports.extendModel = function extendModel(Post, Posts, ghostBookshelf) { serialize: function serialize(options) { const authors = this.related('authors'); - let attrs = proto.serialize.call(this, options); // CASE: e.g. you stub model response in the test diff --git a/package.json b/package.json index 2c9e7b5db1..7472dfeced 100644 --- a/package.json +++ b/package.json @@ -41,7 +41,7 @@ "bluebird": "3.5.3", "body-parser": "1.18.3", "bookshelf": "0.14.2", - "bookshelf-relations": "0.2.1", + "bookshelf-relations": "1.0.0", "brute-knex": "3.0.1", "bson-objectid": "1.2.4", "chalk": "2.4.2", diff --git a/yarn.lock b/yarn.lock index 3c3bd7c9aa..45cd92af1d 100644 --- a/yarn.lock +++ b/yarn.lock @@ -532,13 +532,13 @@ body@^5.1.0: raw-body "~1.1.0" safe-json-parse "~1.0.1" -bookshelf-relations@0.2.1: - version "0.2.1" - resolved "https://registry.yarnpkg.com/bookshelf-relations/-/bookshelf-relations-0.2.1.tgz#6974c0ec92409807d2cf6e73e111a38ebc03e7cd" +bookshelf-relations@1.0.0: + version "1.0.0" + resolved "https://registry.yarnpkg.com/bookshelf-relations/-/bookshelf-relations-1.0.0.tgz#709b49a8b51bbe30873a453b6b18c74858e52839" dependencies: - bluebird "^3.4.1" - ghost-ignition "2.9.2" - lodash "4.17.10" + bluebird "3.5.3" + ghost-ignition "3.0.0" + lodash "4.17.11" bookshelf@0.14.2: version "0.14.2"