From 914955e9d74352105e4f9ecfee789664f97bcb1c Mon Sep 17 00:00:00 2001 From: sighphyre Date: Thu, 20 Oct 2022 12:11:29 +0200 Subject: [PATCH] add methods for checking if a store is expecting to be in transactional mode --- src/lib/db/group-store.ts | 20 ++-- src/lib/db/transactional.ts | 14 +++ src/lib/services/group-service.ts | 34 ++++--- .../fixtures/fake-feature-toggle-store.ts | 8 ++ src/test/transactional.test.ts | 96 +++++++++++++++++++ 5 files changed, 145 insertions(+), 27 deletions(-) diff --git a/src/lib/db/group-store.ts b/src/lib/db/group-store.ts index 1a7995b2ba..1ddaef52db 100644 --- a/src/lib/db/group-store.ts +++ b/src/lib/db/group-store.ts @@ -9,8 +9,7 @@ import Group, { IGroupUser, IGroupUserModel, } from '../types/group'; -import Transaction = Knex.Transaction; -import { Transactor } from './transactional'; +import { expectTransaction, Transactor } from './transactional'; const T = { GROUPS: 'groups', @@ -178,7 +177,6 @@ export default class GroupStore groupId: number, users: IGroupUserModel[], userName: string, - transaction?: Transaction, ): Promise { const rows = users.map((user) => { return { @@ -187,14 +185,11 @@ export default class GroupStore created_by: userName, }; }); - return (transaction || this.db).batchInsert(T.GROUP_USER, rows); + return this.db.batchInsert(T.GROUP_USER, rows); } - async deleteUsersFromGroup( - deletableUsers: IGroupUser[], - transaction?: Transaction, - ): Promise { - return (transaction || this.db)(T.GROUP_USER) + async deleteUsersFromGroup(deletableUsers: IGroupUser[]): Promise { + return this.db(T.GROUP_USER) .whereIn( ['group_id', 'user_id'], deletableUsers.map((user) => [user.groupId, user.userId]), @@ -209,10 +204,9 @@ export default class GroupStore deletableUsers: IGroupUser[], userName: string, ): Promise { - await this.db.transaction(async (tx) => { - await this.addUsersToGroup(groupId, newUsers, userName, tx); - await this.deleteUsersFromGroup(deletableUsers, tx); - }); + expectTransaction(this.db); + await this.addUsersToGroup(groupId, newUsers, userName); + await this.deleteUsersFromGroup(deletableUsers); } async getNewGroupsForExternalUser( diff --git a/src/lib/db/transactional.ts b/src/lib/db/transactional.ts index 864855f22c..7d8e3fab93 100644 --- a/src/lib/db/transactional.ts +++ b/src/lib/db/transactional.ts @@ -20,3 +20,17 @@ export abstract class Transactor implements Transactional { return clone as T; } } + +export const expectTransaction = (db: Knex | Knex.Transaction): void => { + if (db.isTransaction) { + return; + } + const isRunningInTest = process.env.NODE_ENV === 'test'; + const errorMessage = + 'A store method that was expected to be run in a transaction was run outside of a transaction'; + if (isRunningInTest) { + throw new Error(errorMessage); + } else { + console.error(errorMessage); + } +}; diff --git a/src/lib/services/group-service.ts b/src/lib/services/group-service.ts index 6287a64b56..8fcc11a54d 100644 --- a/src/lib/services/group-service.ts +++ b/src/lib/services/group-service.ts @@ -15,6 +15,7 @@ import { IEventStore } from '../types/stores/event-store'; import NameExistsError from '../error/name-exists-error'; import { IUserStore } from '../types/stores/user-store'; import { IUser } from '../types/user'; +import { Knex } from 'knex'; export class GroupService { private groupStore: IGroupStore; @@ -25,14 +26,17 @@ export class GroupService { private logger: Logger; + private db: Knex; + constructor( stores: Pick, - { getLogger }: Pick, + { getLogger }: Pick, // db: Knex, ) { this.logger = getLogger('service/group-service.js'); this.groupStore = stores.groupStore; this.eventStore = stores.eventStore; this.userStore = stores.userStore; + // this.db = db; } async getAll(): Promise { @@ -118,19 +122,21 @@ export class GroupService { ); const deletableUserIds = deletableUsers.map((g) => g.userId); - await this.groupStore.updateGroupUsers( - newGroup.id, - group.users.filter( - (user) => !existingUserIds.includes(user.user.id), - ), - group.users.filter( - (user) => - existingUserIds.includes(user.user.id) && - !deletableUserIds.includes(user.user.id), - ), - deletableUsers, - userName, - ); + this.db.transaction(async (tx) => { + await this.groupStore.transactional(tx).updateGroupUsers( + newGroup.id, + group.users.filter( + (user) => !existingUserIds.includes(user.user.id), + ), + group.users.filter( + (user) => + existingUserIds.includes(user.user.id) && + !deletableUserIds.includes(user.user.id), + ), + deletableUsers, + userName, + ); + }); await this.eventStore.store({ type: GROUP_UPDATED, diff --git a/src/test/fixtures/fake-feature-toggle-store.ts b/src/test/fixtures/fake-feature-toggle-store.ts index 412269ee6a..c3a7f288b9 100644 --- a/src/test/fixtures/fake-feature-toggle-store.ts +++ b/src/test/fixtures/fake-feature-toggle-store.ts @@ -4,6 +4,7 @@ import { } from '../../lib/types/stores/feature-toggle-store'; import NotFoundError from '../../lib/error/notfound-error'; import { FeatureToggle, FeatureToggleDTO, IVariant } from 'lib/types/model'; +import { Knex } from 'knex'; export default class FakeFeatureToggleStore implements IFeatureToggleStore { features: FeatureToggle[] = []; @@ -135,4 +136,11 @@ export default class FakeFeatureToggleStore implements IFeatureToggleStore { feature.variants = newVariants; return feature; } + + transactional( + /* eslint-disable-next-line */ + transaction: Knex.Transaction, + ): IFeatureToggleStore { + throw new Error('Method not implemented.'); + } } diff --git a/src/test/transactional.test.ts b/src/test/transactional.test.ts index 5b5a8d9b98..9bad637e8a 100644 --- a/src/test/transactional.test.ts +++ b/src/test/transactional.test.ts @@ -29,6 +29,102 @@ test('should actually do something transactional mode', async () => { expect(createdGroup).toBeDefined(); }); +test('should fail if we run a method that requires transactional outside of a transaction', async () => { + const newUser = await stores.userStore.insert({ + name: 'Tyler Durden', + }); + const oldUser = await stores.userStore.insert({ + name: 'Bob Poulson', + }); + + const group = await stores.groupStore.create({ + name: 'TestGroup', + }); + + await expect(async () => + stores.groupStore.updateGroupUsers( + group.id, + [ + { + user: newUser, + }, + ], + [], + [ + { + groupId: group.id, + userId: oldUser.id, + }, + ], + 'David Fincher', + ), + ).rejects.toThrow(Error); +}); + +test('should not fail if we run a method that requires transactional outside of a transaction in prod mode', async () => { + const newUser = await stores.userStore.insert({ + name: 'Tyler Durden', + }); + const oldUser = await stores.userStore.insert({ + name: 'Bob Poulson', + }); + + const group = await stores.groupStore.create({ + name: 'TestGroup', + }); + + process.env.NODE_ENV = 'prod'; + + await stores.groupStore.updateGroupUsers( + group.id, + [ + { + user: newUser, + }, + ], + [], + [ + { + groupId: group.id, + userId: oldUser.id, + }, + ], + 'David Fincher', + ); +}); + +test('should not fail if we run a method that requires transactional inside of a transaction in test mode', async () => { + const newUser = await stores.userStore.insert({ + name: 'Tyler Durden', + }); + const oldUser = await stores.userStore.insert({ + name: 'Bob Poulson', + }); + + const group = await stores.groupStore.create({ + name: 'TestGroup', + }); + + await db.db.transaction(async (trx) => { + await stores.groupStore.transactional(trx).updateGroupUsers( + group.id, + [ + { + user: newUser, + }, + ], + [], + [ + { + groupId: group.id, + userId: oldUser.id, + }, + ], + 'David Fincher', + ); + }); +}); + test('should fail entire transaction if encountering an error', async () => { await db.db.transaction(async (trx) => { const featureDTO = {