fix: better I/O errors when migrate!() cannot read a file

abonander · abonander · commit d005111494a5 · 2024-03-06T22:20:59.000-08:00
diff --git 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
@@ -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<Vec<Migration>, BoxDynError>> {
         Box::pin(async move {
-            let mut s = fs::read_dir(self.canonicalize()?).await?;
-            let mut migrations = Vec::new();
-
-            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::<Vec<_>>();
-
-                if parts.len() != 2 || !parts[1].ends_with(".sql") {
-                    // not of the format: <VERSION>_<DESCRIPTION>.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)
+            let canonical = self.canonicalize()?;
+            let migrations_with_paths =
+                crate::rt::spawn_blocking(move || resolve_blocking(canonical)).await?;
+
+            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<io::Error>,
+}
+
+// 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<Vec<(Migration, PathBuf)>, 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::<Vec<_>>();
+
+        if parts.len() != 2 || !parts[1].ends_with(".sql") {
+            // not of the format: <VERSION>_<DESCRIPTION>.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
@@ -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};
+
+pub struct QuoteMigrationType(MigrationType);
 
-impl ToTokens for QuotedMigrationType {
+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<u8>,
+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<TokenStream> {
     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<TokenStream> {
-    let mut migrations = Vec::new();
-
-    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::<Vec<_>>();
-
-        if parts.len() != 2 || !parts[1].ends_with(".sql") {
-            // not of the format: <VERSION>_<DESCRIPTION>.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);
+    let path = path.canonicalize().map_err(|e| {
+        format!(
+            "error canonicalizing migration directory {}: {e}",
+            path.display()
+        )
+    })?;
+
+    // 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: {:?}",
