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

[MattGson]

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]

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

[tyt2y3]

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

[smonv]

@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]

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]

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]

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

[billy1624]

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

[smonv]

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

yes, i'm good with it.

[nahuakang]

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

[nahuakang]

@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:

• 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?

[billy1624]

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

• https://github.com/SeaQL/sea-query/blob/58e11cc2ff8faf5ad3f2c4691fead5e0b5bd6be4/src/table/create.rs#L79-L88
• https://github.com/SeaQL/sea-query/blob/58e11cc2ff8faf5ad3f2c4691fead5e0b5bd6be4/src/types.rs#L74-L92

Let me know if it was unclear.

[nahuakang]

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]

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]

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]

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.

• https://github.com/SeaQL/sea-orm/blob/183639dc8c817fbd3211a87c67bcb983db4050ed/sea-orm-migration/src/manager.rs#L37-L89

[nahuakang]

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

[billy1624]

Hey @nahuakang, I have a plan:

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

• -s / --database-schema: database schema (default: DATABASE_SCHEMA specified in ENV)

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:

• get_connection() -> &DbConn
• get_schema_name() -> Option<String>

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:

• install: setup seaql_migrations table on the corresponding schema
• up: apply migration scripts in the corresponding schema
• down: rollback migration scripts in the corresponding schema
• reset: rollback all migration scripts in the corresponding schema
• fresh: drop all tables in the corresponding schema then apply all migration scripts
• refresh: rollback all migration scripts in the corresponding schema then reapplying all

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]

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