Improve Bounds validation and error handling

RELEASE_NOTES.md

@@ -52,6 +52,16 @@
 
     A well-formed `DeliveryArea` has a non-empty `code` and a specified (non-`UNSPECIFIED`) `code_type`. Constructing one with invalid data currently emits a `DeprecationWarning`; a future release will replace the warning with a hard `ValueError`. To opt into the upcoming behavior right now, pass `_raise_on_invalid=True` to the constructor. Prefer `delivery_area_from_proto2` to load delivery areas from the wire — malformed messages become `InvalidDeliveryArea` instances instead.
 
+* `frequenz.client.common.metrics.proto.v1alpha8.bounds_from_proto` is now deprecated; use `bounds_from_proto2` instead.
+
+    The new converter returns `Bounds | InvalidBounds` and surfaces malformed wire data at the type level rather than raising a `ValueError` when `lower > upper`. The old converter continues to work but emits a `DeprecationWarning`.
+
+* `frequenz.client.common.metrics.proto.v1alpha8.bounds_from_proto_with_issues` is now deprecated with no direct replacement.
+
+    Validity is now encoded in the return type of `bounds_from_proto2` (`Bounds | InvalidBounds`), so callers should inspect the returned type instead of collecting issue strings via a side channel. The old converter continues to work but emits a `DeprecationWarning`.
+
+* `frequenz.client.common.metrics.Bounds.__str__` now renders as `[lower,upper]` (no space after the comma) to match the compact format used by `Lifetime` and to compose cleanly with the `<invalid:...>` marker on `InvalidBounds`.
+
 ## New Features
 
 * Added 4 new electrical component classes for categories that previously collapsed into `UnrecognizedElectricalComponent`:
@@ -81,6 +91,7 @@
     * `frequenz.client.common.grid.DeliveryArea.get_code_type()`
     * `frequenz.client.common.metrics.MetricConnection.get_category()`
     * `frequenz.client.common.metrics.MetricSample.get_metric()`
+    * `frequenz.client.common.microgrid.electrical_components.ElectricalComponent.get_metric_config_bounds()`
 
 * Added new delivery-area class hierarchy:
 
@@ -92,6 +103,8 @@
 
 * Added a new `frequenz.client.common.microgrid.Lifetime` type together with the `frequenz.client.common.microgrid.proto.v1alpha8.lifetime_from_proto` conversion function.
 
+* Added `frequenz.client.common.metrics.proto.v1alpha8.bounds_from_proto2` returning `Bounds | InvalidBounds`. This is the replacement for the now-deprecated `bounds_from_proto`.
+
 * Added a new `frequenz.client.common.types.Location` type together with the `frequenz.client.common.types.proto.v1alpha8.location_from_proto` conversion function.
 
 * Added a new `frequenz.client.common.microgrid.Microgrid` type, together with the `frequenz.client.common.microgrid.proto.v1alpha8.microgrid_from_proto` conversion function.
@@ -100,6 +113,8 @@
 
     The class of a component is its identity; components don't carry category or type attributes. The only exceptions are the error-recovery classes `UnrecognizedElectricalComponent` and `MismatchedCategoryElectricalComponent` (with a raw protobuf `category` value) and `UnrecognizedBattery`, `UnrecognizedInverter` and `UnrecognizedEvCharger` (with a raw protobuf `type` value), which preserve the raw protobuf values received from the protocol version used to load them.
 
+    `ElectricalComponent.metric_config_bounds` is typed `Mapping[Metric | int, Bounds | InvalidBounds]`: malformed wire entries are preserved as `InvalidBounds` instead of being silently dropped, and entries that named a metric but carried no (or an empty) `config_bounds` submessage load as an unbounded `Bounds()` (a `Bounds` with neither bound set imposes no limit in either direction). Use `get_metric_config_bounds()` to resolve an entry to a valid `Bounds` or a clear `InvalidBoundsError`; it mimics `dict.get()`, returning an unbounded `Bounds()` (or a caller-supplied `default`) for absent metrics.
+
 * Added a new `frequenz.client.common.microgrid.Microgrid` type with a raising `is_active()` method, together with the `frequenz.client.common.microgrid.proto.v1alpha8.microgrid_from_proto` conversion function.
 
 ## Bug Fixes

