diff --git a/Cargo.lock b/Cargo.lock index 0a8fee1247..4f95182600 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7794,11 +7794,15 @@ dependencies = [ "spin-factor-variables", "spin-factor-wasi", "spin-factors", + "spin-factors-test", "spin-key-value-azure", "spin-key-value-redis", "spin-key-value-spin", "spin-sqlite", "spin-trigger", + "spin-world", + "tempfile", + "tokio", "toml 0.8.14", ] diff --git a/crates/factor-key-value/Cargo.toml b/crates/factor-key-value/Cargo.toml index f9b9e4c9d2..626fe85c99 100644 --- a/crates/factor-key-value/Cargo.toml +++ b/crates/factor-key-value/Cargo.toml @@ -19,8 +19,8 @@ tracing = { workspace = true } [dev-dependencies] spin-factors-test = { path = "../factors-test" } -spin-key-value-redis = { path = "../key-value-redis" } spin-key-value-spin = { path = "../key-value-spin" } +spin-key-value-redis = { path = "../key-value-redis" } tempfile = "3.12.0" tokio = { version = "1", features = ["macros", "rt"] } diff --git a/crates/factor-key-value/src/host.rs b/crates/factor-key-value/src/host.rs index adb1d00c15..3a28c115af 100644 --- a/crates/factor-key-value/src/host.rs +++ b/crates/factor-key-value/src/host.rs @@ -1,4 +1,3 @@ -use crate::util::EmptyStoreManager; use anyhow::{Context, Result}; use spin_core::{async_trait, wasmtime::component::Resource}; use spin_world::v2::key_value; @@ -40,23 +39,22 @@ pub struct KeyValueDispatch { } impl KeyValueDispatch { - pub fn new() -> Self { - Self::new_with_capacity(DEFAULT_STORE_TABLE_CAPACITY) + pub fn new(allowed_stores: HashSet, manager: Arc) -> Self { + Self::new_with_capacity(allowed_stores, manager, DEFAULT_STORE_TABLE_CAPACITY) } - pub fn new_with_capacity(capacity: u32) -> Self { + pub fn new_with_capacity( + allowed_stores: HashSet, + manager: Arc, + capacity: u32, + ) -> Self { Self { - allowed_stores: HashSet::new(), - manager: Arc::new(EmptyStoreManager), + allowed_stores, + manager, stores: Table::new(capacity), } } - pub fn init(&mut self, allowed_stores: HashSet, manager: Arc) { - self.allowed_stores = allowed_stores; - self.manager = manager; - } - pub fn get_store(&self, store: Resource) -> anyhow::Result<&Arc> { self.stores.get(store.rep()).context("invalid store") } @@ -66,12 +64,6 @@ impl KeyValueDispatch { } } -impl Default for KeyValueDispatch { - fn default() -> Self { - Self::new() - } -} - #[async_trait] impl key_value::Host for KeyValueDispatch {} diff --git a/crates/factor-key-value/src/lib.rs b/crates/factor-key-value/src/lib.rs index 28696712ed..685766b51c 100644 --- a/crates/factor-key-value/src/lib.rs +++ b/crates/factor-key-value/src/lib.rs @@ -12,7 +12,6 @@ use spin_factors::{ ConfigureAppContext, Factor, FactorInstanceBuilder, InitContext, PrepareContext, RuntimeFactors, }; use spin_locked_app::MetadataKey; -use util::DefaultManagerGetter; /// Metadata key for key-value stores. pub const KEY_VALUE_STORES_KEY: MetadataKey> = MetadataKey::new("key_value_stores"); @@ -21,19 +20,15 @@ pub use runtime_config::RuntimeConfig; pub use util::{CachingStoreManager, DelegatingStoreManager}; /// A factor that provides key-value storage. +#[derive(Default)] pub struct KeyValueFactor { - default_label_resolver: Arc, + _priv: (), } impl KeyValueFactor { /// Create a new KeyValueFactor. - /// - /// The `default_label_resolver` is used to resolve store managers for labels that - /// are not defined in the runtime configuration. - pub fn new(default_label_resolver: impl DefaultLabelResolver + 'static) -> Self { - Self { - default_label_resolver: Arc::new(default_label_resolver), - } + pub fn new() -> Self { + Self { _priv: () } } } @@ -53,13 +48,10 @@ impl Factor for KeyValueFactor { mut ctx: ConfigureAppContext, ) -> anyhow::Result { let store_managers = ctx.take_runtime_config().unwrap_or_default(); - let default_label_resolver = self.default_label_resolver.clone(); - let default_fn: DefaultManagerGetter = - Arc::new(move |label| default_label_resolver.default(label)); - let delegating_manager = DelegatingStoreManager::new(store_managers, default_fn); + let delegating_manager = DelegatingStoreManager::new(store_managers); let caching_manager = CachingStoreManager::new(delegating_manager); - let store_manager_manager = Arc::new(caching_manager); + let store_manager = Arc::new(caching_manager); // Build component -> allowed stores map let mut component_allowed_stores = HashMap::new(); @@ -73,8 +65,7 @@ impl Factor for KeyValueFactor { for label in &key_value_stores { // TODO: port nicer errors from KeyValueComponent (via error type?) ensure!( - store_manager_manager.is_defined(label) - || self.default_label_resolver.default(label).is_some(), + store_manager.is_defined(label), "unknown key_value_stores label {label:?} for component {component_id:?}" ); } @@ -83,7 +74,7 @@ impl Factor for KeyValueFactor { } Ok(AppState { - store_manager: store_manager_manager, + store_manager, component_allowed_stores, }) } @@ -159,22 +150,10 @@ impl FactorInstanceBuilder for InstanceBuilder { store_manager, allowed_stores, } = self; - let mut dispatch = KeyValueDispatch::new_with_capacity(u32::MAX); - dispatch.init(allowed_stores, store_manager); - Ok(dispatch) - } -} - -/// Resolves a label to a default [`StoreManager`]. -pub trait DefaultLabelResolver: Send + Sync { - /// If there is no runtime configuration for a given store label, return a default store manager. - /// - /// If `Option::None` is returned, the store is not allowed. - fn default(&self, label: &str) -> Option>; -} - -impl DefaultLabelResolver for Arc { - fn default(&self, label: &str) -> Option> { - self.as_ref().default(label) + Ok(KeyValueDispatch::new_with_capacity( + allowed_stores, + store_manager, + u32::MAX, + )) } } diff --git a/crates/factor-key-value/src/runtime_config.rs b/crates/factor-key-value/src/runtime_config.rs index 76e06da9d9..70b99bb568 100644 --- a/crates/factor-key-value/src/runtime_config.rs +++ b/crates/factor-key-value/src/runtime_config.rs @@ -18,6 +18,16 @@ impl RuntimeConfig { pub fn add_store_manager(&mut self, label: String, store_manager: Arc) { self.store_managers.insert(label, store_manager); } + + /// Returns whether a store manager exists for the store with the given label. + pub fn has_store_manager(&self, label: &str) -> bool { + self.store_managers.contains_key(label) + } + + /// Returns the store manager for the store with the given label. + pub fn get_store_manager(&self, label: &str) -> Option> { + self.store_managers.get(label).cloned() + } } impl IntoIterator for RuntimeConfig { diff --git a/crates/factor-key-value/src/runtime_config/spin.rs b/crates/factor-key-value/src/runtime_config/spin.rs index cae8b6eb42..0a444b4403 100644 --- a/crates/factor-key-value/src/runtime_config/spin.rs +++ b/crates/factor-key-value/src/runtime_config/spin.rs @@ -1,7 +1,6 @@ //! Runtime configuration implementation used by Spin CLI. -use crate::StoreManager; -use crate::{DefaultLabelResolver, RuntimeConfig}; +use crate::{RuntimeConfig, StoreManager}; use anyhow::Context as _; use serde::de::DeserializeOwned; use serde::{Deserialize, Serialize}; @@ -99,7 +98,25 @@ impl RuntimeConfigResolver { } /// Resolves a toml table into a runtime config. - pub fn resolve_from_toml( + /// + /// The default stores are also added to the runtime config. + pub fn resolve(&self, table: Option<&impl GetTomlValue>) -> anyhow::Result { + let mut runtime_config = self.resolve_from_toml(table)?.unwrap_or_default(); + + for (&label, config) in &self.defaults { + if !runtime_config.store_managers.contains_key(label) { + let store_manager = self + .store_manager_from_config(config.clone()) + .with_context(|| { + format!("could not configure key-value store with label '{label}'") + })?; + runtime_config.add_store_manager(label.to_owned(), store_manager); + } + } + Ok(runtime_config) + } + + fn resolve_from_toml( &self, table: Option<&impl GetTomlValue>, ) -> anyhow::Result> { @@ -115,6 +132,7 @@ impl RuntimeConfigResolver { })?; runtime_config.add_store_manager(label.clone(), store_manager); } + Ok(Some(runtime_config)) } @@ -134,15 +152,6 @@ impl RuntimeConfigResolver { } } -impl DefaultLabelResolver for RuntimeConfigResolver { - fn default(&self, label: &str) -> Option> { - let config = self.defaults.get(label)?; - // TODO(rylev): The unwrap here is not ideal. We should return a Result instead. - // Piping that through `DefaultLabelResolver` is a bit awkward, though. - Some(self.store_manager_from_config(config.clone()).unwrap()) - } -} - #[derive(Deserialize, Clone)] pub struct StoreConfig { #[serde(rename = "type")] diff --git a/crates/factor-key-value/src/util.rs b/crates/factor-key-value/src/util.rs index 9ba6ad8edd..57c4968011 100644 --- a/crates/factor-key-value/src/util.rs +++ b/crates/factor-key-value/src/util.rs @@ -13,39 +13,15 @@ use tokio::{ }; use tracing::Instrument; -const DEFAULT_CACHE_SIZE: usize = 256; - -pub struct EmptyStoreManager; - -#[async_trait] -impl StoreManager for EmptyStoreManager { - async fn get(&self, _name: &str) -> Result, Error> { - Err(Error::NoSuchStore) - } - - fn is_defined(&self, _store_name: &str) -> bool { - false - } -} - -/// A function that takes a store label and returns the default store manager, if one exists. -pub type DefaultManagerGetter = Arc Option> + Send + Sync>; - +/// A [`StoreManager`] which delegates to other `StoreManager`s based on the store label. pub struct DelegatingStoreManager { delegates: HashMap>, - default_manager: DefaultManagerGetter, } impl DelegatingStoreManager { - pub fn new( - delegates: impl IntoIterator)>, - default_manager: DefaultManagerGetter, - ) -> Self { + pub fn new(delegates: impl IntoIterator)>) -> Self { let delegates = delegates.into_iter().collect(); - Self { - delegates, - default_manager, - } + Self { delegates } } } @@ -54,12 +30,7 @@ impl StoreManager for DelegatingStoreManager { async fn get(&self, name: &str) -> Result, Error> { match self.delegates.get(name) { Some(store) => store.get(name).await, - None => { - (self.default_manager)(name) - .ok_or(Error::NoSuchStore)? - .get(name) - .await - } + None => Err(Error::NoSuchStore), } } @@ -71,7 +42,7 @@ impl StoreManager for DelegatingStoreManager { if let Some(store) = self.delegates.get(store_name) { return store.summary(store_name); } - (self.default_manager)(store_name)?.summary(store_name) + None } } @@ -104,6 +75,8 @@ pub struct CachingStoreManager { inner: T, } +const DEFAULT_CACHE_SIZE: usize = 256; + impl CachingStoreManager { pub fn new(inner: T) -> Self { Self::new_with_capacity(NonZeroUsize::new(DEFAULT_CACHE_SIZE).unwrap(), inner) diff --git a/crates/factor-key-value/tests/factor_test.rs b/crates/factor-key-value/tests/factor_test.rs index bdccc89638..83a67700d7 100644 --- a/crates/factor-key-value/tests/factor_test.rs +++ b/crates/factor-key-value/tests/factor_test.rs @@ -1,13 +1,9 @@ -use anyhow::Context as _; -use spin_factor_key_value::{ - runtime_config::spin::{MakeKeyValueStore, RuntimeConfigResolver}, - KeyValueFactor, RuntimeConfig, -}; -use spin_factors::{FactorRuntimeConfigSource, RuntimeConfigSourceFinalizer, RuntimeFactors}; +use anyhow::bail; +use spin_core::async_trait; +use spin_factor_key_value::{KeyValueFactor, RuntimeConfig, Store, StoreManager}; +use spin_factors::RuntimeFactors; use spin_factors_test::{toml, TestEnvironment}; -use spin_key_value_redis::RedisKeyValueStore; -use spin_key_value_spin::{SpinKeyValueRuntimeConfig, SpinKeyValueStore}; -use spin_world::v2::key_value::HostStore; +use spin_world::v2::key_value::{Error, HostStore}; use std::{collections::HashSet, sync::Arc}; #[derive(RuntimeFactors)] @@ -15,260 +11,133 @@ struct TestFactors { key_value: KeyValueFactor, } +impl From for TestFactorsRuntimeConfig { + fn from(value: RuntimeConfig) -> Self { + Self { + key_value: Some(value), + } + } +} + #[tokio::test] -async fn default_key_value_works() -> anyhow::Result<()> { - let mut test_resolver = RuntimeConfigResolver::new(); - test_resolver.register_store_type(SpinKeyValueStore::new(None))?; - test_resolver - .add_default_store::("default", SpinKeyValueRuntimeConfig::new(None))?; +async fn works_when_allowed_store_is_defined() -> anyhow::Result<()> { + let mut runtime_config = RuntimeConfig::default(); + runtime_config.add_store_manager("default".into(), mock_store_manager()); let factors = TestFactors { - key_value: KeyValueFactor::new(test_resolver), + key_value: KeyValueFactor::new(), }; let env = TestEnvironment::new(factors).extend_manifest(toml! { [component.test-component] source = "does-not-exist.wasm" key_value_stores = ["default"] }); - let state = env.build_instance_state().await?; + let mut state = env + .runtime_config(runtime_config)? + .build_instance_state() + .await?; assert_eq!( state.key_value.allowed_stores(), &["default".into()].into_iter().collect::>() ); - Ok(()) -} -async fn run_test_with_config_and_stores_for_label( - runtime_config: Option, - store_types: Vec, - labels: Vec<&str>, -) -> anyhow::Result { - let mut test_resolver = RuntimeConfigResolver::new(); - for store_type in store_types { - test_resolver.register_store_type(store_type)?; - } - let test_resolver = Arc::new(test_resolver); - let factors = TestFactors { - key_value: KeyValueFactor::new(test_resolver.clone()), - }; - let labels_clone = labels.clone(); - let env = TestEnvironment::new(factors) - .extend_manifest(toml! { - [component.test-component] - source = "does-not-exist.wasm" - key_value_stores = labels_clone - }) - .runtime_config(TomlConfig::new(test_resolver, runtime_config))?; - let state = env.build_instance_state().await?; - assert_eq!( - labels, - state.key_value.allowed_stores().iter().collect::>() - ); - - Ok(state) -} - -#[tokio::test] -async fn overridden_default_key_value_works() -> anyhow::Result<()> { - let runtime_config = toml::toml! { - [key_value_store.default] - type = "redis" - url = "redis://localhost:6379" - }; - run_test_with_config_and_stores_for_label( - Some(runtime_config), - vec![RedisKeyValueStore::new()], - vec!["default"], - ) - .await?; + assert!(state.key_value.open("default".to_owned()).await?.is_ok()); Ok(()) } #[tokio::test] -async fn custom_spin_key_value_works() -> anyhow::Result<()> { - let runtime_config = toml::toml! { - [key_value_store.custom] - type = "spin" - }; - run_test_with_config_and_stores_for_label( - Some(runtime_config), - vec![SpinKeyValueStore::new(None)], - vec!["custom"], - ) - .await?; - Ok(()) -} - -#[tokio::test(flavor = "multi_thread", worker_threads = 1)] -async fn custom_spin_key_value_works_with_absolute_path() -> anyhow::Result<()> { - let tmp_dir = tempfile::TempDir::with_prefix("example")?; - 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 +async fn errors_when_store_is_not_defined() -> anyhow::Result<()> { + let runtime_config = RuntimeConfig::default(); + let factors = TestFactors { + key_value: KeyValueFactor::new(), }; - let mut state = run_test_with_config_and_stores_for_label( - Some(runtime_config), - vec![SpinKeyValueStore::new(Some( - std::env::current_dir().context("failed to get current directory")?, - ))], - vec!["custom"], - ) - .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??; - - // Check that the parent has been created - assert!(db_path.exists()); - Ok(()) -} - -#[tokio::test(flavor = "multi_thread", worker_threads = 1)] -async fn custom_spin_key_value_works_with_relative_path() -> anyhow::Result<()> { - let tmp_dir = tempfile::TempDir::with_prefix("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 runtime_config = toml::toml! { - [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 = ["default"] + }); + let Err(err) = env + .runtime_config(runtime_config)? + .build_instance_state() + .await + else { + bail!("expected instance build to fail but it didn't"); }; - let mut state = run_test_with_config_and_stores_for_label( - Some(runtime_config), - vec![SpinKeyValueStore::new(Some(tmp_dir.path().to_owned()))], - vec!["custom"], - ) - .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??; - - // Check that the correct store in the config was chosen by verifying the existence of the DB - assert!(db_path.exists()); - Ok(()) -} + assert!(err + .to_string() + .contains(r#"unknown key_value_stores label "default""#)); -#[tokio::test] -async fn custom_redis_key_value_works() -> anyhow::Result<()> { - let runtime_config = toml::toml! { - [key_value_store.custom] - type = "redis" - url = "redis://localhost:6379" - }; - run_test_with_config_and_stores_for_label( - Some(runtime_config), - vec![RedisKeyValueStore::new()], - vec!["custom"], - ) - .await?; Ok(()) } #[tokio::test] -async fn misconfigured_spin_key_value_fails() -> anyhow::Result<()> { - let tmp_dir = tempfile::TempDir::with_prefix("example")?; - let runtime_config = toml::toml! { - [key_value_store.custom] - type = "spin" - path = "/$$&/bad/path/foo.db" - }; - let result = run_test_with_config_and_stores_for_label( - Some(runtime_config), - vec![SpinKeyValueStore::new(Some(tmp_dir.path().to_owned()))], - vec!["custom"], - ) - .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(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 = tempfile::TempDir::with_prefix("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(Some(tmp_dir.path().to_owned())))?; - let test_resolver = Arc::new(test_resolver); +async fn errors_when_store_is_not_allowed() -> anyhow::Result<()> { + let mut runtime_config = RuntimeConfig::default(); + runtime_config.add_store_manager("default".into(), mock_store_manager()); let factors = TestFactors { - key_value: KeyValueFactor::new(test_resolver.clone()), + key_value: KeyValueFactor::new(), }; - 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 = [] + }); + let mut state = env + .runtime_config(runtime_config)? + .build_instance_state() + .await?; - }; - 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(runtime_config)))?; - let mut state = env.build_instance_state().await?; + assert_eq!(state.key_value.allowed_stores(), &HashSet::new()); - // 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!(matches!( + state.key_value.open("default".to_owned()).await?, + Err(Error::AccessDenied) + )); - assert_eq!( - state.key_value.allowed_stores(), - &["custom".into()].into_iter().collect::>() - ); - // Check that the correct store in the config was chosen by verifying the existence of the DB - assert!(db_path.exists()); Ok(()) } -struct TomlConfig { - resolver: Arc, - toml: Option, +fn mock_store_manager() -> Arc { + Arc::new(MockStoreManager) } -impl TomlConfig { - fn new(resolver: Arc, toml: Option) -> Self { - Self { resolver, toml } - } -} +struct MockStoreManager; -impl TryFrom for TestFactorsRuntimeConfig { - type Error = anyhow::Error; - - fn try_from(value: TomlConfig) -> Result { - Self::from_source(value) +#[async_trait] +impl StoreManager for MockStoreManager { + async fn get(&self, name: &str) -> Result, Error> { + let _ = name; + Ok(Arc::new(MockStore)) } -} -impl FactorRuntimeConfigSource for TomlConfig { - fn get_runtime_config(&mut self) -> anyhow::Result> { - self.resolver.resolve_from_toml(self.toml.as_ref()) + fn is_defined(&self, store_name: &str) -> bool { + let _ = store_name; + todo!() } } -impl RuntimeConfigSourceFinalizer for TomlConfig { - fn finalize(&mut self) -> anyhow::Result<()> { - Ok(()) +struct MockStore; + +#[async_trait] +impl Store for MockStore { + async fn get(&self, key: &str) -> Result>, Error> { + let _ = key; + todo!() + } + async fn set(&self, key: &str, value: &[u8]) -> Result<(), Error> { + let _ = (key, value); + todo!() + } + async fn delete(&self, key: &str) -> Result<(), Error> { + let _ = key; + todo!() + } + async fn exists(&self, key: &str) -> Result { + let _ = key; + todo!() + } + async fn get_keys(&self) -> Result, Error> { + todo!() } } diff --git a/crates/key-value-spin/src/store.rs b/crates/key-value-spin/src/store.rs index 3bdf795778..7fb17fa0e5 100644 --- a/crates/key-value-spin/src/store.rs +++ b/crates/key-value-spin/src/store.rs @@ -159,19 +159,15 @@ mod test { #[tokio::test(flavor = "multi_thread", worker_threads = 1)] async fn all() -> Result<()> { - let mut kv = KeyValueDispatch::new(); - kv.init( + let mut kv = KeyValueDispatch::new( ["default", "foo"] .into_iter() .map(ToOwned::to_owned) .collect(), - Arc::new(DelegatingStoreManager::new( - [( - "default".to_owned(), - Arc::new(KeyValueSqlite::new(DatabaseLocation::InMemory)) as _, - )], - Arc::new(|_: &str| -> Option> { None }), - )), + Arc::new(DelegatingStoreManager::new([( + "default".to_owned(), + Arc::new(KeyValueSqlite::new(DatabaseLocation::InMemory)) as _, + )])), ); assert!(matches!( diff --git a/crates/runtime-config/Cargo.toml b/crates/runtime-config/Cargo.toml index e8230cff8f..072e8477e6 100644 --- a/crates/runtime-config/Cargo.toml +++ b/crates/runtime-config/Cargo.toml @@ -30,5 +30,12 @@ spin-sqlite = { path = "../sqlite" } spin-trigger = { path = "../trigger" } toml = "0.8" +[dev-dependencies] +spin-factors-test = { path = "../factors-test" } +spin-world = { path = "../world" } +tokio = { version = "1", features = ["macros"] } +tempfile = "3.2" +toml = "0.8" + [lints] workspace = true diff --git a/crates/runtime-config/src/lib.rs b/crates/runtime-config/src/lib.rs index 5b2ae380d9..0605883ab1 100644 --- a/crates/runtime-config/src/lib.rs +++ b/crates/runtime-config/src/lib.rs @@ -300,7 +300,7 @@ impl FactorRuntimeConfigSource for TomlRuntimeConfigSource<'_, ' fn get_runtime_config( &mut self, ) -> anyhow::Result> { - self.key_value.resolve_from_toml(Some(&self.toml.table)) + Ok(Some(self.key_value.resolve(Some(&self.toml.table))?)) } } @@ -368,17 +368,7 @@ impl FactorRuntimeConfigSource for TomlRuntimeConfigSource<' impl FactorRuntimeConfigSource for TomlRuntimeConfigSource<'_, '_> { fn get_runtime_config(&mut self) -> anyhow::Result> { - let mut config = self - .sqlite - .resolve_from_toml(&self.toml.table)? - .unwrap_or_default(); - // If the user did not provide configuration for the default label, add it. - if !config.connection_creators.contains_key("default") { - config - .connection_creators - .insert("default".to_owned(), self.sqlite.default()); - } - Ok(Some(config)) + Ok(Some(self.sqlite.resolve(&self.toml.table)?)) } } @@ -448,32 +438,57 @@ fn sqlite_config_resolver( #[cfg(test)] mod tests { + use std::{collections::HashMap, sync::Arc}; + use spin_factors::RuntimeFactors; + use spin_factors_test::TestEnvironment; use super::*; + /// Define a test factor with the given field and factor type. + macro_rules! define_test_factor { + ($field:ident : $factor:ty) => { + #[derive(RuntimeFactors)] + struct TestFactors { + $field: $factor, + } + impl TryFrom> for TestFactorsRuntimeConfig { + type Error = anyhow::Error; + + fn try_from(value: TomlRuntimeConfigSource<'_, '_>) -> Result { + Self::from_source(value) + } + } + fn resolve_toml( + toml: toml::Table, + path: impl AsRef, + ) -> ResolvedRuntimeConfig { + ResolvedRuntimeConfig::::new( + toml_resolver(&toml), + Some(path.as_ref()), + false, + ) + .unwrap() + } + }; + } + #[test] fn sqlite_is_configured_correctly() { - #[derive(RuntimeFactors)] - struct Factors { - sqlite: SqliteFactor, - } - impl TryFrom> for FactorsRuntimeConfig { - type Error = anyhow::Error; - - fn try_from(value: TomlRuntimeConfigSource<'_, '_>) -> Result { - Self::from_source(value) + define_test_factor!(sqlite: SqliteFactor); + + impl TestFactorsRuntimeConfig { + /// Get the connection creators for the configured sqlite databases. + fn connection_creators( + &self, + ) -> &HashMap> { + &self.sqlite.as_ref().unwrap().connection_creators } - } - impl FactorsRuntimeConfig { /// Get the labels of the configured sqlite databases. fn configured_labels(&self) -> Vec<&str> { let mut configured_labels = self - .sqlite - .as_ref() - .unwrap() - .connection_creators + .connection_creators() .keys() .map(|s| s.as_str()) .collect::>(); @@ -488,20 +503,85 @@ mod tests { [sqlite_database.foo] type = "spin" }; - let config = - ResolvedRuntimeConfig::::new(toml_resolver(&toml), None, false) - .unwrap(); assert_eq!( - config.runtime_config.configured_labels(), + resolve_toml(toml, ".").runtime_config.configured_labels(), vec!["default", "foo"] ); // Test that the default label is added with an empty toml config. let toml = toml::Table::new(); - let config = - ResolvedRuntimeConfig::::new(toml_resolver(&toml), None, false) - .unwrap(); - assert_eq!(config.runtime_config.configured_labels(), vec!["default"]); + let runtime_config = resolve_toml(toml, "config.toml").runtime_config; + assert_eq!(runtime_config.configured_labels(), vec!["default"]); + } + + #[test] + fn key_value_is_configured_correctly() { + define_test_factor!(key_value: KeyValueFactor); + impl TestFactorsRuntimeConfig { + /// Get whether the store manager exists for the given label. + fn has_store_manager(&self, label: &str) -> bool { + self.key_value.as_ref().unwrap().has_store_manager(label) + } + } + + // Test that the default label is added if not provided. + let toml = toml::toml! { + [key_value_store.foo] + type = "spin" + }; + let runtime_config = resolve_toml(toml, "config.toml").runtime_config; + assert!(["default", "foo"] + .iter() + .all(|label| runtime_config.has_store_manager(label))); + } + + #[tokio::test(flavor = "multi_thread", worker_threads = 1)] + async fn custom_spin_key_value_works_with_custom_paths() -> anyhow::Result<()> { + use spin_world::v2::key_value::HostStore; + define_test_factor!(key_value: KeyValueFactor); + let tmp_dir = tempfile::TempDir::with_prefix("example")?; + let absolute_path = tmp_dir.path().join("foo/custom.db"); + let relative_path = tmp_dir.path().join("custom.db"); + // Check that the dbs do not exist yet - they will exist by the end of the test + assert!(!absolute_path.exists()); + assert!(!relative_path.exists()); + + let path_str = absolute_path.to_str().unwrap(); + let runtime_config = toml::toml! { + [key_value_store.absolute] + type = "spin" + path = path_str + + [key_value_store.relative] + type = "spin" + path = "custom.db" + }; + let factors = TestFactors { + key_value: KeyValueFactor::new(), + }; + let env = TestEnvironment::new(factors) + .extend_manifest(toml::toml! { + [component.test-component] + source = "does-not-exist.wasm" + key_value_stores = ["absolute", "relative"] + }) + .runtime_config( + resolve_toml(runtime_config, tmp_dir.path().join("runtime-config.toml")) + .runtime_config, + )?; + let mut state = env.build_instance_state().await?; + + // Actually get a key since store creation is lazy + let store = state.key_value.open("absolute".to_owned()).await??; + let _ = state.key_value.get(store, "foo".to_owned()).await??; + + let store = state.key_value.open("relative".to_owned()).await??; + let _ = state.key_value.get(store, "foo".to_owned()).await??; + + // Check that the dbs have been created + assert!(absolute_path.exists()); + assert!(relative_path.exists()); + Ok(()) } fn toml_resolver(toml: &toml::Table) -> TomlResolver<'_> { diff --git a/crates/runtime-factors/src/build.rs b/crates/runtime-factors/src/build.rs index f04a73cde8..15a32b578f 100644 --- a/crates/runtime-factors/src/build.rs +++ b/crates/runtime-factors/src/build.rs @@ -38,7 +38,6 @@ impl RuntimeFactorsBuilder for FactorsBuilder { runtime_config.state_dir(), config.working_dir.clone(), args.allow_transient_write, - runtime_config.key_value_resolver.clone(), use_gpu, ) .context("failed to create factors")?; diff --git a/crates/runtime-factors/src/lib.rs b/crates/runtime-factors/src/lib.rs index 1ee3a8959d..8c7bd97d7d 100644 --- a/crates/runtime-factors/src/lib.rs +++ b/crates/runtime-factors/src/lib.rs @@ -40,13 +40,12 @@ impl TriggerFactors { state_dir: Option, working_dir: impl Into, allow_transient_writes: bool, - default_key_value_label_resolver: impl spin_factor_key_value::DefaultLabelResolver + 'static, use_gpu: bool, ) -> anyhow::Result { Ok(Self { wasi: wasi_factor(working_dir, allow_transient_writes), variables: VariablesFactor::default(), - key_value: KeyValueFactor::new(default_key_value_label_resolver), + key_value: KeyValueFactor::new(), outbound_networking: outbound_networking_factor(), outbound_http: OutboundHttpFactor::default(), sqlite: SqliteFactor::new(), diff --git a/crates/sqlite/src/lib.rs b/crates/sqlite/src/lib.rs index ed5aad2375..3e687fc27f 100644 --- a/crates/sqlite/src/lib.rs +++ b/crates/sqlite/src/lib.rs @@ -1,6 +1,7 @@ //! Spin's default handling of the runtime configuration for SQLite databases. use std::{ + collections::HashMap, path::{Path, PathBuf}, sync::Arc, }; @@ -49,7 +50,25 @@ impl RuntimeConfigResolver { /// type = "$database-type" /// ... extra type specific configuration ... /// ``` - pub fn resolve_from_toml( + /// + /// Configuration is automatically added for the 'default' label if it is not provided. + pub fn resolve( + &self, + table: &impl GetTomlValue, + ) -> anyhow::Result { + let mut runtime_config = self.resolve_from_toml(table)?.unwrap_or_default(); + // If the user did not provide configuration for the default label, add it. + if !runtime_config.connection_creators.contains_key("default") { + runtime_config + .connection_creators + .insert("default".to_owned(), self.default()); + } + + Ok(runtime_config) + } + + /// Get the runtime configuration for SQLite databases from a TOML table. + fn resolve_from_toml( &self, table: &impl GetTomlValue, ) -> anyhow::Result> { @@ -61,7 +80,8 @@ impl RuntimeConfigResolver { let connection_creators = config .into_iter() .map(|(k, v)| Ok((k, self.get_connection_creator(v)?))) - .collect::>()?; + .collect::>>()?; + Ok(Some(spin_factor_sqlite::runtime_config::RuntimeConfig { connection_creators, }))