diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index a98dc6f..f3efc57 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -2,8 +2,7 @@ ## Summary -Add generic API credential fallbacks to `dispatch-cli`, add steam-boiler dispatch -targets, and reject invalid `start_time` values passed to `DispatchApiClient.create()`. + ## Upgrading @@ -13,13 +12,9 @@ targets, and reject invalid `start_time` values passed to `DispatchApiClient.cre -- `TargetCategories` now supports the `STEAM_BOILER` component category. This - requires `frequenz-client-common` 0.3.8 or newer. The minimum supported - `grpcio` version is now 1.80.0, as required by its `frequenz-api-common` - dependency. -- `dispatch-cli` accepts `FREQUENZ_API_KEY` and `FREQUENZ_API_SECRET` as a - fallback pair for `DISPATCH_API_AUTH_KEY` and `DISPATCH_API_SIGN_SECRET`. - ## Bug Fixes -- `DispatchApiClient.create()`: Passing an invalid `start_time` (not a `datetime` or `"NOW"`) previously silently created a dispatch with an epoch timestamp (1970-01-01). It now raises `ValueError` immediately. + + +- `FakeService`: An update whose field mask referenced an unknown path was silently ignored, while the real service rejects it. It now raises an `INVALID_ARGUMENT` error too, so tests can no longer pass against updates that would fail in production. Tests relying on the old behaviour need to drop the unknown paths. +- `FakeService`: An update whose field mask contained a bare `recurrence` path, which the real service accepts to replace the whole recurrence rule, crashed with an `IndexError`. It now replaces the rule. diff --git a/src/frequenz/client/dispatch/_client.py b/src/frequenz/client/dispatch/_client.py index 9381c4e..6ffd443 100644 --- a/src/frequenz/client/dispatch/_client.py +++ b/src/frequenz/client/dispatch/_client.py @@ -382,7 +382,7 @@ async def update( # pylint: disable=too-many-branches The `new_fields` argument is a dictionary of fields to update. The keys are the field names, and the values are the new values for the fields. - For recurrence fields, the keys are preceeded by "recurrence.". + For recurrence fields, the keys are preceded by "recurrence.". Note that updating `type` and `dry_run` is not supported. @@ -395,7 +395,8 @@ async def update( # pylint: disable=too-many-branches Dispatch: The updated dispatch. Raises: - ValueError: If updating `type` or `dry_run`. + ValueError: If `new_fields` contains a key that is not an updatable + field, which includes `type` and `dry_run`. """ msg = UpdateMicrogridDispatchRequest( dispatch_id=int(dispatch_id), microgrid_id=int(microgrid_id) diff --git a/src/frequenz/client/dispatch/test/_service.py b/src/frequenz/client/dispatch/test/_service.py index 14fe86f..e5916bc 100644 --- a/src/frequenz/client/dispatch/test/_service.py +++ b/src/frequenz/client/dispatch/test/_service.py @@ -300,6 +300,28 @@ async def UpdateMicrogridDispatch( getattr(pb_dispatch.data, split_path[0]).CopyFrom( getattr(request.update, split_path[0]) ) + # The whole recurrence rule, as opposed to the "recurrence." + # paths handled below. `RecurrenceRuleUpdate` is a distinct message + # from `RecurrenceRule`, so its fields have to be copied one by one. + case "recurrence" if len(split_path) == 1: + recurrence_update = request.update.recurrence + pb_recurrence = pb_dispatch.data.recurrence + pb_recurrence.freq = recurrence_update.freq + pb_recurrence.interval = recurrence_update.interval + # Copying an unset `end_criteria` would leave it present but + # empty, which reads back as an end criteria with neither a + # count nor an end time, rather than no end criteria at all. + if recurrence_update.HasField("end_criteria"): + pb_recurrence.end_criteria.CopyFrom( + recurrence_update.end_criteria + ) + else: + pb_recurrence.ClearField("end_criteria") + pb_recurrence.byminutes[:] = recurrence_update.byminutes + pb_recurrence.byhours[:] = recurrence_update.byhours + pb_recurrence.byweekdays[:] = recurrence_update.byweekdays + pb_recurrence.bymonthdays[:] = recurrence_update.bymonthdays + pb_recurrence.bymonths[:] = recurrence_update.bymonths case "recurrence": match split_path[1]: case "end_criteria": @@ -325,15 +347,27 @@ async def UpdateMicrogridDispatch( ) case _: # `split_path[1]` is an arbitrary string, so mypy can - # never consider the cases above exhaustive. Paths that - # don't match a known recurrence field are ignored, same - # as unrecognized top-level paths below. - pass + # never consider the cases above exhaustive. The real + # service rejects unknown recurrence paths, so the fake + # must too, or tests would pass against updates that + # fail in production. + error = grpc.RpcError() + # pylint: disable=protected-access + error._code = grpc.StatusCode.INVALID_ARGUMENT # type: ignore + error._details = f"Invalid recurrence path: {path}" # type: ignore + # pylint: enable=protected-access + raise error case _: # `split_path[0]` is an arbitrary string, so mypy can never - # consider the cases above exhaustive. Unrecognized top-level - # paths are ignored. - pass + # consider the cases above exhaustive. The real service rejects + # unknown paths, so the fake must too, or tests would pass + # against updates that fail in production. + error = grpc.RpcError() + # pylint: disable=protected-access + error._code = grpc.StatusCode.INVALID_ARGUMENT # type: ignore + error._details = "Invalid fields in update_mask" # type: ignore + # pylint: enable=protected-access + raise error dispatch = Dispatch.from_protobuf(pb_dispatch) dispatch = replace( diff --git a/tests/test_proto.py b/tests/test_proto.py index c2efb05..f84c382 100644 --- a/tests/test_proto.py +++ b/tests/test_proto.py @@ -75,6 +75,8 @@ def test_end_criteria() -> None: EndCriteria( count=10, ), + # Neither set: the `count_or_until` oneof stays unset. + EndCriteria(), ): assert EndCriteria.from_protobuf(end_criteria.to_protobuf()) == end_criteria diff --git a/tests/test_service.py b/tests/test_service.py index 02d768d..45304ad 100644 --- a/tests/test_service.py +++ b/tests/test_service.py @@ -5,6 +5,8 @@ from datetime import datetime, timedelta, timezone +import grpc +import pytest from frequenz.api.common.v1alpha8.pagination.pagination_params_pb2 import ( PaginationParams, ) @@ -14,11 +16,12 @@ from frequenz.api.dispatch.v1.dispatch_pb2 import ( DispatchFilter, ListMicrogridDispatchesRequest, + UpdateMicrogridDispatchRequest, ) from frequenz.client.base.conversion import to_timestamp from frequenz.client.common.microgrid import MicrogridId -from frequenz.client.dispatch.recurrence import RecurrenceRule +from frequenz.client.dispatch.recurrence import EndCriteria, Frequency, RecurrenceRule from frequenz.client.dispatch.test._service import FakeService from frequenz.client.dispatch.types import ( Dispatch, @@ -147,3 +150,86 @@ async def test_list_dispatches_filter_end_time() -> None: d.start_time + d.duration < now + timedelta(minutes=5) # type: ignore[operator] for d in filtered_dispatches ) + + +@pytest.mark.parametrize( + "path, expected_details", + [ + ("bogus", "Invalid fields in update_mask"), + ("recurrence.bogus", "Invalid recurrence path: recurrence.bogus"), + ], +) +async def test_update_dispatch_rejects_unknown_mask_path( + path: str, expected_details: str +) -> None: + """Test that unknown update mask paths are rejected, like the real service does.""" + service = FakeService() + now = datetime.now(timezone.utc) + dispatch = Dispatch( + id=DispatchId(1), + start_time=now, + duration=timedelta(minutes=1), + type="test", + target=TargetIds(1), + active=True, + dry_run=False, + payload={}, + recurrence=RecurrenceRule(), + create_time=now, + update_time=now, + ) + service.dispatches[MicrogridId(1)] = [dispatch] + + req = UpdateMicrogridDispatchRequest(microgrid_id=1, dispatch_id=1) + req.update_mask.paths.append(path) + + with pytest.raises(grpc.RpcError) as exc_info: + await service.UpdateMicrogridDispatch(req) + + # pylint: disable=protected-access + assert exc_info.value._code == grpc.StatusCode.INVALID_ARGUMENT # type: ignore + assert exc_info.value._details == expected_details # type: ignore + # pylint: enable=protected-access + + # The stored dispatch must be left untouched. + assert service.dispatches[MicrogridId(1)] == [dispatch] + + +async def test_update_dispatch_whole_recurrence() -> None: + """Test that a bare "recurrence" mask path replaces the whole recurrence rule.""" + service = FakeService() + now = datetime.now(timezone.utc) + dispatch = Dispatch( + id=DispatchId(1), + start_time=now, + duration=timedelta(minutes=1), + type="test", + target=TargetIds(1), + active=True, + dry_run=False, + payload={}, + recurrence=RecurrenceRule( + frequency=Frequency.DAILY, + interval=1, + end_criteria=EndCriteria(count=10), + ), + create_time=now, + update_time=now, + ) + service.dispatches[MicrogridId(1)] = [dispatch] + + new_recurrence = RecurrenceRule( + frequency=Frequency.WEEKLY, interval=3, byhours=[6, 18] + ) + req = UpdateMicrogridDispatchRequest(microgrid_id=1, dispatch_id=1) + req.update.recurrence.freq = new_recurrence.frequency.value + req.update.recurrence.interval = new_recurrence.interval + req.update.recurrence.byhours.extend(new_recurrence.byhours) + req.update_mask.paths.append("recurrence") + + response = await service.UpdateMicrogridDispatch(req) + + updated = Dispatch.from_protobuf(response.dispatch).recurrence + assert updated == new_recurrence + # The replaced rule had an end criteria, the new one does not. + assert updated is not None and updated.end_criteria is None