Migrate utility layer to HTTP types (PR 11)#623
Migrate utility layer to HTTP types (PR 11)#623prk-Jr wants to merge 3 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
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
aram356
left a comment
There was a problem hiding this comment.
Summary
Prior blocking findings (duplicate-header drops in to_fastly_request and copy_custom_headers) are both fixed in 2817761, with round-trip regression tests added. CI is green. Approving; non-blocking observations below.
Non-blocking
🤔 thinking
from_fastly_request_refsilently drops body (crates/trusted-server-core/src/compat.rs:54): docs note the body is empty, but the name doesn't signal it. Footgun if a future caller passes a POST expecting body access. Consider renaming tofrom_fastly_headers_refor adding adebug_assert!when the source request has a non-empty body. All current callers only read headers, so not a bug today.- Double conversion on the auction hot path:
auction/endpoints.rs:50buildshttp_reqonce, butauction/formats.rs:93(convert_tsjs_to_auction_request) re-converts the same underlyingfastly::Requestagain viafrom_fastly_request_ref. Each conversion iterates all headers — two copies per auction request. Acceptable for a temporary bridge (PR 15 removes it), but worth a comment referencing PR 15 so the duplication doesn't get frozen. forward_cookie_headerusesinsert, notappend(crates/trusted-server-core/src/cookies.rs:167,175,184): fine for HTTP/1 whereCookieis a single semicolon-joined value, but HTTP/2 (RFC 7540 §8.1.2.5) allows splitting across multiple headers and only the first would be forwarded. Matches pre-migration behavior and Fastly concatenates HTTP/2 cookie headers upstream, so likely not a live bug — worth documenting the assumption.
🌱 seedling
- Migration guard scope is "utility modules only" (
crates/trusted-server-core/src/migration_guards.rs:27-37): guards 6 files. Handler/integration files (publisher.rs,proxy.rs,registry.rs,integrations/*.rs,auction/*.rs) still usefastly::Requestby design. PR 12–14 should extend the banned-patterns list as files are migrated.
⛏ nitpick
- Dead case check in
copy_fastly_custom_headers(crates/trusted-server-core/src/compat.rs:144): checks both"x-"and"X-"thoughfastly::Requestnormalizes to lowercase. Not worth a commit in temporary code — trim on the next touch.
CI Status
- integration tests: PASS
- browser integration tests: PASS
- prepare integration artifacts: PASS
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!)