Preserve integration configuration 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
prk-Jr
left a comment
There was a problem hiding this comment.
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 outsidesettings.rstests.- The live path is
ts config push→BlobEnvelope→settings_from_config_blob→from_json_value. EdgeZero's app-config env overlay (edgezero-corecoerce_env_value, v0.0.4) coerces each env value to the existing TOML leaf's type and errors otherwise, so no untyped strings reachIntegrationSettingsfrom the overlay. - Quoted scalars now surface loudly rather than silently:
validate_settings_for_deploy→validate_enabled_integrationsparses every integration atts config pushtime, soenabled = "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"]') anddocs/guide/configuration.md:1071-1073(BID_PARAM_OVERRIDES,BID_PARAM_ZONE_OVERRIDES,BID_PARAM_OVERRIDE_RULESas JSON strings) document exactly the JSON-string decoding this PR deletes. The PR removes the identical examples from the rustdoc onPrebidIntegrationConfig, 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_typeswas the only test that pinned the old direction, and it went away without a replacement pinning the new one. A small test insettings.rs— TOML with[integrations.testlight] enabled = "true", assertingintegration_config::<TestlightConfig>(orvalidate_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-pushedapp_configblob was produced by the old CLI and still has the coerced values baked in (publisherIdstored as a number). Deploying the new WASM without re-runningts config pushleaves #932 unfixed with no error anywhere. Worth one sentence: re-push config after upgrading.
🌱 seedling
from_toml_and_envis now a test-only fixture with no production analogue (crates/trusted-server-core/src/settings.rs:1995): withnormalizegone it exercisesconfig-crateEnvironmentsemantics that no runtime path uses, while the real overlay lives inedgezero-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 throughserde_json::to_value→from_json_valuemirrors the actualBlobEnvelopetransition rather than onlyfrom_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() { |
There was a problem hiding this comment.
👍 praise — This is the right shape for the regression test. Going through serde_json::to_value → Settings::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(); |
There was a problem hiding this comment.
📝 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
left a comment
There was a problem hiding this comment.
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 → typedPrebidIntegrationConfig→ compiled-rule OpenRTB emission — and asserts numeric-looking strings survive static, zone, and canonical-rule merges. Ported ontomainit fails withNumber(12345)vsString("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.rsretains 8 dedicated tests.
Approving. Two non-blocking notes inline.
CI Status
Locally reproduced against a worktree of the head commit:
cargo fmt --all -- --check: PASScargo clippy-axum: PASScargo 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).
| ### 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. |
There was a problem hiding this comment.
📝 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>>, |
There was a problem hiding this comment.
📌 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.
Summary
Closes #932
Follow-up: #972 tracks outdated integration environment-override documentation that predates this PR.
Testing
cargo fmt --all -- --checkcargo test_details -p trusted-server-corecargo clippy-fastly && cargo clippy-axum && cargo clippy-cloudflare && cargo clippy-cloudflare-wasm && cargo clippy-spin-native && cargo clippy-spin-wasmcargo test-fastly && cargo test-axum && cargo test-cloudflare && cargo test-spincargo test --manifest-path crates/trusted-server-integration-tests/Cargo.toml --test paritycd crates/trusted-server-js/lib && npx vitest run && npm run format