Add support for stable prefixes for MSC2285 (#2524)

Co-authored-by: Travis Ralston <travisr@matrix.org>

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;
+                    }
+                };
 
-            expect(room.getEventReadUpTo(userA)).toEqual("eventId1");
+                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}`);
+                }
+            });
+
+            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`);
+                });
+            });
         });
     });
 });

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]: {

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();
+        });
+    });
 });

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",
 }

src/client.ts

@@ -1088,11 +1088,12 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
                 // Figure out if we've read something or if it's just informational
                 const content = event.getContent();
                 const isSelf = Object.keys(content).filter(eid => {
-                    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<EmittedEvents, ClientEventHa
             room?.addLocalEchoReceipt(this.credentials.userId, rpEvent, ReceiptType.ReadPrivate);
         }
 
-        return this.setRoomReadMarkersHttpRequest(roomId, rmEventId, rrEventId, rpEventId);
+        return await this.setRoomReadMarkersHttpRequest(roomId, rmEventId, rrEventId, rpEventId);
     }
 
     /**
@@ -7500,7 +7501,7 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
      * don't want other users to see the read receipts. This is experimental. Optional.
      * @return {Promise} Resolves: the empty object, {}.
      */
-    public setRoomReadMarkersHttpRequest(
+    public async setRoomReadMarkersHttpRequest(
         roomId: string,
         rmEventId: string,
         rrEventId: string,
@@ -7513,9 +7514,13 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
         const content = {
             [ReceiptType.FullyRead]: rmEventId,
             [ReceiptType.Read]: rrEventId,
-            [ReceiptType.ReadPrivate]: rpEventId,
         };
 
+        const privateField = await utils.getPrivateReadReceiptField(this);
+        if (privateField) {
+            content[privateField] = rpEventId;
+        }
+
         return this.http.authedRequest(undefined, Method.Post, path, undefined, content);
     }
 

src/models/room.ts

@@ -2514,7 +2514,7 @@ export class Room extends TypedEventEmitter<EmittedEvents, RoomEventHandlerMap>
      */
     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<EmittedEvents, RoomEventHandlerMap>
      * @return {String} ID of the latest event that the given user has read, or null.
      */
     public getEventReadUpTo(userId: string, ignoreSynthesized = false): string | null {
-        const timelineSet = this.getUnfilteredTimelineSet();
-        const publicReadReceipt = this.getReadReceiptForUserId(userId, ignoreSynthesized, ReceiptType.Read);
-        const privateReadReceipt = this.getReadReceiptForUserId(userId, ignoreSynthesized, ReceiptType.ReadPrivate);
+        // 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
 
-        // If we have both, compare them
-        let comparison: number | undefined;
-        if (publicReadReceipt?.eventId && privateReadReceipt?.eventId) {
-            comparison = timelineSet.compareEventOrdering(publicReadReceipt?.eventId, privateReadReceipt?.eventId);
+        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,
+        );
+
+        // 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;
+        let latest = privateReadReceipt;
+        [unstablePrivateReadReceipt, publicReadReceipt].forEach((receipt) => {
+            if (receipt?.data?.ts > latest?.data?.ts) {
+                latest = receipt;
+            }
+        });
+        if (latest?.eventId) return latest?.eventId;
 
-        // 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;
+        // 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
+        );
     }
 
     /**

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) => {
+                    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.Read][userId],
-                                type: ReceiptType.Read,
+                                data: e.content[eventId][key][userId],
+                                type: key as ReceiptType,
                                 eventId: eventId,
                             };
                         });
-                    }
-                    const readPrivate = e.content[eventId][ReceiptType.ReadPrivate];
-                    if (readPrivate) {
-                        Object.keys(readPrivate).forEach((userId) => {
-                            // clobber on user ID
-                            currentData._readReceipts[userId] = {
-                                data: e.content[eventId][ReceiptType.ReadPrivate][userId],
-                                type: ReceiptType.ReadPrivate,
-                                eventId: eventId,
-                            };
-                        });
-                    }
+                    });
                 });
             });
         }

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<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);
+}
+