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(psl, qe): Add relation mode prismaSkipIntegrity for MongoDB #4116

Open
wants to merge 29 commits into
base: main
Choose a base branch
from

Conversation

ItzSiL3Nce
Copy link

@ItzSiL3Nce ItzSiL3Nce commented Aug 7, 2023

This PR adds a new relationMode for Prisma called prismaSkipIntegrity.
Related to the discussion prisma/prisma#20544
Closes prisma/prisma#15759

This PR, solves a common problem and also
drastically increases the performance of updates and deletes in MongoDB.

This new mode works only for the MongoDB connector and is needed to skip any (emulated) integrity check.
This is needed in cases like mine where, for example, deleting a document from the database must not check for its references and leave them as they are, so they can be compared with backups and improve (by a lot) the performance of write operations, as they do not have to perform any emulation of relations.

This mode also makes the MongoDB connector work just like the official MongoDB client would (not caring about leaving "pending" relations).

Of course, users using this relationMode should be careful and be aware of the potential problems arising during write operations, at the PrismaClient level. This can be done easily though, either by checking before running the query or with try-catch patterns (or similar).

@ItzSiL3Nce ItzSiL3Nce requested a review from a team as a code owner August 7, 2023 15:05
@codspeed-hq
Copy link

codspeed-hq bot commented Aug 7, 2023

CodSpeed Performance Report

Merging #4116 will not alter performance

Comparing ItzSiL3Nce:feat/mongo-relationMode-prismaSkipIntegrity (ed91a7b) with main (3b74ba3)

Summary

✅ 11 untouched benchmarks

@janpio
Copy link
Contributor

janpio commented Aug 7, 2023

This PR is a nice surprise, thanks @ItzSiL3Nce.

Until your first PR is merged, we will need to allow the GitHub Actions to run by manually clicking - feel free to ping me in Prisma Slack (obvious name) if you need it to happen sooner.

You are kinda "forcing" us to figure out our internal position on this now - but in a good way 😆. We started to dig into our design documents of both relationMode and referential actions/integrity support again. I think our next step will be to make sure we have a proper public issue for this where we pull in all the information, and that we can then use to dig into understanding your suggested solution.

@CLAassistant
Copy link

CLAassistant commented Aug 7, 2023

CLA assistant check
All committers have signed the CLA.

@ItzSiL3Nce ItzSiL3Nce force-pushed the feat/mongo-relationMode-prismaSkipIntegrity branch from af68f4a to 2fa2a53 Compare August 7, 2023 18:28
@ItzSiL3Nce
Copy link
Author

(I had to rebase and do some git evil tricks to change my author name and email in the commits, to sign the CLA. Sorry for the chaos)

… mode with the new 'prismaSkipIntegrity' mode
…existence of the new 'prismaSkipIntegrity' relation mode
@Jolg42
Copy link
Contributor

Jolg42 commented Aug 8, 2023

I think this existing issue captures the feature request here: prisma/prisma#15759
Do you agree @ItzSiL3Nce?

@ItzSiL3Nce
Copy link
Author

ItzSiL3Nce commented Aug 8, 2023

@Jolg42 Yes, I didn't notice that feature request, but yes. My PR allows setting the relationMode prismaSkipIntegrity that would act exactly like you explained in your request, even though you called it "none".
Currently in my PR this mode is allowed for MongoDB only, though it's very easy to extend if for (possibly) future non-relational DBs. I don't know about the Weak Relations you've mentioned for standard SQL databases, though.

@janpio
Copy link
Contributor

janpio commented Aug 8, 2023

As "weak relations" we usually refer to a relation that is not backed by a foreign key in relational databases. So there are no constraints that enforce the values you can set, and also no referential actions and no guarantee of referential integrity. Not that different what is the default in MongoDB honestly.

Regarding None vs prismaSkipIntegrity, I think your name actually captures the true intent better @ItzSiL3Nce - the relations are still fully understood and used by Prisma - there is just no check of referential integrity on the data, and no referential actions to maintain it. prismaNoIntegrity could also be a name for this maybe.

I wonder how Prisma now deals with data that is invalid, so a "relation scalar field" that points to another entry that does not exist for example. Or one side of a required relation, that does not actually have a counterpart on the other side.

I think we have a quite extensive test suite in prisma/prisma for this, so maybe we can get an integration version here @Jolg42 that we can continue poking at over at prisma/prisma?

@ItzSiL3Nce
Copy link
Author

ItzSiL3Nce commented Aug 8, 2023

