Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Remove support for the unstable dir flag on relations. #14106

Merged
merged 4 commits into from
Oct 7, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/14106.removal
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Remove the unstable identifier for [MSC3715](https://github.com/matrix-org/matrix-doc/pull/3715).
3 changes: 0 additions & 3 deletions synapse/config/experimental.py
Original file line number Diff line number Diff line change
Expand Up @@ -100,9 +100,6 @@ def read_config(self, config: JsonDict, **kwargs: Any) -> None:
# MSC3773: Thread notifications
self.msc3773_enabled: bool = experimental.get("msc3773_enabled", False)

# MSC3715: dir param on /relations.
self.msc3715_enabled: bool = experimental.get("msc3715_enabled", False)

# MSC3848: Introduce errcodes for specific event sending failures
self.msc3848_enabled: bool = experimental.get("msc3848_enabled", False)

Expand Down
33 changes: 16 additions & 17 deletions synapse/handlers/relations.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
from synapse.events import EventBase, relation_from_event
from synapse.logging.opentracing import trace
from synapse.storage.databases.main.relations import _RelatedEvent
from synapse.streams.config import PaginationConfig
from synapse.types import JsonDict, Requester, StreamToken, UserID
from synapse.visibility import filter_events_for_client

Expand Down Expand Up @@ -72,13 +73,10 @@ async def get_relations(
requester: Requester,
event_id: str,
room_id: str,
pagin_config: PaginationConfig,
include_original_event: bool,
relation_type: Optional[str] = None,
event_type: Optional[str] = None,
limit: int = 5,
direction: str = "b",
from_token: Optional[StreamToken] = None,
to_token: Optional[StreamToken] = None,
include_original_event: bool = False,
) -> JsonDict:
"""Get related events of a event, ordered by topological ordering.

Expand All @@ -88,14 +86,10 @@ async def get_relations(
requester: The user requesting the relations.
event_id: Fetch events that relate to this event ID.
room_id: The room the event belongs to.
pagin_config: The pagination config rules to apply, if any.
include_original_event: Whether to include the parent event.
relation_type: Only fetch events with this relation type, if given.
event_type: Only fetch events with this event type, if given.
limit: Only fetch the most recent `limit` events.
direction: Whether to fetch the most recent first (`"b"`) or the
oldest first (`"f"`).
from_token: Fetch rows from the given token, or from the start if None.
to_token: Fetch rows up to the given token, or up to the end if None.
include_original_event: Whether to include the parent event.

Returns:
The pagination chunk.
Expand All @@ -114,6 +108,9 @@ async def get_relations(
if event is None:
raise SynapseError(404, "Unknown parent event.")

# TODO Update pagination config to not allow None limits.
assert pagin_config.limit is not None

# Note that ignored users are not passed into get_relations_for_event
# below. Ignored users are handled in filter_events_for_client (and by
# not passing them in here we should get a better cache hit rate).
Expand All @@ -123,10 +120,10 @@ async def get_relations(
room_id=room_id,
relation_type=relation_type,
event_type=event_type,
limit=limit,
direction=direction,
from_token=from_token,
to_token=to_token,
limit=pagin_config.limit,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mypy unhappy here. Is this just an assert expr is not None? (Having said that, I'm wondering if PaginationConfig.limit being None makes sense?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if PaginationConfig.limit being None makes sense?)

I don't think it does, it looks like there might be a single spot it gets used? I'm looking.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although it looks invasive enough that I might go with the assert for now?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naively I'd expect "don't use a limit" to be represented by a lack of PaginationConfig.

Happy with an assert for now!

direction=pagin_config.direction,
from_token=pagin_config.from_token,
to_token=pagin_config.to_token,
)

events = await self._main_store.get_events_as_list(
Expand Down Expand Up @@ -162,8 +159,10 @@ async def get_relations(
if next_token:
return_value["next_batch"] = await next_token.to_string(self._main_store)

if from_token:
return_value["prev_batch"] = await from_token.to_string(self._main_store)
if pagin_config.from_token:
return_value["prev_batch"] = await pagin_config.from_token.to_string(
self._main_store
)

return return_value

Expand Down
45 changes: 10 additions & 35 deletions synapse/rest/client/relations.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,11 @@
from typing import TYPE_CHECKING, Optional, Tuple

from synapse.http.server import HttpServer
from synapse.http.servlet import RestServlet, parse_integer, parse_string
from synapse.http.servlet import RestServlet
from synapse.http.site import SynapseRequest
from synapse.rest.client._base import client_patterns
from synapse.types import JsonDict, StreamToken
from synapse.streams.config import PaginationConfig
from synapse.types import JsonDict

if TYPE_CHECKING:
from synapse.server import HomeServer
Expand All @@ -41,9 +42,8 @@ class RelationPaginationServlet(RestServlet):
def __init__(self, hs: "HomeServer"):
super().__init__()
self.auth = hs.get_auth()
self.store = hs.get_datastores().main
self._store = hs.get_datastores().main
self._relations_handler = hs.get_relations_handler()
self._msc3715_enabled = hs.config.experimental.msc3715_enabled

async def on_GET(
self,
Expand All @@ -55,49 +55,24 @@ async def on_GET(
) -> Tuple[int, JsonDict]:
requester = await self.auth.get_user_by_req(request, allow_guest=True)

limit = parse_integer(request, "limit", default=5)
# Fetch the direction parameter, if provided.
#
# TODO Use PaginationConfig.from_request when the unstable parameter is
# no longer needed.
direction = parse_string(request, "dir", allowed_values=["f", "b"])
if direction is None:
if self._msc3715_enabled:
direction = parse_string(
request,
"org.matrix.msc3715.dir",
DMRobertson marked this conversation as resolved.
Show resolved Hide resolved
default="b",
allowed_values=["f", "b"],
)
else:
direction = "b"
from_token_str = parse_string(request, "from")
to_token_str = parse_string(request, "to")

# Return the relations
from_token = None
if from_token_str:
from_token = await StreamToken.from_string(self.store, from_token_str)
to_token = None
if to_token_str:
to_token = await StreamToken.from_string(self.store, to_token_str)
pagination_config = await PaginationConfig.from_request(
self._store, request, default_limit=5, default_dir="b"
)

# The unstable version of this API returns an extra field for client
# compatibility, see https://github.com/matrix-org/synapse/issues/12930.
assert request.path is not None
include_original_event = request.path.startswith(b"/_matrix/client/unstable/")

# Return the relations
result = await self._relations_handler.get_relations(
requester=requester,
event_id=parent_id,
room_id=room_id,
pagin_config=pagination_config,
include_original_event=include_original_event,
relation_type=relation_type,
event_type=event_type,
limit=limit,
direction=direction,
from_token=from_token,
to_token=to_token,
include_original_event=include_original_event,
)

return 200, result
Expand Down
6 changes: 4 additions & 2 deletions synapse/streams/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,12 @@ async def from_request(
cls,
store: "DataStore",
request: SynapseRequest,
raise_invalid_params: bool = True,
DMRobertson marked this conversation as resolved.
Show resolved Hide resolved
default_limit: Optional[int] = None,
default_dir: str = "f",
) -> "PaginationConfig":
direction = parse_string(request, "dir", default="f", allowed_values=["f", "b"])
direction = parse_string(
request, "dir", default=default_dir, allowed_values=["f", "b"]
)

from_tok_str = parse_string(request, "from")
to_tok_str = parse_string(request, "to")
Expand Down