From 527a19e67ed43db8c390c16a375c24bc4d6bd148 Mon Sep 17 00:00:00 2001 From: Michelle Noorali Date: Tue, 21 Jul 2026 19:17:59 -0400 Subject: [PATCH 1/2] rm outdated trigger compatibility check for manifest v2 + resolves #3626 Signed-off-by: Michelle Noorali --- crates/templates/src/app_info.rs | 84 ++++++++++++++++++++++---------- crates/templates/src/run.rs | 7 ++- crates/templates/src/template.rs | 2 +- 3 files changed, 65 insertions(+), 28 deletions(-) diff --git a/crates/templates/src/app_info.rs b/crates/templates/src/app_info.rs index 9a6df58b78..6f09ea486b 100644 --- a/crates/templates/src/app_info.rs +++ b/crates/templates/src/app_info.rs @@ -8,7 +8,6 @@ use std::{ sync::LazyLock, }; -use anyhow::ensure; use serde::Deserialize; use spin_manifest::schema::v1; @@ -16,7 +15,11 @@ use crate::store::TemplateLayout; pub(crate) struct AppInfo { manifest_format: u32, - trigger_type: Option, // None = v2 template does not contain any triggers yet + // Used for v1 manifests, which have exactly one trigger type and are the + // only manifests subject to a compatibility check. v2 manifests may host + // multiple trigger types. Triggerless templates also allowed for all + // manifest versions. + trigger_types: Option>, } impl AppInfo { @@ -53,27 +56,28 @@ impl AppInfo { spin_manifest::ManifestVersion::V1 => 1, spin_manifest::ManifestVersion::V2 => 2, }; - let trigger_type = match manifest_version { - spin_manifest::ManifestVersion::V1 => Some( + let trigger_types = match manifest_version { + // v1 manifests always have exactly one trigger type. + spin_manifest::ManifestVersion::V1 => Some(vec![ toml::from_str::(manifest_str)? .trigger .trigger_type, - ), + ]), + // v2 manifests may declare multiple trigger types. spin_manifest::ManifestVersion::V2 => { let triggers = toml::from_str::(manifest_str)? .trigger .unwrap_or_default(); - let type_count = triggers.len(); - ensure!( - type_count <= 1, - "only 1 trigger type currently supported; got {type_count}" - ); - triggers.into_iter().next().map(|t| t.0) + if triggers.is_empty() { + None + } else { + Some(triggers.into_iter().map(|(trigger_type, _)| trigger_type).collect()) + } } }; Ok(Self { manifest_format, - trigger_type, + trigger_types, }) } @@ -104,20 +108,22 @@ impl AppInfo { } fn from_v2_template_text(manifest_tpl_str: &str) -> anyhow::Result { - let trigger_types: HashSet<_> = manifest_tpl_str + // Preserve declaration order while removing duplicate trigger types. + // Order preservation useful for UX, predictable trigger listing. + let mut seen = HashSet::new(); + let trigger_types: Vec = manifest_tpl_str .lines() .filter_map(infer_trigger_type_from_raw_line) + .filter(|trigger_type| seen.insert(trigger_type.clone())) .collect(); - let type_count = trigger_types.len(); - ensure!( - type_count <= 1, - "only 1 trigger type currently supported; got {type_count}" - ); - let trigger_type = trigger_types.into_iter().next(); - + let trigger_types = if trigger_types.is_empty() { + None + } else { + Some(trigger_types) + }; Ok(Self { manifest_format: 2, - trigger_type, + trigger_types, }) } @@ -125,8 +131,8 @@ impl AppInfo { self.manifest_format } - pub fn trigger_type(&self) -> Option<&str> { - self.trigger_type.as_deref() + pub fn trigger_types(&self) -> Option<&[String]> { + self.trigger_types.as_deref() } } @@ -197,7 +203,7 @@ mod test { let info = AppInfo::from_template_text(tpl).unwrap(); assert_eq!(1, info.manifest_format); - assert_eq!("triggy", info.trigger_type.unwrap()); + assert_eq!(vec!["triggy".to_owned()], info.trigger_types.unwrap()); } #[test] @@ -219,7 +225,33 @@ mod test { let info = AppInfo::from_template_text(tpl).unwrap(); assert_eq!(2, info.manifest_format); - assert_eq!("triggy", info.trigger_type.unwrap()); + assert_eq!(vec!["triggy".to_owned()], info.trigger_types.unwrap()); + } + + #[test] + fn can_read_app_info_from_multi_trigger_template_v2() { + let tpl = r#"spin_manifest_version = 2 + name = "{{ thingy }}" + version = "1.2.3" + + [application.trigger.triggy] + arg = "{{ another-thingy }}" + + [[trigger.triggy]] + spork = "{{ utensil }}" + component = "{{ thingy | kebab_case }}" + + [[trigger.anothertrigger]] + spork = "{{ utensil }}" + component = "{{ thingy | kebab_case }}" + + [component.{{ thingy | kebab_case }}] + source = "path/to/{{ thingy | snake_case }}.wasm" + "#; + + let info = AppInfo::from_template_text(tpl).unwrap(); + assert_eq!(2, info.manifest_format); + assert_eq!(vec!["triggy".to_owned(), "anothertrigger".to_owned()], info.trigger_types.unwrap()); } #[test] @@ -231,6 +263,6 @@ mod test { let info = AppInfo::from_template_text(tpl).unwrap(); assert_eq!(2, info.manifest_format); - assert_eq!(None, info.trigger_type); + assert_eq!(None, info.trigger_types); } } diff --git a/crates/templates/src/run.rs b/crates/templates/src/run.rs index 7ddc47371f..d6478ad1c8 100644 --- a/crates/templates/src/run.rs +++ b/crates/templates/src/run.rs @@ -254,7 +254,12 @@ impl Run { match crate::app_info::AppInfo::from_file(manifest_path) { Some(Ok(app_info)) if app_info.manifest_format() == 1 => self .template - .check_compatible_trigger(app_info.trigger_type()), + .check_compatible_trigger( + app_info + .trigger_types() + .and_then(|types| types.first()) + .map(|trigger_type| trigger_type.as_str()) + ), _ => Ok(()), // Fail forgiving - don't block the user if things are under construction } } diff --git a/crates/templates/src/template.rs b/crates/templates/src/template.rs index cd0c10a8c5..a23ce8ca26 100644 --- a/crates/templates/src/template.rs +++ b/crates/templates/src/template.rs @@ -345,7 +345,7 @@ impl Template { fn infer_trigger_type(layout: &TemplateLayout) -> TemplateTriggerCompatibility { match crate::app_info::AppInfo::from_layout(layout) { - Some(Ok(app_info)) => match app_info.trigger_type() { + Some(Ok(app_info)) => match app_info.trigger_types().and_then(|types| types.first()) { None => TemplateTriggerCompatibility::Any, Some(t) => TemplateTriggerCompatibility::Only(t.to_owned()), }, From 043d82a4e6da9fe30e7ce95cc37b077ba54e7f40 Mon Sep 17 00:00:00 2001 From: Michelle Dhanani Date: Wed, 22 Jul 2026 16:11:11 -0400 Subject: [PATCH 2/2] error when adding component for manifest v1 app + removes trigger validation as it is no needed + spin add for manifest v1 no longer supported Signed-off-by: Michelle Noorali --- crates/templates/src/app_info.rs | 228 +++---------------------------- crates/templates/src/manager.rs | 2 +- crates/templates/src/reader.rs | 5 +- crates/templates/src/run.rs | 15 +- crates/templates/src/template.rs | 49 ------- 5 files changed, 29 insertions(+), 270 deletions(-) diff --git a/crates/templates/src/app_info.rs b/crates/templates/src/app_info.rs index 6f09ea486b..1fe8e8d985 100644 --- a/crates/templates/src/app_info.rs +++ b/crates/templates/src/app_info.rs @@ -2,32 +2,13 @@ // interest to the template system. spin_loader does too // much processing to fit our needs here. -use std::{ - collections::HashSet, - path::{Path, PathBuf}, - sync::LazyLock, -}; - -use serde::Deserialize; -use spin_manifest::schema::v1; - -use crate::store::TemplateLayout; +use std::path::Path; pub(crate) struct AppInfo { manifest_format: u32, - // Used for v1 manifests, which have exactly one trigger type and are the - // only manifests subject to a compatibility check. v2 manifests may host - // multiple trigger types. Triggerless templates also allowed for all - // manifest versions. - trigger_types: Option>, } impl AppInfo { - pub fn from_layout(layout: &TemplateLayout) -> Option> { - Self::layout_manifest_path(layout) - .map(|manifest_path| Self::from_existent_template(&manifest_path)) - } - pub fn from_file(manifest_path: &Path) -> Option> { if manifest_path.exists() { Some(Self::from_existent_file(manifest_path)) @@ -36,15 +17,6 @@ impl AppInfo { } } - fn layout_manifest_path(layout: &TemplateLayout) -> Option { - let manifest_path = layout.content_dir().join("spin.toml"); - if manifest_path.exists() { - Some(manifest_path) - } else { - None - } - } - fn from_existent_file(manifest_path: &Path) -> anyhow::Result { let manifest_str = std::fs::read_to_string(manifest_path)?; Self::from_manifest_text(&manifest_str) @@ -56,107 +28,12 @@ impl AppInfo { spin_manifest::ManifestVersion::V1 => 1, spin_manifest::ManifestVersion::V2 => 2, }; - let trigger_types = match manifest_version { - // v1 manifests always have exactly one trigger type. - spin_manifest::ManifestVersion::V1 => Some(vec![ - toml::from_str::(manifest_str)? - .trigger - .trigger_type, - ]), - // v2 manifests may declare multiple trigger types. - spin_manifest::ManifestVersion::V2 => { - let triggers = toml::from_str::(manifest_str)? - .trigger - .unwrap_or_default(); - if triggers.is_empty() { - None - } else { - Some(triggers.into_iter().map(|(trigger_type, _)| trigger_type).collect()) - } - } - }; - Ok(Self { - manifest_format, - trigger_types, - }) - } - - fn from_existent_template(manifest_path: &Path) -> anyhow::Result { - // This has to be cruder, because (with the v2 style of component) a template - // is no longer valid TOML, so `from_existent_file` fails at manifest - // version inference. - let read_to_string = std::fs::read_to_string(manifest_path)?; - let manifest_tpl_str = read_to_string; - - Self::from_template_text(&manifest_tpl_str) - } - - fn from_template_text(manifest_tpl_str: &str) -> anyhow::Result { - // TODO: investigate using a TOML parser or regex to be more accurate - let is_v1_tpl = manifest_tpl_str.contains("spin_manifest_version = \"1\""); - let is_v2_tpl = manifest_tpl_str.contains("spin_manifest_version = 2"); - if is_v1_tpl { - // V1 manifest templates are valid TOML - return Self::from_manifest_text(manifest_tpl_str); - } - if !is_v2_tpl { - // The system will default to being permissive in this case - anyhow::bail!("Unsure of template manifest version"); - } - - Self::from_v2_template_text(manifest_tpl_str) - } - - fn from_v2_template_text(manifest_tpl_str: &str) -> anyhow::Result { - // Preserve declaration order while removing duplicate trigger types. - // Order preservation useful for UX, predictable trigger listing. - let mut seen = HashSet::new(); - let trigger_types: Vec = manifest_tpl_str - .lines() - .filter_map(infer_trigger_type_from_raw_line) - .filter(|trigger_type| seen.insert(trigger_type.clone())) - .collect(); - let trigger_types = if trigger_types.is_empty() { - None - } else { - Some(trigger_types) - }; - Ok(Self { - manifest_format: 2, - trigger_types, - }) + Ok(Self { manifest_format }) } pub fn manifest_format(&self) -> u32 { self.manifest_format } - - pub fn trigger_types(&self) -> Option<&[String]> { - self.trigger_types.as_deref() - } -} - -static EXTRACT_TRIGGER: LazyLock = LazyLock::new(|| { - regex::Regex::new(r"^\s*\[\[trigger\.(?[a-zA-Z0-9-]+)") - .expect("Invalid unknown filter regex") -}); - -fn infer_trigger_type_from_raw_line(line: &str) -> Option { - EXTRACT_TRIGGER - .captures(line) - .map(|c| c["trigger"].to_owned()) -} - -#[derive(Deserialize)] -struct ManifestV1TriggerProbe { - // `trigger = { type = "", ...}` - trigger: v1::AppTriggerV1, -} - -#[derive(Deserialize)] -struct ManifestV2TriggerProbe { - /// `[trigger.]` - empty will not have a trigger table in v2 - trigger: Option, } #[cfg(test)] @@ -164,105 +41,38 @@ mod test { use super::*; #[test] - fn can_extract_triggers() { - assert_eq!( - "http", - infer_trigger_type_from_raw_line("[[trigger.http]]").unwrap() - ); - assert_eq!( - "http", - infer_trigger_type_from_raw_line(" [[trigger.http]]").unwrap() - ); - assert_eq!( - "fie", - infer_trigger_type_from_raw_line(" [[trigger.fie]]").unwrap() - ); - assert_eq!( - "x-y", - infer_trigger_type_from_raw_line(" [[trigger.x-y]]").unwrap() - ); - - assert_eq!(None, infer_trigger_type_from_raw_line("# [[trigger.http]]")); - assert_eq!(None, infer_trigger_type_from_raw_line("trigger. But,")); - assert_eq!(None, infer_trigger_type_from_raw_line("[[trigger. snerk")); - } - - #[test] - fn can_read_app_info_from_template_v1() { - let tpl = r#"spin_manifest_version = "1" - name = "{{ thingy }}" + fn can_detect_v1_manifest_format() { + let manifest = r#"spin_manifest_version = "1" + name = "test" version = "1.2.3" - trigger = { type = "triggy", arg = "{{ another-thingy }}" } + trigger = { type = "http" } [[component]] - id = "{{ thingy | kebab_case }}" - source = "path/to/{{ thingy | snake_case }}.wasm" + id = "test" + source = "test.wasm" [component.trigger] - spork = "{{ utensil }}" + route = "/" "#; - let info = AppInfo::from_template_text(tpl).unwrap(); + let info = AppInfo::from_manifest_text(manifest).unwrap(); assert_eq!(1, info.manifest_format); - assert_eq!(vec!["triggy".to_owned()], info.trigger_types.unwrap()); - } - - #[test] - fn can_read_app_info_from_template_v2() { - let tpl = r#"spin_manifest_version = 2 - name = "{{ thingy }}" - version = "1.2.3" - - [application.trigger.triggy] - arg = "{{ another-thingy }}" - - [[trigger.triggy]] - spork = "{{ utensil }}" - component = "{{ thingy | kebab_case }}" - - [component.{{ thingy | kebab_case }}] - source = "path/to/{{ thingy | snake_case }}.wasm" - "#; - - let info = AppInfo::from_template_text(tpl).unwrap(); - assert_eq!(2, info.manifest_format); - assert_eq!(vec!["triggy".to_owned()], info.trigger_types.unwrap()); } #[test] - fn can_read_app_info_from_multi_trigger_template_v2() { - let tpl = r#"spin_manifest_version = 2 - name = "{{ thingy }}" + fn can_detect_v2_manifest_format() { + let manifest = r#"spin_manifest_version = 2 + name = "test" version = "1.2.3" - [application.trigger.triggy] - arg = "{{ another-thingy }}" - - [[trigger.triggy]] - spork = "{{ utensil }}" - component = "{{ thingy | kebab_case }}" - - [[trigger.anothertrigger]] - spork = "{{ utensil }}" - component = "{{ thingy | kebab_case }}" + [[trigger.http]] + route = "/" + component = "test" - [component.{{ thingy | kebab_case }}] - source = "path/to/{{ thingy | snake_case }}.wasm" - "#; - - let info = AppInfo::from_template_text(tpl).unwrap(); - assert_eq!(2, info.manifest_format); - assert_eq!(vec!["triggy".to_owned(), "anothertrigger".to_owned()], info.trigger_types.unwrap()); - } - - #[test] - fn can_read_app_info_from_triggerless_template_v2() { - let tpl = r#"spin_manifest_version = 2 - name = "{{ thingy }}" - version = "1.2.3" + [component.test] + source = "test.wasm" "#; - let info = AppInfo::from_template_text(tpl).unwrap(); + let info = AppInfo::from_manifest_text(manifest).unwrap(); assert_eq!(2, info.manifest_format); - assert_eq!(None, info.trigger_types); } } diff --git a/crates/templates/src/manager.rs b/crates/templates/src/manager.rs index 39b0f5e7c8..1759d8d25d 100644 --- a/crates/templates/src/manager.rs +++ b/crates/templates/src/manager.rs @@ -1132,7 +1132,7 @@ mod tests { } #[tokio::test] - async fn cannot_add_component_that_does_not_match_manifest() { + async fn cannot_add_component_to_v1_manifest() { let manager = TempManager::new_with_this_repo_templates().await; let dest_temp_dir = tempdir().unwrap(); diff --git a/crates/templates/src/reader.rs b/crates/templates/src/reader.rs index 40b05a2a55..cdfe4b9012 100644 --- a/crates/templates/src/reader.rs +++ b/crates/templates/src/reader.rs @@ -17,7 +17,10 @@ pub(crate) enum RawTemplateManifest { pub(crate) struct RawTemplateManifestV1 { pub id: String, pub description: Option, - pub trigger_type: Option, + // Retained so existing templates that declare `trigger_type` continue + // to parse under `deny_unknown_fields`; the value is no longer used. + #[allow(dead_code)] + pub trigger_type: Option, pub tags: Option>, pub new_application: Option, pub add_component: Option, diff --git a/crates/templates/src/run.rs b/crates/templates/src/run.rs index d6478ad1c8..b736bfb019 100644 --- a/crates/templates/src/run.rs +++ b/crates/templates/src/run.rs @@ -97,7 +97,7 @@ impl Run { interaction: impl InteractionStrategy, ) -> anyhow::Result> { self.validate_version()?; - self.validate_trigger()?; + self.validate_add_component_manifest_version()?; // TODO: rationalise `path` and `dir` let to = self.generation_target_dir(); @@ -247,19 +247,14 @@ impl Run { } } - fn validate_trigger(&self) -> anyhow::Result<()> { + fn validate_add_component_manifest_version(&self) -> anyhow::Result<()> { match &self.options.variant { TemplateVariantInfo::NewApplication => Ok(()), TemplateVariantInfo::AddComponent { manifest_path } => { match crate::app_info::AppInfo::from_file(manifest_path) { - Some(Ok(app_info)) if app_info.manifest_format() == 1 => self - .template - .check_compatible_trigger( - app_info - .trigger_types() - .and_then(|types| types.first()) - .map(|trigger_type| trigger_type.as_str()) - ), + Some(Ok(app_info)) if app_info.manifest_format() == 1 => Err(anyhow!( + "Components cannot be added to Spin manifest version 1 applications.", + )), _ => Ok(()), // Fail forgiving - don't block the user if things are under construction } } diff --git a/crates/templates/src/template.rs b/crates/templates/src/template.rs index a23ce8ca26..f91d17c1b2 100644 --- a/crates/templates/src/template.rs +++ b/crates/templates/src/template.rs @@ -25,7 +25,6 @@ pub struct Template { tags: HashSet, description: Option, installed_from: InstalledFrom, - trigger: TemplateTriggerCompatibility, variants: HashMap, parameters: Vec, extra_outputs: Vec, @@ -117,12 +116,6 @@ pub(crate) enum Condition { Always(bool), } -#[derive(Clone, Debug, Eq, PartialEq, Hash)] -pub(crate) enum TemplateTriggerCompatibility { - Any, - Only(String), -} - #[derive(Clone, Debug)] pub(crate) enum TemplateParameterDataType { String(StringConstraints), @@ -199,7 +192,6 @@ impl Template { tags: raw.tags.map(Self::normalize_tags).unwrap_or_default(), description: raw.description.clone(), installed_from, - trigger: Self::parse_trigger_type(raw.trigger_type, layout), variants: Self::parse_template_variants(raw.new_application, raw.add_component), parameters: Self::parse_parameters(&raw.parameters)?, extra_outputs: Self::parse_extra_outputs(&raw.outputs)?, @@ -333,26 +325,6 @@ impl Template { tags.into_iter().map(|tag| tag.to_lowercase()).collect() } - fn parse_trigger_type( - raw: Option, - layout: &TemplateLayout, - ) -> TemplateTriggerCompatibility { - match raw { - None => Self::infer_trigger_type(layout), - Some(t) => TemplateTriggerCompatibility::Only(t), - } - } - - fn infer_trigger_type(layout: &TemplateLayout) -> TemplateTriggerCompatibility { - match crate::app_info::AppInfo::from_layout(layout) { - Some(Ok(app_info)) => match app_info.trigger_types().and_then(|types| types.first()) { - None => TemplateTriggerCompatibility::Any, - Some(t) => TemplateTriggerCompatibility::Only(t.to_owned()), - }, - _ => TemplateTriggerCompatibility::Any, // Fail forgiving - } - } - fn parse_template_variants( new_application: Option, add_component: Option, @@ -457,26 +429,6 @@ impl Template { .collect() } - pub(crate) fn check_compatible_trigger(&self, app_trigger: Option<&str>) -> anyhow::Result<()> { - // The application we are merging into might not have a trigger yet, in which case - // we're good to go. - let Some(app_trigger) = app_trigger else { - return Ok(()); - }; - match &self.trigger { - TemplateTriggerCompatibility::Any => Ok(()), - TemplateTriggerCompatibility::Only(t) => { - if app_trigger == t { - Ok(()) - } else { - Err(anyhow!( - "Component trigger type '{t}' does not match application trigger type '{app_trigger}'" - )) - } - } - } - } - pub(crate) fn check_compatible_manifest_format( &self, manifest_format: u32, @@ -769,7 +721,6 @@ mod test { tags: HashSet::new(), description: None, installed_from: InstalledFrom::Unknown, - trigger: TemplateTriggerCompatibility::Any, variants, parameters: vec![], extra_outputs: vec![],