fix: synchronous transaction errors (#4164)

Co-authored-by: Dan Ribbens <dan.ribbens@gmail.com>

packages/payload/src/auth/operations/access.ts

@@ -2,7 +2,10 @@ import type { PayloadRequest } from '../../express/types'
 import type { AllOperations } from '../../types'
 import type { Permissions } from '../types'
 
+import { commitTransaction } from '../../utilities/commitTransaction'
 import { getEntityPolicies } from '../../utilities/getEntityPolicies'
+import { initTransaction } from '../../utilities/initTransaction'
+import { killTransaction } from '../../utilities/killTransaction'
 import { adminInit as adminInitTelemetry } from '../../utilities/telemetry/events/adminInit'
 
 const allOperations: AllOperations[] = ['create', 'read', 'update', 'delete']
@@ -38,57 +41,64 @@ async function accessOperation(args: Arguments): Promise<Permissions> {
     results.canAccessAdmin = false
   }
 
-  await Promise.all(
+  try {
-    config.collections.map(async (collection) => {
+    const shouldCommit = await initTransaction(req)
-      const collectionOperations = [...allOperations]
+    await Promise.all(
+      config.collections.map(async (collection) => {
+        const collectionOperations = [...allOperations]
 
-      if (
+        if (
-        collection.auth &&
+          collection.auth &&
-        typeof collection.auth.maxLoginAttempts !== 'undefined' &&
+          typeof collection.auth.maxLoginAttempts !== 'undefined' &&
-        collection.auth.maxLoginAttempts !== 0
+          collection.auth.maxLoginAttempts !== 0
-      ) {
+        ) {
-        collectionOperations.push('unlock')
+          collectionOperations.push('unlock')
-      }
+        }
 
-      if (collection.versions) {
+        if (collection.versions) {
-        collectionOperations.push('readVersions')
+          collectionOperations.push('readVersions')
-      }
+        }
 
-      const collectionPolicy = await getEntityPolicies({
+        const collectionPolicy = await getEntityPolicies({
-        entity: collection,
+          entity: collection,
-        operations: collectionOperations,
+          operations: collectionOperations,
-        req,
+          req,
-        type: 'collection',
+          type: 'collection',
-      })
+        })
-      results.collections = {
+        results.collections = {
-        ...results.collections,
+          ...results.collections,
-        [collection.slug]: collectionPolicy,
+          [collection.slug]: collectionPolicy,
-      }
+        }
-    }),
+      }),
-  )
+    )
 
-  await Promise.all(
+    await Promise.all(
-    config.globals.map(async (global) => {
+      config.globals.map(async (global) => {
-      const globalOperations: AllOperations[] = ['read', 'update']
+        const globalOperations: AllOperations[] = ['read', 'update']
 
-      if (global.versions) {
+        if (global.versions) {
-        globalOperations.push('readVersions')
+          globalOperations.push('readVersions')
-      }
+        }
 
-      const globalPolicy = await getEntityPolicies({
+        const globalPolicy = await getEntityPolicies({
-        entity: global,
+          entity: global,
-        operations: globalOperations,
+          operations: globalOperations,
-        req,
+          req,
-        type: 'global',
+          type: 'global',
-      })
+        })
-      results.globals = {
+        results.globals = {
-        ...results.globals,
+          ...results.globals,
-        [global.slug]: globalPolicy,
+          [global.slug]: globalPolicy,
-      }
+        }
-    }),
+      }),
-  )
+    )
 
-  return results
+    if (shouldCommit) await commitTransaction(req)
+    return results
+  } catch (e: unknown) {
+    await killTransaction(req)
+    throw e
+  }
 }
 
 export default accessOperation

packages/payload/src/collections/operations/docAccess.ts

@@ -2,7 +2,10 @@ import type { CollectionPermission } from '../../auth'
 import type { PayloadRequest } from '../../express/types'
 import type { AllOperations } from '../../types'
 
+import { commitTransaction } from '../../utilities/commitTransaction'
 import { getEntityPolicies } from '../../utilities/getEntityPolicies'
