From 5c1b270622df454aa5744203abf5dade40ce797f Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 15:07:15 +0000 Subject: [PATCH 01/10] feat(model): add Relation.load recursive coercion for dataclass targets Split out of canonical/operator#2557, where this shipped alongside the ops_tracing de-pydantic work. Relation.load now recursively constructs nested dataclasses and coerces Enum field values when the target is a non-pydantic dataclass, so callers get typed nested objects instead of raw decoded dicts. This carries over the original implementation from #2557 unmodified; the review at REVIEW-2557.md found several holes in it, fixed in the commits that follow. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01J1dP9XqjiuoYz4oHjWYAQ6 --- ops/charm.py | 40 ++++++++++++++++++++++++++++++++++++++++ ops/model.py | 8 ++++++++ 2 files changed, 48 insertions(+) diff --git a/ops/charm.py b/ops/charm.py index 9f2ad1ced..5558888e7 100644 --- a/ops/charm.py +++ b/ops/charm.py @@ -21,6 +21,7 @@ import logging import os import pathlib +import typing import warnings from collections.abc import Mapping from typing import ( @@ -33,6 +34,7 @@ TypedDict, TypeVar, cast, + get_type_hints, ) from . import model @@ -1699,6 +1701,44 @@ def _juju_fields(cls: type[object]) -> dict[str, str]: raise ValueError('Unable to find class fields') +def _coerce_field(tp: Any, value: Any) -> Any: + """Coerce a decoded ``value`` into the dataclass field type ``tp``. + + Used by :meth:`ops.Relation.load` to recursively construct nested + dataclasses and enum values from JSON-decoded relation data. + """ + origin = typing.get_origin(tp) + if origin is not None: + args = typing.get_args(tp) + if origin in (list, tuple) and args: + return [_coerce_field(args[0], v) for v in value] + if origin in (set, frozenset) and args: + return {_coerce_field(args[0], v) for v in value} + # Literal, Union, Optional, Dict, etc.: accept the value as-is. + return value + if isinstance(tp, type): + if dataclasses.is_dataclass(tp): + return _build_dataclass(tp, value) + if issubclass(tp, enum.Enum): + return tp(value) + return value + + +def _build_dataclass(cls: Any, data: Mapping[str, Any]) -> Any: + """Construct dataclass ``cls`` from ``data``, recursively coercing nested fields. + + Raises ``TypeError`` (via the dataclass ``__init__``) if a required field is + missing, and ``ValueError``/``TypeError`` from coercion of malformed values. + """ + hints = get_type_hints(cls) + kwargs: dict[str, Any] = {} + for field in dataclasses.fields(cls): + if field.name not in data: + continue + kwargs[field.name] = _coerce_field(hints[field.name], data[field.name]) + return cls(**kwargs) + + class CharmMeta: """Object containing the metadata for the charm. diff --git a/ops/model.py b/ops/model.py index a462d42d5..295d23dab 100644 --- a/ops/model.py +++ b/ops/model.py @@ -1823,6 +1823,14 @@ def _observer(self, event: ops.RelationEvent): data[key] = decoder(value) elif key in fields: data[fields[key]] = decoder(value) + # For plain (non-pydantic) dataclass targets, recursively coerce nested + # dataclass / enum / list / set fields. Pydantic handles its own coercion. + if ( + not args + and dataclasses.is_dataclass(cls) + and not getattr(cls, '__is_pydantic_dataclass__', False) + ): + return _charm._build_dataclass(cls, data) return cls(*args, **data) def save( From 498554a0ac99dfb372d9eb3cb088e3517c7efb7b Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 15:08:28 +0000 Subject: [PATCH 02/10] fix(model): coerce Optional/Union and dict/Mapping fields in Relation.load _coerce_field returned early for anything whose typing.get_origin wasn't list/tuple/set/frozenset, which includes Optional[X] (a Union) and dict[str, X] - both common shapes for nested-model fields. A charm reading data.inner.a on an Optional[Nested] field got an AttributeError instead, since inner stayed a plain dict. Coerce a Union against its single non-None member (a Union of more than one concrete type has no way to pick a target, so it still passes through as-is), and a dict/Mapping's values against the value type. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01J1dP9XqjiuoYz4oHjWYAQ6 --- ops/charm.py | 17 ++++++- test/test_model_relation_data_class.py | 62 ++++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 2 deletions(-) diff --git a/ops/charm.py b/ops/charm.py index 5558888e7..aca6fb6dd 100644 --- a/ops/charm.py +++ b/ops/charm.py @@ -21,6 +21,7 @@ import logging import os import pathlib +import types import typing import warnings from collections.abc import Mapping @@ -1705,16 +1706,28 @@ def _coerce_field(tp: Any, value: Any) -> Any: """Coerce a decoded ``value`` into the dataclass field type ``tp``. Used by :meth:`ops.Relation.load` to recursively construct nested - dataclasses and enum values from JSON-decoded relation data. + dataclasses and enum values from JSON-decoded relation data. An + ``Optional``/``Union`` field is coerced against its single non-``None`` + member; ``dict``/``Mapping`` fields are coerced against their value type. """ origin = typing.get_origin(tp) if origin is not None: args = typing.get_args(tp) + if origin is typing.Union or origin is types.UnionType: + non_none = [a for a in args if a is not type(None)] + if len(non_none) == 1: + # Optional[X]: coerce against the one concrete member. + return _coerce_field(non_none[0], value) + # A Union of more than one concrete type: no way to tell which + # member to coerce against, so accept the value as-is. + return value if origin in (list, tuple) and args: return [_coerce_field(args[0], v) for v in value] if origin in (set, frozenset) and args: return {_coerce_field(args[0], v) for v in value} - # Literal, Union, Optional, Dict, etc.: accept the value as-is. + if isinstance(origin, type) and issubclass(origin, Mapping) and len(args) == 2: + return {k: _coerce_field(args[1], v) for k, v in value.items()} + # Literal and other constructed generics: accept the value as-is. return value if isinstance(tp, type): if dataclasses.is_dataclass(tp): diff --git a/test/test_model_relation_data_class.py b/test/test_model_relation_data_class.py index aebb1b332..e7a876b16 100644 --- a/test/test_model_relation_data_class.py +++ b/test/test_model_relation_data_class.py @@ -432,6 +432,68 @@ def _on_relation_changed(self, event: ops.RelationChangedEvent): assert obj.c == 'foo' +def _load_into(cls: type[Any], remote_app_data: dict[str, str]) -> Any: + """Load ``remote_app_data`` into ``cls`` via ``Relation.load`` and return the result.""" + + class Charm(ops.CharmBase): + def __init__(self, framework: ops.Framework): + super().__init__(framework) + framework.observe(self.on['db'].relation_changed, self._on_relation_changed) + + def _on_relation_changed(self, event: ops.RelationChangedEvent): + self.data = event.relation.load(cls, event.app) + + ctx = testing.Context(Charm, meta={'name': 'foo', 'requires': {'db': {'interface': 'db-int'}}}) + rel = testing.Relation('db', remote_app_data=remote_app_data) + state_in = testing.State(leader=True, relations={rel}) + with ctx(ctx.on.relation_changed(rel), state_in) as mgr: + mgr.run() + return mgr.charm.data + + +def test_relation_load_optional_nested_dataclass(): + """Optional[X] (a Union with one concrete member) is coerced against X.""" + + @dataclasses.dataclass + class Data: + inner: Nested | None = None + + obj = _load_into(Data, {'inner': json.dumps({'sub': 1})}) + assert isinstance(obj.inner, Nested) + assert obj.inner.sub == 1 + + obj = _load_into(Data, {}) + assert obj.inner is None + + +def test_relation_load_dict_of_nested_dataclass(): + """dict[str, X] fields are coerced against X for each value.""" + + @dataclasses.dataclass + class Data: + by_name: dict[str, Nested] + + obj = _load_into(Data, {'by_name': json.dumps({'a': {'sub': 1}, 'b': {'sub': 2}})}) + assert obj.by_name == {'a': Nested(sub=1), 'b': Nested(sub=2)} + assert all(isinstance(v, Nested) for v in obj.by_name.values()) + + +def test_relation_load_union_of_two_concrete_types_passes_through(): + """A Union with more than one concrete member is passed through as-is. + + There is no way to tell which member to coerce against, so this is a + regression check that such fields keep working uncoerced rather than + raising. + """ + + @dataclasses.dataclass + class Data: + value: int | str + + obj = _load_into(Data, {'value': json.dumps('x')}) + assert obj.value == 'x' + + @pytest.mark.parametrize('charm_class', _test_classes) def test_relation_save_simple(charm_class: type[BaseTestCharm]): class Charm(charm_class): From 518ca110affff3fbcf8f821bb69fc76a89544c66 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 15:09:29 +0000 Subject: [PATCH 03/10] fix(model): preserve tuple type and coerce heterogeneous tuples positionally origin in (list, tuple) shared one branch, so tuple[X, ...] fields came back as a list, and every element of a fixed-length tuple[X, Y] was coerced against args[0], leaving Y untouched. Split the branch: a tuple stays a tuple, a variable-length tuple[X, ...] (args[-1] is Ellipsis) coerces every element against X, and a fixed-length tuple coerces each position against its own type. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01J1dP9XqjiuoYz4oHjWYAQ6 --- ops/charm.py | 10 +++++++-- test/test_model_relation_data_class.py | 29 ++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/ops/charm.py b/ops/charm.py index aca6fb6dd..46ba0f8be 100644 --- a/ops/charm.py +++ b/ops/charm.py @@ -1708,7 +1708,9 @@ def _coerce_field(tp: Any, value: Any) -> Any: Used by :meth:`ops.Relation.load` to recursively construct nested dataclasses and enum values from JSON-decoded relation data. An ``Optional``/``Union`` field is coerced against its single non-``None`` - member; ``dict``/``Mapping`` fields are coerced against their value type. + member; ``dict``/``Mapping`` fields are coerced against their value type; + a variable-length ``tuple[X, ...]`` is coerced element-wise against ``X`` + and a fixed-length ``tuple[X, Y, ...]`` is coerced positionally. """ origin = typing.get_origin(tp) if origin is not None: @@ -1721,8 +1723,12 @@ def _coerce_field(tp: Any, value: Any) -> Any: # A Union of more than one concrete type: no way to tell which # member to coerce against, so accept the value as-is. return value - if origin in (list, tuple) and args: + if origin is list and args: return [_coerce_field(args[0], v) for v in value] + if origin is tuple and args: + if args[-1] is Ellipsis: + return tuple(_coerce_field(args[0], v) for v in value) + return tuple(_coerce_field(t, v) for t, v in zip(args, value, strict=True)) if origin in (set, frozenset) and args: return {_coerce_field(args[0], v) for v in value} if isinstance(origin, type) and issubclass(origin, Mapping) and len(args) == 2: diff --git a/test/test_model_relation_data_class.py b/test/test_model_relation_data_class.py index e7a876b16..598e8d740 100644 --- a/test/test_model_relation_data_class.py +++ b/test/test_model_relation_data_class.py @@ -45,6 +45,11 @@ class Nested: sub: int = 28 +class _Colour(enum.Enum): + RED = 'red' + BLUE = 'blue' + + class DatabagProtocol(Protocol): foo: str bar: int @@ -494,6 +499,30 @@ class Data: assert obj.value == 'x' +def test_relation_load_variable_length_tuple(): + """tuple[X, ...] is coerced element-wise against X and stays a tuple.""" + + @dataclasses.dataclass + class Data: + items: tuple[Nested, ...] + + obj = _load_into(Data, {'items': json.dumps([{'sub': 1}, {'sub': 2}])}) + assert obj.items == (Nested(sub=1), Nested(sub=2)) + assert isinstance(obj.items, tuple) + + +def test_relation_load_heterogeneous_tuple(): + """A fixed-length tuple[X, Y] is coerced positionally against each type.""" + + @dataclasses.dataclass + class Data: + pair: tuple[int, _Colour] + + obj = _load_into(Data, {'pair': json.dumps([1, 'red'])}) + assert obj.pair == (1, _Colour.RED) + assert isinstance(obj.pair, tuple) + + @pytest.mark.parametrize('charm_class', _test_classes) def test_relation_save_simple(charm_class: type[BaseTestCharm]): class Charm(charm_class): From a6cb2255d94b44566ec98e7bdcd540d993df3283 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 15:10:00 +0000 Subject: [PATCH 04/10] fix(model): detect pydantic dataclasses via __pydantic_validator__ The guard used __is_pydantic_dataclass__, which only exists from pydantic 2.11. Measured across installed versions, it's absent on 2.0.3, 2.4.2, 2.6.4 and 2.10.6. ops declares no pydantic dependency - the charm's own pin decides - and ops's test extra permits 2.10.x, so a charm pinned there got ops's pre-coercion applied ahead of pydantic's own validators, and any extra kwargs pydantic would have accepted silently dropped. '__pydantic_validator__' in cls.__dict__ is what pydantic.dataclasses.is_pydantic_dataclass itself checks for, and was present on every version tested from 2.0.3 up. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01J1dP9XqjiuoYz4oHjWYAQ6 --- ops/model.py | 5 ++++- test/test_model_relation_data_class.py | 23 +++++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/ops/model.py b/ops/model.py index 295d23dab..517fa1699 100644 --- a/ops/model.py +++ b/ops/model.py @@ -1825,10 +1825,13 @@ def _observer(self, event: ops.RelationEvent): data[fields[key]] = decoder(value) # For plain (non-pydantic) dataclass targets, recursively coerce nested # dataclass / enum / list / set fields. Pydantic handles its own coercion. + # '__pydantic_validator__' is what pydantic.dataclasses.is_pydantic_dataclass + # itself checks for; '__is_pydantic_dataclass__' only exists from pydantic + # 2.11, so relying on it missed every earlier 2.x pydantic dataclass. if ( not args and dataclasses.is_dataclass(cls) - and not getattr(cls, '__is_pydantic_dataclass__', False) + and '__pydantic_validator__' not in cls.__dict__ ): return _charm._build_dataclass(cls, data) return cls(*args, **data) diff --git a/test/test_model_relation_data_class.py b/test/test_model_relation_data_class.py index 598e8d740..eeef8a114 100644 --- a/test/test_model_relation_data_class.py +++ b/test/test_model_relation_data_class.py @@ -523,6 +523,29 @@ class Data: assert isinstance(obj.pair, tuple) +def test_relation_load_pydantic_dataclass_guard_without_is_pydantic_dataclass(): + """The pydantic guard must key off __pydantic_validator__, not __is_pydantic_dataclass__. + + __is_pydantic_dataclass__ only exists from pydantic 2.11; older pydantic + dataclasses (as old as 2.0.3) have __pydantic_validator__ in their + __dict__ instead. Simulate that older shape on a plain dataclass, without + needing multiple installed pydantic versions, and confirm Relation.load + still treats it as a pydantic target: ops's own recursive coercion must + not run, so a nested-dataclass-typed field stays a plain decoded dict + rather than being (mis-)coerced ahead of pydantic's own validation. + """ + + @dataclasses.dataclass + class Data: + nested: Nested + + # Simulate pydantic < 2.11's shape. + Data.__pydantic_validator__ = object() # pyright: ignore[reportAttributeAccessIssue] + + obj = _load_into(Data, {'nested': json.dumps({'sub': 1})}) + assert isinstance(obj.nested, dict) + + @pytest.mark.parametrize('charm_class', _test_classes) def test_relation_save_simple(charm_class: type[BaseTestCharm]): class Charm(charm_class): From f2d48c9dd0cb314fe69b2fbd85a40ca89b8a45fc Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 15:10:37 +0000 Subject: [PATCH 05/10] fix(model): fall back to uncoerced construction when type hints can't be resolved get_type_hints(cls) resolves every field's annotation eagerly, so a TYPE_CHECKING-only import with no runtime name raised NameError even for relation data that never touched the affected field - a regression against main's cls(**data) path, which didn't need the hints at all. ops's own ruff config disables TC001/2/3, so charms following ops's own conventions are the ones most likely to hit this. Fall back to the un-coerced cls(**data) when hints can't be resolved. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01J1dP9XqjiuoYz4oHjWYAQ6 --- ops/charm.py | 11 +++++++++- test/test_model_relation_data_class.py | 28 +++++++++++++++++++++++++- 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/ops/charm.py b/ops/charm.py index 46ba0f8be..9bfb781fa 100644 --- a/ops/charm.py +++ b/ops/charm.py @@ -1746,10 +1746,19 @@ def _coerce_field(tp: Any, value: Any) -> Any: def _build_dataclass(cls: Any, data: Mapping[str, Any]) -> Any: """Construct dataclass ``cls`` from ``data``, recursively coercing nested fields. + Falls back to the un-coerced ``cls(**data)`` if ``cls``'s type hints can't + be resolved, for example a ``TYPE_CHECKING``-only import with no runtime + name: ``get_type_hints`` resolves every field's annotation eagerly, so one + unresolvable field would otherwise break construction even when the + relation data at hand doesn't touch it. + Raises ``TypeError`` (via the dataclass ``__init__``) if a required field is missing, and ``ValueError``/``TypeError`` from coercion of malformed values. """ - hints = get_type_hints(cls) + try: + hints = get_type_hints(cls) + except NameError: + return cls(**data) kwargs: dict[str, Any] = {} for field in dataclasses.fields(cls): if field.name not in data: diff --git a/test/test_model_relation_data_class.py b/test/test_model_relation_data_class.py index eeef8a114..3108c37f7 100644 --- a/test/test_model_relation_data_class.py +++ b/test/test_model_relation_data_class.py @@ -21,10 +21,16 @@ import json import urllib.parse from collections.abc import Callable, Iterable -from typing import Any, Protocol, cast +from typing import TYPE_CHECKING, Any, Protocol, cast import pytest +if TYPE_CHECKING: + # Used only by test_relation_load_falls_back_when_type_hints_unresolvable, + # which needs an annotation naming a type that is never actually imported + # at runtime. + import decimal + try: import pydantic import pydantic.dataclasses @@ -546,6 +552,26 @@ class Data: assert isinstance(obj.nested, dict) +def test_relation_load_falls_back_when_type_hints_unresolvable(): + """get_type_hints raises NameError on a TYPE_CHECKING-only annotation. + + ops's own ruff config disables TC001/2/3, so charms following ops's + conventions are the ones most likely to hit this. Relation.load must + fall back to the un-coerced constructor rather than raising, matching + what main's cls(**data) path already did before recursive coercion + existed. + """ + + @dataclasses.dataclass + class Data: + amount: decimal.Decimal | None = None + name: str = '' + + obj = _load_into(Data, {'name': json.dumps('x')}) + assert obj.name == 'x' + assert obj.amount is None + + @pytest.mark.parametrize('charm_class', _test_classes) def test_relation_save_simple(charm_class: type[BaseTestCharm]): class Charm(charm_class): From ad1b14cdf0386ce84c1a003a07f8f5817afc8c6a Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 15:11:23 +0000 Subject: [PATCH 06/10] fix(model): coerce remaining fields when Relation.load is given positional args The `not args` guard meant any positional argument silently turned off coercion entirely: relation.load(MyData, event.app) coerced, and relation.load(MyData, event.app, something) did not, with nothing telling the caller so. args are matched to cls's leading dataclass fields by position, so there is nothing to coerce them against - but the remaining fields, still supplied from the relation data, can and should keep being coerced. _build_dataclass now takes the positional args through and only skips coercion for the fields they fill. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01J1dP9XqjiuoYz4oHjWYAQ6 --- ops/charm.py | 25 +++++++++++-------- ops/model.py | 10 ++++---- test/test_model_relation_data_class.py | 33 ++++++++++++++++++++++++++ 3 files changed, 52 insertions(+), 16 deletions(-) diff --git a/ops/charm.py b/ops/charm.py index 9bfb781fa..b19cca591 100644 --- a/ops/charm.py +++ b/ops/charm.py @@ -1743,14 +1743,19 @@ def _coerce_field(tp: Any, value: Any) -> Any: return value -def _build_dataclass(cls: Any, data: Mapping[str, Any]) -> Any: - """Construct dataclass ``cls`` from ``data``, recursively coercing nested fields. +def _build_dataclass(cls: Any, data: Mapping[str, Any], *args: Any) -> Any: + """Construct dataclass ``cls`` from ``data`` and any positional ``args``. - Falls back to the un-coerced ``cls(**data)`` if ``cls``'s type hints can't - be resolved, for example a ``TYPE_CHECKING``-only import with no runtime - name: ``get_type_hints`` resolves every field's annotation eagerly, so one - unresolvable field would otherwise break construction even when the - relation data at hand doesn't touch it. + Recursively coerces nested dataclass / enum / list / set / tuple / dict + fields supplied via ``data``. Any leading fields already filled + positionally by ``args`` are matched by position, not by name, so they are + passed through as given rather than coerced. + + Falls back to the un-coerced ``cls(*args, **data)`` if ``cls``'s type hints + can't be resolved, for example a ``TYPE_CHECKING``-only import with no + runtime name: ``get_type_hints`` resolves every field's annotation + eagerly, so one unresolvable field would otherwise break construction even + when the relation data at hand doesn't touch it. Raises ``TypeError`` (via the dataclass ``__init__``) if a required field is missing, and ``ValueError``/``TypeError`` from coercion of malformed values. @@ -1758,13 +1763,13 @@ def _build_dataclass(cls: Any, data: Mapping[str, Any]) -> Any: try: hints = get_type_hints(cls) except NameError: - return cls(**data) + return cls(*args, **data) kwargs: dict[str, Any] = {} - for field in dataclasses.fields(cls): + for field in dataclasses.fields(cls)[len(args) :]: if field.name not in data: continue kwargs[field.name] = _coerce_field(hints[field.name], data[field.name]) - return cls(**kwargs) + return cls(*args, **kwargs) class CharmMeta: diff --git a/ops/model.py b/ops/model.py index 517fa1699..68a66668c 100644 --- a/ops/model.py +++ b/ops/model.py @@ -1828,12 +1828,10 @@ def _observer(self, event: ops.RelationEvent): # '__pydantic_validator__' is what pydantic.dataclasses.is_pydantic_dataclass # itself checks for; '__is_pydantic_dataclass__' only exists from pydantic # 2.11, so relying on it missed every earlier 2.x pydantic dataclass. - if ( - not args - and dataclasses.is_dataclass(cls) - and '__pydantic_validator__' not in cls.__dict__ - ): - return _charm._build_dataclass(cls, data) + # Any fields filled positionally by args are left uncoerced, since args + # are matched to the class's leading fields by position, not by name. + if dataclasses.is_dataclass(cls) and '__pydantic_validator__' not in cls.__dict__: + return _charm._build_dataclass(cls, data, *args) return cls(*args, **data) def save( diff --git a/test/test_model_relation_data_class.py b/test/test_model_relation_data_class.py index 3108c37f7..6c944b20c 100644 --- a/test/test_model_relation_data_class.py +++ b/test/test_model_relation_data_class.py @@ -572,6 +572,39 @@ class Data: assert obj.amount is None +def test_relation_load_extra_args_still_coerces_remaining_fields(): + """A positional arg must not silently disable coercion for other fields. + + relation.load(cls, src, *args) matches args to cls's leading fields by + position; any fields filled that way are left uncoerced (there's nothing + to coerce them against without knowing which field each arg is for), but + fields still supplied from the relation data should keep being coerced. + """ + + @dataclasses.dataclass + class Data: + a: int + b: Nested + + class Charm(ops.CharmBase): + def __init__(self, framework: ops.Framework): + super().__init__(framework) + framework.observe(self.on['db'].relation_changed, self._on_relation_changed) + + def _on_relation_changed(self, event: ops.RelationChangedEvent): + self.data = event.relation.load(Data, event.app, 10) + + ctx = testing.Context(Charm, meta={'name': 'foo', 'requires': {'db': {'interface': 'db-int'}}}) + rel = testing.Relation('db', remote_app_data={'b': json.dumps({'sub': 1})}) + state_in = testing.State(leader=True, relations={rel}) + with ctx(ctx.on.relation_changed(rel), state_in) as mgr: + mgr.run() + obj = mgr.charm.data + assert obj.a == 10 + assert isinstance(obj.b, Nested) + assert obj.b.sub == 1 + + @pytest.mark.parametrize('charm_class', _test_classes) def test_relation_save_simple(charm_class: type[BaseTestCharm]): class Charm(charm_class): From 14f95db0d820ae3ce18a4549c4b460494d7fbee7 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 9 Sep 2026 15:11:45 +0000 Subject: [PATCH 07/10] docs(model): document which Relation.load field types are coerced The docstring still said the data "is passed to the data class's __init__ method as keyword arguments", which after recursive coercion holds only for pydantic targets and flat dataclasses. State which field types are coerced, which pass through unchanged, and what happens when positional arguments or unresolvable type hints are involved. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01J1dP9XqjiuoYz4oHjWYAQ6 --- ops/model.py | 43 ++++++++++++++++++++++++++++++++++++++----- 1 file changed, 38 insertions(+), 5 deletions(-) diff --git a/ops/model.py b/ops/model.py index 68a66668c..c6c0564e6 100644 --- a/ops/model.py +++ b/ops/model.py @@ -1766,9 +1766,9 @@ def load( ) -> _T: """Load the data for this relation into an instance of a data class. - The raw Juju relation data is passed to the data class's ``__init__`` - method as keyword arguments, with values decoded using the provided - decoder function, or :func:`json.loads` if no decoder is provided. + The raw Juju relation data is decoded using the provided decoder + function, or :func:`json.loads` if no decoder is provided, and passed + to the data class's ``__init__`` method as keyword arguments. For example:: @@ -1792,8 +1792,41 @@ def _observer(self, event: ops.RelationEvent): data = event.relation.load(Data, event.app) secret = self.model.get_secret(data.secret_id) - Any additional positional or keyword arguments will be passed through to - the data class ``__init__``. + For a Pydantic ``BaseModel`` or pydantic dataclass, the decoded values + are passed straight through as keyword arguments and Pydantic + performs its own coercion and validation. + + For any other :func:`dataclasses.dataclass`, the decoded values are + also recursively coerced to match each field's type hint before being + passed to ``__init__``: + + - A nested dataclass or :class:`enum.Enum` field is constructed from + its decoded value. + - ``list``, ``set``, and ``frozenset`` fields coerce each element + against the type argument. + - A variable-length ``tuple[X, ...]`` coerces every element against + ``X``; a fixed-length ``tuple[X, Y, ...]`` coerces each position + against its own type, and stays a ``tuple``. + - A ``dict``/``Mapping`` field coerces its values against the value + type. + - An ``Optional``/``Union`` field is coerced against its single + non-``None`` member; a ``Union`` of more than one concrete type is + passed through as-is, since there is no way to tell which member to + coerce against. + - ``Literal`` fields, and any other constructed generic not listed + above, are passed through unchanged. + + If the class's type hints can't be resolved at all - for example, a + ``TYPE_CHECKING``-only import with no runtime name - the values are + passed through uncoerced instead of raising. + + Any additional positional or keyword arguments will be passed through + to the data class ``__init__``. For a non-pydantic dataclass target, + positional arguments are matched to the class's leading fields by + position; those fields are passed through as given rather than + coerced, since there is no field name to coerce them against, but any + remaining fields supplied from the relation data are still coerced as + above. Args: cls: A class, typically a Pydantic `BaseModel` subclass or a From d81e1cd75fbca3fe5b642d6b76224352e9d86233 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Thu, 10 Sep 2026 16:52:15 +1200 Subject: [PATCH 08/10] Apply suggestion from @tonyandrewmeyer --- ops/model.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ops/model.py b/ops/model.py index c6c0564e6..b9e0ac8d2 100644 --- a/ops/model.py +++ b/ops/model.py @@ -1860,7 +1860,7 @@ def _observer(self, event: ops.RelationEvent): # dataclass / enum / list / set fields. Pydantic handles its own coercion. # '__pydantic_validator__' is what pydantic.dataclasses.is_pydantic_dataclass # itself checks for; '__is_pydantic_dataclass__' only exists from pydantic - # 2.11, so relying on it missed every earlier 2.x pydantic dataclass. + # 2.11, so relying on it misses every earlier 2.x pydantic dataclass. # Any fields filled positionally by args are left uncoerced, since args # are matched to the class's leading fields by position, not by name. if dataclasses.is_dataclass(cls) and '__pydantic_validator__' not in cls.__dict__: From 99b6a122fce53fd6b7a977c82532ecd86b3c80f6 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Wed, 16 Sep 2026 09:32:00 +1200 Subject: [PATCH 09/10] fix(model): don't coerce Relation.load values that the type can't take Three problems with _coerce_field, all from review of #2741: An Optional[X] field whose databag value is null was coerced against X, which cannot accept None: a nested dataclass raised TypeError from the membership test, a list raised on iteration, and an enum raised ValueError. None now short-circuits, so `field: Nested | None` with `null` in the databag gives None as it did before coercion existed. A frozenset[X] field was built with a set comprehension, so it always produced a set. The set and frozenset branches are now separate and each produces its own type. A str, bytes or mapping value for a sequence field was iterated element-wise, quietly producing a list of characters or of the mapping's keys rather than failing. Those three are now rejected with a TypeError naming the expected type, as is a non-mapping value for a dict field, which previously surfaced as AttributeError: 'list' object has no attribute 'items'. Co-Authored-By: Claude Opus 5 (1M context) --- ops/charm.py | 32 +++++++++- test/test_model_relation_data_class.py | 84 ++++++++++++++++++++++++++ 2 files changed, 113 insertions(+), 3 deletions(-) diff --git a/ops/charm.py b/ops/charm.py index b19cca591..89bded968 100644 --- a/ops/charm.py +++ b/ops/charm.py @@ -1711,6 +1711,11 @@ def _coerce_field(tp: Any, value: Any) -> Any: member; ``dict``/``Mapping`` fields are coerced against their value type; a variable-length ``tuple[X, ...]`` is coerced element-wise against ``X`` and a fixed-length ``tuple[X, Y, ...]`` is coerced positionally. + + Raises ``TypeError`` if the value for a sequence field is a string, bytes + or a mapping, or if the value for a mapping field is not a mapping: those + are all iterable, so coercing them element-wise would quietly produce a + wrong answer rather than fail. """ origin = typing.get_origin(tp) if origin is not None: @@ -1718,21 +1723,42 @@ def _coerce_field(tp: Any, value: Any) -> Any: if origin is typing.Union or origin is types.UnionType: non_none = [a for a in args if a is not type(None)] if len(non_none) == 1: - # Optional[X]: coerce against the one concrete member. + # Optional[X]: coerce against the one concrete member, unless + # the value really is None, which X itself won't accept. + if value is None: + return None return _coerce_field(non_none[0], value) # A Union of more than one concrete type: no way to tell which # member to coerce against, so accept the value as-is. return value + # A str, bytes or mapping is iterable, so coercing element-wise would + # silently succeed with nonsense: a list of characters, or of the + # mapping's keys. None of those is a sequence the charm meant, so + # refuse rather than hand back the wrong answer. + if ( + origin in (list, tuple, set, frozenset) + and args + and isinstance(value, (str, bytes, Mapping)) + ): + given = cast('Any', value) + raise TypeError(f'expected a sequence for {tp}, got {type(given).__name__}: {given!r}') if origin is list and args: return [_coerce_field(args[0], v) for v in value] if origin is tuple and args: if args[-1] is Ellipsis: return tuple(_coerce_field(args[0], v) for v in value) return tuple(_coerce_field(t, v) for t, v in zip(args, value, strict=True)) - if origin in (set, frozenset) and args: + if origin is set and args: return {_coerce_field(args[0], v) for v in value} + if origin is frozenset and args: + return frozenset(_coerce_field(args[0], v) for v in value) if isinstance(origin, type) and issubclass(origin, Mapping) and len(args) == 2: - return {k: _coerce_field(args[1], v) for k, v in value.items()} + if not isinstance(value, Mapping): + raise TypeError( + f'expected a mapping for {tp}, got {type(value).__name__}: {value!r}' + ) + mapping = cast('Mapping[Any, Any]', value) + return {k: _coerce_field(args[1], v) for k, v in mapping.items()} # Literal and other constructed generics: accept the value as-is. return value if isinstance(tp, type): diff --git a/test/test_model_relation_data_class.py b/test/test_model_relation_data_class.py index 6c944b20c..38de6fd24 100644 --- a/test/test_model_relation_data_class.py +++ b/test/test_model_relation_data_class.py @@ -477,6 +477,28 @@ class Data: assert obj.inner is None +def test_relation_load_optional_explicit_null(): + """An Optional[X] field whose databag value is null stays None. + + The value is not coerced against X, which would reject None: building a + nested dataclass, iterating a list, or calling an enum all fail on it. + """ + + @dataclasses.dataclass + class Data: + inner: Nested | None = None + items: list[str] | None = None + colour: _Colour | None = None + + obj = _load_into( + Data, + {'inner': json.dumps(None), 'items': json.dumps(None), 'colour': json.dumps(None)}, + ) + assert obj.inner is None + assert obj.items is None + assert obj.colour is None + + def test_relation_load_dict_of_nested_dataclass(): """dict[str, X] fields are coerced against X for each value.""" @@ -529,6 +551,68 @@ class Data: assert isinstance(obj.pair, tuple) +def test_relation_load_set_and_frozenset(): + """set[X] and frozenset[X] coerce their elements and keep their own type.""" + + @dataclasses.dataclass + class Data: + mutable: set[_Colour] + immutable: frozenset[_Colour] + + obj = _load_into( + Data, {'mutable': json.dumps(['red']), 'immutable': json.dumps(['red', 'blue'])} + ) + assert obj.mutable == {_Colour.RED} + assert type(obj.mutable) is set + assert obj.immutable == frozenset({_Colour.RED, _Colour.BLUE}) + assert type(obj.immutable) is frozenset + + +def test_relation_load_sequence_field_rejects_string_or_mapping(monkeypatch: pytest.MonkeyPatch): + """A str, bytes or mapping value for a sequence field raises, rather than being iterated. + + All three are iterable, so coercing element-wise would silently produce a + list of characters, or of the mapping's keys, instead of failing. + """ + monkeypatch.setenv('SCENARIO_BARE_CHARM_ERRORS', 'true') + + @dataclasses.dataclass + class Data: + tags: list[str] + + with pytest.raises(TypeError, match='expected a sequence'): + _load_into(Data, {'tags': json.dumps('hello')}) + + with pytest.raises(TypeError, match='expected a sequence'): + _load_into(Data, {'tags': json.dumps({'a': 1})}) + + @dataclasses.dataclass + class SetData: + tags: set[str] + + with pytest.raises(TypeError, match='expected a sequence'): + _load_into(SetData, {'tags': json.dumps('hello')}) + + # A genuine sequence is still coerced. + obj = _load_into(Data, {'tags': json.dumps(['hello'])}) + assert obj.tags == ['hello'] + + +def test_relation_load_mapping_field_rejects_non_mapping(monkeypatch: pytest.MonkeyPatch): + """A non-mapping value for a dict field raises a clear error.""" + monkeypatch.setenv('SCENARIO_BARE_CHARM_ERRORS', 'true') + + @dataclasses.dataclass + class Data: + by_name: dict[str, int] + + with pytest.raises(TypeError, match='expected a mapping'): + _load_into(Data, {'by_name': json.dumps([1, 2])}) + + obj = _load_into(Data, {'by_name': json.dumps({'a': 1})}) + assert obj.by_name == {'a': 1} + + def test_relation_load_pydantic_dataclass_guard_without_is_pydantic_dataclass(): """The pydantic guard must key off __pydantic_validator__, not __is_pydantic_dataclass__. From 4c0d6bf4e8dc168502de629cc22c5d8427ba3601 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Wed, 16 Sep 2026 12:05:05 +1200 Subject: [PATCH 10/10] fix(model): don't build a dataclass out of a value that isn't a mapping `_build_dataclass` chooses which fields to fill with `field.name not in data`, which is False for every field of a string or a list, so a remote app writing `"oops"` where a nested dataclass belongs got a confidently default-constructed object corresponding to nothing in the databag - and `"subscribe"`, which happens to contain `sub`, got "string indices must be integers" from inside ops instead. A charm can't stop a remote app writing nonsense, so this is the path that matters. The nested-dataclass branch now rejects a non-mapping the way the sequence and mapping branches already do, naming the class and what arrived. A value that is already an instance of the field's class is passed through: `Relation.load`'s keyword arguments go through the same coercion as the databag, and they are documented as passed through to the data class, so `load(Data, app, nested=Nested(sub=5))` must not try to build a `Nested` out of a `Nested`. `_juju_fields` also keys its pydantic check off `__pydantic_validator__` rather than `__is_pydantic_dataclass__`, which is what `Relation.load` already does and what `pydantic.dataclasses.is_pydantic_dataclass` itself checks. `__is_pydantic_dataclass__` only exists from pydantic 2.11, so on 2.10 an aliased field came back under its field name: measured against pydantic 2.10.6, `_juju_fields` gave `{'secret-id': 'secret_id'}` before this and `{'secret-id': 'secret-id'}` after, matching 2.13.4 either way. ops's own test extra is `pydantic~=2.10`, so both are in scope. Co-Authored-By: Claude Opus 5 (1M context) --- ops/charm.py | 26 ++++++++--- test/test_model_relation_data_class.py | 60 ++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 5 deletions(-) diff --git a/ops/charm.py b/ops/charm.py index 89bded968..f74541e68 100644 --- a/ops/charm.py +++ b/ops/charm.py @@ -1683,10 +1683,13 @@ def _juju_fields(cls: type[object]) -> dict[str, str]: for field in dataclasses.fields(cls): alias = field.metadata.get('alias', field.name) # If this a Pydantic dataclass, then it handles the alias. - # Using pydantic.dataclasses.is_pydantic_dataclass() would be - # best here, but we don't want to import pydantic in ops, so - # we look more explicitly. - if getattr(cls, '__is_pydantic_dataclass__', False): + # Using pydantic.dataclasses.is_pydantic_dataclass() would be best + # here, but we don't want to import pydantic in ops, so we check + # for the attribute that function itself checks for. Note that + # '__is_pydantic_dataclass__' only exists from pydantic 2.11, so + # relying on that one misses every earlier 2.x pydantic dataclass, + # which reads the aliases back under their field names instead. + if '__pydantic_validator__' in cls.__dict__: juju_to_arg[alias] = alias else: juju_to_arg[alias] = field.name @@ -1763,7 +1766,20 @@ def _coerce_field(tp: Any, value: Any) -> Any: return value if isinstance(tp, type): if dataclasses.is_dataclass(tp): - return _build_dataclass(tp, value) + if isinstance(value, tp): + # Already the class we want: a caller's keyword argument passed + # through to `Relation.load`, rather than anything from a + # databag, so there is nothing to coerce. + return value + if not isinstance(value, Mapping): + # Without this, a remote app writing a string or a list where a + # nested dataclass belongs gets a default-constructed object + # that corresponds to nothing in the databag: `field.name not in + # 'oops'` is False for every field, so every one is skipped. + raise TypeError( + f'expected a mapping for {tp.__name__}, got {type(value).__name__}: {value!r}' + ) + return _build_dataclass(tp, cast('Mapping[str, Any]', value)) if issubclass(tp, enum.Enum): return tp(value) return value diff --git a/test/test_model_relation_data_class.py b/test/test_model_relation_data_class.py index 38de6fd24..11804ede9 100644 --- a/test/test_model_relation_data_class.py +++ b/test/test_model_relation_data_class.py @@ -636,6 +636,66 @@ class Data: assert isinstance(obj.nested, dict) +@pytest.mark.parametrize( + 'written', + [ + pytest.param(json.dumps('oops'), id='string'), + pytest.param(json.dumps([1, 2]), id='list'), + pytest.param(json.dumps(3), id='int'), + # A string that happens to contain every field name: the membership + # test that skips absent fields passes for the wrong reason. + pytest.param(json.dumps('subscribe'), id='string-containing-field-name'), + ], +) +def test_relation_load_rejects_a_non_mapping_for_a_nested_dataclass(written: str): + """A remote app can write anything, and ops must not invent an object from it. + + `_build_dataclass` decides which fields to fill with `field.name not in + data`, which is False for every field of a string or a list, so without + this guard the charm is handed a confidently default-constructed object + corresponding to nothing in the databag - or a TypeError from inside ops, + depending on the value. + """ + + @dataclasses.dataclass + class Data: + nested: Nested | None = None + + # The charm doesn't catch it, so ops.testing reports it as an uncaught error. + with pytest.raises(testing.errors.UncaughtCharmError, match='expected a mapping for Nested'): + _load_into(Data, {'nested': written}) + + +def test_relation_load_passes_through_an_already_built_keyword_argument(): + """`kwargs` are documented as passed through to the data class. + + They go through the same coercion as the databag, so an argument that is + already the nested class has to be recognised rather than treated as data + to build one from. + """ + + @dataclasses.dataclass + class Data: + name: str = '' + nested: Nested | None = None + + class Charm(ops.CharmBase): + def __init__(self, framework: ops.Framework): + super().__init__(framework) + framework.observe(self.on['db'].relation_changed, self._on_relation_changed) + + def _on_relation_changed(self, event: ops.RelationChangedEvent): + self.data = event.relation.load(Data, event.app, nested=Nested(sub=5)) + + ctx = testing.Context(Charm, meta={'name': 'foo', 'requires': {'db': {'interface': 'db-int'}}}) + rel = testing.Relation('db', remote_app_data={'name': json.dumps('x')}) + with ctx(ctx.on.relation_changed(rel), testing.State(relations={rel})) as mgr: + mgr.run() + data = mgr.charm.data + + assert data == Data(name='x', nested=Nested(sub=5)) + + def test_relation_load_falls_back_when_type_hints_unresolvable(): """get_type_hints raises NameError on a TYPE_CHECKING-only annotation.