Use serde tag enum representation for CommandStatus

As suggested by thin-edge#2245 (comment), the reason field makes sense only for a Failed command. Signed-off-by: Didier Wenzek <[email protected]>
didier-wenzek · Sep 25, 2023 · 90d05dd · 90d05dd
1 parent 3ba5851
commit 90d05dd
Show file tree

Hide file tree

Showing 9 changed files with 34 additions and 54 deletions.
diff --git a/crates/core/tedge_agent/src/restart_manager/tests.rs b/crates/core/tedge_agent/src/restart_manager/tests.rs
@@ -36,7 +36,6 @@ async fn test_pending_restart_operation() -> Result<(), DynError> {
             cmd_id: "1234".to_string(),
             payload: RestartCommandPayload {
                 status: CommandStatus::Successful,
-                reason: "".to_string(),
             },
         }])
         .await;
@@ -60,8 +59,9 @@ async fn test_pending_restart_operation_failed() -> Result<(), DynError> {
             target: EntityTopicId::default_main_device(),
             cmd_id: "1234".to_string(),
             payload: RestartCommandPayload {
-                status: CommandStatus::Failed,
-                reason: "The agent has been restarted but not the device".to_string(),
+                status: CommandStatus::Failed {
+                    reason: "The agent has been restarted but not the device".to_string(),
+                },
             },
         }])
         .await;
@@ -86,7 +86,6 @@ async fn test_pending_restart_operation_successful() -> Result<(), DynError> {
             cmd_id: "1234".to_string(),
             payload: RestartCommandPayload {
                 status: CommandStatus::Successful,
-                reason: "".to_string(),
             },
         }])
         .await;
@@ -109,7 +108,6 @@ async fn test_new_restart_operation() -> Result<(), DynError> {
             cmd_id: "1234".to_string(),
             payload: RestartCommandPayload {
                 status: CommandStatus::Init,
-                reason: "".to_string(),
             },
         })
         .await?;

diff --git a/crates/core/tedge_agent/src/tedge_operation_converter/tests.rs b/crates/core/tedge_agent/src/tedge_operation_converter/tests.rs
@@ -108,7 +108,6 @@ async fn convert_incoming_restart_request() -> Result<(), DynError> {
             cmd_id: "random".to_string(),
             payload: RestartCommandPayload {
                 status: CommandStatus::Init,
-                reason: "".to_string(),
             },
         }])
         .await;
@@ -196,7 +195,6 @@ async fn convert_outgoing_restart_response() -> Result<(), DynError> {
         cmd_id: "abc".to_string(),
         payload: RestartCommandPayload {
             status: CommandStatus::Executing,
-            reason: "".to_string(),
         },
     };
     restart_box.send(executing_response).await?;

