Migrate to cattrs for centralized model structuring by iMicknl · Pull Request #1994 · iMicknl/python-overkiz-api

iMicknl · 2026-04-17T15:03:02Z

Summary

Alternative to #1991 — replaces per-model converter functions with a centralized cattrs Converter.

Adds pyoverkiz/converter.py with hooks for primitive unions, optional attrs classes, enums (with UnknownEnumMixin), and custom containers (States, CommandDefinitions)
Strips all converter infrastructure from models.py — models become pure @define(kw_only=True) data classes with only type annotations and defaults
Updates client.py to use converter.structure() instead of Model(**data)
Updates tests to use the converter for API-like dict structuring

Comparison with #1991

	#1991 (model-simplifications)	This PR (cattrs)
Converter location	Per-model (`_flexible_init`, `_to_list`, etc.)	Centralized `converter.py`
Model classes	Still have some converter logic	Pure data classes
Dependencies	None new	Adds `cattrs`
Forward references	Eliminated by class reordering	Not needed — cattrs resolves types
Splitting models later	Requires careful import ordering	No ordering constraints

Breaking Changes

ServerConfig.type renamed to ServerConfig.api_type.
Gateway.connectivity type changed to Connectivity | None (was non-optional).
Gateway.id and Place.id changed from mutable fields to read-only properties.
States.__getitem__ now raises KeyError instead of returning None — use States.get() for the old behavior.
CommandDefinitions.__getitem__ now raises KeyError instead of returning None — use CommandDefinitions.get() for the old behavior.
Event.setup_oid — field name unchanged (cattrs handles the setupOID → setup_oid conversion automatically).
Execution.action_group type changed from list[Action] to ActionGroup | None.
ActionGroup.label default simplified (no longer falls back to empty string).

Test plan

All 291 tests pass
ruff check passes
mypy passes
ty check passes
Compare readability and maintainability with Simplify model definitions with attrs converters and declarative patterns #1991

Copilot

Pull request overview

This PR migrates Overkiz API response structuring from per-model init/converter logic to a centralized cattrs converter, making pyoverkiz.models primarily plain attrs data classes and updating the client/tests to structure API-like dicts through the shared converter.

Changes:

Added a centralized pyoverkiz/converter.py with cattrs hooks (unknown key filtering, enums, unions, and custom containers).
Refactored pyoverkiz/models.py to remove most custom __init__ converter logic and rely on cattrs structuring + __attrs_post_init__ where needed.
Updated pyoverkiz/client.py and tests to use converter.structure(...); added cattrs as a dependency.

Reviewed changes

Copilot reviewed 8 out of 9 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
`pyoverkiz/converter.py`	Introduces centralized cattrs converter and structuring hooks.
`pyoverkiz/models.py`	Simplifies models/containers to attrs classes and post-init computed fields.
`pyoverkiz/client.py`	Switches API parsing to `converter.structure()` across endpoints.
`pyoverkiz/auth/factory.py`	Updates `ServerConfig` field rename usage (`type` → `api_type`).
`tests/test_models.py`	Updates model tests to structure API-like dicts via converter; adds container behavior tests.
`tests/test_ui_profile.py`	Updates UI profile tests to use structured models and converter.
`tests/test_client.py`	Updates tests for `ServerConfig.api_type` and `Action` kw-only initialization.
`pyproject.toml`	Adds `cattrs` dependency.
`uv.lock`	Locks `cattrs` and updates dependency graph accordingly.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-17T15:08:19Z

    def __setitem__(self, name: str, state: State) -> None:
        """Set or append a State identified by name."""
-        found = self.__getitem__(name)
-        if found is None:
-            self._states.append(state)
+        if name in self._index:
+            idx = self._states.index(self._index[name])
+            self._states[idx] = state
        else:
-            self._states[self._states.index(found)] = state
+            self._states.append(state)
+        self._index[name] = state


