Handled CollectionPost relations manually (#18081)

refs https://github.com/TryGhost/Arch/issues/86

bookshelf-relations was generating tonnes of select queries from the
posts table in order to update the relations. We've instead implemented
this ourselves, so as to avoid the superfluous fetches. Working closer to
the db like this is nice, and makes you think more about performance.

This logic could be pulled out into a util (not bookshelf plugin) where
it could be used explicitly, but with the complexity hidden, we'll see ig.

ghost/core/core/server/services/collections/BookshelfCollectionsRepository.js

@@ -1,6 +1,7 @@
 const logger = require('@tryghost/logging');
 const Collection = require('@tryghost/collections').Collection;
 const sentry = require('../../../shared/sentry');
+const {default: ObjectID} = require('bson-objectid');
 /**
  * @typedef {import('@tryghost/collections/src/CollectionRepository')} CollectionRepository
  */
@@ -10,8 +11,10 @@ const sentry = require('../../../shared/sentry');
  */
 module.exports = class BookshelfCollectionsRepository {
     #model;
-    constructor(model) {
+    #relationModel;
+    constructor(model, relationModel) {
         this.#model = model;
+        this.#relationModel = relationModel;
     }
 
     async createTransaction(cb) {
@@ -101,10 +104,21 @@ module.exports = class BookshelfCollectionsRepository {
      * @returns {Promise<void>}
      */
     async save(collection, options = {}) {
+        if (!options.transaction) {
+            return this.createTransaction((transaction) => {
+                return this.save(collection, {
+                    ...options,
+                    transaction
+                });
+            });
+        }
+
         if (collection.deleted) {
-            await this.#model.destroy({id: collection.id});
+            await this.#relationModel.query().delete().where('collection_id', collection.id).transacting(options.transaction);
+            await this.#model.query().delete().where('id', collection.id).transacting(options.transaction);
             return;
         }
+
         const data = {
             id: collection.id,
             slug: collection.slug,
@@ -113,7 +127,6 @@ module.exports = class BookshelfCollectionsRepository {
             filter: collection.filter,
             type: collection.type,
             feature_image: collection.featureImage || null,
-            posts: collection.posts.map(postId => ({id: postId})),
             created_at: collection.createdAt,
             updated_at: collection.updatedAt
         };
@@ -122,7 +135,8 @@ module.exports = class BookshelfCollectionsRepository {
             {id: data.id},
             {
                 require: false,
-                transacting: options.transaction
+                transacting: options.transaction,
+                withRelated: ['collectionPosts']
             }
         );
 
@@ -130,11 +144,59 @@ module.exports = class BookshelfCollectionsRepository {
             await this.#model.add(data, {
                 transacting: options.transaction
             });
+            const collectionPostsRelations = collection.posts.map((postId, index) => {
+                return {
+                    id: (new ObjectID).toHexString(),
+                    sort_order: collection.type === 'manual' ? index : 0,
+                    collection_id: collection.id,
+                    post_id: postId
+                };
+            });
+            if (collectionPostsRelations.length > 0) {
+                await this.#relationModel.query().insert(collectionPostsRelations).transacting(options.transaction);
+            }
         } else {
-            return this.#model.edit(data, {
+            await this.#model.edit(data, {
                 id: data.id,
                 transacting: options.transaction
             });
+
+            const collectionPostsRelations = collection.posts.map((postId, index) => {
+                return {
+                    id: (new ObjectID).toHexString(),
+                    sort_order: collection.type === 'manual' ? index : 0,
+                    collection_id: collection.id,
+                    post_id: postId
+                };
+            });
+
+            const collectionPostRelationsToDeleteIds = [];
+
+            if (collection.type === 'manual') {
+                await this.#relationModel.query().delete().where('collection_id', collection.id).transacting(options.transaction);
+            } else {
+                const existingRelations = existing.toJSON().collectionPosts;
+
+                for (const existingRelation of existingRelations) {
+                    const found = collectionPostsRelations.find((thing) => {
+                        return thing.post_id === existingRelation.post_id;
+                    });
+                    if (found) {
+                        found.id = null;
+                    } else {
+                        collectionPostRelationsToDeleteIds.push(existingRelation.id);
+                    }
+                }
+            }
+
+            const missingCollectionPostsRelations = collectionPostsRelations.filter(thing => thing.id !== null);
+
+            if (missingCollectionPostsRelations.length > 0) {
+                await this.#relationModel.query().insert(missingCollectionPostsRelations).transacting(options.transaction);
+            }
+            if (collectionPostRelationsToDeleteIds.length > 0) {
+                await this.#relationModel.query().delete().whereIn('id', collectionPostRelationsToDeleteIds).transacting(options.transaction);
+            }
         }
     }
 };

ghost/core/core/server/services/collections/service.js

@@ -12,7 +12,7 @@ class CollectionsServiceWrapper {
         const DomainEvents = require('@tryghost/domain-events');
         const postsRepository = require('./PostsRepository').getInstance();
         const models = require('../../models');
-        const collectionsRepositoryInMemory = new BookshelfCollectionsRepository(models.Collection);
+        const collectionsRepositoryInMemory = new BookshelfCollectionsRepository(models.Collection, models.CollectionPost);
 
         const collectionsService = new CollectionsService({
             collectionsRepository: collectionsRepositoryInMemory,

ghost/core/test/e2e-api/admin/posts-bulk.test.js

@@ -30,8 +30,8 @@ describe('Posts Bulk API', function () {
 
             assert(amount > 0, 'Expect at least one post to be affected for this test to work');
 
-            let featuredCollection = await models.Collection.findPage({filter: 'slug:featured', limit: 1, withRelated: ['posts']});
+            let featuredCollection = await models.Collection.findPage({filter: 'slug:featured', limit: 1, withRelated: ['collectionPosts']});
-            let featuredCollectionPostsAmount = featuredCollection.data[0].toJSON().posts.length;
+            let featuredCollectionPostsAmount = featuredCollection.data[0].toJSON().collectionPosts.length;
             assert(featuredCollectionPostsAmount > 0, 'Expect to have multiple featured collection posts');
 
             const response = await agent
@@ -52,8 +52,8 @@ describe('Posts Bulk API', function () {
             const posts = await models.Post.findAll({filter, status: 'all'});
             assert.equal(posts.length, amount, `Expect all matching posts (${amount}) to be changed`);
 
-            featuredCollection = await models.Collection.findPage({filter: 'slug:featured', limit: 1, withRelated: ['posts']});
+            featuredCollection = await models.Collection.findPage({filter: 'slug:featured', limit: 1, withRelated: ['collectionPosts']});
-            featuredCollectionPostsAmount = featuredCollection.data[0].toJSON().posts.length;
+            featuredCollectionPostsAmount = featuredCollection.data[0].toJSON().collectionPosts.length;
             assert.equal(featuredCollectionPostsAmount, amount, 'Expect to have same amount featured collection posts as changed');
 
             for (const post of posts) {
@@ -70,8 +70,8 @@ describe('Posts Bulk API', function () {
 
             assert(amount > 0, 'Expect at least one post to be affected for this test to work');
 
-            let featuredCollection = await models.Collection.findPage({filter: 'slug:featured', limit: 1, withRelated: ['posts']});
+            let featuredCollection = await models.Collection.findPage({filter: 'slug:featured', limit: 1, withRelated: ['collectionPosts']});
-            let featuredCollectionPostsAmount = featuredCollection.data[0].toJSON().posts.length;
+            let featuredCollectionPostsAmount = featuredCollection.data[0].toJSON().collectionPosts.length;
             assert(featuredCollectionPostsAmount > 0, 'Expect to have multiple featured collection posts');
 
             const response = await agent
@@ -92,8 +92,8 @@ describe('Posts Bulk API', function () {
             const posts = await models.Post.findAll({filter, status: 'all'});
             assert.equal(posts.length, amount, `Expect all matching posts (${amount}) to be changed`);
 
-            featuredCollection = await models.Collection.findPage({filter: 'slug:featured', limit: 1, withRelated: ['posts']});
+            featuredCollection = await models.Collection.findPage({filter: 'slug:featured', limit: 1, withRelated: ['collectionPosts']});
-            featuredCollectionPostsAmount = featuredCollection.data[0].toJSON().posts.length;
+            featuredCollectionPostsAmount = featuredCollection.data[0].toJSON().collectionPosts.length;
             assert.equal(featuredCollectionPostsAmount, 0, 'Expect to have no featured collection posts');
 
             for (const post of posts) {
@@ -354,8 +354,8 @@ describe('Posts Bulk API', function () {
             const posts = await models.Post.findPage({filter, status: 'all'});
             assert.equal(posts.meta.pagination.total, 0, `Expect all matching posts (${amount}) to be deleted`);
 
-            let latestCollection = await models.Collection.findPage({filter: 'slug:latest', limit: 1, withRelated: ['posts']});
+            let latestCollection = await models.Collection.findPage({filter: 'slug:latest', limit: 1, withRelated: ['collectionPosts']});
-            latestCollection = latestCollection.data[0].toJSON().posts.length;
+            latestCollection = latestCollection.data[0].toJSON().collectionPosts.length;
             assert.equal(latestCollection, 0, 'Expect to have no collection posts');
         });
     });