Fix issues with duplicated MatrixEvent objects around threads (#2256)

spec/test-utils/test-utils.js

@@ -85,7 +85,7 @@ export function mkEvent(opts) {
         room_id: opts.room,
         sender: opts.sender || opts.user, // opts.user for backwards-compat
         content: opts.content,
-        unsigned: opts.unsigned,
+        unsigned: opts.unsigned || {},
         event_id: "$" + Math.random() + "-" + Math.random(),
     };
     if (opts.skey !== undefined) {

spec/unit/event-mapper.spec.ts

@@ -0,0 +1,180 @@
+/*
+Copyright 2022 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, MatrixEventEvent, MatrixScheduler, Room } from "../../src";
+import { eventMapperFor } from "../../src/event-mapper";
+import { IStore } from "../../src/store";
+
+describe("eventMapperFor", function() {
+    let rooms: Room[] = [];
+
+    const userId = "@test:example.org";
+
+    let client: MatrixClient;
+
+    beforeEach(() => {
+        client = new MatrixClient({
+            baseUrl: "https://my.home.server",
+            accessToken: "my.access.token",
+            request: function() {} as any, // NOP
+            store: {
+                getRoom(roomId: string): Room | null {
+                    return rooms.find(r => r.roomId === roomId);
+                },
+            } as IStore,
+            scheduler: {
+                setProcessFunction: jest.fn(),
+            } as unknown as MatrixScheduler,
+            userId: userId,
+        });
+
+        rooms = [];
+    });
+
+    it("should de-duplicate MatrixEvent instances by means of findEventById on the room object", async () => {
+        const roomId = "!room:example.org";
+        const room = new Room(roomId, client, userId);
+        rooms.push(room);
+
+        const mapper = eventMapperFor(client, {
+            preventReEmit: true,
+            decrypt: false,
+        });
+
+        const eventId = "$event1:server";
+        const eventDefinition = {
+            type: "m.room.message",
+            room_id: roomId,
+            sender: userId,
+            content: {
+                body: "body",
+            },
+            unsigned: {},
+            event_id: eventId,
+        };
+
+        const event = mapper(eventDefinition);
+        expect(event).toBeInstanceOf(MatrixEvent);
+
+        room.addLiveEvents([event]);
+        expect(room.findEventById(eventId)).toBe(event);
+
+        const event2 = mapper(eventDefinition);
+        expect(event).toBe(event2);
+    });
+
+    it("should not de-duplicate state events due to directionality of sentinel members", async () => {
+        const roomId = "!room:example.org";
+        const room = new Room(roomId, client, userId);
+        rooms.push(room);
+
+        const mapper = eventMapperFor(client, {
+            preventReEmit: true,
+            decrypt: false,
+        });
+
+        const eventId = "$event1:server";
+        const eventDefinition = {
+            type: "m.room.name",
+            room_id: roomId,
+            sender: userId,
+            content: {
+                name: "Room name",
+            },
+            unsigned: {},
+            event_id: eventId,
+            state_key: "",
+        };
+
+        const event = mapper(eventDefinition);
+        expect(event).toBeInstanceOf(MatrixEvent);
+
+        room.oldState.setStateEvents([event]);
+        room.currentState.setStateEvents([event]);
+        room.addLiveEvents([event]);
+        expect(room.findEventById(eventId)).toBe(event);
+
+        const event2 = mapper(eventDefinition);
+        expect(event).not.toBe(event2);
+    });
+
+    it("should decrypt appropriately", async () => {
+        const roomId = "!room:example.org";
+        const room = new Room(roomId, client, userId);
+        rooms.push(room);
+
+        const eventId = "$event1:server";
+        const eventDefinition = {
+            type: "m.room.encrypted",
+            room_id: roomId,
+            sender: userId,
+            content: {
+                ciphertext: "",
+            },
+            unsigned: {},
+            event_id: eventId,
+        };
+
+        const decryptEventIfNeededSpy = jest.spyOn(client, "decryptEventIfNeeded");
+        decryptEventIfNeededSpy.mockResolvedValue(); // stub it out
+
+        const mapper = eventMapperFor(client, {
+            decrypt: true,
+        });
+        const event = mapper(eventDefinition);
+        expect(event).toBeInstanceOf(MatrixEvent);
+        expect(decryptEventIfNeededSpy).toHaveBeenCalledWith(event);
+    });
+
+    it("should configure re-emitter appropriately", async () => {
+        const roomId = "!room:example.org";
+        const room = new Room(roomId, client, userId);
+        rooms.push(room);
+
+        const eventId = "$event1:server";
+        const eventDefinition = {
+            type: "m.room.message",
+            room_id: roomId,
+            sender: userId,
+            content: {
+                body: "body",
+            },
+            unsigned: {},
+            event_id: eventId,
+        };
+
+        const evListener = jest.fn();
+        client.on(MatrixEventEvent.Replaced, evListener);
+
+        const noReEmitMapper = eventMapperFor(client, {
+            preventReEmit: true,
+        });
+        const event1 = noReEmitMapper(eventDefinition);
+        expect(event1).toBeInstanceOf(MatrixEvent);
+        event1.emit(MatrixEventEvent.Replaced, event1);
+        expect(evListener).not.toHaveBeenCalled();
+
+        const reEmitMapper = eventMapperFor(client, {
+            preventReEmit: false,
+        });
+        const event2 = reEmitMapper(eventDefinition);
+        expect(event2).toBeInstanceOf(MatrixEvent);
+        event2.emit(MatrixEventEvent.Replaced, event2);
+        expect(evListener.mock.calls[0][0]).toEqual(event2);
+
+        expect(event1).not.toBe(event2); // the event wasn't added to a room so de-duplication wouldn't occur
+    });
+});

spec/unit/event.spec.js

@@ -1,6 +1,6 @@
 /*
 Copyright 2017 New Vector Ltd
-Copyright 2019 The Matrix.org Foundaction C.I.C.
+Copyright 2019 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.

spec/unit/matrix-client.spec.ts

@@ -1,3 +1,19 @@
+/*
+Copyright 2022 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 { logger } from "../../src/logger";
 import { MatrixClient } from "../../src/client";
 import { Filter } from "../../src/filter";

spec/unit/room.spec.ts

@@ -26,7 +26,6 @@ import { Room } from "../../src/models/room";
 import { RoomState } from "../../src/models/room-state";
 import { UNSTABLE_ELEMENT_FUNCTIONAL_USERS } from "../../src/@types/event";
 import { TestClient } from "../TestClient";
-import { Thread } from "../../src/models/thread";
 
 describe("Room", function() {
     const roomId = "!foo:bar";
@@ -1867,54 +1866,6 @@ describe("Room", function() {
 
                 expect(() => room.createThread(rootEvent, [])).not.toThrow();
             });
-
-            it("should not add events before server supports is known", function() {
-                Thread.hasServerSideSupport = undefined;
-
-                const rootEvent = new MatrixEvent({
-                    event_id: "$666",
-                    room_id: roomId,
-                    content: {},
-                    unsigned: {
-                        "age": 1,
-                        "m.relations": {
-                            "m.thread": {
-                                latest_event: null,
-                                count: 1,
-                                current_user_participated: false,
-                            },
-                        },
-                    },
-                });
-
-                let age = 1;
-                function mkEvt(id): MatrixEvent {
-                    return new MatrixEvent({
-                        event_id: id,
-                        room_id: roomId,
-                        content: {
-                            "m.relates_to": {
-                                "rel_type": "m.thread",
-                                "event_id": "$666",
-                            },
-                        },
-                        unsigned: {
-                            "age": age++,
-                        },
-                    });
-                }
-
-                const thread = room.createThread(rootEvent, []);
-                expect(thread.length).toBe(0);
-
-                thread.addEvent(mkEvt("$1"));
-                expect(thread.length).toBe(0);
-
-                Thread.hasServerSideSupport = true;
-
-                thread.addEvent(mkEvt("$2"));
-                expect(thread.length).toBeGreaterThan(0);
-            });
         });
     });
 });

src/client.ts

@@ -3951,7 +3951,6 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
     /**
      * Returns the eventType that should be used taking encryption into account
      * for a given eventType.
-     * @param {MatrixClient} client the client
      * @param {string} roomId the room for the events `eventType` relates to
      * @param {string} eventType the event type
      * @return {string} the event type taking encryption into account
@@ -6621,18 +6620,18 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
             fetchedEventType,
             opts);
         const mapper = this.getEventMapper();
-        let originalEvent: MatrixEvent;
-        if (result.original_event) {
-            originalEvent = mapper(result.original_event);
-        }
+
+        const originalEvent = result.original_event ? mapper(result.original_event) : undefined;
         let events = result.chunk.map(mapper);
+
         if (fetchedEventType === EventType.RoomMessageEncrypted) {
             const allEvents = originalEvent ? events.concat(originalEvent) : events;
             await Promise.all(allEvents.map(e => this.decryptEventIfNeeded(e)));
             if (eventType !== null) {
                 events = events.filter(e => e.getType() === eventType);
             }
         }
+
         if (originalEvent && relationType === RelationType.Replace) {
             events = events.filter(e => e.getSender() === originalEvent.getSender());
         }
@@ -8866,12 +8865,11 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
         }
 
         const parentEventId = event.getAssociatedId();
-        const parentEvent = room?.findEventById(parentEventId) || events.find((mxEv: MatrixEvent) => (
+        const parentEvent = room?.findEventById(parentEventId) ?? events.find((mxEv: MatrixEvent) => (
             mxEv.getId() === parentEventId
         ));
 
-        // A reaction targetting the thread root needs to be routed to both the
-        // the main timeline and the associated thread
+        // A reaction targeting the thread root needs to be routed to both the main timeline and the associated thread
         const targetingThreadRoot = parentEvent?.isThreadRoot || roots.has(event.relationEventId);
         if (targetingThreadRoot) {
             return {
@@ -8887,18 +8885,21 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
         // we want that redaction to be pushed to both timeline
         if (parentEvent?.getAssociatedId()) {
             return this.eventShouldLiveIn(parentEvent, room, events, roots);
-        } else {
-            // We've exhausted all scenarios, can safely assume that this event
-            // should live in the room timeline
-            return {
-                shouldLiveInRoom: true,
-                shouldLiveInThread: false,
-            };
         }
+
+        // We've exhausted all scenarios, can safely assume that this event
+        // should live in the room timeline
+        return {
+            shouldLiveInRoom: true,
+            shouldLiveInThread: false,
+        };
     }
 
-    public partitionThreadedEvents(events: MatrixEvent[]): [MatrixEvent[], MatrixEvent[]] {
-        // Indices to the events array, for readibility
+    public partitionThreadedEvents(events: MatrixEvent[]): [
+        timelineEvents: MatrixEvent[],
+        threadedEvents: MatrixEvent[],
+    ] {
+        // Indices to the events array, for readability
         const ROOM = 0;
         const THREAD = 1;
         if (this.supportsExperimentalThreads()) {

src/event-mapper.ts

@@ -29,9 +29,22 @@ export function eventMapperFor(client: MatrixClient, options: MapperOpts): Event
     const decrypt = options.decrypt !== false;
 
     function mapper(plainOldJsObject: Partial<IEvent>) {
-        const event = new MatrixEvent(plainOldJsObject);
+        const room = client.getRoom(plainOldJsObject.room_id);
+
+        let event: MatrixEvent;
+        // If the event is already known to the room, let's re-use the model rather than duplicating.
+        // We avoid doing this to state events as they may be forward or backwards looking which tweaks behaviour.
+        if (room && plainOldJsObject.state_key === undefined) {
+            event = room.findEventById(plainOldJsObject.event_id);
+        }
+
+        if (!event || event.status) {
+            event = new MatrixEvent(plainOldJsObject);
+        } else {
+            // merge the latest unsigned data from the server
+            event.setUnsigned({ ...event.getUnsigned(), ...plainOldJsObject.unsigned });
+        }
 
-        const room = client.getRoom(event.getRoomId());
         if (room?.threads.has(event.getId())) {
             event.setThread(room.threads.get(event.getId()));
         }
@@ -46,6 +59,7 @@ export function eventMapperFor(client: MatrixClient, options: MapperOpts): Event
                 client.decryptEventIfNeeded(event);
             }
         }
+
         if (!preventReEmit) {
             client.reEmitter.reEmit(event, [
                 MatrixEventEvent.Replaced,

src/models/event.ts

@@ -287,7 +287,7 @@ export class MatrixEvent extends TypedEventEmitter<EmittedEvents, MatrixEventHan
     public target: RoomMember = null;
     public status: EventStatus = null;
     public error: MatrixError = null;
-    public forwardLooking = true;
+    public forwardLooking = true; // only state events may be backwards looking
 
     /* If the event is a `m.key.verification.request` (or to_device `m.key.verification.start`) event,
      * `Crypto` will set this the `VerificationRequest` for the event