Skip to content

Preserve integration configuration string types - #933

Open
ChristianPavilonis wants to merge 6 commits into
mainfrom
preserve-prebid-bid-param-string-types
Open

Preserve integration configuration string types#933
ChristianPavilonis wants to merge 6 commits into
mainfrom
preserve-prebid-bid-param-string-types

Conversation

@ChristianPavilonis

@ChristianPavilonis ChristianPavilonis commented Jul 18, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • preserve scalar types for every integration configuration on production TOML and config-blob loading paths; this fixes Prebid bidder parameters that look numeric
  • remove the obsolete legacy test-only integration environment coercion and its stale coverage
  • regress TOML → runtime JSON config loading through typed Prebid rule compilation and emitted OpenRTB bidder parameters
  • document the breaking migration for quoted integration booleans and numbers

Closes #932

Follow-up: #972 tracks outdated integration environment-override documentation that predates this PR.

Testing

  • cargo fmt --all -- --check
  • cargo test_details -p trusted-server-core
  • cargo clippy-fastly && cargo clippy-axum && cargo clippy-cloudflare && cargo clippy-cloudflare-wasm && cargo clippy-spin-native && cargo clippy-spin-wasm
  • cargo test-fastly && cargo test-axum && cargo test-cloudflare && cargo test-spin
  • cargo test --manifest-path crates/trusted-server-integration-tests/Cargo.toml --test parity
  • cd crates/trusted-server-js/lib && npx vitest run && npm run format

@aram356 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Summary

Correct fix for a real and broader-than-Prebid defect: IntegrationSettings::normalize() ran from finalize_deserialized(), so it applied to Settings::from_toml (adapters) and Settings::from_json_value — the runtime config-store blob path in config_payload.rs:39. Every string in every integration's config was re-parsed as JSON, turning publisherId = "12345" into 12345 before it reached the auction request. Scoping the coercion to #[cfg(test)] and calling it only from the already-#[cfg(test)] from_toml_and_env helper is the right layering, since deserialize_with = "from_value_or_str" is the sanctioned per-field string-tolerance mechanism.

I verified the removal is safe rather than assuming it: EdgeZero v0.0.4's apply_env_overlay (edgezero-core/src/app_config.rs:404-480) coerces each env string into the existing TOML value's type and explicitly rejects array/table targets. So no untyped strings reach production config, and the env-var doc examples removed from prebid.rs documented a capability production never had.

Requesting changes on two points: a missing operator-facing migration note, and four leftover tests that now assert behavior no production path has.

Blocking

🔧 wrench

  • Operator-visible behavior change has no CHANGELOG entry (CHANGELOG.md): This is not only a Prebid fix — it changes type handling for all 15 integration configs. None of their enabled: bool fields use deserialize_with = "from_value_or_str" (checked every file in crates/trusted-server-core/src/integrations/), so a config carrying enabled = "true" or timeout_ms = "1000" previously loaded fine and now fails at startup with invalid type: string "true", expected a boolean. Relatedly, is_explicitly_disabled can no longer see enabled = "false", so that config becomes a parse error instead of a clean disable — loud failure is arguably the better outcome, but it should be an intentional, documented one. Unreleased → Changed already documents an equivalent bid_param_zone_overrides breaking change; please add a sibling entry telling operators to audit quoted scalars in [integrations.*].

❓ question

  • Four legacy-env tests still assert behavior production does not have (settings.rs:2032 — details inline).

Non-blocking

♻️ refactor

  • Regression test stops one layer short of the defect (settings.rs:3200 — details inline).

🤔 thinking

  • PR description understates the scope: the title and body read as Prebid-only, but the behavior change spans every integration's config and every non-test load path. Worth stating so reviewers and operators reading the merge commit understand the blast radius.

📌 out of scope

  • Docs still advertise integration env-var overrides, including forms EdgeZero rejects: docs/guide/integrations-overview.md:293-308, docs/guide/error-reference.md:117-123, and trusted-server.example.toml:163 (TRUSTED_SERVER__CREATIVE_OPPORTUNITIES__SLOT='[{…}]' — an array override the overlay refuses). These were already inaccurate before this PR, but removing the equivalent Prebid examples here makes the inconsistency conspicuous. Worth a follow-up issue.

⛏ nitpick

  • normalize_legacy_env lost its doc comment and the valvalue rename is diff noise (settings.rs:213 — details inline).

👍 praise

  • Fixed at the source instead of special-casing Prebid — deleting the blanket coercion rather than adding a string-preserving exception for bid_param_overrides removes a whole class of silent retyping.
  • The new test is a genuine regression test (settings.rs:3200) — cherry-picked onto main it fails with left: Number(12345), right: String("12345"), and it covers bid_param_zone_overrides and bid_param_override_rules in addition to the two shapes named in the PR body.

CI Status

All 19 GitHub checks pass (fmt, clippy, cargo test across fastly/axum/cloudflare/spin/CLI, cross-adapter parity, integration tests, vitest, CodeQL).

Independently reproduced locally against a worktree of the head commit:

  • cargo fmt --all -- --check: PASS
  • cargo clippy-axum: PASS
  • cargo test -p trusted-server-core: PASS (1647 passed, 0 failed; main baseline 1648)
  • cargo test-axum: PASS

Comment thread crates/trusted-server-core/src/settings.rs Outdated
Comment thread crates/trusted-server-core/src/settings.rs Outdated
Comment thread crates/trusted-server-core/src/settings.rs Outdated
@ChristianPavilonis ChristianPavilonis changed the title Preserve Prebid bidder parameter string types Preserve integration configuration string types Jul 27, 2026
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.

Preserve string types in Prebid bidder parameter overrides

2 participants