Uh oh!
There was an error while loading. Please reload this page.
Support per-section GAM ad unit paths via gam_unit_path templates - #957
Conversation
- Remove now-dead resolved_gam_unit_path (superseded by
render_gam_unit_path) and its two tests; the default
/<network_id>/<slot_id> path now lives solely in render_gam_unit_path
- Tighten compile_unit_template and render_gam_unit_path to pub(crate)
- Document that validate_runtime must run after compile_unit_templates
for the {section} -> section_root check to fire
- Use fictional gam_network_id 99999 in the new build_slot_json test…nto 954-per-section-gam-unit-path
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
The implementation is generally well-tested and CI is green, but the current change has important configuration-compatibility and inventory-routing risks. In particular, config blobs can break deployment rollback, the template cache can silently discard explicit GAM paths outside the finalized-settings path, and the section derivation policy remains hardcoded despite the originating requirement.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Resolve the review findings on the gam_unit_path templating PR.
- Make section derivation fully publisher-configurable with
`section_segment` (0-based, default 0), so a locale-prefixed site
(/en/news/article) selects `news` rather than `en`. Paths with no
segment at that index fall back to `section_root`. Adds
`CreativeOpportunitiesConfig::section_for_path` so a call site cannot
apply one policy knob and forget the other.
- Skip serializing `section_root` and `section_segment` when unset.
`ts config push` re-serializes the typed config into the pushed blob,
and these structs use `deny_unknown_fields`, so emitting a null key
would make a binary rollback fail at config load. Comment both keys
out of the example config for the same reason.
- Stop conflating "no template" with "not compiled". `compiled_unit:
None` also covers a slot deserialized or built without
`compile_unit_templates`, and treating it as absent silently routed
such slots to /<network_id>/<slot_id> — bidding against the wrong
inventory. `render_gam_unit_path` now falls back to the raw template,
and `template_uses_section` reads it too so validation cannot skip the
`section_root` requirement.
- Reject a blank `gam_network_id` at startup when slots are configured,
closing the case where `gam_unit_path = "{network_id}"` passes
validation and renders an empty path into googletag.defineSlot. An
empty slot list disables the feature, so the id stays unchecked there.
- Fix the documented page_patterns: `/news/*` does not match `/news`, so
the example lost every section landing page it claimed to serve.
`resolved_gam_unit_path` is deliberately not restored as a deprecated
wrapper: it is the path-independent resolver this issue filed as the
bug, so a shim would silently return the untemplated path. The crate is
`publish = false`; `render_gam_unit_path` and `derive_section` are now
`pub` as the supported replacement, making a build break the failure
mode instead of wrong inventory.
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
Reviewed the current head after the previous feedback round. The request-path threading, parser, sanitization, initial/SPA consistency, and prior fixes are generally sound, and CI is green. I found one high rollback-compatibility risk, two medium correctness/resource risks, and one low documentation inconsistency; details are inline.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
aram356
left a comment
There was a problem hiding this comment.
Summary
Reviewed the current head against main. The core design — startup-compiled templates, fail-loud validation, and the raw-path no-decode contract — is sound, and the initial-render and SPA paths cannot diverge: both share build_slot_json, and the SPA path parameter round-trips percent-encoding correctly (encodeURIComponent → form_urlencoded decode) before the same normalization. Two documentation corrections are required (both cheap): the placeholder table still describes pre-section_segment behavior, and the rollback note affirmatively claims a clean rollback for configs that would silently render placeholder braces literally on an older binary. Six non-blocking suggestions are inline.
CI Status
- fmt: PASS
- clippy (all targets): PASS
- rust tests (fastly/axum/cloudflare/spin/parity/CLI): PASS
- js tests (vitest): PASS
- integration + browser tests, CodeQL: PASS
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
aram356
left a comment
There was a problem hiding this comment.
Summary
Re-reviewed the head after the previous feedback round. All prior findings are addressed with regression tests: the rollback marker (section_segment materialized during typed/startup finalization) is verified against the actual ts config push path (TrustedServerAppConfig deserialize → finalize_deserialized → prepare_runtime → compile_unit_templates) and pinned by the legacy-schema tests; network-id validation is scoped to consumption; dynamic rendering is byte-bounded with startup validation of configured values and byte-boundary tests; the case-preservation guidance matches Google's documentation. Three non-blocking suggestions are inline.
CI Status
All checks PASS on 52630418b (fmt, clippy targets, all adapter tests, parity, vitest, integration + browser tests, CodeQL).
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Brings in the squashed versions of PR #957 (per-section gam_unit_path templates) and PR #967 (decoupled prebid tsjs shim). rc/july already carried pre-squash versions of both feature branches, so every conflict was the same feature at two different review stages. Conflict resolution: - creative_opportunities.rs, publisher.rs, settings.rs, docs/guide/configuration.md, CHANGELOG.md — took main's refined gam_unit_path work: the 100-byte dynamic render cap (MAX_DYNAMIC_GAM_UNIT_PATH_BYTES), Option-returning render_gam_unit_path/build_slot_json, match_renderable_slots, the narrowed gam_network_id requirement (only when a default path or {network_id} template consumes it), section_segment finalization marker, and the accompanying tests. - creative_opportunities.rs validate_runtime — kept rc/july's providers.aps compatibility warning on top of main's validation body. - prebid index.ts and its test — kept rc/july's render-trace and APS renderer additions (installPrebidRenderTrace, apsRenderer fixture, extra mocks); the rest matches main. Verified: cargo fmt, all six clippy targets, test-fastly / test-axum / test-cloudflare / test-spin, the parity suite, JS vitest, and JS + docs prettier checks all pass.
Only the CHANGELOG Added list conflicted: main's #957 entry landed where this branch had rewritten the creative-processing entry for the restored rewrite_creatives default. Both entries are kept.
Summary
creative_opportunities.slot.gam_unit_pathis now a template with{network_id},{section}, and{slot_id}placeholders, so a publisher whose GAM ad unit varies by site section expresses that in one slot rule instead of one rule per (slot × section).{section}is derived from the request path (first segment, sanitized), with a requiredsection_rootfor/— the URL→section convention lives in config, not core.gam_unit_pathreachesgoogletag.defineSlotclient-side but is not in the OpenRTB request, so the wire shape is unchanged and there is no JS change.Changes
crates/trusted-server-core/src/creative_opportunities.rsUnitTemplatePart,parse_unit_template,sanitize_section,derive_section; slotcompiled_unitcache +compile_unit_template/render_gam_unit_path/template_uses_section; configsection_root+compile_unit_templates;validate_runtimenow enforcessection_rootwhen{section}is used and drops the render-time emptiness checkcrates/trusted-server-core/src/publisher.rsbuild_slot_json/build_ad_slots_scripttakerequest_path; render the template on both the initial-render andhandle_page_bids(SPA) pathscrates/trusted-server-core/src/settings.rsprepare_runtimeparses templates viacompile_unit_templatesbefore validation, surfacing parse errors asConfigurationerrorsdocs/guide/configuration.md{section}derivation, the no-decode rule, and unmatched-route behaviortrusted-server.example.tomlsection_rootand a templated-slot exampledocs/superpowers/specs/,docs/superpowers/plans/Closes
Closes#954
Test plan
cargo test-fastly && cargo test-axum(alsotest-cloudflare,test-spin— all pass)cargo fmt --all -- --checkcargo clippy-fastly(remaining clippy targets left to CI)cd docs && npm run formatclippy-axum/clippy-cloudflare*/clippy-spin*— left to CINew tests: template parsing (unknown placeholder, unmatched/nested brace, empty), section derivation (
/, single/multi-segment, unsafe/undecoded segmentnew%20s→new_20s),render_gam_unit_path(template, default, verbatim), startup validation (missing/invalidsection_root, parse error), and initial-render/SPA equivalence for the same path.Hardening note
Template parsing and
section_rootvalidation are config-derived and run at startup inSettings::prepare_runtime. Invalid enabled config fails startup viaReport<TrustedServerError::Configuration>— nopanic!,unwrap(), orexpect()on the config path.compile_unit_templatesrejects malformed templates (unknown placeholder, unmatched/nested brace, empty);validate_runtimerejects a{section}template with a missing or non-[A-Za-z0-9_-]section_root. Regression coverage:compile_unit_templates_surfaces_parse_error,validate_runtime_requires_section_root_when_template_uses_section,validate_runtime_rejects_invalid_section_root, andsettings_rejects_creative_opportunity_slot_with_empty_gam_unit_path. Existing staticgam_unit_pathconfigs are unaffected (verbatim when no placeholders;/{network_id}/{slot_id}when absent).Checklist
unwrap()in production code — useexpect("should ...")logmacros (notprintln!)