Skip to content

Preserve integration configuration string types - #933

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

Preserve integration configuration string types#933
ChristianPavilonis wants to merge 7 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

@prk-Jr prk-Jr 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

Deletes IntegrationSettings::normalize / normalize_env_value and its call in finalize_deserialized, so integration config values are no longer recursively re-parsed as JSON. This is the right fix for #932: the recursive re-parse ran on every load path (from_toml and from_json_value, via finalize_deserialized), so a TOML publisherId = "12345" was silently rewritten to the number 12345 before it ever reached PrebidIntegrationConfig.

Verified the removal is safe for production paths:

  • Settings::from_toml_and_env — the only source that ever produced JSON-encoded strings for integration values — is already #[cfg(test)]-gated and has no callers outside settings.rs tests.
  • The live path is ts config pushBlobEnvelopesettings_from_config_blobfrom_json_value. EdgeZero's app-config env overlay (edgezero-core coerce_env_value, v0.0.4) coerces each env value to the existing TOML leaf's type and errors otherwise, so no untyped strings reach IntegrationSettings from the overlay.
  • Quoted scalars now surface loudly rather than silently: validate_settings_for_deployvalidate_enabled_integrations parses every integration at ts config push time, so enabled = "true" fails there instead of coercing.

Local verification (native host target): cargo test -p trusted-server-core --lib integrations::prebid → 143 passed; --lib settings → 118 passed; --lib config → 99 passed.

Approving — no blocking findings.

Non-blocking

📌 out of scope

  • Env-override docs still advertise the removed behavior: docs/guide/configuration.md:142 (BIDDERS='["kargo","rubicon"]') and docs/guide/configuration.md:1071-1073 (BID_PARAM_OVERRIDES, BID_PARAM_ZONE_OVERRIDES, BID_PARAM_OVERRIDE_RULES as JSON strings) document exactly the JSON-string decoding this PR deletes. The PR removes the identical examples from the rustdoc on PrebidIntegrationConfig, so the two now disagree. Acknowledged and tracked in #972 — noting it only so the follow-up isn't lost, since an operator copying those lines today gets a config that no longer applies.

♻️ refactor

  • No test locks in the new failure behavior: the CHANGELOG entry claims "quoted numeric and boolean scalars now fail validation instead of silently converting", but nothing asserts it. The deleted test_env_var_roundtrip_normalizes_integration_types was the only test that pinned the old direction, and it went away without a replacement pinning the new one. A small test in settings.rs — TOML with [integrations.testlight] enabled = "true", asserting integration_config::<TestlightConfig> (or validate_settings_for_deploy) returns a configuration error — would keep a future refactor from quietly reintroducing coercion.

📝 note

  • Upgrade ordering deserves a CHANGELOG line: the breaking-change entry tells operators to audit [integrations.*], but the already-pushed app_config blob was produced by the old CLI and still has the coerced values baked in (publisherId stored as a number). Deploying the new WASM without re-running ts config push leaves #932 unfixed with no error anywhere. Worth one sentence: re-push config after upgrading.

🌱 seedling

  • from_toml_and_env is now a test-only fixture with no production analogue (crates/trusted-server-core/src/settings.rs:1995): with normalize gone it exercises config-crate Environment semantics that no runtime path uses, while the real overlay lives in edgezero-core. The remaining tests built on it (test_handlers_override_with_env, the ec/partner env tests) are now testing a legacy helper rather than shipped behavior. Not for this PR, but the helper looks ready to retire.

👍 praise

  • Regression test targets the real path (crates/trusted-server-core/src/integrations/prebid.rs:5722): asserting through serde_json::to_valuefrom_json_value mirrors the actual BlobEnvelope transition rather than only from_toml, and it covers all three override surfaces (static, zone, canonical rule) in one pass. That is the version of this test that would have caught #932.

CI Status

  • fmt: PASS
  • clippy: PASS (covered by the per-adapter check/build jobs)
  • rust tests: PASS (cargo test, axum native, cloudflare native + wasm32-unknown-unknown, spin native + wasm32-wasip1, ts CLI native, cross-adapter parity)
  • js tests: PASS (vitest, format-typescript, format-docs)
  • prepare integration artifacts: still pending at review time — worth a glance before merge.

}

