Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions core/core/src/raw/time.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,12 @@
//! Time related utils.

use crate::*;
use jiff::SignedDuration;

use std::fmt;
use std::ops::{Add, AddAssign, Sub, SubAssign};
use std::str::FromStr;

pub use jiff::SignedDuration;
pub use std::time::{Duration, UNIX_EPOCH};
#[cfg(not(all(target_arch = "wasm32", target_os = "unknown")))]
pub use std::time::{Instant, SystemTime};
Expand Down Expand Up @@ -254,15 +255,20 @@ impl SubAssign<Duration> for Timestamp {
}
}

/// Convert an unsigned [`Duration`] into a jiff [`SignedDuration`].
/// Parse a duration encoded either as ISO-8601 (e.g. `PT5M`) or friendly (e.g. `5m`).
#[inline]
pub fn signed_to_duration(value: &str) -> Result<Duration> {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a follow up I actually think we can investiagte the usage of this method and port all of them to signed_duration_to_duration. This makes all XxxConfig use a typed struct rather than ttl: Option<String> and relying on the implicit contract.

let signed = value.parse::<SignedDuration>().map_err(|err| {
Error::new(ErrorKind::ConfigInvalid, "failed to parse duration").set_source(err)
})?;

Duration::try_from(signed).map_err(|err| {
signed_duration_to_duration(signed)
}

/// Convert a jiff [`SignedDuration`] into an unsigned [`Duration`].
#[inline]
pub fn signed_duration_to_duration(value: SignedDuration) -> Result<Duration> {
Duration::try_from(value).map_err(|err| {
Error::new(
ErrorKind::ConfigInvalid,
"duration must not be negative or overflow",
Expand Down Expand Up @@ -304,4 +310,9 @@ mod tests {
let v = Timestamp::parse_rfc2822(s).unwrap();
assert_eq!("Sat, 29 Oct 1994 19:43:31 GMT", v.format_http_date());
}

#[test]
fn test_signed_duration_to_duration_rejects_negative_values() {
assert!(signed_duration_to_duration(SignedDuration::from_secs(-1)).is_err());
}
}
15 changes: 12 additions & 3 deletions core/services/cloudflare-kv/src/backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ use super::writer::CloudflareWriter;
#[derive(Default)]
pub struct CloudflareKvBuilder {
pub(super) config: CloudflareKvConfig,
pub(super) default_ttl: Option<Duration>,
}

impl Debug for CloudflareKvBuilder {
Expand Down Expand Up @@ -76,7 +77,7 @@ impl CloudflareKvBuilder {
///
/// If set, we will specify `EX` for write operations.
pub fn default_ttl(mut self, ttl: Duration) -> Self {
self.config.default_ttl = Some(ttl);
self.default_ttl = Some(ttl);
self
}

Expand All @@ -96,6 +97,14 @@ impl Builder for CloudflareKvBuilder {
type Config = CloudflareKvConfig;

fn build(self) -> Result<impl Service> {
let default_ttl = match self.default_ttl {
Some(ttl) => Some(ttl),
None => self
.config
.default_ttl
.map(signed_duration_to_duration)
.transpose()?,
};
let api_token = match &self.config.api_token {
Some(api_token) => format_authorization_by_bearer(api_token)?,
None => {
Expand All @@ -121,7 +130,7 @@ impl Builder for CloudflareKvBuilder {
};

// Validate default TTL is at least 60 seconds if specified
if let Some(ttl) = self.config.default_ttl
if let Some(ttl) = default_ttl
&& ttl < Duration::from_secs(60)
{
return Err(Error::new(
Expand All @@ -143,7 +152,7 @@ impl Builder for CloudflareKvBuilder {
api_token,
account_id,
namespace_id,
expiration_ttl: self.config.default_ttl,
expiration_ttl: default_ttl,
info: ServiceInfo::new(CLOUDFLARE_KV_SCHEME, &root, ""),
capability: Capability {
create_dir: true,
Expand Down
15 changes: 13 additions & 2 deletions core/services/cloudflare-kv/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ pub struct CloudflareKvConfig {
/// The namespace ID. Used as URI path parameter.
pub namespace_id: Option<String>,
/// The default ttl for write operations.
pub default_ttl: Option<Duration>,
pub default_ttl: Option<SignedDuration>,

/// Root within this backend.
pub root: Option<String>,
Expand Down Expand Up @@ -94,7 +94,10 @@ impl opendal_core::Configurator for CloudflareKvConfig {
}

fn into_builder(self) -> Self::Builder {
CloudflareKvBuilder { config: self }
CloudflareKvBuilder {
config: self,
default_ttl: None,
}
}
}

Expand Down Expand Up @@ -125,4 +128,12 @@ mod tests {

assert!(CloudflareKvConfig::from_uri(&uri).is_err());
}

#[test]
fn from_iter_parses_default_ttl() {
let cfg = CloudflareKvConfig::from_iter([("default_ttl".to_string(), "PT1M".to_string())])
.unwrap();

assert_eq!(cfg.default_ttl, Some(SignedDuration::from_mins(1)));
}
}
13 changes: 11 additions & 2 deletions core/services/memcached/src/backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ use super::writer::MemcachedWriter;
#[derive(Debug, Default)]
pub struct MemcachedBuilder {
pub(super) config: MemcachedConfig,
pub(super) default_ttl: Option<Duration>,
}

impl MemcachedBuilder {
Expand Down Expand Up @@ -74,7 +75,7 @@ impl MemcachedBuilder {

/// Set the default ttl for memcached services.
pub fn default_ttl(mut self, ttl: Duration) -> Self {
self.config.default_ttl = Some(ttl);
self.default_ttl = Some(ttl);
self
}

Expand All @@ -97,6 +98,14 @@ impl Builder for MemcachedBuilder {
type Config = MemcachedConfig;

fn build(self) -> Result<impl Service> {
let default_ttl = match self.default_ttl {
Some(ttl) => Some(ttl),
None => self
.config
.default_ttl
.map(signed_duration_to_duration)
.transpose()?,
};
let endpoint_raw = self.config.endpoint.clone().ok_or_else(|| {
Error::new(ErrorKind::ConfigInvalid, "endpoint is empty")
.with_context("service", MEMCACHED_SCHEME)
Expand Down Expand Up @@ -171,7 +180,7 @@ impl Builder for MemcachedBuilder {
endpoint,
self.config.username,
self.config.password,
self.config.default_ttl,
default_ttl,
self.config.connection_pool_max_size,
))
.with_normalized_root(root))
Expand Down
15 changes: 13 additions & 2 deletions core/services/memcached/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ pub struct MemcachedConfig {
/// Memcached password, optional.
pub password: Option<String>,
/// The default ttl for put operations.
pub default_ttl: Option<Duration>,
pub default_ttl: Option<SignedDuration>,
/// The maximum number of connections allowed.
///
/// default is 10
Expand Down Expand Up @@ -81,7 +81,10 @@ impl Configurator for MemcachedConfig {
}

fn into_builder(self) -> Self::Builder {
MemcachedBuilder { config: self }
MemcachedBuilder {
config: self,
default_ttl: None,
}
}
}

Expand All @@ -101,4 +104,12 @@ mod tests {
assert_eq!(cfg.root.as_deref(), Some("app/session"));
Ok(())
}

#[test]
fn from_iter_parses_default_ttl() -> Result<()> {
let cfg = MemcachedConfig::from_iter([("default_ttl".to_string(), "1500ms".to_string())])?;

assert_eq!(cfg.default_ttl, Some(SignedDuration::from_millis(1500)));
Ok(())
}
}
15 changes: 12 additions & 3 deletions core/services/redis/src/backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ const DEFAULT_REDIS_PORT: u16 = 6379;
#[derive(Debug, Default)]
pub struct RedisBuilder {
pub(super) config: RedisConfig,
pub(super) default_ttl: Option<Duration>,
}

impl RedisBuilder {
Expand Down Expand Up @@ -108,7 +109,7 @@ impl RedisBuilder {
///
/// If set, we will specify `EX` for write operations.
pub fn default_ttl(mut self, ttl: Duration) -> Self {
self.config.default_ttl = Some(ttl);
self.default_ttl = Some(ttl);
self
}

Expand Down Expand Up @@ -144,6 +145,14 @@ impl Builder for RedisBuilder {
type Config = RedisConfig;

fn build(self) -> Result<impl Service> {
let default_ttl = match self.default_ttl {
Some(ttl) => Some(ttl),
None => self
.config
.default_ttl
.map(signed_duration_to_duration)
.transpose()?,
};
let root = normalize_root(
self.config
.root
Expand All @@ -170,7 +179,7 @@ impl Builder for RedisBuilder {
endpoints,
None,
Some(client),
self.config.default_ttl,
default_ttl,
self.config.connection_pool_max_size,
))
.with_normalized_root(root))
Expand All @@ -194,7 +203,7 @@ impl Builder for RedisBuilder {
endpoint,
Some(client),
None,
self.config.default_ttl,
default_ttl,
self.config.connection_pool_max_size,
))
.with_normalized_root(root))
Expand Down
15 changes: 13 additions & 2 deletions core/services/redis/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ pub struct RedisConfig {
/// default is db 0
pub db: i64,
/// The default ttl for put operations.
pub default_ttl: Option<Duration>,
pub default_ttl: Option<SignedDuration>,
}

impl Debug for RedisConfig {
Expand Down Expand Up @@ -120,7 +120,10 @@ impl Configurator for RedisConfig {
}

fn into_builder(self) -> Self::Builder {
RedisBuilder { config: self }
RedisBuilder {
config: self,
default_ttl: None,
}
}
}

Expand Down Expand Up @@ -156,6 +159,14 @@ mod tests {
Ok(())
}

#[test]
fn from_iter_parses_default_ttl() -> Result<()> {
let cfg = RedisConfig::from_iter([("default_ttl".to_string(), "5s".to_string())])?;

assert_eq!(cfg.default_ttl, Some(SignedDuration::from_secs(5)));
Ok(())
}

#[test]
fn test_redis_builder_interface() {
// Test that RedisBuilder still works with the new implementation
Expand Down
17 changes: 15 additions & 2 deletions dev/src/generate/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ pub enum ConfigType {
Bool,
/// Mapping to rust's `String`
String,
/// Mapping to rust's `Duration`
/// Mapping to rust's `Duration` and `SignedDuration`
Duration,

/// Mapping to rust's `usize`
Expand Down Expand Up @@ -113,7 +113,7 @@ impl FromStr for ConfigType {
Ok(match s {
"bool" => ConfigType::Bool,
"String" => ConfigType::String,
"Duration" => ConfigType::Duration,
"Duration" | "SignedDuration" => ConfigType::Duration,

"usize" => ConfigType::Usize,
"u64" => ConfigType::U64,
Expand Down Expand Up @@ -514,6 +514,19 @@ mod tests {
example: None,
},
),
(
"default_ttl: Option<SignedDuration>",
Config {
name: "default_ttl".to_string(),
value: ConfigType::Duration,
optional: true,
deprecated: None,
comments: "".to_string(),
group: None,
default_value: None,
example: None,
},
),
];

for (input, expected) in cases {
Expand Down
3 changes: 1 addition & 2 deletions dev/src/generate/python.rs
Original file line number Diff line number Diff line change
Expand Up @@ -264,8 +264,7 @@ fn is_path_like_field(name: &str) -> bool {
fn make_config_field_type(field: ViaDeserialize<Config>) -> Result<String, minijinja::Error> {
Ok(match field.value {
ConfigType::Bool => "bool".to_string(),
// Duration is typed `str` (a humantime string, e.g. "5s"). See #7887:
// core cannot yet deserialize Duration config fields from strings.
// Duration values cross the Python binding as strings, e.g. "5s".
ConfigType::Duration => "str".to_string(),
ConfigType::Usize
| ConfigType::U64
Expand Down
Loading