fixes validation check logic

Signed-off-by: Aminu Oluwaseun Joshua <[email protected]>

Author: seun-ja

Cargo.lock

@@ -8,7 +8,6 @@ edition = { workspace = true }
 anyhow = { workspace = true }
 async-trait = { workspace = true }
 futures = { workspace = true }
-serde = { workspace = true }
 spin-locked-app = { path = "../locked-app" }
 thiserror = { workspace = true }
 

crates/expressions/Cargo.toml

@@ -1,7 +1,7 @@
 pub mod provider;
 mod template;
 
-use std::{borrow::Cow, collections::HashMap, fmt::Debug};
+use std::{borrow::Cow, collections::HashMap, fmt::Debug, vec};
 
 use spin_locked_app::Variable;
 
@@ -92,28 +92,45 @@ impl ProviderResolver {
         Ok(PreparedResolver { variables })
     }
 
-    /// Prepares the resolver by attempting to resolve all variables, printing warnings for any
-    /// that cannot be resolved.
-    pub async fn pre_runtime_prepare(&self) -> Result<()> {
-        for name in self.internal.variables.keys() {
-            self.check_variable_existence(name).await?;
+    /// Validates `Provider` that are `ProviderVariableKind::Static` provides its variable during startup.
+    pub async fn validate_variable_existence(&self) -> Result<()> {
+        // If a dynamic provider is available, skip validation.
+        if self
+            .providers
+            .iter()
+            .any(|p| p.kind() == ProviderVariableKind::Dynamic)
+        {
+            return Ok(());
         }
-        Ok(())
-    }
 
-    async fn check_variable_existence(&self, key: &str) -> Result<()> {
-        for provider in &self.providers {
-            if provider.kind() == &ProviderVariableKind::Dynamic {
-                continue;
+        let mut unvalidated_keys = vec![];
+        for key in self.internal.variables.keys() {
+            let mut resolved = false;
+
+            for provider in &self.providers {
+                if provider
+                    .get(&Key(key))
+                    .await
+                    .map_err(Error::Provider)?
+                    .is_some()
+                {
+                    resolved = true;
+                    break;
+                }
             }
 
-            match provider.get(&Key(key)).await {
-                Ok(Some(_)) => return Ok(()),
-                Err(_) | Ok(None) => return self.internal.resolve_variable(key).map(|_| ()),
+            if !resolved {
+                unvalidated_keys.push(key);
             }
         }
 
-        Ok(())
+        if unvalidated_keys.is_empty() {
+            Ok(())
+        } else {
+            Err(Error::Provider(anyhow::anyhow!(
+                "no provider resolved required variables: {unvalidated_keys:?}",
+            )))
+        }
     }
 
     async fn resolve_variable(&self, key: &str) -> Result<String> {
@@ -351,8 +368,8 @@ mod tests {
             }
         }
 
-        fn kind(&self) -> &ProviderVariableKind {
-            &ProviderVariableKind::Static
+        fn kind(&self) -> ProviderVariableKind {
+            ProviderVariableKind::Static
         }
     }
 

crates/expressions/src/lib.rs

@@ -1,7 +1,6 @@
 use std::fmt::Debug;
 
 use async_trait::async_trait;
-use serde::Deserialize;
 
 use crate::Key;
 
@@ -10,17 +9,15 @@ 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;
+    fn kind(&self) -> ProviderVariableKind;
 }
 
 /// The dynamism of a Provider.
-#[derive(Clone, Debug, Default, Deserialize, PartialEq)]
+#[derive(Clone, Debug, Default, PartialEq)]
 pub enum ProviderVariableKind {
     /// Variable must be declared on start
-    #[serde(rename = "static")]
     Static,
     /// Variable can be made available at runtime
-    #[serde(rename = "dynamic")]
     #[default]
     Dynamic,
 }

crates/expressions/src/provider.rs

@@ -47,7 +47,7 @@ impl Provider for MockProvider {
         }
     }
 
-    fn kind(&self) -> &ProviderVariableKind {
-        &ProviderVariableKind::Dynamic
+    fn kind(&self) -> ProviderVariableKind {
+        ProviderVariableKind::Dynamic
     }
 }

crates/factor-variables/tests/factor_test.rs

@@ -8,7 +8,7 @@ use spin_runtime_config::ResolvedRuntimeConfig;
 use spin_trigger::cli::{
     FactorsConfig, InitialKvSetterHook, KeyValueDefaultStoreSummaryHook, MaxInstanceMemoryHook,
     RuntimeFactorsBuilder, SqlStatementExecutorHook, SqliteDefaultStoreSummaryHook,
-    StdioLoggingExecutorHooks, VariablesPreparationExecutorHook,
+    StdioLoggingExecutorHooks, VariablesValidatorHook,
 };
 use spin_variables_static::StaticVariablesProvider;
 
@@ -72,7 +72,7 @@ impl RuntimeFactorsBuilder for FactorsBuilder {
         executor.add_hooks(InitialKvSetterHook::new(args.key_values.clone()));
         executor.add_hooks(SqliteDefaultStoreSummaryHook);
         executor.add_hooks(KeyValueDefaultStoreSummaryHook);
-        executor.add_hooks(VariablesPreparationExecutorHook);
+        executor.add_hooks(VariablesValidatorHook);
 
         let max_instance_memory = args
             .max_instance_memory

crates/runtime-factors/src/build.rs

@@ -26,7 +26,7 @@ pub use sqlite_statements::SqlStatementExecutorHook;
 use stdio::FollowComponents;
 pub use stdio::StdioLoggingExecutorHooks;
 pub use summary::{KeyValueDefaultStoreSummaryHook, SqliteDefaultStoreSummaryHook};
-pub use variable::VariablesPreparationExecutorHook;
+pub use variable::VariablesValidatorHook;
 
 pub const APP_LOG_DIR: &str = "APP_LOG_DIR";
 pub const SPIN_TRUNCATE_LOGS: &str = "SPIN_TRUNCATE_LOGS";

crates/trigger/src/cli.rs

@@ -3,18 +3,18 @@ use spin_factors::RuntimeFactors;
 use spin_factors_executor::ExecutorHooks;
 
 /// An executor hook that prepares the variables factor before runtime execution.
-pub struct VariablesPreparationExecutorHook;
+pub struct VariablesValidatorHook;
 
 #[spin_core::async_trait]
-impl<F: RuntimeFactors, U> ExecutorHooks<F, U> for VariablesPreparationExecutorHook {
+impl<F: RuntimeFactors, U> ExecutorHooks<F, U> for VariablesValidatorHook {
     async fn configure_app(
         &self,
         configured_app: &spin_factors::ConfiguredApp<F>,
     ) -> anyhow::Result<()> {
         let variables_factor = configured_app.app_state::<VariablesFactor>()?;
 
         let expression_resolver = variables_factor.expression_resolver();
-        expression_resolver.pre_runtime_prepare().await?;
+        expression_resolver.validate_variable_existence().await?;
 
         Ok(())
     }

crates/trigger/src/cli/variable.rs

@@ -107,7 +107,6 @@ pub enum AzureKeyVaultAuthOptions {
 #[derive(Debug)]
 pub struct AzureKeyVaultProvider {
     secret_client: SecretClient,
-    kind: ProviderVariableKind,
 }
 
 impl AzureKeyVaultProvider {
@@ -137,7 +136,6 @@ impl AzureKeyVaultProvider {
 
         Ok(Self {
             secret_client: SecretClient::new(&vault_url.into(), token_credential)?,
-            kind: ProviderVariableKind::Dynamic,
         })
     }
 }
@@ -154,8 +152,8 @@ impl Provider for AzureKeyVaultProvider {
         Ok(Some(secret.value))
     }
 
-    fn kind(&self) -> &ProviderVariableKind {
-        &self.kind
+    fn kind(&self) -> ProviderVariableKind {
+        ProviderVariableKind::Dynamic
     }
 }
 

crates/variables-azure/src/lib.rs

@@ -35,7 +35,6 @@ pub struct EnvVariablesProvider {
     env_fetcher: EnvFetcherFn,
     dotenv_path: Option<PathBuf>,
     dotenv_cache: OnceLock<HashMap<String, String>>,
-    kind: ProviderVariableKind,
 }
 
 impl Default for EnvVariablesProvider {
@@ -45,7 +44,6 @@ impl Default for EnvVariablesProvider {
             env_fetcher: Box::new(|s| std::env::var(s)),
             dotenv_path: Some(".env".into()),
             dotenv_cache: Default::default(),
-            kind: ProviderVariableKind::Static,
         }
     }
 }
@@ -68,7 +66,6 @@ impl EnvVariablesProvider {
             dotenv_path,
             env_fetcher: Box::new(env_fetcher),
             dotenv_cache: Default::default(),
-            kind: ProviderVariableKind::Static,
         }
     }
 
@@ -136,8 +133,8 @@ impl Provider for EnvVariablesProvider {
         tokio::task::block_in_place(|| self.get_sync(key))
     }
 
-    fn kind(&self) -> &ProviderVariableKind {
-        &self.kind
+    fn kind(&self) -> ProviderVariableKind {
+        ProviderVariableKind::Static
     }
 }