Handle Postgres schema name for sea-query statements

[nahuakang]

PR Info

• Closes sea-query statements should qualify schema name #338

Fixes

This should enable SchemaManager in sea-orm-migration to inject table reference & schema information for Postgres.

[nahuakang]

@billy1624 @ikrivosheev 👋 Hey guys, I've made some adjustments to all table-related statements used by SchemaManager:

• TableCreateStatement
• TableAlterStatement
• TableDropStatement
• TableRenameStatement
• TableTruncateStatement

Then, I made some changes for index-related statements for SchemaManager (including a copy-paste, which I wonder if it's okay to do so):

• IndexCreateStatement
• IndexDropStatement

Is this going in the right direction? The remaining are the ForeignKey and Type related statements.

@billy1624 You mentioned:

Also, some statement support schema scope, for example, type name could have schema prefix e.g. in Postgres. We can update it from Iden to IdenList.

I'm still not sure where to find this in the code base :D Can you give some more hints? 🙏

[ikrivosheev]

@nahuakang thank you for PR! Some comments...

[ikrivosheev]

I'm still not sure where to find this in the code base :D Can you give some more hints? pray

@nahuakang I think @billy1624 mean: https://github.com/SeaQL/sea-query/blob/master/src/extension/postgres/types.rs

[nahuakang]

@billy1624 Besides the question on Type, do we need to change the table from Option<DynIden> to Option<TableRef> for both ForeignKeyDropStatement and TableForeignKey (even if the former contains the latter and thus contains TableRef)?

pub struct ForeignKeyDropStatement {
    pub(crate) foreign_key: TableForeignKey,
    pub(crate) table: Option<DynIden>,
}

#[derive(Debug, Clone)]
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>,
}

And is it safe to assume that schema can be prepended to the table names in any statement involving creating & dropping foreign keys (both the from and to tables)? Thanks :)

[billy1624]

Hey @nahuakang @ikrivosheev, yes, I mean https://github.com/SeaQL/sea-query/blob/master/src/extension/postgres/types.rs

[billy1624]

Do we need to change the table from Option<DynIden> to Option<TableRef> for both ForeignKeyDropStatement and TableForeignKey (even if the former contains the latter and thus contains TableRef)?

Yes, we should change both to Option<TableRef>

And is it safe to assume that schema can be prepended to the table names in any statement involving creating & dropping foreign keys (both the from and to tables)? Thanks :)

This assumption is valid :)

[tyt2y3]

Thank you for the hard work! My question is why do we needed TypeRef in addition to TableRef?
It seems like TypeRef a subset of TableRef's variants.
And so, we can either reuse TableRef or make TypeRef more similar to TableRef, I mean having three variants, Name, SchemaName and DatabaseSchemaName?

[ikrivosheev]

@nahuakang thank you for PR! Some comments

[nahuakang]

@tyt2y3 Thanks for your review :) I think we shouldn't reuse TableRef since we need a specific way to mark a type name that might be schema-qualified.

If we must have consistency between TabelRef and TypeRef, perhaps we could do the following (and adjust the IntoTypeRef implementations as well as the prepare_type_ref for TypeBuilder? (wdyt @ikrivosheev since Ivan came up with TypeRef?)

#[derive(Debug, Clone)]
pub enum TypeRef {
    Type(DynIden),
    SchemaType(DynIden, DynIden),
    DatabaseSchemaType(DynIden, DynIden, DynIden),
}

[ikrivosheev]

Thank you for the hard work! My question is why do we needed TypeRef in addition to TableRef? It seems like TypeRef a subset of TableRef's variants. And so, we can either reuse TableRef or make TypeRef more similar to TableRef, I mean having three variants, Name, SchemaName and DatabaseSchemaName?

This was my idea. We don't need all the options from TableRef and this solution looks more type-safe then panic with invalid option

[billy1624]

I don't think we can create index across database? i.e.

CREATE INDEX idx_aaa 
ON aaa.public.accounts (password); # `public.accounts` is valid but not `aaa.public.accounts`

https://dbfiddle.uk/?rdbms=postgres_14&fiddle=67b77e58c1c524174d1675ff92511e2d

[nahuakang]

That was removed :) Thanks for catching this. I think I probably missed testing this in my test DB 🙏

[billy1624]

Hey @nahuakang, all in all it looks good!
Thanks @nahuakang @ikrivosheev for the contrition and help :D

[ikrivosheev]

@nahuakang thank you! LGMT!

[billy1624]

Hey @nahuakang, sorry for the long delay. I think we can refactor it by adding a TableRefBuilder trait. To handle the writing of TableRef as SQL string. From my perspective that looks cleaner and we don't need to repeat same piece of code again and again.

• https://github.com/nahuakang/sea-query/compare/postgres-table-ref...billy1624:sea-query:pr/385

Please check if that make any sense :)
Thanks!!

[nahuakang]

@billy1624 This refactoring makes perfect sense and is a lot cleaner for the hierarchy of the trait requirements. Hope it's okay that I merge it into my branch? :)

[tyt2y3]

Thank you for your patience and diligence!