States.__setitem__ updates the index using the provided name key, but does not validate that state.name matches that key. This can silently corrupt the index (e.g., storing a State under a different name than its own). Consider enforcing state.name == name (raise) or overriding state.name to name before indexing.

Copilot · 2026-04-17T15:08:19Z

+def _is_primitive_union(t: Any) -> bool:
+    """Check if a union type only contains primitive/JSON-native types and enums.
+
+    Returns False if any union member is an attrs class (needs structuring).
+    """
+    if not _is_union(t):
+        return False
+    for arg in get_args(t):
+        if arg is type(None):
+            continue
+        if isinstance(arg, type) and attr.has(arg):
+            return False
+    return True
+
+
+def _make_converter() -> cattrs.Converter:
+    c = cattrs.Converter(forbid_extra_keys=False)
+
+    # ------------------------------------------------------------------
+    # Primitive union types (str | Enum, StateType, etc.): passthrough
+    # These only contain JSON-native types and enums, no attrs classes.
+    # ------------------------------------------------------------------
+    c.register_structure_hook_func(_is_primitive_union, lambda v, _: v)
+
+    # ------------------------------------------------------------------
+    # Optional[AttrsClass] unions: structure the non-None member
+    # ------------------------------------------------------------------
+    def _is_optional_attrs(t: Any) -> bool:
+        if not _is_union(t):
+            return False
+        args = get_args(t)
+        non_none = [a for a in args if a is not type(None)]
+        return (
+            len(non_none) == 1
+            and isinstance(non_none[0], type)
+            and attr.has(non_none[0])
+        )
+
+    def _structure_optional_attrs(v: Any, t: Any) -> Any:
+        if v is None:
+            return None
+        args = get_args(t)
+        cls = next(a for a in args if a is not type(None))
+        return c.structure(v, cls)
+
+    c.register_structure_hook_func(_is_optional_attrs, _structure_optional_attrs)
+
+    # ------------------------------------------------------------------
+    # Enums: use the enum constructor which handles UnknownEnumMixin
+    # ------------------------------------------------------------------
+    c.register_structure_hook_func(
+        lambda t: isinstance(t, type) and issubclass(t, Enum),
+        lambda v, t: v if isinstance(v, t) else t(v),
+    )


The “primitive union passthrough” structure hook currently matches Optional[Enum] / Enum | None fields (e.g., FailureType | None, GatewayType | None) and returns the raw value unchanged, preventing the Enum hook from running. This regresses previous behavior where these fields were converted to their Enum types. Consider excluding Enum subclasses from _is_primitive_union, and/or adding a dedicated hook for Optional[Enum] so unions containing enums are still structured into enums.

Copilot

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-18T10:39:40Z

+def _is_primitive_union(t: Any) -> bool:
+    """True for unions of JSON-native types (e.g. StateType).
+
+    Excludes unions containing attrs classes (e.g. Definition | None) since those
+    need actual structuring by cattrs.
+    """
+    origin = get_origin(t)
+    if origin is not Union and not isinstance(t, types.UnionType):
+        return False
+    return all(
+        arg is type(None) or not (isinstance(arg, type) and attr.has(arg))
+        for arg in get_args(t)
+    )
+
+
+def _make_converter() -> cattrs.Converter:
+    c = cattrs.Converter()
+
+    # JSON-native unions like StateType (str | int | float | … | None) are already the
+    # correct Python type after JSON parsing — tell cattrs to pass them through as-is.
+    c.register_structure_hook_func(_is_primitive_union, lambda v, _: v)
+
+    # Enums: call the constructor so UnknownEnumMixin._missing_ can handle unknown values
+    c.register_structure_hook_func(
+        lambda t: isinstance(t, type) and issubclass(t, Enum),
+        lambda v, t: v if isinstance(v, t) else t(v),
+    )


