diff --git a/crates/expressions/src/lib.rs b/crates/expressions/src/lib.rs index 30eb200003..4ef0552464 100644 --- a/crates/expressions/src/lib.rs +++ b/crates/expressions/src/lib.rs @@ -11,8 +11,6 @@ pub use provider::Provider; use template::Part; pub use template::Template; -use crate::provider::ProviderVariableKind; - /// A [`ProviderResolver`] that can be shared. pub type SharedPreparedResolver = std::sync::Arc>>; @@ -92,50 +90,25 @@ impl ProviderResolver { Ok(PreparedResolver { variables }) } - /// Ensures that all required variables are resolvable at startup - pub async fn ensure_required_variables_resolvable(&self) -> Result<()> { - // If a dynamic provider is available, skip validation. - if self - .providers - .iter() - .any(|p| p.kind() == ProviderVariableKind::Dynamic) - { - return Ok(()); - } - - let mut unvalidated_keys = vec![]; - for key in self.internal.variables.keys() { - // If default value is provided, skip validation. - if let Some(value) = self.internal.variables.get(key) { - if value.default.is_some() { - continue; - } - } - - let mut resolved = false; - - for provider in &self.providers { - if provider - .get(&Key(key)) - .await - .map_err(Error::Provider)? - .is_some() - { - resolved = true; - break; - } - } - - if !resolved { - unvalidated_keys.push(key); + /// Ensures that all required variables are not unresolvable + pub fn ensure_required_variables_resolvable(&self) -> Result<()> { + let mut unresolvable_keys = vec![]; + for key in self.internal.required_variables() { + let key = Key::new(key)?; + let resolvable = self + .providers + .iter() + .any(|provider| provider.may_resolve(&key)); + if !resolvable { + unresolvable_keys.push(key); } } - if unvalidated_keys.is_empty() { + if unresolvable_keys.is_empty() { Ok(()) } else { Err(Error::Provider(anyhow::anyhow!( - "no provider resolved required variable(s): {unvalidated_keys:?}", + "no provider resolved required variable(s): {unresolvable_keys:?}", ))) } } @@ -248,6 +221,12 @@ impl Resolver { })?; Ok(template) } + + fn required_variables(&self) -> impl Iterator { + self.variables + .iter() + .filter_map(|(name, variable)| variable.default.is_none().then_some(name.as_str())) + } } /// A resolver who has resolved all variables. @@ -360,7 +339,6 @@ mod tests { use async_trait::async_trait; use super::*; - use crate::provider::ProviderVariableKind; #[derive(Debug)] struct TestProvider; @@ -375,8 +353,8 @@ mod tests { } } - fn kind(&self) -> ProviderVariableKind { - ProviderVariableKind::Static + fn may_resolve(&self, key: &Key) -> bool { + key.as_ref() == "required" } } diff --git a/crates/expressions/src/provider.rs b/crates/expressions/src/provider.rs index 30ef1bd3c0..cc69910f1e 100644 --- a/crates/expressions/src/provider.rs +++ b/crates/expressions/src/provider.rs @@ -9,15 +9,13 @@ use crate::Key; pub trait Provider: Debug + Send + Sync { /// Returns the value at the given config path, if it exists. async fn get(&self, key: &Key) -> anyhow::Result>; - fn kind(&self) -> ProviderVariableKind; -} -/// The dynamism of a Provider. -#[derive(Clone, Debug, Default, PartialEq)] -pub enum ProviderVariableKind { - /// Variable must be declared on start - Static, - /// Variable can be made available at runtime - #[default] - Dynamic, + /// Returns true if the given key _might_ be resolvable by this Provider. + /// + /// Dynamic resolvers will typically return true unconditionally, which is + /// the default implementation. + fn may_resolve(&self, key: &Key) -> bool { + let _ = key; + true + } } diff --git a/crates/expressions/tests/validation_test.rs b/crates/expressions/tests/validation_test.rs index 4da78cca3b..2f5c1d1f42 100644 --- a/crates/expressions/tests/validation_test.rs +++ b/crates/expressions/tests/validation_test.rs @@ -1,7 +1,7 @@ use std::collections::HashMap; use async_trait::async_trait; -use spin_expressions::{provider::ProviderVariableKind, Key, Provider, ProviderResolver}; +use spin_expressions::{Key, Provider, ProviderResolver}; use spin_locked_app::Variable; #[derive(Default)] @@ -43,8 +43,8 @@ impl ResolverTester { } } -#[tokio::test(flavor = "multi_thread")] -async fn if_single_static_provider_with_no_key_to_resolve_is_valid() -> anyhow::Result<()> { +#[test] +fn if_single_static_provider_with_no_key_to_resolve_is_valid() -> anyhow::Result<()> { let resolver = ResolverTester::new() .with_provider(StaticProvider::with_variable( "database_host", @@ -52,14 +52,14 @@ async fn if_single_static_provider_with_no_key_to_resolve_is_valid() -> anyhow:: )) .make_resolver()?; - resolver.ensure_required_variables_resolvable().await?; + resolver.ensure_required_variables_resolvable()?; Ok(()) } -#[tokio::test(flavor = "multi_thread")] -async fn if_single_static_provider_has_data_for_variable_key_to_resolve_it_succeeds( -) -> anyhow::Result<()> { +#[test] +fn if_single_static_provider_has_data_for_variable_key_to_resolve_it_succeeds() -> anyhow::Result<()> +{ let resolver = ResolverTester::new() .with_provider(StaticProvider::with_variable( "database_host", @@ -68,13 +68,13 @@ async fn if_single_static_provider_has_data_for_variable_key_to_resolve_it_succe .with_variable("database_host", None) .make_resolver()?; - resolver.ensure_required_variables_resolvable().await?; + resolver.ensure_required_variables_resolvable()?; Ok(()) } -#[tokio::test(flavor = "multi_thread")] -async fn if_there_is_a_single_static_provider_and_it_does_not_contain_a_required_variable_then_validation_fails( +#[test] +fn if_there_is_a_single_static_provider_and_it_does_not_contain_a_required_variable_then_validation_fails( ) -> anyhow::Result<()> { let resolver = ResolverTester::new() .with_provider(StaticProvider::with_variable( @@ -84,29 +84,26 @@ async fn if_there_is_a_single_static_provider_and_it_does_not_contain_a_required .with_variable("api_key", None) .make_resolver()?; - assert!(resolver - .ensure_required_variables_resolvable() - .await - .is_err()); + assert!(resolver.ensure_required_variables_resolvable().is_err()); Ok(()) } -#[tokio::test(flavor = "multi_thread")] -async fn if_there_is_a_dynamic_provider_then_validation_succeeds_even_without_default_value_in_play( +#[test] +fn if_there_is_a_dynamic_provider_then_validation_succeeds_even_without_default_value_in_play( ) -> anyhow::Result<()> { let resolver = ResolverTester::new() .with_provider(DynamicProvider) .with_variable("api_key", None) .make_resolver()?; - resolver.ensure_required_variables_resolvable().await?; + resolver.ensure_required_variables_resolvable()?; Ok(()) } -#[tokio::test(flavor = "multi_thread")] -async fn if_there_is_a_dynamic_provider_and_static_provider_but_the_variable_to_be_resolved_is_not_in_play( +#[test] +fn if_there_is_a_dynamic_provider_and_static_provider_but_the_variable_to_be_resolved_is_not_in_play( ) -> anyhow::Result<()> { let resolver = ResolverTester::new() .with_provider(DynamicProvider) @@ -117,13 +114,13 @@ async fn if_there_is_a_dynamic_provider_and_static_provider_but_the_variable_to_ .with_variable("api_key", None) .make_resolver()?; - resolver.ensure_required_variables_resolvable().await?; + resolver.ensure_required_variables_resolvable()?; Ok(()) } -#[tokio::test(flavor = "multi_thread")] -async fn if_there_is_a_dynamic_provider_and_a_static_provider_then_validation_succeeds_even_if_there_is_a_variable_in_play( +#[test] +fn if_there_is_a_dynamic_provider_and_a_static_provider_then_validation_succeeds_even_if_there_is_a_variable_in_play( ) -> anyhow::Result<()> { let resolver = ResolverTester::new() .with_provider(DynamicProvider) @@ -134,13 +131,13 @@ async fn if_there_is_a_dynamic_provider_and_a_static_provider_then_validation_su .with_variable("api_key", Some("super-secret-key")) .make_resolver()?; - resolver.ensure_required_variables_resolvable().await?; + resolver.ensure_required_variables_resolvable()?; Ok(()) } -#[tokio::test(flavor = "multi_thread")] -async fn if_there_are_two_static_providers_where_one_has_data_is_valid() -> anyhow::Result<()> { +#[test] +fn if_there_are_two_static_providers_where_one_has_data_is_valid() -> anyhow::Result<()> { let resolver = ResolverTester::new() .with_provider(StaticProvider::with_variable( "database_host", @@ -153,14 +150,14 @@ async fn if_there_are_two_static_providers_where_one_has_data_is_valid() -> anyh .with_variable("database_host", None) .make_resolver()?; - resolver.ensure_required_variables_resolvable().await?; + resolver.ensure_required_variables_resolvable()?; Ok(()) } // Ensure that if there are two or more static providers and the first one does not have data for the variable to be resolved, // but the second or subsequent one does, then validation still succeeds. -#[tokio::test(flavor = "multi_thread")] -async fn if_there_are_two_static_providers_where_first_provider_does_not_have_data_while_second_provider_does( +#[test] +fn if_there_are_two_static_providers_where_first_provider_does_not_have_data_while_second_provider_does( ) -> anyhow::Result<()> { let resolver = ResolverTester::new() .with_provider(StaticProvider::with_variable( @@ -174,13 +171,13 @@ async fn if_there_are_two_static_providers_where_first_provider_does_not_have_da .with_variable("api_key", None) .make_resolver()?; - resolver.ensure_required_variables_resolvable().await?; + resolver.ensure_required_variables_resolvable()?; Ok(()) } -#[tokio::test(flavor = "multi_thread")] -async fn if_there_is_two_static_providers_neither_having_data_is_invalid() -> anyhow::Result<()> { +#[test] +fn if_there_is_two_static_providers_neither_having_data_is_invalid() -> anyhow::Result<()> { let resolver = ResolverTester::new() .with_provider(StaticProvider::with_variable( "database_host", @@ -193,37 +190,30 @@ async fn if_there_is_two_static_providers_neither_having_data_is_invalid() -> an .with_variable("hello", None) .make_resolver()?; - assert!(resolver - .ensure_required_variables_resolvable() - .await - .is_err()); + assert!(resolver.ensure_required_variables_resolvable().is_err()); Ok(()) } -#[tokio::test(flavor = "multi_thread")] -async fn no_provider_data_available_but_variable_default_value_needed_is_invalid( -) -> anyhow::Result<()> { +#[test] +fn no_provider_data_available_but_variable_default_value_needed_is_invalid() -> anyhow::Result<()> { let resolver = ResolverTester::new() .with_variable("api_key", None) .make_resolver()?; - assert!(resolver - .ensure_required_variables_resolvable() - .await - .is_err()); + assert!(resolver.ensure_required_variables_resolvable().is_err()); Ok(()) } -#[tokio::test(flavor = "multi_thread")] -async fn no_provider_data_available_but_variable_has_default_value_needed_is_valid( -) -> anyhow::Result<()> { +#[test] +fn no_provider_data_available_but_variable_has_default_value_needed_is_valid() -> anyhow::Result<()> +{ let resolver = ResolverTester::new() .with_variable("api_key", Some("super-secret-key")) .make_resolver()?; - resolver.ensure_required_variables_resolvable().await?; + resolver.ensure_required_variables_resolvable()?; Ok(()) } @@ -247,8 +237,8 @@ impl Provider for StaticProvider { Ok(self.variables.get(key.as_str()).cloned().flatten()) } - fn kind(&self) -> ProviderVariableKind { - ProviderVariableKind::Static + fn may_resolve(&self, key: &Key) -> bool { + self.variables.contains_key(key.as_str()) } } @@ -260,8 +250,4 @@ impl Provider for DynamicProvider { async fn get(&self, _key: &Key) -> anyhow::Result> { panic!("validation should never call get for a dynamic provider") } - - fn kind(&self) -> ProviderVariableKind { - ProviderVariableKind::Dynamic - } } diff --git a/crates/factor-variables/tests/factor_test.rs b/crates/factor-variables/tests/factor_test.rs index bbdf3e2428..b7fee44697 100644 --- a/crates/factor-variables/tests/factor_test.rs +++ b/crates/factor-variables/tests/factor_test.rs @@ -1,4 +1,4 @@ -use spin_expressions::{provider::ProviderVariableKind, Key, Provider}; +use spin_expressions::{Key, Provider}; use spin_factor_variables::{runtime_config::RuntimeConfig, VariablesFactor}; use spin_factors::{anyhow, RuntimeFactors}; use spin_factors_test::{toml, TestEnvironment}; @@ -47,7 +47,7 @@ impl Provider for MockProvider { } } - fn kind(&self) -> ProviderVariableKind { - ProviderVariableKind::Static + fn may_resolve(&self, key: &Key) -> bool { + key.as_ref() == "foo" } } diff --git a/crates/trigger/src/cli/variable.rs b/crates/trigger/src/cli/variable.rs index 71a0db3f81..b0662b800b 100644 --- a/crates/trigger/src/cli/variable.rs +++ b/crates/trigger/src/cli/variable.rs @@ -14,9 +14,7 @@ impl ExecutorHooks for VariablesValidatorHook { let variables_factor_app_state = configured_app.app_state::()?; let expression_resolver = variables_factor_app_state.expression_resolver(); - expression_resolver - .ensure_required_variables_resolvable() - .await?; + expression_resolver.ensure_required_variables_resolvable()?; Ok(()) } diff --git a/crates/variables-azure/src/lib.rs b/crates/variables-azure/src/lib.rs index 908c86c95b..691c980a2b 100644 --- a/crates/variables-azure/src/lib.rs +++ b/crates/variables-azure/src/lib.rs @@ -4,7 +4,7 @@ use anyhow::Context as _; use azure_core::{auth::TokenCredential, Url}; use azure_security_keyvault::SecretClient; use serde::Deserialize; -use spin_expressions::{provider::ProviderVariableKind, Key, Provider}; +use spin_expressions::{Key, Provider}; use spin_factors::anyhow; use spin_world::async_trait; use tracing::{instrument, Level}; @@ -151,10 +151,6 @@ impl Provider for AzureKeyVaultProvider { .context("Failed to read variable from Azure Key Vault")?; Ok(Some(secret.value)) } - - fn kind(&self) -> ProviderVariableKind { - ProviderVariableKind::Dynamic - } } impl From for Url { diff --git a/crates/variables-env/src/lib.rs b/crates/variables-env/src/lib.rs index 8ea94073a8..7b2de92955 100644 --- a/crates/variables-env/src/lib.rs +++ b/crates/variables-env/src/lib.rs @@ -6,7 +6,7 @@ use std::{ }; use serde::Deserialize; -use spin_expressions::{provider::ProviderVariableKind, Key, Provider}; +use spin_expressions::{Key, Provider}; use spin_factors::anyhow::{self, Context as _}; use spin_world::async_trait; use tracing::{instrument, Level}; @@ -133,8 +133,8 @@ impl Provider for EnvVariablesProvider { tokio::task::block_in_place(|| self.get_sync(key)) } - fn kind(&self) -> ProviderVariableKind { - ProviderVariableKind::Static + fn may_resolve(&self, key: &Key) -> bool { + matches!(self.get_sync(key), Ok(Some(_))) } } diff --git a/crates/variables-static/src/lib.rs b/crates/variables-static/src/lib.rs index da69c77c35..ab17059080 100644 --- a/crates/variables-static/src/lib.rs +++ b/crates/variables-static/src/lib.rs @@ -1,7 +1,7 @@ use std::{collections::HashMap, hash::Hash, sync::Arc}; use serde::Deserialize; -use spin_expressions::{async_trait::async_trait, provider::ProviderVariableKind, Key, Provider}; +use spin_expressions::{async_trait::async_trait, Key, Provider}; use spin_factors::anyhow; pub use source::*; @@ -19,8 +19,8 @@ impl Provider for StaticVariablesProvider { Ok(self.values.get(key.as_str()).cloned()) } - fn kind(&self) -> ProviderVariableKind { - ProviderVariableKind::Static + fn may_resolve(&self, key: &Key) -> bool { + self.values.contains_key(key.as_str()) } } diff --git a/crates/variables-vault/src/lib.rs b/crates/variables-vault/src/lib.rs index bfdba99520..0bd7e8ed6c 100644 --- a/crates/variables-vault/src/lib.rs +++ b/crates/variables-vault/src/lib.rs @@ -1,5 +1,5 @@ use serde::{Deserialize, Serialize}; -use spin_expressions::{async_trait::async_trait, provider::ProviderVariableKind}; +use spin_expressions::async_trait::async_trait; use spin_factors::anyhow::{self, Context as _}; use tracing::{instrument, Level}; use vaultrs::{ @@ -52,8 +52,4 @@ impl Provider for VaultVariablesProvider { Err(e) => Err(e).context("Failed to check Vault for config"), } } - - fn kind(&self) -> ProviderVariableKind { - ProviderVariableKind::Dynamic - } }