sqlx icon indicating copy to clipboard operation
sqlx copied to clipboard

feat: add migration_table name option

Open kriswuollett opened this issue 2 years ago • 5 comments

Add migration_table option to enable sharing a database among multiple apps/clients. It is the responsibility of the user to ensure database permissions for the database role performing the migration is not able affect other co-hosted apps/clients done in separate migration "partitions".

  • fixes #1835, fixes #1698, fixes #1356, fixes #946

kriswuollett avatar Sep 01 '23 05:09 kriswuollett

Tested locally with sqlite by cargo install the cli with --git flag for this branch.

kriswuollett avatar Sep 01 '23 05:09 kriswuollett

Does this seem like a reasonable approach to quickly satisfy the needs outlined in the use cases before I do more things like add tests?

If more migration options, or some more refactoring is needed later, it may be warranted to look at refactoring the relationship between the migration and the database connection. They seem too tightly bound when the database connection is required to know about the migration table, _sqlx_migrations, when it may be more reasonable to just have a function to ask if a table exists, select from it, etc. -- currently the migration table name is hard-coded in every database connection implementation, e.g., postgres:

https://github.com/launchbadge/sqlx/blob/d0fbe7feff4b3a200cf0453417d5e53bd011643a/sqlx-postgres/src/migrate.rs#L90-L111

kriswuollett avatar Sep 01 '23 12:09 kriswuollett

Hey @kriswuollett any updates on this? I would like to see this merged too.

IgnisDa avatar Sep 28 '23 06:09 IgnisDa

@abonander, are you willing to work with an externally contributed PR to implement a migration table name option? I assume I'd need to add in schema/table name validation for the new option. Is there some other approach that would be preferred to the rather direct way I implemented the PR?

kriswuollett avatar Sep 28 '23 11:09 kriswuollett

+1 @abonander is there any interest on getting @kriswuollett's change reviewed and merged, or potentially helping define what you'd prefer to see? Or is this a non-priority at the moment for launchbadge (which is fine too)?

bilemon avatar Jan 17 '24 00:01 bilemon

@kriswuollett I'm so sorry. Once a PR gets pushed onto the second page I tend to forget about it.

I'm inclined to say that this should be a property that's specified at the crate level since that seems to be the most common use-case. --migration-table/.set_migration_table() seems like it'd be too easy to forget.

I'm thinking maybe we have a configuration file that lives in the migrations directory itself which both sqlx-cli and migrate!() can read automatically.

I'm thinking one of a few options (both dotfiles so they're sorted separately from migrations):

  • migrations/.table_name
    • Simple text file containing just the table name
    • Fast to read in migrate!() (important since proc macros compile in debug mode by default)
  • migrations/.config.toml
    • Extensible
    • More complex to parse
  • sqlx.toml (at the crate root)
    • A single, global configuration file per crate
    • Already planning on adding this to configure the query macros

I think I'm kinda leaning towards sqlx.toml but migrations/.table_name would be really quick to add.

What would work best for you?

abonander avatar Jul 12 '24 02:07 abonander

I'd be in favour of sqlx.toml

IgnisDa avatar Jul 12 '24 03:07 IgnisDa

Closing to clear out the PR queue. If you have time to come back to this, please address my previous comment.

abonander avatar Jul 19 '24 21:07 abonander