From 566dc7ccc8a0e3a3d41297f4d93dd475c8926772 Mon Sep 17 00:00:00 2001 From: K1rL3s Date: Thu, 23 Jul 2026 17:37:04 +0300 Subject: [PATCH 1/2] feat: inheritance mode, body-field descriptions, keep soft keywords Three additions, driven by generating a hand-written client's shape: - `--inheritance` renders `allOf: [{$ref: Base}, ...]` as a real base class instead of merging the parent's fields into every subtype. A discriminated base stays a model (its own properties survive) and its mapped subtypes inherit from it, re-declaring only the discriminator tag. Declarations are emitted parent-first so a `class Sub(Base)` statement resolves, and model constructors become keyword-only because a subclass may pin an inherited field to a default while adding required fields of its own. - `IRBodyField.description` carries the schema description of a spread request body field, which was silently dropped. `IRParameter` already had it. - `sanitize_identifier` no longer suffixes soft keywords: `type` is a legal attribute name and an extremely common spec field, so `type_` was noise. `_` stays reserved. Verified on a real 130-schema spec: both file layouts x all three serializers generate code that passes `ruff --isolated` and `mypy --strict`. --- README.md | 44 +++- src/unihttp_openapi_generator/cli.py | 8 + src/unihttp_openapi_generator/config.py | 1 + src/unihttp_openapi_generator/config_file.py | 1 + src/unihttp_openapi_generator/ir/builder.py | 135 ++++++++++++- src/unihttp_openapi_generator/ir/models.py | 9 + src/unihttp_openapi_generator/ir/naming.py | 8 +- .../ir/operations.py | 1 + src/unihttp_openapi_generator/pipeline.py | 1 + .../render/file_layout.py | 11 +- .../render/serializers/adaptix.py | 7 +- .../render/serializers/base.py | 6 + .../render/serializers/msgspec.py | 4 +- .../render/serializers/pydantic.py | 2 +- tests/test_builder.py | 191 ++++++++++++++++++ tests/test_naming.py | 10 + tests/test_render_models.py | 60 ++++++ 17 files changed, 482 insertions(+), 17 deletions(-) diff --git a/README.md b/README.md index f301074..4328b2e 100644 --- a/README.md +++ b/README.md @@ -204,6 +204,7 @@ unihttp-openapi-generator generate SPEC [options] | `--style` | `declarative` · `imperative` (`declarative`) | | `--optional` | `none` · `omitted` (`none`) — `omitted` distinguishes absent from null (adaptix) | | `--strip-prefix` | `auto` or a dotted prefix to drop from schema names (e.g. `io.k8s.api.core.v1.Pod` → `CoreV1Pod`) | +| `--inheritance` | off by default — render `allOf: [$ref]` as a base class instead of merging its fields in | | `--check` | run `ruff` and `mypy --strict` on the output | | `--config` | TOML config file | @@ -248,6 +249,7 @@ file_layout = "single" # single | per-object (files on disk) style = "declarative" # declarative | imperative (method style) optional = "none" # none | omitted (optional model fields) strip_prefix = "auto" # "auto" or a dotted prefix to drop from schema names +inheritance = false # allOf: [$ref] -> a base class instead of merged fields check = true # run ruff + mypy --strict on the output ``` @@ -343,12 +345,48 @@ How optional model fields are represented (adaptix only). middle_name: Omittable[str | None] = Omitted() ``` +### Inheritance — `--inheritance` + +What to do with `allOf: [{$ref: Base}, ...]`. + +- off (default) — the base's properties are **merged into** each subtype, and a base + with a `discriminator` becomes a union alias: + ```python + @dataclass + class CallbackButton: + text: str # copied from Button + payload: str + type: Literal['callback'] = 'callback' + + type Button = CallbackButton | LinkButton + ``` +- `--inheritance` — the base stays a class and subtypes **inherit** from it, keeping + only their own properties plus the discriminator tag: + ```python + @dataclass(kw_only=True) + class Button: + type: str + text: str + + @dataclass(kw_only=True) + class CallbackButton(Button): + payload: str + type: Literal['callback'] = 'callback' + ``` + Annotations then refer to the base (`list[Button]`) and `isinstance` works across + the hierarchy. Model constructors become keyword-only: a subclass may pin an + inherited field to a default while adding required fields of its own, which + positional ordering cannot express. Only an `allOf` with exactly one `$ref` maps + onto a base class — several refs are mixin-style composition with no single parent + to pick, so those keep the merge behaviour. + ## OpenAPI coverage - 3.0 and 3.1; JSON or YAML; file or URL; internal and external `$ref`. -- Schemas: objects, `allOf` merge, `oneOf`/`anyOf`, discriminator (including - polymorphic bases), enums and `const`, formats, nullable, `additionalProperties`, - constraints, recursion, and `readOnly` (excluded from request bodies). +- Schemas: objects, `allOf` (merged, or real inheritance with `--inheritance`), + `oneOf`/`anyOf`, discriminator (including polymorphic bases), enums and `const`, + formats, nullable, `additionalProperties`, constraints, recursion, and `readOnly` + (excluded from request bodies). - Operations: path/query/header parameters with defaults, JSON/form/multipart bodies, file uploads, typed responses, and `deprecated`. - Security: apiKey, http bearer/basic, oauth2, openIdConnect. diff --git a/src/unihttp_openapi_generator/cli.py b/src/unihttp_openapi_generator/cli.py index 1f7351d..c809b24 100644 --- a/src/unihttp_openapi_generator/cli.py +++ b/src/unihttp_openapi_generator/cli.py @@ -61,6 +61,13 @@ def generate( str | None, typer.Option("--strip-prefix", help="'auto' or a dotted prefix to drop from schema names."), ] = None, + inheritance: Annotated[ + bool | None, + typer.Option( + "--inheritance/--no-inheritance", + help="Render 'allOf: [$ref]' as a base class instead of merging its fields in.", + ), + ] = None, check: Annotated[bool | None, typer.Option("--check/--no-check")] = None, config: Annotated[ Path | None, @@ -89,6 +96,7 @@ def generate( "optional": optional, "file_layout": file_layout, "strip_prefix": strip_prefix, + "inheritance": inheritance, "check": check, } diff --git a/src/unihttp_openapi_generator/config.py b/src/unihttp_openapi_generator/config.py index ad36dbe..dd2e1d2 100644 --- a/src/unihttp_openapi_generator/config.py +++ b/src/unihttp_openapi_generator/config.py @@ -71,6 +71,7 @@ class GeneratorConfig(BaseModel): optional: OptionalStyle = OptionalStyle.NONE file_layout: FileLayout = FileLayout.SINGLE strip_prefix: str | None = None # "auto" or a dotted prefix to drop from schema names + inheritance: bool = False # allOf: [$ref] -> a real base class instead of merged fields check: bool = False @model_validator(mode="after") diff --git a/src/unihttp_openapi_generator/config_file.py b/src/unihttp_openapi_generator/config_file.py index 5697db4..f1d965d 100644 --- a/src/unihttp_openapi_generator/config_file.py +++ b/src/unihttp_openapi_generator/config_file.py @@ -33,6 +33,7 @@ "optional", "file_layout", "strip_prefix", + "inheritance", "check", } ) diff --git a/src/unihttp_openapi_generator/ir/builder.py b/src/unihttp_openapi_generator/ir/builder.py index 1e81868..271b530 100644 --- a/src/unihttp_openapi_generator/ir/builder.py +++ b/src/unihttp_openapi_generator/ir/builder.py @@ -103,11 +103,13 @@ def __init__( *, omit_optionals: bool = False, strip_prefix: str | None = None, + inheritance: bool = False, ) -> None: self._spec = spec self._resolver = resolver self._root_uri = root_uri self._omit_optionals = omit_optionals + self._inheritance = inheritance self._strip_segments = self._resolve_strip_segments(strip_prefix) self._declarations: dict[str, Declaration] = {} # one registry for all top-level class names (models, enums, aliases, method @@ -144,12 +146,39 @@ def build(self) -> IRDocument: title=info.get("title", "API"), version=info.get("version", "0.0.0"), base_url=base_url, - declarations=list(self._declarations.values()), + declarations=self._ordered_declarations(), operations=operations, security_schemes=self._build_security_schemes(), servers=servers, ) + def _ordered_declarations(self) -> list[Declaration]: + """Declarations in emit order: a base class always precedes its subclasses. + + Subtypes are converted while their base is still being built, so insertion + order alone would put a subclass before the class it inherits from - and a + ``class Sub(Base)`` statement evaluates its base at definition time. + """ + declarations = list(self._declarations.values()) + if not self._inheritance: + return declarations + by_name = {decl.name: decl for decl in declarations} + ordered: list[Declaration] = [] + visited: set[str] = set() + + def visit(decl: Declaration) -> None: + if decl.name in visited: + return + visited.add(decl.name) # marked first: a base cycle must not recurse forever + base = decl.base if isinstance(decl, IRModel) else None + if base is not None and base in by_name: + visit(by_name[base]) + ordered.append(decl) + + for decl in declarations: + visit(decl) + return ordered + def _build_servers(self) -> list[Server]: raw = self._spec.get("servers") or [] servers: list[Server] = [] @@ -292,6 +321,21 @@ def _apply_discriminator_tag(self, key: tuple[str, str], name: str) -> None: f.default = value f.omittable = False return + if decl.base is None: + return + # Inheritance mode: the tag property is declared by the base class, so the + # subtype re-declares it pinned to its own tag. + decl.fields.insert( + 0, + IRField( + name=field_name(prop), + wire_name=prop, + type=LiteralType((value,)), + required=True, + default=value, + has_default=True, + ), + ) def _build_named( self, name: str, schema: Any, base_uri: str, *, is_disc_subtype: bool = False @@ -305,6 +349,17 @@ def _build_named( return disc_raw = schema.get("discriminator") if isinstance(disc_raw, dict) and isinstance(disc_raw.get("mapping"), dict): + if self._inheritance: + # The base keeps its own properties and stays a class; subtypes + # inherit from it instead of being folded into a union alias. It is + # declared *before* they are converted, so their own `_build_object` + # can see that their base resolves to a model. + model = self._build_object(name, schema, base_uri) + self._declarations[name] = model + self._build_mapped_subtypes(disc_raw["mapping"], base_uri, name) + # Re-resolve now that every mapped subtype has a class name. + model.discriminator = self._discriminator(schema, base_uri) + return self._build_discriminated_base(name, schema, base_uri, description) return # A discriminator subtype (``allOf: [{$ref: base}, ...]``) is always a concrete @@ -327,6 +382,18 @@ def _build_named( target = self._convert(schema, base_uri, name) self._declarations[name] = IRAlias(name=name, target=target, description=description) + def _build_mapped_subtypes(self, mapping: dict[str, Any], base_uri: str, hint: str) -> None: + """Force-declare every subtype named in a discriminator ``mapping``. + + Without the union alias nothing else pulls a subtype in, so an unreferenced + variant would silently vanish from the output. Converting them before the + base is built also lets ``_discriminator`` resolve the mapping to class names. + """ + for target in mapping.values(): + ref = self._mapping_ref(target) + if ref is not None: + self._convert_ref(ref, base_uri, hint) + def _build_discriminated_base( self, name: str, schema: dict[str, Any], base_uri: str, description: str | None ) -> None: @@ -504,22 +571,30 @@ def _build_anonymous_object(self, schema: dict[str, Any], base_uri: str, hint: s return RefType(name) def _flatten_object( - self, schema: dict[str, Any], base_uri: str + self, schema: dict[str, Any], base_uri: str, *, keep_base: bool = False ) -> tuple[dict[str, tuple[Any, str]], set[str], Any, Discriminator | None]: properties: dict[str, tuple[Any, str]] = {} required: set[str] = set() additional: Any = None discriminator = self._discriminator(schema, base_uri) + inherited = self._inherited_ref(schema) if keep_base else None for sub in schema.get("allOf", []): sub_schema, sub_base = self._deref(sub, base_uri) if not isinstance(sub_schema, dict): continue p, r, a, d = self._flatten_object(sub_schema, sub_base) - properties.update(p) + if sub is not inherited: + properties.update(p) + if a is not None: + additional = a + # A discriminator belongs to the class that declares it. Inheriting + # one would make every subtype look like a tagged-union base of the + # whole family. + discriminator = discriminator or d + # A base's ``required`` still applies to any property the subtype + # re-declares (specs routinely restate one only to add a description), + # so it is merged even when the properties themselves stay on the base. required |= r - if a is not None: - additional = a - discriminator = discriminator or d for prop_name, prop_schema in schema.get("properties", {}).items(): properties[prop_name] = (prop_schema, base_uri) required |= set(schema.get("required", [])) @@ -527,10 +602,43 @@ def _flatten_object( additional = schema["additionalProperties"] return properties, required, additional, discriminator + @staticmethod + def _inherited_ref(schema: dict[str, Any]) -> dict[str, Any] | None: + """The single ``allOf`` member that should become a real base class, if any. + + Only an unambiguous ``allOf`` with exactly one ``$ref`` maps onto Python + inheritance. With several refs (mixin-style composition) there is no single + parent to pick, so those keep the merge behaviour. + """ + allof = schema.get("allOf") + if not isinstance(allof, list): + return None + refs = [ + member + for member in allof + if isinstance(member, dict) and isinstance(member.get("$ref"), str) + ] + return refs[0] if len(refs) == 1 else None + def _build_object(self, name: str, schema: dict[str, Any], base_uri: str) -> IRModel: - properties, required, additional, discriminator = self._flatten_object(schema, base_uri) + base: str | None = None + if self._inheritance: + inherited = self._inherited_ref(schema) + if inherited is not None: + base_type = self._convert_ref(inherited["$ref"], base_uri, name) + # Only a model can be subclassed; a ref to an enum/alias is still merged. + if isinstance(base_type, RefType) and isinstance( + self._declarations.get(base_type.name), IRModel + ): + base = base_type.name + properties, required, additional, discriminator = self._flatten_object( + schema, base_uri, keep_base=base is not None + ) model = IRModel( - name=name, description=schema.get("description"), discriminator=discriminator + name=name, + description=schema.get("description"), + discriminator=discriminator, + base=base, ) field_names = NameRegistry() for prop_name, (prop_schema, prop_base) in properties.items(): @@ -850,6 +958,9 @@ def _build_body_fields( wire_name=wire_name, type=ftype, required=is_required, + description=( + prop_schema.get("description") if isinstance(prop_schema, dict) else None + ), is_file=is_file, default=default, has_default=has_default, @@ -913,9 +1024,15 @@ def build_ir( *, omit_optionals: bool = False, strip_prefix: str | None = None, + inheritance: bool = False, ) -> IRDocument: return IRBuilder( - spec, resolver, root_uri, omit_optionals=omit_optionals, strip_prefix=strip_prefix + spec, + resolver, + root_uri, + omit_optionals=omit_optionals, + strip_prefix=strip_prefix, + inheritance=inheritance, ).build() diff --git a/src/unihttp_openapi_generator/ir/models.py b/src/unihttp_openapi_generator/ir/models.py index d6db0cf..8f8ae94 100644 --- a/src/unihttp_openapi_generator/ir/models.py +++ b/src/unihttp_openapi_generator/ir/models.py @@ -47,6 +47,13 @@ class IRModel: description: str | None = None additional_properties: IRType | None = None discriminator: Discriminator | None = None + base: str | None = None + """Name of the model this one inherits from (inheritance mode only). + + Set when the schema is ``allOf: [{$ref: Base}, ...]`` and the builder ran with + ``inheritance=True``; ``fields`` then holds only this model's *own* properties. + Without that flag the base's properties are merged in and ``base`` stays None. + """ def imports(self) -> set[Import]: imports: set[Import] = set() @@ -62,6 +69,8 @@ def referenced_models(self) -> set[str]: names |= f.type.referenced_models() if self.additional_properties is not None: names |= self.additional_properties.referenced_models() + if self.base is not None: + names.add(self.base) return names - {self.name} diff --git a/src/unihttp_openapi_generator/ir/naming.py b/src/unihttp_openapi_generator/ir/naming.py index 6b5b72b..9f3b81f 100644 --- a/src/unihttp_openapi_generator/ir/naming.py +++ b/src/unihttp_openapi_generator/ir/naming.py @@ -9,6 +9,12 @@ _LOWER_UPPER = re.compile(r"(?<=[a-z0-9])(?=[A-Z])") _UPPER_RUN = re.compile(r"(?<=[A-Z])(?=[A-Z][a-z])") +# Soft keywords (``type``, ``match``, ``case``, ``_``) are legal identifiers, and a +# spec field named ``type`` is common enough that suffixing it would be noise. Only +# ``_`` is reserved: it is conventionally a throwaway name and reads as a bug in a +# field/parameter position. +_RESERVED_SOFT_KEYWORDS = frozenset({"_"}) + def _split_words(name: str) -> list[str]: spaced = _WORD_BOUNDARY.sub(" ", name) @@ -34,7 +40,7 @@ def sanitize_identifier(name: str, *, fallback: str = "field") -> str: candidate = _WORD_BOUNDARY.sub("_", candidate).strip("_") or fallback if candidate and candidate[0].isdigit(): candidate = f"_{candidate}" - if keyword.iskeyword(candidate) or keyword.issoftkeyword(candidate): + if keyword.iskeyword(candidate) or candidate in _RESERVED_SOFT_KEYWORDS: candidate = f"{candidate}_" return candidate diff --git a/src/unihttp_openapi_generator/ir/operations.py b/src/unihttp_openapi_generator/ir/operations.py index 6880a7d..171263c 100644 --- a/src/unihttp_openapi_generator/ir/operations.py +++ b/src/unihttp_openapi_generator/ir/operations.py @@ -46,6 +46,7 @@ class IRBodyField: wire_name: str type: IRType required: bool + description: str | None = None is_file: bool = False default: Any = None has_default: bool = False diff --git a/src/unihttp_openapi_generator/pipeline.py b/src/unihttp_openapi_generator/pipeline.py index fd31a59..1173c1d 100644 --- a/src/unihttp_openapi_generator/pipeline.py +++ b/src/unihttp_openapi_generator/pipeline.py @@ -57,6 +57,7 @@ def run_generation(spec_source: str, config: GeneratorConfig) -> Path: root_uri=spec_source, omit_optionals=config.optional is OptionalStyle.OMITTED, strip_prefix=config.strip_prefix, + inheritance=config.inheritance, ) root = write_package(doc, config) logger.info("generated %s client at %s", config.package_name, root) diff --git a/src/unihttp_openapi_generator/render/file_layout.py b/src/unihttp_openapi_generator/render/file_layout.py index e11185e..cde0d98 100644 --- a/src/unihttp_openapi_generator/render/file_layout.py +++ b/src/unihttp_openapi_generator/render/file_layout.py @@ -81,13 +81,22 @@ def _type_checking_block(package: str, plan: LayoutPlan, refs: set[str]) -> list def render_declaration_module( decl: Declaration, strategy: SerializerStrategy, package: str, plan: LayoutPlan ) -> str: - """Render a single ``models/.py`` module for one declaration.""" + """Render a single ``models/.py`` module for one declaration. + + A parent model appears in the ``class Sub(Base)`` statement, which is evaluated + at definition time, so it is imported at runtime; every other model reference + lives only inside annotations and stays deferred. + """ imports = set(strategy.declaration_imports(decl)) refs = decl.referenced_models() # Strip cross-model refs from the runtime imports: they live in the # TYPE_CHECKING block (and ``from __future__ import annotations`` keeps the # annotations lazy). Stdlib/typing/serializer imports stay at runtime. imports = {imp for imp in imports if imp.name not in plan.model_modules} + base = decl.base if isinstance(decl, IRModel) else None + if base is not None and base in plan.model_modules: + imports.add(Import(plan.model_dotted(package, base), base)) + refs = refs - {base} body = strategy.render_declaration(decl) tc_lines = _type_checking_block(package, plan, refs) if tc_lines: diff --git a/src/unihttp_openapi_generator/render/serializers/adaptix.py b/src/unihttp_openapi_generator/render/serializers/adaptix.py index 1468966..66f6e8a 100644 --- a/src/unihttp_openapi_generator/render/serializers/adaptix.py +++ b/src/unihttp_openapi_generator/render/serializers/adaptix.py @@ -39,7 +39,12 @@ def _has_factory_default(model: IRModel) -> bool: return any(f.has_default and isinstance(f.default, list | dict) for f in model.fields) def render_model(self, model: IRModel) -> str: - lines = ["@dataclass", f"class {model.name}:"] + # Keyword-only once inheritance is in play: a subclass may pin an inherited + # field to a default while adding required fields of its own, which the + # positional "defaults last" rule forbids. + decorator = "@dataclass(kw_only=True)" if self.uses_inheritance else "@dataclass" + header = f"class {model.name}({model.base}):" if model.base else f"class {model.name}:" + lines = [decorator, header] doc = docstring(model.description, " ") if doc: lines.append(doc.rstrip("\n")) diff --git a/src/unihttp_openapi_generator/render/serializers/base.py b/src/unihttp_openapi_generator/render/serializers/base.py index 49a4ad4..b2fc831 100644 --- a/src/unihttp_openapi_generator/render/serializers/base.py +++ b/src/unihttp_openapi_generator/render/serializers/base.py @@ -72,11 +72,17 @@ def __init__(self) -> None: # strategies that need to inspect sibling models (e.g. pydantic # discriminated unions). Empty unless a document context is bound. self.models_by_name: dict[str, IRModel] = {} + # Whether any model in the bound document inherits from another. Model + # constructors then become keyword-only: a subclass may pin an inherited + # field to a default (a discriminator tag) while declaring required fields + # of its own, which positional ordering cannot express. + self.uses_inheritance = False def bind_document(self, doc: IRDocument) -> None: self.models_by_name = { decl.name: decl for decl in doc.declarations if isinstance(decl, IRModel) } + self.uses_inheritance = any(model.base for model in self.models_by_name.values()) # -- imports --------------------------------------------------------------- diff --git a/src/unihttp_openapi_generator/render/serializers/msgspec.py b/src/unihttp_openapi_generator/render/serializers/msgspec.py index 7abddf3..651d340 100644 --- a/src/unihttp_openapi_generator/render/serializers/msgspec.py +++ b/src/unihttp_openapi_generator/render/serializers/msgspec.py @@ -67,7 +67,9 @@ def _sort_key(f: IRField) -> tuple[bool, bool]: return (f.has_default, f.needs_alias) def render_model(self, model: IRModel) -> str: - lines = [f"class {model.name}(Struct):"] + # See the adaptix strategy: inheritance forces keyword-only constructors. + options = ", kw_only=True" if self.uses_inheritance else "" + lines = [f"class {model.name}({model.base or 'Struct'}{options}):"] doc = docstring(model.description, " ") if doc: lines.append(doc.rstrip("\n")) diff --git a/src/unihttp_openapi_generator/render/serializers/pydantic.py b/src/unihttp_openapi_generator/render/serializers/pydantic.py index 7e1d0cf..93285dd 100644 --- a/src/unihttp_openapi_generator/render/serializers/pydantic.py +++ b/src/unihttp_openapi_generator/render/serializers/pydantic.py @@ -148,7 +148,7 @@ def _pydantic_field_name(cls, name: str) -> str: return safe def render_model(self, model: IRModel) -> str: - lines = [f"class {model.name}(BaseModel):"] + lines = [f"class {model.name}({model.base or 'BaseModel'}):"] doc = docstring(model.description, " ") if doc: lines.append(doc.rstrip("\n")) diff --git a/tests/test_builder.py b/tests/test_builder.py index 8f09456..3f57896 100644 --- a/tests/test_builder.py +++ b/tests/test_builder.py @@ -409,6 +409,47 @@ def test_readonly_json_body_spreads_writable_fields() -> None: assert "id" in {f.name for f in thing.fields} +def test_body_fields_carry_description() -> None: + """Spread body fields keep their schema ``description`` (as parameters do).""" + spec: dict[str, Any] = { + "openapi": "3.1.0", + "info": {"title": "S", "version": "1.0.0"}, + "paths": { + "/d": { + "post": { + "operationId": "createDoc", + "tags": ["d"], + "requestBody": { + "required": True, + "content": { + "application/json": { + "schema": { + "type": "object", + "required": ["title"], + "properties": { + "title": { + "type": "string", + "description": "Document title.", + }, + "body": {"type": "string"}, + }, + } + } + }, + }, + "responses": {"204": {"description": "no content"}}, + } + } + }, + } + ir = build_ir(spec, RefResolver(spec)) + op = next(o for o in ir.operations if o.method_name == "create_doc") + assert op.body is not None + fields = {f.name: f for f in op.body.fields} + assert fields["title"].description == "Document title." + assert fields["body"].description is None + + def test_readonly_form_field_dropped() -> None: spec: dict[str, Any] = { "openapi": "3.1.0", @@ -657,3 +698,153 @@ def test_strip_prefix_auto() -> None: # longest common segment prefix is io.k8s -> stripped from both assert "ApiCoreV1Pod" in names assert "ApimachineryMetaV1ObjectMeta" in names + + +# -- inheritance mode --------------------------------------------------------------- + + +_INHERITANCE_SPEC: dict[str, Any] = { + "openapi": "3.0.0", + "info": {"title": "I", "version": "1.0.0"}, + "paths": {}, + "components": { + "schemas": { + "Button": { + "type": "object", + "required": ["type", "text"], + "properties": {"type": {"type": "string"}, "text": {"type": "string"}}, + "discriminator": { + "propertyName": "type", + "mapping": { + "callback": "#/components/schemas/CallbackButton", + "link": "#/components/schemas/LinkButton", + }, + }, + }, + "CallbackButton": { + "allOf": [ + {"$ref": "#/components/schemas/Button"}, + { + "required": ["payload"], + "properties": { + # restated only to add prose: it must stay required + "text": {"type": "string", "description": "Visible label."}, + "payload": {"type": "string"}, + }, + }, + ] + }, + # marker subtype: nothing but the tag distinguishes it + "LinkButton": {"allOf": [{"$ref": "#/components/schemas/Button"}]}, + "Owner": { + "type": "object", + "required": ["id"], + "properties": {"id": {"type": "integer"}}, + }, + "NamedOwner": { + "allOf": [ + {"$ref": "#/components/schemas/Owner"}, + {"properties": {"name": {"type": "string"}}}, + ] + }, + "Mixed": { + "allOf": [ + {"$ref": "#/components/schemas/Owner"}, + {"$ref": "#/components/schemas/Button"}, + ] + }, + } + }, +} + + +@pytest.fixture +def inherited() -> IRDocument: + spec = _INHERITANCE_SPEC + return build_ir(spec, RefResolver(spec), inheritance=True) + + +def test_inheritance_keeps_parent_fields_on_parent(inherited: IRDocument) -> None: + base = _decl(inherited, "Button") + assert isinstance(base, IRModel) + assert base.base is None + assert [f.name for f in base.fields] == ["type", "text"] + + sub = _decl(inherited, "CallbackButton") + assert sub.base == "Button" + # own fields only: the restated ``text``, the new ``payload``, and the pinned tag + assert {f.name for f in sub.fields} == {"type", "text", "payload"} + + +def test_inheritance_restated_field_keeps_parent_required(inherited: IRDocument) -> None: + sub = _decl(inherited, "CallbackButton") + text = next(f for f in sub.fields if f.name == "text") + # ``required`` comes from Button; without it the field would widen to `str | None` + # and violate the base's declaration. + assert text.required is True + assert text.type.annotation() == "str" + assert text.description == "Visible label." + + +def test_inheritance_pins_discriminator_tag(inherited: IRDocument) -> None: + sub = _decl(inherited, "CallbackButton") + tag = next(f for f in sub.fields if f.name == "type") + assert tag.type == LiteralType(("callback",)) + assert tag.has_default is True + assert tag.default == "callback" + + +def test_inheritance_keeps_marker_subtype_as_class(inherited: IRDocument) -> None: + marker = _decl(inherited, "LinkButton") + assert isinstance(marker, IRModel) + assert marker.base == "Button" + assert [f.name for f in marker.fields] == ["type"] + + +def test_inheritance_discriminated_base_is_a_model(inherited: IRDocument) -> None: + base = _decl(inherited, "Button") + assert isinstance(base, IRModel) + assert base.discriminator is not None + assert base.discriminator.mapping == { + "callback": "CallbackButton", + "link": "LinkButton", + } + + +def test_inheritance_subtype_does_not_inherit_discriminator(inherited: IRDocument) -> None: + # A discriminator belongs to the class that declares it: copying it down would + # make every subtype look like a tagged-union base of the whole family. + assert _decl(inherited, "CallbackButton").discriminator is None + assert _decl(inherited, "LinkButton").discriminator is None + + +def test_inheritance_orders_bases_before_subclasses(inherited: IRDocument) -> None: + order = [d.name for d in inherited.declarations] + assert order.index("Button") < order.index("CallbackButton") + assert order.index("Button") < order.index("LinkButton") + assert order.index("Owner") < order.index("NamedOwner") + + +def test_inheritance_plain_allof_ref_becomes_base(inherited: IRDocument) -> None: + sub = _decl(inherited, "NamedOwner") + assert sub.base == "Owner" + assert [f.name for f in sub.fields] == ["name"] + assert sub.referenced_models() == {"Owner"} + + +def test_inheritance_multiple_refs_still_merge(inherited: IRDocument) -> None: + # Two `$ref`s give no single parent to pick, so the merge behaviour is kept. + mixed = _decl(inherited, "Mixed") + assert mixed.base is None + assert {f.name for f in mixed.fields} == {"id", "type", "text"} + + +def test_without_inheritance_parent_fields_are_merged() -> None: + spec = _INHERITANCE_SPEC + ir = build_ir(spec, RefResolver(spec)) + sub = _decl(ir, "CallbackButton") + assert isinstance(sub, IRModel) + assert sub.base is None + assert {f.name for f in sub.fields} == {"type", "text", "payload"} + # the discriminated base collapses into a union alias, as before + assert isinstance(_decl(ir, "Button"), IRAlias) diff --git a/tests/test_naming.py b/tests/test_naming.py index b2014dd..b783afa 100644 --- a/tests/test_naming.py +++ b/tests/test_naming.py @@ -47,6 +47,16 @@ def test_field_name_handles_keyword() -> None: assert field_name("for") == "for_" +def test_field_name_keeps_soft_keywords() -> None: + # Soft keywords are legal identifiers; ``type`` in particular is a very common + # spec field name and suffixing it would be pure noise. + assert field_name("type") == "type" + assert field_name("match") == "match" + assert field_name("case") == "case" + # ``_`` reads as a throwaway name, so it stays reserved. + assert field_name("_") == "__" + + def test_field_name_handles_leading_digit() -> None: assert field_name("2fa") == "_2fa" diff --git a/tests/test_render_models.py b/tests/test_render_models.py index cfcfff9..3ad78fc 100644 --- a/tests/test_render_models.py +++ b/tests/test_render_models.py @@ -339,3 +339,63 @@ def test_docstring_with_backslash_is_raw_and_warning_free() -> None: with warnings.catch_warnings(): warnings.simplefilter("error") # SyntaxWarning -> error compile(src, "t.py", "exec") # must not raise + + +# -- inheritance mode --------------------------------------------------------------- + + +_INHERITED_SPEC: dict[str, Any] = { + "openapi": "3.0.0", + "info": {"title": "I", "version": "1.0.0"}, + "paths": {}, + "components": { + "schemas": { + "Button": { + "type": "object", + "required": ["type", "text"], + "properties": {"type": {"type": "string"}, "text": {"type": "string"}}, + "discriminator": { + "propertyName": "type", + "mapping": {"callback": "#/components/schemas/CallbackButton"}, + }, + }, + "CallbackButton": { + "allOf": [ + {"$ref": "#/components/schemas/Button"}, + {"required": ["payload"], "properties": {"payload": {"type": "string"}}}, + ] + }, + } + }, +} + + +@pytest.mark.parametrize( + "serializer", [Serializer.ADAPTIX, Serializer.PYDANTIC, Serializer.MSGSPEC] +) +def test_inherited_models_subclass_their_base(serializer: Serializer, tmp_path: Path) -> None: + ir = build_ir(_INHERITED_SPEC, RefResolver(_INHERITED_SPEC), inheritance=True) + source = format_python( + render_models_module(ir, get_strategy(serializer)), filename="models.py" + ) + assert "class CallbackButton(Button" in source + module = _load(source, tmp_path, f"genmodels_inherit_{serializer.value}") + assert issubclass(module.CallbackButton, module.Button) + # A subclass pinning an inherited field to a default while adding a required one + # of its own only works with keyword-only construction. + button = module.CallbackButton(text="hi", payload="p") + assert button.type == "callback" + assert button.text == "hi" + + +def test_inherited_adaptix_models_are_kw_only() -> None: + ir = build_ir(_INHERITED_SPEC, RefResolver(_INHERITED_SPEC), inheritance=True) + source = render_models_module(ir, get_strategy(Serializer.ADAPTIX)) + assert "@dataclass(kw_only=True)" in source + assert "@dataclass\n" not in source + + +def test_models_without_inheritance_keep_positional_dataclasses() -> None: + ir = build_ir(_INHERITED_SPEC, RefResolver(_INHERITED_SPEC)) + source = render_models_module(ir, get_strategy(Serializer.ADAPTIX)) + assert "@dataclass(kw_only=True)" not in source From 53f0765d622e4580ab9fe9d9ba1b3abe1f2a2a2e Mon Sep 17 00:00:00 2001 From: K1rL3s Date: Thu, 23 Jul 2026 17:25:06 +0300 Subject: [PATCH 2/2] fix: correctness fixes for inheritance mode from code review Review of 566dc7c found the inheritance path silently changing decoded data in several shapes. Fixes, most severe first: - A `oneOf` + `discriminator` union holder is no longer force-built as a class. It declares no properties, so `--inheritance` produced `class Button: pass` and every `list[Button]` payload decoded into it, dropping each variant's fields. It stays a union alias; only bases with their own properties become classes. Verified: pydantic again decodes `CallbackButton(payload=...)` and `isinstance(x, ButtonBase)` still holds. - The pinned discriminator tag is reserved against the subtype's existing field names. A sibling property whose wire name only differs in case (`Type` vs `type`) snake-cases to the same identifier, and the two class attributes collapsed into one -- destroying the tag, with ruff reporting nothing. - A subtype that restates an inherited property only to attach prose, or to relax it to nullable, now inherits it instead of emitting an override. `v: str | None` over the base's `v: str` is an `[assignment]` error under `mypy --strict`, so `--inheritance --check` failed on ordinary specs. Genuine narrowings (a `Literal` tag over a `str`) are kept. - The base class is resolved from the schema, not from the half-built `_declarations` registry. A base whose own body refers back to its subtype (a recursive hierarchy) has no entry yet, so inheritance silently degraded to a field merge based on nothing but graph traversal order. - Keyword-only constructors are limited to the models in a hierarchy. One `allOf` subtype used to flip every model in the package, breaking positional construction for models with no relation to it. - A discriminated base kept as a class emits its mapping as a comment. No serializer resolves a subtype from a base-class annotation on its own, and `IRModel.discriminator` was read by nobody, so that was dropped on the floor. - `IRBodyField.description` (and `IRParameter.description`) now reach the generated code as PEP 258 attribute docstrings. Both were carried through the IR and rendered nowhere. Also: `IRModel.base` -> `base_model`, so it can't be confused with `IREnum.base` (the enum's "str"/"int" value type); soft keywords use an allow-list over `keyword.issoftkeyword` so a future Python's new soft keyword stays guarded; `_ordered_declarations` breaks an inheritance cycle instead of emitting a class before its base; the two discriminator-mapping loops share one helper; `_inherited_ref` is computed once and passed down. Verified: both file layouts x all three serializers generate code that passes `ruff check --isolated` and `mypy --strict` (only the pre-existing `BaseMethod.__init_subclass__` no-untyped-call remains). --- README.md | 33 ++- src/unihttp_openapi_generator/ir/builder.py | 212 ++++++++++++++--- src/unihttp_openapi_generator/ir/models.py | 12 +- src/unihttp_openapi_generator/ir/naming.py | 16 +- .../render/file_layout.py | 2 +- .../render/methods.py | 22 +- .../render/serializers/adaptix.py | 11 +- .../render/serializers/base.py | 48 +++- .../render/serializers/msgspec.py | 4 +- .../render/serializers/pydantic.py | 2 +- tests/test_builder.py | 220 ++++++++++++++++-- tests/test_render_methods.py | 54 +++++ tests/test_render_models.py | 48 +++- 13 files changed, 593 insertions(+), 91 deletions(-) diff --git a/README.md b/README.md index 4328b2e..4d12c53 100644 --- a/README.md +++ b/README.md @@ -373,12 +373,33 @@ What to do with `allOf: [{$ref: Base}, ...]`. payload: str type: Literal['callback'] = 'callback' ``` - Annotations then refer to the base (`list[Button]`) and `isinstance` works across - the hierarchy. Model constructors become keyword-only: a subclass may pin an - inherited field to a default while adding required fields of its own, which - positional ordering cannot express. Only an `allOf` with exactly one `$ref` maps - onto a base class — several refs are mixin-style composition with no single parent - to pick, so those keep the merge behaviour. + `isinstance` then works across the hierarchy, and a subtype's own properties stay in + one place instead of being copied into every variant. + + Scope and rules: + + - Only an `allOf` with exactly **one** `$ref` maps onto a base class — several refs + are mixin-style composition with no single parent to pick, so those keep the merge + behaviour. So does a `$ref` to an enum or a non-object schema. + - Only a base that declares **its own properties** becomes a class. The usual + polymorphism idiom puts the discriminator on a bare `oneOf` holder that has no + properties at all; there is nothing to inherit from it, so it stays a union alias + (`type Button = CallbackButton | LinkButton`) and keeps decoding into the concrete + variant. `--inheritance` only changes how the subtypes get *their* shared fields. + - Constructors become keyword-only **for the models in a hierarchy** — a subclass may + pin an inherited field to a default while adding required fields of its own, which + positional ordering cannot express. Models outside every hierarchy are untouched. + - A subtype that restates an inherited property just to attach prose, or to relax it + to nullable, simply **inherits** it: re-declaring `v: str | None` over the base's + `v: str` is rejected by `mypy --strict`. Genuine narrowings (a `Literal` tag over a + `str`) are kept. + + One thing to know: when a discriminated base *does* stay a class, no serializer + resolves the concrete subtype from a base-class annotation on its own — a field typed + `Button` decodes into `Button`. The generated class carries a + `# discriminator: type (callback=CallbackButton, ...)` comment with the mapping so + the tagged decoding can be wired in `_serialization.py`. Leave `--inheritance` off if + you want polymorphic responses to parse into subtypes out of the box. ## OpenAPI coverage diff --git a/src/unihttp_openapi_generator/ir/builder.py b/src/unihttp_openapi_generator/ir/builder.py index 271b530..1aa5568 100644 --- a/src/unihttp_openapi_generator/ir/builder.py +++ b/src/unihttp_openapi_generator/ir/builder.py @@ -2,6 +2,7 @@ from __future__ import annotations +import logging from typing import Any from urllib.parse import urlsplit @@ -54,6 +55,8 @@ ) from unihttp_openapi_generator.refs import RefResolver +logger = logging.getLogger(__name__) + _HTTP_METHODS = ("get", "put", "post", "delete", "patch", "head", "options", "trace") # Identifiers imported into generated modules; a model/method class must never reuse one @@ -164,15 +167,28 @@ def _ordered_declarations(self) -> list[Declaration]: return declarations by_name = {decl.name: decl for decl in declarations} ordered: list[Declaration] = [] - visited: set[str] = set() + done: set[str] = set() + on_stack: set[str] = set() def visit(decl: Declaration) -> None: - if decl.name in visited: + if decl.name in done: return - visited.add(decl.name) # marked first: a base cycle must not recurse forever - base = decl.base if isinstance(decl, IRModel) else None + # Grey marking: a name still on the stack means the base chain loops back + # on itself. Emitting either end first is wrong, so break the cycle by + # dropping the inheritance edge rather than emitting unresolvable code. + on_stack.add(decl.name) + base = decl.base_model if isinstance(decl, IRModel) else None if base is not None and base in by_name: - visit(by_name[base]) + if base in on_stack: + logger.warning( + "inheritance cycle at %r -> %r; dropping the base class", decl.name, base + ) + assert isinstance(decl, IRModel) + decl.base_model = None + else: + visit(by_name[base]) + on_stack.discard(decl.name) + done.add(decl.name) ordered.append(decl) for decl in declarations: @@ -321,14 +337,21 @@ def _apply_discriminator_tag(self, key: tuple[str, str], name: str) -> None: f.default = value f.omittable = False return - if decl.base is None: + if decl.base_model is None: return # Inheritance mode: the tag property is declared by the base class, so the - # subtype re-declares it pinned to its own tag. + # subtype re-declares it pinned to its own tag. The python name has to be + # reserved against the fields already on this model: a sibling property whose + # wire name only differs in case (``Type`` vs ``type``) snake-cases to the same + # identifier, and two identically named class attributes would silently + # collapse into one -- destroying the tag. + used = NameRegistry() + for existing in decl.fields: + used.reserve(existing.name) decl.fields.insert( 0, IRField( - name=field_name(prop), + name=used.reserve(field_name(prop)), wire_name=prop, type=LiteralType((value,)), required=True, @@ -349,14 +372,20 @@ def _build_named( return disc_raw = schema.get("discriminator") if isinstance(disc_raw, dict) and isinstance(disc_raw.get("mapping"), dict): - if self._inheritance: + # ``_is_object`` is the load-bearing guard: only a base that declares its + # own structure can *be* a class. The common OpenAPI idiom puts the + # discriminator on a bare ``oneOf`` holder that has no properties of its + # own -- turning that into a class would emit an empty ``class Base: pass`` + # and silently swallow every payload annotated with it, so it stays a union + # alias even in inheritance mode. + if self._inheritance and self._is_object(schema): # The base keeps its own properties and stays a class; subtypes # inherit from it instead of being folded into a union alias. It is # declared *before* they are converted, so their own `_build_object` # can see that their base resolves to a model. model = self._build_object(name, schema, base_uri) self._declarations[name] = model - self._build_mapped_subtypes(disc_raw["mapping"], base_uri, name) + self._convert_mapped_subtypes(disc_raw["mapping"], base_uri, name) # Re-resolve now that every mapped subtype has a class name. model.discriminator = self._discriminator(schema, base_uri) return @@ -382,17 +411,23 @@ def _build_named( target = self._convert(schema, base_uri, name) self._declarations[name] = IRAlias(name=name, target=target, description=description) - def _build_mapped_subtypes(self, mapping: dict[str, Any], base_uri: str, hint: str) -> None: - """Force-declare every subtype named in a discriminator ``mapping``. - - Without the union alias nothing else pulls a subtype in, so an unreferenced - variant would silently vanish from the output. Converting them before the - base is built also lets ``_discriminator`` resolve the mapping to class names. + def _convert_mapped_subtypes( + self, mapping: dict[str, Any], base_uri: str, hint: str + ) -> list[IRType]: + """Convert every subtype named in a discriminator ``mapping``, in mapping order. + + Shared by both discriminated-base strategies. In union mode the results are the + union members; in inheritance mode they are discarded and this runs purely for + its side effect -- without the union alias nothing else pulls a subtype in, so + an unreferenced variant would silently vanish from the output. Either way the + conversion also populates ``_ref_to_name`` so ``_discriminator`` can resolve the + mapping to generated class names. """ - for target in mapping.values(): - ref = self._mapping_ref(target) - if ref is not None: - self._convert_ref(ref, base_uri, hint) + return [ + self._convert_ref(ref, base_uri, hint) + for target in mapping.values() + if (ref := self._mapping_ref(target)) is not None + ] def _build_discriminated_base( self, name: str, schema: dict[str, Any], base_uri: str, description: str | None @@ -401,11 +436,7 @@ def _build_discriminated_base( mapping = schema["discriminator"]["mapping"] members: list[IRType] = [] seen: set[str] = set() - for target in mapping.values(): - ref = self._mapping_ref(target) - if ref is None: - continue - ir = self._convert_ref(ref, base_uri, name) + for ir in self._convert_mapped_subtypes(mapping, base_uri, name): anno = ir.annotation() if anno not in seen: seen.add(anno) @@ -571,13 +602,19 @@ def _build_anonymous_object(self, schema: dict[str, Any], base_uri: str, hint: s return RefType(name) def _flatten_object( - self, schema: dict[str, Any], base_uri: str, *, keep_base: bool = False + self, schema: dict[str, Any], base_uri: str, *, inherited: dict[str, Any] | None = None ) -> tuple[dict[str, tuple[Any, str]], set[str], Any, Discriminator | None]: + """Merge an ``allOf`` chain into one property set. + + ``inherited`` is the single ``allOf`` member the caller already turned into a + real base class (inheritance mode); its properties stay on the base instead of + being copied down. It is passed in rather than recomputed so the caller's + decision and this merge can never disagree. + """ properties: dict[str, tuple[Any, str]] = {} required: set[str] = set() additional: Any = None discriminator = self._discriminator(schema, base_uri) - inherited = self._inherited_ref(schema) if keep_base else None for sub in schema.get("allOf", []): sub_schema, sub_base = self._deref(sub, base_uri) if not isinstance(sub_schema, dict): @@ -620,37 +657,136 @@ def _inherited_ref(schema: dict[str, Any]) -> dict[str, Any] | None: ] return refs[0] if len(refs) == 1 else None + def _resolve_base_model( + self, inherited: dict[str, Any], base_uri: str, hint: str + ) -> str | None: + """Generated class name to subclass for an ``allOf`` ref, or None to merge. + + Only a model can be subclassed, so a ref to an enum/alias/scalar keeps the merge + behaviour. The check cannot go through ``self._declarations``: a base whose own + body refers back to this subtype (a recursive hierarchy) is still mid-build and + has no entry yet, which would silently downgrade the subtype to a merge based on + nothing but graph traversal order. Decide from the *schema* instead, using the + same predicate ``_build_named`` will apply when it declares the base. + """ + base_type = self._convert_ref(inherited["$ref"], base_uri, hint) + if not isinstance(base_type, RefType): + return None + declared = self._declarations.get(base_type.name) + if declared is not None: + return base_type.name if isinstance(declared, IRModel) else None + resolved = self._resolver.resolve_ref(inherited["$ref"], base_uri) + key = (resolved.base_uri, resolved.pointer) + will_be_model = self._declares_model( + resolved.value, is_disc_subtype=key in self._disc_subtype + ) + return base_type.name if will_be_model else None + + def _declares_model(self, schema: Any, *, is_disc_subtype: bool = False) -> bool: + """Whether ``_build_named`` will declare ``schema`` as an ``IRModel``. + + Mirrors the dispatch in ``_build_named``; kept next to nothing else so the two + stay reviewable side by side. + """ + if not isinstance(schema, dict): + return False + if "enum" in schema and "properties" not in schema: + return False + disc = schema.get("discriminator") + if isinstance(disc, dict) and isinstance(disc.get("mapping"), dict): + return self._inheritance and self._is_object(schema) + if is_disc_subtype and "allOf" in schema: + return True + return self._is_object(schema) + def _build_object(self, name: str, schema: dict[str, Any], base_uri: str) -> IRModel: base: str | None = None - if self._inheritance: - inherited = self._inherited_ref(schema) - if inherited is not None: - base_type = self._convert_ref(inherited["$ref"], base_uri, name) - # Only a model can be subclassed; a ref to an enum/alias is still merged. - if isinstance(base_type, RefType) and isinstance( - self._declarations.get(base_type.name), IRModel - ): - base = base_type.name + inherited = self._inherited_ref(schema) if self._inheritance else None + if inherited is not None: + base = self._resolve_base_model(inherited, base_uri, name) properties, required, additional, discriminator = self._flatten_object( - schema, base_uri, keep_base=base is not None + schema, base_uri, inherited=inherited if base is not None else None ) model = IRModel( name=name, description=schema.get("description"), discriminator=discriminator, - base=base, + base_model=base, ) field_names = NameRegistry() for prop_name, (prop_schema, prop_base) in properties.items(): f = self._build_field(name, prop_name, prop_schema, prop_base, prop_name in required) f.name = field_names.reserve(f.name) # distinct wire names can collapse (e.g. +1/-1) model.fields.append(f) + if base is not None: + self._drop_unsafe_overrides(model, base) if isinstance(additional, dict): model.additional_properties = self._convert(additional, base_uri, name + "Value") elif additional is True: model.additional_properties = ANY return model + def _drop_unsafe_overrides(self, model: IRModel, base: str) -> None: + """Remove re-declared inherited fields that would not type-check as overrides. + + Specs routinely restate a base property in a subtype just to attach prose, or to + relax it to nullable. Re-emitting those produces ``class Sub(Base)`` with an + attribute whose type is not a subtype of the base's, which ``mypy --strict`` + rejects outright (``Incompatible types in assignment``). The base's declaration + already covers the field, so anything that is not a genuine narrowing is + dropped and simply inherited. + """ + parent = self._declarations.get(base) + if not isinstance(parent, IRModel): + return + inherited_types = {f.wire_name: f.type for f in parent.fields} + kept: list[IRField] = [] + for f in model.fields: + base_type = inherited_types.get(f.wire_name) + if base_type is None or self._is_narrowing(f.type, base_type): + kept.append(f) + else: + logger.debug( + "%s.%s re-declares %s.%s incompatibly (%s vs %s); inheriting instead", + model.name, + f.name, + base, + f.wire_name, + f.type.annotation(), + base_type.annotation(), + ) + model.fields = kept + + @classmethod + def _is_narrowing(cls, sub: IRType, base: IRType) -> bool: + """Whether ``sub`` is safe to re-declare over an inherited ``base`` annotation. + + Deliberately conservative: it only says yes for the shapes that are provably + assignable, because a false yes emits code that fails ``mypy --strict`` while a + false no merely inherits a slightly less precise type. + """ + if sub.annotation() == base.annotation(): + return False # a pure restatement: nothing to gain, just inherit it + if isinstance(base, PrimitiveType) and base.py == "Any": + return True + if isinstance(base, OptionalType): + # ``T | None`` admits ``T`` and anything that narrows ``T``. + return sub.annotation() == base.inner.annotation() or cls._is_narrowing(sub, base.inner) + if isinstance(sub, OptionalType): + return False # adding None to a non-optional base widens it + if isinstance(sub, LiteralType): + # The discriminator-tag case: Literal["a"] over a str/int base, or over a + # wider Literal that already admits every value. + if isinstance(base, LiteralType): + return set(sub.values) <= set(base.values) + return isinstance(base, PrimitiveType) and base.py in ("str", "int", "bool") + if isinstance(base, UnionType): + return any( + cls._is_narrowing(sub, m) or sub.annotation() == m.annotation() + for m in base.members + ) + return False + @staticmethod def _default_assignable(default: Any, ftype: IRType) -> bool: """Whether ``default`` can be written as a Python literal of ``ftype``.""" diff --git a/src/unihttp_openapi_generator/ir/models.py b/src/unihttp_openapi_generator/ir/models.py index 8f8ae94..bc18531 100644 --- a/src/unihttp_openapi_generator/ir/models.py +++ b/src/unihttp_openapi_generator/ir/models.py @@ -47,12 +47,16 @@ class IRModel: description: str | None = None additional_properties: IRType | None = None discriminator: Discriminator | None = None - base: str | None = None + base_model: str | None = None """Name of the model this one inherits from (inheritance mode only). Set when the schema is ``allOf: [{$ref: Base}, ...]`` and the builder ran with ``inheritance=True``; ``fields`` then holds only this model's *own* properties. - Without that flag the base's properties are merged in and ``base`` stays None. + Without that flag the base's properties are merged in and this stays None. + + Deliberately *not* named ``base``: ``IREnum.base`` is the enum's value type + ("str"/"int"), so a ``getattr(decl, "base", None)`` loop over declarations would + silently read an enum's ``"str"`` as a superclass name. """ def imports(self) -> set[Import]: @@ -69,8 +73,8 @@ def referenced_models(self) -> set[str]: names |= f.type.referenced_models() if self.additional_properties is not None: names |= self.additional_properties.referenced_models() - if self.base is not None: - names.add(self.base) + if self.base_model is not None: + names.add(self.base_model) return names - {self.name} diff --git a/src/unihttp_openapi_generator/ir/naming.py b/src/unihttp_openapi_generator/ir/naming.py index 9f3b81f..4d008c8 100644 --- a/src/unihttp_openapi_generator/ir/naming.py +++ b/src/unihttp_openapi_generator/ir/naming.py @@ -9,11 +9,14 @@ _LOWER_UPPER = re.compile(r"(?<=[a-z0-9])(?=[A-Z])") _UPPER_RUN = re.compile(r"(?<=[A-Z])(?=[A-Z][a-z])") -# Soft keywords (``type``, ``match``, ``case``, ``_``) are legal identifiers, and a -# spec field named ``type`` is common enough that suffixing it would be noise. Only -# ``_`` is reserved: it is conventionally a throwaway name and reads as a bug in a -# field/parameter position. -_RESERVED_SOFT_KEYWORDS = frozenset({"_"}) +# Soft keywords are legal identifiers, and a spec field named ``type`` is common +# enough that suffixing it would be noise -- so they are allowed through by name. +# An allow-list (rather than a deny-list) keeps the guard correct when a future +# Python release promotes a new word to a soft keyword: unknown soft keywords keep +# the ``_`` suffix until they are reviewed and added here. +# ``_`` is deliberately absent: it is conventionally a throwaway name and reads as a +# bug in a field/parameter position. +_ALLOWED_SOFT_KEYWORDS = frozenset({"type", "match", "case"}) def _split_words(name: str) -> list[str]: @@ -40,7 +43,8 @@ def sanitize_identifier(name: str, *, fallback: str = "field") -> str: candidate = _WORD_BOUNDARY.sub("_", candidate).strip("_") or fallback if candidate and candidate[0].isdigit(): candidate = f"_{candidate}" - if keyword.iskeyword(candidate) or candidate in _RESERVED_SOFT_KEYWORDS: + soft = keyword.issoftkeyword(candidate) and candidate not in _ALLOWED_SOFT_KEYWORDS + if keyword.iskeyword(candidate) or soft: candidate = f"{candidate}_" return candidate diff --git a/src/unihttp_openapi_generator/render/file_layout.py b/src/unihttp_openapi_generator/render/file_layout.py index cde0d98..1a3f474 100644 --- a/src/unihttp_openapi_generator/render/file_layout.py +++ b/src/unihttp_openapi_generator/render/file_layout.py @@ -93,7 +93,7 @@ def render_declaration_module( # TYPE_CHECKING block (and ``from __future__ import annotations`` keeps the # annotations lazy). Stdlib/typing/serializer imports stay at runtime. imports = {imp for imp in imports if imp.name not in plan.model_modules} - base = decl.base if isinstance(decl, IRModel) else None + base = decl.base_model if isinstance(decl, IRModel) else None if base is not None and base in plan.model_modules: imports.add(Import(plan.model_dotted(package, base), base)) refs = refs - {base} diff --git a/src/unihttp_openapi_generator/render/methods.py b/src/unihttp_openapi_generator/render/methods.py index 6b6e5bf..8e0ae2a 100644 --- a/src/unihttp_openapi_generator/render/methods.py +++ b/src/unihttp_openapi_generator/render/methods.py @@ -54,6 +54,7 @@ class OperationField: has_default: bool default: object is_factory: bool # default needs ``field(default_factory=...)`` semantics + description: str | None = None # schema prose, rendered as an attribute docstring def operation_fields(op: IROperation) -> list[OperationField]: @@ -68,9 +69,12 @@ def add( is_required: bool, has_default: bool, default: object, + description: str | None = None, ) -> None: is_factory = has_default and not is_required and isinstance(default, list | dict) - spec = OperationField(name, marker, inner, is_required, has_default, default, is_factory) + spec = OperationField( + name, marker, inner, is_required, has_default, default, is_factory, description + ) (required if is_required else optional).append(spec) for param in op.parameters: @@ -82,6 +86,7 @@ def add( param.required, param.has_default, param.default, + param.description, ) if op.body is not None: @@ -96,7 +101,15 @@ def add( marker = "Body" else: marker = "Form" - add(f.name, marker, f.type.annotation(), f.required, f.has_default, f.default) + add( + f.name, + marker, + f.type.annotation(), + f.required, + f.has_default, + f.default, + f.description, + ) return [*required, *optional] @@ -119,6 +132,11 @@ def _collect_field_lines(op: IROperation) -> tuple[list[str], set[str], bool, bo else: uses_omitted = True lines.append(f"{spec.py_name}: {spec.marker}[Omittable[{spec.inner}]] = Omitted()") + # PEP 258 attribute docstring: the only place a parameter's / body field's + # schema prose can land without changing the constructor signature. + doc = docstring(spec.description, "") + if doc: + lines.extend(doc.rstrip("\n").split("\n")) return lines, markers, uses_omitted, uses_field diff --git a/src/unihttp_openapi_generator/render/serializers/adaptix.py b/src/unihttp_openapi_generator/render/serializers/adaptix.py index 66f6e8a..a0c4fa6 100644 --- a/src/unihttp_openapi_generator/render/serializers/adaptix.py +++ b/src/unihttp_openapi_generator/render/serializers/adaptix.py @@ -39,11 +39,12 @@ def _has_factory_default(model: IRModel) -> bool: return any(f.has_default and isinstance(f.default, list | dict) for f in model.fields) def render_model(self, model: IRModel) -> str: - # Keyword-only once inheritance is in play: a subclass may pin an inherited - # field to a default while adding required fields of its own, which the - # positional "defaults last" rule forbids. - decorator = "@dataclass(kw_only=True)" if self.uses_inheritance else "@dataclass" - header = f"class {model.name}({model.base}):" if model.base else f"class {model.name}:" + # Keyword-only for the models in an inheritance hierarchy: a subclass may pin an + # inherited field to a default while adding required fields of its own, which + # the positional "defaults last" rule forbids. + decorator = "@dataclass(kw_only=True)" if self.is_kw_only(model) else "@dataclass" + base = model.base_model + header = f"class {model.name}({base}):" if base else f"class {model.name}:" lines = [decorator, header] doc = docstring(model.description, " ") if doc: diff --git a/src/unihttp_openapi_generator/render/serializers/base.py b/src/unihttp_openapi_generator/render/serializers/base.py index b2fc831..79c6de9 100644 --- a/src/unihttp_openapi_generator/render/serializers/base.py +++ b/src/unihttp_openapi_generator/render/serializers/base.py @@ -9,7 +9,13 @@ from unihttp_openapi_generator.config import Serializer from unihttp_openapi_generator.ir.document import IRDocument -from unihttp_openapi_generator.ir.models import Declaration, IRAlias, IREnum, IRModel +from unihttp_openapi_generator.ir.models import ( + Declaration, + Discriminator, + IRAlias, + IREnum, + IRModel, +) from unihttp_openapi_generator.ir.types import Import # Generated packages ship without a ``[tool.ruff]`` table, so ruff lints them with @@ -72,17 +78,28 @@ def __init__(self) -> None: # strategies that need to inspect sibling models (e.g. pydantic # discriminated unions). Empty unless a document context is bound. self.models_by_name: dict[str, IRModel] = {} - # Whether any model in the bound document inherits from another. Model - # constructors then become keyword-only: a subclass may pin an inherited - # field to a default (a discriminator tag) while declaring required fields - # of its own, which positional ordering cannot express. - self.uses_inheritance = False + # Names of the models that take part in an inheritance hierarchy (as a base or + # as a subclass). Only these need keyword-only constructors -- a subclass may + # pin an inherited field to a default (a discriminator tag) while declaring + # required fields of its own, which positional ordering cannot express. Models + # outside every hierarchy keep positional construction so one `allOf` subtype + # in a spec does not silently break the constructor of every other model. + self._kw_only_models: frozenset[str] = frozenset() def bind_document(self, doc: IRDocument) -> None: self.models_by_name = { decl.name: decl for decl in doc.declarations if isinstance(decl, IRModel) } - self.uses_inheritance = any(model.base for model in self.models_by_name.values()) + hierarchy: set[str] = set() + for model in self.models_by_name.values(): + if model.base_model is not None: + hierarchy.add(model.name) + hierarchy.add(model.base_model) + self._kw_only_models = frozenset(hierarchy) + + def is_kw_only(self, model: IRModel) -> bool: + """Whether ``model``'s constructor must be keyword-only (see ``bind_document``).""" + return model.name in self._kw_only_models # -- imports --------------------------------------------------------------- @@ -105,7 +122,22 @@ def render_declaration(self, decl: Declaration) -> str: return self.render_enum(decl) if isinstance(decl, IRAlias): return self.render_alias(decl) - return self.render_model(decl) + body = self.render_model(decl) + if decl.discriminator is not None: + # A discriminated base kept as a class (inheritance mode). No serializer + # resolves a subtype from a base-class annotation on its own, so surface + # the mapping instead of dropping it: it is what a reader needs to wire + # tagged decoding by hand. + body = f"{self._discriminator_comment(decl.discriminator)}\n{body}" + return body + + @staticmethod + def _discriminator_comment(disc: Discriminator) -> str: + mapping = ", ".join(f"{value}={name}" for value, name in sorted(disc.mapping.items())) + note = f"# discriminator: {disc.property_name}" + if mapping: + note += f" ({mapping})" + return f"{note}\n# subtype resolution is left to the serializer config" # -- shared renderers ------------------------------------------------------ diff --git a/src/unihttp_openapi_generator/render/serializers/msgspec.py b/src/unihttp_openapi_generator/render/serializers/msgspec.py index 651d340..af7ff9d 100644 --- a/src/unihttp_openapi_generator/render/serializers/msgspec.py +++ b/src/unihttp_openapi_generator/render/serializers/msgspec.py @@ -68,8 +68,8 @@ def _sort_key(f: IRField) -> tuple[bool, bool]: def render_model(self, model: IRModel) -> str: # See the adaptix strategy: inheritance forces keyword-only constructors. - options = ", kw_only=True" if self.uses_inheritance else "" - lines = [f"class {model.name}({model.base or 'Struct'}{options}):"] + options = ", kw_only=True" if self.is_kw_only(model) else "" + lines = [f"class {model.name}({model.base_model or 'Struct'}{options}):"] doc = docstring(model.description, " ") if doc: lines.append(doc.rstrip("\n")) diff --git a/src/unihttp_openapi_generator/render/serializers/pydantic.py b/src/unihttp_openapi_generator/render/serializers/pydantic.py index 93285dd..b226672 100644 --- a/src/unihttp_openapi_generator/render/serializers/pydantic.py +++ b/src/unihttp_openapi_generator/render/serializers/pydantic.py @@ -148,7 +148,7 @@ def _pydantic_field_name(cls, name: str) -> str: return safe def render_model(self, model: IRModel) -> str: - lines = [f"class {model.name}({model.base or 'BaseModel'}):"] + lines = [f"class {model.name}({model.base_model or 'BaseModel'}):"] doc = docstring(model.description, " ") if doc: lines.append(doc.rstrip("\n")) diff --git a/tests/test_builder.py b/tests/test_builder.py index 3f57896..6eb37b3 100644 --- a/tests/test_builder.py +++ b/tests/test_builder.py @@ -767,23 +767,85 @@ def inherited() -> IRDocument: def test_inheritance_keeps_parent_fields_on_parent(inherited: IRDocument) -> None: base = _decl(inherited, "Button") assert isinstance(base, IRModel) - assert base.base is None + assert base.base_model is None assert [f.name for f in base.fields] == ["type", "text"] sub = _decl(inherited, "CallbackButton") - assert sub.base == "Button" - # own fields only: the restated ``text``, the new ``payload``, and the pinned tag - assert {f.name for f in sub.fields} == {"type", "text", "payload"} + assert sub.base_model == "Button" + # own fields only: the new ``payload`` and the pinned tag. ``text`` is restated by + # the spec purely to attach prose, so it is inherited rather than re-declared. + assert {f.name for f in sub.fields} == {"type", "payload"} -def test_inheritance_restated_field_keeps_parent_required(inherited: IRDocument) -> None: +def test_inheritance_drops_redundant_restatement(inherited: IRDocument) -> None: + # ``CallbackButton`` restates ``text`` only to add a description. Re-emitting it + # would put ``text: str`` on the subclass shadowing an identical base attribute -- + # noise at best, and a mypy ``[assignment]`` error as soon as the restatement + # differs at all (see ``test_inheritance_drops_widening_restatement``). sub = _decl(inherited, "CallbackButton") - text = next(f for f in sub.fields if f.name == "text") - # ``required`` comes from Button; without it the field would widen to `str | None` - # and violate the base's declaration. + assert "text" not in {f.name for f in sub.fields} + base = _decl(inherited, "Button") + assert isinstance(base, IRModel) + text = next(f for f in base.fields if f.name == "text") assert text.required is True assert text.type.annotation() == "str" - assert text.description == "Visible label." + + +def test_inheritance_drops_widening_restatement() -> None: + """A subtype relaxing an inherited field must not emit an unsound override. + + ``class C(P)`` with ``v: str | None`` over ``v: str`` is rejected by + ``mypy --strict``, so the subtype inherits the base's declaration instead. + """ + spec: dict[str, Any] = { + "openapi": "3.1.0", + "info": {"title": "S", "version": "1.0.0"}, + "paths": {}, + "components": { + "schemas": { + "P": {"type": "object", "required": ["v"], "properties": {"v": {"type": "string"}}}, + "C": { + "allOf": [ + {"$ref": "#/components/schemas/P"}, + {"properties": {"v": {"type": "string", "nullable": True}}}, + ] + }, + } + }, + } + ir = build_ir(spec, RefResolver(spec), inheritance=True) + sub = _decl(ir, "C") + assert isinstance(sub, IRModel) + assert sub.base_model == "P" + assert sub.fields == [] + + +def test_inheritance_keeps_narrowing_restatement() -> None: + """A genuine narrowing (``Literal`` over ``str``) is a sound override, so it stays.""" + spec: dict[str, Any] = { + "openapi": "3.1.0", + "info": {"title": "S", "version": "1.0.0"}, + "paths": {}, + "components": { + "schemas": { + "P": { + "type": "object", + "required": ["k"], + "properties": {"k": {"type": "string"}}, + }, + "C": { + "allOf": [ + {"$ref": "#/components/schemas/P"}, + {"required": ["k"], "properties": {"k": {"enum": ["one", "two"]}}}, + ] + }, + } + }, + } + ir = build_ir(spec, RefResolver(spec), inheritance=True) + sub = _decl(ir, "C") + assert isinstance(sub, IRModel) + assert [f.type.annotation() for f in sub.fields] == ["Literal['one', 'two']"] def test_inheritance_pins_discriminator_tag(inherited: IRDocument) -> None: @@ -797,7 +859,7 @@ def test_inheritance_pins_discriminator_tag(inherited: IRDocument) -> None: def test_inheritance_keeps_marker_subtype_as_class(inherited: IRDocument) -> None: marker = _decl(inherited, "LinkButton") assert isinstance(marker, IRModel) - assert marker.base == "Button" + assert marker.base_model == "Button" assert [f.name for f in marker.fields] == ["type"] @@ -827,7 +889,7 @@ def test_inheritance_orders_bases_before_subclasses(inherited: IRDocument) -> No def test_inheritance_plain_allof_ref_becomes_base(inherited: IRDocument) -> None: sub = _decl(inherited, "NamedOwner") - assert sub.base == "Owner" + assert sub.base_model == "Owner" assert [f.name for f in sub.fields] == ["name"] assert sub.referenced_models() == {"Owner"} @@ -835,7 +897,7 @@ def test_inheritance_plain_allof_ref_becomes_base(inherited: IRDocument) -> None def test_inheritance_multiple_refs_still_merge(inherited: IRDocument) -> None: # Two `$ref`s give no single parent to pick, so the merge behaviour is kept. mixed = _decl(inherited, "Mixed") - assert mixed.base is None + assert mixed.base_model is None assert {f.name for f in mixed.fields} == {"id", "type", "text"} @@ -844,7 +906,139 @@ def test_without_inheritance_parent_fields_are_merged() -> None: ir = build_ir(spec, RefResolver(spec)) sub = _decl(ir, "CallbackButton") assert isinstance(sub, IRModel) - assert sub.base is None + assert sub.base_model is None assert {f.name for f in sub.fields} == {"type", "text", "payload"} # the discriminated base collapses into a union alias, as before assert isinstance(_decl(ir, "Button"), IRAlias) + + +def test_inheritance_oneof_discriminator_base_stays_a_union() -> None: + """The common polymorphism idiom must not be turned into an empty class. + + ``{oneOf: [...], discriminator: {mapping}}`` declares no properties of its own, so + there is nothing to inherit. Rendering it as ``class Button`` would emit an empty + class and every ``list[Button]`` payload would decode into it, silently dropping + each variant's fields -- so it stays a union alias even in inheritance mode. + """ + spec: dict[str, Any] = { + "openapi": "3.0.0", + "info": {"title": "S", "version": "1.0.0"}, + "paths": {}, + "components": { + "schemas": { + "Button": { + "oneOf": [ + {"$ref": "#/components/schemas/CallbackButton"}, + {"$ref": "#/components/schemas/LinkButton"}, + ], + "discriminator": { + "propertyName": "type", + "mapping": { + "callback": "#/components/schemas/CallbackButton", + "link": "#/components/schemas/LinkButton", + }, + }, + }, + "ButtonBase": { + "type": "object", + "required": ["type", "text"], + "properties": {"type": {"type": "string"}, "text": {"type": "string"}}, + }, + "CallbackButton": { + "allOf": [ + {"$ref": "#/components/schemas/ButtonBase"}, + {"required": ["payload"], "properties": {"payload": {"type": "string"}}}, + ] + }, + "LinkButton": { + "allOf": [ + {"$ref": "#/components/schemas/ButtonBase"}, + {"required": ["url"], "properties": {"url": {"type": "string"}}}, + ] + }, + } + }, + } + ir = build_ir(spec, RefResolver(spec), inheritance=True) + button = _decl(ir, "Button") + assert isinstance(button, IRAlias) + assert button.target.annotation() == "CallbackButton | LinkButton" + # the real base of the family is still turned into a superclass + assert _decl(ir, "CallbackButton").base_model == "ButtonBase" + assert _decl(ir, "LinkButton").base_model == "ButtonBase" + + +def test_inheritance_recursive_base_still_subclasses() -> None: + """Inheritance must not depend on the order the schema graph happens to be walked. + + ``Node`` is built first and reaches ``LeafNode`` through its own ``child`` property, + so ``LeafNode`` resolves its base while ``Node`` has no ``_declarations`` entry yet. + Deciding from the schema (not from the half-built registry) keeps it a subclass. + """ + spec: dict[str, Any] = { + "openapi": "3.1.0", + "info": {"title": "S", "version": "1.0.0"}, + "paths": {}, + "components": { + "schemas": { + "Node": { + "type": "object", + "required": ["id"], + "properties": { + "id": {"type": "string"}, + "child": {"$ref": "#/components/schemas/LeafNode"}, + }, + }, + "LeafNode": { + "allOf": [ + {"$ref": "#/components/schemas/Node"}, + {"properties": {"value": {"type": "string"}}}, + ] + }, + } + }, + } + ir = build_ir(spec, RefResolver(spec), inheritance=True) + leaf = _decl(ir, "LeafNode") + assert leaf.base_model == "Node" + assert [f.name for f in leaf.fields] == ["value"] # not a copy of Node's fields + + +def test_inheritance_tag_field_never_collides_with_a_sibling() -> None: + """The pinned tag needs its own python name, not one already taken on the subtype. + + ``Type`` and ``type`` snake-case to the same identifier. Emitting both unqualified + would put two identical attribute names in one class body: the later wins and the + discriminator tag is silently destroyed. + """ + spec: dict[str, Any] = { + "openapi": "3.0.0", + "info": {"title": "S", "version": "1.0.0"}, + "paths": {}, + "components": { + "schemas": { + "B": { + "type": "object", + "required": ["type"], + "properties": {"type": {"type": "string"}}, + "discriminator": { + "propertyName": "type", + "mapping": {"a": "#/components/schemas/A"}, + }, + }, + "A": { + "allOf": [ + {"$ref": "#/components/schemas/B"}, + {"properties": {"Type": {"type": "integer"}}}, + ] + }, + } + }, + } + ir = build_ir(spec, RefResolver(spec), inheritance=True) + sub = _decl(ir, "A") + names = [f.name for f in sub.fields] + assert len(names) == len(set(names)), f"duplicate class attributes: {names}" + tag = next(f for f in sub.fields if f.wire_name == "type") + assert tag.type == LiteralType(("a",)) + assert tag.needs_alias is True # renamed, so the wire name is restored by an alias diff --git a/tests/test_render_methods.py b/tests/test_render_methods.py index 836639e..699dc0b 100644 --- a/tests/test_render_methods.py +++ b/tests/test_render_methods.py @@ -140,3 +140,57 @@ def test_blank_line_between_dunders_and_params(sample_spec: dict[str, Any]) -> N source = format_python(render_methods_module(ir, "pets", "acme"), filename="pets.py") # methods with parameters get a blank line after __method__ assert '__method__ = "GET"\n\n x_request_id' in source + + +def test_body_field_and_param_descriptions_become_attribute_docstrings() -> None: + """Schema prose on a parameter / spread body field has to land somewhere. + + ``IRBodyField.description`` and ``IRParameter.description`` are only worth carrying + if they reach the generated package; a PEP 258 attribute docstring is the one place + they fit without touching the constructor signature. + """ + spec: dict[str, Any] = { + "openapi": "3.1.0", + "info": {"title": "S", "version": "1.0.0"}, + "paths": { + "/d": { + "post": { + "operationId": "createDoc", + "tags": ["d"], + "parameters": [ + { + "name": "dry_run", + "in": "query", + "schema": {"type": "boolean"}, + "description": "Validate without persisting.", + } + ], + "requestBody": { + "required": True, + "content": { + "application/json": { + "schema": { + "type": "object", + "required": ["title"], + "properties": { + "title": { + "type": "string", + "description": "Document title.", + }, + "body": {"type": "string"}, + }, + } + } + }, + }, + "responses": {"204": {"description": "no content"}}, + } + } + }, + } + ir = build_ir(spec, RefResolver(spec)) + source = format_python(render_methods_module(ir, "d", "pkg"), filename="d.py") + assert '"""Document title."""' in source + assert '"""Validate without persisting."""' in source + # a field without prose gets no stray docstring + assert source.count('"""') == 2 * 3 # module docstring + the two attribute ones diff --git a/tests/test_render_models.py b/tests/test_render_models.py index 3ad78fc..583f005 100644 --- a/tests/test_render_models.py +++ b/tests/test_render_models.py @@ -375,9 +375,7 @@ def test_docstring_with_backslash_is_raw_and_warning_free() -> None: ) def test_inherited_models_subclass_their_base(serializer: Serializer, tmp_path: Path) -> None: ir = build_ir(_INHERITED_SPEC, RefResolver(_INHERITED_SPEC), inheritance=True) - source = format_python( - render_models_module(ir, get_strategy(serializer)), filename="models.py" - ) + source = format_python(render_models_module(ir, get_strategy(serializer)), filename="models.py") assert "class CallbackButton(Button" in source module = _load(source, tmp_path, f"genmodels_inherit_{serializer.value}") assert issubclass(module.CallbackButton, module.Button) @@ -391,11 +389,51 @@ def test_inherited_models_subclass_their_base(serializer: Serializer, tmp_path: def test_inherited_adaptix_models_are_kw_only() -> None: ir = build_ir(_INHERITED_SPEC, RefResolver(_INHERITED_SPEC), inheritance=True) source = render_models_module(ir, get_strategy(Serializer.ADAPTIX)) - assert "@dataclass(kw_only=True)" in source - assert "@dataclass\n" not in source + assert "@dataclass(kw_only=True)\nclass Button:" in source + assert "@dataclass(kw_only=True)\nclass CallbackButton(Button):" in source + + +def test_only_hierarchy_members_become_kw_only() -> None: + """One ``allOf`` subtype must not silently break every other model's constructor. + + ``kw_only`` is what lets a subclass pin an inherited field to a default while + adding required fields of its own -- a constraint that exists only inside a + hierarchy. Models outside one keep positional construction. + """ + spec: dict[str, Any] = { + "openapi": "3.0.0", + "info": {"title": "I", "version": "1.0.0"}, + "paths": {}, + "components": { + "schemas": { + **_INHERITED_SPEC["components"]["schemas"], + "Unrelated": { + "type": "object", + "required": ["n"], + "properties": {"n": {"type": "string"}}, + }, + } + }, + } + ir = build_ir(spec, RefResolver(spec), inheritance=True) + source = render_models_module(ir, get_strategy(Serializer.ADAPTIX)) + assert "@dataclass\nclass Unrelated:" in source + assert "@dataclass(kw_only=True)\nclass Button:" in source def test_models_without_inheritance_keep_positional_dataclasses() -> None: ir = build_ir(_INHERITED_SPEC, RefResolver(_INHERITED_SPEC)) source = render_models_module(ir, get_strategy(Serializer.ADAPTIX)) assert "@dataclass(kw_only=True)" not in source + + +def test_discriminated_base_class_keeps_its_mapping_visible() -> None: + """A base kept as a class must not swallow the discriminator it declares. + + No serializer resolves a concrete subtype from a base-class annotation on its own, + so the mapping is the one thing a reader needs to wire tagged decoding by hand. + Dropping it would leave that information nowhere in the generated package. + """ + ir = build_ir(_INHERITED_SPEC, RefResolver(_INHERITED_SPEC), inheritance=True) + source = render_models_module(ir, get_strategy(Serializer.ADAPTIX)) + assert "# discriminator: type (callback=CallbackButton)" in source