BurntSushi commented on code in PR #634:
URL: https://github.com/apache/opendal-reqsign/pull/634#discussion_r2375809327
##########
services/aws-v4/src/provide_credential/cognito.rs:
##########
@@ -224,8 +224,8 @@ impl CognitoIdentityCredentialProvider {
.map_err(|e| Error::unexpected(format!("failed to parse
credentials response: {e}")))?;
let creds = result.credentials;
- let expires_in = DateTime::from_timestamp(creds.expiration, 0)
- .ok_or_else(|| Error::unexpected("invalid expiration
timestamp".to_string()))?;
+ let expires_in = DateTime::new(creds.expiration, 0)
Review Comment:
Nit: Maybe `DateTime::from_second` here instead?
##########
core/src/time.rs:
##########
@@ -18,68 +18,26 @@
//! Time related utils.
use crate::Error;
-use chrono::format::Fixed;
-use chrono::format::Item;
-use chrono::format::Numeric;
-use chrono::format::Pad;
-use chrono::SecondsFormat;
-use chrono::Utc;
+use std::str::FromStr;
-/// DateTime is the alias for chrono::DateTime<Utc>.
-pub type DateTime = chrono::DateTime<Utc>;
+/// DateTime is the alias for `jiff::Timestamp`.
+pub type DateTime = jiff::Timestamp;
Review Comment:
If you're doing a breaking change anyway, maybe it makes sense to rename
this to a `Timestamp`. I think `DateTime` is somewhat odd for just a simple
instant in time. (It's definitely not the worst thing though.)
##########
services/azure-storage/src/provide_credential/client_certificate.rs:
##########
@@ -33,8 +33,8 @@ use crate::credential::Credential;
/// Generate a unique JWT ID using timestamp and a pseudo-random component
fn generate_jti(now: u64) -> String {
// Use timestamp in nanoseconds + a hash of the timestamp for uniqueness
- let nano_time = std::time::SystemTime::now()
- .duration_since(std::time::UNIX_EPOCH)
+ let nano_time = SystemTime::now()
+ .duration_since(UNIX_EPOCH)
Review Comment:
I think this is just `jiff::Timestamp::now().as_nanosecond()`. Although I
guess that will behave differently than the code here for clocks set to a time
before `1970-01-01T00:00:00Z`.
##########
services/azure-storage/src/provide_credential/azure_cli.rs:
##########
@@ -96,12 +96,12 @@ impl ProvideCredential for AzureCliCredentialProvider {
// Calculate expiration time
let expires_on = if let Some(timestamp) = token.expires_on_timestamp {
- Some(chrono::DateTime::from_timestamp(timestamp, 0).unwrap())
+ Some(jiff::Timestamp::new(timestamp, 0).unwrap())
} else if let Some(expires_str) = token.expires_on {
// Parse the string format "2023-10-31 21:59:10.000000"
- chrono::NaiveDateTime::parse_from_str(&expires_str, "%Y-%m-%d
%H:%M:%S%.f")
+ jiff::civil::DateTime::strptime("%Y-%m-%d %H:%M:%S%.f",
&expires_str)
Review Comment:
Unless you specifically want to be strict here, you can just use
`expires_str.parse::<jiff::civil::DateTime>()` and it will handle `2023-10-31
21:59:10.000000` automatically. (But it will of course allow for other things
not allowed by `%Y-%m-%d %H:%M:%S%.f`.)
##########
services/azure-storage/src/provide_credential/azure_cli.rs:
##########
@@ -96,12 +96,12 @@ impl ProvideCredential for AzureCliCredentialProvider {
// Calculate expiration time
let expires_on = if let Some(timestamp) = token.expires_on_timestamp {
- Some(chrono::DateTime::from_timestamp(timestamp, 0).unwrap())
+ Some(jiff::Timestamp::new(timestamp, 0).unwrap())
Review Comment:
Nit: `jiff::Timestamp::from_second`
##########
core/src/time.rs:
##########
@@ -105,21 +63,30 @@ pub fn format_rfc3339(t: DateTime) -> String {
/// - `2022-03-01T08:12:34+00:00`
/// - `2022-03-01T08:12:34.00+00:00`
pub fn parse_rfc3339(s: &str) -> crate::Result<DateTime> {
- Ok(chrono::DateTime::parse_from_rfc3339(s)
- .map_err(|err| {
- Error::unexpected(format!("parse '{s}' into rfc3339
failed")).with_source(err)
- })?
- .with_timezone(&Utc))
+ FromStr::from_str(s).map_err(|err| {
Review Comment:
I think `s.parse().mape_err(...)` should work here?
##########
core/src/time.rs:
##########
@@ -89,12 +47,12 @@ const HTTP_DATE: &[Item<'static>] = &[
/// - Timezone is fixed to GMT.
/// - Day must be 2 digit.
pub fn format_http_date(t: DateTime) -> String {
- t.format_with_items(HTTP_DATE.iter()).to_string()
+ t.strftime("%a, %d %b %Y %T GMT").to_string()
Review Comment:
Would it make sense to use an explicit [RFC 9110
printer](https://docs.rs/jiff/latest/jiff/fmt/rfc2822/struct.DateTimePrinter.html#method.timestamp_to_rfc9110_string)
instead?
Note that I can't immediately spot anything obviously wrong with what you
have here. Probably the dedicated printer is faster, but I'm not sure if that
matters given the `String` alloc here anyway.
##########
services/azure-storage/src/provide_credential/azure_cli.rs:
##########
@@ -96,12 +96,12 @@ impl ProvideCredential for AzureCliCredentialProvider {
// Calculate expiration time
let expires_on = if let Some(timestamp) = token.expires_on_timestamp {
- Some(chrono::DateTime::from_timestamp(timestamp, 0).unwrap())
+ Some(jiff::Timestamp::new(timestamp, 0).unwrap())
} else if let Some(expires_str) = token.expires_on {
// Parse the string format "2023-10-31 21:59:10.000000"
- chrono::NaiveDateTime::parse_from_str(&expires_str, "%Y-%m-%d
%H:%M:%S%.f")
+ jiff::civil::DateTime::strptime("%Y-%m-%d %H:%M:%S%.f",
&expires_str)
+ .and_then(|dt| jiff::tz::TimeZone::UTC.to_timestamp(dt))
Review Comment:
This makes me somewhat sad. I realize this is what you were doing before,
but assuming UTC for datetime string without any time zone or offset is kind of
a bummer. It can be rather ambiguous. It's too easy for users to assume the
wrong thing (e.g., a local time).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
