From aea0513517937ddc0c49f8cae7cb3a1f943c2367 Mon Sep 17 00:00:00 2001 From: Will Schurman Date: Thu, 28 May 2026 09:36:32 -0700 Subject: [PATCH] fix: require transactional query context for createOrGetExistingAsync utility method --- .../EntityCreationUtils-test.ts | 51 -- .../entity/src/utils/EntityCreationUtils.ts | 34 +- .../__tests__/EntityCreationUtils-test.ts | 442 +++++++----------- 3 files changed, 179 insertions(+), 348 deletions(-) diff --git a/packages/entity-database-adapter-knex/src/__integration-tests__/EntityCreationUtils-test.ts b/packages/entity-database-adapter-knex/src/__integration-tests__/EntityCreationUtils-test.ts index da69003d4e..242ea45ead 100644 --- a/packages/entity-database-adapter-knex/src/__integration-tests__/EntityCreationUtils-test.ts +++ b/packages/entity-database-adapter-knex/src/__integration-tests__/EntityCreationUtils-test.ts @@ -33,57 +33,6 @@ describe(createWithUniqueConstraintRecoveryAsync, () => { }); describe.each([true, false])('is parallel creations %p', (parallel) => { - it('recovers when the same entity is created twice outside of transaction', async () => { - const vc1 = new ViewerContext(createKnexIntegrationTestEntityCompanionProvider(knexInstance)); - - const args = { - name: 'unique', - }; - - let createdEntities: [PostgresUniqueTestEntity, PostgresUniqueTestEntity]; - if (parallel) { - createdEntities = await Promise.all([ - createWithUniqueConstraintRecoveryAsync( - vc1, - PostgresUniqueTestEntity, - PostgresUniqueTestEntity.getByNameAsync, - args, - PostgresUniqueTestEntity.createWithNameAsync, - args, - ), - createWithUniqueConstraintRecoveryAsync( - vc1, - PostgresUniqueTestEntity, - PostgresUniqueTestEntity.getByNameAsync, - args, - PostgresUniqueTestEntity.createWithNameAsync, - args, - ), - ]); - } else { - createdEntities = [ - await createWithUniqueConstraintRecoveryAsync( - vc1, - PostgresUniqueTestEntity, - PostgresUniqueTestEntity.getByNameAsync, - args, - PostgresUniqueTestEntity.createWithNameAsync, - args, - ), - await createWithUniqueConstraintRecoveryAsync( - vc1, - PostgresUniqueTestEntity, - PostgresUniqueTestEntity.getByNameAsync, - args, - PostgresUniqueTestEntity.createWithNameAsync, - args, - ), - ]; - } - - expect(createdEntities[0].getID()).toEqual(createdEntities[1].getID()); - }); - it('recovers when the same entity is created twice within same transaction', async () => { const vc1 = new ViewerContext(createKnexIntegrationTestEntityCompanionProvider(knexInstance)); diff --git a/packages/entity/src/utils/EntityCreationUtils.ts b/packages/entity/src/utils/EntityCreationUtils.ts index a25b95bca3..ef9e098f42 100644 --- a/packages/entity/src/utils/EntityCreationUtils.ts +++ b/packages/entity/src/utils/EntityCreationUtils.ts @@ -46,25 +46,18 @@ export async function createOrGetExistingAsync< queryContext?: EntityTransactionalQueryContext, ) => Promise, createArgs: TCreateArgs, - queryContext?: EntityTransactionalQueryContext, + queryContext: EntityTransactionalQueryContext, ): Promise { - if (!queryContext) { - const maybeEntity = await getAsync(viewerContext, getArgs); - if (maybeEntity) { - return maybeEntity; - } - } else { - // This is done in a nested transaction since entity may negatively cache load results per-transaction (when configured). - // Without it, it would - // 1. load the entity in the current query context, negatively cache it - // 2. then try to create it in the nested transaction, which may fail due to a unique constraint error - // 3. then try to load the entity again in the current query context, which would return null due to negative cache - const maybeEntity = await queryContext.runInNestedTransactionAsync((nestedQueryContext) => - getAsync(viewerContext, getArgs, nestedQueryContext), - ); - if (maybeEntity) { - return maybeEntity; - } + // This is done in a nested transaction since entity may negatively cache load results per-transaction (when configured). + // Without it, it would + // 1. load the entity in the current query context, negatively cache it + // 2. then try to create it in the nested transaction, which may fail due to a unique constraint error + // 3. then try to load the entity again in the current query context, which would return null due to negative cache + const maybeEntity = await queryContext.runInNestedTransactionAsync((nestedQueryContext) => + getAsync(viewerContext, getArgs, nestedQueryContext), + ); + if (maybeEntity) { + return maybeEntity; } return await createWithUniqueConstraintRecoveryAsync( viewerContext, @@ -118,12 +111,9 @@ export async function createWithUniqueConstraintRecoveryAsync< queryContext?: EntityTransactionalQueryContext, ) => Promise, createArgs: TCreateArgs, - queryContext?: EntityTransactionalQueryContext, + queryContext: EntityTransactionalQueryContext, ): Promise { try { - if (!queryContext) { - return await createAsync(viewerContext, createArgs); - } return await queryContext.runInNestedTransactionAsync((nestedQueryContext) => createAsync(viewerContext, createArgs, nestedQueryContext), ); diff --git a/packages/entity/src/utils/__tests__/EntityCreationUtils-test.ts b/packages/entity/src/utils/__tests__/EntityCreationUtils-test.ts index a59c249936..be66aaf2da 100644 --- a/packages/entity/src/utils/__tests__/EntityCreationUtils-test.ts +++ b/packages/entity/src/utils/__tests__/EntityCreationUtils-test.ts @@ -13,48 +13,30 @@ import { createUnitTestEntityCompanionProvider } from '../__testfixtures__/creat type TArgs = object; -describe.each([true, false])('in transaction %p', (inTransaction) => { - describe(createOrGetExistingAsync, () => { - it('does not create when already exists', async () => { - const companionProvider = createUnitTestEntityCompanionProvider(); - const viewerContext = new ViewerContext(companionProvider); - - const entity = await SimpleTestEntity.creator(viewerContext).createAsync(); - - const args: TArgs = {}; - - const getFn = jest.fn( - async ( - _vc: ViewerContext, - _args: TArgs, - _queryContext?: EntityTransactionalQueryContext, - ) => { - return entity; - }, - ); - - const createFn = jest.fn( - async (vc: ViewerContext, _args: TArgs, queryContext?: EntityTransactionalQueryContext) => { - return await SimpleTestEntity.creator(vc, queryContext).createAsync(); - }, - ); - - if (inTransaction) { - await viewerContext.runInTransactionForDatabaseAdapterFlavorAsync( - 'postgres', - async (queryContext) => { - await createOrGetExistingAsync( - viewerContext, - SimpleTestEntity, - getFn, - args, - createFn, - args, - queryContext, - ); - }, - ); - } else { +describe(createOrGetExistingAsync, () => { + it('does not create when already exists', async () => { + const companionProvider = createUnitTestEntityCompanionProvider(); + const viewerContext = new ViewerContext(companionProvider); + + const entity = await SimpleTestEntity.creator(viewerContext).createAsync(); + + const args: TArgs = {}; + + const getFn = jest.fn( + async (_vc: ViewerContext, _args: TArgs, _queryContext?: EntityTransactionalQueryContext) => { + return entity; + }, + ); + + const createFn = jest.fn( + async (vc: ViewerContext, _args: TArgs, queryContext?: EntityTransactionalQueryContext) => { + return await SimpleTestEntity.creator(vc, queryContext).createAsync(); + }, + ); + + await viewerContext.runInTransactionForDatabaseAdapterFlavorAsync( + 'postgres', + async (queryContext) => { await createOrGetExistingAsync( viewerContext, SimpleTestEntity, @@ -62,51 +44,36 @@ describe.each([true, false])('in transaction %p', (inTransaction) => { args, createFn, args, + queryContext, ); - } + }, + ); - expect(getFn).toHaveBeenCalledTimes(1); - expect(createFn).toHaveBeenCalledTimes(0); - }); + expect(getFn).toHaveBeenCalledTimes(1); + expect(createFn).toHaveBeenCalledTimes(0); + }); - it('creates when not found', async () => { - const companionProvider = createUnitTestEntityCompanionProvider(); - const viewerContext = new ViewerContext(companionProvider); + it('creates when not found', async () => { + const companionProvider = createUnitTestEntityCompanionProvider(); + const viewerContext = new ViewerContext(companionProvider); - const args: TArgs = {}; + const args: TArgs = {}; - const getFn = jest.fn( - async ( - _vc: ViewerContext, - _args: TArgs, - _queryContext?: EntityTransactionalQueryContext, - ) => { - return null; - }, - ); + const getFn = jest.fn( + async (_vc: ViewerContext, _args: TArgs, _queryContext?: EntityTransactionalQueryContext) => { + return null; + }, + ); - const createFn = jest.fn( - async (vc: ViewerContext, _args: TArgs, queryContext?: EntityTransactionalQueryContext) => { - return await SimpleTestEntity.creator(vc, queryContext).createAsync(); - }, - ); - - if (inTransaction) { - await viewerContext.runInTransactionForDatabaseAdapterFlavorAsync( - 'postgres', - async (queryContext) => { - await createOrGetExistingAsync( - viewerContext, - SimpleTestEntity, - getFn, - args, - createFn, - args, - queryContext, - ); - }, - ); - } else { + const createFn = jest.fn( + async (vc: ViewerContext, _args: TArgs, queryContext?: EntityTransactionalQueryContext) => { + return await SimpleTestEntity.creator(vc, queryContext).createAsync(); + }, + ); + + await viewerContext.runInTransactionForDatabaseAdapterFlavorAsync( + 'postgres', + async (queryContext) => { await createOrGetExistingAsync( viewerContext, SimpleTestEntity, @@ -114,53 +81,38 @@ describe.each([true, false])('in transaction %p', (inTransaction) => { args, createFn, args, + queryContext, ); - } + }, + ); - expect(getFn).toHaveBeenCalledTimes(1); - expect(createFn).toHaveBeenCalledTimes(1); - }); + expect(getFn).toHaveBeenCalledTimes(1); + expect(createFn).toHaveBeenCalledTimes(1); }); +}); - describe(createWithUniqueConstraintRecoveryAsync, () => { - it('does not call get when creation succeeds', async () => { - const companionProvider = createUnitTestEntityCompanionProvider(); - const viewerContext = new ViewerContext(companionProvider); +describe(createWithUniqueConstraintRecoveryAsync, () => { + it('does not call get when creation succeeds', async () => { + const companionProvider = createUnitTestEntityCompanionProvider(); + const viewerContext = new ViewerContext(companionProvider); - const args: TArgs = {}; + const args: TArgs = {}; - const getFn = jest.fn( - async ( - _vc: ViewerContext, - _args: TArgs, - _queryContext?: EntityTransactionalQueryContext, - ) => { - return null; - }, - ); + const getFn = jest.fn( + async (_vc: ViewerContext, _args: TArgs, _queryContext?: EntityTransactionalQueryContext) => { + return null; + }, + ); - const createFn = jest.fn( - async (vc: ViewerContext, _args: TArgs, queryContext?: EntityTransactionalQueryContext) => { - return await SimpleTestEntity.creator(vc, queryContext).createAsync(); - }, - ); - - if (inTransaction) { - await viewerContext.runInTransactionForDatabaseAdapterFlavorAsync( - 'postgres', - async (queryContext) => { - await createWithUniqueConstraintRecoveryAsync( - viewerContext, - SimpleTestEntity, - getFn, - args, - createFn, - args, - queryContext, - ); - }, - ); - } else { + const createFn = jest.fn( + async (vc: ViewerContext, _args: TArgs, queryContext?: EntityTransactionalQueryContext) => { + return await SimpleTestEntity.creator(vc, queryContext).createAsync(); + }, + ); + + await viewerContext.runInTransactionForDatabaseAdapterFlavorAsync( + 'postgres', + async (queryContext) => { await createWithUniqueConstraintRecoveryAsync( viewerContext, SimpleTestEntity, @@ -168,57 +120,38 @@ describe.each([true, false])('in transaction %p', (inTransaction) => { args, createFn, args, + queryContext, ); - } + }, + ); + + expect(getFn).toHaveBeenCalledTimes(0); + expect(createFn).toHaveBeenCalledTimes(1); + }); - expect(getFn).toHaveBeenCalledTimes(0); - expect(createFn).toHaveBeenCalledTimes(1); - }); + it('calls get when database adapter throws EntityDatabaseAdapterUniqueConstraintError', async () => { + const companionProvider = createUnitTestEntityCompanionProvider(); + const viewerContext = new ViewerContext(companionProvider); - it('calls get when database adapter throws EntityDatabaseAdapterUniqueConstraintError', async () => { - const companionProvider = createUnitTestEntityCompanionProvider(); - const viewerContext = new ViewerContext(companionProvider); + const entity = await SimpleTestEntity.creator(viewerContext).createAsync(); - const entity = await SimpleTestEntity.creator(viewerContext).createAsync(); + const args: TArgs = {}; - const args: TArgs = {}; + const getFn = jest.fn( + async (_vc: ViewerContext, _args: TArgs, _queryContext?: EntityTransactionalQueryContext) => { + return entity; + }, + ); - const getFn = jest.fn( - async ( - _vc: ViewerContext, - _args: TArgs, - _queryContext?: EntityTransactionalQueryContext, - ) => { - return entity; - }, - ); - - const createFn = jest.fn( - async ( - _vc: ViewerContext, - _args: TArgs, - _queryContext?: EntityTransactionalQueryContext, - ) => { - throw new EntityDatabaseAdapterUniqueConstraintError('wat'); - }, - ); - - if (inTransaction) { - await viewerContext.runInTransactionForDatabaseAdapterFlavorAsync( - 'postgres', - async (queryContext) => { - await createWithUniqueConstraintRecoveryAsync( - viewerContext, - SimpleTestEntity, - getFn, - args, - createFn, - args, - queryContext, - ); - }, - ); - } else { + const createFn = jest.fn( + async (_vc: ViewerContext, _args: TArgs, _queryContext?: EntityTransactionalQueryContext) => { + throw new EntityDatabaseAdapterUniqueConstraintError('wat'); + }, + ); + + await viewerContext.runInTransactionForDatabaseAdapterFlavorAsync( + 'postgres', + async (queryContext) => { await createWithUniqueConstraintRecoveryAsync( viewerContext, SimpleTestEntity, @@ -226,131 +159,90 @@ describe.each([true, false])('in transaction %p', (inTransaction) => { args, createFn, args, + queryContext, ); - } - - expect(getFn).toHaveBeenCalledTimes(1); - expect(createFn).toHaveBeenCalledTimes(1); - }); + }, + ); - it('throws an EntityNotFoundError when database adapter throws EntityDatabaseAdapterUniqueConstraintError and getFn returns null', async () => { - const companionProvider = createUnitTestEntityCompanionProvider(); - const viewerContext = new ViewerContext(companionProvider); - - const args: TArgs = {}; + expect(getFn).toHaveBeenCalledTimes(1); + expect(createFn).toHaveBeenCalledTimes(1); + }); - const getFn = jest.fn( - async ( - _vc: ViewerContext, - _args: TArgs, - _queryContext?: EntityTransactionalQueryContext, - ) => { - return null; - }, - ); - - const createFn = jest.fn( - async ( - _vc: ViewerContext, - _args: TArgs, - _queryContext?: EntityTransactionalQueryContext, - ) => { - throw new EntityDatabaseAdapterUniqueConstraintError('wat'); - }, - ); - - if (inTransaction) { - await expect( - viewerContext.runInTransactionForDatabaseAdapterFlavorAsync( - 'postgres', - async (queryContext) => { - return await createWithUniqueConstraintRecoveryAsync( - viewerContext, - SimpleTestEntity, - getFn, - args, - createFn, - args, - queryContext, - ); - }, - ), - ).rejects.toThrow(EntityNotFoundError); - } else { - await expect( - createWithUniqueConstraintRecoveryAsync( + it('throws an EntityNotFoundError when database adapter throws EntityDatabaseAdapterUniqueConstraintError and getFn returns null', async () => { + const companionProvider = createUnitTestEntityCompanionProvider(); + const viewerContext = new ViewerContext(companionProvider); + + const args: TArgs = {}; + + const getFn = jest.fn( + async (_vc: ViewerContext, _args: TArgs, _queryContext?: EntityTransactionalQueryContext) => { + return null; + }, + ); + + const createFn = jest.fn( + async (_vc: ViewerContext, _args: TArgs, _queryContext?: EntityTransactionalQueryContext) => { + throw new EntityDatabaseAdapterUniqueConstraintError('wat'); + }, + ); + + await expect( + viewerContext.runInTransactionForDatabaseAdapterFlavorAsync( + 'postgres', + async (queryContext) => { + return await createWithUniqueConstraintRecoveryAsync( viewerContext, SimpleTestEntity, getFn, args, createFn, args, - ), - ).rejects.toThrow(EntityNotFoundError); - } - - expect(getFn).toHaveBeenCalledTimes(1); - expect(createFn).toHaveBeenCalledTimes(1); - }); - - it('rethrows whatever error is thrown from database adapter if not EntityDatabaseAdapterUniqueConstraintError', async () => { - const companionProvider = createUnitTestEntityCompanionProvider(); - const viewerContext = new ViewerContext(companionProvider); - - const args: TArgs = {}; - - const getFn = jest.fn( - async ( - _vc: ViewerContext, - _args: TArgs, - _queryContext?: EntityTransactionalQueryContext, - ) => { - return null; + queryContext, + ); }, - ); - - const createFn = jest.fn( - async ( - _vc: ViewerContext, - _args: TArgs, - _queryContext?: EntityTransactionalQueryContext, - ) => { - throw new Error('wat'); - }, - ); - - if (inTransaction) { - await expect( - viewerContext.runInTransactionForDatabaseAdapterFlavorAsync( - 'postgres', - async (queryContext) => { - return await createWithUniqueConstraintRecoveryAsync( - viewerContext, - SimpleTestEntity, - getFn, - args, - createFn, - args, - queryContext, - ); - }, - ), - ).rejects.toThrow('wat'); - } else { - await expect( - createWithUniqueConstraintRecoveryAsync( + ), + ).rejects.toThrow(EntityNotFoundError); + + expect(getFn).toHaveBeenCalledTimes(1); + expect(createFn).toHaveBeenCalledTimes(1); + }); + + it('rethrows whatever error is thrown from database adapter if not EntityDatabaseAdapterUniqueConstraintError', async () => { + const companionProvider = createUnitTestEntityCompanionProvider(); + const viewerContext = new ViewerContext(companionProvider); + + const args: TArgs = {}; + + const getFn = jest.fn( + async (_vc: ViewerContext, _args: TArgs, _queryContext?: EntityTransactionalQueryContext) => { + return null; + }, + ); + + const createFn = jest.fn( + async (_vc: ViewerContext, _args: TArgs, _queryContext?: EntityTransactionalQueryContext) => { + throw new Error('wat'); + }, + ); + + await expect( + viewerContext.runInTransactionForDatabaseAdapterFlavorAsync( + 'postgres', + async (queryContext) => { + return await createWithUniqueConstraintRecoveryAsync( viewerContext, SimpleTestEntity, getFn, args, createFn, args, - ), - ).rejects.toThrow('wat'); - } + queryContext, + ); + }, + ), + ).rejects.toThrow('wat'); - expect(getFn).toHaveBeenCalledTimes(0); - expect(createFn).toHaveBeenCalledTimes(1); - }); + expect(getFn).toHaveBeenCalledTimes(0); + expect(createFn).toHaveBeenCalledTimes(1); }); });