diff --git a/crates/core/tedge_api/src/messages.rs b/crates/core/tedge_api/src/messages.rs
@@ -525,12 +525,7 @@ impl RestartCommand {
 
     /// Return the current status of the command
     pub fn status(&self) -> CommandStatus {
-        self.payload.status
-    }
-
-    /// Return the reason why this command failed
-    pub fn reason(&self) -> &str {
-        &self.payload.reason
+        self.payload.status.clone()
     }
 
     /// Set the status of the command
@@ -541,8 +536,7 @@ impl RestartCommand {
 
     /// Set the failure reason of the command
     pub fn with_error(mut self, reason: String) -> Self {
-        self.payload.status = CommandStatus::Failed;
-        self.payload.reason = reason;
+        self.payload.status = CommandStatus::Failed { reason };
         self
     }
 
@@ -600,10 +594,8 @@ impl RestartCommand {
 #[derive(Debug, Clone, Deserialize, Serialize, Eq, PartialEq)]
 #[serde(rename_all = "camelCase")]
 pub struct RestartCommandPayload {
+    #[serde(flatten)]
     pub status: CommandStatus,
-
-    #[serde(default, skip_serializing_if = "String::is_empty")]
-    pub reason: String,
 }
 
 impl<'a> Jsonify<'a> for RestartCommandPayload {}
@@ -612,18 +604,17 @@ impl Default for RestartCommandPayload {
     fn default() -> Self {
         RestartCommandPayload {
             status: CommandStatus::Init,
-            reason: String::new(),
         }
     }
 }
 
-#[derive(Debug, Deserialize, Serialize, PartialEq, Copy, Eq, Clone)]
-#[serde(rename_all = "camelCase")]
+#[derive(Debug, Deserialize, Serialize, PartialEq, Eq, Clone)]
+#[serde(rename_all = "camelCase", tag = "status")]
 pub enum CommandStatus {
     Init,
     Executing,
     Successful,
-    Failed,
+    Failed { reason: String },
 }
 
 #[derive(Debug, Deserialize, Serialize, Eq, PartialEq)]
@@ -637,7 +628,8 @@ impl<'a> Jsonify<'a> for LogMetadata {}
 #[derive(Debug, Deserialize, Serialize, Eq, PartialEq, Clone)]
 #[serde(rename_all = "camelCase")]
 pub struct LogUploadCmdPayload {
-    pub status: CommandStatus, //Define a different enum if this op needs more states,
+    #[serde(flatten)]
+    pub status: CommandStatus,
     pub tedge_url: String,
     #[serde(rename = "type")]
     pub log_type: String,
@@ -648,93 +640,86 @@ pub struct LogUploadCmdPayload {
     #[serde(skip_serializing_if = "Option::is_none")]
     pub search_text: Option<String>,
     pub lines: usize,
-    #[serde(skip_serializing_if = "Option::is_none")]
-    pub reason: Option<String>,
 }
 
 impl<'a> Jsonify<'a> for LogUploadCmdPayload {}
 
 impl LogUploadCmdPayload {
     pub fn executing(&mut self) {
         self.status = CommandStatus::Executing;
-        self.reason = None;
     }
 
     pub fn successful(&mut self) {
         self.status = CommandStatus::Successful;
-        self.reason = None;
     }
 
     pub fn failed(&mut self, reason: impl Into<String>) {
-        self.status = CommandStatus::Failed;
-        self.reason = Some(reason.into());
+        self.status = CommandStatus::Failed {
+            reason: reason.into(),
+        };
     }
 }
 
 #[derive(Debug, Deserialize, Serialize, Eq, PartialEq, Clone)]
 #[serde(rename_all = "camelCase")]
 pub struct ConfigSnapshotCmdPayload {
+    #[serde(flatten)]
     pub status: CommandStatus,
     pub tedge_url: String,
     #[serde(rename = "type")]
     pub config_type: String,
     #[serde(skip_serializing_if = "Option::is_none")]
     pub path: Option<String>,
-    #[serde(skip_serializing_if = "Option::is_none")]
-    pub reason: Option<String>,
 }
 
 impl<'a> Jsonify<'a> for ConfigSnapshotCmdPayload {}
 
 impl ConfigSnapshotCmdPayload {
     pub fn executing(&mut self) {
         self.status = CommandStatus::Executing;
-        self.reason = None;
     }
 
     pub fn successful(&mut self, path: impl Into<String>) {
         self.status = CommandStatus::Successful;
-        self.reason = None;
         self.path = Some(path.into())
     }
 
     pub fn failed(&mut self, reason: impl Into<String>) {
-        self.status = CommandStatus::Failed;
-        self.reason = Some(reason.into());
+        self.status = CommandStatus::Failed {
+            reason: reason.into(),
+        }
     }
 }
 
 #[derive(Debug, Deserialize, Serialize, Eq, PartialEq, Clone)]
 #[serde(rename_all = "camelCase")]
 pub struct ConfigUpdateCmdPayload {
+    #[serde(flatten)]
     pub status: CommandStatus,
     pub tedge_url: String,
     pub remote_url: String,
     #[serde(rename = "type")]
     pub config_type: String,
     #[serde(skip_serializing_if = "Option::is_none")]
     pub path: Option<String>,
-    #[serde(skip_serializing_if = "Option::is_none")]
-    pub reason: Option<String>,
 }
 
 impl<'a> Jsonify<'a> for ConfigUpdateCmdPayload {}
 
 impl ConfigUpdateCmdPayload {
     pub fn executing(&mut self) {
         self.status = CommandStatus::Executing;
-        self.reason = None;
     }
 
     pub fn successful(&mut self, path: impl Into<String>) {
         self.status = CommandStatus::Successful;
-        self.reason = None;
         self.path = Some(path.into())
     }
 
     pub fn failed(&mut self, reason: impl Into<String>) {
-        self.status = CommandStatus::Failed;
-        self.reason = Some(reason.into());
+        self.status = CommandStatus::Failed {
+            reason: reason.into(),
+        };
     }
 }
 

diff --git a/crates/extensions/c8y_mapper_ext/src/converter.rs b/crates/extensions/c8y_mapper_ext/src/converter.rs
@@ -1060,10 +1060,10 @@ impl CumulocityConverter {
                     Message::new(&topic, smartrest_set_operation),
                 ])
             }
-            CommandStatus::Failed => {
+            CommandStatus::Failed { ref reason } => {
                 let smartrest_set_operation = SmartRestSetOperationToFailed::new(
                     CumulocitySupportedOperations::C8yRestartRequest,
-                    format!("Restart Failed: {}", command.reason()),
+                    format!("Restart Failed: {}", reason),
                 )
                 .to_smartrest()?;
                 Ok(vec![

diff --git a/crates/extensions/c8y_mapper_ext/src/log_upload.rs b/crates/extensions/c8y_mapper_ext/src/log_upload.rs
@@ -78,7 +78,6 @@ impl CumulocityConverter {
             date_to: log_request.date_to,
             search_text: log_request.search_text,
             lines: log_request.lines,
-            reason: None,
         };
 
         // Command messages must be retained
@@ -157,10 +156,10 @@ impl CumulocityConverter {
                     .with_qos(QoS::AtLeastOnce);
                 vec![c8y_notification, clean_operation]
             }
-            CommandStatus::Failed => {
+            CommandStatus::Failed { ref reason } => {
                 let smartrest_operation_status = SmartRestSetOperationToFailed::new(
                     CumulocitySupportedOperations::C8yLogFileRequest,
-                    response.reason.clone().unwrap_or_default(),
+                    reason.clone(),
                 )
                 .to_smartrest()?;
                 let c8y_notification = Message::new(&smartrest_topic, smartrest_operation_status);

diff --git a/crates/extensions/tedge_config_manager/src/actor.rs b/crates/extensions/tedge_config_manager/src/actor.rs
@@ -116,7 +116,7 @@ impl ConfigManagerActor {
                     self.handle_config_snapshot_request(&message.topic, request)
                         .await?;
                 }
-                CommandStatus::Successful | CommandStatus::Failed => {}
+                CommandStatus::Successful | CommandStatus::Failed { .. } => {}
             },
             Ok(Some(ConfigOperation::Update(request))) => match request.status {
                 CommandStatus::Init => {
@@ -132,7 +132,7 @@ impl ConfigManagerActor {
                     self.handle_config_update_request(&message.topic, request)
                         .await?;
                 }
-                CommandStatus::Successful | CommandStatus::Failed => {}
+                CommandStatus::Successful | CommandStatus::Failed { .. } => {}
             },
             Ok(None) => {}
             Err(ConfigManagementError::InvalidTopicError) => {

diff --git a/crates/extensions/tedge_config_manager/src/tests.rs b/crates/extensions/tedge_config_manager/src/tests.rs
@@ -325,7 +325,7 @@ async fn request_config_snapshot_that_does_not_exist() -> Result<(), anyhow::Err
         mqtt.recv().await,
         Some(MqttMessage::new(
             &config_topic,
-            r#"{"status":"failed","tedgeUrl":"http://127.0.0.1:3000/tedge/file-transfer/main/config-snapshot/type_five-1234","type":"type_five","reason":"Handling of operation failed with The requested config_type type_five is not defined in the plugin configuration file."}"#
+            r#"{"status":"failed","reason":"Handling of operation failed with The requested config_type type_five is not defined in the plugin configuration file.","tedgeUrl":"http://127.0.0.1:3000/tedge/file-transfer/main/config-snapshot/type_five-1234","type":"type_five"}"#
         ).with_retain())
     );
 
@@ -378,7 +378,7 @@ async fn put_config_snapshot_without_permissions() -> Result<(), anyhow::Error>
             mqtt.recv().await,
             Some(MqttMessage::new(
                 &config_topic,
-                r#"{"status":"failed","tedgeUrl":"http://127.0.0.1:3000/tedge/file-transfer/main/config-snapshot/type_two-1234","type":"type_two","reason":"Handling of operation failed with Failed with HTTP error status 403 Forbidden"}"#
+                r#"{"status":"failed","reason":"Handling of operation failed with Failed with HTTP error status 403 Forbidden","tedgeUrl":"http://127.0.0.1:3000/tedge/file-transfer/main/config-snapshot/type_two-1234","type":"type_two"}"#
             ).with_retain())
         );
 

diff --git a/crates/extensions/tedge_log_manager/src/actor.rs b/crates/extensions/tedge_log_manager/src/actor.rs
@@ -99,7 +99,7 @@ impl LogManagerActor {
                         self.handle_logfile_request_operation(&message.topic, request)
                             .await?;
                     }
-                    CommandStatus::Successful | CommandStatus::Failed => {
+                    CommandStatus::Successful | CommandStatus::Failed { .. } => {
                         self.config.current_operations.remove(&message.topic.name);
                     }
                 },

diff --git a/crates/extensions/tedge_log_manager/src/tests.rs b/crates/extensions/tedge_log_manager/src/tests.rs
@@ -262,7 +262,7 @@ async fn request_logtype_that_does_not_exist() -> Result<(), anyhow::Error> {
         mqtt.recv().await,
         Some(MqttMessage::new(
             &logfile_topic,
-            r#"{"status":"failed","tedgeUrl":"http://127.0.0.1:3000/tedge/file-transfer/main/log_upload/type_four-1234","type":"type_four","dateFrom":"1970-01-01T00:00:00Z","dateTo":"1970-01-01T00:00:30Z","lines":1000,"reason":"Handling of operation failed with No such file or directory for log type: type_four"}"#
+            r#"{"status":"failed","reason":"Handling of operation failed with No such file or directory for log type: type_four","tedgeUrl":"http://127.0.0.1:3000/tedge/file-transfer/main/log_upload/type_four-1234","type":"type_four","dateFrom":"1970-01-01T00:00:00Z","dateTo":"1970-01-01T00:00:30Z","lines":1000}"#
         ).with_retain())
     );
 
@@ -316,7 +316,7 @@ async fn put_logfiles_without_permissions() -> Result<(), anyhow::Error> {
             mqtt.recv().await,
             Some(MqttMessage::new(
                 &logfile_topic,
-                r#"{"status":"failed","tedgeUrl":"http://127.0.0.1:3000/tedge/file-transfer/main/log_upload/type_two-1234","type":"type_two","dateFrom":"1970-01-01T00:00:00Z","dateTo":"1970-01-01T00:00:30Z","lines":1000,"reason":"Handling of operation failed with Failed with HTTP error status 403 Forbidden"}"#
+                r#"{"status":"failed","reason":"Handling of operation failed with Failed with HTTP error status 403 Forbidden","tedgeUrl":"http://127.0.0.1:3000/tedge/file-transfer/main/log_upload/type_two-1234","type":"type_two","dateFrom":"1970-01-01T00:00:00Z","dateTo":"1970-01-01T00:00:30Z","lines":1000}"#
             ).with_retain())
         );
 
@@ -422,7 +422,7 @@ async fn read_log_from_file_that_does_not_exist() -> Result<(), anyhow::Error> {
         mqtt.recv().await,
         Some(MqttMessage::new(
             &logfile_topic,
-            r#"{"status":"failed","tedgeUrl":"http://127.0.0.1:3000/tedge/file-transfer/main/log_upload/type_three-1234","type":"type_three","dateFrom":"1970-01-01T00:00:00Z","dateTo":"1970-01-01T00:00:30Z","lines":1000,"reason":"Handling of operation failed with No such file or directory for log type: type_three"}"#
+            r#"{"status":"failed","reason":"Handling of operation failed with No such file or directory for log type: type_three","tedgeUrl":"http://127.0.0.1:3000/tedge/file-transfer/main/log_upload/type_three-1234","type":"type_three","dateFrom":"1970-01-01T00:00:00Z","dateTo":"1970-01-01T00:00:30Z","lines":1000}"#
         ).with_retain())
     );