From 6e9a3c485a2e114085d5348e0ad9031dee21072f Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Fri, 20 Oct 2023 15:49:30 +0100 Subject: [PATCH 01/26] Ignore receipts pointing at missing or invalid events --- spec/unit/read-receipt.spec.ts | 1 + spec/unit/room.spec.ts | 23 ++++++----- src/models/read-receipt.ts | 71 ++++++++++++++++++++++++++++++++-- 3 files changed, 82 insertions(+), 13 deletions(-) diff --git a/spec/unit/read-receipt.spec.ts b/spec/unit/read-receipt.spec.ts index 953643c8cf9..ea27d515219 100644 --- a/spec/unit/read-receipt.spec.ts +++ b/spec/unit/read-receipt.spec.ts @@ -225,6 +225,7 @@ describe("Read receipt", () => { it("should not allow an older unthreaded receipt to clobber a `main` threaded one", () => { const userId = client.getSafeUserId(); const room = new Room(ROOM_ID, client, userId); + room.findEventById = jest.fn().mockReturnValue({} as MatrixEvent); const unthreadedReceipt: WrappedReceipt = { eventId: "$olderEvent", diff --git a/spec/unit/room.spec.ts b/spec/unit/room.spec.ts index b1db43f5dcd..9e35bf124cf 100644 --- a/spec/unit/room.spec.ts +++ b/spec/unit/room.spec.ts @@ -1746,6 +1746,7 @@ describe("Room", function () { it("should acknowledge if an event has been read", function () { const ts = 13787898424; room.addReceipt(mkReceipt(roomId, [mkRecord(eventToAck.getId()!, "m.read", userB, ts)])); + room.findEventById = jest.fn().mockReturnValue({} as MatrixEvent); expect(room.hasUserReadEvent(userB, eventToAck.getId()!)).toEqual(true); }); it("return false for an unknown event", function () { @@ -3151,7 +3152,7 @@ describe("Room", function () { room.getReadReceiptForUserId = (userId, ignore, receiptType): WrappedReceipt | null => { return receiptType === ReceiptType.ReadPrivate ? ({ eventId: "eventId" } as WrappedReceipt) : null; }; - + room.findEventById = jest.fn().mockReturnValue({} as MatrixEvent); expect(room.getEventReadUpTo(userA)).toEqual("eventId"); }); @@ -3170,10 +3171,11 @@ describe("Room", function () { for (let i = 1; i <= 2; i++) { room.getUnfilteredTimelineSet = () => ({ - compareEventOrdering: (event1, event2) => { + compareEventOrdering: (event1: string) => { return event1 === `eventId${i}` ? 1 : -1; }, - } as EventTimelineSet); + findEventById: jest.fn().mockReturnValue({} as MatrixEvent), + } as unknown as EventTimelineSet); expect(room.getEventReadUpTo(userA)).toEqual(`eventId${i}`); } @@ -3184,8 +3186,9 @@ describe("Room", function () { for (let i = 1; i <= 2; i++) { room.getUnfilteredTimelineSet = () => ({ - compareEventOrdering: (_1, _2) => null, - } as EventTimelineSet); + compareEventOrdering: () => null, + findEventById: jest.fn().mockReturnValue({} as MatrixEvent), + } as unknown as EventTimelineSet); room.getReadReceiptForUserId = (userId, ignore, receiptType): WrappedReceipt | null => { if (receiptType === ReceiptType.ReadPrivate) { return { eventId: "eventId1", data: { ts: i === 1 ? 2 : 1 } } as WrappedReceipt; @@ -3203,8 +3206,9 @@ describe("Room", function () { it("should correctly compare, if private read receipt is missing", () => { room.getUnfilteredTimelineSet = () => ({ - compareEventOrdering: (_1, _2) => null, - } as EventTimelineSet); + compareEventOrdering: () => null, + findEventById: jest.fn().mockReturnValue({} as MatrixEvent), + } as unknown as EventTimelineSet); room.getReadReceiptForUserId = (userId, ignore, receiptType): WrappedReceipt | null => { if (receiptType === ReceiptType.Read) { return { eventId: "eventId2", data: { ts: 1 } } as WrappedReceipt; @@ -3220,8 +3224,9 @@ describe("Room", function () { beforeAll(() => { room.getUnfilteredTimelineSet = () => ({ - compareEventOrdering: (_1, _2) => null, - } as EventTimelineSet); + compareEventOrdering: () => null, + findEventById: jest.fn().mockReturnValue({} as MatrixEvent), + } as unknown as EventTimelineSet); }); it("should give precedence to m.read.private", () => { diff --git a/src/models/read-receipt.ts b/src/models/read-receipt.ts index 29eb1409a2c..2684d7916a3 100644 --- a/src/models/read-receipt.ts +++ b/src/models/read-receipt.ts @@ -26,6 +26,7 @@ import { EventType } from "../@types/event"; import { EventTimelineSet } from "./event-timeline-set"; import { MapWithDefault } from "../utils"; import { NotificationCountType } from "./room"; +import { logger } from "../logger"; export function synthesizeReceipt(userId: string, event: MatrixEvent, receiptType: ReceiptType): MatrixEvent { return new MatrixEvent({ @@ -95,14 +96,74 @@ export abstract class ReadReceipt< /** * Get the ID of the event that a given user has read up to, or null if we - * have received no read receipts from them. + * have received no read receipts from them, or if the receipt we have + * points at an event we don't have, or if the thread ID in the receipt does + * not match the thread root of the referenced event. + * * @param userId - The user ID to get read receipt event ID for * @param ignoreSynthesized - If true, return only receipts that have been * sent by the server, not implicit ones generated * by the JS SDK. - * @returns ID of the latest event that the given user has read, or null. + * @returns ID of the latest existing event that the given user has read, or null. */ public getEventReadUpTo(userId: string, ignoreSynthesized = false): string | null { + // Find what the latest receipt says is the latest event we have read + const latestReceipt = this.getLatestReceipt(userId, ignoreSynthesized); + + if (!latestReceipt) { + return null; + } + + return this.receiptPointsAtConsistentEvent(latestReceipt) ? latestReceipt.eventId : null; + } + + /** + * Returns true if the event pointed at by this receipt exists, and its + * threadRootId is consistent with the thread information in the receipt. + */ + private receiptPointsAtConsistentEvent(receipt: WrappedReceipt): boolean { + // If the receipt points at a non-existent event, it's not consistent + const event = this.findEventById(receipt.eventId); + if (!event) { + logger.warn(`Ignoring receipt for missing event with id ${receipt.eventId}`); + return false; + } + + // If this is an unthreaded receipt, we're good + if (!receipt.data?.thread_id) { + return true; + } + + // If the receipt is for the main timeline + if (receipt.data.thread_id === MAIN_ROOM_TIMELINE) { + // If the event is not in a thread, this checks out + if (!event.threadRootId) { + return true; + } + + // If the event is a thread root, we need a special case because + // event.threadRootId won't match, but thread root events are + // actually on the main timeline, so we return successfully. + if (event.isThreadRoot) { + return true; + } + } + + // If the receipt is for the correct thread, we are good. + if (event.threadRootId === receipt.data.thread_id) { + return true; + } + + // The thread ID doesn't match up + logger.warn( + `Ignoring receipt because its thread_id (${receipt.data.thread_id}) disagrees \ + with the thread root (${event.threadRootId}) of the referenced event \ + (event ID = ${receipt.eventId})`, + ); + return false; + } + + private getLatestReceipt(userId: string, ignoreSynthesized = false): WrappedReceipt | null { // XXX: This is very very ugly and I hope I won't have to ever add a new // receipt type here again. IMHO this should be done by the server in // some more intelligent manner or the client should just use timestamps @@ -118,10 +179,10 @@ export abstract class ReadReceipt< // The public receipt is more likely to drift out of date so the private // one has precedence - if (!comparison) return privateReadReceipt?.eventId ?? publicReadReceipt?.eventId ?? null; + if (!comparison) return privateReadReceipt ?? publicReadReceipt ?? null; // If public read receipt is older, return the private one - return (comparison < 0 ? privateReadReceipt?.eventId : publicReadReceipt?.eventId) ?? null; + return (comparison < 0 ? privateReadReceipt : publicReadReceipt) ?? null; } public addReceiptToStructure( @@ -229,6 +290,8 @@ export abstract class ReadReceipt< public abstract setUnread(type: NotificationCountType, count: number): void; + public abstract findEventById(eventId: string): MatrixEvent | undefined; + /** * This issue should also be addressed on synapse's side and is tracked as part * of https://github.com/matrix-org/synapse/issues/14837 From 5cfe82e9d541673156e88474eb4e4cd5376460d7 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Tue, 31 Oct 2023 14:08:35 +0000 Subject: [PATCH 02/26] Remove extra whitespace from log message --- src/models/read-receipt.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/models/read-receipt.ts b/src/models/read-receipt.ts index 2684d7916a3..56d93b4ccee 100644 --- a/src/models/read-receipt.ts +++ b/src/models/read-receipt.ts @@ -156,9 +156,9 @@ export abstract class ReadReceipt< // The thread ID doesn't match up logger.warn( - `Ignoring receipt because its thread_id (${receipt.data.thread_id}) disagrees \ - with the thread root (${event.threadRootId}) of the referenced event \ - (event ID = ${receipt.eventId})`, + `Ignoring receipt because its thread_id (${receipt.data.thread_id}) disagrees ` + + `with the thread root (${event.threadRootId}) of the referenced event ` + + `(event ID = ${receipt.eventId})`, ); return false; } From f5423152cc34eec6c3ec663ea0d88a3dd023619f Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Tue, 31 Oct 2023 14:08:52 +0000 Subject: [PATCH 03/26] Unit tests for ignoring invalid receipts --- spec/unit/room.spec.ts | 245 +++++++++++++++++++++++++++-------------- 1 file changed, 165 insertions(+), 80 deletions(-) diff --git a/spec/unit/room.spec.ts b/spec/unit/room.spec.ts index 9e35bf124cf..d2a4060c537 100644 --- a/spec/unit/room.spec.ts +++ b/spec/unit/room.spec.ts @@ -3148,110 +3148,195 @@ describe("Room", function () { const client = new TestClient(userA).client; const room = new Room(roomId, client, userA); - it("handles missing receipt type", () => { - room.getReadReceiptForUserId = (userId, ignore, receiptType): WrappedReceipt | null => { - return receiptType === ReceiptType.ReadPrivate ? ({ eventId: "eventId" } as WrappedReceipt) : null; - }; - room.findEventById = jest.fn().mockReturnValue({} as MatrixEvent); - expect(room.getEventReadUpTo(userA)).toEqual("eventId"); - }); + describe("invalid receipts", () => { + beforeEach(() => { + // Clear the spies on logger.warn + jest.clearAllMocks(); + }); - describe("prefers newer receipt", () => { - it("should compare correctly using timelines", () => { - room.getReadReceiptForUserId = (userId, ignore, receiptType): WrappedReceipt | null => { - if (receiptType === ReceiptType.ReadPrivate) { - return { eventId: "eventId1" } as WrappedReceipt; - } - if (receiptType === ReceiptType.Read) { - return { eventId: "eventId2" } as WrappedReceipt; - } - return null; + it("ignores receipts pointing at missing events", () => { + // Given a receipt exists + room.getReadReceiptForUserId = (): WrappedReceipt | null => { + return { eventId: "missingEventId" } as WrappedReceipt; }; + // But the event ID it contains does not refer to an event we have + room.findEventById = jest.fn().mockReturnValue(null); - for (let i = 1; i <= 2; i++) { - room.getUnfilteredTimelineSet = () => - ({ - compareEventOrdering: (event1: string) => { - return event1 === `eventId${i}` ? 1 : -1; - }, - findEventById: jest.fn().mockReturnValue({} as MatrixEvent), - } as unknown as EventTimelineSet); + // When we ask what they have read + // Then we say "nothing" + expect(room.getEventReadUpTo(userA)).toBeNull(); + expect(logger.warn).toHaveBeenCalledWith("Ignoring receipt for missing event with id missingEventId"); + }); - expect(room.getEventReadUpTo(userA)).toEqual(`eventId${i}`); - } + it("ignores receipts pointing at the wrong thread", () => { + // Given a threaded receipt exists + room.getReadReceiptForUserId = (): WrappedReceipt | null => { + return { eventId: "wrongThreadEventId", data: { ts: 0, thread_id: "thread1" } } as WrappedReceipt; + }; + // But the event it refers to is in a thread + room.findEventById = jest.fn().mockReturnValue({ threadRootId: "thread2" } as MatrixEvent); + + // When we ask what they have read + // Then we say "nothing" + expect(room.getEventReadUpTo(userA)).toBeNull(); + expect(logger.warn).toHaveBeenCalledWith( + "Ignoring receipt because its thread_id (thread1) disagrees with the thread root (thread2) " + + "of the referenced event (event ID = wrongThreadEventId)", + ); }); - describe("correctly compares by timestamp", () => { - it("should correctly compare, if we have all receipts", () => { - for (let i = 1; i <= 2; i++) { - room.getUnfilteredTimelineSet = () => - ({ - compareEventOrdering: () => null, - findEventById: jest.fn().mockReturnValue({} as MatrixEvent), - } as unknown as EventTimelineSet); - room.getReadReceiptForUserId = (userId, ignore, receiptType): WrappedReceipt | null => { - if (receiptType === ReceiptType.ReadPrivate) { - return { eventId: "eventId1", data: { ts: i === 1 ? 2 : 1 } } as WrappedReceipt; - } - if (receiptType === ReceiptType.Read) { - return { eventId: "eventId2", data: { ts: i === 2 ? 2 : 1 } } as WrappedReceipt; - } - return null; - }; + it("accepts unthreaded receipts pointing at an event in a thread", () => { + // Given an unthreaded receipt exists + room.getReadReceiptForUserId = (): WrappedReceipt | null => { + return { eventId: "inThreadEventId" } as WrappedReceipt; + }; + // And the event it refers to is in a thread + room.findEventById = jest.fn().mockReturnValue({ threadRootId: "thread2" } as MatrixEvent); - expect(room.getEventReadUpTo(userA)).toEqual(`eventId${i}`); - } - }); + // When we ask what they have read + // Then we say the event + expect(room.getEventReadUpTo(userA)).toEqual("inThreadEventId"); + }); - it("should correctly compare, if private read receipt is missing", () => { - room.getUnfilteredTimelineSet = () => - ({ - compareEventOrdering: () => null, - findEventById: jest.fn().mockReturnValue({} as MatrixEvent), - } as unknown as EventTimelineSet); - room.getReadReceiptForUserId = (userId, ignore, receiptType): WrappedReceipt | null => { - if (receiptType === ReceiptType.Read) { - return { eventId: "eventId2", data: { ts: 1 } } as WrappedReceipt; - } - return null; - }; + it("accepts main thread receipts pointing at an event in main timeline", () => { + // Given a threaded receipt exists, in main thread + room.getReadReceiptForUserId = (): WrappedReceipt | null => { + return { eventId: "mainThreadEventId", data: { ts: 12, thread_id: "main" } } as WrappedReceipt; + }; + // And the event it refers to is in a thread + room.findEventById = jest.fn().mockReturnValue({ threadRootId: undefined } as MatrixEvent); - expect(room.getEventReadUpTo(userA)).toEqual(`eventId2`); - }); + // When we ask what they have read + // Then we say the event + expect(room.getEventReadUpTo(userA)).toEqual("mainThreadEventId"); }); - describe("fallback precedence", () => { - beforeAll(() => { - room.getUnfilteredTimelineSet = () => - ({ - compareEventOrdering: () => null, - findEventById: jest.fn().mockReturnValue({} as MatrixEvent), - } as unknown as EventTimelineSet); - }); + it("accepts main thread receipts pointing at a thread root", () => { + // Given a threaded receipt exists, in main thread + room.getReadReceiptForUserId = (): WrappedReceipt | null => { + return { eventId: "rootId", data: { ts: 12, thread_id: "main" } } as WrappedReceipt; + }; + // And the event it refers to is in a thread, because it is a thread root + room.findEventById = jest + .fn() + .mockReturnValue({ isThreadRoot: true, threadRootId: "thread1" } as MatrixEvent); - it("should give precedence to m.read.private", () => { + // When we ask what they have read + // Then we say the event + expect(room.getEventReadUpTo(userA)).toEqual("rootId"); + }); + }); + + describe("valid receipts", () => { + beforeEach(() => { + // When we look up the event referred to by the receipt, it exists + room.findEventById = jest.fn().mockReturnValue({} as MatrixEvent); + }); + + it("handles missing receipt type", () => { + room.getReadReceiptForUserId = (userId, ignore, receiptType): WrappedReceipt | null => { + return receiptType === ReceiptType.ReadPrivate ? ({ eventId: "eventId" } as WrappedReceipt) : null; + }; + expect(room.getEventReadUpTo(userA)).toEqual("eventId"); + }); + + describe("prefers newer receipt", () => { + it("should compare correctly using timelines", () => { room.getReadReceiptForUserId = (userId, ignore, receiptType): WrappedReceipt | null => { if (receiptType === ReceiptType.ReadPrivate) { - return { eventId: "eventId1", data: { ts: 123 } }; + return { eventId: "eventId1" } as WrappedReceipt; } if (receiptType === ReceiptType.Read) { - return { eventId: "eventId2", data: { ts: 123 } }; + return { eventId: "eventId2" } as WrappedReceipt; } return null; }; - expect(room.getEventReadUpTo(userA)).toEqual(`eventId1`); + for (let i = 1; i <= 2; i++) { + room.getUnfilteredTimelineSet = () => + ({ + compareEventOrdering: (event1: string) => { + return event1 === `eventId${i}` ? 1 : -1; + }, + findEventById: jest.fn().mockReturnValue({} as MatrixEvent), + } as unknown as EventTimelineSet); + + expect(room.getEventReadUpTo(userA)).toEqual(`eventId${i}`); + } }); - it("should give precedence to m.read", () => { - room.getReadReceiptForUserId = (userId, ignore, receiptType): WrappedReceipt | null => { - if (receiptType === ReceiptType.Read) { - return { eventId: "eventId3" } as WrappedReceipt; + describe("correctly compares by timestamp", () => { + it("should correctly compare, if we have all receipts", () => { + for (let i = 1; i <= 2; i++) { + room.getUnfilteredTimelineSet = () => + ({ + compareEventOrdering: () => null, + findEventById: jest.fn().mockReturnValue({} as MatrixEvent), + } as unknown as EventTimelineSet); + room.getReadReceiptForUserId = (userId, ignore, receiptType): WrappedReceipt | null => { + if (receiptType === ReceiptType.ReadPrivate) { + return { eventId: "eventId1", data: { ts: i === 1 ? 2 : 1 } } as WrappedReceipt; + } + if (receiptType === ReceiptType.Read) { + return { eventId: "eventId2", data: { ts: i === 2 ? 2 : 1 } } as WrappedReceipt; + } + return null; + }; + + expect(room.getEventReadUpTo(userA)).toEqual(`eventId${i}`); } - return null; - }; + }); + + it("should correctly compare, if private read receipt is missing", () => { + room.getUnfilteredTimelineSet = () => + ({ + compareEventOrdering: () => null, + findEventById: jest.fn().mockReturnValue({} as MatrixEvent), + } as unknown as EventTimelineSet); + room.getReadReceiptForUserId = (userId, ignore, receiptType): WrappedReceipt | null => { + if (receiptType === ReceiptType.Read) { + return { eventId: "eventId2", data: { ts: 1 } } as WrappedReceipt; + } + return null; + }; + + expect(room.getEventReadUpTo(userA)).toEqual(`eventId2`); + }); + }); - expect(room.getEventReadUpTo(userA)).toEqual(`eventId3`); + describe("fallback precedence", () => { + beforeAll(() => { + room.getUnfilteredTimelineSet = () => + ({ + compareEventOrdering: () => null, + findEventById: jest.fn().mockReturnValue({} as MatrixEvent), + } as unknown as EventTimelineSet); + }); + + it("should give precedence to m.read.private", () => { + room.getReadReceiptForUserId = (userId, ignore, receiptType): WrappedReceipt | null => { + if (receiptType === ReceiptType.ReadPrivate) { + return { eventId: "eventId1", data: { ts: 123 } }; + } + if (receiptType === ReceiptType.Read) { + return { eventId: "eventId2", data: { ts: 123 } }; + } + return null; + }; + + expect(room.getEventReadUpTo(userA)).toEqual(`eventId1`); + }); + + it("should give precedence to m.read", () => { + room.getReadReceiptForUserId = (userId, ignore, receiptType): WrappedReceipt | null => { + if (receiptType === ReceiptType.Read) { + return { eventId: "eventId3" } as WrappedReceipt; + } + return null; + }; + + expect(room.getEventReadUpTo(userA)).toEqual(`eventId3`); + }); }); }); }); From 9fa5374376204644803b39ea476dbc2b07080112 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Wed, 1 Nov 2023 12:27:26 +0000 Subject: [PATCH 04/26] Improve comments around getEventReadUpTo --- src/models/read-receipt.ts | 82 ++++++++++++++++++++++++++++---------- 1 file changed, 60 insertions(+), 22 deletions(-) diff --git a/src/models/read-receipt.ts b/src/models/read-receipt.ts index 56d93b4ccee..a9aa63f5765 100644 --- a/src/models/read-receipt.ts +++ b/src/models/read-receipt.ts @@ -95,15 +95,19 @@ export abstract class ReadReceipt< } /** - * Get the ID of the event that a given user has read up to, or null if we - * have received no read receipts from them, or if the receipt we have - * points at an event we don't have, or if the thread ID in the receipt does - * not match the thread root of the referenced event. + * Get the ID of the event that a given user has read up to, or null if: + * - we have received no read receipts for them, or + * - the receipt we have points at an event we don't have, or + * - the thread ID in the receipt does not match the thread root of the + * referenced event. + * + * (The event might not exist if it is not loaded, and the thread ID might + * not match if the event has moved thread because it was redacted.) * * @param userId - The user ID to get read receipt event ID for * @param ignoreSynthesized - If true, return only receipts that have been - * sent by the server, not implicit ones generated - * by the JS SDK. + * sent by the server, not implicit ones generated + * by the JS SDK. * @returns ID of the latest existing event that the given user has read, or null. */ public getEventReadUpTo(userId: string, ignoreSynthesized = false): string | null { @@ -122,48 +126,77 @@ export abstract class ReadReceipt< * threadRootId is consistent with the thread information in the receipt. */ private receiptPointsAtConsistentEvent(receipt: WrappedReceipt): boolean { - // If the receipt points at a non-existent event, it's not consistent const event = this.findEventById(receipt.eventId); if (!event) { + // If the receipt points at a non-existent event, we have 2 + // possibilities: + // + // 1. We don't have the event because it's not loaded yet - probably + // it's old and we're best off ignoring the receipt - we can just + // send a new one when we read a new event. + // + // 2. We have a bug e.g. we misclassified this event into the wrong + // thread. logger.warn(`Ignoring receipt for missing event with id ${receipt.eventId}`); + + // This receipt is not "valid" because it doesn't point at an event + // we have. We want to pretend it doesn't exist. return false; } - // If this is an unthreaded receipt, we're good if (!receipt.data?.thread_id) { + // If this is an unthreaded receipt, it could point at any event, so + // there is no need to validate further - this receipt is valid. return true; } + // Otherwise it is a threaded receipt... - // If the receipt is for the main timeline if (receipt.data.thread_id === MAIN_ROOM_TIMELINE) { - // If the event is not in a thread, this checks out - if (!event.threadRootId) { + // The receipt is for the main timeline: we check that the event is + // in the main timeline. + + // There are two ways to know an event is in the main timeline: + // either it has no threadRootId, or it is a thread root. + // (Note: it's a little odd because the thread root is in the main + // timeline, but it still has a threadRootId.) + const eventIsInMainTimeline = !event.threadRootId || event.isThreadRoot; + + if (eventIsInMainTimeline) { + // The receipt is for the main timeline, and so is the event, so + // the receipt is valid. return true; } + } else { + // The receipt is for a different thread (not the main timeline) - // If the event is a thread root, we need a special case because - // event.threadRootId won't match, but thread root events are - // actually on the main timeline, so we return successfully. - if (event.isThreadRoot) { + if (event.threadRootId === receipt.data.thread_id) { + // If the receipt and event agree on the thread ID, the receipt + // is valid. return true; } } - // If the receipt is for the correct thread, we are good. - if (event.threadRootId === receipt.data.thread_id) { - return true; - } - - // The thread ID doesn't match up + // The receipt thread ID disagrees with the event thread ID. There are 2 + // possibilities: + // + // 1. The event moved to a different thread after the receipt was + // created. This can happen if the event was redacted because that + // moves it to the main timeline. + // + // 2. There is a bug somewhere - either we put the event into the wrong + // thread, or someone sent an incorrect receipt. logger.warn( `Ignoring receipt because its thread_id (${receipt.data.thread_id}) disagrees ` + `with the thread root (${event.threadRootId}) of the referenced event ` + `(event ID = ${receipt.eventId})`, ); + + // This receipt is not "valid" because it disagrees with us about what + // thread the event is in. We want to pretend it doesn't exist. return false; } - private getLatestReceipt(userId: string, ignoreSynthesized = false): WrappedReceipt | null { + private getLatestReceipt(userId: string, ignoreSynthesized: boolean): WrappedReceipt | null { // XXX: This is very very ugly and I hope I won't have to ever add a new // receipt type here again. IMHO this should be done by the server in // some more intelligent manner or the client should just use timestamps @@ -290,6 +323,11 @@ export abstract class ReadReceipt< public abstract setUnread(type: NotificationCountType, count: number): void; + /** + * Look in this room/thread's timeline to find an event. If `this` is a + * room, we look in all threads, but if `this` is a thread, we look only + * inside this thread. + */ public abstract findEventById(eventId: string): MatrixEvent | undefined; /** From 9060e0ec576a731491341d7301e6739b0bcbfea8 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Wed, 1 Nov 2023 14:12:32 +0000 Subject: [PATCH 05/26] Re-instate second param to compareEventOrdering in test Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- spec/unit/room.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/unit/room.spec.ts b/spec/unit/room.spec.ts index d2a4060c537..481ccdcafe0 100644 --- a/spec/unit/room.spec.ts +++ b/spec/unit/room.spec.ts @@ -3255,7 +3255,7 @@ describe("Room", function () { for (let i = 1; i <= 2; i++) { room.getUnfilteredTimelineSet = () => ({ - compareEventOrdering: (event1: string) => { + compareEventOrdering: (event1: string, _event2: string) => { return event1 === `eventId${i}` ? 1 : -1; }, findEventById: jest.fn().mockReturnValue({} as MatrixEvent), From 3686d618e26f09d18fc281f9ca06b094150836fb Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Wed, 1 Nov 2023 14:20:37 +0000 Subject: [PATCH 06/26] Further improve comments around getEventReadUpTo --- src/models/read-receipt.ts | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/models/read-receipt.ts b/src/models/read-receipt.ts index a9aa63f5765..d1c0f10c9c3 100644 --- a/src/models/read-receipt.ts +++ b/src/models/read-receipt.ts @@ -128,7 +128,7 @@ export abstract class ReadReceipt< private receiptPointsAtConsistentEvent(receipt: WrappedReceipt): boolean { const event = this.findEventById(receipt.eventId); if (!event) { - // If the receipt points at a non-existent event, we have 2 + // If the receipt points at a non-existent event, we have multiple // possibilities: // // 1. We don't have the event because it's not loaded yet - probably @@ -137,6 +137,12 @@ export abstract class ReadReceipt< // // 2. We have a bug e.g. we misclassified this event into the wrong // thread. + // + // 3. The referenced event moved out of this thread (e.g. because it + // was deleted.) + // + // 4. The receipt had the incorrect thread ID (due to a bug in a + // client, or malicious behaviour). logger.warn(`Ignoring receipt for missing event with id ${receipt.eventId}`); // This receipt is not "valid" because it doesn't point at an event @@ -185,6 +191,11 @@ export abstract class ReadReceipt< // // 2. There is a bug somewhere - either we put the event into the wrong // thread, or someone sent an incorrect receipt. + // + // In many cases, we won't get here because the call to findEventById + // would have already returned null. We include this check to cover + // cases when `this` is a room, meaning findEventById will find events + // in any thread, and to be defensive against unforeseen code paths. logger.warn( `Ignoring receipt because its thread_id (${receipt.data.thread_id}) disagrees ` + `with the thread root (${event.threadRootId}) of the referenced event ` + From 4ee0fba48eac50ac4d78f653eee211334c7f1bb0 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Fri, 3 Nov 2023 15:58:37 +0000 Subject: [PATCH 07/26] WIP --- spec/test-utils/thread.ts | 20 ++ spec/unit/models/thread.spec.ts | 20 +- spec/unit/room.spec.ts | 65 +++- src/models/compare-events-in-stream-order.ts | 137 +++++++ src/models/read-receipt.ts | 45 --- src/models/room-receipts.ts | 353 +++++++++++++++++++ src/models/room.ts | 31 ++ src/models/thread.ts | 15 +- 8 files changed, 622 insertions(+), 64 deletions(-) create mode 100644 src/models/compare-events-in-stream-order.ts create mode 100644 src/models/room-receipts.ts diff --git a/spec/test-utils/thread.ts b/spec/test-utils/thread.ts index 2b84239d367..58e89cf526c 100644 --- a/spec/test-utils/thread.ts +++ b/spec/test-utils/thread.ts @@ -161,3 +161,23 @@ export const mkThread = ({ return { thread, rootEvent, events }; }; + +/** + * Create a thread, and make sure the events added to the thread and the room's + * timeline as if they came in via sync. + * + * Note that mkThread doesn't actually add the events properly to the room. + */ +export const populateThread = async ({ + room, + client, + authorId, + participantUserIds, + length = 2, + ts = 1, +}: MakeThreadProps): Promise => { + const ret = mkThread({ room, client, authorId, participantUserIds, length, ts }); + ret.thread.initialEventsFetched = true; + room.addLiveEvents(ret.events); + return ret; +}; diff --git a/spec/unit/models/thread.spec.ts b/spec/unit/models/thread.spec.ts index 05bd12200e6..f8858e60c56 100644 --- a/spec/unit/models/thread.spec.ts +++ b/spec/unit/models/thread.spec.ts @@ -19,7 +19,7 @@ import { mocked } from "jest-mock"; import { MatrixClient, PendingEventOrdering } from "../../../src/client"; import { Room, RoomEvent } from "../../../src/models/room"; import { FeatureSupport, Thread, THREAD_RELATION_TYPE, ThreadEvent } from "../../../src/models/thread"; -import { makeThreadEvent, mkThread } from "../../test-utils/thread"; +import { makeThreadEvent, mkThread, populateThread } from "../../test-utils/thread"; import { TestClient } from "../../TestClient"; import { emitPromise, mkEdit, mkMessage, mkReaction, mock } from "../../test-utils/test-utils"; import { Direction, EventStatus, EventType, MatrixEvent } from "../../../src"; @@ -148,8 +148,8 @@ describe("Thread", () => { expect(thread.hasUserReadEvent(myUserId, events.at(-1)!.getId() ?? "")).toBeTruthy(); }); - it("considers other events with no RR as unread", () => { - const { thread, events } = mkThread({ + it("considers other events with no RR as unread", async () => { + const { thread, events, threadRoot } = await populateThread({ room, client, authorId: myUserId, @@ -158,6 +158,20 @@ describe("Thread", () => { ts: 190, }); + const receipt = new MatrixEvent({ + type: "m.receipt", + room_id: room.roomId, + content: { + // unthreaded receipt for the thread root + [threadRoot.getId()]: { + [ReceiptType.Read]: { + [myUserId]: { ts: 200 }, + }, + }, + }, + }); + room.addReceipt(receipt); + // Before alice's last unthreaded receipt expect(thread.hasUserReadEvent("@alice:example.org", events.at(1)!.getId() ?? "")).toBeTruthy(); diff --git a/spec/unit/room.spec.ts b/spec/unit/room.spec.ts index 481ccdcafe0..0ffccbe72f0 100644 --- a/spec/unit/room.spec.ts +++ b/spec/unit/room.spec.ts @@ -1743,13 +1743,72 @@ describe("Room", function () { }); describe("hasUserReadUpTo", function () { - it("should acknowledge if an event has been read", function () { + it("returns true if there is a receipt for this event (main timeline)", function () { const ts = 13787898424; + room.addLiveEvents([eventToAck]); room.addReceipt(mkReceipt(roomId, [mkRecord(eventToAck.getId()!, "m.read", userB, ts)])); - room.findEventById = jest.fn().mockReturnValue({} as MatrixEvent); expect(room.hasUserReadEvent(userB, eventToAck.getId()!)).toEqual(true); }); - it("return false for an unknown event", function () { + + it("returns true if there is a receipt for a later event (main timeline)", async function () { + // Given some events exist in the room + const events: MatrixEvent[] = [ + utils.mkMessage({ + room: roomId, + user: userA, + msg: "1111", + event: true, + }), + utils.mkMessage({ + room: roomId, + user: userA, + msg: "2222", + event: true, + }), + utils.mkMessage({ + room: roomId, + user: userA, + msg: "3333", + event: true, + }), + ]; + await room.addLiveEvents(events); + + // When I add a receipt for the latest one + room.addReceipt(mkReceipt(roomId, [mkRecord(events[2].getId()!, "m.read", userB, 102)])); + + // Then the older ones are read too + expect(room.hasUserReadEvent(userB, events[0].getId()!)).toEqual(true); + expect(room.hasUserReadEvent(userB, events[1].getId()!)).toEqual(true); + }); + + describe("threads enabled", () => { + let oldSupportsThreads: () => boolean; + + beforeEach(() => { + oldSupportsThreads = room.client.supportsThreads; + room.client.supportsThreads = jest.fn().mockReturnValue(true); + }); + + afterEach(() => { + room.client.supportsThreads = oldSupportsThreads; + }); + + it("returns true if there is an unthreaded receipt for a later event in a thread", async () => { + // Given a thread exists in the room + const { thread, events } = mkThread({ room, length: 3 }); + thread.initialEventsFetched = true; + await room.addLiveEvents(events); + + // When I add an unthreaded receipt for the latest thread message + room.addReceipt(mkReceipt(roomId, [mkRecord(events[2].getId()!, "m.read", userB, 102)])); + + // Then the main timeline message is read + expect(room.hasUserReadEvent(userB, events[0].getId()!)).toEqual(true); + }); + }); + + it("returns false for an unknown event", function () { expect(room.hasUserReadEvent(userB, "unknown_event")).toEqual(false); }); }); diff --git a/src/models/compare-events-in-stream-order.ts b/src/models/compare-events-in-stream-order.ts new file mode 100644 index 00000000000..da3a8ad5326 --- /dev/null +++ b/src/models/compare-events-in-stream-order.ts @@ -0,0 +1,137 @@ +/* +Copyright 2023 The Matrix.org Foundation C.I.C. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +import { MatrixEvent } from "./event"; +import { Room } from "./room"; +import { Thread } from "./thread"; + +/** + * Determine the order of two events in a room. + * + * In principle this should use the same order as the server, but in practice + * this is difficult for events that were not received over the Sync API. See + * MSC4033 for details. + * + * This implementation leans on the order of events within their timelines, and + * falls back to comparing event timestamps when they are in different + * timelines. + * + * See https://github.com/matrix-org/matrix-js-sdk/issues/3325 for where we are + * tracking the work to fix this. + * + * @param room - the room we are looking in + * @param leftEventId - the id of the first event + * @param rightEventId - the id of the second event + + * @returns -1 if left \< right, 1 if left \> right, 0 if left == right, null if + * we can't tell (because we can't find the events, or they are both from old + * timelines). + */ +export function compareEventOrdering(room: Room, leftEventId: string, rightEventId: string): number | null { + const leftEvent = room.findEventById(leftEventId); + const rightEvent = room.findEventById(rightEventId); + + if (!leftEvent || !rightEvent) { + // Without the events themselves, we can't find their thread or + // timeline, or guess based on timestamp, so we just don't know. + return null; + } + + // Check whether the events are in threads. + const leftThread = leftEvent.getThread(); + const rightThread = rightEvent.getThread(); + + if (leftThread || rightThread) { + // At least one event is in a thread, so we can't use the room's + // unfiltered timeline set. + return compareEventsInThreads(leftEventId, rightEventId, leftEvent, rightEvent, leftThread, rightThread); + } else { + return compareUnthreadedEvents(room, leftEventId, rightEventId, leftEvent, rightEvent); + } +} + +function compareUnthreadedEvents( + room: Room, + leftEventId: string, + rightEventId: string, + leftEvent: MatrixEvent, + rightEvent: MatrixEvent, +): number | null { + // Get the timeline set that contains all the events. + const timelineSet = room.getUnfilteredTimelineSet(); + + // If they are in the same timeline, compareEventOrdering does what we need + const compareSameTimeline = timelineSet.compareEventOrdering(leftEventId, rightEventId); + if (compareSameTimeline !== null) { + return compareSameTimeline; + } + + // Find which timeline each event is in. Refuse to provide an ordering if we + // can't find either of the events. + + const leftTimeline = timelineSet.getTimelineForEvent(leftEventId); + if (leftTimeline === timelineSet.getLiveTimeline()) { + // The left event is part of the live timeline, so it must be after the + // right event (since they are not in the same timeline or we would have + // returned after compareEventOrdering. + return 1; + } + + const rightTimeline = timelineSet.getTimelineForEvent(rightEventId); + if (rightTimeline === timelineSet.getLiveTimeline()) { + // The right event is part of the live timeline, so it must be after the + // left event. + return -1; + } + + // They are in older timeline sets (because they were fetched by paging up). + return guessOrderBasedOnTimestamp(leftEvent, rightEvent); +} + +function compareEventsInThreads( + leftEventId: string, + rightEventId: string, + leftEvent: MatrixEvent, + rightEvent: MatrixEvent, + leftThread: Thread | undefined, + rightThread: Thread | undefined, +): number | null { + if (leftThread && leftThread === rightThread) { + // They are in the same thread, so we can ask the thread's timeline to + // figure it out for us + return leftThread.timelineSet.compareEventOrdering(leftEventId, rightEventId); + } else { + return guessOrderBasedOnTimestamp(leftEvent, rightEvent); + } +} + +/** + * Guess the order of events based on server timestamp. This is not good, but + * difficult to avoid without MSC4033. + * + * See https://github.com/matrix-org/matrix-js-sdk/issues/3325 + */ +function guessOrderBasedOnTimestamp(leftEvent: MatrixEvent, rightEvent: MatrixEvent): number { + const leftTs = leftEvent.getTs(); + const rightTs = rightEvent.getTs(); + if (leftTs < rightTs) { + return -1; + } else if (leftTs > rightTs) { + return 1; + } else { + return 0; + } +} diff --git a/src/models/read-receipt.ts b/src/models/read-receipt.ts index d1c0f10c9c3..4c2575102d3 100644 --- a/src/models/read-receipt.ts +++ b/src/models/read-receipt.ts @@ -387,49 +387,4 @@ export abstract class ReadReceipt< return receipt.userId; }); } - - /** - * Determines if the given user has read a particular event ID with the known - * history of the room. This is not a definitive check as it relies only on - * what is available to the room at the time of execution. - * @param userId - The user ID to check the read state of. - * @param eventId - The event ID to check if the user read. - * @returns True if the user has read the event, false otherwise. - */ - public hasUserReadEvent(userId: string, eventId: string): boolean { - const readUpToId = this.getEventReadUpTo(userId, false); - if (readUpToId === eventId) return true; - - if ( - this.timeline?.length && - this.timeline[this.timeline.length - 1].getSender() && - this.timeline[this.timeline.length - 1].getSender() === userId - ) { - // It doesn't matter where the event is in the timeline, the user has read - // it because they've sent the latest event. - return true; - } - - for (let i = this.timeline?.length - 1; i >= 0; --i) { - const ev = this.timeline[i]; - - // If we encounter the target event first, the user hasn't read it - // however if we encounter the readUpToId first then the user has read - // it. These rules apply because we're iterating bottom-up. - if (ev.getId() === eventId) return false; - if (ev.getId() === readUpToId) return true; - } - - // We don't know if the user has read it, so assume not. - return false; - } - - /** - * Returns the most recent unthreaded receipt for a given user - * @param userId - the MxID of the User - * @returns an unthreaded Receipt. Can be undefined if receipts have been disabled - * or a user chooses to use private read receipts (or we have simply not received - * a receipt from this user yet). - */ - public abstract getLastUnthreadedReceiptFor(userId: string): Receipt | undefined; } diff --git a/src/models/room-receipts.ts b/src/models/room-receipts.ts new file mode 100644 index 00000000000..32282e3e7f3 --- /dev/null +++ b/src/models/room-receipts.ts @@ -0,0 +1,353 @@ +/* +Copyright 2023 The Matrix.org Foundation C.I.C. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +import { MAIN_ROOM_TIMELINE, Receipt, ReceiptContent } from "../@types/read_receipts"; +import { threadIdForReceipt } from "../client"; +import { Room } from "./room"; + +/** + * The latest receipts we have for a room. + */ +export class RoomReceipts { + private room: Room; + private threadedReceipts: ThreadedReceipts; + private unthreadedReceipts: ReceiptsByUser; + private danglingReceipts: DanglingReceipts; + + public constructor(room: Room) { + this.room = room; + this.threadedReceipts = new ThreadedReceipts(room); + this.unthreadedReceipts = new ReceiptsByUser(room); + this.danglingReceipts = new DanglingReceipts(); + } + + /** + * Remember the receipt information supplied. For each receipt: + * + * If we don't have the event for this receipt, store it as "dangling" so we + * can process it later. + * + * Otherwise store it per-user in either the threaded store for its + * thread_id, or the unthreaded store if there is no thread_id. + * + * Ignores any receipt that is before an existing receipt for the same user + * (in the same thread, if applicable). "Before" is defined by the + * unfilteredTimelineSet of the room. + */ + public add(receiptContent: ReceiptContent, synthetic: boolean): void { + /* + Transform this structure: + { + "$EVENTID": { + "m.read|m.read.private": { + "@user:example.org": { + "ts": 1661, + "thread_id": "main|$THREAD_ROOT_ID" // or missing/undefined for an unthreaded receipt + } + } + }, + ... + } + into maps of: + threaded :: threadid :: userId :: ReceiptInfo + unthreaded :: userId :: ReceiptInfo + dangling :: eventId :: DanglingReceipt + */ + for (const [eventId, eventReceipt] of Object.entries(receiptContent)) { + for (const [receiptType, receiptsByUser] of Object.entries(eventReceipt)) { + for (const [userId, receipt] of Object.entries(receiptsByUser)) { + const referencedEvent = this.room.findEventById(eventId); + if (!referencedEvent) { + this.danglingReceipts.add( + new DanglingReceipt(eventId, receiptType, userId, receipt, synthetic), + ); + } else if (receipt.thread_id) { + this.threadedReceipts.set( + receipt.thread_id, + eventId, + receiptType, + userId, + receipt.ts, + synthetic, + ); + } else { + this.unthreadedReceipts.set(eventId, receiptType, userId, receipt.ts, synthetic); + } + } + } + } + } + + public hasUserReadEvent(userId: string, eventId: string): boolean { + const unthreaded = this.unthreadedReceipts.get(userId); + if (unthreaded) { + if (isAfterOrSame(unthreaded.eventId, eventId, this.room)) { + // The unthreaded receipt is after this event, so we have read it. + return true; + } + } + + const event = this.room.findEventById(eventId); + if (!event) { + // We don't know whether the user has read it - default to caution and say no. + return false; + } + + const threadId = threadIdForReceipt(event); + const threaded = this.threadedReceipts.get(threadId, userId); + if (threaded) { + if (isAfterOrSame(threaded.eventId, eventId, this.room)) { + // The threaded receipt is after this event, so we have read it. + return true; + } + } + + const timeline = + threadId === MAIN_ROOM_TIMELINE + ? this.room.getLiveTimeline().getEvents() + : this.room.getThread(threadId)?.timeline; + if (timeline?.at(-1)?.getSender() === userId) { + // The user sent the latest message in this event's thread, so we + // consider everything in the thread to be read. + return true; + } + + // Neither of the receipts were after the event, so it's unread. + return false; + } +} + +// --- implementation details --- + +/** + * The information "inside" a receipt once it has been stored inside + * RoomReceipts - what eventId it refers to, its type, and its ts. + * + * Does not contain userId or threadId since these are stored as keys of the + * maps in RoomReceipts. + */ +class ReceiptInfo { + public constructor(public eventId: string, public receiptType: string, public ts: number) {} +} + +/** + * Everything we know about a receipt that is "dangling" because we can't find + * the event to which it refers. + */ +class DanglingReceipt { + public constructor( + public eventId: string, + public receiptType: string, + public userId: string, + public receipt: Receipt, + public synthetic: boolean, + ) {} +} + +class UserReceipts { + private room: Room; + + /** + * The real receipt for this user. + */ + private real: ReceiptInfo | undefined; + + /** + * The synthetic receipt for this user. If this is defined, it is later than real. + */ + private synthetic: ReceiptInfo | undefined; + + public constructor(room: Room) { + this.room = room; + this.real = undefined; + this.synthetic = undefined; + } + + public set(synthetic: boolean, receiptInfo: ReceiptInfo): void { + if (synthetic) { + this.synthetic = receiptInfo; + } else { + this.real = receiptInfo; + } + + // Preserve the invariant: synthetic is only defined if it's later than real + if (this.synthetic && this.real) { + if (isAfterOrSame(this.real.eventId, this.synthetic.eventId, this.room)) { + this.synthetic = undefined; + } + } + } + + /** + * Return the latest receipt we have - synthetic if we have one (and it's + * later), otherwise real. + */ + public get(): ReceiptInfo | undefined { + // Relies on the invariant that synthetic is only defined if it's later than real. + return this.synthetic ?? this.real; + } + + /** + * Return the latest receipt we have of the specified type (synthetic or not). + */ + public getByType(synthetic: boolean): ReceiptInfo | undefined { + return synthetic ? this.synthetic : this.real; + } +} + +/** + * The latest receipt info we have, either for a single thread, or all the + * unthreaded receipts for a room. + * + * userId: ReceiptInfo + */ +class ReceiptsByUser { + private room: Room; + private data: Map; + + public constructor(room: Room) { + this.room = room; + this.data = new Map(); + } + + /** + * Add the supplied receipt to our structure, if it is not earlier than the + * one we already hold for this user. + */ + public set(eventId: string, receiptType: string, userId: string, ts: number, synthetic: boolean): void { + const userReceipts = getOrCreate(this.data, userId, () => new UserReceipts(this.room)); + + const existingReceipt = userReceipts.getByType(synthetic); + if (existingReceipt) { + const unfilteredTimelineSet = this.room.getUnfilteredTimelineSet(); + const comparison = unfilteredTimelineSet.compareEventOrdering(eventId, existingReceipt.eventId); + + if (comparison !== null && comparison < 0) { + // The new receipt is before the existing one - don't store it. + return; + } + } + + // Either there was no (valid) existing receipt, or it was before this + // one - add the new receipt. + userReceipts.set(synthetic, new ReceiptInfo(eventId, receiptType, ts)); + } + + /** + * Find the latest receipt we have for this user. (Note - there is only one + * receipt per user, because we are already inside a specific thread or + * unthreaded list.) + * + * If there is a later synthetic receipt for this user, return that. + * Otherwise, return the real receipt. + * + * @returns the found receipt info, or undefined if we have no receipt for this user. + */ + public get(userId: string): ReceiptInfo | undefined { + return this.data.get(userId)?.get(); + } +} + +/** + * The latest threaded receipts we have for a room. + */ +class ThreadedReceipts { + private room: Room; + private data: Map; + + public constructor(room: Room) { + this.room = room; + this.data = new Map(); + } + + /** + * Add the supplied receipt to our structure, if it is not earlier than one + * we already hold for this user in this thread. + */ + public set( + threadId: string, + eventId: string, + receiptType: string, + userId: string, + ts: number, + synthetic: boolean, + ): void { + const receiptsByUser = getOrCreate(this.data, threadId, () => new ReceiptsByUser(this.room)); + receiptsByUser.set(eventId, receiptType, userId, ts, synthetic); + } + + /** + * Find the latest threaded receipt for the supplied user in the supplied thread. + * + * @returns the found receipt info or undefined if we don't have one. + */ + public get(threadId: string, userId: string): ReceiptInfo | undefined { + return this.data.get(threadId)?.get(userId); + } + + ///** + // * Find the latest threaded receipt of the specified type for the supplied user in the supplied thread. + // * + // * @returns the found receipt info or undefined if we don't have one. + // */ + //public getByType(threadId: string, userId: string, synthetic: boolean): ReceiptInfo | undefined { + // return this.data.get(threadId)?.getByType(userId, synthetic); + //} +} + +/** + * All the receipts that we have received but can't process because we can't + * find the event they refer to. + * + * We hold on to them so we can process them if their event arrives later. + */ +class DanglingReceipts { + /** + * eventId: DanglingReceipt + */ + private data = new Map(); + + /** + * Remember the supplied dangling receipt. + */ + public add(danglingReceipt: DanglingReceipt): void { + this.data.set(danglingReceipt.eventId, danglingReceipt); + } +} + +function getOrCreate(m: Map, key: K, createFn: () => V): V { + const found = m.get(key); + if (found) { + return found; + } else { + const created = createFn(); + m.set(key, created); + return created; + } +} + +/** + * Is left after right in this room's unfiltered timeline? + * + * Only returns true if both events can be found, and left is after or the same + * as right. + * + * @returns left \>= right + */ +function isAfterOrSame(leftEventId: string, rightEventId: string, room: Room): boolean { + const comparison = room.compareEventOrdering(leftEventId, rightEventId); + return comparison !== null && comparison >= 0; +} diff --git a/src/models/room.ts b/src/models/room.ts index 4b78567c958..613337d380f 100644 --- a/src/models/room.ts +++ b/src/models/room.ts @@ -66,6 +66,9 @@ import { IStateEventWithRoomId } from "../@types/search"; import { RelationsContainer } from "./relations-container"; import { ReadReceipt, synthesizeReceipt } from "./read-receipt"; import { isPollEvent, Poll, PollEvent } from "./poll"; +import { RoomReceipts } from "./room-receipts"; +import { RoomEventsInStreamOrder } from "./room-events-in-stream-order"; +import { compareEventOrdering } from "./compare-events-in-stream-order"; // These constants are used as sane defaults when the homeserver doesn't support // the m.room_versions capability. In practice, KNOWN_SAFE_ROOM_VERSION should be @@ -432,6 +435,12 @@ export class Room extends ReadReceipt { */ private visibilityEvents = new Map(); + /** + * The latest receipts (synthetic and real) for each user in each thread + * (and unthreaded). + */ + private roomReceipts = new RoomReceipts(this); + /** * Construct a new Room. * @@ -2935,6 +2944,10 @@ export class Room extends ReadReceipt { */ public addReceipt(event: MatrixEvent, synthetic = false): void { const content = event.getContent(); + + this.roomReceipts.add(content, synthetic); + + // START OF TO-DELETE Object.keys(content).forEach((eventId: string) => { Object.keys(content[eventId]).forEach((receiptType: ReceiptType | string) => { Object.keys(content[eventId][receiptType]).forEach((userId: string) => { @@ -2996,6 +3009,7 @@ export class Room extends ReadReceipt { }); }); }); + // END OF TO-DELETE // send events after we've regenerated the structure & cache, otherwise things that // listened for the event would read stale data. @@ -3582,6 +3596,19 @@ export class Room extends ReadReceipt { return this.oldestThreadedReceiptTs; } + /** + * Determines if the given user has read a particular event ID with the known + * history of the room. This is not a definitive check as it relies only on + * what is available to the room at the time of execution. + * + * @param userId - The user ID to check the read state of. + * @param eventId - The event ID to check if the user read. + * @returns true if the user has read the event, false otherwise. + */ + public hasUserReadEvent(userId: string, eventId: string): boolean { + return this.roomReceipts.hasUserReadEvent(userId, eventId); + } + /** * Returns the most recent unthreaded receipt for a given user * @param userId - the MxID of the User @@ -3615,6 +3642,10 @@ export class Room extends ReadReceipt { thread.fixupNotifications(userId); } } + + public compareEventOrdering(leftEventId: string, rightEventId: string): number | null { + return compareEventOrdering(this, leftEventId, rightEventId); + } } // a map from current event status to a list of allowed next statuses diff --git a/src/models/thread.ts b/src/models/thread.ts index 6b8069c9a5f..1e7d90b4ce1 100644 --- a/src/models/thread.ts +++ b/src/models/thread.ts @@ -27,7 +27,7 @@ import { RoomState } from "./room-state"; import { ServerControlledNamespacedValue } from "../NamespacedValue"; import { logger } from "../logger"; import { ReadReceipt } from "./read-receipt"; -import { CachedReceiptStructure, Receipt, ReceiptType } from "../@types/read_receipts"; +import { CachedReceiptStructure, ReceiptType } from "../@types/read_receipts"; import { Feature, ServerSupport } from "../feature"; export enum ThreadEvent { @@ -813,23 +813,12 @@ export class Thread extends ReadReceipt Date: Tue, 7 Nov 2023 09:14:08 +0000 Subject: [PATCH 08/26] Fix typo in thread test --- spec/unit/models/thread.spec.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/unit/models/thread.spec.ts b/spec/unit/models/thread.spec.ts index f8858e60c56..3eb9592e0e3 100644 --- a/spec/unit/models/thread.spec.ts +++ b/spec/unit/models/thread.spec.ts @@ -149,7 +149,7 @@ describe("Thread", () => { }); it("considers other events with no RR as unread", async () => { - const { thread, events, threadRoot } = await populateThread({ + const { thread, events, rootEvent } = await populateThread({ room, client, authorId: myUserId, @@ -163,7 +163,7 @@ describe("Thread", () => { room_id: room.roomId, content: { // unthreaded receipt for the thread root - [threadRoot.getId()]: { + [rootEvent.getId()!]: { [ReceiptType.Read]: { [myUserId]: { ts: 200 }, }, From 26598e15b84bc048e87aaa22e3f4628b3c8e11ab Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Fri, 10 Nov 2023 10:53:42 +0000 Subject: [PATCH 09/26] Remove extraneous import --- src/models/room.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/models/room.ts b/src/models/room.ts index 6e13b38b9fc..c64d9e26766 100644 --- a/src/models/room.ts +++ b/src/models/room.ts @@ -67,7 +67,6 @@ import { RelationsContainer } from "./relations-container"; import { ReadReceipt, synthesizeReceipt } from "./read-receipt"; import { isPollEvent, Poll, PollEvent } from "./poll"; import { RoomReceipts } from "./room-receipts"; -import { RoomEventsInStreamOrder } from "./room-events-in-stream-order"; import { compareEventOrdering } from "./compare-events-in-stream-order"; // These constants are used as sane defaults when the homeserver doesn't support From 2bf53dfd4cb6dc901db57fa31230518b1775cd04 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Fri, 10 Nov 2023 12:19:42 +0000 Subject: [PATCH 10/26] First ideas of tests for RoomReceipts --- spec/unit/models/room-receipts.spec.ts | 146 +++++++++++++++++++++++++ 1 file changed, 146 insertions(+) create mode 100644 spec/unit/models/room-receipts.spec.ts diff --git a/spec/unit/models/room-receipts.spec.ts b/spec/unit/models/room-receipts.spec.ts new file mode 100644 index 00000000000..b1384aa9471 --- /dev/null +++ b/spec/unit/models/room-receipts.spec.ts @@ -0,0 +1,146 @@ +/* +Copyright 2023 The Matrix.org Foundation C.I.C. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +import { MatrixClient, MatrixEvent } from "../../../src"; +import { Room } from "../../../src/models/room"; + +/** + * Note, these tests check the functionality of the RoomReceipts class, but most + * of them access that functionality via the surrounding Room class, because a + * room is required for RoomReceipts to function, and this matches the pattern + * of how this code is used in the wild. + */ +describe("RoomReceipts", () => { + it("reports events unread if there are no receipts", () => { + // Given there are no receipts in the room + const room = createRoom(); + const [event] = createEvent(); + room.addLiveEvents([event]); + + // When I ask about any event, then it is unread + expect(room.hasUserReadEvent(readerId, event.getId()!)).toBe(false); + }); + + it("reports read if we receive an unthreaded receipt for this event", () => { + // Given my event exists and is unread + const room = createRoom(); + const [event, eventId] = createEvent(); + room.addLiveEvents([event]); + expect(room.hasUserReadEvent(readerId, eventId)).toBe(false); + + // When we receive a receipt for this event+user + room.addReceipt(createReceipt(readerId, event)); + + // Then that event is read + expect(room.hasUserReadEvent(readerId, eventId)).toBe(true); + }); + + it("reports read if we receive an unthreaded receipt for a later event", () => { + // Given we have 2 events + const room = createRoom(); + const [event1, event1Id] = createEvent(); + const [event2] = createEvent(); + room.addLiveEvents([event1, event2]); + + // When we receive a receipt for the later event + room.addReceipt(createReceipt(readerId, event2)); + + // Then the earlier one is read + expect(room.hasUserReadEvent(readerId, event1Id)).toBe(true); + }); + + it("reports unread if we receive an unthreaded receipt for an earlier event", () => { + // Given we have 2 events + const room = createRoom(); + const [event1] = createEvent(); + const [event2, event2Id] = createEvent(); + room.addLiveEvents([event1, event2]); + + // When we receive a receipt for the earlier event + room.addReceipt(createReceipt(readerId, event1)); + + // Then the later one is unread + expect(room.hasUserReadEvent(readerId, event2Id)).toBe(false); + }); + + it("reports unread if we receive an unthreaded receipt for a different user", () => { + // Given my event exists and is unread + const room = createRoom(); + const [event, eventId] = createEvent(); + room.addLiveEvents([event]); + expect(room.hasUserReadEvent(readerId, eventId)).toBe(false); + + // When we receive a receipt for this event+user + room.addReceipt(createReceipt(otherUserId, event)); + + // Then the event is still unread since the receipt was not for us + expect(room.hasUserReadEvent(readerId, eventId)).toBe(false); + + // But it's read for the other person + expect(room.hasUserReadEvent(otherUserId, eventId)).toBe(true); + }); + + ("reports unread if we receive a receipt for an earlier event"); + ("reports read if we receive a receipt for a later event"); + ("different ways of events being later or earlier"); + ("the sender of an event has alreadys read it due to synthetic receipts"); + ("favours a synthetic receipt if it is later than the real one"); + ("ignores an earlier receipt"); + ("threaded receipts"); + ("mixture of threaded and unthreaded receipts"); +}); + +function createFakeClient(): MatrixClient { + return { + getUserId: jest.fn(), + getEventMapper: jest.fn().mockReturnValue(jest.fn()), + isInitialSyncComplete: jest.fn().mockReturnValue(true), + supportsThreads: jest.fn().mockReturnValue(true), + } as unknown as MatrixClient; +} + +const senderId = "sender:s.ss"; +const readerId = "reader:r.rr"; +const otherUserId = "other:o.oo"; + +function createRoom(): Room { + return new Room("!rid", createFakeClient(), "@u:s.nz"); +} + +let idCounter = 0; +function nextId(): string { + return "$" + (idCounter++).toString(10); +} + +function createEvent(): [MatrixEvent, string] { + const event = new MatrixEvent({ sender: senderId, event_id: nextId() }); + return [event, event.getId()!]; +} + +function createReceipt(userId: string, referencedEvent: MatrixEvent): MatrixEvent { + return new MatrixEvent({ + type: "m.receipt", + content: { + [referencedEvent.getId()!]: { + "m.read": { + [userId]: { + ts: 123, + }, + }, + }, + }, + }); +} From c6b0a893b5cced9cfd803f6281ce80f17ab1716f Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Fri, 10 Nov 2023 14:27:05 +0000 Subject: [PATCH 11/26] More tests for RoomReceipts --- spec/unit/models/room-receipts.spec.ts | 76 +++++++++++++++++++++++--- 1 file changed, 68 insertions(+), 8 deletions(-) diff --git a/spec/unit/models/room-receipts.spec.ts b/spec/unit/models/room-receipts.spec.ts index b1384aa9471..9c26dd324b9 100644 --- a/spec/unit/models/room-receipts.spec.ts +++ b/spec/unit/models/room-receipts.spec.ts @@ -34,6 +34,17 @@ describe("RoomReceipts", () => { expect(room.hasUserReadEvent(readerId, event.getId()!)).toBe(false); }); + it("reports events we sent as read even if there are no receipts", () => { + // Given there are no receipts in the room + const room = createRoom(); + const [event] = createEventSentBy(readerId); + room.addLiveEvents([event]); + + // When I ask about an event I sent, it is read (because a synthetic + // receipt was created and stored in RoomReceipts) + expect(room.hasUserReadEvent(readerId, event.getId()!)).toBe(true); + }); + it("reports read if we receive an unthreaded receipt for this event", () => { // Given my event exists and is unread const room = createRoom(); @@ -62,6 +73,21 @@ describe("RoomReceipts", () => { expect(room.hasUserReadEvent(readerId, event1Id)).toBe(true); }); + it("reports read for a non-live event if we receive an unthreaded receipt for a live one", () => { + // Given we have 2 events: one live and one old + const room = createRoom(); + const [oldEvent, oldEventId] = createEvent(); + const [liveEvent] = createEvent(); + room.addLiveEvents([liveEvent]); + createOldTimeline(room, [oldEvent]); + + // When we receive a receipt for the live event + room.addReceipt(createReceipt(readerId, liveEvent)); + + // Then the earlier one is read + expect(room.hasUserReadEvent(readerId, oldEventId)).toBe(true); + }); + it("reports unread if we receive an unthreaded receipt for an earlier event", () => { // Given we have 2 events const room = createRoom(); @@ -93,12 +119,37 @@ describe("RoomReceipts", () => { expect(room.hasUserReadEvent(otherUserId, eventId)).toBe(true); }); - ("reports unread if we receive a receipt for an earlier event"); - ("reports read if we receive a receipt for a later event"); - ("different ways of events being later or earlier"); - ("the sender of an event has alreadys read it due to synthetic receipts"); - ("favours a synthetic receipt if it is later than the real one"); - ("ignores an earlier receipt"); + it("reports events we sent as read even if an earlier receipt arrives", () => { + // Given we sent an event after some other event + const room = createRoom(); + const [previousEvent] = createEvent(); + const [myEvent] = createEventSentBy(readerId); + room.addLiveEvents([previousEvent, myEvent]); + + // And I just received a receipt for the previous event + room.addReceipt(createReceipt(readerId, previousEvent)); + + // When I ask about the event I sent, it is read (because of synthetic receipts) + expect(room.hasUserReadEvent(readerId, myEvent.getId()!)).toBe(true); + }); + + it("correctly reports readness even when receipts arrive out of order", () => { + // Given we have 3 events + const room = createRoom(); + const [event1] = createEvent(); + const [event2, event2Id] = createEvent(); + const [event3, event3Id] = createEvent(); + room.addLiveEvents([event1, event2, event3]); + + // When we receive receipts for the older events out of order + room.addReceipt(createReceipt(readerId, event2)); + room.addReceipt(createReceipt(readerId, event1)); + + // Then we correctly ignore the older receipt + expect(room.hasUserReadEvent(readerId, event2Id)).toBe(true); + expect(room.hasUserReadEvent(readerId, event3Id)).toBe(false); + }); + ("threaded receipts"); ("mixture of threaded and unthreaded receipts"); }); @@ -117,7 +168,7 @@ const readerId = "reader:r.rr"; const otherUserId = "other:o.oo"; function createRoom(): Room { - return new Room("!rid", createFakeClient(), "@u:s.nz"); + return new Room("!rid", createFakeClient(), "@u:s.nz", { timelineSupport: true }); } let idCounter = 0; @@ -126,7 +177,11 @@ function nextId(): string { } function createEvent(): [MatrixEvent, string] { - const event = new MatrixEvent({ sender: senderId, event_id: nextId() }); + return createEventSentBy(senderId); +} + +function createEventSentBy(customSenderId: string): [MatrixEvent, string] { + const event = new MatrixEvent({ sender: customSenderId, event_id: nextId() }); return [event, event.getId()!]; } @@ -144,3 +199,8 @@ function createReceipt(userId: string, referencedEvent: MatrixEvent): MatrixEven }, }); } + +function createOldTimeline(room: Room, events: MatrixEvent[]) { + const oldTimeline = room.getUnfilteredTimelineSet().addTimeline(); + room.getUnfilteredTimelineSet().addEventsToTimeline(events, true, oldTimeline); +} From 684b83173c2a8b3ef2af43e1cffb7b144bd50640 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Fri, 10 Nov 2023 16:41:38 +0000 Subject: [PATCH 12/26] More tests and a fix because unit tests rule --- spec/unit/models/room-receipts.spec.ts | 199 +++++++++++++++++++++++-- src/models/room-receipts.ts | 35 ++++- 2 files changed, 217 insertions(+), 17 deletions(-) diff --git a/spec/unit/models/room-receipts.spec.ts b/spec/unit/models/room-receipts.spec.ts index 9c26dd324b9..c417a412931 100644 --- a/spec/unit/models/room-receipts.spec.ts +++ b/spec/unit/models/room-receipts.spec.ts @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -import { MatrixClient, MatrixEvent } from "../../../src"; +import { FeatureSupport, MatrixClient, MatrixEvent, ReceiptContent, THREAD_RELATION_TYPE, Thread } from "../../../src"; import { Room } from "../../../src/models/room"; /** @@ -24,6 +24,17 @@ import { Room } from "../../../src/models/room"; * of how this code is used in the wild. */ describe("RoomReceipts", () => { + let previousThreadHasServerSideSupport: FeatureSupport; + + beforeAll(() => { + previousThreadHasServerSideSupport = Thread.hasServerSideSupport; + Thread.hasServerSideSupport = FeatureSupport.Stable; + }); + + afterAll(() => { + Thread.hasServerSideSupport = previousThreadHasServerSideSupport; + }); + it("reports events unread if there are no receipts", () => { // Given there are no receipts in the room const room = createRoom(); @@ -109,7 +120,7 @@ describe("RoomReceipts", () => { room.addLiveEvents([event]); expect(room.hasUserReadEvent(readerId, eventId)).toBe(false); - // When we receive a receipt for this event+user + // When we receive a receipt for another user room.addReceipt(createReceipt(otherUserId, event)); // Then the event is still unread since the receipt was not for us @@ -150,8 +161,126 @@ describe("RoomReceipts", () => { expect(room.hasUserReadEvent(readerId, event3Id)).toBe(false); }); - ("threaded receipts"); - ("mixture of threaded and unthreaded receipts"); + it("reports read if we receive a threaded receipt for this event (main)", () => { + // Given my event exists and is unread + const room = createRoom(); + const [event, eventId] = createEvent(); + room.addLiveEvents([event]); + expect(room.hasUserReadEvent(readerId, eventId)).toBe(false); + + // When we receive a receipt for this event+user + room.addReceipt(createThreadedReceipt(readerId, event, "main")); + + // Then that event is read + expect(room.hasUserReadEvent(readerId, eventId)).toBe(true); + }); + + it("reports read if we receive a threaded receipt for this event (non-main)", () => { + // Given my event exists and is unread + const room = createRoom(); + const [root, rootId] = createEvent(); + const [event, eventId] = createThreadedEvent(root); + setupThread(room, root); + room.addLiveEvents([root, event]); + expect(room.hasUserReadEvent(readerId, eventId)).toBe(false); + + // When we receive a receipt for this event on this thread + room.addReceipt(createThreadedReceipt(readerId, event, rootId)); + + // Then that event is read + expect(room.hasUserReadEvent(readerId, eventId)).toBe(true); + }); + + it("reports read if we receive an threaded receipt for a later event", () => { + // Given we have 2 events in a thread + const room = createRoom(); + const [root, rootId] = createEvent(); + const [event1, event1Id] = createThreadedEvent(root); + const [event2] = createThreadedEvent(root); + setupThread(room, root); + room.addLiveEvents([root, event1, event2]); + + // When we receive a receipt for the later event + room.addReceipt(createThreadedReceipt(readerId, event2, rootId)); + + // Then the earlier one is read + expect(room.hasUserReadEvent(readerId, event1Id)).toBe(true); + }); + + it("reports unread if we receive an threaded receipt for an earlier event", () => { + // Given we have 2 events in a thread + const room = createRoom(); + const [root, rootId] = createEvent(); + const [event1] = createThreadedEvent(root); + const [event2, event2Id] = createThreadedEvent(root); + setupThread(room, root); + room.addLiveEvents([root, event1, event2]); + + // When we receive a receipt for the earlier event + room.addReceipt(createThreadedReceipt(readerId, event1, rootId)); + + // Then the later one is unread + expect(room.hasUserReadEvent(readerId, event2Id)).toBe(false); + }); + + it("reports unread if we receive an threaded receipt for a different user", () => { + // Given my event exists and is unread + const room = createRoom(); + const [root, rootId] = createEvent(); + const [event, eventId] = createThreadedEvent(root); + setupThread(room, root); + room.addLiveEvents([root, event]); + expect(room.hasUserReadEvent(readerId, eventId)).toBe(false); + + // When we receive a receipt for another user + room.addReceipt(createThreadedReceipt(otherUserId, event, rootId)); + + // Then the event is still unread since the receipt was not for us + expect(room.hasUserReadEvent(readerId, eventId)).toBe(false); + + // But it's read for the other person + expect(room.hasUserReadEvent(otherUserId, eventId)).toBe(true); + }); + + it("reports unread if we receive a receipt for a later event in a different thread", () => { + // Given 2 events exist in different threads + const room = createRoom(); + const [root1] = createEvent(); + const [root2] = createEvent(); + const [thread1, thread1Id] = createThreadedEvent(root1); + const [thread2] = createThreadedEvent(root2); + setupThread(room, root1); + setupThread(room, root2); + room.addLiveEvents([root1, root2, thread1, thread2]); + + // When we receive a receipt for the later event + room.addReceipt(createThreadedReceipt(readerId, thread2, root2.getId()!)); + + // Then the old one is still unread since the receipt was not for this thread + expect(room.hasUserReadEvent(readerId, thread1Id)).toBe(false); + }); + + it("correctly reports readness even when threaded receipts arrive out of order", () => { + // Given we have 3 events + const room = createRoom(); + const [root, rootId] = createEvent(); + const [event1] = createThreadedEvent(root); + const [event2, event2Id] = createThreadedEvent(root); + const [event3, event3Id] = createThreadedEvent(root); + setupThread(room, root); + room.addLiveEvents([root, event1, event2, event3]); + + // When we receive receipts for the older events out of order + room.addReceipt(createThreadedReceipt(readerId, event2, rootId)); + room.addReceipt(createThreadedReceipt(readerId, event1, rootId)); + + // Then we correctly ignore the older receipt + expect(room.hasUserReadEvent(readerId, event2Id)).toBe(true); + expect(room.hasUserReadEvent(readerId, event3Id)).toBe(false); + }); + + // TODO: mixture of threaded and unthreaded receipts + // TODO: late-arriving receipts (dangling) }); function createFakeClient(): MatrixClient { @@ -160,6 +289,9 @@ function createFakeClient(): MatrixClient { getEventMapper: jest.fn().mockReturnValue(jest.fn()), isInitialSyncComplete: jest.fn().mockReturnValue(true), supportsThreads: jest.fn().mockReturnValue(true), + fetchRoomEvent: jest.fn().mockResolvedValue({}), + paginateEventTimeline: jest.fn(), + canSupport: { get: jest.fn() }, } as unknown as MatrixClient; } @@ -185,18 +317,56 @@ function createEventSentBy(customSenderId: string): [MatrixEvent, string] { return [event, event.getId()!]; } +function createThreadedEvent(root: MatrixEvent): [MatrixEvent, string] { + const rootEventId = root.getId()!; + const event = new MatrixEvent({ + sender: senderId, + event_id: nextId(), + content: { + "m.relates_to": { + event_id: rootEventId, + rel_type: THREAD_RELATION_TYPE.name, + ["m.in_reply_to"]: { + event_id: rootEventId, + }, + }, + }, + }); + return [event, event.getId()!]; +} + function createReceipt(userId: string, referencedEvent: MatrixEvent): MatrixEvent { + const content: ReceiptContent = { + [referencedEvent.getId()!]: { + "m.read": { + [userId]: { + ts: 123, + }, + }, + }, + }; + return new MatrixEvent({ type: "m.receipt", - content: { - [referencedEvent.getId()!]: { - "m.read": { - [userId]: { - ts: 123, - }, + content, + }); +} + +function createThreadedReceipt(userId: string, referencedEvent: MatrixEvent, threadId: string): MatrixEvent { + const content: ReceiptContent = { + [referencedEvent.getId()!]: { + "m.read": { + [userId]: { + ts: 123, + thread_id: threadId, }, }, }, + }; + + return new MatrixEvent({ + type: "m.receipt", + content, }); } @@ -204,3 +374,12 @@ function createOldTimeline(room: Room, events: MatrixEvent[]) { const oldTimeline = room.getUnfilteredTimelineSet().addTimeline(); room.getUnfilteredTimelineSet().addEventsToTimeline(events, true, oldTimeline); } + +/** + * Perform the hacks required for this room to create a thread based on the root + * event supplied. + */ +function setupThread(room: Room, root: MatrixEvent) { + const thread = room.createThread(root.getId()!, root, [root], false); + thread.initialEventsFetched = true; +} diff --git a/src/models/room-receipts.ts b/src/models/room-receipts.ts index 32282e3e7f3..08d79b1455f 100644 --- a/src/models/room-receipts.ts +++ b/src/models/room-receipts.ts @@ -232,17 +232,26 @@ class ReceiptsByUser { const existingReceipt = userReceipts.getByType(synthetic); if (existingReceipt) { - const unfilteredTimelineSet = this.room.getUnfilteredTimelineSet(); - const comparison = unfilteredTimelineSet.compareEventOrdering(eventId, existingReceipt.eventId); - - if (comparison !== null && comparison < 0) { + if (isAfter(existingReceipt.eventId, eventId, this.room)) { // The new receipt is before the existing one - don't store it. return; } } - // Either there was no (valid) existing receipt, or it was before this - // one - add the new receipt. + // Possibilities: + // + // 1. there was no existing receipt, or + // 2. the existing receipt was before this one, or + // 3. we were unable to compare the receipts. + // + // In the case of 3 it's difficult to decide what to do, so the + // most-recently-received receipt wins. + // + // Case 3 can only happen if the events for these receipts have + // disappeared, which is quite unlikely since the new one has just been + // checked, and the old one was checked before it was inserted here. + // + // We go ahead and store this receipt (replacing the other if it exists) userReceipts.set(synthetic, new ReceiptInfo(eventId, receiptType, ts)); } @@ -340,7 +349,7 @@ function getOrCreate(m: Map, key: K, createFn: () => V): V { } /** - * Is left after right in this room's unfiltered timeline? + * Is left after right (or the same)? * * Only returns true if both events can be found, and left is after or the same * as right. @@ -351,3 +360,15 @@ function isAfterOrSame(leftEventId: string, rightEventId: string, room: Room): b const comparison = room.compareEventOrdering(leftEventId, rightEventId); return comparison !== null && comparison >= 0; } + +/** + * Is left strictly after right? + * + * Only returns true if both events can be found, and left is strictly after right. + * + * @returns left \> right + */ +function isAfter(leftEventId: string, rightEventId: string, room: Room): boolean { + const comparison = room.compareEventOrdering(leftEventId, rightEventId); + return comparison !== null && comparison > 0; +} From 045e16938e53d32952c78345ac1c9160148941fe Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Fri, 10 Nov 2023 17:05:04 +0000 Subject: [PATCH 13/26] Add a test for mixing threaded and unthreaded receipts --- spec/unit/models/room-receipts.spec.ts | 59 +++++++++++++++++++++++++- 1 file changed, 58 insertions(+), 1 deletion(-) diff --git a/spec/unit/models/room-receipts.spec.ts b/spec/unit/models/room-receipts.spec.ts index c417a412931..9a27feb8031 100644 --- a/spec/unit/models/room-receipts.spec.ts +++ b/spec/unit/models/room-receipts.spec.ts @@ -279,7 +279,64 @@ describe("RoomReceipts", () => { expect(room.hasUserReadEvent(readerId, event3Id)).toBe(false); }); - // TODO: mixture of threaded and unthreaded receipts + it("correctly reports readness when mixing threaded and unthreaded receipts", () => { + // Given we have a setup from this presentation: + // https://docs.google.com/presentation/d/1H1gxRmRFAm8d71hCILWmpOYezsvdlb7cB6ANl-20Gns/edit?usp=sharing + // + // Main1----\ + // | ---Thread1a <- threaded receipt + // | | + // | Thread1b + // threaded receipt -> Main2--\ + // | ----------------Thread2a <- unthreaded receipt + // Main3 | + // Thread2b <- threaded receipt + // + const room = createRoom(); + const [main1, main1Id] = createEvent(); + const [main2, main2Id] = createEvent(); + const [main3, main3Id] = createEvent(); + const [thread1a, thread1aId] = createThreadedEvent(main1); + const [thread1b, thread1bId] = createThreadedEvent(main1); + const [thread2a, thread2aId] = createThreadedEvent(main2); + const [thread2b, thread2bId] = createThreadedEvent(main2); + setupThread(room, main1); + setupThread(room, main2); + room.addLiveEvents([main1, thread1a, thread1b, main2, thread2a, main3, thread2b]); + + // And the timestamps on the events are consistent with the order above + main1.event.origin_server_ts = 1; + thread1a.event.origin_server_ts = 2; + thread1b.event.origin_server_ts = 3; + main2.event.origin_server_ts = 4; + thread2a.event.origin_server_ts = 5; + main3.event.origin_server_ts = 6; + thread2b.event.origin_server_ts = 7; + // (Note: in principle, we have the information needed to order these + // events without using their timestamps, since they all came in via + // addLiveEvents. In reality, some of them would have come in via the + // /relations API, making it impossible to get the correct ordering + // without MSC4033, which is why we fall back to timestamps. I.e. we + // definitely could fix the code to make the above + // timestamp-manipulation unnecessary, but it would only make this test + // neater, not actually help in the real world.) + + // When the receipts arrive + room.addReceipt(createThreadedReceipt(readerId, main2, "main")); + room.addReceipt(createThreadedReceipt(readerId, thread1a, main1Id)); + room.addReceipt(createReceipt(readerId, thread2a)); + room.addReceipt(createThreadedReceipt(readerId, thread2b, main2Id)); + + // Then we correctly identify that only main3 is unread + expect(room.hasUserReadEvent(readerId, main1Id)).toBe(true); + expect(room.hasUserReadEvent(readerId, main2Id)).toBe(true); + expect(room.hasUserReadEvent(readerId, main3Id)).toBe(false); + expect(room.hasUserReadEvent(readerId, thread1aId)).toBe(true); + expect(room.hasUserReadEvent(readerId, thread1bId)).toBe(true); + expect(room.hasUserReadEvent(readerId, thread2aId)).toBe(true); + expect(room.hasUserReadEvent(readerId, thread2bId)).toBe(true); + }); + // TODO: late-arriving receipts (dangling) }); From fce2d98cee6ae12e0bbeab095c015078d4d827ee Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Fri, 10 Nov 2023 17:20:08 +0000 Subject: [PATCH 14/26] Break out user-sent-last-message logic into a function --- src/models/room-receipts.ts | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/src/models/room-receipts.ts b/src/models/room-receipts.ts index 08d79b1455f..2dddabbfd60 100644 --- a/src/models/room-receipts.ts +++ b/src/models/room-receipts.ts @@ -115,19 +115,31 @@ export class RoomReceipts { } } - const timeline = - threadId === MAIN_ROOM_TIMELINE - ? this.room.getLiveTimeline().getEvents() - : this.room.getThread(threadId)?.timeline; - if (timeline?.at(-1)?.getSender() === userId) { + if (this.userSentLatestEventInThread(threadId, userId)) { // The user sent the latest message in this event's thread, so we // consider everything in the thread to be read. + // + // Note: maybe we don't need this because synthetic receipts should + // do this job for us? return true; } // Neither of the receipts were after the event, so it's unread. return false; } + + /** + * @returns true if the thread with this ID can be found, and the supplied + * user sent the latest message in it. + */ + private userSentLatestEventInThread(threadId: string, userId: String): boolean { + const timeline = + threadId === MAIN_ROOM_TIMELINE + ? this.room.getLiveTimeline().getEvents() + : this.room.getThread(threadId)?.timeline; + + return !!(timeline && timeline.length > 0 && timeline[timeline.length - 1].getSender() === userId); + } } // --- implementation details --- From b61bc6962d5e78c387b9e75245ac3f7c14137579 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Fri, 10 Nov 2023 17:29:08 +0000 Subject: [PATCH 15/26] Fix incorrect failing test --- spec/unit/models/thread.spec.ts | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/spec/unit/models/thread.spec.ts b/spec/unit/models/thread.spec.ts index 3eb9592e0e3..711d8801e86 100644 --- a/spec/unit/models/thread.spec.ts +++ b/spec/unit/models/thread.spec.ts @@ -149,21 +149,27 @@ describe("Thread", () => { }); it("considers other events with no RR as unread", async () => { - const { thread, events, rootEvent } = await populateThread({ + // Given a long thread exists + const { thread, events } = await populateThread({ room, client, - authorId: myUserId, - participantUserIds: [myUserId], + authorId: "@other:foo.com", + participantUserIds: ["@other:foo.com"], length: 25, ts: 190, }); + const event1 = events.at(1)!; + const event2 = events.at(2)!; + const event24 = events.at(24)!; + + // And we have read the second message in it with an unthreaded receipt const receipt = new MatrixEvent({ type: "m.receipt", room_id: room.roomId, content: { - // unthreaded receipt for the thread root - [rootEvent.getId()!]: { + // unthreaded receipt for the second message in the thread + [event2.getId()!]: { [ReceiptType.Read]: { [myUserId]: { ts: 200 }, }, @@ -172,11 +178,9 @@ describe("Thread", () => { }); room.addReceipt(receipt); - // Before alice's last unthreaded receipt - expect(thread.hasUserReadEvent("@alice:example.org", events.at(1)!.getId() ?? "")).toBeTruthy(); - - // After alice's last unthreaded receipt - expect(thread.hasUserReadEvent("@alice:example.org", events.at(-1)!.getId() ?? "")).toBeFalsy(); + // Then we have read the first message in the thread, and not the last + expect(thread.hasUserReadEvent(myUserId, event1.getId()!)).toBe(true); + expect(thread.hasUserReadEvent(myUserId, event24.getId()!)).toBe(false); }); it("considers event as read if there's a more recent unthreaded receipt", () => { From b88706e367b8001c3f3794d1938075aa8edb40b7 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Fri, 10 Nov 2023 17:31:41 +0000 Subject: [PATCH 16/26] Enhance mock to allow tests to pass with new cod --- spec/unit/room.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/unit/room.spec.ts b/spec/unit/room.spec.ts index 35addf31c1c..cff8ef2252b 100644 --- a/spec/unit/room.spec.ts +++ b/spec/unit/room.spec.ts @@ -1747,7 +1747,7 @@ describe("Room", function () { const ts = 13787898424; room.addLiveEvents([eventToAck]); room.addReceipt(mkReceipt(roomId, [mkRecord(eventToAck.getId()!, "m.read", userB, ts)])); - room.findEventById = jest.fn().mockReturnValue({} as MatrixEvent); + room.findEventById = jest.fn().mockReturnValue({ getThread: jest.fn() } as unknown as MatrixEvent); expect(room.hasUserReadEvent(userB, eventToAck.getId()!)).toEqual(true); }); From 4c04a7ba8940238a7e833d1a77d8d0192b3a028b Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Mon, 13 Nov 2023 11:35:07 +0000 Subject: [PATCH 17/26] Add a test for two events in old timelines --- spec/unit/models/room-receipts.spec.ts | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/spec/unit/models/room-receipts.spec.ts b/spec/unit/models/room-receipts.spec.ts index 9a27feb8031..f1bcf7ddf35 100644 --- a/spec/unit/models/room-receipts.spec.ts +++ b/spec/unit/models/room-receipts.spec.ts @@ -99,6 +99,25 @@ describe("RoomReceipts", () => { expect(room.hasUserReadEvent(readerId, oldEventId)).toBe(true); }); + it("compares by timestamp if two events are in separate old timelines", () => { + // Given we have 2 events, both in old timelines, with event2 after + // event1 in terms of timestamps + const room = createRoom(); + const [event1, event1Id] = createEvent(); + const [event2, event2Id] = createEvent(); + event1.event.origin_server_ts = 1; + event2.event.origin_server_ts = 2; + createOldTimeline(room, [event1]); + createOldTimeline(room, [event2]); + + // When we receive a receipt for the older event + room.addReceipt(createReceipt(readerId, event1)); + + // Then the earlier one is read and the later one is not + expect(room.hasUserReadEvent(readerId, event1Id)).toBe(true); + expect(room.hasUserReadEvent(readerId, event2Id)).toBe(false); + }); + it("reports unread if we receive an unthreaded receipt for an earlier event", () => { // Given we have 2 events const room = createRoom(); From d1cb13d8d3b6deaefe371de473b6549abae54e8e Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Mon, 13 Nov 2023 12:13:00 +0000 Subject: [PATCH 18/26] Add a test for events received after we sent one --- spec/unit/models/room-receipts.spec.ts | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/spec/unit/models/room-receipts.spec.ts b/spec/unit/models/room-receipts.spec.ts index f1bcf7ddf35..8cf49d6b44b 100644 --- a/spec/unit/models/room-receipts.spec.ts +++ b/spec/unit/models/room-receipts.spec.ts @@ -163,6 +163,17 @@ describe("RoomReceipts", () => { expect(room.hasUserReadEvent(readerId, myEvent.getId()!)).toBe(true); }); + it("considers events after ones we sent to be unread", () => { + // Given we sent an event, then another event came in + const room = createRoom(); + const [myEvent] = createEventSentBy(readerId); + const [laterEvent] = createEvent(); + room.addLiveEvents([myEvent, laterEvent]); + + // When I ask about the later event, it is unread (because it's after the synthetic receipt) + expect(room.hasUserReadEvent(readerId, laterEvent.getId()!)).toBe(false); + }); + it("correctly reports readness even when receipts arrive out of order", () => { // Given we have 3 events const room = createRoom(); From 5e7dfd36bf7bde362c534f0c9f8601283d92dc15 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Mon, 13 Nov 2023 12:17:25 +0000 Subject: [PATCH 19/26] Rename compare-event-ordering --- ...pare-events-in-stream-order.ts => compare-event-ordering.ts} | 0 src/models/room.ts | 2 +- 2 files changed, 1 insertion(+), 1 deletion(-) rename src/models/{compare-events-in-stream-order.ts => compare-event-ordering.ts} (100%) diff --git a/src/models/compare-events-in-stream-order.ts b/src/models/compare-event-ordering.ts similarity index 100% rename from src/models/compare-events-in-stream-order.ts rename to src/models/compare-event-ordering.ts diff --git a/src/models/room.ts b/src/models/room.ts index c64d9e26766..ee41c5f2bcb 100644 --- a/src/models/room.ts +++ b/src/models/room.ts @@ -67,7 +67,7 @@ import { RelationsContainer } from "./relations-container"; import { ReadReceipt, synthesizeReceipt } from "./read-receipt"; import { isPollEvent, Poll, PollEvent } from "./poll"; import { RoomReceipts } from "./room-receipts"; -import { compareEventOrdering } from "./compare-events-in-stream-order"; +import { compareEventOrdering } from "./compare-event-ordering"; // These constants are used as sane defaults when the homeserver doesn't support // the m.room_versions capability. In practice, KNOWN_SAFE_ROOM_VERSION should be From 7aa570e28d6a0732142f573f60e154f0ec8e8095 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Mon, 13 Nov 2023 12:54:33 +0000 Subject: [PATCH 20/26] Make compareEventOrdering use -1, 0 or 1 (or null) exclusively --- src/models/event-timeline-set.ts | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/src/models/event-timeline-set.ts b/src/models/event-timeline-set.ts index 0427967fd3b..a61fc9dce7e 100644 --- a/src/models/event-timeline-set.ts +++ b/src/models/event-timeline-set.ts @@ -899,11 +899,10 @@ export class EventTimelineSet extends TypedEventEmitter 0) { + return 1; + } else { + return 0; + } } // the events are in different timelines. Iterate through the From 59f3214ffde70c48b2e0e27c9b75ccde97d6a928 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Mon, 13 Nov 2023 12:54:55 +0000 Subject: [PATCH 21/26] Re-instate methods I deleted from public interfaces --- src/models/read-receipt.ts | 21 +++++++++++++++++++++ src/models/thread.ts | 13 ++++++++++++- 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/src/models/read-receipt.ts b/src/models/read-receipt.ts index 4c2575102d3..285838cef00 100644 --- a/src/models/read-receipt.ts +++ b/src/models/read-receipt.ts @@ -387,4 +387,25 @@ export abstract class ReadReceipt< return receipt.userId; }); } + + /** + * Determines if the given user has read a particular event ID with the known + * history of the room. This is not a definitive check as it relies only on + * what is available to the room at the time of execution. + * @param userId - The user ID to check the read state of. + * @param eventId - The event ID to check if the user read. + * @returns True if the user has read the event, false otherwise. + */ + public abstract hasUserReadEvent(userId: string, eventId: string): boolean; + + /** + * Returns the most recent unthreaded receipt for a given user + * @param userId - the MxID of the User + * @returns an unthreaded Receipt. Can be undefined if receipts have been disabled + * or a user chooses to use private read receipts (or we have simply not received + * a receipt from this user yet). + * + * @deprecated use `hasUserReadEvent` or `getEventReadUpTo` instead + */ + public abstract getLastUnthreadedReceiptFor(userId: string): Receipt | undefined; } diff --git a/src/models/thread.ts b/src/models/thread.ts index 5f61b92daf0..edf68c68a05 100644 --- a/src/models/thread.ts +++ b/src/models/thread.ts @@ -27,7 +27,7 @@ import { RoomState } from "./room-state"; import { ServerControlledNamespacedValue } from "../NamespacedValue"; import { logger } from "../logger"; import { ReadReceipt } from "./read-receipt"; -import { CachedReceiptStructure, ReceiptType } from "../@types/read_receipts"; +import { CachedReceiptStructure, Receipt, ReceiptType } from "../@types/read_receipts"; import { Feature, ServerSupport } from "../feature"; export enum ThreadEvent { @@ -819,6 +819,17 @@ export class Thread extends ReadReceipt Date: Mon, 13 Nov 2023 12:55:17 +0000 Subject: [PATCH 22/26] Update comment to remove unexpected case --- src/models/compare-event-ordering.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/models/compare-event-ordering.ts b/src/models/compare-event-ordering.ts index da3a8ad5326..c32445fb03f 100644 --- a/src/models/compare-event-ordering.ts +++ b/src/models/compare-event-ordering.ts @@ -37,8 +37,7 @@ import { Thread } from "./thread"; * @param rightEventId - the id of the second event * @returns -1 if left \< right, 1 if left \> right, 0 if left == right, null if - * we can't tell (because we can't find the events, or they are both from old - * timelines). + * we can't tell (because we can't find the events). */ export function compareEventOrdering(room: Room, leftEventId: string, rightEventId: string): number | null { const leftEvent = room.findEventById(leftEventId); From f6d9cd8a35ada8d3147ec39f6fc309b25e314b0b Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Mon, 13 Nov 2023 17:20:04 +0000 Subject: [PATCH 23/26] Improve comments about code to delete --- src/models/room.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/models/room.ts b/src/models/room.ts index ee41c5f2bcb..45b2155b3e1 100644 --- a/src/models/room.ts +++ b/src/models/room.ts @@ -2938,7 +2938,7 @@ export class Room extends ReadReceipt { this.roomReceipts.add(content, synthetic); - // START OF TO-DELETE + // TODO: delete the following code when it has been replaced by RoomReceipts Object.keys(content).forEach((eventId: string) => { Object.keys(content[eventId]).forEach((receiptType: ReceiptType | string) => { Object.keys(content[eventId][receiptType]).forEach((userId: string) => { @@ -3000,7 +3000,7 @@ export class Room extends ReadReceipt { }); }); }); - // END OF TO-DELETE + // End of code to delete when replaced by RoomReceipts // send events after we've regenerated the structure & cache, otherwise things that // listened for the event would read stale data. From 56875a7152eaafe3eb6349db48e341f7a93f7165 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Thu, 16 Nov 2023 14:47:00 +0000 Subject: [PATCH 24/26] BREAKS UNIT TESTS use correct thread when synthesizing receipts --- spec/unit/read-receipt.spec.ts | 14 ++++++++++++++ spec/unit/room.spec.ts | 2 +- src/models/read-receipt.ts | 3 ++- 3 files changed, 17 insertions(+), 2 deletions(-) diff --git a/spec/unit/read-receipt.spec.ts b/spec/unit/read-receipt.spec.ts index ea27d515219..234f5854d64 100644 --- a/spec/unit/read-receipt.spec.ts +++ b/spec/unit/read-receipt.spec.ts @@ -43,8 +43,10 @@ const THREAD_ID = "$thread_event_id"; const ROOM_ID = "!123:matrix.org"; describe("Read receipt", () => { + let threadRoot: MatrixEvent; let threadEvent: MatrixEvent; let roomEvent: MatrixEvent; + let editOfThreadRoot: MatrixEvent; beforeEach(() => { httpBackend = new MockHttpBackend(); @@ -57,6 +59,14 @@ describe("Read receipt", () => { client.isGuest = () => false; client.supportsThreads = () => true; + threadRoot = utils.mkEvent({ + event: true, + type: EventType.RoomMessage, + user: "@bob:matrix.org", + room: ROOM_ID, + content: { body: "This is the thread root" }, + }); + threadEvent = utils.mkEvent({ event: true, type: EventType.RoomMessage, @@ -82,6 +92,9 @@ describe("Read receipt", () => { body: "Hello from a room", }, }); + + editOfThreadRoot = utils.mkEdit(threadRoot, client, "@bob:matrix.org", ROOM_ID); + editOfThreadRoot.setThreadId(THREAD_ID); }); describe("sendReceipt", () => { @@ -208,6 +221,7 @@ describe("Read receipt", () => { it.each([ { getEvent: () => roomEvent, destinationId: MAIN_ROOM_TIMELINE }, { getEvent: () => threadEvent, destinationId: THREAD_ID }, + { getEvent: () => editOfThreadRoot, destinationId: MAIN_ROOM_TIMELINE }, ])("adds the receipt to $destinationId", ({ getEvent, destinationId }) => { const event = getEvent(); const userId = "@bob:example.org"; diff --git a/spec/unit/room.spec.ts b/spec/unit/room.spec.ts index efe4fbfea43..0700069871f 100644 --- a/spec/unit/room.spec.ts +++ b/spec/unit/room.spec.ts @@ -3030,7 +3030,7 @@ describe("Room", function () { const threadRoot = mkMessage(); const threadResponse1 = mkThreadResponse(threadRoot); const threadReaction1 = utils.mkReaction(threadRoot, room.client, userA, roomId); - const threadReaction2 = utils.mkReaction(threadRoot, room.client, userA, roomId); + const threadReaction2 = utils.mkEdit(threadRoot, room.client, userA, roomId); const threadReaction2Redaction = mkRedaction(threadReaction2); const roots = new Set([threadRoot.getId()!]); diff --git a/src/models/read-receipt.ts b/src/models/read-receipt.ts index 285838cef00..f277a65bb43 100644 --- a/src/models/read-receipt.ts +++ b/src/models/read-receipt.ts @@ -27,6 +27,7 @@ import { EventTimelineSet } from "./event-timeline-set"; import { MapWithDefault } from "../utils"; import { NotificationCountType } from "./room"; import { logger } from "../logger"; +import { threadIdForReceipt } from "../client"; export function synthesizeReceipt(userId: string, event: MatrixEvent, receiptType: ReceiptType): MatrixEvent { return new MatrixEvent({ @@ -35,7 +36,7 @@ export function synthesizeReceipt(userId: string, event: MatrixEvent, receiptTyp [receiptType]: { [userId]: { ts: event.getTs(), - thread_id: event.threadRootId ?? MAIN_ROOM_TIMELINE, + thread_id: threadIdForReceipt(event), }, }, }, From 0861f6bff9a7af13b7bb2fad3ef0303b6689567d Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Fri, 17 Nov 2023 08:39:42 +0000 Subject: [PATCH 25/26] Fix incorrect thread ID in read-receipt tests --- spec/unit/read-receipt.spec.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/unit/read-receipt.spec.ts b/spec/unit/read-receipt.spec.ts index 234f5854d64..1fe7d4dc3f4 100644 --- a/spec/unit/read-receipt.spec.ts +++ b/spec/unit/read-receipt.spec.ts @@ -66,6 +66,7 @@ describe("Read receipt", () => { room: ROOM_ID, content: { body: "This is the thread root" }, }); + threadRoot.event.event_id = THREAD_ID; threadEvent = utils.mkEvent({ event: true, From 612e35e319432d3fc964311b8b50d32125cf01b9 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Fri, 17 Nov 2023 08:42:59 +0000 Subject: [PATCH 26/26] Fix unit tests to reflect the fix putting synthetic receipts in the right thread --- spec/unit/models/thread.spec.ts | 67 +++++++++++++++++++++------------ 1 file changed, 43 insertions(+), 24 deletions(-) diff --git a/spec/unit/models/thread.spec.ts b/spec/unit/models/thread.spec.ts index 711d8801e86..559fb430c09 100644 --- a/spec/unit/models/thread.spec.ts +++ b/spec/unit/models/thread.spec.ts @@ -499,13 +499,13 @@ describe("Thread", () => { // And a thread with an added event (with later timestamp) const userId = "user1"; - const { thread, message } = await createThreadAndEvent(client, 1, 100, userId); + const { thread, message2 } = await createThreadAnd2Events(client, 1, 100, 200, userId); // Then a receipt was added to the thread const receipt = thread.getReadReceiptForUserId(userId); expect(receipt).toBeTruthy(); - expect(receipt?.eventId).toEqual(message.getId()); - expect(receipt?.data.ts).toEqual(100); + expect(receipt?.eventId).toEqual(message2.getId()); + expect(receipt?.data.ts).toEqual(200); expect(receipt?.data.thread_id).toEqual(thread.id); // (And the receipt was synthetic) @@ -523,14 +523,14 @@ describe("Thread", () => { // And a thread with an added event with a lower timestamp than its other events const userId = "user1"; - const { thread } = await createThreadAndEvent(client, 200, 100, userId); + const { thread, message1 } = await createThreadAnd2Events(client, 300, 200, 100, userId); - // Then no receipt was added to the thread (the receipt is still - // for the thread root). This happens because since we have no + // Then the receipt is for the first message, because its + // timestamp is later. This happens because since we have no // recursive relations support, we know that sometimes events // appear out of order, so we have to check their timestamps as // a guess of the correct order. - expect(thread.getReadReceiptForUserId(userId)?.eventId).toEqual(thread.rootEvent?.getId()); + expect(thread.getReadReceiptForUserId(userId)?.eventId).toEqual(message1.getId()); }); }); @@ -548,11 +548,11 @@ describe("Thread", () => { // And a thread with an added event (with later timestamp) const userId = "user1"; - const { thread, message } = await createThreadAndEvent(client, 1, 100, userId); + const { thread, message2 } = await createThreadAnd2Events(client, 1, 100, 200, userId); // Then a receipt was added to the thread const receipt = thread.getReadReceiptForUserId(userId); - expect(receipt?.eventId).toEqual(message.getId()); + expect(receipt?.eventId).toEqual(message2.getId()); }); it("Creates a local echo receipt even for events BEFORE an existing receipt", async () => { @@ -568,22 +568,24 @@ describe("Thread", () => { // And a thread with an added event with a lower timestamp than its other events const userId = "user1"; - const { thread, message } = await createThreadAndEvent(client, 200, 100, userId); + const { thread, message2 } = await createThreadAnd2Events(client, 300, 200, 100, userId); - // Then a receipt was added to the thread, because relations - // recursion is available, so we trust the server to have - // provided us with events in the right order. + // Then a receipt was added for the last message, even though it + // has lower ts, because relations recursion is available, so we + // trust the server to have provided us with events in the right + // order. const receipt = thread.getReadReceiptForUserId(userId); - expect(receipt?.eventId).toEqual(message.getId()); + expect(receipt?.eventId).toEqual(message2.getId()); }); }); - async function createThreadAndEvent( + async function createThreadAnd2Events( client: MatrixClient, rootTs: number, - eventTs: number, + message1Ts: number, + message2Ts: number, userId: string, - ): Promise<{ thread: Thread; message: MatrixEvent }> { + ): Promise<{ thread: Thread; message1: MatrixEvent; message2: MatrixEvent }> { const room = new Room("room1", client, userId); // Given a thread @@ -594,24 +596,41 @@ describe("Thread", () => { participantUserIds: [], ts: rootTs, }); - // Sanity: the current receipt is for the thread root - expect(thread.getReadReceiptForUserId(userId)?.eventId).toEqual(thread.rootEvent?.getId()); + // Sanity: there is no read receipt on the thread yet because the + // thread events don't get properly added to the room by mkThread. + expect(thread.getReadReceiptForUserId(userId)).toBeNull(); const awaitTimelineEvent = new Promise((res) => thread.on(RoomEvent.Timeline, () => res())); - // When we add a message that is before the latest receipt - const message = makeThreadEvent({ + // Add a message with ts message1Ts + const message1 = makeThreadEvent({ event: true, rootEventId: thread.id, replyToEventId: thread.id, user: userId, room: room.roomId, - ts: eventTs, + ts: message1Ts, }); - await thread.addEvent(message, false, true); + await thread.addEvent(message1, false, true); await awaitTimelineEvent; - return { thread, message }; + // Sanity: the thread now has a properly-added event, so this event + // has a synthetic receipt. + expect(thread.getReadReceiptForUserId(userId)?.eventId).toEqual(message1.getId()); + + // Add a message with ts message2Ts + const message2 = makeThreadEvent({ + event: true, + rootEventId: thread.id, + replyToEventId: thread.id, + user: userId, + room: room.roomId, + ts: message2Ts, + }); + await thread.addEvent(message2, false, true); + await awaitTimelineEvent; + + return { thread, message1, message2 }; } function createClientWithEventMapper(canSupport: Map = new Map()): MatrixClient {