Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix misleading error message if built-in app trigger was missing required parameter #1440

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions crates/loader/src/local/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,21 @@ fn test_unknown_version_is_rejected() {
);
}

#[tokio::test]
async fn gives_correct_error_when_missing_app_trigger_field() -> Result<()> {
const MANIFEST: &str = "tests/missing-http-base.toml";

let app = raw_manifest_from_file(&PathBuf::from(MANIFEST)).await;

let e = format!("{:#}", app.unwrap_err());
assert!(
e.contains("missing field `base`"),
"Expected error to contain trigger field information but was '{e}'"
);

Ok(())
}

#[test]
fn test_wagi_executor_with_custom_entrypoint() -> Result<()> {
const MANIFEST: &str = include_str!("../../tests/wagi-custom-entrypoint.toml");
Expand Down
12 changes: 12 additions & 0 deletions crates/loader/tests/missing-http-base.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
spin_version = "1"
authors = ["Gul Madred", "Edward Jellico", "JL"]
description = "A HTTP application without a base"
name = "missing-http-base"
trigger = {type = "http"}
version = "6.11.2"

[[component]]
id = "test"
source = "nope/nope/nope"
[component.trigger]
route = "/test"
64 changes: 36 additions & 28 deletions crates/manifest/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ pub enum Error {
/// No 'type' key in trigger declaration.
#[error("the application did not specify a trigger type")]
MissingTriggerType,
/// No 'type' key in trigger declaration.
#[error("could not load application trigger parameters: {0}")]
InvalidTriggerTypeParameters(String),
/// Non-string 'type' key in trigger declaration.
#[error("the trigger type must be a string")]
NonStringTriggerType,
Expand Down Expand Up @@ -116,7 +119,7 @@ pub enum ApplicationOrigin {
#[serde(
deny_unknown_fields,
rename_all = "camelCase",
try_from = "ApplicationTriggerSerialised",
try_from = "ApplicationTriggerDeserialised",
into = "ApplicationTriggerSerialised"
)]
pub enum ApplicationTrigger {
Expand All @@ -128,19 +131,27 @@ pub enum ApplicationTrigger {
External(ExternalTriggerConfiguration),
}

/// Serialisation helper - we need all unmatched `trigger.type` values to
/// map to `ApplicationTrigger::External`, but `#[serde(other)]` can
/// only be applied to unit types. The following types cause recognised
/// tags to map to the Internal case and unrecognised ones to the
/// External case.
#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)]
#[derive(Clone, Debug, PartialEq, Serialize)]
#[serde(deny_unknown_fields, rename_all = "camelCase", untagged)]
enum ApplicationTriggerSerialised {
Internal(InternalApplicationTriggerSerialised),
/// A trigger type that is not built in.
External(HashMap<String, toml::Value>),
}

/// Deserialisation helper - we need all unmatched `trigger.type` values to
/// map to `ApplicationTrigger::External`, but `#[serde(other)]` can
/// only be applied to unit types. The following types cause recognised
/// tags to map to the Internal case and unrecognised ones to the
/// External case.
#[derive(Deserialize)]
struct ApplicationTriggerDeserialised {
#[serde(rename = "type")]
trigger_type: String,
#[serde(flatten)]
parameters: toml::Value,
}

#[derive(Clone, Debug, Deserialize, PartialEq, Serialize, Eq)]
#[serde(deny_unknown_fields, rename_all = "camelCase", tag = "type")]
enum InternalApplicationTriggerSerialised {
Expand All @@ -150,29 +161,26 @@ enum InternalApplicationTriggerSerialised {
Redis(RedisTriggerConfiguration),
}

impl TryFrom<ApplicationTriggerSerialised> for ApplicationTrigger {
impl TryFrom<ApplicationTriggerDeserialised> for ApplicationTrigger {
type Error = Error;

fn try_from(value: ApplicationTriggerSerialised) -> Result<Self, Self::Error> {
match value {
ApplicationTriggerSerialised::Internal(InternalApplicationTriggerSerialised::Http(
h,
)) => Ok(Self::Http(h)),
ApplicationTriggerSerialised::Internal(
InternalApplicationTriggerSerialised::Redis(r),
) => Ok(Self::Redis(r)),
ApplicationTriggerSerialised::External(mut map) => match map.remove("type") {
Some(toml::Value::String(ty)) => {
let ext_config = ExternalTriggerConfiguration {
trigger_type: ty,
parameters: map,
};
Ok(Self::External(ext_config))
}
Some(_) => Err(Error::NonStringTriggerType),
None => Err(Error::MissingTriggerType),
},
}
fn try_from(value: ApplicationTriggerDeserialised) -> Result<Self, Self::Error> {
let trigger = match value.trigger_type.as_str() {
"http" => ApplicationTrigger::Http(
HttpTriggerConfiguration::deserialize(value.parameters)
.map_err(|e| Error::InvalidTriggerTypeParameters(e.to_string()))?,
),
"redis" => ApplicationTrigger::Redis(
RedisTriggerConfiguration::deserialize(value.parameters)
.map_err(|e| Error::InvalidTriggerTypeParameters(e.to_string()))?,
),
_ => ApplicationTrigger::External(ExternalTriggerConfiguration {
trigger_type: value.trigger_type,
parameters: HashMap::deserialize(value.parameters)
.map_err(|e| Error::InvalidTriggerTypeParameters(e.to_string()))?,
}),
};
Ok(trigger)
}
}

Expand Down
Loading