diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 6d193065c2..7e2cc65057 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -19,7 +19,7 @@ Bug fixes should include a regression test which fails before the fix and passes New features *should* include unit or integration tests in the appropriate folders. Database specific tests should go in `tests/`. -Note that unsolicited pull requests implementing large or complex changes may not be reviwed right away. +Note that unsolicited pull requests implementing large or complex changes may not be reviewed right away. Maintainer time and energy is limited and massive unsolicited pull requests require an outsized effort to review. To make the best use of your time and ours, search for and participate in existing discussion on the issue tracker before opening a pull request. diff --git a/Cargo.toml b/Cargo.toml index 3738b814ca..d02b174e33 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -193,6 +193,7 @@ uuid = "1.12.1" base64 = { version = "0.22.1", default-features = false, features = ["alloc"] } cfg-if = "1.0.0" dotenvy = { version = "0.15.7", default-features = false } +tempfile = "3.10.1" thiserror = { version = "2.0.18", default-features = false, features = ["std"] } # Cryptography @@ -247,7 +248,7 @@ serde = { version = "1.0.219", features = ["derive"] } serde_json = "1.0.142" url = "2.2.2" hex = "0.4.3" -tempfile = "3.10.1" +tempfile = { workspace = true } criterion = { version = "0.5.1", features = ["async_tokio"] } libsqlite3-sys = { version = "0.37.0" } diff --git a/sqlx-macros-core/Cargo.toml b/sqlx-macros-core/Cargo.toml index 534b92764d..a5a0617b24 100644 --- a/sqlx-macros-core/Cargo.toml +++ b/sqlx-macros-core/Cargo.toml @@ -80,6 +80,9 @@ syn = { version = "2.0.87", default-features = false, features = ["full", "deriv quote = { version = "1.0.35", default-features = false } url = { version = "2.2.2" } +[dev-dependencies] +tempfile = { workspace = true } + [lints.rust.unexpected_cfgs] level = "warn" check-cfg = ['cfg(sqlx_macros_unstable)', 'cfg(procmacro2_semver_exempt)'] diff --git a/sqlx-macros-core/src/query/metadata.rs b/sqlx-macros-core/src/query/metadata.rs index b3b31c0eb5..ea9fc1463b 100644 --- a/sqlx-macros-core/src/query/metadata.rs +++ b/sqlx-macros-core/src/query/metadata.rs @@ -29,32 +29,44 @@ impl Metadata { pub fn workspace_root(&self) -> PathBuf { let mut root = self.workspace_root.lock().unwrap(); if root.is_none() { - use serde::Deserialize; - use std::process::Command; - - let cargo = crate::env("CARGO").unwrap(); - - let output = Command::new(cargo) - .args(["metadata", "--format-version=1", "--no-deps"]) - .current_dir(&self.manifest_dir) - .env_remove("__CARGO_FIX_PLZ") - .output() - .expect("Could not fetch metadata"); - - #[derive(Deserialize)] - struct CargoMetadata { - workspace_root: PathBuf, - } - - let metadata: CargoMetadata = - serde_json::from_slice(&output.stdout).expect("Invalid `cargo metadata` output"); - - *root = Some(metadata.workspace_root); + *root = Some(resolve_workspace_root( + crate::env("SQLX_WORKSPACE_DIR").ok().map(PathBuf::from), + || { + use serde::Deserialize; + use std::process::Command; + + let cargo = crate::env("CARGO").unwrap(); + + let output = Command::new(cargo) + .args(["metadata", "--format-version=1", "--no-deps"]) + .current_dir(&self.manifest_dir) + .env_remove("__CARGO_FIX_PLZ") + .output() + .expect("Could not fetch metadata"); + + #[derive(Deserialize)] + struct CargoMetadata { + workspace_root: PathBuf, + } + + let metadata: CargoMetadata = serde_json::from_slice(&output.stdout) + .expect("Invalid `cargo metadata` output"); + + metadata.workspace_root + }, + )); } root.clone().unwrap() } } +fn resolve_workspace_root( + override_dir: Option, + cargo_fallback: impl FnOnce() -> PathBuf, +) -> PathBuf { + override_dir.unwrap_or_else(cargo_fallback) +} + pub fn try_for_crate() -> crate::Result> { /// The `MtimeCache` in this type covers the config itself, /// any changes to which will indirectly invalidate the loaded env vars as well. @@ -89,7 +101,7 @@ pub fn try_for_crate() -> crate::Result> { }) } -fn load_env( +fn load_from_dotenv( manifest_dir: &Path, config: &Config, builder: &mut MtimeCacheBuilder, @@ -143,16 +155,32 @@ fn load_env( } } + Ok(Arc::new(from_dotenv)) +} + +fn load_env( + manifest_dir: &Path, + config: &Config, + builder: &mut MtimeCacheBuilder, +) -> crate::Result> { + let database_url_env = crate::env_opt(config.common.database_url_var())?; + let offline_dir_env = crate::env_opt("SQLX_OFFLINE_DIR")?.map(PathBuf::from); + let offline_env = crate::env_opt("SQLX_OFFLINE")?.map(|val| is_truthy_bool(&val)); + + // Don't load .env files if all environment variables are set: we may be in + // a non-Cargo build system like buck2. + let dotenv = if database_url_env.is_none() || offline_dir_env.is_none() || offline_env.is_none() + { + Some(load_from_dotenv(manifest_dir, config, builder)?) + } else { + None + }; + Ok(Arc::new(MacrosEnv { - // Make set variables take precedent - database_url: crate::env_opt(config.common.database_url_var())? - .or(from_dotenv.database_url), - offline_dir: crate::env_opt("SQLX_OFFLINE_DIR")? - .map(PathBuf::from) - .or(from_dotenv.offline_dir), - offline: crate::env_opt("SQLX_OFFLINE")? - .map(|val| is_truthy_bool(&val)) - .or(from_dotenv.offline), + // Make set variables take precedence + database_url: database_url_env.or_else(|| dotenv.as_ref()?.database_url.clone()), + offline_dir: offline_dir_env.or_else(|| dotenv.as_ref()?.offline_dir.clone()), + offline: offline_env.or_else(|| dotenv.as_ref()?.offline), })) } @@ -160,3 +188,51 @@ fn load_env( fn is_truthy_bool(val: &str) -> bool { val.eq_ignore_ascii_case("true") || val == "1" } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn load_from_dotenv_reads_env_file() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write( + dir.path().join(".env"), + "DATABASE_URL=postgres://test\nSQLX_OFFLINE_DIR=/some/dir\nSQLX_OFFLINE=true\n", + ) + .unwrap(); + + let cache: MtimeCache> = MtimeCache::new(); + let env = cache + .get_or_try_init(|builder| load_from_dotenv(dir.path(), &Config::default(), builder)) + .unwrap(); + + assert_eq!(env.database_url.as_deref(), Some("postgres://test")); + assert_eq!(env.offline_dir, Some(PathBuf::from("/some/dir"))); + assert_eq!(env.offline, Some(true)); + } + + #[test] + fn load_from_dotenv_empty_when_no_env_file() { + // The ancestor walk finds nothing as long as no ancestor of the OS temp dir has a .env. + let dir = tempfile::tempdir().unwrap(); + + let cache: MtimeCache> = MtimeCache::new(); + let env = cache + .get_or_try_init(|builder| load_from_dotenv(dir.path(), &Config::default(), builder)) + .unwrap(); + + assert!(env.database_url.is_none()); + assert!(env.offline_dir.is_none()); + assert!(env.offline.is_none()); + } + + #[test] + fn resolve_workspace_root_prefers_override() { + let dir = PathBuf::from("/fake/workspace"); + let result = resolve_workspace_root(Some(dir.clone()), || { + panic!("cargo fallback must not be called when override is set") + }); + assert_eq!(result, dir); + } +}