Merge pull request #2665 from fermyon/remove-toml-assumption

Remove toml assumption

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

@@ -1,5 +1,6 @@
 use crate::runtime_config::RuntimeConfigResolver;
 use crate::store::{store_from_toml_fn, MakeKeyValueStore, StoreFromToml};
+use serde::Deserialize;
 use spin_key_value::StoreManager;
 use std::{collections::HashMap, sync::Arc};
 
@@ -9,20 +10,13 @@ pub struct DelegatingRuntimeConfigResolver {
     defaults: HashMap<&'static str, StoreConfig>,
 }
 
-type StoreConfig = (&'static str, toml::value::Table);
-
 impl DelegatingRuntimeConfigResolver {
     pub fn new() -> Self {
         Self::default()
     }
 
-    pub fn add_default_store(
-        &mut self,
-        label: &'static str,
-        store_kind: &'static str,
-        config: toml::value::Table,
-    ) {
-        self.defaults.insert(label, (store_kind, config));
+    pub fn add_default_store(&mut self, label: &'static str, config: StoreConfig) {
+        self.defaults.insert(label, config);
     }
 }
 
@@ -43,20 +37,27 @@ impl DelegatingRuntimeConfigResolver {
 }
 
 impl RuntimeConfigResolver for DelegatingRuntimeConfigResolver {
-    fn get_store(
-        &self,
-        store_kind: &str,
-        config: toml::Table,
-    ) -> anyhow::Result<Arc<dyn StoreManager>> {
+    type Config = StoreConfig;
+
+    fn get_store(&self, config: StoreConfig) -> anyhow::Result<Arc<dyn StoreManager>> {
+        let store_kind = config.type_.as_str();
         let store_from_toml = self
             .store_types
             .get(store_kind)
             .ok_or_else(|| anyhow::anyhow!("unknown store kind: {}", store_kind))?;
-        store_from_toml(config)
+        store_from_toml(config.config)
     }
 
     fn default_store(&self, label: &str) -> Option<Arc<dyn StoreManager>> {
-        let (store_kind, config) = self.defaults.get(label)?;
-        self.get_store(store_kind, config.to_owned()).ok()
+        let config = self.defaults.get(label)?;
+        self.get_store(config.clone()).ok()
     }
 }
+
+#[derive(Deserialize, Clone)]
+pub struct StoreConfig {
+    #[serde(rename = "type")]
+    pub type_: String,
+    #[serde(flatten)]
+    pub config: toml::Table,
+}

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

@@ -8,7 +8,7 @@ use std::{
 };
 
 use anyhow::ensure;
-use runtime_config::RuntimeConfig;
+use runtime_config::{RuntimeConfig, RuntimeConfigResolver};
 use spin_factors::{
     ConfigureAppContext, Factor, FactorInstanceBuilder, InitContext, InstanceBuilders,
     PrepareContext, RuntimeFactors,
@@ -19,22 +19,20 @@ use spin_key_value::{
 };
 pub use store::MakeKeyValueStore;
 
-pub struct KeyValueFactor {
-    runtime_config_resolver: Arc<dyn runtime_config::RuntimeConfigResolver>,
+pub struct KeyValueFactor<R> {
+    runtime_config_resolver: Arc<R>,
 }
 
-impl KeyValueFactor {
-    pub fn new(
-        runtime_config_resolver: impl runtime_config::RuntimeConfigResolver + 'static,
-    ) -> Self {
+impl<R> KeyValueFactor<R> {
+    pub fn new(runtime_config_resolver: R) -> Self {
         Self {
             runtime_config_resolver: Arc::new(runtime_config_resolver),
         }
     }
 }
 
-impl Factor for KeyValueFactor {
-    type RuntimeConfig = RuntimeConfig;
+impl<R: RuntimeConfigResolver + 'static> Factor for KeyValueFactor<R> {
+    type RuntimeConfig = RuntimeConfig<R::Config>;
     type AppState = AppState;
     type InstanceBuilder = InstanceBuilder;
 
@@ -51,17 +49,8 @@ impl Factor for KeyValueFactor {
         // Build StoreManager from runtime config
         let mut store_managers: HashMap<String, Arc<dyn StoreManager>> = HashMap::new();
         if let Some(runtime_config) = ctx.take_runtime_config() {
-            for (
-                store_label,
-                runtime_config::StoreConfig {
-                    type_: store_kind,
-                    config,
-                },
-            ) in runtime_config.store_configs
-            {
-                let store = self
-                    .runtime_config_resolver
-                    .get_store(&store_kind, config)?;
+            for (store_label, config) in runtime_config.store_configs {
+                let store = self.runtime_config_resolver.get_store(config)?;
                 store_managers.insert(store_label, store);
             }
         }

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

@@ -1,38 +1,26 @@
 use std::{collections::HashMap, sync::Arc};
 
-use serde::Deserialize;
+use serde::{de::DeserializeOwned, Deserialize};
 use spin_factors::{anyhow, FactorRuntimeConfig};
 use spin_key_value::StoreManager;
 
 #[derive(Deserialize)]
 #[serde(transparent)]
-pub struct RuntimeConfig {
-    pub store_configs: HashMap<String, StoreConfig>,
+pub struct RuntimeConfig<C> {
+    pub store_configs: HashMap<String, C>,
 }
 
-impl FactorRuntimeConfig for RuntimeConfig {
+impl<C: DeserializeOwned> FactorRuntimeConfig for RuntimeConfig<C> {
     const KEY: &'static str = "key_value_store";
 }
 
-#[derive(Deserialize)]
-pub struct StoreConfig {
-    #[serde(rename = "type")]
-    pub type_: String,
-    #[serde(flatten)]
-    pub config: toml::Table,
-}
-
 /// Resolves some piece of runtime configuration to a connection pool
 pub trait RuntimeConfigResolver: Send + Sync {
-    /// Get a store manager for a given store kind and the raw configuration.
-    ///
-    /// `store_kind` is equivalent to the `type` field in the
-    /// `[key_value_store.$storename]` runtime configuration table.
-    fn get_store(
-        &self,
-        store_kind: &str,
-        config: toml::Table,
-    ) -> anyhow::Result<Arc<dyn StoreManager>>;
+    /// The type of configuration that this resolver can handle.
+    type Config: DeserializeOwned;
+
+    /// Get a store manager for a given config.
+    fn get_store(&self, config: Self::Config) -> anyhow::Result<Arc<dyn StoreManager>>;
 
     /// Returns a default store manager for a given label. Should only be called
     /// if there is no runtime configuration for the label.

crates/factor-sqlite/src/lib.rs

@@ -7,28 +7,29 @@ use std::sync::Arc;
 use host::InstanceState;
 
 use async_trait::async_trait;
+use runtime_config::RuntimeConfigResolver;
 use spin_factors::{anyhow, Factor};
 use spin_locked_app::MetadataKey;
 use spin_world::v1::sqlite as v1;
 use spin_world::v2::sqlite as v2;
 
-pub struct SqliteFactor {
-    runtime_config_resolver: Arc<dyn runtime_config::RuntimeConfigResolver>,
+pub struct SqliteFactor<R> {
+    runtime_config_resolver: Arc<R>,
 }
 
-impl SqliteFactor {
+impl<R> SqliteFactor<R> {
     /// Create a new `SqliteFactor`
-    pub fn new(
-        runtime_config_resolver: impl runtime_config::RuntimeConfigResolver + 'static,
-    ) -> Self {
+    ///
+    /// Takes a `runtime_config_resolver` that can resolve a runtime configuration into a connection pool.
+    pub fn new(runtime_config_resolver: R) -> Self {
         Self {
             runtime_config_resolver: Arc::new(runtime_config_resolver),
         }
     }
 }
 
-impl Factor for SqliteFactor {
-    type RuntimeConfig = runtime_config::RuntimeConfig;
+impl<R: RuntimeConfigResolver + 'static> Factor for SqliteFactor<R> {
+    type RuntimeConfig = runtime_config::RuntimeConfig<R::Config>;
     type AppState = AppState;
     type InstanceBuilder = InstanceState;
 
@@ -47,17 +48,8 @@ impl Factor for SqliteFactor {
     ) -> anyhow::Result<Self::AppState> {
         let mut connection_pools = HashMap::new();
         if let Some(runtime_config) = ctx.take_runtime_config() {
-            for (
-                database_label,
-                runtime_config::StoreConfig {
-                    type_: database_kind,
-                    config,
-                },
-            ) in runtime_config.store_configs
-            {
-                let pool = self
-                    .runtime_config_resolver
-                    .get_pool(&database_kind, config)?;
+            for (database_label, config) in runtime_config.store_configs {
+                let pool = self.runtime_config_resolver.get_pool(config)?;
                 connection_pools.insert(database_label, pool);
             }
         }

crates/factor-sqlite/src/runtime_config.rs

@@ -3,40 +3,28 @@ pub mod spin;
 
 use std::{collections::HashMap, sync::Arc};
 
-use serde::Deserialize;
+use serde::{de::DeserializeOwned, Deserialize};
 use spin_factors::{anyhow, FactorRuntimeConfig};
 
 use crate::ConnectionPool;
 
 #[derive(Deserialize)]
 #[serde(transparent)]
-pub struct RuntimeConfig {
-    pub store_configs: HashMap<String, StoreConfig>,
+pub struct RuntimeConfig<C> {
+    pub store_configs: HashMap<String, C>,
 }
 
-impl FactorRuntimeConfig for RuntimeConfig {
+impl<C: DeserializeOwned> FactorRuntimeConfig for RuntimeConfig<C> {
     const KEY: &'static str = "sqlite_database";
 }
 
-#[derive(Deserialize)]
-pub struct StoreConfig {
-    #[serde(rename = "type")]
-    pub type_: String,
-    #[serde(flatten)]
-    pub config: toml::Table,
-}
-
 /// Resolves some piece of runtime configuration to a connection pool
 pub trait RuntimeConfigResolver: Send + Sync {
-    /// Get a connection pool for a given database kind and the raw configuration.
+    type Config: DeserializeOwned;
+
+    /// Get a connection pool for a given config.
     ///
-    /// `database_kind` is equivalent to the `type` field in the
-    /// `[sqlite_database.$databasename]` runtime configuration table.
-    fn get_pool(
-        &self,
-        database_kind: &str,
-        config: toml::Table,
-    ) -> anyhow::Result<Arc<dyn ConnectionPool>>;
+    fn get_pool(&self, config: Self::Config) -> anyhow::Result<Arc<dyn ConnectionPool>>;
 
     /// If there is no runtime configuration for a given database label, return a default connection pool.
     ///

crates/factor-sqlite/src/runtime_config/spin.rs

@@ -50,22 +50,29 @@ impl SpinSqliteRuntimeConfig {
     }
 }
 
+#[derive(Deserialize)]
+pub struct RuntimeConfig {
+    #[serde(rename = "type")]
+    pub type_: String,
+    #[serde(flatten)]
+    pub config: toml::Table,
+}
+
 impl RuntimeConfigResolver for SpinSqliteRuntimeConfig {
-    fn get_pool(
-        &self,
-        database_kind: &str,
-        config: toml::Table,
-    ) -> anyhow::Result<Arc<dyn ConnectionPool>> {
+    type Config = RuntimeConfig;
+
+    fn get_pool(&self, config: RuntimeConfig) -> anyhow::Result<Arc<dyn ConnectionPool>> {
+        let database_kind = config.type_.as_str();
         let pool = match database_kind {
             "spin" => {
-                let config: LocalDatabase = config.try_into()?;
+                let config: LocalDatabase = config.config.try_into()?;
                 config.pool(&self.local_database_dir)?
             }
             "libsql" => {
-                let config: LibSqlDatabase = config.try_into()?;
+                let config: LibSqlDatabase = config.config.try_into()?;
                 config.pool()?
             }
-            _ => anyhow::bail!("Unknown database kind: {}", database_kind),
+            _ => anyhow::bail!("Unknown database kind: {database_kind}"),
         };
         Ok(Arc::new(pool))
     }

crates/factor-sqlite/tests/factor.rs

@@ -1,6 +1,6 @@
 use std::{collections::HashSet, sync::Arc};
 
-use factor_sqlite::SqliteFactor;
+use factor_sqlite::{runtime_config::spin::RuntimeConfig, SqliteFactor};
 use spin_factors::{
     anyhow::{self, bail},
     RuntimeFactors,
@@ -9,7 +9,7 @@ use spin_factors_test::{toml, TestEnvironment};
 
 #[derive(RuntimeFactors)]
 struct TestFactors {
-    sqlite: SqliteFactor,
+    sqlite: SqliteFactor<RuntimeConfigResolver>,
 }
 
 #[tokio::test]
@@ -70,7 +70,9 @@ async fn no_error_when_database_is_configured() -> anyhow::Result<()> {
         [sqlite_database.foo]
         type = "sqlite"
     };
-    assert!(env.build_instance_state(factors).await.is_ok());
+    if let Err(e) = env.build_instance_state(factors).await {
+        bail!("Expected build_instance_state to succeed but it errored: {e}");
+    }
 
     Ok(())
 }
@@ -89,12 +91,13 @@ impl RuntimeConfigResolver {
 }
 
 impl factor_sqlite::runtime_config::RuntimeConfigResolver for RuntimeConfigResolver {
+    type Config = RuntimeConfig;
+
     fn get_pool(
         &self,
-        database_kind: &str,
-        config: toml::Table,
+        config: RuntimeConfig,
     ) -> anyhow::Result<Arc<dyn factor_sqlite::ConnectionPool>> {
-        let _ = (database_kind, config);
+        let _ = config;
         Ok(Arc::new(InvalidConnectionPool))
     }
 

crates/factors/tests/smoke.rs

@@ -4,7 +4,8 @@ use anyhow::Context;
 use http_body_util::BodyExt;
 use spin_app::App;
 use spin_factor_key_value::{
-    delegating_resolver::DelegatingRuntimeConfigResolver, KeyValueFactor, MakeKeyValueStore,
+    delegating_resolver::{DelegatingRuntimeConfigResolver, StoreConfig},
+    KeyValueFactor, MakeKeyValueStore,
 };
 use spin_factor_key_value_redis::RedisKeyValueStore;
 use spin_factor_key_value_spin::{SpinKeyValueRuntimeConfig, SpinKeyValueStore};
@@ -21,7 +22,7 @@ struct Factors {
     variables: VariablesFactor,
     outbound_networking: OutboundNetworkingFactor,
     outbound_http: OutboundHttpFactor,
-    key_value: KeyValueFactor,
+    key_value: KeyValueFactor<DelegatingRuntimeConfigResolver>,
 }
 
 struct Data {
@@ -42,8 +43,10 @@ async fn smoke_test_works() -> anyhow::Result<()> {
         SpinKeyValueRuntimeConfig::default(Some(PathBuf::from("tests/smoke-app/.spin")));
     key_value_resolver.add_default_store(
         "default",
-        SpinKeyValueStore::RUNTIME_CONFIG_TYPE,
-        toml::value::Table::try_from(default_config)?,
+        StoreConfig {
+            type_: SpinKeyValueStore::RUNTIME_CONFIG_TYPE.to_string(),
+            config: toml::value::Table::try_from(default_config)?,
+        },
     );
     key_value_resolver.add_store_type(SpinKeyValueStore::new(None)?)?;
     key_value_resolver.add_store_type(RedisKeyValueStore)?;