From 84fe4cc0df889cdcd8d5a73eb3d0f1b14b6a4c16 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Thu, 27 Aug 2026 12:30:05 +0000 Subject: [PATCH 01/11] Rename the timestamp conversion module to `_datetime` Every other conversion module is named after the wrapper type it produces, not after the protobuf message it consumes: `_delivery_area`, `_location`, `_bounds`. This one converts to and from `datetime`, so `_timestamp` reads like the odd one out, and the tests already agree -- they have always lived in `tests/proto/test_datetime.py`. Rename it and leave the contents alone. Nothing public moves; the symbols are re-exported from `frequenz.client.common.proto` either way. Signed-off-by: Leandro Lucarella --- src/frequenz/client/common/proto/__init__.py | 2 +- .../client/common/proto/{_timestamp.py => _datetime.py} | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename src/frequenz/client/common/proto/{_timestamp.py => _datetime.py} (100%) diff --git a/src/frequenz/client/common/proto/__init__.py b/src/frequenz/client/common/proto/__init__.py index 0212ad5e..756407fb 100644 --- a/src/frequenz/client/common/proto/__init__.py +++ b/src/frequenz/client/common/proto/__init__.py @@ -4,7 +4,7 @@ """General utilities for converting common types to/from protobuf types.""" from ._enum import enum_from_proto -from ._timestamp import datetime_from_proto, datetime_to_proto +from ._datetime import datetime_from_proto, datetime_to_proto __all__ = [ "datetime_from_proto", diff --git a/src/frequenz/client/common/proto/_timestamp.py b/src/frequenz/client/common/proto/_datetime.py similarity index 100% rename from src/frequenz/client/common/proto/_timestamp.py rename to src/frequenz/client/common/proto/_datetime.py From 3f2ae0b8e7a46b47332e2ab2798e07c2aa50a4f9 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Thu, 27 Aug 2026 12:30:43 +0000 Subject: [PATCH 02/11] Add an `InvalidDatetime` wrapper type A protobuf `Timestamp` is two plain integers on the wire, so a decoded message can break the `Timestamp` contract in two ways: a `seconds` count outside the years 1 to 9999, or a `nanos` fraction outside `[0, 999999999]`. Today the first takes the whole conversion down with an exception, and the second is silently repaired into a wrong but plausible-looking `datetime`. Add the wrapper that will carry both instead. `InvalidDatetime` keeps `seconds` and `nanos` exactly as received, so callers can inspect, report or reinterpret what the server actually sent. `InvalidDatetimeError` is the error the semantic accessors will raise once wrapper fields start holding one. Both go in the top-level package rather than in `types`, next to the conversion function that will produce them in `proto/`. `types` wraps `frequenz-api-common` messages, and a timestamp is not one of them: it is a well-known protobuf type, which is exactly why its conversion function lives in the top-level `proto/` package and not in a versioned `proto/v1alphaN/` one. The wrapper follows the converter. Nothing uses it yet; the conversion function and the wrapper fields follow. Signed-off-by: Leandro Lucarella --- src/frequenz/client/common/__init__.py | 3 + src/frequenz/client/common/_datetime.py | 82 +++++++++++++++++++++++++ tests/test_datetime.py | 73 ++++++++++++++++++++++ 3 files changed, 158 insertions(+) create mode 100644 src/frequenz/client/common/_datetime.py create mode 100644 tests/test_datetime.py diff --git a/src/frequenz/client/common/__init__.py b/src/frequenz/client/common/__init__.py index 0580c58d..e5e10304 100644 --- a/src/frequenz/client/common/__init__.py +++ b/src/frequenz/client/common/__init__.py @@ -3,6 +3,7 @@ """Common code and utilities for Frequenz API clients.""" +from ._datetime import InvalidDatetime, InvalidDatetimeError from ._exception import ( ClientCommonError, InvalidAttributeError, @@ -14,6 +15,8 @@ __all__ = [ "ClientCommonError", "InvalidAttributeError", + "InvalidDatetime", + "InvalidDatetimeError", "MissingFieldError", "UnrecognizedEnumValueError", "UnspecifiedEnumValueError", diff --git a/src/frequenz/client/common/_datetime.py b/src/frequenz/client/common/_datetime.py new file mode 100644 index 00000000..40f8e92b --- /dev/null +++ b/src/frequenz/client/common/_datetime.py @@ -0,0 +1,82 @@ +# License: MIT +# Copyright © 2026 Frequenz Energy-as-a-Service GmbH + +"""Timestamps that have no Python equivalent.""" + +from dataclasses import dataclass + +from ._exception import InvalidAttributeError + + +@dataclass(frozen=True, kw_only=True) +class InvalidDatetime: + """A wire timestamp with no [`datetime`][datetime.datetime] equivalent. + + A protobuf `Timestamp` counts whole [`seconds`][.seconds] since the Unix + epoch plus a fraction in [`nanos`][.nanos]. Both are plain integers on the + wire, so a decoded message can carry a value with no meaningful + [`datetime`][datetime.datetime]: + + - a [`seconds`][.seconds] count outside the years 1 to 9999, which is both + the range the protobuf specification allows and the range + [`datetime`][datetime.datetime] covers; + - a [`nanos`][.nanos] fraction outside `[0, 999999999]`, which the + protobuf specification does not allow and whose intended meaning is + therefore unknown. + + This wrapper keeps both raw numbers unchanged so callers can inspect, + report, or reinterpret what the server sent, instead of losing the message + to an exception or to a silently repaired value. It is + protobuf-independent, so it can appear in public wrapper fields. + """ + + seconds: int + """The raw number of seconds since 1970-01-01T00:00:00Z.""" + + nanos: int + """The raw fraction of a second, in nanoseconds, to add to [`seconds`][..seconds].""" + + def __str__(self) -> str: + """Return a compact representation flagging this as an invalid value.""" + return f"" + + +class InvalidDatetimeError(InvalidAttributeError): + """Raised when a semantic accessor sees a timestamp with no `datetime` equivalent. + + The offending [`InvalidDatetime`][..InvalidDatetime] is available as the + [`datetime`][.datetime] attribute so callers can inspect the raw wire + data. + + This is also a [`ValueError`][] for convenience. + """ + + def __init__( + self, + instance: object, + attr_name: str, + datetime: InvalidDatetime, + 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. + datetime: The invalid timestamp instance. + message: A custom error message. If `None`, a default message + mentioning the invalid timestamp is used. + """ + self.datetime: InvalidDatetime = datetime + """The invalid timestamp that caused this error.""" + + super().__init__( + instance, + attr_name, + ( + message + if message is not None + else f"invalid timestamp {datetime} for attribute {attr_name!r} " + f"in {instance}" + ), + ) diff --git a/tests/test_datetime.py b/tests/test_datetime.py new file mode 100644 index 00000000..6967516f --- /dev/null +++ b/tests/test_datetime.py @@ -0,0 +1,73 @@ +# License: MIT +# Copyright © 2026 Frequenz Energy-as-a-Service GmbH + +"""Tests for the `InvalidDatetime` wrapper type and its error.""" + +from frequenz.client.common import ( + ClientCommonError, + InvalidAttributeError, + InvalidDatetime, + InvalidDatetimeError, +) + + +def test_stores_raw_numbers() -> None: + """`InvalidDatetime` stores the raw wire numbers verbatim.""" + invalid = InvalidDatetime(seconds=253402300800, nanos=1) + assert invalid.seconds == 253402300800 + assert invalid.nanos == 1 + + +def test_equality() -> None: + """Two `InvalidDatetime` with the same numbers are equal and hash the same.""" + a = InvalidDatetime(seconds=253402300800, nanos=1) + b = InvalidDatetime(seconds=253402300800, nanos=1) + assert a == b + assert hash(a) == hash(b) + assert a != InvalidDatetime(seconds=253402300800, nanos=2) + + +def test_str() -> None: + """`InvalidDatetime.__str__` renders with a compact invalid marker.""" + assert str(InvalidDatetime(seconds=253402300800, nanos=0)) == ( + "" + ) + + +def test_str_negative_nanos() -> None: + """A negative fraction keeps its sign, so the two numbers stay readable.""" + assert str(InvalidDatetime(seconds=-1, nanos=-1)) == "" + + +def test_error_inherits_invalid_attribute_error() -> None: + """`InvalidDatetimeError` inherits `InvalidAttributeError` (and thus `ValueError`).""" + assert issubclass(InvalidDatetimeError, InvalidAttributeError) + assert issubclass(InvalidDatetimeError, ClientCommonError) + assert issubclass(InvalidDatetimeError, ValueError) + + +def test_error_stores_instance_attr_name_and_datetime() -> None: + """`InvalidDatetimeError` stores `instance`, `attr_name` and `datetime`.""" + instance = object() + invalid = InvalidDatetime(seconds=253402300800, nanos=0) + error = InvalidDatetimeError(instance, "sample_time", invalid) + assert error.instance is instance + assert error.attr_name == "sample_time" + assert error.datetime is invalid + + +def test_error_default_message() -> None: + """The default message follows the `invalid timestamp ...` template.""" + invalid = InvalidDatetime(seconds=253402300800, nanos=0) + assert str(InvalidDatetimeError("some-instance", "sample_time", invalid)) == ( + "invalid timestamp for attribute " + "'sample_time' in some-instance" + ) + + +def test_error_custom_message_replaces_the_default() -> None: + """A custom message replaces the default entirely.""" + invalid = InvalidDatetime(seconds=0, nanos=-1) + assert str(InvalidDatetimeError("i", "a", invalid, "explicit msg")) == ( + "explicit msg" + ) From 51085458ccb0eaac73c310f6346fde96849c64bd Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Thu, 27 Aug 2026 12:31:34 +0000 Subject: [PATCH 03/11] Add `datetime_from_proto2` `datetime_from_proto()` mishandles both ways a decoded `Timestamp` can break its own contract. An out-of-range `seconds` count raises, so the whole conversion fails: datetime_from_proto(Timestamp(seconds=253402300800)) ValueError: year 10000 is out of range An out-of-range `nanos` fraction is worse, because it fails silently: `int(nanos / 1000)` truncates it into the microsecond field, so `nanos=1000000000` becomes a whole extra second and `nanos=-1` becomes zero, and the caller gets a `datetime` that looks fine. Protobuf's own `ToDatetime()` rejects both. `datetime_from_proto()` is released, so this needs a new name rather than a changed contract. Add `datetime_from_proto2()`, returning `datetime | InvalidDatetime`, which keeps the raw `seconds` and `nanos` in either case. It takes no `tz`. A `Timestamp` denotes a UTC instant, and the protobuf `seconds` range is exactly the range `datetime` covers -- both are the years 1 to 9999 -- so in UTC, well-formed and representable are the same thing and `InvalidDatetime` has a single meaning. Accepting a `tz` would break that: converting an in-range instant into another zone overflows within a day of either end, so the wrapper would also have to mean "valid, but not in the zone you asked for". Callers who need another zone call `.astimezone()`, and handle that case themselves. The conversion goes through `timedelta` rather than `datetime.fromtimestamp()`. A `float` timestamp has about 16 significant digits, while the far end of the range needs 18 to keep microsecond resolution, so the released function silently loses sub-second precision there. Building the offset from integers keeps the new one exact across the whole range. Nothing uses it yet; the callers move over one wrapper at a time, and `datetime_from_proto()` is deprecated once none is left. Signed-off-by: Leandro Lucarella --- src/frequenz/client/common/proto/__init__.py | 3 +- src/frequenz/client/common/proto/_datetime.py | 62 ++++++++- tests/proto/test_datetime.py | 128 +++++++++++++++++- 3 files changed, 184 insertions(+), 9 deletions(-) diff --git a/src/frequenz/client/common/proto/__init__.py b/src/frequenz/client/common/proto/__init__.py index 756407fb..7ce6aa53 100644 --- a/src/frequenz/client/common/proto/__init__.py +++ b/src/frequenz/client/common/proto/__init__.py @@ -3,11 +3,12 @@ """General utilities for converting common types to/from protobuf types.""" +from ._datetime import datetime_from_proto, datetime_from_proto2, datetime_to_proto from ._enum import enum_from_proto -from ._datetime import datetime_from_proto, datetime_to_proto __all__ = [ "datetime_from_proto", + "datetime_from_proto2", "datetime_to_proto", "enum_from_proto", ] diff --git a/src/frequenz/client/common/proto/_datetime.py b/src/frequenz/client/common/proto/_datetime.py index c7fe8962..b99bcc22 100644 --- a/src/frequenz/client/common/proto/_datetime.py +++ b/src/frequenz/client/common/proto/_datetime.py @@ -3,11 +3,19 @@ """Helper functions to convert protobuf Timestamp <-> Python datetime.""" -from datetime import datetime, timezone +from datetime import datetime, timedelta, timezone from typing import overload from google.protobuf import timestamp_pb2 +from .._datetime import InvalidDatetime + +_EPOCH = datetime(1970, 1, 1, tzinfo=timezone.utc) +"""The Unix epoch, the instant a protobuf `Timestamp` counts seconds from.""" + +_MAX_NANOS = 999_999_999 +"""The largest fraction of a second a protobuf `Timestamp` may carry.""" + @overload def datetime_to_proto(dt: datetime) -> timestamp_pb2.Timestamp: @@ -65,3 +73,55 @@ def datetime_from_proto( # Add microseconds and add nanoseconds converted to microseconds microseconds = int(ts.nanos / 1000) return datetime.fromtimestamp(ts.seconds + microseconds * 1e-6, tz=tz) + + +def datetime_from_proto2( + ts: timestamp_pb2.Timestamp, +) -> datetime | InvalidDatetime: + """Convert a protobuf Timestamp to a UTC datetime, preserving invalid data. + + A protobuf `Timestamp` is a `seconds` count since the Unix epoch plus a + `nanos` fraction. Both are plain integers on the wire, so a decoded message + can carry values the `Timestamp` contract does not allow. This function + keeps those in its return type instead of raising, so the caller still + receives what the server sent. + + A timestamp is well-formed when `seconds` is in + `[-62135596800, 253402300799]` — the years 1 to 9999 — and `nanos` is in + `[0, 999999999]`. That `seconds` range is exactly the range + [`datetime`][datetime.datetime] covers, so every well-formed timestamp has + a UTC [`datetime`][datetime.datetime]. The `nanos` fraction is truncated to + the microsecond resolution of [`datetime`][datetime.datetime]. + + An out-of-range `nanos` is not carried over into `seconds`. The `Timestamp` + contract does not allow it, so its intended meaning is unknown, and + guessing one would hand the caller a plausible-looking timestamp built from + data the sender never promised. + + Note: + The result is always in UTC, which is what a `Timestamp` denotes. Call + [`astimezone()`][datetime.datetime.astimezone] on it for another zone. + That conversion can itself overflow within a day of either end of the + range, which is why it is left to the caller rather than hidden in a + `tz` argument here. + + Args: + ts: The Timestamp object to convert. + + Returns: + The Timestamp converted to a UTC datetime, or an + [`InvalidDatetime`][frequenz.client.common.InvalidDatetime] + carrying the raw `seconds` and `nanos` when the timestamp is not + well-formed. + """ + seconds = ts.seconds + nanos = ts.nanos + if not 0 <= nanos <= _MAX_NANOS: + return InvalidDatetime(seconds=seconds, nanos=nanos) + try: + # Going through `timedelta` instead of `datetime.fromtimestamp()` keeps + # the conversion exact: a `float` timestamp cannot hold microsecond + # resolution across the whole protobuf range. + return _EPOCH + timedelta(seconds=seconds, microseconds=nanos // 1000) + except OverflowError: + return InvalidDatetime(seconds=seconds, nanos=nanos) diff --git a/tests/proto/test_datetime.py b/tests/proto/test_datetime.py index 82c6eb49..fd0f0c06 100644 --- a/tests/proto/test_datetime.py +++ b/tests/proto/test_datetime.py @@ -3,7 +3,9 @@ """Test conversion helper functions.""" -from datetime import datetime, timezone +from datetime import datetime, timedelta, timezone + +import pytest # pylint: disable=no-name-in-module from google.protobuf.timestamp_pb2 import Timestamp @@ -12,7 +14,13 @@ from hypothesis import given from hypothesis import strategies as st -from frequenz.client.common.proto import datetime_from_proto, datetime_to_proto +from frequenz.client.common import InvalidDatetime +from frequenz.client.common.proto import datetime_from_proto2, datetime_to_proto + +# The oldest and newest instants both protobuf and Python can represent. +_MIN_SECONDS = -62135596800 # 0001-01-01T00:00:00Z +_MAX_SECONDS = 253402300799 # 9999-12-31T23:59:59Z +_MAX_NANOS = 999999999 # Strategy for generating datetime objects # It requires naive datetime objects because it creates the timezone via a strategy @@ -37,9 +45,7 @@ def test_to_timestamp_with_datetime(dt: datetime) -> None: """Test conversion from datetime to Timestamp.""" ts = datetime_to_proto(dt) assert ts is not None - converted_back_dt = datetime_from_proto(ts) - assert dt.tzinfo == converted_back_dt.tzinfo - assert dt.timestamp() == converted_back_dt.timestamp() + assert datetime_from_proto2(ts) == dt def test_to_timestamp_with_none() -> None: @@ -50,8 +56,8 @@ def test_to_timestamp_with_none() -> None: @given(timestamp_strategy) def test_to_datetime(ts: Timestamp) -> None: """Test conversion from Timestamp to datetime.""" - dt = datetime_from_proto(ts) - assert dt is not None + dt = datetime_from_proto2(ts) + assert isinstance(dt, datetime) # Convert back to Timestamp and compare converted_back_ts = datetime_to_proto(dt) assert ts.seconds == converted_back_ts.seconds @@ -68,3 +74,111 @@ def test_no_none_datetime(dt: datetime) -> None: assert ts is not None assert ts2 is None + + +def test_from_proto2_epoch() -> None: + """An all-zero timestamp is the Unix epoch in UTC.""" + assert datetime_from_proto2(Timestamp()) == datetime( + 1970, 1, 1, tzinfo=timezone.utc + ) + + +def test_from_proto2_truncates_nanos_to_microseconds() -> None: + """Sub-microsecond precision is truncated, not rounded.""" + assert datetime_from_proto2(Timestamp(seconds=0, nanos=1999)) == datetime( + 1970, 1, 1, 0, 0, 0, 1, tzinfo=timezone.utc + ) + + +def test_from_proto2_negative_seconds() -> None: + """A `nanos` fraction is added to (not subtracted from) negative seconds.""" + assert datetime_from_proto2(Timestamp(seconds=-1, nanos=500000000)) == datetime( + 1969, 12, 31, 23, 59, 59, 500000, tzinfo=timezone.utc + ) + + +def test_from_proto2_min() -> None: + """The oldest instant protobuf allows is representable.""" + assert datetime_from_proto2(Timestamp(seconds=_MIN_SECONDS)) == datetime( + 1, 1, 1, tzinfo=timezone.utc + ) + + +def test_from_proto2_max() -> None: + """The newest instant protobuf allows is representable, down to the microsecond.""" + assert datetime_from_proto2( + Timestamp(seconds=_MAX_SECONDS, nanos=_MAX_NANOS) + ) == datetime(9999, 12, 31, 23, 59, 59, 999999, tzinfo=timezone.utc) + + +def test_from_proto2_is_always_utc() -> None: + """The result is aware and in UTC, which is what a `Timestamp` denotes.""" + converted = datetime_from_proto2(Timestamp(seconds=0)) + assert isinstance(converted, datetime) + assert converted.tzinfo is timezone.utc + + +def test_from_proto2_result_converts_to_another_zone() -> None: + """The caller expresses the instant in another zone themselves.""" + tz = timezone(timedelta(hours=5, minutes=30)) + converted = datetime_from_proto2(Timestamp(seconds=0)) + assert isinstance(converted, datetime) + in_tz = converted.astimezone(tz) + assert in_tz == converted + assert (in_tz.hour, in_tz.minute) == (5, 30) + + +@pytest.mark.parametrize( + "seconds", + [ + pytest.param(_MAX_SECONDS + 1, id="above-max"), + pytest.param(_MIN_SECONDS - 1, id="below-min"), + pytest.param(2**63 - 1, id="int64-max"), + pytest.param(-(2**63), id="int64-min"), + ], +) +def test_from_proto2_seconds_out_of_python_range(seconds: int) -> None: + """A seconds count outside the years 1 to 9999 is preserved, not raised.""" + assert datetime_from_proto2(Timestamp(seconds=seconds)) == InvalidDatetime( + seconds=seconds, nanos=0 + ) + + +@pytest.mark.parametrize( + "nanos", + [ + pytest.param(-1, id="negative"), + pytest.param(_MAX_NANOS + 1, id="a-whole-second"), + pytest.param(-(2**31), id="int32-min"), + pytest.param(2**31 - 1, id="int32-max"), + ], +) +def test_from_proto2_nanos_out_of_spec(nanos: int) -> None: + """A `nanos` fraction outside `[0, 999999999]` is preserved, not normalized.""" + assert datetime_from_proto2(Timestamp(seconds=1, nanos=nanos)) == InvalidDatetime( + seconds=1, nanos=nanos + ) + + +def test_from_proto2_out_of_range_survives_the_wire() -> None: + """A value only a decoded message can carry round-trips into the wrapper.""" + raw = Timestamp(seconds=_MAX_SECONDS + 1, nanos=-1).SerializeToString() + decoded = Timestamp() + decoded.ParseFromString(raw) + assert datetime_from_proto2(decoded) == InvalidDatetime( + seconds=_MAX_SECONDS + 1, nanos=-1 + ) + + +def test_from_proto2_accepts_every_well_formed_timestamp() -> None: + """The protobuf `seconds` range is exactly the range `datetime` covers. + + That equality is what lets a single `InvalidDatetime` mean "not a + well-formed `Timestamp`" without also having to mean "valid but not + representable". + """ + for seconds in (_MIN_SECONDS, 0, _MAX_SECONDS): + for nanos in (0, _MAX_NANOS): + assert isinstance( + datetime_from_proto2(Timestamp(seconds=seconds, nanos=nanos)), datetime + ) From 3858de10eba11c63253be0b734b017d249312d58 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Thu, 27 Aug 2026 12:32:14 +0000 Subject: [PATCH 04/11] Keep a malformed `MetricSample` sample time `metric_sample_from_proto()` promises to report malformed input through the returned type, but its very first line called `datetime_from_proto()`, so a sample whose timestamp broke the `Timestamp` contract either raised or was silently repaired. When it raised, a single bad sample in a stream failed the whole conversion, and the caller never saw the metric, the value, the bounds or the connection that came with it. Move the converters to `datetime_from_proto2()`, so the rest of the sample survives and the raw seconds and nanoseconds stay available for diagnosis. `MetricSample.sample_time` is released as a plain `datetime`, so it cannot simply widen. Follow the scheme already used in this class for `bounds` / `bounds_set`: the field becomes `sample_time2`, typed `datetime | InvalidDatetime`, and `sample_time` stays as a deprecated read-only property returning a `datetime`. For a malformed timestamp it raises `InvalidDatetimeError`, which is a `ValueError` -- not the same exception as before, but the same kind, and better than returning a repaired value. `get_sample_time()` does the same without the warning, for code that has already migrated. Constructing with `sample_time=` is deliberately *not* deprecated, unlike `bounds=`. A `list[Bounds]` will never be a valid `bounds_set`, so that argument has to warn; a `datetime` will still be a valid `sample_time` after `sample_time2` is renamed back at the next minor release, so warning about it would only create work for callers. `sample_time2=` exists for building a sample from a malformed wire timestamp, and for `dataclasses.replace()`, which passes field names. The released `metric_sample_from_proto_with_issues()` keeps raising, now a plain `ValueError`. It is the loud path callers already depend on, and quietly returning a sample carrying an `InvalidDatetime` instead would let a malformed timestamp through a `major_issues` acceptance check unnoticed. Only the unreleased `metric_sample_from_proto()` keeps the value. Both `__init__` arguments that accept an older spelling are resolved in small static helpers, so the constructor stays readable and both can be deleted in one piece later. Signed-off-by: Leandro Lucarella --- src/frequenz/client/common/metrics/_sample.py | 147 ++++++++++++++++-- .../common/metrics/proto/v1alpha8/_sample.py | 33 ++-- .../v1alpha8/test_sample_metric_sample.py | 57 +++++++ tests/metrics/test_sample_metric_sample.py | 84 +++++++++- 4 files changed, 297 insertions(+), 24 deletions(-) diff --git a/src/frequenz/client/common/metrics/_sample.py b/src/frequenz/client/common/metrics/_sample.py index 79a38917..7e85bef8 100644 --- a/src/frequenz/client/common/metrics/_sample.py +++ b/src/frequenz/client/common/metrics/_sample.py @@ -13,6 +13,7 @@ from frequenz.core.typing import FloatInt from typing_extensions import deprecated +from .._datetime import InvalidDatetime, InvalidDatetimeError from .._exception import UnrecognizedEnumValueError, UnspecifiedEnumValueError from ._bounds import Bounds, BoundsSet, InvalidBoundsSet, InvalidBoundsSetError from ._metric import Metric @@ -186,8 +187,22 @@ class MetricSample: to request current values within the bounds. """ - sample_time: datetime - """The moment when the metric was sampled.""" + sample_time2: datetime | InvalidDatetime + """The moment when the metric was sampled. + + A [`datetime`][datetime.datetime] for a well-formed wire timestamp, or an + [`InvalidDatetime`][....InvalidDatetime] preserving the raw seconds and + nanoseconds when the wire carried a malformed one. + + Tip: + Prefer [`get_sample_time()`][..get_sample_time] to obtain a valid + [`datetime`][datetime.datetime] or a clear error. + + Note: + This field replaces the deprecated [`sample_time`][..sample_time] + property, which cannot express the malformed case. It will be renamed + back to `sample_time` once that property is removed. + """ metric: Metric | int """The metric that was sampled. @@ -242,12 +257,14 @@ class MetricSample: sampled from is important. """ - # This custom `__init__` should be removed once the deprecated `bounds` field is removed. + # This custom `__init__` should be removed once the deprecated `bounds` and + # `sample_time` fields are removed. # pylint: disable-next=too-many-arguments - def __init__( + def __init__( # noqa: DOC502 self, *, - sample_time: datetime, + sample_time: datetime | None = None, + sample_time2: datetime | InvalidDatetime | None = None, metric: Metric | int, value: FloatInt | AggregatedMetricValue | None, bounds_set: BoundsSet | InvalidBoundsSet | None = None, @@ -257,7 +274,14 @@ def __init__( """Initialize this metric sample. Args: - sample_time: The moment when the metric was sampled. + sample_time: The moment when the metric was sampled, as a + well-formed [`datetime`][datetime.datetime]. This spelling + stays valid: it will accept the wider type once + [`sample_time2`][..sample_time2] is renamed back to + `sample_time`. + sample_time2: The moment when the metric was sampled, which may be + an [`InvalidDatetime`][....InvalidDatetime]. Use this to build + a sample from a malformed wire timestamp. metric: The metric that was sampled. value: The value of the sampled metric. bounds_set: The bounds that apply to the metric sample. @@ -268,7 +292,59 @@ def __init__( Raises: TypeError: If both `bounds_set` and the deprecated `bounds` are - given, or if neither is given. + given, or if neither is given; or if both `sample_time` and + `sample_time2` are given, or if neither is given. + """ + sample_time2 = self._resolve_sample_time(sample_time, sample_time2) + bounds_set = self._resolve_bounds_set(bounds_set, bounds) + object.__setattr__(self, "sample_time2", sample_time2) + object.__setattr__(self, "metric", metric) + object.__setattr__(self, "value", value) + object.__setattr__(self, "bounds_set", bounds_set) + object.__setattr__(self, "connection", connection) + + @staticmethod + def _resolve_sample_time( + sample_time: datetime | None, sample_time2: datetime | InvalidDatetime | None + ) -> datetime | InvalidDatetime: + """Pick the sample time from the current and the compatibility argument. + + Args: + sample_time: The compatibility argument. + sample_time2: The current argument. + + Returns: + The sample time to store. + + Raises: + TypeError: If both or neither argument is given. + """ + if sample_time is not None and sample_time2 is not None: + raise TypeError( + "`MetricSample` accepts either `sample_time` or `sample_time2`, " + "not both." + ) + if sample_time is not None: + return sample_time + if sample_time2 is None: + raise TypeError("`MetricSample` requires the `sample_time2` argument.") + return sample_time2 + + @staticmethod + def _resolve_bounds_set( + bounds_set: BoundsSet | InvalidBoundsSet | None, bounds: list[Bounds] | None + ) -> BoundsSet | InvalidBoundsSet: + """Pick the bounds set from the current and the deprecated argument. + + Args: + bounds_set: The current argument. + bounds: The deprecated argument. + + Returns: + The bounds set to store. + + Raises: + TypeError: If both or neither argument is given. """ if bounds is not None and bounds_set is not None: raise TypeError( @@ -279,16 +355,12 @@ def __init__( warnings.warn( "The `bounds` argument is deprecated; use `bounds_set` instead.", DeprecationWarning, - stacklevel=2, + stacklevel=4, ) - bounds_set = BoundsSet(bounds=tuple(bounds)) + return BoundsSet(bounds=tuple(bounds)) if bounds_set is None: raise TypeError("`MetricSample` requires the `bounds_set` argument.") - object.__setattr__(self, "sample_time", sample_time) - object.__setattr__(self, "metric", metric) - object.__setattr__(self, "value", value) - object.__setattr__(self, "bounds_set", bounds_set) - object.__setattr__(self, "connection", connection) + return bounds_set def __str__(self) -> str: """Return a compact string representation of this sample.""" @@ -308,6 +380,29 @@ def __str__(self) -> str: sample = f"{sample}@{self.connection}" return sample + @property + @deprecated("`MetricSample.sample_time` is deprecated; use `sample_time2` instead.") + def sample_time(self) -> datetime: # noqa: DOC502 + """The moment when the metric was sampled. + + Warning: Deprecated + Use [`sample_time2`][..sample_time2] instead, or + [`get_sample_time()`][..get_sample_time] when a valid + [`datetime`][datetime.datetime] is required. This property keeps + the released `datetime` type, so it cannot express a malformed wire + timestamp and raises for one instead. + + Returns: + The sample time, when it is a valid + [`datetime`][datetime.datetime]. + + Raises: + InvalidDatetimeError: If the sample time is an + [`InvalidDatetime`][....InvalidDatetime]. The offending + instance is available on the error's `datetime` attribute. + """ + return self.get_sample_time() + @property @deprecated("`MetricSample.bounds` is deprecated; use `bounds_set` instead.") def bounds(self) -> list[Bounds]: @@ -362,6 +457,30 @@ def as_single_value( case unexpected: assert_never(unexpected) + def get_sample_time(self) -> datetime: + """Return the sample time as a valid `datetime`. + + This is the higher-level accessor for the lower-level + [`sample_time2`][frequenz.client.common.metrics.MetricSample.sample_time2] + field: it returns a valid [`datetime`][datetime.datetime] or raises + instead of exposing an [`InvalidDatetime`][....InvalidDatetime]. + + Returns: + The sample time when it is a valid [`datetime`][datetime.datetime]. + + Raises: + InvalidDatetimeError: If the sample time is an + [`InvalidDatetime`][....InvalidDatetime]. The offending + instance is available on the error's `datetime` attribute. + """ + match self.sample_time2: + case datetime() as sample_time: + return sample_time + case InvalidDatetime() as invalid: + raise InvalidDatetimeError(self, "sample_time2", invalid) + case unexpected: + assert_never(unexpected) + def get_metric(self) -> Metric: """Return the sampled metric as a known enum member. diff --git a/src/frequenz/client/common/metrics/proto/v1alpha8/_sample.py b/src/frequenz/client/common/metrics/proto/v1alpha8/_sample.py index bb924083..706da037 100644 --- a/src/frequenz/client/common/metrics/proto/v1alpha8/_sample.py +++ b/src/frequenz/client/common/metrics/proto/v1alpha8/_sample.py @@ -8,7 +8,8 @@ from frequenz.api.common.v1alpha8.metrics import metrics_pb2 from typing_extensions import deprecated -from ....proto import datetime_from_proto +from ...._datetime import InvalidDatetime +from ....proto import datetime_from_proto2 from ..._metric import Metric from ..._sample import ( AggregatedMetricValue, @@ -75,8 +76,9 @@ def metric_sample_from_proto( Malformed or forward-incompatible input is surfaced through the returned type rather than a side channel: an unspecified metric is preserved as the raw integer `0` and an unrecognized one as its raw integer value in the - `metric` field (typed `Metric | int`), and malformed bounds as an - `InvalidBoundsSet` in `bounds_set`. + `metric` field (typed `Metric | int`), malformed bounds as an + `InvalidBoundsSet` in `bounds_set`, and a malformed sample time as an + `InvalidDatetime` in `sample_time2`. Args: message: The protobuf message to convert. @@ -84,7 +86,7 @@ def metric_sample_from_proto( Returns: The resulting [`MetricSample`][....MetricSample] object. """ - sample_time = datetime_from_proto(message.sample_time) + sample_time = datetime_from_proto2(message.sample_time) raw_metric = message.metric metric: Metric | int = ( @@ -108,7 +110,7 @@ def metric_sample_from_proto( connection = metric_connection_from_proto(message.connection) return MetricSample( - sample_time=sample_time, + sample_time2=sample_time, metric=metric, value=value, bounds_set=bounds_set, @@ -174,10 +176,17 @@ def metric_sample_from_proto_with_issues( Warning: Deprecated Use [`metric_sample_from_proto`][..metric_sample_from_proto] instead and inspect the returned type. The new converter encodes an - unspecified or unrecognized `metric` (`Metric | int`) and malformed - bounds (`InvalidBoundsSet`) in the returned `MetricSample` rather than + unspecified or unrecognized `metric` (`Metric | int`), malformed + bounds (`InvalidBoundsSet`) and an unrepresentable sample time + (`InvalidDatetime`) in the returned `MetricSample` rather than routing them through a side-channel string list. + Note: + A malformed `sample_time` still raises `ValueError`, as it did when the + conversion went through `datetime_from_proto`. Only + [`metric_sample_from_proto`][..metric_sample_from_proto] keeps it in + the returned sample. + Args: message: The protobuf message to convert. major_issues: A list to append major issues to. @@ -185,8 +194,14 @@ def metric_sample_from_proto_with_issues( Returns: The resulting [`MetricSample`][....MetricSample] object. + + Raises: + ValueError: If the sample time is not a well-formed protobuf + `Timestamp`. """ - sample_time = datetime_from_proto(message.sample_time) + sample_time = datetime_from_proto2(message.sample_time) + if isinstance(sample_time, InvalidDatetime): + raise ValueError(f"malformed sample_time {sample_time}") raw_metric = message.metric metric: Metric | int = ( @@ -214,7 +229,7 @@ def metric_sample_from_proto_with_issues( ) return MetricSample( - sample_time=sample_time, + sample_time2=sample_time, metric=metric, value=value, bounds_set=bounds_set, diff --git a/tests/metrics/proto/v1alpha8/test_sample_metric_sample.py b/tests/metrics/proto/v1alpha8/test_sample_metric_sample.py index 4b13bbff..13f85d37 100644 --- a/tests/metrics/proto/v1alpha8/test_sample_metric_sample.py +++ b/tests/metrics/proto/v1alpha8/test_sample_metric_sample.py @@ -13,6 +13,7 @@ from frequenz.api.common.v1alpha8.metrics import bounds_pb2, metrics_pb2 from google.protobuf.timestamp_pb2 import Timestamp +from frequenz.client.common import InvalidDatetime, InvalidDatetimeError from frequenz.client.common.metrics import ( AggregatedMetricValue, Bounds, @@ -378,3 +379,59 @@ def test_from_proto_unspecified_metric() -> None: assert sample.metric == 0 assert not isinstance(sample.metric, Metric) + + +def _sample_with_time(sample_time: Timestamp) -> metrics_pb2.MetricSample: + """Build a minimal well-formed sample carrying the given time. + + Args: + sample_time: The timestamp to put in the `sample_time` field. + + Returns: + The protobuf message. + """ + return metrics_pb2.MetricSample( + sample_time=sample_time, + metric=metric_to_proto(Metric.AC_POWER_ACTIVE), + value=metrics_pb2.MetricValueVariant( + simple_metric=metrics_pb2.SimpleMetricValue(value=5.0) + ), + ) + + +@pytest.mark.parametrize( + "sample_time", + [ + pytest.param(Timestamp(seconds=253402300800), id="year-10000"), + pytest.param(Timestamp(seconds=-62135596801), id="before-year-1"), + pytest.param(Timestamp(seconds=0, nanos=-1), id="negative-nanos"), + pytest.param(Timestamp(seconds=0, nanos=1000000000), id="a-whole-second"), + ], +) +def test_from_proto_unrepresentable_sample_time(sample_time: Timestamp) -> None: + """An unrepresentable sample time is preserved instead of raising.""" + sample = metric_sample_from_proto(_sample_with_time(sample_time)) + + assert sample.sample_time2 == InvalidDatetime( + seconds=sample_time.seconds, nanos=sample_time.nanos + ) + with pytest.raises(InvalidDatetimeError): + sample.get_sample_time() + + +def test_from_proto_with_issues_malformed_sample_time_raises() -> None: + """The released converter keeps raising, as it did before the union.""" + major_issues: list[str] = [] + minor_issues: list[str] = [] + + with ( + pytest.deprecated_call(match="metric_sample_from_proto"), + pytest.raises( + ValueError, match=r"malformed sample_time " + ), + ): + metric_sample_from_proto_with_issues( + _sample_with_time(Timestamp(seconds=253402300800)), + major_issues=major_issues, + minor_issues=minor_issues, + ) diff --git a/tests/metrics/test_sample_metric_sample.py b/tests/metrics/test_sample_metric_sample.py index aeee0140..1b90f58c 100644 --- a/tests/metrics/test_sample_metric_sample.py +++ b/tests/metrics/test_sample_metric_sample.py @@ -9,6 +9,8 @@ from frequenz.core.typing import FloatInt from frequenz.client.common import ( + InvalidDatetime, + InvalidDatetimeError, UnrecognizedEnumValueError, UnspecifiedEnumValueError, ) @@ -72,7 +74,7 @@ def test_creation( bounds_set=bounds_set, connection=connection, ) - assert sample.sample_time == now + assert sample.sample_time2 == now assert sample.metric == Metric.AC_POWER_ACTIVE assert sample.value == value assert sample.bounds_set == bounds_set @@ -372,3 +374,83 @@ def test_get_bounds_set_invalid_raises(now: datetime) -> None: with pytest.raises(InvalidBoundsSetError) as exc_info: sample.get_bounds_set() assert exc_info.value.bounds_set is invalid + + +def test_get_sample_time_returns_valid(now: datetime) -> None: + """get_sample_time returns the time when it is a valid datetime.""" + sample = MetricSample( + sample_time=now, + metric=Metric.AC_POWER_ACTIVE, + value=5.0, + bounds_set=BoundsSet(), + ) + assert sample.get_sample_time() is now + + +def test_get_sample_time_invalid_raises() -> None: + """get_sample_time raises InvalidDatetimeError for an InvalidDatetime.""" + invalid = InvalidDatetime(seconds=253402300800, nanos=0) + sample = MetricSample( + sample_time2=invalid, + metric=Metric.AC_POWER_ACTIVE, + value=5.0, + bounds_set=BoundsSet(), + ) + assert sample.sample_time2 is invalid + with pytest.raises(InvalidDatetimeError) as exc_info: + sample.get_sample_time() + assert exc_info.value.datetime is invalid + assert exc_info.value.attr_name == "sample_time2" + + +def test_deprecated_sample_time_property(now: datetime) -> None: + """The deprecated `sample_time` property still returns the valid datetime.""" + sample = MetricSample( + sample_time=now, + metric=Metric.AC_POWER_ACTIVE, + value=5.0, + bounds_set=BoundsSet(), + ) + assert sample.sample_time2 is now + with pytest.deprecated_call( + match="`MetricSample.sample_time` is deprecated; use `sample_time2` instead." + ): + assert sample.sample_time is now + + +def test_deprecated_sample_time_property_raises_for_invalid() -> None: + """The deprecated property cannot express an `InvalidDatetime`, so it raises.""" + invalid = InvalidDatetime(seconds=253402300800, nanos=0) + sample = MetricSample( + sample_time2=invalid, + metric=Metric.AC_POWER_ACTIVE, + value=5.0, + bounds_set=BoundsSet(), + ) + with pytest.deprecated_call(), pytest.raises(InvalidDatetimeError): + _ = sample.sample_time + + +def test_sample_time_and_sample_time2_raises(now: datetime) -> None: + """Passing both spellings of the sample time is an error.""" + with pytest.raises( + TypeError, + match=r"accepts either `sample_time` or `sample_time2`, not both", + ): + MetricSample( + sample_time=now, + sample_time2=now, + metric=Metric.AC_POWER_ACTIVE, + value=5.0, + bounds_set=BoundsSet(), + ) + + +def test_missing_sample_time_raises() -> None: + """Passing neither spelling of the sample time is an error.""" + with pytest.raises(TypeError, match=r"requires the `sample_time2` argument"): + MetricSample( # pylint: disable=missing-kwoa + metric=Metric.AC_POWER_ACTIVE, + value=5.0, + bounds_set=BoundsSet(), + ) From d08f8dd1f2f50be522ad46f4a78af1996d663c04 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Thu, 27 Aug 2026 16:23:10 +0200 Subject: [PATCH 05/11] Fix the `Deprecated:` section header on `bounds` `mkdocs-material` has no such admonition, so it renders as a generic note. Use `Warning: Deprecated` instead. Signed-off-by: Leandro Lucarella --- src/frequenz/client/common/metrics/_sample.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/frequenz/client/common/metrics/_sample.py b/src/frequenz/client/common/metrics/_sample.py index 7e85bef8..ea80c836 100644 --- a/src/frequenz/client/common/metrics/_sample.py +++ b/src/frequenz/client/common/metrics/_sample.py @@ -408,7 +408,7 @@ def sample_time(self) -> datetime: # noqa: DOC502 def bounds(self) -> list[Bounds]: """The valid bounds that apply to the metric sample. - Deprecated: + Warning: Deprecated Use `bounds_set` instead. For backward compatibility this returns only the valid [`Bounds`][...Bounds] from `bounds_set` (dropping any malformed entries, as the old field did), but it returns the From 898268b562a9e3fc9861f7ddea6f896de4345d80 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Thu, 27 Aug 2026 12:32:46 +0000 Subject: [PATCH 06/11] Keep a malformed `Microgrid.create_time` `microgrid_from_proto()` converts the creation timestamp before it has built anything else, so a `create_timestamp` outside the years 1 to 9999 discarded the whole microgrid -- its ID, name, delivery area and location included -- over a field most callers never read. Move it to `datetime_from_proto2()` and widen `Microgrid.create_time` to `datetime | InvalidDatetime`, plus a `get_create_time()` accessor alongside `get_delivery_area()` and `get_location()` for callers that do read it. `Microgrid` is unreleased, so the annotation widens in place; no compatibility property is needed here. Signed-off-by: Leandro Lucarella --- .../client/common/microgrid/_microgrid.py | 39 ++++++++++++++++- .../microgrid/proto/v1alpha8/_microgrid.py | 8 +++- .../proto/v1alpha8/test_microgrid.py | 42 +++++++++++++++++-- tests/microgrid/test_microgrid.py | 35 +++++++++++++++- 4 files changed, 115 insertions(+), 9 deletions(-) diff --git a/src/frequenz/client/common/microgrid/_microgrid.py b/src/frequenz/client/common/microgrid/_microgrid.py index bac17238..f83f3ff2 100644 --- a/src/frequenz/client/common/microgrid/_microgrid.py +++ b/src/frequenz/client/common/microgrid/_microgrid.py @@ -7,6 +7,7 @@ from dataclasses import dataclass, field from typing import assert_never +from .._datetime import InvalidDatetime, InvalidDatetimeError from .._exception import ( MissingFieldError, UnrecognizedEnumValueError, @@ -63,8 +64,17 @@ class Microgrid: # pylint: disable=too-many-instance-attributes location: Location | None """The physical location of the microgrid, in geographical co-ordinates.""" - create_time: datetime.datetime - """The UTC timestamp indicating when the microgrid was initially created.""" + create_time: datetime.datetime | InvalidDatetime + """The UTC timestamp indicating when the microgrid was initially created. + + An [`InvalidDatetime`][....InvalidDatetime] preserves the raw seconds + and nanoseconds when the wire carried a timestamp Python cannot represent. + + Tip: + This is the lower-level field; prefer + [`get_create_time()`][..get_create_time] to obtain a valid + [`datetime`][datetime.datetime] or a clear error. + """ _active: bool | int """Whether the microgrid is active. @@ -180,6 +190,31 @@ def get_delivery_area_or_none(self) -> DeliveryArea | None: case unknown: assert_never(unknown) + def get_create_time(self) -> datetime.datetime: + """Return the creation time as a valid `datetime`. + + This is the higher-level accessor for the [`create_time`][..create_time] + attribute: it resolves the field to a valid + [`datetime`][datetime.datetime] or raises a clear, catchable error. + + Returns: + The creation time, when it is a valid + [`datetime`][datetime.datetime]. + + Raises: + InvalidDatetimeError: If the creation time is an + [`InvalidDatetime`][....InvalidDatetime]. The offending + instance is available on the exception's `datetime` + attribute. + """ + match self.create_time: + case datetime.datetime() as valid: + return valid + case InvalidDatetime() as invalid: + raise InvalidDatetimeError(self, "create_time", invalid) + case unknown: + assert_never(unknown) + def get_location(self) -> Location: """Return the location as a [`Location`][....types.Location]. diff --git a/src/frequenz/client/common/microgrid/proto/v1alpha8/_microgrid.py b/src/frequenz/client/common/microgrid/proto/v1alpha8/_microgrid.py index 5a7ae505..38bd8361 100644 --- a/src/frequenz/client/common/microgrid/proto/v1alpha8/_microgrid.py +++ b/src/frequenz/client/common/microgrid/proto/v1alpha8/_microgrid.py @@ -7,7 +7,7 @@ from ....grid import DeliveryArea, InvalidDeliveryArea from ....grid.proto.v1alpha8 import delivery_area_from_proto2 -from ....proto import datetime_from_proto +from ....proto import datetime_from_proto2 from ....types import Location from ....types.proto.v1alpha8 import location_from_proto from ..._ids import EnterpriseId, MicrogridId @@ -38,6 +38,10 @@ def _microgrid_status_to_active(value: int) -> bool | int: def microgrid_from_proto(message: microgrid_pb2.Microgrid) -> Microgrid: """Convert a protobuf message to a [`Microgrid`][....Microgrid] object. + Malformed input is surfaced through the returned object rather than a side + channel: a malformed delivery area becomes an `InvalidDeliveryArea` and a + creation time Python cannot represent an `InvalidDatetime`. + Args: message: The protobuf message to convert. @@ -59,7 +63,7 @@ def microgrid_from_proto(message: microgrid_pb2.Microgrid) -> Microgrid: name=message.name, delivery_area=delivery_area, location=location, - create_time=datetime_from_proto(message.create_timestamp), + create_time=datetime_from_proto2(message.create_timestamp), _active=_microgrid_status_to_active(message.status), _allow_construction=True, ) diff --git a/tests/microgrid/proto/v1alpha8/test_microgrid.py b/tests/microgrid/proto/v1alpha8/test_microgrid.py index 2297df3e..67a92435 100644 --- a/tests/microgrid/proto/v1alpha8/test_microgrid.py +++ b/tests/microgrid/proto/v1alpha8/test_microgrid.py @@ -11,7 +11,12 @@ from frequenz.api.common.v1alpha8.grid import delivery_area_pb2 from frequenz.api.common.v1alpha8.microgrid import microgrid_pb2 +# pylint: disable-next=no-name-in-module +from google.protobuf.timestamp_pb2 import Timestamp + from frequenz.client.common import ( + InvalidDatetime, + InvalidDatetimeError, UnrecognizedEnumValueError, UnspecifiedEnumValueError, ) @@ -131,16 +136,18 @@ def _assert_active(info: Microgrid, expected_active: bool | int) -> None: "frequenz.client.common.microgrid.proto.v1alpha8._microgrid.delivery_area_from_proto2" ) @patch("frequenz.client.common.microgrid.proto.v1alpha8._microgrid.location_from_proto") -@patch("frequenz.client.common.microgrid.proto.v1alpha8._microgrid.datetime_from_proto") +@patch( + "frequenz.client.common.microgrid.proto.v1alpha8._microgrid.datetime_from_proto2" +) def test_from_proto( - mock_datetime_from_proto: Mock, + mock_datetime_from_proto2: Mock, mock_location_from_proto: Mock, mock_delivery_area_from_proto: Mock, case: _ProtoConversionTestCase, ) -> None: """Test conversion from protobuf message to Microgrid.""" now = datetime.now(timezone.utc) - mock_datetime_from_proto.return_value = now + mock_datetime_from_proto2.return_value = now mock_location = ( Location( @@ -198,7 +205,7 @@ def test_from_proto( _assert_active(info, case.expected_active) # Verify mock calls - mock_datetime_from_proto.assert_called_once_with(proto.create_timestamp) + mock_datetime_from_proto2.assert_called_once_with(proto.create_timestamp) if case.has_delivery_area: mock_delivery_area_from_proto.assert_called_once_with(proto.delivery_area) @@ -213,3 +220,30 @@ def test_from_proto( else: mock_location_from_proto.assert_not_called() assert info.location is None + + +@pytest.mark.parametrize( + "create_timestamp", + [ + pytest.param(Timestamp(seconds=253402300800), id="year-10000"), + pytest.param(Timestamp(seconds=0, nanos=1000000000), id="a-whole-second"), + pytest.param(Timestamp(seconds=0, nanos=-1), id="negative-nanos"), + ], +) +def test_from_proto_unrepresentable_create_time(create_timestamp: Timestamp) -> None: + """An unrepresentable creation time is preserved instead of raising.""" + proto = microgrid_pb2.Microgrid( + id=1234, + enterprise_id=5678, + name="Test Grid", + status=microgrid_pb2.MICROGRID_STATUS_ACTIVE, + create_timestamp=create_timestamp, + ) + + info = microgrid_from_proto(proto) + + assert info.create_time == InvalidDatetime( + seconds=create_timestamp.seconds, nanos=create_timestamp.nanos + ) + with pytest.raises(InvalidDatetimeError): + info.get_create_time() diff --git a/tests/microgrid/test_microgrid.py b/tests/microgrid/test_microgrid.py index 7a56bbd0..fa014fc4 100644 --- a/tests/microgrid/test_microgrid.py +++ b/tests/microgrid/test_microgrid.py @@ -9,6 +9,8 @@ import pytest from frequenz.client.common import ( + InvalidDatetime, + InvalidDatetimeError, MissingFieldError, UnrecognizedEnumValueError, UnspecifiedEnumValueError, @@ -198,6 +200,7 @@ def test_replace_preserves_construction() -> None: def _make_microgrid( delivery_area: DeliveryArea | InvalidDeliveryArea | None = None, location: Location | None = None, + create_time: datetime | InvalidDatetime | None = None, ) -> Microgrid: """Build a Microgrid with the given delivery area and location for accessor tests.""" return Microgrid( @@ -206,7 +209,9 @@ def _make_microgrid( name="", delivery_area=delivery_area, location=location, - create_time=datetime.now(timezone.utc), + create_time=( + datetime.now(timezone.utc) if create_time is None else create_time + ), _active=True, _allow_construction=True, ) @@ -311,3 +316,31 @@ def test_get_location_error_is_value_error() -> None: info = _make_microgrid() with pytest.raises(ValueError): info.get_location() + + +def test_get_create_time_returns_valid() -> None: + """`get_create_time()` returns the stored `datetime` unchanged.""" + now = datetime.now(timezone.utc) + info = _make_microgrid(create_time=now) + assert info.get_create_time() is now + + +def test_get_create_time_raises_invalid_for_invalid_datetime() -> None: + """`get_create_time()` raises `InvalidDatetimeError` for an `InvalidDatetime`.""" + invalid = InvalidDatetime(seconds=253402300800, nanos=0) + info = _make_microgrid(create_time=invalid) + assert info.create_time is invalid + with pytest.raises( + InvalidDatetimeError, + match=r"invalid timestamp for attribute " + r"'create_time' in MID1234", + ) as exc_info: + info.get_create_time() + assert exc_info.value.datetime is invalid + + +def test_get_create_time_error_is_value_error() -> None: + """The `InvalidDatetimeError` raised by the accessor is also a `ValueError`.""" + info = _make_microgrid(create_time=InvalidDatetime(seconds=0, nanos=-1)) + with pytest.raises(ValueError): + info.get_create_time() From cd8eb6bdf293a8cddb18af831c36e9501c079f05 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Thu, 27 Aug 2026 12:33:23 +0000 Subject: [PATCH 07/11] Keep a malformed timestamp in `InvalidLifetime` `lifetime_from_proto()` already returns `Lifetime | InvalidLifetime`, but it built both ends with `datetime_from_proto()`, so a malformed timestamp raised before the invariant check it feeds could ever run. Every electrical component and connection converter goes through this function, so one such lifetime failed the whole component. Move it to `datetime_from_proto2()` and let `InvalidLifetime` hold what it returns: its two fields become `datetime | InvalidDatetime | None`. `Lifetime` keeps `datetime | None`, because it always represents a valid lifetime. This makes `BaseLifetime` pointless. It existed only to hold two fields both subclasses shared, and they no longer share them: a subclass cannot narrow an inherited attribute, so keeping the base would mean declaring `Lifetime.start_time` as a union it never holds and adding accessors to unwrap a case it rejects. `Lifetime` and `InvalidLifetime` become two independent frozen dataclasses (like `BoundsSet` and `InvalidBoundsSet`, which are two independent classes for the same reason). `BaseLifetime` was never released so we can simply remove it. Signed-off-by: Leandro Lucarella --- docs/user-guide/overview.md | 5 +- .../client/common/microgrid/__init__.py | 2 - .../client/common/microgrid/_lifetime.py | 112 ++++++++++++------ .../proto/v1alpha8/_electrical_component.py | 4 +- .../_electrical_component_connection.py | 4 +- .../microgrid/proto/v1alpha8/_lifetime.py | 25 ++-- .../microgrid/_lifetime/test_base_lifetime.py | 14 --- .../_lifetime/test_invalid_lifetime.py | 29 +++-- tests/microgrid/_lifetime/test_lifetime.py | 7 +- .../proto/v1alpha8/conftest.py | 4 +- .../microgrid/proto/v1alpha8/test_lifetime.py | 35 ++++++ 11 files changed, 154 insertions(+), 87 deletions(-) delete mode 100644 tests/microgrid/_lifetime/test_base_lifetime.py diff --git a/docs/user-guide/overview.md b/docs/user-guide/overview.md index 270dc565..92a86e2f 100644 --- a/docs/user-guide/overview.md +++ b/docs/user-guide/overview.md @@ -37,9 +37,8 @@ Use [`Microgrid`][frequenz.client.common.microgrid.Microgrid], [`MicrogridId`][frequenz.client.common.microgrid.MicrogridId], [`EnterpriseId`][frequenz.client.common.microgrid.EnterpriseId], and [`Lifetime`][frequenz.client.common.microgrid.Lifetime]. -[`BaseLifetime`][frequenz.client.common.microgrid.BaseLifetime] and -[`InvalidLifetime`][frequenz.client.common.microgrid.InvalidLifetime] represent -the lifetime variants, while +[`InvalidLifetime`][frequenz.client.common.microgrid.InvalidLifetime] carries +malformed wire data, while [`InvalidLifetimeError`][frequenz.client.common.microgrid.InvalidLifetimeError] is raised by safe accessors. diff --git a/src/frequenz/client/common/microgrid/__init__.py b/src/frequenz/client/common/microgrid/__init__.py index ee982607..27580c4a 100644 --- a/src/frequenz/client/common/microgrid/__init__.py +++ b/src/frequenz/client/common/microgrid/__init__.py @@ -5,7 +5,6 @@ from ._ids import EnterpriseId, MicrogridId from ._lifetime import ( - BaseLifetime, InvalidLifetime, InvalidLifetimeError, Lifetime, @@ -13,7 +12,6 @@ from ._microgrid import Microgrid __all__ = [ - "BaseLifetime", "EnterpriseId", "InvalidLifetime", "InvalidLifetimeError", diff --git a/src/frequenz/client/common/microgrid/_lifetime.py b/src/frequenz/client/common/microgrid/_lifetime.py index c35bce31..7cc086b6 100644 --- a/src/frequenz/client/common/microgrid/_lifetime.py +++ b/src/frequenz/client/common/microgrid/_lifetime.py @@ -5,49 +5,25 @@ from dataclasses import dataclass from datetime import datetime, timezone -from typing import Any, Self +from typing import assert_never +from .._datetime import InvalidDatetime from .._exception import InvalidAttributeError @dataclass(frozen=True, kw_only=True) -class BaseLifetime: - """A base class for well-formed and malformed operational lifetimes. - - This class cannot be instantiated directly. Use [`Lifetime`][..Lifetime] - for a valid period or [`InvalidLifetime`][..InvalidLifetime] to preserve - malformed wire data. - """ - - start_time: datetime | None = None - """The moment when the asset became operationally active. - - If `None`, the asset is considered to be active in any past moment previous to the - [`end_time`][..end_time]. - """ - - end_time: datetime | None = None - """The moment when the asset's operational activity ceased. - - If `None`, the asset is considered to be active with no plans to be deactivated. - """ - - # pylint: disable-next=unused-argument - def __new__(cls, *args: Any, **kwargs: Any) -> Self: - """Prevent instantiation of this class.""" - if cls is BaseLifetime: - raise TypeError(f"Cannot instantiate {cls.__name__} directly") - return super().__new__(cls) - - -@dataclass(frozen=True, kw_only=True) -class Lifetime(BaseLifetime): +class Lifetime: """An active operational period of an asset. When both [`start_time`][.start_time] and [`end_time`][.end_time] are `None`, the lifetime is unbounded and the asset is considered operational at every timestamp. + Both timestamps are well-formed [`datetime`][datetime.datetime] values. + A lifetime built from a malformed wire timestamp is an + [`InvalidLifetime`][..InvalidLifetime] instead, so code holding a + `Lifetime` can compare and order its ends without checking them first. + Warning: The [`end_time`][.end_time] timestamp indicates that the asset has been permanently removed from service. @@ -59,8 +35,26 @@ class Lifetime(BaseLifetime): data received from the wire. """ + start_time: datetime | None = None + """The moment when the asset became operationally active. + + If `None`, the asset is considered to be active in any past moment previous to the + [`end_time`][..end_time]. + """ + + end_time: datetime | None = None + """The moment when the asset's operational activity ceased. + + If `None`, the asset is considered to be active with no plans to be deactivated. + """ + def __post_init__(self) -> None: - """Validate this lifetime.""" + """Validate this lifetime. + + Raises: + ValueError: If [`start_time`][..start_time] is later than + [`end_time`][..end_time]. + """ if ( self.start_time is not None and self.end_time is not None @@ -97,7 +91,7 @@ def is_operational_now(self) -> bool: @dataclass(frozen=True, kw_only=True) -class InvalidLifetime(BaseLifetime): +class InvalidLifetime: """An operational lifetime with malformed data received from the wire. This class preserves lifetime data that fails the invariants required for @@ -105,17 +99,59 @@ class InvalidLifetime(BaseLifetime): timestamps without accidentally using them for operational checks. Use a semantic accessor, such as `ElectricalComponent.get_operational_lifetime()`, to receive a clear [`InvalidLifetimeError`][..InvalidLifetimeError]. + + Either end may also be an [`InvalidDatetime`][...InvalidDatetime], for a + wire timestamp that is not a well-formed protobuf `Timestamp`. This class + enforces no invariants, so it provides no operational checks and no + accessors: code that reaches an `InvalidLifetime` is already handling + malformed data and reads the two fields directly. + """ + + start_time: datetime | InvalidDatetime | None = None + """The moment when the asset became operationally active. + + `None` when the wire did not set it. An + [`InvalidDatetime`][....InvalidDatetime] when the wire set a malformed + timestamp. + """ + + end_time: datetime | InvalidDatetime | None = None + """The moment when the asset's operational activity ceased. + + `None` when the wire did not set it. An + [`InvalidDatetime`][....InvalidDatetime] when the wire set a malformed + timestamp. """ def __str__(self) -> str: """Return a compact string representation of this invalid lifetime.""" - start_str = ( - self.start_time.isoformat() if self.start_time is not None else "-inf" - ) - end_str = self.end_time.isoformat() if self.end_time is not None else "+inf" + start_str = _format_time(self.start_time, unset="-inf") + end_str = _format_time(self.end_time, unset="+inf") return f"" +def _format_time(value: datetime | InvalidDatetime | None, *, unset: str) -> str: + """Render one end of an invalid lifetime range. + + Args: + value: The raw field value. + unset: The text to use when the field is unset. + + Returns: + The ISO 8601 representation of a well-formed timestamp, the invalid + marker of a malformed one, or `unset`. + """ + match value: + case None: + return unset + case datetime() as valid: + return valid.isoformat() + case InvalidDatetime() as invalid: + return str(invalid) + case unknown: + assert_never(unknown) + + class InvalidLifetimeError(InvalidAttributeError): """Raised when a semantic accessor sees an invalid lifetime. diff --git a/src/frequenz/client/common/microgrid/electrical_components/proto/v1alpha8/_electrical_component.py b/src/frequenz/client/common/microgrid/electrical_components/proto/v1alpha8/_electrical_component.py index 0ad1baa4..84f06aa1 100644 --- a/src/frequenz/client/common/microgrid/electrical_components/proto/v1alpha8/_electrical_component.py +++ b/src/frequenz/client/common/microgrid/electrical_components/proto/v1alpha8/_electrical_component.py @@ -1265,8 +1265,8 @@ def _get_operational_lifetime_from_proto( Returns: The extracted operational lifetime, an invalid lifetime preserving - malformed timestamp ordering, or an unbounded lifetime if the field - is missing. + malformed timestamp ordering or a timestamp Python cannot + represent, or an unbounded lifetime if the field is missing. """ if message.HasField("operational_lifetime"): return lifetime_from_proto(message.operational_lifetime) diff --git a/src/frequenz/client/common/microgrid/electrical_components/proto/v1alpha8/_electrical_component_connection.py b/src/frequenz/client/common/microgrid/electrical_components/proto/v1alpha8/_electrical_component_connection.py index a27de45b..0482fda7 100644 --- a/src/frequenz/client/common/microgrid/electrical_components/proto/v1alpha8/_electrical_component_connection.py +++ b/src/frequenz/client/common/microgrid/electrical_components/proto/v1alpha8/_electrical_component_connection.py @@ -62,8 +62,8 @@ def _get_operational_lifetime_from_proto( Returns: The extracted operational lifetime, an invalid lifetime preserving - malformed timestamp ordering, or an unbounded lifetime if the field - is missing. + malformed timestamp ordering or a timestamp Python cannot + represent, or an unbounded lifetime if the field is missing. """ if message.HasField("operational_lifetime"): return lifetime_from_proto(message.operational_lifetime) diff --git a/src/frequenz/client/common/microgrid/proto/v1alpha8/_lifetime.py b/src/frequenz/client/common/microgrid/proto/v1alpha8/_lifetime.py index 9c6451e0..3e3e7fae 100644 --- a/src/frequenz/client/common/microgrid/proto/v1alpha8/_lifetime.py +++ b/src/frequenz/client/common/microgrid/proto/v1alpha8/_lifetime.py @@ -5,7 +5,8 @@ from frequenz.api.common.v1alpha8.microgrid import lifetime_pb2 -from ....proto import datetime_from_proto +from ...._datetime import InvalidDatetime +from ....proto import datetime_from_proto2 from ..._lifetime import InvalidLifetime, Lifetime @@ -16,23 +17,25 @@ def lifetime_from_proto(message: lifetime_pb2.Lifetime) -> Lifetime | InvalidLif message: The protobuf message to convert. Returns: - A [`Lifetime`][....Lifetime] when the timestamps form a valid range, or - an [`InvalidLifetime`][....InvalidLifetime] preserving malformed - timestamp ordering. A present but empty protobuf message becomes an - unbounded `Lifetime()`. + A [`Lifetime`][....Lifetime] when both timestamps are well-formed and + form a valid range, or an + [`InvalidLifetime`][....InvalidLifetime] preserving a malformed + timestamp or a reversed range. A present but empty protobuf + message becomes an unbounded `Lifetime()`. """ start = ( - datetime_from_proto(message.start_timestamp) + datetime_from_proto2(message.start_timestamp) if message.HasField("start_timestamp") else None ) end = ( - datetime_from_proto(message.end_timestamp) + datetime_from_proto2(message.end_timestamp) if message.HasField("end_timestamp") else None ) - try: - return Lifetime(start_time=start, end_time=end) - except ValueError: - pass + if not isinstance(start, InvalidDatetime) and not isinstance(end, InvalidDatetime): + try: + return Lifetime(start_time=start, end_time=end) + except ValueError: + pass return InvalidLifetime(start_time=start, end_time=end) diff --git a/tests/microgrid/_lifetime/test_base_lifetime.py b/tests/microgrid/_lifetime/test_base_lifetime.py deleted file mode 100644 index 7b633c86..00000000 --- a/tests/microgrid/_lifetime/test_base_lifetime.py +++ /dev/null @@ -1,14 +0,0 @@ -# License: MIT -# Copyright © 2026 Frequenz Energy-as-a-Service GmbH - -"""Tests for `BaseLifetime`.""" - -import pytest - -from frequenz.client.common.microgrid import BaseLifetime - - -def test_cannot_be_instantiated_directly() -> None: - """`BaseLifetime` refuses direct instantiation.""" - with pytest.raises(TypeError, match="Cannot instantiate BaseLifetime directly"): - BaseLifetime() diff --git a/tests/microgrid/_lifetime/test_invalid_lifetime.py b/tests/microgrid/_lifetime/test_invalid_lifetime.py index 1b3ffc3e..24e83b7f 100644 --- a/tests/microgrid/_lifetime/test_invalid_lifetime.py +++ b/tests/microgrid/_lifetime/test_invalid_lifetime.py @@ -8,7 +8,8 @@ import pytest -from frequenz.client.common.microgrid import BaseLifetime, InvalidLifetime +from frequenz.client.common import InvalidDatetime +from frequenz.client.common.microgrid import InvalidLifetime @dataclass(frozen=True, kw_only=True) @@ -18,21 +19,16 @@ class _StrTestCase: name: str """The description of the test case.""" - start_time: datetime | None + start_time: datetime | InvalidDatetime | None """The start time to use for the invalid lifetime.""" - end_time: datetime | None + end_time: datetime | InvalidDatetime | None """The end time to use for the invalid lifetime.""" expected_str: str """The expected string representation.""" -def test_is_base_lifetime_subclass() -> None: - """`InvalidLifetime` is a subclass of `BaseLifetime`.""" - assert issubclass(InvalidLifetime, BaseLifetime) - - def test_accepts_invalid_range(present: datetime, future: datetime) -> None: """`InvalidLifetime` preserves an end time before its start time.""" lifetime = InvalidLifetime(start_time=future, end_time=present) @@ -41,6 +37,15 @@ def test_accepts_invalid_range(present: datetime, future: datetime) -> None: assert lifetime.end_time is present +def test_accepts_invalid_datetime(present: datetime) -> None: + """`InvalidLifetime` preserves a timestamp with no `datetime` equivalent.""" + invalid = InvalidDatetime(seconds=253402300800, nanos=0) + lifetime = InvalidLifetime(start_time=present, end_time=invalid) + + assert lifetime.start_time is present + assert lifetime.end_time is invalid + + @pytest.mark.parametrize( "case", [ @@ -70,6 +75,14 @@ def test_accepts_invalid_range(present: datetime, future: datetime) -> None: end_time=None, expected_str="", ), + _StrTestCase( + name="unrepresentable_start", + start_time=InvalidDatetime(seconds=253402300800, nanos=0), + end_time=datetime(2025, 1, 1, 12, 0, 0, tzinfo=timezone.utc), + expected_str=( + ",2025-01-01T12:00:00+00:00]>" + ), + ), ], ids=lambda case: case.name, ) diff --git a/tests/microgrid/_lifetime/test_lifetime.py b/tests/microgrid/_lifetime/test_lifetime.py index f6757570..0587f2ff 100644 --- a/tests/microgrid/_lifetime/test_lifetime.py +++ b/tests/microgrid/_lifetime/test_lifetime.py @@ -9,7 +9,7 @@ import pytest -from frequenz.client.common.microgrid import BaseLifetime, Lifetime +from frequenz.client.common.microgrid import Lifetime class _Time(Enum): @@ -96,11 +96,6 @@ class _StrTestCase: """The expected string representation.""" -def test_is_base_lifetime_subclass() -> None: - """`Lifetime` is a subclass of `BaseLifetime`.""" - assert issubclass(Lifetime, BaseLifetime) - - @pytest.mark.parametrize( "case", [ diff --git a/tests/microgrid/electrical_components/proto/v1alpha8/conftest.py b/tests/microgrid/electrical_components/proto/v1alpha8/conftest.py index 4f2fdfd6..3e445151 100644 --- a/tests/microgrid/electrical_components/proto/v1alpha8/conftest.py +++ b/tests/microgrid/electrical_components/proto/v1alpha8/conftest.py @@ -142,7 +142,9 @@ def base_data_as_proto( (provides_telemetry, accepts_control) ], ) - if base_data.lifetime: + # This builder only ever receives valid lifetimes; a malformed timestamp has + # no protobuf spelling to write back. + if isinstance(base_data.lifetime, Lifetime): lifetime_dict: dict[str, Timestamp] = {} if base_data.lifetime.start_time is not None: lifetime_dict["start_timestamp"] = datetime_to_proto( diff --git a/tests/microgrid/proto/v1alpha8/test_lifetime.py b/tests/microgrid/proto/v1alpha8/test_lifetime.py index c901b657..9cbbff86 100644 --- a/tests/microgrid/proto/v1alpha8/test_lifetime.py +++ b/tests/microgrid/proto/v1alpha8/test_lifetime.py @@ -11,6 +11,7 @@ from frequenz.api.common.v1alpha8.microgrid import lifetime_pb2 from google.protobuf import timestamp_pb2 +from frequenz.client.common import InvalidDatetime from frequenz.client.common.microgrid import InvalidLifetime from frequenz.client.common.microgrid.proto.v1alpha8 import lifetime_from_proto @@ -115,3 +116,37 @@ def test_from_proto_preserves_start_after_end( assert isinstance(lifetime, InvalidLifetime) assert lifetime.start_time == future assert lifetime.end_time == now + + +@pytest.mark.parametrize("field_name", ["start_timestamp", "end_timestamp"]) +@pytest.mark.parametrize( + "unrepresentable", + [ + pytest.param(timestamp_pb2.Timestamp(seconds=253402300800), id="year-10000"), + pytest.param( + timestamp_pb2.Timestamp(seconds=0, nanos=1000000000), + id="a-whole-second-of-nanos", + ), + pytest.param(timestamp_pb2.Timestamp(seconds=0, nanos=-1), id="negative-nanos"), + ], +) +def test_from_proto_preserves_unrepresentable_timestamp( + now: datetime, field_name: str, unrepresentable: timestamp_pb2.Timestamp +) -> None: + """A timestamp with no `datetime` equivalent makes the whole lifetime invalid.""" + now_ts = timestamp_pb2.Timestamp() + now_ts.FromDatetime(now) + + proto_kwargs: dict[str, Any] = { + "start_timestamp": now_ts, + "end_timestamp": now_ts, + } + proto_kwargs[field_name] = unrepresentable + + lifetime = lifetime_from_proto(lifetime_pb2.Lifetime(**proto_kwargs)) + attr_name = field_name.replace("timestamp", "time") + + assert isinstance(lifetime, InvalidLifetime) + assert getattr(lifetime, attr_name) == InvalidDatetime( + seconds=unrepresentable.seconds, nanos=unrepresentable.nanos + ) From 8424832dbf297d4a2d7b24cf055180c14c092b31 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Thu, 27 Aug 2026 16:35:31 +0200 Subject: [PATCH 08/11] Document when not to use a base class The wrapping guide told when to add a base class, but it didn't explicitly mention when not to do it, so make it more explicit, including examples. Signed-off-by: Leandro Lucarella --- docs/wrapping-guide/validity-in-the-type.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/docs/wrapping-guide/validity-in-the-type.md b/docs/wrapping-guide/validity-in-the-type.md index 0fe58953..6be58808 100644 --- a/docs/wrapping-guide/validity-in-the-type.md +++ b/docs/wrapping-guide/validity-in-the-type.md @@ -18,6 +18,14 @@ For example, [`InvalidDeliveryArea`][frequenz.client.common.grid.InvalidDeliveryArea] share a base while making their validity visible in annotations. +Share a base only while both types hold the same field types. When the invalid +type has to accept a wider type in a field, because that field can itself +carry an `Invalid*` wrapper, write two independent classes instead. +[`BoundsSet`][frequenz.client.common.metrics.BoundsSet] and +[`InvalidBoundsSet`][frequenz.client.common.metrics.InvalidBoundsSet] do this, +as do [`Lifetime`][frequenz.client.common.microgrid.Lifetime] and +[`InvalidLifetime`][frequenz.client.common.microgrid.InvalidLifetime]. + Normal constructors enforce the valid subclass's rules. A conversion function that sees invalid protobuf data creates the matching invalid subclass and returns `X | InvalidX`. The invalid subclass keeps the raw fields for From abe0ac6a4a401f91ecfb10f473036110bdb5230e Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Thu, 27 Aug 2026 12:33:57 +0000 Subject: [PATCH 09/11] Deprecate `datetime_from_proto` Nothing in the library calls it any more: metric samples, microgrids and lifetimes all went over to `datetime_from_proto2()`. Point the external callers there too via a deprecation warning. The warning mentions the replacement and its return type, and the docstring explains the three reasons to move: the new function keeps a malformed timestamp in the return type instead of raising or repairing it, it does not lose sub-second precision far from the epoch, and it returns UTC rather than taking a `tz`. The `Raises` section is new. The old function always raised on an out-of-range timestamp, but never said so, which is a large part of why callers were surprised by it. Signed-off-by: Leandro Lucarella --- src/frequenz/client/common/proto/_datetime.py | 23 ++++++++++++++++++- tests/proto/test_datetime.py | 22 +++++++++++++++++- 2 files changed, 43 insertions(+), 2 deletions(-) diff --git a/src/frequenz/client/common/proto/_datetime.py b/src/frequenz/client/common/proto/_datetime.py index b99bcc22..80bf1f0c 100644 --- a/src/frequenz/client/common/proto/_datetime.py +++ b/src/frequenz/client/common/proto/_datetime.py @@ -7,6 +7,7 @@ from typing import overload from google.protobuf import timestamp_pb2 +from typing_extensions import deprecated from .._datetime import InvalidDatetime @@ -58,17 +59,37 @@ def datetime_to_proto(dt: datetime | None) -> timestamp_pb2.Timestamp | None: return ts -def datetime_from_proto( +@deprecated( + "`datetime_from_proto` is deprecated; use " + "`datetime_from_proto2` (returns `datetime | InvalidDatetime`) instead." +) +def datetime_from_proto( # noqa: DOC502 ts: timestamp_pb2.Timestamp, tz: timezone = timezone.utc ) -> datetime: """Convert a protobuf Timestamp to a datetime. + Warning: Deprecated + Use [`datetime_from_proto2`][..datetime_from_proto2] instead. The new + conversion function keeps a malformed timestamp in its return type + (`datetime | InvalidDatetime`) rather than raising or silently + repairing it, and is exact across the whole protobuf range, where this + function loses sub-second precision far from the epoch. It always + returns UTC; call [`astimezone()`][datetime.datetime.astimezone] on the + result instead of passing `tz`. + Args: ts: The Timestamp object to convert. tz: The timezone to use for the datetime. Returns: The Timestamp converted to a datetime. + + Raises: + ValueError: If the timestamp has no [`datetime`][datetime.datetime] + equivalent in `tz`. + OverflowError: If the timestamp is so far from the epoch that the + conversion itself overflows. + OSError: If the underlying platform call fails. """ # Add microseconds and add nanoseconds converted to microseconds microseconds = int(ts.nanos / 1000) diff --git a/tests/proto/test_datetime.py b/tests/proto/test_datetime.py index fd0f0c06..eb2b82d8 100644 --- a/tests/proto/test_datetime.py +++ b/tests/proto/test_datetime.py @@ -15,7 +15,11 @@ from hypothesis import strategies as st from frequenz.client.common import InvalidDatetime -from frequenz.client.common.proto import datetime_from_proto2, datetime_to_proto +from frequenz.client.common.proto import ( + datetime_from_proto, + datetime_from_proto2, + datetime_to_proto, +) # The oldest and newest instants both protobuf and Python can represent. _MIN_SECONDS = -62135596800 # 0001-01-01T00:00:00Z @@ -76,6 +80,22 @@ def test_no_none_datetime(dt: datetime) -> None: assert ts2 is None +def test_from_proto_is_deprecated() -> None: + """`datetime_from_proto` warns and still converts as it always did.""" + with pytest.deprecated_call( + match=r"`datetime_from_proto` is deprecated; use `datetime_from_proto2` " + r"\(returns `datetime \| InvalidDatetime`\) instead\." + ): + converted = datetime_from_proto(Timestamp(seconds=1, nanos=500000000)) + assert converted == datetime(1970, 1, 1, 0, 0, 1, 500000, tzinfo=timezone.utc) + + +def test_from_proto_still_raises_out_of_range() -> None: + """`datetime_from_proto` keeps raising, which is why it is deprecated.""" + with pytest.deprecated_call(), pytest.raises((ValueError, OverflowError)): + datetime_from_proto(Timestamp(seconds=_MAX_SECONDS + 1)) + + def test_from_proto2_epoch() -> None: """An all-zero timestamp is the Unix epoch in UTC.""" assert datetime_from_proto2(Timestamp()) == datetime( From ccd5563a0a739d108258b495077c5ddcda40191a Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Thu, 27 Aug 2026 12:34:26 +0000 Subject: [PATCH 10/11] Document `InvalidDatetime` in the guides The three guides were written against the wrapper surface as it was, so they describe `Location` as the only type with per-field invalid wrappers and list no invalid timestamp at all. Now that wrapper timestamps are unions, a reader following the User Guide would meet an `InvalidDatetime` the guide never mentions. Add it to the wrapper overview, and to the User Guide page on validity with a worked example of both ways to read such a field. Add it to the string-output page too, which enumerates the types using the `` marker. The Wrapping Guide gains the rule the timestamp wrapper demonstrates: when the same field appears in several types, write one field wrapper and one error for all of them, and put it where the thing it wraps belongs. For a well-known protobuf type, that is the top-level package rather than a versioned domain. Signed-off-by: Leandro Lucarella --- docs/user-guide/overview.md | 7 ++++ docs/user-guide/reading-string-output.md | 6 +++ docs/user-guide/validity-in-the-type.md | 42 +++++++++++++++++++++ docs/wrapping-guide/validity-in-the-type.md | 11 ++++++ 4 files changed, 66 insertions(+) diff --git a/docs/user-guide/overview.md b/docs/user-guide/overview.md index 92a86e2f..ed238732 100644 --- a/docs/user-guide/overview.md +++ b/docs/user-guide/overview.md @@ -4,6 +4,13 @@ The library groups its wrappers by the kind of common API data you receive. Use this map to find the relevant domain, then follow its links for the public API details. +## Utilities + +[`InvalidDatetime`][frequenz.client.common.InvalidDatetime] is not part of +`frequenz-api-common` so it lives in the top-level package. It preserves a wire +timestamp that is not well-formed and can appear in any wrapper where a +[`datetime`][datetime.datetime] is expected. + ## Grid Start with [`DeliveryArea`][frequenz.client.common.grid.DeliveryArea] and diff --git a/docs/user-guide/reading-string-output.md b/docs/user-guide/reading-string-output.md index ff674825..8a9f4dbd 100644 --- a/docs/user-guide/reading-string-output.md +++ b/docs/user-guide/reading-string-output.md @@ -23,6 +23,12 @@ and [`InvalidCountryCode`][frequenz.client.common.types.InvalidCountryCode] use the same marker. See [validity in the type](validity-in-the-type.md) to handle these values. +[`InvalidDatetime`][frequenz.client.common.InvalidDatetime] prints both raw +numbers of a malformed wire timestamp inside the marker, as +``. The nanosecond part always carries its sign, +because a fraction outside `[0, 999999999]` is one of the two reasons the +timestamp is there at all. + An unexpected raw number without `` means something different: the data is well-formed, but this client version does not recognize it yet. For example, [`UnrecognizedElectricalComponent`][frequenz.client.common.microgrid.electrical_components.UnrecognizedElectricalComponent] diff --git a/docs/user-guide/validity-in-the-type.md b/docs/user-guide/validity-in-the-type.md index d037987f..5d8f37f4 100644 --- a/docs/user-guide/validity-in-the-type.md +++ b/docs/user-guide/validity-in-the-type.md @@ -66,6 +66,48 @@ latitude or raises [`InvalidLatitudeError`][frequenz.client.common.types.InvalidLatitudeError], whose [`InvalidLatitudeError.value`][frequenz.client.common.types.InvalidLatitudeError.value] is the raw value. +Timestamps work the same way. A protobuf `Timestamp` counts seconds and +nanoseconds as plain integers, so a message can carry values the `Timestamp` +contract does not allow: an instant outside the years 1 to 9999, or a +nanosecond fraction outside `[0, 999999999]`. Every wrapper timestamp field is +therefore `datetime | `[`InvalidDatetime`][frequenz.client.common.InvalidDatetime], +with [`InvalidDatetime.seconds`][frequenz.client.common.InvalidDatetime.seconds] +and [`InvalidDatetime.nanos`][frequenz.client.common.InvalidDatetime.nanos] +holding exactly what the server sent: + +```python +from datetime import datetime + +from frequenz.client.common.metrics import BoundsSet, Metric, MetricSample +from frequenz.client.common import InvalidDatetime, InvalidDatetimeError + +sample = MetricSample( + sample_time2=InvalidDatetime(seconds=253402300800, nanos=0), + metric=Metric.AC_POWER_ACTIVE, + value=42.0, + bounds_set=BoundsSet(), +) + +match sample.sample_time2: + case InvalidDatetime(seconds=raw_seconds): + print(raw_seconds) # 253402300800 + case datetime() as sample_time: + print(sample_time.isoformat()) + +try: + sample.get_sample_time() +except InvalidDatetimeError as error: + print(error.attr_name) # sample_time2 + print(error.datetime) # +``` + +A [`Lifetime`][frequenz.client.common.microgrid.Lifetime] is the exception: its +two ends are plain `datetime | None`, because a period whose ends cannot be +ordered is not a usable period. Only +[`InvalidLifetime`][frequenz.client.common.microgrid.InvalidLifetime] can hold +an [`InvalidDatetime`][frequenz.client.common.InvalidDatetime], and a malformed +wire timestamp gives you one of those instead. + Some wrapper types use a dedicated subclass for a value that is invalid, unspecified, or unrecognized. For a battery, that can be [`UnspecifiedBattery`][frequenz.client.common.microgrid.electrical_components.UnspecifiedBattery] diff --git a/docs/wrapping-guide/validity-in-the-type.md b/docs/wrapping-guide/validity-in-the-type.md index 6be58808..ab8de2d6 100644 --- a/docs/wrapping-guide/validity-in-the-type.md +++ b/docs/wrapping-guide/validity-in-the-type.md @@ -58,6 +58,17 @@ with [`InvalidLatitude`][frequenz.client.common.types.InvalidLatitude], [`InvalidCountryCode`][frequenz.client.common.types.InvalidCountryCode]. Each wrapper keeps the raw value while leaving the other fields usable. +Reuse an existing field wrapper when several types have the same field. Every +wrapper timestamp is `datetime | InvalidDatetime` because a protobuf +`Timestamp` can break its own contract no matter which message it arrives in. +One wrapper and one +[`InvalidDatetimeError`][frequenz.client.common.InvalidDatetimeError] mean a +caller learns the pattern once. A field wrapper is protobuf-independent, so it +belongs in a public type module, not next to the conversion function that +produces it. When, it wraps no `frequenz-api-common` message at all, like a +timestamp, put it directly in the top-level package or an utility-specific +module, do not mix it with a domain-specific module. + ## Represent protobuf recovery as a subtype When the class identifies a protobuf category or type, use dedicated subclasses From 90897ccb956b5efaa7da62c136adacbe67046920 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Thu, 27 Aug 2026 12:34:40 +0000 Subject: [PATCH 11/11] Update release notes Signed-off-by: Leandro Lucarella --- RELEASE_NOTES.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 44b3c0c0..b022dc72 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -91,6 +91,16 @@ Migrate to `bounds_set` (or `get_bounds_set()`) for all of these. +* `frequenz.client.common.proto.datetime_from_proto` is now deprecated; use `datetime_from_proto2` instead. + +* `frequenz.client.common.metrics.MetricSample.sample_time` is now a deprecated read-only property; use the new `sample_time2` field instead. + + The field became `datetime | InvalidDatetime`, which the released `datetime` annotation cannot express, so it was renamed. Reading `sample_time` still returns a `datetime` and now emits a `DeprecationWarning`; for a malformed wire timestamp it raises `InvalidDatetimeError` (a `ValueError`) rather than returning a repaired value. `get_sample_time()` does the same without the warning. + + Constructing with `sample_time=` is **not** deprecated and keeps working: it accepts a well-formed `datetime` today and will accept the wider type once `sample_time2` is renamed back to `sample_time`. Use `sample_time2=` to build a sample from a malformed wire timestamp. + + Because `sample_time` is no longer a real field, `dataclasses.fields()`, `asdict()`, `astuple()` and `replace()` see `sample_time2`. + * `frequenz.client.common.metrics.proto.v1alpha8.metric_sample_from_proto_with_issues` no longer drops invalid bounds or reports them as a major issue. Malformed bounds are now preserved in the returned `MetricSample.bounds_set` as an `InvalidBoundsSet` (validity is encoded in the type), so the previous "bounds for ... is invalid, ignoring these bounds" major issue is no longer produced. @@ -136,8 +146,15 @@ * `frequenz.client.common.metrics.MetricConnection.get_category()` * `frequenz.client.common.metrics.MetricSample.get_metric()` * `frequenz.client.common.metrics.MetricSample.get_bounds_set()` + * `frequenz.client.common.metrics.MetricSample.get_sample_time()` * `frequenz.client.common.microgrid.electrical_components.ElectricalComponent.get_metric_config_bounds()` +* Added `frequenz.client.common.InvalidDatetime`, a protobuf-independent wrapper preserving the raw `seconds` and `nanos` of a wire timestamp that is not a well-formed protobuf `Timestamp`, and `frequenz.client.common.InvalidDatetimeError`, raised by the safe accessors for those fields. Both are exported from the top-level package, not from `frequenz.client.common.types`, because a timestamp is not a `frequenz-api-common` message. See the Upgrading section for the fields that can now hold one. + +* Added `frequenz.client.common.metrics.MetricSample.sample_time2`, typed `datetime | InvalidDatetime`, replacing the now-deprecated `sample_time` property (see Upgrading). + +* Added `frequenz.client.common.proto.datetime_from_proto2` returning `datetime | InvalidDatetime`. This is the replacement for the now-deprecated `datetime_from_proto`. + * Added new delivery-area class hierarchy: * `frequenz.client.common.grid.BaseDeliveryArea` — abstract common supertype of the two concrete leaves; not directly instantiable.