olivierwilkinson / prisma-extension-soft-delete

Prisma extension for adding soft delete to Prisma models, even when using nested queries
Apache License 2.0
109 stars 16 forks source link

Transactions Failing When Utilizing Extension #24

Open arimgibson opened 6 months ago

arimgibson commented 6 months ago

When using the soft delete client to try and perform a transaction, the following error is thrown:

error: All elements of the array need to be Prisma Client promises. Hint: Please make sure you are not awaiting the Prisma client calls you intended to pass in the $transaction function.

The same transaction will run successfully when not utilizing the extension. Workaround: utilize a normal Prisma client for transactions

Can come back later this week to provide a reproduction; don't have time right this second and thought I'd throw this out there in case this is something we're already aware of. Couldn't find in other bug reports

rpaterson commented 5 months ago

The error appears on sequential (array) style calls to $transaction([ ... ]), but not on interactive style calls to $transaction(async (prisma) => { ... })

rpaterson commented 5 months ago

So as a workaround it's easy to convert from "sequential" to "interactive" style

rpaterson commented 5 months ago

Actually looking at the code I think "interactive" style transactions don't throw this error, but the queries are not actually run inside the transaction, which is even worse!

baojr commented 5 months ago

I just found the same problem @rpaterson described in my project. After installing this extension, previously working code is now unable to read data written within the same transaction, which violates the isolation properties of SQL transactions.

vinnymac commented 4 months ago

I ran into this issue today as well. Although in my case, this appears to also happen if I configure additional extensions. If prisma-extension-soft-delete is the only extension configured, transactions work just fine 🤷🏼‍♂️.

In my case, both removing prisma-extension-soft-delete, or making it the only extension resolves the issue.

arimgibson commented 4 months ago

I ran into this issue today as well. Although in my case, this appears to also happen if I configure additional extensions. If prisma-extension-soft-delete is the only extension configured, transactions work just fine 🤷🏼‍♂️.

In my case, both removing prisma-extension-soft-delete, or making it the only extension resolves the issue.

Interesting observation; haven't seen the same here! This is the only one we're using and still having issues. Will try to get around to that reproducible example this weekend; keeps slipping my mind

For others with the same, please +1 (thumbs up) this comment instead of starting a long chain of just "same" -- keeps things cleaner. Thank you :pray:

olivierwilkinson commented 4 months ago

Hi all, thank you for raising this! Sorry for the slow response, I've been trying to figure out exactly what the problem is!

Unfortunately at this point I think this is a limitation in the Prisma extensions API. This extension changes the operation type from deletes to updates, and this requires using the previous instance of the client, the one that is being extended. When using interactive transactions the client that needs to be used is the one that is provided in the $transaction callback, which is not the one used when changing operations. For array transactions ($transaction([...])) the problem is less clear, however it seems likely that changing the client used causes issues there too.

