diff --git a/packages/overture-schema-codegen/changelog.d/758.feature.md b/packages/overture-schema-codegen/changelog.d/758.feature.md new file mode 100644 index 000000000..acdc9e707 --- /dev/null +++ b/packages/overture-schema-codegen/changelog.d/758.feature.md @@ -0,0 +1 @@ +Carried each field's declared default through extraction as `FieldSpec.default`, and made extraction refuse fields that declare `default_factory`. The Markdown reference shows a non-null default as a note on the field's row. diff --git a/packages/overture-schema-codegen/docs/walkthrough.md b/packages/overture-schema-codegen/docs/walkthrough.md index 07fd5ef38..d7362afd5 100644 --- a/packages/overture-schema-codegen/docs/walkthrough.md +++ b/packages/overture-schema-codegen/docs/walkthrough.md @@ -575,6 +575,16 @@ named source live on the NewType's own page. Model-level constraints annotate top-level field rows (those without dot-notation prefixes) using the `field_notes` dict from `analyze_model_constraints`. +### Default annotation + +A declared default annotates its field's row as an italic `Default:` note, ahead of any +constraint notes. Every row gets one, including dot-notation rows expanded from a +sub-model. Enum members show their value, the form a record +carries. A default of `None` gets no note: `= None` is how a Pydantic field is +declared optional, and the `(optional)` qualifier in the type column already says so. +The Overture schema forbids non-null defaults (#695), so the note appears only for +models outside it. + ### Example formatting Example values render in backticks for monospace consistency. Booleans use diff --git a/packages/overture-schema-codegen/src/overture/schema/codegen/extraction/model_extraction.py b/packages/overture-schema-codegen/src/overture/schema/codegen/extraction/model_extraction.py index 7371db208..689b488e1 100644 --- a/packages/overture-schema-codegen/src/overture/schema/codegen/extraction/model_extraction.py +++ b/packages/overture-schema-codegen/src/overture/schema/codegen/extraction/model_extraction.py @@ -5,6 +5,7 @@ from collections.abc import Mapping from pydantic import BaseModel +from pydantic.experimental.missing_sentinel import MISSING from pydantic.fields import FieldInfo from pydantic_core import PydanticUndefined @@ -15,7 +16,7 @@ ModelRef, UnionRef, ) -from .specs import FieldSpec, RecordSpec, is_model_class +from .specs import UNDEFINED, FieldSpec, RecordSpec, is_model_class from .type_analyzer import ( ModelResolver, UnionResolver, @@ -46,12 +47,61 @@ def resolve_field_alias(field_name: str, field_info: FieldInfo) -> str: return field_name -def _is_field_required(field_info: FieldInfo, is_optional: bool) -> bool: - """Determine whether a field is required (no default and not Optional).""" - has_default = ( - field_info.default is not PydanticUndefined - or field_info.default_factory is not None +def _field_default(field_info: FieldInfo) -> object: + """Return the field's declared default, or `UNDEFINED`. + + Two sentinels mean "no declared default", not one. `PydanticUndefined` is + Pydantic's, which `UNDEFINED` re-exports. `MISSING` is the one + `Omitable[T]` installs (`Field(default=MISSING)`) to get JSON Schema + omissibility instead of Pydantic nullability -- see + `overture.schema.system.optionality`. It is machinery for "this key may be + absent", never a value anyone declared, so carrying it through as a default + makes every `Omitable` field claim a default it does not have. `Feature.bbox` + and `Feature.id` are both `Omitable`, and every feature model inherits them, + so a check built on this carrier to warn on declared defaults would warn on + every feature type twice over. + """ + if field_info.default is MISSING: + return UNDEFINED + return field_info.default + + +def _reject_default_factory( + model_class: type[BaseModel], field_name: str, field_info: FieldInfo +) -> None: + """Refuse a `default_factory`, naming the field that declared one. + + A factory is a Python callable, and no target this IR feeds can render + one. Invoking it here would freeze one sample of a value meant to be + produced per instance, and recording it as "no default" would hide a + declared default from every consumer, including any check looking for + declared defaults. Refusing is the only one of the three that the author + can see. + """ + if field_info.default_factory is None: + return + raise TypeError( + f"{model_class.__name__}.{field_name} declares default_factory, which " + "the extraction IR does not carry: a factory is a callable and no " + "target can render one. Declare a literal default, or none." ) + + +def _is_field_required(field_info: FieldInfo, is_optional: bool) -> bool: + """Determine whether a field is required (no default and not Optional). + + `default_factory` is not consulted: `_reject_default_factory` has already + refused any field declaring one, so a factory cannot reach here. Restore + the check if that refusal is ever relaxed. + + `MISSING` counts as a default here even though `_field_default` reports it + as none: `Omitable[T]` means the key may be absent, so the field is not + required, but the sentinel is not a value the author declared, so it is not + a default either. It compares against `PydanticUndefined` rather than the + `UNDEFINED` that re-exports it, because what it reads is a `FieldInfo`, + not a `FieldSpec`. + """ + has_default = field_info.default is not PydanticUndefined return not has_default and not is_optional @@ -163,6 +213,7 @@ def _extract_model_recursive( fields: list[FieldSpec] = [] for field_name in _field_order(model_class): field_info = model_class.model_fields[field_name] + _reject_default_factory(model_class, field_name, field_info) annotation = field_info.annotation if annotation is None: continue @@ -185,6 +236,7 @@ def _extract_model_recursive( description=field_info.description or ti_description, is_required=_is_field_required(field_info, is_optional), is_optional=is_optional, + default=_field_default(field_info), ) ) diff --git a/packages/overture-schema-codegen/src/overture/schema/codegen/extraction/specs.py b/packages/overture-schema-codegen/src/overture/schema/codegen/extraction/specs.py index 6b5259da1..320d0cf42 100644 --- a/packages/overture-schema-codegen/src/overture/schema/codegen/extraction/specs.py +++ b/packages/overture-schema-codegen/src/overture/schema/codegen/extraction/specs.py @@ -9,6 +9,7 @@ from annotated_types import Interval from pydantic import BaseModel, RootModel +from pydantic_core import PydanticUndefined from overture.schema.system.discovery.tag import get_values_for_key from overture.schema.system.model_constraint import ModelConstraint @@ -120,6 +121,20 @@ class EnumSpec(_SourceTypeIdentityMixin): source_type: type | None = None +# Sentinel for "this field declares no default", distinct from a declared +# default of `None`, which is legal and different. +# +# This is Pydantic's own `PydanticUndefined`, re-exported from the IR under a +# shorter name. Re-exporting rather than minting a second sentinel keeps a +# renderer from reaching past the IR into `pydantic_core` to read a +# `FieldSpec`, and keeps the two values that already mean this -- the one +# extraction compares against, and the one the IR hands out -- from drifting +# apart. It also inherits `PydanticUndefined`'s identity under +# `copy.deepcopy` and `pickle`; a freshly minted singleton comes back from +# either as a different object, and `is UNDEFINED` then reads false. +UNDEFINED = PydanticUndefined + + @dataclass class FieldSpec: """Specification for a model field: header metadata plus structural shape. @@ -127,6 +142,17 @@ class FieldSpec: `shape` is the full `FieldShape` tree, including any sub-model (`ModelRef`) and sub-union (`UnionRef`) references already resolved during extraction. + + `default` carries the field's declared literal default, or + `UNDEFINED` when it declares none. A declared default of `None` is + stored as `None` and is not the absent case -- the distinction + `_is_field_required` already draws on `FieldInfo.default`. + + `= None` is also how a Pydantic field is declared optional -- the IR + reports the declared default it finds and does not judge whether one + was meant, because nothing in the source distinguishes the two. A + consumer that cares about real defaults tests for `not None` as well + as `not UNDEFINED`. """ name: str @@ -134,6 +160,7 @@ class FieldSpec: description: str | None = None is_required: bool = True is_optional: bool = False + default: Any = UNDEFINED @dataclass diff --git a/packages/overture-schema-codegen/src/overture/schema/codegen/markdown/renderer.py b/packages/overture-schema-codegen/src/overture/schema/codegen/markdown/renderer.py index 8f0911ab8..1d35efc8d 100644 --- a/packages/overture-schema-codegen/src/overture/schema/codegen/markdown/renderer.py +++ b/packages/overture-schema-codegen/src/overture/schema/codegen/markdown/renderer.py @@ -6,6 +6,7 @@ import re from collections.abc import Callable, Iterable from dataclasses import dataclass +from enum import Enum from pathlib import Path from typing import TypedDict, cast @@ -24,6 +25,7 @@ ) from ..extraction.model_constraints import analyze_model_constraints from ..extraction.specs import ( + UNDEFINED, AnnotatedField, EnumSpec, FieldSpec, @@ -215,11 +217,11 @@ def _field_template_context( ) -def _annotate_constraint_notes( +def _annotate_notes( row: _FieldRow, notes: list[str], ) -> None: - """Append italic constraint descriptions to a field's description cell.""" + """Append italic notes to a field's description cell.""" formatted = "
".join(f"*{note}*" for note in notes) if row["description"]: row["description"] = f"{row['description']}

{formatted}" @@ -227,6 +229,30 @@ def _annotate_constraint_notes( row["description"] = formatted +def _format_default(value: object) -> str: + """Format a declared default as the value a record would carry. + + An enum member shows its value, which is what appears in data. An empty + string shows as `""`, where an example cell would be left blank. + """ + if isinstance(value, Enum): + value = value.value + if value == "": + return '`""`' + return _format_example_value(value) + + +def _annotate_default(row: _FieldRow, field: FieldSpec) -> None: + """Annotate a field row with its declared default. + + A default of `None` is not shown: `= None` is how a Pydantic field is + declared optional, and the `(optional)` qualifier already says so. + """ + if field.default is UNDEFINED or field.default is None: + return + _annotate_notes(row, [f"Default: {_format_default(field.default)}"]) + + def _link_fn_from_ctx(ctx: LinkContext | None) -> _LinkFn: r"""Build a TypeIdentity-to-markdown-link resolver from a LinkContext. @@ -259,7 +285,7 @@ def directly_applied(prefix: str, sources: Iterable[ConstraintSource]) -> list[s notes += directly_applied("key: ", key_constraints) notes += directly_applied("value: ", value_constraints) if notes: - _annotate_constraint_notes(row, notes) + _annotate_notes(row, notes) def _expandable_list_suffix(field_spec: FieldSpec) -> str: @@ -303,7 +329,7 @@ def _annotate_top_level_constraints( continue field_name = name.split("[")[0] if field_name in constraint_notes: - _annotate_constraint_notes(row, constraint_notes[field_name]) + _annotate_notes(row, constraint_notes[field_name]) def _expand_model_fields( @@ -321,6 +347,7 @@ def _expand_model_fields( row = _field_template_context(field_spec, ctx) name = f"{prefix}{field_spec.name}" if prefix else field_spec.name row["name"] = f"{name}{_expandable_list_suffix(field_spec)}" + _annotate_default(row, field_spec) if not prefix: _annotate_field_constraints(row, field_spec, ctx) result.append(row) @@ -375,9 +402,10 @@ def _expand_union_fields( name = field_spec.name suffix = _expandable_list_suffix(field_spec) + _annotate_default(row, field_spec) _annotate_field_constraints(row, field_spec, ctx) if constraint_notes and field_spec.name in constraint_notes: - _annotate_constraint_notes(row, constraint_notes[field_spec.name]) + _annotate_notes(row, constraint_notes[field_spec.name]) tag = _variant_tag(annotated, spec.name) if tag is not None: diff --git a/packages/overture-schema-codegen/tests/test_markdown_renderer.py b/packages/overture-schema-codegen/tests/test_markdown_renderer.py index 3c929f289..3bc2263b1 100644 --- a/packages/overture-schema-codegen/tests/test_markdown_renderer.py +++ b/packages/overture-schema-codegen/tests/test_markdown_renderer.py @@ -613,6 +613,99 @@ def test_venue_reference_unlinked_without_context(self) -> None: assert "aggregation, part of" in ref_line +class TestRenderFeatureDefaults: + """A declared default renders as a note in the field's description cell. + + `None` does not: `= None` is how a Pydantic field is declared optional, + so the `(optional)` qualifier already says what it would. + """ + + @staticmethod + def _row(result: str, name: str) -> str: + return next(li for li in result.splitlines() if f"| `{name}` |" in li) + + def test_literal_default_shows_note(self) -> None: + class ModelWithLevel(BaseModel): + """Model.""" + + level: int = Field(0, description="Z-order.") + + result = render_model(extract_model(ModelWithLevel)) + assert "Z-order.

*Default: `0`*" in self._row(result, "level") + + def test_none_default_shows_no_note(self) -> None: + class ModelWithOptional(BaseModel): + """Model.""" + + nickname: str | None = Field(None, description="Nickname.") + + result = render_model(extract_model(ModelWithOptional)) + assert "Default" not in self._row(result, "nickname") + + def test_field_without_default_shows_no_note(self) -> None: + class ModelWithRequired(BaseModel): + """Model.""" + + name: str = Field(description="Name.") + + result = render_model(extract_model(ModelWithRequired)) + assert "Default" not in self._row(result, "name") + + def test_enum_default_shows_member_value(self) -> None: + class Surface(Enum): + PAVED = "paved" + UNPAVED = "unpaved" + + class ModelWithEnumDefault(BaseModel): + """Model.""" + + surface: Surface = Surface.PAVED + + result = render_model(extract_model(ModelWithEnumDefault)) + assert "*Default: `paved`*" in self._row(result, "surface") + + def test_empty_string_default_is_visible(self) -> None: + class ModelWithEmptyDefault(BaseModel): + """Model.""" + + label: str = "" + + result = render_model(extract_model(ModelWithEmptyDefault)) + assert '*Default: `""`*' in self._row(result, "label") + + def test_nested_field_default_shows_note(self) -> None: + class Inner(BaseModel): + """Inner.""" + + level: int = 0 + + class Outer(BaseModel): + """Outer.""" + + inner: Inner + inners: list[Inner] + + result = render_model(extract_model(Outer)) + assert "*Default: `0`*" in self._row(result, "inner.level") + assert "*Default: `0`*" in self._row(result, "inners[].level") + + def test_union_field_default_shows_note(self) -> None: + spec = make_union_spec( + annotated_fields=[ + AnnotatedField( + field_spec=FieldSpec( + name="flag", + shape=STR_TYPE, + is_required=False, + default=False, + ), + variant_sources=None, + ), + ], + ) + assert "*Default: `false`*" in self._row(render_model(spec), "flag") + + class TestRenderFeatureMapConstraints: """Tests for map key/value constraint notes in field description cells. diff --git a/packages/overture-schema-codegen/tests/test_model_extraction.py b/packages/overture-schema-codegen/tests/test_model_extraction.py index e8b3fd2c7..9d809276b 100644 --- a/packages/overture-schema-codegen/tests/test_model_extraction.py +++ b/packages/overture-schema-codegen/tests/test_model_extraction.py @@ -2,8 +2,10 @@ from typing import Annotated, Optional +import pytest from codegen_test_support import FeatureWithRootModel from pydantic import BaseModel, Field +from pydantic.experimental.missing_sentinel import MISSING from overture.schema.codegen.extraction.field import ( ArrayOf, @@ -15,7 +17,9 @@ from overture.schema.codegen.extraction.field_walk import terminal_of from overture.schema.codegen.extraction.length_constraints import ArrayMinLen from overture.schema.codegen.extraction.model_extraction import extract_model +from overture.schema.codegen.extraction.specs import UNDEFINED from overture.schema.common.scoping.vehicle import VehicleSelector +from overture.schema.system.optionality import Omitable def test_extract_model_populates_union_terminal() -> None: @@ -109,7 +113,7 @@ def test_self_referential_list_forward_ref_resolves_to_cycle() -> None: class Node(BaseModel): val: Annotated[int, Field(ge=0)] - children: list["Node"] = Field(default_factory=list) + children: list["Node"] spec = extract_model(Node) children = next(f for f in spec.fields if f.name == "children") @@ -148,7 +152,7 @@ def test_nested_list_forward_ref_resolves_to_cycle() -> None: class Node(BaseModel): val: int - grid: list[list["Node"]] = Field(default_factory=list) + grid: list[list["Node"]] spec = extract_model(Node) grid = next(f for f in spec.fields if f.name == "grid") @@ -179,3 +183,95 @@ class M(BaseModel): assert isinstance(items_field.shape, ArrayOf) constraints = [cs.constraint for cs in items_field.shape.constraints] assert ArrayMinLen(min_length=2) in constraints + + +def test_field_with_no_default_carries_undefined() -> None: + """A field with no declared default reports `UNDEFINED`, not `None`. + + `None` is a legal declared default (see below); collapsing "no + default" into `None` would make the two indistinguishable to a + consumer trying to warn on declared defaults. + """ + + class M(BaseModel): + name: str + + spec = extract_model(M) + name_field = next(f for f in spec.fields if f.name == "name") + + assert name_field.default is UNDEFINED + + +def test_field_with_non_none_default_carries_declared_value() -> None: + """A field with a plain non-None default carries that value on the spec.""" + + class M(BaseModel): + count: int = 3 + + spec = extract_model(M) + count_field = next(f for f in spec.fields if f.name == "count") + + assert count_field.default == 3 + + +def test_field_with_none_default_is_distinguished_from_no_default() -> None: + """A field whose declared default IS `None` must not read as "no default". + + This is the load-bearing case: `field_info.default` is `None` here, + not `UNDEFINED`, so a consumer can tell "declares a default + of None" apart from "declares no default at all". + """ + + class M(BaseModel): + note: str | None = None + + spec = extract_model(M) + note_field = next(f for f in spec.fields if f.name == "note") + + assert note_field.default is None + assert note_field.default is not UNDEFINED + + +def test_default_factory_is_refused_by_name() -> None: + """A `default_factory` field is refused, naming the model and field. + + A factory is a Python callable. No target this IR feeds can render + one -- not Markdown, not a PySpark expression, not JSON Schema -- and + invoking it at extraction would freeze one sample of a value meant to + be produced per instance. Carrying it as "no default" instead hides a + declared default from anything reading the IR, so extraction refuses + it where the author can still see which field is at fault. + """ + + class M(BaseModel): + children: list[str] = Field(default_factory=list) + + with pytest.raises(TypeError, match=r"M\.children.*default_factory"): + extract_model(M) + + +def test_omitable_field_reports_no_default_not_the_missing_sentinel() -> None: + """`Omitable[T]` must not read as declaring a default. + + `Omitable[T]` is `Field(default=MISSING)` -- machinery for "this key may + be absent", chosen to get JSON Schema omissibility instead of Pydantic + nullability. `MISSING` is not a value anyone declared, so carrying it + through would make every `Omitable` field claim a default it does not + have. The carrier exists so a consumer can warn on declared defaults, and + `Feature.bbox` and `Feature.id` are both `Omitable`, so every feature model + in the schema inherits two of them -- each one a false warning. + + The over-reach direction -- normalizing away a genuine default too -- is + covered by `test_field_with_none_default_is_distinguished_from_no_default`, + confirmed by mutation: replacing the body with an unconditional + `return UNDEFINED` fails that test. + """ + + class M(BaseModel): + maybe: Omitable[int] + + spec = extract_model(M) + maybe_field = next(f for f in spec.fields if f.name == "maybe") + + assert maybe_field.default is UNDEFINED + assert maybe_field.default is not MISSING diff --git a/packages/overture-schema-codegen/tests/test_specs.py b/packages/overture-schema-codegen/tests/test_specs.py index 1688e05c0..0d3e8a8c1 100644 --- a/packages/overture-schema-codegen/tests/test_specs.py +++ b/packages/overture-schema-codegen/tests/test_specs.py @@ -1,5 +1,6 @@ """Tests for spec data structures and predicates.""" +import copy from typing import Annotated import pytest @@ -10,9 +11,11 @@ make_union_spec, ) from pydantic import BaseModel, Field +from pydantic_core import PydanticUndefined from overture.schema.codegen.extraction.model_extraction import extract_model from overture.schema.codegen.extraction.specs import ( + UNDEFINED, AnnotatedField, EnumSpec, FieldSpec, @@ -51,6 +54,22 @@ def test_carries_shape_and_optional_flag(self) -> None: assert fs.is_required is False assert fs.is_optional is True + def test_undefined_is_pydantics_own_sentinel_re_exported(self) -> None: + """`UNDEFINED` is `PydanticUndefined` under another name. + + Two things a consumer relies on. Code comparing a `FieldInfo.default` + against `PydanticUndefined` and code comparing a `FieldSpec.default` + against `UNDEFINED` reach the same verdict, so the IR's name can be + used without checking which sentinel produced the value. And + `copy.deepcopy` returns the same object: a freshly minted singleton + comes back as a different one, which then reads as a declared default + of some opaque value. + """ + assert UNDEFINED is PydanticUndefined + + fs = FieldSpec(name="x", shape=STR_TYPE) + assert copy.deepcopy(fs).default is UNDEFINED + class TestAnnotatedField: def test_stores_field_and_variant_sources(self) -> None: