From fb565f301b96fc962dd4b4ba7c234f1e87a61eeb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brandner?= Date: Mon, 12 Sep 2022 18:04:14 +0200 Subject: [PATCH] Remove support for unstable private read receipts (#2624) --- spec/unit/room.spec.ts | 42 +++++-------------- spec/unit/sync-accumulator.spec.ts | 6 --- spec/unit/utils.spec.ts | 36 ----------------- src/@types/read_receipts.ts | 4 -- src/client.ts | 5 +-- src/models/room.ts | 65 ++++++++---------------------- src/utils.ts | 14 +------ 7 files changed, 31 insertions(+), 141 deletions(-) diff --git a/spec/unit/room.spec.ts b/spec/unit/room.spec.ts index 800415d2b..f94eca4a9 100644 --- a/spec/unit/room.spec.ts +++ b/spec/unit/room.spec.ts @@ -2452,15 +2452,13 @@ describe("Room", function() { if (receiptType === ReceiptType.ReadPrivate) { return { eventId: "eventId1" } as IWrappedReceipt; } - if (receiptType === ReceiptType.UnstableReadPrivate) { + if (receiptType === ReceiptType.Read) { return { eventId: "eventId2" } as IWrappedReceipt; } - if (receiptType === ReceiptType.Read) { - return { eventId: "eventId3" } as IWrappedReceipt; - } + return null; }; - for (let i = 1; i <= 3; i++) { + for (let i = 1; i <= 2; i++) { room.getUnfilteredTimelineSet = () => ({ compareEventOrdering: (event1, event2) => { return (event1 === `eventId${i}`) ? 1 : -1; } } as EventTimelineSet); @@ -2471,20 +2469,18 @@ describe("Room", function() { describe("correctly compares by timestamp", () => { it("should correctly compare, if we have all receipts", () => { - for (let i = 1; i <= 3; i++) { + for (let i = 1; i <= 2; 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; + return { eventId: "eventId1", data: { ts: i === 1 ? 2 : 1 } } as IWrappedReceipt; } if (receiptType === ReceiptType.Read) { - return { eventId: "eventId3", data: { ts: i === 3 ? 1 : 0 } } as IWrappedReceipt; + return { eventId: "eventId2", data: { ts: i === 2 ? 2 : 1 } } as IWrappedReceipt; } + return null; }; expect(room.getEventReadUpTo(userA)).toEqual(`eventId${i}`); @@ -2496,12 +2492,10 @@ describe("Room", function() { compareEventOrdering: (_1, _2) => null, } as EventTimelineSet); room.getReadReceiptForUserId = (userId, ignore, receiptType) => { - if (receiptType === ReceiptType.UnstableReadPrivate) { - return { eventId: "eventId1", data: { ts: 0 } } as IWrappedReceipt; - } if (receiptType === ReceiptType.Read) { return { eventId: "eventId2", data: { ts: 1 } } as IWrappedReceipt; } + return null; }; expect(room.getEventReadUpTo(userA)).toEqual(`eventId2`); @@ -2520,35 +2514,21 @@ describe("Room", function() { if (receiptType === ReceiptType.ReadPrivate) { return { eventId: "eventId1" } as IWrappedReceipt; } - if (receiptType === ReceiptType.UnstableReadPrivate) { + if (receiptType === ReceiptType.Read) { return { eventId: "eventId2" } as IWrappedReceipt; } - if (receiptType === ReceiptType.Read) { - return { eventId: "eventId3" } as IWrappedReceipt; - } + return null; }; 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; } + return null; }; expect(room.getEventReadUpTo(userA)).toEqual(`eventId3`); diff --git a/spec/unit/sync-accumulator.spec.ts b/spec/unit/sync-accumulator.spec.ts index b5385330b..645efbfbb 100644 --- a/spec/unit/sync-accumulator.spec.ts +++ b/spec/unit/sync-accumulator.spec.ts @@ -302,9 +302,6 @@ 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" }, }, @@ -350,9 +347,6 @@ 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 be30890ed..5d804022e 100644 --- a/spec/unit/utils.spec.ts +++ b/spec/unit/utils.spec.ts @@ -528,38 +528,6 @@ describe("utils", function() { }); }); - 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(); @@ -569,10 +537,6 @@ describe("utils", function() { 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 16a67feb3..4e90ac2ea 100644 --- a/src/@types/read_receipts.ts +++ b/src/@types/read_receipts.ts @@ -18,8 +18,4 @@ export enum ReceiptType { Read = "m.read", FullyRead = "m.fully_read", 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 dabb1a959..0c42fdd8c 100644 --- a/src/client.ts +++ b/src/client.ts @@ -7543,9 +7543,8 @@ export class MatrixClient extends TypedEventEmitter // 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 unstablePrivateReadReceipt = this.getReadReceiptForUserId( - userId, - ignoreSynthesized, - ReceiptType.UnstableReadPrivate, - ); + const publicReadReceipt = this.getReadReceiptForUserId(userId, ignoreSynthesized, ReceiptType.Read); + const privateReadReceipt = this.getReadReceiptForUserId(userId, ignoreSynthesized, ReceiptType.ReadPrivate); - // 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 have both, compare them + let comparison: number | null | undefined; + if (publicReadReceipt?.eventId && privateReadReceipt?.eventId) { + comparison = timelineSet.compareEventOrdering(publicReadReceipt?.eventId, privateReadReceipt?.eventId); } - let latest = privateReadReceipt; - [unstablePrivateReadReceipt, publicReadReceipt].forEach((receipt) => { - if (receipt?.data?.ts > latest?.data?.ts || !latest) { - latest = receipt; - } - }); - if (latest?.eventId) return latest?.eventId; + // If we didn't get a comparison try to compare the ts of the receipts + if (!comparison && publicReadReceipt?.data?.ts && privateReadReceipt?.data?.ts) { + comparison = publicReadReceipt?.data?.ts - privateReadReceipt?.data?.ts; + } - // 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 - ); + // 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) ?? null; } /** diff --git a/src/utils.ts b/src/utils.ts index 13f5dd44c..f51be323b 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -24,7 +24,7 @@ import unhomoglyph from "unhomoglyph"; import promiseRetry from "p-retry"; import type * as NodeCrypto from "crypto"; -import { MatrixClient, MatrixEvent } from "."; +import { MatrixEvent } from "."; import { M_TIMESTAMP } from "./@types/location"; import { ReceiptType } from "./@types/read_receipts"; @@ -670,17 +670,7 @@ export function sortEventsByLatestContentTimestamp(left: MatrixEvent, right: Mat 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); + return [ReceiptType.Read, ReceiptType.ReadPrivate].includes(receiptType as ReceiptType); }