Remove support for unstable private read receipts (#2624)

SimonBrandner · web-flow · commit fb565f301b96 · 2022-09-12T18:04:14.000+02:00
diff --git 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) {
-                        return { eventId: "eventId2" } as IWrappedReceipt;
-                    }
                     if (receiptType === ReceiptType.Read) {
-                        return { eventId: "eventId3" } as IWrappedReceipt;
+                        return { eventId: "eventId2" } 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) {
-                            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;
                         }
+                        return null;
                     };
 
-                    expect(room.getEventReadUpTo(userA)).toEqual(`eventId2`);
+                    expect(room.getEventReadUpTo(userA)).toEqual(`eventId1`);
                 });
 
                 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
@@ -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
@@ -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
@@ -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
@@ -7543,9 +7543,8 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
             [ReceiptType.Read]: rrEventId,
         };
 
-        const privateField = await utils.getPrivateReadReceiptField(this);
-        if (privateField) {
-            content[privateField] = rpEventId;
+        if (await this.doesServerSupportUnstableFeature("org.matrix.msc2285.stable")) {
+            content[ReceiptType.ReadPrivate] = rpEventId;
         }
 
         return this.http.authedRequest(undefined, Method.Post, path, undefined, content);
diff --git a/src/models/room.ts b/src/models/room.ts
@@ -2600,59 +2600,26 @@ export class Room extends TypedEventEmitter<EmittedEvents, RoomEventHandlerMap>
         // 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;
-
-        // 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
-        );
+        // 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 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
@@ -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<ReceiptType | null> {
-    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);
 }
 

Original file line number	Diff line number	Diff line change
`@@ -18,8 +18,4 @@ export enum ReceiptType {`
`18`	`18`	`Read = "m.read",`
`19`	`19`	`FullyRead = "m.fully_read",`
`20`	`20`	`ReadPrivate = "m.read.private",`
`21`		`- /**`
`22`		`- * @deprecated Please use the ReadPrivate type when possible. This value may be removed at any time without notice.`
`23`		`- */`
`24`		`- UnstableReadPrivate = "org.matrix.msc2285.read.private",`
`25`	`21`	`}`