There was a separate but related issue which seemed to be fixed by replacing each client query method to use the previous client (#14), however this has broken the fluent API (#22) and fixed the issue by breaking transactions for every operation.

My current plan is to try to create a repro in the Prisma repo to prove this and add some tests, however if I am right it might take a while to resolve by the Prisma team due to it requiring an API change to extensions.

For now I recommend using the middleware version of this library which has feature parity with this one. I know that middleware are deprecated but I remember reading that the Prisma team are not intending to remove middleware in the foreseeable future, they deprecated them to encourage people to move to extensions. Due to middleware supporting changing operations I don't think these issues will be present, but I will add the tests provided in #25 to double check that. (Thank you @rpaterson for raising that PR!)

When using middleware with extensions the middleware needs to be applied to the initial (non-extended) client, however for the soft-delete behaviour in these libraries I don't think that will be a problem. There is a note about the semantics of using middleware with extensions here.

If anybody has any thoughts or concerns surrounding this please feel free to reply here! I really appreciate everyones patience with this issue as it is a major one! ❤️

vinnymac commented 4 months ago

I can confirm switching to the middleware works with transactions correctly. Even while using additional extensions.

Thanks for investigating it!

BobReid commented 3 months ago

I spent the day looking into this and will share my findings.

There are 2 distinct issues going on with transactions, one of the transaction array API and the other on the interactive API.

Transaction Array API

This extension seems to utalize the model method overrides as opposed to the query overrides. By that I mean the extension defines its overrides along the lines of

prisma.$extends({
  model: {
    [Model]: { 
      count({...})  { ... }, 
        ... 
     ]
  }
}

as opposed to

prisma.$extends({
  query: {
    [Model]: { 
      count({...})  { ... }, 
        ... 
     ]
  }
}

When the model extensions are used the result is always a Promise as opposed to a PrismaPromise. I tried many different techniques including returning an unresolved PrismaPromise and it always gets unwrapped and rewrapped as a vanilla JS Promise.

PrismaPromises are the deferred promises that prisma uses to delay sql exection, allowing them to be collected and run in a transaction together, as opposed to firing off when they are first created. Vanilla Promises cannot be passed to the transaction API for this reason.

Interactive Transaction Issue

@olivierwilkinson your intuition on the interaction transaction issue is close but not 100% correct, as far as I can tell. The operation switching from delete to update is a problem. But I don't believe it is because you are switching client instances (extended vs non-extended).

I tested this theory by referencing the extended client via a closure.

const extended = prisma.$extends({
  query: {
    [Model]: {
      delete(...) { 
        return extended[Model].update(...)
      }
    }
  }
})

The results were identical

The issue is that when the extension switched the operation by calling the client directly instead of a transaction that the API requires. As far as I can tell there is no api to access the current transaction.

Consider the code needed to create and then delete a user

prisma.$transaction((tx) => {
  const user = await tx.user.create(...)
  await tx.user.delete({where: { id: user.id } })
});  

When the operation is rewritten it is the equivalent of

prisma.$transaction((tx) => {
  const user = await prisma.user.create(...)
  await prisma.user.delete({where: { id: user.id } })
});  

This code will execute but the statements are not attached to the transaction itself.

Until the extensions API exposes the current transaction there is no way to safely rewrite the operation.

olivierwilkinson commented 1 month ago

Hi all,

I'm unfortunately back to thinking that it is some interaction between how different extensions are handling transactions. I'm going to make another concerted effort to fix this issue, I would really appreciate if everyone could list the extensions they are using so I can look into their implementation and create a reproduction.

@BobReid thank you for looking into this! I think you are correct that my intuition was not quite right, unfortunately I am not using the extended client in the same way as in your snippet:

const extended = prisma.$extends({
  query: {
    [Model]: {
      delete(...) { 
        return extended[Model].update(...)
      }
    }
  }
})

I am instead using the previous client using the defineExtension helper:

Prisma.defineExtension((client) => {
  return client.$extends({
    query: {
      [Model]: {
        delete(...) {
          return client[Model].update(...)
        }
      }
    }
  });
});

When I tried to reproduce the problem in the Prisma repo using the second snippet everything seems to work fine so I think there is some deeper reason for the issue, possibly related to nested transactions: https://github.com/prisma/prisma/issues/19346

rpaterson commented 1 month ago

The failing test cases I created in #25 reproduce the problems without any other extensions.

olivierwilkinson commented 1 month ago

The failing test cases I created in #25 reproduce the problems without any other extensions.

Ah yes you are right! Sorry! I'm getting a little ahead of myself and thinking about the issue with row level security which was also related to transactions (and might be reintroduced when I fix the fluent API).

Anyways that is not directly related to this issue and I will focus on fixing the tests in your PR, thank you again for doing that.

It might be that they are helpful producing a repro in the Prisma repo if that is where the issue lies, I was focused on rollback before but your tests are more nuanced.

BobReid commented 3 weeks ago

@olivierwilkinson

Yes I know my snippet was different than your code. My snippet was trying to test using the extended client which still doesn't work.

The issue is that neither the un-extended, not the extended client are the transaction instance that is passed as a argument to the interaction transaction block.

IMO the extension API should pass the client used to make the call (either the based client or the transaction if the operation is invoked on a transaction) as an argment to the extension method.

something like

Prisma.defineExtension((client) => {
  return client.$extends({
    query: {
      [Model]: {
        delete(client, ..rest) {
          return client.update(...)
        }
      }
    }
  });
});

I briefly browsed the prisma source code, it seems trivial to do, but we would need to convince the team that this is the correct approach.