fix #2383: Use "service" if service type missing (#2385)

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

crates/core/c8y_api/src/smartrest/inventory.rs

@@ -22,8 +22,27 @@ pub fn child_device_creation_message(
     device_name: Option<&str>,
     device_type: Option<&str>,
     ancestors: &[String],
-) -> Message {
-    Message::new(
+) -> Result<Message, InvalidValueError> {
+    if child_id.is_empty() {
+        return Err(InvalidValueError {
+            field_name: "child_id".to_string(),
+            value: child_id.to_string(),
+        });
+    }
+    if let Some("") = device_name {
+        return Err(InvalidValueError {
+            field_name: "device_name".to_string(),
+            value: "".to_string(),
+        });
+    }
+    if let Some("") = device_type {
+        return Err(InvalidValueError {
+            field_name: "device_type".to_string(),
+            value: "".to_string(),
+        });
+    }
+
+    Ok(Message::new(
         &publish_topic_from_ancestors(ancestors),
         // XXX: if any arguments contain commas, output will be wrong
         format!(
@@ -32,7 +51,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 +63,41 @@ pub fn service_creation_message(
     service_type: &str,
     service_status: &str,
     ancestors: &[String],
-) -> Message {
-    Message::new(
+) -> Result<Message, InvalidValueError> {
+    // 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 +126,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,
+}

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<Config, anyhow::Error> {
     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<Confi
         service_type.as_str(),
         "down",
         &[],
-    );
+    )?;
 
     let mqtt_config = tedge_config
         .mqtt_config()?

crates/extensions/c8y_mapper_ext/Cargo.toml

@@ -10,6 +10,7 @@ homepage = { workspace = true }
 repository = { workspace = true }
 
 [dependencies]
+anyhow = { workspace = true }
 async-trait = { workspace = true }
 c8y_api = { workspace = true }
 c8y_auth_proxy = { workspace = true }
@@ -45,7 +46,6 @@ toml = { workspace = true }
 tracing = { workspace = true }
 
 [dev-dependencies]
-anyhow = { workspace = true }
 assert-json-diff = { workspace = true }
 assert_matches = { workspace = true }
 proptest = { workspace = true }

crates/extensions/c8y_mapper_ext/src/converter.rs

@@ -8,6 +8,7 @@ use crate::actor::IdDownloadRequest;
 use crate::dynamic_discovery::DiscoverOp;
 use crate::error::ConversionError;
 use crate::json;
+use anyhow::Context;
 use c8y_api::http_proxy::C8yEndPoint;
 use c8y_api::json_c8y::C8yCreateEvent;
 use c8y_api::json_c8y::C8yUpdateSoftwareListResponse;
@@ -189,7 +190,13 @@ impl CumulocityConverter {
         let device_id = config.device_id.clone();
         let device_topic_id = config.device_topic_id.clone();
         let device_type = config.device_type.clone();
-        let service_type = config.service_type.clone();
+
+        let service_type = if config.service_type.is_empty() {
+            "service".to_string()
+        } else {
+            config.service_type.clone()
+        };
+
         let c8y_host = config.c8y_host.clone();
         let cfg_dir = config.config_dir.clone();
 
@@ -272,7 +279,8 @@ impl CumulocityConverter {
                     display_name,
                     display_type,
                     &ancestors_external_ids,
-                );
+                )
+                .context("Could not create device creation message")?;
                 Ok(vec![child_creation_message])
             }
             EntityType::Service => {
@@ -289,7 +297,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])
             }
         }
@@ -1431,6 +1440,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;
@@ -2702,12 +2712,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<C8YRestRequest, C8YRestResult>,
     ) {
+        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");
 
@@ -2725,7 +2771,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(),
@@ -2739,15 +2785,25 @@ pub(crate) mod tests {
             Capabilities::default(),
             auth_proxy_addr,
             auth_proxy_port,
-        );
+        )
+    }
 
+    fn create_c8y_converter_from_config(
+        config: C8yMapperConfig,
+    ) -> (
+        CumulocityConverter,
+        SimpleMessageBox<C8YRestRequest, C8YRestResult>,
+    ) {
         let mqtt_builder: SimpleMessageBoxBuilder<MqttMessage, MqttMessage> =
             SimpleMessageBoxBuilder::new("MQTT", 5);
         let mqtt_publisher = LoggingSender::new("MQTT".into(), mqtt_builder.build().sender_clone());
 
         let mut c8y_proxy_builder: SimpleMessageBoxBuilder<C8YRestRequest, C8YRestResult> =
             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<IdDownloadResult, IdDownloadRequest> =

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)]

crates/extensions/tedge_health_ext/src/lib.rs

@@ -39,7 +39,7 @@ impl HealthMonitorBuilder {
         mqtt: &mut (impl ServiceProvider<MqttMessage, MqttMessage, TopicFilter> + AsMut<MqttConfig>),
         // 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,

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}