fix(core): deserialize duration config values from strings - #7983
Conversation
0970b04 to
cc2163b
Compare
tisonkun
left a comment
There was a problem hiding this comment.
jiff's SignedDuration should accept "friendly format" already and the comment may not applied to other bindings.
impl core::str::FromStr for SignedDuration {
type Err = Error;
#[inline]
fn from_str(string: &str) -> Result<SignedDuration, Error> {
parse_iso_or_friendly(string.as_bytes())
}
}|
@subotac could you elaborate why jiff's friendly parser not enough? |
The jiff friendly parser is sufficient for parsing the value, and this patch already uses it through The missing part is the serde bridge:
The internal enum could use |
|
Thanks! I wonder if we can use |
Yes, that direction can work. I checked the three service configs and there are two compatibility details to account for:
If that tradeoff is acceptable i can rework the three config fields to use |
|
@subotac your analyze makes sense. Please go ahead! |
|
@tisonkun Thanks for the guidance. The agreed This should address the requested changes. Could you please take another look when you have a chance? If there are additional improvements beyond the scope of this issue, I would be happy to handle them separately. |
Signed-off-by: tison <wander4096@gmail.com>
| /// 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> { |
There was a problem hiding this comment.
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.
Sorry I just see this comment. |
|
You can sent a revert commit and resubmit it later. I'm sorry. |
|
@tisonkun It's fine. I saw you merged the PR. I will track this in the upcoming release issue. |
|
Thanks! |
Which issue does this PR close?
Closes #7887.
Rationale for this change
ConfigDeserializersupplies configuration-map values as strings, but serde'sDurationimplementation expects a structured value. As a result, thedefault_ttlfields for Redis, Memcached, and Cloudflare KV could not be setthrough
Operator::via_iter, URI options, or other string-backed configuration.What changes are included in this PR?
strings through the existing
signed_to_duration()parser.Option<Duration>fields and builder APIs unchanged.string formats.
structured input, and rejection of a unitless duration.
Validation:
cargo test -p opendal-core -p opendal-service-redis -p opendal-service-memcached -p opendal-service-cloudflare-kvcargo clippy -p opendal-core -p opendal-service-redis -p opendal-service-memcached -p opendal-service-cloudflare-kv --all-targets --all-features -- -D warningscargo +1.91.0 clippy -p opendal-core -p opendal-service-redis -p opendal-service-memcached -p opendal-service-cloudflare-kv --all-targets --all-features -- -D warningscargo doc -p opendal-core -p opendal-service-redis -p opendal-service-memcached -p opendal-service-cloudflare-kv --lib --no-deps --all-featurespython scripts/workspace.py cargo fmt -- --checkAre there any user-facing changes?
Yes. String-backed configuration can now set the three
default_ttlfieldswith explicit-unit friendly values such as
5sand1500ms, or ISO-8601 valuessuch as
PT1M. Unitless values remain invalid.AI Usage Statement
AI assisted with analysis and testing.