Skip to content
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

feat: implemented Postgres::with_init_sql #182

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

CommanderStorm
Copy link
Contributor

@CommanderStorm CommanderStorm commented Aug 19, 2024

Resolves #164

The implementation is a bit hacky, am open to better approaches or other feedback ^^

src/postgres/mod.rs Show resolved Hide resolved
Comment on lines +71 to +115
/// Registers sql to be executed automatically when the container starts.
///
/// # Example
///
/// ```
/// # use testcontainers_modules::postgres::Postgres;
/// let postgres_image = Postgres::default()
/// .with_init_sql("CREATE EXTENSION IF NOT EXISTS hstore;");
/// # assert!(postgres_image.is_ok())
/// ```
///
/// ```rust,ignore
/// # use testcontainers_modules::postgres::Postgres;
/// let postgres_image = Postgres::default()
/// .with_init_sql(include_str!("path_to_init.sql"));
/// # assert!(postgres_image.is_ok())
/// ```
pub fn with_init_sql(mut self, init_sql: &str) -> Result<Self, std::io::Error> {
let init_path = match self.tempdir.as_ref() {
Some(t) => t
.path()
.join(format!("init_{index}.sql", index = self.init_mounts.len())),
None => {
return Err(std::io::Error::other(
"Could not create a temporary directory",
))
}
};
let mut tmp_file = File::create(init_path.clone())?;
write!(tmp_file, "{}", init_sql)?;
// convert the init_path to a file mount
let file_name = init_path
.file_name()
.expect("we created files with filenames, not directories")
.to_str()
.expect("we created files with filenames, not directories");
let container_path = format!("/docker-entrypoint-initdb.d/{file_name}",);
self.init_mounts.push(Mount::bind_mount(
init_path
.to_str()
.expect("the path we put is a valid string"),
container_path,
));
Ok(self)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The similar approach can be implemented for some others DB modules (e.g mysq, mariadb)

Also, important part here: it uses blocking API and might be called from async code.

I think we need to have more general interface for these modules, with ability to run async and sync (we can spawn a blocking task at least)

I can suggest to extract a trait for RDBMS (e.g InitSql and InitSqlAsync). Also this common module may have different strategies: TempFile, Sqlx and etc.
For now we can implement only the one on top of TempFile, it will create a temp directory/file and return a structure, like TemporaryScript { tempdir: TempDir, path: PathBuf }
So Postgres module will need only to mount this and keep the structure for it's lifetime


Probably it's overwhelming comment and not well described. Feel free to clarify anything and share your opinion

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might have misunderstood something ^^

This could work similarly, to the wait's-stratgy patern..

#[derive(Default)]
enum InitSql {
  #[default]
  None,
  TemporaryDirectory(TemporaryDirectoryStrategy), // deliberatley not a TempFile, as this way multiple files are supported
  Sqlx(SqlxStrategy),
}

impl InitSql{
  pub fn mount_temporary_files(TemporaryDirectoryStrategy)
}

// InitSqlAsync is the same but with async in all relevant names

Not quite sure what a InitSqlStrategy trait would look like.
Any draft that I came up with shifts a lot of the work onto the RDMS..

Running sqlx commands after something is healthy and mounting files is not quite similar.
=> Not sure if marrying them at the hip as above is a good idea.. TemporaryDirectory needs a base path to retunrn the mounts while Sqlx needs a healthy connectionstring/PGConnection/...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that sqlx can be used with exec_after_start, while TemporaryDirectory utilizes mounts

Both strategies are intended to execute arbitrary SQL before returning Container to the user, but in different ways. I don't see issues here actually, the responsibility is the same, but the implementation differs.

Copy link
Contributor

@DDtKey DDtKey Aug 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite sure what a InitSqlStrategy trait would look like.
Any draft that I came up with shifts a lot of the work onto the RDMS..

Trait is needed to alight interface for users across different modules. Internal functionality can be implemented on top of structures.

In fact, I still not sure about the case when we may need sqlx strategy. It was more like an example and we don't need to implement it right now.
It barely makes sense to allow users to choose strategies, it's more to unify the implementation across modules. For example, we may need this if module Y can implement only strategy B.

For now, I think we can do something like that:

trait InitSql {
  fn with_init_sql(mut self, script: &str) -> Result<Self, Error>;
}

impl InitSql for Postgres {
    fn with_init_sql(mut self, script: &str) -> Result<Self, Error> {
        // it's up to us to decide what strategy to use, we even can do custom implementation, but trait unifies usage for users 
        let strategy = self.init_strategy.take().unwrap_or(|| InitSqlStrategy::temporary_directory());
        strategy.add_script(sql); // `add_script` impl depends on underlying strategy
        self.init_strategy.replace(strategy);
        self
    }
}

// strategy also can have private method that returns paths that must be mounted. Thus `Image::mounts` will utilize it, using DB-specific paths

@DDtKey
Copy link
Contributor

DDtKey commented Aug 20, 2024

Thank you for the contribution! ❤️
Left a couple of suggestions that could significantly expand the capabilities of the crate, but they need to be discussed and possibly postponed

@CommanderStorm CommanderStorm marked this pull request as draft August 20, 2024 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

initSQL for postgres container
2 participants