Merge pull request #2493 from jarhodes314/bug/smartrest-errors-quotes

Support quotes in Cumulocity failureReason field

crates/core/c8y_api/Cargo.toml

@@ -31,7 +31,6 @@ anyhow = { workspace = true }
 assert-json-diff = { workspace = true }
 assert_matches = { workspace = true }
 maplit = { workspace = true }
-regex = { workspace = true }
 tempfile = { workspace = true }
 test-case = { workspace = true }
 time = { workspace = true, features = ["macros"] }

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

@@ -1,5 +1,6 @@
 use crate::json_c8y::AlarmSeverity;
 use crate::json_c8y::C8yAlarm;
+use crate::smartrest::csv::fields_to_csv_string;
 use time::format_description::well_known::Rfc3339;
 
 /// Serialize C8yAlarm to SmartREST message
@@ -20,7 +21,7 @@ pub fn serialize_alarm(c8y_alarm: &C8yAlarm) -> Result<String, time::error::Form
                 alarm.time.format(&Rfc3339)?
             )
         }
-        C8yAlarm::Clear(alarm) => format!("306,{}", alarm.alarm_type),
+        C8yAlarm::Clear(alarm) => fields_to_csv_string(&["306", &alarm.alarm_type]),
     };
     Ok(smartrest)
 }

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

@@ -0,0 +1,25 @@
+pub fn fields_to_csv_string(record: &[&str]) -> String {
+    let mut writer = csv::Writer::from_writer(vec![]);
+    writer
+        .write_record(record)
+        .expect("write to vec never fails");
+    let mut output = writer.into_inner().expect("write to vec never fails");
+    output.pop();
+    String::from_utf8(output).expect("all input is utf-8")
+}
+
+#[cfg(test)]
+mod tests {
+    use crate::smartrest::csv::fields_to_csv_string;
+
+    #[test]
+    fn normal_fields_containing_commas_are_quoted() {
+        assert_eq!(fields_to_csv_string(&["503", "test,me"]), "503,\"test,me\"");
+    }
+
+    #[test]
+    fn normal_fields_containing_quotes_are_quoted() {
+        let rcd = fields_to_csv_string(&["503", r#"A value"with" quotes"#, "field"]);
+        assert_eq!(rcd, r#"503,"A value""with"" quotes",field"#);
+    }
+}

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

@@ -9,6 +9,7 @@
 // smartrest messages are sent. There should be one comprehensive API for
 // generating them.
 
+use crate::smartrest::csv::fields_to_csv_string;
 use crate::smartrest::topic::publish_topic_from_ancestors;
 use mqtt_channel::Message;
 
@@ -92,11 +93,13 @@ pub fn service_creation_message(
 
     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
-        ),
+        fields_to_csv_string(&[
+            "102",
+            service_id,
+            service_type,
+            service_name,
+            service_status,
+        ]),
     ))
 }
 
