From 7273786459befa2e9897dfe144f29a76e7836831 Mon Sep 17 00:00:00 2001 From: Katharina Irrgang Date: Sun, 15 Apr 2018 12:12:20 +0200 Subject: [PATCH] Fetch relations by default when insert/updating posts (#9568) no issue - required for model events - otherwise you won't receive a full data set - in worst case you have to re-fetch the post - required for the url service - the url service always needs relations (authors,tags) to be able to generate the url properly @IMPORTANT - no API change, we still return what you are asking for - we first edit/add the resource - then we fetch the data with the API options - @TODO: this can be optimised and will improve performance picking/selecting it from the insert/update response - this is an internal change --- core/server/models/base/index.js | 6 + core/server/models/post.js | 24 +- core/test/unit/models/post_spec.js | 431 ++++++++++++++++++++- core/test/utils/fixtures/data-generator.js | 1 + 4 files changed, 459 insertions(+), 3 deletions(-) diff --git a/core/server/models/base/index.js b/core/server/models/base/index.js index 6ee2fe9ba6..6644b9647a 100644 --- a/core/server/models/base/index.js +++ b/core/server/models/base/index.js @@ -604,6 +604,10 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({ permittedOptions = _.union(permittedOptions, extraAllowedProperties); options = _.pick(options, permittedOptions); + if (this.defaultRelations) { + options = this.defaultRelations(methodName, options); + } + return options; }, @@ -643,6 +647,8 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({ * information about the request (page, limit), along with the * info needed for pagination (pages, total). * + * @TODO: This model function does return JSON O_O. + * * **response:** * * { diff --git a/core/server/models/post.js b/core/server/models/post.js index 2891165a70..1da961ef94 100644 --- a/core/server/models/post.js +++ b/core/server/models/post.js @@ -563,6 +563,20 @@ Post = ghostBookshelf.Model.extend({ return options; }, + /** + * We have to ensure consistency. If you listen on model events (e.g. `post.published`), you can expect that you always + * receive all fields including relations. Otherwise you can't rely on a consistent flow. And we want to avoid + * that event listeners have to re-fetch a resource. This function is used in the context of inserting + * and updating resources. We won't return the relations by default for now. + */ + defaultRelations: function defaultRelations(methodName, options) { + if (['edit', 'add'].indexOf(methodName) !== -1) { + options.withRelated = _.union(this.prototype.relationships, options.withRelated || []); + } + + return options; + }, + /** * Manually add 'tags' attribute since it's not in the schema and call parent. * @@ -611,7 +625,10 @@ Post = ghostBookshelf.Model.extend({ return ghostBookshelf.Model.edit.call(this, data, options) .then((post) => { - return this.findOne({status: 'all', id: options.id}, options) + return this.findOne({ + status: 'all', + id: options.id + }, _.merge({transacting: options.transacting}, unfilteredOptions)) .then((found) => { if (found) { // Pass along the updated attributes for checking status changes @@ -643,7 +660,10 @@ Post = ghostBookshelf.Model.extend({ const addPost = (() => { return ghostBookshelf.Model.add.call(this, data, options) .then((post) => { - return this.findOne({status: 'all', id: post.id}, options); + return this.findOne({ + status: 'all', + id: post.id + }, _.merge({transacting: options.transacting}, unfilteredOptions)); }); }); diff --git a/core/test/unit/models/post_spec.js b/core/test/unit/models/post_spec.js index da91584fed..8305895219 100644 --- a/core/test/unit/models/post_spec.js +++ b/core/test/unit/models/post_spec.js @@ -1,4 +1,5 @@ 'use strict'; +/* eslint no-invalid-this:0 */ const should = require('should'), // jshint ignore:line sinon = require('sinon'), @@ -6,6 +7,7 @@ const should = require('should'), // jshint ignore:line testUtils = require('../../utils'), knex = require('../../../server/data/db').knex, settingsCache = require('../../../server/services/settings/cache'), + schema = require('../../../server/data/schema'), models = require('../../../server/models'), common = require('../../../server/lib/common'), security = require('../../../server/lib/security'), @@ -56,7 +58,227 @@ describe('Unit: models/post', function () { }); }); - describe('Edit', function () { + describe('add', function () { + describe('ensure full set of data for model events', function () { + it('default', function () { + const events = { + post: [] + }; + + sandbox.stub(models.Post.prototype, 'emitChange').callsFake(function (event) { + events.post.push({event: event, data: this.toJSON()}); + }); + + return models.Post.add({ + title: 'My beautiful title.', + tags: [{ + name: 'my-tag' + }] + }, testUtils.context.editor) + .then((post) => { + post.get('title').should.eql('My beautiful title.'); + post = post.toJSON(); + + _.each(_.keys(_.omit(schema.tables.posts, ['mobiledoc', 'amp', 'plaintext'])), (key) => { + should.exist(post.hasOwnProperty(key)); + }); + + should.not.exist(post.authors); + should.not.exist(post.primary_author); + should.not.exist(post.tags); + should.not.exist(post.primary_tag); + + events.post[0].event.should.eql('added'); + + _.each(_.keys(_.omit(schema.tables.posts, ['mobiledoc', 'amp', 'plaintext'])), (key) => { + should.exist(events.post[0].data.hasOwnProperty(key)); + }); + + should.exist(events.post[0].data.authors); + should.exist(events.post[0].data.primary_author); + should.exist(events.post[0].data.tags); + should.exist(events.post[0].data.primary_tag); + }); + }); + + it('use `withRelated=tags`', function () { + const events = { + post: [] + }; + + sandbox.stub(models.Post.prototype, 'emitChange').callsFake(function (event) { + events.post.push({event: event, data: this.toJSON()}); + }); + + return models.Post.add({ + title: 'My beautiful title.', + tags: [{ + name: 'my-tag' + }] + }, _.merge({ + withRelated: ['tags'] + }, testUtils.context.editor)) + .then((post) => { + post.get('title').should.eql('My beautiful title.'); + post = post.toJSON(); + + _.each(_.keys(_.omit(schema.tables.posts, ['mobiledoc', 'amp', 'plaintext'])), (key) => { + should.exist(post.hasOwnProperty(key)); + }); + + should.not.exist(post.authors); + should.not.exist(post.primary_author); + should.exist(post.tags); + should.exist(post.primary_tag); + + events.post[0].event.should.eql('added'); + + _.each(_.keys(_.omit(schema.tables.posts, ['mobiledoc', 'amp', 'plaintext'])), (key) => { + should.exist(events.post[0].data.hasOwnProperty(key)); + }); + + should.exist(events.post[0].data.authors); + should.exist(events.post[0].data.primary_author); + should.exist(events.post[0].data.tags); + should.exist(events.post[0].data.primary_tag); + }); + }); + + it('use `withRelated=tags,authors`', function () { + const events = { + post: [] + }; + + sandbox.stub(models.Post.prototype, 'emitChange').callsFake(function (event) { + events.post.push({event: event, data: this.toJSON()}); + }); + + return models.Post.add({ + title: 'My beautiful title.', + tags: [{ + name: 'my-tag' + }] + }, _.merge({ + withRelated: ['tags', 'authors'] + }, testUtils.context.editor)) + .then((post) => { + post.get('title').should.eql('My beautiful title.'); + post = post.toJSON(); + + _.each(_.keys(_.omit(schema.tables.posts, ['mobiledoc', 'amp', 'plaintext'])), (key) => { + should.exist(post.hasOwnProperty(key)); + }); + + should.exist(post.authors); + should.exist(post.primary_author); + should.exist(post.tags); + should.exist(post.primary_tag); + + events.post[0].event.should.eql('added'); + + _.each(_.keys(_.omit(schema.tables.posts, ['mobiledoc', 'amp', 'plaintext'])), (key) => { + should.exist(events.post[0].data.hasOwnProperty(key)); + }); + + should.exist(events.post[0].data.authors); + should.exist(events.post[0].data.primary_author); + should.exist(events.post[0].data.tags); + should.exist(events.post[0].data.primary_tag); + }); + }); + + it('use `columns=title`', function () { + const events = { + post: [] + }; + + sandbox.stub(models.Post.prototype, 'emitChange').callsFake(function (event) { + events.post.push({event: event, data: this.toJSON()}); + }); + + return models.Post.add({ + title: 'My beautiful title.', + tags: [{ + name: 'my-tag' + }] + }, _.merge({ + columns: ['title'] + }, testUtils.context.editor)) + .then((post) => { + post.get('title').should.eql('My beautiful title.'); + post = post.toJSON(); + + _.each(_.keys(_.omit(schema.tables.posts, ['title', 'id'])), (key) => { + should.not.exist(post[key]); + }); + + should.exist(post.id); + should.exist(post.title); + + should.not.exist(post.authors); + should.not.exist(post.primary_author); + should.not.exist(post.tags); + should.not.exist(post.primary_tag); + + events.post[0].event.should.eql('added'); + + _.each(_.keys(_.omit(schema.tables.posts, ['mobiledoc', 'amp', 'plaintext'])), (key) => { + should.exist(events.post[0].data.hasOwnProperty(key)); + }); + + should.exist(events.post[0].data.authors); + should.exist(events.post[0].data.primary_author); + should.exist(events.post[0].data.tags); + should.exist(events.post[0].data.primary_tag); + }); + }); + + it('use `formats=mobiledoc`', function () { + const events = { + post: [] + }; + + sandbox.stub(models.Post.prototype, 'emitChange').callsFake(function (event) { + events.post.push({event: event, data: this.toJSON()}); + }); + + return models.Post.add({ + title: 'My beautiful title.', + tags: [{ + name: 'my-tag' + }] + }, _.merge({ + formats: ['mobiledoc'] + }, testUtils.context.editor)) + .then((post) => { + post.get('title').should.eql('My beautiful title.'); + post = post.toJSON(); + + _.each(_.keys(_.omit(schema.tables.posts, ['html', 'amp', 'plaintext'])), (key) => { + should.exist(post.hasOwnProperty(key)); + }); + + should.not.exist(post.authors); + should.not.exist(post.primary_author); + should.not.exist(post.tags); + should.not.exist(post.primary_tag); + + events.post[0].event.should.eql('added'); + + _.each(_.keys(_.omit(schema.tables.posts, ['mobiledoc', 'amp', 'plaintext'])), (key) => { + should.exist(events.post[0].data.hasOwnProperty(key)); + }); + + should.exist(events.post[0].data.authors); + should.exist(events.post[0].data.primary_author); + should.exist(events.post[0].data.tags); + should.exist(events.post[0].data.primary_tag); + }); + }); + }); + }); + + describe('edit', function () { it('update post, relation has not changed', function () { const events = { post: [], @@ -133,6 +355,213 @@ describe('Unit: models/post', function () { post.get('custom_excerpt').should.eql(''); }); }); + + describe('ensure full set of data for model events', function () { + it('default', function () { + const events = { + post: [] + }; + + sandbox.stub(models.Post.prototype, 'emitChange').callsFake(function (event) { + events.post.push({event: event, data: this.toJSON()}); + }); + + return models.Post.edit({ + title: 'My beautiful title.' + }, _.merge({id: testUtils.DataGenerator.forKnex.posts[3].id}, testUtils.context.editor)) + .then((post) => { + post.get('title').should.eql('My beautiful title.'); + post = post.toJSON(); + + _.each(_.keys(_.omit(schema.tables.posts, ['mobiledoc', 'amp', 'plaintext'])), (key) => { + should.exist(post.hasOwnProperty(key)); + }); + + should.not.exist(post.authors); + should.not.exist(post.primary_author); + should.not.exist(post.tags); + should.not.exist(post.primary_tag); + + events.post[0].event.should.eql('edited'); + + _.each(_.keys(_.omit(schema.tables.posts, ['mobiledoc', 'amp', 'plaintext'])), (key) => { + should.exist(events.post[0].data.hasOwnProperty(key)); + }); + + should.exist(events.post[0].data.authors); + should.exist(events.post[0].data.primary_author); + should.exist(events.post[0].data.tags); + should.exist(events.post[0].data.primary_tag); + }); + }); + + it('use `withRelated=tags`', function () { + const events = { + post: [] + }; + + sandbox.stub(models.Post.prototype, 'emitChange').callsFake(function (event) { + events.post.push({event: event, data: this.toJSON()}); + }); + + return models.Post.edit({ + title: 'My beautiful title.' + }, _.merge({ + id: testUtils.DataGenerator.forKnex.posts[3].id, + withRelated: ['tags'] + }, testUtils.context.editor)) + .then((post) => { + post.get('title').should.eql('My beautiful title.'); + post = post.toJSON(); + + _.each(_.keys(_.omit(schema.tables.posts, ['mobiledoc', 'amp', 'plaintext'])), (key) => { + should.exist(post.hasOwnProperty(key)); + }); + + should.not.exist(post.authors); + should.not.exist(post.primary_author); + should.exist(post.tags); + should.exist(post.primary_tag); + + events.post[0].event.should.eql('edited'); + + _.each(_.keys(_.omit(schema.tables.posts, ['mobiledoc', 'amp', 'plaintext'])), (key) => { + should.exist(events.post[0].data.hasOwnProperty(key)); + }); + + should.exist(events.post[0].data.authors); + should.exist(events.post[0].data.primary_author); + should.exist(events.post[0].data.tags); + should.exist(events.post[0].data.primary_tag); + }); + }); + + it('use `withRelated=tags,authors`', function () { + const events = { + post: [] + }; + + sandbox.stub(models.Post.prototype, 'emitChange').callsFake(function (event) { + events.post.push({event: event, data: this.toJSON()}); + }); + + return models.Post.edit({ + title: 'My beautiful title.' + }, _.merge({ + id: testUtils.DataGenerator.forKnex.posts[3].id, + withRelated: ['tags', 'authors'] + }, testUtils.context.editor)) + .then((post) => { + post.get('title').should.eql('My beautiful title.'); + post = post.toJSON(); + + _.each(_.keys(_.omit(schema.tables.posts, ['mobiledoc', 'amp', 'plaintext'])), (key) => { + should.exist(post.hasOwnProperty(key)); + }); + + should.exist(post.authors); + should.exist(post.primary_author); + should.exist(post.tags); + should.exist(post.primary_tag); + + events.post[0].event.should.eql('edited'); + + _.each(_.keys(_.omit(schema.tables.posts, ['mobiledoc', 'amp', 'plaintext'])), (key) => { + should.exist(events.post[0].data.hasOwnProperty(key)); + }); + + should.exist(events.post[0].data.authors); + should.exist(events.post[0].data.primary_author); + should.exist(events.post[0].data.tags); + should.exist(events.post[0].data.primary_tag); + }); + }); + + it('use `columns=title`', function () { + const events = { + post: [] + }; + + sandbox.stub(models.Post.prototype, 'emitChange').callsFake(function (event) { + events.post.push({event: event, data: this.toJSON()}); + }); + + return models.Post.edit({ + title: 'My beautiful title.' + }, _.merge({ + id: testUtils.DataGenerator.forKnex.posts[3].id, + columns: ['title'] + }, testUtils.context.editor)) + .then((post) => { + post.get('title').should.eql('My beautiful title.'); + post = post.toJSON(); + + _.each(_.keys(_.omit(schema.tables.posts, ['title', 'id'])), (key) => { + should.not.exist(post[key]); + }); + + should.exist(post.id); + should.exist(post.title); + + should.not.exist(post.authors); + should.not.exist(post.primary_author); + should.not.exist(post.tags); + should.not.exist(post.primary_tag); + + events.post[0].event.should.eql('edited'); + + _.each(_.keys(_.omit(schema.tables.posts, ['mobiledoc', 'amp', 'plaintext'])), (key) => { + should.exist(events.post[0].data.hasOwnProperty(key)); + }); + + should.exist(events.post[0].data.authors); + should.exist(events.post[0].data.primary_author); + should.exist(events.post[0].data.tags); + should.exist(events.post[0].data.primary_tag); + }); + }); + + it('use `formats=mobiledoc`', function () { + const events = { + post: [] + }; + + sandbox.stub(models.Post.prototype, 'emitChange').callsFake(function (event) { + events.post.push({event: event, data: this.toJSON()}); + }); + + return models.Post.edit({ + title: 'My beautiful title.' + }, _.merge({ + id: testUtils.DataGenerator.forKnex.posts[3].id, + formats: ['mobiledoc'] + }, testUtils.context.editor)) + .then((post) => { + post.get('title').should.eql('My beautiful title.'); + post = post.toJSON(); + + _.each(_.keys(_.omit(schema.tables.posts, ['html', 'amp', 'plaintext'])), (key) => { + should.exist(post.hasOwnProperty(key)); + }); + + should.not.exist(post.authors); + should.not.exist(post.primary_author); + should.not.exist(post.tags); + should.not.exist(post.primary_tag); + + events.post[0].event.should.eql('edited'); + + _.each(_.keys(_.omit(schema.tables.posts, ['mobiledoc', 'amp', 'plaintext'])), (key) => { + should.exist(events.post[0].data.hasOwnProperty(key)); + }); + + should.exist(events.post[0].data.authors); + should.exist(events.post[0].data.primary_author); + should.exist(events.post[0].data.tags); + should.exist(events.post[0].data.primary_tag); + }); + }); + }); }); describe('Relations', function () { diff --git a/core/test/utils/fixtures/data-generator.js b/core/test/utils/fixtures/data-generator.js index ba15b5d852..97f680b14a 100644 --- a/core/test/utils/fixtures/data-generator.js +++ b/core/test/utils/fixtures/data-generator.js @@ -432,6 +432,7 @@ DataGenerator.forKnex = (function () { uuid: uuid.v4(), title: 'title', status: 'published', + feature_image: null, featured: false, page: false, author_id: DataGenerator.Content.users[0].id,