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
me: fix regression w/ mapped enum names on postgres #3368
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When calculating a SQL schema from a Prisma schema, the Postgres connector of the migration engine would use the name of the enum instead of its database name (see parser-database docs for the reference on what these mean). To picture this with an example, the following schema. ``` enum PersonType { Dog Cat @@Map("pet_preference") } ``` would correspond, before the regression, and after this commit, to: ``` CREATE TYPE "pet_preference" AS ENUM ('Dog', 'Cat'); ``` but the regression caused it to correspond to: ``` CREATE TYPE "PersonType" AS ENUM ('Dog', 'Cat'); ``` This does not only affect newly created enums: we diff the wrong enums, causing bad migrations for existing users of Migrate that have the enums with the correct (mapped) names in their database. Users reported the regression in prisma/prisma#16180 This commit should completely fix the problem. The regression was introduced in an unrelated refactoring in b6400f1. Enum names in column types were replace with enum ids — a win for correctness. As part of this refactoring, the `sql_schema_describer::Enum` type was made private, and adding enums to a schema became possible only through `SqlSchema::push_enum`. In the SQL migration connector What could have caught this: - Testing that the name of the database enum in the migration is the mapped name. We can test this: - Through `.assert_schema()`, which has existed for a long time. It is a gap in our test coverage that we weren't testing this. - The new single migration tests, like the test in this commit. They are new, and they are less work than the previous option. - migrations-ci could have caught this, but it is currently neglected.
Looking into the vitess test failures. edit: they're almost certainly unrelated, see the PR in my next comment. |
garrensmith
approved these changes
Nov 9, 2022
tomhoule
added a commit
that referenced
this pull request
Nov 9, 2022
* me: fix regression w/ mapped enum names on postgres When calculating a SQL schema from a Prisma schema, the Postgres connector of the migration engine would use the name of the enum instead of its database name (see parser-database docs for the reference on what these mean). To picture this with an example, the following schema. ``` enum PersonType { Dog Cat @@Map("pet_preference") } ``` would correspond, before the regression, and after this commit, to: ``` CREATE TYPE "pet_preference" AS ENUM ('Dog', 'Cat'); ``` but the regression caused it to correspond to: ``` CREATE TYPE "PersonType" AS ENUM ('Dog', 'Cat'); ``` This does not only affect newly created enums: we diff the wrong enums, causing bad migrations for existing users of Migrate that have the enums with the correct (mapped) names in their database. Users reported the regression in prisma/prisma#16180 This commit should completely fix the problem. The regression was introduced in an unrelated refactoring in b6400f1. Enum names in column types were replace with enum ids — a win for correctness. As part of this refactoring, the `sql_schema_describer::Enum` type was made private, and adding enums to a schema became possible only through `SqlSchema::push_enum`. In the SQL migration connector What could have caught this: - Testing that the name of the database enum in the migration is the mapped name. We can test this: - Through `.assert_schema()`, which has existed for a long time. It is a gap in our test coverage that we weren't testing this. - The new single migration tests, like the test in this commit. They are new, and they are less work than the previous option. - migrations-ci could have caught this, but it is currently neglected. * me: move enum test to enums directory
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When calculating a SQL schema from a Prisma schema, the Postgres
connector of the migration engine would use the name of the enum instead
of its database name (see parser-database docs for the reference on what
these mean). To picture this with an example, the following schema.
would correspond, before the regression, and after this commit, to:
but the regression caused it to correspond to:
This does not only affect newly created enums: we diff the wrong enums,
causing bad migrations for existing users of Migrate that have the enums
with the correct (mapped) names in their database.
Users reported the regression in prisma/prisma#16180
This commit should completely fix the problem.
The regression was introduced in an unrelated refactoring in
b6400f1. Enum names in column types
were replace with enum ids — a win for correctness. As part of this
refactoring, the
sql_schema_describer::Enum
type was made private, andadding enums to a schema became possible only through
SqlSchema::push_enum
. In the SQL migration connectorWhat could have caught this:
mapped name. We can test this:
.assert_schema()
, which has existed for a long time.It is a gap in our test coverage that we weren't testing this.
They are new, and they are less work than the previous option.