fix(core): deserialize duration config values from strings - #7983
fix(core): deserialize duration config values from strings#7983subotac wants to merge 4 commits into
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. |
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.