From 9dde39b2a4b42adc93d48c05b3b584317bd7b460 Mon Sep 17 00:00:00 2001 From: Fabien 'egg' O'Carroll Date: Wed, 13 Sep 2023 14:16:22 +0700 Subject: [PATCH] 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. --- .../BookshelfCollectionsRepository.js | 72 +++++++++++++++++-- .../server/services/collections/service.js | 2 +- .../test/e2e-api/admin/posts-bulk.test.js | 20 +++--- 3 files changed, 78 insertions(+), 16 deletions(-) diff --git a/ghost/core/core/server/services/collections/BookshelfCollectionsRepository.js b/ghost/core/core/server/services/collections/BookshelfCollectionsRepository.js index f62ad52d21..3a05bfccf1 100644 --- a/ghost/core/core/server/services/collections/BookshelfCollectionsRepository.js +++ b/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} */ 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); + } } } }; diff --git a/ghost/core/core/server/services/collections/service.js b/ghost/core/core/server/services/collections/service.js index bcbe488b59..3bdc9faf72 100644 --- a/ghost/core/core/server/services/collections/service.js +++ b/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, diff --git a/ghost/core/test/e2e-api/admin/posts-bulk.test.js b/ghost/core/test/e2e-api/admin/posts-bulk.test.js index 1318d72884..3714dc007e 100644 --- a/ghost/core/test/e2e-api/admin/posts-bulk.test.js +++ b/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 featuredCollectionPostsAmount = featuredCollection.data[0].toJSON().posts.length; + let featuredCollection = await models.Collection.findPage({filter: 'slug:featured', limit: 1, withRelated: ['collectionPosts']}); + 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']}); - featuredCollectionPostsAmount = featuredCollection.data[0].toJSON().posts.length; + featuredCollection = await models.Collection.findPage({filter: 'slug:featured', limit: 1, withRelated: ['collectionPosts']}); + 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 featuredCollectionPostsAmount = featuredCollection.data[0].toJSON().posts.length; + let featuredCollection = await models.Collection.findPage({filter: 'slug:featured', limit: 1, withRelated: ['collectionPosts']}); + 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']}); - featuredCollectionPostsAmount = featuredCollection.data[0].toJSON().posts.length; + featuredCollection = await models.Collection.findPage({filter: 'slug:featured', limit: 1, withRelated: ['collectionPosts']}); + 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']}); - latestCollection = latestCollection.data[0].toJSON().posts.length; + let latestCollection = await models.Collection.findPage({filter: 'slug:latest', limit: 1, withRelated: ['collectionPosts']}); + latestCollection = latestCollection.data[0].toJSON().collectionPosts.length; assert.equal(latestCollection, 0, 'Expect to have no collection posts'); }); });