Added getLazyRelation model helper method (#14943)

closes https://github.com/TryGhost/Team/issues/1626

- getLazyRelation is a safer shorthand for `model.related('relationName').fetch()`
- prevents doing a `fetch` operation on a relation that is already loaded, which can cause issues when `formatOnWrite` has a custom implementation
- uses the already loaded relation if it exists, or loads the relation
- doesn't reload if already loaded
- reload is forceable using the forceRefresh option

core/server/models/base/bookshelf.js

@@ -61,6 +61,8 @@ ghostBookshelf.plugin(require('./plugins/data-manipulation'));
 
 ghostBookshelf.plugin(require('./plugins/overrides'));
 
+ghostBookshelf.plugin(require('./plugins/relations'));
+
 // Manages nested updates (relationships)
 ghostBookshelf.plugin('bookshelf-relations', {
     allowedOptions: ['context', 'importing', 'migrating'],

core/server/models/base/plugins/data-manipulation.js

@@ -31,6 +31,7 @@ module.exports = function (Bookshelf) {
 
             _.each(attrs, function each(value, key) {
                 if (value !== null
+                && Object.prototype.hasOwnProperty.call(schema.tables, self.tableName)
                 && Object.prototype.hasOwnProperty.call(schema.tables[self.tableName], key)
                 && schema.tables[self.tableName][key].type === 'dateTime') {
                     attrs[key] = moment(value).format('YYYY-MM-DD HH:mm:ss');

core/server/models/base/plugins/relations.js

@@ -0,0 +1,30 @@
+/**
+ * @param {import('bookshelf')} Bookshelf
+ */
+module.exports = function (Bookshelf) {
+    Bookshelf.Model = Bookshelf.Model.extend({
+        /**
+         * Return a relation, and load it if it hasn't been loaded already (or force a refresh with the forceRefresh option).
+         * refs https://github.com/TryGhost/Team/issues/1626
+         * @param {string} name Name of the relation to load
+         * @param {Object} [options] Options to pass to the fetch when not yet loaded (or when force refreshing) 
+         * @param {boolean} [options.forceRefresh] If true, the relation will be fetched again even if it has already been loaded.
+         * @returns {Promise<import('bookshelf').Model|import('bookshelf').Collection|null>}
+         */
+        getLazyRelation: async function (name, options = {}) {
+            if (this.relations[name] && !options.forceRefresh) {
+                // Relation was already loaded
+                return this.relations[name];
+            }
+
+            if (!this[name]) {
+                return undefined;
+            }
+            // Not yet loaded, or force refresh
+            // Note that we don't use .refresh on the relation on options.forceRefresh
+            // Because the relation can also be a collection, which doesn't have a refresh method
+            this.relations[name] = this[name]();
+            return this.relations[name].fetch(options);
+        }
+    });
+};

core/server/models/post.js

@@ -691,7 +691,7 @@ Post = ghostBookshelf.Model.extend({
         // ensure draft posts have the email_recipient_filter reset unless an email has already been sent
         if (newStatus === 'draft' && this.hasChanged('status')) {
             ops.push(function ensureSendEmailWhenPublishedIsUnchanged() {
-                return self.related('email').fetch({transacting: options.transacting}).then((email) => {
+                return self.getLazyRelation('email', {transacting: options.transacting}).then((email) => {
                     if (!email) {
                         self.set('email_recipient_filter', 'all');
                         self.set('newsletter_id', null);

core/server/services/bulk-email/bulk-email-processor.js

@@ -168,7 +168,7 @@ module.exports = {
 
         try {
             // Load newsletter data on email
-            await emailBatchModel.relations.email.related('newsletter').fetch(Object.assign({}, {require: false}, knexOptions));
+            await emailBatchModel.relations.email.getLazyRelation('newsletter', {require: false, ...knexOptions});
 
             // send the email
             const sendResponse = await this.send(emailBatchModel.relations.email.toJSON(), recipientRows, memberSegment);

core/server/services/mega/email-preview.js

@@ -10,7 +10,7 @@ class EmailPreview {
      * @returns {Promise<Object>}
      */
     async generateEmailContent(post, {newsletter, memberSegment} = {}) {
-        let newsletterModel = post.relations.newsletter ?? await post.related('newsletter').fetch();
+        let newsletterModel = await post.getLazyRelation('newsletter');
         if (!newsletterModel) {
             if (newsletter) {
                 newsletterModel = await models.Newsletter.findOne({slug: newsletter});

core/server/services/mega/mega.js

@@ -51,7 +51,7 @@ const getReplyToAddress = (fromAddress, replyAddressOption) => {
  * @param {Object} options
  */
 const getEmailData = async (postModel, options) => {
-    let newsletter = postModel.relations.newsletter ?? await postModel.related('newsletter').fetch();
+    let newsletter = await postModel.getLazyRelation('newsletter');
     if (!newsletter) {
         // The postModel doesn't have a newsletter in test emails
         newsletter = await models.Newsletter.getDefaultNewsletter();
@@ -194,9 +194,8 @@ const addEmail = async (postModel, options) => {
 
     const knexOptions = _.pick(options, ['transacting', 'forUpdate']);
     const filterOptions = {...knexOptions, limit: 1};
-
-    // TODO: this is a hack for https://github.com/TryGhost/Team/issues/1626
-    const newsletter = postModel.relations.newsletter ?? await postModel.related('newsletter').fetch({require: true, ..._.pick(options, ['transacting'])});
+    const sharedOptions = _.pick(options, ['transacting']);
+    const newsletter = await postModel.getLazyRelation('newsletter', {require: true, ...sharedOptions});
 
     if (newsletter.get('status') !== 'active') {
         // A post might have been scheduled to an archived newsletter.
@@ -360,9 +359,10 @@ async function sendEmailJob({emailModel, options}) {
  */
 async function getEmailMemberRows({emailModel, memberSegment, options}) {
     const knexOptions = _.pick(options, ['transacting', 'forUpdate']);
+    const sharedOptions = _.pick(options, ['transacting']);
     const filterOptions = Object.assign({}, knexOptions);
 
-    const newsletter = emailModel.relations.newsletter ?? await emailModel.related('newsletter').fetch(Object.assign({}, {require: true}, _.pick(options, ['transacting'])));
+    const newsletter = await emailModel.getLazyRelation('newsletter', {require: true, ...sharedOptions});
     const recipientFilter = transformEmailRecipientFilter(newsletter, emailModel.get('recipient_filter'), 'recipient_filter');
     filterOptions.filter = recipientFilter;
 

test/unit/server/models/base/relations.test.js

@@ -0,0 +1,89 @@
+const should = require('should');
+const sinon = require('sinon');
+const models = require('../../../../../core/server/models');
+
+describe('Models: getLazyRelation', function () {
+    before(function () {
+        models.init();
+    });
+
+    afterEach(function () {
+        sinon.restore();
+    });
+
+    it('can fetch collections', async function () {
+        var OtherModel = models.Base.Model.extend({
+            tableName: 'other_models'
+        });
+
+        const TestModel = models.Base.Model.extend({
+            tableName: 'test_models',
+            tiers() {
+                return this.belongsToMany(OtherModel, 'test_others', 'test_id', 'other_id');
+            }
+        });
+        let rel = null;
+        const fetchStub = sinon.stub(models.Base.Collection.prototype, 'fetch').callsFake(function () {
+            if (rel !== null) {
+                throw new Error('Called twice');
+            }
+            rel = this;
+            return this;
+        });
+
+        const options = {test: true};
+        const modelA = TestModel.forge({id: '1'});
+        (await modelA.getLazyRelation('tiers', options)).should.eql(rel);
+        fetchStub.calledOnceWithExactly(options).should.be.true();
+
+        // Check if it can reuse it again
+        (await modelA.getLazyRelation('tiers', options)).should.eql(rel);
+        fetchStub.calledOnceWithExactly(options).should.be.true();
+
+        // Check if we can force reload
+        await should(modelA.getLazyRelation('tiers', {forceRefresh: true})).rejectedWith(/Called twice/);
+        fetchStub.calledTwice.should.be.true();
+    });
+
+    it('can fetch models', async function () {
+        var OtherModel = models.Base.Model.extend({
+            tableName: 'other_models'
+        });
+
+        const TestModel = models.Base.Model.extend({
+            tableName: 'test_models',
+            other() {
+                return this.belongsTo(OtherModel, 'other_id', 'id');
+            }
+        });
+        let rel = null;
+        const fetchStub = sinon.stub(OtherModel.prototype, 'fetch').callsFake(function () {
+            if (rel !== null) {
+                throw new Error('Called twice');
+            }
+            rel = this;
+            return this;
+        });
+
+        const options = {test: true};
+        const modelA = TestModel.forge({id: '1'});
+        (await modelA.getLazyRelation('other', options)).should.eql(rel);
+        fetchStub.calledOnceWithExactly(options).should.be.true();
+
+        // Check if it can reuse it again
+        (await modelA.getLazyRelation('other', options)).should.eql(rel);
+        fetchStub.calledOnceWithExactly(options).should.be.true();
+
+        // Check if we can force reload
+        await should(modelA.getLazyRelation('other', {forceRefresh: true})).rejectedWith(/Called twice/);
+        fetchStub.calledTwice.should.be.true();
+    });
+
+    it('returns undefined for nonexistent relations', async function () {
+        const TestModel = models.Base.Model.extend({
+            tableName: 'test_models'
+        });
+        const modelA = TestModel.forge({id: '1'});
+        should.not.exist(await modelA.getLazyRelation('other'));
+    });
+});

test/unit/server/services/mega/mega.test.js

@@ -17,10 +17,8 @@ describe('MEGA', function () {
             const postModel = {
                 get: sinon.stub().returns('none'),
                 relations: {},
-                related: sinon.stub().returns({
-                    fetch: sinon.stub().returns({
-                        get: sinon.stub().returns('active')
-                    })
+                getLazyRelation: sinon.stub().returns({
+                    get: sinon.stub().returns('active')
                 })
             };
 
@@ -37,10 +35,8 @@ describe('MEGA', function () {
             const postModel = {
                 get: sinon.stub().returns('all'),
                 relations: {},
-                related: sinon.stub().returns({
-                    fetch: sinon.stub().returns({
-                        get: sinon.stub().returns('archived')
-                    })
+                getLazyRelation: sinon.stub().returns({
+                    get: sinon.stub().returns('archived')
                 })
             };
 
@@ -62,10 +58,8 @@ describe('MEGA', function () {
             const postModel = {
                 get: sinon.stub().returns('status:free'),
                 relations: {},
-                related: sinon.stub().returns({
-                    fetch: sinon.stub().returns({
-                        get: newsletterGetter
-                    })
+                getLazyRelation: sinon.stub().returns({
+                    get: newsletterGetter
                 })
             };
             sinon.stub(labs, 'isSet').returns(true);
@@ -109,11 +103,9 @@ describe('MEGA', function () {
             const emailModel = {
                 get: sinon.stub().returns('none'),
                 relations: {},
-                related: sinon.stub().returns({
-                    fetch: sinon.stub().returns({
-                        id: 'test',
-                        newsletterGetter
-                    })
+                getLazyRelation: sinon.stub().returns({
+                    id: 'test',
+                    newsletterGetter
                 })
             };