Fix path handling in sqlite and kv

Signed-off-by: Ryan Levick <[email protected]>

Author: rylev

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<PathBuf>,
 }
 
 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<PathBuf>) -> 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<PathBuf>) -> 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<Self::StoreManager> {
-        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)
+}

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<dyn StoreManager>) {
         self.store_managers.insert(label, store_manager);
     }

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<T>(
+        &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.

crates/factor-key-value/tests/factor_test.rs

@@ -1,38 +1,25 @@
-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)]
 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::<SpinKeyValueStore>("default", Default::default())?;
     let factors = TestFactors {
         key_value: KeyValueFactor::new(test_resolver),
     };
@@ -47,16 +34,14 @@ async fn default_key_value_works() -> anyhow::Result<()> {
         state.key_value.allowed_stores(),
         &["default".into()].into_iter().collect::<HashSet<_>>()
     );
-    // Ensure the database directory is created
-    assert!(dir.path().exists());
     Ok(())
 }
 
 async fn run_test_with_config_and_stores_for_label(
     runtime_config: Option<toml::Table>,
     store_types: Vec<impl MakeKeyValueStore>,
     labels: Vec<&str>,
-) -> anyhow::Result<()> {
+) -> anyhow::Result<TestFactorsInstanceState> {
     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::<Vec<_>>()
     );
 
-    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::<HashSet<_>>()
     );
-    // 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(())
 }