time-rs / time

The most used Rust library for date and time handling.
https://time-rs.github.io
Apache License 2.0
1.06k stars 261 forks source link

Bug with leap second parsing ISO 8601 #674

Closed dracarys18 closed 3 months ago

dracarys18 commented 3 months ago

Facing an issue while parsing leap seconds in Iso8601. Currently in our system we generate current time and format it to Iso8601 format serialize and save it in redis. This serialization code goes like this,

    pub fn serialize<S>(date_time: &PrimitiveDateTime, serializer: S) -> Result<S::Ok, S::Error>
    where
        S: Serializer,
    {
        date_time
            .assume_utc()
            .format(&Iso8601::<FORMAT_CONFIG>)
            .map_err(S::Error::custom)?
            .serialize(serializer)
    }

And this method generates leap second every once in a while, example: 2023-12-11T11:31:60.000Z.

But while parsing it back to PrimitiveDateTime I am facing en error which says second must be in the range 0..=59. Deserializer code looks like this

    pub fn deserialize<'a, D>(deserializer: D) -> Result<PrimitiveDateTime, D::Error>
    where
        D: Deserializer<'a>,
    {
        let val = String::deserialize(deserializer)?;
        let time = OffsetDateTime::parse(&val, &Iso8601::<FORMAT_CONFIG>)
            .map_err(de::Error::custom)?
            .to_offset(UtcOffset::UTC);

        Ok(PrimitiveDateTime::new(time.date(), time.time()))
    }
jhpratt commented 3 months ago

Two things here:

  1. Leap seconds are not properly handled. A trivial demo shows this using a leap second that has actually occurred. This is a bug and will be fixed.
  2. If you are using 2023-12-11T11:31:60.000Z as your test case, don't. Leap seconds have only occurred as the last second of either half of the year (i.e. Jun 30, Dec 31 at 23:59:60). December 11 obviously is not that. Theoretically they can happen at the end of any month (per definition of UTC), but in reality preference is given to June or December, with March and September being alternates.
dracarys18 commented 3 months ago
  1. @jhpratt This is not a testcase this is from our logs
jhpratt commented 3 months ago

I would suggest looking into your logging system in that case, as it's producing invalid values.

dracarys18 commented 3 months ago

@jhpratt So this is generated by this function and I got this exact value from logs which was throwing deserialisation error.

jhpratt commented 3 months ago

Time holding a seconds value of 60 would be the only way for the crate to output that string barring a hard-coded literal in the format description. After quickly checking what is called all the way down, there's no way that that is what's causing this. The seconds field is ultimately calculated in Time::adjusting_add, and it's quite easy to verify that the implementation there is correct.

jhpratt commented 3 months ago

Fixed on main. It will be released shortly.