Skip to content

Reserve RootModel for value types: RootModel wrappers make extra-field policy unsettable by consumers #1077

Closed
#1084
Closed
Reserve RootModel for value types: RootModel wrappers make extra-field policy unsettable by consumers#1077
#1084
Labels
bugSomething isn't workingclaude-triaged

Description

@KonstantinMirin
KonstantinMirin
opened on Aug 24, 2026

Summary

Please restrict RootModel to genuine value types (a scalar or a single wrapped value like Country, Gtin, PostalArea) and model object/discriminated-union types as plain BaseModel with a discriminated field instead.

A RootModel wrapper cannot carry an extra-field policy, and pydantic refuses to let a subclass add one:

from pydantic import ConfigDict
from adcp.types.generated_poc.core.pricing_option import PricingOption

class OurPricingOption(PricingOption):
    model_config = ConfigDict(extra="forbid")

# pydantic.errors.PydanticUserError:
#   `RootModel` does not support setting `model_config['extra']`

Verified against adcp==6.6.0 / spec 3.1.1 on pydantic 2.12.

Why this matters to a consumer

The generated option members carry extra="allow". A seller implementation that wants a stricter policy — ours is forbid in dev/CI and ignore in production, never allow — has no way to apply it to the union as a whole, because the only place to hook is the RootModel and that is exactly where extra is refused.

Two concrete consequences we hit:

  1. Non-spec fields silently reach the wire. extra="allow" accepts undeclared attributes, and they serialize. We were emitting two fields on pricing options that do not exist in core/pricing-option.json. additionalProperties is absent from that schema, so draft-07 permits them and no conformance check objects — the only thing that would have caught it is an extra policy we could not set.

  2. Legacy field names are accepted indefinitely. With extra="allow", a pre-V3 rate= keyword is accepted and echoed back rather than rejected, so a consumer gets no signal that it should have migrated to fixed_price. Setting forbid on our own subclass surfaced it immediately.

What works today, and what doesn't

The members of the union (CpmPricingOption, VcpmPricingOption, …) are plain BaseModels and subclass cleanly — we can set extra and add internal-only fields with exclude=True on each. So the members are fine; it is only the RootModel wrapper that is unworkable.

Our workaround is to subclass all nine members and rebuild the union ourselves, which means tracking the SDK's shape by hand and re-doing it whenever a member is added. CpaPricingOption and TimeBasedPricingOption already landed after our first pass, which is the maintenance cost this shape imposes.

Requested change

  • Keep RootModel for value types where the wrapped value is the model.
  • For discriminated unions and object types, generate a plain BaseModel carrying the discriminated union as a normal field (e.g. PricingOption.option: Annotated[Cpm | Vcpm | …, Field(discriminator="pricing_model")]), or expose the union as a bare Annotated[...] type alias so consumers can compose it themselves.

Either form lets every consumer apply its own extra policy without re-declaring the SDK's shapes.

Affected types

26 exported RootModels in adcp.types; the ones we consume are AccountReference, Country, GeoCountry, GeoRegion, PostalArea, PropertyTag, PublisherDomain, SignalRef, plus core.pricing_option.PricingOption. Several of those (Country, Gtin, PostalArea) look like legitimate value types; PricingOption and AccountReference are the object/union cases where the wrapper blocks policy.

