Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: allow overwrite in AND, OR in where queries #11

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

velenyak
Copy link

Proposal
Allow the deleted field to be overwritten when used inside the AND, OR operators.

Before:

const users = await testClient.user.findMany({
  where: { name: 'John', OR: [{ deleted: false }, { deleted: true }] },
});
// Runs query:
// SELECT * FROM 'users' WHERE name = 'John' AND (deleted = true OR deleted = false) AND deleted = true

After:

const users = await testClient.user.findMany({
  where: { name: 'John', OR: [{ deleted: false }, { deleted: true }] },
});
// Runs query:
// SELECT * FROM 'users' WHERE name = 'John' AND (deleted = true OR deleted = false)

Let me know what do you think about these changes?

@olivierwilkinson
Copy link
Owner

olivierwilkinson commented Aug 28, 2023

Heya,

Thanks for raising this, the idea is to be able to get mixed results based on the OR statement right? Part of the strategy I took is to make sure mixed lists of soft-deleted and non-deleted rows can't be queried, to avoid accidentally operating on a soft-deleted, or worse, non deleted rows. I'm a bit nervous about making this the default behaviour since it would enable that to happen.

I'll have a think about how to enable this somehow, maybe by adding some sort of API to invert control so you can add this behaviour yourself more easily?

In the meantime you could do this by adding a second middleware after this one, I haven't tested it but something like this should work assuming you use the same deleted field for every model:

import unset from 'lodash/unset';
import { createSoftDeleteMiddleware } from 'prisma-soft-delete-middleware';
import { createNestedMiddleware } from 'prisma-nested-middleware';

[...]

client.$use(createSoftDeleteMiddleware({...}));

// enable querying mixed lists of soft-deleted and non-deleted rows using logical where statements
client.$use(createNestedMiddleware((params, next) => {
  const actions = [
    'updateMany',
    'findFirst',
    'findMany',
    'groupBy',
    'count',
    'aggregate',
    'include',
    'select'
  ];

  if (
    actions.includes(params.action) &&
    isDeletedFieldOverWritten('deleted', params.args.where)
  ) {
    return next(unset(params, 'args.where.deleted'));
  }

  return next(params);
}));

I hope this is helpful, please let me know if I got the wrong end of the stick! 😄

@velenyak
Copy link
Author

Hello,

Yes, that's the use case I want to have, to be able to see all of the items, both deleted and not deleted.
I totally understand your concerns about making this the default behaviour, but having the ability do it with the package would be nice.
Thank you for the suggestion, that works perfectly!

@nullfox
Copy link

nullfox commented Nov 20, 2023

I know this PR is closed or superseded but wanted to say thanks to @velenyak and @olivierwilkinson for the code here.

I made one minor tweak to isDeletedFieldOverWritten when using it as it's own middleware which was to change:

if (where[field] !== undefined) {
  return true;
}

to the following:

// Added the null check because by the soft delete middleware
  // sets the field to null by default and will always trigger the unset()
  if (where[field] !== undefined && where[field] !== null) {
    return true;
  }

@khalilsarwari
Copy link

khalilsarwari commented Jan 16, 2024

If explicitly querying soft-deleted and non-deleted rows, I don't think its unexpected or dangerous to be returning the mixed results accordingly (as in the default behaviour is unchanged/intuitive). It would be great if doing something like OR: [{ deleted: false }, { deleted: true }] to get back all records worked natively instead of having to extend the library! (this whole time I thought it would return all records, only after reading this issue did I realize it is not the case).

I do see the concern though (and really appreciate this library!), just wanted to share my two cents. The perspective here would be that explicit overriding the implicit default/behaviour is not too unexpected imo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants