add MemberWriter, and only return MemberChange's if something changed

src/matrix/room/members/RoomMember.js

@@ -103,36 +103,34 @@ export class RoomMember {
     serialize() {
         return this._data;
     }
+
+    equals(other) {
+        const data = this._data;
+        const otherData = other._data;
+        return data.roomId === otherData.roomId &&
+            data.userId === otherData.userId &&
+            data.membership === otherData.membership &&
+            data.displayName === otherData.displayName &&
+            data.avatarUrl === otherData.avatarUrl;
+    }
 }
 
 export class MemberChange {
-    constructor(roomId, memberEvent) {
-        this._roomId = roomId;
-        this._memberEvent = memberEvent;
-        this._member = null;
-    }
-
-    get member() {
-        if (!this._member) {
-            this._member = RoomMember.fromMemberEvent(this._roomId, this._memberEvent);
-        }
-        return this._member;
+    constructor(member, previousMembership) {
+        this.member = member;
+        this.previousMembership = previousMembership;
     }
 
     get roomId() {
-        return this._roomId;
+        return this.member.roomId;
     }
 
     get userId() {
-        return this._memberEvent.state_key;
-    }
-
-    get previousMembership() {
-        return getPrevContentFromStateEvent(this._memberEvent)?.membership;
+        return this.member.userId;
     }
 
     get membership() {
-        return this._memberEvent.content?.membership;
+        return this.member.membership;
     }
 
     get hasLeft() {

src/matrix/room/timeline/persistence/MemberWriter.js

@@ -0,0 +1,203 @@
+/*
+Copyright 2021 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 {MemberChange, RoomMember, EVENT_TYPE as MEMBER_EVENT_TYPE} from "../../members/RoomMember.js";
+import {LRUCache} from "../../../../utils/LRUCache.js";
+
+export class MemberWriter {
+    constructor(roomId) {
+        this._roomId = roomId;
+        this._cache = new LRUCache(5, member => member.userId);
+    }
+
+    writeTimelineMemberEvent(event, txn) {
+        return this._writeMemberEvent(event, false, txn);
+    }
+
+    writeStateMemberEvent(event, isLimited, txn) {
+        // member events in the state section when the room response
+        // is not limited must always be lazy loaded members.
+        // If they are not, they will be repeated in the timeline anyway.
+        return this._writeMemberEvent(event, !isLimited, txn);
+    }
+
+    async _writeMemberEvent(event, isLazyLoadingMember, txn) {
+        const userId = event.state_key;
+        if (!userId) {
+            return;
+        }
+        const member = RoomMember.fromMemberEvent(this._roomId, event);
+        if (!member) {
+            return;
+        }
+
+        let existingMember = this._cache.get(userId);
+        if (!existingMember) {
+            const memberData = await txn.roomMembers.get(this._roomId, userId);
+            if (memberData) {
+                existingMember = new RoomMember(memberData);
+            }
+        }
+
+        // either never heard of the member, or something changed
+        if (!existingMember || !existingMember.equals(member)) {
+            txn.roomMembers.set(member.serialize());
+            this._cache.set(member);
+
+            if (!isLazyLoadingMember) {
+                return new MemberChange(member, existingMember?.membership);
+            }
+        }
+    }
+
+    async lookupMember(userId, timelineEvents, txn) {
+        let member = this._cache.get(userId);
+        if (!member) {
+            const memberData = await txn.roomMembers.get(this._roomId, userId);
+            if (memberData) {
+                member = new RoomMember(memberData);
+                this._cache.set(member);
+            }
+        }
+        if (!member) {
+            // sometimes the member event isn't included in state, but rather in the timeline,
+            // even if it is not the first event in the timeline. In this case, go look for the
+            // first occurence
+            const memberEvent = timelineEvents.find(e => {
+                return e.type === MEMBER_EVENT_TYPE && e.state_key === userId;
+            });
+            if (memberEvent) {
+                member = RoomMember.fromMemberEvent(this._roomId, memberEvent);
+                // adding it to the cache, but not storing it for now;
+                // we'll do that when we get to the event
+                this._cache.set(member);
+            }
+        }
+        return member;
+    }
+}
+
+export function tests() {
+
+    function createMemberEvent(membership, userId, displayName, avatarUrl) {
+        return {
+            content: {
+                membership,
+                "displayname": displayName,
+                "avatar_url": avatarUrl
+            },
+            sender: userId,
+            "state_key": userId,
+            type: "m.room.member"
+        };
+    }
+
+
+    function createStorage(initialMembers = []) {
+        const members = new Map();
+        for (const m of initialMembers) {
+            members.set(m.userId, m);
+        }
+        return {
+            members,
+            roomMembers: {
+                async get(_, userId) {
+                    return members.get(userId);
+                },
+                set(member) {
+                    members.set(member.userId, member);
+                }
+            }
+        }
+    }
+
+    function member(...args) {
+        return RoomMember.fromMemberEvent(roomId, createMemberEvent.apply(null, args));
+    }
+
+    const roomId = "abc";
+    const alice = "@alice:hs.tld";
+    const avatar = "mxc://hs.tld/def";
+
+    return {
+        "new join through state": async assert => {
+            const writer = new MemberWriter(roomId);
+            const txn = createStorage();
+            const change = await writer.writeStateMemberEvent(createMemberEvent("join", alice), true, txn);
+            assert(change.hasJoined);
+            assert.equal(txn.members.get(alice).membership, "join");
+        },
+        "accept invite through state": async assert => {
+            const writer = new MemberWriter(roomId);
+            const txn = createStorage([member("invite", alice)]);
+            const change = await writer.writeStateMemberEvent(createMemberEvent("join", alice), true, txn);
+            assert.equal(change.previousMembership, "invite");
+            assert(change.hasJoined);
+            assert.equal(txn.members.get(alice).membership, "join");
+        },
+        "change display name through timeline": async assert => {
+            const writer = new MemberWriter(roomId);
+            const txn = createStorage([member("join", alice, "Alice")]);
+            const change = await writer.writeTimelineMemberEvent(createMemberEvent("join", alice, "Alies"), txn);
+            assert(!change.hasJoined);
+            assert.equal(change.member.displayName, "Alies");
+            assert.equal(txn.members.get(alice).displayName, "Alies");
+        },
+        "set avatar through timeline": async assert => {
+            const writer = new MemberWriter(roomId);
+            const txn = createStorage([member("join", alice, "Alice")]);
+            const change = await writer.writeTimelineMemberEvent(createMemberEvent("join", alice, "Alice", avatar), txn);
+            assert(!change.hasJoined);
+            assert.equal(change.member.avatarUrl, avatar);
+            assert.equal(txn.members.get(alice).avatarUrl, avatar);
+        },
+        "ignore redundant member event": async assert => {
+            const writer = new MemberWriter(roomId);
+            const txn = createStorage([member("join", alice, "Alice", avatar)]);
+            const change = await writer.writeTimelineMemberEvent(createMemberEvent("join", alice, "Alice", avatar), txn);
+            assert(!change);
+        },
+        "leave": async assert => {
+            const writer = new MemberWriter(roomId);
+            const txn = createStorage([member("join", alice, "Alice")]);
+            const change = await writer.writeTimelineMemberEvent(createMemberEvent("leave", alice, "Alice"), txn);
+            assert(change.hasLeft);
+            assert(!change.hasJoined);
+        },
+        "ban": async assert => {
+            const writer = new MemberWriter(roomId);
+            const txn = createStorage([member("join", alice, "Alice")]);
+            const change = await writer.writeTimelineMemberEvent(createMemberEvent("ban", alice, "Alice"), txn);
+            assert(change.hasLeft);
+            assert(!change.hasJoined);
+        },
+        "reject invite": async assert => {
+            const writer = new MemberWriter(roomId);
+            const txn = createStorage([member("invite", alice, "Alice")]);
+            const change = await writer.writeTimelineMemberEvent(createMemberEvent("leave", alice, "Alice"), txn);
+            assert(!change.hasLeft);
+            assert(!change.hasJoined);
+        },
+        "lazy loaded member is written but no change returned": async assert => {
+            const writer = new MemberWriter(roomId);
+            const txn = createStorage();
+            const change = await writer.writeStateMemberEvent(createMemberEvent("join", alice, "Alice"), false, txn);
+            assert(!change);
+            assert.equal(txn.members.get(alice).displayName, "Alice");
+        },
+        
+    };
+}

src/matrix/room/timeline/persistence/SyncWriter.js

@@ -1,5 +1,6 @@
 /*
 Copyright 2020 Bruno Windels <bruno@windels.cloud>
+Copyright 2020 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.
@@ -18,7 +19,8 @@ import {EventKey} from "../EventKey.js";
 import {EventEntry} from "../entries/EventEntry.js";
 import {FragmentBoundaryEntry} from "../entries/FragmentBoundaryEntry.js";
 import {createEventEntry} from "./common.js";
-import {MemberChange, RoomMember, EVENT_TYPE as MEMBER_EVENT_TYPE} from "../../members/RoomMember.js";
+import {EVENT_TYPE as MEMBER_EVENT_TYPE} from "../../members/RoomMember.js";
+import {MemberWriter} from "./MemberWriter.js";
 
 // Synapse bug? where the m.room.create event appears twice in sync response
 // when first syncing the room
@@ -37,6 +39,7 @@ function deduplicateEvents(events) {
 export class SyncWriter {
     constructor({roomId, fragmentIdComparer}) {
         this._roomId = roomId;
+        this._memberWriter = new MemberWriter(roomId);
         this._fragmentIdComparer = fragmentIdComparer;
         this._lastLiveKey = null;
     }
@@ -130,38 +133,20 @@ export class SyncWriter {
         return currentKey;
     }
 
-    _writeMember(event, txn) {
-        const userId = event.state_key;
-        if (userId) {
-            const memberChange = new MemberChange(this._roomId, event);
-            const {member} = memberChange;
-            if (member) {
-                // TODO: can we avoid writing redundant members here by checking
-                // if this is not a limited sync and the state is not in the timeline?
-                txn.roomMembers.set(member.serialize());
-                return memberChange;
-            }
-        }
-    }
-
-    _writeStateEvent(event, txn) {
-        if (event.type === MEMBER_EVENT_TYPE) {
-            return this._writeMember(event, txn);
-        } else {
-            txn.roomState.set(this._roomId, event);
-        }
-    }
-
-    _writeStateEvents(roomResponse, memberChanges, txn, log) {
+    async _writeStateEvents(roomResponse, memberChanges, isLimited, txn, log) {
         // persist state
         const {state} = roomResponse;
         if (Array.isArray(state?.events)) {
             log.set("stateEvents", state.events.length);
             for (const event of state.events) {
-                const memberChange = this._writeStateEvent(event, txn);
+                if (event.type === MEMBER_EVENT_TYPE) {
+                    const memberChange = await this._memberWriter.writeStateMemberEvent(event, isLimited, txn);
                     if (memberChange) {
                         memberChanges.set(memberChange.userId, memberChange);
                     }
+                } else {
+                    txn.roomState.set(this._roomId, event);
+                }
             }
         }
     }
@@ -177,21 +162,27 @@ export class SyncWriter {
                 // store event in timeline
                 currentKey = currentKey.nextKey();
                 const entry = createEventEntry(currentKey, this._roomId, event);
-                let memberData = await this._findMemberData(event.sender, events, txn);
-                if (memberData) {
-                    entry.displayName = memberData.displayName;
-                    entry.avatarUrl = memberData.avatarUrl;
+                let member = await this._memberWriter.lookupMember(event.sender, events, txn);
+                if (member) {
+                    entry.displayName = member.displayName;
+                    entry.avatarUrl = member.avatarUrl;
                 }
                 txn.timelineEvents.insert(entry);
                 entries.push(new EventEntry(entry, this._fragmentIdComparer));
 
-                // process live state events first, so new member info is available
+                // update state events after writing event, so for a member event,
+                // we only update the member info after having written the member event
+                // to the timeline, as we want that event to have the old profile info
                 if (typeof event.state_key === "string") {
                     timelineStateEventCount += 1;
-                    const memberChange = this._writeStateEvent(event, txn);
+                    if (event.type === MEMBER_EVENT_TYPE) {
+                        const memberChange = await this._memberWriter.writeTimelineMemberEvent(event, txn);
                         if (memberChange) {
                             memberChanges.set(memberChange.userId, memberChange);
                         }
+                    } else {
+                        txn.roomState.set(this._roomId, event);
+                    }
                 }
             }
             log.set("timelineStateEventCount", timelineStateEventCount);
@@ -199,24 +190,6 @@ export class SyncWriter {
         return currentKey;
     }
 
-    async _findMemberData(userId, events, txn) {
-        // TODO: perhaps add a small cache here?
-        const memberData = await txn.roomMembers.get(this._roomId, userId);
-        if (memberData) {
-            return memberData;
-        } else {
-            // sometimes the member event isn't included in state, but rather in the timeline,
-            // even if it is not the first event in the timeline. In this case, go look for the
-            // first occurence
-            const memberEvent = events.find(e => {
-                return e.type === MEMBER_EVENT_TYPE && e.state_key === userId;
-            });
-            if (memberEvent) {
-                return RoomMember.fromMemberEvent(this._roomId, memberEvent)?.serialize(); 
-            }
-        }
-    }
-
     /**
      * @type {SyncWriterResult}
      * @property {Array<BaseEntry>} entries new timeline entries written
@@ -233,7 +206,7 @@ export class SyncWriter {
         const memberChanges = new Map();
         // important this happens before _writeTimeline so
         // members are available in the transaction
-        this._writeStateEvents(roomResponse, memberChanges, txn, log);
+        await this._writeStateEvents(roomResponse, memberChanges, timeline?.limited, txn, log);
         const currentKey = await this._writeTimeline(entries, timeline, this._lastLiveKey, memberChanges, txn, log);
         log.set("memberChanges", memberChanges.size);
         return {entries, newLiveKey: currentKey, memberChanges};