From 715ec60abcd61c06be7c2de77b79a52e27a05f6a Mon Sep 17 00:00:00 2001 From: Pino de Candia <32303022+pinodeca@users.noreply.github.com> Date: Tue, 19 May 2026 23:56:23 +0000 Subject: [PATCH 1/7] feat(provider): add MigrationPolicy / ProviderConfig for VerifyOnly construction Introduces an optional ProviderConfig accepted by two new constructors: - PostgresProvider::new_with_config(url, config) - PostgresProvider::new_with_schema_and_config(url, schema, config) ProviderConfig::migration_policy controls startup behavior: - ApplyAll (default): apply pending embedded migrations, identical to today. - VerifyOnly: do not run DDL; verify _duroxide_migrations exists and every bundled migration has been applied. Intended for processes that must not modify schema -- e.g. application backends, when a separately privileged worker is responsible for migrations. VerifyOnly does not take the migration advisory lock and does not create or modify any database objects, eliminating per-connection lock contention and DDL-grant requirements for backend processes. Existing constructors (new, new_with_schema, new_with_entra, new_with_schema_and_entra) are unchanged at the source and behavior level; new_with_schema is refactored to delegate to new_with_schema_and_config with ProviderConfig::default(), preserving ApplyAll semantics. New types are re-exported from the crate root. Added integration tests in tests/migration_policy_tests.rs covering ApplyAll, successful VerifyOnly against an initialized schema, and VerifyOnly's error path against an uninitialized schema. --- CHANGELOG.md | 20 +++++ src/lib.rs | 2 +- src/migrations.rs | 58 +++++++++++++++ src/provider.rs | 70 +++++++++++++++++- tests/migration_policy_tests.rs | 125 ++++++++++++++++++++++++++++++++ 5 files changed, 272 insertions(+), 3 deletions(-) create mode 100644 tests/migration_policy_tests.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 0bade2b..c66184f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,26 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### Added + +- **Configurable migration policy at provider construction.** New + `MigrationPolicy` enum and `ProviderConfig` struct, plus two new + constructors `PostgresProvider::new_with_config` and + `PostgresProvider::new_with_schema_and_config`. The default policy is + `MigrationPolicy::ApplyAll`, which preserves pre-feature behavior — all + existing constructors (`new`, `new_with_schema`, `new_with_entra`, + `new_with_schema_and_entra`) continue to apply pending migrations on + startup. The new `MigrationPolicy::VerifyOnly` policy skips migration + application and instead verifies that the `_duroxide_migrations` tracking + table exists in the target schema and that every embedded migration has + already been applied, returning an error otherwise. Intended for + processes that must not run DDL — e.g. application backends, where a + separately privileged worker is responsible for applying schema + changes. `VerifyOnly` does not take the migration advisory lock and does + not create or modify any database objects. + ## [0.1.33] - 2026-05-13 ### Fixed diff --git a/src/lib.rs b/src/lib.rs index fd21d04..09b0493 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -82,4 +82,4 @@ pub mod migrations; pub mod provider; pub use entra::EntraAuthOptions; -pub use provider::PostgresProvider; +pub use provider::{MigrationPolicy, PostgresProvider, ProviderConfig}; diff --git a/src/migrations.rs b/src/migrations.rs index 1a2228c..5d07f04 100644 --- a/src/migrations.rs +++ b/src/migrations.rs @@ -77,6 +77,64 @@ impl MigrationRunner { result } + /// Verify that the migration tracking table exists and that every embedded + /// migration has already been applied. Does not take the migration + /// advisory lock and does not create or modify any database objects. + /// + /// Returns an error if the `_duroxide_migrations` table is missing in + /// `schema_name` or if any bundled migration version is absent from it. + /// + /// Intended for processes that must not perform DDL (e.g. application + /// backends, where a separately privileged worker is responsible for + /// applying schema changes). + pub async fn verify(&self) -> Result<()> { + let mut conn = self.pool.acquire().await?; + let conn = &mut *conn; + + // Check that the tracking table exists in the target schema. + let table_exists: bool = sqlx::query_scalar( + "SELECT EXISTS(SELECT 1 FROM information_schema.tables \ + WHERE table_schema = $1 AND table_name = '_duroxide_migrations')", + ) + .bind(&self.schema_name) + .fetch_one(&mut *conn) + .await?; + + if !table_exists { + anyhow::bail!( + "duroxide migrations not initialized: schema {:?} does not \ + contain _duroxide_migrations. Construct a provider with \ + MigrationPolicy::ApplyAll (the default) from a process with \ + DDL privileges before using MigrationPolicy::VerifyOnly.", + self.schema_name + ); + } + + let migrations = self.load_migrations()?; + let applied: std::collections::HashSet = + self.get_applied_versions(conn).await?.into_iter().collect(); + + let mut missing: Vec = migrations + .iter() + .map(|m| m.version) + .filter(|v| !applied.contains(v)) + .collect(); + missing.sort_unstable(); + + if !missing.is_empty() { + anyhow::bail!( + "duroxide migrations not up to date in schema {:?}: missing \ + versions {:?}. Run migrations from a provider configured with \ + MigrationPolicy::ApplyAll before constructing VerifyOnly \ + providers.", + self.schema_name, + missing, + ); + } + + Ok(()) + } + async fn migrate_inner(&self, conn: &mut sqlx::postgres::PgConnection) -> Result<()> { // Ensure schema exists if self.schema_name != "public" { diff --git a/src/provider.rs b/src/provider.rs index ad8fc2d..80ef00c 100644 --- a/src/provider.rs +++ b/src/provider.rs @@ -107,6 +107,51 @@ pub struct PostgresProvider { _refresh_task: Option, } +/// Migration policy applied at [`PostgresProvider`] construction. +/// +/// Defaults to [`MigrationPolicy::ApplyAll`], preserving pre-feature behavior: +/// all constructors that do not take a [`ProviderConfig`] apply pending +/// migrations on startup. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub enum MigrationPolicy { + /// Apply any pending embedded migrations at startup. Default. + /// + /// Requires the database role to have DDL privileges on `schema_name` + /// (or `public` if none was supplied). + #[default] + ApplyAll, + /// Skip migration application. Verify that the `_duroxide_migrations` + /// tracking table exists and every embedded migration has already been + /// applied; return an error otherwise. + /// + /// Intended for processes that must not run DDL — e.g. application + /// backends, when a separately privileged worker is responsible for + /// applying schema changes. + VerifyOnly, +} + +/// Optional configuration for [`PostgresProvider`] constructors. +/// +/// Construct via [`ProviderConfig::default`] and adjust fields as needed: +/// +/// ```rust,no_run +/// use duroxide_pg::{MigrationPolicy, PostgresProvider, ProviderConfig}; +/// +/// # async fn example() -> anyhow::Result<()> { +/// let mut config = ProviderConfig::default(); +/// config.migration_policy = MigrationPolicy::VerifyOnly; +/// let provider = +/// PostgresProvider::new_with_config("postgres://localhost/mydb", config).await?; +/// # Ok(()) +/// # } +/// ``` +#[derive(Debug, Clone, Default)] +#[non_exhaustive] +pub struct ProviderConfig { + /// Policy for handling embedded migrations at startup. + pub migration_policy: MigrationPolicy, +} + /// Newtype around `tokio::task::AbortHandle` that aborts the task on drop. /// Used to ensure the Entra token refresh task is cleaned up when the /// provider is dropped. @@ -124,6 +169,24 @@ impl PostgresProvider { } pub async fn new_with_schema(database_url: &str, schema_name: Option<&str>) -> Result { + Self::new_with_schema_and_config(database_url, schema_name, ProviderConfig::default()).await + } + + /// Same as [`Self::new`] but accepts a [`ProviderConfig`] to control + /// startup behavior (currently: migration application policy). + pub async fn new_with_config(database_url: &str, config: ProviderConfig) -> Result { + Self::new_with_schema_and_config(database_url, None, config).await + } + + /// Same as [`Self::new_with_schema`] but accepts a [`ProviderConfig`]. + /// + /// Use this constructor to skip migration application in processes that + /// must not run DDL — see [`MigrationPolicy::VerifyOnly`]. + pub async fn new_with_schema_and_config( + database_url: &str, + schema_name: Option<&str>, + config: ProviderConfig, + ) -> Result { let max_connections = std::env::var("DUROXIDE_PG_POOL_MAX") .ok() .and_then(|s| s.parse::().ok()) @@ -145,9 +208,12 @@ impl PostgresProvider { _refresh_task: None, }; - // Run migrations to initialize schema + // Apply the configured migration policy. let migration_runner = MigrationRunner::new(provider.pool.clone(), schema_name.clone()); - migration_runner.migrate().await?; + match config.migration_policy { + MigrationPolicy::ApplyAll => migration_runner.migrate().await?, + MigrationPolicy::VerifyOnly => migration_runner.verify().await?, + } Ok(provider) } diff --git a/tests/migration_policy_tests.rs b/tests/migration_policy_tests.rs new file mode 100644 index 0000000..f09b837 --- /dev/null +++ b/tests/migration_policy_tests.rs @@ -0,0 +1,125 @@ +//! Tests for [`MigrationPolicy`] / [`ProviderConfig`] / `new_with_config`. +//! +//! These tests verify that: +//! 1. `MigrationPolicy::ApplyAll` (the default) creates schema + tables, matching +//! pre-feature behavior. +//! 2. `MigrationPolicy::VerifyOnly` succeeds against a schema where migrations +//! have already been applied. +//! 3. `MigrationPolicy::VerifyOnly` returns an error against an uninitialized +//! schema, without creating any objects. + +use duroxide_pg::{MigrationPolicy, PostgresProvider, ProviderConfig}; +use sqlx::postgres::PgPoolOptions; +use sqlx::Row; + +fn get_database_url() -> String { + dotenvy::dotenv().ok(); + std::env::var("DATABASE_URL").expect("DATABASE_URL must be set in environment or .env file") +} + +fn get_test_schema() -> String { + let guid = uuid::Uuid::new_v4().to_string(); + let suffix = &guid[guid.len() - 8..]; + format!("test_migpolicy_{suffix}") +} + +async fn drop_schema(schema_name: &str) { + let pool = PgPoolOptions::new() + .max_connections(1) + .connect(&get_database_url()) + .await + .expect("connect for cleanup"); + sqlx::query(&format!("DROP SCHEMA IF EXISTS {schema_name} CASCADE")) + .execute(&pool) + .await + .expect("drop schema"); +} + +async fn schema_exists(schema_name: &str) -> bool { + let pool = PgPoolOptions::new() + .max_connections(1) + .connect(&get_database_url()) + .await + .expect("connect for schema check"); + let row = sqlx::query( + "SELECT EXISTS(SELECT 1 FROM information_schema.schemata WHERE schema_name = $1) AS e", + ) + .bind(schema_name) + .fetch_one(&pool) + .await + .expect("query schema existence"); + row.get::("e") +} + +#[tokio::test] +async fn apply_all_creates_schema_and_tables() { + let database_url = get_database_url(); + let schema = get_test_schema(); + + // `ProviderConfig::default()` is equivalent to `MigrationPolicy::ApplyAll`, + // so this also smoke-tests the default-derived value. + let _provider = PostgresProvider::new_with_schema_and_config( + &database_url, + Some(&schema), + ProviderConfig::default(), + ) + .await + .expect("ApplyAll should succeed against a fresh schema"); + + assert!( + schema_exists(&schema).await, + "schema {schema} should exist after ApplyAll" + ); + + drop_schema(&schema).await; +} + +#[tokio::test] +async fn verify_only_succeeds_against_initialized_schema() { + let database_url = get_database_url(); + let schema = get_test_schema(); + + // First, apply migrations. + let _bootstrap = PostgresProvider::new_with_schema(&database_url, Some(&schema)) + .await + .expect("bootstrap apply"); + + // Then construct a VerifyOnly provider against the same schema. + let mut config = ProviderConfig::default(); + config.migration_policy = MigrationPolicy::VerifyOnly; + + let _verify = + PostgresProvider::new_with_schema_and_config(&database_url, Some(&schema), config) + .await + .expect("VerifyOnly should succeed when migrations are up to date"); + + drop_schema(&schema).await; +} + +#[tokio::test] +async fn verify_only_errors_against_uninitialized_schema() { + let database_url = get_database_url(); + let schema = get_test_schema(); + + let mut config = ProviderConfig::default(); + config.migration_policy = MigrationPolicy::VerifyOnly; + + let result = + PostgresProvider::new_with_schema_and_config(&database_url, Some(&schema), config).await; + + let err = match result { + Ok(_) => panic!("VerifyOnly must fail when the target schema has no migrations applied"), + Err(e) => e.to_string(), + }; + + assert!( + err.contains("not initialized") || err.contains("_duroxide_migrations"), + "error should mention missing migration table; got: {err}" + ); + + // VerifyOnly must not create the schema. + assert!( + !schema_exists(&schema).await, + "VerifyOnly should not create schema {schema}" + ); +} From 17898d08ac23de09d08573a6034aecd32966de33 Mon Sep 17 00:00:00 2001 From: Pino de Candia <32303022+pinodeca@users.noreply.github.com> Date: Wed, 20 May 2026 15:46:56 +0000 Subject: [PATCH 2/7] feat(migrations): reject unknown applied migrations Both MigrationPolicy::ApplyAll and MigrationPolicy::VerifyOnly now fail fast when the _duroxide_migrations tracking table records versions that are not bundled with the running binary (schema is ahead of the code). Under ApplyAll the check runs under the migration advisory lock and short-circuits before any DDL is executed, so an older binary cannot rewrite a schema that is ahead of its code. Under VerifyOnly the check runs between the tracking-table-exists check and the missing-versions check, producing a distinct ahead-of-code error instead of a misleading verification success. Tests added (ported from duroxide-pg-opt): - verify_only_rejects_unknown_migrations: VerifyOnly errors when an unknown version is present in the tracking table. - apply_all_unknown_migrations_causes_no_mutations: ApplyAll errors when an unknown version is present AND no DDL is executed (asserts the previously-deleted real migration is still absent and the previously-dropped core table is still missing). --- CHANGELOG.md | 8 ++ src/migrations.rs | 64 +++++++++++++++- tests/migration_policy_tests.rs | 132 ++++++++++++++++++++++++++++++++ 3 files changed, 203 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c66184f..63ac893 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- **Reject schemas ahead of the running binary.** Both `MigrationPolicy::ApplyAll` + and `MigrationPolicy::VerifyOnly` now fail fast when the `_duroxide_migrations` + tracking table records migration versions that are not bundled with the + running binary. Under `ApplyAll` the check runs under the migration advisory + lock and short-circuits before any DDL is executed, so an older binary + cannot rewrite a schema that is ahead of its code. The error message names + the unknown versions and instructs the operator to update the code. + - **Configurable migration policy at provider construction.** New `MigrationPolicy` enum and `ProviderConfig` struct, plus two new constructors `PostgresProvider::new_with_config` and diff --git a/src/migrations.rs b/src/migrations.rs index 5d07f04..a057e7c 100644 --- a/src/migrations.rs +++ b/src/migrations.rs @@ -71,7 +71,14 @@ impl MigrationRunner { let conn = &mut *conn; self.lock_for_migrations(conn).await?; - let result = self.migrate_inner(conn).await; + // Reject unknown migrations while holding the advisory lock so that an + // older binary cannot rewrite a schema that is ahead of its code. + // Short-circuit: do NOT run migrate_inner if unknown migrations are + // detected. + let result = match self.check_no_unknown_migrations(conn).await { + Ok(()) => self.migrate_inner(conn).await, + Err(e) => Err(e), + }; self.unlock_for_migrations(conn).await; result @@ -110,6 +117,10 @@ impl MigrationRunner { ); } + // Reject schemas that have migrations the running binary does not + // recognize (schema is ahead of the code). + self.check_no_unknown_migrations(conn).await?; + let migrations = self.load_migrations()?; let applied: std::collections::HashSet = self.get_applied_versions(conn).await?.into_iter().collect(); @@ -135,6 +146,57 @@ impl MigrationRunner { Ok(()) } + /// Check that the database has no migrations the running binary does not + /// recognize. Used by both `migrate()` (to refuse running DDL against a + /// schema ahead of the code) and `verify()` (to refuse claiming + /// successful verification of an unknown schema). + /// + /// Returns `Ok(())` if the tracking table does not yet exist: under + /// `ApplyAll` it will be created by `migrate_inner`, and under + /// `VerifyOnly` the missing table is reported separately before this is + /// called. + async fn check_no_unknown_migrations( + &self, + conn: &mut sqlx::postgres::PgConnection, + ) -> Result<()> { + let tracking_exists: bool = sqlx::query_scalar( + "SELECT EXISTS(SELECT 1 FROM information_schema.tables \ + WHERE table_schema = $1 AND table_name = '_duroxide_migrations')", + ) + .bind(&self.schema_name) + .fetch_one(&mut *conn) + .await?; + + if !tracking_exists { + return Ok(()); + } + + let applied = self.get_applied_versions(conn).await?; + let expected: std::collections::HashSet = self + .load_migrations()? + .into_iter() + .map(|m| m.version) + .collect(); + + let mut unknown: Vec = applied + .into_iter() + .filter(|v| !expected.contains(v)) + .collect(); + unknown.sort_unstable(); + + if !unknown.is_empty() { + anyhow::bail!( + "schema {:?} has migrations not recognized by this version of \ + the code: {:?}. The database schema is ahead of the code. \ + Update the code to a compatible version.", + self.schema_name, + unknown, + ); + } + + Ok(()) + } + async fn migrate_inner(&self, conn: &mut sqlx::postgres::PgConnection) -> Result<()> { // Ensure schema exists if self.schema_name != "public" { diff --git a/tests/migration_policy_tests.rs b/tests/migration_policy_tests.rs index f09b837..1c9507e 100644 --- a/tests/migration_policy_tests.rs +++ b/tests/migration_policy_tests.rs @@ -123,3 +123,135 @@ async fn verify_only_errors_against_uninitialized_schema() { "VerifyOnly should not create schema {schema}" ); } + +#[tokio::test] +async fn verify_only_rejects_unknown_migrations() { + let database_url = get_database_url(); + let schema = get_test_schema(); + + // Bring the schema up to date via ApplyAll. + let bootstrap = PostgresProvider::new_with_schema(&database_url, Some(&schema)) + .await + .expect("bootstrap apply"); + + // Insert an "unknown" migration version directly into the tracking table. + sqlx::query(&format!( + "INSERT INTO {schema}._duroxide_migrations (version, name) VALUES ($1, $2)" + )) + .bind(9_999_i64) + .bind("9999_future.sql") + .execute(bootstrap.pool()) + .await + .expect("insert unknown migration row"); + drop(bootstrap); + + // VerifyOnly must refuse to claim a schema that is ahead of the code. + let mut config = ProviderConfig::default(); + config.migration_policy = MigrationPolicy::VerifyOnly; + + let result = + PostgresProvider::new_with_schema_and_config(&database_url, Some(&schema), config).await; + let msg = match result { + Ok(_) => panic!("VerifyOnly must reject unknown applied migrations"), + Err(e) => format!("{e:#}"), + }; + assert!( + msg.contains("not recognized") && msg.contains("9999"), + "expected ahead-of-code error mentioning the unknown version; got: {msg}" + ); + + drop_schema(&schema).await; +} + +#[tokio::test] +async fn apply_all_unknown_migrations_causes_no_mutations() { + let database_url = get_database_url(); + let schema = get_test_schema(); + + // Fully initialize the schema. + let bootstrap = PostgresProvider::new_with_schema(&database_url, Some(&schema)) + .await + .expect("bootstrap apply"); + + // Snapshot the applied migrations before the perturbation. + let before: Vec<(i64, String)> = sqlx::query_as(&format!( + "SELECT version, name FROM {schema}._duroxide_migrations ORDER BY version" + )) + .fetch_all(bootstrap.pool()) + .await + .expect("read migrations"); + let last_version = before.last().expect("at least one bundled migration").0; + + // Create a "pending migration" condition by removing the last real + // migration record, drop a core table to disable the re-apply-if-missing + // path, then insert an unknown future migration. + sqlx::query(&format!( + "DELETE FROM {schema}._duroxide_migrations WHERE version = $1" + )) + .bind(last_version) + .execute(bootstrap.pool()) + .await + .expect("delete last migration"); + + sqlx::query(&format!("DROP TABLE {schema}.instances")) + .execute(bootstrap.pool()) + .await + .expect("drop instances table"); + + sqlx::query(&format!( + "INSERT INTO {schema}._duroxide_migrations (version, name) VALUES ($1, $2)" + )) + .bind(9_999_i64) + .bind("9999_future.sql") + .execute(bootstrap.pool()) + .await + .expect("insert unknown migration row"); + + drop(bootstrap); + + // ApplyAll must short-circuit on the unknown migration and run no DDL. + let result = PostgresProvider::new_with_schema(&database_url, Some(&schema)).await; + let msg = match result { + Ok(_) => panic!("ApplyAll must reject unknown applied migrations"), + Err(e) => format!("{e:#}"), + }; + assert!( + msg.contains("not recognized") && msg.contains("9999"), + "expected ahead-of-code error; got: {msg}" + ); + + // Prove no DDL fired: the deleted real migration is still absent and the + // dropped core table is still missing. + let pool = PgPoolOptions::new() + .max_connections(1) + .connect(&database_url) + .await + .expect("connect for verification"); + + let after_versions: Vec = sqlx::query_scalar(&format!( + "SELECT version FROM {schema}._duroxide_migrations" + )) + .fetch_all(&pool) + .await + .expect("read migrations after rejection"); + assert!( + !after_versions.contains(&last_version), + "migrate_inner should not have re-applied migration {last_version}; \ + current set: {after_versions:?}" + ); + + let instances_exists: bool = sqlx::query_scalar( + "SELECT EXISTS(SELECT 1 FROM information_schema.tables \ + WHERE table_schema = $1 AND table_name = 'instances')", + ) + .bind(&schema) + .fetch_one(&pool) + .await + .expect("check instances table after rejection"); + assert!( + !instances_exists, + "migrate_inner should not have recreated the instances table" + ); + + drop_schema(&schema).await; +} From 92d476c09e8db57185ca19878ac2fd8446c95b29 Mon Sep 17 00:00:00 2001 From: Pino de Candia <32303022+pinodeca@users.noreply.github.com> Date: Wed, 20 May 2026 16:36:03 +0000 Subject: [PATCH 3/7] refactor(provider)!: collapse constructors into new_with_config Consolidate the provider construction surface to a single config-driven entry point, ported from duroxide-pg-opt: PostgresProvider::new_with_config(ProviderConfig) ProviderConfig now carries the connection variant via a new ConnectionConfig enum (Url(String) or Entra { host, port, database, user, options }), plus the optional schema name and the migration policy. Construct via ProviderConfig::url(database_url) or ProviderConfig::entra(host, port, db, user, options) and adjust fields as needed. Breaking changes (unreleased API surface only): - Removed new_with_config(url, ProviderConfig) and new_with_schema_and_config(url, schema, ProviderConfig). These shipped only on this feature branch and were never released. Compatibility: - new(url) and new_with_schema(url, schema) remain as thin convenience wrappers around the new path. - new_with_entra and new_with_schema_and_entra are deprecated and delegate to new_with_config(ProviderConfig::entra(...)). They will be removed in a future release. - initialize_schema is deprecated; every constructor already runs the migration runner. Internal changes: - new_from_url is a private URL-path helper. - new_with_entra_with_token_source (the crate-internal test seam) now also takes MigrationPolicy, so VerifyOnly applies to the Entra path too. The three internal pipeline tests pass ApplyAll. Tests: - tests/migration_policy_tests.rs migrated to the new API. - tests/entra_live_test.rs migrated to the new API. Verified: cargo build --tests clean, cargo fmt --check clean, cargo clippy --tests produces no new warnings, all 38 lib tests, 5 migration_policy_tests, 2 regression_tests, 2 concurrent_migration tests, 1 cached_plan_retryable test, plus basic_tests all pass. --- CHANGELOG.md | 35 +++++- src/entra.rs | 5 +- src/lib.rs | 10 +- src/provider.rs | 195 +++++++++++++++++++++++++------- tests/entra_live_test.rs | 17 +-- tests/migration_policy_tests.rs | 38 +++---- 6 files changed, 214 insertions(+), 86 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 63ac893..7c0290c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,31 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- **BREAKING (unreleased API surface only):** Collapsed all `*_with_config` + and Entra-specific constructors into a single + `PostgresProvider::new_with_config(ProviderConfig)`. `ProviderConfig` now + carries the connection variant via a new `ConnectionConfig` enum + (`Url(String)` or `Entra { host, port, database, user, options }`), + the optional schema name, and the migration policy. Construct via + `ProviderConfig::url(database_url)` or + `ProviderConfig::entra(host, port, db, user, options)` and adjust fields + as needed. The previously unreleased `new_with_config(url, config)` and + `new_with_schema_and_config(url, schema, config)` constructors are + removed. `new(url)` and `new_with_schema(url, schema)` remain as + convenience wrappers. + +### Deprecated + +- `PostgresProvider::new_with_entra` and + `PostgresProvider::new_with_schema_and_entra` are deprecated in favor of + `new_with_config(ProviderConfig::entra(...))`. They continue to work and + delegate to the new path; they will be removed in a future release. +- `PostgresProvider::initialize_schema` is deprecated. Every constructor + already runs the migration runner; this back-compat shim will be removed + in a future release. + ### Added - **Reject schemas ahead of the running binary.** Both `MigrationPolicy::ApplyAll` @@ -18,11 +43,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 the unknown versions and instructs the operator to update the code. - **Configurable migration policy at provider construction.** New - `MigrationPolicy` enum and `ProviderConfig` struct, plus two new - constructors `PostgresProvider::new_with_config` and - `PostgresProvider::new_with_schema_and_config`. The default policy is - `MigrationPolicy::ApplyAll`, which preserves pre-feature behavior — all - existing constructors (`new`, `new_with_schema`, `new_with_entra`, + `MigrationPolicy` enum, `ProviderConfig` struct, and `ConnectionConfig` + enum, plus the single new constructor `PostgresProvider::new_with_config`. + The default policy is `MigrationPolicy::ApplyAll`, which preserves + pre-feature behavior — all existing constructors (`new`, + `new_with_schema`, and the deprecated `new_with_entra` / `new_with_schema_and_entra`) continue to apply pending migrations on startup. The new `MigrationPolicy::VerifyOnly` policy skips migration application and instead verifies that the `_duroxide_migrations` tracking diff --git a/src/entra.rs b/src/entra.rs index ad3fac9..fc680ff 100644 --- a/src/entra.rs +++ b/src/entra.rs @@ -2,9 +2,8 @@ //! for [`PostgresProvider`](crate::PostgresProvider). //! //! This module exposes [`EntraAuthOptions`] — the configuration type passed to -//! `PostgresProvider::new_with_entra` and `PostgresProvider::new_with_schema_and_entra` -//! (added in Phase 2) — plus the internal credential abstractions used to -//! fetch and rotate Entra access tokens. +//! [`ProviderConfig::entra`](crate::ProviderConfig::entra) — plus the internal +//! credential abstractions used to fetch and rotate Entra access tokens. //! //! Azure SDK types (`azure_core::credentials::TokenCredential`, //! `azure_identity::ManagedIdentityCredential`, etc.) are intentionally **not diff --git a/src/lib.rs b/src/lib.rs index 09b0493..42a0642 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -43,17 +43,17 @@ //! [`EntraAuthOptions`] for tunables. //! //! ```rust,no_run -//! use duroxide_pg::{EntraAuthOptions, PostgresProvider}; +//! use duroxide_pg::{EntraAuthOptions, PostgresProvider, ProviderConfig}; //! //! # async fn example() -> anyhow::Result<()> { -//! let provider = PostgresProvider::new_with_entra( +//! let config = ProviderConfig::entra( //! "myserver.postgres.database.azure.com", //! 5432, //! "mydb", //! "my-entra-principal@contoso.onmicrosoft.com", //! EntraAuthOptions::new(), -//! ) -//! .await?; +//! ); +//! let provider = PostgresProvider::new_with_config(config).await?; //! # Ok(()) //! # } //! ``` @@ -82,4 +82,4 @@ pub mod migrations; pub mod provider; pub use entra::EntraAuthOptions; -pub use provider::{MigrationPolicy, PostgresProvider, ProviderConfig}; +pub use provider::{ConnectionConfig, MigrationPolicy, PostgresProvider, ProviderConfig}; diff --git a/src/provider.rs b/src/provider.rs index 80ef00c..739bc2f 100644 --- a/src/provider.rs +++ b/src/provider.rs @@ -54,17 +54,17 @@ use crate::migrations::MigrationRunner; /// ## Azure Database for PostgreSQL with Microsoft Entra ID /// /// ```rust,no_run -/// use duroxide_pg::{EntraAuthOptions, PostgresProvider}; +/// use duroxide_pg::{EntraAuthOptions, PostgresProvider, ProviderConfig}; /// /// # async fn example() -> anyhow::Result<()> { -/// let provider = PostgresProvider::new_with_entra( +/// let config = ProviderConfig::entra( /// "myserver.postgres.database.azure.com", /// 5432, /// "mydb", /// "my-entra-principal@contoso.onmicrosoft.com", /// EntraAuthOptions::new(), -/// ) -/// .await?; +/// ); +/// let provider = PostgresProvider::new_with_config(config).await?; /// # Ok(()) /// # } /// ``` @@ -130,28 +130,90 @@ pub enum MigrationPolicy { VerifyOnly, } -/// Optional configuration for [`PostgresProvider`] constructors. +/// Configuration for [`PostgresProvider::new_with_config`]. /// -/// Construct via [`ProviderConfig::default`] and adjust fields as needed: +/// Construct via [`ProviderConfig::url`] (standard `postgres://` URL) or +/// [`ProviderConfig::entra`] (Azure Database for PostgreSQL with Microsoft +/// Entra ID), then adjust fields as needed: /// /// ```rust,no_run /// use duroxide_pg::{MigrationPolicy, PostgresProvider, ProviderConfig}; /// /// # async fn example() -> anyhow::Result<()> { -/// let mut config = ProviderConfig::default(); +/// let mut config = ProviderConfig::url("postgres://localhost/mydb"); +/// config.schema_name = Some("my_app".into()); /// config.migration_policy = MigrationPolicy::VerifyOnly; -/// let provider = -/// PostgresProvider::new_with_config("postgres://localhost/mydb", config).await?; +/// let provider = PostgresProvider::new_with_config(config).await?; /// # Ok(()) /// # } /// ``` -#[derive(Debug, Clone, Default)] +#[derive(Debug, Clone)] #[non_exhaustive] pub struct ProviderConfig { + /// How the provider should reach the database. + pub connection: ConnectionConfig, + /// PostgreSQL schema for tenant isolation. `None` resolves to `public`. + pub schema_name: Option, /// Policy for handling embedded migrations at startup. pub migration_policy: MigrationPolicy, } +impl ProviderConfig { + /// Build a config from a standard PostgreSQL connection URL + /// (`postgres://user:pass@host:port/db`). Schema defaults to `public` + /// and migration policy defaults to [`MigrationPolicy::ApplyAll`]. + pub fn url(database_url: impl Into) -> Self { + Self { + connection: ConnectionConfig::Url(database_url.into()), + schema_name: None, + migration_policy: MigrationPolicy::default(), + } + } + + /// Build a config for Azure Database for PostgreSQL with Microsoft + /// Entra ID. Schema defaults to `public` and migration policy defaults + /// to [`MigrationPolicy::ApplyAll`]. All Entra connections use + /// `PgSslMode::VerifyFull`. + pub fn entra( + host: impl Into, + port: u16, + database: impl Into, + user: impl Into, + options: EntraAuthOptions, + ) -> Self { + Self { + connection: ConnectionConfig::Entra { + host: host.into(), + port, + database: database.into(), + user: user.into(), + options, + }, + schema_name: None, + migration_policy: MigrationPolicy::default(), + } + } +} + +/// How [`PostgresProvider`] should reach the database. +/// +/// Constructed indirectly via [`ProviderConfig::url`] or +/// [`ProviderConfig::entra`]. +#[derive(Debug, Clone)] +#[non_exhaustive] +pub enum ConnectionConfig { + /// Standard PostgreSQL connection URL. + Url(String), + /// Azure Database for PostgreSQL with Microsoft Entra ID. + Entra { + host: String, + port: u16, + database: String, + user: String, + options: EntraAuthOptions, + }, +} + /// Newtype around `tokio::task::AbortHandle` that aborts the task on drop. /// Used to ensure the Entra token refresh task is cleaned up when the /// provider is dropped. @@ -164,28 +226,70 @@ impl Drop for AbortOnDropHandle { } impl PostgresProvider { + /// Create a provider from a PostgreSQL connection URL, using the + /// `public` schema and applying any pending migrations. + /// + /// Convenience wrapper around [`Self::new_with_config`]. pub async fn new(database_url: &str) -> Result { - Self::new_with_schema(database_url, None).await + Self::new_with_config(ProviderConfig::url(database_url)).await } + /// Create a provider from a PostgreSQL connection URL, using a custom + /// schema for tenant isolation, and applying any pending migrations. + /// + /// Convenience wrapper around [`Self::new_with_config`]. pub async fn new_with_schema(database_url: &str, schema_name: Option<&str>) -> Result { - Self::new_with_schema_and_config(database_url, schema_name, ProviderConfig::default()).await + let mut config = ProviderConfig::url(database_url); + config.schema_name = schema_name.map(str::to_string); + Self::new_with_config(config).await } - /// Same as [`Self::new`] but accepts a [`ProviderConfig`] to control - /// startup behavior (currently: migration application policy). - pub async fn new_with_config(database_url: &str, config: ProviderConfig) -> Result { - Self::new_with_schema_and_config(database_url, None, config).await + /// Create a provider from a [`ProviderConfig`]. This is the single + /// constructor that fully exposes the configuration surface: + /// connection variant (URL or Entra), schema, and migration policy. + /// + /// All other public constructors delegate to this one. + pub async fn new_with_config(config: ProviderConfig) -> Result { + let ProviderConfig { + connection, + schema_name, + migration_policy, + } = config; + + match connection { + ConnectionConfig::Url(database_url) => { + Self::new_from_url(&database_url, schema_name.as_deref(), migration_policy).await + } + ConnectionConfig::Entra { + host, + port, + database, + user, + options, + } => { + let token_source = options.default_token_source().context( + "Entra credential resolution failed: could not build the default credential chain", + )?; + Self::new_with_entra_with_token_source( + &host, + port, + &database, + &user, + schema_name.as_deref(), + options, + token_source, + PgSslMode::VerifyFull, + migration_policy, + ) + .await + } + } } - /// Same as [`Self::new_with_schema`] but accepts a [`ProviderConfig`]. - /// - /// Use this constructor to skip migration application in processes that - /// must not run DDL — see [`MigrationPolicy::VerifyOnly`]. - pub async fn new_with_schema_and_config( + async fn new_from_url( database_url: &str, schema_name: Option<&str>, - config: ProviderConfig, + migration_policy: MigrationPolicy, ) -> Result { let max_connections = std::env::var("DUROXIDE_PG_POOL_MAX") .ok() @@ -208,9 +312,8 @@ impl PostgresProvider { _refresh_task: None, }; - // Apply the configured migration policy. - let migration_runner = MigrationRunner::new(provider.pool.clone(), schema_name.clone()); - match config.migration_policy { + let migration_runner = MigrationRunner::new(provider.pool.clone(), schema_name); + match migration_policy { MigrationPolicy::ApplyAll => migration_runner.migrate().await?, MigrationPolicy::VerifyOnly => migration_runner.verify().await?, } @@ -244,6 +347,10 @@ impl PostgresProvider { /// # Errors /// Returns an error if credential resolution fails, the initial token /// cannot be acquired, the database connection fails, or migrations fail. + #[deprecated( + since = "0.2.0", + note = "use `PostgresProvider::new_with_config(ProviderConfig::entra(...))` instead" + )] pub async fn new_with_entra( host: &str, port: u16, @@ -251,11 +358,15 @@ impl PostgresProvider { user: &str, options: EntraAuthOptions, ) -> Result { - Self::new_with_schema_and_entra(host, port, database, user, None, options).await + Self::new_with_config(ProviderConfig::entra(host, port, database, user, options)).await } /// Same as [`Self::new_with_entra`] but uses a custom schema for tenant /// isolation. + #[deprecated( + since = "0.2.0", + note = "use `PostgresProvider::new_with_config(ProviderConfig::entra(...))` with `schema_name` set instead" + )] #[instrument( skip(options), fields(host = %host, port = %port, database = %database, user = %user, schema = ?schema_name), @@ -269,21 +380,9 @@ impl PostgresProvider { schema_name: Option<&str>, options: EntraAuthOptions, ) -> Result { - let token_source = options.default_token_source().context( - "Entra credential resolution failed: could not build the default credential chain", - )?; - - Self::new_with_entra_with_token_source( - host, - port, - database, - user, - schema_name, - options, - token_source, - PgSslMode::VerifyFull, - ) - .await + let mut config = ProviderConfig::entra(host, port, database, user, options); + config.schema_name = schema_name.map(str::to_string); + Self::new_with_config(config).await } /// Crate-internal Entra constructor. Accepts an explicit @@ -295,6 +394,7 @@ impl PostgresProvider { /// connect-options → pool → migrations → refresh task) against a local /// PostgreSQL without an Azure dependency, by injecting a fake /// [`TokenSource`] that returns the local password and disabling TLS. + #[allow(clippy::too_many_arguments)] pub(crate) async fn new_with_entra_with_token_source( host: &str, port: u16, @@ -304,6 +404,7 @@ impl PostgresProvider { options: EntraAuthOptions, token_source: Arc, ssl_mode: PgSslMode, + migration_policy: MigrationPolicy, ) -> Result { let audience = options.audience_str().to_string(); let token = token_source @@ -326,7 +427,10 @@ impl PostgresProvider { let schema_name = schema_name.unwrap_or("public").to_string(); let migration_runner = MigrationRunner::new(pool.clone(), schema_name.clone()); - migration_runner.migrate().await?; + match migration_policy { + MigrationPolicy::ApplyAll => migration_runner.migrate().await?, + MigrationPolicy::VerifyOnly => migration_runner.verify().await?, + } let refresh_handle = spawn_token_refresh_task( pool.clone(), @@ -345,6 +449,10 @@ impl PostgresProvider { }) } + #[deprecated( + since = "0.2.0", + note = "schema initialization is now run automatically by every constructor; this shim will be removed in a future release" + )] #[instrument(skip(self), target = "duroxide::providers::postgres")] pub async fn initialize_schema(&self) -> Result<()> { // Schema initialization is now handled by migrations @@ -3043,6 +3151,7 @@ mod entra_pipeline_tests { EntraAuthOptions::new(), token_source, PgSslMode::Disable, + MigrationPolicy::ApplyAll, ) .await .expect("Entra pipeline must succeed against local PG with correct token"); @@ -3084,6 +3193,7 @@ mod entra_pipeline_tests { EntraAuthOptions::new(), token_source, PgSslMode::Disable, + MigrationPolicy::ApplyAll, ) .await; @@ -3132,6 +3242,7 @@ mod entra_pipeline_tests { EntraAuthOptions::new().refresh_interval(Duration::from_secs(60 * 60)), token_source, PgSslMode::Disable, + MigrationPolicy::ApplyAll, ) .await .expect("default-constructor variant must succeed"); diff --git a/tests/entra_live_test.rs b/tests/entra_live_test.rs index 15ad814..afd34a2 100644 --- a/tests/entra_live_test.rs +++ b/tests/entra_live_test.rs @@ -32,7 +32,7 @@ //! cargo test --test entra_live_test -- --ignored --nocapture //! ``` -use duroxide_pg::{EntraAuthOptions, PostgresProvider}; +use duroxide_pg::{EntraAuthOptions, PostgresProvider, ProviderConfig}; use sqlx::Row; const ENABLE_VAR: &str = "DUROXIDE_PG_ENTRA_LIVE_TEST"; @@ -77,16 +77,11 @@ async fn entra_live_smoke_test() { "Live Entra smoke test: host={host} port={port} db={database} user={user} schema={schema}" ); - let provider = PostgresProvider::new_with_schema_and_entra( - &host, - port, - &database, - &user, - Some(&schema), - EntraAuthOptions::new(), - ) - .await - .expect("provider construction with default Entra credential chain must succeed"); + let mut config = ProviderConfig::entra(&host, port, &database, &user, EntraAuthOptions::new()); + config.schema_name = Some(schema.clone()); + let provider = PostgresProvider::new_with_config(config) + .await + .expect("provider construction with default Entra credential chain must succeed"); // Use a basic query to prove the pool is functional and the schema/migrations // were applied. We look for one of the well-known tables migrations create. diff --git a/tests/migration_policy_tests.rs b/tests/migration_policy_tests.rs index 1c9507e..b0b9b38 100644 --- a/tests/migration_policy_tests.rs +++ b/tests/migration_policy_tests.rs @@ -56,15 +56,13 @@ async fn apply_all_creates_schema_and_tables() { let database_url = get_database_url(); let schema = get_test_schema(); - // `ProviderConfig::default()` is equivalent to `MigrationPolicy::ApplyAll`, - // so this also smoke-tests the default-derived value. - let _provider = PostgresProvider::new_with_schema_and_config( - &database_url, - Some(&schema), - ProviderConfig::default(), - ) - .await - .expect("ApplyAll should succeed against a fresh schema"); + // `ProviderConfig::url` defaults to `MigrationPolicy::ApplyAll`, + // so this also smoke-tests the default policy via the new API. + let mut config = ProviderConfig::url(&database_url); + config.schema_name = Some(schema.clone()); + let _provider = PostgresProvider::new_with_config(config) + .await + .expect("ApplyAll should succeed against a fresh schema"); assert!( schema_exists(&schema).await, @@ -85,13 +83,13 @@ async fn verify_only_succeeds_against_initialized_schema() { .expect("bootstrap apply"); // Then construct a VerifyOnly provider against the same schema. - let mut config = ProviderConfig::default(); + let mut config = ProviderConfig::url(&database_url); + config.schema_name = Some(schema.clone()); config.migration_policy = MigrationPolicy::VerifyOnly; - let _verify = - PostgresProvider::new_with_schema_and_config(&database_url, Some(&schema), config) - .await - .expect("VerifyOnly should succeed when migrations are up to date"); + let _verify = PostgresProvider::new_with_config(config) + .await + .expect("VerifyOnly should succeed when migrations are up to date"); drop_schema(&schema).await; } @@ -101,11 +99,11 @@ async fn verify_only_errors_against_uninitialized_schema() { let database_url = get_database_url(); let schema = get_test_schema(); - let mut config = ProviderConfig::default(); + let mut config = ProviderConfig::url(&database_url); + config.schema_name = Some(schema.clone()); config.migration_policy = MigrationPolicy::VerifyOnly; - let result = - PostgresProvider::new_with_schema_and_config(&database_url, Some(&schema), config).await; + let result = PostgresProvider::new_with_config(config).await; let err = match result { Ok(_) => panic!("VerifyOnly must fail when the target schema has no migrations applied"), @@ -146,11 +144,11 @@ async fn verify_only_rejects_unknown_migrations() { drop(bootstrap); // VerifyOnly must refuse to claim a schema that is ahead of the code. - let mut config = ProviderConfig::default(); + let mut config = ProviderConfig::url(&database_url); + config.schema_name = Some(schema.clone()); config.migration_policy = MigrationPolicy::VerifyOnly; - let result = - PostgresProvider::new_with_schema_and_config(&database_url, Some(&schema), config).await; + let result = PostgresProvider::new_with_config(config).await; let msg = match result { Ok(_) => panic!("VerifyOnly must reject unknown applied migrations"), Err(e) => format!("{e:#}"), From 859b0628454ab29c1b3498455288862d46ab5163 Mon Sep 17 00:00:00 2001 From: Pino de Candia <32303022+pinodeca@users.noreply.github.com> Date: Wed, 20 May 2026 16:52:22 +0000 Subject: [PATCH 4/7] feat(provider)!: validate schema names Add a conservative schema-name validator and call it from PostgresProvider::new_with_config (the single entry point all other constructors now delegate to). Schema names that do not match [A-Za-z_][A-Za-z0-9_]* are rejected up front with a clear error. Why a validator is needed: PostgreSQL identifiers cannot be bound as SQL parameters, so the schema name is interpolated directly into the DDL and DML the provider issues (e.g. CREATE SCHEMA {schema}, INSERT INTO {schema}._duroxide_migrations ...). Without validation, a caller that forwards attacker-controlled input as schema_name would create a SQL injection vector. PostgreSQL's full identifier grammar (including quoted identifiers) is broader; this validation is intentionally conservative. Breaking changes (unreleased API surface only): - new_with_config, new_with_schema, and the deprecated Entra constructors now return Err for schema names outside the accepted subset. Already-shipped releases (<= 0.1.33) are unaffected. - Callers passing only constants from their own code are unaffected. Tests: - schema_name_validation_rejects_unsafe_identifiers exercises the rejection path via new_with_config("bad-name"). Verified: cargo build --tests clean, cargo fmt --check clean, cargo clippy --tests clean on touched files, all 6 migration_policy_tests pass. --- CHANGELOG.md | 20 +++++++++++++++++++ src/provider.rs | 34 +++++++++++++++++++++++++++++++++ tests/migration_policy_tests.rs | 19 ++++++++++++++++++ 3 files changed, 73 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7c0290c..1f87a42 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,8 +7,28 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Security + +- **Schema name validation at provider construction.** All constructors now + reject schema names that do not match `^[A-Za-z_][A-Za-z0-9_]*$`. + PostgreSQL identifiers cannot be bound as SQL parameters, so the schema + name is interpolated directly into the DDL and DML the provider issues. + Restricting the accepted character set up front eliminates the SQL + injection vector that would otherwise exist for callers that pass + attacker-controlled schema names. PostgreSQL's full identifier grammar + (including quoted identifiers) is broader; this validation is + intentionally conservative. + ### Changed +- **BREAKING (unreleased API surface only):** `PostgresProvider::new_with_config`, + `new_with_schema`, and the deprecated Entra constructors now return an + error when `schema_name` contains characters outside + `[A-Za-z_][A-Za-z0-9_]*`. Previously such names were silently + interpolated into SQL. Callers passing only constants from their own code + (the common case) are unaffected. Already-shipped releases (`<= 0.1.33`) + are unaffected. + - **BREAKING (unreleased API surface only):** Collapsed all `*_with_config` and Entra-specific constructors into a single `PostgresProvider::new_with_config(ProviderConfig)`. `ProviderConfig` now diff --git a/src/provider.rs b/src/provider.rs index 739bc2f..4e04942 100644 --- a/src/provider.rs +++ b/src/provider.rs @@ -214,6 +214,36 @@ pub enum ConnectionConfig { }, } +/// Validate that `schema_name` is a safe PostgreSQL identifier. +/// +/// PostgreSQL identifiers cannot be bound as SQL parameters, so the schema +/// name is interpolated directly into the SQL we issue (e.g. +/// `CREATE SCHEMA {schema}`, `SELECT … FROM {schema}.instances`). To prevent +/// SQL injection, we restrict schema names to a conservative subset: +/// +/// `^[A-Za-z_][A-Za-z0-9_]*$` +/// +/// PostgreSQL's full identifier grammar is broader (quoted identifiers can +/// contain almost anything), but accepting the broader grammar would require +/// quoting every interpolation site and validating that the input does not +/// contain a closing quote — strictly less safe than refusing surprising +/// names up front. +fn validate_schema_name(schema_name: &str) -> Result<()> { + let mut chars = schema_name.chars(); + let Some(first) = chars.next() else { + anyhow::bail!("Invalid schema_name '': must match [A-Za-z_][A-Za-z0-9_]*"); + }; + if !(first == '_' || first.is_ascii_alphabetic()) { + anyhow::bail!("Invalid schema_name '{schema_name}': must match [A-Za-z_][A-Za-z0-9_]*"); + } + for ch in chars { + if !(ch == '_' || ch.is_ascii_alphanumeric()) { + anyhow::bail!("Invalid schema_name '{schema_name}': must match [A-Za-z_][A-Za-z0-9_]*"); + } + } + Ok(()) +} + /// Newtype around `tokio::task::AbortHandle` that aborts the task on drop. /// Used to ensure the Entra token refresh task is cleaned up when the /// provider is dropped. @@ -256,6 +286,10 @@ impl PostgresProvider { migration_policy, } = config; + if let Some(ref s) = schema_name { + validate_schema_name(s)?; + } + match connection { ConnectionConfig::Url(database_url) => { Self::new_from_url(&database_url, schema_name.as_deref(), migration_policy).await diff --git a/tests/migration_policy_tests.rs b/tests/migration_policy_tests.rs index b0b9b38..40c66cb 100644 --- a/tests/migration_policy_tests.rs +++ b/tests/migration_policy_tests.rs @@ -253,3 +253,22 @@ async fn apply_all_unknown_migrations_causes_no_mutations() { drop_schema(&schema).await; } + +#[tokio::test] +async fn schema_name_validation_rejects_unsafe_identifiers() { + let database_url = get_database_url(); + + let mut config = ProviderConfig::url(&database_url); + config.schema_name = Some("bad-name".to_string()); + + match PostgresProvider::new_with_config(config).await { + Ok(_) => panic!("Expected schema name validation to fail"), + Err(e) => { + let msg = format!("{e:#}"); + assert!( + msg.contains("Invalid schema_name"), + "expected validation error, got: {msg}" + ); + } + } +} From d5d16192a0d09dfbc4c2c48c195f6dca33d0a9d2 Mon Sep 17 00:00:00 2001 From: Pino de Candia <32303022+pinodeca@users.noreply.github.com> Date: Wed, 20 May 2026 17:04:27 +0000 Subject: [PATCH 5/7] test(provider): port remaining opt initialization tests Add four integration tests against the new ProviderConfig / new_with_config API, covering provider initialization paths that were previously implicit: - verify_only_errors_when_schema_missing: VerifyOnly against a database where the target schema does not exist at all. - verify_only_errors_when_tracking_table_missing: VerifyOnly against a bare schema (CREATE SCHEMA was run, but no migrations applied) must reject with a clear "not initialized" error. - verify_only_errors_when_migrations_behind: VerifyOnly against a schema whose tracking table is missing the most recent bundled migration must name the missing version. - concurrent_apply_all_is_serialized: two concurrent ApplyAll initializations against the same fresh schema must both succeed, exercising the migration advisory lock end-to-end. These are the equivalents of the tests added in the optimistic sibling crate, adapted to this provider's actual API (ProviderConfig::url + new_with_config) and error messages. Pure additive: no library code changes. Verified: cargo fmt --check clean, all 10 migration_policy_tests pass (6 pre-existing + 4 new). Pre-existing clippy warnings in other test targets and src/provider.rs are unrelated. --- CHANGELOG.md | 7 ++ tests/migration_policy_tests.rs | 155 ++++++++++++++++++++++++++++++++ 2 files changed, 162 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1f87a42..5bd211a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -78,6 +78,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 changes. `VerifyOnly` does not take the migration advisory lock and does not create or modify any database objects. +- **Initialization regression tests.** Added integration tests for the + provider initialization paths: `VerifyOnly` against a missing schema, a + bare schema with no tracking table, and a schema whose tracking table is + behind the bundled migrations; and a concurrency test that exercises the + migration advisory lock by running two `ApplyAll` initializations against + the same fresh schema in parallel. + ## [0.1.33] - 2026-05-13 ### Fixed diff --git a/tests/migration_policy_tests.rs b/tests/migration_policy_tests.rs index 40c66cb..8cc7107 100644 --- a/tests/migration_policy_tests.rs +++ b/tests/migration_policy_tests.rs @@ -272,3 +272,158 @@ async fn schema_name_validation_rejects_unsafe_identifiers() { } } } + +#[tokio::test] +async fn verify_only_errors_when_schema_missing() { + let database_url = get_database_url(); + let schema = get_test_schema(); + + // Belt-and-braces: make sure the schema is absent. + drop_schema(&schema).await; + + let mut config = ProviderConfig::url(&database_url); + config.schema_name = Some(schema.clone()); + config.migration_policy = MigrationPolicy::VerifyOnly; + + let result = PostgresProvider::new_with_config(config).await; + let msg = match result { + Ok(_) => panic!("VerifyOnly should fail when the target schema does not exist"), + Err(e) => format!("{e:#}"), + }; + assert!( + msg.contains("not initialized") || msg.contains("_duroxide_migrations"), + "expected missing-schema/missing-tracking-table error, got: {msg}" + ); + + // VerifyOnly must not create the schema as a side effect. + assert!( + !schema_exists(&schema).await, + "VerifyOnly must not create schema {schema}" + ); +} + +#[tokio::test] +async fn verify_only_errors_when_tracking_table_missing() { + let database_url = get_database_url(); + let schema = get_test_schema(); + + // Create a bare schema with no tables (no migrations applied). + let pool = PgPoolOptions::new() + .max_connections(1) + .connect(&database_url) + .await + .expect("connect for setup"); + sqlx::query(&format!("CREATE SCHEMA {schema}")) + .execute(&pool) + .await + .expect("create bare schema"); + + let mut config = ProviderConfig::url(&database_url); + config.schema_name = Some(schema.clone()); + config.migration_policy = MigrationPolicy::VerifyOnly; + + let result = PostgresProvider::new_with_config(config).await; + let msg = match result { + Ok(_) => panic!("VerifyOnly should fail when the tracking table is missing"), + Err(e) => format!("{e:#}"), + }; + assert!( + msg.contains("not initialized") && msg.contains("_duroxide_migrations"), + "expected tracking-table-missing error, got: {msg}" + ); + + drop_schema(&schema).await; +} + +#[tokio::test] +async fn verify_only_errors_when_migrations_behind() { + let database_url = get_database_url(); + let schema = get_test_schema(); + + // Fully initialize, then synthesize a "behind" state by deleting the + // most recent migration record without dropping the tracking table. + let bootstrap = PostgresProvider::new_with_schema(&database_url, Some(&schema)) + .await + .expect("bootstrap apply"); + + let last_version: i64 = sqlx::query_scalar(&format!( + "SELECT MAX(version) FROM {schema}._duroxide_migrations" + )) + .fetch_one(bootstrap.pool()) + .await + .expect("read max version"); + + sqlx::query(&format!( + "DELETE FROM {schema}._duroxide_migrations WHERE version = $1" + )) + .bind(last_version) + .execute(bootstrap.pool()) + .await + .expect("delete most-recent migration row"); + + drop(bootstrap); + + let mut config = ProviderConfig::url(&database_url); + config.schema_name = Some(schema.clone()); + config.migration_policy = MigrationPolicy::VerifyOnly; + + let result = PostgresProvider::new_with_config(config).await; + let msg = match result { + Ok(_) => panic!("VerifyOnly should fail when migrations are behind"), + Err(e) => format!("{e:#}"), + }; + assert!( + msg.contains("not up to date") && msg.contains(&last_version.to_string()), + "expected behind-schema error mentioning version {last_version}; got: {msg}" + ); + + drop_schema(&schema).await; +} + +#[tokio::test] +async fn concurrent_apply_all_is_serialized() { + let database_url = get_database_url(); + let schema = get_test_schema(); + + // Ensure a clean slate. + drop_schema(&schema).await; + + let make_config = |schema: String| { + let mut cfg = ProviderConfig::url(&database_url); + cfg.schema_name = Some(schema); + cfg.migration_policy = MigrationPolicy::ApplyAll; + cfg + }; + + let h1 = { + let cfg = make_config(schema.clone()); + tokio::spawn(async move { PostgresProvider::new_with_config(cfg).await }) + }; + let h2 = { + let cfg = make_config(schema.clone()); + tokio::spawn(async move { PostgresProvider::new_with_config(cfg).await }) + }; + + let (r1, r2) = tokio::join!(h1, h2); + let p1 = r1 + .expect("task 1 panicked") + .expect("concurrent ApplyAll #1 should succeed under the advisory lock"); + let _p2 = r2 + .expect("task 2 panicked") + .expect("concurrent ApplyAll #2 should succeed under the advisory lock"); + + // Sanity check: the schema is fully migrated. + let row_count: (i64,) = sqlx::query_as(&format!( + "SELECT COUNT(*) FROM {schema}._duroxide_migrations" + )) + .fetch_one(p1.pool()) + .await + .expect("count applied migrations"); + assert!( + row_count.0 > 0, + "expected at least one applied migration after concurrent ApplyAll" + ); + + drop(p1); + drop_schema(&schema).await; +} From 4f635e688d7fdcdab08edc745fe62ff840f968a9 Mon Sep 17 00:00:00 2001 From: Pino de Candia Date: Fri, 22 May 2026 19:24:31 +0000 Subject: [PATCH 6/7] Align deprecation since to 0.1.34 (drop 0.2.0 semver bump) The 0.1.x series is not enforcing strict SemVer; this release will ship as 0.1.34 instead of 0.2.0. Update deprecation attributes on new_with_entra, new_with_schema_and_entra, and initialize_schema to match. Also trims a now-redundant comment on initialize_schema. --- src/provider.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/provider.rs b/src/provider.rs index 4e04942..2f83970 100644 --- a/src/provider.rs +++ b/src/provider.rs @@ -382,7 +382,7 @@ impl PostgresProvider { /// Returns an error if credential resolution fails, the initial token /// cannot be acquired, the database connection fails, or migrations fail. #[deprecated( - since = "0.2.0", + since = "0.1.34", note = "use `PostgresProvider::new_with_config(ProviderConfig::entra(...))` instead" )] pub async fn new_with_entra( @@ -398,7 +398,7 @@ impl PostgresProvider { /// Same as [`Self::new_with_entra`] but uses a custom schema for tenant /// isolation. #[deprecated( - since = "0.2.0", + since = "0.1.34", note = "use `PostgresProvider::new_with_config(ProviderConfig::entra(...))` with `schema_name` set instead" )] #[instrument( @@ -484,13 +484,11 @@ impl PostgresProvider { } #[deprecated( - since = "0.2.0", + since = "0.1.34", note = "schema initialization is now run automatically by every constructor; this shim will be removed in a future release" )] #[instrument(skip(self), target = "duroxide::providers::postgres")] pub async fn initialize_schema(&self) -> Result<()> { - // Schema initialization is now handled by migrations - // This method is kept for backward compatibility but delegates to migrations let migration_runner = MigrationRunner::new(self.pool.clone(), self.schema_name.clone()); migration_runner.migrate().await?; Ok(()) From ed395131d0979571169b88c91d0f45eb512197e5 Mon Sep 17 00:00:00 2001 From: pinodeca Date: Sat, 23 May 2026 11:07:53 -0500 Subject: [PATCH 7/7] test(provider): verify core tables in VerifyOnly --- docs/migration-policy.md | 62 +++++++++++++++++++++++++++++++++ src/migrations.rs | 11 ++++++ tests/migration_policy_tests.rs | 35 +++++++++++++++++++ 3 files changed, 108 insertions(+) create mode 100644 docs/migration-policy.md diff --git a/docs/migration-policy.md b/docs/migration-policy.md new file mode 100644 index 0000000..e5716d0 --- /dev/null +++ b/docs/migration-policy.md @@ -0,0 +1,62 @@ +# Migration Policy + +`duroxide-pg` supports separating schema migration from normal runtime database +operations. This matters in production because DDL usually belongs to a +controlled deploy or upgrade step, while application backends and workers often +run with lower-privilege DML-only roles. + +## Policies + +`MigrationPolicy::ApplyAll` is the default. It preserves the original provider +behavior: provider construction creates the target schema when needed, creates +the `_duroxide_migrations` tracking table, rejects schemas that contain unknown +future migration versions, and applies any bundled migrations that have not yet +been recorded. + +`MigrationPolicy::VerifyOnly` executes no DDL. It verifies that the migration +tracking table exists, that the schema has no unknown future migration versions, +that every bundled migration has already been applied, and that core provider +tables still exist. This is intended for client/backend/worker processes where a +separate privileged process has already run migrations. + +## Behavior Matrix + +| Scenario | `ApplyAll` | `VerifyOnly` | +|---|---|---| +| Schema does not exist | Creates it | Error | +| Schema exists but tracking table is missing | Creates tracking table and runs migrations | Error | +| Schema is behind bundled migrations | Applies pending migrations | Error | +| Schema has unknown future migrations | Error before DDL | Error | +| Migrations are recorded but core tables are missing | Re-applies migrations | Error | +| Schema is fully migrated | No-op | No-op | + +`ApplyAll` takes a PostgreSQL session advisory lock scoped to the target schema +before checking or applying migrations. That serializes concurrent provider +startup so multiple nodes do not race to apply the same migration. + +## Error Messages + +`VerifyOnly` fails with messages in these shapes: + +- Missing tracking table: `duroxide migrations not initialized: schema "..." does not contain _duroxide_migrations...` +- Missing bundled migration versions: `duroxide migrations not up to date in schema "...": missing versions [...]...` +- Unknown future migration versions: `schema "..." has migrations not recognized by this version of the code: [...]...` +- Missing core tables after complete migration records: `duroxide migrations recorded as complete in schema "...", but core tables are missing...` + +`ApplyAll` also rejects unknown future migration versions before it runs DDL, so +an older binary will not mutate a schema that appears ahead of its code. + +## Example + +```rust +use duroxide_pg::{MigrationPolicy, PostgresProvider, ProviderConfig}; + +# async fn example(database_url: &str) -> anyhow::Result<()> { +let mut config = ProviderConfig::url(database_url); +config.schema_name = Some("duroxide".to_string()); +config.migration_policy = MigrationPolicy::VerifyOnly; + +let provider = PostgresProvider::new_with_config(config).await?; +# Ok(()) +# } +``` \ No newline at end of file diff --git a/src/migrations.rs b/src/migrations.rs index a057e7c..eb3b6d3 100644 --- a/src/migrations.rs +++ b/src/migrations.rs @@ -143,6 +143,17 @@ impl MigrationRunner { ); } + if !self.check_tables_exist(conn).await.unwrap_or(false) { + anyhow::bail!( + "duroxide migrations recorded as complete in schema {:?}, but \ + core tables are missing. The schema may be corrupted; run \ + migrations from a provider configured with \ + MigrationPolicy::ApplyAll before constructing VerifyOnly \ + providers.", + self.schema_name, + ); + } + Ok(()) } diff --git a/tests/migration_policy_tests.rs b/tests/migration_policy_tests.rs index 8cc7107..2005dee 100644 --- a/tests/migration_policy_tests.rs +++ b/tests/migration_policy_tests.rs @@ -380,6 +380,41 @@ async fn verify_only_errors_when_migrations_behind() { drop_schema(&schema).await; } +#[tokio::test] +async fn verify_only_errors_when_core_tables_are_missing() { + let database_url = get_database_url(); + let schema = get_test_schema(); + + // Fully initialize, then leave migration records intact while dropping a + // core table. VerifyOnly must not trust the tracking table alone. + let bootstrap = PostgresProvider::new_with_schema(&database_url, Some(&schema)) + .await + .expect("bootstrap apply"); + + sqlx::query(&format!("DROP TABLE {schema}.instances")) + .execute(bootstrap.pool()) + .await + .expect("drop instances table"); + + drop(bootstrap); + + let mut config = ProviderConfig::url(&database_url); + config.schema_name = Some(schema.clone()); + config.migration_policy = MigrationPolicy::VerifyOnly; + + let result = PostgresProvider::new_with_config(config).await; + let msg = match result { + Ok(_) => panic!("VerifyOnly should fail when core tables are missing"), + Err(e) => format!("{e:#}"), + }; + assert!( + msg.contains("core tables are missing") || msg.contains("corrupted"), + "expected missing-core-tables error, got: {msg}" + ); + + drop_schema(&schema).await; +} + #[tokio::test] async fn concurrent_apply_all_is_serialized() { let database_url = get_database_url();