diff --git a/src/commands/migration/apply.rs b/src/commands/migration/apply.rs index 7b6564b..73bc7fc 100644 --- a/src/commands/migration/apply.rs +++ b/src/commands/migration/apply.rs @@ -221,9 +221,13 @@ mod tests { // Resolve it, as a real render would while streaming to psql. secrets.resolve("application_password").await.unwrap(); + // Value at the deepest level. Only reached because redact_secrets + // formats with `{:?}`, which walks `source()`; `{}` would leak it. let simulated_psql_error = anyhow!( "psql exited with code 1: ERROR: duplicate key value\nDETAIL: Key (password)=(hunter2) already exists." - ); + ) + .context("could not render include: error in \"roles/app_role.sql\"") + .context("Migration '20260101000000-bootstrap' failed"); let redacted = redact_secrets(simulated_psql_error, &secrets, SqlDialect::Postgres); diff --git a/src/docs.rs b/src/docs.rs new file mode 100644 index 0000000..39761e8 --- /dev/null +++ b/src/docs.rs @@ -0,0 +1,20 @@ +//! URLs of published doc pages, for anywhere Spawn produces output — errors, +//! `--help`, warnings. +//! +//! Centralised so a moved page is one edit here, not a hunt through every +//! string naming it. `tests/doc_links.rs` verifies each still resolves. + +/// Declares a link and registers it in `ALL`, so a new constant can't escape +/// the link test. +macro_rules! docs { + ($($(#[$m:meta])* $name:ident = $slug:literal;)*) => { + $($(#[$m])* pub const $name: &str = concat!("https://docs.spawn.dev/", $slug, "/");)* + /// Every link above, as (constant name, slug), for `tests/doc_links.rs`. + pub const ALL: &[(&str, &str)] = &[$((stringify!($name), $slug)),*]; + }; +} + +docs! { + /// Defining `[secrets]` and the sources available to them. + SECRETS = "guides/secrets"; +} diff --git a/src/engine/mod.rs b/src/engine/mod.rs index f94bdcb..a4c4b92 100644 --- a/src/engine/mod.rs +++ b/src/engine/mod.rs @@ -109,8 +109,10 @@ pub enum MigrationError { info: ExistingMigrationInfo, }, - /// Database or connection error - #[error("database error: {0}")] + /// Database or connection error. + /// + /// Transparent: a prefix would duplicate the line `source()` already gives. + #[error(transparent)] Database(#[from] anyhow::Error), // Could not get advisory lock diff --git a/src/engine/postgres_psql.rs b/src/engine/postgres_psql.rs index 98049c1..c56c391 100644 --- a/src/engine/postgres_psql.rs +++ b/src/engine/postgres_psql.rs @@ -901,30 +901,33 @@ impl PSQL { let duration = start_time.elapsed().as_secs_f32(); - // Determine status based on session 1 result - let (status, migration_error) = match &migration_result { - Ok(()) => (MigrationStatus::Success, None), - Err(EngineError::ExecutionFailed { exit_code, stderr }) => { - if stderr.contains("Could not acquire advisory lock") { - return Err(MigrationError::AdvisoryLock(std::io::Error::new( - std::io::ErrorKind::Other, - stderr.clone(), - ))); + // Determine status based on session 1 result. + let (status, migration_error): (MigrationStatus, Option) = + match migration_result { + Ok(()) => (MigrationStatus::Success, None), + Err(EngineError::ExecutionFailed { exit_code, stderr }) => { + if stderr.contains("Could not acquire advisory lock") { + return Err(MigrationError::AdvisoryLock(std::io::Error::other(stderr))); + } + ( + MigrationStatus::Failure, + Some(anyhow!("psql exited with code {}: {}", exit_code, stderr)), + ) } - ( - MigrationStatus::Failure, - Some(format!("psql exited with code {}: {}", exit_code, stderr)), - ) - } - Err(EngineError::Io(e)) => { - // Writer failed (e.g. an unresolved secret), not psql itself. - // Still record a Failure row — SQL may already have run. - ( - MigrationStatus::Failure, - Some(format!("failed while streaming migration SQL: {}", e)), - ) - } - }; + Err(EngineError::Io(e)) => { + // Writer failed (e.g. an unresolved secret), not psql itself. + // Still record a Failure row — SQL may already have run. + ( + MigrationStatus::Failure, + Some( + anyhow::Error::from(e).context("failed while streaming migration SQL"), + ), + ) + } + }; + + // NotRecorded holds strings; `{:#}` flattens the chain onto one line. + let migration_error_text = migration_error.as_ref().map(|e| format!("{:#}", e)); // Session 2: Record the outcome (success or failure) let record_result = self @@ -948,25 +951,24 @@ impl PSQL { name: migration_name.to_string(), migration_outcome: MigrationStatus::Success, migration_error: None, - recording_error: format!("{}", record_err), + recording_error: format!("{:#}", anyhow::Error::new(record_err)), }); } // Both migration and recording failed return Err(MigrationError::NotRecorded { name: migration_name.to_string(), migration_outcome: MigrationStatus::Failure, - migration_error: migration_error.clone(), - recording_error: format!("{}", record_err), + migration_error: migration_error_text, + recording_error: format!("{:#}", anyhow::Error::new(record_err)), }); } - // If the migration itself failed (but was recorded), return that error - if let Some(err_msg) = migration_error { - return Err(MigrationError::Database(anyhow!( - "Migration '{}' failed: {}", - migration_name, - err_msg - ))); + // If the migration itself failed (but was recorded), return that error. + // A context layer, not an interpolated string, so the chain survives. + if let Some(err) = migration_error { + return Err(MigrationError::Database( + err.context(format!("Migration '{}' failed", migration_name)), + )); } Ok("Migration applied successfully".to_string()) diff --git a/src/lib.rs b/src/lib.rs index 1773a88..c31e1a7 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -2,6 +2,7 @@ pub mod cli; pub mod commands; pub mod completions; pub mod config; +pub mod docs; pub mod engine; pub mod escape; pub mod hash; diff --git a/src/secrets.rs b/src/secrets.rs index 5081821..ff4de1c 100644 --- a/src/secrets.rs +++ b/src/secrets.rs @@ -178,11 +178,31 @@ impl SecretsRepository { ) } + /// The error for a `secret()` call naming something that isn't configured. + /// + /// Lists the defined names so a typo is obvious; names are safe to print, + /// their values are not. Single line, because minijinja appends + /// `(in :)` after it. + fn undefined_secret(&self, name: &str) -> anyhow::Error { + let mut known: Vec<&str> = self.secrets.keys().map(String::as_str).collect(); + known.sort_unstable(); + let defined = if known.is_empty() { + "none are defined".to_string() + } else { + format!("defined: {}", known.join(", ")) + }; + + anyhow!( + "secret '{name}' is not defined in spawn.toml ({defined}); see {url}", + url = crate::docs::SECRETS + ) + } + pub async fn resolve(&self, name: &str) -> Result { let secret = self .secrets .get(name) - .ok_or_else(|| anyhow!("no secret named '{}' is defined in spawn.toml", name))?; + .ok_or_else(|| self.undefined_secret(name))?; // Always resolve for real, even when masking the result: this is // what lets `build`/`test build` verify a secret is reachable @@ -383,10 +403,59 @@ mod tests { } #[tokio::test] - async fn missing_secret_definition_errors() { + async fn missing_secret_with_none_configured_says_so_and_shows_the_fix() { let repository = repo(HashMap::new(), "prod", SecretsRenderMode::Revealed); let err = repository.resolve("nope").await.unwrap_err(); - assert!(err.to_string().contains("no secret named 'nope'")); + let message = err.to_string(); + assert!(message.contains("secret 'nope' is not defined in spawn.toml")); + assert!( + message.contains("none are defined"), + "should say nothing is configured, got: {message}" + ); + assert!( + message.contains("https://docs.spawn.dev/guides/secrets/"), + "should point at the secrets guide, got: {message}" + ); + } + + #[tokio::test] + async fn missing_secret_lists_the_defined_names_but_never_their_values() { + let definitions = defs(vec![ + ( + "zeta_password", + SecretDefinition { + default: SecretSource::Literal { + value: "s3cret-zeta-value".to_string(), + insecure: true, + }, + environments: HashMap::new(), + }, + ), + ( + "alpha_password", + SecretDefinition { + default: SecretSource::Literal { + value: "s3cret-alpha-value".to_string(), + insecure: true, + }, + environments: HashMap::new(), + }, + ), + ]); + let repository = repo(definitions, "prod", SecretsRenderMode::Revealed); + let err = repository.resolve("aplha_password").await.unwrap_err(); + let message = format!("{:?}", err); + + assert!(message.contains("secret 'aplha_password' is not defined in spawn.toml")); + assert!( + message.contains("defined: alpha_password, zeta_password"), + "should list defined names in sorted order, got: {message}" + ); + // Names are safe to print; their values are not. + assert!( + !message.contains("s3cret-alpha-value") && !message.contains("s3cret-zeta-value"), + "a configured secret's value must never appear, got: {message}" + ); } #[tokio::test] diff --git a/src/template.rs b/src/template.rs index 11c7f23..d60a869 100644 --- a/src/template.rs +++ b/src/template.rs @@ -356,6 +356,9 @@ pub struct Generation { pub struct StreamingGeneration { store: Store, template_contents: String, + /// The template's real path, used as its minijinja template name so render + /// errors name the file on disk. + template_label: String, /// Name of the migration or test being rendered, exposed to templates /// as `builtin.script_name` (see `BuiltinContext`). script_name: String, @@ -404,8 +407,8 @@ impl StreamingGeneration { self.engine.clone(), ); let mut env = template_env(self.store, &self.engine, self.secrets, builtin)?; - env.add_template("migration.sql", &self.template_contents)?; - let tmpl = env.get_template("migration.sql")?; + env.add_template(&self.template_label, &self.template_contents)?; + let tmpl = env.get_template(&self.template_label)?; tmpl.render_to_write( context!(env => self.environment, variables => self.variables), writer, @@ -507,6 +510,7 @@ pub async fn generate_streaming_with_store( Ok(StreamingGeneration { store, template_contents: contents, + template_label: path.to_string(), script_name: script_name.to_string(), script_type, environment: environment.to_string(), @@ -964,6 +968,7 @@ mod tests { StreamingGeneration { store, template_contents: template_contents.to_string(), + template_label: "migrations/test/up.sql".to_string(), script_name: "test".to_string(), script_type: ScriptType::Migration, environment: "prod".to_string(), @@ -1053,4 +1058,80 @@ mod tests { "different up.sql content must produce different checksums" ); } + + /// minijinja hides an `{% include %}` failure's real cause behind a + /// `BadInclude` wrapper, reachable only via `.source()`. + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn include_failure_keeps_the_real_cause_reachable() { + use crate::config::FolderPather; + use opendal::services::Memory; + use opendal::Operator; + + let op = Operator::new(Memory::default()).unwrap(); + op.write( + "components/roles/app_role.sql", + r#"CREATE ROLE app_user WITH LOGIN PASSWORD {{ secret("application_password") }};"#, + ) + .await + .unwrap(); + + let pather = FolderPather { + spawn_folder: "".to_string(), + }; + let store = Store::new(Box::new(Latest::new("").unwrap()), op.clone(), pather).unwrap(); + + let gen = StreamingGeneration { + store, + template_contents: "BEGIN;\n\n{% include \"roles/app_role.sql\" %}\n\nCOMMIT;\n" + .to_string(), + template_label: "migrations/20260101000000-bootstrap/up.sql".to_string(), + script_name: "20260101000000-bootstrap".to_string(), + script_type: ScriptType::Migration, + environment: "staging".to_string(), + variables: crate::variables::Variables::default(), + engine: EngineType::PostgresPSQL, + secrets: Arc::new(SecretsRepository::empty(op)), + pin_hash: None, + }; + + let mut buffer = Vec::new(); + let err = gen.render_to_writer(&mut buffer).unwrap_err(); + let chain = format!("{:?}", err); + + assert!( + chain.contains("is not defined in spawn.toml"), + "the underlying cause must survive the BadInclude wrapper, got: {chain}" + ); + assert!( + chain.contains("roles/app_role.sql:1"), + "the cause must name the component and the line within it, got: {chain}" + ); + assert!( + chain.contains("migrations/20260101000000-bootstrap/up.sql:3"), + "the outer frame must name the migration's real path and line, got: {chain}" + ); + } + + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn render_errors_are_labelled_with_the_templates_real_path() { + let gen = streaming_generation( + r#"SELECT {{ secret("nope") }};"#, + SecretsRepository::empty( + opendal::Operator::new(opendal::services::Memory::default()).unwrap(), + ), + ); + + let mut buffer = Vec::new(); + let err = gen.render_to_writer(&mut buffer).unwrap_err(); + let chain = format!("{:?}", err); + + assert!( + chain.contains("migrations/test/up.sql"), + "expected the real template path in the error, got: {chain}" + ); + assert!( + !chain.contains("migration.sql"), + "the hardcoded placeholder name must be gone, got: {chain}" + ); + } } diff --git a/tests/doc_links.rs b/tests/doc_links.rs new file mode 100644 index 0000000..fd5b539 --- /dev/null +++ b/tests/doc_links.rs @@ -0,0 +1,17 @@ +//! Starlight derives a page's URL from its path under +//! `docs/src/content/docs`, so checking the file checks the URL. + +use std::path::Path; + +#[test] +fn every_doc_link_points_at_a_real_page() { + let root = Path::new(env!("CARGO_MANIFEST_DIR")).join("docs/src/content/docs"); + + for (name, slug) in spawn_db::docs::ALL { + let page = root.join(slug); + assert!( + ["md", "mdx"].iter().any(|ext| page.with_extension(ext).exists()), + "docs::{name} points at /{slug}/, but no docs/src/content/docs/{slug}.md or .mdx exists" + ); + } +} diff --git a/tests/integration_postgres.rs b/tests/integration_postgres.rs index 3ab9964..39dcf5a 100644 --- a/tests/integration_postgres.rs +++ b/tests/integration_postgres.rs @@ -1346,13 +1346,25 @@ async fn test_migration_writer_failure_still_records_history() -> Result<()> { IntegrationTestHelper::new("test_migration_writer_failure_still_records_history", None) .await?; + // Inside an {% include %}: minijinja hides an include's real cause behind + // "could not render include". + let config = helper.migration_helper.load_config().await?; + helper + .migration_helper + .fs + .write( + &format!("{}/roles/app_role.sql", config.pather().components_folder()), + r#"CREATE ROLE app_user WITH LOGIN PASSWORD {{ secret("does_not_exist") }};"#, + ) + .await?; + // CREATE TABLE autocommits before the engine reaches the failing // secret() call, proving streamed SQL persists past a writer failure. let bad_migration = r#"CREATE TABLE writer_failure_check (id integer); BEGIN; -SELECT {{ secret("does_not_exist") }}; +{% include "roles/app_role.sql" %} COMMIT;"#; @@ -1362,9 +1374,29 @@ COMMIT;"#; .await?; let result = helper.apply_migration(&migration_name).await; + let apply_err = result + .err() + .expect("Expected migration with an undefined secret() call to fail"); + + // apply must report *why* rendering stopped, not just that it did. + let chain = format!("{:?}", apply_err); assert!( - result.is_err(), - "Expected migration with an undefined secret() call to fail" + chain.contains("secret 'does_not_exist' is not defined in spawn.toml"), + "apply must surface the underlying cause, got: {}", + chain + ); + assert!( + chain.contains("roles/app_role.sql:1"), + "apply must name the component and line the failure came from, got: {}", + chain + ); + assert!( + chain.contains(&format!( + "{}:5", + config.pather().migration_script_file_path(&migration_name) + )), + "apply must name the migration's real path and the include's line, got: {}", + chain ); assert!( diff --git a/tests/migration_build.rs b/tests/migration_build.rs index e81f823..30f0d0f 100644 --- a/tests/migration_build.rs +++ b/tests/migration_build.rs @@ -948,6 +948,62 @@ async fn test_pin_verify_detects_missing_root() -> Result<(), Box Result<(), Box> { + let helper = MigrationTestHelper::new_empty().await?; + let config = helper.load_config().await?; + + helper + .fs + .write( + &format!("{}/roles/app_role.sql", config.pather().components_folder()), + r#"CREATE ROLE app_user WITH LOGIN PASSWORD {{ secret("application_password") }};"#, + ) + .await?; + + let migration_name = helper + .create_migration_manual( + "create-app-role", + "BEGIN;\n\n{% include \"roles/app_role.sql\" %}\n\nCOMMIT;\n".to_string(), + ) + .await?; + + let err = helper + .build_migration(&migration_name, false) + .await + .expect_err("a migration referencing an undefined secret must fail to build"); + let chain = format!("{:?}", err); + + assert!( + chain.contains("secret 'application_password' is not defined in spawn.toml"), + "the real cause must survive minijinja's include wrapper, got: {chain}" + ); + assert!( + chain.contains("none are defined"), + "with no secrets configured at all, the message should say so, got: {chain}" + ); + assert!( + chain.contains("roles/app_role.sql:1"), + "the cause must name the component and the line within it, got: {chain}" + ); + assert!( + chain.contains(&format!( + "{}:3", + config.pather().migration_script_file_path(&migration_name) + )), + "the outer frame must name the migration's real path and line, got: {chain}" + ); + assert!( + !chain.contains("(in migration.sql"), + "the hardcoded placeholder template name must be gone, got: {chain}" + ); + + Ok(()) +} + /// Exercises the full stack for secrets: a [secrets.*] table round-tripped /// through real TOML (via ConfigLoaderSaver::save/Config::load), an /// environment-specific override, and both masked and revealed builds.