From d005111494a5202d64d5c0442a542f94a2c1983c Mon Sep 17 00:00:00 2001 From: Austin Bonander Date: Wed, 6 Mar 2024 17:25:31 -0800 Subject: [PATCH] fix: better I/O errors when `migrate!()` cannot read a file --- sqlx-core/src/migrate/mod.rs | 3 + sqlx-core/src/migrate/source.rs | 146 ++++++++++++++++++++++---------- sqlx-macros-core/src/migrate.rs | 122 +++++++++++--------------- 3 files changed, 152 insertions(+), 119 deletions(-) diff --git a/sqlx-core/src/migrate/mod.rs b/sqlx-core/src/migrate/mod.rs index a1095fb8..f035b8d3 100644 --- a/sqlx-core/src/migrate/mod.rs +++ b/sqlx-core/src/migrate/mod.rs @@ -12,3 +12,6 @@ pub use migration::{AppliedMigration, Migration}; pub use migration_type::MigrationType; pub use migrator::Migrator; pub use source::MigrationSource; + +#[doc(hidden)] +pub use source::resolve_blocking; diff --git a/sqlx-core/src/migrate/source.rs b/sqlx-core/src/migrate/source.rs index 7966dd12..b5e4b816 100644 --- a/sqlx-core/src/migrate/source.rs +++ b/sqlx-core/src/migrate/source.rs @@ -1,10 +1,11 @@ use crate::error::BoxDynError; -use crate::fs; use crate::migrate::{Migration, MigrationType}; use futures_core::future::BoxFuture; use std::borrow::Cow; use std::fmt::Debug; +use std::fs; +use std::io; use std::path::{Path, PathBuf}; /// In the default implementation, a MigrationSource is a directory which @@ -28,51 +29,11 @@ pub trait MigrationSource<'s>: Debug { impl<'s> MigrationSource<'s> for &'s Path { fn resolve(self) -> BoxFuture<'s, Result, BoxDynError>> { Box::pin(async move { - let mut s = fs::read_dir(self.canonicalize()?).await?; - let mut migrations = Vec::new(); + let canonical = self.canonicalize()?; + let migrations_with_paths = + crate::rt::spawn_blocking(move || resolve_blocking(canonical)).await?; - while let Some(entry) = s.next().await? { - // std::fs::metadata traverses symlinks - if !std::fs::metadata(&entry.path)?.is_file() { - // not a file; ignore - continue; - } - - let file_name = entry.file_name.to_string_lossy(); - - let parts = file_name.splitn(2, '_').collect::>(); - - if parts.len() != 2 || !parts[1].ends_with(".sql") { - // not of the format: _.sql; ignore - continue; - } - - let version: i64 = parts[0].parse() - .map_err(|_e| { - format!("error parsing migration filename {file_name:?}; expected integer version prefix (e.g. `01_foo.sql`)") - })?; - - let migration_type = MigrationType::from_filename(parts[1]); - // remove the `.sql` and replace `_` with ` ` - let description = parts[1] - .trim_end_matches(migration_type.suffix()) - .replace('_', " ") - .to_owned(); - - let sql = fs::read_to_string(&entry.path).await?; - - migrations.push(Migration::new( - version, - Cow::Owned(description), - migration_type, - Cow::Owned(sql), - )); - } - - // ensure that we are sorted by `VERSION ASC` - migrations.sort_by_key(|m| m.version); - - Ok(migrations) + Ok(migrations_with_paths.into_iter().map(|(m, _p)| m).collect()) }) } } @@ -82,3 +43,98 @@ impl MigrationSource<'static> for PathBuf { Box::pin(async move { self.as_path().resolve().await }) } } + +#[derive(thiserror::Error, Debug)] +#[error("{message}")] +pub struct ResolveError { + message: String, + #[source] + source: Option, +} + +// FIXME: paths should just be part of `Migration` but we can't add a field backwards compatibly +// since it's `#[non_exhaustive]`. +pub fn resolve_blocking(path: PathBuf) -> Result, ResolveError> { + let mut s = fs::read_dir(&path).map_err(|e| ResolveError { + message: format!("error reading migration directory {}: {e}", path.display()), + source: Some(e), + })?; + + let mut migrations = Vec::new(); + + while let Some(res) = s.next() { + let entry = res.map_err(|e| ResolveError { + message: format!( + "error reading contents of migration directory {}: {e}", + path.display() + ), + source: Some(e), + })?; + + let entry_path = entry.path(); + + let metadata = fs::metadata(&entry_path).map_err(|e| ResolveError { + message: format!( + "error getting metadata of migration path {}", + entry_path.display() + ), + source: Some(e), + })?; + + if !metadata.is_file() { + // not a file; ignore + continue; + } + + let file_name = entry.file_name(); + // This is arguably the wrong choice, + // but it really only matters for parsing the version and description. + // + // Using `.to_str()` and returning an error if the filename is not UTF-8 + // would be a breaking change. + let file_name = file_name.to_string_lossy(); + + let parts = file_name.splitn(2, '_').collect::>(); + + if parts.len() != 2 || !parts[1].ends_with(".sql") { + // not of the format: _.sql; ignore + continue; + } + + let version: i64 = parts[0].parse() + .map_err(|_e| ResolveError { + message: format!("error parsing migration filename {file_name:?}; expected integer version prefix (e.g. `01_foo.sql`)"), + source: None, + })?; + + let migration_type = MigrationType::from_filename(parts[1]); + // remove the `.sql` and replace `_` with ` ` + let description = parts[1] + .trim_end_matches(migration_type.suffix()) + .replace('_', " ") + .to_owned(); + + let sql = fs::read_to_string(&entry_path).map_err(|e| ResolveError { + message: format!( + "error reading contents of migration {}: {e}", + entry_path.display() + ), + source: Some(e), + })?; + + migrations.push(( + Migration::new( + version, + Cow::Owned(description), + migration_type, + Cow::Owned(sql), + ), + entry_path, + )); + } + + // Ensure that we are sorted by version in ascending order. + migrations.sort_by_key(|(m, _)| m.version); + + Ok(migrations) +} diff --git a/sqlx-macros-core/src/migrate.rs b/sqlx-macros-core/src/migrate.rs index 7defc095..ff0eb9f9 100644 --- a/sqlx-macros-core/src/migrate.rs +++ b/sqlx-macros-core/src/migrate.rs @@ -1,17 +1,17 @@ #[cfg(any(sqlx_macros_unstable, procmacro2_semver_exempt))] extern crate proc_macro; +use std::path::{Path, PathBuf}; + use proc_macro2::TokenStream; use quote::{quote, ToTokens, TokenStreamExt}; -use sha2::{Digest, Sha384}; -use sqlx_core::migrate::MigrationType; -use std::fs; -use std::path::Path; use syn::LitStr; -pub struct QuotedMigrationType(MigrationType); +use sqlx_core::migrate::{Migration, MigrationType}; -impl ToTokens for QuotedMigrationType { +pub struct QuoteMigrationType(MigrationType); + +impl ToTokens for QuoteMigrationType { fn to_tokens(&self, tokens: &mut TokenStream) { let ts = match self.0 { MigrationType::Simple => quote! { ::sqlx::migrate::MigrationType::Simple }, @@ -24,31 +24,51 @@ impl ToTokens for QuotedMigrationType { } } -struct QuotedMigration { - version: i64, - description: String, - migration_type: QuotedMigrationType, - path: String, - checksum: Vec, +struct QuoteMigration { + migration: Migration, + path: PathBuf, } -impl ToTokens for QuotedMigration { +impl ToTokens for QuoteMigration { fn to_tokens(&self, tokens: &mut TokenStream) { - let QuotedMigration { + let Migration { version, description, migration_type, - path, checksum, - } = &self; + .. + } = &self.migration; + + let migration_type = QuoteMigrationType(*migration_type); + + let sql = self + .path + .canonicalize() + .map_err(|e| { + format!( + "error canonicalizing migration path {}: {e}", + self.path.display() + ) + }) + .and_then(|path| { + let path_str = path.to_str().ok_or_else(|| { + format!( + "migration path cannot be represented as a string: {}", + self.path.display() + ) + })?; + + // this tells the compiler to watch this path for changes + Ok(quote! { include_str!(#path_str) }) + }) + .unwrap_or_else(|e| quote! { compile_error!(#e) }); let ts = quote! { ::sqlx::migrate::Migration { version: #version, description: ::std::borrow::Cow::Borrowed(#description), migration_type: #migration_type, - // this tells the compiler to watch this path for changes - sql: ::std::borrow::Cow::Borrowed(include_str!(#path)), + sql: ::std::borrow::Cow::Borrowed(#sql), checksum: ::std::borrow::Cow::Borrowed(&[ #(#checksum),* ]), @@ -59,7 +79,6 @@ impl ToTokens for QuotedMigration { } } -// mostly copied from sqlx-core/src/migrate/source.rs pub fn expand_migrator_from_lit_dir(dir: LitStr) -> crate::Result { expand_migrator_from_dir(&dir.value(), dir.span()) } @@ -74,65 +93,20 @@ pub(crate) fn expand_migrator_from_dir( } pub(crate) fn expand_migrator(path: &Path) -> crate::Result { - let mut migrations = Vec::new(); + let path = path.canonicalize().map_err(|e| { + format!( + "error canonicalizing migration directory {}: {e}", + path.display() + ) + })?; - for entry in fs::read_dir(&path)? { - let entry = entry?; - if !fs::metadata(entry.path())?.is_file() { - // not a file; ignore - continue; - } - - let file_name = entry.file_name(); - let file_name = file_name.to_string_lossy(); - - let parts = file_name.splitn(2, '_').collect::>(); - - if parts.len() != 2 || !parts[1].ends_with(".sql") { - // not of the format: _.sql; ignore - continue; - } - - let version: i64 = parts[0].parse()?; - - let migration_type = MigrationType::from_filename(parts[1]); - // remove the `.sql` and replace `_` with ` ` - let description = parts[1] - .trim_end_matches(migration_type.suffix()) - .replace('_', " ") - .to_owned(); - - let sql = fs::read_to_string(&entry.path())?; - - let checksum = Vec::from(Sha384::digest(sql.as_bytes()).as_slice()); - - // canonicalize the path so we can pass it to `include_str!()` - let path = entry.path().canonicalize()?; - let path = path - .to_str() - .ok_or_else(|| { - format!( - "migration path cannot be represented as a string: {:?}", - path - ) - })? - .to_owned(); - - migrations.push(QuotedMigration { - version, - description, - migration_type: QuotedMigrationType(migration_type), - path, - checksum, - }) - } - - // ensure that we are sorted by `VERSION ASC` - migrations.sort_by_key(|m| m.version); + // Use the same code path to resolve migrations at compile time and runtime. + let migrations = sqlx_core::migrate::resolve_blocking(path)? + .into_iter() + .map(|(migration, path)| QuoteMigration { migration, path }); #[cfg(any(sqlx_macros_unstable, procmacro2_semver_exempt))] { - let path = path.canonicalize()?; let path = path.to_str().ok_or_else(|| { format!( "migration directory path cannot be represented as a string: {:?}",