From 7c6f690eb585f1dcf5b46202f49fddc5c8c8025b Mon Sep 17 00:00:00 2001 From: Katharina Irrgang Date: Mon, 26 Mar 2018 15:12:02 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20=20Fixed=20`updated=5Fat`=20not?= =?UTF-8?q?=20being=20updated=20(#9532)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit closes #9520 - it contains a dependency bump of the latest Bookshelf release - Bookshelf introduced a bug in the last release - see https://github.com/bookshelf/bookshelf/pull/1583 - see https://github.com/bookshelf/bookshelf/pull/1798 - this has caused trouble in Ghost - the `updated_at` attribute was not automatically set anymore --- The bookshelf added one breaking change: it's allow to pass custom `updated_at` and `created_at`. We already have a protection for not being able to override the `created_at` date on update. We had to add another protection to now allow to only change the `updated_at` property. You can only change `updated_at` if you actually change something else e.g. the title of a post. To be able to implement this check i discovered that Bookshelfs `model.changed` object has a tricky behaviour. It remembers **all** attributes, which where changed, doesn't matter if they are valid or invalid model properties. We had to add a line of code to avoid remembering none valid model attributes in this object. e.g. you change `tag.parent` (no valid model attribute). The valid property is `tag.parent_id`. If you pass `tag.parent` but the value has **not** changed (`tag.parent` === `tag.parent_id`), it will output you `tag.changed.parent`. But this is wrong. Bookshelf detects `changed` attributes too early. Or if you think the other way around, Ghost detects valid attributes too late. But the current earliest possible stage is the `onSaving` event, there is no earlier way to pick valid attributes (except of `.forge`, but we don't use this fn ATM). Later: the API should transform `tag.parent` into `tag.parent_id`, but we are not using it ATM, so no need to pre-optimise. The API already transforms `post.author` into `post.author_id`. --- core/server/models/base/index.js | 17 ++++++++++++ .../integration/model/model_posts_spec.js | 26 +++++++++---------- package.json | 2 +- yarn.lock | 6 ++--- 4 files changed, 33 insertions(+), 18 deletions(-) diff --git a/core/server/models/base/index.js b/core/server/models/base/index.js index 721808794b..599b59087d 100644 --- a/core/server/models/base/index.js +++ b/core/server/models/base/index.js @@ -171,6 +171,18 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({ this.attributes = this.pick(this.permittedAttributes()); // Store the previous attributes so we can tell what was updated later this._updatedAttributes = newObj.previousAttributes(); + + /** + * Bookshelf keeps none valid model attributes in `model.changed`. This causes problems + * when detecting if a model has changed. Bookshelf detects changed attributes too early. + * So we have to manually remove invalid model attributes from this object. + * + * e.g. if you pass `tag.parent` into the model layer, but the value has not changed, + * the attribute (`tag.parent`) is still kept in the `changed` object. This is wrong. + * + * TLDR; only keep valid model attributes in the changed object + */ + this.changed = _.pick(this.changed, Object.keys(this.attributes)); }, /** @@ -243,6 +255,11 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({ } } + // CASE: you only change the `updated_at` property. This is not allowed. + if (newObj.hasChanged() && Object.keys(newObj.changed).length === 1 && newObj.changed.hasOwnProperty('updated_at')) { + newObj.set('updated_at', this.previous('updated_at')); + } + return Promise.resolve(this.onValidate(newObj, attr, options)); }, diff --git a/core/test/integration/model/model_posts_spec.js b/core/test/integration/model/model_posts_spec.js index 039dac5525..926d2757c8 100644 --- a/core/test/integration/model/model_posts_spec.js +++ b/core/test/integration/model/model_posts_spec.js @@ -1811,17 +1811,12 @@ describe('Post Model', function () { }); it('can\'t edit dates and authors of existing tag', function () { - var newJSON = _.cloneDeep(postJSON), updatedAtFormat; + var newJSON = _.cloneDeep(postJSON), updatedAtFormat, createdAtFormat; // Add an existing tag to the beginning of the array - newJSON.tags = [{ - id: postJSON.tags[0].id, - slug: 'eins', - created_at: moment().add(2, 'days').format('YYYY-MM-DD HH:mm:ss'), - updated_at: moment().add(2, 'days').format('YYYY-MM-DD HH:mm:ss'), - created_by: 2, - updated_by: 2 - }]; + newJSON.tags = [_.cloneDeep(postJSON.tags[0])]; + newJSON.tags[0].created_at = moment().add(2, 'days').format('YYYY-MM-DD HH:mm:ss'); + newJSON.tags[0].updated_at = moment().add(2, 'days').format('YYYY-MM-DD HH:mm:ss'); // Edit the post return Promise.delay(1000) @@ -1834,16 +1829,19 @@ describe('Post Model', function () { updatedPost.tags.should.have.lengthOf(1); updatedPost.tags[0].should.have.properties({ name: postJSON.tags[0].name, - slug: 'eins', + slug: postJSON.tags[0].slug, id: postJSON.tags[0].id, - created_at: postJSON.tags[0].created_at, - created_by: postJSON.created_by, - updated_by: postJSON.updated_by + created_by: postJSON.tags[0].created_by, + updated_by: postJSON.tags[0].updated_by }); updatedAtFormat = moment(updatedPost.tags[0].updated_at).format('YYYY-MM-DD HH:mm:ss'); - updatedAtFormat.should.not.eql(moment(postJSON.updated_at).format('YYYY-MM-DD HH:mm:ss')); + updatedAtFormat.should.eql(moment(postJSON.tags[0].updated_at).format('YYYY-MM-DD HH:mm:ss')); updatedAtFormat.should.not.eql(moment(newJSON.tags[0].updated_at).format('YYYY-MM-DD HH:mm:ss')); + + createdAtFormat = moment(updatedPost.tags[0].created_at).format('YYYY-MM-DD HH:mm:ss'); + createdAtFormat.should.eql(moment(postJSON.tags[0].created_at).format('YYYY-MM-DD HH:mm:ss')); + createdAtFormat.should.not.eql(moment(newJSON.tags[0].created_at).format('YYYY-MM-DD HH:mm:ss')); }); }); diff --git a/package.json b/package.json index ddd6a901d3..e142270a3f 100644 --- a/package.json +++ b/package.json @@ -34,7 +34,7 @@ "bcryptjs": "2.4.3", "bluebird": "3.5.1", "body-parser": "1.18.2", - "bookshelf": "0.13.0", + "bookshelf": "0.13.3", "bookshelf-relations": "0.2.0", "brute-knex": "https://github.com/cobbspur/brute-knex/tarball/4feff38ad2e4ccd8d9de05f04a2ad7a5eb3e0ac1", "bson-objectid": "1.2.2", diff --git a/yarn.lock b/yarn.lock index 049f8298ba..fa3b2fb60c 100644 --- a/yarn.lock +++ b/yarn.lock @@ -535,9 +535,9 @@ bookshelf-relations@0.2.0: ghost-ignition "^2.8.16" lodash "^4.17.4" -bookshelf@0.13.0: - version "0.13.0" - resolved "https://registry.yarnpkg.com/bookshelf/-/bookshelf-0.13.0.tgz#dec282886c7653436a43c1e55fcb873c5822cb4a" +bookshelf@0.13.3: + version "0.13.3" + resolved "https://registry.yarnpkg.com/bookshelf/-/bookshelf-0.13.3.tgz#aa73d9159b6cac92830dc1ff37325490c3c6dfba" dependencies: babel-runtime "^6.26.0" bluebird "^3.4.3"