From 577f08e5f53597c3c4ed65ecc403686b84408fc9 Mon Sep 17 00:00:00 2001 From: Rijk van Zanten Date: Tue, 16 Apr 2024 04:13:33 -0400 Subject: [PATCH] Remove nested transactions (#22023) Co-authored-by: Pascal Jufer Co-authored-by: Brainslug --- .changeset/witty-lamps-worry.md | 5 +++++ api/src/services/collections.ts | 15 ++++++++------- api/src/services/extensions.ts | 3 ++- api/src/services/fields.ts | 7 ++++--- api/src/services/import-export.ts | 7 ++++--- api/src/services/items.ts | 15 ++++++++------- api/src/services/relations.ts | 7 ++++--- api/src/services/roles.ts | 3 ++- api/src/services/users.ts | 3 ++- api/src/utils/apply-diff.ts | 3 ++- api/src/utils/transaction.ts | 16 ++++++++++++++++ 11 files changed, 57 insertions(+), 27 deletions(-) create mode 100644 .changeset/witty-lamps-worry.md create mode 100644 api/src/utils/transaction.ts diff --git a/.changeset/witty-lamps-worry.md b/.changeset/witty-lamps-worry.md new file mode 100644 index 0000000000..42efba6e82 --- /dev/null +++ b/.changeset/witty-lamps-worry.md @@ -0,0 +1,5 @@ +--- +'@directus/api': patch +--- + +Fixed various transaction related issues in CockroachDB by preventing transactions from being nested diff --git a/api/src/services/collections.ts b/api/src/services/collections.ts index 7c8accc4ad..3af7d1ac26 100644 --- a/api/src/services/collections.ts +++ b/api/src/services/collections.ts @@ -2,6 +2,7 @@ import { useEnv } from '@directus/env'; import { ForbiddenError, InvalidPayloadError } from '@directus/errors'; import type { SchemaInspector, Table } from '@directus/schema'; import { createInspector } from '@directus/schema'; +import { systemCollectionRows, type BaseCollectionMeta } from '@directus/system-data'; import type { Accountability, FieldMeta, RawField, SchemaOverview } from '@directus/types'; import { addFieldFlag } from '@directus/utils'; import type Keyv from 'keyv'; @@ -16,9 +17,9 @@ import emitter from '../emitter.js'; import type { AbstractServiceOptions, ActionEventParams, Collection, MutationOptions } from '../types/index.js'; import { getSchema } from '../utils/get-schema.js'; import { shouldClearCache } from '../utils/should-clear-cache.js'; +import { transaction } from '../utils/transaction.js'; import { FieldsService } from './fields.js'; import { ItemsService } from './items.js'; -import { systemCollectionRows, type BaseCollectionMeta } from '@directus/system-data'; export type RawCollection = { collection: string; @@ -78,7 +79,7 @@ export class CollectionsService { // Create the collection/fields in a transaction so it'll be reverted in case of errors or // permission problems. This might not work reliably in MySQL, as it doesn't support DDL in // transactions. - await this.knex.transaction(async (trx) => { + await transaction(this.knex, async (trx) => { if (payload.schema) { // Directus heavily relies on the primary key of a collection, so we have to make sure that // every collection that is created has a primary key. If no primary key field is created @@ -219,7 +220,7 @@ export class CollectionsService { const nestedActionEvents: ActionEventParams[] = []; try { - const collections = await this.knex.transaction(async (trx) => { + const collections = await transaction(this.knex, async (trx) => { const service = new CollectionsService({ schema: this.schema, accountability: this.accountability, @@ -466,7 +467,7 @@ export class CollectionsService { const nestedActionEvents: ActionEventParams[] = []; try { - await this.knex.transaction(async (trx) => { + await transaction(this.knex, async (trx) => { const collectionItemsService = new CollectionsService({ knex: trx, accountability: this.accountability, @@ -521,7 +522,7 @@ export class CollectionsService { const nestedActionEvents: ActionEventParams[] = []; try { - await this.knex.transaction(async (trx) => { + await transaction(this.knex, async (trx) => { const service = new CollectionsService({ schema: this.schema, accountability: this.accountability, @@ -578,7 +579,7 @@ export class CollectionsService { throw new ForbiddenError(); } - await this.knex.transaction(async (trx) => { + await transaction(this.knex, async (trx) => { if (collectionToBeDeleted!.schema) { await trx.schema.dropTable(collectionKey); } @@ -705,7 +706,7 @@ export class CollectionsService { const nestedActionEvents: ActionEventParams[] = []; try { - await this.knex.transaction(async (trx) => { + await transaction(this.knex, async (trx) => { const service = new CollectionsService({ schema: this.schema, accountability: this.accountability, diff --git a/api/src/services/extensions.ts b/api/src/services/extensions.ts index 665ed3426c..257b6ffdf2 100644 --- a/api/src/services/extensions.ts +++ b/api/src/services/extensions.ts @@ -9,6 +9,7 @@ import getDatabase from '../database/index.js'; import { getExtensionManager } from '../extensions/index.js'; import type { ExtensionManager } from '../extensions/manager.js'; import type { AbstractServiceOptions } from '../types/index.js'; +import { transaction } from '../utils/transaction.js'; import { ItemsService } from './items.js'; export class ExtensionReadError extends Error { @@ -195,7 +196,7 @@ export class ExtensionsService { } async updateOne(id: string, data: DeepPartial) { - const result = await this.knex.transaction(async (trx) => { + const result = await transaction(this.knex, async (trx) => { if (!isObject(data.meta)) { throw new InvalidPayloadError({ reason: `"meta" is required` }); } diff --git a/api/src/services/fields.ts b/api/src/services/fields.ts index 74e55d66ee..2720e4f12e 100644 --- a/api/src/services/fields.ts +++ b/api/src/services/fields.ts @@ -21,6 +21,7 @@ import getLocalType from '../utils/get-local-type.js'; import { getSchema } from '../utils/get-schema.js'; import { sanitizeColumn } from '../utils/sanitize-schema.js'; import { shouldClearCache } from '../utils/should-clear-cache.js'; +import { transaction } from '../utils/transaction.js'; import { ItemsService } from './items.js'; import { PayloadService } from './payload.js'; import { RelationsService } from './relations.js'; @@ -282,7 +283,7 @@ export class FieldsService { addFieldFlag(field, flagToAdd); } - await this.knex.transaction(async (trx) => { + await transaction(this.knex, async (trx) => { const itemsService = new ItemsService('directus_fields', { knex: trx, accountability: this.accountability, @@ -436,7 +437,7 @@ export class FieldsService { if (!isEqual(columnToCompare, hookAdjustedField.schema)) { try { - await this.knex.transaction(async (trx) => { + await transaction(this.knex, async (trx) => { await trx.schema.alterTable(collection, async (table) => { if (!hookAdjustedField.schema) return; this.addColumnToTable(table, field, existingColumn); @@ -577,7 +578,7 @@ export class FieldsService { ); } - await this.knex.transaction(async (trx) => { + await transaction(this.knex, async (trx) => { const relations = this.schema.relations.filter((relation) => { return ( (relation.collection === collection && relation.field === field) || diff --git a/api/src/services/import-export.ts b/api/src/services/import-export.ts index 6444fdfa08..fa42ccbeb0 100644 --- a/api/src/services/import-export.ts +++ b/api/src/services/import-export.ts @@ -25,6 +25,7 @@ import emitter from '../emitter.js'; import { useLogger } from '../logger.js'; import type { AbstractServiceOptions, ActionEventParams } from '../types/index.js'; import { getDateFormatted } from '../utils/get-date-formatted.js'; +import { transaction } from '../utils/transaction.js'; import { Url } from '../utils/url.js'; import { userName } from '../utils/user-name.js'; import { FilesService } from './files.js'; @@ -78,7 +79,7 @@ export class ImportService { const extractJSON = StreamArray.withParser(); const nestedActionEvents: ActionEventParams[] = []; - return this.knex.transaction((trx) => { + return transaction(this.knex, (trx) => { const service = new ItemsService(collection, { knex: trx, schema: this.schema, @@ -126,7 +127,7 @@ export class ImportService { const nestedActionEvents: ActionEventParams[] = []; - return this.knex.transaction((trx) => { + return transaction(this.knex, (trx) => { const service = new ItemsService(collection, { knex: trx, schema: this.schema, @@ -274,7 +275,7 @@ export class ExportService { const database = getDatabase(); - await database.transaction(async (trx) => { + await transaction(database, async (trx) => { const service = new ItemsService(collection, { accountability: this.accountability, schema: this.schema, diff --git a/api/src/services/items.ts b/api/src/services/items.ts index a0eb7e6ea7..660d660095 100644 --- a/api/src/services/items.ts +++ b/api/src/services/items.ts @@ -22,6 +22,7 @@ import emitter from '../emitter.js'; import type { AbstractService, AbstractServiceOptions, ActionEventParams, MutationOptions } from '../types/index.js'; import getASTFromQuery from '../utils/get-ast-from-query.js'; import { shouldClearCache } from '../utils/should-clear-cache.js'; +import { transaction } from '../utils/transaction.js'; import { validateKeys } from '../utils/validate-keys.js'; import { AuthorizationService } from './authorization.js'; import { PayloadService } from './payload.js'; @@ -119,7 +120,7 @@ export class ItemsService implements AbstractSer // changes in the DB if any of the parts contained within throws an error. This also means // that any errors thrown in any nested relational changes will bubble up and cancel the whole // update tree - const primaryKey: PrimaryKey = await this.knex.transaction(async (trx) => { + const primaryKey: PrimaryKey = await transaction(this.knex, async (trx) => { // We're creating new services instances so they can use the transaction as their Knex interface const payloadService = new PayloadService(this.collection, { accountability: this.accountability, @@ -344,11 +345,11 @@ export class ItemsService implements AbstractSer async createMany(data: Partial[], opts: MutationOptions = {}): Promise { if (!opts.mutationTracker) opts.mutationTracker = this.createMutationTracker(); - const { primaryKeys, nestedActionEvents } = await this.knex.transaction(async (trx) => { + const { primaryKeys, nestedActionEvents } = await transaction(this.knex, async (knex) => { const service = new ItemsService(this.collection, { accountability: this.accountability, schema: this.schema, - knex: trx, + knex: knex, }); const primaryKeys: PrimaryKey[] = []; @@ -560,7 +561,7 @@ export class ItemsService implements AbstractSer const keys: PrimaryKey[] = []; try { - await this.knex.transaction(async (trx) => { + await transaction(this.knex, async (trx) => { const service = new ItemsService(this.collection, { accountability: this.accountability, knex: trx, @@ -649,7 +650,7 @@ export class ItemsService implements AbstractSer throw opts.preMutationError; } - await this.knex.transaction(async (trx) => { + await transaction(this.knex, async (trx) => { const payloadService = new PayloadService(this.collection, { accountability: this.accountability, knex: trx, @@ -836,7 +837,7 @@ export class ItemsService implements AbstractSer async upsertMany(payloads: Partial[], opts: MutationOptions = {}): Promise { if (!opts.mutationTracker) opts.mutationTracker = this.createMutationTracker(); - const primaryKeys = await this.knex.transaction(async (trx) => { + const primaryKeys = await transaction(this.knex, async (trx) => { const service = new ItemsService(this.collection, { accountability: this.accountability, schema: this.schema, @@ -927,7 +928,7 @@ export class ItemsService implements AbstractSer ); } - await this.knex.transaction(async (trx) => { + await transaction(this.knex, async (trx) => { await trx(this.collection).whereIn(primaryKeyField, keys).delete(); if (this.accountability && this.schema.collections[this.collection]!.accountability !== null) { diff --git a/api/src/services/relations.ts b/api/src/services/relations.ts index 0b05c487a6..b7483d3dfd 100644 --- a/api/src/services/relations.ts +++ b/api/src/services/relations.ts @@ -14,6 +14,7 @@ import emitter from '../emitter.js'; import type { AbstractServiceOptions, ActionEventParams, MutationOptions } from '../types/index.js'; import { getDefaultIndexName } from '../utils/get-default-index-name.js'; import { getSchema } from '../utils/get-schema.js'; +import { transaction } from '../utils/transaction.js'; import { ItemsService, type QueryOptions } from './items.js'; import { PermissionsService } from './permissions/index.js'; @@ -191,7 +192,7 @@ export class RelationsService { one_collection: relation.related_collection || null, }; - await this.knex.transaction(async (trx) => { + await transaction(this.knex, async (trx) => { if (relation.related_collection) { await trx.schema.alterTable(relation.collection!, async (table) => { this.alterType(table, relation, fieldSchema.nullable); @@ -290,7 +291,7 @@ export class RelationsService { const nestedActionEvents: ActionEventParams[] = []; try { - await this.knex.transaction(async (trx) => { + await transaction(this.knex, async (trx) => { if (existingRelation.related_collection) { await trx.schema.alterTable(collection, async (table) => { let constraintName: string = getDefaultIndexName('foreign', collection, field); @@ -404,7 +405,7 @@ export class RelationsService { const nestedActionEvents: ActionEventParams[] = []; try { - await this.knex.transaction(async (trx) => { + await transaction(this.knex, async (trx) => { const existingConstraints = await this.schemaInspector.foreignKeys(); const constraintNames = existingConstraints.map((key) => key.constraint_name); diff --git a/api/src/services/roles.ts b/api/src/services/roles.ts index fe7053daf7..11383a368f 100644 --- a/api/src/services/roles.ts +++ b/api/src/services/roles.ts @@ -2,6 +2,7 @@ import { ForbiddenError, InvalidPayloadError, UnprocessableContentError } from ' import type { Alterations, Item, PrimaryKey, Query, User } from '@directus/types'; import { getMatch } from 'ip-matching'; import type { AbstractServiceOptions, MutationOptions } from '../types/index.js'; +import { transaction } from '../utils/transaction.js'; import { ItemsService } from './items.js'; import { PermissionsService } from './permissions/index.js'; import { PresetsService } from './presets.js'; @@ -263,7 +264,7 @@ export class RolesService extends ItemsService { opts.preMutationError = err; } - await this.knex.transaction(async (trx) => { + await transaction(this.knex, async (trx) => { const itemsService = new ItemsService('directus_roles', { knex: trx, accountability: this.accountability, diff --git a/api/src/services/users.ts b/api/src/services/users.ts index 8b0ad09b58..eef2b542bb 100644 --- a/api/src/services/users.ts +++ b/api/src/services/users.ts @@ -13,6 +13,7 @@ import type { AbstractServiceOptions, MutationOptions } from '../types/index.js' import isUrlAllowed from '../utils/is-url-allowed.js'; import { verifyJWT } from '../utils/jwt.js'; import { stall } from '../utils/stall.js'; +import { transaction } from '../utils/transaction.js'; import { Url } from '../utils/url.js'; import { ItemsService } from './items.js'; import { MailService } from './mail/index.js'; @@ -239,7 +240,7 @@ export class UsersService extends ItemsService { const keys: PrimaryKey[] = []; - await this.knex.transaction(async (trx) => { + await transaction(this.knex, async (trx) => { const service = new UsersService({ accountability: this.accountability, knex: trx, diff --git a/api/src/utils/apply-diff.ts b/api/src/utils/apply-diff.ts index eeb6a9a4d4..d67685b6da 100644 --- a/api/src/utils/apply-diff.ts +++ b/api/src/utils/apply-diff.ts @@ -20,6 +20,7 @@ import type { SnapshotField, } from '../types/index.js'; import { DiffKind } from '../types/index.js'; +import { transaction } from '../utils/transaction.js'; import { getSchema } from './get-schema.js'; type CollectionDelta = { @@ -48,7 +49,7 @@ export async function applyDiff( const runPostColumnChange = await helpers.schema.preColumnChange(); - await database.transaction(async (trx) => { + await transaction(database, async (trx) => { const collectionsService = new CollectionsService({ knex: trx, schema }); const getNestedCollectionsToCreate = (currentLevelCollection: string) => diff --git a/api/src/utils/transaction.ts b/api/src/utils/transaction.ts new file mode 100644 index 0000000000..61e0783996 --- /dev/null +++ b/api/src/utils/transaction.ts @@ -0,0 +1,16 @@ +import type { Knex } from 'knex'; + +/** + * Execute the given handler within the current transaction or a newly created one + * if the current knex state isn't a transaction yet. + * + * Can be used to ensure the handler is run within a transaction, + * while preventing nested transactions. + */ +export const transaction = (knex: Knex, handler: (knex: Knex) => Promise): Promise => { + if (knex.isTransaction) { + return handler(knex); + } else { + return knex.transaction((trx) => handler(trx)); + } +};