Migrate handler layer to HTTP types (PR 12)#624
Migrate handler layer to HTTP types (PR 12)#624prk-Jr wants to merge 5 commits intofeature/edgezero-pr11-utility-layer-migration-v2from
Conversation
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Clean migration of the handler layer from fastly::Request/fastly::Response to http::Request<EdgeBody>/http::Response<EdgeBody>. The conversion boundary is pushed to the adapter entry/exit and the still-unmigrated integration trait boundary. Well-tested, CI passes.
Non-blocking
♻️ refactor
- Duplicate error-to-response functions:
http_error_response(new, returnsHttpResponse) andto_error_response(existing, returnsFastlyResponse) implement identical logic with different return types. Expected migration scaffolding — track for cleanup in PR 15 when Fastly types are fully removed. (main.rs:255-267/error.rs:10)
CI Status
- integration tests: PASS
- browser integration tests: PASS
- prepare integration artifacts: PASS
aram356
left a comment
There was a problem hiding this comment.
Summary
Solid mechanical migration of the handler layer from Fastly request/response types to http crate types. The conversion boundary is cleanly pushed to the adapter entry point, with proper compat shims for the still-unmigrated integration/provider boundary.
Non-blocking
🤔 thinking
platform_response_to_fastlyduplicated into orchestrator.rs: Copy-pasted fromproxy.rsrather than shared viacompat.rs. Two separate HTTP→Fastly response paths risk divergence (this copy usesset_headerwhich drops multi-value headers;compat::to_fastly_responsedoesn't). (orchestrator.rs:15)- sync→async
handle_publisher_request: Public API signature change — only caller usesblock_onso it works, but worth documenting. (publisher.rs:305)
♻️ refactor
Cursor::new(body.into_bytes())repeated 4 times: Could extract a smallbody_as_readerhelper to centralize body materialization. (proxy.rs:175,publisher.rs:228,246,263)
🌱 seedling
DEFAULT_PUBLISHER_FIRST_BYTE_TIMEOUThardcoded at 15s: Good to make it explicit, but may need to be configurable per deployment. (publisher.rs:22)
📝 note
- Integration proxy double-conversion: GTM, GPT, testlight each wrap
proxy_requestwithfrom_fastly_request/to_fastly_response, copying bodies twice. Expected for the PR 13 boundary.
⛏ nitpick
- Inconsistent test type alias style:
proxy.rstests useHttpMethod/HttpStatusCodeprefixes;publisher.rstests don't. The unprefixed style is cleaner.
CI Status
- integration tests: PASS
- browser integration tests: PASS
- prepare integration artifacts: PASS
aram356
left a comment
There was a problem hiding this comment.
Summary
Mechanically sound migration of the handler layer from fastly::{Request,Response} to http::{Request,Response}<EdgeBody>. The adapter entry/exit is now the only fastly conversion site for handlers, and the migration guard test is extended to lock this in. Overall high quality — a handful of concrete concerns to address before merge, mostly around silent failure modes that the type migration makes easier to miss.
Blocking
🔧 wrench
- Silent
EdgeBody::Streamdrop —compat.rs:132-134andauction/orchestrator.rs:29-32log a warning and drop the body on theStreamvariant. No test pins this behavior;debug_assert!only fires in debug. If a streaming body ever reaches this boundary, the client gets an empty 200. - Unbounded request body →
String—proxy.rs:889andproxy.rs:1003doString::from_utf8(req.into_body().into_bytes().to_vec())with no size cap. OOM/DoS risk on hostile POST. - Silent JSON serialization fallback —
proxy.rs:1156returns{}onserde_json::to_stringfailure instead of propagating an error. - Geo lookup moved before auth —
main.rs:95-110runs the geo lookup on every request, including ones that will immediately 401. Confirm this is intentional.
❓ question
- Integration trait still fastly-based —
main.rs:182-196pays a fullhttp → fastly → httpround-trip per integration request. Deferred to PR 13? If so, please leave a// TODO(PR13)marker. to_fastly_request_refdrops body —auction/endpoints.rs:90call site has no comment explaining why the empty body is safe. Please add one line so this invariant doesn't regress.
Non-blocking
🤔 thinking
- Multi-valued
Set-Cookiesurvival throughrebuild_response_with_body(proxy.rs:138) — add a test, also considermem::taketo avoid the header clone. - Invalid
settings.response_headersare silently warn-skipped at request time (main.rs:250-262); CLAUDE.md prefers startup-time validation.
♻️ refactor
migration_guards.rsuses substring matching on"fastly::Request"— prefer a\bfastly::(Request|Response|http::(Method|StatusCode))\bregex to avoid false positives on future string literals or doc comments.
🌱 seedling
- End-to-end streaming (
EdgeBody::Stream) is effectively unreachable today. Worth a follow-up to preserve streaming semantics through publisher fetch → rewrite → response once PR 13 lands.
📝 note
publisher.rs:325vspublisher.rs:361— inconsistentservices.client_info()vsservices.client_info.client_ipaccess idiom.
👍 praise
- Tight adapter boundary in
main.rs:89-120: onefrom_fastly_request/to_fastly_responsepair framingroute_request. Very clean. insert_geo_headerwarn-and-continue is the right shape for derived geo data.- Migration guard extended to cover the handler layer — exactly the right regression gate for this refactor.
CI Status
- fmt: PASS
- clippy: PASS
- rust tests: PASS
- js tests: PASS
(All three GitHub Actions checks — browser integration tests, integration tests, prepare integration artifacts — are green.)
publisher.rs:325vspublisher.rs:361— inconsistentservices.client_info()(method) vsservices.client_info.client_ip(field) access idiom. Please pick one.
Brings in PR11 review-finding fixes (compat shim additions, testlight integration changes, migration guard comments). Conflicts resolved by keeping PR12's http-native handler layer throughout — all compat shim insertions from PR11 are superseded by the http type migration.
Summary
httprequest/response types so core handlers no longer depend on Fastly request/response APIs.Changes
crates/trusted-server-adapter-fastly/src/main.rscrates/trusted-server-core/src/auction/endpoints.rs/auctionhandler tohttptypes and rebuild a Fastly request only at the provider-facingAuctionContextboundary.crates/trusted-server-core/src/auction/formats.rscrates/trusted-server-core/src/auction/orchestrator.rscrates/trusted-server-core/src/compat.rscrates/trusted-server-core/src/geo.rscrates/trusted-server-core/src/integrations/google_tag_manager.rscrates/trusted-server-core/src/integrations/gpt.rscrates/trusted-server-core/src/integrations/testlight.rscrates/trusted-server-core/src/migration_guards.rscrates/trusted-server-core/src/proxy.rscrates/trusted-server-core/src/publisher.rscrates/trusted-server-core/src/request_signing/endpoints.rsCloses
Closes #493
Test plan
cargo test --workspacecargo clippy --workspace --all-targets --all-features -- -D warningscargo fmt --all -- --checkcd crates/js/lib && npx vitest run(not run; no JS/TS files changed)cd crates/js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute serveChecklist
unwrap()in production code — useexpect("should ...")tracingmacros (notprintln!)