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

Clarify code that chooses a thread ID to include in a receipt #3797

Merged
merged 4 commits into from
Oct 16, 2023
Merged

Clarify code that chooses a thread ID to include in a receipt #3797

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6443bd9
Extract threadIdForReceipt function from sendReceipt
andybalaam Oct 13, 2023
f162c64
Tests for threadIdForReceipt
andybalaam Oct 13, 2023
fd235e3
Correct test of threadIdForReceipt to expect main for redaction of th…
andybalaam Oct 13, 2023
6c7626b
Expand and comment implementation of threadIdForReceipt
andybalaam Oct 13, 2023
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
198 changes: 197 additions & 1 deletion spec/unit/read-receipt.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import MockHttpBackend from "matrix-mock-request";

import { MAIN_ROOM_TIMELINE, ReceiptType, WrappedReceipt } from "../../src/@types/read_receipts";
import { MatrixClient } from "../../src/client";
import { EventType, MatrixEvent, Room } from "../../src/matrix";
import { EventType, MatrixEvent, RelationType, Room, threadIdForReceipt } from "../../src/matrix";
import { synthesizeReceipt } from "../../src/models/read-receipt";
import { encodeUri } from "../../src/utils";
import * as utils from "../test-utils/test-utils";
Expand Down Expand Up @@ -258,4 +258,200 @@ describe("Read receipt", () => {
expect(room.getEventReadUpTo(userId)).toBe(mainTimelineReceipt.eventId);
});
});

describe("Determining the right thread ID for a receipt", () => {
it("provides the thread root ID for a normal threaded message", () => {
const event = utils.mkEvent({
event: true,
type: EventType.RoomMessage,
user: "@bob:matrix.org",
room: "!roomx",
content: {
"body": "Hello from a thread",
"m.relates_to": {
"event_id": "$thread1",
"m.in_reply_to": {
event_id: "$thread1",
},
"rel_type": "m.thread",
},
},
});

expect(threadIdForReceipt(event)).toEqual("$thread1");
});

it("provides 'main' for a non-thread message", () => {
const event = utils.mkEvent({
event: true,
type: EventType.RoomMessage,
user: "@bob:matrix.org",
room: "!roomx",
content: { body: "Hello" },
});

expect(threadIdForReceipt(event)).toEqual("main");
});

it("provides 'main' for a thread root", () => {
const event = utils.mkEvent({
event: true,
type: EventType.RoomMessage,
user: "@bob:matrix.org",
room: "!roomx",
content: { body: "Hello" },
});
// Set thread ID to this event's ID, meaning this is the thread root
event.setThreadId(event.getId());

expect(threadIdForReceipt(event)).toEqual("main");
});

it("provides 'main' for a reaction to a thread root", () => {
const event = utils.mkEvent({
event: true,
type: EventType.Reaction,
user: "@bob:matrix.org",
room: "!roomx",
content: {
"m.relates_to": {
rel_type: RelationType.Annotation,
event_id: "$thread1",
key: Math.random().toString(),
},
},
});

// Set thread Id, meaning this looks like it's in the thread (this
// happens for relations like this, so that they appear in the
// thread's timeline).
event.setThreadId("$thread1");

// But because it's a reaction to the thread root, it's in main
expect(threadIdForReceipt(event)).toEqual("main");
});

it("provides the thread ID for a reaction to a threaded message", () => {
const event = utils.mkEvent({
event: true,
type: EventType.Reaction,
user: "@bob:matrix.org",
room: "!roomx",
content: {
"m.relates_to": {
rel_type: RelationType.Annotation,
event_id: "$withinthread2",
key: Math.random().toString(),
},
},
});

// Set thread Id, to say this message is in the thread. This happens
// when the message arrived and is classified.
event.setThreadId("$thread1");

// It's in the thread because it refers to something else, not the
// thread root
expect(threadIdForReceipt(event)).toEqual("$thread1");
});

it("(suprisingly?) provides 'main' for a redaction of a threaded message", () => {
const event = utils.mkEvent({
event: true,
type: EventType.RoomRedaction,
content: {
reason: "Spamming",
},
redacts: "$withinthread2",
room: "!roomx",
user: "@bob:matrix.org",
});

// Set thread Id, to say this message is in the thread.
event.setThreadId("$thread1");

// Because redacting a message removes all its m.relations, the
// message is no longer in the thread, so we must send a receipt for
// it in the main timeline.
//
// This is surprising, but it follows the spec (at least up to
// current latest room version, 11). In fact, the event should no
// longer have a thread ID set on it, so this testcase should not
// come up. (At time of writing, this is not the case though - it
// does still have threadId set.)
expect(threadIdForReceipt(event)).toEqual("main");
});

it("provides the thread ID for an edit of a threaded message", () => {
const event = utils.mkEvent({
event: true,
type: EventType.RoomRedaction,
content: {
"body": "Edited!",
"m.new_content": {
body: "Edited!",
},
"m.relates_to": {
rel_type: RelationType.Replace,
event_id: "$withinthread2",
},
},
room: "!roomx",
user: "@bob:matrix.org",
});

// Set thread Id, to say this message is in the thread.
event.setThreadId("$thread1");

// It's in the thread, because it redacts something inside the
// thread (not the thread root)
expect(threadIdForReceipt(event)).toEqual("$thread1");
});

it("provides 'main' for an edit of a thread root", () => {
const event = utils.mkEvent({
event: true,
type: EventType.RoomRedaction,
content: {
"body": "Edited!",
"m.new_content": {
body: "Edited!",
},
"m.relates_to": {
rel_type: RelationType.Replace,
event_id: "$thread1",
},
},
room: "!roomx",
user: "@bob:matrix.org",
});

// Set thread Id, to say this message is in the thread.
event.setThreadId("$thread1");

// It's in the thread, because it redacts something inside the
// thread (not the thread root)
expect(threadIdForReceipt(event)).toEqual("main");
});

it("provides 'main' for a redaction of the thread root", () => {
const event = utils.mkEvent({
event: true,
type: EventType.RoomRedaction,
content: {
reason: "Spamming",
},
redacts: "$thread1",
room: "!roomx",
user: "@bob:matrix.org",
});

// Set thread Id, to say this message is in the thread.
event.setThreadId("$thread1");

// It's in the thread, because it redacts something inside the
// thread (not the thread root)
expect(threadIdForReceipt(event)).toEqual("main");
});
});
});
85 changes: 68 additions & 17 deletions src/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5157,24 +5157,12 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
$eventId: event.getId()!,
});

if (!unthreaded && this.supportsThreads()) {
// XXX: the spec currently says a threaded read receipt can be sent for the root of a thread,
// but in practice this isn't possible and the spec needs updating.
const isThread =
!!event.threadRootId &&
// A thread cannot be just a thread root and a thread root can only be read in the main timeline
!event.isThreadRoot &&
// Similarly non-thread relations upon the thread root (reactions, edits) should also be for the main timeline.
event.isRelation() &&
(event.isRelation(THREAD_RELATION_TYPE.name) || event.relationEventId !== event.threadRootId);
body = {
...body,
// Only thread replies should define a specific thread. Thread roots can only be read in the main timeline.
thread_id: isThread ? event.threadRootId : MAIN_ROOM_TIMELINE,
};
}
// Unless we're explicitly making an unthreaded receipt or we don't
// support threads, include the `thread_id` property in the body.
const shouldAddThreadId = !unthreaded && this.supportsThreads();
const fullBody = shouldAddThreadId ? { ...body, thread_id: threadIdForReceipt(event) } : body;

const promise = this.http.authedRequest<{}>(Method.Post, path, undefined, body || {});
const promise = this.http.authedRequest<{}>(Method.Post, path, undefined, fullBody || {});

const room = this.getRoom(event.getRoomId());
if (room && this.credentials.userId) {
Expand Down Expand Up @@ -9925,3 +9913,66 @@ export function fixNotificationCountOnDecryption(cli: MatrixClient, event: Matri
}
}
}

/**
* Given an event, figure out the thread ID we should use for it in a receipt.
*
* This will either be "main", or event.threadRootId. For the thread root, or
* e.g. reactions to the thread root, this will be main. For events inside the
* thread, or e.g. reactions to them, this will be event.threadRootId.
*
* (Exported for test.)
*/
export function threadIdForReceipt(event: MatrixEvent): string {
return inMainTimelineForReceipt(event) ? MAIN_ROOM_TIMELINE : event.threadRootId!;
}

/**
* a) True for non-threaded messages, thread roots and non-thread relations to thread roots.
* b) False for messages with thread relations to the thread root.
* c) False for messages with any kind of relation to a message from case b.
*
* Note: true for redactions of messages that are in threads. Redacted messages
* are not really in threads (because their relations are gone), so if they look
* like they are in threads, that is a sign of a bug elsewhere. (At time of
* writing, this bug definitely exists - messages are not moved to another
* thread when they are redacted.)
*
* @returns true if this event is considered to be in the main timeline as far
* as receipts are concerned.
*/
function inMainTimelineForReceipt(event: MatrixEvent): boolean {
if (!event.threadRootId) {
// Not in a thread: then it is in the main timeline
return true;
}

if (event.isThreadRoot) {
// Thread roots are in the main timeline. Note: the spec is ambiguous (or
// wrong) on this - see
// https://github.com/matrix-org/matrix-spec-proposals/pull/4037
return true;
}

if (!event.isRelation()) {
// If it's not related to anything, it can't be related via a chain of
// relations to a thread root.
//
// Note: this is a bug, because how does it have a threadRootId if it is
// neither a thread root, nor related to one?
logger.warn(`Event is not a relation or a thread root, but still has a threadRootId! id=${event.getId()}`);
return true;
}

if (event.isRelation(THREAD_RELATION_TYPE.name)) {
// It's a message in a thread - definitely not in the main timeline.
return false;
}

const isRelatedToRoot = event.relationEventId === event.threadRootId;

// If it's related to the thread root (and we already know it's not a thread
// relation) then it's in the main timeline. If it's related to something
// else, then it's in the thread (because it has a thread ID).
return isRelatedToRoot;
}