From ca81c5098b305df45e332e3d21a128c9e7e5cc25 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Mon, 17 Mar 2025 12:22:06 +0100 Subject: [PATCH 1/7] Sort exported symbols Signed-off-by: Leandro Lucarella --- src/frequenz/client/microgrid/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/frequenz/client/microgrid/__init__.py b/src/frequenz/client/microgrid/__init__.py index 56e9cedb..46d4f4f6 100644 --- a/src/frequenz/client/microgrid/__init__.py +++ b/src/frequenz/client/microgrid/__init__.py @@ -65,7 +65,6 @@ from ._metadata import Location, Metadata __all__ = [ - "MicrogridApiClient", "ApiClientError", "BatteryComponentState", "BatteryData", @@ -100,6 +99,7 @@ "Location", "Metadata", "MeterData", + "MicrogridApiClient", "OperationAborted", "OperationCancelled", "OperationNotImplemented", From ec1b4726fd6d56fd78ae5149e56e1ef6c9fee221 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Mon, 17 Mar 2025 12:22:48 +0100 Subject: [PATCH 2/7] Use `Iterator` from `collections.abs` The symbol in `typing` is deprecated. Signed-off-by: Leandro Lucarella --- tests/test_metadata.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_metadata.py b/tests/test_metadata.py index acc01e20..55f93a1d 100644 --- a/tests/test_metadata.py +++ b/tests/test_metadata.py @@ -3,7 +3,7 @@ """Tests for the microgrid metadata types.""" -from typing import Iterator +from collections.abc import Iterator from unittest.mock import MagicMock, patch from zoneinfo import ZoneInfo From 51128dd5f12a612bc8e338aa318fad716a025712 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Mon, 17 Mar 2025 13:42:45 +0100 Subject: [PATCH 3/7] tests: Import from public modules when possible Signed-off-by: Leandro Lucarella --- tests/test_client.py | 2 +- tests/test_component.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/test_client.py b/tests/test_client.py index 86090fe7..33d7affe 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -21,6 +21,7 @@ Component, ComponentCategory, ComponentData, + Connection, EVChargerData, Fuse, GridMetadata, @@ -29,7 +30,6 @@ MeterData, MicrogridApiClient, ) -from frequenz.client.microgrid._connection import Connection class _TestClient(MicrogridApiClient): diff --git a/tests/test_component.py b/tests/test_component.py index d7ed3d40..85f7f519 100644 --- a/tests/test_component.py +++ b/tests/test_component.py @@ -6,11 +6,11 @@ import pytest from frequenz.api.common import components_pb2 -from frequenz.client.microgrid._component import ( +from frequenz.client.microgrid import ( Component, ComponentCategory, - component_category_from_protobuf, ) +from frequenz.client.microgrid._component import component_category_from_protobuf def test_component_category_from_protobuf() -> None: From 88ad8dcabaa47ea65bac4a87d4e67b4fce60ccf7 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Mon, 17 Mar 2025 13:00:30 +0100 Subject: [PATCH 4/7] Add strongly typed IDs for microgrids and components Introduce `MicrogridId` and `ComponentId` classes to replace plain integer IDs. These classes provide type safety and prevent accidental errors by: - Making it impossible to mix up microgrid and component IDs (equality comparisons between different ID types always return false). - Preventing accidental math operations on IDs. - Providing clear string representations for debugging (MID42, CID42). - Ensuring proper hash behavior in collections. Signed-off-by: Leandro Lucarella --- src/frequenz/client/microgrid/_id.py | 102 +++++++++++++++++++++++++++ tests/test_id.py | 91 ++++++++++++++++++++++++ 2 files changed, 193 insertions(+) create mode 100644 src/frequenz/client/microgrid/_id.py create mode 100644 tests/test_id.py diff --git a/src/frequenz/client/microgrid/_id.py b/src/frequenz/client/microgrid/_id.py new file mode 100644 index 00000000..411b532d --- /dev/null +++ b/src/frequenz/client/microgrid/_id.py @@ -0,0 +1,102 @@ +# License: MIT +# Copyright © 2025 Frequenz Energy-as-a-Service GmbH + +"""Strongly typed IDs for microgrids and components.""" + + +class MicrogridId: + """A unique identifier for a microgrid.""" + + def __init__(self, id_: int, /) -> None: + """Initialize this instance. + + Args: + id_: The numeric unique identifier of the microgrid. + + Raises: + ValueError: If the ID is negative. + """ + if id_ < 0: + raise ValueError("Microgrid ID can't be negative.") + self._id = id_ + + def __int__(self) -> int: + """Return the numeric ID of this instance.""" + return self._id + + def __eq__(self, other: object) -> bool: + """Check if this instance is equal to another object.""" + # This is not an unidiomatic typecheck, that's an odd name for the check. + # isinstance() returns True for subclasses, which is not what we want here. + # pylint: disable-next=unidiomatic-typecheck + return type(other) is MicrogridId and self._id == other._id + + def __lt__(self, other: object) -> bool: + """Check if this instance is less than another object.""" + # pylint: disable-next=unidiomatic-typecheck + if type(other) is MicrogridId: + return self._id < other._id + return NotImplemented + + def __hash__(self) -> int: + """Return the hash of this instance.""" + # We include the class because we explicitly want to avoid the same ID to give + # the same hash for different classes of IDs + return hash((MicrogridId, self._id)) + + def __repr__(self) -> str: + """Return the string representation of this instance.""" + return f"{type(self).__name__}({self._id!r})" + + def __str__(self) -> str: + """Return the short string representation of this instance.""" + return f"MID{self._id}" + + +class ComponentId: + """A unique identifier for a microgrid component.""" + + def __init__(self, id_: int, /) -> None: + """Initialize this instance. + + Args: + id_: The numeric unique identifier of the microgrid component. + + Raises: + ValueError: If the ID is negative. + """ + if id_ < 0: + raise ValueError("Component ID can't be negative.") + self._id = id_ + + def __int__(self) -> int: + """Return the numeric ID of this instance.""" + return self._id + + def __eq__(self, other: object) -> bool: + """Check if this instance is equal to another object.""" + # This is not an unidiomatic typecheck, that's an odd name for the check. + # isinstance() returns True for subclasses, which is not what we want here. + # pylint: disable-next=unidiomatic-typecheck + return type(other) is ComponentId and self._id == other._id + + def __lt__(self, other: object) -> bool: + """Check if this instance is less than another object.""" + # pylint: disable-next=unidiomatic-typecheck + if type(other) is ComponentId: + return self._id < other._id + return NotImplemented + + def __hash__(self) -> int: + """Return the hash of this instance.""" + # We include the class because we explicitly want to avoid the same ID to give + # the same hash for different classes of IDs + return hash((ComponentId, self._id)) + + def __repr__(self) -> str: + """Return the string representation of this instance.""" + return f"{type(self).__name__}({self._id!r})" + + def __str__(self) -> str: + """Return the short string representation of this instance.""" + return f"CID{self._id}" diff --git a/tests/test_id.py b/tests/test_id.py new file mode 100644 index 00000000..ff5476ab --- /dev/null +++ b/tests/test_id.py @@ -0,0 +1,91 @@ +# License: MIT +# Copyright © 2025 Frequenz Energy-as-a-Service GmbH + +"""Tests for the microgrid and component IDs.""" + +from dataclasses import dataclass + +import pytest + +from frequenz.client.microgrid import ComponentId, MicrogridId + + +@dataclass(frozen=True) +class IdTypeInfo: + """Information about an ID type for testing.""" + + id_class: type + str_prefix: str + error_prefix: str + + +# Define all ID types to test here +ID_TYPES: list[IdTypeInfo] = [ + IdTypeInfo(MicrogridId, "MID", "Microgrid"), + IdTypeInfo(ComponentId, "CID", "Component"), +] + + +@pytest.mark.parametrize( + "type_info", + ID_TYPES, + ids=lambda type_info: type_info.id_class.__name__, +) +class TestIds: + """Tests for ID classes.""" + + def test_valid_id(self, type_info: IdTypeInfo) -> None: + """Test creating a valid ID.""" + id_obj = type_info.id_class(42) + assert int(id_obj) == 42 + + def test_negative_id_raises(self, type_info: IdTypeInfo) -> None: + """Test that creating a negative ID raises ValueError.""" + error_msg = f"{type_info.error_prefix} ID can't be negative" + with pytest.raises(ValueError, match=error_msg): + type_info.id_class(-1) + + def test_equality(self, type_info: IdTypeInfo) -> None: + """Test equality comparison.""" + assert type_info.id_class(1) == type_info.id_class(1) + assert type_info.id_class(1) != type_info.id_class(2) + + # Test against all other types + for other_type in ID_TYPES: + if other_type != type_info: + assert type_info.id_class(1) != other_type.id_class(1) + + def test_ordering(self, type_info: IdTypeInfo) -> None: + """Test ordering comparison.""" + assert type_info.id_class(1) < type_info.id_class(2) + assert not type_info.id_class(2) < type_info.id_class(1) + + # Test against all other types + for other_type in ID_TYPES: + if other_type != type_info: + with pytest.raises(TypeError): + _ = type_info.id_class(1) < other_type.id_class(2) + + def test_hash(self, type_info: IdTypeInfo) -> None: + """Test hash behavior.""" + # Same IDs should hash to same value + assert hash(type_info.id_class(1)) == hash(type_info.id_class(1)) + # Different IDs should hash to different values + assert hash(type_info.id_class(1)) != hash(type_info.id_class(2)) + + # Test against all other types + for other_type in ID_TYPES: + if other_type != type_info: + # Same ID but different types should hash to different values + assert hash(type_info.id_class(1)) != hash(other_type.id_class(1)) + + def test_str_and_repr(self, type_info: IdTypeInfo) -> None: + """Test string representations.""" + id_obj = type_info.id_class(42) + assert str(id_obj) == f"{type_info.str_prefix}42" + assert repr(id_obj) == f"{type_info.id_class.__name__}(42)" + + def test_invalid_creation(self, type_info: IdTypeInfo) -> None: + """Test that creating an ID with a non-integer raises TypeError.""" + with pytest.raises(TypeError): + type_info.id_class("not-an-int") From cab70f99cb45ea2b6f54ca96e0bf4caa728e7abe Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Mon, 17 Mar 2025 12:19:06 +0100 Subject: [PATCH 5/7] Use the new ID types throughout the client Now the client uses the new strongly typed IDs for components and microgrids. Signed-off-by: Leandro Lucarella --- src/frequenz/client/microgrid/__init__.py | 3 + src/frequenz/client/microgrid/_client.py | 59 ++++--- src/frequenz/client/microgrid/_component.py | 9 +- .../client/microgrid/_component_data.py | 11 +- src/frequenz/client/microgrid/_connection.py | 8 +- src/frequenz/client/microgrid/_metadata.py | 4 +- tests/test_client.py | 167 ++++++++++-------- tests/test_component.py | 22 +-- tests/test_component_data.py | 5 +- tests/test_connection.py | 14 +- 10 files changed, 170 insertions(+), 132 deletions(-) diff --git a/src/frequenz/client/microgrid/__init__.py b/src/frequenz/client/microgrid/__init__.py index 46d4f4f6..c88e2244 100644 --- a/src/frequenz/client/microgrid/__init__.py +++ b/src/frequenz/client/microgrid/__init__.py @@ -62,6 +62,7 @@ UnknownError, UnrecognizedGrpcStatus, ) +from ._id import ComponentId, MicrogridId from ._metadata import Location, Metadata __all__ = [ @@ -75,6 +76,7 @@ "Component", "ComponentCategory", "ComponentData", + "ComponentId", "ComponentMetadata", "ComponentMetricId", "ComponentType", @@ -100,6 +102,7 @@ "Metadata", "MeterData", "MicrogridApiClient", + "MicrogridId", "OperationAborted", "OperationCancelled", "OperationNotImplemented", diff --git a/src/frequenz/client/microgrid/_client.py b/src/frequenz/client/microgrid/_client.py index f6dd4973..9aa73547 100644 --- a/src/frequenz/client/microgrid/_client.py +++ b/src/frequenz/client/microgrid/_client.py @@ -34,6 +34,7 @@ from ._connection import Connection from ._constants import RECEIVER_MAX_SIZE from ._exception import ApiClientError, ClientNotConnected +from ._id import ComponentId, MicrogridId from ._metadata import Location, Metadata DEFAULT_GRPC_CALL_TIMEOUT = 60.0 @@ -91,7 +92,9 @@ def __init__( connect=connect, channel_defaults=channel_defaults, ) - self._broadcasters: dict[int, streaming.GrpcStreamBroadcaster[Any, Any]] = {} + self._broadcasters: dict[ + ComponentId, streaming.GrpcStreamBroadcaster[Any, Any] + ] = {} self._retry_strategy = retry_strategy @property @@ -134,7 +137,7 @@ async def components( # noqa: DOC502 (raises ApiClientError indirectly) ) result: Iterable[Component] = map( lambda c: Component( - c.id, + ComponentId(c.id), component_category_from_protobuf(c.category), component_type_from_protobuf(c.category, c.inverter), component_metadata_from_protobuf(c.category, c.grid), @@ -176,12 +179,14 @@ async def metadata(self) -> Metadata: longitude=microgrid_metadata.location.longitude, ) - return Metadata(microgrid_id=microgrid_metadata.microgrid_id, location=location) + return Metadata( + microgrid_id=MicrogridId(microgrid_metadata.microgrid_id), location=location + ) async def connections( # noqa: DOC502 (raises ApiClientError indirectly) self, - starts: Set[int] = frozenset(), - ends: Set[int] = frozenset(), + starts: Set[ComponentId] = frozenset(), + ends: Set[ComponentId] = frozenset(), ) -> Iterable[Connection]: """Fetch the connections between components in the microgrid. @@ -199,7 +204,13 @@ async def connections( # noqa: DOC502 (raises ApiClientError indirectly) most likely a subclass of [GrpcError][frequenz.client.microgrid.GrpcError]. """ - connection_filter = microgrid_pb2.ConnectionFilter(starts=starts, ends=ends) + # Convert ComponentId to raw int for the API call + start_ids = {int(start) for start in starts} + end_ids = {int(end) for end in ends} + + connection_filter = microgrid_pb2.ConnectionFilter( + starts=start_ids, ends=end_ids + ) valid_components, all_connections = await asyncio.gather( self.components(), client.call_stub_method( @@ -214,7 +225,7 @@ async def connections( # noqa: DOC502 (raises ApiClientError indirectly) # Filter out the components filtered in `components` method. # id=0 is an exception indicating grid component. - valid_ids = {c.component_id for c in valid_components} + valid_ids = {int(c.component_id) for c in valid_components} valid_ids.add(0) connections = filter( @@ -223,7 +234,7 @@ async def connections( # noqa: DOC502 (raises ApiClientError indirectly) ) result: Iterable[Connection] = map( - lambda c: Connection(c.start, c.end), connections + lambda c: Connection(ComponentId(c.start), ComponentId(c.end)), connections ) return result @@ -231,7 +242,7 @@ async def connections( # noqa: DOC502 (raises ApiClientError indirectly) async def _new_component_data_receiver( self, *, - component_id: int, + component_id: ComponentId, expected_category: ComponentCategory, transform: Callable[[microgrid_pb2.ComponentData], _ComponentDataT], maxsize: int, @@ -262,7 +273,7 @@ async def _new_component_data_receiver( f"raw-component-data-{component_id}", lambda: aiter( self.stub.StreamComponentData( - microgrid_pb2.ComponentIdParam(id=component_id) + microgrid_pb2.ComponentIdParam(id=int(component_id)) ) ), transform, @@ -276,7 +287,7 @@ async def _new_component_data_receiver( async def _expect_category( self, - component_id: int, + component_id: ComponentId, expected_category: ComponentCategory, ) -> None: """Check if the given component_id is of the expected type. @@ -296,19 +307,17 @@ async def _expect_category( if comp.component_id == component_id ) except StopIteration as exc: - raise ValueError( - f"Unable to find component with id {component_id}" - ) from exc + raise ValueError(f"Unable to find {component_id}") from exc if comp.category != expected_category: raise ValueError( - f"Component id {component_id} is a {comp.category.name.lower()}" + f"{component_id} is a {comp.category.name.lower()}" f", not a {expected_category.name.lower()}." ) async def meter_data( # noqa: DOC502 (ValueError is raised indirectly by _expect_category) self, - component_id: int, + component_id: ComponentId, maxsize: int = RECEIVER_MAX_SIZE, ) -> Receiver[MeterData]: """Return a channel receiver that provides a `MeterData` stream. @@ -332,7 +341,7 @@ async def meter_data( # noqa: DOC502 (ValueError is raised indirectly by _expec async def battery_data( # noqa: DOC502 (ValueError is raised indirectly by _expect_category) self, - component_id: int, + component_id: ComponentId, maxsize: int = RECEIVER_MAX_SIZE, ) -> Receiver[BatteryData]: """Return a channel receiver that provides a `BatteryData` stream. @@ -356,7 +365,7 @@ async def battery_data( # noqa: DOC502 (ValueError is raised indirectly by _exp async def inverter_data( # noqa: DOC502 (ValueError is raised indirectly by _expect_category) self, - component_id: int, + component_id: ComponentId, maxsize: int = RECEIVER_MAX_SIZE, ) -> Receiver[InverterData]: """Return a channel receiver that provides an `InverterData` stream. @@ -380,7 +389,7 @@ async def inverter_data( # noqa: DOC502 (ValueError is raised indirectly by _ex async def ev_charger_data( # noqa: DOC502 (ValueError is raised indirectly by _expect_category) self, - component_id: int, + component_id: ComponentId, maxsize: int = RECEIVER_MAX_SIZE, ) -> Receiver[EVChargerData]: """Return a channel receiver that provides an `EvChargeData` stream. @@ -403,7 +412,7 @@ async def ev_charger_data( # noqa: DOC502 (ValueError is raised indirectly by _ ) async def set_power( # noqa: DOC502 (raises ApiClientError indirectly) - self, component_id: int, power_w: float + self, component_id: ComponentId, power_w: float ) -> None: """Send request to the Microgrid to set power for component. @@ -425,7 +434,7 @@ async def set_power( # noqa: DOC502 (raises ApiClientError indirectly) self, lambda: self.stub.SetPowerActive( microgrid_pb2.SetPowerActiveParam( - component_id=component_id, power=power_w + component_id=int(component_id), power=power_w ), timeout=int(DEFAULT_GRPC_CALL_TIMEOUT), ), @@ -433,7 +442,7 @@ async def set_power( # noqa: DOC502 (raises ApiClientError indirectly) ) async def set_reactive_power( # noqa: DOC502 (raises ApiClientError indirectly) - self, component_id: int, reactive_power_var: float + self, component_id: ComponentId, reactive_power_var: float ) -> None: """Send request to the Microgrid to set reactive power for component. @@ -453,7 +462,7 @@ async def set_reactive_power( # noqa: DOC502 (raises ApiClientError indirectly) self, lambda: self.stub.SetPowerReactive( microgrid_pb2.SetPowerReactiveParam( - component_id=component_id, power=reactive_power_var + component_id=int(component_id), power=reactive_power_var ), timeout=int(DEFAULT_GRPC_CALL_TIMEOUT), ), @@ -462,7 +471,7 @@ async def set_reactive_power( # noqa: DOC502 (raises ApiClientError indirectly) async def set_bounds( # noqa: DOC503 (raises ApiClientError indirectly) self, - component_id: int, + component_id: ComponentId, lower: float, upper: float, ) -> None: @@ -492,7 +501,7 @@ async def set_bounds( # noqa: DOC503 (raises ApiClientError indirectly) self, lambda: self.stub.AddInclusionBounds( microgrid_pb2.SetBoundsParam( - component_id=component_id, + component_id=int(component_id), target_metric=target_metric, bounds=metrics_pb2.Bounds(lower=lower, upper=upper), ), diff --git a/src/frequenz/client/microgrid/_component.py b/src/frequenz/client/microgrid/_component.py index 9e1feca8..32b6bce9 100644 --- a/src/frequenz/client/microgrid/_component.py +++ b/src/frequenz/client/microgrid/_component.py @@ -9,6 +9,8 @@ from frequenz.api.common import components_pb2 from frequenz.api.microgrid import grid_pb2, inverter_pb2 +from ._id import ComponentId + class ComponentType(Enum): """A base class from which individual component types are derived.""" @@ -160,7 +162,7 @@ def component_metadata_from_protobuf( class Component: """Metadata for a single microgrid component.""" - component_id: int + component_id: ComponentId """The ID of this component.""" category: ComponentCategory @@ -180,8 +182,9 @@ def is_valid(self) -> bool: == 0` and `type` is `GRID`, `False` otherwise """ return ( - self.component_id > 0 and any(t == self.category for t in ComponentCategory) - ) or (self.component_id == 0 and self.category == ComponentCategory.GRID) + int(self.component_id) > 0 + and any(t == self.category for t in ComponentCategory) + ) or (int(self.component_id) == 0 and self.category == ComponentCategory.GRID) def __hash__(self) -> int: """Compute a hash of this instance, obtained by hashing the `component_id` field. diff --git a/src/frequenz/client/microgrid/_component_data.py b/src/frequenz/client/microgrid/_component_data.py index bd79b236..0784ba53 100644 --- a/src/frequenz/client/microgrid/_component_data.py +++ b/src/frequenz/client/microgrid/_component_data.py @@ -18,13 +18,14 @@ EVChargerComponentState, InverterComponentState, ) +from ._id import ComponentId @dataclass(frozen=True) class ComponentData(ABC): """A private base class for strongly typed component data classes.""" - component_id: int + component_id: ComponentId """The ID identifying this component in the microgrid.""" timestamp: datetime @@ -129,7 +130,7 @@ def from_proto(cls, raw: microgrid_pb2.ComponentData) -> Self: Instance of MeterData created from the protobuf message. """ meter_data = cls( - component_id=raw.id, + component_id=ComponentId(raw.id), timestamp=raw.ts.ToDatetime(tzinfo=timezone.utc), active_power=raw.meter.data.ac.power_active.value, active_power_per_phase=( @@ -247,7 +248,7 @@ def from_proto(cls, raw: microgrid_pb2.ComponentData) -> Self: """ raw_power = raw.battery.data.dc.power battery_data = cls( - component_id=raw.id, + component_id=ComponentId(raw.id), timestamp=raw.ts.ToDatetime(tzinfo=timezone.utc), soc=raw.battery.data.soc.avg, soc_lower_bound=raw.battery.data.soc.system_inclusion_bounds.lower, @@ -385,7 +386,7 @@ def from_proto(cls, raw: microgrid_pb2.ComponentData) -> Self: """ raw_power = raw.inverter.data.ac.power_active inverter_data = cls( - component_id=raw.id, + component_id=ComponentId(raw.id), timestamp=raw.ts.ToDatetime(tzinfo=timezone.utc), active_power=raw.inverter.data.ac.power_active.value, active_power_per_phase=( @@ -541,7 +542,7 @@ def from_proto(cls, raw: microgrid_pb2.ComponentData) -> Self: """ raw_power = raw.ev_charger.data.ac.power_active ev_charger_data = cls( - component_id=raw.id, + component_id=ComponentId(raw.id), timestamp=raw.ts.ToDatetime(tzinfo=timezone.utc), active_power=raw_power.value, active_power_per_phase=( diff --git a/src/frequenz/client/microgrid/_connection.py b/src/frequenz/client/microgrid/_connection.py index fb077d96..afcd2b4e 100644 --- a/src/frequenz/client/microgrid/_connection.py +++ b/src/frequenz/client/microgrid/_connection.py @@ -6,15 +6,17 @@ from dataclasses import dataclass +from ._id import ComponentId + @dataclass(frozen=True) class Connection: """Metadata for a connection between microgrid components.""" - start: int + start: ComponentId """The component ID that represents the start component of the connection.""" - end: int + end: ComponentId """The component ID that represents the end component of the connection.""" def is_valid(self) -> bool: @@ -24,4 +26,4 @@ def is_valid(self) -> bool: `True` if `start >= 0`, `end > 0`, and `start != end`, `False` otherwise. """ - return self.start >= 0 and self.end > 0 and self.start != self.end + return int(self.start) >= 0 and int(self.end) > 0 and self.start != self.end diff --git a/src/frequenz/client/microgrid/_metadata.py b/src/frequenz/client/microgrid/_metadata.py index 040bcdb5..b0b7454b 100644 --- a/src/frequenz/client/microgrid/_metadata.py +++ b/src/frequenz/client/microgrid/_metadata.py @@ -8,6 +8,8 @@ from timezonefinder import TimezoneFinder +from ._id import MicrogridId + _timezone_finder = TimezoneFinder() @@ -45,7 +47,7 @@ def __post_init__(self) -> None: class Metadata: """Metadata for the microgrid.""" - microgrid_id: int | None = None + microgrid_id: MicrogridId | None = None """The ID of the microgrid.""" location: Location | None = None diff --git a/tests/test_client.py b/tests/test_client.py index 33d7affe..a6a3e960 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -21,6 +21,7 @@ Component, ComponentCategory, ComponentData, + ComponentId, Connection, EVChargerData, Fuse, @@ -62,7 +63,9 @@ async def test_components() -> None: id=0, category=components_pb2.ComponentCategory.COMPONENT_CATEGORY_METER ) ) - assert set(await client.components()) == {Component(0, ComponentCategory.METER)} + assert set(await client.components()) == { + Component(ComponentId(0), ComponentCategory.METER) + } server_response.components.append( microgrid_pb2.Component( @@ -70,8 +73,8 @@ async def test_components() -> None: ) ) assert set(await client.components()) == { - Component(0, ComponentCategory.METER), - Component(0, ComponentCategory.BATTERY), + Component(ComponentId(0), ComponentCategory.METER), + Component(ComponentId(0), ComponentCategory.BATTERY), } server_response.components.append( @@ -80,9 +83,9 @@ async def test_components() -> None: ) ) assert set(await client.components()) == { - Component(0, ComponentCategory.METER), - Component(0, ComponentCategory.BATTERY), - Component(0, ComponentCategory.METER), + Component(ComponentId(0), ComponentCategory.METER), + Component(ComponentId(0), ComponentCategory.BATTERY), + Component(ComponentId(0), ComponentCategory.METER), } # sensors are not counted as components by the API client @@ -92,9 +95,9 @@ async def test_components() -> None: ) ) assert set(await client.components()) == { - Component(0, ComponentCategory.METER), - Component(0, ComponentCategory.BATTERY), - Component(0, ComponentCategory.METER), + Component(ComponentId(0), ComponentCategory.METER), + Component(ComponentId(0), ComponentCategory.BATTERY), + Component(ComponentId(0), ComponentCategory.METER), } _replace_components( @@ -118,9 +121,9 @@ async def test_components() -> None: ], ) assert set(await client.components()) == { - Component(9, ComponentCategory.METER), - Component(99, ComponentCategory.INVERTER, InverterType.NONE), - Component(999, ComponentCategory.BATTERY), + Component(ComponentId(9), ComponentCategory.METER), + Component(ComponentId(99), ComponentCategory.INVERTER, InverterType.NONE), + Component(ComponentId(999), ComponentCategory.BATTERY), } _replace_components( @@ -165,17 +168,17 @@ async def test_components() -> None: grid_fuse = Fuse(123.0) assert set(await client.components()) == { - Component(100, ComponentCategory.NONE), + Component(ComponentId(100), ComponentCategory.NONE), Component( - 101, + ComponentId(101), ComponentCategory.GRID, None, GridMetadata(fuse=grid_fuse), ), - Component(104, ComponentCategory.METER), - Component(105, ComponentCategory.INVERTER, InverterType.NONE), - Component(106, ComponentCategory.BATTERY), - Component(107, ComponentCategory.EV_CHARGER), + Component(ComponentId(104), ComponentCategory.METER), + Component(ComponentId(105), ComponentCategory.INVERTER, InverterType.NONE), + Component(ComponentId(106), ComponentCategory.BATTERY), + Component(ComponentId(107), ComponentCategory.EV_CHARGER), } _replace_components( @@ -203,9 +206,9 @@ async def test_components() -> None: ) assert set(await client.components()) == { - Component(9, ComponentCategory.METER), - Component(99, ComponentCategory.INVERTER, InverterType.BATTERY), - Component(999, ComponentCategory.BATTERY), + Component(ComponentId(9), ComponentCategory.METER), + Component(ComponentId(99), ComponentCategory.INVERTER, InverterType.BATTERY), + Component(ComponentId(999), ComponentCategory.BATTERY), } @@ -247,7 +250,9 @@ def assert_filter(*, starts: set[int], ends: set[int]) -> None: assert_filter(starts=set(), ends=set()) connections_response.connections.append(microgrid_pb2.Connection(start=0, end=0)) - assert set(await client.connections()) == {Connection(0, 0)} + assert set(await client.connections()) == { + Connection(ComponentId(0), ComponentId(0)) + } components_response.components.extend( [ @@ -263,15 +268,15 @@ def assert_filter(*, starts: set[int], ends: set[int]) -> None: ) connections_response.connections.append(microgrid_pb2.Connection(start=7, end=9)) assert set(await client.connections()) == { - Connection(0, 0), - Connection(7, 9), + Connection(ComponentId(0), ComponentId(0)), + Connection(ComponentId(7), ComponentId(9)), } connections_response.connections.append(microgrid_pb2.Connection(start=0, end=0)) assert set(await client.connections()) == { - Connection(0, 0), - Connection(7, 9), - Connection(0, 0), + Connection(ComponentId(0), ComponentId(0)), + Connection(ComponentId(7), ComponentId(9)), + Connection(ComponentId(0), ComponentId(0)), } _replace_connections( @@ -291,10 +296,10 @@ def assert_filter(*, starts: set[int], ends: set[int]) -> None: ) ) assert set(await client.connections()) == { - Connection(999, 9), - Connection(99, 19), - Connection(909, 101), - Connection(99, 91), + Connection(ComponentId(999), ComponentId(9)), + Connection(ComponentId(99), ComponentId(19)), + Connection(ComponentId(909), ComponentId(101)), + Connection(ComponentId(99), ComponentId(91)), } for component_id in [1, 2, 3, 4, 5, 6, 7, 8]: @@ -320,16 +325,16 @@ def assert_filter(*, starts: set[int], ends: set[int]) -> None: ], ) assert set(await client.connections()) == { - Connection(1, 2), - Connection(2, 3), - Connection(2, 4), - Connection(2, 5), - Connection(4, 3), - Connection(4, 5), - Connection(4, 6), - Connection(5, 4), - Connection(5, 7), - Connection(5, 8), + Connection(ComponentId(1), ComponentId(2)), + Connection(ComponentId(2), ComponentId(3)), + Connection(ComponentId(2), ComponentId(4)), + Connection(ComponentId(2), ComponentId(5)), + Connection(ComponentId(4), ComponentId(3)), + Connection(ComponentId(4), ComponentId(5)), + Connection(ComponentId(4), ComponentId(6)), + Connection(ComponentId(5), ComponentId(4)), + Connection(ComponentId(5), ComponentId(7)), + Connection(ComponentId(5), ComponentId(8)), } # passing empty sets is the same as passing `None`, @@ -340,25 +345,28 @@ def assert_filter(*, starts: set[int], ends: set[int]) -> None: # include filter for connection start client.mock_stub.reset_mock() - await client.connections(starts={1, 2}) + await client.connections(starts={ComponentId(1), ComponentId(2)}) assert_filter(starts={1, 2}, ends=set()) client.mock_stub.reset_mock() - await client.connections(starts={2}) + await client.connections(starts={ComponentId(2)}) assert_filter(starts={2}, ends=set()) # include filter for connection end client.mock_stub.reset_mock() - await client.connections(ends={1}) + await client.connections(ends={ComponentId(1)}) assert_filter(starts=set(), ends={1}) client.mock_stub.reset_mock() - await client.connections(ends={2, 4, 5}) + await client.connections(ends={ComponentId(2), ComponentId(4), ComponentId(5)}) assert_filter(starts=set(), ends={2, 4, 5}) # different filters combine with AND logic client.mock_stub.reset_mock() - await client.connections(starts={1, 2, 4}, ends={4, 5, 6}) + await client.connections( + starts={ComponentId(1), ComponentId(2), ComponentId(4)}, + ends={ComponentId(4), ComponentId(5), ComponentId(6)}, + ) assert_filter(starts={1, 2, 4}, ends={4, 5, 6}) @@ -431,21 +439,23 @@ async def test_data_component_not_found(method: str) -> None: client.mock_stub.ListComponents.return_value = microgrid_pb2.ComponentList() # It should raise a ValueError for a missing component_id - with pytest.raises(ValueError, match="Unable to find component with id 20"): - await getattr(client, method)(20) + with pytest.raises(ValueError, match="Unable to find CID20"): + await getattr(client, method)(ComponentId(20)) @pytest.mark.parametrize( "method, component_id", [ - ("meter_data", 38), - ("battery_data", 83), - ("inverter_data", 83), - ("ev_charger_data", 99), + ("meter_data", ComponentId(38)), + ("battery_data", ComponentId(83)), + ("inverter_data", ComponentId(83)), + ("ev_charger_data", ComponentId(99)), ], ) async def test_data_bad_category( - method: str, component_id: int, component_list: list[microgrid_pb2.Component] + method: str, + component_id: ComponentId, + component_list: list[microgrid_pb2.Component], ) -> None: """Test the meter_data() method.""" client = _TestClient() @@ -455,7 +465,7 @@ async def test_data_bad_category( # It should raise a ValueError for a wrong component category with pytest.raises( - ValueError, match=f"Component id {component_id} is a .*, not a {method[:-5]}" + ValueError, match=f"{component_id} is a .*, not a {method[:-5]}" ): await getattr(client, method)(component_id) @@ -463,15 +473,15 @@ async def test_data_bad_category( @pytest.mark.parametrize( "method, component_id, component_class", [ - ("meter_data", 83, MeterData), - ("battery_data", 38, BatteryData), - ("inverter_data", 99, InverterData), - ("ev_charger_data", 101, EVChargerData), + ("meter_data", ComponentId(83), MeterData), + ("battery_data", ComponentId(38), BatteryData), + ("inverter_data", ComponentId(99), InverterData), + ("ev_charger_data", ComponentId(101), EVChargerData), ], ) async def test_component_data( method: str, - component_id: int, + component_id: ComponentId, component_class: type[ComponentData], component_list: list[microgrid_pb2.Component], ) -> None: @@ -484,7 +494,7 @@ async def test_component_data( async def stream_data( *args: Any, **kwargs: Any # pylint: disable=unused-argument ) -> AsyncIterator[microgrid_pb2.ComponentData]: - yield microgrid_pb2.ComponentData(id=component_id) + yield microgrid_pb2.ComponentData(id=int(component_id)) client.mock_stub.StreamComponentData.side_effect = stream_data receiver = await getattr(client, method)(component_id) @@ -500,15 +510,15 @@ async def stream_data( @pytest.mark.parametrize( "method, component_id, component_class", [ - ("meter_data", 83, MeterData), - ("battery_data", 38, BatteryData), - ("inverter_data", 99, InverterData), - ("ev_charger_data", 101, EVChargerData), + ("meter_data", ComponentId(83), MeterData), + ("battery_data", ComponentId(38), BatteryData), + ("inverter_data", ComponentId(99), InverterData), + ("ev_charger_data", ComponentId(101), EVChargerData), ], ) async def test_component_data_grpc_error( method: str, - component_id: int, + component_id: ComponentId, component_class: type[ComponentData], component_list: list[microgrid_pb2.Component], caplog: pytest.LogCaptureFixture, @@ -537,7 +547,7 @@ async def stream_data( f"fake grpc details num_calls={num_calls}", "fake grpc debug_error_string", ) - yield microgrid_pb2.ComponentData(id=component_id) + yield microgrid_pb2.ComponentData(id=int(component_id)) client.mock_stub.StreamComponentData.side_effect = stream_data receiver = await getattr(client, method)(component_id) @@ -581,11 +591,12 @@ async def test_set_power_ok(power_w: float, meter83: microgrid_pb2.Component) -> components=[meter83] ) - await client.set_power(component_id=83, power_w=power_w) + component_id = ComponentId(83) + await client.set_power(component_id=component_id, power_w=power_w) client.mock_stub.SetPowerActive.assert_called_once() call_args = client.mock_stub.SetPowerActive.call_args[0] assert call_args[0] == microgrid_pb2.SetPowerActiveParam( - component_id=83, power=power_w + component_id=int(component_id), power=power_w ) @@ -605,7 +616,7 @@ async def test_set_power_grpc_error() -> None: r">: fake grpc details " r"\(fake grpc debug_error_string\)", ): - await client.set_power(component_id=83, power_w=100.0) + await client.set_power(component_id=ComponentId(83), power_w=100.0) @pytest.mark.parametrize( @@ -621,13 +632,14 @@ async def test_set_reactive_power_ok( components=[meter83] ) + component_id = ComponentId(83) await client.set_reactive_power( - component_id=83, reactive_power_var=reactive_power_var + component_id=component_id, reactive_power_var=reactive_power_var ) client.mock_stub.SetPowerReactive.assert_called_once() call_args = client.mock_stub.SetPowerReactive.call_args[0] assert call_args[0] == microgrid_pb2.SetPowerReactiveParam( - component_id=83, power=reactive_power_var + component_id=int(component_id), power=reactive_power_var ) @@ -647,7 +659,9 @@ async def test_set_reactive_power_grpc_error() -> None: r">: fake grpc details " r"\(fake grpc debug_error_string\)", ): - await client.set_reactive_power(component_id=83, reactive_power_var=100.0) + await client.set_reactive_power( + component_id=ComponentId(83), reactive_power_var=100.0 + ) @pytest.mark.parametrize( @@ -669,11 +683,12 @@ async def test_set_bounds_ok( components=[inverter99] ) - await client.set_bounds(99, bounds.lower, bounds.upper) + component_id = ComponentId(99) + await client.set_bounds(component_id, bounds.lower, bounds.upper) client.mock_stub.AddInclusionBounds.assert_called_once() call_args = client.mock_stub.AddInclusionBounds.call_args[0] assert call_args[0] == microgrid_pb2.SetBoundsParam( - component_id=99, + component_id=int(component_id), target_metric=microgrid_pb2.SetBoundsParam.TargetMetric.TARGET_METRIC_POWER_ACTIVE, bounds=bounds, ) @@ -698,7 +713,7 @@ async def test_set_bounds_fail( ) with pytest.raises(ValueError): - await client.set_bounds(99, bounds.lower, bounds.upper) + await client.set_bounds(ComponentId(99), bounds.lower, bounds.upper) client.mock_stub.AddInclusionBounds.assert_not_called() @@ -718,7 +733,7 @@ async def test_set_bounds_grpc_error() -> None: r">: fake grpc details " r"\(fake grpc debug_error_string\)", ): - await client.set_bounds(99, 0.0, 100.0) + await client.set_bounds(ComponentId(99), 0.0, 100.0) def _clear_components(component_list: microgrid_pb2.ComponentList) -> None: diff --git a/tests/test_component.py b/tests/test_component.py index 85f7f519..86e0aa1a 100644 --- a/tests/test_component.py +++ b/tests/test_component.py @@ -9,6 +9,7 @@ from frequenz.client.microgrid import ( Component, ComponentCategory, + ComponentId, ) from frequenz.client.microgrid._component import component_category_from_protobuf @@ -68,29 +69,30 @@ def test_component_category_from_protobuf() -> None: # pylint: disable=invalid-name def test_Component() -> None: """Test the component category.""" - c0 = Component(0, ComponentCategory.GRID) + c0 = Component(ComponentId(0), ComponentCategory.GRID) assert c0.is_valid() - c1 = Component(1, ComponentCategory.GRID) + c1 = Component(ComponentId(1), ComponentCategory.GRID) assert c1.is_valid() - c4 = Component(4, ComponentCategory.METER) + c4 = Component(ComponentId(4), ComponentCategory.METER) assert c4.is_valid() - c5 = Component(5, ComponentCategory.INVERTER) + c5 = Component(ComponentId(5), ComponentCategory.INVERTER) assert c5.is_valid() - c6 = Component(6, ComponentCategory.BATTERY) + c6 = Component(ComponentId(6), ComponentCategory.BATTERY) assert c6.is_valid() - c7 = Component(7, ComponentCategory.EV_CHARGER) + c7 = Component(ComponentId(7), ComponentCategory.EV_CHARGER) assert c7.is_valid() - invalid_grid_id = Component(-1, ComponentCategory.GRID) - assert not invalid_grid_id.is_valid() + with pytest.raises(ValueError): + # Should raise error with negative ID + Component(ComponentId(-1), ComponentCategory.GRID) - invalid_type = Component(666, -1) # type: ignore + invalid_type = Component(ComponentId(666), -1) # type: ignore assert not invalid_type.is_valid() - another_invalid_type = Component(666, 666) # type: ignore + another_invalid_type = Component(ComponentId(666), 666) # type: ignore assert not another_invalid_type.is_valid() diff --git a/tests/test_component_data.py b/tests/test_component_data.py index 0de14a2e..fcc5dc7a 100644 --- a/tests/test_component_data.py +++ b/tests/test_component_data.py @@ -13,6 +13,7 @@ from frequenz.client.microgrid import ( ComponentData, + ComponentId, InverterComponentState, InverterData, InverterError, @@ -23,7 +24,7 @@ def test_component_data_abstract_class() -> None: """Verify the base class ComponentData may not be instantiated.""" with pytest.raises(TypeError): # pylint: disable=abstract-class-instantiated - ComponentData(0, datetime.now(timezone.utc)) # type: ignore + ComponentData(ComponentId(0), datetime.now(timezone.utc)) # type: ignore def test_inverter_data() -> None: @@ -83,7 +84,7 @@ def test_inverter_data() -> None: ) inv_data = InverterData.from_proto(raw) - assert inv_data.component_id == 5 + assert inv_data.component_id == ComponentId(5) assert inv_data.timestamp == datetime.fromtimestamp(seconds, timezone.utc) assert inv_data.component_state is InverterComponentState.DISCHARGING assert inv_data.errors == [InverterError(message="error message")] diff --git a/tests/test_connection.py b/tests/test_connection.py index 2d8ce46a..6bc6c43e 100644 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -3,26 +3,26 @@ """Tests for the microgrid Connection type.""" -from frequenz.client.microgrid import Connection +from frequenz.client.microgrid import ComponentId, Connection # pylint: disable=invalid-name def test_Connection() -> None: """Test the microgrid Connection type.""" - c00 = Connection(0, 0) + c00 = Connection(ComponentId(0), ComponentId(0)) assert not c00.is_valid() - c01 = Connection(0, 1) + c01 = Connection(ComponentId(0), ComponentId(1)) assert c01.is_valid() - c10 = Connection(1, 0) + c10 = Connection(ComponentId(1), ComponentId(0)) assert not c10.is_valid() - c11 = Connection(1, 1) + c11 = Connection(ComponentId(1), ComponentId(1)) assert not c11.is_valid() - c12 = Connection(1, 2) + c12 = Connection(ComponentId(1), ComponentId(2)) assert c12.is_valid() - c21 = Connection(2, 1) + c21 = Connection(ComponentId(2), ComponentId(1)) assert c21.is_valid() From b38bb9b1c6b5641618d1c4559a9b0127f42007da Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Mon, 17 Mar 2025 12:24:16 +0100 Subject: [PATCH 6/7] Add metadata initialization tests Signed-off-by: Leandro Lucarella --- tests/test_metadata.py | 45 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-) diff --git a/tests/test_metadata.py b/tests/test_metadata.py index 55f93a1d..cf737044 100644 --- a/tests/test_metadata.py +++ b/tests/test_metadata.py @@ -9,7 +9,7 @@ import pytest -from frequenz.client.microgrid import Location +from frequenz.client.microgrid import Location, Metadata, MicrogridId @pytest.fixture @@ -65,3 +65,46 @@ def test_location_timezone_lookup( else: assert location.timezone == ZoneInfo(key=timezone) timezone_finder.timezone_at.assert_called_once_with(lat=52.52, lng=13.405) + + +def test_metadata_initialization() -> None: + """Test initialization of Metadata class.""" + # Test with no parameters + metadata = Metadata() + assert metadata.microgrid_id is None + assert metadata.location is None + + # Test with only microgrid_id + microgrid_id = MicrogridId(42) + metadata = Metadata(microgrid_id=microgrid_id) + assert metadata.microgrid_id == microgrid_id + assert metadata.location is None + + # Test with only location + location = Location(latitude=52.52, longitude=13.405) + metadata = Metadata(location=location) + assert metadata.microgrid_id is None + assert metadata.location == location + + # Test with both parameters + metadata = Metadata(microgrid_id=microgrid_id, location=location) + assert metadata.microgrid_id == microgrid_id + assert metadata.location == location + + +def test_metadata_microgrid_id_validation() -> None: + """Test validation of microgrid_id in Metadata class.""" + # Valid microgrid_id should work + metadata = Metadata(microgrid_id=MicrogridId(0)) + assert metadata.microgrid_id == MicrogridId(0) + + metadata = Metadata(microgrid_id=MicrogridId(42)) + assert metadata.microgrid_id == MicrogridId(42) + + # None should be accepted as a valid value + metadata = Metadata(microgrid_id=None) + assert metadata.microgrid_id is None + + # Negative IDs should raise ValueError + with pytest.raises(ValueError): + Metadata(microgrid_id=MicrogridId(-1)) From b6dbc6af8a8a127bde925491d75076f3f591bc06 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Mon, 17 Mar 2025 13:20:24 +0100 Subject: [PATCH 7/7] Update release notes Signed-off-by: Leandro Lucarella --- RELEASE_NOTES.md | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 80e5f0cb..b9819303 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -6,7 +6,16 @@ ## Upgrading - +- Now component and microgrid IDs are wrapped in new classes: `ComponentId` and `MicrogridId` respectively. + + These classes provide type safety and prevent accidental errors by: + + - Making it impossible to mix up microgrid and component IDs (equality comparisons between different ID types always return false). + - Preventing accidental math operations on IDs. + - Providing clear string representations for debugging (MID42, CID42). + - Ensuring proper hash behavior in collections. + + To migrate you just need to wrap your `int` IDs with the appropriate class: `0` -> `ComponentId(0)` / `MicrogridId(0)`. ## New Features