Add auction endpoint request handling coverage#690
Conversation
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
The route coverage is useful, but one new test now codifies a configuration typo as a request-time 502 instead of a startup/configuration failure.
Blocking
🔧 wrench
- Missing auction provider should fail startup, not become request-time 502:
auction.providers = ["missing-provider"]currently builds the route stack and only fails when/auctionis called. Invalid enabled providers should fail during startup/registration so deployments do not start with a mistyped provider list.
CI Status
- cargo fmt / clippy: PASS
- rust tests: PASS
- js tests: PASS
- integration tests: PASS
aram356
left a comment
There was a problem hiding this comment.
Summary
The endpoint change correctly fixes a real bug — malformed /auction JSON now returns 400 instead of 502 — and the three accompanying tests for that path are solid. The contentious piece (already flagged by @prk-Jr) is the fourth test, which codifies a configuration typo (providers = ["missing-provider"]) as a request-time 502. That conflicts with the project rule in CLAUDE.md and locks in disputed behavior with an asserting test ahead of the #669 follow-up. Inline notes cover the blocking item, two thinking notes, two refactor suggestions, and one seedling.
Blocking
🔧 wrench
- Test codifies a config typo as request-time 502, conflicting with the project rule (
crates/trusted-server-adapter-fastly/src/route_tests.rs:389-400). See inline comment for the two suggested resolutions (validate at startup, or drop just this test pending #669).
Non-blocking
🤔 thinking
- Orchestrator guard behavior change is broader than the typo case (
crates/trusted-server-core/src/auction/orchestrator.rs:365-369). Inline. - Test name "no_providers" is ambiguous (
crates/trusted-server-adapter-fastly/src/route_tests.rs:377). Inline.
♻️ refactor
- Error message lacks diagnostic info (
crates/trusted-server-core/src/auction/orchestrator.rs:367). Inline alongside the thinking note. - TOML duplication between test settings helpers (
crates/trusted-server-adapter-fastly/src/route_tests.rs:166-196). Inline.
🌱 seedling
- Add a unit-level test for the new orchestrator guard. The existing TODO at
orchestrator.rs:765-778already notes the gap. A fakeAuctionProviderwhoserequest_bidsreturnsErrwould cover the "registered-but-failed-to-launch" path that the route-level test doesn't reach. Could land with the #669 follow-up.
CI Status
- cargo fmt: PASS
- cargo clippy: PASS
- cargo test (Rust): PASS
- vitest (JS): PASS
- format-typescript / format-docs: PASS
- integration tests + browser integration tests: PASS
- CodeQL / Analyze: PASS
aram356
left a comment
There was a problem hiding this comment.
Summary
Adds route- and orchestrator-level coverage for /auction, reclassifies malformed-body parsing as 400, fails the auction with 502 when no provider request launches, and validates configured provider/mediator names at startup. The changes are correct, well-tested, and convention-aligned. Verified locally: cargo fmt --check, cargo clippy --all-targets --all-features -D warnings, and the affected auction:: / route_tests suites all pass. No blocking issues — approving with a few non-blocking observations.
Non-blocking
🤔 thinking
- Whole-service blast radius for an auction config typo —
build_orchestratorruns in the per-request entrypoint (main.rs:85) before routing, so the newvalidate_configured_provider_namesmakes an unregistered/typo'd provider return aConfigurationerror (500) for every route, not just/auction. This matches the pre-existing fail-closed posture ofbuild_orchestrator(a badserver_urlalready does this) and aligns with the "don't silently disable invalid providers" convention — but it's the opposite philosophy from the adjacent consent-store handling, which deliberately scopes failures to consent routes (enshrined byconfigured_missing_consent_store_only_breaks_consent_routes). Worth a conscious maintainer thumbs-up that taking the whole service down on an[auction].providerstypo is the intended tradeoff. (crates/trusted-server-core/src/auction/orchestrator.rs:56,crates/trusted-server-adapter-fastly/src/main.rs:85) - Empty-provider route test covers the old guard, not the new branch — see inline comment on
route_tests.rs:383.
🌱 seedling / ⛏ nitpick
- Validation message wording — see inline comment on
orchestrator.rs:72. - Irrelevant
bidsinvalid_banner_ad_unit_body— see inline comment onroute_tests.rs:248.
CI Status
- fmt: PASS (local)
- clippy: PASS (local, core + adapter,
-D warnings) - rust tests: PASS (local —
auction::29 passed,route_tests4 passed) - js tests: N/A (no JS changes)
- CodeQL (GitHub): PASS
|
|
||
| assert_eq!( | ||
| response.get_status(), | ||
| StatusCode::BAD_GATEWAY, |
There was a problem hiding this comment.
🤔 thinking — This test uses providers = [], which hits the pre-existing "No providers configured" guard in orchestrator.rs (run_providers_parallel, ~line 284), not the new pending_requests.is_empty() branch (~line 391) this PR adds. Both return TrustedServerError::Auction → 502, so the route-level mapping is verified — but the new branch's end-to-end route mapping isn't covered here (only the LaunchFailingProvider orchestrator unit test exercises it).
Consider a route test with a registered-but-disabled provider so the request reaches the new branch end-to-end.
| if !self.providers.contains_key(provider_name) { | ||
| return Err(Report::new(TrustedServerError::Configuration { | ||
| message: format!( | ||
| "Auction provider `{provider_name}` is configured but not registered" |
There was a problem hiding this comment.
🌱 seedling — The message "configured but not registered" leans on internal vocabulary. The most common real cause is a disabled (or absent) integration, which "not registered" doesn't hint at. A more actionable phrasing, e.g.:
message: format!(
"Auction provider `{provider_name}` is listed in [auction] but no enabled integration provides it"
),would point an operator straight at the fix (enable the integration / fix the name).
| "sizes": [[300, 250]] | ||
| } | ||
| }, | ||
| "bids": [ |
There was a problem hiding this comment.
⛏ nitpick — This bids array references bidder "missing-provider" that is never asserted on, and the only caller runs with providers = [], so it's irrelevant to the assertion. For a helper named valid_banner_ad_unit_body it's a bit confusing — dropping the bids block would read cleaner.
Summary
/auctionrequest handling400 Bad Requestfor malformed auction JSON payloads/auctionnow returns502 Bad Gatewaywhen every configured provider is skipped, disabled, or fails to launch instead of returning200with an emptyseatbid.Closes #452
Testing
cargo test -p trusted-server-adapter-fastly route_tests -- --nocapturecargo test -p trusted-server-core auction:: -- --nocapturecargo fmt --all -- --checkcargo test --workspacecargo clippy --workspace --all-targets --all-features -- -D warnings