Activity

  1. bokelley
    added
    claude-triagingTriage routine is actively working on this issue (1-3 min)
    on Aug 24, 2026

  2. bokelley commented on Aug 24, 2026

    @bokelley
    bokelley
    on Aug 24, 2026
    Contributor

    Triage

    Classification: Bug (live gap) + partial version mismatch
    Bucket(s): types (no client label in this repo — flagged for gap)
    Status: ready-for-human

    What the experts said:

    • code-reviewer: PricingOption is already resolved — the public adcp.types.PricingOption is a bare union alias since 8.0.0b7 (aliases.py line 1529). AccountReference is the live gap: the public export re-packages the generated RootModel[AccountReference1 | AccountReference2], reproducing the reported subclassing error. Non-breaking additive fix exists; replacement alias is minor-breaking.
    • dx-expert: Three union-wrapper RootModels in the public namespace that block extra policy: AccountReference, PostalArea, SignalRef. Five additional scalar string wrappers (Country, GeoCountry, GeoRegion, PropertyTag, PublisherDomain) cause .root-access friction but are a separate concern.

    My take: Fixing the union-wrapper cases by shadowing the _generated re-exports with bare union aliases in aliases.py is the right non-codegen path — precedent already set by PricingOption. However, it removes RootModel.model_validate() and isinstance(x, AccountReference) semantics, which is minor-breaking. Flagging for @bokelley to choose whether this ships in a patch or a minor version.

    Implementation scope (union-wrapper fix):

    • src/adcp/types/aliases.py: add AccountReference = AccountReference1 | AccountReference2, PostalArea = PostalArea1 | PostalArea2, SignalRef = SignalRef1 | SignalRef2 | SignalRef3 — mirrors PricingOption at line 1529.
    • Shadow the _generated.py re-exports so aliases win in _eager.py.
    • Migration note in PR: use AccountReferenceById.model_validate(raw) instead of AccountReference.model_validate(raw) — those concrete aliases already exist.
    • Non-breaking alternative: add new AccountReferenceUnion names and keep old RootModel; leaves the friction in place but avoids any break.
    • Also clarify generated_poc.* is unsupported in docs (the reporter's PricingOption example used that internal path).

    Triaged by Claude Code. Session: https://claude.ai/code/${CLAUDE_CODE_REMOTE_SESSION_ID}

    Generated by Claude Code

  3. bokelley
    added
    bugSomething isn't working
    claude-triaged
    and removed
    claude-triagingTriage routine is actively working on this issue (1-3 min)
    on Aug 24, 2026
  4. bokelley
    closed this as completedin #1084on Aug 25, 2026
  5. KonstantinMirin
    added a commit that references this issue on Aug 25, 2026

    refactor: own the pricing-option models so non-spec fields stay off t…

    9964b7e

  6. KonstantinMirin commented on Oct 3, 2026

    @KonstantinMirin
    KonstantinMirin
    on Oct 3, 2026
    CollaboratorAuthor

    #1077 asked for a property, not two fixes: keep RootModel for value types, and
    emit a plain model or a discriminated union where the root composes. The two
    classes named in the issue were resolved by adding them to
    _UNWRAP_TO_UNION in scripts/post_generate_fixes.py. The property was not
    adopted, and the population the issue describes has grown.

    Measured against adcp==8.0.0 (repo at 6677169):

    • 45 types exported from adcp.types are RootModel subclasses.
    • 45 of 45 refuse ConfigDict(extra="forbid") on a subclass.
    • 27 of the 45 wrap something that composes — 24 unions, one object $ref, one
      array, one Literal.
    • _UNWRAP_TO_UNION holds 43 names. It is the only thing that decides which
      wrapper goes; a new schema with a composing root arrives wrapped.
    >>> import adcp.types as T
>>> from pydantic import ConfigDict
>>> class Ours(T.SignalTargeting):
...     model_config = ConfigDict(extra="forbid")
pydantic.errors.PydanticUserError: `RootModel` does not support setting `model_config['extra']`

    Every other generated model accepts that subclass. Which side of the line a type
    falls on is decided by whether someone added its name to the set.

    The name-driven unwrap also drops the discriminator

    This is the part the allowlist makes invisible. unwrap_rootmodel_unions
    replaces the whole class with TypeName = Variant1 | Variant2, discarding the
    root: annotation — and with it the Field(discriminator=...) the generator
    emitted from the schema. Four of the 43 names come from schemas that declare
    discriminator, and all four lose it.

    media-buy/list-products-response.json declares
    discriminator: {propertyName: "outcome"}. On 8.0.0:

    >>> from pydantic import TypeAdapter
>>> import adcp.types as T
>>> TypeAdapter(T.ListProductsResponse).validate_python(
...     {"outcome": "nope", "feed_version": "v1", "cache_scope": "global"})
ValidationError: 5 validation errors
  ListProductsResponse1.outcome      Input should be 'listed'
  ListProductsResponse1.products     Field required
  ListProductsResponse1.cache_scope  Input should be 'public' or 'account'
  ListProductsResponse2.outcome      Input should be 'unchanged'
  ListProductsResponse2.cache_scope  Input should be 'public' or 'account'

    A sender that misspells one tag gets five errors spread across both arms and no
    union_tag_invalid. A caller who sends outcome: "listed" and omits products
    gets four errors, two of them about the arm they did not select. The same holds
    for ControlMediaBuyResponse (7 errors), MediaBuyCommitmentResponse (10) and
    PreviewCreativeResponse (9).

    core/deployment.json declares the same thing and keeps its wrapper, so it keeps
    its discriminator and reports one union_tag_invalid. The SDK has two
    populations with opposite defects, and membership is a hand-maintained list.

    Layer

    CODEGEN. The schemas carry everything needed: oneOf/anyOf says the root
    composes, discriminator says how, a $ref root says the type IS the target.
    datamodel-code-generator resolves all of it and emits it onto the root:
    field; the post-generation pass then either keeps a wrapper that cannot be
    subclassed or deletes a wrapper along with its metadata. Nothing in the schemas
    asks for either.

    Arm names (the related half)

    Arms are named by position — CreateMediaBuyResponse1/2/3,
    Deployment1/Deployment2 — while the schema supplies a name.
    media-buy/list-products-response.json is typical: its arms are
    outcome: "listed" and outcome: "unchanged", emitted as
    ListProductsResponse1 and ListProductsResponse2. Positional names renumber
    on unrelated schema edits — Idempotency3 became Idempotency1 — so a
    consumer's import silently re-points at a different arm.

    Counting schemas with a root-level multi-arm oneOf/anyOf, excluding
    bundled/: 250 root sites, and 87 of them carry a title on every arm.
    Restricted to what codegen reads (mcp/ is excluded from generation): 156 root
    sites, 62 with a title on every arm.

    That is a separate change from the one in #1353, which keeps the generated arm
    names. Filing it here because it is the same defect: schema information read and
    then discarded.

  7. bokelley
    added 2 commits that reference this issue on Oct 4, 2026

    feat(types)!: emit a composing schema root as what it composes (#1353)

    e5c2712

    fix(codegen)!: resolve structural pointer refs to the type they select (

    6fa21d2
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

    bugSomething isn't workingclaude-triaged

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions