Rethink PartialEq implementation for sql-schema-describer::ColumnTypeFamily #3373
Labels
kind/tech
A technical change.
team/schema
Issue for team Schema.
tech/engines
Issue for tech Engines.
Comments
tomhoule
added
process/candidate
kind/tech
tomhoule
added a commit
that referenced
this issue
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. 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 is chiming 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
added a commit
that referenced
this issue
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.
tomhoule
added a commit
that referenced
this issue
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.
tomhoule
added a commit
that referenced
this issue
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
It compares IDs, which we absolutely do not want in the migration engine differ, but is harmless everywhere else. Should we break up ColumnTypeFamily variants into scalar and enum and unsupported variants? Remove the PartialEq impl?
#3372
The text was updated successfully, but these errors were encountered: