From 6ae20bea8dc12b8d8f119260815c95ce0ca4d8ee Mon Sep 17 00:00:00 2001 From: faukah Date: Mon, 25 May 2026 01:15:08 +0200 Subject: [PATCH] store: clean up StoreBackend --- Cargo.lock | 68 +------- Cargo.toml | 1 - README.md | 4 +- src/diff.rs | 33 ++-- src/json.rs | 16 +- src/main.rs | 9 +- src/store.rs | 142 ++++++++-------- src/store/{db_common.rs => db.rs} | 107 ++++++++++++- src/store/db_eager.rs | 118 -------------- src/store/db_lazy.rs | 258 ------------------------------ src/store/nix_command.rs | 33 ++-- src/store/test_utils.rs | 113 ++----------- 12 files changed, 235 insertions(+), 667 deletions(-) rename src/store/{db_common.rs => db.rs} (54%) delete mode 100644 src/store/db_eager.rs delete mode 100644 src/store/db_lazy.rs diff --git a/Cargo.lock b/Cargo.lock index e5f647c..31a7073 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -11,12 +11,6 @@ dependencies = [ "memchr", ] -[[package]] -name = "aliasable" -version = "0.1.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "250f629c0161ad8107cf89319e990051fae62832fd343083bea452d93e2205fd" - [[package]] name = "anstream" version = "1.0.0" @@ -160,7 +154,7 @@ version = "4.6.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f2ce8604710f6733aa641a2b3731eaa1e8b3d9973d5e3565da11800813f997a9" dependencies = [ - "heck 0.5.0", + "heck", "proc-macro2", "quote", "syn", @@ -238,7 +232,6 @@ dependencies = [ "diff", "eyre", "itertools", - "ouroboros", "pathfinding", "proptest", "regex", @@ -385,12 +378,6 @@ dependencies = [ "hashbrown 0.16.1", ] -[[package]] -name = "heck" -version = "0.4.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "95505c38b4572b2d910cecb0281560f54b440a19336cbbcb27bf6ce6adc6f5a8" - [[package]] name = "heck" version = "0.5.0" @@ -564,30 +551,6 @@ version = "1.70.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "384b8ab6d37215f3c5301a95a4accb5d64aa607f1fcb26a11b5303878451b4fe" -[[package]] -name = "ouroboros" -version = "0.18.5" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1e0f050db9c44b97a94723127e6be766ac5c340c48f2c4bb3ffa11713744be59" -dependencies = [ - "aliasable", - "ouroboros_macro", - "static_assertions", -] - -[[package]] -name = "ouroboros_macro" -version = "0.18.5" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3c7028bdd3d43083f6d8d4d5187680d0d3560d54df4cc9d752005268b41e64d0" -dependencies = [ - "heck 0.4.1", - "proc-macro2", - "proc-macro2-diagnostics", - "quote", - "syn", -] - [[package]] name = "pathfinding" version = "4.15.0" @@ -642,19 +605,6 @@ dependencies = [ "unicode-ident", ] -[[package]] -name = "proc-macro2-diagnostics" -version = "0.10.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "af066a9c399a26e020ada66a034357a868728e72cd426f3adcd35f80d88d88c8" -dependencies = [ - "proc-macro2", - "quote", - "syn", - "version_check", - "yansi", -] - [[package]] name = "proptest" version = "1.11.0" @@ -927,12 +877,6 @@ dependencies = [ "wasm-bindgen", ] -[[package]] -name = "static_assertions" -version = "1.1.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a2eb9349b6444b326872e140eb1cf5e7c522154d69e7a0ffb0fb81c06b37543f" - [[package]] name = "strsim" version = "0.11.1" @@ -1101,12 +1045,6 @@ version = "0.2.15" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "accd4ea62f7bb7a82fe23066fb0957d48ef677f6eeb8215f372f52e48bb32426" -[[package]] -name = "version_check" -version = "0.9.5" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0b928f33d975fc6ad9f86c8f283853ad26bdd5b10b7f1542aa2fa15e2289105a" - [[package]] name = "wait-timeout" version = "0.2.1" @@ -1250,7 +1188,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ea61de684c3ea68cb082b7a88508a8b27fcc8b797d738bfc99a82facf1d752dc" dependencies = [ "anyhow", - "heck 0.5.0", + "heck", "wit-parser", ] @@ -1261,7 +1199,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b7c566e0f4b284dd6561c786d9cb0142da491f46a9fbed79ea69cdad5db17f21" dependencies = [ "anyhow", - "heck 0.5.0", + "heck", "indexmap", "prettyplease", "syn", diff --git a/Cargo.toml b/Cargo.toml index a2bc246..a0eeda0 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -17,7 +17,6 @@ diff = "0.1.13" itertools = "0.14.0" tracing = "0.1" tracing-subscriber = { version = "0.3", features = [ "env-filter" ] } -ouroboros = "0.18.5" pathfinding = "4.14.0" regex = "1.11.1" rusqlite = { features = [ "bundled" ], version = "0.39.0" } diff --git a/README.md b/README.md index e5c8b6a..2bff3d7 100644 --- a/README.md +++ b/README.md @@ -35,11 +35,11 @@ Options: [possible values: auto, always, never] --force-correctness - Fall back to a backend that is focused solely on absolutely guaranteeing correct results at the cost of memory usage and query speed. + Fall back to a backend chain that skips SQLite immutable mode. This is relevant if the output of dix is to be used for more critical applications and not just as human-readable overview. - In the vast, vast majority of cases, the default backend should be sufficient. + The default backend falls back to opening Nix's SQLite database with `?immutable=1` if the normal connection fails. That is faster than Nix commands, but can be inaccurate if the database is being written to at the same time. --output Select the output format to use diff --git a/src/diff.rs b/src/diff.rs index 65fa6de..1b15a2e 100644 --- a/src/diff.rs +++ b/src/diff.rs @@ -54,16 +54,6 @@ use crate::{ }, }; -pub(crate) fn create_backend<'a>( - force_correctness: bool, -) -> store::CombinedStoreBackend<'a> { - if force_correctness { - store::CombinedStoreBackend::default_eager() - } else { - store::CombinedStoreBackend::default_lazy() - } -} - #[derive(Debug, Eq, PartialEq, Ord, PartialOrd)] #[cfg_attr(feature = "json", derive(Serialize))] pub struct Diff> { @@ -213,7 +203,8 @@ pub fn write_package_diff( force_correctness = force_correctness, "starting package diff computation" ); - let mut connection = create_backend(force_correctness); + let mut connection = + store::CombinedStoreBackend::for_correctness(force_correctness); connection.connect()?; tracing::debug!("querying dependencies for old path"); @@ -417,18 +408,20 @@ fn count_versions(versions: Vec) -> HashMap { /// Returns an error if it fails writing to the `writer` pub fn write_packages_diff( writer: &mut impl fmt::Write, - paths_old: impl Iterator, - paths_new: impl Iterator, - system_paths_old: impl Iterator, - system_paths_new: impl Iterator, + paths_old: impl IntoIterator, + paths_new: impl IntoIterator, + system_paths_old: impl IntoIterator, + system_paths_new: impl IntoIterator, ) -> Result { let paths_map = collect_path_versions(paths_old, paths_new); let sys_old_set: HashSet = system_paths_old + .into_iter() .filter_map(|p| p.parse_name_and_version().ok().map(|(n, _)| n.into())) .collect(); let sys_new_set: HashSet = system_paths_new + .into_iter() .filter_map(|p| p.parse_name_and_version().ok().map(|(n, _)| n.into())) .collect(); @@ -446,10 +439,11 @@ pub fn write_packages_diff( /// Takes an iterator of store paths and extracts the package names, /// filtering out any that cannot be parsed. Logs warnings for parse failures. pub(crate) fn collect_system_names( - paths: impl Iterator, + paths: impl IntoIterator, context: &str, ) -> HashSet { paths + .into_iter() .filter_map(|path| { match path.parse_name_and_version() { Ok((name, _)) => Some(name.into()), @@ -468,8 +462,8 @@ pub(crate) fn collect_system_names( /// For each package, stores a tuple of (`old_versions`, `new_versions`). /// Handles parsing errors by logging warnings and skipping problematic entries. pub(crate) fn collect_path_versions( - old: impl Iterator, - new: impl Iterator, + old: impl IntoIterator, + new: impl IntoIterator, ) -> HashMap, Vec)> { let mut paths: HashMap, Vec)> = HashMap::new(); let mut old_count = 0usize; @@ -891,7 +885,8 @@ pub fn spawn_size_diff( tracing::debug!("calculating closure sizes in background"); thread::spawn(move || { - let mut connection = create_backend(force_correctness); + let mut connection = + store::CombinedStoreBackend::for_correctness(force_correctness); connection.connect()?; let result = ( diff --git a/src/json.rs b/src/json.rs index ff36f58..a4cdff2 100644 --- a/src/json.rs +++ b/src/json.rs @@ -15,10 +15,12 @@ use crate::{ add_selection_status, collect_path_versions, collect_system_names, - create_backend, }, generate_diffs_from_paths, - store::StoreBackend, + store::{ + CombinedStoreBackend, + StoreBackend, + }, }; pub fn display_diff( @@ -26,16 +28,16 @@ pub fn display_diff( path_new: &PathBuf, force_correctness: bool, ) -> Result<()> { - let mut connection = create_backend(force_correctness); + let mut connection = CombinedStoreBackend::for_correctness(force_correctness); connection.connect()?; generate_diff(&mut std::io::stdout(), path_old, path_new, &connection) } -fn generate_diff<'a>( +fn generate_diff( out: &mut dyn Write, path_old: &PathBuf, path_new: &PathBuf, - backend: &impl StoreBackend<'a>, + backend: &impl StoreBackend, ) -> Result<()> { // Query dependencies for old path let paths_old = backend.query_dependents(path_old).with_context(|| { @@ -106,7 +108,7 @@ mod tests { use super::*; use crate::store::{ - LazyDBConnection, + DbConnection, test_utils::{ self, // TestDbBuilder, @@ -118,7 +120,7 @@ mod tests { let db_builder = test_utils::create_system_test_db().expect("failed to create test db"); let db_path = db_builder.db_path().to_string_lossy().to_string(); - let mut db = LazyDBConnection::new(&db_path); + let mut db = DbConnection::new(&db_path); db.connect().unwrap(); let system_old = db_builder.resolve_fixture_path(&fixtures::system_path("nixos-25.11")); diff --git a/src/main.rs b/src/main.rs index e734cfe..c76375d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -43,14 +43,15 @@ struct Cli { )] color: clap::ColorChoice, - /// Fall back to a backend that is focused solely on absolutely guaranteeing - /// correct results at the cost of memory usage and query speed. + /// Fall back to a backend chain that skips SQLite immutable mode. /// /// This is relevant if the output of dix is to be used for more /// critical applications and not just as human-readable overview. /// - /// In the vast, vast majority of cases, the default backend should be - /// sufficient. + /// The default backend falls back to opening Nix's SQLite database with + /// `?immutable=1` if the normal connection fails. That is faster than Nix + /// commands, but can be inaccurate if the database is being written to at + /// the same time. #[arg(long, default_value_t = false, global = true)] force_correctness: bool, diff --git a/src/store.rs b/src/store.rs index 3fdee5a..8b2ab95 100644 --- a/src/store.rs +++ b/src/store.rs @@ -1,12 +1,8 @@ //! Provides an interface for querying data from the nix store. //! -//! - [`LazyDBConnection`] is a lazy connection the underlying sqlite database. -//! - [`EagerDBConnection`] is an eager connection the underlying sqlite -//! database. +//! - [`DbConnection`] is a direct connection to the underlying sqlite database. //! - [`CommandBackend`] uses nix commands to interact with the store. -pub mod db_common; -pub mod db_eager; -pub mod db_lazy; +mod db; pub mod nix_command; mod queries; // Make the test db available for the rest of the crate. @@ -14,12 +10,10 @@ mod queries; use std::{ fmt::Display, - iter::Iterator, path::Path, }; -pub use db_eager::EagerDBConnection; -pub use db_lazy::LazyDBConnection; +pub use db::DbConnection; use eyre::{ Result, eyre, @@ -44,73 +38,67 @@ pub const DATABASE_PATH_IMMUTABLE: &str = /// /// This allows us to construct a backend that can fall back /// to e.g. shell commands should something go wrong. -pub trait StoreBackend<'a> { +pub trait StoreBackend: Display { fn connect(&mut self) -> Result<()>; fn connected(&self) -> bool; fn close(&mut self) -> Result<()>; fn query_closure_size(&self, path: &Path) -> Result; - fn query_system_derivations( - &self, - system: &Path, - ) -> Result + '_>>; - fn query_dependents( - &self, - path: &Path, - ) -> Result + '_>>; + fn query_system_derivations(&self, system: &Path) -> Result>; + fn query_dependents(&self, path: &Path) -> Result>; } -/// wrapper trait for debug information -pub trait StoreBackendPrintable<'a>: StoreBackend<'a> + Display {} - -impl<'a, T> StoreBackendPrintable<'a> for T where T: StoreBackend<'a> + Display {} - /// combines multiple store backends by falling back to the next one if the /// current one fails. /// -/// Currently, the first backend that works when connecting is used. -pub struct CombinedStoreBackend<'a> { +/// Queries try connected backends in order. +pub struct CombinedStoreBackend { /// The underlying store backend implementations. - backends: Vec>>, + backends: Vec>, } -impl<'a> CombinedStoreBackend<'a> { - pub fn new(backends: Vec>>) -> Self { +impl CombinedStoreBackend { + pub fn new(backends: Vec>) -> Self { Self { backends } } - /// Returns a backend that is focused on performance. + pub fn for_correctness(force_correctness: bool) -> Self { + if force_correctness { + Self::default_correct() + } else { + Self::default_fast() + } + } + + /// Returns a backend that is focused on performance and availability. /// - /// The first choice is using direct sqlite queries that - /// return the rows lazily, but might skip rows should - /// a row conversion after the first row fail. Note that - /// this should be extremely unlikely / impossible since - /// the current row mappings perform only very basic conversion. - pub fn default_lazy() -> Self { + /// This first tries the regular sqlite database, then falls back to opening + /// it with `immutable=1`, and finally falls back to Nix commands. + pub fn default_fast() -> Self { CombinedStoreBackend::new(vec![ - Box::new(LazyDBConnection::new(DATABASE_PATH)), - Box::new(EagerDBConnection::new(DATABASE_PATH_IMMUTABLE)), + Box::new(DbConnection::new(DATABASE_PATH)), + Box::new(DbConnection::new(DATABASE_PATH_IMMUTABLE)), Box::new(CommandBackend::default()), ]) } /// Returns a backend that is focused solely on absolutely guaranteeing - /// correct results at the cost of memory usage and database speed. + /// correct results if the regular sqlite database cannot be opened. /// /// Note that [`DATABASE_PATH_IMMUTABLE`] is not used here, since opening /// the database can lead to undefined results (also silently with no errors) /// if the database is actually modified while opened. - pub fn default_eager() -> Self { + pub fn default_correct() -> Self { CombinedStoreBackend::new(vec![ - Box::new(EagerDBConnection::new(DATABASE_PATH)), + Box::new(DbConnection::new(DATABASE_PATH)), Box::new(CommandBackend::default()), ]) } // tries to execute a query until it succeeds or all connected backends have // been tried - fn fallback_query<'b, F, Ret>(&'b self, query: F, path: &Path) -> Result + fn fallback_query(&self, query: F, path: &Path) -> Result where - F: Fn(&'b Box>, &Path) -> Result, + F: Fn(&dyn StoreBackend, &Path) -> Result, { let mut combined_err: Option = None; // attempt to cycle through backends until a successful query is made @@ -121,7 +109,7 @@ impl<'a> CombinedStoreBackend<'a> { ); continue; } - let res = query(backend, path); + let res = query(backend.as_ref(), path); match res { Ok(_) => return res, Err(err) => { @@ -142,13 +130,19 @@ impl<'a> CombinedStoreBackend<'a> { } } -impl Default for CombinedStoreBackend<'_> { +impl Display for CombinedStoreBackend { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "CombinedStoreBackend({} backends)", self.backends.len()) + } +} + +impl Default for CombinedStoreBackend { fn default() -> Self { - Self::default_lazy() + Self::default_fast() } } -impl<'a> StoreBackend<'a> for CombinedStoreBackend<'a> { +impl StoreBackend for CombinedStoreBackend { /// connects to all backends. Returns an error if all backends fail fn connect(&mut self) -> Result<()> { tracing::debug!( @@ -223,28 +217,18 @@ impl<'a> StoreBackend<'a> for CombinedStoreBackend<'a> { } fn query_closure_size(&self, path: &Path) -> Result { - self.fallback_query( - |backend, path| (**backend).query_closure_size(path), - path, - ) + self.fallback_query(|backend, path| backend.query_closure_size(path), path) } - fn query_system_derivations( - &self, - system: &Path, - ) -> Result + '_>> { + fn query_system_derivations(&self, system: &Path) -> Result> { self.fallback_query( - |backend, system| (**backend).query_system_derivations(system), + |backend, system| backend.query_system_derivations(system), system, ) } - fn query_dependents( - &self, - path: &Path, - ) -> Result + '_>> { - self - .fallback_query(|backend, path| (**backend).query_dependents(path), path) + fn query_dependents(&self, path: &Path) -> Result> { + self.fallback_query(|backend, path| backend.query_dependents(path), path) } } @@ -283,7 +267,7 @@ mod test { } } - impl StoreBackend<'_> for MockStoreBackend { + impl StoreBackend for MockStoreBackend { fn connect(&mut self) -> Result<()> { if self.fail_connect { Err(eyre!("Connection failed")) @@ -314,15 +298,22 @@ mod test { fn query_system_derivations( &self, _system: &Path, - ) -> Result + '_>> { - unimplemented!() + ) -> Result> { + *self.query_called.borrow_mut() = true; + if self.fail_query { + Err(eyre!("Query failed")) + } else { + Ok(Vec::new()) + } } - fn query_dependents( - &self, - _path: &Path, - ) -> Result + '_>> { - unimplemented!() + fn query_dependents(&self, _path: &Path) -> Result> { + *self.query_called.borrow_mut() = true; + if self.fail_query { + Err(eyre!("Query failed")) + } else { + Ok(Vec::new()) + } } } @@ -359,6 +350,19 @@ mod test { assert_eq!(res.unwrap(), Size::from_bytes(100)); } + #[test] + fn test_path_query_fallback() { + let f1 = Box::new(MockStoreBackend::new("f1", false, true)); + let f2 = Box::new(MockStoreBackend::new("f2", false, false)); + let mut combined = CombinedStoreBackend::new(vec![f1, f2]); + + combined.connect().unwrap(); + + let res = combined.query_dependents(Path::new("/dummy")); + assert!(res.is_ok()); + assert_eq!(res.unwrap(), Vec::new()); + } + #[test] fn test_query_skip_unconnected() { let f1 = Box::new(MockStoreBackend::new("f1", true, false)); diff --git a/src/store/db_common.rs b/src/store/db.rs similarity index 54% rename from src/store/db_common.rs rename to src/store/db.rs index 5555470..db11747 100644 --- a/src/store/db_common.rs +++ b/src/store/db.rs @@ -1,4 +1,13 @@ -use std::path::Path; +use std::{ + fmt::{ + self, + Display, + }, + path::{ + Path, + PathBuf, + }, +}; use eyre::{ Context as _, @@ -12,13 +21,80 @@ use rusqlite::{ use size::Size; use crate::{ + StorePath, path_to_canonical_string, - store::queries, + store::{ + StoreBackend, + queries, + }, }; -pub fn default_sqlite_connection(path: &str) -> Result { +#[derive(Debug)] +pub struct DbConnection { + path: String, + conn: Option, +} + +impl Display for DbConnection { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "DBConnection({})", self.path) + } +} + +impl DbConnection { + /// Create a new connection. + pub fn new(path: impl AsRef) -> Self { + Self { + path: path.as_ref().to_owned(), + conn: None, + } + } + + /// returns a reference to the inner connection + /// + /// raises an error if the connection has not been established + fn get_inner(&self) -> Result<&Connection> { + self + .conn + .as_ref() + .ok_or_else(|| eyre!("Attempted to use database before connecting.")) + } +} + +impl StoreBackend for DbConnection { + fn connect(&mut self) -> Result<()> { + self.conn = Some(open_connection(&self.path)?); + Ok(()) + } + + fn connected(&self) -> bool { + self.conn.is_some() + } + + fn close(&mut self) -> Result<()> { + close_inner_connection(&self.path, &mut self.conn) + } + + fn query_closure_size(&self, path: &Path) -> Result { + query_closure_size(self.get_inner()?, path) + } + + fn query_system_derivations(&self, system: &Path) -> Result> { + query_store_paths( + self.get_inner()?, + queries::QUERY_SYSTEM_DERIVATIONS, + system, + ) + } + + fn query_dependents(&self, path: &Path) -> Result> { + query_store_paths(self.get_inner()?, queries::QUERY_DEPENDENTS, path) + } +} + +fn open_connection(path: &str) -> Result { tracing::debug!(database_path = path, "opening sqlite connection"); - let inner = rusqlite::Connection::open_with_flags( + let inner = Connection::open_with_flags( path, OpenFlags::SQLITE_OPEN_READ_ONLY // We only run queries, safeguard against corrupting the DB. | OpenFlags::SQLITE_OPEN_NO_MUTEX // Part of the default flags, rusqlite takes care of locking anyways. @@ -68,9 +144,7 @@ pub fn default_sqlite_connection(path: &str) -> Result { Ok(inner) } -// FIXME: why is this marked as dead code? It is used by both the lazy -// and eager backend implementation -pub fn default_close_inner_connection( +fn close_inner_connection( path: &str, maybe_conn: &mut Option, ) -> Result<()> { @@ -83,7 +157,7 @@ pub fn default_close_inner_connection( }) } -pub fn query_closure_size(conn: &Connection, path: &Path) -> Result { +fn query_closure_size(conn: &Connection, path: &Path) -> Result { tracing::trace!(path = %path.display(), "querying closure size"); let path = path_to_canonical_string(path)?; @@ -93,3 +167,20 @@ pub fn query_closure_size(conn: &Connection, path: &Path) -> Result { Ok(closure_size) } + +fn query_store_paths( + conn: &Connection, + query: &str, + path: &Path, +) -> Result> { + let path = path_to_canonical_string(path)?; + let mut query = conn.prepare_cached(query)?; + let rows = query.query_map([path], |row| row.get::<_, String>(0))?; + + let mut paths = Vec::new(); + for row in rows { + paths.push(StorePath::try_from(PathBuf::from(row?))?); + } + + Ok(paths) +} diff --git a/src/store/db_eager.rs b/src/store/db_eager.rs deleted file mode 100644 index 17dda88..0000000 --- a/src/store/db_eager.rs +++ /dev/null @@ -1,118 +0,0 @@ -use std::{ - fmt::{ - self, - Display, - }, - path::Path, -}; - -use eyre::{ - Result, - eyre, -}; -use rusqlite::Row; - -use crate::{ - StorePath, - path_to_canonical_string, - store::{ - StoreBackend, - db_common::{ - self, - }, - queries, - }, -}; - -#[derive(Debug)] -pub struct EagerDBConnection<'a> { - path: &'a str, - conn: Option, -} - -impl Display for EagerDBConnection<'_> { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "DBConnection({})", self.path) - } -} - -impl<'a> EagerDBConnection<'a> { - /// Create a new connection. - pub const fn new(path: &'a str) -> Self { - Self { path, conn: None } - } - /// returns a reference to the inner connection - /// - /// raises an error if the connection has not been established - fn get_inner(&self) -> Result<&rusqlite::Connection> { - self - .conn - .as_ref() - .ok_or_else(|| eyre!("Attempted to use database before connecting.")) - } - - /// Executes a query that returns multiple rows and returns - /// an iterator over them where the `map` is used to map - /// the rows to `T`. - /// - /// Note that this function collects all rows before returning - /// and raises the first error that is encountered (if any exist). - pub(crate) fn execute_row_query_with_path( - &self, - query: &str, - path: &Path, - map: M, - ) -> Result + '_>> - where - T: 'static, - M: Fn(&Row) -> rusqlite::Result + 'static, - { - let path = path_to_canonical_string(path)?; - let mut results = Vec::new(); - let mut query = self.get_inner()?.prepare_cached(query)?; - let queried_rows = query.query_map([path], map)?; - for row in queried_rows { - results.push(row?); - } - Ok(Box::new(results.into_iter())) - } -} - -impl StoreBackend<'_> for EagerDBConnection<'_> { - fn connect(&mut self) -> Result<()> { - self.conn = Some(db_common::default_sqlite_connection(self.path)?); - Ok(()) - } - - fn connected(&self) -> bool { - self.conn.is_some() - } - - fn close(&mut self) -> Result<()> { - db_common::default_close_inner_connection(self.path, &mut self.conn) - } - - fn query_closure_size(&self, path: &std::path::Path) -> Result { - db_common::query_closure_size(self.get_inner()?, path) - } - - fn query_system_derivations( - &self, - system: &std::path::Path, - ) -> Result + '_>> { - self.execute_row_query_with_path( - queries::QUERY_SYSTEM_DERIVATIONS, - system, - |row| Ok(StorePath(row.get::<_, String>(0)?.into())), - ) - } - - fn query_dependents( - &self, - path: &std::path::Path, - ) -> Result + '_>> { - self.execute_row_query_with_path(queries::QUERY_DEPENDENTS, path, |row| { - Ok(StorePath(row.get::<_, String>(0)?.into())) - }) - } -} diff --git a/src/store/db_lazy.rs b/src/store/db_lazy.rs deleted file mode 100644 index f57e475..0000000 --- a/src/store/db_lazy.rs +++ /dev/null @@ -1,258 +0,0 @@ -use std::{ - fmt::{ - self, - Display, - }, - iter::{ - FilterMap, - Peekable, - }, - path::Path, -}; - -use eyre::{ - Context, - Result, - eyre, -}; -use ouroboros::self_referencing; -use rusqlite::{ - CachedStatement, - MappedRows, - Row, -}; -use size::Size; -use tracing::warn; - -use crate::{ - StorePath, - path_to_canonical_string, - store::{ - StoreBackend, - db_common::{ - self, - }, - queries, - }, -}; -type FilterOkFunc = fn(Result) -> Option; - -#[self_referencing] -/// Contains the SQL statement and the query resulting from it. -/// -/// This is necessary since the statement is only created during -/// the query method on the Connection. The query however contains -/// a reference to it, so we can't simply return the Query -struct QueryIteratorCell<'conn, T, F> -where - T: 'static, - F: Fn(&rusqlite::Row) -> rusqlite::Result, -{ - /// statement prepared by the sql connection - stmt: CachedStatement<'conn>, - /// The actual iterator we generate from the query iterator - /// - /// Note that the concrete datatype is rather complicated - /// since we currently only have a single - /// way to deal with queries that return multiple rows and - /// we therefore don't need to use a box. - #[borrows(mut stmt)] - #[not_covariant] - inner: FilterMap>, FilterOkFunc>, -} - -/// The iterator over the data resulting from a SQL query, -/// where the rows are mapped to `T`. -/// -/// We ignore all rows where the conversion fails, -/// but take a look at the first row to make sure -/// the conversion is not trivially wrong. -/// -/// The idea is to only use very trivial -/// conversions that will never fail -/// if the query actually returns the correct number -/// of rows. -pub struct QueryIterator<'conn, T, F> -where - T: 'static, - F: Fn(&rusqlite::Row) -> rusqlite::Result, -{ - cell: QueryIteratorCell<'conn, T, F>, -} - -impl<'conn, T, F> QueryIterator<'conn, T, F> -where - F: Fn(&rusqlite::Row) -> rusqlite::Result, -{ - /// May fail if the query itself fails or - /// if the first row of the query result can not - /// be mapped to `T`. - pub fn try_new( - stmt: CachedStatement<'conn>, - params: P, - map: F, - ) -> Result { - let cell_res = QueryIteratorCell::try_new(stmt, |stmt| { - let inner_iter = stmt - .query_map(params, map) - .map(Iterator::peekable) - .with_context(|| "Unable to perform query"); - - match inner_iter { - Ok(mut iter) => { - #[expect(clippy::pattern_type_mismatch)] - if let Some(Err(err)) = iter.peek() { - return Err(eyre!("First row conversion failed: {err:?}")); - } - let iter_filtered = iter.filter_map( - (|row| { - if let Err(ref err) = row { - tracing::warn!("Row conversion failed: {err:?}"); - } - row.ok() - }) as FilterOkFunc, - ); - - Ok(iter_filtered) - }, - Err(err) => Err(err), - } - }); - cell_res.map(|cell| Self { cell }) - } -} - -impl Iterator for QueryIterator<'_, T, F> -where - F: Fn(&rusqlite::Row) -> rusqlite::Result, -{ - type Item = T; - fn next(&mut self) -> Option { - self.cell.with_inner_mut(|inner| inner.next()) - } -} - -/// A lazy Nix database connection. -/// -/// All returned iterators are lazy (except for the first row) and provide rows -/// as soon as they are returned by the underlying sqlite connection. -/// -/// The first row is queried eagerly to catch any obvious errors in the -/// query. -/// -/// # Important -/// If any errors occur in rows after the first one, these errors **will not** -/// be visible and the errors will be lost. -/// -/// You may consider using an [`crate::store::EagerDBConnection`] instead. -#[derive(Debug)] -pub struct LazyDBConnection<'a> { - path: &'a str, - conn: Option, -} - -impl Display for LazyDBConnection<'_> { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "DBConnection({})", self.path) - } -} - -impl<'a> LazyDBConnection<'a> { - /// Create a new connection. - pub const fn new(path: &'a str) -> Self { - Self { path, conn: None } - } - /// returns a reference to the inner connection - /// - /// raises an error if the connection has not been established - fn get_inner(&self) -> Result<&rusqlite::Connection> { - self - .conn - .as_ref() - .ok_or_else(|| eyre!("Attempted to use database before connecting.")) - } - /// Executes a query that returns multiple rows and returns - /// an iterator over them where the `map` is used to map - /// the rows to `T`. - pub(crate) fn execute_row_query_with_path( - &self, - query: &str, - path: &Path, - map: M, - ) -> Result + '_>> - where - T: 'static, - M: Fn(&Row) -> rusqlite::Result + 'static, - { - let path = path_to_canonical_string(path)?; - let stmt = self.get_inner()?.prepare_cached(query)?; - let iter = QueryIterator::try_new(stmt, [path], map)?; - Ok(Box::new(iter)) - } -} - -/// makes sure the database tries to close the connection -/// when being dropped. This is done anyways by the internal -/// connection of rusqlite, but here the error gets logged should -/// something go wrong. -impl Drop for LazyDBConnection<'_> { - /// close the connection if it is still open - fn drop(&mut self) { - // try to close the connection - if let Some(conn) = self.conn.take() - && let Err(err) = conn.close() - { - warn!( - "Tried closing database on drop but encountered error: {:?}", - err - ); - } - } -} - -impl StoreBackend<'_> for LazyDBConnection<'_> { - fn connected(&self) -> bool { - self.conn.is_some() - } - /// Connects to the Nix database - /// - /// and sets some basic settings - fn connect(&mut self) -> Result<()> { - self.conn = Some(db_common::default_sqlite_connection(self.path)?); - Ok(()) - } - - /// close the inner connection to the database - fn close(&mut self) -> Result<()> { - db_common::default_close_inner_connection(self.path, &mut self.conn) - } - /// Gets the total closure size of the given store path by summing up the nar - /// size of all dependent derivations. - fn query_closure_size(&self, path: &Path) -> Result { - db_common::query_closure_size(self.get_inner()?, path) - } - - /// Gets the derivations that are directly included in the system derivation. - /// - /// Will not work on non-system derivations. - fn query_system_derivations( - &self, - system: &Path, - ) -> Result + '_>> { - self.execute_row_query_with_path( - queries::QUERY_SYSTEM_DERIVATIONS, - system, - |row| Ok(StorePath(row.get::<_, String>(0)?.into())), - ) - } - - /// Gathers all derivations that the given profile path depends on. - fn query_dependents( - &self, - path: &Path, - ) -> Result + '_>> { - self.execute_row_query_with_path(queries::QUERY_DEPENDENTS, path, |row| { - Ok(StorePath(row.get::<_, String>(0)?.into())) - }) - } -} diff --git a/src/store/nix_command.rs b/src/store/nix_command.rs index 06a179d..5899225 100644 --- a/src/store/nix_command.rs +++ b/src/store/nix_command.rs @@ -67,16 +67,21 @@ impl CommandBackend { } } -fn nix_command_query<'a>( - cmd_store: &str, - args: &'a [&'a str], -) -> Result>> { +fn nix_command_query(cmd_store: &str, args: &[&str]) -> Result> { let command_str = format!("{cmd_store} {}", args.join(" ")); tracing::debug!(command = %command_str, "executing nix command"); let references = Command::new(cmd_store).args(args).output(); let query = references?; tracing::trace!(command = %command_str, "nix command executed successfully"); + if !query.status.success() { + let stderr = String::from_utf8_lossy(&query.stderr); + bail!( + "nix command exited with non-zero status {status}: {err}", + status = query.status, + err = stderr.trim() + ); + } // We just collect into a vec, as this method of // querying data is slow anyways let mut paths = Vec::new(); @@ -87,10 +92,10 @@ fn nix_command_query<'a>( paths.push(path); } - Ok(Box::new(paths.into_iter())) + Ok(paths) } -impl StoreBackend<'_> for CommandBackend { +impl StoreBackend for CommandBackend { /// Does nothing (we spawn a new process everytime). fn connect(&mut self) -> Result<()> { Ok(()) @@ -134,10 +139,7 @@ impl StoreBackend<'_> for CommandBackend { } } - fn query_system_derivations( - &self, - system: &Path, - ) -> Result + '_>> { + fn query_system_derivations(&self, system: &Path) -> Result> { nix_command_query(&self.nix_cmd, &[ "--query", "--references", @@ -145,10 +147,7 @@ impl StoreBackend<'_> for CommandBackend { ]) } - fn query_dependents( - &self, - path: &Path, - ) -> Result + '_>> { + fn query_dependents(&self, path: &Path) -> Result> { nix_command_query(&self.nix_cmd, &[ "--query", "--requisites", @@ -289,8 +288,7 @@ mod tests { let (_tmpdir, backend) = setup_fake_nix_command_backend(); let mut references = backend .query_system_derivations(Path::new(FAKE_STORE_PATH)) - .unwrap() - .collect::>(); + .unwrap(); references.sort(); let mut expected = FAKE_PATHS .lines() @@ -306,8 +304,7 @@ mod tests { let (_tmpdir, backend) = setup_fake_nix_command_backend(); let mut references = backend .query_dependents(Path::new(FAKE_STORE_PATH)) - .unwrap() - .collect::>(); + .unwrap(); references.sort(); let mut expected = FAKE_PATHS .lines() diff --git a/src/store/test_utils.rs b/src/store/test_utils.rs index 9dab439..cf56f26 100644 --- a/src/store/test_utils.rs +++ b/src/store/test_utils.rs @@ -358,9 +358,8 @@ mod tests { use super::*; use crate::store::{ + DbConnection, StoreBackend, - db_eager::EagerDBConnection, - db_lazy::LazyDBConnection, }; #[test] @@ -395,13 +394,13 @@ mod tests { } #[test] - fn test_eager_query_closure_size() { + fn test_db_query_closure_size() { let db = create_simple_test_db().unwrap(); let db_path = db.db_path().to_string_lossy().to_string(); let root_fixture = fixtures::store_path("root-package"); let root = db.resolve_fixture_path(&root_fixture); - let mut conn = EagerDBConnection::new(&db_path); + let mut conn = DbConnection::new(&db_path); conn.connect().unwrap(); assert!(conn.connected()); let size = conn.query_closure_size(&root).unwrap(); @@ -411,91 +410,33 @@ mod tests { } #[test] - fn test_lazy_query_closure_size() { - let db = create_simple_test_db().unwrap(); - let db_path = db.db_path().to_string_lossy().to_string(); - let root_fixture = fixtures::store_path("root-package"); - let root = db.resolve_fixture_path(&root_fixture); - - let mut conn = LazyDBConnection::new(&db_path); - conn.connect().unwrap(); - assert!(conn.connected()); - let size = conn.query_closure_size(&root).unwrap(); - assert_eq!(size, Size::from_bytes(200)); - conn.close().unwrap(); - assert!(!conn.connected()); - } - - #[test] - fn test_eager_query_dependents() { - let db = create_diamond_test_db().unwrap(); - let db_path = db.db_path().to_string_lossy().to_string(); - let a_fixture = fixtures::store_path("package-a"); - let a = db.resolve_fixture_path(&a_fixture); - - let mut conn = EagerDBConnection::new(&db_path); - conn.connect().unwrap(); - let dependents: Vec<_> = conn.query_dependents(&a).unwrap().collect(); - assert_eq!(dependents.len(), 4); - conn.close().unwrap(); - } - - #[test] - fn test_lazy_query_dependents() { + fn test_db_query_dependents() { let db = create_diamond_test_db().unwrap(); let db_path = db.db_path().to_string_lossy().to_string(); let a_fixture = fixtures::store_path("package-a"); let a = db.resolve_fixture_path(&a_fixture); - let mut conn = LazyDBConnection::new(&db_path); + let mut conn = DbConnection::new(&db_path); conn.connect().unwrap(); - let dependents: Vec<_> = conn.query_dependents(&a).unwrap().collect(); + let dependents = conn.query_dependents(&a).unwrap(); assert_eq!(dependents.len(), 4); conn.close().unwrap(); } #[test] - fn test_eager_query_system_derivations() { + fn test_db_query_system_derivations() { let db = create_system_test_db().unwrap(); let db_path = db.db_path().to_string_lossy().to_string(); let system_fixture = fixtures::system_path("nixos-25.11"); let system = db.resolve_fixture_path(&system_fixture); - let mut conn = EagerDBConnection::new(&db_path); + let mut conn = DbConnection::new(&db_path); conn.connect().unwrap(); - let derivations: Vec<_> = - conn.query_system_derivations(&system).unwrap().collect(); + let derivations = conn.query_system_derivations(&system).unwrap(); assert!(!derivations.is_empty()); conn.close().unwrap(); } - #[test] - fn test_lazy_query_system_derivations() { - let db = create_system_test_db().unwrap(); - let db_path = db.db_path().to_string_lossy().to_string(); - let system_fixture = fixtures::system_path("nixos-25.11"); - let system = db.resolve_fixture_path(&system_fixture); - - let mut conn = LazyDBConnection::new(&db_path); - conn.connect().unwrap(); - let derivations: Vec<_> = - conn.query_system_derivations(&system).unwrap().collect(); - assert!(!derivations.is_empty()); - conn.close().unwrap(); - } - - #[test] - fn test_lazy_auto_close_on_drop() { - let db = create_simple_test_db().unwrap(); - let db_path = db.db_path().to_string_lossy().to_string(); - - { - let mut conn = LazyDBConnection::new(&db_path); - conn.connect().unwrap(); - assert!(conn.connected()); - } - } - #[test] fn test_self_referential_path() { let db = edge_cases::create_self_reference_test_db().unwrap(); @@ -503,7 +444,7 @@ mod tests { let path_fixture = fixtures::store_path("self-referential"); let path = db.resolve_fixture_path(&path_fixture); - let mut conn = EagerDBConnection::new(&db_path); + let mut conn = DbConnection::new(&db_path); conn.connect().unwrap(); let size = conn.query_closure_size(&path).unwrap(); assert_eq!(size, Size::from_bytes(500)); @@ -515,7 +456,7 @@ mod tests { let db = edge_cases::create_circular_test_db().unwrap(); let db_path = db.db_path().to_string_lossy().to_string(); - let mut conn = EagerDBConnection::new(&db_path); + let mut conn = DbConnection::new(&db_path); conn.connect().unwrap(); for letter in ["a", "b", "c"] { let path = db.resolve_fixture_path(&fixtures::store_path(&format!( @@ -537,12 +478,12 @@ mod tests { let db_path = db.db_path().to_string_lossy().to_string(); let path = db.resolve_fixture_path(&fixtures::store_path("wide-root")); - let mut conn = EagerDBConnection::new(&db_path); + let mut conn = DbConnection::new(&db_path); conn.connect().unwrap(); let size = conn.query_closure_size(&path).unwrap(); assert_eq!(size, Size::from_bytes(6000)); // 1000 + 100*50 - let dependents: Vec<_> = conn.query_dependents(&path).unwrap().collect(); + let dependents = conn.query_dependents(&path).unwrap(); assert_eq!(dependents.len(), 101); // root + 100 children conn.close().unwrap(); } @@ -553,37 +494,13 @@ mod tests { let db_path = db.db_path().to_string_lossy().to_string(); let path = db.resolve_fixture_path(&fixtures::store_path("deep-0")); - let mut conn = LazyDBConnection::new(&db_path); + let mut conn = DbConnection::new(&db_path); conn.connect().unwrap(); let size = conn.query_closure_size(&path).unwrap(); assert_eq!(size, Size::from_bytes(10000)); // 100 * 100 - let dependents: Vec<_> = conn.query_dependents(&path).unwrap().collect(); + let dependents = conn.query_dependents(&path).unwrap(); assert_eq!(dependents.len(), 100); conn.close().unwrap(); } - - #[test] - fn test_both_backends_produce_same_results() { - let db = edge_cases::create_circular_test_db().unwrap(); - let db_path = db.db_path().to_string_lossy().to_string(); - let path = db.resolve_fixture_path(&fixtures::store_path("circular-a")); - - let mut eager = EagerDBConnection::new(&db_path); - let mut lazy = LazyDBConnection::new(&db_path); - eager.connect().unwrap(); - lazy.connect().unwrap(); - - assert_eq!( - eager.query_closure_size(&path).unwrap(), - lazy.query_closure_size(&path).unwrap() - ); - assert_eq!( - eager.query_dependents(&path).unwrap().count(), - lazy.query_dependents(&path).unwrap().count() - ); - - eager.close().unwrap(); - lazy.close().unwrap(); - } }