Uh oh!
There was an error while loading. Please reload this page.
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
Uh oh!
There was an error while loading. Please reload this page.
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
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
4703b0e to
efaaba6CompareUh oh!
There was an error while loading. Please reload this page.
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