From b6a6750499a09d68489329608d71a6e455745893 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matej=20Jane=C5=BEi=C4=8D?= Date: Wed, 9 Sep 2026 12:04:50 +0000 Subject: [PATCH] fix: dumps and imports that quietly did the wrong thing --- src/cmd/argv.rs | 32 +++- src/cmd/mod.rs | 4 + src/cmd/postgres.rs | 13 +- src/commands/postgres.rs | 252 ++++++++++++++++++++++---------- src/commands/postgres/server.rs | 25 +++- src/commands/postgres/shape.rs | 12 +- src/commands/status.rs | 20 ++- src/fsops.rs | 15 +- src/project.rs | 93 ++++++++++-- 9 files changed, 353 insertions(+), 113 deletions(-) diff --git a/src/cmd/argv.rs b/src/cmd/argv.rs index e7ae74c..ed44190 100644 --- a/src/cmd/argv.rs +++ b/src/cmd/argv.rs @@ -2,7 +2,7 @@ use std::{ ffi::{OsStr, OsString}, fmt::Display, fs::File, - os::unix::process::CommandExt, + os::unix::process::{CommandExt, ExitStatusExt}, path::Path, process::{Command, ExitStatus, Output, Stdio}, }; @@ -16,6 +16,12 @@ use crate::output::write_err; #[derive(Default, Clone)] pub struct Argv(Vec); +// a dry run spawned nothing, so there is no status; nothing ran and nothing +// failed, which is what every caller checking one of these wants to hear +fn planned() -> ExitStatus { + ExitStatus::from_raw(0) +} + impl Display for Argv { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.write_str(&self.quoted()) @@ -135,10 +141,18 @@ impl Argv { // the status rather than an error, for callers with something better to say pub fn status(&self, ctx: &Ctx) -> Result { + if self.skipped(ctx) { + return Ok(planned()); + } + Ok(self.command(ctx)?.spawn()?.wait()?) } pub fn stdin_from(&self, ctx: &Ctx, input: &Path) -> Result { + if self.skipped(ctx) { + return Ok(planned()); + } + let stdin = self.opened(input)?; Ok(self.command(ctx)?.stdin(stdin).spawn()?.wait()?) @@ -146,6 +160,22 @@ impl Argv { // both streams held back, for callers that read the command's complaints pub fn stdin_from_captured(&self, ctx: &Ctx, input: &Path) -> Result { + if self.skipped(ctx) { + return Ok(Output { + status: planned(), + stdout: Vec::new(), + stderr: Vec::new(), + }); + } + + let stdin = self.opened(input)?; + + Ok(self.command(ctx)?.stdin(stdin).output()?) + } + + // a probe that reads its input: like capture, it is asking rather than + // changing, so a dry run gets the real answer and plans with it + pub fn probe_with_stdin(&self, ctx: &Ctx, input: &Path) -> Result { let stdin = self.opened(input)?; Ok(self.command(ctx)?.stdin(stdin).output()?) diff --git a/src/cmd/mod.rs b/src/cmd/mod.rs index 4c05793..a88486f 100644 --- a/src/cmd/mod.rs +++ b/src/cmd/mod.rs @@ -80,6 +80,10 @@ pub trait Cmd { self.argv().stdin_from_captured(ctx, input) } + fn probe_with_stdin(&self, ctx: &Ctx, input: &Path) -> Result { + self.argv().probe_with_stdin(ctx, input) + } + fn quietly_succeeds(&self, ctx: &Ctx) -> Result { self.argv().quietly_succeeds(ctx) } diff --git a/src/cmd/postgres.rs b/src/cmd/postgres.rs index 74b9aea..e2dbe7d 100644 --- a/src/cmd/postgres.rs +++ b/src/cmd/postgres.rs @@ -25,6 +25,9 @@ impl Cmd for DropDb<'_> { Argv::new("dropdb") .flag("--username", self.username) .arg("--if-exists") + // a database name is a positional, and getopt would read one + // beginning with a dash as an option instead + .arg("--") .arg(self.dbname) } } @@ -42,6 +45,7 @@ impl Cmd for CreateDb<'_> { .flag("--username", self.username) .flag("--encoding", "utf8") .flag("--template", "template0") + .arg("--") .arg(self.dbname) } } @@ -76,7 +80,7 @@ impl Cmd for PgRestore<'_> { .flag("--dbname", self.dbname); match self.from { - Some(path) => argv.arg(path), + Some(path) => argv.arg("--").arg(path), None => argv, } } @@ -179,6 +183,7 @@ impl Cmd for PgDump<'_> { Some(path) => argv.flag("--file", path), None => argv, } + .arg("--") .arg(self.dbname) } } @@ -208,7 +213,7 @@ mod tests { } .argv() .quoted(), - "dropdb --username u --if-exists db" + "dropdb --username u --if-exists -- db" ); } @@ -216,11 +221,11 @@ mod tests { fn a_directory_dump_names_the_file_it_writes() { assert_eq!( PgDump::new("u", "db", "d").to("/tmp/dump").argv().quoted(), - "pg_dump --username u --format d --file /tmp/dump db" + "pg_dump --username u --format d --file /tmp/dump -- db" ); assert_eq!( PgDump::new("u", "db", "c").argv().quoted(), - "pg_dump --username u --format c db" + "pg_dump --username u --format c -- db" ); } diff --git a/src/commands/postgres.rs b/src/commands/postgres.rs index a36b9b2..735cf05 100644 --- a/src/commands/postgres.rs +++ b/src/commands/postgres.rs @@ -1,9 +1,8 @@ mod server; mod shape; -use fs_err::File; use std::io::{self, IsTerminal, Write}; -use std::path::Path; +use std::path::{Path, PathBuf}; use std::process::Stdio; use anyhow::{Context, Result, bail}; @@ -16,15 +15,29 @@ use crate::cmd::{ Stop, Up, }; use crate::ctx::Ctx; -use crate::fsops::{remove_file, rename, suffixed}; -use crate::output::note; +use crate::fsops::{create_private_new, remove_file, rename, suffixed}; +use crate::output::{note, warning}; // unique per run: docker cp will not copy a directory over an existing path, and // quietly leaves whatever was there for pg_restore to read instead fn remote_dump() -> String { - format!("/tmp/ahab-dump-{}", std::process::id()) + // the container's /tmp is shared with whatever else runs in it, and a name + // that can be worked out in advance is one a symlink can be planted at + let spun = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map_or(0, |since| since.subsec_nanos()); + + format!("/tmp/ahab-dump-{}-{spun:09}", std::process::id()) } +// beside the target but unique to this run: a fixed name is a path a second +// dump would share, and one a symlink can be planted at ahead of time +fn partial_path(file: &Path) -> PathBuf { + suffixed(file, &format!(".{}.partial", std::process::id())) +} + +// pg_dumpall recreates roles the cluster already has, so this is the expected +// complaint rather than a failure fn is_existing_role_error(line: &str) -> bool { line.starts_with("ERROR:") && line.contains("role \"") && line.ends_with("already exists") } @@ -39,13 +52,23 @@ fn restore_cluster(ctx: &Ctx, db: &Database, restore: &dyn Cmd, file: &Path) -> io::stdout().write_all(&out.stdout).ok(); let mut existing = 0; + let mut failed = 0; + for line in String::from_utf8_lossy(&out.stderr).lines() { if is_existing_role_error(line) { existing += 1; continue; } - note!(ctx, "{line}"); + // this psql runs without ON_ERROR_STOP, so it exits 0 whatever the sql + // did: these lines are the only account of what happened, which makes + // them the result rather than progress, and --quiet has to keep them + if line.starts_with("ERROR:") { + failed += 1; + warning!("{line}"); + } else { + note!(ctx, "{line}"); + } } if existing > 0 { @@ -59,17 +82,82 @@ fn restore_cluster(ctx: &Ctx, db: &Database, restore: &dyn Cmd, file: &Path) -> if !out.status.success() { bail!("psql failed, the database is left empty"); } + if failed > 0 { + bail!( + "psql reported {failed} error{}, so the cluster restored only in part", + if failed == 1 { "" } else { "s" } + ); + } Ok(()) } +// which shape is inside the compression, read through the container's own gunzip +// rather than assuming the host has one. this only reads, so it is a probe and +// runs even in a dry run, where the answer decides what the plan says +fn gzip_kind(ctx: &Ctx, db: &Database, file: &Path) -> Result { + wait_until_ready(ctx, db)?; + + let out = Gunzip + .pipe(&Head::bytes(HEADER_LEN)) + // head closes the pipe once it has its bytes, which kills gunzip + .allow_early_close() + .in_container(&db.container) + .interactive() + .probe_with_stdin(ctx, file) + .context("reading the compressed dump's header")?; + + // head exits 0 whatever gunzip did, so an empty header is the only + // sign that decompression failed + if !out.status.success() || out.stdout.is_empty() { + let reason = String::from_utf8_lossy(&out.stderr); + let reason = reason.trim(); + + bail!( + "could not decompress {}: {}", + file.display(), + if reason.is_empty() { + "no data came out" + } else { + reason + } + ); + } + + Ok(Kind::of(&out.stdout)) +} + pub fn import(ctx: &Ctx, file: &Path) -> Result<()> { let dump = Dump::of(file)?; let db = Database::resolve(ctx)?; + if matches!(dump, Dump::Directory) { + local_path_for_docker(file)?; + } + note!(ctx, "stopping all containers"); Stop { quiet: ctx.quiet }.run(ctx)?; + // everything from here runs with the project down, so an error leaves it + // that way and the user has no reason to guess as much + imported(ctx, &db, &dump, file) + .map_err(|e| e.context("the project is left stopped, `docker compose up` starts it again")) +} + +// docker cp reads `container:path`, splitting on the first colon, so a local +// path holding one is read as a container name and something else entirely +fn local_path_for_docker(file: &Path) -> Result<()> { + match file.to_string_lossy().contains(':') { + true => bail!( + "{} has a colon in it, which docker cp reads as a container name; \ + rename it to copy it in or out", + file.display() + ), + false => Ok(()), + } +} + +fn imported(ctx: &Ctx, db: &Database, dump: &Dump, file: &Path) -> Result<()> { note!(ctx, "starting db container"); Start { service: &db.service, @@ -81,42 +169,23 @@ pub fn import(ctx: &Ctx, file: &Path) -> Result<()> { // a directory cannot be streamed, so it is the one shape that gets copied in if matches!(dump, Dump::Directory) { - when_ready(ctx, &db, &Cp::into_container(file, &db.container, &remote))?; + when_ready(ctx, db, &Cp::into_container(file, &db.container, &remote))?; } let kind = match &dump { Dump::Directory => Kind::Archive, Dump::Header(header) => Kind::of(header), - Dump::Gzip => { - wait_until_ready(ctx, &db)?; - let out = Gunzip - .pipe(&Head::bytes(HEADER_LEN)) - // head closes the pipe once it has its bytes, which kills gunzip - .allow_early_close() - .in_container(&db.container) - .interactive() - .stdin_from_captured(ctx, file) - .context("reading the compressed dump's header")?; - - // head exits 0 whatever gunzip did, so an empty header is the only - // sign that decompression failed - if !out.status.success() || out.stdout.is_empty() { - let reason = String::from_utf8_lossy(&out.stderr); - let reason = reason.trim(); - - bail!( - "could not decompress {}: {}", - file.display(), - if reason.is_empty() { - "no data came out" - } else { - reason - } - ); + Dump::Gzip => match gzip_kind(ctx, db, file) { + Ok(kind) => kind, + // looking inside needs a container to decompress with, and a dry run + // is worth printing with the project down, which is when it is most + // likely to be asked for + Err(e) if ctx.dry_run => { + note!(ctx, "planning for a single database dump: {e:#}"); + Kind::Sql } - - Kind::of(&out.stdout) - } + Err(e) => return Err(e), + }, }; let restore = db.restore_with(kind); @@ -126,7 +195,7 @@ pub fn import(ctx: &Ctx, file: &Path) -> Result<()> { when_ready( ctx, - &db, + db, &DropDb { username: &db.user, dbname: &db.name, @@ -139,7 +208,7 @@ pub fn import(ctx: &Ctx, file: &Path) -> Result<()> { if kind != Kind::Cluster { when_ready( ctx, - &db, + db, &CreateDb { username: &db.user, dbname: &db.name, @@ -148,48 +217,16 @@ pub fn import(ctx: &Ctx, file: &Path) -> Result<()> { )?; } - wait_until_ready(ctx, &db)?; - if ctx.dry_run { - note!(ctx, "would restore with {tool}"); - } else { - // a directory dump was copied in whole, so pg_restore reads it from the - // container; every other shape is fed in on stdin, through gunzip when it - // arrives compressed - if matches!(dump, Dump::Directory) { - let status = PgRestore::new(&db.user, &db.name) - .from(&remote) - .in_container(&db.container) - .status(ctx)?; - - if !status.success() { - bail!("{tool} failed, the database is left empty"); - } - } else { - let restore: Box = match dump { - Dump::Gzip => Box::new(Gunzip.pipe(&*restore)), - _ => restore, - }; - - if kind == Kind::Cluster { - // psql's output is read rather than streamed here, to keep the - // expected role errors out of the way - restore_cluster(ctx, &db, &*restore, file)?; - } else { - let status = restore - .in_container(&db.container) - .interactive() - .stdin_from(ctx, file)?; - - if !status.success() { - bail!("{tool} failed, the database is left empty"); - } - } - } - } + wait_until_ready(ctx, db)?; + let restored = restore_dump(ctx, db, dump, kind, file, &remote, &tool, restore); + // the copy inside the container holds the whole database, and its /tmp + // outlives the command: it goes whether or not the restore worked, which is + // exactly when it used to be left behind if matches!(dump, Dump::Directory) { let _ = Rm::recursive(&remote).in_container(&db.container).run(ctx); } + restored?; note!(ctx, "restarting containers"); Stop { quiet: ctx.quiet }.run(ctx)?; @@ -198,6 +235,61 @@ pub fn import(ctx: &Ctx, file: &Path) -> Result<()> { Ok(()) } +#[allow(clippy::too_many_arguments)] +fn restore_dump( + ctx: &Ctx, + db: &Database, + dump: &Dump, + kind: Kind, + file: &Path, + remote: &str, + tool: &str, + restore: Box, +) -> Result<()> { + if ctx.dry_run { + note!(ctx, "would restore with {tool}"); + return Ok(()); + } + + // a directory dump was copied in whole, so pg_restore reads it from the + // container; every other shape is fed in on stdin, through gunzip when it + // arrives compressed + if matches!(dump, Dump::Directory) { + let status = PgRestore::new(&db.user, &db.name) + .from(remote) + .in_container(&db.container) + .status(ctx)?; + + if !status.success() { + bail!("{tool} failed, the database is left empty"); + } + + return Ok(()); + } + + let restore: Box = match dump { + Dump::Gzip => Box::new(Gunzip.pipe(&*restore)), + _ => restore, + }; + + if kind == Kind::Cluster { + // psql's output is read rather than streamed here, to keep the + // expected role errors out of the way + return restore_cluster(ctx, db, &*restore, file); + } + + let status = restore + .in_container(&db.container) + .interactive() + .stdin_from(ctx, file)?; + + if !status.success() { + bail!("{tool} failed, the database is left empty"); + } + + Ok(()) +} + pub fn psql(ctx: &Ctx, rest: &[String]) -> Result<()> { let db = Database::resolve(ctx)?; @@ -225,12 +317,12 @@ pub fn dump(ctx: &Ctx, file: &Path, format: Format, gzip: bool) -> Result<()> { // written beside the target and renamed once the dump succeeds, so a failure // cannot destroy the dump that is already there - let partial = suffixed(file, ".partial"); + let partial = partial_path(file); // a dry run produces no dump, so it must not lay a hand on the target either let stdout = if ctx.dry_run { Stdio::null() } else { - Stdio::from(std::fs::File::from(File::create(&partial)?)) + Stdio::from(create_private_new(&partial)?) }; let dumping = dump_command(&db, format); @@ -265,7 +357,9 @@ fn dump_command(db: &Database, format: Format) -> Box { // pg_dump writes a directory format dump itself rather than to stdout, so it lands // in the container and comes back with docker cp fn dump_directory(ctx: &Ctx, db: &Database, target: &Path) -> Result<()> { - if target.exists() { + // symlink_metadata rather than exists(), which follows the link and so is + // false for a dangling one: docker cp would then write through it + if std::fs::symlink_metadata(target).is_ok() { bail!( "{} already exists; a directory dump will not be written over it", target.display() diff --git a/src/commands/postgres/server.rs b/src/commands/postgres/server.rs index 7e840cd..0f92b73 100644 --- a/src/commands/postgres/server.rs +++ b/src/commands/postgres/server.rs @@ -1,7 +1,7 @@ use std::thread; use std::time::{Duration, Instant}; -use anyhow::{Result, anyhow, bail}; +use anyhow::{Result, bail}; use super::shape::Kind; use crate::cmd::{Cmd, PgIsReady, PgRestore, Ps, Psql}; @@ -10,6 +10,8 @@ use crate::project::Project; const READY_TIMEOUT: Duration = Duration::from_secs(60); const POLL_INTERVAL: Duration = Duration::from_secs(1); +// stands in for an id a dry run has no running container to look up +const PLANNED_CONTAINER: &str = ""; pub(super) struct Database { pub(super) service: String, @@ -22,13 +24,24 @@ impl Database { pub(super) fn resolve(ctx: &Ctx) -> Result { let compose = Project::resolve(ctx)?; let service = compose.postgres()?; - let (user, name) = compose.postgres_credentials(&service); + let (user, name) = compose.postgres_credentials(&service)?; - let container = Ps::id_of(&service).capture(ctx)?.trim().to_string(); + let listed = Ps::id_of(&service).capture(ctx)?; + let mut ids = listed.lines().map(str::trim).filter(|id| !id.is_empty()); - if container.is_empty() { - return Err(anyhow!("service {service} has no running container")); - } + let container = match (ids.next(), ids.next()) { + (Some(id), None) => id.to_string(), + // one id per line: a scaled service has several, and the whole + // listing would go to docker exec as though it were a single id + (Some(_), Some(_)) => bail!( + "service {service} has more than one container running; \ + scale it to one first" + ), + // a dry run only prints a plan, and it is worth printing with the + // project down, which is when it is most likely to be asked for + (None, _) if ctx.dry_run => PLANNED_CONTAINER.to_string(), + (None, _) => bail!("service {service} has no running container"), + }; Ok(Self { service, diff --git a/src/commands/postgres/shape.rs b/src/commands/postgres/shape.rs index 7508fc4..5add983 100644 --- a/src/commands/postgres/shape.rs +++ b/src/commands/postgres/shape.rs @@ -55,14 +55,14 @@ impl Kind { } } -// pg_dumpall recreates roles the cluster already has, so this is the expected - fn read_header(path: &Path) -> Result> { - let mut file = File::open(path)?; - let mut header = vec![0; HEADER_LEN]; + let file = File::open(path)?; + let mut header = Vec::with_capacity(HEADER_LEN); - let read = file.read(&mut header)?; - header.truncate(read); + // read fills what it can rather than what was asked for, and a header that + // arrives in more than one piece would lose the cluster marker sitting a few + // bytes in, leaving a whole-cluster dump looking like a single database + file.take(HEADER_LEN as u64).read_to_end(&mut header)?; Ok(header) } diff --git a/src/commands/status.rs b/src/commands/status.rs index 0da1340..05651e2 100644 --- a/src/commands/status.rs +++ b/src/commands/status.rs @@ -69,14 +69,20 @@ fn role(ctx: &Ctx, role: &str, detected: Result, project: &Project) { } if role == "postgres" { - let (user, database) = project.postgres_credentials(&service); - let source = |key: &str| match project.env(&service, key) { - Some(_) => "", - None => " (default, nothing in the environment)", - }; + // status reports what it cannot work out rather than stopping, so a name + // the postgres tools would refuse is something to say, not to fail on + match project.postgres_credentials(&service) { + Ok((user, database)) => { + let source = |key: &str| match project.env(&service, key) { + Some(_) => "", + None => " (default, nothing in the environment)", + }; - line!("\tuser: {user}{}", source("POSTGRES_USER")); - line!("\tdatabase: {database}{}", source("POSTGRES_DB")); + line!("\tuser: {user}{}", source("POSTGRES_USER")); + line!("\tdatabase: {database}{}", source("POSTGRES_DB")); + } + Err(e) => line!("\tcredentials: {e:#}"), + } } } diff --git a/src/fsops.rs b/src/fsops.rs index 9615028..c97bd63 100644 --- a/src/fsops.rs +++ b/src/fsops.rs @@ -1,5 +1,5 @@ use std::io::Write; -use std::os::unix::fs::DirBuilderExt; +use std::os::unix::fs::{DirBuilderExt, OpenOptionsExt}; use std::path::{Path, PathBuf}; use anyhow::{Context, Result, anyhow}; @@ -74,6 +74,19 @@ pub fn place_link(ctx: &Ctx, link_path: &Path, target: &Path) -> Result<()> { Ok(fs::rename(&tmp, link_path)?) } +// 0600 and only where nothing is yet. a dump is the whole database, and +// pg_dumpall's is every role's password hash, so it is not the owner's to share +// by default; create_new also refuses to follow a symlink planted at the path, +// which File::create would open and truncate +pub fn create_private_new(path: &Path) -> Result { + std::fs::OpenOptions::new() + .write(true) + .create_new(true) + .mode(0o600) + .open(path) + .with_context(|| format!("creating {}", path.display())) +} + // the store holds what should not be reachable from the repository, so the // directories it is kept in are the owner's alone. only the tree at or below // `base` is created here; what is above it is the user's own business diff --git a/src/project.rs b/src/project.rs index 239df00..124c936 100644 --- a/src/project.rs +++ b/src/project.rs @@ -47,10 +47,23 @@ impl Project { match serving.as_slice() { [only] => Ok(only.to_string()), + // publishing ports is the tie-break, so say which way it + // failed: none of them serving is the opposite complaint + // from all of them serving + [] => Err(anyhow!( + "cannot tell which service runs django, {} all build an image \ + and set {DJANGO_SETTINGS_MODULE}, and none publishes ports \ + to tell them apart", + several.join(", ") + )), _ => Err(anyhow!( "cannot tell which service runs django, {} all build an image, set \ {DJANGO_SETTINGS_MODULE} and publish ports", - several.join(", ") + serving + .iter() + .map(|name| **name) + .collect::>() + .join(", ") )), } } @@ -82,13 +95,16 @@ impl Project { } } - pub fn postgres_credentials(&self, service: &str) -> (String, String) { - let service = &self.services[service]; + pub fn postgres_credentials(&self, service: &str) -> Result<(String, String)> { + let entry = &self.services[service]; - let user = env_var(service, "POSTGRES_USER").unwrap_or_else(|| "db".to_string()); - let database = env_var(service, "POSTGRES_DB").unwrap_or_else(|| "db".to_string()); + let user = env_var(entry, "POSTGRES_USER").unwrap_or_else(|| "db".to_string()); + let database = env_var(entry, "POSTGRES_DB").unwrap_or_else(|| "db".to_string()); - (user, database) + Ok(( + usable_name(service, "POSTGRES_USER", user)?, + usable_name(service, "POSTGRES_DB", database)?, + )) } pub fn names(&self) -> Vec<&str> { @@ -128,6 +144,35 @@ fn env_var(service: &Value, key: &str) -> Option { .map(str::to_string) } +// the compose file chooses these, and every postgres tool ahab runs takes them +// as a role or database name. libpq reads a name holding `=` or a uri as a whole +// connection string, so one could send a dump to another server entirely, and a +// leading dash is read as an option however the argv is quoted +fn usable_name(service: &str, key: &str, value: String) -> Result { + let wrong = if value.is_empty() { + Some("is empty") + } else if value.starts_with('-') { + Some("starts with a dash, which the postgres tools read as an option") + } else if value.contains('=') { + Some("holds an `=`, which libpq reads as a connection string") + } else if value.contains("://") { + Some("holds a url, which libpq reads as a connection string") + } else if value.chars().any(|c| c.is_whitespace() || c.is_control()) { + Some("holds whitespace") + } else { + None + }; + + match wrong { + None => Ok(value), + Some(wrong) => Err(anyhow!( + "{key} of service {service} {wrong}: {value:?}. \ + ahab passes it to psql, pg_dump and dropdb as a name, \ + so it has to be one" + )), + } +} + fn is_postgres_image(image: &str) -> bool { let image = image.to_lowercase(); POSTGRES_IMAGES.iter().any(|kind| image.contains(kind)) @@ -135,13 +180,43 @@ fn is_postgres_image(image: &str) -> bool { #[cfg(test)] mod tests { - use super::{Project, is_postgres_image}; + use super::{Project, is_postgres_image, usable_name}; use serde_json::json; fn compose(services: serde_json::Value) -> Project { Project { services } } + #[test] + fn a_name_the_postgres_tools_would_read_as_something_else_is_refused() { + // libpq takes a dbname holding `=` or a url as a whole conninfo string, + // so a compose file could otherwise pick the server a dump goes to + let refused = [ + "postgresql://myproject:pw@evil.example.net:5432/loot", + "host=evil.example.net dbname=loot", + "--file=/var/lib/postgresql/data/pg_hba.conf", + "-h evil.example.net", + "my db", + "", + ]; + + for value in refused { + assert!( + usable_name("db", "POSTGRES_DB", value.to_string()).is_err(), + "should have been refused: {value:?}" + ); + } + + // and the names real projects actually use are left alone + for value in ["myproject_db", "my-db", "db", "app.prod", "DB_2"] { + assert_eq!( + usable_name("db", "POSTGRES_DB", value.to_string()).unwrap(), + value, + "should have been accepted: {value:?}" + ); + } + } + #[test] fn recognises_postgres_flavours() { for image in [ @@ -242,7 +317,7 @@ mod tests { }, })); - let (user, database) = compose.postgres_credentials("db"); + let (user, database) = compose.postgres_credentials("db").unwrap(); assert_eq!( (user.as_str(), database.as_str()), ("myproject", "myproject_db") @@ -253,7 +328,7 @@ mod tests { fn falls_back_to_db_when_the_service_says_nothing() { let compose = compose(json!({"db": {"image": "postgres:18"}})); - let (user, database) = compose.postgres_credentials("db"); + let (user, database) = compose.postgres_credentials("db").unwrap(); assert_eq!((user.as_str(), database.as_str()), ("db", "db")); } }