diff --git a/spec/unit/room.spec.ts b/spec/unit/room.spec.ts index 4ed31380dfc..410959ad774 100644 --- a/spec/unit/room.spec.ts +++ b/spec/unit/room.spec.ts @@ -2435,16 +2435,96 @@ describe("Room", function() { expect(room.getEventReadUpTo(userA)).toEqual("eventId"); }); - it("prefers older receipt", () => { - room.getReadReceiptForUserId = (userId, ignore, receiptType) => { - return (receiptType === ReceiptType.Read - ? { eventId: "eventId1" } - : { eventId: "eventId2" } - ) as IWrappedReceipt; - }; - room.getUnfilteredTimelineSet = () => ({ compareEventOrdering: (event1, event2) => 1 } as EventTimelineSet); + describe("prefers newer receipt", () => { + it("should compare correctly using timelines", () => { + room.getReadReceiptForUserId = (userId, ignore, receiptType) => { + if (receiptType === ReceiptType.ReadPrivate) { + return { eventId: "eventId1" } as IWrappedReceipt; + } + if (receiptType === ReceiptType.UnstableReadPrivate) { + return { eventId: "eventId2" } as IWrappedReceipt; + } + if (receiptType === ReceiptType.Read) { + return { eventId: "eventId3" } as IWrappedReceipt; + } + }; + + for (let i = 1; i <= 3; i++) { + room.getUnfilteredTimelineSet = () => ({ compareEventOrdering: (event1, event2) => { + return (event1 === `eventId${i}`) ? 1 : -1; + } } as EventTimelineSet); + + expect(room.getEventReadUpTo(userA)).toEqual(`eventId${i}`); + } + }); + + it("should compare correctly by timestamp", () => { + for (let i = 1; i <= 3; i++) { + room.getUnfilteredTimelineSet = () => ({ + compareEventOrdering: (_1, _2) => null, + } as EventTimelineSet); + room.getReadReceiptForUserId = (userId, ignore, receiptType) => { + if (receiptType === ReceiptType.ReadPrivate) { + return { eventId: "eventId1", data: { ts: i === 1 ? 1 : 0 } } as IWrappedReceipt; + } + if (receiptType === ReceiptType.UnstableReadPrivate) { + return { eventId: "eventId2", data: { ts: i === 2 ? 1 : 0 } } as IWrappedReceipt; + } + if (receiptType === ReceiptType.Read) { + return { eventId: "eventId3", data: { ts: i === 3 ? 1 : 0 } } as IWrappedReceipt; + } + }; + + expect(room.getEventReadUpTo(userA)).toEqual(`eventId${i}`); + } + }); - expect(room.getEventReadUpTo(userA)).toEqual("eventId1"); + describe("fallback precedence", () => { + beforeAll(() => { + room.getUnfilteredTimelineSet = () => ({ + compareEventOrdering: (_1, _2) => null, + } as EventTimelineSet); + }); + + it("should give precedence to m.read.private", () => { + room.getReadReceiptForUserId = (userId, ignore, receiptType) => { + if (receiptType === ReceiptType.ReadPrivate) { + return { eventId: "eventId1" } as IWrappedReceipt; + } + if (receiptType === ReceiptType.UnstableReadPrivate) { + return { eventId: "eventId2" } as IWrappedReceipt; + } + if (receiptType === ReceiptType.Read) { + return { eventId: "eventId3" } as IWrappedReceipt; + } + }; + + expect(room.getEventReadUpTo(userA)).toEqual(`eventId1`); + }); + + it("should give precedence to org.matrix.msc2285.read.private", () => { + room.getReadReceiptForUserId = (userId, ignore, receiptType) => { + if (receiptType === ReceiptType.UnstableReadPrivate) { + return { eventId: "eventId2" } as IWrappedReceipt; + } + if (receiptType === ReceiptType.Read) { + return { eventId: "eventId2" } as IWrappedReceipt; + } + }; + + expect(room.getEventReadUpTo(userA)).toEqual(`eventId2`); + }); + + it("should give precedence to m.read", () => { + room.getReadReceiptForUserId = (userId, ignore, receiptType) => { + if (receiptType === ReceiptType.Read) { + return { eventId: "eventId3" } as IWrappedReceipt; + } + }; + + expect(room.getEventReadUpTo(userA)).toEqual(`eventId3`); + }); + }); }); }); }); diff --git a/spec/unit/sync-accumulator.spec.ts b/spec/unit/sync-accumulator.spec.ts index 645efbfbba4..b5385330be0 100644 --- a/spec/unit/sync-accumulator.spec.ts +++ b/spec/unit/sync-accumulator.spec.ts @@ -302,6 +302,9 @@ describe("SyncAccumulator", function() { [ReceiptType.ReadPrivate]: { "@dan:localhost": { ts: 4 }, }, + [ReceiptType.UnstableReadPrivate]: { + "@matthew:localhost": { ts: 5 }, + }, "some.other.receipt.type": { "@should_be_ignored:localhost": { key: "val" }, }, @@ -347,6 +350,9 @@ describe("SyncAccumulator", function() { [ReceiptType.ReadPrivate]: { "@dan:localhost": { ts: 4 }, }, + [ReceiptType.UnstableReadPrivate]: { + "@matthew:localhost": { ts: 5 }, + }, }, "$event2:localhost": { [ReceiptType.Read]: { diff --git a/spec/unit/utils.spec.ts b/spec/unit/utils.spec.ts index 03f663ab39c..36ad9e164bc 100644 --- a/spec/unit/utils.spec.ts +++ b/spec/unit/utils.spec.ts @@ -15,6 +15,7 @@ import { import { logger } from "../../src/logger"; import { mkMessage } from "../test-utils/test-utils"; import { makeBeaconEvent } from "../test-utils/beacon"; +import { ReceiptType } from "../../src/@types/read_receipts"; // TODO: Fix types throughout @@ -523,4 +524,54 @@ describe("utils", function() { ).toEqual([beaconEvent2, beaconEvent1, beaconEvent3]); }); }); + + describe('getPrivateReadReceiptField', () => { + it('should return m.read.private if server supports stable', async () => { + expect(await utils.getPrivateReadReceiptField({ + doesServerSupportUnstableFeature: jest.fn().mockImplementation((feature) => { + return feature === "org.matrix.msc2285.stable"; + }), + } as any)).toBe(ReceiptType.ReadPrivate); + }); + + it('should return m.read.private if server supports stable and unstable', async () => { + expect(await utils.getPrivateReadReceiptField({ + doesServerSupportUnstableFeature: jest.fn().mockImplementation((feature) => { + return ["org.matrix.msc2285.stable", "org.matrix.msc2285"].includes(feature); + }), + } as any)).toBe(ReceiptType.ReadPrivate); + }); + + it('should return org.matrix.msc2285.read.private if server supports unstable', async () => { + expect(await utils.getPrivateReadReceiptField({ + doesServerSupportUnstableFeature: jest.fn().mockImplementation((feature) => { + return feature === "org.matrix.msc2285"; + }), + } as any)).toBe(ReceiptType.UnstableReadPrivate); + }); + + it('should return none if server does not support either', async () => { + expect(await utils.getPrivateReadReceiptField({ + doesServerSupportUnstableFeature: jest.fn().mockResolvedValue(false), + } as any)).toBeFalsy(); + }); + }); + + describe('isSupportedReceiptType', () => { + it('should support m.read', () => { + expect(utils.isSupportedReceiptType(ReceiptType.Read)).toBeTruthy(); + }); + + it('should support m.read.private', () => { + expect(utils.isSupportedReceiptType(ReceiptType.ReadPrivate)).toBeTruthy(); + }); + + it('should support org.matrix.msc2285.read.private', () => { + expect(utils.isSupportedReceiptType(ReceiptType.UnstableReadPrivate)).toBeTruthy(); + }); + + it('should not support other receipt types', () => { + expect(utils.isSupportedReceiptType("this is a receipt type")).toBeFalsy(); + }); + }); }); diff --git a/src/@types/read_receipts.ts b/src/@types/read_receipts.ts index 7a3ba268446..16a67feb3f3 100644 --- a/src/@types/read_receipts.ts +++ b/src/@types/read_receipts.ts @@ -17,5 +17,9 @@ limitations under the License. export enum ReceiptType { Read = "m.read", FullyRead = "m.fully_read", - ReadPrivate = "org.matrix.msc2285.read.private" + ReadPrivate = "m.read.private", + /** + * @deprecated Please use the ReadPrivate type when possible. This value may be removed at any time without notice. + */ + UnstableReadPrivate = "org.matrix.msc2285.read.private", } diff --git a/src/client.ts b/src/client.ts index 975de9f017a..e1d805d9d88 100644 --- a/src/client.ts +++ b/src/client.ts @@ -1088,11 +1088,12 @@ export class MatrixClient extends TypedEventEmitter { - const read = content[eid][ReceiptType.Read]; - if (read && Object.keys(read).includes(this.getUserId())) return true; + for (const [key, value] of Object.entries(content[eid])) { + if (!utils.isSupportedReceiptType(key)) continue; + if (!value) continue; - const readPrivate = content[eid][ReceiptType.ReadPrivate]; - if (readPrivate && Object.keys(readPrivate).includes(this.getUserId())) return true; + if (Object.keys(value).includes(this.getUserId())) return true; + } return false; }).length > 0; @@ -4660,7 +4661,7 @@ export class MatrixClient extends TypedEventEmitter */ public getUsersReadUpTo(event: MatrixEvent): string[] { return this.getReceiptsForEvent(event).filter(function(receipt) { - return [ReceiptType.Read, ReceiptType.ReadPrivate].includes(receipt.type); + return utils.isSupportedReceiptType(receipt.type); }).map(function(receipt) { return receipt.userId; }); @@ -2548,25 +2548,64 @@ export class Room extends TypedEventEmitter * @return {String} ID of the latest event that the given user has read, or null. */ public getEventReadUpTo(userId: string, ignoreSynthesized = false): string | 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 + const timelineSet = this.getUnfilteredTimelineSet(); - const publicReadReceipt = this.getReadReceiptForUserId(userId, ignoreSynthesized, ReceiptType.Read); - const privateReadReceipt = this.getReadReceiptForUserId(userId, ignoreSynthesized, ReceiptType.ReadPrivate); + const publicReadReceipt = this.getReadReceiptForUserId( + userId, + ignoreSynthesized, + ReceiptType.Read, + ); + const privateReadReceipt = this.getReadReceiptForUserId( + userId, + ignoreSynthesized, + ReceiptType.ReadPrivate, + ); + const unstablePrivateReadReceipt = this.getReadReceiptForUserId( + userId, + ignoreSynthesized, + ReceiptType.UnstableReadPrivate, + ); - // If we have both, compare them - let comparison: number | undefined; - if (publicReadReceipt?.eventId && privateReadReceipt?.eventId) { - comparison = timelineSet.compareEventOrdering(publicReadReceipt?.eventId, privateReadReceipt?.eventId); + // If we have all, compare them + if (publicReadReceipt?.eventId && privateReadReceipt?.eventId && unstablePrivateReadReceipt?.eventId) { + const comparison1 = timelineSet.compareEventOrdering( + publicReadReceipt.eventId, + privateReadReceipt.eventId, + ); + const comparison2 = timelineSet.compareEventOrdering( + publicReadReceipt.eventId, + unstablePrivateReadReceipt.eventId, + ); + const comparison3 = timelineSet.compareEventOrdering( + privateReadReceipt.eventId, + unstablePrivateReadReceipt.eventId, + ); + if (comparison1 && comparison2 && comparison3) { + return (comparison1 > 0) + ? ((comparison2 > 0) ? publicReadReceipt.eventId : unstablePrivateReadReceipt.eventId) + : ((comparison3 > 0) ? privateReadReceipt.eventId : unstablePrivateReadReceipt.eventId); + } } - // If we didn't get a comparison try to compare the ts of the receipts - if (!comparison) comparison = publicReadReceipt?.data?.ts - privateReadReceipt?.data?.ts; - - // 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 public read receipt is older, return the private one - return (comparison < 0) ? privateReadReceipt?.eventId : publicReadReceipt?.eventId; + let latest = privateReadReceipt; + [unstablePrivateReadReceipt, publicReadReceipt].forEach((receipt) => { + if (receipt?.data?.ts > latest?.data?.ts) { + latest = receipt; + } + }); + if (latest?.eventId) return latest?.eventId; + + // The more less likely it is for a read receipt to drift out of date + // the bigger is its precedence + return ( + privateReadReceipt?.eventId ?? + unstablePrivateReadReceipt?.eventId ?? + publicReadReceipt?.eventId ?? + null + ); } /** diff --git a/src/sync-accumulator.ts b/src/sync-accumulator.ts index 6c22be065d1..08686c32d67 100644 --- a/src/sync-accumulator.ts +++ b/src/sync-accumulator.ts @@ -20,7 +20,7 @@ limitations under the License. */ import { logger } from './logger'; -import { deepCopy } from "./utils"; +import { deepCopy, isSupportedReceiptType } from "./utils"; import { IContent, IUnsigned } from "./models/event"; import { IRoomSummary } from "./models/room-summary"; import { EventType } from "./@types/event"; @@ -417,31 +417,18 @@ export class SyncAccumulator { // of a hassle to work with. We'll inflate this back out when // getJSON() is called. Object.keys(e.content).forEach((eventId) => { - if (!e.content[eventId][ReceiptType.Read] && !e.content[eventId][ReceiptType.ReadPrivate]) { - return; - } - const read = e.content[eventId][ReceiptType.Read]; - if (read) { - Object.keys(read).forEach((userId) => { - // clobber on user ID - currentData._readReceipts[userId] = { - data: e.content[eventId][ReceiptType.Read][userId], - type: ReceiptType.Read, - eventId: eventId, - }; - }); - } - const readPrivate = e.content[eventId][ReceiptType.ReadPrivate]; - if (readPrivate) { - Object.keys(readPrivate).forEach((userId) => { + Object.entries(e.content[eventId]).forEach(([key, value]) => { + if (!isSupportedReceiptType(key)) return; + + Object.keys(value).forEach((userId) => { // clobber on user ID currentData._readReceipts[userId] = { - data: e.content[eventId][ReceiptType.ReadPrivate][userId], - type: ReceiptType.ReadPrivate, + data: e.content[eventId][key][userId], + type: key as ReceiptType, eventId: eventId, }; }); - } + }); }); }); } diff --git a/src/utils.ts b/src/utils.ts index 6cf459097c1..8ebafc1ae30 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -24,8 +24,9 @@ import unhomoglyph from "unhomoglyph"; import promiseRetry from "p-retry"; import type * as NodeCrypto from "crypto"; -import { MatrixEvent } from "."; +import { MatrixClient, MatrixEvent } from "."; import { M_TIMESTAMP } from "./@types/location"; +import { ReceiptType } from "./@types/read_receipts"; /** * Encode a dictionary of query parameters. @@ -648,3 +649,18 @@ function getContentTimestampWithFallback(event: MatrixEvent): number { export function sortEventsByLatestContentTimestamp(left: MatrixEvent, right: MatrixEvent): number { return getContentTimestampWithFallback(right) - getContentTimestampWithFallback(left); } + +export async function getPrivateReadReceiptField(client: MatrixClient): Promise { + if (await client.doesServerSupportUnstableFeature("org.matrix.msc2285.stable")) return ReceiptType.ReadPrivate; + if (await client.doesServerSupportUnstableFeature("org.matrix.msc2285")) return ReceiptType.UnstableReadPrivate; + return null; +} + +export function isSupportedReceiptType(receiptType: string): boolean { + return [ + ReceiptType.Read, + ReceiptType.ReadPrivate, + ReceiptType.UnstableReadPrivate, + ].includes(receiptType as ReceiptType); +} +