@@ -110,21 +113,16 @@ pub fn service_creation_message(
 /// to set the status of).
 ///
 /// https://cumulocity.com/guides/reference/smartrest-two/#104
-pub fn service_status_update_message(external_ids: &[String], service_status: &str) -> Message {
+pub fn service_status_update_message(
+    external_ids: &[impl AsRef<str>],
+    service_status: &str,
+) -> Message {
     let topic = publish_topic_from_ancestors(external_ids);
 
-    let mut service_status = sanitize_for_smartrest(
-        service_status.into(),
-        super::message::MAX_PAYLOAD_LIMIT_IN_BYTES,
-    );
-
-    if service_status.contains(',') {
-        service_status = format!("\"{service_status}\"");
-    }
-
-    let payload = format!("104,{service_status}");
+    let service_status =
+        sanitize_for_smartrest(service_status, super::message::MAX_PAYLOAD_LIMIT_IN_BYTES);
 
-    Message::new(&topic, payload)
+    Message::new(&topic, fields_to_csv_string(&["104", &service_status]))
 }
 
 #[derive(thiserror::Error, Debug)]

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

@@ -34,17 +34,18 @@ pub fn get_smartrest_template_id(payload: &str) -> String {
 /// - Strip the input according to `max_size`.
 // TODO: make this return Result
 // TODO: make a variant which assumes `max_size = MAX_PAYLOAD_LIMIT_IN_BYTES`
-pub fn sanitize_for_smartrest(input: Vec<u8>, max_size: usize) -> String {
-    String::from_utf8(input)
-        .unwrap_or_else(|err| {
-            error!("The input contains invalid UTF-8: {err}");
-            String::default()
-        })
+pub fn sanitize_bytes_for_smartrest(input: &[u8], max_size: usize) -> String {
+    let input = std::str::from_utf8(input).unwrap_or_else(|err| {
+        error!("The input contains invalid UTF-8: {err}");
+        ""
+    });
+    sanitize_for_smartrest(input, max_size)
+}
+
+pub fn sanitize_for_smartrest(input: &str, max_size: usize) -> String {
+    input
         .chars()
         .filter(|&c| c == '\r' || c == '\n' || c == '\t' || !c.is_control())
-        .collect::<String>()
-        .replace('"', "\"\"")
-        .chars()
         .scan(0, |bytes_count, c| {
             *bytes_count += c.len_utf8();
             Some((*bytes_count, c))
@@ -62,17 +63,16 @@ pub fn sanitize_for_smartrest(input: Vec<u8>, max_size: usize) -> String {
 /// If the input has only one line, returns the line only.
 /// If the input is empty or contains invalid UTF-8, it returns an empty String.
 /// The output is ensured to be SmartREST compatible.
-pub fn get_failure_reason_for_smartrest(input: Vec<u8>, max_size: usize) -> String {
-    let input_string = String::from_utf8(input).unwrap_or_else(|err| {
+pub fn get_failure_reason_for_smartrest(input: &[u8], max_size: usize) -> String {
+    let input_string = std::str::from_utf8(input).unwrap_or_else(|err| {
         error!("The input contains invalid UTF-8: {err}");
-        String::default()
+        ""
     });
-    let last_line = input_string.lines().last().unwrap_or_default();
-    let failure_reason = match input_string.lines().count() {
-        0 | 1 => last_line.to_string(),
-        _ => format!("{}\n\n{}", last_line, input_string.as_str()),
-    };
-    sanitize_for_smartrest(failure_reason.as_bytes().to_vec(), max_size)
+    let last_line = input_string.trim_end().lines().last().unwrap_or_default();
+    match input_string.lines().count() {
+        0 | 1 => sanitize_for_smartrest(last_line, max_size),
+        _ => sanitize_for_smartrest(&format!("{last_line}\n\n{input_string}"), max_size),
+    }
 }
 
 /// Split MQTT message payload to multiple SmartREST messages.
@@ -139,7 +139,6 @@ pub fn collect_smartrest_messages(data: &str) -> Vec<String> {
 #[cfg(test)]
 mod tests {
     use super::*;
-    use regex::Regex;
     use test_case::test_case;
 
     #[test_case("512,device_id", Some("device_id"); "valid")]
@@ -167,63 +166,62 @@ mod tests {
 
     #[test]
     fn selected_control_chars_remain() {
-        let input = vec![0x00, 0x09, 0x0A, 0x0D]; // NULL, \t, \n, \r
-        let sanitized = sanitize_for_smartrest(input, MAX_PAYLOAD_LIMIT_IN_BYTES);
+        let input = b"\0\t\n\r";
+        let sanitized = sanitize_bytes_for_smartrest(input, MAX_PAYLOAD_LIMIT_IN_BYTES);
         assert_eq!(sanitized, "\t\n\r".to_string());
     }
 
     #[test]
     fn control_chars_are_removed() {
-        let input: Vec<u8> = (0x00..0xff).collect();
-        let sanitized = sanitize_for_smartrest(input, MAX_PAYLOAD_LIMIT_IN_BYTES);
-        let re = Regex::new(r"[^\x20-\x7E\xA0-\xFF\t\n\r]").unwrap();
-        assert!(!re.is_match(&sanitized));
+        let input: Vec<u8> = (0x00..=0x7f).collect();
+        let sanitized = sanitize_bytes_for_smartrest(&input, MAX_PAYLOAD_LIMIT_IN_BYTES);
+        assert_eq!(
+            sanitized,
+            "\t\n\r !\"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~"
+        );
     }
 
     #[test]
     fn invalid_utf8_is_contained() {
         let invalid_sparkle_heart = vec![0, 159, 146, 150];
-        let sanitized = sanitize_for_smartrest(invalid_sparkle_heart, MAX_PAYLOAD_LIMIT_IN_BYTES);
-        assert_eq!(sanitized, "".to_string());
+        let sanitized =
+            sanitize_bytes_for_smartrest(&invalid_sparkle_heart, MAX_PAYLOAD_LIMIT_IN_BYTES);
+        assert_eq!(sanitized, "");
     }
 
     #[test]
     fn invalid_utf8_is_contained_last_line() {
         let invalid_sparkle_heart = vec![0, 159, 146, 150];
         let last_line =
-            get_failure_reason_for_smartrest(invalid_sparkle_heart, MAX_PAYLOAD_LIMIT_IN_BYTES);
-        assert_eq!(last_line, "".to_string());
+            get_failure_reason_for_smartrest(&invalid_sparkle_heart, MAX_PAYLOAD_LIMIT_IN_BYTES);
+        assert_eq!(last_line, "");
     }
 
     #[test_case("foo bar baz\n", "foo bar baz\n"; "standard")]
-    #[test_case("foo\r\nbar\tbaz\0\"", "foo\r\nbar\tbaz\"\""; "with control chars")]
+    #[test_case("foo\r\nbar\tbaz\0\"", "foo\r\nbar\tbaz\""; "with control chars")]
     #[test_case("baz", "baz"; "no new line")]
     #[test_case("", ""; "empty")]
     #[test_case("こんにちは", "こんにちは"; "no ascii")]
-    #[test_case("こんにちは\"\n\0こんにちは", "こんにちは\"\"\nこんにちは"; "no ascii and control chars")]
+    #[test_case("こんにちは\"\n\0こんにちは", "こんにちは\"\nこんにちは"; "no ascii and control chars")]
     fn u8_vec_is_sanitized(input: &str, expected_output: &str) {
-        let vec_u8 = input.as_bytes().to_vec();
-        let sanitized = sanitize_for_smartrest(vec_u8, MAX_PAYLOAD_LIMIT_IN_BYTES);
+        let sanitized = sanitize_for_smartrest(input, MAX_PAYLOAD_LIMIT_IN_BYTES);
         assert_eq!(sanitized, expected_output.to_string());
     }
 
     #[test_case("foo bar bye", "foo bar by"; "ascii")]
     #[test_case("こんにちは", "こんに"; "no ascii")]
     fn size_u8_vec_is_stripped(input: &str, expected_output: &str) {
-        let vec_u8 = input.as_bytes().to_vec();
-        let stripped = sanitize_for_smartrest(vec_u8, 10);
+        let stripped = sanitize_for_smartrest(input, 10);
         assert_eq!(stripped, expected_output.to_string());
     }
-
     #[test_case("baz\n", "baz"; "one line")]
     #[test_case("baz", "baz"; "no new line")]
     #[test_case("foo\r\nbar\n\nbaz\n", "baz\n\nfoo\r\n"; "multiline")]
     #[test_case("", ""; "empty")]
     #[test_case("おはよう\nこんにちは\n", "こんに"; "no ascii")]
     #[test_case("あ\nい\nう\nえ\nお\n", "お\n\nあ\n"; "no ascii2")]
     fn return_formatted_text_for_failure_reason_from_vec_u8(input: &str, expected_output: &str) {
-        let vec_u8 = input.as_bytes().to_vec();
-        let last_line = get_failure_reason_for_smartrest(vec_u8, 10);
+        let last_line = get_failure_reason_for_smartrest(input.as_bytes(), 10);
         assert_eq!(last_line.as_str(), expected_output);
     }
 

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

@@ -1,4 +1,5 @@
 pub mod alarm;
+pub mod csv;
 pub mod error;
 pub mod inventory;
 pub mod message;

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

@@ -5,12 +5,10 @@ use std::path::Path;
 use std::path::PathBuf;
 
 use crate::smartrest::error::OperationsError;
-use crate::smartrest::smartrest_serializer::SmartRestSerializer;
-use crate::smartrest::smartrest_serializer::SmartRestSetSupportedOperations;
+use crate::smartrest::smartrest_serializer::declare_supported_operations;
 use serde::Deserialize;
 use serde::Deserializer;
 
-use super::error::SmartRestSerializerError;
 use std::time::Duration;
 
 const DEFAULT_GRACEFUL_TIMEOUT: Duration = Duration::from_secs(3600);
@@ -173,11 +171,11 @@ impl Operations {
             .collect::<HashSet<String>>()
     }
 
-    pub fn create_smartrest_ops_message(&self) -> Result<String, SmartRestSerializerError> {
+    pub fn create_smartrest_ops_message(&self) -> String {
         let mut ops = self.get_operations_list();
         ops.sort();
-        let ops = ops.iter().map(|op| op as &str).collect::<Vec<&str>>();
-        SmartRestSetSupportedOperations::new(&ops).to_smartrest()
+        let ops = ops.iter().map(|op| op.as_str()).collect::<Vec<_>>();
+        declare_supported_operations(&ops)
     }
 }