From 241dc05caa282c0a688226fc161cb44f8c59174b Mon Sep 17 00:00:00 2001 From: Andrew Ferrazzutti Date: Mon, 2 Nov 2026 16:46:57 -0500 Subject: [PATCH] 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 | 1 + synapse/rest/client/room.py | 50 +++++---------- tests/rest/client/test_delayed_events.py | 80 +++++------------------- tests/rest/client/utils.py | 5 +- 4 files changed, 35 insertions(+), 101 deletions(-) create mode 100644 changelog.d/20378.feature diff --git a/changelog.d/20378.feature b/changelog.d/20378.feature new file mode 100644 index 0000000000..c15f031942 --- /dev/null +++ b/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. diff --git a/synapse/rest/client/room.py b/synapse/rest/client/room.py index b5dcc8902e..7ea2120e78 100644 --- a/synapse/rest/client/room.py +++ b/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: diff --git a/tests/rest/client/test_delayed_events.py b/tests/rest/client/test_delayed_events.py index ecb6639ff6..d86c085978 100644 --- a/tests/rest/client/test_delayed_events.py +++ b/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( diff --git a/tests/rest/client/utils.py b/tests/rest/client/utils.py index fccf8df0a0..a06aef0ec4 100644 --- a/tests/rest/client/utils.py +++ b/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(