fix(rpc): parse Accept header as a list of media ranges

[ygd58]

Problem

ApiVersion::from_accept_header() matched the entire trimmed header value as a single media type. Any real-world Accept header with more than one media range (e.g. text/html, application/vnd.arc.v1+json) or any parameters (e.g. application/vnd.arc.v1+json; q=0.9) failed to match anything and fell through to a 406, even though a supported version was present in the header.

Fix

Per RFC 9110 SS12.5.1, Accept is a comma-separated list of media ranges, each optionally carrying ;-separated parameters including q. Rewrote the parser to split on ,, strip parameters per range, and select the supported version with the highest q among ranges not explicitly marked unacceptable (q=0). A malformed q value falls back to fully acceptable (q=1) rather than rejecting the range -- documented in the docstring and covered by a dedicated test.

No new dependency -- self-contained parser matching the codebase's existing minimal-dependency style for this module.

Testing

Added unit tests in version.rs covering: multiple ranges, quality parameters, q=0 exclusion, whitespace variance, malformed q values, and quality-based tie-breaking among supported ranges.

Also added status_for_accept()-based tests in middleware.rs that build a minimal router with just the extract_version layer attached and assert the actual HTTP status (200/406) for representative headers -- so the fix is verified through the real middleware path, not just the parser in isolation.

$ cargo test -p arc-node-consensus accept_header
... 20 passed, 0 failed
$ cargo test -p arc-node-consensus rpc::
... 86 passed, 0 failed

Fixes #341

[osr21]

Disclosure: I'm not affiliated with Circle — an external community contributor, not a maintainer, with no write access. This review is advisory only and carries no merge authority; please defer to Circle maintainers.

---

I can't compile Rust in my environment, so I verified this by reading main, running a faithful port of the new from_accept_header against RFC vectors, and checking every spec claim against RFC 9110 and the f32::from_str docs. CI is authoritative over anything below.

The bug is real. On main the function trims the whole field value and compares it as one media type, so strip_suffix("+json") fails the moment a ;q= parameter is appended, and strip_prefix(MEDIA_TYPE_PREFIX) fails the moment another range precedes it. Both reported headers reach None, and extract_version maps None straight to 406. The diagnosis in #341 is accurate.

Your test count checks out exactly, which is a good sign the output is real: cargo test -p arc-node-consensus accept_header filters by substring, and the crate is indeed named arc-node-consensus. That filter matches 6 pre-existing test_from_accept_header_* tests + 10 new ones in version.rs + 4 new test_accept_header_* in middleware.rs = 20.

This coverage is genuinely CI-enforced. malachite-app is a workspace member and the test job runs cargo nextest run --locked --workspace. The tower::ServiceExt + oneshot test style also already exists in this crate at rpc/routes.rs:298, so the middleware tests introduce no new dev-dependency or feature risk. Driving the assertions through the real extract_version layer rather than the parser alone was the right call.

Findings below, from the port. Everything marked ❌ diverges from RFC 9110 or from this PR's own docstring.

1. q=nan and q=inf slip past the malformed-q fallback ❌

This is the one I'd fix before merge. The docstring promises "a malformed q value is treated as q=1", and test_..._malformed_quality_value_is_treated_as_acceptable appears to lock that in — but it only passes because "not-a-number" fails to parse. Per the f32::from_str grammar:

Float ::= Sign? ( 'inf' | 'infinity' | 'nan' | Number )
"Note that alphabetical characters are not case-sensitive."

So q=nan parses successfully and never reaches unwrap_or(1.0). It yields NaN, and both guards then behave counter-intuitively:

• if q <= 0.0 → false for NaN, so the range is not skipped;
• if q > best_q → also false for NaN, so it can never be selected.

Result: Accept: application/vnd.arc.v1+json; q=nan returns 406, the exact opposite of the documented contract. And q=inf parses to +∞, which outranks every legitimate range — RFC 9110 §12.4.2 caps a qvalue at 1 with at most three decimals, so no conforming sender can produce that. Filtering to finite values and clamping keeps the documented behaviour honest:

q = q_str
    .trim()
    .parse::<f32>()
    .ok()
    .filter(|v| v.is_finite())
    .map(|v| v.clamp(0.0, 1.0))
    .unwrap_or(1.0);

2. Case sensitivity — including one narrow regression vs main ❌

RFC 9110 is explicit in three places:

• §8.3.1: "The type and subtype tokens are case-insensitive."
• §5.6.6: "Parameter names are case-insensitive."
• §12.4.2: "a common parameter, named q (case-insensitive)"

