SeaQL / sea-query

🔱 A dynamic SQL query builder for MySQL, Postgres and SQLite
https://www.sea-ql.org
Other
1.18k stars 195 forks source link

migration to modify_column by adding unique_key() fails on Postgres #452

Closed epipheus closed 2 years ago

epipheus commented 2 years ago

Description

migration to modify column to add unique_key on Postgres errors out with Execution Error: error returned from database: syntax error at or near "UNIQUE"

Steps to Reproduce

I setup migration like this:

    async fn up(&self, manager: &SchemaManager) -> Result<(), DbErr> {
        manager
            .alter_table(
                Table::alter()
                    .table(User::Table)
                    .modify_column(ColumnDef::new(User::Email).string().not_null().unique_key())
                    .to_owned(),
            )
            .await
    }

Expected Behavior

migration without error and unique constraint added in postgres,

Actual Behavior

Migration output:

Applying 1 pending migrations
Applying migration 'm20220925_053725_alter_email_to_unique'
Execution Error: error returned from database: syntax error at or near "UNIQUE"

Reproduces How Often

every time

Versions

❯ cargo tree | grep sea-
│   ├── sea-orm v0.9.2
│   │   ├── sea-orm-macros v0.9.2 (proc-macro)
│   │   ├── sea-query v0.26.3
│   │   │   ├── sea-query-derive v0.2.0 (proc-macro)
│   │   │   ├── sea-query-driver v0.2.2 (proc-macro)
│   │   ├── sea-strum v0.23.0
│   │   │   └── sea-strum_macros v0.23.0 (proc-macro)
│   └── sea-orm-migration v0.9.2
│       ├── sea-orm v0.9.2 (*)
│       ├── sea-orm-cli v0.9.2
│       │   ├── sea-schema v0.9.4
│       │   │   ├── sea-query v0.26.3 (*)
│       │   │   └── sea-schema-derive v0.1.0 (proc-macro)
│       ├── sea-schema v0.9.4 (*)
├── sea-orm v0.9.2 (*)

Mac OS BigSur 11.4 Postgres 14

billy1624 commented 2 years ago

Hey @epipheus, hi again! PostgreSQL doesn't support adding unique constraint on a column in ALTER TABLE statement. Instead, you need to create a unique index for it.

billy1624 commented 2 years ago

I'm going to transfer this issue over to SeaQuery

epipheus commented 2 years ago

Thanks for response. A couple questions.

First question is really about whether ORM users should ever care about the peculiarities of each DB as a design principle. I tend to use an ORM so that I have a single DSL that can with minimal effort switch to a different DB and work. And having had horrible experiences manually changing DB platforms in middle of projects, I've grown to generally appreciate ORMs, but I don't know if that's a view held here.

Next question: on creation it doesn't seem to complain, like if I do a migration fresh and then edit the old migration and reapply them all creating the User Table this time with a unique_key(), the migration happens without error message. Is it creating a unique index under the hood or do I still need to for the constraint to work.

manager
            .create_table(
                Table::create()
                    .table(User::Table)
                    .if_not_exists()
                    .col(
                        ColumnDef::new(User::Id)
                            .integer()
                            .not_null()
                            .auto_increment()
                            .primary_key(),
                    )
                    .col(ColumnDef::new(User::Name).string().not_null())
                    .col(ColumnDef::new(User::Email).string().not_null().unique_key())
                    .to_owned(),
            )
            .await
epipheus commented 2 years ago

Also have this in entities:

#[derive(Clone, Debug, PartialEq, DeriveEntityModel)] 
pub struct Model {
    #[sea_orm(primary_key)]
    pub id: i32,
    pub name: String,
    #[sea_orm(indexed, unique)]
    pub email: String,
}

A regular index for email already exists through another migration. Does one need all 3?

  1. unique_key() in original migration
  2. [sea_orm(indexed, unique)] in entity

  3. a unique index
billy1624 commented 2 years ago

First question is really about whether ORM users should ever care about the peculiarities of each DB as a design principle. I tend to use an ORM so that I have a single DSL that can with minimal effort switch to a different DB and work. And having had horrible experiences manually changing DB platforms in middle of projects, I've grown to generally appreciate ORMs, but I don't know if that's a view held here.

The construction of SQL rely on SeaQuery where it's designed to write once and translate to different SQL dialect. On this you can have faith on it. However, I have to admit there might be some edge case where it didn't handle properly. Just like PostgreSQL didn't support adding unique key constraint to a existing column with this syntax ALTER TABLE ... MODIFY COLUMN ....

Next question: on creation it doesn't seem to complain, like if I do a migration fresh and then edit the old migration and reapply them all creating the User Table this time with a unique_key(), the migration happens without error message. Is it creating a unique index under the hood or do I still need to for the constraint to work.

Table::create()
    .table(User::Table)
    .if_not_exists()
    .col(
        ColumnDef::new(User::Id)
            .integer()
            .not_null()
            .auto_increment()
            .primary_key(),
    )
    .col(ColumnDef::new(User::Name).string().not_null())
    .col(ColumnDef::new(User::Email).string().not_null().unique_key())
    .to_owned(),

The code above will just work as expected. Create a table with a email column storing unique emails.

billy1624 commented 2 years ago

Note that #[sea_orm(indexed, unique)] doesn't create a index nor unique index for you. It only expand inside the macros into ColumnType::String(None).def().unique().indexed() See https://www.sea-ql.org/SeaORM/docs/generate-entity/expanded-entity-structure/#additional-properties

Therefore, one of below should be enough:

ikrivosheev commented 2 years ago

@billy1624: ColumnType::String(None).def().unique().indexed() - this is code to reproduce the problem?

billy1624 commented 2 years ago

Hey @ikrivosheev, to reproduce the error.

Table::alter()
    .table(User::Table)
    .modify_column(ColumnDef::new(User::Email).string().not_null().unique_key())
    .to_string(PostgresQueryBuilder)
billy1624 commented 2 years ago

I'm thinking... for example, this alter statement

assert_eq!(
    Table::alter()
        .table(Glyph::Table)
        .modify_column(ColumnDef::new(Glyph::Aspect).string().not_null().unique_key().primary_key())
        .to_string(PostgresQueryBuilder),
    r#"ALTER TABLE "glyph" ALTER COLUMN "aspect" TYPE varchar, ALTER COLUMN "aspect" SET NOT NULL, ALTER COLUMN "aspect" SET UNIQUE, ALTER COLUMN "aspect" SET PRIMARY KEY"#
);

We could write the SQL as

ALTER TABLE "glyph" ALTER COLUMN "aspect" TYPE varchar,
  ALTER COLUMN "aspect" SET NOT NULL,
  ADD UNIQUE("aspect"),
  ADD PRIMARY KEY("aspect")

Docs: https://www.postgresql.org/docs/current/sql-altertable.html Demo: https://dbfiddle.uk/9CSVktyX

ikrivosheev commented 2 years ago

@epipheus @billy1624 hello! I create PR.