fix(client): preserve canonical format options in get_products - #1006
Conversation
There was a problem hiding this comment.
Clean fix. Right shape: the canonical GetProductsResponse is the primary read surface, so it must parse before the legacy compatibility model — not after it has already silently stripped format_kind/params.
Real bug. The old _canonicalize_get_products_result routed every response through LegacyGetProductsResponse first, whose generated ProductFormatDeclaration lacks the canonical fields, so canonical-only products were discarded and re-emitted as FORMAT_PROJECTION_FAILED. Live regression in the PR body tells the story: v7.0.0-rc turned 2 wire products into 0 with four projection errors; this branch preserves both plus their image declarations.
Things I checked
- No fall-through regression for legacy
format_ids. The publicGetProductsResponse(src/adcp/types/_eager.py:732→canonical_creative.py:631) inherits_reject_legacy_creative_identity, amode="before"validator that raises onformat_id/format_ids/v1_format_refat any depth (canonical_creative.py:323). So a legacy-only or dual-emission product fails whole-response canonical validation deterministically →canonical_result.successis False → falls through toproject_legacy_product.test_get_products_still_projects_legacy_format_idspasses because canonical parse fails, which is the intended mechanism, not a coincidence. - No mixed-response data loss. Whole-response
model_validateis atomic — one legacy product routes the entire set through projection.format_idsis never silently stripped on input; presence raises.ad-tech-protocol-expert: sound — "the failure mode you'd worry about is structurally impossible." - Control flow at
client.py:1514. Checkingraw_result.successinstead oflegacy_result.successis equivalent:_parse_responseshort-circuits on a failed/empty raw result before parsing (protocols/base.py:114), socanonical_resultin that branch is byte-for-byte the oldlegacy_resultreturn. - In-place
canonical_result.metadata = metadata(client.py:1521) is safe —TaskResultis not frozen and has novalidate_assignment; the object is freshly built by_parse_response. - Diagnostics envelope parity.
{"diagnostics": []}on the direct path is accurate — a fully canonical response required zero projection. No diagnostics are lost: any malformedformat_optionsentry fails canonical parse and the legacy path emitsFORMAT_PROJECTION_FAILEDas before. - Semver signal. Private helper; public
get_products()behavior only restores dropped data.fix(client):is correct — no!needed. - Test plan:
make test6,200 passed,make typecheck-allpassed,make lintpassed, Bandit passed.code-reviewer: no blockers.ad-tech-protocol-expert: sound.
Follow-ups (non-blocking — file as issues)
- Response-level all-or-nothing fallback. One legacy or malformed product in an otherwise-canonical response routes all products through projection. Functionally a superset handler, but a clean product's direct-vs-projection path now depends on its siblings. Worth a comment near
client.py:1508documenting that the fast path is reachable only for fully-canonical responses.
Minor nits (non-blocking)
- Double parse on the legacy path.
client.py:1509now runs a full canonical parse that always fails before the legacy parse for legacy-only sellers. Negligible, but notable for high-QPS wholesale-feed enumeration.
Approving on the strength of the structural fall-through guarantee plus the live regression.
Closes #1005.
What changed
get_productsresponses through the canonicalGetProductsResponsemodel first.format_idsfallback.Root cause
_canonicalize_get_products_result()parsed every response throughLegacyGetProductsResponsebefore projection. The generated legacyProductFormatDeclarationdoes not declareformat_kindorparams, so Pydantic discarded those canonical fields beforeproject_legacy_product()saw them. Valid canonical-only products were then omitted withFORMAT_PROJECTION_FAILEDdiagnostics.Impact
Python buyers using the primary
ADCPClient.get_products()API now retain canonicalformat_options. Legacy-only sellers continue through the compatibility projector.Validation
make test: 6,200 passed, 41 skipped, 1 xfailed; 84.18% coveragemake typecheck-all: passed (932 SDK source files plus strict adopter fixtures)make lint: passedv7.0.0-rc: 2 wire products became 0 with four projection errorsimagedeclarations were preserved