-
Notifications
You must be signed in to change notification settings - Fork 12
Add a server-side ad template switch and cache policy #1008
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
56083e3
d7bfa92
e9a55d4
03e429a
38c9636
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -587,10 +587,11 @@ mod tests { | |
| use crate::consent::types::ConsentContext; | ||
| use crate::openrtb::Uid; | ||
| use crate::platform::test_support::{ | ||
| NoopBackend, NoopConfigStore, NoopGeo, NoopHttpClient, NoopSecretStore, noop_services, | ||
| NoopBackend, NoopConfigStore, NoopGeo, NoopHttpClient, NoopSecretStore, StubHttpClient, | ||
| noop_services, | ||
| }; | ||
| use crate::platform::{ClientInfo, PlatformResponse}; | ||
| use crate::test_support::tests::create_test_settings; | ||
| use crate::platform::{ClientInfo, PlatformHttpClient, PlatformHttpRequest, PlatformResponse}; | ||
| use crate::test_support::tests::{crate_test_settings_str, create_test_settings}; | ||
| use base64::Engine as _; | ||
| use base64::engine::general_purpose::STANDARD as BASE64; | ||
| use serde_json::json; | ||
|
|
@@ -675,6 +676,124 @@ mod tests { | |
| } | ||
| } | ||
|
|
||
| /// Provider used to prove that direct `/auction` remains available when | ||
| /// publisher server-side ad templates are disabled. | ||
| struct TemplateSwitchProbeProvider { | ||
| calls: Arc<Mutex<usize>>, | ||
| } | ||
|
|
||
| #[async_trait::async_trait(?Send)] | ||
| impl AuctionProvider for TemplateSwitchProbeProvider { | ||
| fn provider_name(&self) -> &'static str { | ||
| "template_switch_probe" | ||
| } | ||
|
|
||
| async fn request_bids( | ||
| &self, | ||
| _request: &AuctionRequest, | ||
| context: &AuctionContext<'_>, | ||
| ) -> Result<ProviderRequestOutcome, Report<TrustedServerError>> { | ||
| *self.calls.lock().expect("should lock provider call count") += 1; | ||
| let request = Request::builder() | ||
| .method("POST") | ||
| .uri("https://bidder.example/auction") | ||
| .body(EdgeBody::empty()) | ||
| .expect("should build probe provider request"); | ||
| context | ||
| .services | ||
| .http_client() | ||
| .send_async(PlatformHttpRequest::new( | ||
| request, | ||
| "template-switch-probe-backend", | ||
| )) | ||
| .await | ||
| .change_context(TrustedServerError::Auction { | ||
| message: "probe provider launch failed".to_string(), | ||
| }) | ||
| .map(ProviderRequestOutcome::pending) | ||
| } | ||
|
|
||
| async fn parse_response( | ||
| &self, | ||
| _response: PlatformResponse, | ||
| _response_time_ms: u64, | ||
| ) -> Result<AuctionResponse, Report<TrustedServerError>> { | ||
| Ok(AuctionResponse::success( | ||
| self.provider_name(), | ||
| Vec::new(), | ||
| 0, | ||
| )) | ||
| } | ||
|
|
||
| fn timeout_ms(&self) -> u32 { | ||
| 100 | ||
| } | ||
|
|
||
| fn backend_name(&self, _services: &RuntimeServices, _timeout_ms: u32) -> Option<String> { | ||
| Some("template-switch-probe-backend".to_string()) | ||
| } | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn direct_auction_remains_available_when_templates_are_disabled() { | ||
|
ChristianPavilonis marked this conversation as resolved.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 praise — this test proves the actual premise of the PR. The whole justification for a dedicated switch instead of reusing |
||
| let settings_toml = format!( | ||
| "{}\n[auction]\nenabled = true\nproviders = [\"template_switch_probe\"]\n\n[creative_opportunities]\nenabled = false\ngam_network_id = \"12345\"\n", | ||
| crate_test_settings_str() | ||
| ); | ||
| let settings = Settings::from_toml(&settings_toml) | ||
| .expect("should parse settings with disabled templates"); | ||
| let calls = Arc::new(Mutex::new(0)); | ||
| let mut orchestrator = AuctionOrchestrator::new(settings.auction.clone()); | ||
| orchestrator.register_provider(Arc::new(TemplateSwitchProbeProvider { | ||
| calls: Arc::clone(&calls), | ||
| })); | ||
|
|
||
| let stub = Arc::new(StubHttpClient::new()); | ||
| stub.push_response(200, b"probe response".to_vec()); | ||
| let services = RuntimeServices::builder() | ||
| .config_store(Arc::new(NoopConfigStore)) | ||
| .secret_store(Arc::new(NoopSecretStore)) | ||
| .kv_store(Arc::new(edgezero_core::key_value_store::NoopKvStore)) | ||
| .backend(Arc::new(NoopBackend)) | ||
| .http_client(Arc::clone(&stub) as Arc<dyn PlatformHttpClient>) | ||
| .geo(Arc::new(NoopGeo)) | ||
| .client_info(ClientInfo::default()) | ||
| .build(); | ||
| let ec_context = make_ec_context(Jurisdiction::NonRegulated, None); | ||
| let body = json!({ | ||
| "adUnits": [{ | ||
| "code": "div-gpt-ad-1", | ||
| "mediaTypes": { "banner": { "sizes": [[300, 250]] } } | ||
| }] | ||
| }); | ||
| let req = Request::builder() | ||
| .method("POST") | ||
| .uri("https://test-publisher.com/auction") | ||
| .body(EdgeBody::from( | ||
| serde_json::to_vec(&body).expect("should serialize body"), | ||
| )) | ||
| .expect("should build auction request"); | ||
|
|
||
| let response = handle_auction( | ||
| &settings, | ||
| &orchestrator, | ||
| None, | ||
| None, | ||
| &ec_context, | ||
| &services, | ||
| req, | ||
| ) | ||
| .await | ||
| .expect("direct auction should remain available"); | ||
|
|
||
| assert_eq!( | ||
| *calls.lock().expect("should lock provider call count"), | ||
| 1, | ||
| "disabling publisher templates must not disable direct /auction" | ||
| ); | ||
| assert_eq!(response.status(), StatusCode::OK); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn auction_endpoint_consent_gate_returns_no_bid_without_contacting_providers() { | ||
| // GDPR/unknown jurisdiction lacking effective TCF Purpose 1 must not run | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -183,10 +183,27 @@ fn derive_section(path: &str, section_root: &str, section_segment: usize) -> Str | |
| } | ||
| } | ||
|
|
||
| const fn default_enabled() -> bool { | ||
| true | ||
| } | ||
|
|
||
| const fn is_default_enabled(value: &bool) -> bool { | ||
| *value == default_enabled() | ||
| } | ||
|
|
||
| /// Top-level configuration for the creative opportunities system. | ||
| #[derive(Debug, Clone, Deserialize, Serialize)] | ||
| #[serde(deny_unknown_fields)] | ||
| pub struct CreativeOpportunitiesConfig { | ||
| /// Enables server-side ad template delivery on publisher HTML and page-bids requests. | ||
| /// | ||
| /// This does not disable the direct `POST /auction` endpoint. The default is | ||
| /// `true` so existing creative-opportunity configurations retain their behavior. | ||
| #[serde( | ||
| default = "default_enabled", | ||
| skip_serializing_if = "is_default_enabled" | ||
|
ChristianPavilonis marked this conversation as resolved.
|
||
| )] | ||
| pub enabled: bool, | ||
|
ChristianPavilonis marked this conversation as resolved.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤔 thinking — the rollback failure mode is guarded only by documentation. Because 👍 for Worth considering a mechanical guard to go with the prose: have |
||
| /// GAM network ID used to build default unit paths. | ||
| pub gam_network_id: String, | ||
| /// Maximum time in milliseconds to wait for the server-side auction before | ||
|
|
@@ -244,7 +261,7 @@ pub struct CreativeOpportunitiesConfig { | |
| /// [`section_root`](Self::section_root) are omitted. | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub section_segment: Option<usize>, | ||
| /// Slot templates. Empty vec = feature disabled (no auction fired, no globals injected). | ||
| /// Slot templates. An empty vec or `enabled = false` disables template delivery. | ||
| #[serde(default, deserialize_with = "vec_from_seq_or_map")] | ||
| pub slot: Vec<CreativeOpportunitySlot>, | ||
| } | ||
|
|
@@ -1143,12 +1160,39 @@ mod tests { | |
| assert_eq!(derive_section("/%%%/x", "home", 0), "_"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn enabled_defaults_true_and_is_omitted_from_serialized_config() { | ||
| let config = make_config_with_section_template(None); | ||
| assert!( | ||
| config.enabled, | ||
| "template delivery should default to enabled" | ||
| ); | ||
| let value = serde_json::to_value(&config).expect("should serialize config"); | ||
| assert!( | ||
| value.get("enabled").is_none(), | ||
| "default enabled value should be omitted for rollback compatibility" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn disabled_template_switch_is_serialized() { | ||
| let mut config = make_config_with_section_template(None); | ||
| config.enabled = false; | ||
| let value = serde_json::to_value(&config).expect("should serialize config"); | ||
| assert_eq!( | ||
| value.get("enabled"), | ||
| Some(&serde_json::Value::Bool(false)), | ||
| "explicitly disabled template delivery must remain in config blobs" | ||
| ); | ||
| } | ||
|
|
||
| fn make_config_with_section_template( | ||
| section_root: Option<&str>, | ||
| ) -> CreativeOpportunitiesConfig { | ||
| let mut slot = make_slot("ad-header-0", vec!["/news/*"]); | ||
| slot.gam_unit_path = Some("/{network_id}/example/{section}".to_string()); | ||
| CreativeOpportunitiesConfig { | ||
| enabled: true, | ||
| gam_network_id: "99999".to_string(), | ||
| auction_timeout_ms: None, | ||
| price_granularity: PriceGranularity::default(), | ||
|
|
@@ -1546,6 +1590,7 @@ mod tests { | |
| // Older binaries deserialize this struct with `deny_unknown_fields`, so | ||
| // a pushed config blob must not carry `"section_root": null`. | ||
| let config = CreativeOpportunitiesConfig { | ||
| enabled: true, | ||
| gam_network_id: "99999".to_string(), | ||
| auction_timeout_ms: None, | ||
| price_granularity: PriceGranularity::default(), | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⛏ nitpick — this weakens an existing test that the PR does not otherwise touch.
enforce_set_cookie_cache_privacyis unchanged by this PR, but the fixture moved frompublic, max-age=600tomax-age=60, which drops the explicit "origin sent a public policy" scenario the test was written to cover. Both values exercise the same branch, so nothing is caught today — but the named scenario is gone.Fix: keep the original case and add the new one, e.g. loop over
["public, max-age=600", "max-age=60"]so both the origin-public and inactive-template policies are proven to downgrade.