From 20dde7633337d686993b8c366588c0499ce3bf6e Mon Sep 17 00:00:00 2001 From: cwasicki <126617870+cwasicki@users.noreply.github.com> Date: Tue, 29 Sep 2026 18:08:58 +0200 Subject: [PATCH 1/3] Reject timezone-naive start/end times in the client A naive datetime is ambiguous: the wire encoding assumes UTC while datetime.timestamp() assumes local time, so the same value would denote different instants. Reject it at the public request boundary rather than silently pick one meaning. Signed-off-by: cwasicki <126617870+cwasicki@users.noreply.github.com> --- README.md | 20 +++++------ RELEASE_NOTES.md | 2 +- src/frequenz/client/reporting/_client.py | 44 +++++++++++++++++++++--- tests/test_client_reporting.py | 32 +++++++++++++++++ 4 files changed, 82 insertions(+), 16 deletions(-) diff --git a/README.md b/README.md index c0531b0..e9e7187 100644 --- a/README.md +++ b/README.md @@ -82,8 +82,8 @@ data = [ Metric.AC_ACTIVE_POWER, # AC active power Metric.AC_REACTIVE_POWER, # AC reactive power ], - start_time=datetime.fromisoformat("2024-05-01T00:00:00"), # Start of query range (UTC) - end_time=datetime.fromisoformat("2024-05-02T00:00:00"), # End of query range (UTC) + start_time=datetime.fromisoformat("2024-05-01T00:00:00+00:00"), # Start of query range (UTC) + end_time=datetime.fromisoformat("2024-05-02T00:00:00+00:00"), # End of query range (UTC) resampling_period=timedelta(seconds=5), # Optional: downsample data to 5-second intervals ) ] @@ -101,8 +101,8 @@ data = [ microgrid_id=1, sensor_id=100, metrics=[Metric.SENSOR_IRRADIANCE], - start_time=datetime.fromisoformat("2024-05-01T00:00:00"), - end_time=datetime.fromisoformat("2024-05-02T00:00:00"), + start_time=datetime.fromisoformat("2024-05-01T00:00:00+00:00"), + end_time=datetime.fromisoformat("2024-05-02T00:00:00+00:00"), resampling_period=timedelta(seconds=1), ) ] @@ -130,8 +130,8 @@ data = [ client.receive_microgrid_components_data( microgrid_components=microgrid_components, metrics=[Metric.AC_ACTIVE_POWER, Metric.AC_REACTIVE_POWER], - start_time=datetime.fromisoformat("2024-05-01T00:00:00"), - end_time=datetime.fromisoformat("2024-05-02T00:00:00"), + start_time=datetime.fromisoformat("2024-05-01T00:00:00+00:00"), + end_time=datetime.fromisoformat("2024-05-02T00:00:00+00:00"), resampling_period=timedelta(seconds=1), include_states=False, # Set to True to include state data include_bounds=False, # Set to True to include metric bounds data @@ -160,8 +160,8 @@ data = [ client.receive_microgrid_sensors_data( microgrid_sensors=microgrid_sensors, metrics=[Metric.SENSOR_IRRADIANCE], - start_time=datetime.fromisoformat("2024-05-01T00:00:00"), - end_time=datetime.fromisoformat("2024-05-02T00:00:00"), + start_time=datetime.fromisoformat("2024-05-01T00:00:00+00:00"), + end_time=datetime.fromisoformat("2024-05-02T00:00:00+00:00"), resampling_period=timedelta(seconds=1), include_states=False, # Set to True to include state data ) @@ -183,8 +183,8 @@ data = [ microgrid_id=microgrid_id, metric=Metric.AC_ACTIVE_POWER, aggregation_formula=formula, - start_time=datetime.fromisoformat("2024-05-01T00:00:00"), - end_time=datetime.fromisoformat("2024-05-02T00:00:00"), + start_time=datetime.fromisoformat("2024-05-01T00:00:00+00:00"), + end_time=datetime.fromisoformat("2024-05-02T00:00:00+00:00"), resampling_period=resampling_period, ) ] diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index f80f12e..a943057 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -6,7 +6,7 @@ ## Upgrading - +- `start_time` and `end_time` must now be timezone-aware. The client raises `ValueError` on a naive datetime instead of letting it be read inconsistently (UTC on the wire, local time elsewhere), and the CLI `--start`/`--end` reject a value without an offset. Add an offset such as `+00:00` to existing naive values. ## New Features diff --git a/src/frequenz/client/reporting/_client.py b/src/frequenz/client/reporting/_client.py index 7a8a42f..c265aad 100644 --- a/src/frequenz/client/reporting/_client.py +++ b/src/frequenz/client/reporting/_client.py @@ -63,6 +63,24 @@ ) +def _reject_naive(**times: datetime | None) -> None: + """Raise if any given datetime is timezone-naive. + + A naive datetime is ambiguous: the wire encoding assumes UTC while + `datetime.timestamp()` assumes local time, so the same value would denote + different instants. + + Args: + **times: datetimes to check, keyed by parameter name for the error message. + + Raises: + ValueError: if any value is a naive datetime. + """ + for name, dt in times.items(): + if dt is not None and dt.tzinfo is None: + raise ValueError(f"{name} must be timezone-aware, got naive {dt!r}") + + class ReportingApiClient(BaseApiClient[ReportingStub]): """A client for the Reporting service.""" @@ -145,7 +163,7 @@ def stub(self) -> ReportingStub: return self._stub # pylint: disable=too-many-arguments - def receive_single_component_data( + def receive_single_component_data( # noqa: DOC502 self, *, microgrid_id: int, @@ -171,6 +189,9 @@ def receive_single_component_data( Returns: A receiver of `MetricSample`s. + + Raises: + ValueError: If start_time or end_time is timezone-naive. """ receiver = self._receive_microgrid_components_data_batch( microgrid_components=[(microgrid_id, [component_id])], @@ -185,7 +206,7 @@ def receive_single_component_data( return BatchUnrollReceiver(receiver) # pylint: disable=too-many-arguments - def receive_microgrid_components_data( + def receive_microgrid_components_data( # noqa: DOC502 self, *, microgrid_components: list[tuple[int, list[int]]], @@ -210,6 +231,9 @@ def receive_microgrid_components_data( Returns: A receiver of `MetricSample`s. + + Raises: + ValueError: If start_time or end_time is timezone-naive. """ receiver = self._receive_microgrid_components_data_batch( microgrid_components=microgrid_components, @@ -237,6 +261,7 @@ def _receive_microgrid_components_data_batch( include_bounds: bool = False, ) -> Receiver[ComponentsDataBatch]: """Return a Receiver for the microgrid component data stream.""" + _reject_naive(start_time=start_time, end_time=end_time) stream_key = ( tuple((mid, tuple(cids)) for mid, cids in microgrid_components), tuple(metric.name for metric in metrics), @@ -332,7 +357,7 @@ def stream_method() -> ( return self._components_data_streams[stream_key].new_receiver() # pylint: disable=too-many-arguments - def receive_single_sensor_data( + def receive_single_sensor_data( # noqa: DOC502 self, *, microgrid_id: int, @@ -356,6 +381,9 @@ def receive_single_sensor_data( Returns: A receiver of `MetricSample`s. + + Raises: + ValueError: If start_time or end_time is timezone-naive. """ receiver = self._receive_microgrid_sensors_data_batch( microgrid_sensors=[(microgrid_id, [sensor_id])], @@ -368,7 +396,7 @@ def receive_single_sensor_data( return BatchUnrollReceiver(receiver) # pylint: disable=too-many-arguments - def receive_microgrid_sensors_data( + def receive_microgrid_sensors_data( # noqa: DOC502 self, *, microgrid_sensors: list[tuple[int, list[int]]], @@ -391,6 +419,9 @@ def receive_microgrid_sensors_data( Returns: A receiver of `MetricSample`s. + + Raises: + ValueError: If start_time or end_time is timezone-naive. """ receiver = self._receive_microgrid_sensors_data_batch( microgrid_sensors=microgrid_sensors, @@ -427,6 +458,7 @@ def _receive_microgrid_sensors_data_batch( Returns: A receiver of `SensorsDataBatch`s. """ + _reject_naive(start_time=start_time, end_time=end_time) stream_key = ( tuple((mid, tuple(sids)) for mid, sids in microgrid_sensors), tuple(metric.name for metric in metrics), @@ -538,8 +570,10 @@ def receive_aggregated_data( A receiver of `MetricSample`s. Raises: - ValueError: If the resampling_period is not provided. + ValueError: If the resampling_period is not provided, or if + start_time or end_time is timezone-naive. """ + _reject_naive(start_time=start_time, end_time=end_time) stream_key = ( microgrid_id, metric.name, diff --git a/tests/test_client_reporting.py b/tests/test_client_reporting.py index a6c0471..4fb9ac6 100644 --- a/tests/test_client_reporting.py +++ b/tests/test_client_reporting.py @@ -3,14 +3,17 @@ """Tests for the frequenz.client.reporting package.""" +from datetime import datetime, timezone from unittest.mock import MagicMock, patch import pytest from frequenz.api.reporting.v1alpha10.reporting_pb2_grpc import ReportingStub from frequenz.client.base.channel import ChannelOptions from frequenz.client.base.client import BaseApiClient +from frequenz.client.common.metrics import Metric from frequenz.client.reporting import ReportingApiClient +from frequenz.client.reporting._client import _reject_naive from frequenz.client.reporting._types import ComponentsDataBatch @@ -42,6 +45,35 @@ async def test_client_initialization() -> None: ) +def test_reject_naive_passes_aware_and_none() -> None: + """Test that aware datetimes and None are accepted.""" + _reject_naive( + start_time=datetime(2025, 1, 1, tzinfo=timezone.utc), + end_time=None, + ) + + +@pytest.mark.parametrize("name", ["start_time", "end_time"]) +def test_reject_naive_raises_on_naive(name: str) -> None: + """Test that a naive datetime is rejected, naming the offending parameter.""" + with pytest.raises(ValueError, match=name): + _reject_naive(**{name: datetime(2025, 1, 1)}) # noqa: DTZ001 + + +def test_public_method_rejects_naive() -> None: + """Test that a public request method rejects naive input before any I/O.""" + with patch.object(BaseApiClient, "__init__", return_value=None): + client = ReportingApiClient("grpc://localhost:50051") + with pytest.raises(ValueError, match="start_time"): + client.receive_microgrid_components_data( + microgrid_components=[(1, [2])], + metrics=Metric.AC_POWER_ACTIVE, + start_time=datetime(2025, 1, 1), # noqa: DTZ001 + end_time=None, + resampling_period=None, + ) + + def test_components_data_batch_is_empty_true() -> None: """Test that the is_empty method returns True when the page is empty.""" data_pb = MagicMock() From 5b4c36723ab4a576f677cca3adbd73434abb81af Mon Sep 17 00:00:00 2001 From: cwasicki <126617870+cwasicki@users.noreply.github.com> Date: Tue, 29 Sep 2026 14:37:57 +0200 Subject: [PATCH 2/3] Require timezone-aware --start/--end in CLI A naive datetime is ambiguous: the wire encoding assumes UTC while datetime.timestamp() assumes local time. Reject it at the argparse boundary so the CLI fails with a clear message instead of a ValueError. Signed-off-by: cwasicki <126617870+cwasicki@users.noreply.github.com> --- src/frequenz/client/reporting/cli/__main__.py | 22 +++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/src/frequenz/client/reporting/cli/__main__.py b/src/frequenz/client/reporting/cli/__main__.py index e1c5884..c576280 100644 --- a/src/frequenz/client/reporting/cli/__main__.py +++ b/src/frequenz/client/reporting/cli/__main__.py @@ -16,6 +16,20 @@ from frequenz.client.reporting._types import MetricSample +def _aware_datetime(value: str) -> datetime: + """Parse an ISO datetime, rejecting naive input. + + A naive datetime is ambiguous: the wire encoding assumes UTC while + datetime.timestamp() assumes local time. + """ + parsed = datetime.fromisoformat(value) + if parsed.tzinfo is None: + raise argparse.ArgumentTypeError( + "datetime must include a timezone offset, e.g. 2024-05-01T00:00:00+00:00" + ) + return parsed + + def main() -> None: """Parse arguments and run the client.""" parser = argparse.ArgumentParser() @@ -58,15 +72,15 @@ def main() -> None: ) parser.add_argument( "--start", - type=datetime.fromisoformat, - help="Start datetime in YYYY-MM-DDTHH:MM:SS format", + type=_aware_datetime, + help="Start datetime in ISO format with a timezone offset", required=False, default=None, ) parser.add_argument( "--end", - type=datetime.fromisoformat, - help="End datetime in YYYY-MM-DDTHH:MM:SS format", + type=_aware_datetime, + help="End datetime in ISO format with a timezone offset", required=False, default=None, ) From 903b80f4859fe7a5b5733bb37365daed424d5ac0 Mon Sep 17 00:00:00 2001 From: cwasicki <126617870+cwasicki@users.noreply.github.com> Date: Tue, 29 Sep 2026 14:38:07 +0200 Subject: [PATCH 3/3] Warn in CLI when --end precedes --start An inverted window is a legal but empty range, so the request still runs; at the CLI it is almost always a typo, and the silent empty output gives no hint why. Signed-off-by: cwasicki <126617870+cwasicki@users.noreply.github.com> --- RELEASE_NOTES.md | 2 +- src/frequenz/client/reporting/cli/__main__.py | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index a943057..15f6b0f 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -10,7 +10,7 @@ ## New Features - +- The CLI now warns when `--end` is before `--start`. The request still runs, but the inverted range returns no data, so the warning flags what is almost always a typo. ## Bug Fixes diff --git a/src/frequenz/client/reporting/cli/__main__.py b/src/frequenz/client/reporting/cli/__main__.py index c576280..0a68c0e 100644 --- a/src/frequenz/client/reporting/cli/__main__.py +++ b/src/frequenz/client/reporting/cli/__main__.py @@ -107,6 +107,13 @@ def main() -> None: default=None, ) args = parser.parse_args() + # An inverted window is a legal but empty range; warn since at the CLI it is + # almost always a typo. + if args.start and args.end and args.end < args.start: + print( + "warning: --end is before --start; no data will be returned", + file=sys.stderr, + ) asyncio.run( run( microgrid_id=args.mid,