Skip to content

refactor: extract the channel context, share Accept parsing, and fold duplicated blocks - #6

Open
ctxswitch wants to merge 1 commit into
cleanup/phase-4a-metrics-errorsfrom
cleanup/phase-4b-extractor-negotiator
Open

refactor: extract the channel context, share Accept parsing, and fold duplicated blocks#6
ctxswitch wants to merge 1 commit into
cleanup/phase-4a-metrics-errorsfrom
cleanup/phase-4b-extractor-negotiator

Conversation

@ctxswitch

Copy link
Copy Markdown
Owner

Summary

Second half of phase four. Based on cleanup/phase-4a-metrics-errors (#5). Net
−118 lines across 9 files.

ChannelContext is now a FromRequestParts extractor. The four-line resolve
prologue leaves 18 handlers (11 in mod.rs, 7 in packages.rs) and the
channel: Option<String> field leaves all seven per-route path structs. Six handlers
stop taking HeaderMap entirely and cargo_config now takes only ChannelContext.

The two Accept-header negotiators now share their parsing. media_range, offer,
acceptable, and outranks are extracted, leaving each negotiator its media-type
table plus one call. The two tie-breaks turned out to be byte-identical once the
variables are renamed, so outranks(preferred, other) covers both — Python calls
outranks(json, html), npm calls outranks(abbreviated, full). The per-protocol
default for a wildcard range covering both representations now lives visibly in the
argument order rather than in two separately-maintained comparisons, which is what let
them drift apart in the first place.

Plus a dozen smaller folds: the OutOfSpace arm is absorbed into
cache_write_failure so all three PUT paths share it; unix_time is replaced by the
injected Clock; fresh_outcome and revalidated come out of fetch_url (~40 lines
shorter); the fetch_npmfetch_encodedfetch_encoded_url
fetch_encoded_url_with_suffix chain collapses and ProxyService::url goes private;
publish_temporary, upstream_response, content_type, and content_length are
shared; proxy::header_length is deleted as a character-identical twin; the three
tokio::select! blocks collapse into one biased block; the NotFound/Deleting
response arms merge; and SpaceLedger::new calls refresh() instead of repeating its
match.

Verification

make ci clean, including clippy --all-targets -- -D warnings; 138 tests.

  • tests/integration/channels.rs (961 lines, both route forms, protected and open
    access, every data route) passes unchanged — it is the real check on the extractor
    refactor, the rest of which the compiler validates.

  • wildcard_accept_ranges_never_outrank_a_named_representation passes unchanged. It
    is table-driven over both npm and python and pins the differing application/*
    defaults that had to survive the merge.

  • Channel expiry and lifecycle tests pass after the Clock injection.

  • Behaviour was verified against the vendored axum 0.8.9: Path reads UrlParams via
    extensions.get() rather than remove (so a handler can take both
    ChannelContext and its own Path<T>), the path deserializer maps structs through
    MapDeserializer with deserialize_ignored_any for unknown keys (so ArtifactPath
    ignores the channel capture), and insert_url_params always inserts even for
    zero-capture routes.

  • make ci

  • Documentation and examples are accurate

  • New or changed behavior has test coverage

Operational impact

One log message changed. The two copies of the cache-write failure path had drifted
and logged different text — "artifact publication failed" and "build-cache write failed". The shared helper logs "cache write failed", so both former messages are
gone.

One error-body change on a pathological input: if a path segment percent-decodes to
invalid UTF-8, the 400 now carries the invalid_channel JSON body from
ChannelContext rather than axum's plain-text Path rejection. Still a 400; no test
covers it either way.

No metric, route, storage format, configuration, or security boundary changes.

Review notes

Start at put_reference (src/transport/http/mod.rs:554) — it is the one handler
that could not take a plain ChannelContext.
Extractors run before the body, so
rejecting there turned malformed_json_still_precedes_protected_channel_authorization
(tests/integration/channels.rs:694) from a 400 into a 401. It takes
Result<ChannelContext, Response> and resolves the rejection after the body parses,
which preserves the ordering exactly. That existing test is what caught it.

Two plan items were declined, both reported rather than forced:

  • No checked() was extracted in channel/service.rs. The plan asked for one, but
    the existing authorize already is that function verbatim — adding checked()
    would have been a third name for the same three lines. authorize_with_lease now
    calls authorize. The pre-gate check is kept, with the comment the plan asked for
    explaining that it is what stops ChannelGates::gate() inserting a permanent
    DashMap entry per unknown channel id.
  • rewrite_metadata's Transform::None arm stays. It is genuinely dead, but
    removing it cleanly means splitting Transform into None plus a three-variant
    MetadataTransform, touching both constructors, upstream_accept, cache_variant,
    rewrite_metadata, cached_outcome's matches!, and the fetch_url dispatch — a
    new type and six edit sites to delete one zero-cost line. It was not replaced with
    unreachable!() either, since that is the same dead code by another name.

Noted for a later phase, not addressed here: put_reference's Reference::parse and
ArtifactId::parse failures return JSON api_error bodies while put_artifact's
equivalents return bare status codes — sibling routes with inconsistent error-response
policy.

… duplicated blocks

ChannelContext is now a FromRequestParts extractor, so the four-line resolve
prologue leaves 18 handlers and the channel field leaves all seven per-route path
structs. put_reference takes its context as a Result and resolves the rejection
in the body: extractors run before the body, so rejecting early would answer a
syntactically broken request with a channel error instead of a 400.

negotiate_python and negotiate_npm shared roughly 25 byte-identical lines, and
the two tie-breaks turn out to be the same function once the variables are
renamed. Both now share media_range, offer, acceptable, and outranks, leaving
each negotiator its media-type table and one call. The per-protocol default for a
wildcard covering both representations -- Python serves JSON, npm abbreviated --
now lives visibly in the argument order.

Also folds the OutOfSpace arm into cache_write_failure so all three PUT paths
share it, replaces unix_time with the injected Clock, extracts fresh_outcome and
revalidated from fetch_url, collapses the fetch_npm chain and the three select!
blocks, shares publish_temporary, upstream_response, content_type, and
content_length, merges the NotFound and Deleting response arms, and has
SpaceLedger::new call refresh instead of repeating its match.
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