Migrate utility layer to HTTP types (PR 11)#623
Migrate utility layer to HTTP types (PR 11)#623prk-Jr wants to merge 2 commits intofeature/edgezero-pr10-abstract-logging-initializationfrom
Conversation
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
This PR cleanly migrates utility-layer functions (auth, cookies, synthetic, http_util, consent/extraction, consent/mod) from fastly::Request/fastly::Response to http::Request/http::Response with EdgeBody. The compat bridge pattern is sound and well-tested.
Two duplicate-header-dropping bugs need fixing before merge — see inline comments.
Blocking
🔧 wrench
to_fastly_requestdrops duplicate headers: Usesset_headerinstead ofappend_header, losing multi-valued headers during conversion (compat.rs:65)copy_custom_headersdrops duplicate X-headers: Usesinsertinstead ofappend, a behavioral regression from the old Fastly-based version (http_util.rs:22)
Non-blocking
🤔 thinking
- Migration guard
strip_line_commentsis naive about string literals: A Rust string literal like"fastly::Request"in a test assertion would trigger a false positive. Currently does not happen, but the approach is fragile. Consider adding a brief comment documenting the limitation. (migration_guards.rs)
🌱 seedling
copy_custom_headersin http_util.rs has no callers outside tests: The integrations (lockr, permutive) now callcompat::copy_fastly_custom_headersinstead. Worth noting for PR 15 cleanup.
⛏ nitpick
- Dead
"X-"check incopy_fastly_custom_headers: The check forname_str.starts_with("X-")in compat.rs:144 is dead code since Fastly normalizes header names to lowercase. The migratedcopy_custom_headersin http_util.rs only checks"x-". Not a correctness issue, but the asymmetry may confuse readers. - Temporary compat functions well-documented with removal targets: Every public function in
compat.rshas a# PR 15 removal targetannotation — great practice for tracking temporary code.
👍 praise
- Thorough compat test coverage: 11 focused tests covering round-trip conversions, duplicate header preservation, header sanitization, cookie forwarding with consent stripping, and synthetic cookie lifecycle. Excellent scaffolding for temporary bridge code.
- Migration guard test is a clever regression barrier: The
include_str!approach to scan source files for banned Fastly types provides a compile-time-like guarantee without proc macros.
📝 note
mimedependency is appropriate: Replacesfastly::mime::APPLICATION_JSONreferences. Well-maintained, zero-dependency crate suitable for the use case.
CI Status
- All checks: PASS
| let (parts, body) = req.into_parts(); | ||
| let mut fastly_req = fastly::Request::new(parts.method, parts.uri.to_string()); | ||
| for (name, value) in &parts.headers { | ||
| fastly_req.set_header(name.as_str(), value.as_bytes()); |
There was a problem hiding this comment.
🔧 wrench — set_header overwrites previous values for the same header name, dropping duplicates.
If an http::Request has multiple values for the same header (e.g., multiple X-Custom values), only the last one survives the conversion. to_fastly_response at line 109 correctly uses append_header — this should match.
Fix:
fastly_req.append_header(name.as_str(), value.as_bytes());| to.set_header(header_name, value); | ||
| } | ||
| if name_str.starts_with("x-") && !INTERNAL_HEADERS.contains(&name_str) { | ||
| to.headers_mut().insert(header_name.clone(), value.clone()); |
There was a problem hiding this comment.
🔧 wrench — insert replaces any prior value for the same header name, dropping duplicates.
This is a behavioral regression from the old Fastly-based copy_custom_headers which preserved duplicates via append_header. If the source request has multiple values for the same X-* header, only the last survives.
Fix:
to.headers_mut().append(header_name.clone(), value.clone());
aram356
left a comment
There was a problem hiding this comment.
Summary
Migrates the utility layer (auth, cookies, synthetic, http_util, consent) off direct fastly::Request/fastly::Response to http::{Request, Response}<EdgeBody> with a temporary compat bridge. Well-structured migration with good test coverage and clear lifecycle annotations. Two header-handling bugs need fixing before merge.
Blocking
🔧 wrench
copy_custom_headersdrops duplicate headers: usesinsertinstead ofappendinhttp_util.rs:22. The compat shim preserves duplicates; this migrated version doesn't. Will become a production regression when the compat layer is removed in PR 15.to_fastly_requestdrops duplicate headers: usesset_headerinstead ofappend_headerincompat.rs:65. Inconsistent withto_fastly_responsewhich correctly usesappend_headeron line 109.
Non-blocking
🤔 thinking
- Migration guard doesn't handle block comments:
strip_line_commentsinmigration_guards.rsonly strips//line comments. A/* fastly::Request */block comment would cause a false positive, and a real regression hidden inside a block comment wouldn't be caught. Acceptable for a guard test (false positives are safe), just noting the limitation.
🌱 seedling
- Add a
to_fastly_requestduplicate-header round-trip test: after fixing theappend_headerissue, a round-trip test (http::Request→fastly::Request→http::Request) verifying duplicate header preservation would prevent future regressions.
⛏ nitpick
- Redundant case check in
copy_fastly_custom_headers:compat.rs:144checks both"x-"and"X-"but Fastly'sHeaderNameis already case-normalized to lowercase. Not worth changing in temporary code.
CI Status
- integration tests: PASS
- browser integration tests: PASS
- prepare integration artifacts: PASS
| to.set_header(header_name, value); | ||
| } | ||
| if name_str.starts_with("x-") && !INTERNAL_HEADERS.contains(&name_str) { | ||
| to.headers_mut().insert(header_name.clone(), value.clone()); |
There was a problem hiding this comment.
🔧 wrench — insert silently drops duplicate headers
headers_mut().insert(...) replaces any existing value for the same header name. If the source request carries duplicate x-custom headers (e.g. x-custom: first and x-custom: second), only the last value survives.
The compat shim copy_fastly_custom_headers correctly uses append_header — and has a test (copy_fastly_custom_headers_preserves_duplicate_values) proving duplicates are preserved. This migrated version lacks that guarantee.
Currently no production code calls this version (all callers use the compat shim), but this is the version that will go live when PR 15 removes the compat layer.
Fix:
to.headers_mut().append(header_name.clone(), value.clone());Also add a test mirroring copy_fastly_custom_headers_preserves_duplicate_values.
| let (parts, body) = req.into_parts(); | ||
| let mut fastly_req = fastly::Request::new(parts.method, parts.uri.to_string()); | ||
| for (name, value) in &parts.headers { | ||
| fastly_req.set_header(name.as_str(), value.as_bytes()); |
There was a problem hiding this comment.
🔧 wrench — set_header drops duplicate headers; should be append_header
set_header replaces rather than appends, so duplicate headers in the source http::Request are collapsed to a single value. to_fastly_response on line 109 correctly uses append_header — this looks like an oversight.
Currently unused outside its own test, but it's a public API that future PRs may call.
Fix:
fastly_req.append_header(name.as_str(), value.as_bytes());| pub fn copy_fastly_custom_headers(from: &fastly::Request, to: &mut fastly::Request) { | ||
| for (name, value) in from.get_headers() { | ||
| let name_str = name.as_str(); | ||
| if (name_str.starts_with("x-") || name_str.starts_with("X-")) |
There was a problem hiding this comment.
⛏ nitpick — Redundant case check
if (name_str.starts_with("x-") || name_str.starts_with("X-"))Fastly's HeaderName is already case-normalized to lowercase, so the "X-" branch never fires. The migrated version in http_util.rs correctly uses only "x-". Not worth changing in temporary compat code, just noting for awareness.
Summary
fastly::Request/fastly::Responseusage so core helpers can operate onhttp::{Request, Response}andedgezero_core::Body.compatbridge at Fastly boundaries so handlers and integrations can keep working while later migration PRs move the remaining call stack.Changes
Cargo.tomlmimedependency used by migrated HTTP response helpers.Cargo.lockmimedependency.crates/trusted-server-adapter-fastly/src/main.rscrates/trusted-server-core/Cargo.tomlmimeworkspace dependency.crates/trusted-server-core/src/auction/endpoints.rscrates/trusted-server-core/src/auction/formats.rscrates/trusted-server-core/src/auth.rshttp::Request/Responseand update tests to HTTP builders.crates/trusted-server-core/src/compat.rscrates/trusted-server-core/src/consent/extraction.rshttp::Request<EdgeBody>.crates/trusted-server-core/src/consent/mod.rscrates/trusted-server-core/src/cookies.rscrates/trusted-server-core/src/http_util.rsClientInfo.crates/trusted-server-core/src/integrations/lockr.rscrates/trusted-server-core/src/integrations/permutive.rscrates/trusted-server-core/src/integrations/prebid.rscrates/trusted-server-core/src/integrations/registry.rscrates/trusted-server-core/src/integrations/testlight.rscrates/trusted-server-core/src/lib.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.rsmime::APPLICATION_JSON.crates/trusted-server-core/src/synthetic.rshttp::Request<EdgeBody>.Closes
Closes #492
Test plan
cargo test --workspacecargo clippy --workspace --all-targets --all-features -- -D warningscargo fmt --all -- --checkcd crates/js/lib && npx vitest runcd crates/js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute servecargo test --package trusted-server-core compat -- --nocapture,cargo test --package trusted-server-core http_util -- --nocapture,cargo test --package trusted-server-core request_signing -- --nocapture, andcargo test --package trusted-server-core migration_guards -- --nocapturecd crates/js/lib && npx vitest runcurrently fails before test execution withERR_REQUIRE_ESMinhtml-encoding-sniffer->@exodus/bytes/encoding-lite.js; leaving CI to capture the current JS environment issue.Hardening note
This PR does not add any new config-derived regex or pattern compilation paths. Basic auth still surfaces invalid enabled handler regex configuration as an error rather than panicking, covered by
auth::tests::returns_error_for_invalid_handler_regex_without_panickingalongside the existing settings startup validation tests.Checklist
unwrap()in production code — useexpect("should ...")println!)