fix: unwrap RootModel unions for consumer subclassing#156
fix: unwrap RootModel unions for consumer subclassing#156bokelley merged 3 commits intoadcontextprotocol:mainfrom
Conversation
…assing (adcontextprotocol#155) Pydantic 2 forbids model_config overrides on RootModel subclasses, which blocks consumers who need extra='forbid', custom validators, or additional fields on Request/Response types. All 28 Request/Response RootModel wrappers are now unwrapped to Union type aliases (e.g. `GetSignalsRequest = GetSignalsRequest1 | GetSignalsRequest2`) during post-generation. Value-type RootModels (PricingOption, Destination, etc.) keep their wrapper + __getattr__ proxy since they are not subclassed. Changes: - Add unwrap_rootmodel_unions() to post_generate_fixes.py with _UNWRAP_TO_UNION set - Run unwrap before __getattr__ proxy so only non-unwrapped RootModels get proxied - Add _make_request() helper in simple.py using TypeAdapter for union types - Add CI guard test (test_no_request_response_rootmodels) to catch regressions - Add subclassing + TypeAdapter validation tests - Update all test files to use TypeAdapter instead of .model_validate()
|
All contributors have agreed to the IPR Policy. Thank you! |
|
I have read the IPR Policy |
|
recheck |
bokelley
left a comment
There was a problem hiding this comment.
Nice work on this PR — the approach is sound and the CI guard test is a great addition. A few items to address before we merge:
Must Fix
_make_request() should cache TypeAdapter instances (src/adcp/simple.py)
TypeAdapter() compiles the validation schema on construction — creating one per call is expensive. Please cache with lru_cache:
from functools import lru_cache
@lru_cache(maxsize=None)
def _get_adapter(request_type: type) -> TypeAdapter:
return TypeAdapter(request_type)
def _make_request(request_type: type, kwargs: dict[str, Any]) -> Any:
if isinstance(request_type, UnionType):
return _get_adapter(request_type).validate_python(kwargs)
return request_type(**kwargs)response_type.__name__ will crash on UnionType (src/adcp/utils/response_parser.py, lines ~108 and ~126)
UnionType objects don't have __name__. Use the safe pattern already used on line 221:
getattr(response_type, "__name__", str(response_type))Should Fix
-
Apply
_make_request()uniformly to all request construction sites insimple.py, not just the 5 currently-unwrapped types. If future schema updates unwrap more types, the others will break withTypeError: cannot instantiate UnionType. -
buying_modeadded to integration tests — the old test wasGetProductsRequest(brief="Coffee brands")without specifyingbuying_mode. The new code requires it. Is this an expected UX change for consumers? Worth a note in the PR description. -
Anyimport cleanup logic (lines ~130-138 ofunwrap_rootmodel_unions()) — the outer condition is hard to reason about. The innerif "Any" not in after_importscheck is the real safety guard. Consider simplifying.
Consider (non-blocking)
- The regex
[^\]]+inclass_patterncan't handle nested brackets in type annotations. Works today but fragile — AST-based approach (likeadd_rootmodel_getattr_proxyalready uses) would be more robust. - A test helper for
TypeAdapter(T).validate_python(data)would reduce verbosity across test files. - Verify that
Field(description=..., examples=[...])metadata from the removed RootModel wrappers isn't needed for MCP tool schema generation.
The TypeAdapter caching and __name__ safety are the two blockers — the rest can be follow-ups. Looking forward to merging this!
bokelley
left a comment
There was a problem hiding this comment.
Nice work on this PR — the approach is sound and the CI guard test is a great addition. A few items to address before we merge:
Must Fix
_make_request() should cache TypeAdapter instances (src/adcp/simple.py)
TypeAdapter() compiles the validation schema on construction — creating one per call is expensive. Please cache with lru_cache:
from functools import lru_cache
@lru_cache(maxsize=None)
def _get_adapter(request_type: type) -> TypeAdapter:
return TypeAdapter(request_type)
def _make_request(request_type: type, kwargs: dict[str, Any]) -> Any:
if isinstance(request_type, UnionType):
return _get_adapter(request_type).validate_python(kwargs)
return request_type(**kwargs)response_type.__name__ will crash on UnionType (src/adcp/utils/response_parser.py, lines ~108 and ~126)
UnionType objects don't have __name__. Use the safe pattern already used on line 221:
getattr(response_type, "__name__", str(response_type))Should Fix
-
Apply
_make_request()uniformly to all request construction sites insimple.py, not just the 5 currently-unwrapped types. If future schema updates unwrap more types, the others will break withTypeError: cannot instantiate UnionType. -
buying_modeadded to integration tests — the old test wasGetProductsRequest(brief="Coffee brands")without specifyingbuying_mode. The new code requires it. Is this an expected UX change for consumers? Worth a note in the PR description. -
Anyimport cleanup logic (lines ~130-138 ofunwrap_rootmodel_unions()) — the outer condition is hard to reason about. The innerif "Any" not in after_importscheck is the real safety guard. Consider simplifying.
Consider (non-blocking)
- The regex in
class_patterncan't handle nested brackets in type annotations. Works today but fragile — AST-based approach (likeadd_rootmodel_getattr_proxyalready uses) would be more robust. - A test helper for
TypeAdapter(T).validate_python(data)would reduce verbosity across test files. - Verify that
Field(description=..., examples=[...])metadata from the removed RootModel wrappers isn't needed for MCP tool schema generation.
The TypeAdapter caching and __name__ safety are the two blockers — the rest can be follow-ups. Looking forward to merging this!
Blockers: - Cache TypeAdapter instances in _get_adapter() to avoid per-call schema compilation; uses plain dict (UnionType not hashable for lru_cache) - Replace bare .__name__ with getattr(x, "__name__", str(x)) in response_parser.py to prevent crash on UnionType Should-fix: - Apply _make_request() uniformly to all 16 SimpleAPI methods (only get_signals retains custom dispatch) - Simplify Any import cleanup logic in post_generate_fixes.py - Add buying_mode="brief" to all get_products calls in tests, examples, README, and docstrings Non-blocking improvements: - Replace regex with AST in unwrap_rootmodel_unions() for robustness with nested brackets; use bracket-depth counting for type extraction - Add validate_union() test helper in tests/conftest.py with caching; migrate all 7 test files from raw TypeAdapter usage - Fix basic_usage.py to use GetProductsRequest object instead of kwargs - Fix type annotations: dict[type | UnionType, TypeAdapter[Any]] - Fix import sorting and line length for ruff compliance
|
Thanks for the thorough review @bokelley! All items addressed in 81d3fd3. Blockers:
Should-fix:
Non-blocking (all done):
ruff and mypy both pass clean on all changed files. |
Union type aliases (e.g. GetProductsRequest = Req1 | Req2 | Req3) are not callable and have no .model_validate() — they are types.UnionType objects, not classes. Pydantic's TypeAdapter provides the equivalent runtime validation for union types. - __main__.py: dispatch table stores TypeAdapter instances; call site uses adapter.validate_python(payload) instead of type(**payload) - sponsored_intelligence.py: replace SiSendMessageRequest.model_validate() with TypeAdapter.validate_python() - preview_cache.py: replace PreviewCreativeRequest() constructor with TypeAdapter.validate_python() - CLAUDE.md: document pre-commit quality gates (ruff, mypy, pytest)
…sses Follow-up to adcontextprotocol#156. The previous fix unwrapped RootModel unions to Union type aliases, but Union aliases are also non-subclassable — consumers that do `class MyType(LibraryType)` still break. Root cause: JSON Schema uses anyOf/oneOf with required-only branches to express "at least one of these fields must be set." datamodel-code-generator misinterprets this as a type union, generating separate variant classes (e.g., FrequencyCap1/2/3) plus a RootModel wrapper. Neither RootModels nor Union type aliases can be subclassed by consumers. Fix: Added flatten_validation_oneof() to generate_types.py that detects required-only anyOf/oneOf patterns in schemas and removes them before code generation. This produces a single BaseModel class per type — restoring the v3.6 API surface where all types were subclassable. 10 types flattened: - FrequencyCap, UserMatch, AudienceMember, Catchment (value types) - GetSignalsRequest, UpdateMediaBuyRequest, GetCreativeDeliveryRequest, ProvidePerformanceFeedbackRequest, SiSendMessageRequest, PackageUpdate (request types) Also adds a consumer subclassability contract test that guards 27 consumer-facing types against becoming non-subclassable in future schema updates.
Summary
Fixes #155 — Pydantic 2 forbids
model_configoverrides onRootModelsubclasses, blocking consumers who subclass library Request/Response types withextra="forbid", custom validators, or additional fields.RootModelwrappers to plainUniontype aliases during post-generation (e.g.GetSignalsRequest = GetSignalsRequest1 | GetSignalsRequest2)test_no_request_response_rootmodels) that fails if any future schema adds a new Request/Response RootModel without unwrapping it_make_request()helper insimple.pyusing cachedTypeAdapterfor union type aliasesPricingOption,Destination, etc.) keep their wrapper +__getattr__proxy — only Request/Response types are unwrappedWhat changed
class GetSignalsRequest(RootModel[V1 | V2])GetSignalsRequest = V1 | V2GetSignalsRequest(field=val)TypeAdapter(GetSignalsRequest).validate_python({...})obj.root.fieldobj.field(direct on variant)Pipeline ordering
unwrap_rootmodel_unions()runs beforeadd_rootmodel_getattr_proxy()inpost_generate_fixes.py, so unwrapped types don't get the proxy while remaining RootModels still do.buying_modeis now requiredGetProductsRequestvariants requirebuying_mode(v3 spec change). All examples, tests, README snippets, and docstrings have been updated. This is not a UX regression — v3 clients must includebuying_mode, and servers receiving pre-v3 requests without it should default to"brief".Review feedback addressed
All items from @bokelley's review have been addressed in 81d3fd3:
Blockers (fixed):
TypeAdaptercaching via_get_adapter()with module-level dict cache__name__access withgetattr(x, "__name__", str(x))on all 3 sites inresponse_parser.pyShould-fix (fixed):
_make_request()applied uniformly to all 16 SimpleAPI methods (onlyget_signalsretains custom dispatch due to intentional keyword-based routing)Anyimport cleanup simplified — removed convoluted outer condition, kept innerafter_importsguardbuying_mode="brief"added to allget_productscalls across tests, examples, README, and docstringsNon-blocking (all addressed):
unwrap_rootmodel_unions()with bracket-depth counting for nested genericsvalidate_union()test helper intests/conftest.pywith caching; migrated all 7 test filesFieldmetadata from removed RootModel wrappers — Request/Response types had no meaningful metadata; value-type RootModels are intentionally excludedFix mypy errors from union type aliases (b8eb472)
Unwrapped union type aliases (
types.UnionType) are not callable and have no.model_validate(). Three call sites in the library itself were using these types as if they were still classes:__main__.pyrequest_type(**payload)TypeAdapter[Any]instances, calladapter.validate_python(payload)server/sponsored_intelligence.pySiSendMessageRequest.model_validate(params)TypeAdapter, call.validate_python(params)utils/preview_cache.pyPreviewCreativeRequest(...)constructor callTypeAdapter.validate_python({...})Also added pre-commit quality gate docs to CLAUDE.md (
ruff check src/,mypy src/adcp/,pytest tests/ -v).Test plan
model_config = ConfigDict(extra="forbid")works on variants