Skip to content

Migrate handler layer to HTTP types (PR 12)#624

Open
prk-Jr wants to merge 5 commits intofeature/edgezero-pr11-utility-layer-migration-v2from
feature/edgezero-pr12-handler-layer-types
Open

Migrate handler layer to HTTP types (PR 12)#624
prk-Jr wants to merge 5 commits intofeature/edgezero-pr11-utility-layer-migration-v2from
feature/edgezero-pr12-handler-layer-types

Conversation

@prk-Jr
Copy link
Copy Markdown
Collaborator

@prk-Jr prk-Jr commented Apr 8, 2026

Summary

  • Move the PR 12 handler boundary to http request/response types so core handlers no longer depend on Fastly request/response APIs.
  • Keep Fastly isolated to the adapter entry/exit path and the still-unmigrated integration/provider boundary, which keeps PR 13 scope separate.
  • Lock the migrated surface with tests and migration guards so later changes do not accidentally reintroduce handler-layer Fastly types.

Changes

File Change
crates/trusted-server-adapter-fastly/src/main.rs Move the conversion boundary to the adapter entry/exit path and route core handlers with HTTP-native requests and responses.
crates/trusted-server-core/src/auction/endpoints.rs Migrate the /auction handler to http types and rebuild a Fastly request only at the provider-facing AuctionContext boundary.
crates/trusted-server-core/src/auction/formats.rs Consume HTTP requests directly and return HTTP auction responses.
crates/trusted-server-core/src/auction/orchestrator.rs Keep provider parsing Fastly-based while making the provider response conversion explicit in the orchestrator.
crates/trusted-server-core/src/compat.rs Add the minimal borrowed HTTP-to-Fastly request bridge needed for the remaining provider boundary.
crates/trusted-server-core/src/geo.rs Make response header injection operate on HTTP responses.
crates/trusted-server-core/src/integrations/google_tag_manager.rs Adapt the integration boundary so the shared proxy helper can stay HTTP-native while the integration trait remains Fastly-based.
crates/trusted-server-core/src/integrations/gpt.rs Adapt the integration boundary so the shared proxy helper can stay HTTP-native while the integration trait remains Fastly-based.
crates/trusted-server-core/src/integrations/testlight.rs Adapt the integration boundary so the shared proxy helper can stay HTTP-native while the integration trait remains Fastly-based.
crates/trusted-server-core/src/migration_guards.rs Extend the guard to cover the handler modules migrated in PR 12.
crates/trusted-server-core/src/proxy.rs Migrate first-party proxy/click/sign/rebuild handlers and response rewriting to HTTP request/response types.
crates/trusted-server-core/src/publisher.rs Migrate publisher handlers to HTTP types and use the platform HTTP client for publisher-origin fetches.
crates/trusted-server-core/src/request_signing/endpoints.rs Migrate request-signing and admin handlers to HTTP request/response types.

Closes

Closes #493

Test plan

  • cargo test --workspace
  • cargo clippy --workspace --all-targets --all-features -- -D warnings
  • cargo fmt --all -- --check
  • JS tests: cd crates/js/lib && npx vitest run (not run; no JS/TS files changed)
  • JS format: cd crates/js/lib && npm run format
  • Docs format: cd docs && npm run format
  • WASM build: cargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1
  • Manual testing via fastly compute serve
  • Other: verified remaining Fastly usage is limited to the adapter boundary and the still-unmigrated integration/provider boundary

Checklist

  • Changes follow CLAUDE.md conventions
  • No unwrap() in production code — use expect("should ...")
  • Uses tracing macros (not println!)
  • New code has tests
  • No secrets or credentials committed

@prk-Jr prk-Jr self-assigned this Apr 8, 2026
@prk-Jr prk-Jr changed the title (PR 12) Migrate handler layer to HTTP types Migrate handler layer to HTTP types (PR 12) Apr 8, 2026
@prk-Jr prk-Jr linked an issue Apr 8, 2026 that may be closed by this pull request
@prk-Jr prk-Jr linked an issue Apr 9, 2026 that may be closed by this pull request
Copy link
Copy Markdown
Collaborator

@ChristianPavilonis ChristianPavilonis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, returns HttpResponse) and to_error_response (existing, returns FastlyResponse) 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

Comment thread crates/trusted-server-adapter-fastly/src/main.rs Outdated
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/auction/formats.rs
Copy link
Copy Markdown
Collaborator

