Merge pull request #3282 from spinframework/provider-may-resolve

variables: Refactor Provider::kind into ::may_resolve

Author: lann

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<std::sync::OnceLock<std::sync::Arc<PreparedResolver>>>;
@@ -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<Item = &str> {
+        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"
         }
     }
 

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<Option<String>>;
-    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
+    }
 }

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,23 +43,23 @@ 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",
             Some("localhost"),
         ))
         .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<Option<String>> {
         panic!("validation should never call get for a dynamic provider")
     }
-
-    fn kind(&self) -> ProviderVariableKind {
-        ProviderVariableKind::Dynamic
-    }
 }

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"
     }
 }

crates/trigger/src/cli/variable.rs

@@ -14,9 +14,7 @@ impl<F: RuntimeFactors, U> ExecutorHooks<F, U> for VariablesValidatorHook {
         let variables_factor_app_state = configured_app.app_state::<VariablesFactor>()?;
 
         let expression_resolver = variables_factor_app_state.expression_resolver();
-        expression_resolver
-            .ensure_required_variables_resolvable()
-            .await?;
+        expression_resolver.ensure_required_variables_resolvable()?;
 
         Ok(())
     }