olivierwilkinson / prisma-extension-soft-delete

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

Problem with other extension #14

Closed leonard-henriquez closed 7 months ago

leonard-henriquez commented 7 months ago

I use another prisma extensions that doesn't work with prisma-extension-soft-delete. It's a very simple extension to wrap queries into a transaction and set a config for a row level security use case.

I've tried to chain them in both ways. This way works on most queries client.$extends(extension).$extends(softDeleteExtension()). But some queries never return and keep running forever so I never get to the following instruction and my backend responses get stuck.

Any clue where it might come from ?

RLS extension:

Prisma.defineExtension((prisma) => {
  if (!organizationId) {
    throw new NotFoundIssue('Organization not found');
  }

  return prisma.$extends({
    query: {
      $allModels: {
        async $allOperations({ args, query }) {
          const [_, result] = await prisma.$transaction([
            prisma.$queryRaw`SELECT set_config('app.current_organization_id', ${organizationId}, TRUE)`,
            query(args),
          ]);
          return result;
        },
      },
    },
  });
});

Soft delete extension:

export const softDeleteExtension = () => {
  return createSoftDeleteExtension({
    models: {
      Workspace: true,
    },
    defaultConfig: {
      field: 'archivedAt',
      createValue: (archived: boolean) => {
        if (archived) return new Date();
        return null;
      },
    },
  });
};
olivierwilkinson commented 7 months ago

Hi there, thank you for the extra detail! This is related to the export logic issue / PR right?

I'll have a look into this soon, one thing that jumps out at me is the use of a transaction in the first extension. I don't know if this is the problem without investigating first but are the queries that hang wrapped in a transaction themselves? I know Prisma has an issue with nested transactions.

I'll have a look into it myself to make sure it's not an issue with this extension 👍

leonard-henriquez commented 7 months ago

This is related to the export logic issue / PR right?

It was in a attempt to define a new extension that would nest both extension but it wasn't a successful attempt.

I don't know if this is the problem without investigating first but are the queries that hang wrapped in a transaction themselves?

From what I've seen in my log it doesn't seem to only concern nested transctions but i'm not 100%

I have tried to use prisma-extension-soft-delete with https://github.com/cerebruminc/yates, https://github.com/dthyresson/prisma-extension-supabase-rls and https://github.com/prisma/prisma-client-extensions/tree/main/row-level-security. They all have the same problem. I'd very much like to help you investigate this issue because it's a blocking issue for me but I don't have any clue where the problem is coming from...

olivierwilkinson commented 7 months ago

Sure thing, I'll try to have a look into it today. Do you have an example query that is showing this behaviour? That would help me narrow down what it could be.

leonard-henriquez commented 7 months ago

Thank you !! For example this one:

UPDATE "public"."Workspace" SET "archivedAt" = $1, "updatedAt" = $2 WHERE ("public"."Workspace"."id" = $3 AND 1=1) RETURNING "public"."Workspace"."id", "public"."Workspace"."createdAt", "public"."Workspace"."updatedAt", "public"."Workspace"."archivedAt", "public"."Workspace"."publishedAt", "public"."Workspace"."title", "public"."Workspace"."companyId", "public"."Workspace"."password", "public"."Workspace"."stage", "public"."Workspace"."closeDate", "public"."Workspace"."value", "public"."Workspace"."ownerId", "public"."Workspace"."workspaceTemplateId", "public"."Workspace"."notes", "public"."Workspace"."mergeAccountId", "public"."Workspace"."organizationId"
olivierwilkinson commented 7 months ago

Heya, I think I've got a repro, I'll see what I can figure out over lunch but I may only be able to really look into it tomorrow morning!

leonard-henriquez commented 7 months ago

Thank you very very much 😍 !!!

olivierwilkinson commented 7 months ago

I've figured out what the problem is! 😄 (at least for the supabase extension)

When a model that is configured for soft-delete attempts to use a delete query this extension changes the operation to be an update instead. The only way to do that in extensions is to use the previous client because params.query is predefined as a delete operation. I thought that the client that is passed to defineExtension would be the client extended by the previous extension, however that is not the case, instead an unextended client is used (I may raise an issue about that in the Prisma repo). This means that the setup done by the supabase extension is not done when the operation is changed (the config for RLS is not set), I don't know why that causes it to hang but it's fixed when using the correct client.

That's an overly detailed explanation but the solution is a bit clearer, I'll need to allow passing in the client to be used when changing operations in the config for this extension. It would change the way you initialise it to the following, or something similar:

export const rlsExtendedClient =
  new PrismaClient().$extends(useSupabaseRowLevelSecurity())

export const client =
  rlsExtendedClient.$extends(
    createSoftDeleteExtension({
      parentClient: rlsExtendedClient,
      models: { DrumMachine: true },
    })
  )

I need to do some work on the underlying prisma-extension-nested-operations library first as that is where the operation changing happens, but it shouldn't take too long. I'll keep you updated with my progress 👍

olivierwilkinson commented 7 months ago

As an aside, if you want to prevent users from deleting using RLS policies you'll need to prevent them from updating if you want to restrict soft-deleting as well.

leonard-henriquez commented 7 months ago

Thank you very much for looking into this issue 😍 Let me know if I can help !

As an aside, if you want to prevent users from deleting using RLS policies you'll need to prevent them from updating if you want to restrict soft-deleting as well.

Totally makes sense ! (but I don't need this)

leonard-henriquez commented 7 months ago

Do you think that there is a way I can help you @olivierwilkinson ? No worries if now is not the good time for you to fix this issues !

olivierwilkinson commented 7 months ago

Hi there, sorry for the delay!

So my initial investigation was correct, the issue is around switching operations and the client used, but I think my conclusions about why it was causing things to hang were wrong. My current thinking is that by using the previous client the RLS extension is in fact running again, and that is causing the setup for RLS to run again nested in the previous setup.

It seems to be fixed when I make queries using this to change the operation, maybe because the setup done by the RLS extensions applies to those queries without needing to be rerun. I'm still investigating the best way to do that, using this in query.$allModels.$allOperations wasn't working for some reason (might not be supported), but using it in query.user.delete does seem to work.

I need to do some more work to get this ready but I am still making progress so hopefully will have a fix out soon, thank you for the offer to help though! 🙏

muliswilliam commented 7 months ago

Hi @olivierwilkinson. Any update on this? I have a similar issue.

olivierwilkinson commented 7 months ago

Heya, I've just raised a PR!

This proved (/is proving) to be a very tricky issue, but I think the fix is to always use the previous client to actually execute the operations, that has at least fixed my repro with prisma-extension-supabase-rls. Using this was actually a false positive and it ended up that the query hooks don't support what I was trying to do...

Anyways it would be really appreciated if one or both of you could try it out in your projects! If it's helpful I can do a pre-release.

Thank you for being so patient 🙏

olivierwilkinson commented 7 months ago

Hi all,

I've merged this as it was passing the tests and my repro using the supabase-rls extension. Let me know if you find any issues with the new version and I'll reopen this 👍

github-actions[bot] commented 7 months ago

:tada: This issue has been resolved in version 1.0.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket: