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..15f6b0f 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -6,11 +6,11 @@ ## 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 - +- 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/_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/src/frequenz/client/reporting/cli/__main__.py b/src/frequenz/client/reporting/cli/__main__.py index e1c5884..0a68c0e 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, ) @@ -93,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, 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()