Conversation
There was a problem hiding this comment.
Pull request overview
Adds an end-to-end integration test suite (Docker Compose + GitHub Actions services) and extends the seed engine to better support reset/id generation during seeding.
Changes:
- Introduces Docker-backed integration tests covering
wait-for,render,fetch,exec, andseedworkflows against real Postgres/MySQL/nginx. - Adds CI workflow to run the integration suite with service containers, plus test inputs/specs under
tests/input/. - Updates seed execution to support phase-level reset and passes an
auto_idcolumn hint through to DBinsert_row.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
tests/integration_test.rs |
New Docker-dependent integration suite invoking the built initium binary and validating behavior against live services |
tests/input/template.conf.tmpl |
Template fixture for render integration test |
tests/input/seed-postgres.yaml |
Postgres seeding fixture with refs + unique keys |
tests/input/seed-mysql.yaml |
MySQL seeding fixture with refs + unique keys |
tests/input/create-schema-postgres.yaml |
Seed spec fixture for schema creation scenarios |
tests/input/create-db-postgres.yaml |
Seed spec fixture for Postgres database creation scenario |
tests/input/create-db-mysql.yaml |
Seed spec fixture for MySQL database creation scenario |
tests/input/create-nonexistent-db-*-postgres.yaml |
Seed spec fixtures for “create DB if missing” Postgres cases |
tests/input/create-nonexistent-db-*-mysql.yaml |
Seed spec fixtures for “create DB if missing” MySQL cases |
tests/docker-compose.yml |
Local developer Compose stack for integration tests |
tests/README.md |
Documentation for running unit vs integration tests and scenario list |
src/seed/executor.rs |
Moves reset behavior to phase level and threads auto_id column into inserts |
src/seed/db.rs |
Extends Database::insert_row signature; updates Postgres insert/exists logic |
CHANGELOG.md |
Documents new integration suite and related seed behavior |
.github/workflows/integration.yml |
Adds a dedicated workflow to run integration tests with service containers |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/seed/db.rs
Outdated
| let value_list: Vec<String> = values.iter().map(|v| escape_sql_value(v)).collect(); | ||
| let returning_col = sanitize_identifier(auto_id_column.unwrap_or("id")); | ||
| let sql = format!( | ||
| "INSERT INTO \"{}\" ({}) VALUES ({}) RETURNING COALESCE(CAST(\"{}\" AS BIGINT), 0)", | ||
| sanitize_identifier(table), | ||
| col_list.join(", "), | ||
| placeholders.join(", "), | ||
| sanitize_identifier(columns.first().map(|s| s.as_str()).unwrap_or("id")) | ||
| value_list.join(", "), | ||
| returning_col | ||
| ); | ||
| let params: Vec<&(dyn postgres::types::ToSql + Sync)> = values | ||
| .iter() | ||
| .map(|v| v as &(dyn postgres::types::ToSql + Sync)) | ||
| .collect(); | ||
| let row = self | ||
| .client | ||
| .query_one(&sql, params.as_slice()) | ||
| .query_one(&sql, &[]) | ||
| .map_err(|e| format!("inserting row into '{}': {}", table, e))?; | ||
| let id: i64 = row.get(0); | ||
| Ok(Some(id)) |
There was a problem hiding this comment.
PostgresDb::insert_row always adds a RETURNING ... CAST(... AS BIGINT) clause and defaults auto_id_column to "id". If a seed spec omits auto_id (valid per schema) or the table’s PK isn’t a BIGINT-compatible column named id, this will fail even though callers may not need a generated id. Consider only using RETURNING when auto_id_column is Some(_) (or when a row _ref requires it) and otherwise execute a plain INSERT and return Ok(None).
There was a problem hiding this comment.
Fixed: insert_row already only uses RETURNING when auto_id_column is Some(_). The current code correctly returns Ok(None) for plain INSERTs without auto_id.
| let value_list: Vec<String> = values.iter().map(|v| escape_sql_value(v)).collect(); | ||
| let returning_col = sanitize_identifier(auto_id_column.unwrap_or("id")); | ||
| let sql = format!( | ||
| "INSERT INTO \"{}\" ({}) VALUES ({}) RETURNING COALESCE(CAST(\"{}\" AS BIGINT), 0)", | ||
| sanitize_identifier(table), | ||
| col_list.join(", "), | ||
| placeholders.join(", "), | ||
| sanitize_identifier(columns.first().map(|s| s.as_str()).unwrap_or("id")) | ||
| value_list.join(", "), | ||
| returning_col | ||
| ); | ||
| let params: Vec<&(dyn postgres::types::ToSql + Sync)> = values | ||
| .iter() | ||
| .map(|v| v as &(dyn postgres::types::ToSql + Sync)) | ||
| .collect(); | ||
| let row = self | ||
| .client | ||
| .query_one(&sql, params.as_slice()) | ||
| .query_one(&sql, &[]) | ||
| .map_err(|e| format!("inserting row into '{}': {}", table, e))?; | ||
| let id: i64 = row.get(0); | ||
| Ok(Some(id)) | ||
| } | ||
|
|
||
| fn row_exists( | ||
| &mut self, | ||
| table: &str, | ||
| unique_columns: &[String], | ||
| unique_values: &[String], | ||
| ) -> Result<bool, String> { | ||
| if unique_columns.is_empty() { | ||
| return Ok(false); | ||
| } | ||
| let conditions: Vec<String> = unique_columns | ||
| .iter() | ||
| .enumerate() | ||
| .map(|(i, c)| format!("\"{}\" = ${}", sanitize_identifier(c), i + 1)) | ||
| .zip(unique_values.iter()) | ||
| .map(|(c, v)| format!("\"{}\" = {}", sanitize_identifier(c), escape_sql_value(v))) | ||
| .collect(); | ||
| let sql = format!( | ||
| "SELECT COUNT(*) FROM \"{}\" WHERE {}", | ||
| sanitize_identifier(table), | ||
| conditions.join(" AND ") | ||
| ); | ||
| let params: Vec<&(dyn postgres::types::ToSql + Sync)> = unique_values | ||
| .iter() | ||
| .map(|v| v as &(dyn postgres::types::ToSql + Sync)) | ||
| .collect(); | ||
| let row = self | ||
| .client | ||
| .query_one(&sql, params.as_slice()) | ||
| .query_one(&sql, &[]) | ||
| .map_err(|e| format!("checking row existence in '{}': {}", table, e))?; |
There was a problem hiding this comment.
PostgresDb::insert_row/row_exists switched from parameterized queries to interpolating escaped literals and calling query_one(..., &[]). This loses type-safe binding (e.g., NULL vs empty string), can cause subtle casting issues, and prevents query plan reuse. Prefer keeping $1..$n placeholders with a params slice, and only sanitize identifiers (table/column names).
There was a problem hiding this comment.
Acknowledged. Parameterized queries with $N placeholders send values as TEXT type, which postgres cannot implicitly cast to other column types (e.g. INTEGER). Since seed values are untyped strings targeting arbitrary columns, we must use escaped string literals (type unknown) which postgres auto-casts. Added a comment in the code explaining this rationale. Identifiers remain sanitized.
| const PG_URL: &str = "postgres://initium:initium@localhost:15432/initium_test"; | ||
| const MYSQL_URL_STR: &str = "mysql://initium:initium@localhost:13306/initium_test"; | ||
| const MYSQL_ROOT_URL_STR: &str = "mysql://root:rootpass@localhost:13306/initium_test"; | ||
|
|
||
| fn pg_client() -> postgres::Client { | ||
| postgres::Client::connect(PG_URL, postgres::NoTls).expect("failed to connect to postgres") | ||
| } | ||
|
|
||
| fn mysql_conn() -> mysql::PooledConn { | ||
| let pool = mysql::Pool::new(MYSQL_URL_STR).expect("failed to connect to mysql"); | ||
| pool.get_conn().expect("failed to get mysql connection") | ||
| } | ||
|
|
||
| fn mysql_root_conn() -> mysql::PooledConn { | ||
| let pool = mysql::Pool::new(MYSQL_ROOT_URL_STR).expect("failed to connect to mysql as root"); | ||
| pool.get_conn() | ||
| .expect("failed to get mysql root connection") | ||
| } |
There was a problem hiding this comment.
These integration tests directly reference the optional postgres and mysql crates/types. Because the project supports --no-default-features builds (e.g. --features sqlite), this test target will fail to compile when the DB driver features are disabled. Consider adding #[cfg(feature = "postgres")] / #[cfg(feature = "mysql")] guards (or splitting tests per feature) so non-default feature test runs can still compile.
There was a problem hiding this comment.
Fixed: added #[cfg(feature = "postgres")] and #[cfg(feature = "mysql")] guards to all database-specific integration tests, helper functions, and constants.
tests/README.md
Outdated
| | `test_waitfor_tcp_postgres` | wait-for TCP against Postgres succeeds | | ||
| | `test_waitfor_tcp_mysql` | wait-for TCP against MySQL succeeds | | ||
| | `test_waitfor_http_server` | wait-for HTTP against nginx returns 200 | | ||
| | `test_waitfor_nonexistent_service` | wait-for against closed port fails with exit code 1 | |
There was a problem hiding this comment.
The test list references test_waitfor_nonexistent_service, but the actual integration test function is named test_waitfor_nonexistent_service_timeout. Update the documentation to match the real test name (or rename the test) so readers can run/locate it easily.
| | `test_waitfor_nonexistent_service` | wait-for against closed port fails with exit code 1 | | |
| | `test_waitfor_nonexistent_service_timeout` | wait-for against closed port fails with exit code 1 | |
There was a problem hiding this comment.
Fixed: updated README to use the correct test name test_waitfor_nonexistent_service_timeout.
CHANGELOG.md
Outdated
| ## [Unreleased] | ||
|
|
||
| ### Changed | ||
| - Removed placeholder `seed_sets` from create-if-missing integration test YAML files; phases with only `create_if_missing` no longer need empty seed sets (`seed_sets` was already optional via `#[serde(default)]`) |
There was a problem hiding this comment.
This changelog entry says placeholder seed_sets were removed from create-if-missing integration test YAML files, but in this PR those YAML specs are being added (and some create-if-missing specs still include placeholder seed_sets). Consider rewording this to describe what actually changed in this PR (e.g., that create-if-missing phases can omit seed_sets) or adjust the YAML files to match the statement.
| - Removed placeholder `seed_sets` from create-if-missing integration test YAML files; phases with only `create_if_missing` no longer need empty seed sets (`seed_sets` was already optional via `#[serde(default)]`) | |
| - Clarified in integration test YAML specs that phases with only `create_if_missing` no longer need an explicit (possibly empty) `seed_sets` field, and added coverage for both with- and without-`seed_sets` configurations (`seed_sets` was already optional via `#[serde(default)]`) |
There was a problem hiding this comment.
Fixed: reworded CHANGELOG entry to accurately describe that seed phases with only create_if_missing can omit the seed_sets field entirely.
…er, fetch, exec, seed PostgreSQL/MySQL with cross-table refs, idempotency, reset, create database/schema, create-if-missing tests
…ix TEXT-to-INTEGER coercion
…e seed specs to files
ca62422 to
7cf446e
Compare
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
The insert_row function always used RETURNING COALESCE(CAST(id AS BIGINT), 0) even when no auto_id_column was specified, defaulting to the id column. For tables with text primary keys (like NetBird's accounts, users, groups), CAST to BIGINT fails with a generic db error. Fix: Only use RETURNING clause when auto_id_column is explicitly set. When no auto_id is needed, use a simple INSERT without RETURNING.
…, add cfg feature guards, fix README test name, document escaped literals rationale, reword CHANGELOG
No description provided.