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.

Sign up for GitHub

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

Jump to bottom

fix(qe): NoAction should be alias of Restrict when relationMode = "prisma" #3276

Merged
merged 12 commits into from Oct 19, 2022
Merged

fix(qe): NoAction should be alias of Restrict when relationMode = "prisma" #3276

merged 12 commits into from Oct 19, 2022

Conversation

jkomyno
Copy link
Contributor

@jkomyno jkomyno commented Oct 11, 2022 •
edited

With this PR:

Contributes to prisma/prisma#15655.

@jkomyno jkomyno added this to the 4.5.0 milestone Oct 11, 2022
@jkomyno jkomyno mentioned this pull request Oct 11, 2022
Draft
@Jolg42 Jolg42 self-requested a review October 11, 2022 07:32
@jkomyno
Copy link
Contributor Author

jkomyno commented Oct 11, 2022 •
edited

We see tests using NoAction failing for mongodb on Buildkite. This is expected, as onUpdate: NoAction is an alias for onUpdate: Restrict, and the same holds for onDelete. Recall that, for mongodb, relationMode can only be "prisma".

@janpio
Copy link
Member

janpio commented Oct 11, 2022

Why are those supposed to fail? Did the expectations change with the rename?

@jkomyno jkomyno mentioned this pull request Oct 11, 2022
Merged
@Jolg42
Copy link
Member

Jolg42 commented Oct 11, 2022 •
edited

Currently, MongoDB (which also means referentialIntegrity="prisma") does "nothing" when set to NoAction, but this is fundamentally wrong, it should behave like Restrict.
So the previous assumption were "the query x should be successful", where it should have been "the query x fails because of NoAction (expected behavior)"

@janpio
Copy link
Member

janpio commented Oct 11, 2022

Ok, so not "expected" as in the sense of "this is the correct thing" but "expected that this fails now until I update the tests".

@jkomyno
Copy link
Contributor Author

jkomyno commented Oct 11, 2022

expected that this fails now until I update the tests

Correct

tomhoule
tomhoule reviewed Oct 11, 2022
View reviewed changes
Copy link
Contributor

@tomhoule tomhoule left a comment

Choose a reason for hiding this comment

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

lgtm, there's only the mongo QE tests question left 👍

query-engine/prisma-models/src/builders/relation_builder.rs Show resolved Hide resolved
@Jolg42 Jolg42 changed the title feat(qe): NoAction should be alias of Restrict when relationMode = "prisma" fix(qe): NoAction should be alias of Restrict when relationMode = "prisma" Oct 12, 2022
@jkomyno jkomyno force-pushed the feat/no-action-should-be-alias-of-restrict-when-relation-mode-is-prisma branch from 9e7c053 to 3c03d27 Compare October 12, 2022 13:16
@jkomyno
Copy link
Contributor Author

jkomyno commented Oct 12, 2022

Note: Tests are currently succeeding, but they should be changed.
relationMode is set by default to prisma on MongoDB (as expected), but it needs to be set explicitly for other databases.

Weakky
Weakky approved these changes Oct 13, 2022
View reviewed changes
Copy link
Member

@Weakky Weakky left a comment

Choose a reason for hiding this comment

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

Great job, thanks for taking the time to fix the tests 💯

@Jolg42 Jolg42 marked this pull request as ready for review October 14, 2022 07:38
@jkomyno
Copy link
Contributor Author

jkomyno commented Oct 16, 2022

The CI was still failing because Postgres / Sqlite no longer support NoAction when relationMode = "prisma".
My latest commit should fix that (hopefully).

@janpio janpio modified the milestones: 4.5.0, 4.6.0 Oct 17, 2022
janpio
janpio requested changes Oct 17, 2022
View reviewed changes
Copy link
Member

@janpio janpio left a comment

Choose a reason for hiding this comment

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

Tests are actually failing here, the last commit just wrongly makes it looks like things are passing.

@jkomyno jkomyno mentioned this pull request Oct 17, 2022
Closed
3 tasks
Weakky
Weakky approved these changes Oct 18, 2022
View reviewed changes
Copy link
Member

@Weakky Weakky left a comment

Choose a reason for hiding this comment

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

LGTM!

janpio
janpio requested changes Oct 18, 2022
View reviewed changes
Copy link
Member

@janpio janpio left a comment

Choose a reason for hiding this comment

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

https://prisma-company.slack.com/archives/C9D3H0P1A/p1666083278351679

@janpio janpio self-requested a review October 18, 2022 22:43
@jkomyno jkomyno merged commit c171e8f into main Oct 19, 2022
@jkomyno jkomyno deleted the feat/no-action-should-be-alias-of-restrict-when-relation-mode-is-prisma branch October 19, 2022 08:46
@jkomyno jkomyno mentioned this pull request Oct 19, 2022
Merged
@Songkeys Songkeys mentioned this pull request Feb 3, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomhoule tomhoule tomhoule left review comments

@Weakky Weakky Weakky approved these changes

@Jolg42 Jolg42 Awaiting requested review from Jolg42

@janpio janpio Awaiting requested review from janpio

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
4.6.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@jkomyno @janpio @Jolg42 @tomhoule @Weakky