_is_primitive_union() currently treats Optional[Enum] / Enum | None (and other unions that include Enum subclasses) as a “primitive union” and returns the raw value unchanged. That bypasses the Enum structure hook, so fields like Event.failure_type_code: FailureType | None and Event.old_state/new_state: ExecutionState | None may remain int/str instead of Enum instances, breaking code that relies on Enum attributes (e.g., .name) and skipping UnknownEnumMixin handling. Consider narrowing this hook to only JSON-native primitives (str/int/float/bool/None/dict/list) and explicitly excluding Enum subclasses (and possibly other non-primitive types).

…ation defaults - Replace manual __init__ methods with attrs converters/factories across all model classes, using _flexible_init decorator to handle unknown API keys. - Rename ServerConfig.type to api_type to avoid shadowing the builtin. Update all references in client.py, auth/factory.py, and tests. - Fix Location.__init__ bug where field() objects were used as parameter defaults instead of None (now fixed by removing manual __init__).

__getitem__ now raises KeyError on missing keys (standard Python mapping protocol). get() returns None for missing keys. __contains__ uses explicit name comparison instead of delegating to __getitem__.

Replace Any types with concrete types based on actual API payloads: metadata -> str, deleted_raw_devices_count -> int, protocol_type -> int.

Both containers used O(n) linear scans for __contains__, __getitem__, get, and select. These are called on hot paths during Home Assistant polling cycles across many devices.

…init Replace manual __init__ with declarative attrs fields and __attrs_post_init__. The ui_class and widget fields are now public, resolved at construction time from either the API kwargs or the definition fallback.

…rties

## Summary - ActionGroup `label` field now defaults to `""` via a `_to_str` converter instead of post-init fixup - Simplifies `__attrs_post_init__` to only handle id/oid resolution Depends on #1991 (model simplifications). ## Breaking changes - `ActionGroup.label` type changed from `str | None` to `str` (always a string, `None` input is converted to `""`) ## Test plan - [ ] ActionGroup tests pass with label=None, label="", and label="value"

The API returns `setupOID` which decamelizes to `setup_oid`, but the field was named `setupoid`. This caused `_flexible_init` to silently drop the value, making `Event.setup_oid` always None.

action_group uses _to_optional which can return None, but the type annotation was non-optional. Also add start_time, execution_type and execution_sub_type which are present in the API response but were silently dropped by _flexible_init.

The field uses _to_optional converter which returns None for None input, but the type annotation was non-optional Connectivity.

Definition.commands uses an inline lambda converter instead. This function was defined but never referenced anywhere.

Reorder class definitions so dependencies are defined before dependents, allowing converter helpers to reference actual classes instead of string names. Setup is moved to the bottom since it references nearly every other model. The only remaining string forward reference is Place.sub_places which is self-referential and unavoidable. Also adds section comments to group models by domain: state/command primitives, device/definition, execution/action groups, infrastructure, configuration, and UI profiles.

Replace per-model converter functions (_flexible_init, _to_list, _to_optional, _to_optional_enum) with a single cattrs Converter that handles all dict-to-model structuring. Models become pure data classes with only type annotations and defaults. - Add cattrs dependency - Add pyoverkiz/converter.py with hooks for primitive unions, optional attrs classes, enums, and container types (States, CommandDefinitions) - Strip all converter infrastructure from models.py - Update client.py to use converter.structure() instead of Model(**data) - Update tests to use converter for API-like dict structuring

cattrs 26.1.0 natively handles Optional[AttrsClass] unions, extra dict keys, and enum structuring — remove the custom hooks that duplicated this built-in behavior.

…ons for clarity

The _is_primitive_union hook was matching unions like FailureType | None and ExecutionState | None, causing those fields to pass through as raw values instead of being structured into enum instances. Exclude pure Optional[Enum] unions so the enum hook handles them correctly.

…nced functionality

… privacy and representation

…te their presence

…redundant id field

iMicknl requested a review from tetienne as a code owner April 17, 2026 15:03

Copilot AI review requested due to automatic review settings April 17, 2026 15:03

Copilot started reviewing on behalf of iMicknl April 17, 2026 15:03 View session

iMicknl added enhancement New feature or request breaking v2 labels Apr 17, 2026

Copilot AI reviewed Apr 17, 2026

View reviewed changes

iMicknl force-pushed the v2/cattrs-models branch from 2aaee1b to 1c7679b Compare April 17, 2026 15:11

iMicknl requested a review from Copilot April 18, 2026 10:35

Copilot started reviewing on behalf of iMicknl April 18, 2026 10:35 View session

Copilot AI reviewed Apr 18, 2026

View reviewed changes

iMicknl added 19 commits April 19, 2026 20:29

Fix States/CommandDefinitions container semantics

229f941

__getitem__ now raises KeyError on missing keys (standard Python mapping protocol). get() returns None for missing keys. __contains__ uses explicit name comparison instead of delegating to __getitem__.

Type Event.metadata, deleted_raw_devices_count, and protocol_type

4b22346

Replace Any types with concrete types based on actual API payloads: metadata -> str, deleted_raw_devices_count -> int, protocol_type -> int.

Add dict index to CommandDefinitions and States for O(1) lookups

67877b8

Both containers used O(n) linear scans for __contains__, __getitem__, get, and select. These are called on hot paths during Home Assistant polling cycles across many devices.

Simplify DeviceIdentifier to use __attrs_post_init__ for base_device_url

784bb08

Replace _to_sub_places with _to_list("Place") forward reference

ea74572

Replace _to_server_enum and _to_api_type with _to_optional_enum

3b7f9d5

Remove unused _LOGGER and stale pylint comment

c42663b

Change Gateway.id and Place.id from mutable copies to read-only prope…

04dfb45

…rties

Fix Event.setupoid field name to match decamelize output

ca6ab24

The API returns `setupOID` which decamelizes to `setup_oid`, but the field was named `setupoid`. This caused `_flexible_init` to silently drop the value, making `Event.setup_oid` always None.

Fix Gateway.connectivity type annotation to be optional

bc72514

The field uses _to_optional converter which returns None for None input, but the type annotation was non-optional Connectivity.

Remove unused _to_command_definitions function

d6a33e3

Definition.commands uses an inline lambda converter instead. This function was defined but never referenced anywhere.

Simplify cattrs converter by removing redundant hooks

32c74ec

cattrs 26.1.0 natively handles Optional[AttrsClass] unions, extra dict keys, and enum structuring — remove the custom hooks that duplicated this built-in behavior.

Improve docstrings for _is_primitive_union and _make_converter functi…

9d71d5d

…ons for clarity

iMicknl added 7 commits April 19, 2026 20:29

Add new fields to Event, Gateway, Location, and Setup models for enha…

d9f301b

…nced functionality

Obfuscate country_code field in Location model for enhanced privacy

6f2ae74

Add UpdateCriticityLevel enum and update Gateway model to use it

184f639

Refactor dataclass fields in AuthContext and Credentials for improved…

688a235

… privacy and representation

Add default values for ui_class and widget in Device model and valida…

017b673

…te their presence

Refactor ActionGroup to use oid as the primary identifier and remove …

569325c

…redundant id field

iMicknl force-pushed the v2/cattrs-models branch from a17da1a to 569325c Compare April 19, 2026 18:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to cattrs for centralized model structuring#1994

Migrate to cattrs for centralized model structuring#1994
iMicknl wants to merge 26 commits intov2/mainfrom
v2/cattrs-models

iMicknl commented Apr 17, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI Apr 17, 2026

Uh oh!

Uh oh!

Uh oh!

Copilot AI Apr 17, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI Apr 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

iMicknl commented Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Comparison with #1991

Breaking Changes

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Apr 18, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

iMicknl commented Apr 17, 2026 •

edited

Loading