search

SeaQL / sea-orm

🐚 An async & dynamic ORM for Rust
https://www.sea-ql.org/SeaORM/
Apache License 2.0
7.3k stars 513 forks source link

sea-orm-cli migrate doesn't respect DATABASE_SCHEMA in .env file #521

Closed MattGson closed 2 years ago

MattGson commented 2 years ago

Description

Running the command:

sea-orm-cli migrate ...

Always runs migrations against the public schema in PostgreSQL rather than the schema specified in .env

Steps to Reproduce

  1. Create .env file with DATABASE_SCHEMA=not_public
  2. Run sea-orm-cli migrate...

Expected Behavior

Migrations are run in specified schema

Actual Behavior

See that all migrations were run in public schema

Versions

└── sea-orm v0.6.0
    ├── sea-orm-macros v0.6.0 (proc-macro)
    ├── sea-query v0.21.0
    │   ├── sea-query-derive v0.2.0 (proc-macro)
    ├── sea-strum v0.23.0
    │   └── sea-strum_macros v0.23.0 (proc-macro)
└── sea-schema v0.5.1
    ├── sea-orm v0.6.0 (*)
    ├── sea-query v0.21.0 (*)
    ├── sea-schema-derive v0.1.0 (proc-macro)
billy1624 commented 2 years ago

Hey @MattGson, PostgreSQL's migration currently only operate on public schema for now. We will respect DATABASE_SCHEMA in .env file soon.

tyt2y3 commented 2 years ago

um... this seems like an easy fix? would appreciate PR on this

smonv commented 2 years ago

@tyt2y3 I would like to work on this issue.

After some research, I identified that sea-schema is the package responsible for handle migration environment variable like DATABASE_URL but I'm still not sure where to start in sea-schema to make it handle DATABASE_SCHEMA for postgresql. Can you help guide me?

billy1624 commented 2 years ago

Hey @smonv, thanks for the interest! Note that the migrator is subject to change, see SeaQL/sea-schema#59.

Anyways, the schema name (value of DATABASE_SCHEMA env) should be injected into sea_query::TableRef. Just like what we did here.

smonv commented 2 years ago

Hey @smonv, thanks for the interest! Note that the migrator is subject to change, see SeaQL/sea-schema#59.

Anyways, the schema name (value of DATABASE_SCHEMA env) should be injected into sea_query::TableRef. Just like what we did here.

@billy1624 Thank for guiding me. I'll follow your PR and change the migrator patch if necessary.

billy1624 commented 2 years ago

Thanks!! @smonv Feel free to ping us if you need helps :)

billy1624 commented 2 years ago

Hey @smonv, @nahuakang would like to take on this issue. Is that fine?

smonv commented 2 years ago

Hey @smonv, @nahuakang would like to take on this issue. Is that fine?

yes, i'm good with it.

nahuakang commented 2 years ago

@smonv Thank you :) I'll claim this!

nahuakang commented 2 years ago

@billy1624 Some updates after digging today:

sea-orm-cli generate entity command actually takes care of DATABASE_SCHEMA for postgres.

However, sea-orm-cli migrate does not take care of DATABASE_SCHEMA for postgres and it relays the command to the migrator CLI instead.

So does this mean we should take care of getting the environment value for DATABASE_SCHEMA in sea-orm-migration, such as in build_cli and then, as you mentioned previously, correctly inject it down in the migration operations into usages of sea_query::TableRef?

Questions:

billy1624 commented 2 years ago

Hey @nahuakang, sorry for the delay. Thanks for the investigations!

So does this mean we should take care of getting the environment value for DATABASE_SCHEMA in sea-orm-migration, such as in build_cli

Correct, and DATABASE_SCHEMA will only be used when the connect is pointing to PostgreSQL.

Questions:

  • I'm not clear how to inject it from here on. Does it mean we should update the methods (up, down, etc.) associated with MigrationTrait?

