MSC4140 + MSC4354: Update sticky delayed event arg (#20378)

- For the dedicated delayed event scheduling endpoint, drop support for
setting the sticky duration in a query parameter, as MSC4354 is slated
to use only a body parameter for that
- Restrict the sticky duration for delayed events to being strictly
positive, analogously to #20350

changelog.d/20378.feature

@@ -0,0 +1 @@
+Update support for scheduling [MSC4140](https://github.com/matrix-org/matrix-spec-proposals/pull/4140) delayed events with a [MSC4354](https://github.com/matrix-org/matrix-spec-proposals/pull/4354) sticky duration.

synapse/rest/client/room.py

@@ -30,7 +30,8 @@ from urllib import parse as urlparse
 
 import attr
 from prometheus_client.core import Histogram
-from pydantic import Field, PositiveInt, StrictStr
+from pydantic import Field, PositiveInt, StrictStr, model_validator
+from typing_extensions import Self
 
 from twisted.web.server import Request
 
@@ -563,11 +564,16 @@ class RoomDelayedEventRestServlet(TransactionRestServlet):
         state_key: StrictStr | AbsentType = Absent
 
     class StickyDelayedEventBodyModel(DelayedEventBodyModel):
-        # MSC4354 allows any integer value in the request, not just positives
-        sticky_duration_ms: int | AbsentType = Field(
+        sticky_duration_ms: PositiveInt | AbsentType = Field(
             Absent, alias=StickyEvent.REQUEST_PARAM_NAME
         )
 
+        @model_validator(mode="after")
+        def validate_sticky(self) -> Self:
+            if self.state_key is not Absent and self.sticky_duration_ms is not Absent:
+                raise ValueError("Cannot specify both state_key and sticky_duration_ms")
+            return self
+
     async def _do(
         self,
         request: SynapseRequest,
@@ -585,42 +591,18 @@ class RoomDelayedEventRestServlet(TransactionRestServlet):
             )
             sticky_duration_ms = None
         else:
-            # Support the parameter in both the query & body until the MSC settles on which one to use
             request_body = parse_and_validate_json_object_from_request(
                 request, self.StickyDelayedEventBodyModel
             )
-            sticky_duration_ms = parse_integer(
-                request,
-                StickyEvent.REQUEST_PARAM_NAME,
-                negative=True,  # MSC4354 allows any integer value in the request, not just positives
+            sticky_duration_ms = (
+                request_body.sticky_duration_ms
+                if request_body.sticky_duration_ms is not Absent
+                else None
             )
-            if request_body.sticky_duration_ms is not Absent:
-                if sticky_duration_ms is None:
-                    sticky_duration_ms = request_body.sticky_duration_ms
-                elif request_body.sticky_duration_ms != sticky_duration_ms:
-                    raise SynapseError(
-                        HTTPStatus.BAD_REQUEST,
-                        f"Conflicting values given for {StickyEvent.REQUEST_PARAM_NAME}",
-                        Codes.INVALID_PARAM,
-                    )
 
-            if sticky_duration_ms is not None and sticky_duration_ms <= 0:
-                raise SynapseError(
-                    HTTPStatus.BAD_REQUEST,
-                    f"{StickyEvent.REQUEST_PARAM_NAME} must be strictly positive",
-                    Codes.INVALID_PARAM,
-                )
-
-        if request_body.state_key is not Absent:
-            if sticky_duration_ms is not None:
-                raise SynapseError(
-                    HTTPStatus.BAD_REQUEST,
-                    "Sticky state events are not allowed. Cannot specify both `state_key` and `sticky_duration_ms`",
-                    Codes.INVALID_PARAM,
-                )
-            state_key = request_body.state_key
-        else:
-            state_key = None
+        state_key = (
+            request_body.state_key if request_body.state_key is not Absent else None
+        )
 
         origin_server_ts = None
         if requester.app_service_id:

tests/rest/client/test_delayed_events.py

@@ -30,7 +30,6 @@ from synapse.api.constants import (
     EventUnsignedContentFields,
     GuestAccess,
     Membership,
-    StickyEvent,
 )
 from synapse.api.errors import Codes, NotFoundError
 from synapse.appservice import ApplicationService
@@ -1514,16 +1513,7 @@ class DelayedStickyEventsTestCase(DelayedEventsTestCaseBase):
         config["experimental_features"] = {"msc4354_enabled": True}
         return config
 
-    @parameterized.expand(
-        (
-            (False, True),
-            (True, False),
-            (True, True),
-        )
-    )
-    def test_delayed_sticky_event_is_sent_with_sticky_duration(
-        self, param_in_body: bool, param_in_query: bool
-    ) -> None:
+    def test_delayed_sticky_event_is_sent_with_sticky_duration(self) -> None:
         """Test that the sticky duration given when scheduling a delayed event
         is applied to the event once it is sent."""
         path = self.helper.build_delayed_event_request_path(
@@ -1532,13 +1522,9 @@ class DelayedStickyEventsTestCase(DelayedEventsTestCaseBase):
         )
         body = self.helper.build_delayed_event_request_body(
             delay=Duration(milliseconds=900),
+            sticky_duration=Duration(minutes=1),
             content={"body": "sticky"},
         )
-        sticky_duration = Duration(minutes=1)
-        if param_in_body:
-            body[StickyEvent.REQUEST_PARAM_NAME] = sticky_duration.as_millis()
-        if param_in_query:
-            path += f"?{StickyEvent.REQUEST_PARAM_NAME}={sticky_duration.as_millis()}"
         channel = self.make_request("PUT", path, body, self.user1_access_token)
         self.assertEqual(HTTPStatus.OK, channel.code, channel.result)
         delay_id = channel.json_body.get("delay_id")
@@ -1557,8 +1543,7 @@ class DelayedStickyEventsTestCase(DelayedEventsTestCaseBase):
             event,
         )
 
-    @parameterized.expand((False, True))
-    def test_delayed_sticky_state_event(self, param_in_body: bool) -> None:
+    def test_delayed_sticky_state_event(self) -> None:
         """Test that delayed event requests forbid scheduling a sticky state event."""
         path = self.helper.build_delayed_event_request_path(
             room_id=self.room_id,
@@ -1566,24 +1551,15 @@ class DelayedStickyEventsTestCase(DelayedEventsTestCaseBase):
         )
         body = self.helper.build_delayed_event_request_body(
             delay=Duration(milliseconds=900),
+            sticky_duration=Duration(minutes=1),
             content={"topic": "This is a topic"},
             state_key="",
         )
-        sticky_duration = Duration(minutes=1)
-        if param_in_body:
-            body[StickyEvent.REQUEST_PARAM_NAME] = sticky_duration.as_millis()
-        else:
-            path += f"?{StickyEvent.REQUEST_PARAM_NAME}={sticky_duration.as_millis()}"
         channel = self.make_request("PUT", path, body, self.user1_access_token)
         self.assertEqual(HTTPStatus.BAD_REQUEST, channel.code, channel.result)
         self.assertEqual(Codes.INVALID_PARAM, channel.json_body["errcode"])
 
-    @parameterized.expand(
-        ((Duration(days=1), param_in_body) for param_in_body in (False, True))
-    )
-    def test_delayed_sticky_event_with_extreme_duration(
-        self, sticky_duration: Duration, param_in_body: bool
-    ) -> None:
+    def test_delayed_sticky_event_with_extreme_duration(self) -> None:
         """Test that delayed event requests permit a very long sticky duration.
         (The actual sticky duration is clamped at event send time.)"""
         path = self.helper.build_delayed_event_request_path(
@@ -1592,29 +1568,17 @@ class DelayedStickyEventsTestCase(DelayedEventsTestCaseBase):
         )
         body = self.helper.build_delayed_event_request_body(
             delay=Duration(milliseconds=900),
+            sticky_duration=Duration(days=1),
             content={"body": "sticky"},
         )
-        if param_in_body:
-            body[StickyEvent.REQUEST_PARAM_NAME] = sticky_duration.as_millis()
-        else:
-            path += f"?{StickyEvent.REQUEST_PARAM_NAME}={sticky_duration.as_millis()}"
         channel = self.make_request("PUT", path, body, self.user1_access_token)
         self.assertEqual(HTTPStatus.OK, channel.code, channel.result)
         delay_id = channel.json_body.get("delay_id")
         assert delay_id is not None
 
-    @parameterized.expand(
-        (
-            (sticky_duration, param_in_body)
-            for sticky_duration in (
-                Duration(),
-                Duration(seconds=-2000),
-            )
-            for param_in_body in (False, True)
-        )
-    )
+    @parameterized.expand((Duration(), Duration(seconds=-2000)))
     def test_delayed_sticky_event_require_positive_duration(
-        self, sticky_duration: Duration, param_in_body: bool
+        self, sticky_duration: Duration
     ) -> None:
         """Test that delayed event requests forbid a zero or negative sticky duration."""
         path = self.helper.build_delayed_event_request_path(
@@ -1623,32 +1587,16 @@ class DelayedStickyEventsTestCase(DelayedEventsTestCaseBase):
         )
         body = self.helper.build_delayed_event_request_body(
             delay=Duration(milliseconds=900),
+            sticky_duration=sticky_duration,
             content={"body": "sticky"},
         )
-        if param_in_body:
-            body[StickyEvent.REQUEST_PARAM_NAME] = sticky_duration.as_millis()
-        else:
-            path += f"?{StickyEvent.REQUEST_PARAM_NAME}={sticky_duration.as_millis()}"
         channel = self.make_request("PUT", path, body, self.user1_access_token)
         self.assertEqual(channel.code, HTTPStatus.BAD_REQUEST, channel.result)
-        self.assertEqual(channel.json_body["errcode"], Codes.INVALID_PARAM)
-
-    def test_delayed_sticky_event_with_conflicting_sticky_durations(self) -> None:
-        """Test that delayed event requests reject having a different sticky duration
-        in the query parameter & request body."""
-        path = self.helper.build_delayed_event_request_path(
-            room_id=self.room_id,
-            event_type=_EVENT_TYPE,
-        )
-        body = self.helper.build_delayed_event_request_body(
-            delay=Duration(milliseconds=900),
-            content={"body": "sticky"},
-        )
-        path += f"?{StickyEvent.REQUEST_PARAM_NAME}=10000"
-        body[StickyEvent.REQUEST_PARAM_NAME] = 20000
-        channel = self.make_request("PUT", path, body, self.user1_access_token)
-        self.assertEqual(HTTPStatus.BAD_REQUEST, channel.code, channel.result)
-        self.assertEqual(Codes.INVALID_PARAM, channel.json_body["errcode"])
+        # Fails with BAD_JSON instead of INVALID_PARAM because the invalid value
+        # actually causes two ValidationErrors (as the value is neither of the
+        # two accepted types of PositiveInt or Absent), and the function for
+        # request body validation uses BAD_JSON when multiple errors are raised.
+        self.assertEqual(channel.json_body["errcode"], Codes.BAD_JSON)
 
 
 class DelayedEventsWorkerTestCase(

tests/rest/client/utils.py

@@ -44,7 +44,7 @@ import attr
 from twisted.internet.testing import MemoryReactorClock
 from twisted.web.server import Site
 
-from synapse.api.constants import EventTypes, Membership, ReceiptTypes
+from synapse.api.constants import EventTypes, Membership, ReceiptTypes, StickyEvent
 from synapse.api.errors import Codes
 from synapse.server import HomeServer
 from synapse.types import JsonDict
@@ -654,6 +654,7 @@ class RestHelper:
         delay: Duration,
         content: JsonDict,
         state_key: str | None = None,
+        sticky_duration: Duration | None = None,
     ) -> JsonDict:
         """Build the request body for scheduling a delayed event via the dedicated endpoint."""
         body = {
@@ -662,6 +663,8 @@ class RestHelper:
         }
         if state_key is not None:
             body["state_key"] = state_key
+        if sticky_duration is not None:
+            body[StickyEvent.REQUEST_PARAM_NAME] = sticky_duration.as_millis()
         return body
 
     def upload_media(