From a2353c77fa2857a3536d683f884e4497366696af Mon Sep 17 00:00:00 2001 From: Marcel Guzik Date: Mon, 21 Oct 2024 13:05:27 +0200 Subject: [PATCH] Convert smartrest_serializer to SmartrestPayload Signed-off-by: Marcel Guzik --- crates/core/c8y_api/src/smartrest/message.rs | 46 +++++- .../src/smartrest/smartrest_serializer.rs | 131 +++++++++--------- .../c8y_mapper_ext/src/converter.rs | 2 +- .../src/operations/handlers/mod.rs | 7 +- 4 files changed, 112 insertions(+), 74 deletions(-) diff --git a/crates/core/c8y_api/src/smartrest/message.rs b/crates/core/c8y_api/src/smartrest/message.rs index 18c5a5f150..95f8aa4a8b 100644 --- a/crates/core/c8y_api/src/smartrest/message.rs +++ b/crates/core/c8y_api/src/smartrest/message.rs @@ -1,3 +1,6 @@ +use std::fmt::Display; + +use serde::Serialize; use tracing::error; // The actual limit defined by c8y is 16184 including header and body. @@ -36,11 +39,23 @@ impl SmartrestPayload { let payload = super::csv::fields_to_csv_string(fields); if payload.len() > super::message::MAX_PAYLOAD_LIMIT_IN_BYTES { - return Err(SmartrestPayloadError(anyhow::anyhow!( - "Message is larger ({}) than size limit ({})", - payload.len(), - MAX_PAYLOAD_LIMIT_IN_BYTES - ))); + return Err(SmartrestPayloadError::TooLarge(payload.len())); + } + + Ok(Self(payload)) + } + + pub fn serialize(record: S) -> Result { + let mut wtr = csv::Writer::from_writer(vec![]); + wtr.serialize(record)?; + let mut vec = wtr.into_inner().unwrap(); + // remove newline character + vec.pop(); + let payload = String::from_utf8(vec) + .expect("TODO: should SmartrestPayload wrap a string or a byte array?"); + + if payload.len() > super::message::MAX_PAYLOAD_LIMIT_IN_BYTES { + return Err(SmartrestPayloadError::TooLarge(payload.len())); } Ok(Self(payload)) @@ -59,8 +74,25 @@ impl SmartrestPayload { /// Errors that can occur when trying to create a SmartREST payload. #[derive(Debug, thiserror::Error)] -#[error("Could not create SmartrestPayload: {0:#}")] -pub struct SmartrestPayloadError(#[from] anyhow::Error); +pub enum SmartrestPayloadError { + #[error("Payload size ({0}) would be bigger than the limit ({MAX_PAYLOAD_LIMIT_IN_BYTES})")] + TooLarge(usize), + + #[error("Could not serialize the record")] + SerializeError(#[from] csv::Error), +} + +impl From for Vec { + fn from(value: SmartrestPayload) -> Self { + value.0.into_bytes() + } +} + +impl Display for SmartrestPayload { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(&self.0) + } +} /// Extract the Device ID from the SmartREST payload. /// diff --git a/crates/core/c8y_api/src/smartrest/smartrest_serializer.rs b/crates/core/c8y_api/src/smartrest/smartrest_serializer.rs index 4d7c63a29b..8329d00c2b 100644 --- a/crates/core/c8y_api/src/smartrest/smartrest_serializer.rs +++ b/crates/core/c8y_api/src/smartrest/smartrest_serializer.rs @@ -1,5 +1,6 @@ use crate::smartrest::csv::fields_to_csv_string; use crate::smartrest::error::SmartRestSerializerError; +use crate::smartrest::message::SmartrestPayloadError; use csv::StringRecord; use serde::ser::SerializeSeq; use serde::Serialize; @@ -17,39 +18,43 @@ pub fn request_pending_operations() -> &'static str { } /// Generates a SmartREST message to set the provided operation to executing -pub fn set_operation_executing_with_name(operation: impl C8yOperation) -> String { - fields_to_csv_string(["501", operation.name()]) +pub fn set_operation_executing_with_name(operation: impl C8yOperation) -> SmartrestPayload { + SmartrestPayload::from_fields(["501", operation.name()]) + .expect("operation name shouldn't put payload over size limit") } /// Generates a SmartREST message to set the provided operation ID to executing -pub fn set_operation_executing_with_id(op_id: &str) -> String { - fields_to_csv_string(["504", op_id]) +pub fn set_operation_executing_with_id(op_id: &str) -> SmartrestPayload { + SmartrestPayload::from_fields(["504", op_id]) + .expect("op_id shouldn't put payload over size limit") } /// Generates a SmartREST message to set the provided operation to failed with the provided reason -pub fn fail_operation_with_name(operation: impl C8yOperation, reason: &str) -> String { +pub fn fail_operation_with_name(operation: impl C8yOperation, reason: &str) -> SmartrestPayload { fail_operation("502", operation.name(), reason) } /// Generates a SmartREST message to set the provided operation ID to failed with the provided reason -pub fn fail_operation_with_id(op_id: &str, reason: &str) -> String { +pub fn fail_operation_with_id(op_id: &str, reason: &str) -> SmartrestPayload { fail_operation("505", op_id, reason) } -fn fail_operation(template_id: &str, operation: &str, reason: &str) -> String { +fn fail_operation(template_id: &str, operation: &str, reason: &str) -> SmartrestPayload { // If the failure reason exceeds 500 bytes, truncate it if reason.len() <= 500 { - fields_to_csv_string([template_id, operation, reason]) + SmartrestPayload::from_fields([template_id, operation, reason]) + .expect("operation name shouldn't put payload over size limit") } else { warn!("Failure reason too long, message truncated to 500 bytes"); - fields_to_csv_string([template_id, operation, &reason[..500]]) + SmartrestPayload::from_fields([template_id, operation, &reason[..500]]) + .expect("operation name shouldn't put payload over size limit") } } /// Generates a SmartREST message to set the provided operation to successful without a payload pub fn succeed_operation_with_name_no_parameters( operation: CumulocitySupportedOperations, -) -> String { +) -> SmartrestPayload { succeed_static_operation_with_name(operation, None::<&str>) } @@ -57,17 +62,20 @@ pub fn succeed_operation_with_name_no_parameters( pub fn succeed_static_operation_with_name( operation: CumulocitySupportedOperations, payload: Option>, -) -> String { +) -> SmartrestPayload { succeed_static_operation("503", operation.name(), payload) } /// Generates a SmartREST message to set the provided operation ID to successful without a payload -pub fn succeed_operation_with_id_no_parameters(op_id: &str) -> String { +pub fn succeed_operation_with_id_no_parameters(op_id: &str) -> SmartrestPayload { succeed_static_operation_with_id(op_id, None::<&str>) } /// Generates a SmartREST message to set the provided operation ID to successful with an optional payload -pub fn succeed_static_operation_with_id(op_id: &str, payload: Option>) -> String { +pub fn succeed_static_operation_with_id( + op_id: &str, + payload: Option>, +) -> SmartrestPayload { succeed_static_operation("506", op_id, payload) } @@ -75,17 +83,12 @@ fn succeed_static_operation( template_id: &str, operation: &str, payload: Option>, -) -> String { - let mut wtr = csv::Writer::from_writer(vec![]); - // Serialization will never fail for text +) -> SmartrestPayload { match payload { - Some(payload) => wtr.serialize((template_id, operation, payload.as_ref())), - None => wtr.serialize((template_id, operation)), + Some(payload) => SmartrestPayload::from_fields([template_id, operation, payload.as_ref()]), + None => SmartrestPayload::from_fields([template_id, operation]), } - .unwrap(); - let mut output = wtr.into_inner().unwrap(); - output.pop(); - String::from_utf8(output).unwrap() + .expect("operation name shouldn't put payload over size limit") } /// Generates a SmartREST message to set the provided custom operation to successful with a text or csv payload @@ -107,20 +110,17 @@ pub fn succeed_operation( template_id: &str, operation: &str, reason: impl Into, -) -> Result { +) -> Result { let reason: TextOrCsv = reason.into(); - let result = { - let mut wtr = csv::Writer::from_writer(vec![]); - wtr.serialize((template_id, operation, &reason))?; - let mut vec = wtr.into_inner().unwrap(); - // remove newline character - vec.pop(); - String::from_utf8(vec)? - }; + let result = SmartrestPayload::serialize((template_id, operation, &reason)); - if result.len() <= super::message::MAX_PAYLOAD_LIMIT_IN_BYTES { - return Ok(result); + match result { + Ok(payload) => return Ok(payload), + Err(SmartrestPayloadError::SerializeError(e)) => { + return Err(SmartRestSerializerError::InvalidCsv(e)) + } + Err(SmartrestPayloadError::TooLarge(_)) => {} } // payload too big, need to trim @@ -156,10 +156,11 @@ pub fn succeed_operation( } let trimmed_reason = &reason[..max_result_limit]; - format!("{prefix},\"{trimmed_reason}{trim_indicator}\"") - }; + let new_reason = format!("{trimmed_reason}{trim_indicator}"); - assert!(result.len() <= super::message::MAX_PAYLOAD_LIMIT_IN_BYTES); + SmartrestPayload::serialize((template_id, operation, new_reason)) + .expect("shouldn't be too large and serialization should succeed") + }; Ok(result) } @@ -167,14 +168,14 @@ pub fn succeed_operation( pub fn succeed_operation_with_name( operation: &str, reason: impl Into, -) -> Result { +) -> Result { succeed_operation("503", operation, reason) } pub fn succeed_operation_with_id( operation: &str, reason: impl Into, -) -> Result { +) -> Result { succeed_operation("506", operation, reason) } @@ -417,10 +418,10 @@ mod tests { fn serialize_smartrest_set_operation_to_executing() { let smartrest = set_operation_executing_with_name(CumulocitySupportedOperations::C8ySoftwareUpdate); - assert_eq!(smartrest, "501,c8y_SoftwareUpdate"); + assert_eq!(smartrest.as_str(), "501,c8y_SoftwareUpdate"); let smartrest = set_operation_executing_with_id("1234"); - assert_eq!(smartrest, "504,1234"); + assert_eq!(smartrest.as_str(), "504,1234"); } #[test] @@ -428,10 +429,10 @@ mod tests { let smartrest = succeed_operation_with_name_no_parameters( CumulocitySupportedOperations::C8ySoftwareUpdate, ); - assert_eq!(smartrest, "503,c8y_SoftwareUpdate"); + assert_eq!(smartrest.as_str(), "503,c8y_SoftwareUpdate"); let smartrest = succeed_operation_with_id_no_parameters("1234"); - assert_eq!(smartrest, "506,1234"); + assert_eq!(smartrest.as_str(), "506,1234"); } #[test] @@ -440,10 +441,10 @@ mod tests { CumulocitySupportedOperations::C8ySoftwareUpdate, Some("a payload"), ); - assert_eq!(smartrest, "503,c8y_SoftwareUpdate,a payload"); + assert_eq!(smartrest.as_str(), "503,c8y_SoftwareUpdate,a payload"); let smartrest = succeed_static_operation_with_id("1234", Some("a payload")); - assert_eq!(smartrest, "506,1234,a payload"); + assert_eq!(smartrest.as_str(), "506,1234,a payload"); } #[test] @@ -453,7 +454,7 @@ mod tests { TextOrCsv::Text("true,false,true".to_owned()), ) .unwrap(); - assert_eq!(smartrest, "503,c8y_RelayArray,\"true,false,true\""); + assert_eq!(smartrest.as_str(), "503,c8y_RelayArray,\"true,false,true\""); } #[test] @@ -463,7 +464,7 @@ mod tests { TextOrCsv::Csv(EmbeddedCsv("true,false,true".to_owned())), ) .unwrap(); - assert_eq!(smartrest, "503,c8y_RelayArray,true,false,true"); + assert_eq!(smartrest.as_str(), "503,c8y_RelayArray,true,false,true"); } #[test] @@ -482,7 +483,10 @@ mod tests { TextOrCsv::Csv(EmbeddedCsv("true,random\"quote".to_owned())), ) .unwrap(); - assert_eq!(smartrest, "503,c8y_RelayArray,true,\"random\"\"quote\""); + assert_eq!( + smartrest.as_str(), + "503,c8y_RelayArray,true,\"random\"\"quote\"" + ); } #[test] @@ -492,21 +496,21 @@ mod tests { "Failed due to permission.", ); assert_eq!( - smartrest, + smartrest.as_str(), "502,c8y_SoftwareUpdate,Failed due to permission." ); let smartrest = fail_operation_with_id("1234", "Failed due to permission."); - assert_eq!(smartrest, "505,1234,Failed due to permission."); + assert_eq!(smartrest.as_str(), "505,1234,Failed due to permission."); } #[test] fn serialize_smartrest_set_custom_operation_to_failed() { let smartrest = fail_operation_with_name("c8y_Custom", "Something went wrong"); - assert_eq!(smartrest, "502,c8y_Custom,Something went wrong"); + assert_eq!(smartrest.as_str(), "502,c8y_Custom,Something went wrong"); let smartrest = fail_operation_with_id("1234", "Something went wrong"); - assert_eq!(smartrest, "505,1234,Something went wrong"); + assert_eq!(smartrest.as_str(), "505,1234,Something went wrong"); } #[test] @@ -516,12 +520,15 @@ mod tests { "Failed due to permi\"ssion.", ); assert_eq!( - smartrest, + smartrest.as_str(), "502,c8y_SoftwareUpdate,\"Failed due to permi\"\"ssion.\"" ); let smartrest = fail_operation_with_id("1234", "Failed due to permi\"ssion."); - assert_eq!(smartrest, "505,1234,\"Failed due to permi\"\"ssion.\""); + assert_eq!( + smartrest.as_str(), + "505,1234,\"Failed due to permi\"\"ssion.\"" + ); } #[test] @@ -531,14 +538,14 @@ mod tests { "Failed to install collectd, modbus, and golang.", ); assert_eq!( - smartrest, + smartrest.as_str(), "502,c8y_SoftwareUpdate,\"Failed to install collectd, modbus, and golang.\"" ); let smartrest = fail_operation_with_id("1234", "Failed to install collectd, modbus, and golang."); assert_eq!( - smartrest, + smartrest.as_str(), "505,1234,\"Failed to install collectd, modbus, and golang.\"" ); } @@ -547,10 +554,10 @@ mod tests { fn serialize_smartrest_set_operation_to_failed_with_empty_reason() { let smartrest = fail_operation_with_name(CumulocitySupportedOperations::C8ySoftwareUpdate, ""); - assert_eq!(smartrest, "502,c8y_SoftwareUpdate,"); + assert_eq!(smartrest.as_str(), "502,c8y_SoftwareUpdate,"); let smartrest = fail_operation_with_id("1234", ""); - assert_eq!(smartrest, "505,1234,"); + assert_eq!(smartrest.as_str(), "505,1234,"); } #[test] @@ -627,7 +634,7 @@ mod tests { /// Make sure that `reason` field is trimmed correctly, even in presence of double quote /// sequences. - #[test_case(MAX_PAYLOAD_LIMIT_IN_BYTES - 1, 2; "skips_final_quote_because_wont_fit")] + #[test_case(MAX_PAYLOAD_LIMIT_IN_BYTES - 1, 0; "skips_final_quote_because_wont_fit")] #[test_case(MAX_PAYLOAD_LIMIT_IN_BYTES - 2, 4; "preserves_final_quote_because_fits")] fn succeed_operation_trims_reason_field_3171(message_len: usize, expected_num_quotes: usize) { let prefix_len = "503,c8y_Command,".len(); @@ -639,19 +646,17 @@ mod tests { // assert message is under size limit and has expected structure assert!( - smartrest.len() <= MAX_PAYLOAD_LIMIT_IN_BYTES, + smartrest.as_str().len() <= MAX_PAYLOAD_LIMIT_IN_BYTES, "bigger than message size limit: {} > {}", - smartrest.len(), + smartrest.as_str().len(), MAX_PAYLOAD_LIMIT_IN_BYTES ); - let mut fields = smartrest.split(','); + let mut fields = smartrest.as_str().split(','); assert_eq!(fields.next().unwrap(), "503"); assert_eq!(fields.next().unwrap(), "c8y_Command"); // assert trimming preserves valid double quotes let reason = fields.next().unwrap(); - assert!(reason.starts_with('"')); - assert!(reason.ends_with('"')); let num_quotes = reason.chars().filter(|c| *c == '"').count(); diff --git a/crates/extensions/c8y_mapper_ext/src/converter.rs b/crates/extensions/c8y_mapper_ext/src/converter.rs index c273f7d2da..6658a0a0fa 100644 --- a/crates/extensions/c8y_mapper_ext/src/converter.rs +++ b/crates/extensions/c8y_mapper_ext/src/converter.rs @@ -1063,7 +1063,7 @@ impl CumulocityConverter { }; mqtt_publisher - .send(MqttMessage::new(&topic, payload.as_bytes())) + .send(MqttMessage::new(&topic, payload.as_str())) .await .unwrap_or_else(|err| { error!( diff --git a/crates/extensions/c8y_mapper_ext/src/operations/handlers/mod.rs b/crates/extensions/c8y_mapper_ext/src/operations/handlers/mod.rs index 3de054869f..ca116a88ed 100644 --- a/crates/extensions/c8y_mapper_ext/src/operations/handlers/mod.rs +++ b/crates/extensions/c8y_mapper_ext/src/operations/handlers/mod.rs @@ -17,6 +17,7 @@ use crate::actor::IdUploadRequest; use crate::actor::IdUploadResult; use crate::Capabilities; use c8y_api::http_proxy::C8yEndPoint; +use c8y_api::smartrest::message::SmartrestPayload; use c8y_api::smartrest::smartrest_serializer::fail_operation_with_id; use c8y_api::smartrest::smartrest_serializer::fail_operation_with_name; use c8y_api::smartrest::smartrest_serializer::set_operation_executing_with_id; @@ -200,7 +201,7 @@ impl OperationContext { &self, operation: CumulocitySupportedOperations, cmd_id: &str, - ) -> c8y_api::smartrest::smartrest_serializer::SmartRest { + ) -> SmartrestPayload { match self.get_operation_id(cmd_id) { Some(op_id) if self.smart_rest_use_operation_id => { succeed_operation_with_id_no_parameters(&op_id) @@ -214,7 +215,7 @@ impl OperationContext { operation: CumulocitySupportedOperations, reason: &str, cmd_id: &str, - ) -> c8y_api::smartrest::smartrest_serializer::SmartRest { + ) -> SmartrestPayload { match self.get_operation_id(cmd_id) { Some(op_id) if self.smart_rest_use_operation_id => { fail_operation_with_id(&op_id, reason) @@ -342,7 +343,7 @@ pub fn get_smartrest_response_for_upload_result( operation: c8y_api::smartrest::smartrest_serializer::CumulocitySupportedOperations, use_operation_id: bool, op_id: Option, -) -> c8y_api::smartrest::smartrest_serializer::SmartRest { +) -> SmartrestPayload { match upload_result { Ok(_) => match op_id { Some(op_id) if use_operation_id => {