From 915fdfc0f99660e11dea8e63acfbbcb144dc4311 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Wed, 28 Oct 2026 15:48:55 +0000 Subject: [PATCH] feat(types)!: let the schema decide which unknown keys a model keeps AdCPBaseModel defaulted to extra='ignore', so a property the pinned model did not declare -- a field a newer spec added, a sibling arm's property the schema admits -- was dropped on parse and absent after dump: 122 fields across 5 schemas on the 3.3 bundle (#1614). The generator already sets extra='forbid' on every class whose schema object is closed and extra='allow' where the schema says true; the classes that said nothing inherited the base default, which contradicted JSON Schema's reading of an absent additionalProperties. The base default is now extra='allow': an unknown key survives in model_extra and is written back on dump, a null value included. ADCP_STRICT_VALIDATION still flips the default to forbid, which is what catches a stray keyword argument or a renamed field in tests. The wire round-trip suite no longer accepts the field_dropped class. Every schema-valid document in the sweep now survives parse -> dump byte-for-byte under the default dump, with one exception classified as arm routing (discriminator_emitted, #1414). BREAKING CHANGE: AdCPBaseModel keeps unknown keys (extra='allow') instead of dropping them. A stray keyword argument on an open model goes to the wire; set ADCP_STRICT_VALIDATION=1 in tests to find them. Closed schema objects keep refusing unknown keys. Co-Authored-By: Claude Fable 5.1 --- MIGRATION_v9_to_v10.md | 1 + docs/validation-contract.md | 1 - src/adcp/types/base.py | 72 +++++++++----- tests/conformance/test_wire_round_trip.py | 19 ++-- tests/test_extra_policy.py | 25 ++--- tests/test_open_objects.py | 110 ++++++++++++++++++++++ tests/test_strict_validation_env.py | 6 +- 7 files changed, 182 insertions(+), 52 deletions(-) create mode 100644 tests/test_open_objects.py diff --git a/MIGRATION_v9_to_v10.md b/MIGRATION_v9_to_v10.md index 73b89c56ef..1493e8beed 100644 --- a/MIGRATION_v9_to_v10.md +++ b/MIGRATION_v9_to_v10.md @@ -28,3 +28,4 @@ the SDK-relevant inventory is [docs/adcp-3.3-sdk-inventory.md](docs/adcp-3.3-sdk | `model_dump()` writes wire names | A field the generator aliased because its wire key is not a Python identifier is emitted under the wire key without `by_alias=True`: `$schema` instead of `field_schema`, `pass` instead of `pass_`, `from` instead of `from_`, `media-buy` instead of `media_buy`, `_truncation` instead of `field_truncation`, `2xl` instead of `field_2xl`. Code that reads a dump by the Python name must read the wire name; pass `by_alias=False` to keep the SDK 9 output. Construction accepts both spellings (`validate_by_name` is on). | | A received `null` is re-emitted; a Python `None` keyword argument is "not supplied" | A property whose schema admits `null` and that arrived as `null` (`model_validate`, `model_validate_json`, nested validation) stays in `model_fields_set` and is written back as `null` by `model_dump()`. Passing `None` as a keyword argument to a constructor, assigning `None` to a field, or passing `None` in `model_copy(update=...)` now leaves the field *unset*: it is omitted from the dump, and `model_dump(exclude_unset=True)` no longer lists it. A `None` inside a dict you validate is a wire `null`. Properties whose schema does not admit `null` are unchanged (`NoExplicitNull` refuses a JSON `null` and never emits one). | | `model_dump()` leaves an unset schema default off | A property the schema lists outside `required` with a `default` (or a `const`) is emitted only when it was set: by the document that was parsed, by a keyword argument, or by assignment. `sandbox: false`, `timezone: "inventory_local"`, `assignment_limit: 50` and the other schema defaults no longer appear on a dump unless you set them; the attribute still reads the default. Required discriminators (`asset_type`, `pricing_model`) are emitted either way. Pass `exclude_unset=False` for the SDK 9 output. Golden-file tests that compare whole dumps need regenerating. If you relied on a default reaching the peer, set it explicitly. `model_dump_as_received()` is deprecated: it returns the same document as `model_dump(mode="json")` and is removed in SDK 11. Generated root models (a union root such as `LocalizedCreativeAsset`) now subclass `AdCPRootModel` and dump with `exclude_none=True` like every other AdCP model. | +| Unknown keys on open objects are kept and written back | `AdCPBaseModel` defaults to `extra='allow'`: a property the pinned model does not declare (a field a newer spec added, a sibling arm's property the schema admits) survives `model_validate` → `model_dump` in `model_extra`, including a `null` value. Generated classes whose schema object is closed (`additionalProperties: false`) keep `extra='forbid'`. SDK 9 dropped unknown keys (`extra='ignore'`). If your code passed stray keyword arguments and relied on them being dropped, remove them; `ADCP_STRICT_VALIDATION=1` turns the default into `forbid` so tests find them. | diff --git a/docs/validation-contract.md b/docs/validation-contract.md index 522b1549d6..586538082c 100644 --- a/docs/validation-contract.md +++ b/docs/validation-contract.md @@ -129,7 +129,6 @@ with the issue that tracks it, and the list only shrinks: | class | what happens | where it stands | | --- | --- | --- | -| `field_dropped` | `extra='ignore'` drops an unknown or sibling-arm property | #1614; not recoverable by any dump. No digest-covered schema is affected. | | `discriminator_emitted` | a document valid under one arm is routed to a sibling arm, whose required single-value `Literal` it does not carry, and the arm's injected default appears | #1414; closed by #1522's construction dispatch. | ## Wire-boundary behavior diff --git a/src/adcp/types/base.py b/src/adcp/types/base.py index 95b05ad46b..64d859ff6d 100644 --- a/src/adcp/types/base.py +++ b/src/adcp/types/base.py @@ -97,30 +97,32 @@ MessageFormatter = Callable[[Any], str] -def _resolve_extra_policy() -> Literal["ignore", "forbid"]: +def _resolve_extra_policy() -> Literal["allow", "forbid"]: """Choose the ``extra`` policy for :class:`AdCPBaseModel`. - ``ignore`` (default) silently drops unknown fields, preserving - forward compatibility when newer spec versions add fields. This is - the production-safe default — a client on spec N sending to a - server on spec N+1 keeps working. + ``allow`` (default) keeps an unknown field in ``model_extra`` and writes + it back on dump. The generator sets ``extra='forbid'`` on every class + whose schema object is closed (``additionalProperties: false``) and + ``extra='allow'`` where the schema says ``true``; a class whose schema + says nothing inherits this default, which is JSON Schema's own reading of + an absent ``additionalProperties``. A document that carried a property the + pinned model does not declare -- a field a newer spec added, a sibling + arm's property the schema admits -- therefore survives parse → dump + (#1614). SDK 9 dropped it (``ignore``). - ``forbid`` raises on unknown fields so CI catches the silent-drop - case that production-safe defaults obscure. Most useful during - upgrades: right after a major spec revision, run tests with - ``ADCP_STRICT_VALIDATION=1`` to surface every place the upgrade - dropped a renamed field, then ship with the flag unset for the - forward-compat default. + ``forbid`` raises on unknown fields so a test run catches a stray keyword + argument or a renamed field. Set ``ADCP_STRICT_VALIDATION=1`` in CI during + upgrades, then ship with the flag unset. Values accepted: ``"1"``, ``"true"``, ``"yes"``, ``"on"`` (any case). Anything else — including empty string and ``"0"`` — keeps - the ``ignore`` default. + the ``allow`` default. """ raw = os.environ.get("ADCP_STRICT_VALIDATION", "").strip().lower() - return "forbid" if raw in {"1", "true", "yes", "on"} else "ignore" + return "forbid" if raw in {"1", "true", "yes", "on"} else "allow" -_EXTRA_POLICY: Literal["ignore", "forbid"] = _resolve_extra_policy() +_EXTRA_POLICY: Literal["allow", "forbid"] = _resolve_extra_policy() def _build_deferred_serializers(value: Any, seen: set[int]) -> None: @@ -687,19 +689,41 @@ def _keep_opaque_nulls( return kept +def _put_back_null_extras( + value: dict[str, Any], extra: dict[str, Any], info: SerializationInfo +) -> None: + """Re-emit an unknown key whose value is ``null`` on an open model. + + ``exclude_none`` drops it with the ``None`` fields; an unknown key is a + value the document carried, and the schema that admits it admits its + ``null`` (#1614). + """ + include, exclude = info.include, info.exclude + for key, item in extra.items(): + if ( + item is None + and key not in value + and (include is None or key in include) + and (exclude is None or key not in exclude) + ): + value[key] = None + + class AdCPBaseModel(BaseModel): """Base model for AdCP types with spec-compliant serialization. - Defaults to ``extra='ignore'`` so unknown fields from newer spec - versions are silently dropped rather than causing validation - errors. Generated types whose schemas set - ``additionalProperties: true`` override this with ``extra='allow'`` - in their own ``model_config``. + Defaults to ``extra='allow'``: an unknown field from a newer spec + version, or a sibling arm's property the schema admits, is kept in + ``model_extra`` and written back on dump instead of being dropped + (#1614). Generated types whose schemas close the object + (``additionalProperties: false``) set ``extra='forbid'`` in their own + ``model_config``; the rest follow the schema's reading of an absent + ``additionalProperties``, which is open. Set ``ADCP_STRICT_VALIDATION=1`` in the environment (``"1"``, ``"true"``, ``"yes"``, ``"on"`` are accepted) to flip the default - to ``extra='forbid'``. Use this during spec upgrades to catch - silently-dropped renamed fields in tests. See :func:`_resolve_extra_policy`. + to ``extra='forbid'``. Use this in tests to catch a stray keyword + argument or a renamed field. See :func:`_resolve_extra_policy`. .. important:: The env var is resolved **once at module import time**. Set it @@ -764,8 +788,10 @@ def _finish_wire_dump(self, value: Any, info: SerializationInfo) -> Any: return value cls = type(self) extra = self.__pydantic_extra__ - if extra and info.exclude_none and not cls.model_fields: - return _keep_opaque_nulls(value, extra, info) + if extra and info.exclude_none: + if not cls.model_fields: + return _keep_opaque_nulls(value, extra, info) + _put_back_null_extras(value, extra, info) if not info.exclude_unset and not _EMIT_UNSET_DEFAULTS.get(): self._drop_unset_defaults(value, info) if info.exclude_none: diff --git a/tests/conformance/test_wire_round_trip.py b/tests/conformance/test_wire_round_trip.py index 8a54408559..e3d263b457 100644 --- a/tests/conformance/test_wire_round_trip.py +++ b/tests/conformance/test_wire_round_trip.py @@ -52,11 +52,6 @@ "discriminator_emitted": ( "a document routed to an arm it does not satisfy gains the arm's discriminator (#1414)" ), - # ``extra='ignore'`` drops a property the pinned model does not declare, - # and a union arm declares only its own properties while the schema's arm - # admits its siblings'. No digest-covered schema is affected at this pin; - # the hazard is a newer-spec field inside one. #1614. - "field_dropped": "unknown and sibling-arm properties are dropped (#1614)", } #: Schemas whose bytes the protocol binds: no difference of any class, under @@ -183,13 +178,11 @@ def test_every_accepted_class_still_occurs() -> None: assert not unknown, f"ACCEPTED names classes the sweep does not produce: {unknown}" -def test_dump_as_received_leaves_only_unknown_fields() -> None: - """The explicit "as received" dump recovers everything but what the model never held. +def test_the_explicit_as_received_settings_leave_no_residue() -> None: + """``exclude_unset=True, exclude_none=False, by_alias=True`` is the document too. - ``AdCPBaseModel.model_dump_as_received`` leaves off unset defaults, keeps - an admitted explicit ``null`` and writes wire names, so the only residue is - a property the model dropped on input (#1614). This is the path a verifier - uses to recompute a digest over a parsed object today. + The explicit settings SDK 9's ``model_dump_as_received`` used recover the + same bytes as the default dump; nothing the model held is left behind. """ residue: dict[str, set[str]] = defaultdict(set) for schema in SCHEMAS: @@ -199,9 +192,9 @@ def test_dump_as_received_leaves_only_unknown_fields() -> None: for d in rt.round_trip(document, as_received=True)[0]: residue[d.kind].add(schema) unexpected = { - kind: sorted(schemas) for kind, schemas in residue.items() if kind != "field_dropped" + kind: sorted(schemas) for kind, schemas in residue.items() if kind not in ACCEPTED } - assert not unexpected, f"dump-as-received residue beyond dropped fields: {unexpected}" + assert not unexpected, f"as-received residue: {unexpected}" def test_refusing_schemas_are_declared() -> None: diff --git a/tests/test_extra_policy.py b/tests/test_extra_policy.py index 5e8465ffd0..b3118a686c 100644 --- a/tests/test_extra_policy.py +++ b/tests/test_extra_policy.py @@ -36,26 +36,27 @@ GENERATED_DIR = Path(__file__).parent.parent / "src" / "adcp" / "types" / "domains" -def test_base_model_config_is_ignore() -> None: - """Sanity check: AdCPBaseModel must have extra='ignore'.""" - assert AdCPBaseModel.model_config.get("extra") == "ignore", ( +def test_base_model_config_is_allow() -> None: + """Sanity check: AdCPBaseModel must have extra='allow' (SDK 10, #1614).""" + assert AdCPBaseModel.model_config.get("extra") == "allow", ( f"AdCPBaseModel.model_config is {AdCPBaseModel.model_config!r} — " "is the package installed from the correct branch?" ) class TestBaseModelDefault: - """AdCPBaseModel defaults to extra='ignore'.""" + """AdCPBaseModel defaults to extra='allow'.""" - def test_base_model_drops_extra_fields(self) -> None: - """Unknown fields are silently dropped, not stored or rejected.""" + def test_base_model_keeps_extra_fields(self) -> None: + """Unknown fields are kept in model_extra and written back, not dropped (#1614).""" class DefaultType(AdCPBaseModel): name: str - obj = DefaultType(name="test", unknown_field="dropped") + obj = DefaultType(name="test", unknown_field="kept") assert obj.name == "test" - assert not hasattr(obj, "unknown_field") + assert obj.model_extra == {"unknown_field": "kept"} + assert obj.model_dump() == {"name": "test", "unknown_field": "kept"} def test_base_model_accepts_known_fields(self) -> None: class DefaultType(AdCPBaseModel): @@ -77,16 +78,16 @@ class ExtensibleType(AdCPBaseModel): assert obj.name == "test" assert obj.extra_field == "stored" # type: ignore[attr-defined] - def test_ignore_inherited_without_explicit_config(self) -> None: - """A subclass without its own model_config inherits ignore from base.""" + def test_allow_inherited_without_explicit_config(self) -> None: + """A subclass without its own model_config inherits allow from base.""" class InheritedType(AdCPBaseModel): name: str value: int - obj = InheritedType(name="test", value=1, surprise="dropped") + obj = InheritedType(name="test", value=1, surprise="kept") assert obj.name == "test" - assert not hasattr(obj, "surprise") + assert obj.model_extra == {"surprise": "kept"} def test_open_payload_field_preserves_extension_data(self) -> None: """x-adcp-open-payload fields keep arbitrary structured payload data.""" diff --git a/tests/test_open_objects.py b/tests/test_open_objects.py new file mode 100644 index 0000000000..8c3cff5777 --- /dev/null +++ b/tests/test_open_objects.py @@ -0,0 +1,110 @@ +"""An open object keeps the keys it does not declare (#1614). + +JSON Schema reads an absent ``additionalProperties`` as open, and the +generator sets ``extra='forbid'`` only where a schema object is closed. In +SDK 9 ``AdCPBaseModel`` defaulted to ``extra='ignore'``, so a property the +pinned model did not declare -- a field a newer spec added, a sibling arm's +property the schema admits -- was dropped on parse and absent after dump. SDK +10 defaults to ``extra='allow'``: the key survives in ``model_extra`` and is +written back, ``null`` included. ``tests/conformance/test_wire_round_trip.py`` +holds every generated model to it with no ``field_dropped`` allowance. +""" + +from __future__ import annotations + +import json + +import pytest +from pydantic import ConfigDict, ValidationError + +from adcp.types.base import AdCPBaseModel + + +class Open(AdCPBaseModel): + x: int | None = None + + +class Closed(AdCPBaseModel): + model_config = ConfigDict(extra="forbid") + x: int | None = None + + +def test_the_default_policy_is_allow() -> None: + assert AdCPBaseModel.model_config["extra"] == "allow" + assert Open.model_config["extra"] == "allow" + + +def test_an_unknown_key_survives_parse_and_dump() -> None: + document = {"x": 1, "added_in_3_4": {"nested": [1, 2]}, "nothing": None} + parsed = Open.model_validate_json(json.dumps(document)) + assert parsed.model_extra == {"added_in_3_4": {"nested": [1, 2]}, "nothing": None} + assert parsed.model_dump(mode="json") == document + assert json.loads(parsed.model_dump_json()) == document + + +def test_include_and_exclude_apply_to_a_null_unknown_key() -> None: + parsed = Open.model_validate({"x": 1, "nothing": None}) + assert parsed.model_dump(mode="json", exclude={"nothing"}) == {"x": 1} + assert parsed.model_dump(mode="json", include={"x"}) == {"x": 1} + + +def test_a_stray_keyword_argument_goes_to_the_wire() -> None: + """Documented: a closed schema object refuses it; an open one carries it.""" + assert Open(x=1, stray="s").model_dump(mode="json") == {"x": 1, "stray": "s"} + with pytest.raises(ValidationError): + Closed(x=1, stray="s") + + +def test_a_generated_closed_object_refuses_an_unknown_key() -> None: + from adcp.types.domains.media_buy.request_proposals_response import PurchaseContinuation1 + + assert PurchaseContinuation1.model_config["extra"] == "forbid" + with pytest.raises(ValidationError): + PurchaseContinuation1.model_validate( + { + "kind": "legacy_create", + "continuation_token": "0123456789abcdef", + "continuation_expires_at": "2026-10-01T00:00:00Z", + "source_adcp_version": "3.1", + "product_ids": ["prod_1"], + "losses": ["feed_version_not_atomic", "pricing_version_not_atomic"], + "requires_explicit_acceptance": True, + "unknown": 1, + } + ) + + +def test_a_generated_open_object_keeps_an_unknown_key() -> None: + from adcp.types.domains.core.targeting_input import TargetingOverlayInput + + assert TargetingOverlayInput.model_config["extra"] == "allow" + document = {"geo_regions": ["US-NY"], "added_in_3_4": {"nested": None}, "nothing": None} + parsed = TargetingOverlayInput.model_validate_json(json.dumps(document)) + assert parsed.model_extra == {"added_in_3_4": {"nested": None}, "nothing": None} + assert parsed.model_dump(mode="json") == document + + +def test_the_generated_tree_follows_its_schemas() -> None: + """Closed schema objects forbid; everything else allows. Nothing ignores.""" + import importlib + import pkgutil + + import adcp.types.domains as domains + + policies: dict[str, int] = {} + for info in pkgutil.walk_packages(domains.__path__, prefix=domains.__name__ + "."): + try: + module = importlib.import_module(info.name) + except ImportError: + continue + for obj in list(vars(module).values()): + if ( + isinstance(obj, type) + and issubclass(obj, AdCPBaseModel) + and obj.__module__ == module.__name__ + ): + policy = str(obj.model_config.get("extra")) + policies[policy] = policies.get(policy, 0) + 1 + assert set(policies) <= {"allow", "forbid"}, policies + assert policies.get("ignore", 0) == 0 + assert policies["allow"] > 1000 and policies["forbid"] > 500, policies diff --git a/tests/test_strict_validation_env.py b/tests/test_strict_validation_env.py index f8898b4d2a..9256bee980 100644 --- a/tests/test_strict_validation_env.py +++ b/tests/test_strict_validation_env.py @@ -30,7 +30,7 @@ def test_falsy_or_unknown_values_keep_default(value: str) -> None: forward-compat ``ignore`` default. Whitespace-around-1 is caught by strip(), but ``"tRUE "`` specifically tests that trailing space is tolerated.""" - expected = "forbid" if value.strip().lower() in {"1", "true", "yes", "on"} else "ignore" + expected = "forbid" if value.strip().lower() in {"1", "true", "yes", "on"} else "allow" with patch.dict("os.environ", {"ADCP_STRICT_VALIDATION": value}): assert _resolve_extra_policy() == expected @@ -38,7 +38,7 @@ def test_falsy_or_unknown_values_keep_default(value: str) -> None: def test_unset_var_keeps_default() -> None: """No env var at all → default (forward-compat safe).""" with patch.dict("os.environ", {}, clear=True): - assert _resolve_extra_policy() == "ignore" + assert _resolve_extra_policy() == "allow" def test_default_is_ignore_for_production_forward_compat() -> None: @@ -51,7 +51,7 @@ def test_default_is_ignore_for_production_forward_compat() -> None: # In the test environment with no ADCP_STRICT_VALIDATION set, it # must be ``ignore`` — a regression here indicates the default # flipped. - assert _EXTRA_POLICY in {"ignore", "forbid"} + assert _EXTRA_POLICY in {"allow", "forbid"} # If this assertion fails, the test environment has the env var # set — check your shell, not the SDK.