Make sure resolved state_dir is used everywhere

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

Author: rylev

crates/factor-key-value-spin/src/lib.rs

@@ -3,7 +3,7 @@ use std::{
     path::{Path, PathBuf},
 };
 
-use anyhow::Context;
+use anyhow::{bail, Context};
 use serde::{Deserialize, Serialize};
 use spin_factor_key_value::runtime_config::spin::MakeKeyValueStore;
 use spin_key_value_sqlite::{DatabaseLocation, KeyValueSqlite};
@@ -25,26 +25,6 @@ impl SpinKeyValueStore {
     }
 }
 
-/// Runtime configuration for the SQLite key-value store.
-#[derive(Deserialize, Serialize)]
-pub struct SpinKeyValueRuntimeConfig {
-    /// The path to the SQLite database file.
-    path: Option<PathBuf>,
-}
-
-impl SpinKeyValueRuntimeConfig {
-    /// The default filename for the SQLite database.
-    const DEFAULT_SPIN_STORE_FILENAME: &'static str = "sqlite_key_value.db";
-}
-
-impl Default for SpinKeyValueRuntimeConfig {
-    fn default() -> Self {
-        Self {
-            path: Some(PathBuf::from(Self::DEFAULT_SPIN_STORE_FILENAME)),
-        }
-    }
-}
-
 impl MakeKeyValueStore for SpinKeyValueStore {
     const RUNTIME_CONFIG_TYPE: &'static str = "spin";
 
@@ -56,24 +36,50 @@ impl MakeKeyValueStore for SpinKeyValueStore {
         &self,
         runtime_config: Self::RuntimeConfig,
     ) -> anyhow::Result<Self::StoreManager> {
-        // 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 location = match (&self.base_path, &runtime_config.path) {
+            // If both the base path and the path are specified, resolve the path against the base path
+            (Some(base_path), Some(path)) => {
                 let path = resolve_relative_path(path, base_path);
-                // Create the store's parent directory if necessary
-                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)
-            } else {
-                DatabaseLocation::InMemory
-            };
+            }
+            // If the base path is `None` but path is an absolute path, use the absolute path
+            (None, Some(path)) if path.is_absolute() => DatabaseLocation::Path(path.clone()),
+            // If the base path is `None` but path is a relative path, error out
+            (None, Some(path)) => {
+                bail!(
+                    "key-value store path '{}' is relative, but no base path is set",
+                    path.display()
+                )
+            }
+            // Otherwise, use an in-memory database
+            (None | Some(_), None) => DatabaseLocation::InMemory,
+        };
+        if let DatabaseLocation::Path(path) = &location {
+            // Create the store's parent directory if necessary
+            if let Some(parent) = path.parent().filter(|p| !p.exists()) {
+                fs::create_dir_all(parent)
+                    .context("Failed to create key value store's parent directory")?;
+            }
+        }
         Ok(KeyValueSqlite::new(location))
     }
 }
 
+/// The serialized runtime configuration for the SQLite key-value store.
+#[derive(Deserialize, Serialize)]
+pub struct SpinKeyValueRuntimeConfig {
+    /// The path to the SQLite database file.
+    path: Option<PathBuf>,
+}
+
+impl SpinKeyValueRuntimeConfig {
+    /// Create a new SpinKeyValueRuntimeConfig with the given parent directory
+    /// where the key-value store will live.
+    pub fn new(path: Option<PathBuf>) -> Self {
+        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.

crates/factor-key-value/src/runtime_config/spin.rs

@@ -28,11 +28,12 @@ type StoreFromToml =
 /// Creates a `StoreFromToml` function from a `MakeKeyValueStore` implementation.
 fn store_from_toml_fn<T: MakeKeyValueStore>(provider_type: T) -> StoreFromToml {
     Arc::new(move |table| {
-        let runtime_config: T::RuntimeConfig =
-            table.try_into().context("could not parse runtime config")?;
+        let runtime_config: T::RuntimeConfig = table
+            .try_into()
+            .context("could not parse key-value runtime config")?;
         let provider = provider_type
             .make_store(runtime_config)
-            .context("could not make store")?;
+            .context("could not make key-value store from runtime config")?;
         Ok(Arc::new(provider))
     })
 }
@@ -108,7 +109,9 @@ impl RuntimeConfigResolver {
 
         let mut runtime_config = RuntimeConfig::default();
         for (label, config) in table {
-            let store_manager = self.store_manager_from_config(config)?;
+            let store_manager = self.store_manager_from_config(config).with_context(|| {
+                format!("could not configure key-value store with label '{label}'")
+            })?;
             runtime_config.add_store_manager(label.clone(), store_manager);
         }
         Ok(Some(runtime_config))
@@ -133,6 +136,8 @@ impl RuntimeConfigResolver {
 impl DefaultLabelResolver for RuntimeConfigResolver {
     fn default(&self, label: &str) -> Option<Arc<dyn StoreManager>> {
         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())
     }
 }

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

@@ -4,7 +4,7 @@ use spin_factor_key_value::{
     KeyValueFactor, RuntimeConfig,
 };
 use spin_factor_key_value_redis::RedisKeyValueStore;
-use spin_factor_key_value_spin::SpinKeyValueStore;
+use spin_factor_key_value_spin::{SpinKeyValueRuntimeConfig, SpinKeyValueStore};
 use spin_factors::{FactorRuntimeConfigSource, RuntimeConfigSourceFinalizer, RuntimeFactors};
 use spin_factors_test::{toml, TestEnvironment};
 use spin_world::v2::key_value::HostStore;
@@ -19,7 +19,8 @@ struct TestFactors {
 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::<SpinKeyValueStore>("default", Default::default())?;
+    test_resolver
+        .add_default_store::<SpinKeyValueStore>("default", SpinKeyValueRuntimeConfig::new(None))?;
     let factors = TestFactors {
         key_value: KeyValueFactor::new(test_resolver),
     };

crates/factors/src/runtime_config/toml.rs

@@ -13,6 +13,7 @@ impl GetTomlValue for toml::Table {
     }
 }
 
+#[derive(Debug, Clone)]
 /// A helper for tracking which keys have been used in a TOML table.
 pub struct TomlKeyTracker<'a> {
     unused_keys: RefCell<HashSet<&'a str>>,

crates/factors/tests/smoke.rs

@@ -5,7 +5,7 @@ use http_body_util::BodyExt;
 use spin_app::App;
 use spin_factor_key_value::{runtime_config::spin::RuntimeConfigResolver, KeyValueFactor};
 use spin_factor_key_value_redis::RedisKeyValueStore;
-use spin_factor_key_value_spin::SpinKeyValueStore;
+use spin_factor_key_value_spin::{SpinKeyValueRuntimeConfig, SpinKeyValueStore};
 use spin_factor_outbound_http::OutboundHttpFactor;
 use spin_factor_outbound_networking::OutboundNetworkingFactor;
 use spin_factor_variables::VariablesFactor;
@@ -39,7 +39,8 @@ impl AsInstanceState<FactorsInstanceState> for Data {
 #[tokio::test(flavor = "multi_thread")]
 async fn smoke_test_works() -> anyhow::Result<()> {
     let mut key_value_resolver = RuntimeConfigResolver::default();
-    key_value_resolver.add_default_store::<SpinKeyValueStore>("default", Default::default())?;
+    key_value_resolver
+        .add_default_store::<SpinKeyValueStore>("default", SpinKeyValueRuntimeConfig::new(None))?;
     key_value_resolver.register_store_type(SpinKeyValueStore::new(Some(
         std::env::current_dir().context("failed to get current directory")?,
     )))?;