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

fix(se): render constraint renames as separate sql statements #4906

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

Conversation

eruditmorina
Copy link

@eruditmorina eruditmorina commented Jun 5, 2024

When applying/generating migrations, renaming primary key constraints in SQL (Postgres) requires statements to be separate from other table alter statements.

If we have multiple changes in the same table, the rename should be rendered as a separate sql statement otherwise it will throw a database error (picture below):

Screenshot 2024-06-05 at 15 43 25

In this PR, I excluded the RenamePrimaryKey from alter table lines and am appending it to after_statements to be executed as a separate ALTER TABLE statement, that way the corresponding sql statements would render correctly.

Related issue:

@eruditmorina eruditmorina requested a review from a team as a code owner June 5, 2024 14:24
@eruditmorina eruditmorina requested review from Weakky and removed request for a team June 5, 2024 14:24
@janpio
Copy link
Member

janpio commented Jun 5, 2024

Neat.

Could you add a test that shows this working now (and failing before your change)?

Problem with prisma/prisma#18274 was that it does not have concrete steps for us to reproduce the problem - from your decsription above I assume this only applies for migration where a primary key is renamed while other things happen in the table?

Copy link

codspeed-hq bot commented Jun 5, 2024

CodSpeed Performance Report

Merging #4906 will not alter performance

Comparing eruditmorina:eruditmorina/schema-engine/fix-invalid-rename-primary-key-constraint (dbc585b) with main (9ee8c02)

Summary

✅ 11 untouched benchmarks

@CLAassistant
Copy link

CLAassistant commented Jun 5, 2024

CLA assistant check
All committers have signed the CLA.

@eruditmorina eruditmorina marked this pull request as draft June 6, 2024 12:54
@eruditmorina eruditmorina marked this pull request as ready for review June 7, 2024 07:24
@eruditmorina
Copy link
Author

@janpio as instructed, I added tests that show the correct rendering of the migration.

And I can confirm that as you put it, the problem applies for migrations where primary key is renamed, together with other changes in the same table, as part of the same migration.

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.

None yet

3 participants