diff --git a/ghost/core/core/server/api/endpoints/collections.js b/ghost/core/core/server/api/endpoints/collections.js index 92e6ffb06b..2bf31facbe 100644 --- a/ghost/core/core/server/api/endpoints/collections.js +++ b/ghost/core/core/server/api/endpoints/collections.js @@ -29,7 +29,11 @@ module.exports = { }, permissions: true, query(frame) { - return collectionsService.api.getAll(frame.options); + return collectionsService.api.getAll({ + filter: frame.options.filter, + limit: frame.options.limit, + page: frame.options.page + }); } }, diff --git a/ghost/core/core/server/services/collections/BookshelfCollectionsRepository.js b/ghost/core/core/server/services/collections/BookshelfCollectionsRepository.js index 3a05bfccf1..855226d756 100644 --- a/ghost/core/core/server/services/collections/BookshelfCollectionsRepository.js +++ b/ghost/core/core/server/services/collections/BookshelfCollectionsRepository.js @@ -28,12 +28,14 @@ module.exports = class BookshelfCollectionsRepository { async getById(id, options = {}) { const model = await this.#model.findOne({id}, { require: false, - withRelated: ['collectionPosts'], transacting: options.transaction }); if (!model) { return null; } + + model.collectionPostIds = await this.#fetchCollectionPostIds(model.id, options); + return this.#modelToCollection(model); } @@ -44,28 +46,63 @@ module.exports = class BookshelfCollectionsRepository { async getBySlug(slug, options = {}) { const model = await this.#model.findOne({slug}, { require: false, - withRelated: ['collectionPosts'], transacting: options.transaction }); + if (!model) { return null; } + + model.collectionPostIds = await this.#fetchCollectionPostIds(model.id, options); + return this.#modelToCollection(model); } + /** + * NOTE: we are only fetching post_id column here to save memory on + * instances with a large amount of posts + * + * The method could be further optimized to fetch posts for + * multiple collections at a time. + * + * @param {string} collectionId collection to fetch post ids for + * @param {Object} options bookshelf options + * + * @returns {Promise>} + */ + async #fetchCollectionPostIds(collectionId, options = {}) { + const toSelect = options.columns || ['post_id']; + + const query = this.#relationModel.query(); + if (options.transaction) { + query.transacting(options.transaction); + } + + return await query + .select(toSelect) + .where('collection_id', collectionId); + } + /** * @param {object} [options] * @param {string} [options.filter] * @param {string} [options.order] + * @param {string[]} [options.withRelated] * @param {import('knex').Transaction} [options.transaction] */ async getAll(options = {}) { const models = await this.#model.findAll({ ...options, - transacting: options.transaction, - withRelated: ['collectionPosts'] + transacting: options.transaction }); + for (const model of models) { + // NOTE: Ideally collection posts would be fetching as a part of findAll query. + // Because bookshelf introduced a massive processing and memory overhead + // we are fetching collection post ids separately using raw knex query + model.collectionPostIds = await this.#fetchCollectionPostIds(model.id, options); + } + return (await Promise.all(models.map(model => this.#modelToCollection(model)))).filter(entity => !!entity); } @@ -78,6 +115,10 @@ module.exports = class BookshelfCollectionsRepository { } try { + // NOTE: collectionPosts are not a part of serialized model + // and are fetched separately to save memory + const posts = model.collectionPostIds; + return await Collection.create({ id: json.id, slug: json.slug, @@ -86,7 +127,7 @@ module.exports = class BookshelfCollectionsRepository { filter: filter, type: json.type, featureImage: json.feature_image, - posts: json.collectionPosts.map(collectionPost => collectionPost.post_id), + posts: posts.map(collectionPost => collectionPost.post_id), createdAt: json.created_at, updatedAt: json.updated_at }); @@ -135,8 +176,7 @@ module.exports = class BookshelfCollectionsRepository { {id: data.id}, { require: false, - transacting: options.transaction, - withRelated: ['collectionPosts'] + transacting: options.transaction } ); @@ -175,7 +215,11 @@ module.exports = class BookshelfCollectionsRepository { if (collection.type === 'manual') { await this.#relationModel.query().delete().where('collection_id', collection.id).transacting(options.transaction); } else { - const existingRelations = existing.toJSON().collectionPosts; + const collectionPostsOptions = { + transaction: options.transaction, + columns: ['id', 'post_id'] + }; + const existingRelations = await this.#fetchCollectionPostIds(collection.id, collectionPostsOptions); for (const existingRelation of existingRelations) { const found = collectionPostsRelations.find((thing) => { diff --git a/ghost/core/test/e2e-api/admin/collections.test.js b/ghost/core/test/e2e-api/admin/collections.test.js index 917832e154..96d362a171 100644 --- a/ghost/core/test/e2e-api/admin/collections.test.js +++ b/ghost/core/test/e2e-api/admin/collections.test.js @@ -117,7 +117,7 @@ describe('Collections API', function () { }); }, this.skip.bind(this)); const collectionRelatedQueries = queries.filter(query => query.sql.includes('collection')); - assert(collectionRelatedQueries.length === 2); + assert(collectionRelatedQueries.length === 3); }); it('Can browse Collections and include the posts count', async function () { @@ -572,7 +572,7 @@ describe('Collections API', function () { }, this.skip.bind(this)); const collectionRelatedQueries = queries.filter(query => query.sql.includes('collection')); - assert.equal(collectionRelatedQueries.length, 8); + assert.equal(collectionRelatedQueries.length, 13); } await agent @@ -604,7 +604,7 @@ describe('Collections API', function () { }, this.skip.bind(this)); const collectionRelatedQueries = queries.filter(query => query.sql.includes('collection')); - assert.equal(collectionRelatedQueries.length, 14); + assert.equal(collectionRelatedQueries.length, 18); } await agent