Skip to content

Commit

Permalink
me: fix regression in enum column type diffing
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
tomhoule committed Nov 9, 2022
1 parent eb39236 commit e23c254
Show file tree
Hide file tree
Showing 3 changed files with 87 additions and 1 deletion.
Expand Up @@ -54,7 +54,10 @@ impl SqlSchemaDifferFlavour for PostgresFlavour {

fn column_type_change(&self, columns: Pair<ColumnWalker<'_>>) -> Option<ColumnTypeChange> {
// Handle the enum cases first.
match columns.map(|col| col.column_type_family().as_enum()).into_tuple() {
match columns
.map(|col| col.column_type_family_as_enum().map(|e| e.name()))
.into_tuple()
{
(Some(previous_enum), Some(next_enum)) if previous_enum == next_enum => return None,
(Some(_), Some(_)) => return Some(ColumnTypeChange::NotCastable),
(None, Some(_)) | (Some(_), None) => return Some(ColumnTypeChange::NotCastable),
Expand Down
@@ -0,0 +1,32 @@
// tags=mysql

datasource mydb {
provider = "mysql"
url = env("TEST_DATABASE_URL")
}

model MyTable {
id Int @id
k Kind? @default(error)
r Result @default(succeeded)
}

enum Result {
succeeded
failed
}

enum Kind {
info
warning
error
}
// Expected Migration:
// -- CreateTable
// CREATE TABLE `MyTable` (
// `id` INTEGER NOT NULL,
// `k` ENUM('info', 'warning', 'error') NULL DEFAULT 'error',
// `r` ENUM('succeeded', 'failed') NOT NULL DEFAULT 'succeeded',
//
// PRIMARY KEY (`id`)
// ) DEFAULT CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci;
@@ -0,0 +1,51 @@
// tags=postgres
// exclude=cockroachdb

datasource mydb {
provider = "postgresql"
url = env("TEST_DATABASE_URL")
}

model MyTable {
id Int @id
a Activity @default(SLEEPING)
k Kind? @default(error)
r Result @default(succeeded)
}

enum Result {
succeeded
failed
}

enum Activity {
DANCING
SLEEPING
READING
}

enum Kind {
info
warning
error
}

// Expected Migration:
// -- CreateEnum
// CREATE TYPE "Result" AS ENUM ('succeeded', 'failed');
//
// -- CreateEnum
// CREATE TYPE "Activity" AS ENUM ('DANCING', 'SLEEPING', 'READING');
//
// -- CreateEnum
// CREATE TYPE "Kind" AS ENUM ('info', 'warning', 'error');
//
// -- CreateTable
// CREATE TABLE "MyTable" (
// "id" INTEGER NOT NULL,
// "a" "Activity" NOT NULL DEFAULT 'SLEEPING',
// "k" "Kind" DEFAULT 'error',
// "r" "Result" NOT NULL DEFAULT 'succeeded',
//
// CONSTRAINT "MyTable_pkey" PRIMARY KEY ("id")
// );

0 comments on commit e23c254

Please sign in to comment.