@janpio I've already built a custom engine and used it, on my machine, with my NodeJS PrismaClient, it works as I expected it to, don't know if the community would expect something else, though.

I don't really know how to submit a public test for it though cause there are a lot of automatic dependencies being downloaded while running the prisma CLI and generating the correct binaries, I just manually plug my custom built binary after everything.

What I did is I tested a delete query, with the 'query' log enabled. There was nothing in the logs other than the delete itself, worked well.
For the failing cases, where there are attempts of "joining" an entity that doesn't exist anymore, just a try-catch is needed around the query, to deal with the possible exceptions (this is the part that I don't know if the community would agree on using).

It could also be possible to do like mongoose's populate function does when a referenced entity doesn't exist anymore, which is returning it as null, but that may be more complex to implement.

It could also be possible to enforce all relations to be optional in the schema while using this relation mode, for better type-safety.

@janpio
Copy link
Contributor

janpio commented Aug 8, 2023

I don't really know how to submit a public test for it though cause there are a lot of automatic dependencies being downloaded while running the prisma CLI and generating the correct binaries, I just manually plug my custom built binary after everything.

Yes, we would take care of that by creating an "integration version" which then gets its own PR and branch at prisma/prisma, which then can be modified with additional tests - in this case something that adds prismaSkipIntegrity so we see what the test suite does.

For the failing cases, where there are attempts of "joining" an entity that doesn't exist anymore, just a try-catch is needed around the query, to deal with the possible exceptions (this is the part that I don't know if the community would agree on using).

Ok, I expect these exceptions and error messages are pretty terrible right now? Can you maybe share a few? That might indeed be one of those areas we want to polish. I could imagine we get a known error that is MongoDB specific that explains the situation a bit.

It could also be possible to do like mongoose's populate function does when a referenced entity doesn't exist anymore, which is returning it as null, but that may be more complex to implement.

Can you explain that a bit more? For me https://mongoosejs.com/docs/populate.html is really what Prisma does automatically when you include a relation - based on the relation scalar field (= foreign key) we load the referenced relation. Or did you mean specifically https://mongoosejs.com/docs/populate.html#doc-not-found?

@ItzSiL3Nce
Copy link
Author

ItzSiL3Nce commented Aug 9, 2023

Ok, I expect these exceptions and error messages are pretty terrible right now? Can you maybe share a few? That might indeed be one of those areas we want to polish. I could imagine we get a known error that is MongoDB specific that explains the situation a bit.

This is the error I get when attempting to include a deleted referenced entity:

