payloadcms / payload

Payload is the open-source, fullstack Next.js framework, giving you instant backend superpowers. Get a full TypeScript backend and admin panel instantly. Use Payload as a headless CMS or for building powerful applications.
https://payloadcms.com
MIT License
24.75k stars 1.57k forks source link

Collections/Globals with hooks that call `await payload.update()` hang on save with Postgres #8852

Open azivkovi opened 3 days ago

azivkovi commented 3 days ago

Describe the Bug

I use afterChange and beforeChange hooks to update other collections when saving an item. When I use await req.payload.update() to do so, "Submitting..." never finishes and my app hangs. When I exclude the req.payload.update() logic, hooks finish as expected and item is saved.

This happens only with Postgres, not Mongodb, and I noticed it affects both Collections and Globals. It also happens if i load the payload instance as const payload = await getPayloadHMR({ config: configPromise }), instead of using req.

Link to the code that reproduces this issue

https://github.com/azivkovi/payload3demo

Reproduction Steps

  1. Initialize the linked repo.
  2. Set up a test postgres database and edit the .env file for database url.
  3. Enter Pages collection and add/edit an item - everything works as expected, as it has no hooks that call await req.payload.update().
  4. Enter Users collection or Settings global and add/edit an item - app hangs on save as they have await req.payload.update() in a hook.

Which area(s) are affected? (Select all that apply)

db-postgres

Environment Info

Node.js v22.9.0

Binaries: Node: 22.9.0 npm: 10.8.2 Yarn: N/A pnpm: 9.12.1 Relevant Packages: payload: 3.0.0-beta.118 next: 15.0.0 @payloadcms/db-mongodb: 3.0.0-beta.118 @payloadcms/db-postgres: 3.0.0-beta.118 @payloadcms/graphql: 3.0.0-beta.118 @payloadcms/next/utilities: 3.0.0-beta.118 @payloadcms/richtext-lexical: 3.0.0-beta.118 @payloadcms/richtext-slate: 3.0.0-beta.118 @payloadcms/translations: 3.0.0-beta.118 @payloadcms/ui/shared: 3.0.0-beta.118 react: 19.0.0-rc-65a56d0e-20241020 react-dom: 19.0.0-rc-65a56d0e-20241020 Operating System: Platform: linux Arch: x64 Version: #1 SMP Fri Mar 29 23:14:13 UTC 2024 Available memory (MB): 31970 Available CPU cores: 24

r1tsuu commented 3 days ago

Try to pass the req here to use the transaction

await req.payload.update({
  collection,
  where: {
    id: {
      in: [1, 2, 3, 5, 6, 7],
    },
  },
  data: {
    featured: true,
  },
})
azivkovi commented 3 days ago

I did

await req.payload.update({
  collection,
  where: {
    id: {
      in: [1, 2, 3, 5, 6, 7],
    },
  },
  data: {
    featured: true,
  },
  req
})

This didn't fix the problem and it still hangs, but the hook is now called repeatedly from what I can see in the console log.

DanRibbens commented 3 days ago

It also happens if i load the payload instance as const payload = await getPayloadHMR({ config: configPromise }), instead of using req.

This is not recommended. You should use the req passed to the hooks.

The reason this doesn't work is because PostgreSQL transactions are meant to be run sequentially. You'll need to either not use transactions or await each call synchronously.

azivkovi commented 3 days ago

I do use req, I just noted that it also happens if I do not.

Did you not see my code? All update calls are awaited.

Sorry, but I am not sure on what grounds you closed the issue on. Can you please elaborate on your answer?

DanRibbens commented 3 days ago

Oh I was thinking that with the for loop they'd be called asynchronously, but that was my mistake.

I'll reopen and keep the discussion going.

DanRibbens commented 3 days ago

In the payload update operation that handles multiple documents, we use promise.all for all docs that match from the where. That is the reason the hook is hanging. We need to revisit if that is the right approach. This tells me we probably also don't have test coverage because I wouldn't expect any update many that interact with multiple docs to work in postgres based on this.

r1tsuu commented 3 days ago

I wonder if we should implement updateMany to the db as we have with deleteMany. I imagine sequential execution for update without this can be very slow

DanRibbens commented 2 days ago

The problem with adding updateMany is that not all documents will have the same update applied due to hooks. Suppose you have a beforeChange hook for a field that is dynamic. Each document, having a different value for that particular dynamic field requires a separate db.updateOne to be called.

Even if we have a db.updateMany that somebody could use in their hooks, I wouldn't recommend that either. It would bypass all the Payload logic like validation, access control, saving versions, handling of localization, and all hooks.