diff --git a/src/adcp/server/responses.py b/src/adcp/server/responses.py index a5b4f9782..aa00ab7fd 100644 --- a/src/adcp/server/responses.py +++ b/src/adcp/server/responses.py @@ -31,6 +31,27 @@ async def get_products(): _logger = logging.getLogger("adcp.server") +def _strip_none_values(value: Any) -> Any: + """Recursively strip None-valued keys from dicts and lists. + + Applied to loose-dict items in asset-bearing response builders so that + optional Pydantic fields (e.g. ``ImageAsset.format``) which default to + ``None`` in Python do not appear as ``null`` on the wire. The bundled + JSON schemas declare those fields as non-nullable (``"type": "string"``, + not ``["string", "null"]``), so a null value causes ``oneOf``/discriminator + validation to fail at the buyer's schema validator. + + Pydantic model items are not passed through this function — their + ``model_dump(exclude_none=True)`` call in :func:`_serialize` already + handles null exclusion. + """ + if isinstance(value, dict): + return {k: _strip_none_values(v) for k, v in value.items() if v is not None} + if isinstance(value, list): + return [_strip_none_values(v) for v in value] + return value + + def _strip_write_only_fields(value: Any) -> Any: """Recursively strip write-only credential fields from a wire dict. @@ -86,17 +107,19 @@ def _serialize(items: list[Any]) -> list[Any]: a hand-built response builder) get a recursive write-only-field strip via :func:`_strip_write_only_fields` so ``governance_agents[i].authentication`` and ``billing_entity.bank`` - can't smuggle through. Pydantic models are passed through their - own ``model_dump`` — the typed projections at - :mod:`adcp.decisioning.account_projection` are responsible for - those. + can't smuggle through, followed by :func:`_strip_none_values` to + remove ``null``-valued keys that the bundled JSON schemas declare as + non-nullable (e.g. ``ImageAsset.format``). Pydantic models are + passed through their own ``model_dump(exclude_none=True)`` — the + typed projections at :mod:`adcp.decisioning.account_projection` are + responsible for the write-only strip on that path. """ out: list[Any] = [] for p in items: if hasattr(p, "model_dump"): out.append(p.model_dump(mode="json", exclude_none=True)) elif isinstance(p, dict): - out.append(_strip_write_only_fields(p)) + out.append(_strip_none_values(_strip_write_only_fields(p))) else: out.append(p) return out @@ -425,7 +448,7 @@ def sync_creatives_response( Optionally: status ("processing"|"pending_review"|"approved"|"rejected"|"archived"). Matches SyncCreativesResponse1 schema (field: "creatives"). """ - return {"creatives": creatives, "sandbox": sandbox} + return {"creatives": _serialize(creatives), "sandbox": sandbox} def list_creatives_response( @@ -492,7 +515,7 @@ def preview_creative_response( """ return { "response_type": "single", - "previews": previews, + "previews": _serialize(previews), "expires_at": expires_at or "2099-12-31T23:59:59Z", "sandbox": sandbox, } @@ -513,11 +536,11 @@ def build_creative_response( """ if isinstance(creative_manifest, list): return { - "creative_manifests": creative_manifest, + "creative_manifests": [_strip_none_values(m) for m in creative_manifest], "sandbox": sandbox, } return { - "creative_manifest": creative_manifest, + "creative_manifest": _strip_none_values(creative_manifest), "sandbox": sandbox, } diff --git a/tests/test_server_dx.py b/tests/test_server_dx.py index 167dff90a..af22412a2 100644 --- a/tests/test_server_dx.py +++ b/tests/test_server_dx.py @@ -179,6 +179,42 @@ def test_uses_creatives_key(self): assert "results" not in result assert result["creatives"] == creatives + def test_strips_none_from_sync_result_fields(self): + """None-valued fields in sync creative dicts are stripped from wire output.""" + creatives = [{"creative_id": "c1", "action": "created", "status": None}] + result = sync_creatives_response(creatives) + assert result["creatives"][0] == {"creative_id": "c1", "action": "created"} + + +class TestStripNoneValues: + """_strip_none_values removes None-valued keys from dicts recursively.""" + + def test_flat_dict_strips_none(self): + from adcp.server.responses import _strip_none_values + + result = _strip_none_values({"a": "hello", "b": None, "c": 42}) + assert result == {"a": "hello", "c": 42} + + def test_nested_dict_strips_none(self): + from adcp.server.responses import _strip_none_values + + result = _strip_none_values( + {"outer": {"inner": None, "keep": "yes"}, "top_none": None} + ) + assert result == {"outer": {"keep": "yes"}} + + def test_list_items_stripped(self): + from adcp.server.responses import _strip_none_values + + result = _strip_none_values([{"x": None, "y": 1}, {"x": 2, "y": None}]) + assert result == [{"y": 1}, {"x": 2}] + + def test_non_none_values_preserved(self): + from adcp.server.responses import _strip_none_values + + result = _strip_none_values({"a": 0, "b": False, "c": "", "d": []}) + assert result == {"a": 0, "b": False, "c": "", "d": []} + class TestListCreativesResponse: def test_basic(self): @@ -225,6 +261,68 @@ def test_preserves_caller_provided_timestamps(self): assert item["created_date"] == created assert item["updated_date"] == updated + def test_strips_none_from_asset_fields_in_dict_creatives(self): + """None-valued asset fields must not appear as null on the wire. + + ImageAsset.format / alt_text / provenance are optional (non-required) + in the JSON schema but non-nullable (type: string, not [string, null]). + When an adopter builds a dict-based creative with None-valued asset + fields, the response builder must strip them before wire serialisation + so the storyboard schema validator does not reject the payload. + """ + creative = { + "creative_id": "c1", + "created_date": "2024-01-01T00:00:00+00:00", + "updated_date": "2024-01-01T00:00:00+00:00", + "assets": { + "banner": { + "asset_type": "image", + "url": "https://cdn.example.com/banner.png", + "width": 300, + "height": 250, + "format": None, + "alt_text": None, + "provenance": None, + } + }, + } + result = list_creatives_response([creative]) + asset = result["creatives"][0]["assets"]["banner"] + assert "format" not in asset, "format: null must be stripped from wire output" + assert "alt_text" not in asset, "alt_text: null must be stripped from wire output" + assert "provenance" not in asset, "provenance: null must be stripped from wire output" + assert asset["asset_type"] == "image" + assert asset["url"] == "https://cdn.example.com/banner.png" + assert asset["width"] == 300 + assert asset["height"] == 250 + + def test_strips_none_from_video_asset_fields(self): + """VideoAsset optional fields (container_format, video_codec, etc.) are non-nullable.""" + creative = { + "creative_id": "v1", + "created_date": "2024-01-01T00:00:00+00:00", + "updated_date": "2024-01-01T00:00:00+00:00", + "assets": { + "main_video": { + "asset_type": "video", + "url": "https://cdn.example.com/video.mp4", + "width": 1920, + "height": 1080, + "container_format": None, + "video_codec": None, + "duration_ms": None, + "provenance": None, + } + }, + } + result = list_creatives_response([creative]) + asset = result["creatives"][0]["assets"]["main_video"] + assert "container_format" not in asset + assert "video_codec" not in asset + assert "duration_ms" not in asset + assert "provenance" not in asset + assert asset["asset_type"] == "video" + class TestPreviewCreativeResponse: def test_basic(self): @@ -234,6 +332,32 @@ def test_basic(self): assert result["previews"] == previews assert "expires_at" in result + def test_strips_none_from_asset_fields_in_preview(self): + """None asset fields in preview input are stripped from wire output.""" + previews = [ + { + "preview_id": "p1", + "input": { + "assets": { + "hero": { + "asset_type": "image", + "url": "https://cdn.example.com/hero.png", + "width": 1200, + "height": 628, + "alt_text": None, + "format": None, + } + } + }, + "renders": [], + } + ] + result = preview_creative_response(previews) + asset = result["previews"][0]["input"]["assets"]["hero"] + assert "alt_text" not in asset + assert "format" not in asset + assert asset["asset_type"] == "image" + class TestBuildCreativeResponse: def test_basic(self): @@ -242,6 +366,48 @@ def test_basic(self): assert result["creative_manifest"] == manifest assert result["sandbox"] is True + def test_strips_none_from_asset_fields_in_manifest(self): + """None asset fields in build_creative manifest are stripped from wire output.""" + manifest = { + "format_id": {"agent_url": "http://localhost", "id": "d300"}, + "name": "Test", + "assets": { + "banner": { + "asset_type": "image", + "url": "https://cdn.example.com/banner.png", + "width": 300, + "height": 250, + "format": None, + "alt_text": None, + } + }, + } + result = build_creative_response(manifest) + asset = result["creative_manifest"]["assets"]["banner"] + assert "format" not in asset + assert "alt_text" not in asset + assert asset["url"] == "https://cdn.example.com/banner.png" + + def test_strips_none_from_multi_manifest(self): + """None stripping works for multi-manifest (list) variant.""" + manifests = [ + { + "name": "A", + "assets": { + "img": { + "asset_type": "image", + "url": "u", + "width": 1, + "height": 1, + "format": None, + } + }, + } + ] + result = build_creative_response(manifests) + asset = result["creative_manifests"][0]["assets"]["img"] + assert "format" not in asset + class TestSignalsResponse: def test_basic(self):