Skip to content

feat(thread_aware): add feature-gated ThreadAware impls for 3rd-party crate types#478

Open
martin-kolinek wants to merge 9 commits into
mainfrom
thread-aware-impls
Open

feat(thread_aware): add feature-gated ThreadAware impls for 3rd-party crate types#478
martin-kolinek wants to merge 9 commits into
mainfrom
thread-aware-impls

Conversation

@martin-kolinek
Copy link
Copy Markdown
Collaborator

@martin-kolinek martin-kolinek commented Jun 5, 2026

Adds opt-in ThreadAware implementations for commonly-used 3rd-party types behind per-crate, version-suffixed Cargo features. By default this crate brings no additional dependencies; users enable only the features they need.

Motivation

Discussed in the Oxidizer Contributors channel (thread by Evgenii Shutov), and inspired by ox-sdk!5243054 which adds ThreadAware for UuidId<T> / UuidString wrappers around uuid::Uuid. Many users wrap or re-export 3rd-party primitive/value types and need them to satisfy ThreadAware to flow through APIs that require it. Doing this in user crates requires the newtype dance; offering the impls here behind opt-in features removes that friction without forcing the dependencies on anyone.

Design

Each supported crate gets its own version-suffixed Cargo feature; enabling it pulls in that crate as an optional dependency and adds blanket ThreadAware impls for its plain value types via a small private macro. The version suffix lets a future major version of the wrapped crate land additively (e.g. bytes_v2) without forcing a breaking release of thread_aware. The default feature set is unchanged.

Feature Types
bytes_v1 Bytes, BytesMut
http_v1 StatusCode, Version, Method, HeaderName, HeaderValue, HeaderMap<T: ThreadAware>, Request<T: ThreadAware>, Response<T: ThreadAware>
jiff_v0_2 Timestamp, Span, SignedDuration, civil::Date, civil::Time, civil::DateTime, civil::ISOWeekDate
uuid_v1 Uuid

HeaderMap<T>, Request<T>, and Response<T> propagate relocate to every header value and (for Request/Response) the body, mirroring how this crate handles Vec<T> and Box<T>.

Intentional omissions

  • jiff::Zoned (carries a TimeZone reference; semantics warrant explicit user choice)
  • http::Uri (holds Bytes/Arc internals; can be added later if needed)
  • chrono and time (deferred per reviewer feedback; can be added later as chrono_v0_4 / time_v0_3 if needed)

Files

  • crates/thread_aware/src/third_party/{mod,bytes_v1,http_v1,jiff_v0_2,uuid_v1}.rs — one submodule per supported crate version, each gated by #[cfg(any(test, feature = "..."))]. mod.rs defines a private impl_noop_thread_aware! macro used by submodules with inert value types.
  • The wrapped crates (bytes, http, jiff, uuid) are unconditional [dev-dependencies] so the test suite exercises every impl with cargo test (no features required).
  • Each submodule has unit tests that statically assert ThreadAware/Send/Sync and a smoke test on the threaded runtime.

allowed_external_types

Because impls on foreign types make those types reachable through this crate's public surface, cargo_check_external_types (run with --all-features in CI) flags them. Explicit type paths are listed in Cargo.toml (rather than wildcards) for visibility.

Verification

  • cargo clippy --all-features --all-targets -- -D warnings
  • cargo test --all-features ✅ (126 unit + integration + 14 doctests)
  • cargo test --tests --no-default-features --workspace
  • just format, just readme, just spellcheck, cargo sort --check --grouped --workspace

Copilot AI review requested due to automatic review settings June 5, 2026 09:37
@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 5, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.0%. Comparing base (bf1ba72) to head (8ea958a).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #478   +/-   ##
=======================================
  Coverage   100.0%   100.0%           
=======================================
  Files         335      340    +5     
  Lines       25586    25683   +97     
=======================================
+ Hits        25586    25683   +97     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR extends the thread_aware crate with opt-in, feature-gated ThreadAware implementations for a set of commonly used third-party value types, while keeping the default build free of extra dependencies.

Changes:

  • Adds third_party submodules that provide no-op ThreadAware impls for types from bytes, chrono, http, jiff, time, and uuid, each behind a dedicated Cargo feature.
  • Updates crate documentation/README to describe the new opt-in feature set.
  • Adds optional dependencies + feature wiring in crates/thread_aware/Cargo.toml and updates allowed_external_types for CI’s external-type checking.

