Repository navigation
A tolerant canonical model subtypes a strict wire model while lacking one of its fields #1397
Description
Summary
A canonical creative model satisfies isinstance against its strict wire base while lacking one of that base's fields. _DeliveryCreativeManifest is the concrete case: it subclasses the strict manifest wire base and the canonical boundary removes format_id, so it is a subtype that does not carry the parent's full field set.
That is a Liskov violation in the direction that matters here: a function annotated for the strict model accepts the tolerant subclass, and the field it was promised is absent.
Current state
The runtime consequence is closed rather than the design question. The strict creative-manifest wire bases carry revalidate_instances="subclass-instances", so a subclass instance passed where a strict model is expected is revalidated instead of being accepted unchecked. That removes the observable failure — a tolerant instance can no longer pass as strict-validated output without going through validation again.
What remains is the shape itself: the type lattice still says the tolerant model IS the strict model, and only a pydantic config keeps that from being exploitable.
The design question
Should a tolerant, boundary-facing model subtype a strict wire model at all?
Two directions, neither chosen:
- Non-subtyping. The canonical model stops inheriting from the strict base and instead shares a common ancestor carrying the fields both genuinely have.
isinstancethen answers honestly, andrevalidate_instancesbecomes unnecessary rather than load-bearing. Cost: the inheritance that currently carries every injected validator to the canonical model would need another route, and that inheritance is the fix for a real defect. - Keep the subtype, make the contract explicit. Document that the canonical models are deliberately tolerant subtypes, keep
revalidate_instancesas the enforcement, and add a guard that fails if a strict base loses that setting.
The first is the honest lattice. The second is cheaper and keeps the validator inheritance intact. Deciding between them needs more than the change that surfaced it.
Why file it rather than fix it
The subclass change that exposes this is what makes injected validators reach the public canonical models — including enforce_root_required_groups, whose not reaching them was a live defect. Re-opening the inheritance question inside that change would couple a correctness fix to a design decision.
Raised by a reviewer on the pull request that introduces the subclass model, with the request that it be tracked so it does not get lost.
Activity
bokelley commentedon Oct 5, 2026
Triage
Classification: Design / architecture question
Bucket(s): validation (type lattice, Pydantic canonical models) — no exact label match found; label gap noted
Status: deferred
What the experts said:
revalidate_instances="subclass-instances"closes the observable failure — a tolerant instance passed where a strict model is expected is revalidated rather than accepted — so the immediate defect surface is not exposed.isinstanceagainst the strict wire type it weakens; the honest type lattice (non-subtyping) is the more defensible long-term position, but the path through there costs validator inheritance that fixed a live defect in this very PR.My take: This is a genuine design question with real cost in both directions — the non-subtyping path requires a new route for injected validators, while the keep-subtype path makes
revalidate_instancesload-bearing in perpetuity. Neither can be resolved while #1392 is still open; the decision belongs after the inheritance mechanism it introduces has landed and the validator inheritance story is stable.Blocked-on: #1392 — resurfaces on merge.
Triaged by Claude Code. Session: https://claude.ai/code/session_01SiY4ZzxEufy6VgQtSgsaYN
Generated by Claude Code
KonstantinMirin commentedon Oct 10, 2026
A second concrete instance of this shape, found while auditing what
get_productsaccepts, with the measurement that makes the consequence user-visible.canonical_creative.ProductFilterssubclasses the generated wire model and removes one field:Measured against
adcp==9.0.0b2(spec 3.2.1):core/product-filters.jsonis a single schema with 35 properties in every version 3.0 through 3.2, andformat_idsthere carries"deprecated": truewith"Deprecated in AdCP 3.2; removed in AdCP 4.0. ... Use format_kinds or format_option_refs."The generated field propagates that faithfully:FieldInfo.deprecated is True, and attribute access emits aDeprecationWarning. So the deprecation is already expressed and already enforced at runtime on the wire model.The consequence is an audit hazard rather than a runtime failure. The two classes agree on 34 of 35 field names, so a consumer asking "what does this request accept?" by importing the familiar top-level name gets an answer that is correct about every field except the one being audited. We reached exactly that wrong conclusion -- "
format_idsis absent from the product filters" -- about a class our request path never instantiates, and only caught it by resolving the annotation offmodel_fieldsinstead.Two things that might be worth separating in this issue's scope:
The subtyping question you raise (should a tolerant boundary model subtype a strict wire model at all) is about assignability. This instance adds a second axis: the top-level export under the familiar name is not the class request models annotate. Even if the subtype relationship were resolved,
adcp.types.ProductFilterspointing at the narrower class would keep the audit hazard.Since field removal here tracks
deprecated: true, the canonical surface looks derivable from the deprecation markers already in the schema rather than from a per-class boundary declaration -- which would make "canonical = generated minus deprecated" a property of codegen that cannot drift, instead of a relationship maintained alongside it.No behavioural bug to report on our side from this one: our request DTO annotates the wire-faithful class, which is correct for accepting deprecated input through 3.x.
KonstantinMirin commentedon Oct 10, 2026
Correcting my previous comment: the second line of that table is misleading, and the "two classes" framing does not hold for the SDK's own request model.
GetProductsRequestthere was our application DTO, notadcp.types.GetProductsRequest. A reader would reasonably take it to mean the latter, and on the latter the two are the same class. Re-measured:So the export-identity axis I raised — "the top-level export is not the class request models annotate" — does not reproduce against the SDK's own models. The SDK is internally consistent here: its request model and its top-level export are the same canonical class. My apologies for the noise.
What is actually true is narrower and is a property of our side: our DTO annotates
domains.core.product_filters.ProductFilters(35 fields,format_idspresent) because we must accept the deprecated arm through the 3.x window, so the 34/35 difference shows up at our boundary rather than inside the SDK.Two caveats on my measurement, stated so nobody has to re-derive them:
9.0.0b2plus four PRs from this repo carried locally), not against upstreammain. One of those carried changes narrowed the SDK's own request model, which is why the classes coincide for me and may not for you at a given commit. Worth checking onmainbefore treating the consistency as settled._DeliveryCreativeManifestcase this issue is actually about — neither that class norrevalidate_instancesappears anywhere in our tree, so I cannot speak to the manifest instance.The subtyping question the issue raises is untouched by any of this: a tolerant boundary model subclassing a strict wire model and removing one of its fields is still the shape, and
canonical_creative.ProductFilters(_LegacyProductFilters, CanonicalBoundaryModel)is still an instance of it. I just should not have claimed an export mismatch on top of it.