Take this as reference, which inject schema name for SeaORM entity https://github.com/SeaQL/sea-orm/blob/3f90c094079501211e88ea2111ce8f2473493bdf/src/entity/base_entity.rs#L31-L37

However, for migration, we're injecting schema name into existing SeaQuery statement. Which will be performed inside methods inside SchemaManager. https://github.com/SeaQL/sea-orm/blob/3f90c094079501211e88ea2111ce8f2473493bdf/sea-orm-migration/src/manager.rs#L37-L133

E.g. Converting the table field inside TableCreateStatement from TableRef::Table into TableRef::SchemaTable

Let me know if it was unclear.

nahuakang commented 2 years ago

I'm guessing that the stmt must be injected of the schema for Postgres before self.conn.execute(builder.build(&stmt)).await.map(|_| ()) is executed in SchemaManager.exec_stmt method?

The issue is that a lot of the other stmt types don't have a reference to TableRef but to Option<DynIden> or simply don't have table reference at all. Below are a few examples from sea-query:

// table reference is Option<DynIden>
pub struct IndexCreateStatement {
    pub(crate) table: Option<DynIden>,
    pub(crate) index: TableIndex,
    pub(crate) primary: bool,
    pub(crate) unique: bool,
    pub(crate) index_type: Option<IndexType>,
}

// No table reference
pub struct TypeCreateStatement {
    pub(crate) name: Option<DynIden>,
    pub(crate) as_type: Option<TypeAs>,
    pub(crate) values: Vec<DynIden>,
}

// This is a field in `ForeignKeyCreateStatement`
pub struct TableForeignKey {
    pub(crate) name: Option<String>,
    pub(crate) table: Option<DynIden>,
    pub(crate) ref_table: Option<DynIden>,
    pub(crate) columns: Vec<DynIden>,
    pub(crate) ref_columns: Vec<DynIden>,
    pub(crate) on_delete: Option<ForeignKeyAction>,
    pub(crate) on_update: Option<ForeignKeyAction>,
}

Let me know what we should do (if we should create new issues and solve another issue first) before we address this one? :)

billy1624 commented 2 years ago

Good point! I think we need to update these statement to quantify the schema name

For PostgreSQL type statement, it actually supports schema name. But we didn't implement it in SeaQuery https://www.postgresql.org/docs/current/sql-createtype.html

nahuakang commented 2 years ago

So what should the next step be in terms of moving forward with this issue? Should we have a separate PR that addresses sea-query? Happy to help on that if you could give some tips on the scope and direction.

billy1624 commented 2 years ago

Yes, please open an issue & PR on SeaQuery. Check all stmt below and see if any table reference is of Iden type, it should be changed to TableRef. Also, some statement support schema scope, for example, type name could have schema prefix. We can update it from Iden to IdenList.

nahuakang commented 2 years ago

With this PR on the way, I can resume this issue :)

billy1624 commented 2 years ago

Hey @nahuakang, I have a plan:

On sea-orm-cli, make sea-orm-cli migrate take a extra option:

The db schema will be passed to MigratorTrait methods where it's updated to take any ConnectionTrait (a new trait defined in sea-orm-migration not the same as sea_orm:: ConnectionTrait).

On sea-orm-migration:

We have a new ConnectionTrait where it has two methods:

Implements ConnectionTrait for &DbConn (for backward compatibility) and MigrationConnection

And we need a new struct, let say MigrationConnection, to hold the connection and schema name.

Then, we modify the behaviour of public API of MigratorTrait:

SchemaManager will store the &ConnectionTrait instead of &DbConn. And all the create_* and drop_* statements is now taking SeaQuery statements that properly quantify table name with TableRef, PR https://github.com/SeaQL/sea-query/pull/385. Then, we can override the TableRef inside each statement and prefix it all with the schema name provided by ConnectionTrait.

nahuakang commented 2 years ago

Thanks! I've read it and will start on it (might take a bit of time :)).