All comparisons here are byte-exact, so Application/JSON → 406 (pre-existing on main). The new part is the q parameter name:

Accept: application/vnd.arc.v1+json; Q=0   ->  main: 406      this PR: 200

Q=0 isn't recognised as a weight, so q stays at the 1.0 default and the range is served — a representation the client explicitly marked not acceptable. main returned 406 here by accident (the value didn't end in +json), so this is a small behavioural regression introduced by the fix. An to_ascii_lowercase() on the media type and on the parameter name closes both this and the Application/JSON case.

3. The tie-breaking test can't fail for the behaviour it pins

test_from_accept_header_prefers_highest_quality_supported_range uses:

"application/vnd.arc.v1+json; q=0.3, application/json; q=0.9"  ->  Some(V1)

Both ranges map to V1, so first-wins, last-wins and highest-q all return Some(V1). The comment says it pins highest-q "for when a second version is introduced", but the assertion holds under every ordering rule — it would keep passing if the selection logic were inverted. Extracting a small helper that returns the winning (ApiVersion, f32) (or the parsed ranges) would make the ordering observable while only one version exists.

4. RFC precedence is by specificity, not header order

The docstring pins "ties keep the first-encountered range". RFC 9110 §12.5.1 says otherwise:

Media ranges can be overridden by more specific media ranges or specific media types. If more than one media range applies to a given type, the most specific reference has precedence.

