From 587ec25ce6982280d8403b0bb2ff6b14b6fc3c7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=20Houl=C3=A9?= Date: Mon, 29 Aug 2022 15:07:25 +0200 Subject: [PATCH] me: never try to migrate a column with @default(dbgenerated()) closes https://github.com/prisma/prisma/issues/14799 --- Cargo.lock | 8 ++-- .../src/defaults.rs | 4 +- libs/sql-schema-describer/src/lib.rs | 4 +- libs/sql-schema-describer/src/postgres.rs | 2 +- .../src/postgres/default.rs | 4 +- .../tests/describers/mysql_describer_tests.rs | 44 ++++++++++++++----- .../src/sql_renderer/mssql_renderer.rs | 3 +- .../src/sql_renderer/mysql_renderer.rs | 10 ++--- .../src/sql_renderer/postgres_renderer.rs | 4 +- .../src/sql_renderer/sqlite_renderer.rs | 11 +++-- .../src/sql_schema_calculator.rs | 5 +-- .../src/sql_schema_differ/column.rs | 3 +- .../sql_schema_differ_flavour/sqlite.rs | 4 ++ .../migration-engine-tests/src/assertions.rs | 2 +- .../tests/migrations/defaults.rs | 4 ++ .../tests/migrations/postgres.rs | 29 ++++++++++++ .../postgres/empty_dbgenerated.prisma | 20 +++++++++ 17 files changed, 124 insertions(+), 37 deletions(-) create mode 100644 migration-engine/migration-engine-tests/tests/single_migration_tests/postgres/empty_dbgenerated.prisma diff --git a/Cargo.lock b/Cargo.lock index 33a5d7784e2d..fedef39cf1b4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -686,18 +686,14 @@ dependencies = [ "chrono", "datamodel-connector", "diagnostics", - "dissimilar", "dml", "either", "enumflags2", - "expect-test", "indoc", "itertools", "mongodb-datamodel-connector", - "native-types", "once_cell", "parser-database", - "pretty_assertions", "regex", "schema-ast", "serde", @@ -3138,6 +3134,10 @@ name = "psl" version = "0.1.0" dependencies = [ "datamodel", + "dissimilar", + "expect-test", + "indoc", + "native-types", ] [[package]] diff --git a/introspection-engine/connectors/sql-introspection-connector/src/defaults.rs b/introspection-engine/connectors/sql-introspection-connector/src/defaults.rs index 762f4c25fd99..908cf77728ea 100644 --- a/introspection-engine/connectors/sql-introspection-connector/src/defaults.rs +++ b/introspection-engine/connectors/sql-introspection-connector/src/defaults.rs @@ -60,7 +60,9 @@ pub(crate) fn calculate_default(column: sql::ColumnWalker<'_>, ctx: &mut Context column, )), (Some(sql::DefaultKind::DbGenerated(default_string)), _) => Some(set_default( - dml::DefaultValue::new_expression(dml::ValueGenerator::new_dbgenerated(default_string.clone())), + dml::DefaultValue::new_expression(dml::ValueGenerator::new_dbgenerated( + default_string.as_ref().unwrap().clone(), + )), column, )), (Some(sql::DefaultKind::Value(val)), _) => { diff --git a/libs/sql-schema-describer/src/lib.rs b/libs/sql-schema-describer/src/lib.rs index caa86bd8b6b7..3174fe65c995 100644 --- a/libs/sql-schema-describer/src/lib.rs +++ b/libs/sql-schema-describer/src/lib.rs @@ -584,12 +584,12 @@ pub enum DefaultKind { /// A unique row ID, UniqueRowid, /// An unrecognized Default Value - DbGenerated(String), + DbGenerated(Option), } impl DefaultValue { pub fn db_generated(val: impl Into) -> Self { - Self::new(DefaultKind::DbGenerated(val.into())) + Self::new(DefaultKind::DbGenerated(Some(val.into()))) } pub fn now() -> Self { diff --git a/libs/sql-schema-describer/src/postgres.rs b/libs/sql-schema-describer/src/postgres.rs index dd938a00f20d..1aed3060e1da 100644 --- a/libs/sql-schema-describer/src/postgres.rs +++ b/libs/sql-schema-describer/src/postgres.rs @@ -636,7 +636,7 @@ impl<'a> SqlSchemaDescriber<'a> { || (self.is_cockroach() && matches!( default.as_ref().map(|d| d.kind()), - Some(DefaultKind::DbGenerated(s)) if s == "unique_rowid()" + Some(DefaultKind::DbGenerated(Some(s))) if s == "unique_rowid()" )); let col = Column { diff --git a/libs/sql-schema-describer/src/postgres/default.rs b/libs/sql-schema-describer/src/postgres/default.rs index 757f4b1ec6fb..51142bb185a4 100644 --- a/libs/sql-schema-describer/src/postgres/default.rs +++ b/libs/sql-schema-describer/src/postgres/default.rs @@ -88,7 +88,7 @@ pub(super) fn get_default_value(default_string: &str, tpe: &ColumnType) -> Optio Some(match parsed_default { Some(default_value) => default_value, - None => DefaultValue::db_generated(default_string), + None => DefaultValue::db_generated(default_string.to_owned()), }) } @@ -504,7 +504,7 @@ fn get_list_default_value(parser: &mut Parser<'_>, tpe: &ColumnType) -> DefaultV values .map(|values| DefaultValue::value(PrismaValue::List(values))) - .unwrap_or_else(|| DefaultValue::db_generated(parser.input)) + .unwrap_or_else(|| DefaultValue::db_generated(parser.input.to_owned())) } /// Some(()) on valid cast or absence of cast. None if we can't make sense of the input. diff --git a/libs/sql-schema-describer/tests/describers/mysql_describer_tests.rs b/libs/sql-schema-describer/tests/describers/mysql_describer_tests.rs index d6bea14dee77..645da1abb57b 100644 --- a/libs/sql-schema-describer/tests/describers/mysql_describer_tests.rs +++ b/libs/sql-schema-describer/tests/describers/mysql_describer_tests.rs @@ -3359,7 +3359,9 @@ fn function_expression_defaults_are_described_as_dbgenerated(api: TestApi) { default: Some( DefaultValue { kind: DbGenerated( - "(abs(8) + abs(8))", + Some( + "(abs(8) + abs(8))", + ), ), constraint_name: None, }, @@ -3386,7 +3388,9 @@ fn function_expression_defaults_are_described_as_dbgenerated(api: TestApi) { default: Some( DefaultValue { kind: DbGenerated( - "(abs(8))", + Some( + "(abs(8))", + ), ), constraint_name: None, }, @@ -3413,7 +3417,9 @@ fn function_expression_defaults_are_described_as_dbgenerated(api: TestApi) { default: Some( DefaultValue { kind: DbGenerated( - "(abs(8))", + Some( + "(abs(8))", + ), ), constraint_name: None, }, @@ -3447,7 +3453,9 @@ fn function_expression_defaults_are_described_as_dbgenerated(api: TestApi) { default: Some( DefaultValue { kind: DbGenerated( - "(abs(8))", + Some( + "(abs(8))", + ), ), constraint_name: None, }, @@ -3474,7 +3482,9 @@ fn function_expression_defaults_are_described_as_dbgenerated(api: TestApi) { default: Some( DefaultValue { kind: DbGenerated( - "(ifnull(1,0))", + Some( + "(ifnull(1,0))", + ), ), constraint_name: None, }, @@ -3503,7 +3513,9 @@ fn function_expression_defaults_are_described_as_dbgenerated(api: TestApi) { default: Some( DefaultValue { kind: DbGenerated( - "(left(uuid(),8))", + Some( + "(left(uuid(),8))", + ), ), constraint_name: None, }, @@ -3559,7 +3571,9 @@ fn function_expression_defaults_are_described_as_dbgenerated(api: TestApi) { default: Some( DefaultValue { kind: DbGenerated( - "(sysdate() - interval 31 day)", + Some( + "(sysdate() - interval 31 day)", + ), ), constraint_name: None, }, @@ -3588,7 +3602,9 @@ fn function_expression_defaults_are_described_as_dbgenerated(api: TestApi) { default: Some( DefaultValue { kind: DbGenerated( - "(conv(10,10,2))", + Some( + "(conv(10,10,2))", + ), ), constraint_name: None, }, @@ -3615,7 +3631,9 @@ fn function_expression_defaults_are_described_as_dbgenerated(api: TestApi) { default: Some( DefaultValue { kind: DbGenerated( - "(trim(_utf8mb4\\'{} \\'))", + Some( + "(trim(_utf8mb4\\'{} \\'))", + ), ), constraint_name: None, }, @@ -3640,7 +3658,9 @@ fn function_expression_defaults_are_described_as_dbgenerated(api: TestApi) { default: Some( DefaultValue { kind: DbGenerated( - "(trim(_utf8mb4\\'x-small \\'))", + Some( + "(trim(_utf8mb4\\'x-small \\'))", + ), ), constraint_name: None, }, @@ -3663,7 +3683,9 @@ fn function_expression_defaults_are_described_as_dbgenerated(api: TestApi) { default: Some( DefaultValue { kind: DbGenerated( - "(trim(_utf8mb4\\' \\'))", + Some( + "(trim(_utf8mb4\\' \\'))", + ), ), constraint_name: None, }, diff --git a/migration-engine/connectors/sql-migration-connector/src/sql_renderer/mssql_renderer.rs b/migration-engine/connectors/sql-migration-connector/src/sql_renderer/mssql_renderer.rs index d8f001d08de3..8210ae5f4987 100644 --- a/migration-engine/connectors/sql-migration-connector/src/sql_renderer/mssql_renderer.rs +++ b/migration-engine/connectors/sql-migration-connector/src/sql_renderer/mssql_renderer.rs @@ -37,6 +37,7 @@ impl MssqlFlavour { } else { column .default() + .filter(|d| !matches!(d.kind(), sql::DefaultKind::DbGenerated(None))) .map(|default| { // named constraints let constraint_name = default @@ -513,7 +514,7 @@ fn escape_string_literal(s: &str) -> String { fn render_default(default: &sql::DefaultValue) -> Cow<'_, str> { match default.kind() { - sql::DefaultKind::DbGenerated(val) => val.as_str().into(), + sql::DefaultKind::DbGenerated(val) => val.as_ref().unwrap().as_str().into(), sql::DefaultKind::Value(PrismaValue::String(val)) | sql::DefaultKind::Value(PrismaValue::Enum(val)) => { Quoted::mssql_string(escape_string_literal(val)).to_string().into() } diff --git a/migration-engine/connectors/sql-migration-connector/src/sql_renderer/mysql_renderer.rs b/migration-engine/connectors/sql-migration-connector/src/sql_renderer/mysql_renderer.rs index 87ccf70ab340..2a4f4e000899 100644 --- a/migration-engine/connectors/sql-migration-connector/src/sql_renderer/mysql_renderer.rs +++ b/migration-engine/connectors/sql-migration-connector/src/sql_renderer/mysql_renderer.rs @@ -27,7 +27,7 @@ impl MysqlFlavour { default: col .default() .filter(|default| { - !matches!(default.kind(), DefaultKind::Sequence(_)) + !matches!(default.kind(), DefaultKind::Sequence(_) | DefaultKind::DbGenerated(None)) // We do not want to render JSON defaults because // they are not supported by MySQL. && !matches!(col.column_type_family(), ColumnTypeFamily::Json) @@ -488,8 +488,8 @@ impl MysqlAlterColumn { // @default(dbgenerated()) does not give us the information in the prisma schema, so we have to // transfer it from the introspected current state of the database. let new_default = match defaults { - (Some(DefaultKind::DbGenerated(previous)), Some(DefaultKind::DbGenerated(next))) - if next.is_empty() && !previous.is_empty() => + (Some(DefaultKind::DbGenerated(Some(previous))), Some(DefaultKind::DbGenerated(next))) + if (next.is_none() || next.as_deref() == Some("")) && !previous.is_empty() => { Some(DefaultValue::db_generated(previous.clone())) } @@ -502,7 +502,7 @@ impl MysqlAlterColumn { fn render_default<'a>(column: ColumnWalker<'a>, default: &'a DefaultValue) -> Cow<'a, str> { match default.kind() { - DefaultKind::DbGenerated(val) => val.as_str().into(), + DefaultKind::DbGenerated(Some(val)) => val.as_str().into(), DefaultKind::Value(PrismaValue::String(val)) | DefaultKind::Value(PrismaValue::Enum(val)) => { Quoted::mysql_string(escape_string_literal(val)).to_string().into() } @@ -519,6 +519,6 @@ fn render_default<'a>(column: ColumnWalker<'a>, default: &'a DefaultValue) -> Co Quoted::mysql_string(dt.to_rfc3339()).to_string().into() } DefaultKind::Value(val) => val.to_string().into(), - DefaultKind::Sequence(_) | DefaultKind::UniqueRowid => unreachable!(), + DefaultKind::DbGenerated(None) | DefaultKind::Sequence(_) | DefaultKind::UniqueRowid => unreachable!(), } } diff --git a/migration-engine/connectors/sql-migration-connector/src/sql_renderer/postgres_renderer.rs b/migration-engine/connectors/sql-migration-connector/src/sql_renderer/postgres_renderer.rs index 86cd4c0c76dc..6c041623166d 100644 --- a/migration-engine/connectors/sql-migration-connector/src/sql_renderer/postgres_renderer.rs +++ b/migration-engine/connectors/sql-migration-connector/src/sql_renderer/postgres_renderer.rs @@ -796,14 +796,14 @@ fn render_default<'a>(default: &'a DefaultValue, full_data_type: &str) -> Cow<'a } match default.kind() { - DefaultKind::DbGenerated(val) => Cow::Borrowed(val.as_str()), + DefaultKind::DbGenerated(Some(val)) => Cow::Borrowed(val.as_str()), DefaultKind::Value(PrismaValue::String(val)) | DefaultKind::Value(PrismaValue::Enum(val)) => { format!("'{}'", escape_string_literal(val)).into() } DefaultKind::Now => "CURRENT_TIMESTAMP".into(), DefaultKind::Value(value) => render_constant_default(value, full_data_type), DefaultKind::UniqueRowid => "unique_rowid()".into(), - DefaultKind::Sequence(_) => Default::default(), + DefaultKind::Sequence(_) | DefaultKind::DbGenerated(None) => Default::default(), } } diff --git a/migration-engine/connectors/sql-migration-connector/src/sql_renderer/sqlite_renderer.rs b/migration-engine/connectors/sql-migration-connector/src/sql_renderer/sqlite_renderer.rs index b033f34a872c..acb825f823d1 100644 --- a/migration-engine/connectors/sql-migration-connector/src/sql_renderer/sqlite_renderer.rs +++ b/migration-engine/connectors/sql-migration-connector/src/sql_renderer/sqlite_renderer.rs @@ -311,7 +311,12 @@ fn render_column<'a>(column: &ColumnWalker<'a>) -> ddl::Column<'a> { autoincrement: column.is_single_primary_key() && column.column_type_family().is_int(), default: column .default() - .filter(|default| !matches!(default.kind(), DefaultKind::Sequence(_))) + .filter(|default| { + !matches!( + default.kind(), + DefaultKind::Sequence(_) | DefaultKind::DbGenerated(None) + ) + }) .map(render_default), name: column.name().into(), not_null: !column.arity().is_nullable(), @@ -322,7 +327,7 @@ fn render_column<'a>(column: &ColumnWalker<'a>) -> ddl::Column<'a> { fn render_default(default: &DefaultValue) -> Cow<'_, str> { match default.kind() { - DefaultKind::DbGenerated(val) => val.as_str().into(), + DefaultKind::DbGenerated(Some(val)) => val.as_str().into(), DefaultKind::Value(PrismaValue::String(val)) | DefaultKind::Value(PrismaValue::Enum(val)) => { Quoted::sqlite_string(escape_quotes(val)).to_string().into() } @@ -334,6 +339,6 @@ fn render_default(default: &DefaultValue) -> Cow<'_, str> { DefaultKind::Now => "CURRENT_TIMESTAMP".into(), DefaultKind::Value(PrismaValue::DateTime(val)) => Quoted::sqlite_string(val).to_string().into(), DefaultKind::Value(val) => val.to_string().into(), - DefaultKind::Sequence(_) | DefaultKind::UniqueRowid => unreachable!(), + DefaultKind::DbGenerated(None) | DefaultKind::Sequence(_) | DefaultKind::UniqueRowid => unreachable!(), } } diff --git a/migration-engine/connectors/sql-migration-connector/src/sql_schema_calculator.rs b/migration-engine/connectors/sql-migration-connector/src/sql_schema_calculator.rs index 0d0b544cbc5e..30c117a9ae4e 100644 --- a/migration-engine/connectors/sql-migration-connector/src/sql_schema_calculator.rs +++ b/migration-engine/connectors/sql-migration-connector/src/sql_schema_calculator.rs @@ -462,9 +462,8 @@ fn push_column_for_builtin_scalar_type( let default: Option = field.default_value().map(|v| { let column_default = { if v.is_dbgenerated() { - unwrap_dbgenerated(v.value()) - .map(|v| ColumnDefault::Available(sql::DefaultValue::new(sql::DefaultKind::DbGenerated(v)))) - .unwrap_or(ColumnDefault::NA) + let value = unwrap_dbgenerated(v.value()); + ColumnDefault::Available(sql::DefaultValue::new(sql::DefaultKind::DbGenerated(value))) } else if v.is_now() { ColumnDefault::Available(sql::DefaultValue::now()) } else if v.is_autoincrement() { diff --git a/migration-engine/connectors/sql-migration-connector/src/sql_schema_differ/column.rs b/migration-engine/connectors/sql-migration-connector/src/sql_schema_differ/column.rs index f792db010364..e293b67a3573 100644 --- a/migration-engine/connectors/sql-migration-connector/src/sql_schema_differ/column.rs +++ b/migration-engine/connectors/sql-migration-connector/src/sql_schema_differ/column.rs @@ -92,6 +92,7 @@ fn defaults_match(cols: Pair>, flavour: &dyn SqlFlavour) -> boo (Some(DefaultKind::DbGenerated(_)), Some(DefaultKind::Value(_))) => false, (Some(DefaultKind::DbGenerated(_)), Some(DefaultKind::Now)) => false, (Some(DefaultKind::DbGenerated(_)), None) => false, + (_, Some(DefaultKind::DbGenerated(None))) => true, (Some(DefaultKind::Sequence(_)), None) => true, // sequences are dropped separately (Some(DefaultKind::Sequence(_)), Some(DefaultKind::Value(_))) => false, @@ -104,7 +105,7 @@ fn defaults_match(cols: Pair>, flavour: &dyn SqlFlavour) -> boo (None, Some(DefaultKind::Value(_))) => false, (None, Some(DefaultKind::Now)) => false, - (Some(DefaultKind::DbGenerated(prev)), Some(DefaultKind::DbGenerated(next))) => { + (Some(DefaultKind::DbGenerated(Some(prev))), Some(DefaultKind::DbGenerated(Some(next)))) => { (prev.eq_ignore_ascii_case(next)) && names_match } (_, Some(DefaultKind::DbGenerated(_))) => false, diff --git a/migration-engine/connectors/sql-migration-connector/src/sql_schema_differ/sql_schema_differ_flavour/sqlite.rs b/migration-engine/connectors/sql-migration-connector/src/sql_schema_differ/sql_schema_differ_flavour/sqlite.rs index f4588c59899f..b0f272fb2705 100644 --- a/migration-engine/connectors/sql-migration-connector/src/sql_schema_differ/sql_schema_differ_flavour/sqlite.rs +++ b/migration-engine/connectors/sql-migration-connector/src/sql_schema_differ/sql_schema_differ_flavour/sqlite.rs @@ -18,6 +18,10 @@ impl SqlSchemaDifferFlavour for SqliteFlavour { false } + fn column_autoincrement_changed(&self, _columns: Pair>) -> bool { + false + } + fn column_type_change(&self, differ: Pair>) -> Option { match (differ.previous.column_type_family(), differ.next.column_type_family()) { (a, b) if a == b => None, diff --git a/migration-engine/migration-engine-tests/src/assertions.rs b/migration-engine/migration-engine-tests/src/assertions.rs index cbfc5720d67a..55e121bc24ea 100644 --- a/migration-engine/migration-engine-tests/src/assertions.rs +++ b/migration-engine/migration-engine-tests/src/assertions.rs @@ -423,7 +423,7 @@ impl<'a> ColumnAssertion<'a> { let found = self.column.default(); match found.map(|d| d.kind()) { - Some(DefaultKind::DbGenerated(val)) => assert!( + Some(DefaultKind::DbGenerated(Some(val))) => assert!( val == expected, "Assertion failed. Expected the default value for `{}` to be dbgenerated with `{:?}`, got `{:?}`", self.column.name(), diff --git a/migration-engine/migration-engine-tests/tests/migrations/defaults.rs b/migration-engine/migration-engine-tests/tests/migrations/defaults.rs index 5f5f3b4f5f12..c1292da448c0 100644 --- a/migration-engine/migration-engine-tests/tests/migrations/defaults.rs +++ b/migration-engine/migration-engine-tests/tests/migrations/defaults.rs @@ -293,6 +293,10 @@ fn schemas_with_dbgenerated_work(api: TestApi) { "#; api.schema_push_w_datasource(dm1).send().assert_green(); + api.schema_push_w_datasource(dm1) + .send() + .assert_green() + .assert_no_steps(); } #[test_connector(tags(Mysql8, Mariadb), exclude(Vitess))] diff --git a/migration-engine/migration-engine-tests/tests/migrations/postgres.rs b/migration-engine/migration-engine-tests/tests/migrations/postgres.rs index 63c184ecac73..2d2d56209bde 100644 --- a/migration-engine/migration-engine-tests/tests/migrations/postgres.rs +++ b/migration-engine/migration-engine-tests/tests/migrations/postgres.rs @@ -724,3 +724,32 @@ fn bigint_defaults_work(api: TestApi) { api.schema_push(schema).send().assert_green(); api.schema_push(schema).send().assert_green().assert_no_steps(); } + +// https://github.com/prisma/prisma/issues/14799 +#[test_connector(tags(Postgres12), exclude(CockroachDb))] +fn dbgenerated_on_generated_columns_is_idempotent(api: TestApi) { + let sql = r#" + CREATE TABLE "table" ( + "id" TEXT NOT NULL, + "hereBeDragons" TEXT NOT NULL GENERATED ALWAYS AS ('this row ID is: '::text || "id") STORED, + + CONSTRAINT "table_pkey" PRIMARY KEY ("id") + ); + "#; + + api.raw_cmd(sql); + + let schema = r#" + datasource db { + provider = "postgresql" + url = env("TEST_DATABASE_URL") + } + + model table { + id String @id + hereBeDragons String @default(dbgenerated()) + } + "#; + + api.schema_push(schema).send().assert_green().assert_no_steps(); +} diff --git a/migration-engine/migration-engine-tests/tests/single_migration_tests/postgres/empty_dbgenerated.prisma b/migration-engine/migration-engine-tests/tests/single_migration_tests/postgres/empty_dbgenerated.prisma new file mode 100644 index 000000000000..b00a90ab05e5 --- /dev/null +++ b/migration-engine/migration-engine-tests/tests/single_migration_tests/postgres/empty_dbgenerated.prisma @@ -0,0 +1,20 @@ +// tags=postgres +// exclude=cockroachdb + +datasource testds { + provider = "postgresql" + url = env("TEST_DATABASE_URL") +} + +model table { + id String @id + hereBeDragons String @default(dbgenerated()) +} + +// -- CreateTable +// CREATE TABLE "table" ( +// "id" TEXT NOT NULL, +// "hereBeDragons" TEXT NOT NULL, +// +// CONSTRAINT "table_pkey" PRIMARY KEY ("id") +// );