src/frequenz/client/common/metrics/__init__.py

@@ -3,7 +3,7 @@
 
 """Metrics definitions."""
 
-from ._bounds import Bounds
+from ._bounds import BaseBounds, Bounds, InvalidBounds, InvalidBoundsError
 from ._metric import Metric
 from ._sample import (
     AggregatedMetricValue,
@@ -16,7 +16,10 @@
 __all__ = [
     "AggregatedMetricValue",
     "AggregationMethod",
+    "BaseBounds",
     "Bounds",
+    "InvalidBounds",
+    "InvalidBoundsError",
     "Metric",
     "MetricConnection",
     "MetricConnectionCategory",

src/frequenz/client/common/metrics/_bounds.py

@@ -5,29 +5,54 @@
 """Definitions for bounds."""
 
 import dataclasses
+from typing import Any, Self
 
+from .._exception import InvalidAttributeError
 
-@dataclasses.dataclass(frozen=True, kw_only=True)
-class Bounds:
-    """A set of lower and upper bounds for any metric.
 
-    The lower bound must be less than or equal to the upper bound.
+@dataclasses.dataclass(frozen=True, kw_only=True)
+class BaseBounds:
+    """A base class for well-formed and malformed metric bounds.
 
-    The units of the bounds are always the same as the related metric.
+    This class cannot be instantiated directly. Use [`Bounds`][..Bounds] for a
+    valid pair of bounds or [`InvalidBounds`][..InvalidBounds] to preserve
+    malformed wire data.
     """
 
-    lower: float | None = None
+    lower: float | int | None = None
     """The lower bound.
 
     If `None`, there is no lower bound.
     """
 
-    upper: float | None = None
+    upper: float | int | None = None
     """The upper bound.
 
     If `None`, there is no upper bound.
     """
 
+    # pylint: disable-next=unused-argument
+    def __new__(cls, *args: Any, **kwargs: Any) -> Self:
+        """Prevent instantiation of this class."""
+        if cls is BaseBounds:
+            raise TypeError(f"Cannot instantiate {cls.__name__} directly")
+        return super().__new__(cls)
+
+
+@dataclasses.dataclass(frozen=True, kw_only=True)
+class Bounds(BaseBounds):
+    """A set of lower and upper bounds for any metric.
+
+    The lower bound must be less than or equal to the upper bound.
+
+    The units of the bounds are always the same as the related metric.
+
+    Note:
+        Raises a `ValueError` if [`lower`][.lower] is greater than
+        [`upper`][.upper]. Use [`InvalidBounds`][..InvalidBounds] to
+        represent malformed bounds data received from the wire.
+    """
+
     def __post_init__(self) -> None:
         """Validate these bounds."""
         if self.lower is None:
@@ -42,4 +67,59 @@ def __post_init__(self) -> None:
 
     def __str__(self) -> str:
         """Return a string representation of these bounds."""
-        return f"[{self.lower}, {self.upper}]"
+        return f"[{self.lower},{self.upper}]"
+
+
+@dataclasses.dataclass(frozen=True, kw_only=True)
+class InvalidBounds(BaseBounds):
+    """Metric bounds with malformed data received from the wire.
+
+    This class preserves bounds data that fails the invariants required for
+    a well-formed [`Bounds`][..Bounds], allowing callers to inspect the raw
+    values without accidentally using them for range checks. Use a semantic
+    accessor, such as `ElectricalComponent.get_metric_config_bounds()`, to
+    receive a clear error on invalid data.
+    """
+
+    def __str__(self) -> str:
+        """Return a compact string representation of these invalid bounds."""
+        return f"<invalid:[{self.lower},{self.upper}]>"
+
+
+class InvalidBoundsError(InvalidAttributeError):
+    """Raised when a semantic accessor sees invalid metric bounds.
+
+    The offending [`InvalidBounds`][..InvalidBounds] is available as the
+    [`bounds`][.bounds] attribute so callers can inspect the raw wire data.
+
+    This is also a [`ValueError`][] for convenience.
+    """
+
+    def __init__(
+        self,
+        instance: object,
+        attr_name: str,
+        bounds: InvalidBounds,
+        message: str | None = None,
+    ) -> None:
+        """Initialize this error.
+
+        Args:
+            instance: The instance that was being accessed when this error was raised.
+            attr_name: The name of the attribute that was being accessed.
+            bounds: The invalid bounds instance.
+            message: A custom error message. If `None`, a default message mentioning
+                the invalid bounds is used.
+        """
+        self.bounds: InvalidBounds = bounds
+        """The invalid bounds that caused this error."""
+
+        super().__init__(
+            instance,
+            attr_name,
+            (
+                message
+                if message is not None
+                else f"invalid bounds {bounds!r} for attribute {attr_name!r} in {instance}"
+            ),
+        )

src/frequenz/client/common/metrics/proto/v1alpha8/__init__.py

@@ -3,7 +3,11 @@
 
 """Conversion of metrics enums from/to protobuf v1alpha8."""
 
-from ._bounds import bounds_from_proto, bounds_from_proto_with_issues
+from ._bounds import (
+    bounds_from_proto,
+    bounds_from_proto2,
+    bounds_from_proto_with_issues,
+)
 from ._metric import metric_from_proto, metric_to_proto
 from ._metric_connection_category import (
     metric_connection_category_from_proto,
@@ -18,6 +22,7 @@
 __all__ = [
     "aggregated_metric_sample_from_proto",
     "bounds_from_proto",
+    "bounds_from_proto2",
     "bounds_from_proto_with_issues",
     "metric_connection_category_from_proto",
     "metric_connection_category_to_proto",

src/frequenz/client/common/metrics/proto/v1alpha8/_bounds.py

@@ -4,13 +4,24 @@
 """Loading of Bounds objects from protobuf messages."""
 
 from frequenz.api.common.v1alpha8.metrics import bounds_pb2
+from typing_extensions import deprecated
 
-from ..._bounds import Bounds
+from ..._bounds import Bounds, InvalidBounds
 
 
+@deprecated(
+    "`bounds_from_proto` is deprecated; use "
+    "`bounds_from_proto2` (returns `Bounds | InvalidBounds`) instead."
+)
 def bounds_from_proto(message: bounds_pb2.Bounds) -> Bounds:  # noqa: DOC502
     """Create a [`Bounds`][....Bounds] object from a protobuf message.
 
+    Warning: Deprecated
+        Use [`bounds_from_proto2`][..bounds_from_proto2] instead. The new
+        converter distinguishes well-formed from malformed data at the
+        type level (`Bounds | InvalidBounds`) rather than raising a
+        `ValueError` when the invariant fires.
+
     Args:
         message: The protobuf message to convert.
 
@@ -26,6 +37,34 @@ def bounds_from_proto(message: bounds_pb2.Bounds) -> Bounds:  # noqa: DOC502
     )
 
 
+def bounds_from_proto2(
+    message: bounds_pb2.Bounds,
+) -> Bounds | InvalidBounds:
+    """Create bounds from a protobuf message, preserving malformed data.
+
+    Args:
+        message: The protobuf message to convert.
+
+    Returns:
+        A [`Bounds`][....Bounds] when the values form a valid range, or an
+            [`InvalidBounds`][....InvalidBounds] preserving values that
+            violate `lower <= upper`. A present but empty protobuf message
+            becomes an unbounded `Bounds()`.
+    """
+    lower = message.lower if message.HasField("lower") else None
+    upper = message.upper if message.HasField("upper") else None
+    try:
+        return Bounds(lower=lower, upper=upper)
+    except ValueError:
+        pass
+    return InvalidBounds(lower=lower, upper=upper)
+
+
+@deprecated(
+    "`bounds_from_proto_with_issues` is deprecated; use "
+    "`bounds_from_proto2` (returns `Bounds | InvalidBounds`) and inspect "
+    "the returned type instead."
+)
 def bounds_from_proto_with_issues(
     message: bounds_pb2.Bounds,
     *,
@@ -34,6 +73,13 @@ def bounds_from_proto_with_issues(
 ) -> Bounds | None:  # noqa: DOC502
     """Create a [`Bounds`][....Bounds] object from a protobuf message, collecting issues.
 
+    Warning: Deprecated
+        Use [`bounds_from_proto2`][..bounds_from_proto2] instead and
+        inspect the returned type. The new converter distinguishes
+        well-formed from malformed data at the type level
+        (`Bounds | InvalidBounds`) rather than routing invalid data
+        through a side-channel string list.
+
     Args:
         message: The protobuf message to convert.
         major_issues: A list to append major issues to.
@@ -43,7 +89,10 @@ def bounds_from_proto_with_issues(
         The corresponding [`Bounds`][....Bounds] object.
     """
     try:
-        return bounds_from_proto(message)
+        return Bounds(
+            lower=message.lower if message.HasField("lower") else None,
+            upper=message.upper if message.HasField("upper") else None,
+        )
     except ValueError as exc:
         major_issues.append(str(exc))
         return None

src/frequenz/client/common/metrics/proto/v1alpha8/_sample.py

@@ -4,19 +4,20 @@
 """Loading of MetricSample and AggregatedMetricValue objects from protobuf messages."""
 
 from collections.abc import Sequence
+from typing import assert_never
 
 from frequenz.api.common.v1alpha8.metrics import bounds_pb2, metrics_pb2
 
 from ....proto import datetime_from_proto
-from ..._bounds import Bounds
+from ..._bounds import Bounds, InvalidBounds
 from ..._metric import Metric
 from ..._sample import (
     AggregatedMetricValue,
     MetricConnection,
     MetricConnectionCategory,
     MetricSample,
 )
-from ._bounds import bounds_from_proto
+from ._bounds import bounds_from_proto2
 from ._metric import metric_from_proto
 from ._metric_connection_category import metric_connection_category_from_proto
 
@@ -144,14 +145,16 @@ def _metric_bounds_from_proto(
     """
     bounds: list[Bounds] = []
     for pb_bound in messages:
-        try:
-            bound = bounds_from_proto(pb_bound)
-        except ValueError as exc:
-            metric_name = metric if isinstance(metric, int) else metric.name
-            major_issues.append(
-                f"bounds for {metric_name} is invalid ({exc}), ignoring these bounds"
-            )
-            continue
-        bounds.append(bound)
+        match bounds_from_proto2(pb_bound):
+            case Bounds() as bound:
+                bounds.append(bound)
+            case InvalidBounds() as bound:
+                metric_name = metric if isinstance(metric, int) else metric.name
+                major_issues.append(
+                    f"bounds for {metric_name} is invalid ({bound}), "
+                    "ignoring these bounds"
+                )
+            case unknown:
+                assert_never(unknown)
 
     return bounds

src/frequenz/client/common/microgrid/electrical_components/__init__.py

@@ -18,7 +18,7 @@
 from ._converter import Converter
 from ._crypto_miner import CryptoMiner
 from ._diagnostic_code import ElectricalComponentDiagnosticCode
-from ._electrical_component import ElectricalComponent
+from ._electrical_component import DefaultT, ElectricalComponent
 from ._electrical_component_connection import (
     BaseElectricalComponentConnection,
     ElectricalComponentConnection,
@@ -85,6 +85,7 @@
     "Converter",
     "CryptoMiner",
     "DcEvCharger",
+    "DefaultT",
     "ElectricalComponent",
     "ElectricalComponentCategory",
     "ElectricalComponentConnection",