@aram356 aram356 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_fastly duplicated into orchestrator.rs: Copy-pasted from proxy.rs rather than shared via compat.rs. Two separate HTTP→Fastly response paths risk divergence (this copy uses set_header which drops multi-value headers; compat::to_fastly_response doesn't). (orchestrator.rs:15)
  • sync→async handle_publisher_request: Public API signature change — only caller uses block_on so it works, but worth documenting. (publisher.rs:305)

♻️ refactor

  • Cursor::new(body.into_bytes()) repeated 4 times: Could extract a small body_as_reader helper to centralize body materialization. (proxy.rs:175, publisher.rs:228,246,263)

🌱 seedling

  • DEFAULT_PUBLISHER_FIRST_BYTE_TIMEOUT hardcoded 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_request with from_fastly_request/to_fastly_response, copying bodies twice. Expected for the PR 13 boundary.

⛏ nitpick

  • Inconsistent test type alias style: proxy.rs tests use HttpMethod/HttpStatusCode prefixes; publisher.rs tests don't. The unprefixed style is cleaner.

CI Status

  • integration tests: PASS
  • browser integration tests: PASS
  • prepare integration artifacts: PASS

Comment thread crates/trusted-server-core/src/auction/orchestrator.rs
Comment thread crates/trusted-server-core/src/auction/orchestrator.rs Outdated
Comment thread crates/trusted-server-core/src/proxy.rs Outdated
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/publisher.rs
Comment thread crates/trusted-server-core/src/proxy.rs Outdated
Comment thread crates/trusted-server-core/src/integrations/google_tag_manager.rs
@prk-Jr prk-Jr requested a review from aram356 April 13, 2026 12:31
Copy link
Copy Markdown
Collaborator

@aram356 aram356 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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::Stream dropcompat.rs:132-134 and auction/orchestrator.rs:29-32 log a warning and drop the body on the Stream variant. 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 → Stringproxy.rs:889 and proxy.rs:1003 do String::from_utf8(req.into_body().into_bytes().to_vec()) with no size cap. OOM/DoS risk on hostile POST.
  • Silent JSON serialization fallbackproxy.rs:1156 returns {} on serde_json::to_string failure instead of propagating an error.
  • Geo lookup moved before authmain.rs:95-110 runs the geo lookup on every request, including ones that will immediately 401. Confirm this is intentional.

❓ question

  • Integration trait still fastly-basedmain.rs:182-196 pays a full http → fastly → http round-trip per integration request. Deferred to PR 13? If so, please leave a // TODO(PR13) marker.
  • to_fastly_request_ref drops bodyauction/endpoints.rs:90 call 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-Cookie survival through rebuild_response_with_body (proxy.rs:138) — add a test, also consider mem::take to avoid the header clone.
  • Invalid settings.response_headers are silently warn-skipped at request time (main.rs:250-262); CLAUDE.md prefers startup-time validation.

♻️ refactor

  • migration_guards.rs uses substring matching on "fastly::Request" — prefer a \bfastly::(Request|Response|http::(Method|StatusCode))\b regex 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:325 vs publisher.rs:361 — inconsistent services.client_info() vs services.client_info.client_ip access idiom.

👍 praise

  • Tight adapter boundary in main.rs:89-120: one from_fastly_request/to_fastly_response pair framing route_request. Very clean.
  • insert_geo_header warn-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:325 vs publisher.rs:361 — inconsistent services.client_info() (method) vs services.client_info.client_ip (field) access idiom. Please pick one.

Comment thread crates/trusted-server-core/src/compat.rs
Comment thread crates/trusted-server-core/src/proxy.rs Outdated
Comment thread crates/trusted-server-core/src/proxy.rs Outdated
Comment thread crates/trusted-server-core/src/proxy.rs Outdated
Comment thread crates/trusted-server-adapter-fastly/src/main.rs
Comment thread crates/trusted-server-core/src/proxy.rs Outdated
Comment thread crates/trusted-server-adapter-fastly/src/main.rs
Comment thread crates/trusted-server-core/src/auction/endpoints.rs
Comment thread crates/trusted-server-core/src/geo.rs
Comment thread crates/trusted-server-core/src/migration_guards.rs
prk-Jr added 2 commits April 16, 2026 12:45
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.
@prk-Jr prk-Jr requested a review from aram356 April 16, 2026 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Thread RuntimeServices into integration and provider entry points Handler layer type migration

3 participants