Skip to content

Preserve Prebid bidder parameter string types#933

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

Preserve Prebid bidder parameter string types#933
ChristianPavilonis wants to merge 4 commits into
mainfrom
preserve-prebid-bid-param-string-types

Conversation

@ChristianPavilonis

Copy link
Copy Markdown
Collaborator

Summary

  • preserve nested JSON scalar types in Prebid bidder parameter override containers
  • decode JSON environment overrides once without recursively coercing numeric-looking strings
  • add regression coverage for TOML-to-runtime round trips and JSON environment overrides

Closes #932

Testing

  • cargo fmt --all -- --check
  • cargo test --package trusted-server-core --target aarch64-apple-darwin bid_param_overrides
  • cargo test-axum
  • cargo test-fastly

@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

.change_context(TrustedServerError::Configuration {
message: "Failed to deserialize configuration".to_string(),
})?;
settings.integrations.normalize_legacy_env();

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.

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:

  1. test_env_var_roundtrip_normalizes_integration_types is documented as testing "the full build.rs round-trip … env vars are baked into Settings at build time via from_toml_and_env, serialized to TOML, then parsed back at runtime". There is no build.rs settings pipeline anymore — ts config push goes through EdgeZero's typed app-config loader.
  2. test_datadome_protection_scope_overrides_with_json_env asserts a table can be replaced wholesale by a JSON env var. EdgeZero's apply_env_overlay coerces 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) {

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.

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 valvalue 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() {

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.

♻️ 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.

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