-
Notifications
You must be signed in to change notification settings - Fork 562
Expose MSC4354 Sticky Events over the legacy (v3) /sync API. #19487
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 all commits
63024c4
69f76c3
c8174c9
48a42e8
6c9c9fe
5f5570c
a70e411
005fcda
4a42a56
e975caf
654cb1d
6368a9e
a4b09ab
543a97e
35a0eb6
2c33715
a6b0ce1
69a07ec
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 |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Expose [MSC4354 Sticky Events](https://github.com/matrix-org/matrix-spec-proposals/pull/4354) over the legacy (v3) /sync API. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,9 +13,7 @@ | |
| import logging | ||
| import random | ||
| from dataclasses import dataclass | ||
| from typing import ( | ||
| TYPE_CHECKING, | ||
| ) | ||
| from typing import TYPE_CHECKING, Collection, cast | ||
|
|
||
| from twisted.internet.defer import Deferred | ||
|
|
||
|
|
@@ -25,6 +23,7 @@ | |
| DatabasePool, | ||
| LoggingDatabaseConnection, | ||
| LoggingTransaction, | ||
| make_in_list_sql_clause, | ||
| ) | ||
| from synapse.storage.databases.main.cache import CacheInvalidationWorkerStore | ||
| from synapse.storage.databases.main.state import StateGroupWorkerStore | ||
|
|
@@ -138,6 +137,98 @@ def get_max_sticky_events_stream_id(self) -> int: | |
| def get_sticky_events_stream_id_generator(self) -> MultiWriterIdGenerator: | ||
| return self._sticky_events_id_gen | ||
|
|
||
| async def get_sticky_events_in_rooms( | ||
| self, | ||
| room_ids: Collection[str], | ||
| *, | ||
| from_id: int, | ||
| to_id: int, | ||
| now: int, | ||
| limit: int | None, | ||
| ) -> tuple[int, dict[str, list[str]]]: | ||
| """ | ||
| Fetch all the sticky events' IDs in the given rooms, with sticky stream IDs satisfying | ||
| from_id < sticky stream ID <= to_id. | ||
|
|
||
| The events are returned ordered by the sticky events stream. | ||
|
|
||
| Args: | ||
| room_ids: The room IDs to return sticky events in. | ||
| from_id: The sticky stream ID that sticky events should be returned from (exclusive). | ||
| to_id: The sticky stream ID that sticky events should end at (inclusive). | ||
|
reivilibre marked this conversation as resolved.
|
||
| now: The current time in unix millis, used for skipping expired events. | ||
| limit: Max sticky events to return, or None to apply no limit. | ||
| Returns: | ||
| to_id, dict[room_id, list[event_ids]] | ||
| """ | ||
| sticky_events_rows = await self.db_pool.runInteraction( | ||
| "get_sticky_events_in_rooms", | ||
| self._get_sticky_events_in_rooms_txn, | ||
| room_ids, | ||
| from_id=from_id, | ||
| to_id=to_id, | ||
| now=now, | ||
| limit=limit, | ||
| ) | ||
|
|
||
| if not sticky_events_rows: | ||
| return to_id, {} | ||
|
|
||
| # Get stream_id of the last row, which is the highest | ||
| new_to_id, _, _ = sticky_events_rows[-1] | ||
|
|
||
| # room ID -> event IDs | ||
| room_id_to_event_ids: dict[str, list[str]] = {} | ||
| for _, room_id, event_id in sticky_events_rows: | ||
| events = room_id_to_event_ids.setdefault(room_id, []) | ||
| events.append(event_id) | ||
|
|
||
| return (new_to_id, room_id_to_event_ids) | ||
|
|
||
| def _get_sticky_events_in_rooms_txn( | ||
| self, | ||
| txn: LoggingTransaction, | ||
| room_ids: Collection[str], | ||
| *, | ||
| from_id: int, | ||
|
reivilibre marked this conversation as resolved.
|
||
| to_id: int, | ||
| now: int, | ||
| limit: int | None, | ||
| ) -> list[tuple[int, str, str]]: | ||
| if len(room_ids) == 0: | ||
| return [] | ||
| room_id_in_list_clause, room_id_in_list_values = make_in_list_sql_clause( | ||
| txn.database_engine, "se.room_id", room_ids | ||
| ) | ||
| limit_clause = "" | ||
| limit_params: tuple[int, ...] = () | ||
| if limit is not None: | ||
| limit_clause = "LIMIT ?" | ||
| limit_params = (limit,) | ||
|
|
||
| if isinstance(self.database_engine, PostgresEngine): | ||
| expr_soft_failed = "COALESCE(((ej.internal_metadata::jsonb)->>'soft_failed')::boolean, FALSE)" | ||
| else: | ||
| expr_soft_failed = "COALESCE(ej.internal_metadata->>'soft_failed', FALSE)" | ||
|
Comment on lines
+209
to
+212
Contributor
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. Should we even care about checking for Applies to the
Contributor
Author
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. Hmm. In an ideal world if it was easy to do so, I think it makes it easier for everyone involved if we do filter here, as it means we're not either 1) looping outside the DB call or 2) looping in the client. I'm not a huge fan of sending empty lists back to the client and making them immediately resync to actually get some data, potentially several times.
Contributor
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. I think Sticky Events is the first place we do this sort of check at this level (hence your JSON operator database compatibility pain).
It is looping outside of the DB call but would be just part of the
I'd be ok with the trade-off for Overall, just pointing it out and ok with it if you're happy to move forward with it. |
||
|
|
||
| txn.execute( | ||
| f""" | ||
| SELECT se.stream_id, se.room_id, event_id | ||
| FROM sticky_events se | ||
| INNER JOIN event_json ej USING (event_id) | ||
| WHERE | ||
| NOT {expr_soft_failed} | ||
| AND ? < expires_at | ||
| AND ? < stream_id | ||
| AND stream_id <= ? | ||
| AND {room_id_in_list_clause} | ||
| ORDER BY stream_id ASC | ||
| {limit_clause} | ||
| """, | ||
| (now, from_id, to_id, *room_id_in_list_values, *limit_params), | ||
| ) | ||
| return cast(list[tuple[int, str, str]], txn.fetchall()) | ||
|
|
||
| async def get_updated_sticky_events( | ||
| self, *, from_id: int, to_id: int, limit: int | ||
| ) -> list[StickyEventUpdate]: | ||
|
|
||
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.
This async wasn't being used, so I thought it was enlightening/self-documenting to remove it.
Also renamed to
maybecause it's just an approximation.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.
Just double-checking, the other manual checks here give us a definite answer?