Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(me): fields whose type is a list of enums with @default([]) are now migrated without crashes on Postgres #3347

Closed
wants to merge 5 commits into from
Closed

fix(me): fields whose type is a list of enums with @default([]) are now migrated without crashes on Postgres #3347

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
26d84fa
fix(me): fields whose type is a list of enums with @default([]) are n…
jkomyno Oct 28, 2022
4ea215b
chore: removed comment on test
jkomyno Oct 28, 2022
a2c40b8
chore: removed dbg
jkomyno Oct 28, 2022
8526f49
Merge branch 'main' into fix/migration-enums-15705
jkomyno Nov 3, 2022
61ebdbd
chore: moved tests to test/migrations/enums.rs
jkomyno Nov 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 20 additions & 10 deletions migration-engine/connectors/sql-migration-connector/src/sql_renderer/postgres_renderer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1005,21 +1005,31 @@ fn render_postgres_alter_enum(alter_enum: &AlterEnum, schemas: Pair<&SqlSchema>)
let columns = schemas.walk(Pair::new(*prev_colidx, next_colidx));
let table_name = columns.previous.table().name();
let column_name = columns.previous.name();
let default_str = columns
.next
.default()
.and_then(|default| default.as_value())
.and_then(|value| value.as_enum_value())
.expect("We should only be setting a changed default if there was one on the previous schema and in the next with the same enum.");

