From 3dd6d047706eb10fc5fc4f406c61898e56007c82 Mon Sep 17 00:00:00 2001 From: Rina Fujino <18257209+rina23q@users.noreply.github.com> Date: Thu, 12 Oct 2023 13:59:07 +0200 Subject: [PATCH 1/2] Add try_get() and try_get_by_external_id() to entity_store As commented in #2303#discussion_r1356364285, we have many patterns using get() or get_by_external_id() and map them None to Err. The new APIs returns Error::UnknownEntity instead of None. Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com> --- crates/core/tedge_api/src/entity_store.rs | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/crates/core/tedge_api/src/entity_store.rs b/crates/core/tedge_api/src/entity_store.rs index 58b6b6e91ec..60758d57e45 100644 --- a/crates/core/tedge_api/src/entity_store.rs +++ b/crates/core/tedge_api/src/entity_store.rs @@ -164,12 +164,27 @@ impl EntityStore { self.entities.get(entity_topic_id) } - /// Returns information for an entity under a given device/service id . + /// Tries to get information about an entity under a given MQTT entity topic identifier. + pub fn try_get(&self, entity_topic_id: &EntityTopicId) -> Result<&EntityMetadata, Error> { + self.get(entity_topic_id) + .ok_or_else(|| Error::UnknownEntity(entity_topic_id.to_string())) + } + + /// Returns information for an entity under a given device/service id. pub fn get_by_external_id(&self, external_id: &EntityExternalId) -> Option<&EntityMetadata> { let topic_id = self.entity_id_index.get(external_id)?; self.get(topic_id) } + /// Tries to get information for an entity under a given device/service id. + pub fn try_get_by_external_id( + &self, + external_id: &EntityExternalId, + ) -> Result<&EntityMetadata, Error> { + self.get_by_external_id(external_id) + .ok_or_else(|| Error::UnknownEntity(external_id.into())) + } + /// Returns the MQTT identifier of the main device. /// /// The main device is an entity with `@type: "device"`. From 35057d996e57a9345d0963bed0f7a7751b75c21c Mon Sep 17 00:00:00 2001 From: Rina Fujino <18257209+rina23q@users.noreply.github.com> Date: Thu, 12 Oct 2023 13:59:23 +0200 Subject: [PATCH 2/2] Apply try_get() and try_get_by_external_id() to c8y-mapper Signed-off-by: Rina Fujino <18257209+rina23q@users.noreply.github.com> --- crates/core/tedge_api/src/entity_store.rs | 6 +- .../c8y_mapper_ext/src/config_operations.rs | 75 +++++++------------ .../c8y_mapper_ext/src/converter.rs | 17 +---- crates/extensions/c8y_mapper_ext/src/error.rs | 7 +- .../c8y_mapper_ext/src/log_upload.rs | 30 ++------ 5 files changed, 41 insertions(+), 94 deletions(-) diff --git a/crates/core/tedge_api/src/entity_store.rs b/crates/core/tedge_api/src/entity_store.rs index 60758d57e45..0a01d30c483 100644 --- a/crates/core/tedge_api/src/entity_store.rs +++ b/crates/core/tedge_api/src/entity_store.rs @@ -164,7 +164,8 @@ impl EntityStore { self.entities.get(entity_topic_id) } - /// Tries to get information about an entity under a given MQTT entity topic identifier. + /// Tries to get information about an entity using its `EntityTopicId`, + /// returning an error if the entity is not registered. pub fn try_get(&self, entity_topic_id: &EntityTopicId) -> Result<&EntityMetadata, Error> { self.get(entity_topic_id) .ok_or_else(|| Error::UnknownEntity(entity_topic_id.to_string())) @@ -176,7 +177,8 @@ impl EntityStore { self.get(topic_id) } - /// Tries to get information for an entity under a given device/service id. + /// Tries to get information about an entity using its `EntityExternalId`, + /// returning an error if the entity is not registered. pub fn try_get_by_external_id( &self, external_id: &EntityExternalId, diff --git a/crates/extensions/c8y_mapper_ext/src/config_operations.rs b/crates/extensions/c8y_mapper_ext/src/config_operations.rs index 0f3e42555d2..4b3ae37e412 100644 --- a/crates/extensions/c8y_mapper_ext/src/config_operations.rs +++ b/crates/extensions/c8y_mapper_ext/src/config_operations.rs @@ -1,7 +1,6 @@ use crate::converter::CumulocityConverter; use crate::error::ConversionError; use crate::error::CumulocityMapperError; -use crate::error::CumulocityMapperError::UnknownDevice; use c8y_api::smartrest::smartrest_deserializer::SmartRestConfigDownloadRequest; use c8y_api::smartrest::smartrest_deserializer::SmartRestConfigUploadRequest; use c8y_api::smartrest::smartrest_deserializer::SmartRestOperationVariant; @@ -77,10 +76,7 @@ impl CumulocityConverter { let snapshot_request = SmartRestConfigUploadRequest::from_smartrest(smartrest)?; let target = self .entity_store - .get_by_external_id(&snapshot_request.device.clone().into()) - .ok_or_else(|| UnknownDevice { - device_id: snapshot_request.device.to_string(), - })?; + .try_get_by_external_id(&snapshot_request.device.clone().into())?; let cmd_id = nanoid!(); let channel = Channel::Command { @@ -88,13 +84,12 @@ impl CumulocityConverter { cmd_id: cmd_id.clone(), }; let topic = self.mqtt_schema.topic_for(&target.topic_id, &channel); - let external_id: String = target.external_id.clone().into(); // Replace '/' with ':' to avoid creating unexpected directories in file transfer repo let tedge_url = format!( "http://{}/tedge/file-transfer/{}/config_snapshot/{}-{}", &self.config.tedge_http_host, - external_id, + target.external_id.as_ref(), snapshot_request.config_type.replace('/', ":"), cmd_id ); @@ -127,13 +122,7 @@ impl CumulocityConverter { return Ok(vec![]); } - // get the device metadata from its id - let device = self.entity_store.get(topic_id).ok_or_else(|| { - CumulocityMapperError::UnregisteredDevice { - topic_id: topic_id.to_string(), - } - })?; - let external_id = &device.external_id; + let external_id = self.entity_store.try_get(topic_id)?.external_id.as_ref(); let smartrest_topic = self.smartrest_publish_topic_for_entity(topic_id)?; let payload = message.payload_str()?; let response = &ConfigSnapshotCmdPayload::from_json(payload)?; @@ -151,7 +140,7 @@ impl CumulocityConverter { .config .data_dir .file_transfer_dir() - .join(device.external_id.as_ref()) + .join(external_id) .join("config_snapshot") .join(format!( "{}-{}", @@ -164,7 +153,7 @@ impl CumulocityConverter { .upload_file( uploaded_file_path.as_std_path(), &response.config_type, - external_id.as_ref().to_string(), + external_id.to_string(), ) .await; // We need to get rid of this await, otherwise it blocks @@ -239,12 +228,9 @@ impl CumulocityConverter { } let smartrest = SmartRestConfigDownloadRequest::from_smartrest(smartrest)?; - let entity = self + let target = self .entity_store - .get_by_external_id(&smartrest.device.clone().into()) - .ok_or_else(|| UnknownDevice { - device_id: smartrest.device.clone(), - })?; + .try_get_by_external_id(&smartrest.device.clone().into())?; let cmd_id = nanoid!(); let remote_url = smartrest.url.as_str(); @@ -255,13 +241,13 @@ impl CumulocityConverter { // No download. Create a symlink and config_update command. info!("Hit the file cache={file_cache_path}. Create a symlink to the file"); self.create_symlink_for_config_update( - entity, + target, &smartrest.config_type, &cmd_id, file_cache_path, )?; - let message = self.create_config_update_cmd(cmd_id.into(), &smartrest, entity); + let message = self.create_config_update_cmd(cmd_id.into(), &smartrest, target); Ok(message) } else { // Require file download @@ -300,26 +286,23 @@ impl CumulocityConverter { smartrest: &SmartRestConfigDownloadRequest, download_result: DownloadResult, ) -> Result, ConversionError> { - let device = self + let target = self .entity_store - .get_by_external_id(&smartrest.device.clone().into()) - .ok_or_else(|| UnknownDevice { - device_id: smartrest.device.to_string(), - })?; + .try_get_by_external_id(&smartrest.device.clone().into())?; match download_result { Ok(download_response) => { self.create_symlink_for_config_update( - device, + target, &smartrest.config_type, &cmd_id, download_response.file_path, )?; - let message = self.create_config_update_cmd(cmd_id, smartrest, device); + let message = self.create_config_update_cmd(cmd_id, smartrest, target); Ok(message) } Err(download_err) => { - let sm_topic = self.smartrest_publish_topic_for_entity(&device.topic_id)?; + let sm_topic = self.smartrest_publish_topic_for_entity(&target.topic_id)?; let smartrest_executing = SmartRestSetOperationToExecuting::new( CumulocitySupportedOperations::C8yDownloadConfigFile, ) @@ -357,11 +340,7 @@ impl CumulocityConverter { return Ok(vec![]); } - let device = self.entity_store.get(topic_id).ok_or_else(|| { - CumulocityMapperError::UnregisteredDevice { - topic_id: topic_id.to_string(), - } - })?; + let target = self.entity_store.try_get(topic_id)?; let sm_topic = self.smartrest_publish_topic_for_entity(topic_id)?; let payload = message.payload_str()?; let response = &ConfigUpdateCmdPayload::from_json(payload)?; @@ -384,7 +363,7 @@ impl CumulocityConverter { .with_retain() .with_qos(QoS::AtLeastOnce); - self.delete_symlink_for_config_update(device, &response.config_type, cmd_id)?; + self.delete_symlink_for_config_update(target, &response.config_type, cmd_id)?; vec![c8y_notification, clear_local_cmd] } @@ -399,7 +378,7 @@ impl CumulocityConverter { .with_retain() .with_qos(QoS::AtLeastOnce); - self.delete_symlink_for_config_update(device, &response.config_type, cmd_id)?; + self.delete_symlink_for_config_update(target, &response.config_type, cmd_id)?; vec![c8y_notification, clear_local_cmd] } @@ -435,20 +414,16 @@ impl CumulocityConverter { let metadata = ConfigMetadata::from_json(message.payload_str()?)?; // get the device metadata from its id - let device = self.entity_store.get(topic_id).ok_or_else(|| { - CumulocityMapperError::UnregisteredDevice { - topic_id: topic_id.to_string(), - } - })?; + let target = self.entity_store.try_get(topic_id)?; // Create a c8y operation file - let dir_path = match device.r#type { + let dir_path = match target.r#type { EntityType::MainDevice => self.ops_dir.clone(), EntityType::ChildDevice => { - match &device.parent { + match &target.parent { Some(parent) if parent.is_default_main_device() => { // Support only first level child devices due to the limitation of our file system supported operations scheme. - self.ops_dir.join(device.external_id.as_ref()) + self.ops_dir.join(target.external_id.as_ref()) } _ => { warn!("config_snapshot and config_update features for nested child devices are currently unsupported"); @@ -510,7 +485,7 @@ impl CumulocityConverter { fn create_symlink_for_config_update( &self, - entity: &EntityMetadata, + target: &EntityMetadata, config_type: &str, cmd_id: &str, original: impl AsRef, @@ -519,7 +494,7 @@ impl CumulocityConverter { .config .data_dir .file_transfer_dir() - .join(entity.external_id.as_ref()) + .join(target.external_id.as_ref()) .join("config_update"); let symlink_path = symlink_dir_path.join(format!("{}-{cmd_id}", config_type.replace('/', ":"))); @@ -534,7 +509,7 @@ impl CumulocityConverter { fn delete_symlink_for_config_update( &self, - entity: &EntityMetadata, + target: &EntityMetadata, config_type: &str, cmd_id: &str, ) -> Result<(), io::Error> { @@ -542,7 +517,7 @@ impl CumulocityConverter { .config .data_dir .file_transfer_dir() - .join(entity.external_id.as_ref()) + .join(target.external_id.as_ref()) .join("config_update"); let symlink_path = symlink_dir_path.join(format!("{}-{cmd_id}", config_type.replace('/', ":"))); diff --git a/crates/extensions/c8y_mapper_ext/src/converter.rs b/crates/extensions/c8y_mapper_ext/src/converter.rs index 8b6d14c7383..fed3a1a99e9 100644 --- a/crates/extensions/c8y_mapper_ext/src/converter.rs +++ b/crates/extensions/c8y_mapper_ext/src/converter.rs @@ -310,11 +310,7 @@ impl CumulocityConverter { &self, entity_topic_id: &EntityTopicId, ) -> Result { - let entity = self.entity_store.get(entity_topic_id).ok_or_else(|| { - CumulocityMapperError::UnregisteredDevice { - topic_id: entity_topic_id.to_string(), - } - })?; + let entity = self.entity_store.try_get(entity_topic_id)?; let mut ancestors_external_ids = self.entity_store.ancestors_external_ids(entity_topic_id)?; @@ -633,12 +629,7 @@ impl CumulocityConverter { ) -> Result, CumulocityMapperError> { let request = SmartRestRestartRequest::from_smartrest(smartrest)?; let device_id = &request.device.into(); - let target = self - .entity_store - .get_by_external_id(device_id) - .ok_or_else(|| CumulocityMapperError::UnknownDevice { - device_id: device_id.as_ref().to_string(), - })?; + let target = self.entity_store.try_get_by_external_id(device_id)?; let command = RestartCommand::new(target.topic_id.clone()); let message = command.command_message(&self.mqtt_schema); Ok(vec![message]) @@ -1295,9 +1286,7 @@ impl CumulocityConverter { .entity_store .get(target) .and_then(C8yTopic::smartrest_response_topic) - .ok_or_else(|| CumulocityMapperError::UnregisteredDevice { - topic_id: target.to_string(), - })?; + .ok_or_else(|| Error::UnknownEntity(target.to_string()))?; match command.status() { CommandStatus::Executing => { diff --git a/crates/extensions/c8y_mapper_ext/src/error.rs b/crates/extensions/c8y_mapper_ext/src/error.rs index 3fa50f7e01f..09f296b0138 100644 --- a/crates/extensions/c8y_mapper_ext/src/error.rs +++ b/crates/extensions/c8y_mapper_ext/src/error.rs @@ -130,11 +130,8 @@ pub enum ConversionError { #[derive(thiserror::Error, Debug)] #[allow(clippy::enum_variant_names)] pub enum CumulocityMapperError { - #[error("Unknown device id: '{device_id}'")] - UnknownDevice { device_id: String }, - - #[error("Unregistered device topic: '{topic_id}'")] - UnregisteredDevice { topic_id: String }, + #[error(transparent)] + FromEntityStore(#[from] tedge_api::entity_store::Error), #[error(transparent)] InvalidTopicError(#[from] tedge_api::TopicError), diff --git a/crates/extensions/c8y_mapper_ext/src/log_upload.rs b/crates/extensions/c8y_mapper_ext/src/log_upload.rs index 1f30a9cf3c1..20448348a92 100644 --- a/crates/extensions/c8y_mapper_ext/src/log_upload.rs +++ b/crates/extensions/c8y_mapper_ext/src/log_upload.rs @@ -1,7 +1,6 @@ use crate::converter::CumulocityConverter; use crate::error::ConversionError; use crate::error::CumulocityMapperError; -use crate::error::CumulocityMapperError::UnknownDevice; use c8y_api::smartrest::smartrest_deserializer::SmartRestLogRequest; use c8y_api::smartrest::smartrest_deserializer::SmartRestRequestGeneric; use c8y_api::smartrest::smartrest_serializer::CumulocitySupportedOperations; @@ -53,10 +52,7 @@ impl CumulocityConverter { let device_external_id = log_request.device.into(); let target = self .entity_store - .get_by_external_id(&device_external_id) - .ok_or_else(|| UnknownDevice { - device_id: device_external_id.into(), - })?; + .try_get_by_external_id(&device_external_id)?; let cmd_id = nanoid!(); let channel = Channel::Command { @@ -102,16 +98,8 @@ impl CumulocityConverter { return Ok(vec![]); } - // get the device metadata from its id - let device = self.entity_store.get(topic_id).ok_or_else(|| { - CumulocityMapperError::UnregisteredDevice { - topic_id: topic_id.to_string(), - } - })?; - let external_id = &device.external_id; - + let external_id = self.entity_store.try_get(topic_id)?.external_id.as_ref(); let smartrest_topic = self.smartrest_publish_topic_for_entity(topic_id)?; - let payload = message.payload_str()?; let response = &LogUploadCmdPayload::from_json(payload)?; @@ -128,7 +116,7 @@ impl CumulocityConverter { .config .data_dir .file_transfer_dir() - .join(device.external_id.as_ref()) + .join(external_id) .join("log_upload") .join(format!("{}-{}", response.log_type, cmd_id)); let result = self @@ -136,7 +124,7 @@ impl CumulocityConverter { .upload_file( uploaded_file_path.as_std_path(), &response.log_type, - external_id.as_ref().to_string(), + external_id.to_string(), ) .await; // We need to get rid of this await, otherwise it blocks @@ -195,17 +183,13 @@ impl CumulocityConverter { let metadata = LogMetadata::from_json(message.payload_str()?)?; // get the device metadata from its id - let device = self.entity_store.get(topic_id).ok_or_else(|| { - CumulocityMapperError::UnregisteredDevice { - topic_id: topic_id.to_string(), - } - })?; + let target = self.entity_store.try_get(topic_id)?; // Create a c8y_LogfileRequest operation file - let dir_path = match device.r#type { + let dir_path = match target.r#type { EntityType::MainDevice => self.ops_dir.clone(), EntityType::ChildDevice => { - let child_dir_name = device.external_id.as_ref(); + let child_dir_name = target.external_id.as_ref(); self.ops_dir.clone().join(child_dir_name) } EntityType::Service => {