-
Notifications
You must be signed in to change notification settings - Fork 404
Add experimental support for MSC4308: Thread Subscriptions extension to Sliding Sync when MSC4306 and MSC4186 are enabled. #18695
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
2961006
875dbf7
09f8633
f1f5657
748316c
4dcd12b
0ce5dce
0c310b9
18881b1
4a34641
e72d6cd
f4cd180
921cd53
168b67b
1374895
0cf178a
924c1bf
fa8e3b6
80679a7
00cb14e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,21 +1,39 @@ | ||
| from http import HTTPStatus | ||
| from typing import TYPE_CHECKING, Optional, Tuple | ||
| from typing import TYPE_CHECKING, Dict, Optional, Tuple | ||
|
|
||
| import attr | ||
| from typing_extensions import TypeAlias | ||
|
|
||
| from synapse.api.errors import Codes, NotFoundError, SynapseError | ||
| from synapse.http.server import HttpServer | ||
| from synapse.http.servlet import ( | ||
| RestServlet, | ||
| parse_and_validate_json_object_from_request, | ||
| parse_integer, | ||
| parse_string, | ||
| ) | ||
| from synapse.http.site import SynapseRequest | ||
| from synapse.rest.client._base import client_patterns | ||
| from synapse.types import JsonDict, RoomID | ||
| from synapse.types import ( | ||
| JsonDict, | ||
| RoomID, | ||
| SlidingSyncStreamToken, | ||
| ThreadSubscriptionsToken, | ||
| ) | ||
| from synapse.types.handlers.sliding_sync import SlidingSyncResult | ||
| from synapse.types.rest import RequestBodyModel | ||
| from synapse.util.pydantic_models import AnyEventId | ||
|
|
||
| if TYPE_CHECKING: | ||
| from synapse.server import HomeServer | ||
|
|
||
| _ThreadSubscription: TypeAlias = ( | ||
| SlidingSyncResult.Extensions.ThreadSubscriptionsExtension.ThreadSubscription | ||
| ) | ||
| _ThreadUnsubscription: TypeAlias = ( | ||
| SlidingSyncResult.Extensions.ThreadSubscriptionsExtension.ThreadUnsubscription | ||
| ) | ||
|
|
||
|
|
||
| class ThreadSubscriptionsRestServlet(RestServlet): | ||
| PATTERNS = client_patterns( | ||
|
|
@@ -100,6 +118,129 @@ async def on_DELETE( | |
| return HTTPStatus.OK, {} | ||
|
|
||
|
|
||
| class ThreadSubscriptionsPaginationRestServlet(RestServlet): | ||
| PATTERNS = client_patterns( | ||
| "/io.element.msc4308/thread_subscriptions$", | ||
| unstable=True, | ||
| releases=(), | ||
| ) | ||
| CATEGORY = "Thread Subscriptions requests (unstable)" | ||
|
|
||
| # Maximum number of thread subscriptions to return in one request. | ||
| MAX_LIMIT = 512 | ||
|
|
||
| def __init__(self, hs: "HomeServer"): | ||
| self.auth = hs.get_auth() | ||
| self.is_mine = hs.is_mine | ||
| self.store = hs.get_datastores().main | ||
|
|
||
| async def on_GET(self, request: SynapseRequest) -> Tuple[int, JsonDict]: | ||
| requester = await self.auth.get_user_by_req(request) | ||
|
|
||
| limit = min( | ||
| parse_integer(request, "limit", default=100, negative=False), | ||
| ThreadSubscriptionsPaginationRestServlet.MAX_LIMIT, | ||
| ) | ||
| from_end_opt = parse_string(request, "from", required=False) | ||
| to_start_opt = parse_string(request, "to", required=False) | ||
| _direction = parse_string(request, "dir", required=True, allowed_values=("b",)) | ||
|
|
||
| if limit <= 0: | ||
| raise SynapseError( | ||
| HTTPStatus.BAD_REQUEST, | ||
| "limit must be greater than 0", | ||
| errcode=Codes.INVALID_PARAM, | ||
| ) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. negative still allows There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, that's annoying. Could you add a quick comment noting that this conditional is needed because of that? |
||
|
|
||
| if from_end_opt is not None: | ||
| try: | ||
| # because of backwards pagination, the `from` token is actually the | ||
| # bound closest to the end of the stream | ||
| end_stream_id = ThreadSubscriptionsToken.from_string( | ||
| from_end_opt | ||
| ).stream_id | ||
| except ValueError: | ||
| raise SynapseError( | ||
| HTTPStatus.BAD_REQUEST, | ||
| "`from` is not a valid token", | ||
| errcode=Codes.INVALID_PARAM, | ||
| ) | ||
| else: | ||
| end_stream_id = self.store.get_max_thread_subscriptions_stream_id() | ||
|
|
||
| if to_start_opt is not None: | ||
| # because of backwards pagination, the `to` token is actually the | ||
| # bound closest to the start of the stream | ||
| try: | ||
| start_stream_id = ThreadSubscriptionsToken.from_string( | ||
| to_start_opt | ||
| ).stream_id | ||
| except ValueError: | ||
| # we also accept sliding sync `pos` tokens on this parameter | ||
| try: | ||
| sliding_sync_pos = await SlidingSyncStreamToken.from_string( | ||
| self.store, to_start_opt | ||
| ) | ||
| start_stream_id = ( | ||
| sliding_sync_pos.stream_token.thread_subscriptions_key | ||
| ) | ||
| except ValueError: | ||
| raise SynapseError( | ||
| HTTPStatus.BAD_REQUEST, | ||
| "`to` is not a valid token", | ||
| errcode=Codes.INVALID_PARAM, | ||
| ) | ||
| else: | ||
| # the start of time is ID 1; the lower bound is exclusive though | ||
| start_stream_id = 0 | ||
anoadragon453 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| subscriptions = ( | ||
| await self.store.get_latest_updated_thread_subscriptions_for_user( | ||
| requester.user.to_string(), | ||
| from_id=start_stream_id, | ||
| to_id=end_stream_id, | ||
| limit=limit, | ||
| ) | ||
| ) | ||
|
|
||
| subscribed_threads: Dict[str, Dict[str, JsonDict]] = {} | ||
| unsubscribed_threads: Dict[str, Dict[str, JsonDict]] = {} | ||
| for stream_id, room_id, thread_root_id, subscribed, automatic in subscriptions: | ||
| if subscribed: | ||
| subscribed_threads.setdefault(room_id, {})[thread_root_id] = ( | ||
| attr.asdict( | ||
| _ThreadSubscription( | ||
| automatic=automatic, | ||
| bump_stamp=stream_id, | ||
| ) | ||
| ) | ||
| ) | ||
| else: | ||
| unsubscribed_threads.setdefault(room_id, {})[thread_root_id] = ( | ||
| attr.asdict(_ThreadUnsubscription(bump_stamp=stream_id)) | ||
| ) | ||
|
|
||
| result: JsonDict = {} | ||
| if subscribed_threads: | ||
| result["subscribed"] = subscribed_threads | ||
| if unsubscribed_threads: | ||
| result["unsubscribed"] = unsubscribed_threads | ||
|
|
||
| if len(subscriptions) == limit: | ||
| # We hit the limit, so there might be more entries to return. | ||
| # Generate a new token that has moved backwards, ready for the next | ||
| # request. | ||
| min_returned_stream_id, _, _, _, _ = subscriptions[0] | ||
| result["end"] = ThreadSubscriptionsToken( | ||
| # We subtract one because the 'later in the stream' bound is inclusive, | ||
| # and we already saw the element at index 0. | ||
| stream_id=min_returned_stream_id - 1 | ||
| ).to_string() | ||
|
|
||
| return HTTPStatus.OK, result | ||
|
|
||
|
|
||
| def register_servlets(hs: "HomeServer", http_server: HttpServer) -> None: | ||
| if hs.config.experimental.msc4306_enabled: | ||
| ThreadSubscriptionsRestServlet(hs).register(http_server) | ||
| ThreadSubscriptionsPaginationRestServlet(hs).register(http_server) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about adding rate-limiting to this handler?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm... do we rate-limit any read operations already?
Just looking at https://spec.matrix.org/v1.15/client-server-api/#get_matrixclientv3roomsroomidmessages and sync and I'm not seeing them be rate limited.
Not that it's a terrible idea, but not sure how to balance with consistency (and the fact you could rate limit general requests with a load balancer e.g. nginx for instance)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mostly asked the question to check that it had been considered (which we tend to forget to do when adding new endpoints).
I'll accept the argument that the endpoint can be rate-limited by IP or access token in the load balancer for now, as I agree that we don't need anything more granular. We also have a
MAX_LIMITvalue defined, which prevents any one request from drawing too many resources.