Fix two bugs in RFC-3339 date formatting (#489)

- Fix year boundaries to strictly adhere to RFC-3339
- Fix nanosecond precision issue

Author: jdisanti

rust-runtime/smithy-types/src/instant/format.rs

@@ -241,7 +241,7 @@ pub mod http_date {
 mod test_http_date {
     use proptest::prelude::*;
 
-    use crate::instant::format::{http_date, iso_8601, DateParseError};
+    use crate::instant::format::{http_date, rfc3339, DateParseError};
     use crate::Instant;
 
     #[test]
@@ -353,21 +353,21 @@ mod test_http_date {
     fn valid_iso_date() {
         let date = "1985-04-12T23:20:50.52Z";
         let expected = Instant::from_secs_and_nanos(482196050, 520000000);
-        assert_eq!(iso_8601::parse(date), Ok(expected));
+        assert_eq!(rfc3339::parse(date), Ok(expected));
     }
 
     #[test]
     fn iso_date_no_fractional() {
         let date = "1985-04-12T23:20:50Z";
         let expected = Instant::from_secs_and_nanos(482196050, 0);
-        assert_eq!(iso_8601::parse(date), Ok(expected));
+        assert_eq!(rfc3339::parse(date), Ok(expected));
     }
 
     #[test]
     fn read_iso_date_comma_split() {
         let date = "1985-04-12T23:20:50Z,1985-04-12T23:20:51Z";
-        let (e1, date) = iso_8601::read(date).expect("should succeed");
-        let (e2, date2) = iso_8601::read(&date[1..]).expect("should succeed");
+        let (e1, date) = rfc3339::read(date).expect("should succeed");
+        let (e2, date2) = rfc3339::read(&date[1..]).expect("should succeed");
         assert_eq!(date2, "");
         assert_eq!(date, ",1985-04-12T23:20:51Z");
         let expected = Instant::from_secs_and_nanos(482196050, 0);
@@ -386,7 +386,7 @@ mod test_http_date {
     }
 }
 
-pub mod iso_8601 {
+pub mod rfc3339 {
     use chrono::format;
 
     use crate::instant::format::DateParseError;
@@ -403,7 +403,7 @@ pub mod iso_8601 {
         let format = format::StrftimeItems::new("%Y-%m-%dT%H:%M:%S%.fZ");
         // TODO: it may be helpful for debugging to keep these errors around
         chrono::format::parse(&mut date, s, format)
-            .map_err(|_| DateParseError::Invalid("invalid iso8601 date"))?;
+            .map_err(|_| DateParseError::Invalid("invalid rfc3339 date"))?;
         let utc_date = date
             .to_naive_datetime_with_offset(0)
             .map_err(|_| DateParseError::Invalid("invalid date"))?;
@@ -413,14 +413,14 @@ pub mod iso_8601 {
         ))
     }
 
-    /// Read 1 ISO8601 date from &str and return the remaining str
+    /// Read 1 RFC-3339 date from &str and return the remaining str
     pub fn read(s: &str) -> Result<(Instant, &str), DateParseError> {
         let delim = s.find('Z').map(|idx| idx + 1).unwrap_or_else(|| s.len());
         let (head, rest) = s.split_at(delim);
         Ok((parse(head)?, &rest))
     }
 
-    /// Format an [Instant] in the ISO-8601 date format
+    /// Format an [Instant] in the RFC-3339 date format
     pub fn format(instant: &Instant) -> String {
         use std::fmt::Write;
         let (year, month, day, hour, minute, second, nanos) = {
@@ -436,79 +436,48 @@ pub mod iso_8601 {
             )
         };
 
+        // This is stated in the assumptions for RFC-3339. ISO-8601 allows for years
+        // between -99,999 and 99,999 inclusive, but RFC-3339 is bound between 0 and 9,999.
         assert!(
-            year.abs() <= 99_999,
-            "years greater than 5 digits are not supported by ISO-8601"
+            (0..=9_999).contains(&year),
+            "years must be between 0 and 9,999 in RFC-3339"
         );
 
         let mut out = String::with_capacity(33);
-        if (0..=9999).contains(&year) {
-            write!(out, "{:04}", year).unwrap();
-        } else if year < 0 {
-            write!(out, "{:05}", year).unwrap();
-        } else {
-            write!(out, "+{:05}", year).unwrap();
-        }
         write!(
             out,
-            "-{:02}-{:02}T{:02}:{:02}:{:02}",
-            month, day, hour, minute, second
+            "{:04}-{:02}-{:02}T{:02}:{:02}:{:02}",
+            year, month, day, hour, minute, second
         )
         .unwrap();
         format_nanos(&mut out, nanos);
         out.push('Z');
         out
     }
 
+    /// Formats sub-second nanos for RFC-3339 (including the '.').
+    /// Expects to be called with a number of `nanos` between 0 and 999_999_999 inclusive.
     fn format_nanos(into: &mut String, nanos: u32) {
+        debug_assert!(nanos < 1_000_000_000);
         if nanos > 0 {
             into.push('.');
-            let mut place = 100_000_000;
-            let mut pushed_non_zero = false;
-            while place > 0 {
-                let digit = (nanos / place) % 10;
-                if pushed_non_zero && digit == 0 {
-                    return;
-                }
-                pushed_non_zero = digit > 0;
+            let (mut remaining, mut place) = (nanos, 100_000_000);
+            while remaining > 0 {
+                let digit = (remaining / place) % 10;
                 into.push(char::from(b'0' + (digit as u8)));
+                remaining -= digit * place;
                 place /= 10;
             }
         }
     }
 }
 
 #[cfg(test)]
-mod test_iso_8601 {
-    use super::iso_8601::format;
+mod test {
+    use super::rfc3339::format;
     use crate::Instant;
-    use chrono::SecondsFormat;
     use proptest::proptest;
 
-    #[test]
-    fn year_edge_cases() {
-        assert_eq!(
-            "-0001-12-31T18:22:50Z",
-            format(&Instant::from_epoch_seconds(-62167239430))
-        );
-        assert_eq!(
-            "0001-05-06T02:15:00Z",
-            format(&Instant::from_epoch_seconds(-62124788700))
-        );
-        assert_eq!(
-            "+33658-09-27T01:46:40Z",
-            format(&Instant::from_epoch_seconds(1_000_000_000_000))
-        );
-        assert_eq!(
-            "-29719-04-05T22:13:20Z",
-            format(&Instant::from_epoch_seconds(-1_000_000_000_000))
-        );
-        assert_eq!(
-            "-1199-02-15T14:13:20Z",
-            format(&Instant::from_epoch_seconds(-100_000_000_000))
-        );
-    }
-
     #[test]
     fn no_nanos() {
         assert_eq!(
@@ -555,16 +524,26 @@ mod test_iso_8601 {
             "1970-01-01T00:00:00.000000001Z",
             format(&Instant::from_secs_and_nanos(0, 000_000_001))
         );
+        assert_eq!(
+            "1970-01-01T00:00:00.101Z",
+            format(&Instant::from_secs_and_nanos(0, 101_000_000))
+        );
     }
 
     proptest! {
-        // Sanity test against chrono (excluding nanos, which format differently)
+        // Sanity test against chrono
         #[test]
-        fn proptest_iso_8601(seconds in -1_000_000_000_000..1_000_000_000_000i64) {
-            let instant = Instant::from_epoch_seconds(seconds);
-            let chrono_formatted = instant.to_chrono_internal().to_rfc3339_opts(SecondsFormat::AutoSi, true);
+        #[cfg(feature = "chrono-conversions")]
+        fn proptest_rfc3339(
+            seconds in 0..253_402_300_799i64, // 0 to 9999-12-31T23:59:59
+            nanos in 0..1_000_000_000u32
+        ) {
+            use chrono::DateTime;
+
+            let instant = Instant::from_secs_and_nanos(seconds, nanos);
             let formatted = format(&instant);
-            assert_eq!(chrono_formatted, formatted);
+            let parsed: Instant = DateTime::parse_from_rfc3339(&formatted).unwrap().into();
+            assert_eq!(instant, parsed);
         }
     }
 }

rust-runtime/smithy-types/src/instant/mod.rs

@@ -61,7 +61,7 @@ impl Instant {
 
     pub fn from_str(s: &str, format: Format) -> Result<Self, DateParseError> {
         match format {
-            Format::DateTime => format::iso_8601::parse(s),
+            Format::DateTime => format::rfc3339::parse(s),
             Format::HttpDate => format::http_date::parse(s),
             Format::EpochSeconds => <f64>::from_str(s)
                 // TODO: Parse base & fraction separately to achieve higher precision
@@ -75,7 +75,7 @@ impl Instant {
     /// Enable parsing multiple dates from the same string
     pub fn read(s: &str, format: Format, delim: char) -> Result<(Self, &str), DateParseError> {
         let (inst, next) = match format {
-            Format::DateTime => format::iso_8601::read(s)?,
+            Format::DateTime => format::rfc3339::read(s)?,
             Format::HttpDate => format::http_date::read(s)?,
             Format::EpochSeconds => {
                 let split_point = s.find(delim).unwrap_or_else(|| s.len());
@@ -134,7 +134,7 @@ impl Instant {
 
     pub fn fmt(&self, format: Format) -> String {
         match format {
-            Format::DateTime => format::iso_8601::format(&self),
+            Format::DateTime => format::rfc3339::format(&self),
             Format::EpochSeconds => {
                 if self.subsecond_nanos == 0 {
                     format!("{}", self.seconds)
@@ -148,6 +148,20 @@ impl Instant {
     }
 }
 
+#[cfg(feature = "chrono-conversions")]
+impl From<DateTime<Utc>> for Instant {
+    fn from(value: DateTime<Utc>) -> Instant {
+        Instant::from_secs_and_nanos(value.timestamp(), value.timestamp_subsec_nanos())
+    }
+}
+
+#[cfg(feature = "chrono-conversions")]
+impl From<DateTime<chrono::FixedOffset>> for Instant {
+    fn from(value: DateTime<chrono::FixedOffset>) -> Instant {
+        value.with_timezone(&Utc).into()
+    }
+}
+
 #[derive(Clone, Copy, Eq, PartialEq)]
 pub enum Format {
     DateTime,
@@ -217,4 +231,13 @@ mod test {
         let (_, next) = Instant::read(s, Format::HttpDate, ',').expect("valid");
         assert_eq!(next, "Tue, 17 Dec 2019 23:48:18 GMT");
     }
+
+    #[test]
+    #[cfg(feature = "chrono-conversions")]
+    fn chrono_conversions_round_trip() {
+        let instant = Instant::from_secs_and_nanos(1234, 56789);
+        let chrono = instant.to_chrono();
+        let instant_again: Instant = chrono.into();
+        assert_eq!(instant, instant_again);
+    }
 }