#[test]
fn bid_param_override_numeric_strings_survive_runtime_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.

👍 praise — This is the right shape for the regression test. Going through serde_json::to_valueSettings::from_json_value mirrors the real BlobEnvelope / settings_from_config_blob transition instead of only exercising from_toml, and it covers all three override surfaces (static bid_param_overrides, bid_param_zone_overrides, and a canonical bid_param_override_rules entry) in one pass. This is the version of the test that would have caught #932.

validation_label: &str,
) -> Result<Self, Report<TrustedServerError>> {
settings.integrations.normalize();
settings.proxy.normalize();

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.

📝 note — Confirmed this is safe to drop rather than move: from_toml_and_env (the only source that produced JSON-encoded strings for integration entries) is already #[cfg(test)]-gated with no callers outside this file's tests, and EdgeZero's app-config env overlay coerces each env value to the existing TOML leaf's type (edgezero-core coerce_env_value, v0.0.4) rather than injecting raw strings. So after this change no production path can hand IntegrationSettings an untyped scalar.

One follow-on worth a CHANGELOG sentence: the already-pushed app_config blob was serialized by the old CLI and still has the coerced values baked in (publisherId stored as a number). Deploying the new WASM without re-running ts config push leaves #932 unfixed and silent.

@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

Re-review of the reworked PR. All findings from the prior CHANGES_REQUESTED review are resolved:

  • The global JSON re-interpretation (normalize / normalize_env_value) is deleted, not shimmed; no production path called it.
  • The regression test now runs the full production shape — TOML → Settings → runtime JSON → typed PrebidIntegrationConfig → compiled-rule OpenRTB emission — and asserts numeric-looking strings survive static, zone, and canonical-rule merges. Ported onto main it fails with Number(12345) vs String("12345"), so it is a genuine regression test.
  • CHANGELOG documents the breaking config change.
  • Deleting the two datadome env tests is not a coverage loss — protection_scope.rs retains 8 dedicated tests.

Approving. Two non-blocking notes inline.

CI Status

Locally reproduced against a worktree of the head commit:

  • cargo fmt --all -- --check: PASS
  • cargo clippy-axum: PASS
  • cargo test -p trusted-server-core: PASS (1729 passed, 0 failed)

GitHub checks green except two still-running at review time (cargo test (axum native), prepare integration artifacts).

Comment thread CHANGELOG.md
### Changed

- **Breaking** — `bid_param_zone_overrides` inner values must now be JSON objects; previously non-object or empty values (`"header" = "x"`, `"header" = {}`) were accepted and silently produced a dead rule at runtime. They now fail at startup with a configuration error. Operators upgrading should audit their `bid_param_zone_overrides` config for non-object zone entries.
- **Breaking** — Integration configuration strings are no longer globally reinterpreted as JSON scalars. Operators upgrading should audit `[integrations.*]` settings and use native TOML/typed-config booleans and numbers (for example, `enabled = true`, not `enabled = "true"`); quoted numeric and boolean scalars now fail validation instead of silently converting.

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.

📝 note — Accurate for typed fields, but the sentence "quoted numeric and boolean scalars now fail validation" could be misread as applying to the free-form bidder-param maps too. Those are the opposite: bid_param_overrides / bid_param_zone_overrides / rule set values are serde_json::Map<String, Json>, so publisherId = "12345" is now preserved as a string rather than failing — which is the whole fix. Consider a trailing clause, e.g. "… free-form bidder-param override values are preserved verbatim (a quoted "12345" stays a string)." Non-blocking.

/// 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>>,

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.

📌 out of scope — Removing these TRUSTED_SERVER__INTEGRATIONS__PREBID__BID_PARAM_OVERRIDES='{...}' doc examples is correct, but equivalent env-var override examples still live in the guide docs and would now mislead operators: 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 EdgeZero's overlay rejects). Worth a follow-up issue rather than expanding this PR.

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

3 participants