Reviewed changes

Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
crates/thread_aware/src/lib.rs Documents the new third-party impl feature set and wires in the third_party module.
crates/thread_aware/src/third_party/mod.rs Introduces a private helper macro and per-feature submodules for third-party impls.
crates/thread_aware/src/third_party/bytes.rs No-op ThreadAware impls + tests for bytes::Bytes / BytesMut.
crates/thread_aware/src/third_party/chrono.rs No-op ThreadAware impls + tests for selected chrono value/time types.
crates/thread_aware/src/third_party/http.rs No-op ThreadAware impls + tests for http inert value types (StatusCode, Version, Method).
crates/thread_aware/src/third_party/jiff.rs No-op ThreadAware impls + tests for selected jiff types, explicitly omitting Zoned.
crates/thread_aware/src/third_party/time.rs No-op ThreadAware impls + tests for selected time types, explicitly omitting Instant.
crates/thread_aware/src/third_party/uuid.rs No-op ThreadAware impl + tests for uuid::Uuid.
crates/thread_aware/README.md Regenerated README reflecting the new feature set.
crates/thread_aware/Cargo.toml Adds per-crate features, optional deps, and expands allowed_external_types.
Cargo.lock Updates lockfile to reflect the new optional dependencies in thread_aware.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread crates/thread_aware/src/lib.rs Outdated
@martin-kolinek
Copy link
Copy Markdown
Collaborator Author

🤖 Fixed in 404dcdb: made third_party and its feature-gated submodules pub so the doc link resolves to this crate's module and users can navigate to it.


AI response generated by Road to Gas Town

Comment thread crates/thread_aware/Cargo.toml Outdated
Comment thread crates/thread_aware/src/third_party/http.rs Outdated
Comment thread crates/thread_aware/Cargo.toml Outdated
Copy link
Copy Markdown
Member

@sandersaares sandersaares left a comment

Choose a reason for hiding this comment

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

time::Instant (requires std feature on time)

I am not sure why this is an omission. Do we really intend to support no-std? Feels unnecessary. If someone is using our SDK, we can assume they are using the standard library.

Comment thread crates/thread_aware/src/third_party/time.rs Outdated
Comment thread crates/thread_aware/README.md Outdated
Comment thread crates/thread_aware/README.md Outdated
Martin Kolinek (from Dev Box) and others added 3 commits June 5, 2026 20:02
… crate types

Adds opt-in ThreadAware implementations for commonly-used 3rd-party types
behind per-crate Cargo features. By default this crate brings no
additional dependencies; users enable only the features they need.

New features and the types they cover:
- `bytes`: `Bytes`, `BytesMut`
- `chrono`: `NaiveDate`, `NaiveTime`, `NaiveDateTime`, `TimeDelta`,
  `Utc`, `FixedOffset`, `Weekday`, `Month`, `DateTime<Utc>`,
  `DateTime<FixedOffset>`
- `http`: `StatusCode`, `Version`, `Method`
- `jiff`: `Timestamp`, `Span`, `SignedDuration`, `civil::Date`,
  `civil::Time`, `civil::DateTime`, `civil::ISOWeekDate`
- `time`: `Date`, `Time`, `PrimitiveDateTime`, `OffsetDateTime`,
  `UtcOffset`, `Duration`, `Weekday`, `Month`
- `uuid`: `Uuid`

Motivated by a Contributors-channel discussion (Evgenii Shutov) and the
ox-sdk!5243054 PR adding ThreadAware impls for UuidId/UuidString wrappers.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Reviewer noted that the crate-level docs link to `third_party` but the module was private, causing the link to resolve as an external crate. Make `third_party` and its feature-gated submodules `pub` so users can navigate to a module page that documents which types each feature covers.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… http, drop chrono+time

Addresses reviewer feedback on PR #478:

* Rename features to embed the wrapped crate's major (or 0.x minor): `bytes_v1`, `http_v1`, `jiff_v0_2`, `uuid_v1`. This lets us support a future `bytes 2.0` / `jiff 0.3` additively (as e.g. `bytes_v2`) without a breaking release of `thread_aware` (martintmk, Vaiz, sandersaares).

* Drop the `chrono` and `time` features; both are treated as legacy as the ecosystem aligns on `jiff` (martintmk).

* Expand `http_v1` to cover `HeaderName`, `HeaderValue`, `HeaderMap<T>`, `Request<T>`, `Response<T>`. Containers propagate `relocate` to inner `T` like `Vec<T>`/`Box<T>` (martintmk).

* Switch the per-feature gates on submodules and the macro to `any(test, feature = ...)` and move `bytes`/`http`/`jiff`/`uuid` to unconditional `dev-dependencies` so `cargo test` exercises all third-party impls without needing to enable any feature flags (sandersaares).

