Skip to content

Commit

Permalink
Merge pull request #3191 from Bravo555/improve/smartrest-types
Browse files Browse the repository at this point in the history
Improve validation of SmartREST payloads
  • Loading branch information
Bravo555 authored Oct 24, 2024
2 parents ba97db0 + 2a06d5f commit 94b992d
Show file tree
Hide file tree
Showing 12 changed files with 231 additions and 105 deletions.
2 changes: 1 addition & 1 deletion crates/core/c8y_api/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ homepage = { workspace = true }
repository = { workspace = true }

[dependencies]
anyhow = { workspace = true }
clock = { workspace = true }
csv = { workspace = true }
download = { workspace = true }
Expand All @@ -27,7 +28,6 @@ toml = { workspace = true }
tracing = { workspace = true }

[dev-dependencies]
anyhow = { workspace = true }
assert-json-diff = { workspace = true }
assert_matches = { workspace = true }
maplit = { workspace = true }
Expand Down
13 changes: 8 additions & 5 deletions crates/core/c8y_api/src/smartrest/alarm.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
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;

use super::payload::SmartrestPayload;

/// Serialize C8yAlarm to SmartREST message
pub fn serialize_alarm(c8y_alarm: &C8yAlarm) -> Result<String, time::error::Format> {
pub fn serialize_alarm(c8y_alarm: &C8yAlarm) -> Result<SmartrestPayload, time::error::Format> {
let smartrest = match c8y_alarm {
C8yAlarm::Create(alarm) => {
let smartrest_code = match alarm.severity {
Expand All @@ -13,14 +14,16 @@ pub fn serialize_alarm(c8y_alarm: &C8yAlarm) -> Result<String, time::error::Form
AlarmSeverity::Minor => "303",
AlarmSeverity::Warning => "304",
};
fields_to_csv_string(&[
SmartrestPayload::serialize([
smartrest_code,
&alarm.alarm_type,
&alarm.text,
&alarm.time.format(&Rfc3339)?,
])
.expect("TODO: should alarm text be trimmed?")
}
C8yAlarm::Clear(alarm) => fields_to_csv_string(&["306", &alarm.alarm_type]),
C8yAlarm::Clear(alarm) => SmartrestPayload::serialize((306, &alarm.alarm_type))
.expect("alarm type should be shorter than payload size limit"),
};
Ok(smartrest)
}
Expand Down Expand Up @@ -134,6 +137,6 @@ mod tests {
)]
fn check_alarm_translation(alarm: C8yAlarm, expected_smartrest_msg: &str) {
let smartrest = serialize_alarm(&alarm);
assert_eq!(smartrest.unwrap(), expected_smartrest_msg);
assert_eq!(smartrest.unwrap().into_inner(), expected_smartrest_msg);
}
}
9 changes: 6 additions & 3 deletions crates/core/c8y_api/src/smartrest/csv.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
pub fn fields_to_csv_string(record: &[&str]) -> String {
pub fn fields_to_csv_string<T>(record: impl IntoIterator<Item = T>) -> String
where
T: AsRef<str> + AsRef<[u8]>,
{
let mut writer = csv::Writer::from_writer(vec![]);
writer
.write_record(record)
Expand All @@ -14,12 +17,12 @@ mod tests {

#[test]
fn normal_fields_containing_commas_are_quoted() {
assert_eq!(fields_to_csv_string(&["503", "test,me"]), "503,\"test,me\"");
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"]);
let rcd = fields_to_csv_string(["503", r#"A value"with" quotes"#, "field"]);
assert_eq!(rcd, r#"503,"A value""with"" quotes",field"#);
}
}
46 changes: 25 additions & 21 deletions crates/core/c8y_api/src/smartrest/inventory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ use mqtt_channel::MqttMessage;
use std::time::Duration;
use tedge_config::TopicPrefix;

use super::payload::SmartrestPayload;

/// Create a SmartREST message for creating a child device under the given ancestors.
///
/// The provided ancestors list must contain all the parents of the given device
Expand Down Expand Up @@ -46,15 +48,17 @@ pub fn child_device_creation_message(
});
}

let payload = SmartrestPayload::serialize((
101,
child_id,
device_name.unwrap_or(child_id),
device_type.unwrap_or("thin-edge.io-child"),
))
.expect("child_id, device_name, device_type should not increase payload size over the limit");

Ok(MqttMessage::new(
&publish_topic_from_ancestors(ancestors, prefix),
// XXX: if any arguments contain commas, output will be wrong
format!(
"101,{},{},{}",
child_id,
device_name.unwrap_or(child_id),
device_type.unwrap_or("thin-edge.io-child")
),
payload.into_inner(),
))
}

Expand All @@ -71,7 +75,8 @@ pub fn service_creation_message(
) -> Result<MqttMessage, InvalidValueError> {
Ok(MqttMessage::new(
&publish_topic_from_ancestors(ancestors, prefix),
service_creation_message_payload(service_id, service_name, service_type, service_status)?,
service_creation_message_payload(service_id, service_name, service_type, service_status)?
.into_inner(),
))
}

Expand All @@ -83,7 +88,7 @@ pub fn service_creation_message_payload(
service_name: &str,
service_type: &str,
service_status: &str,
) -> Result<String, InvalidValueError> {
) -> Result<SmartrestPayload, InvalidValueError> {
// TODO: most of this noise can be eliminated by implementing `Serialize`/`Deserialize` for smartrest format
if service_id.is_empty() {
return Err(InvalidValueError {
Expand All @@ -110,13 +115,13 @@ pub fn service_creation_message_payload(
});
}

Ok(fields_to_csv_string(&[
"102",
service_id,
service_type,
service_name,
service_status,
]))
let payload =
SmartrestPayload::serialize((102, service_id, service_type, service_name, service_status))
.expect(
"TODO: message can get over the limit but none of the fields can be reasonably trimmed",
);

Ok(payload)
}

/// Create a SmartREST message to set a response interval for c8y_RequiredAvailability.
Expand All @@ -135,10 +140,9 @@ impl From<C8ySmartRestSetInterval117> for MqttMessage {
fn from(value: C8ySmartRestSetInterval117) -> Self {
let topic = value.c8y_topic.to_topic(&value.prefix).unwrap();
let interval_in_minutes = value.interval.as_secs() / 60;
MqttMessage::new(
&topic,
fields_to_csv_string(&["117", &interval_in_minutes.to_string()]),
)
let payload = SmartrestPayload::serialize((117, &interval_in_minutes))
.expect("interval should not increase size over the limit");
MqttMessage::new(&topic, payload.into_inner())
}
}

Expand All @@ -149,7 +153,7 @@ impl From<C8ySmartRestSetInterval117> for MqttMessage {
/// update and configuration update), the `profile_executed` field should be set
/// to `true`, otherwise it should be `false`.
pub fn set_c8y_profile_target_payload(profile_executed: bool) -> String {
fields_to_csv_string(&["121", &profile_executed.to_string()])
fields_to_csv_string(["121", &profile_executed.to_string()])
}

#[derive(thiserror::Error, Debug)]
Expand Down
1 change: 1 addition & 0 deletions crates/core/c8y_api/src/smartrest/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ pub mod error;
pub mod inventory;
pub mod message;
pub mod operations;
pub mod payload;
pub mod smartrest_deserializer;
pub mod smartrest_serializer;
pub mod topic;
4 changes: 3 additions & 1 deletion crates/core/c8y_api/src/smartrest/operations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ use std::path::PathBuf;
use std::time::Duration;
use tracing::warn;

use super::payload::SmartrestPayload;

const DEFAULT_GRACEFUL_TIMEOUT: Duration = Duration::from_secs(3600);
const DEFAULT_FORCEFUL_TIMEOUT: Duration = Duration::from_secs(60);

Expand Down Expand Up @@ -71,7 +73,7 @@ impl Operations {
.collect::<HashSet<String>>()
}

pub fn create_smartrest_ops_message(&self) -> String {
pub fn create_smartrest_ops_message(&self) -> SmartrestPayload {
let mut ops = self.get_operations_list();
ops.sort();
let ops = ops.iter().map(|op| op.as_str()).collect::<Vec<_>>();
Expand Down
99 changes: 99 additions & 0 deletions crates/core/c8y_api/src/smartrest/payload.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
use std::fmt::Display;

use serde::Serialize;
use tracing::error;

use super::message::MAX_PAYLOAD_LIMIT_IN_BYTES;

/// A Cumulocity SmartREST message payload.
///
/// A SmartREST message is either an HTTP request or an MQTT message that contains SmartREST topic and payload. The
/// payload is a CSV-like format that is backed by templates, either static or registered by the user. This struct
/// represents that payload, and should be used as such in SmartREST 1.0 and 2.0 message implementations.
///
/// # Example
///
/// ```text
/// 503,c8y_Command,"This is a ""Set operation to SUCCESSFUL (503)"" message payload; it has a template id (503),
/// operation fragment (c8y_Command), and optional parameters."
/// ```
///
/// # Reference
///
/// - https://cumulocity.com/docs/smartrest/smartrest-introduction/
#[derive(Debug, Clone, PartialEq, Eq)]
// TODO: pub(crate) for now so it can be constructed manually in serializer::succeed_operation, need to figure out a
// good API
pub struct SmartrestPayload(pub(crate) String);

impl SmartrestPayload {
/// Creates a payload that consists of a single record.
///
/// Doesn't trim any fields, so if the resulting payload is above size limit, returns an error.
pub fn serialize<S: Serialize>(record: S) -> Result<Self, SmartrestPayloadError> {
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("csv::Writer should never write invalid utf-8");

if payload.len() > MAX_PAYLOAD_LIMIT_IN_BYTES {
return Err(SmartrestPayloadError::TooLarge(payload.len()));
}

Ok(Self(payload))
}

/// Returns a string slice view of the payload.
pub fn as_str(&self) -> &str {
self.0.as_str()
}

/// Moves the underlying `String` out of the payload.
pub fn into_inner(self) -> String {
self.0
}
}

/// Errors that can occur when trying to create a SmartREST payload.
#[derive(Debug, thiserror::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<SmartrestPayload> for Vec<u8> {
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)
}
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn serializes_payload() {
let payload = SmartrestPayload::serialize((121, true)).unwrap();
assert_eq!(payload.as_str(), "121,true");
}

#[test]
fn returns_err_when_over_size_limit() {
let payload = "A".repeat(MAX_PAYLOAD_LIMIT_IN_BYTES + 1);
let payload = SmartrestPayload::serialize(payload);
assert!(matches!(payload, Err(SmartrestPayloadError::TooLarge(_))))
}
}
Loading

0 comments on commit 94b992d

Please sign in to comment.