From ca6c8bd4d4ff6252b13a8c262aa31dfddfefc189 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Sun, 1 Nov 2026 23:55:34 +0000 Subject: [PATCH] fix(codegen)!: name each generated enum value once datamodel-code-generator merges an anyOf arm's narrower enum into the root enum without deduplicating, so postal_country_system.System carried zip = 'zip' and zip_1 = 'zip' (19 enums on 10.0.0b4; #1782). Python makes the second member an alias of the first, so nothing broke at runtime, but __members__, autocomplete and member.name offered names that are not AdCP values. post_generate_fixes.dedupe_enum_member_values keeps the first member for each value and removes the repeats (44 members across 11 modules on 3.3.0-beta.3). tests/test_generated_enums_have_no_aliases.py holds the whole generated tree to one member per value. BREAKING CHANGE: the _1 alias members of generated enums (System.zip_1, Field17.ad_request_url_1, ...) are removed. Use the real member (System.zip); System("zip") and member.value are unchanged. Co-Authored-By: Claude Fable 5.1 --- MIGRATION_v9_to_v10.md | 1 + scripts/post_generate_fixes.py | 52 ++++++++++++ scripts/post_generation_manifest.json | 1 + .../types/domains/core/assets/asset_union.py | 2 - .../domains/core/assets/display_tag_asset.py | 2 - src/adcp/types/domains/core/postal_area.py | 8 -- .../domains/core/postal_country_system.py | 8 -- .../creative/list_creatives_response.py | 2 - .../canonical/coordinated_placements.py | 2 - .../seller_rendered_stateful_display.py | 2 - .../media_buy/accept_proposal_response.py | 2 - .../media_buy/buy_products_response.py | 2 - .../media_buy/get_media_buys_response.py | 12 --- .../media_buy_commitment_response.py | 2 - tests/test_generated_enums_have_no_aliases.py | 85 +++++++++++++++++++ 15 files changed, 139 insertions(+), 44 deletions(-) create mode 100644 tests/test_generated_enums_have_no_aliases.py diff --git a/MIGRATION_v9_to_v10.md b/MIGRATION_v9_to_v10.md index a4f374f860..993ec4971a 100644 --- a/MIGRATION_v9_to_v10.md +++ b/MIGRATION_v9_to_v10.md @@ -42,3 +42,4 @@ the SDK-relevant inventory is [docs/adcp-3.3-sdk-inventory.md](docs/adcp-3.3-sdk | `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. | | Permissive adagents.json resolution requires the fetched domain | `resolve_properties_for_agent(doc, agent_url, mode="permissive")` raises `ValueError` without `publisher_domain=`. Pass the domain the document was fetched for: a bare entry's top-level properties are then kept only when every identifier is a `type: "domain"` under that domain, so a file on another host can no longer claim a publisher's site, app bundle or DOOH venue through a bare entry. Properties identified by bundle, store or venue IDs, and properties whose `publisher_domain` names another domain, are dropped from the bare fallback. `allow_any_domain=True` restores the SDK 9 result. Typed selectors and strict mode are unchanged. See [docs/adagents-discovery.md](docs/adagents-discovery.md) for `properties_for_publisher_domain` and `verify_cross_domain_properties`. | +| Generated enums name each value once | The `_1` alias members codegen emitted when an `anyOf` arm repeated an enum value (`System.zip_1`, `Field17.ad_request_url_1`, 19 enums on 10.0.0b4) are gone; each value has one member, the first-declared one. Code that referenced an alias name (`System.zip_1`) must use the real member (`System.zip`); `System("zip")` and `member.value` are unchanged. | diff --git a/scripts/post_generate_fixes.py b/scripts/post_generate_fixes.py index 9e8cc380f0..8d947f91d1 100644 --- a/scripts/post_generate_fixes.py +++ b/scripts/post_generate_fixes.py @@ -7631,6 +7631,57 @@ def point_rootmodels_at_adcp_root() -> None: print(f" Pointed root models at AdCPRootModel in {modified} file(s)") +def dedupe_enum_member_values() -> None: + """Drop the ``_N`` enum members that repeat a value (#1782). + + When an object's ``anyOf`` arm re-declares a property with a narrower + ``enum`` (``postal-country-system.json`` arm 0 narrows ``system`` to + ``['zip', 'zip_plus_four']``), datamodel-code-generator merges the arm's + values into the root enum without deduplicating, so the generated + ``StrEnum`` carries ``zip = 'zip'`` and ``zip_1 = 'zip'``. Python makes the + second member an alias of the first, so nothing breaks at runtime, but + ``__members__``, autocomplete and ``member.name`` then offer names that are + not AdCP values. Keep the first member for each value and remove the rest; + ``tests/test_generated_enums_have_no_aliases.py`` holds the tree to it. + """ + modified = 0 + removed = 0 + for path in sorted(OUTPUT_DIR.rglob("*.py")): + source = path.read_text() + if "Enum)" not in source: + continue + tree = ast.parse(source) + lines = source.splitlines(keepends=True) + doomed: list[tuple[int, int]] = [] + for node in tree.body: + if not isinstance(node, ast.ClassDef) or not any( + isinstance(base, ast.Name) and base.id.endswith("Enum") for base in node.bases + ): + continue + seen: set[object] = set() + for statement in node.body: + if ( + isinstance(statement, ast.Assign) + and len(statement.targets) == 1 + and isinstance(statement.targets[0], ast.Name) + and isinstance(statement.value, ast.Constant) + ): + value = statement.value.value + if value in seen: + assert statement.end_lineno is not None + doomed.append((statement.lineno, statement.end_lineno)) + else: + seen.add(value) + if not doomed: + continue + for start, end in sorted(doomed, reverse=True): + del lines[start - 1 : end] + removed += 1 + path.write_text("".join(lines)) + modified += 1 + print(f" Removed {removed} duplicate-valued enum member(s) from {modified} module(s)") + + def preserve_request_signing_operation_strings() -> None: """Keep request-signing operation lists source-compatible with beta.5. @@ -8781,6 +8832,7 @@ def main(argv: list[str] | None = None) -> int: fix_product_fields_item_reference, fix_audience_evidence_attestation_subject, fix_legacy_purchase_accepted_losses, + dedupe_enum_member_values, drop_empty_model_arms_from_array_fields, preserve_request_signing_operation_strings, enforce_change_term_runtime_constraints, diff --git a/scripts/post_generation_manifest.json b/scripts/post_generation_manifest.json index ee509f8712..ba0f764a83 100644 --- a/scripts/post_generation_manifest.json +++ b/scripts/post_generation_manifest.json @@ -36,6 +36,7 @@ "fix_product_fields_item_reference": true, "fix_audience_evidence_attestation_subject": true, "fix_legacy_purchase_accepted_losses": true, + "dedupe_enum_member_values": true, "drop_empty_model_arms_from_array_fields": true, "preserve_request_signing_operation_strings": true, "enforce_change_term_runtime_constraints": true, diff --git a/src/adcp/types/domains/core/assets/asset_union.py b/src/adcp/types/domains/core/assets/asset_union.py index 2f4c312555..4fe46cde10 100644 --- a/src/adcp/types/domains/core/assets/asset_union.py +++ b/src/adcp/types/domains/core/assets/asset_union.py @@ -562,8 +562,6 @@ class Field1(StrEnum): content = 'content' ad_request_url = 'ad_request_url' clickthrough_url = 'clickthrough_url' - ad_request_url_1 = 'ad_request_url' - clickthrough_url_1 = 'clickthrough_url' class Location5(AdCPBaseModel): diff --git a/src/adcp/types/domains/core/assets/display_tag_asset.py b/src/adcp/types/domains/core/assets/display_tag_asset.py index 585f6bf739..07cef3a0af 100644 --- a/src/adcp/types/domains/core/assets/display_tag_asset.py +++ b/src/adcp/types/domains/core/assets/display_tag_asset.py @@ -86,8 +86,6 @@ class Field17(StrEnum): content = 'content' ad_request_url = 'ad_request_url' clickthrough_url = 'clickthrough_url' - ad_request_url_1 = 'ad_request_url' - clickthrough_url_1 = 'clickthrough_url' class Location18(AdCPBaseModel): diff --git a/src/adcp/types/domains/core/postal_area.py b/src/adcp/types/domains/core/postal_area.py index be68fe7669..5d56cfeda1 100644 --- a/src/adcp/types/domains/core/postal_area.py +++ b/src/adcp/types/domains/core/postal_area.py @@ -37,8 +37,6 @@ class System(StrEnum): au_postcode = 'au_postcode' ch_plz = 'ch_plz' at_plz = 'at_plz' - zip_1 = 'zip' - zip_plus_four_1 = 'zip_plus_four' class PostalArea11(AdCPBaseModel): @@ -76,8 +74,6 @@ class System1(StrEnum): au_postcode = 'au_postcode' ch_plz = 'ch_plz' at_plz = 'at_plz' - outward_1 = 'outward' - full_1 = 'full' class PostalArea12(AdCPBaseModel): @@ -115,8 +111,6 @@ class System2(StrEnum): au_postcode = 'au_postcode' ch_plz = 'ch_plz' at_plz = 'at_plz' - fsa_1 = 'fsa' - full_1 = 'full' class PostalArea13(AdCPBaseModel): @@ -260,8 +254,6 @@ class System9(StrEnum): au_postcode = 'au_postcode' ch_plz = 'ch_plz' at_plz = 'at_plz' - postal_code_1 = 'postal_code' - custom_1 = 'custom' class PostalArea110(AdCPBaseModel): diff --git a/src/adcp/types/domains/core/postal_country_system.py b/src/adcp/types/domains/core/postal_country_system.py index 9e138e0d15..6b59bd1e86 100644 --- a/src/adcp/types/domains/core/postal_country_system.py +++ b/src/adcp/types/domains/core/postal_country_system.py @@ -35,8 +35,6 @@ class System(StrEnum): au_postcode = 'au_postcode' ch_plz = 'ch_plz' at_plz = 'at_plz' - zip_1 = 'zip' - zip_plus_four_1 = 'zip_plus_four' class PostalCountrySystem1(AdCPBaseModel): @@ -74,8 +72,6 @@ class System11(StrEnum): au_postcode = 'au_postcode' ch_plz = 'ch_plz' at_plz = 'at_plz' - outward_1 = 'outward' - full_1 = 'full' class PostalCountrySystem2(AdCPBaseModel): @@ -113,8 +109,6 @@ class System12(StrEnum): au_postcode = 'au_postcode' ch_plz = 'ch_plz' at_plz = 'at_plz' - fsa_1 = 'fsa' - full_1 = 'full' class PostalCountrySystem3(AdCPBaseModel): @@ -258,8 +252,6 @@ class System19(StrEnum): au_postcode = 'au_postcode' ch_plz = 'ch_plz' at_plz = 'at_plz' - postal_code_1 = 'postal_code' - custom_1 = 'custom' class PostalCountrySystem10(AdCPBaseModel): diff --git a/src/adcp/types/domains/creative/list_creatives_response.py b/src/adcp/types/domains/creative/list_creatives_response.py index 3dc7eaaa39..41aa1134b1 100644 --- a/src/adcp/types/domains/creative/list_creatives_response.py +++ b/src/adcp/types/domains/creative/list_creatives_response.py @@ -72,8 +72,6 @@ class IndicatorTypesEvaluatedEnum(StrEnum): budget_constrained = 'budget_constrained' flight_extension_opportunity = 'flight_extension_opportunity' scale_budget_opportunity = 'scale_budget_opportunity' - creative_fatigue_1 = 'creative_fatigue' - creative_quality_opportunity_1 = 'creative_quality_opportunity' class Indicator(AdCPBaseModel): diff --git a/src/adcp/types/domains/formats/canonical/coordinated_placements.py b/src/adcp/types/domains/formats/canonical/coordinated_placements.py index d4ae47fb91..b6dcfa5cb1 100644 --- a/src/adcp/types/domains/formats/canonical/coordinated_placements.py +++ b/src/adcp/types/domains/formats/canonical/coordinated_placements.py @@ -476,8 +476,6 @@ class TransitionMode(StrEnum): instant = 'instant' animated = 'animated' scroll_linked = 'scroll_linked' - instant_1 = 'instant' - animated_1 = 'animated' class ScrollReference(StrEnum): diff --git a/src/adcp/types/domains/formats/canonical/seller_rendered_stateful_display.py b/src/adcp/types/domains/formats/canonical/seller_rendered_stateful_display.py index a386dbdb9a..e6d792c580 100644 --- a/src/adcp/types/domains/formats/canonical/seller_rendered_stateful_display.py +++ b/src/adcp/types/domains/formats/canonical/seller_rendered_stateful_display.py @@ -201,8 +201,6 @@ class TransitionMode(StrEnum): instant = 'instant' animated = 'animated' scroll_linked = 'scroll_linked' - instant_1 = 'instant' - animated_1 = 'animated' class ScrollReference(StrEnum): diff --git a/src/adcp/types/domains/media_buy/accept_proposal_response.py b/src/adcp/types/domains/media_buy/accept_proposal_response.py index e3148f0cb3..53bc821470 100644 --- a/src/adcp/types/domains/media_buy/accept_proposal_response.py +++ b/src/adcp/types/domains/media_buy/accept_proposal_response.py @@ -42,8 +42,6 @@ class Code(StrEnum): inventory_shortfall_forecast = 'inventory_shortfall_forecast' flight_change_creates_pacing_risk = 'flight_change_creates_pacing_risk' fields_ignored_due_to_precedence = 'fields_ignored_due_to_precedence' - inventory_shortfall_forecast_1 = 'inventory_shortfall_forecast' - flight_change_creates_pacing_risk_1 = 'flight_change_creates_pacing_risk' class Status(StrEnum): diff --git a/src/adcp/types/domains/media_buy/buy_products_response.py b/src/adcp/types/domains/media_buy/buy_products_response.py index f1c2f82b3d..a82d0b58c6 100644 --- a/src/adcp/types/domains/media_buy/buy_products_response.py +++ b/src/adcp/types/domains/media_buy/buy_products_response.py @@ -42,8 +42,6 @@ class Code(StrEnum): inventory_shortfall_forecast = 'inventory_shortfall_forecast' flight_change_creates_pacing_risk = 'flight_change_creates_pacing_risk' fields_ignored_due_to_precedence = 'fields_ignored_due_to_precedence' - inventory_shortfall_forecast_1 = 'inventory_shortfall_forecast' - flight_change_creates_pacing_risk_1 = 'flight_change_creates_pacing_risk' class Status(StrEnum): diff --git a/src/adcp/types/domains/media_buy/get_media_buys_response.py b/src/adcp/types/domains/media_buy/get_media_buys_response.py index 1a663c54e1..aa8c0afb3a 100644 --- a/src/adcp/types/domains/media_buy/get_media_buys_response.py +++ b/src/adcp/types/domains/media_buy/get_media_buys_response.py @@ -62,9 +62,6 @@ class IndicatorTypesEvaluatedEnum(StrEnum): budget_constrained = 'budget_constrained' flight_extension_opportunity = 'flight_extension_opportunity' scale_budget_opportunity = 'scale_budget_opportunity' - budget_constrained_1 = 'budget_constrained' - flight_extension_opportunity_1 = 'flight_extension_opportunity' - scale_budget_opportunity_1 = 'scale_budget_opportunity' class ProposalStatus(StrEnum): @@ -114,13 +111,6 @@ class IndicatorTypesEvaluatedEnum1(StrEnum): budget_constrained = 'budget_constrained' flight_extension_opportunity = 'flight_extension_opportunity' scale_budget_opportunity = 'scale_budget_opportunity' - creative_diversity_low_1 = 'creative_diversity_low' - audience_saturation_1 = 'audience_saturation' - inventory_shortfall_forecast_1 = 'inventory_shortfall_forecast' - pacing_risk_1 = 'pacing_risk' - budget_constrained_1 = 'budget_constrained' - flight_extension_opportunity_1 = 'flight_extension_opportunity' - scale_budget_opportunity_1 = 'scale_budget_opportunity' class Cancellation1(AdCPBaseModel): @@ -151,8 +141,6 @@ class IndicatorTypesEvaluatedEnum2(StrEnum): budget_constrained = 'budget_constrained' flight_extension_opportunity = 'flight_extension_opportunity' scale_budget_opportunity = 'scale_budget_opportunity' - creative_fatigue_1 = 'creative_fatigue' - creative_quality_opportunity_1 = 'creative_quality_opportunity' class Snapshot(AdCPBaseModel): diff --git a/src/adcp/types/domains/media_buy/media_buy_commitment_response.py b/src/adcp/types/domains/media_buy/media_buy_commitment_response.py index c3708eda5e..bfa389ee9b 100644 --- a/src/adcp/types/domains/media_buy/media_buy_commitment_response.py +++ b/src/adcp/types/domains/media_buy/media_buy_commitment_response.py @@ -40,8 +40,6 @@ class Code(StrEnum): inventory_shortfall_forecast = 'inventory_shortfall_forecast' flight_change_creates_pacing_risk = 'flight_change_creates_pacing_risk' fields_ignored_due_to_precedence = 'fields_ignored_due_to_precedence' - inventory_shortfall_forecast_1 = 'inventory_shortfall_forecast' - flight_change_creates_pacing_risk_1 = 'flight_change_creates_pacing_risk' class Status(StrEnum): diff --git a/tests/test_generated_enums_have_no_aliases.py b/tests/test_generated_enums_have_no_aliases.py new file mode 100644 index 0000000000..a8b1fd7bf0 --- /dev/null +++ b/tests/test_generated_enums_have_no_aliases.py @@ -0,0 +1,85 @@ +"""A generated enum names each value once (#1782). + +datamodel-code-generator merges an ``anyOf`` arm's narrower ``enum`` into the +root enum without deduplicating, so ``postal_country_system.System`` carried +``zip = 'zip'`` and ``zip_1 = 'zip'`` (19 enums on 10.0.0b4). Python makes the +second member an alias of the first, so nothing breaks at runtime, but +``__members__``, autocomplete and ``member.name`` then offer names that are not +AdCP values. ``post_generate_fixes.dedupe_enum_member_values`` removes the +repeats; these tests hold the whole generated tree to it. +""" + +from __future__ import annotations + +import importlib +import pkgutil +from enum import Enum +from pathlib import Path + +import adcp.types.domains as domains +from scripts import post_generate_fixes as fixes + + +def _generated_enums() -> list[type[Enum]]: + found: list[type[Enum]] = [] + for info in pkgutil.walk_packages(domains.__path__, domains.__name__ + "."): + try: + module = importlib.import_module(info.name) + except ImportError: # a removed-schema shim that raises on import + continue + for obj in vars(module).values(): + if ( + isinstance(obj, type) + and issubclass(obj, Enum) + and obj.__module__ == module.__name__ + ): + found.append(obj) + return found + + +def test_no_generated_enum_has_an_alias_member() -> None: + offenders = { + f"{enum.__module__}.{enum.__name__}": [ + alias for alias, member in enum.__members__.items() if alias != member.name + ] + for enum in _generated_enums() + } + offenders = {name: aliases for name, aliases in offenders.items() if aliases} + assert offenders == {}, offenders + + +def test_the_postal_system_enum_names_zip_once() -> None: + from adcp.types.domains.core.postal_country_system import System + + assert [m.value for m in System].count("zip") == 1 + assert "zip_1" not in System.__members__ + assert System("zip") is System.zip + + +def test_the_pass_keeps_the_first_member_for_each_value(tmp_path: Path, monkeypatch) -> None: + generated = tmp_path / "domains" + (generated / "core").mkdir(parents=True) + target = generated / "core" / "thing.py" + target.write_text( + "from adcp.types._str_enum import StrEnum\n\n\n" + "class System(StrEnum):\n" + " zip = 'zip'\n" + " zip_plus_four = 'zip_plus_four'\n" + " zip_1 = 'zip'\n" + " zip_plus_four_1 = 'zip_plus_four'\n\n\n" + "class Untouched(StrEnum):\n" + " a = 'a'\n" + " b = 'b'\n\n\n" + "class NotAnEnum:\n" + " x = 'a'\n" + " y = 'a'\n" + ) + monkeypatch.setattr(fixes, "OUTPUT_DIR", generated) + fixes.dedupe_enum_member_values() + once = target.read_text() + fixes.dedupe_enum_member_values() + assert target.read_text() == once, "the pass must be idempotent" + assert "zip_1" not in once and "zip_plus_four_1" not in once + assert " zip = 'zip'\n zip_plus_four = 'zip_plus_four'\n" in once + assert " a = 'a'\n b = 'b'\n" in once + assert " x = 'a'\n y = 'a'\n" in once