diff --git a/crates/factor-key-value-spin/src/lib.rs b/crates/factor-key-value-spin/src/lib.rs index 062398a01..636b6c491 100644 --- a/crates/factor-key-value-spin/src/lib.rs +++ b/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, -} - -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 { - // 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, +} + +impl SpinKeyValueRuntimeConfig { + /// Create a new SpinKeyValueRuntimeConfig with the given parent directory + /// where the key-value store will live. + pub fn new(path: Option) -> 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. diff --git a/crates/factor-key-value/src/runtime_config/spin.rs b/crates/factor-key-value/src/runtime_config/spin.rs index 860208b7e..e604d3a5f 100644 --- a/crates/factor-key-value/src/runtime_config/spin.rs +++ b/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(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> { 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()) } } diff --git a/crates/factor-key-value/tests/factor_test.rs b/crates/factor-key-value/tests/factor_test.rs index f796162d6..1b0facbf6 100644 --- a/crates/factor-key-value/tests/factor_test.rs +++ b/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::("default", Default::default())?; + test_resolver + .add_default_store::("default", SpinKeyValueRuntimeConfig::new(None))?; let factors = TestFactors { key_value: KeyValueFactor::new(test_resolver), }; diff --git a/crates/factors/src/runtime_config/toml.rs b/crates/factors/src/runtime_config/toml.rs index 42231a148..23e3ef1e2 100644 --- a/crates/factors/src/runtime_config/toml.rs +++ b/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>, diff --git a/crates/factors/tests/smoke.rs b/crates/factors/tests/smoke.rs index 95e77d7b6..628b29136 100644 --- a/crates/factors/tests/smoke.rs +++ b/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 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::("default", Default::default())?; + key_value_resolver + .add_default_store::("default", SpinKeyValueRuntimeConfig::new(None))?; key_value_resolver.register_store_type(SpinKeyValueStore::new(Some( std::env::current_dir().context("failed to get current directory")?, )))?; diff --git a/crates/runtime-config/src/lib.rs b/crates/runtime-config/src/lib.rs index a7217c8e9..8fe53a17c 100644 --- a/crates/runtime-config/src/lib.rs +++ b/crates/runtime-config/src/lib.rs @@ -3,7 +3,7 @@ use std::path::{Path, PathBuf}; use anyhow::Context as _; use spin_factor_key_value::runtime_config::spin::{self as key_value}; use spin_factor_key_value::{DefaultLabelResolver as _, KeyValueFactor}; -use spin_factor_key_value_spin::SpinKeyValueStore; +use spin_factor_key_value_spin::{SpinKeyValueRuntimeConfig, SpinKeyValueStore}; use spin_factor_llm::{spin as llm, LlmFactor}; use spin_factor_outbound_http::OutboundHttpFactor; use spin_factor_outbound_mqtt::OutboundMqttFactor; @@ -42,24 +42,31 @@ pub struct ResolvedRuntimeConfig { impl ResolvedRuntimeConfig where - T: for<'a> TryFrom>, - for<'a> >>::Error: Into, + T: for<'a, 'b> TryFrom>, + for<'a, 'b> >>::Error: Into, { + /// Creates a new resolved runtime configuration from an optional runtime config source TOML file. + pub fn from_optional_file( + runtime_config_path: Option<&Path>, + provided_state_dir: Option<&Path>, + use_gpu: bool, + ) -> anyhow::Result { + match runtime_config_path { + Some(runtime_config_path) => { + Self::from_file(runtime_config_path, provided_state_dir, use_gpu) + } + None => Self::new(Default::default(), None, provided_state_dir, use_gpu), + } + } + /// Creates a new resolved runtime configuration from a runtime config source TOML file. /// - /// `state_dir` is the explicitly provided state directory, if any. Some("") will be treated as - /// as `None`. + /// `provided_state_dir` is the explicitly provided state directory, if any. pub fn from_file( runtime_config_path: &Path, - state_dir: Option<&str>, + provided_state_dir: Option<&Path>, use_gpu: bool, ) -> anyhow::Result { - let tls_resolver = SpinTlsRuntimeConfig::new(runtime_config_path); - let key_value_config_resolver = key_value_config_resolver(state_dir.map(Into::into)); - - let sqlite_config_resolver = sqlite_config_resolver(state_dir.map(Into::into)) - .context("failed to resolve sqlite runtime config")?; - let file = std::fs::read_to_string(runtime_config_path).with_context(|| { format!( "failed to read runtime config file '{}'", @@ -72,22 +79,37 @@ where runtime_config_path.display() ) })?; + + Self::new(toml, Some(runtime_config_path), provided_state_dir, use_gpu) + } + + /// Creates a new resolved runtime configuration from a TOML table. + pub fn new( + toml: toml::Table, + runtime_config_path: Option<&Path>, + provided_state_dir: Option<&Path>, + use_gpu: bool, + ) -> anyhow::Result { + let toml_resolver = TomlResolver::new(&toml, provided_state_dir); + let tls_resolver = runtime_config_path.map(SpinTlsRuntimeConfig::new); + let key_value_config_resolver = key_value_config_resolver(toml_resolver.state_dir()?); + let sqlite_config_resolver = sqlite_config_resolver(toml_resolver.state_dir()?) + .context("failed to resolve sqlite runtime config")?; + let source = TomlRuntimeConfigSource::new( - &toml, - state_dir, + toml_resolver.clone(), &key_value_config_resolver, - &tls_resolver, + tls_resolver.as_ref(), &sqlite_config_resolver, use_gpu, ); - let state_dir = source.state_dir(); let runtime_config: T = source.try_into().map_err(Into::into)?; Ok(Self { runtime_config, key_value_resolver: key_value_config_resolver, sqlite_resolver: sqlite_config_resolver, - state_dir, + state_dir: toml_resolver.state_dir()?, }) } @@ -118,140 +140,164 @@ where } } -impl ResolvedRuntimeConfig { - /// Creates a new resolved runtime configuration with default values. - pub fn default(state_dir: Option<&str>) -> Self { - let state_dir = state_dir.map(PathBuf::from); +#[derive(Clone, Debug)] +/// Resolves runtime configuration from a TOML file. +pub struct TomlResolver<'a> { + table: TomlKeyTracker<'a>, + /// Explicitly provided state directory. + state_dir: Option<&'a Path>, +} + +impl<'a> TomlResolver<'a> { + /// Create a new TOML resolver. + /// + /// The `state_dir` is the explicitly provided state directory, if any. + pub fn new(table: &'a toml::Table, state_dir: Option<&'a Path>) -> Self { Self { - sqlite_resolver: sqlite_config_resolver(state_dir.clone()) - .expect("failed to resolve sqlite runtime config"), - key_value_resolver: key_value_config_resolver(state_dir.clone()), - runtime_config: Default::default(), + table: TomlKeyTracker::new(table), state_dir, } } + + /// Get the configured state_directory. + /// + /// Errors if the path cannot be converted to an absolute path. + pub fn state_dir(&self) -> std::io::Result> { + let from_toml = || { + self.table + .get("state_dir") + .and_then(|v| v.as_str()) + .filter(|v| !v.is_empty()) + .map(Path::new) + }; + // Prefer explicitly provided state directory, then take from toml. + self.state_dir + .or_else(from_toml) + .map(PathBuf::from) + .map(std::fs::canonicalize) + .transpose() + } + + /// Validate that all keys in the TOML file have been used. + pub fn validate_all_keys_used(&self) -> spin_factors::Result<()> { + self.table.validate_all_keys_used() + } +} + +impl AsRef for TomlResolver<'_> { + fn as_ref(&self) -> &toml::Table { + self.table.as_ref() + } } /// The TOML based runtime configuration source Spin CLI. -pub struct TomlRuntimeConfigSource<'a> { - table: TomlKeyTracker<'a>, - /// Explicitly provided state directory. - state_dir: Option<&'a str>, +pub struct TomlRuntimeConfigSource<'a, 'b> { + toml: TomlResolver<'b>, key_value: &'a key_value::RuntimeConfigResolver, - tls: &'a SpinTlsRuntimeConfig, + tls: Option<&'a SpinTlsRuntimeConfig>, sqlite: &'a sqlite::RuntimeConfigResolver, use_gpu: bool, } -impl<'a> TomlRuntimeConfigSource<'a> { +impl<'a, 'b> TomlRuntimeConfigSource<'a, 'b> { pub fn new( - table: &'a toml::Table, - state_dir: Option<&'a str>, + toml_resolver: TomlResolver<'b>, key_value: &'a key_value::RuntimeConfigResolver, - tls: &'a SpinTlsRuntimeConfig, + tls: Option<&'a SpinTlsRuntimeConfig>, sqlite: &'a sqlite::RuntimeConfigResolver, use_gpu: bool, ) -> Self { Self { - table: TomlKeyTracker::new(table), - state_dir, + toml: toml_resolver, key_value, tls, sqlite, use_gpu, } } - - /// Get the configured state_directory. - pub fn state_dir(&self) -> Option { - let from_toml = || self.table.get("state_dir").and_then(|v| v.as_str()); - // Prefer explicitly provided state directory, then take from toml. - self.state_dir - .or_else(from_toml) - // Treat "" as None. - .filter(|s| !s.is_empty()) - .map(PathBuf::from) - } } -impl FactorRuntimeConfigSource for TomlRuntimeConfigSource<'_> { +impl FactorRuntimeConfigSource for TomlRuntimeConfigSource<'_, '_> { fn get_runtime_config( &mut self, ) -> anyhow::Result> { - self.key_value.resolve_from_toml(Some(self.table.as_ref())) + self.key_value.resolve_from_toml(Some(self.toml.as_ref())) } } -impl FactorRuntimeConfigSource for TomlRuntimeConfigSource<'_> { +impl FactorRuntimeConfigSource for TomlRuntimeConfigSource<'_, '_> { fn get_runtime_config( &mut self, ) -> anyhow::Result::RuntimeConfig>> { - self.tls.config_from_table(self.table.as_ref()) + let Some(tls) = self.tls else { + return Ok(None); + }; + tls.config_from_table(self.toml.as_ref()) } } -impl FactorRuntimeConfigSource for TomlRuntimeConfigSource<'_> { +impl FactorRuntimeConfigSource for TomlRuntimeConfigSource<'_, '_> { fn get_runtime_config( &mut self, ) -> anyhow::Result::RuntimeConfig>> { Ok(Some(variables::runtime_config_from_toml( - self.table.as_ref(), + self.toml.as_ref(), )?)) } } -impl FactorRuntimeConfigSource for TomlRuntimeConfigSource<'_> { +impl FactorRuntimeConfigSource for TomlRuntimeConfigSource<'_, '_> { fn get_runtime_config(&mut self) -> anyhow::Result> { Ok(None) } } -impl FactorRuntimeConfigSource for TomlRuntimeConfigSource<'_> { +impl FactorRuntimeConfigSource for TomlRuntimeConfigSource<'_, '_> { fn get_runtime_config(&mut self) -> anyhow::Result> { Ok(None) } } -impl FactorRuntimeConfigSource for TomlRuntimeConfigSource<'_> { +impl FactorRuntimeConfigSource for TomlRuntimeConfigSource<'_, '_> { fn get_runtime_config(&mut self) -> anyhow::Result> { - llm::runtime_config_from_toml(self.table.as_ref(), self.state_dir(), self.use_gpu) + llm::runtime_config_from_toml(self.toml.as_ref(), self.toml.state_dir()?, self.use_gpu) } } -impl FactorRuntimeConfigSource for TomlRuntimeConfigSource<'_> { +impl FactorRuntimeConfigSource for TomlRuntimeConfigSource<'_, '_> { fn get_runtime_config(&mut self) -> anyhow::Result> { Ok(None) } } -impl FactorRuntimeConfigSource for TomlRuntimeConfigSource<'_> { +impl FactorRuntimeConfigSource for TomlRuntimeConfigSource<'_, '_> { fn get_runtime_config(&mut self) -> anyhow::Result> { Ok(None) } } -impl FactorRuntimeConfigSource for TomlRuntimeConfigSource<'_> { +impl FactorRuntimeConfigSource for TomlRuntimeConfigSource<'_, '_> { fn get_runtime_config(&mut self) -> anyhow::Result> { Ok(None) } } -impl FactorRuntimeConfigSource for TomlRuntimeConfigSource<'_> { +impl FactorRuntimeConfigSource for TomlRuntimeConfigSource<'_, '_> { fn get_runtime_config(&mut self) -> anyhow::Result> { Ok(None) } } -impl FactorRuntimeConfigSource for TomlRuntimeConfigSource<'_> { +impl FactorRuntimeConfigSource for TomlRuntimeConfigSource<'_, '_> { fn get_runtime_config(&mut self) -> anyhow::Result> { - self.sqlite.resolve_from_toml(self.table.as_ref()) + self.sqlite.resolve_from_toml(self.toml.as_ref()) } } -impl RuntimeConfigSourceFinalizer for TomlRuntimeConfigSource<'_> { +impl RuntimeConfigSourceFinalizer for TomlRuntimeConfigSource<'_, '_> { fn finalize(&mut self) -> anyhow::Result<()> { - Ok(self.table.validate_all_keys_used()?) + Ok(self.toml.validate_all_keys_used()?) } } @@ -259,7 +305,8 @@ const DEFAULT_KEY_VALUE_STORE_LABEL: &str = "default"; /// The key-value runtime configuration resolver. /// -/// Takes a base path for the local store. +/// Takes a base path that all local key-value stores which are configured with +/// relative paths will be relative to. pub fn key_value_config_resolver( local_store_base_path: Option, ) -> key_value::RuntimeConfigResolver { @@ -282,11 +329,18 @@ pub fn key_value_config_resolver( // Add handling of "default" store. // Unwraps are safe because the store is known to be serializable as toml. key_value - .add_default_store::(DEFAULT_KEY_VALUE_STORE_LABEL, Default::default()) + .add_default_store::( + DEFAULT_KEY_VALUE_STORE_LABEL, + SpinKeyValueRuntimeConfig::new( + local_store_base_path.map(|p| p.join(DEFAULT_SPIN_STORE_FILENAME)), + ), + ) .unwrap(); key_value } +/// The default filename for the SQLite database. +const DEFAULT_SPIN_STORE_FILENAME: &'static str = "sqlite_key_value.db"; /// The sqlite runtime configuration resolver. /// diff --git a/crates/trigger/src/cli.rs b/crates/trigger/src/cli.rs index 6aa99ab4a..bee7f81e1 100644 --- a/crates/trigger/src/cli.rs +++ b/crates/trigger/src/cli.rs @@ -304,17 +304,16 @@ impl TriggerAppBuilder { }; self.trigger.add_to_linker(core_engine_builder.linker())?; + // Hardcode `use_gpu` to true for now let use_gpu = true; - let runtime_config = match options.runtime_config_file { - Some(runtime_config_path) => { - ResolvedRuntimeConfig::::from_file( - runtime_config_path, - options.state_dir, - use_gpu, - )? - } - None => ResolvedRuntimeConfig::default(options.state_dir), - }; + // Make sure `--state-dir=""` unsets the state dir + let state_dir = options.state_dir.filter(|s| !s.is_empty()).map(Path::new); + let runtime_config = + ResolvedRuntimeConfig::::from_optional_file( + options.runtime_config_file, + state_dir, + use_gpu, + )?; runtime_config .set_initial_key_values(&options.initial_key_values) diff --git a/crates/trigger/src/factors.rs b/crates/trigger/src/factors.rs index 1aa92f13e..3a1b0993a 100644 --- a/crates/trigger/src/factors.rs +++ b/crates/trigger/src/factors.rs @@ -81,10 +81,10 @@ fn outbound_networking_factor() -> OutboundNetworkingFactor { factor } -impl TryFrom> for TriggerFactorsRuntimeConfig { +impl TryFrom> for TriggerFactorsRuntimeConfig { type Error = anyhow::Error; - fn try_from(value: TomlRuntimeConfigSource<'_>) -> Result { + fn try_from(value: TomlRuntimeConfigSource<'_, '_>) -> Result { Self::from_source(value) } }