From e23c2543e0ace716d5c96c102888b079a8d208e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=20Houl=C3=A9?= Date: Wed, 9 Nov 2022 13:58:50 +0100 Subject: [PATCH] me: fix regression in enum column type diffing This regression was reported by users in https://github.com/prisma/prisma/issues/16180 It was introduced on b6400f10407836a032b36e9e084abc7b5f602933 by the same change that caused the _other_ regression that was fixed in eb39236598ac57256952cacb32c2f6cf0f97eadc. 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 b6400f10407836a032b36e9e084abc7b5f602933, 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: https://github.com/prisma/prisma-engines/issues/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. --- .../sql_schema_differ_flavour/postgres.rs | 5 +- .../mysql/enums/enum_fields.prisma | 32 ++++++++++++ .../postgres/enums/enum_fields.prisma | 51 +++++++++++++++++++ 3 files changed, 87 insertions(+), 1 deletion(-) create mode 100644 migration-engine/migration-engine-tests/tests/single_migration_tests/mysql/enums/enum_fields.prisma create mode 100644 migration-engine/migration-engine-tests/tests/single_migration_tests/postgres/enums/enum_fields.prisma diff --git a/migration-engine/connectors/sql-migration-connector/src/sql_schema_differ/sql_schema_differ_flavour/postgres.rs b/migration-engine/connectors/sql-migration-connector/src/sql_schema_differ/sql_schema_differ_flavour/postgres.rs index adac95313b73..8ad23ee9f15e 100644 --- a/migration-engine/connectors/sql-migration-connector/src/sql_schema_differ/sql_schema_differ_flavour/postgres.rs +++ b/migration-engine/connectors/sql-migration-connector/src/sql_schema_differ/sql_schema_differ_flavour/postgres.rs @@ -54,7 +54,10 @@ impl SqlSchemaDifferFlavour for PostgresFlavour { fn column_type_change(&self, columns: Pair>) -> Option { // 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), diff --git a/migration-engine/migration-engine-tests/tests/single_migration_tests/mysql/enums/enum_fields.prisma b/migration-engine/migration-engine-tests/tests/single_migration_tests/mysql/enums/enum_fields.prisma new file mode 100644 index 000000000000..04286a0151b1 --- /dev/null +++ b/migration-engine/migration-engine-tests/tests/single_migration_tests/mysql/enums/enum_fields.prisma @@ -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; diff --git a/migration-engine/migration-engine-tests/tests/single_migration_tests/postgres/enums/enum_fields.prisma b/migration-engine/migration-engine-tests/tests/single_migration_tests/postgres/enums/enum_fields.prisma new file mode 100644 index 000000000000..4285e0eb71e8 --- /dev/null +++ b/migration-engine/migration-engine-tests/tests/single_migration_tests/postgres/enums/enum_fields.prisma @@ -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") +// );