fix #2383: Use "service" if service type missing

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;
@@ -190,7 +191,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();
 
@@ -279,7 +286,8 @@ impl CumulocityConverter {
                     display_name,
                     display_type,
                     &ancestors_external_ids,
-                );
+                )
+                .context("Could not create device creation message")?;
                 Ok(vec![child_creation_message])
             }
             EntityType::Service => {
@@ -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<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");
 
@@ -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,15 +2791,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}