This repository has been archived by the owner on Apr 26, 2024. It is now read-only.
-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Add a thread relation type per MSC3440. #11088
Merged
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
e22c941
Add a thread relation type.
clokep 4f64841
Only include an aggregation key if one is provided.
clokep 02f021b
Include a thread-summary for bundled relations.
clokep cf387c2
Add an experimental configuration flag.
clokep 8ff22d0
Newsfragment
clokep 7569579
Fix queries on Postgres.
clokep c22f292
Merge remote-tracking branch 'origin/develop' into clokep/thread-rela…
clokep f93f3ad
Review feedback.
clokep 8b245b0
Do not include the thread senders in the thread.
clokep 4da5294
Review comments.
clokep File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Experimental support for the thread relation defined in [MSC3440](https://github.com/matrix-org/matrix-doc/pull/3440). |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,7 +13,7 @@ | |
# limitations under the License. | ||
|
||
import logging | ||
from typing import Optional | ||
from typing import Optional, Tuple | ||
|
||
import attr | ||
|
||
|
@@ -269,6 +269,63 @@ def _get_applicable_edit_txn(txn): | |
|
||
return await self.get_event(edit_id, allow_none=True) | ||
|
||
@cached() | ||
async def get_thread_summary( | ||
self, event_id: str | ||
) -> Tuple[int, Optional[EventBase]]: | ||
"""Get the number of threaded replies, the senders of those replies, and | ||
the latest reply (if any) for the given event. | ||
|
||
Args: | ||
event_id: The original event ID | ||
|
||
Returns: | ||
The number of items in the thread and the most recent response, if any. | ||
""" | ||
|
||
def _get_thread_summary_txn(txn) -> Tuple[int, Optional[str]]: | ||
# Fetch the count of threaded events and the latest event ID. | ||
# TODO Should this only allow m.room.message events. | ||
sql = """ | ||
SELECT event_id | ||
FROM event_relations | ||
INNER JOIN events USING (event_id) | ||
WHERE | ||
relates_to_id = ? | ||
AND relation_type = ? | ||
clokep marked this conversation as resolved.
Show resolved
Hide resolved
|
||
ORDER BY topological_ordering DESC, stream_ordering DESC | ||
LIMIT 1 | ||
""" | ||
|
||
txn.execute(sql, (event_id, RelationTypes.THREAD)) | ||
row = txn.fetchone() | ||
if row is None: | ||
return 0, None | ||
|
||
latest_event_id = row[0] | ||
|
||
sql = """ | ||
SELECT COALESCE(COUNT(event_id), 0) | ||
FROM event_relations | ||
WHERE | ||
relates_to_id = ? | ||
AND relation_type = ? | ||
""" | ||
txn.execute(sql, (event_id, RelationTypes.THREAD)) | ||
count = txn.fetchone()[0] | ||
Comment on lines
+307
to
+315
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. It came up in a discussion about whether this should consider event redactions or not. |
||
|
||
return count, latest_event_id | ||
|
||
count, latest_event_id = await self.db_pool.runInteraction( | ||
"get_thread_summary", _get_thread_summary_txn | ||
) | ||
|
||
latest_event = None | ||
if latest_event_id: | ||
latest_event = await self.get_event(latest_event_id, allow_none=True) | ||
|
||
return count, latest_event | ||
|
||
async def has_user_annotated_event( | ||
self, parent_id: str, event_type: str, aggregation_key: str, sender: str | ||
) -> bool: | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -101,10 +101,10 @@ def test_deny_double_react(self): | |
|
||
def test_basic_paginate_relations(self): | ||
"""Tests that calling pagination API correctly the latest relations.""" | ||
channel = self._send_relation(RelationTypes.ANNOTATION, "m.reaction") | ||
channel = self._send_relation(RelationTypes.ANNOTATION, "m.reaction", "a") | ||
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. Annotations require a |
||
self.assertEquals(200, channel.code, channel.json_body) | ||
|
||
channel = self._send_relation(RelationTypes.ANNOTATION, "m.reaction") | ||
channel = self._send_relation(RelationTypes.ANNOTATION, "m.reaction", "b") | ||
self.assertEquals(200, channel.code, channel.json_body) | ||
annotation_id = channel.json_body["event_id"] | ||
|
||
|
@@ -141,8 +141,10 @@ def test_repeated_paginate_relations(self): | |
""" | ||
|
||
expected_event_ids = [] | ||
for _ in range(10): | ||
channel = self._send_relation(RelationTypes.ANNOTATION, "m.reaction") | ||
for idx in range(10): | ||
channel = self._send_relation( | ||
RelationTypes.ANNOTATION, "m.reaction", chr(ord("a") + idx) | ||
) | ||
self.assertEquals(200, channel.code, channel.json_body) | ||
expected_event_ids.append(channel.json_body["event_id"]) | ||
|
||
|
@@ -386,8 +388,9 @@ def test_aggregation_must_be_annotation(self): | |
) | ||
self.assertEquals(400, channel.code, channel.json_body) | ||
|
||
@unittest.override_config({"experimental_features": {"msc3440_enabled": True}}) | ||
def test_aggregation_get_event(self): | ||
"""Test that annotations and references get correctly bundled when | ||
"""Test that annotations, references, and threads get correctly bundled when | ||
getting the parent event. | ||
""" | ||
|
||
|
@@ -410,6 +413,13 @@ def test_aggregation_get_event(self): | |
self.assertEquals(200, channel.code, channel.json_body) | ||
reply_2 = channel.json_body["event_id"] | ||
|
||
channel = self._send_relation(RelationTypes.THREAD, "m.room.test") | ||
self.assertEquals(200, channel.code, channel.json_body) | ||
|
||
channel = self._send_relation(RelationTypes.THREAD, "m.room.test") | ||
self.assertEquals(200, channel.code, channel.json_body) | ||
thread_2 = channel.json_body["event_id"] | ||
|
||
channel = self.make_request( | ||
"GET", | ||
"/rooms/%s/event/%s" % (self.room, self.parent_id), | ||
|
@@ -429,6 +439,25 @@ def test_aggregation_get_event(self): | |
RelationTypes.REFERENCE: { | ||
"chunk": [{"event_id": reply_1}, {"event_id": reply_2}] | ||
}, | ||
RelationTypes.THREAD: { | ||
"count": 2, | ||
"latest_event": { | ||
"age": 100, | ||
"content": { | ||
"m.relates_to": { | ||
"event_id": self.parent_id, | ||
"rel_type": RelationTypes.THREAD, | ||
} | ||
}, | ||
"event_id": thread_2, | ||
"origin_server_ts": 1600, | ||
"room_id": self.room, | ||
"sender": self.user_id, | ||
"type": "m.room.test", | ||
"unsigned": {"age": 100}, | ||
"user_id": self.user_id, | ||
}, | ||
Comment on lines
+444
to
+459
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 debated about slimming down what fields we're checking against, most of them don't really matter here. |
||
}, | ||
}, | ||
) | ||
|
||
|
@@ -559,7 +588,6 @@ def test_edit_reply(self): | |
{ | ||
"m.relates_to": { | ||
"event_id": self.parent_id, | ||
"key": None, | ||
"rel_type": "m.reference", | ||
} | ||
}, | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Originally I removed this check entirely, but I'm unsure if that's the "right" thing to do or not.
If we removed this check then unknown / unstable / experimental / custom relations would be stored into the database. This seems good since you can then query them using
/relations
, but they would not appear in bundled aggregations since they're not understood.Additionally if another field needs to be pulled out (e..g if a relation of type
blah
has afoo
key that needs to be stored in the database) then it wouldn't be added to the table. I think storing all relations would still be an improvement as it would be easier to add support forblah
via a database migration + a background task to fill a newfoo
column.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.
Also -- I didn't put this behind a configuration flag since I think we want to store it regardless (kind of inline with the last paragraph above).
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.
@erikjohnston Any thoughts on if we should allow any relation type here or not?
I think we might also need a background update to find any of these events which occurred before someone upgrades. 😢
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 don't have a super strong opinion. I think I would prefer to only handle "known" types of relations, as otherwise we don't know how to handle them, but OTOH that makes the background updates take a lot longer 🤷
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.
Fair enough! Well we'll need to do a background update regardless for this, so I'm not going to worry about it.