Remove Spin KV store base path default to current working dir and syn…

…tactical nits

Signed-off-by: Kate Goldenring <[email protected]>

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

@@ -30,6 +30,11 @@ impl MakeKeyValueStore for AzureKeyValueStore {
         &self,
         runtime_config: Self::RuntimeConfig,
     ) -> anyhow::Result<Self::StoreManager> {
-        KeyValueAzureCosmos::new(runtime_config.key, runtime_config.account, runtime_config.database, runtime_config.container)
+        KeyValueAzureCosmos::new(
+            runtime_config.key,
+            runtime_config.account,
+            runtime_config.database,
+            runtime_config.container,
+        )
     }
 }

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

@@ -16,13 +16,8 @@ pub struct SpinKeyValueStore {
 
 impl SpinKeyValueStore {
     /// Create a new SpinKeyValueStore with the given base path.
-    /// If the base path is None, the current directory is used.
-    pub fn new(base_path: Option<PathBuf>) -> anyhow::Result<Self> {
-        let base_path = match base_path {
-            Some(path) => path,
-            None => std::env::current_dir().context("failed to get current directory")?,
-        };
-        Ok(Self { base_path })
+    pub fn new(base_path: PathBuf) -> Self {
+        Self { base_path }
     }
 }
 
@@ -38,10 +33,11 @@ impl SpinKeyValueRuntimeConfig {
     const DEFAULT_SPIN_STORE_FILENAME: &'static str = "sqlite_key_value.db";
 
     /// Create a new runtime configuration with the given state directory.
-    /// If the state directory is None, the database is in-memory.
-    /// If the state directory is Some, the database is stored in a file in the state directory.
-    pub fn default(state_dir: Option<PathBuf>) -> Self {
-        let path = state_dir.map(|dir| dir.join(Self::DEFAULT_SPIN_STORE_FILENAME));
+    ///
+    /// 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 state 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 }
     }
 }

crates/factor-key-value/src/delegating_resolver.rs

@@ -1,15 +1,18 @@
 use crate::runtime_config::RuntimeConfigResolver;
 use crate::store::{store_from_toml_fn, MakeKeyValueStore, StoreFromToml};
-use serde::Deserialize;
+use serde::{Deserialize, Serialize};
 use spin_key_value::StoreManager;
 use std::{collections::HashMap, sync::Arc};
 
-/// A RuntimeConfigResolver that delegates to the appropriate key-value store for a given label.
-/// The store types are registered with the resolver using `add_store_type`.
-/// The default store for a label is registered using `add_default_store`.
+/// A RuntimeConfigResolver that delegates to the appropriate key-value store
+/// for a given label.
+///
+/// The store types are registered with the resolver using `add_store_type`. The
+/// default store for a label is registered using `add_default_store`.
 #[derive(Default)]
 pub struct DelegatingRuntimeConfigResolver {
-    /// A map of store types to a function that returns the appropriate store manager from runtime config TOML.
+    /// A map of store types to a function that returns the appropriate store
+    /// manager from runtime config TOML.
     store_types: HashMap<&'static str, StoreFromToml>,
     /// A map of default store configurations for a label.
     defaults: HashMap<&'static str, StoreConfig>,
@@ -54,6 +57,7 @@ impl RuntimeConfigResolver for DelegatingRuntimeConfigResolver {
     }
 
     /// Get the default store manager for the given label.
+    ///
     /// Returns None if no default store is registered for the label.
     fn default_store(&self, label: &str) -> Option<Arc<dyn StoreManager>> {
         let config = self.defaults.get(label)?;
@@ -68,3 +72,15 @@ pub struct StoreConfig {
     #[serde(flatten)]
     pub config: toml::Table,
 }
+
+impl StoreConfig {
+    pub fn new<T>(type_: String, config: T) -> anyhow::Result<Self>
+    where
+        T: Serialize,
+    {
+        Ok(Self {
+            type_,
+            config: toml::value::Table::try_from(config)?,
+        })
+    }
+}

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

@@ -1,7 +1,7 @@
 pub mod delegating_resolver;
 mod runtime_config;
 mod store;
-pub use delegating_resolver::DelegatingRuntimeConfigResolver;
+pub use delegating_resolver::{DelegatingRuntimeConfigResolver, StoreConfig};
 
 use std::{
     collections::{HashMap, HashSet},
@@ -27,7 +27,9 @@ pub struct KeyValueFactor<R> {
 }
 
 impl<R> KeyValueFactor<R> {
-    /// Create a new KeyValueFactor.
+    /// Create a new KeyValueFactor.  
+    ///  
+    /// The `runtime_config_resolver` is used to resolve runtime configuration into store managers.
     pub fn new(runtime_config_resolver: R) -> Self {
         Self {
             runtime_config_resolver: Arc::new(runtime_config_resolver),
@@ -54,10 +56,11 @@ impl<R: RuntimeConfigResolver + 'static> Factor for KeyValueFactor<R> {
         let mut store_managers: HashMap<String, Arc<dyn StoreManager>> = HashMap::new();
         if let Some(runtime_config) = ctx.take_runtime_config() {
             for (store_label, config) in runtime_config.store_configs {
-                let store = self.runtime_config_resolver.get_store(config)?;
                 if let std::collections::hash_map::Entry::Vacant(e) =
                     store_managers.entry(store_label)
                 {
+                    // Only add manager for labels that are not already configured. Runtime config
+                    // takes top-down precedence.
                     let store = self.runtime_config_resolver.get_store(config)?;
                     e.insert(store);
                 }
@@ -119,19 +122,25 @@ impl<R: RuntimeConfigResolver + 'static> Factor for KeyValueFactor<R> {
 type AppStoreManager = CachingStoreManager<DelegatingStoreManager>;
 
 pub struct AppState {
-    /// The store manager for the app. This is a cache around a delegating store
-    /// manager. For `get` requests, first checks the cache before delegating to
-    /// the underlying store manager.
+    /// The store manager for the app.
+    ///
+    /// This is a cache around a delegating store manager. For `get` requests,
+    /// first checks the cache before delegating to the underlying store
+    /// manager.
     store_manager: Arc<AppStoreManager>,
     /// The allowed stores for each component.
-    /// This is a map from component ID to the set of store labels that the component is allowed to use.
+    ///
+    /// This is a map from component ID to the set of store labels that the
+    /// component is allowed to use.
     component_allowed_stores: HashMap<String, HashSet<String>>,
 }
 
 pub struct InstanceBuilder {
-    /// The store manager for the app. This is a cache around a delegating store
-    /// manager. For `get` requests, first checks the cache before delegating to
-    /// the underlying store manager.
+    /// The store manager for the app.
+    ///
+    /// This is a cache around a delegating store manager. For `get` requests,
+    /// first checks the cache before delegating to the underlying store
+    /// manager.
     store_manager: Arc<AppStoreManager>,
     /// The allowed stores for this component instance.
     allowed_stores: HashSet<String>,

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

@@ -1,27 +1,32 @@
-use std::collections::HashSet;
-use spin_factor_key_value::{DelegatingRuntimeConfigResolver, KeyValueFactor, MakeKeyValueStore};
+use anyhow::Context;
+use spin_factor_key_value::{
+    DelegatingRuntimeConfigResolver, KeyValueFactor, MakeKeyValueStore, StoreConfig,
+};
 use spin_factor_key_value_redis::RedisKeyValueStore;
 use spin_factor_key_value_spin::{SpinKeyValueRuntimeConfig, SpinKeyValueStore};
 use spin_factors::RuntimeFactors;
 use spin_factors_test::{toml, TestEnvironment};
+use std::collections::HashSet;
 
 #[derive(RuntimeFactors)]
 struct TestFactors {
-    key_value: KeyValueFactor,
+    key_value: KeyValueFactor<DelegatingRuntimeConfigResolver>,
 }
 
 fn default_key_value_resolver(
 ) -> anyhow::Result<(DelegatingRuntimeConfigResolver, tempdir::TempDir)> {
     let mut test_resolver = DelegatingRuntimeConfigResolver::new();
-    test_resolver.add_store_type(SpinKeyValueStore::new(None)?)?;
+    test_resolver.add_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));
-    test_resolver.add_default_store(
-        "default",
-        SpinKeyValueStore::RUNTIME_CONFIG_TYPE,
-        toml::value::Table::try_from(default_config)?,
-    );
+    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))
 }
 
@@ -69,7 +74,6 @@ async fn run_test_with_config_and_stores_for_label(
         env.runtime_config.extend(runtime_config);
     }
     let state = env.build_instance_state(factors).await?;
-    // String::new("foo").as
     assert_eq!(
         labels,
         state.key_value.allowed_stores().iter().collect::<Vec<_>>()
@@ -101,7 +105,9 @@ async fn custom_spin_key_value_works() -> anyhow::Result<()> {
     };
     run_test_with_config_and_stores_for_label(
         Some(runtime_config),
-        vec![SpinKeyValueStore::new(None)?],
+        vec![SpinKeyValueStore::new(
+            std::env::current_dir().context("failed to get current directory")?,
+        )],
         vec!["custom"],
     )
     .await
@@ -119,7 +125,9 @@ async fn custom_spin_key_value_works_with_absolute_path() -> anyhow::Result<()>
     };
     run_test_with_config_and_stores_for_label(
         Some(runtime_config),
-        vec![SpinKeyValueStore::new(None)?],
+        vec![SpinKeyValueStore::new(
+            std::env::current_dir().context("failed to get current directory")?,
+        )],
         vec!["custom"],
     )
     .await?;
@@ -138,7 +146,7 @@ async fn custom_spin_key_value_works_with_relative_path() -> anyhow::Result<()>
     };
     run_test_with_config_and_stores_for_label(
         Some(runtime_config),
-        vec![SpinKeyValueStore::new(Some(path))?],
+        vec![SpinKeyValueStore::new(path)],
         vec!["custom"],
     )
     .await?;
@@ -170,7 +178,9 @@ async fn misconfigured_spin_key_value_fails() -> anyhow::Result<()> {
     };
     assert!(run_test_with_config_and_stores_for_label(
         Some(runtime_config),
-        vec![SpinKeyValueStore::new(None)?],
+        vec![SpinKeyValueStore::new(
+            std::env::current_dir().context("failed to get current directory")?
+        )],
         vec!["custom"]
     )
     .await
@@ -179,7 +189,7 @@ async fn misconfigured_spin_key_value_fails() -> anyhow::Result<()> {
 }
 
 #[tokio::test]
-async fn multiple_custom_key_value_fails() -> anyhow::Result<()> {
+async fn multiple_custom_key_value_uses_first_store() -> anyhow::Result<()> {
     let tmp_dir = tempdir::TempDir::new("example")?;
     let runtime_config = toml::toml! {
         [key_value_store.custom]
@@ -192,7 +202,7 @@ async fn multiple_custom_key_value_fails() -> anyhow::Result<()> {
     };
     let mut test_resolver = DelegatingRuntimeConfigResolver::new();
     test_resolver.add_store_type(RedisKeyValueStore)?;
-    test_resolver.add_store_type(SpinKeyValueStore::new(Some(tmp_dir.path().to_owned()))?)?;
+    test_resolver.add_store_type(SpinKeyValueStore::new(tmp_dir.path().to_owned()))?;
     let factors = TestFactors {
         key_value: KeyValueFactor::new(test_resolver),
     };

crates/factors/tests/smoke.rs

@@ -48,7 +48,9 @@ async fn smoke_test_works() -> anyhow::Result<()> {
             config: toml::value::Table::try_from(default_config)?,
         },
     );
-    key_value_resolver.add_store_type(SpinKeyValueStore::new(None)?)?;
+    key_value_resolver.add_store_type(SpinKeyValueStore::new(
+        std::env::current_dir().context("failed to get current directory")?,
+    ))?;
     key_value_resolver.add_store_type(RedisKeyValueStore)?;
 
     let mut factors = Factors {