Wrapped collection's post adding logic in transaction

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

- Having transactional collection post updates makes sure there are no race conditions when updating collection_posts relations. Without the transactions collection was prone to update relations based on a stale state causing problems like described in the linked issue
This commit is contained in:
Naz 2023-07-19 21:30:16 +08:00 committed by naz
parent 04c92d2ca5
commit 0880770d50
4 changed files with 47 additions and 30 deletions

View File

@ -1,8 +1,9 @@
import {Collection} from './Collection'; import {Collection} from './Collection';
export interface CollectionRepository { export interface CollectionRepository {
save(collection: Collection): Promise<void> createTransaction(fn: (transaction: any) => Promise<any>): Promise<any>
getById(id: string): Promise<Collection | null> save(collection: Collection, options?: {transaction: any}): Promise<void>
getById(id: string, options?: {transaction: any}): Promise<Collection | null>
getBySlug(slug: string): Promise<Collection | null> getBySlug(slug: string): Promise<Collection | null>
getAll(options?: any): Promise<Collection[]> getAll(options?: any): Promise<Collection[]>
} }

View File

@ -6,6 +6,10 @@ export class CollectionsRepositoryInMemory extends InMemoryRepository<string, Co
return entity.toJSON(); return entity.toJSON();
} }
createTransaction(cb: (transaction: any) => Promise<any>): Promise<any> {
return cb(null);
}
async getBySlug(slug: string): Promise<Collection | null> { async getBySlug(slug: string): Promise<Collection | null> {
return this.store.find(item => item.slug === slug) || null; return this.store.find(item => item.slug === slug) || null;
} }

View File

@ -92,6 +92,7 @@ type QueryOptions = {
include?: string; include?: string;
page?: number; page?: number;
limit?: number; limit?: number;
transaction?: any;
} }
interface PostsRepository { interface PostsRepository {
@ -253,32 +254,22 @@ export class CollectionsService {
} }
private async addPostToMatchingCollections(post: CollectionPost) { private async addPostToMatchingCollections(post: CollectionPost) {
const collections = await this.collectionsRepository.getAll({ return await this.collectionsRepository.createTransaction(async (transaction) => {
filter: 'type:automatic' const collections = await this.collectionsRepository.getAll({
}); filter: 'type:automatic',
transaction: transaction
});
for (const collection of collections) { for (const collection of collections) {
const added = await collection.addPost(post); const added = await collection.addPost(post);
if (added) { if (added) {
await this.collectionsRepository.save(collection); await this.collectionsRepository.save(collection, {
transaction: transaction
});
}
} }
}
}
/**
* @description Updates all automatic collections. Can be time intensive and is a temporary solution
* while all of the events are mapped out and handled optimally
*/
async updateCollections() {
const collections = await this.collectionsRepository.getAll({
filter: 'type:automatic'
}); });
for (const collection of collections) {
await this.updateAutomaticCollectionItems(collection);
await this.collectionsRepository.save(collection);
}
} }
async updatePostInMatchingCollections(postEdit: PostEditedEvent['data']) { async updatePostInMatchingCollections(postEdit: PostEditedEvent['data']) {

View File

@ -12,6 +12,10 @@ module.exports = class BookshelfCollectionsRepository {
this.#model = model; this.#model = model;
} }
async createTransaction(cb) {
return this.#model.transaction(cb);
}
/** /**
* @param {string} id * @param {string} id
* @returns {Promise<Collection>} * @returns {Promise<Collection>}
@ -40,9 +44,15 @@ module.exports = class BookshelfCollectionsRepository {
* @param {object} [options] * @param {object} [options]
* @param {string} [options.filter] * @param {string} [options.filter]
* @param {string} [options.order] * @param {string} [options.order]
* @param {import('knex').Transaction} [options.transaction]
*/ */
async getAll(options = {}) { async getAll(options = {}) {
const models = await this.#model.findAll({...options, withRelated: ['posts']}); const models = await this.#model.findAll({
...options,
transacting: options.transaction,
withRelated: ['posts']
});
return await Promise.all(models.map(model => this.#modelToCollection(model))); return await Promise.all(models.map(model => this.#modelToCollection(model)));
} }
@ -65,9 +75,11 @@ module.exports = class BookshelfCollectionsRepository {
/** /**
* @param {Collection} collection * @param {Collection} collection
* @param {object} [options]
* @param {import('knex').Transaction} [options.transaction]
* @returns {Promise<void>} * @returns {Promise<void>}
*/ */
async save(collection) { async save(collection, options = {}) {
if (collection.deleted) { if (collection.deleted) {
await this.#model.destroy({id: collection.id}); await this.#model.destroy({id: collection.id});
return; return;
@ -85,13 +97,22 @@ module.exports = class BookshelfCollectionsRepository {
updated_at: collection.updatedAt updated_at: collection.updatedAt
}; };
const existing = await this.#model.findOne({id: data.id}, {require: false}); const existing = await this.#model.findOne(
{id: data.id},
{
require: false,
transacting: options.transaction
}
);
if (!existing) { if (!existing) {
await this.#model.add(data); await this.#model.add(data, {
transacting: options.transaction
});
} else { } else {
await this.#model.edit(data, { return this.#model.edit(data, {
id: data.id id: data.id,
transacting: options.transaction
}); });
} }
} }