Moot today — every match resolves to V1. It stops being moot the moment V2 exists, which is exactly the scenario the docstring is written for: Accept: */*, application/vnd.arc.v2+json at equal q should prefer the specific range. Worth either implementing specificity or marking the deviation explicitly.

5. type/* ranges still 406

application/* is a valid media range that covers the vendor type, but only the literal */* is recognised, so it returns 406. Pre-existing, though "parse the header as media ranges" is this PR's premise, and it's one more arm next to the */* branch.

6. Quoted parameter values containing commas

#341 specifically flagged "quoted parameter and quality-value edge cases" as the reason to consider an established parser. Splitting on , before accounting for quoted-string (§5.6.6 allows parameter-value = token / quoted-string) reproduces the same inversion as #2:

Accept: application/vnd.arc.v1+json; foo="x,y"; q=0   ->  200 (should be 406)

The range splits in two; the first half keeps the media type and loses the q=0. Rare in practice, and I don't think it justifies a dependency — but since the issue raised it, the PR body's "no new dependency" rationale would be stronger for acknowledging the tradeoff.

---

Summary

Net clear improvement — the common cases in #341 are genuinely fixed, the middleware-level tests exercise the real path, and the docstring is unusually careful about stating its own contract. Not approving only because #1 and #2 are each a couple of lines and #2 changes a 406 into a 200 for a client that said q=0. Items 3–6 are non-blocking.

[ygd58]

Thanks for the thorough port-and-verify review -- both blocking items addressed in 32fa054:

1. q=nan/q=inf: filter to is_finite() before the clamp, matching your suggested fix exactly. Added tests for nan (both cases), inf, and -infinity -- the last one turned up an error in my own first draft of the test (I initially expected -inf to clamp to 0.0, but is_finite() filters it out before the clamp ever runs, so it falls back to q=1 like nan/+inf/unparsable -- fixed the test to match the actual, correct behavior instead of the code).
2. Case sensitivity: to_ascii_lowercase() on the media type, eq_ignore_ascii_case on the q= parameter name. Added tests for Application/JSON, an uppercase versioned media type, and the Q=0 regression case specifically.

Also took your suggestion on #3 -- extracted best_match_with_quality() (test-only, returns the winning (ApiVersion, f32)) so the tie-breaking test asserts on the actual winning q (0.9) in both orderings, rather than an assertion that would pass under any selection rule.

#4 (specificity-over-order on ties) and #6 (quoted comma-containing params) are now called out explicitly in the docstring as known deviations that don't affect any header this API needs to accept today, per your framing of them as non-blocking. Left #5 (type/* wildcards) alone for the same reason -- happy to add if you'd rather it not wait.

24/24 accept_header-filtered tests passing, full rpc:: suite at 90/90.

[osr21]

Disclosure: I'm not affiliated with Circle — an external community contributor, not a maintainer, with no write access. This review is advisory only and carries no merge authority; please defer to Circle maintainers.

---

Re-ran the port against 32fa054. Both blocking items are genuinely fixed — not just described as fixed:

header                                          before      after
application/vnd.arc.v1+json; q=nan              406         V1 (q=1)
application/vnd.arc.v1+json; q=NaN              406         V1 (q=1)
application/vnd.arc.v1+json; q=inf              V1 (q=inf)  V1 (q=1)
application/vnd.arc.v1+json; Q=0                200         406
Application/JSON                                406         V1 (q=1)
APPLICATION/VND.ARC.V1+JSON                     406         V1 (q=1)
application/vnd.arc.v1+json; q=1.5              V1 (q=1.5)  V1 (q=1, clamped)

Your 24 also reconciles exactly: grep -c "fn test_.*accept_header" gives 20 in version.rs and 4 in middleware.rs.

Two things I checked that could have bitten you and don't. param[2..] is panic-safe here, because param.get(..2) returns None unless byte 2 is both in range and a char boundary — so the slice is only reached when it's valid, and a multi-byte leading character can't produce a panic. And the workspace only denies arithmetic_side_effects, cast_possible_truncation and unwrap_used; indexing_slicing isn't in the set, so the slice won't trip -D warnings either.

The #3 fix is the part I'd call out as done properly. The two assertions pin the rule as a pair, which is what makes it real coverage:

"v1;q=0.3, json;q=0.9"  expect 0.9  ->  first-wins yields 0.3  (test A fails)
"json;q=0.9, v1;q=0.3"  expect 0.9  ->  last-wins  yields 0.3  (test B fails)

Neither ordering alone would catch a regression — A only rules out first-wins, B only rules out last-wins. Together they pin highest-q, with one version defined. That's the inert-test problem actually solved rather than papered over.

Good catch on -infinity in your own test draft, too. That's the right instinct: the code was correct and the expectation was wrong.

One leftover asymmetry (non-blocking, your call)

is_finite() runs before the clamp, so non-finite values skip clamping entirely and take the q=1 fallback — while finite out-of-range negatives are clamped to 0.0 and then dropped by the q <= 0.0 guard:

q=-1          -> 406        q=-inf        -> V1 (q=1)
q=-0.5        -> 406        q=-infinity   -> V1 (q=1)
                            q=nan         -> V1 (q=1)
                            q=bogus       -> V1 (q=1)

So q=-1 is the single malformed input class that produces a hard 406, while every other non-conforming value — including a more nonsensical -inf — gets served at full preference. Both are equally unproducible by a conforming sender, so this is cosmetic rather than a real-world path, and the current behaviour is safe in the direction that matters (it never serves something marked q=0). If you want it uniform, .filter(|v| (0.0..=1.0).contains(v)) in place of is_finite() routes every out-of-range value to the documented q=1 default and makes the clamp redundant. Equally fine to leave it and let the docstring stand.

On #5, since you asked

I'd leave type/* out of this PR. Partial wildcards and the specificity rule you deferred in #4 are the same problem wearing two hats: the moment application/* is recognised, the interesting question is what happens when it appears alongside a specific range at equal q — which is exactly the precedence ordering you've documented as needing revisit once a second version exists. Adding wildcard matching now would half-answer it and bake in the order-based tie-break you've flagged as a deviation. Cleaner as one change when V2 lands and both rules can be settled together against something observable.

Approving. I can't compile Rust here, so this rests on source review plus the port and the spec citations — CI on the pinned toolchain remains authoritative.

[ygd58]

Thanks -- took the suggested fix in a1bf80f: replaced .filter(is_finite).map(clamp) with .filter(|v| (0.0..=1.0).contains(v)), which routes every out-of-range value (finite or not) to the documented q=1 fallback uniformly, and makes the clamp step redundant (removed). Added a test for the specific asymmetry you flagged (q=-1, q=-0.5, q=1.5 all now resolve to q=1 via best_match_with_quality, consistent with nan/inf/unparsable).

25/25 accept_header tests passing, full rpc:: suite at 91/91.

[osr21]

Re-verified at a1bf80f. Approval stands (it's recorded against 32fa054, so it may show as stale). Two small things worth fixing, neither blocking.

The fix does what you describe

Ported both revisions of best_match_with_quality to JS (faithful f32::from_str grammar, Math.fround) and diffed the outcomes:

Accept	32fa054	a1bf80f
...v1+json; q=-1	406	200 V1, q=1
...v1+json; q=-0.5	406	200 V1, q=1
...v1+json; q=1.5	200 V1, q=1	200 V1, q=1
...v1+json; q=0 / Q=0 / q=-0.0 / q=0.000	406	406
...v1+json; q=nan / inf / -infinity / not-a-number	200 V1, q=1	200 V1, q=1

The one thing that could have broken with a range filter didn't: 0.0 is inside (0.0..=1.0), so an explicit q=0 still reaches the q <= 0.0 skip rather than the fallback, and -0.0 lands there too (-0.0 == 0.0, so contains accepts it and the skip still fires). middleware.rs is byte-identical to the reviewed revision (blob 5fbf4c2).

RFC 9110 §12.4.1 backs the lenient direction explicitly: when nothing is acceptable the origin server "can either honor the header field by sending a 406 (Not Acceptable) response or disregard the header field." Uniform q=1 is squarely inside that latitude.

1. A malformed q isn't just acceptable, it's maximally preferred

This is the part the single-range tests can't show. The fallback is 1.0, which is the top of the scale, so a malformed range now outranks a conforming one:

application/vnd.arc.v1+json;q=-1, application/json;q=0.5
  32fa054 -> winning q = 0.5   (the -1 range was clamped out)
  a1bf80f -> winning q = 1.0   (the -1 range wins)

Invisible today, because both ranges map to V1. Once a second version exists, ...v1+json;q=-1, ...v2+json;q=0.9 selects V1 — the client's junk preference beating its stated one. That is the same shape of latent problem as the specificity-on-ties caveat you already documented, and it lands at the same moment (V2). Suggest folding one clause into that existing note rather than changing code: a malformed q is promoted to maximum preference, revisit alongside the tie-break rule.

2. The clamp is gone but four comments still describe it

• version.rs:120 — "filter to finite values first"; the code filters on range now, not finiteness.
• version.rs:354 — "must be filtered out explicitly by the is_finite() check".
• version.rs:372-376 — "is_finite() filters all three out before the clamp step ... the clamp only ever applies to a finite out-of-range value, e.g. a hypothetical q=1.5". This one is contradicted by the test immediately below it, which asserts q=1.5 taking the fallback path.
• version.rs:72-74 (the public docstring, so this is the contract, not just a comment) — still says only "fails to parse, or parses to a non-finite value ... is treated as q=1". Finite-but-out-of-range is now in that same bucket and isn't mentioned, even though the new test asserts it.

3. Which of the new assertions are load-bearing

In test_from_accept_header_out_of_range_finite_quality_falls_back_to_acceptable, the q=-1 and q=-0.5 assertions are real regression guards — they fail on 32fa054. The q=1.5 one passes on both revisions, since the clamp already produced 1.0. Fine to keep as documentation, but it isn't pinning this change.

Counts reconcile exactly

• 25 accept_header tests ✓ — 21 in version.rs + 4 in middleware.rs. (A fn .*accept_header grep returns 26 lines; the extra is the production from_accept_header itself, which a nextest name filter doesn't match.)
• 91 rpc:: tests ✓ — src/rpc/*.rs carries 92 #[test]/#[tokio::test] attributes, but one is commented out at routes.rs:1350 (test_add_persistent_peer_missing_p2p, disabled since the initial open-source commit pending feat(network)!: require PeerId in persistent peers configuration malachite#1485). 92 − 1 = 91.

Unrelated pre-existing nit

Present on main, not introduced here: MEDIA_TYPE_PREFIX already ends in v and ApiVersion::from_str accepts both "v1" and "1", so application/vnd.arc.vv1+json negotiates V1. Harmless, tighten only if you're touching from_str anyway.

---

Verification caveat: no Rust toolchain in my environment, so the above is source review plus a JS port of the parser run against the vectors shown — CI is authoritative for the actual test results.

Disclosure: I'm an external community contributor, unaffiliated with Circle, with no write access to this repository. My reviews and approvals are advisory only and carry no merge authority.

[ygd58]

Thanks for catching both -- fixed in e22ea13, no code changes, comments/docstring only:

1. Folded the malformed-q-outranks-a-conforming-range interaction into the existing tie-break docstring note, as suggested, rather than changing behavior -- both are the same shape of latent V2-only problem and now sit together.
2. Updated all four stale is_finite()/clamp references (docstring, the core match-arm comment, and both the nan and infinite test comments) to describe the actual [0,1]-range check. The infinite-test comment in particular was directly contradicted by the test immediately below it, as you noted -- good catch.

Left the q=1.5 test as documentation per your point 3, and the pre-existing vv1/from_str overlap untouched per your point 5 (agreed it is not worth touching while unrelated to this change).

25/25 accept_header, 91/91 rpc:: still green.