Skip to content

codegen: required + nullable collapses to non-nullable — CreateMediaBuySuccess.confirmed_at cannot hold the null its schema permits #1137

Description

@KonstantinMirin

media-buy/create-media-buy-response.json @ 3.1 types confirmed_at as ["string", "null"]
and lists it in the Success branch's required. That pair means the key must be present and
its value may be null. The generated CreateMediaBuyResponse1 types it non-nullable, so the
model cannot hold the value its own source schema permits.

import json, os, adcp
base = os.path.join(os.path.dirname(adcp.__file__), "_schemas", "3.1")
d = json.load(open(os.path.join(base, "media-buy", "create-media-buy-response.json")))
branch = d["oneOf"][0]
print(branch["properties"]["confirmed_at"]["type"], "confirmed_at" in branch["required"])
# ['string', 'null'] True

from adcp.types.generated_poc.media_buy.create_media_buy_response import CreateMediaBuyResponse1 as C
print(C.model_fields["confirmed_at"].annotation)   # <class 'pydantic.types.AwareDatetime'>
C(media_buy_id="x", packages=[], status="completed", confirmed_at=None, revision=1)
# pydantic_core.ValidationError: confirmed_at — Input should be a valid datetime

Why the null is a real state, not a theoretical one

confirmed_at is documented as the instant the seller committed the buy. A buy created in
pending_creatives has not been committed — the seller is holding it until creatives arrive —
so there is no instant to report, and the schema says so by permitting null while still
requiring the key. A seller that must supply a datetime has two choices, and both are wrong:
report a commitment that did not happen, or drop a key the schema requires.

Scope

Three required-AND-nullable declarations exist across every pinned response schema in the
3.1 tree:

confirmed_at   CreateMediaBuySuccess         media-buy/create-media-buy-response.json
confirmed_at   CreateMediaBuySuccess         bundled/media-buy/create-media-buy-response.json  (same one, bundled)
cursor         Registry Feed Response        core/registry-feed-response.json

So this is one field, not a pattern — but it is the field on the busiest response in the
protocol, and the codegen rule that produced it (required read as "not Optional", without
consulting the declared type array) would narrow any future one the same way.

Second-order effect: exclude_none=True then drops the key

AdCPBaseModel.model_dump serializes with a blanket exclude_none=True. That is right for an
optional field — AdCP omits rather than sending null — and wrong for a field the schema lists
in required while typing it nullable. So a downstream model that widens the annotation to
express the permitted null gets the key dropped from the wire at exactly the moment the value
is legal, producing a document that fails validation against the pinned schema. The two
behaviours are individually defensible and jointly make the correct document unreachable.

Worth considering alongside the type fix: exclude None only for fields the model does not
mark required. That is derivable from model_fields alone, needs no schema lookup, and
reproduces today's behaviour everywhere except the case above.

Workaround in use

Widening the annotation on our subclass, with a # type: ignore[assignment] for the Liskov
violation, plus a serializer hook that re-inserts required-and-nullable fields after
exclude_none has removed them. Both delete when the generated type matches the schema.

Suggested fix

Generate AwareDatetime | None for a property whose type array contains "null", keeping
it required (no default) when the schema lists it in required. required and nullable are
independent axes in JSON Schema; the generator currently collapses them.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions