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

Support Parsing Named Timezones #423

Closed
naftulikay opened this issue Jan 5, 2022 · 4 comments
Closed

Support Parsing Named Timezones #423

naftulikay opened this issue Jan 5, 2022 · 4 comments
Labels
A-parsing Area: parsing

Comments

@naftulikay
Copy link

I'm building a simple demo application which maintains a list of X509 certificates obtained from www.googleapis.com/oauth2/v1/certs. These certificates are used in Google's OAuth flow, these certificates sign and can be used to verify the returned JWT tokens. As part of my demo, I'm writing a service to wrap my certificate store which runs at an interval to refresh the certificates, based on the value of the expires HTTP header.

I'm using time because of the localtime_r security issue in chrono. I know it's fairly rare, but I like to have my code pass cargo audit.

Essentially this boils down to a loop:

loop {
    let resp = isahc::get_async("https://www.googleapis.com/oauth2/v1/certs").await?;

    let expires: OffsetDateTime = resp.headers().get("expires")
        .map(|h| h.to_str().unwrap())
        .map(|s| parse_expires_date(s))
        .unwrap();

    update_certificate_store(resp.json().await?);

    sleep_until(expires);
}

I was able to figure out how to parse datetimes using the format_description! macro; it wasn't easy but I was able to compile a description:

static EXPIRES_TIME_FORMAT: &'static [FormatItem<'static>] = format_description!(
    "[weekday repr:short], [day padding:zero] [month repr:short] [year repr:full] [hour repr:24]:[minute padding:zero]:[second padding:zero]"
);

The format of the expires header looks like this:

Thu, 06 Jan 2022 02:53:35 GMT

This is a well-known and widely used format described in RFC 7231.

I've written a test case:

static EXPIRES_TIME_FORMAT: &'static [FormatItem<'static>] = format_description!(
    "[weekday repr:short], [day padding:zero] [month repr:short] [year repr:full] [hour repr:24]:[minute padding:zero]:[second padding:zero]"
);

const EXPIRES_EXAMPLE: &'static str = "Thu, 06 Jan 2022 02:53:35";

#[test]
fn test_expires_header_parse() {
    let date = OffsetDateTime::parse(EXPIRES_EXAMPLE, EXPIRES_TIME_FORMAT).unwrap();

    println!("{:?}", date);
}

This unfortunately fails at runtime:

called `Result::unwrap()` on an `Err` value: TryFromParsed(InsufficientInformation)
thread 'google::tests::test_expires_header_parse' panicked at 'called `Result::unwrap()` on an `Err` value: TryFromParsed(InsufficientInformation)'

The only way I can get this to work is:

  1. Change the string datetime to Thu, 06 Jan 2022 02:53:35 00:00, replacing GMT with the hours/minutes UTC offset.
  2. Append [offset_hour padding:zero]:[offset_minute padding:zero] to the format description.

In the format description documentation, I don't see a way to specify a timezone by name, which makes parsing this HTTP date string impossible.

I can probably work around this issue by using regular expressions and another crate to replace the timezone name with the hours/minutes offset, but if this crate is going to be the default choice for the Rust community for datetime formatting/parsing/etc. we should probably support parsing timezones by name.

Is there any way that I can help with a fix?

@jhpratt
Copy link
Member

jhpratt commented Jan 5, 2022

I have not checked for the nuances of the format, but it appears to be roughly similar to RFC2822. If this is sufficient for your use case, it's already implemented and will be included in the next release.

In terms of actually supporting time zone names, this is unlikely. While GMT is unambiguous to my knowledge, other names are not. What does "CST" refer to? US central time, Mexico central time, or China standard time? Just to name a few — there's probably others as well. Even then this would require tzdb integration, which is tracked in #193.

@naftulikay
Copy link
Author

I have not checked for the nuances of the format, but it appears to be roughly similar to RFC2822. If this is sufficient for your use case, it's already implemented and will be included in the next release.

I'll take a look at that RFC. Hopefully the implementation of this will cover parsing the expires header as well, as that would be very convenient.

In terms of actually supporting time zone names, this is unlikely. While GMT is unambiguous to my knowledge, other names are not. What does "CST" refer to? US central time, Mexico central time, or China standard time?

Yes, you're absolutely right. I wish the format would be more like ISO-8601 and use either an explicit hours/minutes offset or Z for UTC. I think for my use-case I'll just replace GMT with 00:00 and then parse the string. I don't know if any implementations out there give timezone abbreviations other than GMT.

Even then this would require tzdb integration, which is tracked in #193.

Yes, saw that issue, that would be very helpful as an addition I think. Hopefully for static builds it would also be possible to bundle the timezone file into the binary to make sure there are no system dependencies at runtime.

@naftulikay
Copy link
Author

@jhpratt I have added a pull request to improve documentation at time-rs/book#2.

@jhpratt
Copy link
Member

jhpratt commented Mar 10, 2022

Closing for the reason of ambiguity as stated above.

@jhpratt jhpratt closed this as completed Mar 10, 2022
@jhpratt jhpratt added the A-parsing Area: parsing label Mar 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-parsing Area: parsing
Projects
None yet
Development

No branches or pull requests

2 participants