Skip to content

Add migration parameters or the possibility of passing migrations by string #3178

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
vszakd opened this issue Apr 6, 2024 · 9 comments
Open
Labels
enhancement New feature or request

Comments

@vszakd
Copy link

vszakd commented Apr 6, 2024

Is your feature request related to a problem? Please describe.

When running migrations from Rust code, the only way (as far as I know) is passing the path to the migrations directory. After this, it is not possible to touch the migrations contents. This makes it impossible to parameterize the migrations. E.g. if a migration needs to be run for multiple schemas.

Describe the solution you'd like

Two complementary solutions could be applied.

The first (easier) step would be modifying the Migrator trait to allow for manual insertion of migrations via string. This way, migration contents would be pushed by the application with arbitrary content. This would solve the problem because the substitution step would be done by the application before passing the migration contents to Migrator.

The second (more complex) step would be to implement in sqlx a variable substitution feature for migrations. So that the application can register the variables and their replacement to the Migrator, which can continue to work with a Path, given that it will take care of variable substitution.

The two solutions could then live side by side for maximum flexibility.

Describe alternatives you've considered

I am trying to work around the problem by manually replacing the content of the migrations field of the Migrator struct, but that does not look like a very clean solution.

@vszakd vszakd added the enhancement New feature or request label Apr 6, 2024
@adriangb
Copy link

We're interested in this feature as well. Might even be able to implement it if there's buy in from maintainers.

@peter-lockhart-pub
Copy link

Also interested. I was intending on providing end users our application binary and the same set of SQL migrations to run as we run on our tech stack. Ideally these would be identical for consistency, but then how might ourselves or end users configure e.g. the password of a particular role? Some substitution of an environment variable in this case would be a solution

@krlohnes
Copy link

goose implements env substitution with a toggle via comment to maintain backwards compatibility with scripts that may use ${ENV_VAR} like syntax in their SQL already. It might be a reasonable approach to implement in sqlx as well.

@abonander
Copy link
Collaborator

See also #2726

krlohnes added a commit to krlohnes/sqlx that referenced this issue Mar 22, 2025
Issue launchbadge#3178

Added migration parameters in a similar fashion to goose's env var
migration parameters. This still needs some tests and documentation.
I wanted to get a proposal for a solution up for folks to take a look
at.
krlohnes added a commit to krlohnes/sqlx that referenced this issue Mar 22, 2025
Issue launchbadge#3178

Added migration parameters in a similar fashion to goose's env var
migration parameters. This still needs some tests and documentation.
I wanted to get a proposal for a solution up for folks to take a look
at.
@krlohnes
Copy link

krlohnes commented Mar 22, 2025

I took a quick pass at implementing something similar to goose's implementation. I'll take a look at #2726 as well, but at a quick glance, you could achieve the goals of either with something along the lines of the following. I'm happy to add some tests, documentation, and push up a PR for this functionality.

use regex::{Captures, Regex};
use std::borrow::Cow;
use std::sync::OnceLock;

use sha2::{Digest, Sha384};

use super::MigrationType;

static ENV_SUB_REGEX: OnceLock<Regex> = OnceLock::new();

#[derive(Debug, Clone)]
pub struct Migration {
    pub version: i64,
    pub description: Cow<'static, str>,
    pub migration_type: MigrationType,
    pub sql: Cow<'static, str>,
    pub checksum: Cow<'static, [u8]>,
    pub no_tx: bool,
}

impl Migration {
    pub fn new(
        version: i64,
        description: Cow<'static, str>,
        migration_type: MigrationType,
        sql: Cow<'static, str>,
        no_tx: bool,
    ) -> Self {
        let checksum = Cow::Owned(Vec::from(Sha384::digest(sql.as_bytes()).as_slice()));
        let re = ENV_SUB_REGEX.get_or_init(|| {
            Regex::new(r"--\s?sqlx envsub on((.|\n|\r)*?)--\s?sqlx envsub off").unwrap()
        });
        let envsub_sql = re
            .replace_all(&sql, |cap: &Captures<'_>| {
                format!(
                    "-- sqlx envsub on{}-- sqlx envsub off",
                    subst::substitute(&cap[1], &subst::Env).unwrap_or(cap[1].to_owned())
                )
            })
            .into_owned();
        Migration {
            version,
            description,
            migration_type,
            sql: Cow::Owned(envsub_sql),
            checksum,
            no_tx,
        }
    }
}

#[derive(Debug, Clone)]
pub struct AppliedMigration {
    pub version: i64,
    pub checksum: Cow<'static, [u8]>,
}

@krlohnes
Copy link

The code above probably needs to get moved over to the Migrator, but the general approach still stands.

@abonander
Copy link
Collaborator

abonander commented Apr 11, 2025

The solution proposed by @krlohnes above / in #3805 is a non-starter for the following reasons:

  • Some users already don't like the reliance on environment variables.
  • It can easy to forget to specify environment variables which would lead to an error at runtime.
  • Users might expect more interaction with migrate!() or may even expect variables to be expanded at compile-time (or at least to have that option).
  • It's impossible to programmatically set these variables without using env::set_var(), which is not thread-safe on most platforms.
  • Using the regex crate to find the start and end of substitution-enabled sections is kind of overkill.

The templating syntax in the migrations is fine, although the -- sqlx envsub on switch needs work. The style doesn't match the existing -- no-transaction annotation or the -- transaction-break annotation we've been working on in #3694.

For bikeshedding purposes, in order of personal preference:

  • -- enable-substitution / -- disable-substitution
  • -- enable-templating / -- disable-templating
  • -- expand-templates / -- no-expand-templates
  • -- arg-substitution-on / -- arg-substitution-off

It's an open question whether we should namespace these annotations: IMO, it just adds noise, and users should not be mixing and matching migration tooling anyway. I'm open to discussing that, but if we're going to namespace one annotation, we should namespace them all, and that's an orthogonal concern.

It should not require the regex crate to parse these annotations. Just scan line by line and check if the whole line consists of the annotation or not.

The Migrator should have a builder method that allows the user to add key-value pairs for templating arguments. When run, it should parse migrations for argument substitutions and search the set it was given, returning an error if one is missing without a default value.

Ideally, the migrate!() macro would parse template arguments out at compile-time and require them to be passed in its input (constant keys with value expressions that are evaluated at runtime). That could theoretically be implemented later, though.

@krlohnes
Copy link

krlohnes commented Apr 11, 2025

@abonander I'm going to take another pass at this. Do you have any thoughts on how to support this with sqlx-cli? With environment variables, it was simply "set the environment variables". My initial thought is a toml file with the key value pairs in it. I'd add a flag to set the path to the file.

@abonander
Copy link
Collaborator

abonander commented Apr 12, 2025

@krlohnes sqlx-cli could still use environment variables, I would just require a command-line switch to enable that. You could add that as a switch to Migrator as well. The biggest issue was making that the default behavior.

Passing arguments on the command line should also be possible, bikeshedding: -p<key>=<value> or --parameters <key>=<value>,<key>=<value>. There's an example of how to do that with Clap: clap-rs/clap#4291 (reply in thread)

That can always be added later though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants