Skip to content
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.

Sign up for GitHub

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

Jump to bottom

bugfix: always prefer unthreaded receipt when >1 exist (MSC4102) #16927

Merged
merged 3 commits into from
Feb 20, 2024
Merged

bugfix: always prefer unthreaded receipt when >1 exist (MSC4102) #16927

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b263f74
bugfix: always prefer unthreaded receipt when >1 exist (MSC4102)
kegsay Feb 16, 2024
5ea29da
Linting and changelog
kegsay Feb 16, 2024
82aa44a
Update changelog.d/16927.bugfix
kegsay Feb 20, 2024
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/16927.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Always prefer unthreaded receipt when >1 exist ([MSC4102](https://github.com/matrix-org/matrix-spec-proposals/pull/4102)).
21 changes: 18 additions & 3 deletions synapse/storage/databases/main/receipts.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,9 +472,24 @@ def f(
event_entry = room_event["content"].setdefault(event_id, {})
receipt_type_dict = event_entry.setdefault(receipt_type, {})

receipt_type_dict[user_id] = db_to_json(data)
if thread_id:
receipt_type_dict[user_id]["thread_id"] = thread_id
# MSC4102: always replace threaded receipts with unthreaded ones if there is a clash.
# Specifically:
# - if there is no existing receipt, great, set the data.
# - if there is an existing receipt, is it threaded (thread_id present)?
# YES: replace if this receipt has no thread id. NO: do not replace.
# This means we will drop some receipts, but MSC4102 is designed to drop semantically
# meaningless receipts, so this is okay. Previously, we would drop meaningful data!
receipt_data = db_to_json(data)
if user_id in receipt_type_dict: # existing receipt
Copy link
Member

Choose a reason for hiding this comment

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

I think the first arm of this if statement does not handle the following cases:

  • there is an existing receipt, and it is unthreaded
  • there is an existing receipt and it is threaded, but there is an incoming threaded receipt

In both of those cases, receipt_type_dict is not modified. Is this on purpose? If not, perhaps it'd be best to pull line 490 up and out of this if statement, so that it is run regardless (and then potentially overridden later).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there is an existing receipt, and it is unthreaded then it should not be replaced so I would expect the conditional to fail.

It is impossible for there to be a threaded receipt and then another threaded receipt for the same (user, event, type) tuple. If that did happen, it's undefined behaviour.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see. I was perplexed that the old code would always update receipt_type_dict, whereas this new code does not in some cases. But I suppose that was the "last receipt always wins" behaviour you described that you seek to replace.

N.B. I find the (prior) code in this method a bit too magical. But your changes look sound.

# is the existing receipt threaded and we are currently processing an unthreaded one?
if "thread_id" in receipt_type_dict[user_id] and not thread_id:
receipt_type_dict[
user_id
] = receipt_data # replace with unthreaded one
else: # receipt does not exist, just set it
receipt_type_dict[user_id] = receipt_data
if thread_id:
receipt_type_dict[user_id]["thread_id"] = thread_id

results = {
room_id: [results[room_id]] if room_id in results else []
Expand Down
Loading