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.