+import { initTransaction } from '../../utilities/initTransaction'
+import { killTransaction } from '../../utilities/killTransaction'
 
 const allOperations: AllOperations[] = ['create', 'read', 'update', 'delete']
 
@@ -34,11 +37,22 @@ export async function docAccess(args: Arguments): Promise<CollectionPermission>
     collectionOperations.push('readVersions')
   }
 
-  return getEntityPolicies({
+  try {
-    id,
+    const shouldCommit = await initTransaction(req)
-    entity: config,
+
-    operations: collectionOperations,
+    const result = await getEntityPolicies({
-    req,
+      id,
-    type: 'collection',
+      entity: config,
-  })
+      operations: collectionOperations,
+      req,
+      type: 'collection',
+    })
+
+    if (shouldCommit) await commitTransaction(req)
+
+    return result
+  } catch (e: unknown) {
+    await killTransaction(req)
+    throw e
+  }
 }

packages/payload/src/express/types.ts

@@ -59,6 +59,10 @@ export declare type PayloadRequest<U = any> = Request & {
    * Identifier for the database transaction for interactions in a single, all-or-nothing operation.
    */
   transactionID?: number | string
+  /**
+   * Used to ensure consistency when multiple operations try to create a transaction concurrently on the same request
+   */
+  transactionIDPromise?: Promise<void>
   /** The signed in user */
   user: (U & User) | null
 }

packages/payload/src/globals/operations/docAccess.ts

@@ -3,7 +3,10 @@ import type { PayloadRequest } from '../../express/types'
 import type { AllOperations } from '../../types'
 import type { SanitizedGlobalConfig } from '../config/types'
 
+import { commitTransaction } from '../../utilities/commitTransaction'
 import { getEntityPolicies } from '../../utilities/getEntityPolicies'
+import { initTransaction } from '../../utilities/initTransaction'
+import { killTransaction } from '../../utilities/killTransaction'
 
 type Arguments = {
   globalConfig: SanitizedGlobalConfig
@@ -19,10 +22,18 @@ export async function docAccess(args: Arguments): Promise<GlobalPermission> {
     globalOperations.push('readVersions')
   }
 
-  return getEntityPolicies({
+  try {
-    entity: globalConfig,
+    const shouldCommit = await initTransaction(req)
-    operations: globalOperations,
+    const result = await getEntityPolicies({
-    req,
+      entity: globalConfig,
-    type: 'global',
+      operations: globalOperations,
-  })
+      req,
+      type: 'global',
+    })
+    if (shouldCommit) await commitTransaction(req)
+    return result
+  } catch (e: unknown) {
+    await killTransaction(req)
+    throw e
+  }
 }

packages/payload/src/utilities/initTransaction.ts

