diff --git a/crates/factor-key-value-spin/src/lib.rs b/crates/factor-key-value-spin/src/lib.rs index 38e9ef442..062398a01 100644 --- a/crates/factor-key-value-spin/src/lib.rs +++ b/crates/factor-key-value-spin/src/lib.rs @@ -11,12 +11,16 @@ use spin_key_value_sqlite::{DatabaseLocation, KeyValueSqlite}; /// A key-value store that uses SQLite as the backend. pub struct SpinKeyValueStore { /// The base path or directory for the SQLite database file. - base_path: PathBuf, + base_path: Option, } impl SpinKeyValueStore { /// Create a new SpinKeyValueStore with the given base path. - pub fn new(base_path: PathBuf) -> Self { + /// + /// If the database directory is None, the database will always be in-memory. + /// If it's `Some`, the database will be stored at the combined `base_path` and + /// the `path` specified in the runtime configuration. + pub fn new(base_path: Option) -> Self { Self { base_path } } } @@ -31,25 +35,14 @@ pub struct SpinKeyValueRuntimeConfig { impl SpinKeyValueRuntimeConfig { /// The default filename for the SQLite database. const DEFAULT_SPIN_STORE_FILENAME: &'static str = "sqlite_key_value.db"; - - /// Create a new runtime configuration with the given directory. - /// - /// If the database directory is None, the database is in-memory. - /// If the database directory is Some, the database is stored in a file in the given directory. - pub fn default(default_database_dir: Option) -> Self { - let path = default_database_dir.map(|dir| dir.join(Self::DEFAULT_SPIN_STORE_FILENAME)); - Self { path } - } } -/// Resolve a relative path against a base dir. -/// -/// If the path is absolute, it is returned as is. Otherwise, it is resolved against the base dir. -fn resolve_relative_path(path: &Path, base_dir: &Path) -> PathBuf { - if path.is_absolute() { - return path.to_owned(); +impl Default for SpinKeyValueRuntimeConfig { + fn default() -> Self { + Self { + path: Some(PathBuf::from(Self::DEFAULT_SPIN_STORE_FILENAME)), + } } - base_dir.join(path) } impl MakeKeyValueStore for SpinKeyValueStore { @@ -63,16 +56,30 @@ impl MakeKeyValueStore for SpinKeyValueStore { &self, runtime_config: Self::RuntimeConfig, ) -> anyhow::Result { - let location = match runtime_config.path { - Some(path) => { - let path = resolve_relative_path(&path, &self.base_path); + // The base path and the subpath must both be set otherwise, we default to in-memory. + let location = + if let (Some(base_path), Some(path)) = (&self.base_path, &runtime_config.path) { + let path = resolve_relative_path(path, base_path); // Create the store's parent directory if necessary - fs::create_dir_all(path.parent().unwrap()) - .context("Failed to create key value store")?; + let parent = path.parent().unwrap(); + if !parent.exists() { + fs::create_dir_all(parent) + .context("Failed to create key value store's parent directory")?; + } DatabaseLocation::Path(path) - } - None => DatabaseLocation::InMemory, - }; + } else { + DatabaseLocation::InMemory + }; Ok(KeyValueSqlite::new(location)) } } + +/// Resolve a relative path against a base dir. +/// +/// If the path is absolute, it is returned as is. Otherwise, it is resolved against the base dir. +fn resolve_relative_path(path: &Path, base_dir: &Path) -> PathBuf { + if path.is_absolute() { + return path.to_owned(); + } + base_dir.join(path) +} diff --git a/crates/factor-key-value/src/runtime_config.rs b/crates/factor-key-value/src/runtime_config.rs index 97e2a3ece..0c83243a3 100644 --- a/crates/factor-key-value/src/runtime_config.rs +++ b/crates/factor-key-value/src/runtime_config.rs @@ -13,6 +13,8 @@ pub struct RuntimeConfig { impl RuntimeConfig { /// Adds a store manager for the store with the given label to the runtime configuration. + /// + /// If a store manager already exists for the given label, it will be replaced. pub fn add_store_manager(&mut self, label: String, store_manager: Arc) { self.store_managers.insert(label, store_manager); } diff --git a/crates/factor-key-value/src/runtime_config/spin.rs b/crates/factor-key-value/src/runtime_config/spin.rs index 3f3e487f5..860208b7e 100644 --- a/crates/factor-key-value/src/runtime_config/spin.rs +++ b/crates/factor-key-value/src/runtime_config/spin.rs @@ -62,8 +62,20 @@ impl RuntimeConfigResolver { /// /// Users must ensure that the store type for `config` has been registered with /// the resolver using [`Self::register_store_type`]. - pub fn add_default_store(&mut self, label: &'static str, config: StoreConfig) { - self.defaults.insert(label, config); + pub fn add_default_store( + &mut self, + label: &'static str, + config: T::RuntimeConfig, + ) -> anyhow::Result<()> + where + T: MakeKeyValueStore, + T::RuntimeConfig: Serialize, + { + self.defaults.insert( + label, + StoreConfig::new(T::RUNTIME_CONFIG_TYPE.to_owned(), config)?, + ); + Ok(()) } /// Registers a store type to the resolver. diff --git a/crates/factor-key-value/tests/factor_test.rs b/crates/factor-key-value/tests/factor_test.rs index 9412b9e51..f796162d6 100644 --- a/crates/factor-key-value/tests/factor_test.rs +++ b/crates/factor-key-value/tests/factor_test.rs @@ -1,12 +1,13 @@ -use anyhow::Context; +use anyhow::Context as _; use spin_factor_key_value::{ - runtime_config::spin::{MakeKeyValueStore, RuntimeConfigResolver, StoreConfig}, + runtime_config::spin::{MakeKeyValueStore, RuntimeConfigResolver}, KeyValueFactor, RuntimeConfig, }; use spin_factor_key_value_redis::RedisKeyValueStore; -use spin_factor_key_value_spin::{SpinKeyValueRuntimeConfig, SpinKeyValueStore}; +use spin_factor_key_value_spin::SpinKeyValueStore; use spin_factors::{FactorRuntimeConfigSource, RuntimeConfigSourceFinalizer, RuntimeFactors}; use spin_factors_test::{toml, TestEnvironment}; +use spin_world::v2::key_value::HostStore; use std::{collections::HashSet, sync::Arc}; #[derive(RuntimeFactors)] @@ -14,25 +15,11 @@ struct TestFactors { key_value: KeyValueFactor, } -fn default_key_value_resolver() -> anyhow::Result<(RuntimeConfigResolver, tempdir::TempDir)> { - let mut test_resolver = RuntimeConfigResolver::new(); - test_resolver.register_store_type(SpinKeyValueStore::new( - std::env::current_dir().context("failed to get current directory")?, - ))?; - let tmp_dir = tempdir::TempDir::new("example")?; - let path = tmp_dir.path().to_path_buf(); - let default_config = SpinKeyValueRuntimeConfig::default(Some(path)); - let store_config = StoreConfig::new( - SpinKeyValueStore::RUNTIME_CONFIG_TYPE.to_string(), - default_config, - )?; - test_resolver.add_default_store("default", store_config); - Ok((test_resolver, tmp_dir)) -} - #[tokio::test] async fn default_key_value_works() -> anyhow::Result<()> { - let (test_resolver, dir) = default_key_value_resolver()?; + let mut test_resolver = RuntimeConfigResolver::new(); + test_resolver.register_store_type(SpinKeyValueStore::new(None))?; + test_resolver.add_default_store::("default", Default::default())?; let factors = TestFactors { key_value: KeyValueFactor::new(test_resolver), }; @@ -47,8 +34,6 @@ async fn default_key_value_works() -> anyhow::Result<()> { state.key_value.allowed_stores(), &["default".into()].into_iter().collect::>() ); - // Ensure the database directory is created - assert!(dir.path().exists()); Ok(()) } @@ -56,7 +41,7 @@ async fn run_test_with_config_and_stores_for_label( runtime_config: Option, store_types: Vec, labels: Vec<&str>, -) -> anyhow::Result<()> { +) -> anyhow::Result { let mut test_resolver = RuntimeConfigResolver::new(); for store_type in store_types { test_resolver.register_store_type(store_type)?; @@ -79,7 +64,7 @@ async fn run_test_with_config_and_stores_for_label( state.key_value.allowed_stores().iter().collect::>() ); - Ok(()) + Ok(state) } #[tokio::test] @@ -94,7 +79,8 @@ async fn overridden_default_key_value_works() -> anyhow::Result<()> { vec![RedisKeyValueStore::new()], vec!["default"], ) - .await + .await?; + Ok(()) } #[tokio::test] @@ -105,52 +91,69 @@ async fn custom_spin_key_value_works() -> anyhow::Result<()> { }; run_test_with_config_and_stores_for_label( Some(runtime_config), - vec![SpinKeyValueStore::new( - std::env::current_dir().context("failed to get current directory")?, - )], + vec![SpinKeyValueStore::new(None)], vec!["custom"], ) - .await + .await?; + Ok(()) } -#[tokio::test] +#[tokio::test(flavor = "multi_thread", worker_threads = 1)] async fn custom_spin_key_value_works_with_absolute_path() -> anyhow::Result<()> { let tmp_dir = tempdir::TempDir::new("example")?; - let path = tmp_dir.path().join("custom.db"); - let path_str = path.to_str().unwrap(); + let db_path = tmp_dir.path().join("foo/custom.db"); + // Check that the db does not exist yet - it will exist by the end of the test + assert!(!db_path.exists()); + + let path_str = db_path.to_str().unwrap(); let runtime_config = toml::toml! { [key_value_store.custom] type = "spin" path = path_str }; - run_test_with_config_and_stores_for_label( + let mut state = run_test_with_config_and_stores_for_label( Some(runtime_config), - vec![SpinKeyValueStore::new( + vec![SpinKeyValueStore::new(Some( std::env::current_dir().context("failed to get current directory")?, - )], + ))], vec!["custom"], ) .await?; - assert!(tmp_dir.path().exists()); + + // Actually et a key since store creation is lazy + let store = state.key_value.open("custom".to_owned()).await??; + let _ = state.key_value.get(store, "foo".to_owned()).await??; + + // Check that the parent has been created + assert!(db_path.exists()); Ok(()) } -#[tokio::test] +#[tokio::test(flavor = "multi_thread", worker_threads = 1)] async fn custom_spin_key_value_works_with_relative_path() -> anyhow::Result<()> { let tmp_dir = tempdir::TempDir::new("example")?; - let path = tmp_dir.path().to_owned(); + let db_path = tmp_dir.path().join("custom.db"); + // Check that the db does not exist yet - it will exist by the end of the test + assert!(!db_path.exists()); + let runtime_config = toml::toml! { [key_value_store.custom] type = "spin" path = "custom.db" }; - run_test_with_config_and_stores_for_label( + let mut state = run_test_with_config_and_stores_for_label( Some(runtime_config), - vec![SpinKeyValueStore::new(path)], + vec![SpinKeyValueStore::new(Some(tmp_dir.path().to_owned()))], vec!["custom"], ) .await?; - assert!(tmp_dir.path().exists()); + + // Actually et a key since store creation is lazy + let store = state.key_value.open("custom".to_owned()).await??; + let _ = state.key_value.get(store, "foo".to_owned()).await??; + + // Check that the correct store in the config was chosen by verifying the existence of the DB + assert!(db_path.exists()); Ok(()) } @@ -166,64 +169,75 @@ async fn custom_redis_key_value_works() -> anyhow::Result<()> { vec![RedisKeyValueStore::new()], vec!["custom"], ) - .await + .await?; + Ok(()) } #[tokio::test] async fn misconfigured_spin_key_value_fails() -> anyhow::Result<()> { + let tmp_dir = tempdir::TempDir::new("example")?; let runtime_config = toml::toml! { [key_value_store.custom] type = "spin" path = "/$$&/bad/path/foo.db" }; - assert!(run_test_with_config_and_stores_for_label( + let result = run_test_with_config_and_stores_for_label( Some(runtime_config), - vec![SpinKeyValueStore::new( - std::env::current_dir().context("failed to get current directory")? - )], - vec!["custom"] + vec![SpinKeyValueStore::new(Some(tmp_dir.path().to_owned()))], + vec!["custom"], ) - .await - .is_err()); + .await; + // TODO(rylev): This only fails on my machine due to a read-only file system error. + // We should consider adding a check for the error message. + assert!(result.is_err()); Ok(()) } -#[tokio::test] -async fn multiple_custom_key_value_uses_first_store() -> anyhow::Result<()> { +#[tokio::test(flavor = "multi_thread", worker_threads = 1)] +// TODO(rylev): consider removing this test as it is really only a consequence of +// toml deserialization and not a feature of the key-value store. +async fn multiple_custom_key_value_uses_second_store() -> anyhow::Result<()> { let tmp_dir = tempdir::TempDir::new("example")?; + let db_path = tmp_dir.path().join("custom.db"); + // Check that the db does not exist yet - it will exist by the end of the test + assert!(!db_path.exists()); + let mut test_resolver = RuntimeConfigResolver::new(); test_resolver.register_store_type(RedisKeyValueStore::new())?; - test_resolver.register_store_type(SpinKeyValueStore::new(tmp_dir.path().to_owned()))?; + test_resolver.register_store_type(SpinKeyValueStore::new(Some(tmp_dir.path().to_owned())))?; let test_resolver = Arc::new(test_resolver); let factors = TestFactors { key_value: KeyValueFactor::new(test_resolver.clone()), }; + let runtime_config = toml::toml! { + [key_value_store.custom] + type = "redis" + url = "redis://localhost:6379" + + [key_value_store.custom] + type = "spin" + path = "custom.db" + + }; let env = TestEnvironment::new(factors) .extend_manifest(toml! { [component.test-component] source = "does-not-exist.wasm" key_value_stores = ["custom"] }) - .runtime_config(TomlConfig::new( - test_resolver, - Some(toml::toml! { - [key_value_store.custom] - type = "spin" - path = "custom.db" + .runtime_config(TomlConfig::new(test_resolver, Some(runtime_config)))?; + let mut state = env.build_instance_state().await?; - [key_value_store.custom] - type = "redis" - url = "redis://localhost:6379" - }), - ))?; - let state = env.build_instance_state().await?; + // Actually et a key since store creation is lazy + let store = state.key_value.open("custom".to_owned()).await??; + let _ = state.key_value.get(store, "foo".to_owned()).await??; assert_eq!( state.key_value.allowed_stores(), &["custom".into()].into_iter().collect::>() ); - // Check that the first store in the config was chosen by verifying the existence of the DB directory - assert!(tmp_dir.path().exists()); + // Check that the correct store in the config was chosen by verifying the existence of the DB + assert!(db_path.exists()); Ok(()) } diff --git a/crates/factor-sqlite/src/runtime_config/spin.rs b/crates/factor-sqlite/src/runtime_config/spin.rs index a98387df1..6274fd97d 100644 --- a/crates/factor-sqlite/src/runtime_config/spin.rs +++ b/crates/factor-sqlite/src/runtime_config/spin.rs @@ -20,7 +20,7 @@ use crate::{Connection, ConnectionCreator, DefaultLabelResolver}; /// This type implements how Spin CLI's SQLite implementation is configured /// through the runtime config toml as well as the behavior of the "default" label. pub struct RuntimeConfigResolver { - default_database_dir: PathBuf, + default_database_dir: Option, local_database_dir: PathBuf, } @@ -29,11 +29,12 @@ impl RuntimeConfigResolver { /// /// This takes as arguments: /// * the directory to use as the default location for SQLite databases. - /// Usually this will be the path to the `.spin` state directory. + /// Usually this will be the path to the `.spin` state directory. If + /// `None`, the default database will be in-memory. /// * the path to the directory from which relative paths to /// local SQLite databases are resolved. (this should most likely be the /// path to the runtime-config file or the current working dir). - pub fn new(default_database_dir: PathBuf, local_database_dir: PathBuf) -> Self { + pub fn new(default_database_dir: Option, local_database_dir: PathBuf) -> Self { Self { default_database_dir, local_database_dir, @@ -102,9 +103,15 @@ impl DefaultLabelResolver for RuntimeConfigResolver { return None; } - let path = self.default_database_dir.join(DEFAULT_SQLITE_DB_FILENAME); + let path = self + .default_database_dir + .as_deref() + .map(|p| p.join(DEFAULT_SQLITE_DB_FILENAME)); let factory = move || { - let location = spin_sqlite_inproc::InProcDatabaseLocation::Path(path.clone()); + let location = match &path { + Some(path) => spin_sqlite_inproc::InProcDatabaseLocation::Path(path.clone()), + None => spin_sqlite_inproc::InProcDatabaseLocation::InMemory, + }; let connection = spin_sqlite_inproc::InProcConnection::new(location)?; Ok(Box::new(connection) as _) }; diff --git a/crates/factor-sqlite/tests/factor_test.rs b/crates/factor-sqlite/tests/factor_test.rs index e2e2ef213..b668bf343 100644 --- a/crates/factor-sqlite/tests/factor_test.rs +++ b/crates/factor-sqlite/tests/factor_test.rs @@ -66,7 +66,7 @@ async fn no_error_when_database_is_configured() -> anyhow::Result<()> { [sqlite_database.foo] type = "spin" }; - let sqlite_config = RuntimeConfigResolver::new("/".into(), "/".into()); + let sqlite_config = RuntimeConfigResolver::new(None, "/".into()); let env = TestEnvironment::new(factors) .extend_manifest(toml! { [component.test-component] diff --git a/crates/factors/tests/smoke.rs b/crates/factors/tests/smoke.rs index 3b31c701d..95e77d7b6 100644 --- a/crates/factors/tests/smoke.rs +++ b/crates/factors/tests/smoke.rs @@ -1,14 +1,11 @@ -use std::{path::PathBuf, sync::Arc}; +use std::sync::Arc; use anyhow::Context; use http_body_util::BodyExt; use spin_app::App; -use spin_factor_key_value::{ - runtime_config::spin::{MakeKeyValueStore, RuntimeConfigResolver, StoreConfig}, - KeyValueFactor, -}; +use spin_factor_key_value::{runtime_config::spin::RuntimeConfigResolver, KeyValueFactor}; use spin_factor_key_value_redis::RedisKeyValueStore; -use spin_factor_key_value_spin::{SpinKeyValueRuntimeConfig, SpinKeyValueStore}; +use spin_factor_key_value_spin::SpinKeyValueStore; use spin_factor_outbound_http::OutboundHttpFactor; use spin_factor_outbound_networking::OutboundNetworkingFactor; use spin_factor_variables::VariablesFactor; @@ -42,18 +39,10 @@ impl AsInstanceState for Data { #[tokio::test(flavor = "multi_thread")] async fn smoke_test_works() -> anyhow::Result<()> { let mut key_value_resolver = RuntimeConfigResolver::default(); - let default_config = - SpinKeyValueRuntimeConfig::default(Some(PathBuf::from("tests/smoke-app/.spin"))); - key_value_resolver.add_default_store( - "default", - StoreConfig { - type_: SpinKeyValueStore::RUNTIME_CONFIG_TYPE.to_string(), - config: toml::value::Table::try_from(default_config)?, - }, - ); - key_value_resolver.register_store_type(SpinKeyValueStore::new( + key_value_resolver.add_default_store::("default", Default::default())?; + key_value_resolver.register_store_type(SpinKeyValueStore::new(Some( std::env::current_dir().context("failed to get current directory")?, - ))?; + )))?; key_value_resolver.register_store_type(RedisKeyValueStore::new())?; let key_value_resolver = Arc::new(key_value_resolver); diff --git a/crates/key-value-sqlite/src/lib.rs b/crates/key-value-sqlite/src/lib.rs index 337667b69..152da5ba3 100644 --- a/crates/key-value-sqlite/src/lib.rs +++ b/crates/key-value-sqlite/src/lib.rs @@ -10,6 +10,7 @@ use std::{ use tokio::task; use tracing::{instrument, Level}; +#[derive(Clone, Debug)] pub enum DatabaseLocation { InMemory, Path(PathBuf), diff --git a/crates/runtime-config/src/lib.rs b/crates/runtime-config/src/lib.rs index cc5e07811..801b60417 100644 --- a/crates/runtime-config/src/lib.rs +++ b/crates/runtime-config/src/lib.rs @@ -1,8 +1,9 @@ use std::path::{Path, PathBuf}; use anyhow::Context as _; -use spin_factor_key_value::runtime_config::spin::{self as key_value, MakeKeyValueStore}; +use spin_factor_key_value::runtime_config::spin::{self as key_value}; use spin_factor_key_value::{DefaultLabelResolver as _, KeyValueFactor}; +use spin_factor_key_value_spin::SpinKeyValueStore; use spin_factor_llm::{spin as llm, LlmFactor}; use spin_factor_outbound_http::OutboundHttpFactor; use spin_factor_outbound_mqtt::OutboundMqttFactor; @@ -46,10 +47,9 @@ where use_gpu: bool, ) -> anyhow::Result { let tls_resolver = SpinTlsRuntimeConfig::new(runtime_config_path); - let state_dir = PathBuf::from(state_dir.unwrap_or(DEFAULT_STATE_DIR)); - let key_value_config_resolver = key_value_config_resolver(state_dir.clone()); + let key_value_config_resolver = key_value_config_resolver(state_dir.map(Into::into)); - let sqlite_config_resolver = sqlite_config_resolver(state_dir.clone()) + let sqlite_config_resolver = sqlite_config_resolver(state_dir.map(Into::into)) .context("failed to resolve sqlite runtime config")?; let file = std::fs::read_to_string(runtime_config_path).with_context(|| { @@ -66,7 +66,7 @@ where })?; let runtime_config: T = TomlRuntimeConfigSource::new( &toml, - state_dir, + state_dir.unwrap_or(DEFAULT_STATE_DIR).into(), &key_value_config_resolver, &tls_resolver, &sqlite_config_resolver, @@ -106,11 +106,11 @@ where impl ResolvedRuntimeConfig { pub fn default(state_dir: Option<&str>) -> Self { - let state_dir = state_dir.unwrap_or(DEFAULT_STATE_DIR); + let state_dir = state_dir.map(PathBuf::from); Self { - sqlite_resolver: sqlite_config_resolver(PathBuf::from(state_dir)) + sqlite_resolver: sqlite_config_resolver(state_dir.clone()) .expect("failed to resolve sqlite runtime config"), - key_value_resolver: key_value_config_resolver(PathBuf::from(state_dir)), + key_value_resolver: key_value_config_resolver(state_dir), runtime_config: Default::default(), } } @@ -227,14 +227,13 @@ impl RuntimeConfigSourceFinalizer for TomlRuntimeConfigSource<'_> { } } -const DEFAULT_KEY_VALUE_STORE_FILENAME: &str = "sqlite_key_value.db"; const DEFAULT_KEY_VALUE_STORE_LABEL: &str = "default"; /// The key-value runtime configuration resolver. /// /// Takes a base path for the local store. pub fn key_value_config_resolver( - local_store_base_path: PathBuf, + local_store_base_path: Option, ) -> key_value::RuntimeConfigResolver { let mut key_value = key_value::RuntimeConfigResolver::new(); @@ -242,7 +241,7 @@ pub fn key_value_config_resolver( // Unwraps are safe because the store types are known to not overlap. key_value .register_store_type(spin_factor_key_value_spin::SpinKeyValueStore::new( - local_store_base_path, + local_store_base_path.clone(), )) .unwrap(); key_value @@ -253,27 +252,25 @@ pub fn key_value_config_resolver( .unwrap(); // Add handling of "default" store. - key_value.add_default_store( - DEFAULT_KEY_VALUE_STORE_LABEL, - key_value::StoreConfig { - type_: spin_factor_key_value_spin::SpinKeyValueStore::RUNTIME_CONFIG_TYPE.to_owned(), - config: toml::toml! { - path = DEFAULT_KEY_VALUE_STORE_FILENAME - }, - }, - ); + // Unwraps are safe because the store is known to be serializable as toml. + key_value + .add_default_store::(DEFAULT_KEY_VALUE_STORE_LABEL, Default::default()) + .unwrap(); key_value } /// The sqlite runtime configuration resolver. /// -/// Takes a base path to the state directory. -fn sqlite_config_resolver(state_dir: PathBuf) -> anyhow::Result { +/// Takes a path to the directory where the default database should be stored. +/// If the path is `None`, the default database will be in-memory. +fn sqlite_config_resolver( + default_database_dir: Option, +) -> anyhow::Result { let local_database_dir = std::env::current_dir().context("failed to get current working directory")?; Ok(sqlite::RuntimeConfigResolver::new( - state_dir, + default_database_dir, local_database_dir, )) }