Skip to content

fix(core): deserialize duration config values from strings - #7983

Open
subotac wants to merge 4 commits into
apache:mainfrom
subotac:fix/string-default-ttl
Open

fix(core): deserialize duration config values from strings#7983
subotac wants to merge 4 commits into
apache:mainfrom
subotac:fix/string-default-ttl

Conversation

@subotac

@subotac subotac commented Jul 29, 2026

Copy link
Copy Markdown

Which issue does this PR close?

Closes #7887.

Rationale for this change

ConfigDeserializer supplies configuration-map values as strings, but serde's
Duration implementation expects a structured value. As a result, the
default_ttl fields for Redis, Memcached, and Cloudflare KV could not be set
through Operator::via_iter, URI options, or other string-backed configuration.

What changes are included in this PR?

  • Add a shared optional-duration deserializer that accepts friendly and ISO-8601
    strings through the existing signed_to_duration() parser.
  • Preserve support for the structured serde representation and keep the public
    Option<Duration> fields and builder APIs unchanged.
  • Apply the helper to all three affected services and document their accepted
    string formats.
  • Add regression coverage for friendly, fractional, and ISO-8601 durations,
    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-kv
  • cargo clippy -p opendal-core -p opendal-service-redis -p opendal-service-memcached -p opendal-service-cloudflare-kv --all-targets --all-features -- -D warnings
  • cargo +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 warnings
  • cargo doc -p opendal-core -p opendal-service-redis -p opendal-service-memcached -p opendal-service-cloudflare-kv --lib --no-deps --all-features
  • python scripts/workspace.py cargo fmt -- --check

Are there any user-facing changes?

Yes. String-backed configuration can now set the three default_ttl fields
with explicit-unit friendly values such as 5s and 1500ms, or ISO-8601 values
such as PT1M. Unitless values remain invalid.

AI Usage Statement

AI assisted with analysis and testing.

@subotac
subotac requested a review from Xuanwo as a code owner July 29, 2026 11:13
@dosubot dosubot Bot added size:M This PR changes 30-99 lines, ignoring generated files. releases-note/fix The PR fixes a bug or has a title that begins with "fix" labels Jul 29, 2026
Comment thread core/core/src/raw/time.rs Outdated
@subotac
subotac force-pushed the fix/string-default-ttl branch from 0970b04 to cc2163b Compare July 29, 2026 19:33
@subotac
subotac requested a review from erickguan July 29, 2026 21:13

@tisonkun tisonkun left a comment

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.

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
subotac requested a review from tisonkun July 30, 2026 11:05
@tisonkun

Copy link
Copy Markdown
Member

@subotac could you elaborate why jiff's friendly parser not enough?

@subotac

subotac commented Jul 30, 2026

Copy link
Copy Markdown
Author

@subotacЧи не могли б ви пояснити, чому дружнього парсера Jiff недостатньо?

The jiff friendly parser is sufficient for parsing the value, and this patch already uses it through signed_to_duration().

The missing part is the serde bridge: ConfigDeserializer provides "5s" as a string, while std::time::Duration's Deserialize implementation expects the structured { secs, nanos }
representation and does not invoke jiff or FromStr.

deserialize_option_duration only routes string-backed configuration through the existing jiff parser and converts the resulting SignedDuration to std::time::Duration. It alsso preserves
compatibility with structured serde input

The internal enum could use SignedDuration directly instead of String and rely on jiff's serde implementation, but an adapter and conversion to std::time::Duration would still be required

@tisonkun

Copy link
Copy Markdown
Member

Thanks! I wonder if we can use jiff::SignedDuration in those service places and convert jiff::SignedDuration to std Duration when it is necessary (perhaps we would find jiff::SignedDuration provides even better interface)

@subotac

subotac commented Jul 30, 2026

Copy link
Copy Markdown
Author

Thanks! I wonder if we can use jiff::SignedDuration in those service places and convert jiff::SignedDuration to std Duration when it is necessary (perhaps we would find jiff::SignedDuration provides even better interface)

Yes, that direction can work. I checked the three service configs and there are two compatibility details to account for:

  • the config structs are public, and the binding generator currently classifies duration fields by the Rust type name Duration, so changing them to SignedDuration also requires updating the generator mapping andt
    changes the public Rust config field type;
  • I would prefer std::time::Duration::try_from(value) at build time instead of unsigned_abs(), so negative TTL values are rejected rather than silently converted to positive values.

If that tradeoff is acceptable i can rework the three config fields to use SignedDuration, keep the builder methods accepting std::time::Duration, update the binding generator classification, and convert once in
each service builder

@tisonkun

Copy link
Copy Markdown
Member

@subotac your analyze makes sense. Please go ahead!

@dosubot dosubot Bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Jul 31, 2026
@subotac

subotac commented Jul 31, 2026

Copy link
Copy Markdown
Author

@tisonkun Thanks for the guidance. The agreed SignedDuration-based implementation is now complete in fed4f0e5: the affected service configs store SignedDuration, the builder API remains on std::time::Duration, negative TTL values are rejected, and the generator handles both duration types. The affected tests, parser tests, formatting, generator checks, and core/dev Clippy checks all pass.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

releases-note/fix The PR fixes a bug or has a title that begins with "fix" size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Duration config fields cannot be set from the string config map

3 participants