@@ -5,12 +5,24 @@ import type { PayloadRequest } from '../express/types'
  * @returns true if beginning a transaction and false when req already has a transaction to use
  */
 export async function initTransaction(req: PayloadRequest): Promise<boolean> {
-  const { payload, transactionID } = req
+  const { payload, transactionID, transactionIDPromise } = req
-  if (!transactionID && typeof payload.db.beginTransaction === 'function') {
+  if (transactionID) {
-    req.transactionID = await payload.db.beginTransaction()
+    // we already have a transaction, we're not in charge of committing it
-    if (req.transactionID) {
+    return false
-      return true
+  }
-    }
+  if (transactionIDPromise) {
+    // wait for whoever else is already creating the transaction
+    await transactionIDPromise
+    return false
+  }
+  if (typeof payload.db.beginTransaction === 'function') {
+    // create a new transaction
+    req.transactionIDPromise = payload.db.beginTransaction().then((transactionID) => {
+      req.transactionID = transactionID
+      delete req.transactionIDPromise
+    })
+    await req.transactionIDPromise
+    return !!req.transactionID
   }
   return false
 }

test/database/config.ts

@@ -0,0 +1,80 @@
+import { buildConfigWithDefaults } from '../buildConfigWithDefaults'
+import { devUser } from '../credentials'
+
+export default buildConfigWithDefaults({
+  collections: [
+    {
+      fields: [
+        {
+          name: 'title',
+          required: true,
+          type: 'text',
+        },
+        {
+          name: 'throwAfterChange',
+          defaultValue: false,
+          hooks: {
+            afterChange: [
+              ({ value }) => {
+                if (value) {
+                  throw new Error('throw after change')
+                }
+              },
+            ],
+          },
+          type: 'checkbox',
+        },
+      ],
+      slug: 'posts',
+    },
+    {
+      fields: [
+        {
+          name: 'relationship',
+          relationTo: 'relation-b',
+          type: 'relationship',
+        },
+        {
+          name: 'richText',
+          type: 'richText',
+        },
+      ],
+      labels: {
+        plural: 'Relation As',
+        singular: 'Relation A',
+      },
+      slug: 'relation-a',
+    },
+    {
+      fields: [
+        {
+          name: 'relationship',
+          relationTo: 'relation-a',
+          type: 'relationship',
+        },
+        {
+          name: 'richText',
+          type: 'richText',
+        },
+      ],
+      labels: {
+        plural: 'Relation Bs',
+        singular: 'Relation B',
+      },
+      slug: 'relation-b',
+    },
+  ],
+  onInit: async (payload) => {
+    await payload.create({
+      collection: 'users',
+      data: {
+        email: devUser.email,
+        password: devUser.password,
+      },
+    })
+  },
+})
+
+export const postDoc = {
+  title: 'test post',
+}

test/database/int.spec.ts

@@ -0,0 +1,195 @@
+import { GraphQLClient } from 'graphql-request'
+
+import type { TypeWithID } from '../../packages/payload/src/collections/config/types'
+import type { PayloadRequest } from '../../packages/payload/src/express/types'
+
+import payload from '../../packages/payload/src'
+import { commitTransaction } from '../../packages/payload/src/utilities/commitTransaction'
+import { initTransaction } from '../../packages/payload/src/utilities/initTransaction'
+import { devUser } from '../credentials'
+import { initPayloadTest } from '../helpers/configHelpers'
+
+describe('database', () => {
+  let serverURL
+  let client: GraphQLClient
+  let token: string
+  const collection = 'posts'
+  const title = 'title'
+  let user: TypeWithID & Record<string, unknown>
+  let useTransactions = true
+
+  beforeAll(async () => {
+    const init = await initPayloadTest({ __dirname, init: { local: false } })
+    serverURL = init.serverURL
+    const url = `${serverURL}/api/graphql`
+    client = new GraphQLClient(url)
+    if (payload.db.name === 'mongoose') {
+      useTransactions = false
+    }
+
+    const loginResult = await payload.login({
+      collection: 'users',
+      data: {
+        email: devUser.email,
+        password: devUser.password,
+      },
+    })
+
+    if (loginResult.token) token = loginResult.token
+    user = loginResult.user
+  })
+
+  describe('transactions', () => {
+    describe('local api', () => {
+      it('should commit multiple operations in isolation', async () => {
+        const req = {
+          payload,
+          user,
+        } as PayloadRequest
+
+        await initTransaction(req)
+
+        const first = await payload.create({
+          collection,
+          data: {
+            title,
+          },
+          req,
+        })
+
+        if (useTransactions) {
+          await expect(() =>
+            payload.findByID({
+              id: first.id,
+              collection,
+              // omitting req for isolation
+            }),
+          ).rejects.toThrow('The requested resource was not found.')
+        }
+
+        const second = await payload.create({
+          collection,
+          data: {
+            title,
+          },
+          req,
+        })
+
+        await commitTransaction(req)
+        expect(req.transactionID).toBeUndefined()
+
+        const firstResult = await payload.findByID({
+          id: first.id,
+          collection,
+          req,
+        })
+        const secondResult = await payload.findByID({
+          id: second.id,
+          collection,
+          req,
+        })
+
+        expect(firstResult.id).toStrictEqual(first.id)
+        expect(secondResult.id).toStrictEqual(second.id)
+      })
+
+      it('should commit multiple operations async', async () => {
+        const req = {
+          payload,
+          user,
+        } as PayloadRequest
+
+        let first
+        let second
+
+        const firstReq = payload
+          .create({
+            collection,
+            data: {
+              title,
+            },
+            req,
+          })
+          .then((res) => {
+            first = res
+          })
+
+        const secondReq = payload
+          .create({
+            collection,
+            data: {
+              title,
+            },
+            req,
+          })
+          .then((res) => {
+            second = res
+          })
+
+        await Promise.all([firstReq, secondReq])
+
+        await commitTransaction(req)
+        expect(req.transactionID).toBeUndefined()
+
+        const firstResult = await payload.findByID({
+          id: first.id,
+          collection,
+          req,
+        })
+        const secondResult = await payload.findByID({
+          id: second.id,
+          collection,
+          req,
+        })
+
+        expect(firstResult.id).toStrictEqual(first.id)
+        expect(secondResult.id).toStrictEqual(second.id)
+      })
+
+      it('should rollback operations on failure', async () => {
+        const req = {
+          payload,
+          user,
+        } as PayloadRequest
+
+        await initTransaction(req)
+
+        const first = await payload.create({
+          collection,
+          data: {
+            title,
+          },
+          req,
+        })
+
+        try {
+          await payload.create({
+            collection,
+            data: {
+              throwAfterChange: true,
+              title,
+            },
+            req,
+          })
+        } catch (error: unknown) {
+          // catch error and carry on
+        }
+
+        expect(req.transactionID).toBeFalsy()
+
+        // this should not do anything but is needed to be certain about the next assertion
+        await commitTransaction(req)
+
+        if (useTransactions) {
+          await expect(() =>
+            payload.findByID({
+              id: first.id,
+              collection,
+              req,
+            }),
+          ).rejects.toThrow('The requested resource was not found.')
+        }
+      })
+    })
+  })
+})

test/database/payload-types.ts

@@ -0,0 +1,90 @@
+/* tslint:disable */
+/* eslint-disable */
+/**
+ * This file was automatically generated by Payload.
+ * DO NOT MODIFY IT BY HAND. Instead, modify your source Payload config,
+ * and re-run `payload generate:types` to regenerate this file.
+ */
+
+export interface Config {
+  collections: {
+    posts: Post
+    'relation-a': RelationA
+    'relation-b': RelationB
+    users: User
+    'payload-preferences': PayloadPreference
+    'payload-migrations': PayloadMigration
+  }
+  globals: {}
+}
+export interface Post {
+  id: string
+  title: string
+  updatedAt: string
+  createdAt: string
+}
+export interface RelationA {
+  id: string
+  relationship?: (string | null) | RelationB
+  richText?:
+    | {
+        [k: string]: unknown
+      }[]
+    | null
+  updatedAt: string
+  createdAt: string
+}
+export interface RelationB {
+  id: string
+  relationship?: (string | null) | RelationA
+  richText?:
+    | {
+        [k: string]: unknown
+      }[]
+    | null
+  updatedAt: string
+  createdAt: string
+}
+export interface User {
+  id: string
+  updatedAt: string
+  createdAt: string
+  email: string
+  resetPasswordToken?: string | null
+  resetPasswordExpiration?: string | null
+  salt?: string | null
+  hash?: string | null
+  loginAttempts?: number | null
+  lockUntil?: string | null
+  password: string | null
+}
+export interface PayloadPreference {
+  id: string
+  user: {
+    relationTo: 'users'
+    value: string | User
+  }
+  key?: string | null
+  value?:
+    | {
+        [k: string]: unknown
+      }
+    | unknown[]
+    | string
+    | number
+    | boolean
+    | null
+  updatedAt: string
+  createdAt: string
+}
+export interface PayloadMigration {
+  id: string
+  name?: string | null
+  batch?: number | null
+  updatedAt: string
+  createdAt: string
+}
+
+declare module 'payload' {
+  export interface GeneratedTypes extends Config {}
+}