prisma:error 
Invalid `x.record.findUnique()` invocation in
/home/silence/Desktop/dev/redacted/packages/tests/dist/mongo.js:42:40

  39     where: { id: relation.id }
  40 });
  41 console.log('Relation deleted, now trying to include the relation when finding the record');
 42 const record = await x.record.findUnique(
Inconsistent query result: Field relation is required to return data, got `null` instead.
PrismaClientUnknownRequestError: 
Invalid `x.record.findUnique()` invocation in
/home/silence/Desktop/dev/redacted/packages/tests/dist/mongo.js:42:40

  39     where: { id: relation.id }
  40 });
  41 console.log('Relation deleted, now trying to include the relation when finding the record');
 42 const record = await x.record.findUnique(
Inconsistent query result: Field relation is required to return data, got `null` instead.
    at Hr.handleRequestError (/home/silence/Desktop/dev/redacted/packages/mongodb/dist/prisma-client/runtime/library.js:122:7171)
    at Hr.handleAndLogRequestError (/home/silence/Desktop/dev/redacted/packages/mongodb/dist/prisma-client/runtime/library.js:122:6388)
    at Hr.request (/home/silence/Desktop/dev/redacted/packages/mongodb/dist/prisma-client/runtime/library.js:122:6108)
    at async l (/home/silence/Desktop/dev/redacted/packages/mongodb/dist/prisma-client/runtime/library.js:126:10298)
    at async main (/home/silence/Desktop/dev/redacted/packages/tests/dist/mongo.js:42:25) {
  clientVersion: '5.1.0'
}

@janpio GOOD NEWS!
I found out that no errors are thrown if I make the relation optional in the schema.prisma file!
And the returned field is just null, as I expected it to be. This is both what I wanted it to be and what happens in mongoose, which most people using javascript/typescript would come from.
Maybe enforcing the relation-fields in the schema, only when using this relation mode, to be optional may be the way to go?

Can you explain that a bit more? For me https://mongoosejs.com/docs/populate.html is really what Prisma does automatically when you include a relation - based on the relation scalar field (= foreign key) we load the referenced relation. Or did you mean specifically https://mongoosejs.com/docs/populate.html#doc-not-found?

Yes I specifically meant the https://mongoosejs.com/docs/populate.html#doc-not-found part.
It'd just return null when attempting to include a missing referenced entity, or an empty array for multiples.
This way it'd be possible to not use exceptions, which are a bit "dirty", and leave the null checks to the user.

@janpio
Copy link
Contributor

janpio commented Aug 9, 2023

Inconsistent query result: Field relation is required to return data, got null instead.

The error message is not even that bad, I am surprised 👍

I found out that no errors are thrown if I make the relation optional in the schema.prisma file!
And the returned field is just null, as I expected it to be.
Maybe enforcing the relation-fields in the schema, only when using this relation mode, to be optional may be the way to go?

That is an interesting and logical observation. It could indeed be a way to help users do the right thing.

On the other hand, I bet there are also users that want it to be a required relation, and there data is currently just too "messy" for it - so they want to be able to work with what they have but explicitly get errors. Does that thought make sense to you?

This is both what I wanted it to be and what happens in mongoose, which most people using javascript/typescript would come from.

Does Mongoose also do something about the "foreign key" field or only for the "relation" (which is set to null)? Does it just keep the invalid value around until you manually update it?

@ItzSiL3Nce
Copy link
Author

ItzSiL3Nce commented Aug 9, 2023

That is an interesting and logical observation. It could indeed be a way to help users do the right thing.

On the other hand, I bet there are also users that want it to be a required relation, and there data is currently just too "messy" for it - so they want to be able to work with what they have but explicitly get errors. Does that thought make sense to you?

Yeah of course, it should be explained very well in order to not cause confusion or unexpected behaviours.
Maybe, it could be possible to have a flag at the PrismaClient side, whether to have explicit errors or implicit null data, when including entities.
This of course would probably require a lot of extra work, though.

This is both what I wanted it to be and what happens in mongoose, which most people using javascript/typescript would come from.

Does Mongoose also do something about the "foreign key" field or only for the "relation" (which is set to null)? Does it just keep the invalid value around until you manually update it?

The foreign key part (the related entity's ObjectId) remains there.

@Jolg42
Copy link
Contributor

Jolg42 commented Aug 9, 2023

Note: I recreated this PR in our repo at #4123
and triggered an integration version, which popped up at prisma/prisma#20618

@ItzSiL3Nce if you want to try it out pin Prisma packages to this version 5.2.0-integration-engines-5-2-0-10-integration-mongo-relation-mode-prisma-skip-integrity-ed91a7b2497b937965f670a81b14ad3b5b2318cc.2

@ItzSiL3Nce
Copy link
Author

Note: I recreated this PR in our repo at #4123 and triggered an integration version, which popped up at prisma/prisma#20618

@ItzSiL3Nce if you want to try it out pin Prisma packages to this version 5.2.0-10.integration-mongo-relation-mode-prisma-skip-integrity-ed91a7b2497b937965f670a81b14ad3b5b2318cc

Thanks a lot, I'm going to use it and I'll also make some of my co-workers use it in our test environments, I'll give feedback in case of problems.

@Jolg42
Copy link
Contributor

Jolg42 commented Aug 9, 2023

@ItzSiL3Nce Note that I messed up the version in my previous post and I updated it now 🙈

@SevInf SevInf added the PR: Feature A PR That introduces a new feature label Mar 20, 2024
@ItzSiL3Nce
Copy link
Author

I've been using, first in test environments and then in production ones, the custom pinned version for almost a year now (since it was published on NPM last August).

I didn't have any problems with it and it has been super helpful. I noticed that a recent release of Prisma fixed and optimized a lot of stuff regarding MongoDB but I didn't see anything regarding this.

It would be very nice if this issue/feature was more looked upon/implemented.

@ItzSiL3Nce ItzSiL3Nce requested a review from a team as a code owner August 13, 2024 10:26
@ItzSiL3Nce ItzSiL3Nce removed the request for review from a team August 13, 2024 10:26
@ItzSiL3Nce ItzSiL3Nce requested a review from Weakky August 13, 2024 10:26
@midoelhawy
Copy link

@ItzSiL3Nce @janpio any news ?
i encountered the same problems while migrating from mongoose to prisma

@ItzSiL3Nce
Copy link
Author

@ItzSiL3Nce @janpio any news ? i encountered the same problems while migrating from mongoose to prisma

I'm waiting for it to be merged

@dottgonzo
Copy link

hi, any news on this? it seems very useful!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Feature A PR That introduces a new feature
Projects
None yet
7 participants