From 3d4447224cd27006f1db4a7ba345c1c55f4080f6 Mon Sep 17 00:00:00 2001 From: Marcel Guzik Date: Mon, 30 Oct 2023 10:59:09 +0000 Subject: [PATCH] fix #2383: Use "service" if service type missing Some additions: 1. A C8y converter test was added to make sure that the emitted service creation message is valid if `service.type` is empty. 2. `c8y_api::inventory` functions which created C8y smartrest messages were changed to return `Result`. Currently we only check if values marked as mandatory in [SmartREST reference documentation][1] are not empty, but we could do more validation in the future. 3. Added `ConversionError::UnexpectedError` variant, which can be constructed from an `anyhow::Error`. _Unexpected_ part is supposed to denote that the error was not related due to invalid input, or the user doing something wrong, but some unexpected conditions occured that makes it so that the request cannot be completed. Like a `panic!`, it can be used when there is a logic error on part of the programmer, but unlike `panic!` it doesn't crash the entire program. More about the idea [here][2]. [1]: https://cumulocity.com/guides/reference/smartrest-two/#102 [2]: https://www.lpalmieri.com/posts/error-handling-rust/#avoid-ball-of-mud-error-enums --- .../core/c8y_api/src/smartrest/inventory.rs | 52 +++++++++++++-- crates/core/tedge_mapper/src/c8y/mapper.rs | 9 ++- crates/extensions/c8y_mapper_ext/Cargo.toml | 2 +- .../c8y_mapper_ext/src/converter.rs | 66 +++++++++++++++++-- crates/extensions/c8y_mapper_ext/src/error.rs | 3 + crates/extensions/tedge_health_ext/src/lib.rs | 6 +- .../service_monitoring.robot | 1 - 7 files changed, 123 insertions(+), 16 deletions(-) diff --git a/crates/core/c8y_api/src/smartrest/inventory.rs b/crates/core/c8y_api/src/smartrest/inventory.rs index 6c6ce07e172..492899a8366 100644 --- a/crates/core/c8y_api/src/smartrest/inventory.rs +++ b/crates/core/c8y_api/src/smartrest/inventory.rs @@ -22,8 +22,15 @@ pub fn child_device_creation_message( device_name: Option<&str>, device_type: Option<&str>, ancestors: &[String], -) -> Message { - Message::new( +) -> Result { + if child_id.is_empty() { + return Err(InvalidValueError { + field_name: "child_id".to_string(), + value: child_id.to_string(), + }); + } + + Ok(Message::new( &publish_topic_from_ancestors(ancestors), // XXX: if any arguments contain commas, output will be wrong format!( @@ -32,7 +39,7 @@ pub fn child_device_creation_message( device_name.unwrap_or(child_id), device_type.unwrap_or("thin-edge.io-child") ), - ) + )) } /// Create a SmartREST message for creating a service on device. @@ -44,15 +51,41 @@ pub fn service_creation_message( service_type: &str, service_status: &str, ancestors: &[String], -) -> Message { - Message::new( +) -> Result { + // TODO: most of this noise can be eliminated by implementing `Serialize`/`Deserialize` for smartrest format + if service_id.is_empty() { + return Err(InvalidValueError { + field_name: "service_id".to_string(), + value: service_id.to_string(), + }); + } + if service_name.is_empty() { + return Err(InvalidValueError { + field_name: "service_name".to_string(), + value: service_name.to_string(), + }); + } + if service_type.is_empty() { + return Err(InvalidValueError { + field_name: "service_type".to_string(), + value: service_type.to_string(), + }); + } + if service_status.is_empty() { + return Err(InvalidValueError { + field_name: "service_status".to_string(), + value: service_status.to_string(), + }); + } + + Ok(Message::new( &publish_topic_from_ancestors(ancestors), // XXX: if any arguments contain commas, output will be wrong format!( "102,{},{},{},{}", service_id, service_type, service_name, service_status ), - ) + )) } /// Create a SmartREST message for updating service status. @@ -81,3 +114,10 @@ pub fn service_status_update_message(external_ids: &[String], service_status: &s Message::new(&topic, payload) } + +#[derive(thiserror::Error, Debug)] +#[error("Field `{field_name}` contains invalid value: {value:?}")] +pub struct InvalidValueError { + field_name: String, + value: String, +} diff --git a/crates/core/tedge_mapper/src/c8y/mapper.rs b/crates/core/tedge_mapper/src/c8y/mapper.rs index 46f8e4bdde4..bceeadf1043 100644 --- a/crates/core/tedge_mapper/src/c8y/mapper.rs +++ b/crates/core/tedge_mapper/src/c8y/mapper.rs @@ -87,7 +87,12 @@ impl TEdgeComponent for CumulocityMapper { pub fn service_monitor_client_config(tedge_config: &TEdgeConfig) -> Result { let main_device_xid: EntityExternalId = tedge_config.device.id.try_read(tedge_config)?.into(); - let service_type = tedge_config.service.ty.clone(); + let service_type = &tedge_config.service.ty; + let service_type = if service_type.is_empty() { + "service".to_string() + } else { + service_type.to_string() + }; // FIXME: this will not work if `mqtt.device_topic_id` is not in default scheme @@ -113,7 +118,7 @@ pub fn service_monitor_client_config(tedge_config: &TEdgeConfig) -> Result { @@ -296,7 +304,8 @@ impl CumulocityConverter { display_type.unwrap_or(&self.service_type), "up", &ancestors_external_ids, - ); + ) + .context("Could not create service creation message")?; Ok(vec![service_creation_message]) } } @@ -1437,6 +1446,7 @@ pub(crate) mod tests { use assert_json_diff::assert_json_eq; use assert_json_diff::assert_json_include; use assert_matches::assert_matches; + use c8y_api::smartrest::topic::SMARTREST_PUBLISH_TOPIC; use c8y_auth_proxy::url::ProxyUrlGenerator; use c8y_http_proxy::handle::C8YHttpProxy; use c8y_http_proxy::messages::C8YRestRequest; @@ -2708,12 +2718,48 @@ pub(crate) mod tests { assert!(!second_registration_message_mapped); } + #[tokio::test] + async fn handles_empty_service_type_2383() { + let tmp_dir = TempTedgeDir::new(); + let mut config = c8y_converter_config(&tmp_dir); + config.service_type = String::new(); + + let (mut converter, _) = create_c8y_converter_from_config(config); + + let service_health_message = Message::new( + &Topic::new_unchecked("te/device/main/service/service1/status/health"), + serde_json::to_string(&json!({"status": "up"})).unwrap(), + ); + + let output = converter.convert(&service_health_message).await; + let service_creation_message = output + .into_iter() + .find(|m| m.topic.name == SMARTREST_PUBLISH_TOPIC) + .expect("service creation message should be present"); + + let mut smartrest_fields = service_creation_message.payload_str().unwrap().split(','); + + assert_eq!(smartrest_fields.next().unwrap(), "102"); + assert_eq!( + smartrest_fields.next().unwrap(), + format!("{}:device:main:service:service1", converter.device_name) + ); + assert_eq!(smartrest_fields.next().unwrap(), "service"); + assert_eq!(smartrest_fields.next().unwrap(), "service1"); + assert_eq!(smartrest_fields.next().unwrap(), "up"); + } + pub(crate) async fn create_c8y_converter( tmp_dir: &TempTedgeDir, ) -> ( CumulocityConverter, SimpleMessageBox, ) { + let config = c8y_converter_config(tmp_dir); + create_c8y_converter_from_config(config) + } + + fn c8y_converter_config(tmp_dir: &TempTedgeDir) -> C8yMapperConfig { tmp_dir.dir("operations").dir("c8y"); tmp_dir.dir("tedge").dir("agent"); @@ -2731,7 +2777,7 @@ pub(crate) mod tests { topics.add_all(crate::log_upload::log_upload_topic_filter(&mqtt_schema)); topics.add_all(C8yMapperConfig::default_external_topic_filter()); - let config = C8yMapperConfig::new( + C8yMapperConfig::new( tmp_dir.to_path_buf(), tmp_dir.utf8_path_buf(), tmp_dir.utf8_path_buf().into(), @@ -2745,8 +2791,15 @@ pub(crate) mod tests { Capabilities::default(), auth_proxy_addr, auth_proxy_port, - ); + ) + } + fn create_c8y_converter_from_config( + config: C8yMapperConfig, + ) -> ( + CumulocityConverter, + SimpleMessageBox, + ) { let mqtt_builder: SimpleMessageBoxBuilder = SimpleMessageBoxBuilder::new("MQTT", 5); let mqtt_publisher = LoggingSender::new("MQTT".into(), mqtt_builder.build().sender_clone()); @@ -2754,6 +2807,9 @@ pub(crate) mod tests { let mut c8y_proxy_builder: SimpleMessageBoxBuilder = SimpleMessageBoxBuilder::new("C8Y", 1); let http_proxy = C8YHttpProxy::new("C8Y", &mut c8y_proxy_builder); + + let auth_proxy_addr = config.auth_proxy_addr; + let auth_proxy_port = config.auth_proxy_port; let auth_proxy = ProxyUrlGenerator::new(auth_proxy_addr, auth_proxy_port); let downloader_builder: SimpleMessageBoxBuilder = diff --git a/crates/extensions/c8y_mapper_ext/src/error.rs b/crates/extensions/c8y_mapper_ext/src/error.rs index 09f296b0138..9c28b6ad576 100644 --- a/crates/extensions/c8y_mapper_ext/src/error.rs +++ b/crates/extensions/c8y_mapper_ext/src/error.rs @@ -125,6 +125,9 @@ pub enum ConversionError { #[error(transparent)] InvalidExternalIdError(#[from] InvalidExternalIdError), + + #[error("Unexpected error")] + UnexpectedError(#[from] anyhow::Error), } #[derive(thiserror::Error, Debug)] diff --git a/crates/extensions/tedge_health_ext/src/lib.rs b/crates/extensions/tedge_health_ext/src/lib.rs index 776fa76551c..85ddee8938f 100644 --- a/crates/extensions/tedge_health_ext/src/lib.rs +++ b/crates/extensions/tedge_health_ext/src/lib.rs @@ -39,7 +39,7 @@ impl HealthMonitorBuilder { mqtt: &mut (impl ServiceProvider + AsMut), // TODO: pass it less annoying way mqtt_schema: &MqttSchema, - service_type: String, + mut service_type: String, ) -> Self { let service_topic_id = &service.service_topic_id; @@ -71,6 +71,10 @@ impl HealthMonitorBuilder { box_builder .set_request_sender(mqtt.connect_consumer(subscriptions, box_builder.get_sender())); + if service_type.is_empty() { + service_type = "service".to_string() + } + let registration_message = EntityRegistrationMessage { topic_id: service_topic_id.entity().clone(), external_id: None, diff --git a/tests/RobotFramework/tests/cumulocity/service_monitoring/service_monitoring.robot b/tests/RobotFramework/tests/cumulocity/service_monitoring/service_monitoring.robot index efb2e5fe796..337f7f19488 100644 --- a/tests/RobotFramework/tests/cumulocity/service_monitoring/service_monitoring.robot +++ b/tests/RobotFramework/tests/cumulocity/service_monitoring/service_monitoring.robot @@ -190,7 +190,6 @@ Check if a service using configured service type Check if a service using configured service type as empty [Arguments] ${service_name} - Skip msg=Skipping due to bug #2383 Execute Command tedge config set service.type "" Custom Test Setup ThinEdgeIO.Restart Service ${service_name}