From 22212dc1da91334a8ec253a65be54e3061d7830e Mon Sep 17 00:00:00 2001 From: "Constantine.mirin" Date: Thu, 15 Oct 2026 16:35:42 -0400 Subject: [PATCH] fix(canonical-formats): compare every parameter a format reference declares MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `core/format-id.json` declares four parameters beyond the `(agent_url, id)` identity — `width`, `height`, `duration_ms` and `pixel_ratio`. Three helpers in `compat_helpers.py` each named three of them in a hand-written tuple, so none of them looked at `pixel_ratio`: - `format_is_supported` accepted a creative that claimed no pixel density for a product requiring 2x, and accepted one claiming a conflicting density; - `formats_are_equivalent` called two references with conflicting densities equal; - `_upgrade_legacy_format_id` discarded the `pixel_ratio` it was handed. Derive the set from the model's fields instead of listing it, so a parameter added to the schema is compared without anyone remembering to name it. A fourth literal would only reset the clock: an independent implementation of the same comparison hit this bug from its own hand-written list of the same three names. `pixel_ratio` keeps one deliberate per-field rule, because the schema gives it one: omitting it selects "the backward-compatible 1x variant", so an absent density reads as 1x rather than as unspecified. A product fixed at 2x therefore refuses a silent request while one fixed at 1x still serves it — without that, closing the gap would introduce the mirror-image false rejection. The new parametrized test reads the parameter list from the bundled schema rather than from the model, so it fails when a helper stops comparing something the spec declares. Closes #1517 --- src/adcp/canonical_formats/compat_helpers.py | 50 ++++++++-- tests/test_canonical_formats_compatibility.py | 96 +++++++++++++++++++ 2 files changed, 138 insertions(+), 8 deletions(-) diff --git a/src/adcp/canonical_formats/compat_helpers.py b/src/adcp/canonical_formats/compat_helpers.py index c0fdb70d0..a9a563043 100644 --- a/src/adcp/canonical_formats/compat_helpers.py +++ b/src/adcp/canonical_formats/compat_helpers.py @@ -28,6 +28,27 @@ r"^display_(?P[1-9][0-9]*)x(?P[1-9][0-9]*)(?:_image)?$" ) +#: The parameters a format reference carries beyond its ``(agent_url, id)`` +#: identity, derived from the model instead of listed. +#: +#: A hand-written list here is the defect that dropped ``pixel_ratio``: three +#: of the four parameters were copied by name, so a creative that claimed no +#: pixel density compared equal to a product format that requires 2x, two +#: references declaring *conflicting* densities compared equal, and the +#: legacy-size upgrade discarded the density it was handed. Deriving the set +#: means a parameter added to ``core/format-id.json`` — and so to the +#: generated model — is compared without being named here. +_PARAMETER_FIELDS: tuple[str, ...] = tuple(sorted(set(FormatId.model_fields) - {"agent_url", "id"})) + +#: Parameters the schema gives a value to when they are absent. +#: +#: ``core/format-id.json`` describes omitting ``pixel_ratio`` as selecting +#: "the backward-compatible 1x variant", which is a value, not silence. An +#: omitted width or duration means "template, unparameterized"; an omitted +#: density means 1x. So a product fixed at 2x refuses a request that says +#: nothing, while one fixed at 1x still serves it. +_IMPLIED_WHEN_ABSENT: dict[str, float] = {"pixel_ratio": 1.0} + def _coerce_format_id( value: FormatIdInput, @@ -74,11 +95,14 @@ def _upgrade_legacy_format_id( return FormatId.model_validate( { + # Every parameter the reference carried, then the two the legacy + # size ID itself states. Reparameterizing the ID must not drop a + # parameter the caller supplied alongside it. + **{field: getattr(fid, field) for field in _PARAMETER_FIELDS}, "agent_url": str(fid.agent_url), "id": "display_image", "width": int(match.group("width")), "height": int(match.group("height")), - "duration_ms": fid.duration_ms, } ) @@ -110,8 +134,9 @@ def formats_are_equivalent( Both inputs are first passed through :func:`upgrade_legacy_format_id`. Declared parameters must not conflict, but an omitted parameter on either side is treated as unspecified rather than a mismatch. Use - :func:`format_is_supported` for product/capability gating where a - supported fixed size or duration requires the request to state that value. + :func:`format_is_supported` for product/capability gating, where a + supported fixed size, duration or pixel density requires the request to + state that value. """ left = _upgrade_legacy_format_id(a, default_agent_url=default_agent_url, param="a") right = _upgrade_legacy_format_id(b, default_agent_url=default_agent_url, param="b") @@ -120,7 +145,7 @@ def formats_are_equivalent( if left.id != right.id: return False - for field in ("width", "height", "duration_ms"): + for field in _PARAMETER_FIELDS: left_value = getattr(left, field) right_value = getattr(right, field) if left_value is not None and right_value is not None and left_value != right_value: @@ -139,8 +164,14 @@ def format_is_supported( This is intentionally stricter than :func:`formats_are_equivalent`. A broad supported format such as ``display_image`` accepts a specific request such as ``display_image`` 300x250, but a fixed supported product - format requires the request to provide and match every fixed parameter - (``width``, ``height``, and ``duration_ms``). + format requires the request to provide and match every parameter the + supported side fixes — every parameter ``core/format-id.json`` declares, + not a chosen few. + + ``pixel_ratio`` is the one parameter whose absence carries a value rather + than meaning "unspecified": the schema describes omitting it as selecting + the backward-compatible 1x variant. A product fixed at 2x therefore + refuses a request that omits the field, and one fixed at 1x accepts it. """ req = _upgrade_legacy_format_id( requested, default_agent_url=default_agent_url, param="requested" @@ -151,11 +182,14 @@ def format_is_supported( if not formats_are_equivalent(req, sup, default_agent_url=default_agent_url): return False - for field in ("width", "height", "duration_ms"): + for field in _PARAMETER_FIELDS: supported_value = getattr(sup, field) if supported_value is None: continue - if getattr(req, field) != supported_value: + requested_value = getattr(req, field) + if requested_value is None: + requested_value = _IMPLIED_WHEN_ABSENT.get(field) + if requested_value != supported_value: return False return True diff --git a/tests/test_canonical_formats_compatibility.py b/tests/test_canonical_formats_compatibility.py index ffd1ee144..29ba34857 100644 --- a/tests/test_canonical_formats_compatibility.py +++ b/tests/test_canonical_formats_compatibility.py @@ -16,6 +16,7 @@ ) from adcp.types import FormatCard from adcp.types.legacy import LegacyFormatId as FormatId +from adcp.validation.schema_loader import get_named_schema_document def test_upgrade_legacy_display_size_to_parameterized_canonical_format_id() -> None: @@ -186,3 +187,98 @@ def test_projection_accepts_a_reference_taken_off_a_model_field() -> None: assert projected.diagnostic is None assert projected.declaration is not None + + +def _schema_parameters() -> frozenset[str]: + """The parameters ``core/format-id.json`` declares beyond the identity pair. + + Read from the bundled schema rather than from the generated model, so the + guard below fails when a helper stops comparing a parameter the *spec* + declares — not merely when it disagrees with the model it was written + against. + """ + document = get_named_schema_document("core/format-id.json") + assert document is not None, "core/format-id.json is absent from the bundled schema tree" + return frozenset(document["properties"]) - frozenset(document["required"]) + + +#: One pair of conflicting, dependency-legal values per declared parameter. +#: ``pixel_ratio`` needs ``width``/``height`` present, which ``_BASE`` supplies. +_CONFLICTING_VALUES: dict[str, tuple[object, object]] = { + "width": (300, 728), + "height": (250, 90), + "duration_ms": (15000, 30000), + "pixel_ratio": (2, 3), +} + +_BASE = { + "agent_url": CANONICAL_CREATIVE_AGENT_URL, + "id": "display_image", + "width": 300, + "height": 250, +} + + +def test_the_conflict_table_covers_every_parameter_the_schema_declares() -> None: + """Keeps the parametrized guard below honest as the schema grows. + + A new parameter in ``core/format-id.json`` fails here until it is given a + conflicting pair, which then puts it under the comparison guard. + """ + assert set(_CONFLICTING_VALUES) == set(_schema_parameters()) + + +@pytest.mark.parametrize("parameter", sorted(_CONFLICTING_VALUES)) +def test_the_helpers_refuse_a_conflict_in_every_declared_parameter(parameter: str) -> None: + """Two references that disagree about any declared parameter differ. + + The comparison walks the parameters the model declares, so a parameter + added to the schema is compared without being named here. + """ + supported_value, requested_value = _CONFLICTING_VALUES[parameter] + supported = {**_BASE, parameter: supported_value} + requested = {**_BASE, parameter: requested_value} + + assert not formats_are_equivalent(requested, supported) + assert not format_is_supported(requested, supported) + + +def test_format_is_supported_refuses_a_request_that_omits_a_required_pixel_ratio() -> None: + """A 2x product is not served by a creative that never claims a density. + + ``core/format-id.json`` makes the omitted form the 1x variant ("Omit for + the backward-compatible 1x variant"), so silence here is a 1x asset being + delivered into a 2x slot, not an unspecified parameter. + """ + supported = {**_BASE, "pixel_ratio": 2} + + assert formats_are_equivalent(_BASE, supported) + assert not format_is_supported(_BASE, supported) + + +def test_format_is_supported_accepts_the_omitted_form_of_a_1x_product() -> None: + """The mirror of the case above, and the reason it is not a plain equality. + + A seller that spells ``pixel_ratio: 1`` publishes the same render as one + that omits it, so a request omitting the field must still be served. + """ + assert format_is_supported(_BASE, {**_BASE, "pixel_ratio": 1}) + + +def test_upgrade_legacy_format_id_keeps_every_parameter_it_was_given() -> None: + """The legacy-size upgrade reparameterizes the ID; it drops no parameter.""" + legacy = FormatId( + agent_url=CANONICAL_CREATIVE_AGENT_URL, + id="display_300x250", + width=300, + height=250, + duration_ms=15000, + pixel_ratio=2, + ) + + upgraded = upgrade_legacy_format_id(legacy) + + assert upgraded.id == "display_image" + assert (upgraded.width, upgraded.height) == (300, 250) + assert upgraded.duration_ms == 15000 + assert upgraded.pixel_ratio == 2