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
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.
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
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.
aram356
left a comment
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
aram356
left a comment
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).
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.
3 participants
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!)