-
Notifications
You must be signed in to change notification settings - Fork 16
Validate range of DateTime values #217
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,41 @@ | ||
| crate::data_type!(DateTime); | ||
|
|
||
| impl DateTime { | ||
| /// Minimum value. | ||
| /// | ||
| /// See also: <https://reference.opcfoundation.org/Core/Part6/v105/docs/5.2.2.5> | ||
| pub const MIN: Self = Self(0); | ||
|
|
||
| /// Maximum value. | ||
| /// | ||
| /// See also: <https://reference.opcfoundation.org/Core/Part6/v105/docs/5.2.2.5> | ||
| pub const MAX: Self = Self(2_650_467_743_990_000_000); | ||
|
|
||
| /// Minimum UTC timestamp. | ||
| /// | ||
| /// See also: <https://reference.opcfoundation.org/Core/Part6/v105/docs/5.2.2.5> | ||
| #[cfg(feature = "time")] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can move all the individually guarded attributes and method |
||
| pub const MIN_UTC: time::OffsetDateTime = time::macros::datetime!(1601-01-01 00:00:00 UTC); | ||
|
|
||
| /// Maximum UTC timestamp. | ||
| /// | ||
| /// See also: <https://reference.opcfoundation.org/Core/Part6/v105/docs/5.2.2.5> | ||
| #[cfg(feature = "time")] | ||
| pub const MAX_UTC: time::OffsetDateTime = time::macros::datetime!(9999-12-31 23:59:59 UTC); | ||
|
|
||
| /// Converts the value to an UTC timestamp. | ||
| /// | ||
| /// The values [`MIN`](Self::MIN) and [`MAX`](Self::MAX) are converted as is and not | ||
| /// mapped to the corresponding limits of [`time::OffsetDateTime`]. | ||
| #[cfg(feature = "time")] | ||
| #[must_use] | ||
| pub fn to_utc(&self) -> Option<time::OffsetDateTime> { | ||
| use open62541_sys::{UA_DATETIME_UNIX_EPOCH, UA_DATETIME_USEC}; | ||
|
|
||
| // TODO: How to handle values that are out of range? | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sgoll ??
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since out-of-range values should never be sent over OPC UA, we can treat them as an exception. I would not panic, however, because those values are still runtime values from another system. That being said, I'd treat them similar to the possible error from
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. According to the spec out of range values should be clamped for encoding, i.e. either 0 (too low) or https://reference.opcfoundation.org/Core/Part6/v105/docs/5.2.2.5 🤔
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that's how I read it, too—when the value is equal to Similarly, for
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That said, I think we have to handle this case specially, i.e. we should have the following conditions:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could expose the "possibly clamped" state in the return type, either as On the other hand, since So, we could leave it to |
||
| //debug_assert!(*self >= Self::MIN, "DateTime exceeds minimum value"); | ||
| //debug_assert!(*self <= Self::MAX, "DateTime exceeds maximum value"); | ||
|
|
||
| // OPC UA encodes `DateTime` as Windows file time: a 64-bit value that represents the number | ||
| // of 100-nanosecond intervals that have elapsed since 12:00 A.M. January 1, 1601 (UTC). | ||
| let ticks_ua = i128::from(self.0); | ||
|
|
@@ -34,7 +64,8 @@ impl TryFrom<time::OffsetDateTime> for DateTime { | |
| /// | ||
| /// # Errors | ||
| /// | ||
| /// The date/time must be valid and in range of the 64-bit representation of [`DateTime`]. | ||
| /// The date/time must be valid and in range of the 64-bit representation of [`DateTime`] | ||
| /// within the limits [`MIN`](Self::MIN) and [`MAX`](Self::MAX). | ||
| fn try_from(from: time::OffsetDateTime) -> Result<Self, Self::Error> { | ||
| use open62541_sys::{UA_DATETIME_UNIX_EPOCH, UA_DATETIME_USEC}; | ||
|
|
||
|
|
@@ -44,6 +75,14 @@ impl TryFrom<time::OffsetDateTime> for DateTime { | |
| let ticks_unix = nanos_unix / i128::from(1000 / UA_DATETIME_USEC); | ||
| let ticks_ua = ticks_unix + i128::from(UA_DATETIME_UNIX_EPOCH); | ||
|
|
||
| // Limit to min/max values. | ||
| if ticks_ua < Self::MIN.0.into() { | ||
| return Err(crate::Error::internal("DateTime exceeds minimum value")); | ||
| } | ||
| if ticks_ua > Self::MAX.0.into() { | ||
| return Err(crate::Error::internal("DateTime exceeds maximum value")); | ||
| } | ||
|
|
||
| i64::try_from(ticks_ua) | ||
| // Explicit module path to avoid linter errors when feature is not enable by `#[cfg()]`. | ||
| .map_err(|_| crate::Error::internal("DateTime should be in range")) | ||
|
|
@@ -72,11 +111,33 @@ mod tests { | |
| let dt = time::macros::datetime!(2023-11-20 16:51:15.9876543 -2:00); | ||
| assert_eq!(time::macros::offset!(-2:00), dt.offset()); | ||
| assert_ne!(time::macros::offset!(UTC), dt.offset()); | ||
| let dt_ua = crate::ua::DateTime::try_from(dt).unwrap(); | ||
| let dt_ua = super::DateTime::try_from(dt).unwrap(); | ||
| let dt_utc = dt_ua.to_utc().unwrap(); | ||
| // Equal to the original timestamp, but the offset is now UTC. | ||
| assert_eq!(time::macros::offset!(UTC), dt_utc.offset()); | ||
| assert_ne!(dt.offset(), dt_utc.offset()); | ||
| assert_eq!(dt, dt_utc); | ||
| } | ||
|
|
||
| #[cfg(feature = "time")] | ||
| #[test] | ||
| fn min_max_utc() { | ||
| let min_ua = super::DateTime::try_from(super::DateTime::MIN_UTC).unwrap(); | ||
| assert_eq!(min_ua, super::DateTime::MIN); | ||
| assert!(super::DateTime::try_from( | ||
| super::DateTime(super::DateTime::MIN.0 - 1) | ||
| .to_utc() | ||
| .unwrap() | ||
| ) | ||
| .is_err()); | ||
|
|
||
| let max_ua = super::DateTime::try_from(super::DateTime::MAX_UTC).unwrap(); | ||
| assert_eq!(max_ua, super::DateTime::MAX); | ||
| assert!(super::DateTime::try_from( | ||
| super::DateTime(super::DateTime::MAX.0 + 1) | ||
| .to_utc() | ||
| .unwrap() | ||
| ) | ||
| .is_err()); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does this exact value come from? Is it the number of 100 nanosecond intervals that matches
9999-12-31 23:59:59 UTC? Maybe we can add a comment or otherwise make it clearer what the value represents.