* Reword the `# Features` doc block to avoid the misleading 'optional dependency' phrasing when a feature is enabled (sandersaares).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings June 5, 2026 18:12
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Without the cfg gate, building with --no-default-features (e.g. coverage CI) defines the macro unconditionally while all its callers are gated to feature/test, causing unused_macros to fire under -D warnings.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings June 5, 2026 18:46
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.

Comment thread crates/thread_aware/src/third_party/http_v1.rs
Comment thread crates/thread_aware/src/third_party/http_v1.rs
Comment thread crates/thread_aware/src/lib.rs
Martin Kolinek (from Dev Box) and others added 2 commits June 5, 2026 20:52
Address review comments on PR #478: Request/Response now relocate every header value in addition to the body, consistent with the documented HeaderMap propagation behavior.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…rMap

Adds tests that populate header maps before calling relocate, covering the new header-propagation loops added in fa83ffd and restoring full patch coverage on codecov.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings June 5, 2026 19:00
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.

Comment thread crates/thread_aware/src/third_party/mod.rs Outdated
Comment thread crates/thread_aware/src/third_party/http_v1.rs Outdated
Comment thread crates/thread_aware/src/third_party/http_v1.rs Outdated
Martin Kolinek (from Dev Box) and others added 2 commits June 5, 2026 21:28
Add a Counter helper type whose relocate increments an observable counter, and use it in HeaderMap<Counter>, Request<Counter>, Response<Counter> tests so mutation testing can detect when the parent impls' relocate bodies are replaced with ().

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Clarify that no-feature default avoids pulling in *wrapped* crates, not all dependencies (derive is still a default dep).

- Document that http::Extensions are not (and cannot be) relocated by Request/Response impls.

- Rename request_relocate_propagates_to_body -> ..._to_body_and_headers since it now covers both.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings June 5, 2026 19:29
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated no new comments.

Counter and its ThreadAware impl are only used by relocate tests gated on feature = threads. Without the gate, --no-default-features builds (coverage CI) fail with dead-code warnings under -D warnings.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 5, 2026

⚠️ Breaking Changes Detected

error: running 'cargo update' on crate 'fetch_tls' failed with output:
-----
    Updating crates.io index
error: failed to get `foreign-types-shared` as a dependency of package `foreign-types v0.3.2`
    ... which satisfies dependency `foreign-types = "^0.3.1"` of package `openssl v0.10.80`
    ... which satisfies dependency `openssl = "^0.10.75"` of package `native-tls v0.2.18`
    ... which satisfies dependency `native-tls = "^0.2.18"` of package `fetch_tls v0.2.2 (/home/runner/work/oxidizer/oxidizer/crates/fetch_tls)`
    ... which satisfies path dependency `fetch_tls` of package `placeholder v0.0.0 (/home/runner/work/oxidizer/oxidizer/target/semver-checks/local-fetch_tls-0_2_2-x86_64_unknown_linux_gnu-ded2144553377442)`

Caused by:
  failed to load source for dependency `foreign-types-shared`

Caused by:
  unable to update registry `crates-io`

Caused by:
  download of fo/re/foreign-types-shared failed

Caused by:
  curl failed

Caused by:
  [16] Error in the HTTP2 framing layer

-----
error: failed to update dependencies for crate fetch_tls v0.2.2
note: this is unlikely to be a bug in cargo-semver-checks,
      and is probably an issue with the crate's Cargo.toml
note: the following command can be used to reproduce the compilation error:
      cargo new --lib example &&
          cd example &&
          echo '[workspace]' >> Cargo.toml &&
          cargo add --path /home/runner/work/oxidizer/oxidizer/crates/fetch_tls --features default,native-tls,rustls &&
          cargo update

error: aborting due to failure to run 'cargo update' for crate fetch_tls v0.2.2

Stack backtrace:
   0: anyhow::error::<impl anyhow::Error>::msg
   1: anyhow::__private::format_err
   2: cargo_semver_checks::data_generation::generate::generate_rustdoc
   3: cargo_semver_checks::data_generation::request::CrateDataRequest::resolve
   4: cargo_semver_checks::rustdoc_gen::StatefulRustdocGenerator<cargo_semver_checks::rustdoc_gen::ReadyState>::load_rustdoc
   5: cargo_semver_checks::Check::check_release::{{closure}}
   6: cargo_semver_checks::Check::check_release
   7: cargo_semver_checks::exit_on_error
   8: cargo_semver_checks::main
   9: std::sys::backtrace::__rust_begin_short_backtrace
  10: main
  11: <unknown>
  12: __libc_start_main
  13: _start

If the breaking changes are intentional then everything is fine - this message is merely informative.

Remember to apply a version number bump with the correct severity when publishing a version with breaking changes (1.x.x -> 2.x.x or 0.1.x -> 0.2.x).

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants