FakeService: Match the real service on update field mask paths

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()`.
+<!-- Here goes a general summary of what this release is about -->
 
 ## Upgrading
 
@@ -13,13 +12,9 @@ targets, and reject invalid `start_time` values passed to `DispatchApiClient.cre
 
 <!-- Here goes the main new features and examples or instructions on how to use them -->
 
-- `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.
+<!-- Here goes notable bug fixes that are worth a special mention or explanation -->
+
+- `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.

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)

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.<field>"
+                # 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(

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
 

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