From 5aaf978f972054dd81777d26b47f967eb9d6f00f Mon Sep 17 00:00:00 2001 From: Lorenz Leutgeb Date: Mon, 2 Mar 2026 16:45:15 +0100 Subject: [PATCH] radicle: Configure database connections on open Configuration of database connections is not performed on `open`, which leaves room for error (e.g. to miss specifying configuration). Methods on `Profile` automatically supply the configuration of the profile. In testing code, just using the default configuration suffices. --- CHANGELOG.md | 4 ++ crates/radicle-cli/tests/util/environment.rs | 2 +- crates/radicle-node/src/runtime.rs | 20 +++--- crates/radicle-node/src/test/node.rs | 16 ++++- crates/radicle-node/src/test/peer.rs | 27 +++---- crates/radicle/CHANGELOG.md | 3 + crates/radicle/src/node/address/store.rs | 2 +- crates/radicle/src/node/db.rs | 15 ++-- crates/radicle/src/node/routing.rs | 2 +- crates/radicle/src/profile.rs | 74 ++++++++++++++------ 10 files changed, 108 insertions(+), 57 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 91f1866c..8e55a33b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 to create temporary patches. - The `rad id` command will provide a better error message when a non-delegate attempts to modify the identity document. +- The `journal_mode` and `synchronous` pragmas can now be configured using the + configuration file. The default values used are `WAL` and `NORMAL`, which + generates less I/O operations. On power loss, transactions might be rolled + back, but SQLite still guarantees consistency in this mode. ## Fixed Bugs diff --git a/crates/radicle-cli/tests/util/environment.rs b/crates/radicle-cli/tests/util/environment.rs index 79e547ae..29417132 100644 --- a/crates/radicle-cli/tests/util/environment.rs +++ b/crates/radicle-cli/tests/util/environment.rs @@ -156,7 +156,7 @@ impl Environment { db.migrate(radicle::cob::migrate::ignore).unwrap(); policy::Store::open(policies_db).unwrap(); - home.database_mut() + home.database_mut(node::db::config::Config::default()) .unwrap() .init( &keypair.pk.into(), diff --git a/crates/radicle-node/src/runtime.rs b/crates/radicle-node/src/runtime.rs index a4fc789a..6ee3734a 100644 --- a/crates/radicle-node/src/runtime.rs +++ b/crates/radicle-node/src/runtime.rs @@ -175,18 +175,14 @@ impl Runtime { .expect("Runtime::init: unable to solve proof-of-work puzzle"); log::info!(target: "node", "Opening node database.."); - let db = home - .database_mut()? - .journal_mode(config.database.sqlite.pragma.journal_mode)? - .synchronous(config.database.sqlite.pragma.synchronous)? - .init( - &id, - announcement.features, - &announcement.alias, - &announcement.agent, - announcement.timestamp, - announcement.addresses.iter(), - )?; + let db = home.database_mut(config.database)?.init( + &id, + announcement.features, + &announcement.alias, + &announcement.agent, + announcement.timestamp, + announcement.addresses.iter(), + )?; let mut stores: service::Stores<_> = db.clone().into(); if config.connect.is_empty() && stores.addresses().is_empty()? { diff --git a/crates/radicle-node/src/test/node.rs b/crates/radicle-node/src/test/node.rs index 268f4284..1cd6db6d 100644 --- a/crates/radicle-node/src/test/node.rs +++ b/crates/radicle-node/src/test/node.rs @@ -150,7 +150,11 @@ impl + cyphernet::Ecdh> NodeHandle { pub fn routing(&self) -> impl Iterator { use node::routing::Store as _; - self.home.routing_mut().unwrap().entries().unwrap() + self.home + .routing_mut(node::db::config::Config::default()) + .unwrap() + .entries() + .unwrap() } pub fn inventory(&self) -> impl Iterator + '_ { @@ -161,7 +165,11 @@ impl + cyphernet::Ecdh> NodeHandle { /// Get sync status of a repo. pub fn synced_seeds(&self, rid: &RepoId) -> Vec { - let db = Database::reader(self.home.node().join(node::NODE_DB_FILE)).unwrap(); + let db = Database::reader( + self.home.node().join(node::NODE_DB_FILE), + node::db::config::Config::default(), + ) + .unwrap(); let seeds = db.seeds_for(rid).unwrap(); seeds.into_iter().collect::, _>>().unwrap() @@ -446,7 +454,9 @@ impl Node { ) .unwrap(); let policies = home.policies_mut().unwrap(); - let db = home.database_mut().unwrap(); + let db = home + .database_mut(node::db::config::Config::default()) + .unwrap(); let db = service::Stores::from(db); log::debug!(target: "test", "Node::init {}: {}", config.alias, signer.public_key()); diff --git a/crates/radicle-node/src/test/peer.rs b/crates/radicle-node/src/test/peer.rs index a61ca837..a2f35428 100644 --- a/crates/radicle-node/src/test/peer.rs +++ b/crates/radicle-node/src/test/peer.rs @@ -173,18 +173,21 @@ where policies.seed(&repo.rid, Scope::Followed).unwrap(); } // Initialize database. - let db = Database::open(config.tmp.path().join(node::NODE_DB_FILE)) - .unwrap() - .init( - &id, - config.config.features(), - &config.config.alias, - &UserAgent::default(), - config.local_time.into(), - config.config.external_addresses.iter(), - ) - .unwrap() - .into(); + let db = Database::open( + config.tmp.path().join(node::NODE_DB_FILE), + node::db::config::Config::default(), + ) + .unwrap() + .init( + &id, + config.config.features(), + &config.config.alias, + &UserAgent::default(), + config.local_time.into(), + config.config.external_addresses.iter(), + ) + .unwrap() + .into(); let announcement = service::gossip::node(&config.config, Timestamp::from(config.local_time) + 1); diff --git a/crates/radicle/CHANGELOG.md b/crates/radicle/CHANGELOG.md index 379b3717..112b8bdf 100644 --- a/crates/radicle/CHANGELOG.md +++ b/crates/radicle/CHANGELOG.md @@ -19,6 +19,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 from `FULL` to `NORMAL`, which generates less I/O operations. On power loss, transactions might be rolled back, but SQLite still guarantees consistency in this mode. +- Opening database connections requires specification of a configuration. + `radicle::Profile` conveniently provides methods that supply the + configuration from `radicle::Profile::config`. ### Removed diff --git a/crates/radicle/src/node/address/store.rs b/crates/radicle/src/node/address/store.rs index ce292a35..6f00f738 100644 --- a/crates/radicle/src/node/address/store.rs +++ b/crates/radicle/src/node/address/store.rs @@ -568,7 +568,7 @@ mod test { fn test_empty() { let tmp = tempfile::tempdir().unwrap(); let path = tmp.path().join("cache"); - let cache = Database::open(path).unwrap(); + let cache = Database::open(path, crate::node::db::config::Config::default()).unwrap(); assert!(cache.is_empty().unwrap()); } diff --git a/crates/radicle/src/node/db.rs b/crates/radicle/src/node/db.rs index 845345cf..4350bf9b 100644 --- a/crates/radicle/src/node/db.rs +++ b/crates/radicle/src/node/db.rs @@ -82,19 +82,19 @@ impl Database { const PRAGMA: &'static str = "PRAGMA foreign_keys = ON"; /// Open a database at the given path. Creates a new database if it - /// doesn't exist. - pub fn open>(path: P) -> Result { + /// does not exist. + pub fn open>(path: P, config: config::Config) -> Result { let mut db = sql::Connection::open_thread_safe(path)?; db.set_busy_timeout(DB_WRITE_TIMEOUT.as_millis() as usize)?; db.execute(Self::PRAGMA)?; migrate(&db)?; - Ok(Self { db: Arc::new(db) }) + Self { db: Arc::new(db) }.configure(config) } /// Same as [`Self::open`], but in read-only mode. This is useful to have multiple /// open databases, as no locking is required. - pub fn reader>(path: P) -> Result { + pub fn reader>(path: P, config: config::Config) -> Result { let mut db = sql::Connection::open_thread_safe_with_flags( path, sql::OpenFlags::new().with_read_only(), @@ -102,7 +102,7 @@ impl Database { db.set_busy_timeout(DB_READ_TIMEOUT.as_millis() as usize)?; db.execute(Self::PRAGMA)?; - Ok(Self { db: Arc::new(db) }) + Self { db: Arc::new(db) }.configure(config) } /// Set `journal_mode` pragma. @@ -163,6 +163,11 @@ impl Database { pub fn bump(&self) -> Result { transaction(&self.db, bump) } + + fn configure(self, config: config::Config) -> Result { + self.journal_mode(config.sqlite.pragma.journal_mode)? + .synchronous(config.sqlite.pragma.synchronous) + } } /// Get the `user_version` value from the database header. diff --git a/crates/radicle/src/node/routing.rs b/crates/radicle/src/node/routing.rs index 2443e4b2..10c084d8 100644 --- a/crates/radicle/src/node/routing.rs +++ b/crates/radicle/src/node/routing.rs @@ -265,7 +265,7 @@ mod test { use crate::test::arbitrary; fn database(path: &str) -> Database { - let db = Database::open(path).unwrap(); + let db = Database::open(path, crate::node::db::config::Config::default()).unwrap(); // We don't want to test foreign key constraints here. db.db.execute("PRAGMA foreign_keys = OFF").unwrap(); diff --git a/crates/radicle/src/profile.rs b/crates/radicle/src/profile.rs index b325bb8a..a88ceeee 100644 --- a/crates/radicle/src/profile.rs +++ b/crates/radicle/src/profile.rs @@ -252,17 +252,14 @@ impl Profile { // Create DBs. home.policies_mut()?; home.notifications_mut()?; - home.database_mut()? - .journal_mode(config.node.database.sqlite.pragma.journal_mode)? - .synchronous(config.node.database.sqlite.pragma.synchronous)? - .init( - &public_key, - config.node.features(), - &config.node.alias, - &UserAgent::default(), - LocalTime::now().into(), - config.node.external_addresses.iter(), - )?; + home.database_mut(config.node.database)?.init( + &public_key, + config.node.features(), + &config.node.alias, + &UserAgent::default(), + LocalTime::now().into(), + config.node.external_addresses.iter(), + )?; // Migrate COBs cache. let mut cobs = home.cobs_db_mut()?; @@ -364,7 +361,7 @@ impl Profile { /// Return a multi-source store for aliases. pub fn aliases(&self) -> Aliases { let policies = self.home.policies().ok(); - let db = self.home.database().ok(); + let db = self.home.database(self.config.node.database).ok(); Aliases { policies, db } } @@ -419,6 +416,24 @@ impl Profile { Err(e) => Err(e.into()), } } + + /// Return a handle to the database of the node, with SQLite configuration + /// from [`Self::config`] applied. + pub fn database_mut(&self) -> Result { + self.home.database_mut(self.config.node.database) + } + + /// Return a handle to a read-only database of the node, with SQLite + /// configuration from [`Self::config`] applied. + pub fn database(&self) -> Result { + self.home.database(self.config.node.database) + } + + /// Returns the routing store, with SQLite + /// configuration from [`Self::config`] applied. + pub fn routing(&self) -> Result { + self.home.routing(self.config.node.database) + } } impl std::ops::Deref for Profile { @@ -630,34 +645,49 @@ impl Home { } /// Return a handle to a read-only database of the node. - pub fn database(&self) -> Result { + pub fn database( + &self, + config: node::db::config::Config, + ) -> Result { let path = self.node().join(node::NODE_DB_FILE); - let db = node::Database::reader(path)?; + let db = node::Database::reader(path, config)?; Ok(db) } /// Return a handle to the database of the node. - pub fn database_mut(&self) -> Result { + pub fn database_mut( + &self, + config: node::db::config::Config, + ) -> Result { let path = self.node().join(node::NODE_DB_FILE); - let db = node::Database::open(path)?; + let db = node::Database::open(path, config)?; Ok(db) } /// Returns the address store. - pub fn addresses(&self) -> Result { - self.database_mut() + pub fn addresses( + &self, + config: node::db::config::Config, + ) -> Result { + self.database_mut(config) } /// Returns the routing store. - pub fn routing(&self) -> Result { - self.database() + pub fn routing( + &self, + config: node::db::config::Config, + ) -> Result { + self.database(config) } /// Returns the routing store, mutably. - pub fn routing_mut(&self) -> Result { - self.database_mut() + pub fn routing_mut( + &self, + config: node::db::config::Config, + ) -> Result { + self.database_mut(config) } /// Get read access to the COBs cache.