diff --git a/README.md b/README.md index cfef2ea39..ea0680e7e 100644 --- a/README.md +++ b/README.md @@ -1218,7 +1218,9 @@ async with ADCPClient(config) as client: `get_media_buys`, or the last successful `update_media_buy`, then pass it on the next mutating update so the seller can reject stale writes. `confirmed_at` is the seller commitment timestamp and should remain stable across later pause/resume or -budget updates. +budget updates. The key is always present on a success response, but its value is +`None` until the seller actually commits — a buy still in `pending_creatives` has +no commitment instant to report — so narrow it before use. ### Complete Creative Workflow diff --git a/scripts/post_generate_fixes.py b/scripts/post_generate_fixes.py index 638bc24f0..da4a78876 100644 --- a/scripts/post_generate_fixes.py +++ b/scripts/post_generate_fixes.py @@ -3785,6 +3785,55 @@ def _response_arm_models(cls) -> tuple[type[{base_name}], ...]: print(f" {relative_path}: restored constructible {base_name} base") +def _schema_permits_null(schema: Any) -> bool: + """True when a JSON Schema property explicitly permits ``null``. + + ``required`` and nullability are independent axes in JSON Schema: a + property listed in ``required`` whose ``type`` array contains ``"null"`` + must be *present* and may be *null*. Both spellings count here — the + ``type: ["string", "null"]`` array form and a ``oneOf``/``anyOf`` branch + of ``{"type": "null"}``. + """ + if not isinstance(schema, dict): + return False + schema_type = schema.get("type") + if isinstance(schema_type, list) and "null" in schema_type: + return True + for keyword in ("oneOf", "anyOf"): + variants = schema.get(keyword) + if isinstance(variants, list) and any( + isinstance(variant, dict) and variant.get("type") == "null" for variant in variants + ): + return True + return False + + +def _top_level_union_parts(annotation: str) -> list[str]: + """Split a rendered annotation on its top-level ``|`` separators.""" + parts: list[str] = [] + current: list[str] = [] + depth = 0 + for char in annotation: + if char in "[(": + depth += 1 + elif char in "])": + depth -= 1 + if char == "|" and depth == 0: + parts.append("".join(current).strip()) + current = [] + continue + current.append(char) + parts.append("".join(current).strip()) + return parts + + +def _union_with_none(annotation: str) -> str: + """Return ``annotation`` widened with ``None``, without duplicating it.""" + if "None" in _top_level_union_parts(annotation): + return annotation + return f"{annotation} | None" + + def restore_response_variant_aliases() -> None: """Restore numbered response arms from schema data, not hand-written payloads. @@ -4151,9 +4200,14 @@ def emit_nested(self, preferred: str, schema: dict[str, Any]) -> str: else: typ = self.type_for(prop_name, prop_schema) if prop_name in required: + # Required-and-nullable: keep the field required (no + # default) while letting it hold the null the schema + # permits. + if _schema_permits_null(prop_schema): + typ = _union_with_none(typ) lines.append(f" {prop_name}: {typ}") else: - lines.append(f" {prop_name}: {typ} | None = None") + lines.append(f" {prop_name}: {_union_with_none(typ)} = None") self.nested.append("\n".join(lines)) return class_name @@ -4221,9 +4275,14 @@ def emit_response_class(self, class_name: str, arm: dict[str, Any]) -> str: if isinstance(const, str): lines.append(f" {prop_name}: {typ} = {const!r}") else: + # Required-and-nullable: keep the field required (no + # default) while letting it hold the null the schema + # permits. + if _schema_permits_null(prop_schema): + typ = _union_with_none(typ) lines.append(f" {prop_name}: {typ}") else: - lines.append(f" {prop_name}: {typ} | None = None") + lines.append(f" {prop_name}: {_union_with_none(typ)} = None") if self.base in { "CreateMediaBuyResponse", "UpdateMediaBuyResponse", diff --git a/src/adcp/types/canonical_creative.pyi b/src/adcp/types/canonical_creative.pyi index 9ac23fe80..4c049fe8c 100644 --- a/src/adcp/types/canonical_creative.pyi +++ b/src/adcp/types/canonical_creative.pyi @@ -1,4 +1,5 @@ from collections.abc import Sequence +from datetime import datetime from typing import Any, ClassVar, Literal, TypeAlias, TypeVar from adcp.types.base import AdCPBaseModel @@ -126,12 +127,16 @@ class UpdateMediaBuyRequest(CanonicalBoundaryModel): class CreateMediaBuyResponse1(CanonicalBoundaryModel): media_buy_id: str packages: list[Package] + # Required *and* nullable: the schema lists confirmed_at in the success + # branch's ``required`` while typing it ``["string", "null"]``. A buy + # awaiting seller commitment carries the key with a null value. + confirmed_at: datetime | None def __init__( self, *, media_buy_id: str, status: Any, - confirmed_at: Any, + confirmed_at: datetime | None, revision: int, packages: list[Package], media_buy_status: Any = ..., diff --git a/src/adcp/types/generated_poc/media_buy/create_media_buy_response.py b/src/adcp/types/generated_poc/media_buy/create_media_buy_response.py index 6153e2901..5ad3e2564 100644 --- a/src/adcp/types/generated_poc/media_buy/create_media_buy_response.py +++ b/src/adcp/types/generated_poc/media_buy/create_media_buy_response.py @@ -35,7 +35,7 @@ class CreateMediaBuyResponse1(AdcpVersionEnvelope): account: account_1.Account | None = None invoice_recipient: business_entity_1.BusinessEntity | None = None media_buy_status: media_buy_status_1.MediaBuyStatus | None = None - confirmed_at: AwareDatetime + confirmed_at: AwareDatetime | None creative_deadline: AwareDatetime | None = None revision: Annotated[int, Field(ge=1)] currency: Annotated[str, StringConstraints(pattern='^[A-Z]{3}$')] | None = None diff --git a/tests/test_code_generation.py b/tests/test_code_generation.py index faf3a72f5..4b31084f4 100644 --- a/tests/test_code_generation.py +++ b/tests/test_code_generation.py @@ -1502,3 +1502,135 @@ def test_consumer_subclassability_contract(): assert failures == [], "Consumer subclassability contract violated:\n" + "\n".join( f" - {f}" for f in failures ) + + +def test_schema_permits_null_reads_both_nullable_spellings(): + """``required`` and nullability are independent axes in JSON Schema. + + The custom response emitter must recognize both spellings the pinned + schemas use for "this value may be null": the ``type`` array form and a + ``oneOf``/``anyOf`` branch of ``{"type": "null"}``. + """ + from scripts.post_generate_fixes import _schema_permits_null + + assert _schema_permits_null({"type": ["string", "null"]}) + assert _schema_permits_null({"type": ["null", "string"], "format": "date-time"}) + assert _schema_permits_null({"oneOf": [{"type": "string"}, {"type": "null"}]}) + assert _schema_permits_null({"anyOf": [{"$ref": "x.json"}, {"type": "null"}]}) + + assert not _schema_permits_null({"type": "string"}) + assert not _schema_permits_null({"type": ["string", "integer"]}) + assert not _schema_permits_null({"oneOf": [{"type": "string"}, {"type": "integer"}]}) + assert not _schema_permits_null(True) + assert not _schema_permits_null(None) + + +def test_union_with_none_does_not_duplicate_existing_none(): + """Widening is idempotent at the top level and bracket-aware.""" + from scripts.post_generate_fixes import _union_with_none + + assert _union_with_none("AwareDatetime") == "AwareDatetime | None" + assert _union_with_none("str | None") == "str | None" + assert _union_with_none("None | str") == "None | str" + # ``None`` nested inside a subscript is not a top-level union member. + assert _union_with_none("dict[str, str | None]") == "dict[str, str | None] | None" + assert ( + _union_with_none("Annotated[str, StringConstraints(pattern='None')]") + == "Annotated[str, StringConstraints(pattern='None')] | None" + ) + + +def test_post_generate_required_nullable_field_stays_required_and_nullable(tmp_path, monkeypatch): + """A ``required`` property typed ``["string", "null"]`` keeps both axes. + + Regression for #1137: the emitter read ``required`` as "not Optional" and + dropped the schema's ``null`` branch, so ``CreateMediaBuySuccess`` could + not hold the null its own source schema permits. The field must gain + ``| None`` **without** gaining a default — it stays required on the wire. + """ + import ast + import json + from pathlib import Path + + from adcp._version import _read_packaged_version + from adcp.validation.version import resolve_bundle_key + from scripts import post_generate_fixes + + generated_dir = tmp_path / "generated_poc" + target = generated_dir / "media_buy" / "create_media_buy_response.py" + target.parent.mkdir(parents=True) + target.write_text( + "# generated by datamodel-codegen:\n" + "# filename: media_buy/create_media_buy_response.json\n\n" + "from __future__ import annotations\n\n" + "from ..core.version_envelope import AdcpVersionEnvelope\n\n\n" + "class CreateMediaBuyResponse(AdcpVersionEnvelope):\n" + " pass\n" + ) + monkeypatch.setattr(post_generate_fixes, "OUTPUT_DIR", generated_dir) + + post_generate_fixes.restore_response_variant_aliases() + + generated_source = target.read_text() + compile(generated_source, str(target), "exec") + + bundle_key = resolve_bundle_key(_read_packaged_version()) + schema_path = ( + Path("schemas") / "cache" / bundle_key / "media-buy" / "create-media-buy-response.json" + ) + success_arm = json.loads(schema_path.read_text())["oneOf"][0] + # Guard the premise: the fix is only meaningful while the schema keeps + # declaring confirmed_at as required-and-nullable. + assert "confirmed_at" in success_arm["required"] + assert "null" in success_arm["properties"]["confirmed_at"]["type"] + + module = ast.parse(generated_source) + success_class = next( + node + for node in module.body + if isinstance(node, ast.ClassDef) and node.name == "CreateMediaBuyResponse1" + ) + confirmed_at = next( + node + for node in success_class.body + if isinstance(node, ast.AnnAssign) + and isinstance(node.target, ast.Name) + and node.target.id == "confirmed_at" + ) + assert ast.unparse(confirmed_at.annotation) == "AwareDatetime | None" + assert confirmed_at.value is None, "required-and-nullable fields must not gain a default" + + # A required non-nullable sibling is untouched — the widening is driven by + # the schema's ``type`` array, not applied to every required field. + media_buy_id = next( + node + for node in success_class.body + if isinstance(node, ast.AnnAssign) + and isinstance(node.target, ast.Name) + and node.target.id == "media_buy_id" + ) + assert ast.unparse(media_buy_id.annotation) == "str" + assert media_buy_id.value is None + + +def test_generated_create_media_buy_success_matches_schema_nullability(): + """The committed generated tree carries the #1137 fix, not just the emitter.""" + import ast + from pathlib import Path + + source = Path("src/adcp/types/generated_poc/media_buy/create_media_buy_response.py").read_text() + module = ast.parse(source) + success_class = next( + node + for node in module.body + if isinstance(node, ast.ClassDef) and node.name == "CreateMediaBuyResponse1" + ) + confirmed_at = next( + node + for node in success_class.body + if isinstance(node, ast.AnnAssign) + and isinstance(node.target, ast.Name) + and node.target.id == "confirmed_at" + ) + assert ast.unparse(confirmed_at.annotation) == "AwareDatetime | None" + assert confirmed_at.value is None diff --git a/tests/test_create_media_buy_response_types.py b/tests/test_create_media_buy_response_types.py index 01c3f0032..efe1e547a 100644 --- a/tests/test_create_media_buy_response_types.py +++ b/tests/test_create_media_buy_response_types.py @@ -87,3 +87,92 @@ def test_handler_create_media_buy_return_type_is_union() -> None: # so signatures carry strings — resolve to runtime objects. hints = typing.get_type_hints(PlatformHandler.create_media_buy) assert hints["return"] == CreateMediaBuyResponse + + +def test_confirmed_at_accepts_null_and_stays_required() -> None: + """``confirmed_at`` is required *and* nullable — both axes hold. + + Regression for #1137. ``media-buy/create-media-buy-response.json`` types + ``confirmed_at`` as ``["string", "null"]`` and lists it in the success + branch's ``required``: a buy still awaiting seller commitment (e.g. + ``pending_creatives``) has no instant to report, so the key must be + present and may be null. + """ + from pydantic import ValidationError + + from adcp.types import CreateMediaBuySuccessResponse + + field = CreateMediaBuySuccessResponse.model_fields["confirmed_at"] + assert field.is_required(), "confirmed_at must not gain a default" + assert type(None) in typing.get_args(field.annotation) + + provisional = CreateMediaBuySuccessResponse( + media_buy_id="mb_1", + packages=[], + confirmed_at=None, + revision=1, + ) + assert provisional.confirmed_at is None + + parsed = CreateMediaBuySuccessResponse.model_validate( + {"media_buy_id": "mb_1", "packages": [], "confirmed_at": None, "revision": 1} + ) + assert parsed.confirmed_at is None + + with pytest.raises(ValidationError): + CreateMediaBuySuccessResponse.model_validate( + {"media_buy_id": "mb_1", "packages": [], "revision": 1} + ) + + +def test_confirmed_at_null_survives_serialization_when_none_is_kept() -> None: + """A null ``confirmed_at`` round-trips whenever ``None`` is not excluded. + + ``AdCPBaseModel.model_dump`` still defaults to ``exclude_none=True``, which + drops the key; reconciling that blanket default with required-and-nullable + fields is tracked separately (#1137's second-order note) and deliberately + out of scope here. What this pins is that the *model* carries the null, so + an explicit ``exclude_none=False`` dump emits the schema-required key. + """ + from adcp.types import CreateMediaBuySuccessResponse + + resp = CreateMediaBuySuccessResponse( + media_buy_id="mb_1", + packages=[], + confirmed_at=None, + revision=1, + ) + dumped = resp.model_dump(exclude_none=False) + assert "confirmed_at" in dumped + assert dumped["confirmed_at"] is None + + assert CreateMediaBuySuccessResponse.model_validate(dumped).confirmed_at is None + + +def test_confirmed_at_still_accepts_a_commitment_timestamp() -> None: + """Widening to ``| None`` must not loosen datetime validation.""" + from datetime import datetime, timezone + + from pydantic import ValidationError + + from adcp.types import CreateMediaBuySuccessResponse + + committed = CreateMediaBuySuccessResponse.model_validate( + { + "media_buy_id": "mb_1", + "packages": [], + "confirmed_at": "2026-05-27T12:00:00Z", + "revision": 1, + } + ) + assert committed.confirmed_at == datetime(2026, 5, 27, 12, 0, tzinfo=timezone.utc) + + with pytest.raises(ValidationError): + CreateMediaBuySuccessResponse.model_validate( + { + "media_buy_id": "mb_1", + "packages": [], + "confirmed_at": "not-a-timestamp", + "revision": 1, + } + ) diff --git a/tests/type_checks/required_nullable_response_fields.py b/tests/type_checks/required_nullable_response_fields.py new file mode 100644 index 000000000..e496556df --- /dev/null +++ b/tests/type_checks/required_nullable_response_fields.py @@ -0,0 +1,82 @@ +"""Adopter pattern: read and set a required-and-nullable response field. + +Regression for #1137. ``media-buy/create-media-buy-response.json`` lists +``confirmed_at`` in the success branch's ``required`` while typing it +``["string", "null"]`` — the key must be present and may be null. The custom +response emitter in ``scripts/post_generate_fixes.py`` used to read +``required`` as "not Optional", so the generated annotation was a bare +``AwareDatetime`` and adopters had to widen it on their own subclass with a +``# type: ignore[assignment]`` Liskov suppression. + +Both surfaces an adopter can reach are pinned here: + +* ``adcp.types.CreateMediaBuySuccessResponse`` — the public canonical alias. + It is built at runtime by ``_canonical_clone``, so type checkers read it + from the hand-maintained ``canonical_creative.pyi`` stub; the stub has to + declare the field for the contract to be visible statically at all. +* ``adcp.types.legacy.LegacyCreateMediaBuyResponse1`` — the raw generated wire + model the stub mirrors, reached through the legacy facade. +""" + +from __future__ import annotations + +from datetime import datetime + +from typing_extensions import assert_type + +from adcp.types import CreateMediaBuySuccessResponse +from adcp.types.legacy import LegacyCreateMediaBuyResponse1 + +# --- Public canonical alias --- + + +def public_commitment_instant(resp: CreateMediaBuySuccessResponse) -> datetime | None: + """A provisional buy reports no commitment instant — ``None`` is legal.""" + assert_type(resp.confirmed_at, datetime | None) + return resp.confirmed_at + + +def build_provisional_buy() -> CreateMediaBuySuccessResponse: + """``confirmed_at=None`` is a valid constructor argument, not an error.""" + return CreateMediaBuySuccessResponse( + media_buy_id="mb_1", + status="completed", + confirmed_at=None, + revision=1, + packages=[], + ) + + +def build_committed_buy(confirmed_at: datetime) -> CreateMediaBuySuccessResponse: + """A real commitment timestamp still satisfies the same parameter.""" + return CreateMediaBuySuccessResponse( + media_buy_id="mb_1", + status="completed", + confirmed_at=confirmed_at, + revision=1, + packages=[], + ) + + +def public_has_committed(resp: CreateMediaBuySuccessResponse) -> bool: + """Narrowing still works, so committed buys keep a non-optional datetime.""" + confirmed_at = resp.confirmed_at + if confirmed_at is None: + return False + assert_type(confirmed_at, datetime) + return confirmed_at <= datetime.now(tz=confirmed_at.tzinfo) + + +# --- Generated wire model behind the canonical alias --- + + +def wire_commitment_instant(resp: LegacyCreateMediaBuyResponse1) -> datetime | None: + """The generated model carries the same required-and-nullable contract.""" + assert_type(resp.confirmed_at, datetime | None) + return resp.confirmed_at + + +def wire_required_sibling_stays_non_optional(resp: LegacyCreateMediaBuyResponse1) -> str: + """Required-and-non-nullable siblings are unaffected by the widening.""" + assert_type(resp.media_buy_id, str) + return resp.media_buy_id