[Rust] Encapsulated HLC Error logic (#596)

Modularized HLC errors, and made them appropriate for user API

Author: cartertinney

rust/azure_iot_operations_protocol/src/application.rs

@@ -8,10 +8,7 @@ use std::{
     time::Duration,
 };
 
-use crate::common::{
-    aio_protocol_error::AIOProtocolError,
-    hybrid_logical_clock::{HybridLogicalClock, DEFAULT_MAX_CLOCK_DRIFT},
-};
+use crate::common::hybrid_logical_clock::{HLCError, HybridLogicalClock, DEFAULT_MAX_CLOCK_DRIFT};
 
 /// Struct containing the application-level [`HybridLogicalClock`].
 pub struct ApplicationHybridLogicalClock {
@@ -47,13 +44,13 @@ impl ApplicationHybridLogicalClock {
     /// other [`HybridLogicalClock`], and the current time, and its counter will also be updated accordingly.
     ///
     /// # Errors
-    /// [`AIOProtocolError`] of kind [`InternalLogicError`](crate::common::aio_protocol_error::AIOProtocolErrorKind::InternalLogicError) if
+    /// [`HLCError`] of kind [`OverflowWarning`](crate::common::hybrid_logical_clock::HLCErrorKind::OverflowWarning) if
     /// the [`ApplicationHybridLogicalClock`]'s counter would be set to a value that would overflow beyond [`u64::MAX`]
     ///
-    /// [`AIOProtocolError`] of kind [`StateInvalid`](crate::common::aio_protocol_error::AIOProtocolErrorKind::StateInvalid) if
+    /// [`HLCError`] of kind [`ClockDrift`](crate::common::hybrid_logical_clock::HLCErrorKind::ClockDrift) if
     /// the latest [`HybridLogicalClock`] (of [`ApplicationHybridLogicalClock`] or `other`)'s timestamp is too far in
     /// the future (determined by [`max_clock_drift`](ApplicationHybridLogicalClock::max_clock_drift)) compared to `SystemTime::now()`
-    pub(crate) fn update(&self, other_hlc: &HybridLogicalClock) -> Result<(), AIOProtocolError> {
+    pub(crate) fn update(&self, other_hlc: &HybridLogicalClock) -> Result<(), HLCError> {
         self.hlc
             .lock()
             .unwrap()
@@ -63,13 +60,13 @@ impl ApplicationHybridLogicalClock {
     /// Updates the [`ApplicationHybridLogicalClock`] with the current time and returns a `String` representation of the updated [`ApplicationHybridLogicalClock`].
     ///
     /// # Errors
-    /// [`AIOProtocolError`] of kind [`InternalLogicError`](crate::common::aio_protocol_error::AIOProtocolErrorKind::InternalLogicError) if
+    /// [`HLCError`] of kind [`OverflowWarning`](crate::common::hybrid_logical_clock::HLCErrorKind::OverflowWarning) if
     /// the [`HybridLogicalClock`]'s counter would be incremented and overflow beyond [`u64::MAX`]
     ///
-    /// [`AIOProtocolError`] of kind [`StateInvalid`](crate::common::aio_protocol_error::AIOProtocolErrorKind::StateInvalid) if
+    /// [`HLCError`] of kind [`ClockDrift`](crate::common::hybrid_logical_clock::HLCErrorKind::ClockDrift) if
     /// the [`ApplicationHybridLogicalClock`]'s timestamp is too far in the future (determined
     /// by [`max_clock_drift`](ApplicationHybridLogicalClock::max_clock_drift)) compared to `SystemTime::now()`
-    pub(crate) fn update_now(&self) -> Result<String, AIOProtocolError> {
+    pub(crate) fn update_now(&self) -> Result<String, HLCError> {
         let mut hlc = self.hlc.lock().unwrap();
         hlc.update_now(self.max_clock_drift)?;
         Ok(hlc.to_string())

rust/azure_iot_operations_protocol/src/common/aio_protocol_error.rs

@@ -5,7 +5,10 @@ use std::error::Error;
 use std::fmt;
 use std::time::Duration;
 
-use crate::common::topic_processor::{TopicPatternError, TopicPatternErrorKind};
+use crate::common::{
+    hybrid_logical_clock::{HLCError, HLCErrorKind, ParseHLCError},
+    topic_processor::{TopicPatternError, TopicPatternErrorKind},
+};
 
 /// Represents the kind of error that occurs in an Azure IoT Operations Protocol
 #[derive(Debug, PartialEq)]
@@ -573,3 +576,36 @@ impl AIOProtocolError {
         }
     }
 }
+
+impl From<HLCError> for AIOProtocolError {
+    fn from(error: HLCError) -> Self {
+        let (property_name, message) = match error.kind() {
+            HLCErrorKind::OverflowWarning => {
+                ("Counter", "Integer overflow on HybridLogicalClock counter")
+            }
+            HLCErrorKind::ClockDrift => (
+                "MaxClockDrift",
+                "HybridLogicalClock drift is greater than the maximum allowed drift",
+            ),
+        };
+
+        AIOProtocolError::new_state_invalid_error(
+            property_name,
+            None,
+            Some(message.to_string()),
+            None,
+        )
+    }
+}
+
+impl From<ParseHLCError> for AIOProtocolError {
+    fn from(error: ParseHLCError) -> Self {
+        AIOProtocolError::new_header_invalid_error(
+            "HybridLogicalClock",
+            error.input.as_str(),
+            false,
+            Some(format!("{error}")),
+            None,
+        )
+    }
+}

rust/azure_iot_operations_protocol/src/common/hybrid_logical_clock.rs

@@ -7,10 +7,9 @@ use std::{
     time::{Duration, SystemTime, UNIX_EPOCH},
 };
 
+use thiserror::Error;
 use uuid::Uuid;
 
-use super::aio_protocol_error::AIOProtocolError;
-
 /// Recommended default value for max clock drift if not specified.
 pub const DEFAULT_MAX_CLOCK_DRIFT: Duration = Duration::from_secs(60);
 
@@ -52,17 +51,17 @@ impl HybridLogicalClock {
     /// is a no-op, and will not result in an error.
     ///
     /// # Errors
-    /// [`AIOProtocolError`] of kind [`InternalLogicError`](crate::common::aio_protocol_error::AIOProtocolErrorKind::InternalLogicError) if
+    /// [`HLCError`] of kind [`OverflowWarning`](HLCErrorKind::OverflowWarning) if
     /// the [`HybridLogicalClock`]'s counter would be set to a value that would overflow beyond [`u64::MAX`]
     ///
-    /// [`AIOProtocolError`] of kind [`StateInvalid`](crate::common::aio_protocol_error::AIOProtocolErrorKind::StateInvalid) if
-    /// the latest [`HybridLogicalClock`] (of `Self` or `other`)'s timestamp is too far in the future (determined by `max_clock_drift`)
+    /// [`HLCError`] of kind [`ClockDrift`](HLCErrorKind::ClockDrift) if the latest [`HybridLogicalClock`]
+    /// (of `Self` or `other`)'s timestamp is too far in the future (determined by `max_clock_drift`)
     /// compared to [`SystemTime::now()`]
     pub fn update(
         &mut self,
         other: &HybridLogicalClock,
         max_clock_drift: Duration,
-    ) -> Result<(), AIOProtocolError> {
+    ) -> Result<(), HLCError> {
         let now = now_ms_precision();
         // Don't update from the same node.
         if self.node_id == other.node_id {
@@ -103,12 +102,13 @@ impl HybridLogicalClock {
     /// Updates the [`HybridLogicalClock`] based on the current time
     ///
     /// # Errors
-    /// [`AIOProtocolError`] of kind [`InternalLogicError`](crate::common::aio_protocol_error::AIOProtocolErrorKind::InternalLogicError) if
-    /// the [`HybridLogicalClock`]'s counter would be incremented and overflow beyond [`u64::MAX`]
+    /// [`HLCError`] of kind [`OverflowWarning`](HLCErrorKind::OverflowWarning) if
+    /// the [`HybridLogicalClock`]'s counter would be set to a value that would overflow beyond [`u64::MAX`]
     ///
-    /// [`AIOProtocolError`] of kind [`StateInvalid`](crate::common::aio_protocol_error::AIOProtocolErrorKind::StateInvalid) if
-    /// the [`HybridLogicalClock`]'s timestamp is too far in the future (determined by `max_clock_drift`) compared to [`SystemTime::now()`]
-    pub fn update_now(&mut self, max_clock_drift: Duration) -> Result<(), AIOProtocolError> {
+    /// [`HLCError`] of kind [`ClockDrift`](HLCErrorKind::ClockDrift) if the [`HybridLogicalClock`]
+    /// timestamp is too far in the future (determined by `max_clock_drift`) compared to [`SystemTime::now()`]
+    /// compared to [`SystemTime::now()`]
+    pub fn update_now(&mut self, max_clock_drift: Duration) -> Result<(), HLCError> {
         let now = now_ms_precision();
 
         // if now later than self, set the time to that and reset the counter
@@ -126,34 +126,18 @@ impl HybridLogicalClock {
     /// and that the counter will not overflow if it is increased.
     ///
     /// # Errors
-    /// [`AIOProtocolError`] of kind [`InternalLogicError`](crate::common::aio_protocol_error::AIOProtocolErrorKind::InternalLogicError)
-    /// if the [`HybridLogicalClock`] counter is at [`u64::MAX`]
+    /// [`HLCError`] of kind [`OverflowWarning`](HLCErrorKind::OverflowWarning) if
+    /// the [`HybridLogicalClock`]'s counter would be set to a value that would overflow beyond [`u64::MAX`]
     ///
-    /// [`AIOProtocolError`] of kind [`StateInvalid`](crate::common::aio_protocol_error::AIOProtocolErrorKind::StateInvalid)
-    /// if the [`HybridLogicalClock`]'s timestamp is too far in the future (determined by `max_clock_drift`)
-    fn validate(&self, now: SystemTime, max_clock_drift: Duration) -> Result<(), AIOProtocolError> {
+    /// [`HLCError`] of kind [`ClockDrift`](HLCErrorKind::ClockDrift) if the [`HybridLogicalClock`]
+    /// timestamp is too far in the future (determined by `max_clock_drift`) compared to [`SystemTime::now()`]
+    fn validate(&self, now: SystemTime, max_clock_drift: Duration) -> Result<(), HLCError> {
         if self.counter == u64::MAX {
-            return Err(AIOProtocolError::new_internal_logic_error(
-                true,
-                false,
-                None,
-                "Counter",
-                None,
-                Some("Integer overflow on HybridLogicalClock counter".to_string()),
-                None,
-            ));
+            return Err(HLCErrorKind::OverflowWarning)?;
         }
         if let Ok(diff) = self.timestamp.duration_since(now) {
             if diff > max_clock_drift {
-                return Err(AIOProtocolError::new_state_invalid_error(
-                    "MaxClockDrift",
-                    None,
-                    Some(
-                        "HybridLogicalClock drift is greater than the maximum allowed drift"
-                            .to_string(),
-                    ),
-                    None,
-                ));
+                return Err(HLCErrorKind::ClockDrift)?;
             }
         } // else negative time difference is ok, we only care if the HLC is too far in the future
 
@@ -177,58 +161,46 @@ impl Display for HybridLogicalClock {
 }
 
 impl FromStr for HybridLogicalClock {
-    type Err = AIOProtocolError;
+    type Err = ParseHLCError;
 
-    fn from_str(s: &str) -> Result<Self, AIOProtocolError> {
+    fn from_str(s: &str) -> Result<Self, ParseHLCError> {
         let parts: Vec<&str> = s.split(':').collect();
         if parts.len() != 3 {
-            return Err(AIOProtocolError::new_header_invalid_error(
-                "HybridLogicalClock",
-                s,
-                false,
-                None,
-                None,
-            ));
+            return Err(ParseHLCError {
+                message: "Incorrect format".to_string(),
+                input: s.to_string(),
+            });
         }
 
         // Validate first part (timestamp)
         let ms_since_epoch = match parts[0].parse::<u64>() {
             Ok(ms) => ms,
             Err(e) => {
-                return Err(AIOProtocolError::new_header_invalid_error(
-                    "HybridLogicalClock",
-                    s,
-                    false,
-                    Some(format!(
+                return Err(ParseHLCError {
+                    message: format!(
                         "Malformed HLC. Could not parse first segment as an integer: {e}"
-                    )),
-                    None,
-                ));
+                    ),
+                    input: s.to_string(),
+                })
             }
         };
         let Some(timestamp) = UNIX_EPOCH.checked_add(Duration::from_millis(ms_since_epoch)) else {
-            return Err(AIOProtocolError::new_header_invalid_error(
-                "HybridLogicalClock",
-                s,
-                false,
-                Some("Malformed HLC. Timestamp is out of range.".to_string()),
-                None,
-            ));
+            return Err(ParseHLCError {
+                message: "Malformed HLC. Timestamp is out of range.".to_string(),
+                input: s.to_string(),
+            });
         };
 
         // Validate second part (counter)
         let counter = match parts[1].parse::<u64>() {
             Ok(val) => val,
             Err(e) => {
-                return Err(AIOProtocolError::new_header_invalid_error(
-                    "HybridLogicalClock",
-                    s,
-                    false,
-                    Some(format!(
+                return Err(ParseHLCError {
+                    message: format!(
                         "Malformed HLC. Could not parse second segment as an integer: {e}"
-                    )),
-                    None,
-                ));
+                    ),
+                    input: s.to_string(),
+                });
             }
         };
 
@@ -278,6 +250,41 @@ fn now_ms_precision() -> SystemTime {
     now
 }
 
+/// Represents errors that occur in the use of an HLC
+#[derive(Debug, Error)]
+#[error("{0}")]
+pub struct HLCError(#[from] HLCErrorKind);
+
+impl HLCError {
+    /// Returns the corresponding [`HLCErrorKind`] for this error
+    #[must_use]
+    pub fn kind(&self) -> &HLCErrorKind {
+        &self.0
+    }
+}
+
+/// A list specifying categories of HLC error
+#[derive(Debug, Error)]
+pub enum HLCErrorKind {
+    /// The counter would be incremented to a value that would overflow beyond [`u64::MAX`]
+    #[error("counter cannot be incremented")]
+    OverflowWarning,
+    /// The HLC's timestamp is too far in the future compared to the current time
+    #[error("exceeds max clock drift")]
+    ClockDrift,
+}
+
+/// Represents errors that occur when parsing an HLC from a string
+#[derive(Debug, Error)]
+#[error("{message}")]
+pub struct ParseHLCError {
+    /// The error message
+    message: String,
+    /// The input string that failed to parse
+    // NOTE: This is only needed for AIOProtocolError compatibility
+    pub input: String,
+}
+
 // Functions to allow manipulation of the system time for testing purposes
 #[cfg(test)]
 use std::cell::Cell;
@@ -296,12 +303,7 @@ fn set_time_offset(offset: Duration, positive: bool) {
 
 #[cfg(test)]
 mod tests {
-    use crate::common::{
-        aio_protocol_error::AIOProtocolErrorKind,
-        hybrid_logical_clock::{
-            now_ms_precision, set_time_offset, HybridLogicalClock, DEFAULT_MAX_CLOCK_DRIFT,
-        },
-    };
+    use super::*;
     use std::time::{Duration, UNIX_EPOCH};
     use test_case::test_case;
     use uuid::Uuid;
@@ -330,9 +332,7 @@ mod tests {
             Ok(()) => assert!(should_succeed),
             Err(e) => {
                 assert!(!should_succeed);
-                assert_eq!(e.kind, AIOProtocolErrorKind::StateInvalid);
-                assert_eq!(e.property_name, Some("MaxClockDrift".to_string()));
-                assert_eq!(e.property_value, None);
+                matches!(e.kind(), HLCErrorKind::ClockDrift);
             }
         }
     }
@@ -362,9 +362,7 @@ mod tests {
         match hlc.validate(now_ms_precision(), max_drift) {
             Ok(()) => panic!("Expected error"),
             Err(e) => {
-                assert_eq!(e.kind, AIOProtocolErrorKind::StateInvalid);
-                assert_eq!(e.property_name, Some("MaxClockDrift".to_string()));
-                assert_eq!(e.property_value, None);
+                matches!(e.kind(), HLCErrorKind::ClockDrift);
             }
         }
     }
@@ -378,11 +376,7 @@ mod tests {
         match hlc.validate(now_ms_precision(), DEFAULT_MAX_CLOCK_DRIFT) {
             Ok(()) => panic!("Expected error"),
             Err(e) => {
-                assert_eq!(e.kind, AIOProtocolErrorKind::InternalLogicError);
-                assert!(e.is_shallow);
-                assert!(!e.is_remote);
-                assert_eq!(e.property_name, Some("Counter".to_string()));
-                assert_eq!(e.property_value, None);
+                matches!(e.kind(), HLCErrorKind::OverflowWarning);
             }
         }
 
@@ -428,9 +422,7 @@ mod tests {
             }
             Err(e) => {
                 assert!(!should_succeed);
-                assert_eq!(e.kind, AIOProtocolErrorKind::StateInvalid);
-                assert_eq!(e.property_name, Some("MaxClockDrift".to_string()));
-                assert_eq!(e.property_value, None);
+                matches!(e.kind(), HLCErrorKind::ClockDrift);
             }
         }
     }
@@ -555,9 +547,7 @@ mod tests {
             }
             Err(e) => {
                 assert!(!should_succeed);
-                assert_eq!(e.kind, AIOProtocolErrorKind::StateInvalid);
-                assert_eq!(e.property_name, Some("MaxClockDrift".to_string()));
-                assert_eq!(e.property_value, None);
+                matches!(e.kind(), HLCErrorKind::ClockDrift);
                 // self hlc should not have been updated
                 assert_eq!(self_hlc.counter, 0);
                 assert_eq!(self_hlc.timestamp, self_ts_copy);
@@ -623,8 +613,7 @@ mod tests {
             }
             Err(e) => {
                 assert!(!should_succeed);
-                assert_eq!(e.kind, AIOProtocolErrorKind::InternalLogicError);
-                assert_eq!(e.property_name, Some("Counter".to_string()));
+                matches!(e.kind(), HLCErrorKind::OverflowWarning);
                 // self hlc should not have been updated
                 assert_eq!(self_hlc.counter, self_counter_copy);
                 assert_eq!(self_hlc.timestamp, self_ts_copy);