Preserve Prebid bidder parameter string types#933
Conversation
aram356
left a comment
There was a problem hiding this comment.
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 theirenabled: boolfields usedeserialize_with = "from_value_or_str"(checked every file incrates/trusted-server-core/src/integrations/), so a config carryingenabled = "true"ortimeout_ms = "1000"previously loaded fine and now fails at startup withinvalid type: string "true", expected a boolean. Relatedly,is_explicitly_disabledcan no longer seeenabled = "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 → Changedalready documents an equivalentbid_param_zone_overridesbreaking 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, andtrusted-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_envlost its doc comment and theval→valuerename 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_overridesremoves a whole class of silent retyping. - The new test is a genuine regression test (
settings.rs:3200) — cherry-picked ontomainit fails withleft: Number(12345), right: String("12345"), and it coversbid_param_zone_overridesandbid_param_override_rulesin 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: PASScargo clippy-axum: PASScargo test -p trusted-server-core: PASS (1647 passed, 0 failed;mainbaseline 1648)cargo test-axum: PASS
| .change_context(TrustedServerError::Configuration { | ||
| message: "Failed to deserialize configuration".to_string(), | ||
| })?; | ||
| settings.integrations.normalize_legacy_env(); |
There was a problem hiding this comment.
❓ question — Why keep the four legacy-env tests that still depend on this coercion?
This PR deletes the two Prebid JSON-env tests, but leaves four tests that are now the only consumers of normalize_env_value / normalize_legacy_env:
test_env_var_roundtrip_normalizes_integration_types(settings.rs:4222)test_integration_settings_from_env(settings.rs:3995)test_datadome_protection_scope_overrides_with_json_env(settings.rs:3249)test_datadome_protection_scope_overrides_with_indexed_env(settings.rs:3340)
I verified this by deleting the call on this line and running cargo test -p trusted-server-core: exactly those 4 fail, the other 1643 pass.
Two reasons they look stale rather than load-bearing:
test_env_var_roundtrip_normalizes_integration_typesis documented as testing "the full build.rs round-trip … env vars are baked intoSettingsat build time viafrom_toml_and_env, serialized to TOML, then parsed back at runtime". There is no build.rs settings pipeline anymore —ts config pushgoes through EdgeZero's typed app-config loader.test_datadome_protection_scope_overrides_with_json_envasserts a table can be replaced wholesale by a JSON env var. EdgeZero'sapply_env_overlaycoerces each env string into the existing TOML value's type and explicitly rejects array/table targets (env var … cannot override array / table values — env overlay supports scalar leaves only), which is exactly why removing the Prebid env doc examples in this PR is correct.
If those four are migrated to from_toml / from_json_value, then normalize_env_value, normalize_legacy_env, and from_toml_and_env can all be deleted outright — finishing what this PR started rather than leaving a #[cfg(test)] shim that keeps semantics alive which no production path has.
| /// build.rs preserves correct types. | ||
| pub fn normalize(&mut self) { | ||
| #[cfg(test)] | ||
| fn normalize_legacy_env(&mut self) { |
There was a problem hiding this comment.
⛏ nitpick — The doc comment that normalize() carried was dropped in the rename. Since this is now a test-only shim, a one-liner explaining why it still exists would save the next reader a git blame:
/// Legacy test-only shim: expands JSON-encoded strings produced by the
/// `config` crate's `Environment` source. Production config never carries
/// untyped strings — `EdgeZero`'s env overlay coerces against the existing
/// TOML value's type — so this must not run on the runtime paths.
#[cfg(test)]
fn normalize_legacy_env(&mut self) {Also, the val → value rename in normalize_env_value above adds a diff line without changing anything.
| ENVIRONMENT_VARIABLE_SEPARATOR, | ||
| ENVIRONMENT_VARIABLE_SEPARATOR | ||
| ); | ||
| fn prebid_numeric_string_bid_param_overrides_survive_config_roundtrip() { |
There was a problem hiding this comment.
♻️ refactor — Good regression test (I confirmed it fails on main with left: Number(12345), right: String("12345")), but it stops one layer short of the reported defect.
It asserts on the raw integrations.get("prebid") JSON map. The bug in #932 is about the params actually sent to bidders, which are produced by the compiled rule engine — CompiledBidParamOverrideRule::from_bidder_override / from_zone_override in integrations/prebid.rs. A type regression introduced during rule compilation or the shallow merge would not be caught here.
Suggest extending the same test to also assert through the typed path:
let cfg = runtime_settings
.integration_config::<PrebidIntegrationConfig>("prebid")
.expect("Prebid config query should succeed")
.expect("Prebid config should exist");
assert_eq!(
cfg.bid_param_overrides["pubmatic"]["publisherId"],
json!("12345"),
"should preserve numeric-looking publisherId through the typed config"
);and ideally one assertion on the merged params emitted by the compiled rules.
One more thing worth a comment in the test: the TOML -> Settings -> serde_json::to_value -> from_json_value chain it exercises is the production ts config push -> BlobEnvelope -> settings_from_config_blob path (config_payload.rs:39). Saying so makes it obvious this isn't a synthetic round-trip.
Summary
Closes #932
Testing
cargo fmt --all -- --checkcargo test --package trusted-server-core --target aarch64-apple-darwin bid_param_overridescargo test-axumcargo test-fastly