From baad503f0208967cc524a81174b8ad18fcce1f02 Mon Sep 17 00:00:00 2001 From: "Mathias L. Baumann" Date: Fri, 18 Sep 2026 15:32:45 +0200 Subject: [PATCH 1/5] Reset release notes for next release v1.1.0 was released from the previous contents; start a clean file for the next version. Signed-off-by: Mathias L. Baumann --- RELEASE_NOTES.md | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index a98dc6f1..72e44a30 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,6 @@ 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. + From 37b1b38aeabbfff4939ed0182f53fe9e14b0e2de Mon Sep 17 00:00:00 2001 From: "Mathias L. Baumann" Date: Fri, 18 Sep 2026 15:32:57 +0200 Subject: [PATCH 2/5] FakeService: Reject unknown paths in update field mask Both match statements over the update mask silently ignored unrecognized paths. The real service rejects them instead: `generate_update_model` returns `Invalid fields in update_mask` when a path went unhandled, and `partial_recurrence_update` returns `Invalid recurrence path: {path}`. That divergence let a test pass against an update that would fail in production, which is the opposite of what a test double is for. Raise `INVALID_ARGUMENT` with the same messages, and drop the comments claiming the silent ignoring was intentional. Signed-off-by: Mathias L. Baumann --- RELEASE_NOTES.md | 2 + src/frequenz/client/dispatch/test/_service.py | 26 ++++++++--- tests/test_service.py | 46 +++++++++++++++++++ 3 files changed, 67 insertions(+), 7 deletions(-) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 72e44a30..22041913 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -15,3 +15,5 @@ ## Bug Fixes + +- `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. diff --git a/src/frequenz/client/dispatch/test/_service.py b/src/frequenz/client/dispatch/test/_service.py index 14fe86f3..adb0a19d 100644 --- a/src/frequenz/client/dispatch/test/_service.py +++ b/src/frequenz/client/dispatch/test/_service.py @@ -325,15 +325,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_service.py b/tests/test_service.py index 02d768d0..c83ef8b4 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,6 +16,7 @@ from frequenz.api.dispatch.v1.dispatch_pb2 import ( DispatchFilter, ListMicrogridDispatchesRequest, + UpdateMicrogridDispatchRequest, ) from frequenz.client.base.conversion import to_timestamp @@ -147,3 +150,46 @@ 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] From 623f0ddf82e6b8d2b6bc1b96cf091ec16f38d562 Mon Sep 17 00:00:00 2001 From: "Mathias L. Baumann" Date: Fri, 18 Sep 2026 15:34:52 +0200 Subject: [PATCH 3/5] FakeService: Handle a bare "recurrence" path in the update field mask The handler for the "recurrence" path went straight to `split_path[1]`, so a bare "recurrence" path raised an `IndexError`. The real service accepts it to replace the whole recurrence rule (`generate_update_model` handles it separately from the "recurrence." paths), so do the same. `RecurrenceRuleUpdate` is a distinct message from `RecurrenceRule`, so the fields are copied one by one. `end_criteria` is cleared rather than copied when unset, as copying it would leave it present but empty, which reads back as an end criteria with no count and no end time. Signed-off-by: Mathias L. Baumann --- RELEASE_NOTES.md | 1 + src/frequenz/client/dispatch/test/_service.py | 22 ++++++++++ tests/test_service.py | 42 ++++++++++++++++++- 3 files changed, 64 insertions(+), 1 deletion(-) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 22041913..f3efc572 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -17,3 +17,4 @@ - `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/test/_service.py b/src/frequenz/client/dispatch/test/_service.py index adb0a19d..e5916bc7 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": diff --git a/tests/test_service.py b/tests/test_service.py index c83ef8b4..45304adb 100644 --- a/tests/test_service.py +++ b/tests/test_service.py @@ -21,7 +21,7 @@ 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, @@ -193,3 +193,43 @@ async def test_update_dispatch_rejects_unknown_mask_path( # 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 From b19f36b3498a4a6a06546724b74cf1cd964dfb73 Mon Sep 17 00:00:00 2001 From: "Mathias L. Baumann" Date: Fri, 18 Sep 2026 15:35:05 +0200 Subject: [PATCH 4/5] tests: Cover an EndCriteria with neither count nor until set `EndCriteria.from_protobuf` handles an unset `count_or_until` oneof, but no test exercised it: the round-trip loop only covered the `count` and `until_time` cases. Signed-off-by: Mathias L. Baumann --- tests/test_proto.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/test_proto.py b/tests/test_proto.py index c2efb056..f84c3823 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 From 8537c7ec81afd751dd81815b957f514bfc8c7ee3 Mon Sep 17 00:00:00 2001 From: "Mathias L. Baumann" Date: Fri, 18 Sep 2026 15:35:53 +0200 Subject: [PATCH 5/5] docs: Fix the update() docstring `update()` raises `ValueError` for any key that is not an updatable field, not only for `type` and `dry_run`. Also fixes a "preceeded" typo. Signed-off-by: Mathias L. Baumann --- src/frequenz/client/dispatch/_client.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/frequenz/client/dispatch/_client.py b/src/frequenz/client/dispatch/_client.py index 9381c4e9..6ffd443d 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)