Merge pull request #2685 from fermyon/code-crimes

Runtime Config Independent of Serialization

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

@@ -1,7 +1,7 @@
 use std::{collections::HashMap, sync::Arc};
 
 use serde::{de::DeserializeOwned, Deserialize};
-use spin_factors::{anyhow, FactorRuntimeConfig};
+use spin_factors::anyhow;
 use spin_key_value::StoreManager;
 
 /// Runtime configuration for all key value stores.
@@ -12,10 +12,6 @@ pub struct RuntimeConfig<C> {
     pub store_configs: HashMap<String, C>,
 }
 
-impl<C: DeserializeOwned> FactorRuntimeConfig for RuntimeConfig<C> {
-    const KEY: &'static str = "key_value_store";
-}
-
 /// Resolves some piece of runtime configuration to a key value store manager.
 pub trait RuntimeConfigResolver: Send + Sync {
     /// The type of configuration that this resolver can handle.

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

@@ -4,7 +4,9 @@ use spin_factor_key_value::{
 };
 use spin_factor_key_value_redis::RedisKeyValueStore;
 use spin_factor_key_value_spin::{SpinKeyValueRuntimeConfig, SpinKeyValueStore};
-use spin_factors::RuntimeFactors;
+use spin_factors::{
+    Factor, FactorRuntimeConfigSource, RuntimeConfigSourceFinalizer, RuntimeFactors,
+};
 use spin_factors_test::{toml, TestEnvironment};
 use std::collections::HashSet;
 
@@ -41,7 +43,7 @@ async fn default_key_value_works() -> anyhow::Result<()> {
         source = "does-not-exist.wasm"
         key_value_stores = ["default"]
     });
