Skip to content

dev: replace unmaintained derivative crate with manual impls - #587

Open
sshaplygin wants to merge 2 commits into
ydb-platform:masterfrom
sshaplygin:dev/replace-derivative
Open

dev: replace unmaintained derivative crate with manual impls#587
sshaplygin wants to merge 2 commits into
ydb-platform:masterfrom
sshaplygin:dev/replace-derivative

Conversation

@sshaplygin

Copy link
Copy Markdown
Contributor

Problem

derivative is flagged unmaintained by RUSTSEC-2024-0388 (upstream is no longer maintained; the advisory suggests derive_more, derive-where or educe). Every cargo audit run against a project depending on ydb reports it.

Change

It was used in exactly three places, all for the same thing: derive Debug while skipping fields that are not Debug themselves. Each becomes a hand-written impl listing the remaining fields, which produces identical output, and the dependency is dropped.

Type Skipped fields
TableSession on_drop_callbacks, channel_pool
RacyRoundRobinState both ConnectionTask queues
GrpcConnectionManagerGeneric interceptor

GrpcConnectionManagerGeneric also used derivative(Clone(bound = "BalancerT: Clone")), so its Clone becomes a manual impl too, preserving the balancer-only bound — ConnectionT sits behind an Arc and does not need Clone. Its Debug keeps the ConnectionT: Debug bound that derivative generated implicitly.

No behaviour change: derivative's Debug = "ignore" omits the field entirely, which is what debug_struct(..).field(..).finish() emits.

Verification

cargo tree -p ydb -e normal -i derivative   # no longer in the tree
cargo fmt --check && cargo clippy --workspace --all-targets --no-deps --exclude=ydb-grpc -- -D warnings
cargo test --workspace     # 214 passed, 88 ignored

The Cargo.lock diff is exactly the removal of derivative 2.2.0 — no incidental version bumps.

🤖 Generated with Claude Code

`derivative` is flagged unmaintained by RUSTSEC-2024-0388, so every
`cargo audit` run against a project depending on `ydb` reports it.

It was used in three places, all for the same thing: derive `Debug`
while skipping fields that are not `Debug` themselves. Replace each with
a hand-written `Debug` impl listing the remaining fields, which produces
the same output, and drop the dependency.

- `TableSession`: skips `on_drop_callbacks` and `channel_pool`.
- `RacyRoundRobinState`: skips the two `ConnectionTask` queues.
- `GrpcConnectionManagerGeneric`: skips `interceptor`; its `Clone` also
  becomes a manual impl to keep the `BalancerT: Clone`-only bound that
  `derivative(Clone(bound = ...))` provided.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@codecov

codecov Bot commented Aug 3, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.68254% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.89%. Comparing base (a6d7911) to head (4d48673).

Files with missing lines Patch % Lines
ydb/src/grpc_connection_manager.rs 92.06% 5 Missing ⚠️
ydb/src/session.rs 84.37% 5 Missing ⚠️
ydb/src/connection_pool.rs 90.32% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #587      +/-   ##
==========================================
- Coverage   86.91%   86.89%   -0.02%     
==========================================
  Files         198      198              
  Lines       19492    19615     +123     
==========================================
+ Hits        16941    17044     +103     
- Misses       2551     2571      +20     
Flag Coverage Δ
rust-1.88.0 86.89% <89.68%> (-0.02%) ⬇️
rust-1.96.1 87.23% <100.00%> (+0.08%) ⬆️
tests 86.89% <89.68%> (-0.02%) ⬇️
ubuntu 86.89% <89.68%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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.

Codecov requires 80% patch coverage; this PR started at 25% because
nothing in the test suite ever formatted the three types whose derives
were replaced by hand-written impls.

That gap matters more than the number. `derivative(Debug = "ignore")`
enforced the skipped fields structurally, whereas a hand-written
`debug_struct` chain can silently start or stop reporting a field. Each
test now pins both halves of the contract: the fields that must appear,
and the non-`Debug` fields that must not.

- `TableSession`: reports id, can_pooled and timeouts; never
  `channel_pool` or `on_drop_callbacks`. The session construction that
  `discard_from_pool_clears_can_pooled` already used moves into a
  `test_session` helper shared by both tests.
- `RacyRoundRobinState`: reports addrs and first_connection; never the
  connection queues. Uses a lazily connected channel, so no server is
  contacted, but it needs a Tokio reactor and is therefore async.
- `GrpcConnectionManagerGeneric`: reports balancer, connections_pool and
  database; never the interceptor. Two further tests pin the
  `BalancerT: Clone`-only bound that `derivative(Clone(bound = ...))`
  provided, and that clones keep sharing one pool.

Patch coverage 25% -> 100% (`cargo llvm-cov --lib`). Module coverage:
grpc_connection_manager.rs 39.3% -> 71.6% lines, session.rs 67.1% ->
74.0%, connection_pool.rs 29.9% -> 39.8%.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sshaplygin

Copy link
Copy Markdown
Contributor Author

Pushed 4d48673 to raise patch coverage: 25% → 100%.

The diff started at 25% because nothing in the suite ever formatted the three types whose derives this PR replaces. That is worth closing for its own sake, not just for the number — derivative(Debug = "ignore") enforced the skipped fields structurally, while a hand-written debug_struct chain can silently start or stop reporting a field. Each test pins both halves of the contract.

Type Must report Must never report
TableSession id, can_pooled, timeouts channel_pool, on_drop_callbacks
RacyRoundRobinState addrs, first_connection the two connection queues
GrpcConnectionManagerGeneric balancer, connections_pool, database interceptor

Two extra tests pin the Clone side: the BalancerT: Clone-only bound that derivative(Clone(bound = ...)) gave us, and that clones keep sharing one Arc pool — checked for both NoBalancer and the SharedLoadBalancer used in production.

Nothing contacts a server: RacyRoundRobinState uses a lazily connected channel (async only because connect_lazy needs a reactor), and the session reuses the construction discard_from_pool_clears_can_pooled already relied on — now extracted into a shared test_session helper.

I checked the tests are not vacuous: dropping .field("timeouts", ..) from the TableSession impl fails the test with timeouts missing: TableSession { id: "session-42", can_pooled: true }.

Coverage

Patch coverage 100% (28/28 changed lines) under cargo llvm-cov --lib. Module coverage: grpc_connection_manager.rs 39.3% → 71.6% lines, session.rs 67.1% → 74.0%, connection_pool.rs 29.9% → 39.8%.

cargo fmt --check, clippy -D warnings and cargo test --workspace all pass (219 passed, 88 ignored).

🤖 Generated with Claude Code

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.

1 participant