fix(compat): warn when brand_manifest non-standard path is silently flattened to domain#686
Conversation
…to domain Resolves silent information loss introduced by the v2.5→v3 adapter: a brand_manifest URL like https://cdn.acmecorp.com/brand-manifest.json is correctly translated to BrandReference.domain but the path is dropped, so v3 sellers will attempt https://cdn.acmecorp.com/.well-known/brand.json which typically 404s with no signal to the operator. Both adapter call sites (get_products.adapt_request and _media_buy_helpers.adapt_brand_manifest_to_brand) now emit a logging.WARNING when urlparse detects a non-standard path. The warning is deduplicated per URL via a module-level set so high-RPS buyers do not saturate log pipelines. Bare-domain inputs (no scheme, urlparse returns hostname=None) are guarded against false positives. Fixes #683. https://claude.ai/code/session_01X44xueoHmfHRYeKL8giv2F
274fb93 to
fdfdd1b
Compare
Independent expert review (post-triage) + rebase summarycode-reviewer (fresh): approved — no blockers. Confirmed warning threshold ( ad-tech-protocol-expert: approved. Confirmed:
Rebase notes: Rebased onto current Did not extend the warning to the inline-object's CI: 15/15 green on the rebased branch. Merging. |
Closes #683
Summary
After PR #679, the v2.5→v3 adapter correctly extracts the hostname from
brand_manifestURLs, but silently drops any non-standard path. A buyer hosting athttps://cdn.acmecorp.com/brand-manifest.jsongets translated to{domain: "cdn.acmecorp.com"}, and the v3 seller then fetcheshttps://cdn.acmecorp.com/.well-known/brand.json— which typically 404s with no diagnostic signal in the SDK.This PR adds a
logging.WARNINGat both adapter call sites (get_products.adapt_requestand_media_buy_helpers.adapt_brand_manifest_to_brand) when the input URL carries a non-standard path. No behavior change; pure observability improvement.Design decisions:
extract_brand_domain— the utility is stateless string transformation; diagnostic responsibility belongs to the callers that have request context.parsed.hostname is not None) —urlparsereturnshostname=Nonefor schemeless strings like"acme.com", which would otherwise false-positive sinceurlparse("acme.com").path == "acme.com".What was tested
pytest tests/test_legacy_adapter_v2_5_get_products.py tests/test_legacy_adapter_v2_5_media_buy.py -v— 52 passedruff check src/adcp/compat/— no issuesmypy src/adcp/compat/— no issues in compat files (pre-existing pydantic stubs missing in env; unrelated to this change)New tests cover: CDN URL warns, standard
/.well-known/brand.jsondoes not warn, bare domain does not warn, same URL warns exactly once (dedup).Nits surfaced by pre-PR review (not fixed in this PR)
extract_brand_domain(in_url.py) also callsurlparseinternally, so both call sites do a secondurlparseto read.hostnameand.path. Harmless but redundant — refactoringextract_brand_domainto accept/return aParseResultwould avoid the double call.manifeststring;urlparseis called onmanifest.strip(). A manifest string with surrounding whitespace would escape dedup. Pathological input, no real-world impact.get_products.pyand_media_buy_helpers.py. A_warn_if_nonstandard_brand_manifest_path(manifest, domain)helper in_media_buy_helpers.pywould unify them (dx-expert's main nit).Pre-PR review
Session: https://claude.ai/code/session_01X44xueoHmfHRYeKL8giv2F
Generated by Claude Code