🐛 Fixed updated_at not being updated

closes TryGhost#9520 - it contains a dependency bump of the latest Bookshelf release - Bookshelf introduced a bug in the last release - see bookshelf/bookshelf#1583 - see bookshelf/bookshelf#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`.
kirrg001 · Mar 26, 2018 · 60ba673 · 60ba673
1 parent 60386db
commit 60ba673
Show file tree

Hide file tree

Showing 4 changed files with 33 additions and 18 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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"