Migrate handler layer to HTTP types (PR 12)#624
Migrate handler layer to HTTP types (PR 12)#624prk-Jr wants to merge 3 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
|
|
||
| // Match known routes and handle them | ||
| let result = match (method, path.as_str()) { | ||
| match (method.clone(), path.as_str()) { |
There was a problem hiding this comment.
🌱 seedling — method is already an owned value (cloned on line 138). Since it's not used after the match, this .clone() is redundant — it could be moved instead:
match (method, path.as_str()) {| /// - The proxy request fails | ||
| /// - The origin backend is unreachable | ||
| pub fn handle_publisher_request( | ||
| pub async fn handle_publisher_request( |
There was a problem hiding this comment.
🤔 thinking — This changed from sync to async since it now calls services.http_client().send().await instead of the Fastly SDK's synchronous req.send(). The caller wraps everything in block_on(route_request(...)), so it should be behaviorally equivalent — just confirming req.send() was previously a blocking call on the Fastly runtime.
| Ok(response) | ||
| } | ||
|
|
||
| fn insert_response_header( |
There was a problem hiding this comment.
🏕 camp site — insert_response_header takes name: &'static str and relies on implicit conversion. Taking http::HeaderName directly as the parameter type would make the API clearer and avoid runtime parsing. Since callers pass known-valid string literals this works fine — just a minor improvement opportunity.
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
| use super::provider::AuctionProvider; | ||
| use super::types::{AuctionContext, AuctionRequest, AuctionResponse, Bid, BidStatus}; | ||
|
|
||
| fn platform_response_to_fastly( |
There was a problem hiding this comment.
🤔 thinking — platform_response_to_fastly duplicated from proxy.rs
This is a copy of the function that was removed from proxy.rs. Meanwhile the integration shims (GTM, GPT, testlight) go through compat::to_fastly_response for the same purpose. Two separate HTTP→Fastly response conversion paths makes divergence easy — e.g., this copy uses set_header (replaces duplicate header names) while compat::to_fastly_response preserves them.
Consider moving this into compat.rs as a shared helper, or at minimum cross-referencing the two so PR 15 removes both.
| }; | ||
| let mut resp = fastly::Response::from_status(parts.status.as_u16()); | ||
| for (name, value) in parts.headers.iter() { | ||
| resp.set_header(name.as_str(), value.as_bytes()); |
There was a problem hiding this comment.
🤔 thinking — set_header drops multi-value headers
resp.set_header(name, value) in a loop replaces per header name. If a provider response carries multi-value headers (e.g. Set-Cookie), only the last value survives. The analogous code in rebuild_response_with_body (proxy.rs) was correctly changed to append.
Low risk since this is a PR 15 removal target and auction provider responses are unlikely to have meaningful duplicates, but worth keeping in mind.
| let mut pipeline = StreamingPipeline::new(config, processor); | ||
| pipeline | ||
| .process(body, &mut output) | ||
| .process(Cursor::new(body.into_bytes()), &mut output) |
There was a problem hiding this comment.
♻️ refactor — Cursor::new(body.into_bytes()) pattern appears 4 times
This same pattern shows up here, and 3 times in publisher.rs (process_response_streaming). A small helper would reduce repetition and centralize the implicit body-materialization:
fn body_as_reader(body: EdgeBody) -> Cursor<bytes::Bytes> {
Cursor::new(body.into_bytes())
}| use crate::synthetic::{get_or_generate_synthetic_id, is_valid_synthetic_id}; | ||
|
|
||
| const SUPPORTED_ENCODING_VALUES: [&str; 3] = ["gzip", "deflate", "br"]; | ||
| const DEFAULT_PUBLISHER_FIRST_BYTE_TIMEOUT: Duration = Duration::from_secs(15); |
There was a problem hiding this comment.
🌱 seedling — Hardcoded 15s timeout
The old BackendConfig::from_url() had its own default; this makes it explicit which is good. But 15 seconds may not be right for all deployments. Worth considering making this configurable via settings.publisher in a follow-up.
| /// - The proxy request fails | ||
| /// - The origin backend is unreachable | ||
| pub fn handle_publisher_request( | ||
| pub async fn handle_publisher_request( |
There was a problem hiding this comment.
📝 note — sync → async signature change
This went from pub fn to pub async fn because it now uses services.http_client().send(...).await instead of Fastly's blocking req.send(). The only caller already wraps in block_on so it works. Just flagging as a public API change.
| use fastly::http::{header, HeaderValue, Method, StatusCode}; | ||
| use fastly::{Request, Response}; | ||
| use http::{ | ||
| header, HeaderValue, Method as HttpMethod, Request as HttpRequest, |
There was a problem hiding this comment.
⛏ nitpick — Prefixed type aliases
HttpMethod, HttpRequest, HttpResponse, HttpStatusCode — the Http prefix doesn't add clarity inside a test module that already imports from the http crate. The publisher.rs tests use the unprefixed names (Method, Request as HttpRequest, StatusCode) which reads more naturally.
| }; | ||
|
|
||
| let mut response = proxy_request(settings, req, proxy_config, services) | ||
| let mut response = compat::to_fastly_response( |
There was a problem hiding this comment.
📝 note — Double-conversion at integration boundary
Fastly Request → HTTP Request → proxy_request → HTTP Response → Fastly Response. This copies the request and response bodies twice. Same pattern in gpt.rs and testlight.rs. Understood this is intentional (integration trait stays Fastly-based until PR 13), just documenting the cost.
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!)