Skip to content
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

Example datetime + tz where FoundDateTimeList is empty #34

Closed
lopopolo opened this issue Aug 5, 2022 · 3 comments
Closed

Example datetime + tz where FoundDateTimeList is empty #34

lopopolo opened this issue Aug 5, 2022 · 3 comments

Comments

@lopopolo
Copy link
Contributor

lopopolo commented Aug 5, 2022

I currently have an integration with tz-rs that looks like this:

pub fn new(
    year: i32,
    month: u8,
    day: u8,
    hour: u8,
    minute: u8,
    second: u8,
    nanoseconds: u32,
    offset: Offset,
) -> Result<Self> {
    let tz = offset.time_zone_ref();
    let found_date_times = DateTime::find(year, month, day, hour, minute, second, nanoseconds, tz)?;

    // .latest() will always return `Some(DateTime)`
    // FIXME: this assertion is not consistent with the docs in `tz-rs`.
    let dt = found_date_times.latest().expect("No datetime found with this offset");
    Ok(Self { inner: dt, offset })
}

https://github.com/artichoke/artichoke/blob/ae40b16294d699ea9e9df459785bd9578372b33a/spinoso-time/src/time/tzrs/mod.rs#L149-L166

I took a peek at the source and it looks like FoundDateTimeList is implemented with an inner Vec and the calls to earliest() and latest() essentially boil down to Vec::first and Vec::last, but I'm wondering: can this expect ever panic?

I'd love an example timespec + TZ where in practice that this found date time list is empty so I can add a test that makes this expect panic and enforce that this case is handled. Do you happen to know of one?

@x-hgg-x
Copy link
Owner

x-hgg-x commented Aug 5, 2022

The corresponding test for this is here:

check(time_zone_ref, results, (1970, 1, 1, 5, 0, 0), &[], None, None, None)?;

Here is a simplified example:

let local_time_types = vec![LocalTimeType::new(0, false, None)?, LocalTimeType::new(3600, false, None)?];

let time_zone = TimeZone::new(vec![Transition::new(0, 1), Transition::new(86400, 1)], local_time_types.clone(), vec![], None)?;
let time_zone_ref = time_zone.as_ref();

let result1 = DateTime::find(1969, 12, 31, 12, 0, 0, 0, time_zone_ref)?;
let result2 = DateTime::find(1970, 1, 1, 12, 0, 0, 0, time_zone_ref)?;
let result3 = DateTime::find(1970, 1, 2, 12, 0, 0, 0, time_zone_ref)?;

assert_eq!(result1.into_inner(), [FoundDateTimeKind::Normal(DateTime::new(1969, 12, 31, 12, 0, 0, 0, local_time_types[0])?)]);
assert_eq!(result2.into_inner(), [FoundDateTimeKind::Normal(DateTime::new(1970, 1, 1, 12, 0, 0, 0, local_time_types[1])?)]);
assert_eq!(result3.into_inner(), []);

The FoundDateTimeList can be empty if the provided time zone has no extra rule and the date time would be located after the last transition. In this case, the TimeZoneRef::find_local_time_type method also returns an error:

assert!(matches!(
    time_zone_ref.find_local_time_type(2 * 86400),
    Err(FindLocalTimeTypeError("no local time type is available for the specified timestamp"))
));

This situation can happen when using a TZif v1 file, which cannot contain a footer with an extra rule definition. If you are using the last version of the Time Zone Database, all TZif v1 files have been replaced by TZif v2 or v3 files, so this error should be uncommon.

@lopopolo
Copy link
Contributor Author

lopopolo commented Aug 5, 2022

Thank you!

@lopopolo lopopolo closed this as completed Aug 5, 2022
lopopolo added a commit to artichoke/artichoke that referenced this issue Aug 6, 2022
Handle a rare error condition in `tzrs::Time::new` which can occur when
using older TZif v1 files in older versions of the Time Zone Database.

Thanks to @x-hgg-x for providing an explainer on the failure scenario
and a test case, which has been replicated in `spinoso-time`.

See:

- x-hgg-x/tz-rs#34
- x-hgg-x/tz-rs#34 (comment)
@lopopolo
Copy link
Contributor Author

lopopolo commented Aug 6, 2022

Thanks @x-hgg-x for the test case!

I've replicated this error condition in a spinoso-time test case:

lopopolo added a commit to artichoke/artichoke that referenced this issue Aug 6, 2022
Handle a rare error condition in `tzrs::Time::new` which can occur when
using older TZif v1 files in older versions of the Time Zone Database.

Thanks to @x-hgg-x for providing an explainer on the failure scenario
and a test case, which has been replicated in `spinoso-time`.

See:

- x-hgg-x/tz-rs#34
- x-hgg-x/tz-rs#34 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants