From 08e6cd1bd2b6a1ce02aecb9ce30cac9ba166ffc4 Mon Sep 17 00:00:00 2001 From: Ivan Kalchev Date: Fri, 13 Nov 2026 16:01:45 +0100 Subject: [PATCH] Wire delegates on characteristics and services The registration side of the delegate interface: a characteristic or a service gets a delegate property, and Service.configure_char accepts one. Assigning an instance or a class - instantiated with no arguments - is when everything about the delegate is decided: the driver records which methods it overrides, and an overridden method that is not a coroutine function fails right there with a TypeError. The read and write paths never introspect a delegate again; None or del removes it. The legacy setter_callback/getter_callback attributes become storage properties that keep returning the assigned callable, so existing code - the driver, the camera stack, tests that patch them - sees no change. Assigning a delegate does not remove a legacy callback; which one wins is decided when the pipelines dispatch delegates. The driver does not call delegates yet - that is the gate pipeline change. This wires them so everything about them is validated at assignment, as the interface promises. --- docs/source/api/delegates.rst | 37 +++++++++ pyhap/characteristic.py | 81 ++++++++++++++++++- pyhap/delegates.py | 71 +++++++++++++++- pyhap/service.py | 69 +++++++++++++++- tests/test_accessory_driver.py | 10 ++- tests/test_admin_only.py | 5 +- tests/test_characteristic.py | 144 +++++++++++++++++++++++++++++++++ tests/test_service.py | 59 ++++++++++++++ 8 files changed, 465 insertions(+), 11 deletions(-) diff --git a/docs/source/api/delegates.rst b/docs/source/api/delegates.rst index 31f4bf54..8e9fc969 100644 --- a/docs/source/api/delegates.rst +++ b/docs/source/api/delegates.rst @@ -26,3 +26,40 @@ call and enforces the context's deadline - there is no sync/async detection. An overridden method that is not a coroutine function fails registration with a :class:`TypeError`, so a wrong method is caught at its source. + +Wiring a delegate +================= + +Assign to the ``delegate`` of a characteristic or a service, or pass +it through :meth:`Service.configure_char +` - an instance, or a class that +is then instantiated with no arguments: + +.. code-block:: python + + class MyLock(CharacteristicDelegate): + async def on_set(self, value, context): + ... + + char = service.configure_char("LockTargetState", delegate=MyLock) + # is the same as + service.get_characteristic("LockTargetState").delegate = MyLock() + +The assignment is when everything about the delegate is decided: which +methods it overrides is recorded, an overridden method that is not a +coroutine function fails with a :class:`TypeError`, and the read and +write paths never introspect the delegate again. ``None`` or ``del`` +removes it. + +One instance may serve many characteristics: the context of a call +carries :attr:`~pyhap.context.CharReadContext.characteristic` and says +which one a call is about, e.g. one delegate per accessory rather than +one per characteristic. + +**Rollout state.** The driver stores and validates delegates from this +release on; the write and read pipelines dispatch them in the following +releases of the callback interface refactor. Until then, a delegate is +wired but not yet called, and the legacy +:attr:`~pyhap.characteristic.Characteristic.setter_callback` / +:attr:`~pyhap.characteristic.Characteristic.getter_callback` attributes +keep working unchanged - assigning a delegate does not remove them. diff --git a/pyhap/characteristic.py b/pyhap/characteristic.py index 6c88e631..8d1b48f4 100644 --- a/pyhap/characteristic.py +++ b/pyhap/characteristic.py @@ -21,6 +21,7 @@ HAP_REPR_VALID_VALUES, HAP_REPR_VALUE, ) +from .delegates import CharacteristicDelegate, _characteristic_delegate from .util import hap_type_to_uuid, uuid_to_hap_type if TYPE_CHECKING: @@ -184,8 +185,10 @@ class Characteristic: "_properties", "type_id", "_value", - "getter_callback", - "setter_callback", + "_getter_callback", + "_setter_callback", + "_delegate", + "_delegate_hooks", "service", "_uuid_str", "_loader_display_name", @@ -236,6 +239,8 @@ def __init__( self._properties: dict[str, Any] = properties self.type_id = type_id self._value = self._get_default_value() + self._delegate: CharacteristicDelegate | None = None + self._delegate_hooks: frozenset[str] = frozenset() self.getter_callback: Callable[[], Any] | None = None # Setters may take just the value or (value, sender_client_addr); see # ``_setter_wants_addr``. The return value is sent back as the write @@ -269,6 +274,78 @@ def display_name(self, value: str) -> None: self._display_name = value self._clear_cache() + @property + def delegate(self) -> CharacteristicDelegate | None: + """The characteristic's delegate, or ``None`` without one. + + Assign a :class:`~pyhap.delegates.CharacteristicDelegate` instance, + or its class to instantiate one with no arguments. Everything about + a delegate is decided at this assignment: the driver records which + methods it overrides, and an overridden method that is not a + coroutine function fails here with a ``TypeError`` - there is no + sync/async detection later. One instance may serve many + characteristics; the context of a call says which one it is about. + """ + return self._delegate + + @delegate.setter + def delegate( + self, delegate: CharacteristicDelegate | type[CharacteristicDelegate] | None + ) -> None: + """Wire the delegate; ``None`` removes it.""" + self._delegate, self._delegate_hooks = ( + (None, frozenset()) + if delegate is None + else _characteristic_delegate(delegate) + ) + + @delegate.deleter + def delegate(self) -> None: + """Remove the delegate.""" + self._delegate, self._delegate_hooks = None, frozenset() + + @property + def getter_callback(self) -> Callable[[], Any] | None: + """The legacy getter, called with nothing on every read of the value. + + A sync callable; a slow one stalls the server. Replaced over time by + a delegate's ``on_get``, assigning one does not remove this. + """ + return self._getter_callback + + @getter_callback.setter + def getter_callback(self, callback: Callable[[], Any] | None) -> None: + """Set the legacy getter; ``None`` removes it.""" + self._getter_callback = callback + + @getter_callback.deleter + def getter_callback(self) -> None: + """Remove the legacy getter.""" + self._getter_callback = None + + @property + def setter_callback(self) -> Callable[..., Any] | None: + """The legacy setter, called with the value on a controller's write. + + May take just the value or ``(value, sender_client_addr)``, see + ``_setter_wants_addr``. The return value is sent back as the write + response when the characteristic has the "wr" permission and the + controller asked for one. May also be a coroutine function; a + controller's write is answered once it is done. Replaced over time + by a delegate's ``on_set``, assigning one does not remove this. + """ + return self._setter_callback + + @setter_callback.setter + def setter_callback(self, callback: Callable[..., Any] | None) -> None: + """Set the legacy setter; ``None`` removes it.""" + self._setter_callback = callback + + @setter_callback.deleter + def setter_callback(self) -> None: + """Remove the legacy setter.""" + self._setter_callback = None + @property def value(self) -> Any: """Return the value of the characteristic.""" diff --git a/pyhap/delegates.py b/pyhap/delegates.py index 41c5b9ee..67df54c1 100644 --- a/pyhap/delegates.py +++ b/pyhap/delegates.py @@ -17,13 +17,82 @@ from __future__ import annotations from collections.abc import Mapping -from typing import TYPE_CHECKING, Any +import inspect +from typing import TYPE_CHECKING, Any, TypeVar from .context import CharReadContext, CharWriteContext if TYPE_CHECKING: from .characteristic import Characteristic +_CHARACTERISTIC_HOOKS = ("on_get", "on_set", "on_subscribe", "on_unsubscribe") +_SERVICE_HOOKS = ("on_set",) +_Wired = TypeVar("_Wired") + + +def _wire_delegate( + delegate: Any, base: type[_Wired], hooks: tuple[str, ...], what: str +) -> tuple[_Wired, frozenset[str]]: + """Validate a delegate at its assignment; return it with its hooks. + + This is the whole of the "handled in init only" promise: the driver + records what a delegate overrides and that every overridden hook is a + coroutine function, and the read and write paths never introspect a + delegate again. Returns the delegate instance, a class is instantiated + with no arguments, and the frozenset of its overridden hook names. + + :param delegate: A delegate instance, or its class. + :param base: The base class the delegate must derive from. + :param hooks: The hook methods ``base`` defines. + :param what: What is being wired, e.g. "a characteristic's delegate", + for error messages. + :raises TypeError: If the delegate is not of the right class, or an + overridden hook is not a coroutine function. + """ + if isinstance(delegate, type): + if not issubclass(delegate, base): + raise TypeError(f"{delegate!r} is not a {base.__name__} subclass") + try: + delegate = delegate() + except TypeError as e: + raise TypeError( + f"{delegate.__name__} is instantiated with no arguments when" + f" it wires {what}: {e}" + ) from e + elif not isinstance(delegate, base): + raise TypeError(f"{delegate!r} is not a {base.__name__} instance") + + overridden = frozenset( + hook + for hook in hooks + if getattr(type(delegate), hook) is not getattr(base, hook) + ) + for hook in overridden: + # Async by design: the driver awaits every call, so a hook that is + # not a coroutine function never runs. Fail at the assignment + # instead, where the mistake is made. + if not inspect.iscoroutinefunction(getattr(delegate, hook)): + raise TypeError( + f"{type(delegate).__name__}.{hook} of {what} is not a coroutine" + " function; delegate methods are async by design, declare" + f" 'async def {hook}'" + ) + return delegate, overridden + + +def _characteristic_delegate( + delegate: Any, +) -> tuple[CharacteristicDelegate, frozenset[str]]: + """Wire a characteristic's delegate; see :func:`_wire_delegate`.""" + return _wire_delegate( + delegate, CharacteristicDelegate, _CHARACTERISTIC_HOOKS, "a characteristic" + ) + + +def _service_delegate(delegate: Any) -> tuple[ServiceDelegate, frozenset[str]]: + """Wire a service's delegate; see :func:`_wire_delegate`.""" + return _wire_delegate(delegate, ServiceDelegate, _SERVICE_HOOKS, "a service") + class CharacteristicDelegate: """Delegate of a characteristic's reads, writes and subscriptions. diff --git a/pyhap/service.py b/pyhap/service.py index d4171ef7..0040fa4d 100644 --- a/pyhap/service.py +++ b/pyhap/service.py @@ -13,6 +13,7 @@ ) from .characteristic import Characteristic +from .delegates import CharacteristicDelegate, ServiceDelegate, _service_delegate from .util import hap_type_to_uuid, uuid_to_hap_type if TYPE_CHECKING: @@ -34,7 +35,9 @@ class Service: "type_id", "linked_services", "is_primary_service", - "setter_callback", + "_setter_callback", + "_delegate", + "_delegate_hooks", "unique_id", "_uuid_str", ) @@ -52,10 +55,58 @@ def __init__( self.display_name = display_name self.type_id = type_id self.is_primary_service = None + self._delegate: ServiceDelegate | None = None + self._delegate_hooks: frozenset[str] = frozenset() self.setter_callback: Callable[[Any], None] | None = None self.unique_id = unique_id self._uuid_str = uuid_to_hap_type(type_id) + @property + def delegate(self) -> ServiceDelegate | None: + """The service's delegate, or ``None`` without one. + + Assign a :class:`~pyhap.delegates.ServiceDelegate` instance, or its + class to instantiate one with no arguments. The assignment is when + everything about the delegate is decided: the driver records that its + ``on_set`` is overridden and a coroutine function, and a method that + is not fails with a ``TypeError``. + """ + return self._delegate + + @delegate.setter + def delegate( + self, delegate: ServiceDelegate | type[ServiceDelegate] | None + ) -> None: + """Wire the delegate; ``None`` removes it.""" + self._delegate, self._delegate_hooks = ( + (None, frozenset()) if delegate is None else _service_delegate(delegate) + ) + + @delegate.deleter + def delegate(self) -> None: + """Remove the delegate.""" + self._delegate, self._delegate_hooks = None, frozenset() + + @property + def setter_callback(self) -> Callable[[Any], None] | None: + """The legacy setter, called with the request's values by name. + + Called with a ``{display_name: value}`` dict after the + per-characteristic setters of a write. Replaced over time by the + delegate's grouped ``on_set``, assigning one does not remove this. + """ + return self._setter_callback + + @setter_callback.setter + def setter_callback(self, callback: Callable[[Any], None] | None) -> None: + """Set the legacy setter; ``None`` removes it.""" + self._setter_callback = callback + + @setter_callback.deleter + def setter_callback(self) -> None: + """Remove the legacy setter.""" + self._setter_callback = None + def __repr__(self) -> str: """Return the representation of the service.""" chars_dict = {c.display_name: c.value for c in self.characteristics} @@ -108,8 +159,20 @@ def configure_char( value: Any = None, setter_callback: Any = None, getter_callback: Any = None, + delegate: CharacteristicDelegate | type[CharacteristicDelegate] | None = None, ) -> Characteristic: - """Helper method to return fully configured characteristic.""" + """Helper method to return fully configured characteristic. + + :param char_name: The name of the characteristic to configure. + :param properties: Properties to override on the characteristic. + :param valid_values: Valid values to set on the characteristic. + :param value: The value to set on the characteristic. + :param setter_callback: The legacy setter of the characteristic. + :param getter_callback: The legacy getter of the characteristic. + :param delegate: The characteristic's + :class:`~pyhap.delegates.CharacteristicDelegate`, an instance or + its class - instantiated with no arguments. + """ char = self.get_characteristic(char_name) if properties or valid_values: char.override_properties(properties, valid_values) @@ -119,6 +182,8 @@ def configure_char( char.setter_callback = setter_callback if getter_callback: char.getter_callback = getter_callback + if delegate is not None: + char.delegate = delegate return char def to_HAP(self, include_value: bool = True) -> dict[str, Any]: diff --git a/tests/test_accessory_driver.py b/tests/test_accessory_driver.py index c78e14d8..60771b93 100644 --- a/tests/test_accessory_driver.py +++ b/tests/test_accessory_driver.py @@ -707,7 +707,8 @@ def test_write_response_returned_when_requested( service_setter = MagicMock() assert char.service is not None char.service.setter_callback = service_setter - char.setter_callback = MagicMock(return_value=returned) + setter = MagicMock(return_value=returned) + char.setter_callback = setter response = _write(driver, acc, char, **{HAP_REPR_WRITE_RESPONSE: True}) @@ -721,7 +722,7 @@ def test_write_response_returned_when_requested( } ] } - char.setter_callback.assert_called_once_with(0) + setter.assert_called_once_with(0) service_setter.assert_called_once_with({"Value": 0}) assert char.value == stored @@ -744,10 +745,11 @@ def test_write_response_not_returned( ) -> None: """Without a request or the wr permission the write has no response body.""" acc, char, _ = _add_bridged_or_standalone(driver, permissions, False) - char.setter_callback = MagicMock(return_value=1) + setter = MagicMock(return_value=1) + char.setter_callback = setter assert _write(driver, acc, char, **query) is None - char.setter_callback.assert_called_once_with(0) + setter.assert_called_once_with(0) assert char.value == 0 diff --git a/tests/test_admin_only.py b/tests/test_admin_only.py index f63f2832..7e7f896c 100644 --- a/tests/test_admin_only.py +++ b/tests/test_admin_only.py @@ -295,7 +295,8 @@ def test_admin_only_null_write_reaches_no_setter( assert driver.accessory is not None service = driver.accessory.get_service("MotionSensor") assert service is not None - service.setter_callback = MagicMock() + service_setter = MagicMock() + service.setter_callback = service_setter driver.accessory.setter_callback = MagicMock() query = { HAP_REPR_CHARS: [ @@ -307,7 +308,7 @@ def test_admin_only_null_write_reaches_no_setter( assert response == _response(char, REFUSED) char.setter.assert_not_called() - service.setter_callback.assert_not_called() + service_setter.assert_not_called() driver.accessory.setter_callback.assert_not_called() diff --git a/tests/test_characteristic.py b/tests/test_characteristic.py index ba10d027..4da9433e 100644 --- a/tests/test_characteristic.py +++ b/tests/test_characteristic.py @@ -18,6 +18,7 @@ HAP_PERMISSION_READ, Characteristic, ) +from pyhap.delegates import CharacteristicDelegate PROPERTIES: dict[str, Any] = { "Format": HAP_FORMAT_INT, @@ -584,3 +585,146 @@ def test_getter_callback() -> None: "perms": ["pr"], "type": "A1", } + + +class _AsyncGetter(CharacteristicDelegate): + """A delegate overriding on_get.""" + + async def on_get(self, context: object) -> int: + return 1 + + +class _SyncGetter(CharacteristicDelegate): + """A delegate whose override is not async, a mistake to catch at wiring.""" + + +def _sync_on_get(self: Any, context: object) -> int: + return 1 + + +_SyncGetter.on_get = _sync_on_get # ty: ignore[invalid-assignment] + + +def test_delegate_instance_roundtrip() -> None: + """An assigned delegate instance is the characteristic's delegate.""" + char = get_char(PROPERTIES) + delegate = _AsyncGetter() + + char.delegate = delegate + + assert char.delegate is delegate + + +def test_delegate_class_is_instantiated() -> None: + """Assigning a class instantiates it with no arguments.""" + char = get_char(PROPERTIES) + + char.delegate = _AsyncGetter + + assert isinstance(char.delegate, _AsyncGetter) + + +def test_delegate_removed() -> None: + """None or del removes the delegate.""" + char = get_char(PROPERTIES) + char.delegate = _AsyncGetter() + + char.delegate = None + + assert char.delegate is None + + char.delegate = _AsyncGetter() + del char.delegate + + assert char.delegate is None + + +def test_delegate_replaced() -> None: + """A new assignment replaces the previous delegate.""" + char = get_char(PROPERTIES) + first = _AsyncGetter() + second = _AsyncGetter() + + char.delegate = first + char.delegate = second + + assert char.delegate is second + + +def test_delegate_must_be_a_delegate() -> None: + """Something else than a delegate fails the assignment.""" + char = get_char(PROPERTIES) + + with pytest.raises(TypeError, match="not a CharacteristicDelegate instance"): + char.delegate = object() # ty: ignore[invalid-assignment] + + with pytest.raises(TypeError, match="not a CharacteristicDelegate subclass"): + char.delegate = int # ty: ignore[invalid-assignment] + + +def test_delegate_class_needs_no_arguments() -> None: + """A class whose constructor needs arguments fails the assignment.""" + + class _NeedsArgs(CharacteristicDelegate): + def __init__(self, needed: int) -> None: + super().__init__() + + char = get_char(PROPERTIES) + + with pytest.raises(TypeError, match="instantiated with no arguments"): + char.delegate = _NeedsArgs + + +def test_delegate_sync_override_fails_assignment() -> None: + """An overridden method that is not async fails, naming the method.""" + char = get_char(PROPERTIES) + + with pytest.raises( + TypeError, match="on_get of a characteristic is not a coroutine" + ): + char.delegate = _SyncGetter() + + +def test_overridden_hooks_are_recorded() -> None: + """Wiring records which methods the delegate overrides.""" + char = get_char(PROPERTIES) + char.delegate = _AsyncGetter() + + assert char._delegate_hooks == frozenset({"on_get"}) + + char.delegate = CharacteristicDelegate() + + assert char._delegate_hooks == frozenset() + + +def test_legacy_and_delegate_coexist() -> None: + """Assigning a delegate does not remove a legacy callback, or the reverse.""" + char = get_char(PROPERTIES) + setter = lambda value: None # noqa: E731 + char.setter_callback = setter + + char.delegate = _AsyncGetter() + + assert char.setter_callback is setter + assert isinstance(char.delegate, _AsyncGetter) + + getter = lambda: 1 # noqa: E731 + char.getter_callback = getter + + assert char.getter_callback is getter + + +def test_legacy_callback_removal() -> None: + """None or del removes a legacy callback, like before the properties.""" + char = get_char(PROPERTIES) + setter = lambda value: None # noqa: E731 + char.setter_callback = setter + + char.setter_callback = None + + assert char.setter_callback is None + + char.setter_callback = setter + del char.setter_callback + + assert char.setter_callback is None diff --git a/tests/test_service.py b/tests/test_service.py index 1b3b44e2..f6f5a15b 100644 --- a/tests/test_service.py +++ b/tests/test_service.py @@ -12,6 +12,7 @@ PROP_PERMISSIONS, Characteristic, ) +from pyhap.delegates import CharacteristicDelegate, ServiceDelegate from pyhap.service import Service CHAR_PROPS = { @@ -246,3 +247,61 @@ def test_from_dict() -> None: mock_char_loader.get_char.assert_has_calls( [call("Char 1"), call("Char 2")], any_order=True ) + + +class _AsyncGate(ServiceDelegate): + """A service delegate overriding the grouped gate.""" + + +class _AsyncCharDelegate(CharacteristicDelegate): + """A characteristic delegate.""" + + +def test_service_delegate_instance() -> None: + """An assigned service delegate is the service's delegate.""" + service = Service(uuid1()) + delegate = _AsyncGate() + + service.delegate = delegate + + assert service.delegate is delegate + + +def test_service_delegate_sync_override_fails() -> None: + """A grouped on_set that is not async fails the assignment.""" + service = Service(uuid1()) + + class _SyncGate(ServiceDelegate): + def on_set( # ty: ignore[invalid-method-override] + self, values: object, context: object + ) -> None: + pass + + with pytest.raises(TypeError, match="on_set of a service is not a coroutine"): + service.delegate = _SyncGate() + + +def test_configure_char_wires_delegate() -> None: + """configure_char wires a delegate instance or class onto the char.""" + service = Service(uuid1()) + service.add_characteristic(*get_chars()) + delegate = _AsyncCharDelegate() + + configured = service.configure_char("Char 1", delegate=delegate) + + assert configured.delegate is delegate + + configured_by_class = service.configure_char("Char 2", delegate=_AsyncCharDelegate) + + assert isinstance(configured_by_class.delegate, _AsyncCharDelegate) + + +def test_configure_char_without_delegate() -> None: + """configure_char without a delegate leaves the char without one.""" + service = Service(uuid1()) + service.add_characteristic(*get_chars()) + + configured = service.configure_char("Char 1", value=5) + + assert configured.delegate is None + assert configured.value == 5