time-rs / time

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

Subsecond format item is ignored if used with unix_timestamp #601

Closed vthib closed 1 year ago

vthib commented 1 year ago

With a format descriptor that contains both a unix timestamp and a subsecond component, the subsecond component is not taken into account.

Here is a repro that details the bug:

use time::OffsetDateTime;
use time::format_description as fd;
use time::macros::datetime;

fn main() {
    let date = OffsetDateTime::parse(
        "1234567890.123",
        &fd::parse("[unix_timestamp].[subsecond digits:3]").unwrap()
    ).unwrap();

    assert_eq!(
        date,
        datetime!(2009-02-13 23:31:30.123 +00:00:00)
    );
}

The assert is expected to match, however the program fails on:

thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `2009-02-13 23:31:30.0 +00:00:00`,
 right: `2009-02-13 23:31:30.123 +00:00:00`', src/main.rs:11:5

From what I can tell, this is caused by this code in parsing::parsed: https://github.com/time-rs/time/blob/d52798ed384a49a92a672cd374fd9ad586344a9b/time/src/parsing/parsed.rs#L795-L805

If the unix timestamp component is set, then a result is returned, and the subsecond component is ignored.

jhpratt commented 1 year ago

I don't think I've ever seen this format in the wild, but it's reasonable enough that I don't particularly care. I'll add this in soon :+1:

vthib commented 1 year ago

Right, I definitely could have added some context to this, sorry about that.

I got this bug while parsing libaudit records. Those are strings that look like this:

audit(1689607281.384:720639): saddr=100000000000000000000000SADDR={ saddr_fam=netlink nlnk-fam=16 nlnk-pid=0 }
audit(1689607281.384:720639): proctitle=617564697463746C002D44
audit(1689677070.760:211530): auid=1000 ses=1 op=add_rule key="toto" list=4 res=1AUID="vthib"

The format is: audit([unix ts].[milliseconds]:[serial number]): ....

So it does appear in the wild, though I agree I'm not entirely sure why they went with this format, instead of just removing the '.'

jhpratt commented 1 year ago

As I was about to implement this, I realized one thing. I store the timestamp (seconds) and timestamp (nanoseconds) in the same field, discarding any knowledge of the precision present. As such, the nanoseconds are implicitly zero, not None. This means that there's no way to tell whether the field is providing information not present or is conflicting with existing data.

Given that there already exist situations where there exists conflicting data and a single arbitrary choice is made, I don't have an issue with doing the same here — the subsecond value will override any equivalent from unix_timestamp.

jhpratt commented 1 year ago

Done.