Implement generic Prebid bid param override rules#618
Conversation
Renames the new field to match the existing `bid_param_zone_overrides` naming convention. Updates all references: struct field, env var key, doc comments, TOML example, tests, and .env.example. Also replaces production IDs in test fixtures and examples with generic placeholder values.
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Well-designed feature. The BidParamOverrideEngine with its canonical rule format and compatibility normalization from bid_param_overrides/bid_param_zone_overrides is a clean architecture. Good validation at startup via try_from_config and json_object_for_override. Test coverage is thorough — especially the engine unit tests and precedence ordering tests. A few suggestions below.
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Well-designed PR — the unified rule engine is clean, well-tested, and the compatibility migration strategy is sound. Approving with two recommended fixes.
aram356
left a comment
There was a problem hiding this comment.
Summary
Replaces the split bid-param override paths with a single generic BidParamOverrideEngine that normalizes all three config surfaces into an ordered rule list at startup. Clean design, solid compatibility story, comprehensive tests.
Non-blocking
♻️ refactor
- Redundant validation-only engine build in
PrebidIntegration::try_new: builds and discards the engine just for validation — same work is repeated inPrebidAuctionProvider::try_new(prebid.rs:233)
🤔 thinking
deny_unknown_fieldsonBidParamOverrideWhen: strict is good, but adding new matcher fields later becomes a breaking config change (prebid.rs:189)- Shallow merge semantics vs. nested override values: real-world bidder params can be nested objects — shallow merge replaces the entire value rather than deep-merging (prebid.rs:504)
⛏ nitpick
parse_prebid_toml_resulterror message: "should be enabled" is misleading whenNonecould also mean the section is missing entirely (prebid.rs:1627)
🌱 seedling
- Zone-only rules (no bidder): the engine requires at least one matcher, so "for all bidders in zone X" rules aren't possible yet — could be useful for zone-wide floor overrides
CI Status
- fmt: PASS
- clippy: PASS
- cargo test: PASS
- vitest: PASS
- integration tests: PASS
- browser integration tests: PASS
aram356
left a comment
There was a problem hiding this comment.
Summary
Adds a canonical ordered bid_param_override_rules engine and normalizes the two compatibility surfaces (bid_param_overrides, bid_param_zone_overrides) into the same runtime engine. Architecture is sound, validation fails startup fast, and the test coverage for parsing, determinism, and application order is strong. The blocker is a spec/impl/docs contradiction on merge semantics that the PR ships consistently wrong across six places.
Blocking
🔧 wrench
- Deep merge contradicts the design spec's explicit non-goal:
merge_bidder_param_objectincrates/trusted-server-core/src/integrations/prebid.rs:509recurses into nested objects, but the spec (docs/superpowers/specs/2026-04-08-prebid-generic-bid-param-override-rules-design.mdline 32 goal, line 40 non-goal, line 81 rule semantics) and the plan (docs/superpowers/plans/2026-04-08-prebid-generic-bid-param-override-rules.md:7) both explicitly say shallow. The rustdoc onBidParamOverrideRuleand the unit testbidder_param_override_deep_merges_nested_objectssay deep. Inline fix suggestion on the helper. - Silent behavior change for existing
bid_param_zone_overrides: the pre-PR code at the previous apply site didbase.extend(...)(shallow). This PR routes the same config through the recursive helper, so any deployment with a nested-object zone override gets a different outgoing request body with no opt-out. Compatibility sugar must preserve runtime semantics. Inline comment atcrates/trusted-server-core/src/integrations/prebid.rs:862. trusted-server.toml:66describes semantics incorrectly: ships in the operator template as "shallow last-write-wins merge" while the code is recursive. Inline comment on the file.docs/guide/configuration.mddescribes merge as shallow in both the table rows (lines 706-707) and the Bid Param Override Surfaces bullets (lines 774-775). Inline comment.docs/guide/integrations/prebid.mddescribes merge as shallow across the table rows (58-59), the TOML example comments (33, 38), thebid_param_overridesbehavior bullets (170), thebid_param_zone_overridesbehavior bullets (198), and the newBid Param Override Rulessection (234). Inline comment on the canonical-rules section.
The resolution is to pick one answer (shallow or deep) and make all six places agree. My recommendation is shallow, because (a) it matches the spec and plan, (b) it preserves existing bid_param_zone_overrides behavior with no silent break, and (c) deep merge is an operator footgun — a rule like set = { keywords = { genre = "news" } } will silently leak stale client-side keys (keywords.sport = "football") through to PBS, which is rarely what an operator intends when they write an override. If deep merge is the intentional choice, the spec, plan, trusted-server.toml:66, both docs pages, the PR description, and a prominent callout of the keywords footgun all need to be updated as part of this PR — the status quo of "code says deep, everything else says shallow" is not acceptable to ship.
Non-blocking
♻️ refactor
- Engine compiled twice on registration (
crates/trusted-server-core/src/integrations/prebid.rs:232-238):PrebidIntegration::try_newbuildsBidParamOverrideEngine, drops it, thenPrebidAuctionProvider::try_newrebuilds it from the same config. The comment in the code already acknowledges this. Cleanest fix is to cache the compiled engine onPrebidIntegrationand thread it intoPrebidAuctionProvider::try_new, so validation and runtime share one instance and can't drift. Alternatively, extract a cheapvalidate_override_config(&config) -> Result<(), …>for the validation-only path. or_insert(Json::Null)placeholder inmerge_bidder_param_object(crates/trusted-server-core/src/integrations/prebid.rs:513): writes aNulland immediately overwrites it on every new key. Aserde_json::map::Entrymatch is cleaner. Skip if you restructure for the shallow-merge fix anyway.
🤔 thinking
- Asymmetric whitespace trimming: matcher strings are
.trim()-ed at compile time (validate_override_matcher_string, line 327), but runtime facts (facts.bidder,facts.zone) are not. Today the runtime facts come from bidder-map keys andtrustedServer.zone, so mismatches are unlikely — but the asymmetry is subtle and worth either dropping the config-side trim (strict equality, "configure what you mean") or explicitly documenting it. Not blocking. - Undocumented case sensitivity: matching is exact and case-sensitive. An operator with
when.bidder = "Kargo"and a runtime bidderkargogets a silently non-matching rule. Recommend a one-line note in the prebid docs. A nice-to-have stretch goal is a startup warning when a rule'swhen.bidderis not inconfig.bidders/client_side_bidders— it catches typos without false positives.
📝 note
- Public API shape change on
PrebidIntegrationConfig.bid_param_zone_overrides(line 115): type went fromHashMap<String, HashMap<String, Json>>toHashMap<String, HashMap<String, serde_json::Map<String, Json>>>. Deserialization is unaffected for any real object-shaped TOML/JSON; it hardens against previously-accepted non-object values that the old runtime silently skipped. Flagging so downstream consumers constructing the struct programmatically are aware.
🌱 seedling
applyis O(bidders × rules) per imp. Fine at today's rule counts, but the rule engine is the natural landing spot for broader override catalogs. If the rule count grows, an index keyed by(bidder, zone)with a wildcard bucket forbidder = Noneturns this into O(matching-rules). Not a concern for this PR.
👍 praise
engine_compiles_compatibility_rules_in_sorted_matcher_orderruns the same config 16 times and asserts the compiled order on each iteration. Exactly the right instinct for catching HashMap iteration non-determinism. Nice defensive test.deny_unknown_fieldsonBidParamOverrideRuleandBidParamOverrideWhen(lines 178-198) turns operator typos likewhen.bidders = "kargo"into hard config errors at startup, which is the right failure mode.- Eager validation via
try_newin bothPrebidIntegrationandPrebidAuctionProviderpropagates config errors throughReport<TrustedServerError>, matching the project rule that invalid enabled config must surface as startup errors rather than be silently disabled.
CI Status
- fmt: PASS
- clippy: PASS
- rust tests: PASS
- vitest: PASS
- format-typescript: PASS
- format-docs: PASS
- integration tests: PASS
- browser integration tests: PASS
- CodeQL: PASS
Replace the recursive deep-merge in merge_bidder_param_object with a
flat shallow merge (top-level keys inserted/replaced, nested objects
replaced wholesale). This aligns the implementation with the design
spec, plan, operator docs, and trusted-server.toml — all of which
described shallow semantics while the code was doing deep merge.
Consequences:
- Existing bid_param_zone_overrides behavior is preserved byte-for-byte;
the previous inline base.extend() was already shallow, and routing
through the engine now produces the same result.
- The operator footgun is eliminated: set = { keywords = { genre = "news" } }
replaces the entire keywords object rather than leaking stale client-side
sub-keys like keywords.sport = "football" through to PBS.
Also addressed in this commit:
- Extract validate_bid_param_override_config() so the validation-only
path in PrebidIntegration::try_new is explicit rather than a let _ =.
- Rename test bidder_param_override_deep_merges_nested_objects to
bidder_param_override_replaces_nested_objects and flip the assertion
on the nested keywords.sport key from "football" to Json::Null.
- Update rustdoc on BidParamOverrideRule and its set field to say
"shallow-merged" with a note that nested objects are replaced wholesale.
- Add a comment above validate_override_matcher_string documenting the
asymmetric whitespace trimming: config strings are trimmed, runtime
facts are not (they come from controlled internal sources).
- Add a case-sensitivity note to the Bid Param Override Rules behavior
list in docs/guide/integrations/prebid.md.
aram356
left a comment
There was a problem hiding this comment.
Summary
Well-scoped refactor: replaces two ad-hoc override paths with one ordered rule engine, introduces a canonical bid_param_override_rules surface, and the shallow-merge revert in eec40e2e correctly restores pre-PR bid_param_zone_overrides semantics. Two concerns remain before this lands cleanly — a user-facing docs section that still describes the old runtime, and an unannounced behavior change for non-object bidder params that slipped through the shallow-merge fix.
Blocking
🔧 wrench
-
Stale implementation bullet lies about runtime behavior —
docs/guide/integrations/prebid.md:374(outside the PR's diff hunks, so inline comment was not possible). Reads "Appliesbid_param_zone_overridestoimp.ext.prebid.bidderbefore request dispatch" after this PR unifies three surfaces through one engine. Suggested replacement:- Applies `bid_param_overrides`, `bid_param_zone_overrides`, and `bid_param_override_rules` via the unified override engine before request dispatch
-
Silent behavior change for non-object bidder params — see inline comment on
crates/trusted-server-core/src/integrations/prebid.rs:518. The newmerge_bidder_param_objectfallback replaces non-objectparamswholesale, whereas the pre-PR zone-override path was a no-op. This contradicts the "byte-for-byte preservation" rationale in theeec40e2ecommit message.
Non-blocking
🤔 thinking
- No integration-level fail-fast test — unit tests verify
CompiledBidParamOverrideRule::try_fromrejects emptywhen/set, but no test drives an invalid override config throughregister()(prebid.rs:357) orregister_auction_provider()(prebid.rs:1484). The comment at prebid.rs:236 explicitly calls out the dual-validation pattern as fail-fast — worth one smoke test per entry-point to pin that promise. Json::Nullvalues insetaccepted silently — see inline comment on prebid.rs:710.
♻️ refactor
- Double-clone of canonical rules during engine compile — see inline comment on prebid.rs:580.
🌱 seedling
- Canonical rule referencing unconfigured bidder is silently ignored —
when.bidder = "misspelled"compiles and never fires, with no log.validate_bid_param_override_configis a natural place to emit a startup warning when a rule'swhen.bidderdoesn't appear inconfig.biddersorconfig.client_side_bidders. Out of scope for this PR.
⛏ nitpick
- Orphan empty table header in
trusted-server.toml— see inline comment on trusted-server.toml:62.
CI Status
- fmt: PASS
- clippy: PASS
- rust tests: PASS
- js tests: PASS
- codeql / integration / browser-integration: PASS
- Revert merge_bidder_param_object to no-op for non-object params, restoring byte-for-byte pre-PR bid_param_zone_overrides behavior - Add TryFrom<&BidParamOverrideRule> to avoid redundant whole-struct clone during engine compile; owned impl now delegates to it - Update stale docs bullet to list all three override surfaces - Document Json::Null behavior in set values in prebid.md behavior list - Comment out orphan empty table header in trusted-server.toml
Summary
bid_param_overridesandbid_param_zone_overridesas compatibility config, while adding canonicalbid_param_override_rulesfor future config-driven overrides.bidder/zonematch with shallow last-write-wins merges.Changes
crates/trusted-server-core/src/integrations/prebid.rsBidParamOverrideRule,BidParamOverrideWhen, andBidParamOverrideEngine; normalize compatibility fields and canonical rules into one runtime path; apply rules during OpenRTB construction; update testscrates/trusted-server-core/src/settings.rsTRUSTED_SERVER__INTEGRATIONS__PREBID__BID_PARAM_OVERRIDE_RULEStrusted-server.tomlbid_param_override_rulesand clarify that compatibility fields normalize into the same enginedocs/superpowers/plans/2026-04-08-prebid-generic-bid-param-override-rules.mdCloses
Closes #617
Related: #339, #383, #384
Test plan
cargo fmt --all -- --checkcargo test -p trusted-server-corecargo clippy -p trusted-server-core --all-targets --all-features -- -D warningscargo test --workspacecargo clippy --workspace --all-targets --all-features -- -D warningsChecklist
logmacros (notprintln!)