let set_default = format!(
"ALTER TABLE {table_name} ALTER COLUMN {column_name} SET DEFAULT '{default}'",
// default_str_option may still be `None`, e.g. when `column_name` is a list of enum types
// with the `@default([])` attribute.
let default_str_option = columns
.next
.default()
.and_then(|default| default.as_value())
.and_then(|value| value.as_enum_value());

let mut alter_sql = format!(
"ALTER TABLE {table_name} ALTER COLUMN {column_name}",
table_name = Quoted::postgres_ident(&table_name),
column_name = Quoted::postgres_ident(&column_name),
default = escape_string_literal(default_str),
);

stmts.push(set_default);
// we only set a default if there is one
if let Some(default_str) = default_str_option {
alter_sql = format!(
"{} SET DEFAULT '{default}'",
alter_sql,
default = escape_string_literal(default_str),
);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the fix can be better expressed as a single line (a .filter() after line 1003). I'd rather we never produce ALTER TABLE ... ALTER COLUMN ... without the last part of the clause. It's most likely a no-op.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean something like:

    default_str_option.filter(|default_str| {
        let set_default = format!(
            "ALTER TABLE {table_name} ALTER COLUMN {column_name} SET DEFAULT '{default}'",
            table_name = Quoted::postgres_ident(&table_name),
            column_name = Quoted::postgres_ident(&column_name),
            default = escape_string_literal(default_str),
        );
        stmts.push(set_default)
    });

Is it idiomatic in Rust to have mutable operations inside functional methods like .filter() and .map()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can also express the same logic with:

    if let Some(default_str) = default_str_option {
        let set_default = format!(
            "ALTER TABLE {table_name} ALTER COLUMN {column_name} SET DEFAULT '{default}'",
            table_name = Quoted::postgres_ident(&table_name),
            column_name = Quoted::postgres_ident(&column_name),
            default = escape_string_literal(default_str),
        );
        stmts.push(set_default)
    }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No usually we avoid mutation inside iterator methods.

I meant at line 1003, insert one more call .filter(|(_, next)| next.default.is_some()), and that would be the only change needed in this file. If you want to also remove the unwrap in the loop body, you can .filter_map(|(previous, next)| next.default.map(|d| ((previous, (next, d)))).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The if let would also work, but it's a larger diff and it adds more noise.


stmts.push(alter_sql);
}
}

Expand Down
165 changes: 165 additions & 0 deletions migration-engine/migration-engine-tests/tests/create_migration/create_migration_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,171 @@ fn creating_a_migration_with_a_non_existent_migrations_directory_should_work(api
.assert_migration_directories_count(1);
}

#[test_connector(tags(Postgres, CockroachDb))]
fn alter_enum_name_remove_and_change_default(mut api: TestApi) {
let plain_dm = api.datamodel_with_provider(
r#"
model Cat {
id Int @id
mood Mood @default(HUNGRY)
}

enum Mood {
HUNGRY
SLEEPY
}
"#,
);

let dir = api.create_migrations_directory();
api.create_migration("plain", &plain_dm, &dir).send_sync();

let custom_dm = api.datamodel_with_provider(
r#"
model Cat {
id Int @id
mood Mood @default(MOODY)
}

enum Mood {
SLEEPY
MOODY
}
"#,
);

let is_postgres = api.is_postgres();
let is_cockroach = api.is_cockroach();

api.create_migration("custom", &custom_dm, &dir)
.send_sync()
.assert_migration_directories_count(2)
.assert_migration("custom", move |migration| {
let expected_script = if is_cockroach {
expect![[r#"
/*
Warnings:

- The values [HUNGRY] on the enum `Mood` will be removed. If these variants are still used in the database, this will fail.

*/
-- AlterEnum
ALTER TABLE "Cat" ALTER COLUMN "mood" DROP DEFAULT;
ALTER TYPE "Mood" ADD VALUE 'MOODY';
ALTER TYPE "Mood" DROP VALUE 'HUNGRY';

-- AlterTable
ALTER TABLE "Cat" ALTER COLUMN "mood" SET DEFAULT 'MOODY';
"#]]
} else if is_postgres {
expect![[r#"
/*
Warnings:

- The values [HUNGRY] on the enum `Mood` will be removed. If these variants are still used in the database, this will fail.

*/
-- AlterEnum
BEGIN;
CREATE TYPE "Mood_new" AS ENUM ('SLEEPY', 'MOODY');
ALTER TABLE "Cat" ALTER COLUMN "mood" DROP DEFAULT;
ALTER TABLE "Cat" ALTER COLUMN "mood" TYPE "Mood_new" USING ("mood"::text::"Mood_new");
ALTER TYPE "Mood" RENAME TO "Mood_old";
ALTER TYPE "Mood_new" RENAME TO "Mood";
DROP TYPE "Mood_old";
ALTER TABLE "Cat" ALTER COLUMN "mood" SET DEFAULT 'MOODY';
COMMIT;

-- AlterTable
ALTER TABLE "Cat" ALTER COLUMN "mood" SET DEFAULT 'MOODY';
"#]]
} else {
panic!()
};

migration.expect_contents(expected_script)
});
}

#[test_connector(tags(Postgres, CockroachDb))]
fn alter_enum_name_with_default_empty_list_shouldnt_set_new_default(mut api: TestApi) {
jkomyno marked this conversation as resolved.
Show resolved Hide resolved
let plain_dm = api.datamodel_with_provider(
r#"
model Cat {
id Int @id
moods Mood[] @default([])
}

enum Mood {
HUNGRY
SLEEPY
}
"#,
);

let dir = api.create_migrations_directory();
api.create_migration("plain", &plain_dm, &dir).send_sync();

let custom_dm = api.datamodel_with_provider(
r#"
model Cat {
id Int @id
moods Mood[] @default([])
}

enum Mood {
SLEEPY
MOODY
}
"#,
);

let is_postgres = api.is_postgres();
let is_cockroach = api.is_cockroach();

api.create_migration("custom", &custom_dm, &dir)
.send_sync()
.assert_migration_directories_count(2)
.assert_migration("custom", move |migration| {
let expected_script = if is_cockroach {
expect![[r#"
/*
Warnings:

- The values [HUNGRY] on the enum `Mood` will be removed. If these variants are still used in the database, this will fail.

*/
-- AlterEnum
ALTER TYPE "Mood" ADD VALUE 'MOODY';
ALTER TYPE "Mood" DROP VALUE 'HUNGRY';
"#]]
} else if is_postgres {
expect![[r#"
/*
Warnings:

- The values [HUNGRY] on the enum `Mood` will be removed. If these variants are still used in the database, this will fail.

*/
-- AlterEnum
BEGIN;
CREATE TYPE "Mood_new" AS ENUM ('SLEEPY', 'MOODY');
ALTER TABLE "Cat" ALTER COLUMN "moods" DROP DEFAULT;
ALTER TABLE "Cat" ALTER COLUMN "moods" TYPE "Mood_new"[] USING ("moods"::text::"Mood_new"[]);
ALTER TYPE "Mood" RENAME TO "Mood_old";
ALTER TYPE "Mood_new" RENAME TO "Mood";
DROP TYPE "Mood_old";
ALTER TABLE "Cat" ALTER COLUMN "moods";
COMMIT;
"#]]
} else {
panic!()
};

migration.expect_contents(expected_script)
});
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we test the following scenario:

  • Single model with a field with mood Mood @default(HUNGRY)
  • Migration to drop one variant from the enum and the default on the field at the same time.

I think it's a more minimal reproduction of the conditions for the crash.

That test can live in tests/migrations/enums.rs

Copy link
Contributor Author

@jkomyno jkomyno Nov 3, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this scenario already tested by alter_enum_name_remove_and_change_default? I could move it to tests/migrations/enums.rs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep we could move it there. It's a different test however, alter_enum_name_remove_and_change_default still has a default, just a different one. We should test what happens when the whole @default() is removed in the second schema.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have moved the tests to tests/migrations/enums.rs and left a few TODOs.


#[test_connector(tags(Mysql, Postgres))]
fn create_enum_step_only_rendered_when_needed(api: TestApi) {
let dm = format!(
Expand Down