-    let state = env.build_instance_state(factors).await?;
+    let state = env.build_instance_state(factors, ()).await?;
 
     assert_eq!(
         state.key_value.allowed_stores(),
@@ -65,15 +67,14 @@ async fn run_test_with_config_and_stores_for_label(
         key_value: KeyValueFactor::new(test_resolver),
     };
     let labels_clone = labels.clone();
-    let mut env = TestEnvironment::default_manifest_extend(toml! {
+    let env = TestEnvironment::default_manifest_extend(toml! {
         [component.test-component]
         source = "does-not-exist.wasm"
         key_value_stores = labels_clone
     });
-    if let Some(runtime_config) = runtime_config {
-        env.runtime_config.extend(runtime_config);
-    }
-    let state = env.build_instance_state(factors).await?;
+    let state = env
+        .build_instance_state(factors, TomlConfig(runtime_config))
+        .await?;
     assert_eq!(
         labels,
         state.key_value.allowed_stores().iter().collect::<Vec<_>>()
@@ -206,13 +207,14 @@ async fn multiple_custom_key_value_uses_first_store() -> anyhow::Result<()> {
     let factors = TestFactors {
         key_value: KeyValueFactor::new(test_resolver),
     };
-    let mut env = TestEnvironment::default_manifest_extend(toml! {
+    let env = TestEnvironment::default_manifest_extend(toml! {
         [component.test-component]
         source = "does-not-exist.wasm"
         key_value_stores = ["custom"]
     });
-    env.runtime_config.extend(runtime_config);
-    let state = env.build_instance_state(factors).await?;
+    let state = env
+        .build_instance_state(factors, TomlConfig(Some(runtime_config)))
+        .await?;
 
     assert_eq!(
         state.key_value.allowed_stores(),
@@ -222,3 +224,32 @@ async fn multiple_custom_key_value_uses_first_store() -> anyhow::Result<()> {
     assert!(tmp_dir.path().exists());
     Ok(())
 }
+
+struct TomlConfig(Option<toml::Table>);
+
+impl TryFrom<TomlConfig> for TestFactorsRuntimeConfig {
+    type Error = anyhow::Error;
+
+    fn try_from(value: TomlConfig) -> Result<Self, Self::Error> {
+        Self::from_source(value)
+    }
+}
+
+impl FactorRuntimeConfigSource<KeyValueFactor<DelegatingRuntimeConfigResolver>> for TomlConfig {
+    fn get_runtime_config(
+        &mut self,
+    ) -> anyhow::Result<
+        Option<<KeyValueFactor<DelegatingRuntimeConfigResolver> as Factor>::RuntimeConfig>,
+    > {
+        let Some(table) = self.0.as_ref().and_then(|t| t.get("key_value_store")) else {
+            return Ok(None);
+        };
+        Ok(Some(table.clone().try_into()?))
+    }
+}
+
+impl RuntimeConfigSourceFinalizer for TomlConfig {
+    fn finalize(&mut self) -> anyhow::Result<()> {
+        Ok(())
+    }
+}

crates/factor-llm/tests/factor.rs

@@ -48,7 +48,7 @@ async fn llm_works() -> anyhow::Result<()> {
         source = "does-not-exist.wasm"
         ai_models = ["llama2-chat"]
     });
-    let mut state = env.build_instance_state(factors).await?;
+    let mut state = env.build_instance_state(factors, ()).await?;
     assert_eq!(
         &*state.llm.allowed_models,
         &["llama2-chat".to_owned()]

crates/factor-outbound-http/tests/factor_test.rs

@@ -4,7 +4,7 @@ use anyhow::bail;
 use http::Request;
 use spin_factor_outbound_http::OutboundHttpFactor;
 use spin_factor_outbound_networking::OutboundNetworkingFactor;
-use spin_factor_variables::VariablesFactor;
+use spin_factor_variables::spin_cli::VariablesFactor;
 use spin_factor_wasi::{DummyFilesMounter, WasiFactor};
 use spin_factors::{anyhow, RuntimeFactors};
 use spin_factors_test::{toml, TestEnvironment};
@@ -37,7 +37,7 @@ async fn disallowed_host_fails() -> anyhow::Result<()> {
         http: OutboundHttpFactor,
     };
     let env = test_env();
-    let mut state = env.build_instance_state(factors).await?;
+    let mut state = env.build_instance_state(factors, ()).await?;
     let mut wasi_http = OutboundHttpFactor::get_wasi_http_impl(&mut state).unwrap();
 
     let req = Request::get("https://denied.test").body(Default::default())?;

crates/factor-outbound-networking/src/lib.rs

@@ -59,7 +59,10 @@ impl Factor for OutboundNetworkingFactor {
             .get(ctx.app_component().id())
             .cloned()
             .context("missing component allowed hosts")?;
-        let resolver = builders.get_mut::<VariablesFactor>()?.resolver().clone();
+        let resolver = builders
+            .get_mut::<VariablesFactor<()>>()?
+            .expression_resolver()
+            .clone();
         let allowed_hosts_future = async move {
             let prepared = resolver.prepare().await?;
             AllowedHostsConfig::parse(&hosts, &prepared)

crates/factor-outbound-networking/tests/factor_test.rs

@@ -1,5 +1,5 @@
 use spin_factor_outbound_networking::OutboundNetworkingFactor;
-use spin_factor_variables::VariablesFactor;
+use spin_factor_variables::spin_cli::VariablesFactor;
 use spin_factor_wasi::{DummyFilesMounter, WasiFactor};
 use spin_factors::{anyhow, RuntimeFactors};
 use spin_factors_test::{toml, TestEnvironment};
@@ -29,7 +29,7 @@ async fn configures_wasi_socket_addr_check() -> anyhow::Result<()> {
     };
 
     let env = test_env();
-    let mut state = env.build_instance_state(factors).await?;
+    let mut state = env.build_instance_state(factors, ()).await?;
     let mut wasi = WasiFactor::get_wasi_impl(&mut state).unwrap();
 
     let network_resource = wasi.instance_network()?;
@@ -62,10 +62,13 @@ async fn wasi_factor_is_optional() -> anyhow::Result<()> {
         networking: OutboundNetworkingFactor,
     }
     TestEnvironment::default()
-        .build_instance_state(WithoutWasi {
-            variables: VariablesFactor::default(),
-            networking: OutboundNetworkingFactor,
-        })
+        .build_instance_state(
+            WithoutWasi {
+                variables: VariablesFactor::default(),
+                networking: OutboundNetworkingFactor,
+            },
+            (),
+        )
         .await?;
     Ok(())
 }

crates/factor-outbound-pg/tests/factor_test.rs

@@ -2,7 +2,7 @@ use anyhow::{bail, Result};
 use spin_factor_outbound_networking::OutboundNetworkingFactor;
 use spin_factor_outbound_pg::client::Client;
 use spin_factor_outbound_pg::OutboundPgFactor;
-use spin_factor_variables::{StaticVariables, VariablesFactor};
+use spin_factor_variables::spin_cli::{StaticVariables, VariablesFactor};
 use spin_factors::{anyhow, RuntimeFactors};
 use spin_factors_test::{toml, TestEnvironment};
 use spin_world::async_trait;
@@ -24,7 +24,7 @@ fn factors() -> Result<TestFactors> {
         networking: OutboundNetworkingFactor,
         pg: OutboundPgFactor::<MockClient>::new(),
     };
-    f.variables.add_provider_type(StaticVariables)?;
+    f.variables.add_provider_resolver(StaticVariables)?;
     Ok(f)
 }
 
@@ -43,7 +43,7 @@ async fn disallowed_host_fails() -> anyhow::Result<()> {
         [component.test-component]
         source = "does-not-exist.wasm"
     });
-    let mut state = env.build_instance_state(factors).await?;
+    let mut state = env.build_instance_state(factors, ()).await?;
 
     let res = state
         .pg
@@ -61,7 +61,7 @@ async fn disallowed_host_fails() -> anyhow::Result<()> {
 async fn allowed_host_succeeds() -> anyhow::Result<()> {
     let factors = factors()?;
     let env = test_env();
-    let mut state = env.build_instance_state(factors).await?;
+    let mut state = env.build_instance_state(factors, ()).await?;
 
     let res = state
         .pg
@@ -78,7 +78,7 @@ async fn allowed_host_succeeds() -> anyhow::Result<()> {
 async fn exercise_execute() -> anyhow::Result<()> {
     let factors = factors()?;
     let env = test_env();
-    let mut state = env.build_instance_state(factors).await?;
+    let mut state = env.build_instance_state(factors, ()).await?;
 
     let connection = state
         .pg
@@ -97,7 +97,7 @@ async fn exercise_execute() -> anyhow::Result<()> {
 async fn exercise_query() -> anyhow::Result<()> {
     let factors = factors()?;
     let env = test_env();
-    let mut state = env.build_instance_state(factors).await?;
+    let mut state = env.build_instance_state(factors, ()).await?;
 
     let connection = state
         .pg

crates/factor-outbound-redis/tests/factor_test.rs

@@ -1,7 +1,7 @@
 use anyhow::bail;
 use spin_factor_outbound_networking::OutboundNetworkingFactor;
 use spin_factor_outbound_redis::OutboundRedisFactor;
-use spin_factor_variables::{StaticVariables, VariablesFactor};
+use spin_factor_variables::spin_cli::{StaticVariables, VariablesFactor};
 use spin_factor_wasi::{DummyFilesMounter, WasiFactor};
 use spin_factors::{anyhow, RuntimeFactors};
 use spin_factors_test::{toml, TestEnvironment};
@@ -28,7 +28,7 @@ fn get_test_factors() -> TestFactors {
 async fn no_outbound_hosts_fails() -> anyhow::Result<()> {
     let mut factors = get_test_factors();
 
-    factors.variables.add_provider_type(StaticVariables)?;
+    factors.variables.add_provider_resolver(StaticVariables)?;
 
     let env = TestEnvironment {
         manifest: toml! {
@@ -41,7 +41,7 @@ async fn no_outbound_hosts_fails() -> anyhow::Result<()> {
         },
         ..Default::default()
     };
-    let mut state = env.build_instance_state(factors).await?;
+    let mut state = env.build_instance_state(factors, ()).await?;
     let connection = state
         .redis
         .open("redis://redis.test:8080".to_string())

crates/factor-sqlite/src/lib.rs

@@ -7,29 +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<R> {
-    runtime_config_resolver: Arc<R>,
+pub struct SqliteFactor {
+    default_label_resolver: Arc<dyn DefaultLabelResolver>,
 }
 
-impl<R> SqliteFactor<R> {
+impl SqliteFactor {
     /// Create a new `SqliteFactor`
     ///
-    /// Takes a `runtime_config_resolver` that can resolve a runtime configuration into a connection pool.
-    pub fn new(runtime_config_resolver: R) -> Self {
+    /// Takes a `default_label_resolver` for how to handle when a database label doesn't
+    /// have a corresponding runtime configuration.
+    pub fn new(default_label_resolver: impl DefaultLabelResolver + 'static) -> Self {
         Self {
-            runtime_config_resolver: Arc::new(runtime_config_resolver),
+            default_label_resolver: Arc::new(default_label_resolver),
         }
     }
 }
 
-impl<R: RuntimeConfigResolver + 'static> Factor for SqliteFactor<R> {
-    type RuntimeConfig = runtime_config::RuntimeConfig<R::Config>;
+impl Factor for SqliteFactor {
+    type RuntimeConfig = runtime_config::RuntimeConfig;
     type AppState = AppState;
     type InstanceBuilder = InstanceState;
 
@@ -46,13 +46,10 @@ impl<R: RuntimeConfigResolver + 'static> Factor for SqliteFactor<R> {
         &self,
         mut ctx: spin_factors::ConfigureAppContext<T, Self>,
     ) -> anyhow::Result<Self::AppState> {
-        let mut connection_pools = HashMap::new();
-        if let Some(runtime_config) = ctx.take_runtime_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);
-            }
-        }
+        let connection_pools = ctx
+            .take_runtime_config()
+            .map(|r| r.pools)
+            .unwrap_or_default();
 
         let allowed_databases = ctx
             .app()
@@ -70,7 +67,7 @@ impl<R: RuntimeConfigResolver + 'static> Factor for SqliteFactor<R> {
                 ))
             })
             .collect::<anyhow::Result<HashMap<_, _>>>()?;
-        let resolver = self.runtime_config_resolver.clone();
+        let resolver = self.default_label_resolver.clone();
         let get_connection_pool: host::ConnectionPoolGetter = Arc::new(move |label| {
             connection_pools
                 .get(label)
@@ -139,6 +136,14 @@ fn ensure_allowed_databases_are_configured(
 
 pub const ALLOWED_DATABASES_KEY: MetadataKey<Vec<String>> = MetadataKey::new("databases");
 
+/// Resolves a label to a default connection pool.
+pub trait DefaultLabelResolver: Send + Sync {
+    /// If there is no runtime configuration for a given database label, return a default connection pool.
+    ///
+    /// If `Option::None` is returned, the database is not allowed.
+    fn default(&self, label: &str) -> Option<Arc<dyn ConnectionPool>>;
+}
+
 pub struct AppState {
     /// A map from component id to a set of allowed database labels.
     allowed_databases: HashMap<String, Arc<HashSet<String>>>,