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 in enum column type diffing #3374
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
This regression was reported by users in prisma/prisma#16180 It was introduced on b6400f1 by the same change that caused the _other_ regression that was fixed in eb39236. Symptoms: enum migrations became non-idempotent in many cases, depending on the ordering of the migrations in the Prisma schema. In that refactoring, we started storing enum ids instead of enum names in `ColumnTypeFamily::Enum(_)`. That makes the `ColumntypeFamily` type smaller, easier to deal with with regards to ownership (`EnumId` is `Copy`), and ready for multi-schema, because the name can be ambiguous and does not carry schema information, whereas the `EnumId` does. But more relevant to this commit: it makes comparing two `ColumnTypeFamily` inaccurate _when the types come from two different schemas_, like in sql_schema_differ. That in turn changed the signature of `column_type_family_as_enum()`, leading to the differ comparing enum ids instead of enum names, which directly causes the false positives in diffing, because the same enum has a different ID in each of the two schemas. The code that caused the regression does not appear in b6400f1, it uses code that was changed there. This is also a problem with the generated implementation of PartialEq for ColumnTypeFamily. Since it would be a larger refactoring, we only deal with this where relevant (sql_schema_differ) and leave removing the dangerous PartialEq implementation to another PR. This is the issue: #3373. How could this have been caught? - Tests that check that migrations are idempotent with multiple enums, all used in model columns _not defined in alphabetical order_ (sad but true). - This chimes with the theme of us not testing with large enough Prisma schemas. - migrations-ci could have caught this. We are neglecting it at the moment.
tomhoule
force-pushed
the
me/fix-enum-migrations-regression-without-map
branch
from
November 9, 2022 13:37
45be4e0
to
e23c254
Compare
eviefp
approved these changes
Nov 9, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
tomhoule
added a commit
that referenced
this pull request
Nov 9, 2022
This regression was reported by users in prisma/prisma#16180 It was introduced on b6400f1 by the same change that caused the _other_ regression that was fixed in eb39236. Symptoms: enum migrations became non-idempotent in many cases, depending on the ordering of the migrations in the Prisma schema. In that refactoring, we started storing enum ids instead of enum names in `ColumnTypeFamily::Enum(_)`. That makes the `ColumntypeFamily` type smaller, easier to deal with with regards to ownership (`EnumId` is `Copy`), and ready for multi-schema, because the name can be ambiguous and does not carry schema information, whereas the `EnumId` does. But more relevant to this commit: it makes comparing two `ColumnTypeFamily` inaccurate _when the types come from two different schemas_, like in sql_schema_differ. That in turn changed the signature of `column_type_family_as_enum()`, leading to the differ comparing enum ids instead of enum names, which directly causes the false positives in diffing, because the same enum has a different ID in each of the two schemas. The code that caused the regression does not appear in b6400f1, it uses code that was changed there. This is also a problem with the generated implementation of PartialEq for ColumnTypeFamily. Since it would be a larger refactoring, we only deal with this where relevant (sql_schema_differ) and leave removing the dangerous PartialEq implementation to another PR. This is the issue: #3373. How could this have been caught? - Tests that check that migrations are idempotent with multiple enums, all used in model columns _not defined in alphabetical order_ (sad but true). - This chimes with the theme of us not testing with large enough Prisma schemas. - migrations-ci could have caught this. We are neglecting it at the moment.
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.
This regression was reported by users in
prisma/prisma#16180
It was introduced on b6400f1 by the
same change that caused the other regression that was fixed in
eb39236.
Symptoms: enum migrations became non-idempotent in many cases, depending
on the ordering of the migrations in the Prisma schema.
In that refactoring, we started storing enum ids instead of enum names
in
ColumnTypeFamily::Enum(_)
. That makes theColumntypeFamily
typesmaller, easier to deal with with regards to ownership (
EnumId
isCopy
), and ready for multi-schema, because the name can be ambiguousand does not carry schema information, whereas the
EnumId
does. Butmore relevant to this commit: it makes comparing two
ColumnTypeFamily
inaccurate when the types come from two differentschemas, like in sql_schema_differ.
That in turn changed the signature of
column_type_family_as_enum()
,leading to the differ comparing enum ids instead of enum names, which
directly causes the false positives in diffing, because the same enum
has a different ID in each of the two schemas. The code that caused the
regression does not appear in b6400f1,
it uses code that was changed there.
This is also a problem with the generated implementation of PartialEq
for ColumnTypeFamily. Since it would be a larger refactoring, we only
deal with this where relevant (sql_schema_differ) and leave removing the
dangerous PartialEq implementation to another PR. This is the issue:
#3373.
How could this have been caught?
all used in model columns not defined in alphabetical order (sad but true).
Prisma Schemas.
moment.