Incorrect conversion of ISO8601 durations to `humantime` for certain values in Admin API

[nikrooz]

There appears to be an issue with the conversion of ISO8601 durations to humantime format when month/year is set:

• ❌P0Y1M1DT5H5M6S -> 1month 18h 31m 30s
• ✅P0Y0M1DT5H5M6S -> 1day 5h 5m 6s

[tillrohrmann]

@slinkydeveloper is this still a problem? If not, then maybe @pcholakov could take a look at it.

[pcholakov]

The trouble here is that humantime and ISO8601 have differing ideas about what is the meaning of a month.

#[test]
fn humantime_vs_iso8601() {
    let iso_duration = "P1M";
    let human_friendly_duration = "1 month";

    let d1 = serde_json::from_value::<MyDuration>(serde_json::Value::String(
        iso_duration.to_owned(),
    ))
    .unwrap()
    .0;
    // prints: d1 seconds: 2592000 (days: 30)
    eprintln!("d1 seconds: {} (days: {})", d1.as_secs(), d1.as_secs_f32() / (24. * 3600.));

    let d2 = serde_json::from_value::<MyDuration>(serde_json::Value::String(
        human_friendly_duration.to_owned(),
    ))
    .unwrap()
    .0;
    // prints: d2 seconds: 2630016 (days: 30.44)
    eprintln!("d2 seconds: {} (days: {})", d2.as_secs(), d2.as_secs_f32() / (24. * 3600.));

    assert_ne!(d1, d2, "you'd think they're the same but they're not");
}

When we parse a string, we turn it into a Duration based on the input format - so internally this is represented as seconds. However when we deserialize it back, we favor humantime - and thus an error can creep in. The ISO8601 standard assumes that by "1 month" you typically mean one calendar month, but that this can also be interpreted[^1] as:

3.15 month unit of time of 28, 29, 30 or 31 days NOTE In certain applications a month is regarded as a unit of time of 30 days

I can see the desire to make the API friendly but think using humantime in this context is just not appropriate. We may accept fuzzy inputs like it, but I strongly feel that we should favor precise outputs like seconds or ISO8601 in our outputs. Since humantime appears to be unmaintained, I'll take a look at jiff or other alternatives to see if we can switch to something else that's better aligned with ISO8601 - otherwise I'd probably advocate for switching to ISO8601 on deserialization, and paper over it in the UI if desired.

^1: https://www.digi.com/resources/documentation/digidocs/90001488-13/reference/r_iso_8601_duration_format.htm

[BurntSushi]

This section in the Jiff docs might help add some clarity here: https://docs.rs/jiff/latest/jiff/fmt/friendly/index.html#comparison-with-the-humantime-crate

That's about Jiff's "friendly" duration format, but Jiff also supports ISO 8601 durations too. Everything there about variable units applies to Jiff's ISO 8601 support too. In other words, with Jiff, you can freely parse variable length units (like years and months) without fear, because Jiff won't let you resolve those to a precise amount of seconds without giving a relative date.

[pcholakov]

Thanks so much @BurntSushi, that makes a lot of sense to me. I love that jiff has distinct concepts for spans and durations. In our case, we are updating values like timeouts and TTLs which need a consistent interpretation, and so SignedDuration is appropriate.

Since some users might input values much larger than 24h, it would be convenient to accept larger units - but then we must always output them back in a precise format like ISO8601. So we accept potentially ambiguous inputs but always serialize using hours as the largest unit, e.g.:

let span: Span = "1 week".parse()?;
let duration = span.to_duration(SpanRelativeTo::days_are_24_hours())?;
assert_eq!(format!("{duration}"), "PT168H");

What might be nice is a SpanRelativeTo::months_are_30_days() variant so that "1 month" consistently turns into "PT720H". Kudos for the jiff API, it's super well thought out!

[BurntSushi]

Thanks for the kind words!

That is an interesting idea. And presumably that would also set days to 24 hours too. I am skeptical of it but open to it.

I believe you can implement that today with a little effort. I'm on mobile, but you would just explicitly convert years and months to lower units based on what your preferred fixed value is. (If it isn't obvious to you how to do this, I would be happy to provide an example.) Days get a little special treatment in the API because it is correct to treat days as always 24 hours when using civil time.

[pcholakov]

I didn't realize just how easy it would be to do it without any additional extension points - I got this to work for parsing periods, we could also do it in reverse when rendering them for display. I'm not convinced we actually want this but it's easy enough to do if desired, and it might well be for dealing with long durations.

The code ended up looking something like:

let mut span: Span = input.parse()?;

let specified_months = span.get_months();
if specified_months > 0 {
    let months_in_hours = specified_months as i64 * 30 * 24.hours();
    span = span.months(0).checked_add(months_in_hours).unwrap();
};
// same for years at the desired conversion rate, e.g. 365.25 * 24h

span.to_duration(SpanRelativeTo::days_are_24_hours())

Thank you for the tips, really appreciate you stopping by! 😄