Skip to content

Add support for custom-defined createValue function #16

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

Merged
merged 4 commits into from
Jan 13, 2024

Conversation

justinxia6dot5
Copy link
Contributor

Proposal:

We made changes so the condition in createValue property that the method must return a falsy value if the record is not deleted and vice versa does not have to be forced. With the modification, we can define createValue functions that have a custom value for whether the record is soft deleted or not. For example, new Date(0) can be a value for a non-deleted record though it is truthy.

Rationale:

We initially used deletedAt: DateTime as the field just like everyone else, with NULL representing a false value for being not deleted, and we were using Postgres as our underlying database. The problem arose with the following chain:

  1. We used some fields along with deletedAt as a UNIQUE CONSTRAINT, but in Postgres, NULL values are not considered distinct, which could bring many potential data integrity problems for us. More details in this post.
  2. The post provided solutions like upgrading to version 15, and using a partial unique index, which one our cloud provider cannot guarantee, and the other Prisma doesn't support.
  3. We thought about converting DateTime to number(TypeScript)/BIGINT(Postgres), and using 0: number to represent the value not being soft deleted. But the issue was BIGINT cannot be saved in JSON format.
  4. We converted DateTime to string, using '0': string as the default value for not being soft deleted, and stringfying the DateTime to represent a record has been soft deleted.

Implementation:

We only changed a few lines in lib/utils/resultFiltering.ts so now the rows are filtered on an exact equality check with the value defined in createValue function. Since deletedAt should really only contain two types of values -- one representing soft-deletedness, the other not being deleted, we believe an equality check is fine, as it is similar to what boolean is doing (one or the other). I also noted that createValue(false) was used extensively throughout the project so why discard checking equality with it during the filter stage?

We are not sure if our change still serves the original purpose of what the package was doing, so please point out if we made a mistake in the fork.

Conclusion:

We really appreciate your work in opensourcing this soft-delete package, which Prisma doesn't support officially, and we believe you had your take and philosophy on designing the function to return only truthy/falsy values, as it's much simpler API defining what users can and should do. We propose this as an alternative to defining createValue in a truthy/falsy way, as defining in that way will still be supported under the current version.

misterapp and others added 4 commits November 8, 2023 17:23
Update resultFiltering helpers to use lodash isEqual to compare values
from createValue function. Lodash isEqual does Date comparison in a way
that works for `new Date(0) === new Date(0)`.

Update README with explanation of new behaviour of createValue.

Update nestedReads tests to use Date(0) as the value for non-deleted
records, ensuring we have test coverage for that behaviour.
@olivierwilkinson
Copy link
Owner

Hi there,

Sorry for taking so long getting back to you, I thought I had at least commented saying thanks for doing this but I clearly forgot! Thanks for raising this, and for being so thorough in your description and explanation 😄

It's taken me a bit of time to get this working because of an issue in the underlying prisma-nested-middleware library, but that's all fixed now!

In terms of the logic you had in the PR I only needed to make a small change: I replaced the referential equality checks with lodash.isEqual so that we can do date comparison. isEqual(new Date(0), new Date(0)) is true whereas new Date(0) === new Date(0) is false.

I also added an update to the README and tests to make sure things are working correctly.

Again thanks for raising this and please raise a PR or issue if you find anything else 🙌

Copy link
Owner

@olivierwilkinson olivierwilkinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@olivierwilkinson olivierwilkinson merged commit d1f62bc into olivierwilkinson:main Jan 13, 2024
Copy link

🎉 This PR is included in version 1.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

3 participants