Skip to content

Preserve Prebid bidder parameter string types #933

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ChristianPavilonis wants to merge 4 commits into
base: main
Choose a base branch
from
+45 −115
Open

Preserve Prebid bidder parameter string types #933

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e55d985
Preserve Prebid bidder parameter string types
ChristianPavilonis Jul 18, 2026
5eef5f1
Satisfy Clippy for override normalization
ChristianPavilonis Jul 18, 2026
02c985f
Simplify Prebid bid parameter normalization
ChristianPavilonis Jul 21, 2026
cbef72f
Merge branch 'main' into preserve-prebid-bid-param-string-types
aram356 Jul 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 0 additions & 10 deletions crates/trusted-server-core/src/integrations/prebid.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,11 +289,6 @@ pub struct PrebidIntegrationConfig {
/// param1 = 12345
/// param2 = "value"
/// ```
///
/// Example via environment variable:
/// ```text
/// TRUSTED_SERVER__INTEGRATIONS__PREBID__BID_PARAM_OVERRIDES='{"bidder-name":{"param1":12345,"param2":"value"}}'
/// ```
#[serde(default)]
pub bid_param_overrides: HashMap<String, serde_json::Map<String, Json>>,
/// Canonical ordered bidder-param override rules.
Expand All @@ -311,11 +306,6 @@ pub struct PrebidIntegrationConfig {
/// when.zone = "header"
/// set = { placementId = "_abc" }
/// ```
///
/// Example via environment variable:
/// ```text
/// TRUSTED_SERVER__INTEGRATIONS__PREBID__BID_PARAM_OVERRIDE_RULES='[{"when":{"bidder":"kargo","zone":"header"},"set":{"placementId":"_abc"}}]'
/// ```
#[serde(default)]
pub bid_param_override_rules: Vec<BidParamOverrideRule>,
/// How consent signals are forwarded to Prebid Server.
Expand Down
150 changes: 45 additions & 105 deletions crates/trusted-server-core/src/settings.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,11 +187,12 @@ impl IntegrationSettings {
Ok(())
}

#[cfg(test)]
fn normalize_env_value(value: JsonValue) -> JsonValue {
match value {
JsonValue::Object(map) => JsonValue::Object(
map.into_iter()
.map(|(key, val)| (key, Self::normalize_env_value(val)))
.map(|(key, value)| (key, Self::normalize_env_value(value)))
.collect(),
),
JsonValue::Array(items) => {
Expand All @@ -208,11 +209,8 @@ impl IntegrationSettings {
}
}

/// Normalizes all entries in place, converting JSON-encoded strings from
/// environment variables into their proper typed representations.
/// Called eagerly after deserialization so that TOML serialization in
/// build.rs preserves correct types.
pub fn normalize(&mut self) {
#[cfg(test)]
fn normalize_legacy_env(&mut self) {

@aram356 aram356 Jul 22, 2026

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 val → value rename in normalize_env_value above adds a diff line without changing anything.

for value in self.entries.values_mut() {
*value = Self::normalize_env_value(value.clone());
}
Expand Down Expand Up @@ -2025,12 +2023,13 @@ impl Settings {
.change_context(TrustedServerError::Configuration {
message: "Failed to build configuration".to_string(),
})?;
let settings: Self =
let mut settings: Self =
config
.try_deserialize()
.change_context(TrustedServerError::Configuration {
message: "Failed to deserialize configuration".to_string(),
})?;
settings.integrations.normalize_legacy_env();

@aram356 aram356 Jul 22, 2026

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.


Self::finalize_deserialized(settings, "Build-time configuration")
}
Expand All @@ -2039,7 +2038,6 @@ impl Settings {
mut settings: Self,
validation_label: &str,
) -> Result<Self, Report<TrustedServerError>> {
settings.integrations.normalize();
settings.proxy.normalize();
settings.image_optimizer.normalize();
settings.consent.validate();
Expand Down Expand Up @@ -3199,109 +3197,51 @@ origin_host_header_overide = "www.example.com""#,
}

#[test]
fn test_prebid_bid_param_overrides_override_with_json_env() {
let toml_str = crate_test_settings_str();
let env_key = format!(
"{}{}INTEGRATIONS{}PREBID{}BID_PARAM_OVERRIDES",
ENVIRONMENT_VARIABLE_PREFIX,
ENVIRONMENT_VARIABLE_SEPARATOR,
ENVIRONMENT_VARIABLE_SEPARATOR,
ENVIRONMENT_VARIABLE_SEPARATOR
);
fn prebid_numeric_string_bid_param_overrides_survive_config_roundtrip() {

@aram356 aram356 Jul 22, 2026

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.

let toml_str = format!(
r#"{}

let origin_key = format!(
"{}{}PUBLISHER{}ORIGIN_URL",
ENVIRONMENT_VARIABLE_PREFIX,
ENVIRONMENT_VARIABLE_SEPARATOR,
ENVIRONMENT_VARIABLE_SEPARATOR
);
temp_env::with_var(
origin_key,
Some("https://origin.test-publisher.com"),
|| {
temp_env::with_var(
env_key,
Some(r#"{"criteo":{"networkId":99999,"pubid":"server-pub"}}"#),
|| {
let settings = Settings::from_toml_and_env(&toml_str)
.expect("Settings should parse with bidder param override env");
let cfg = settings
.integration_config::<PrebidIntegrationConfig>("prebid")
.expect("Prebid config query should succeed")
.expect("Prebid config should exist with env override");
let cfg_json =
serde_json::to_value(&cfg).expect("should serialize config to JSON");
[integrations.prebid.bid_param_overrides.pubmatic]
publisherId = "12345"
adSlot = "67890"

assert_eq!(
cfg_json["bid_param_overrides"]["criteo"]["networkId"],
json!(99999),
"should deserialize networkId override from env JSON"
);
assert_eq!(
cfg_json["bid_param_overrides"]["criteo"]["pubid"],
json!("server-pub"),
"should deserialize pubid override from env JSON"
);
},
);
},
);
}
[integrations.prebid.bid_param_zone_overrides.pubmatic]
header = {{ placementId = "24680" }}

#[test]
fn test_prebid_bid_param_override_rules_override_with_json_env() {
let toml_str = crate_test_settings_str();
let env_key = format!(
"{}{}INTEGRATIONS{}PREBID{}BID_PARAM_OVERRIDE_RULES",
ENVIRONMENT_VARIABLE_PREFIX,
ENVIRONMENT_VARIABLE_SEPARATOR,
ENVIRONMENT_VARIABLE_SEPARATOR,
ENVIRONMENT_VARIABLE_SEPARATOR
[[integrations.prebid.bid_param_override_rules]]
when = {{ bidder = "pubmatic", zone = "in_content" }}
set = {{ placementId = "13579" }}
"#,
crate_test_settings_str()
);
let settings = Settings::from_toml(&toml_str).expect("should parse TOML settings");
let serialized = serde_json::to_value(&settings).expect("should serialize settings");
let runtime_settings =
Settings::from_json_value(serialized).expect("should parse runtime JSON settings");
let raw = runtime_settings
.integrations
.get("prebid")
.expect("should contain Prebid settings");

let origin_key = format!(
"{}{}PUBLISHER{}ORIGIN_URL",
ENVIRONMENT_VARIABLE_PREFIX,
ENVIRONMENT_VARIABLE_SEPARATOR,
ENVIRONMENT_VARIABLE_SEPARATOR
assert_eq!(
raw["bid_param_overrides"]["pubmatic"]["publisherId"],
json!("12345"),
"should preserve numeric-looking publisherId as a string"
);
temp_env::with_var(
origin_key,
Some("https://origin.test-publisher.com"),
|| {
temp_env::with_var(
env_key,
Some(
r#"[{"when":{"bidder":"kargo","zone":"header"},"set":{"placementId":"server-header","keep":"yes"}}]"#,
),
|| {
let settings = Settings::from_toml_and_env(&toml_str)
.expect("Settings should parse canonical bidder param override rules");
let cfg = settings
.integration_config::<PrebidIntegrationConfig>("prebid")
.expect("Prebid config query should succeed")
.expect("Prebid config should exist with env override");
let cfg_json =
serde_json::to_value(&cfg).expect("should serialize config to JSON");

assert_eq!(
cfg_json["bid_param_override_rules"][0]["when"]["bidder"],
json!("kargo"),
"should deserialize bidder matcher from env JSON"
);
assert_eq!(
cfg_json["bid_param_override_rules"][0]["when"]["zone"],
json!("header"),
"should deserialize zone matcher from env JSON"
);
assert_eq!(
cfg_json["bid_param_override_rules"][0]["set"]["placementId"],
json!("server-header"),
"should deserialize set object from env JSON"
);
},
);
},
assert_eq!(
raw["bid_param_overrides"]["pubmatic"]["adSlot"],
json!("67890"),
"should preserve numeric-looking adSlot as a string"
);
assert_eq!(
raw["bid_param_zone_overrides"]["pubmatic"]["header"]["placementId"],
json!("24680"),
"should preserve numeric-looking zone override parameters as strings"
);
assert_eq!(
raw["bid_param_override_rules"][0]["set"]["placementId"],
json!("13579"),
"should preserve numeric-looking rule parameters as strings"
);
}

Expand Down
Loading