Add simple profile cache invalidation

src/stores/UserProfilesStores.ts

@@ -15,7 +15,7 @@ limitations under the License.
 */
 
 import { logger } from "matrix-js-sdk/src/logger";
-import { IMatrixProfile, MatrixClient } from "matrix-js-sdk/src/matrix";
+import { IMatrixProfile, MatrixClient, MatrixEvent, RoomMember, RoomMemberEvent } from "matrix-js-sdk/src/matrix";
 
 import { LruCache } from "../utils/LruCache";
 
@@ -25,12 +25,17 @@ type StoreProfileValue = IMatrixProfile | undefined | null;
 
 /**
  * This store provides cached access to user profiles.
+ * Listens for membership events and invalidates the cache for a profile on update with different profile values.
  */
 export class UserProfilesStore {
     private profiles = new LruCache<string, IMatrixProfile | null>(cacheSize);
     private knownProfiles = new LruCache<string, IMatrixProfile | null>(cacheSize);
 
-    public constructor(private client?: MatrixClient) {}
+    public constructor(private client?: MatrixClient) {
+        if (client) {
+            client.on(RoomMemberEvent.Membership, this.onRoomMembershipEvent);
+        }
+    }
 
     /**
      * Synchronously get a profile from the store cache.
@@ -84,8 +89,6 @@ export class UserProfilesStore {
      *          Null if the profile does not exist or there was an error fetching it.
      */
     public async fetchOnlyKnownProfile(userId: string): Promise<StoreProfileValue> {
-        console.log("fetchOnlyKnownProfile");
-
         // Do not look up unknown users. The test for existence in knownProfiles is a performance optimisation.
         // If the user Id exists in knownProfiles we know them.
         if (!this.knownProfiles.has(userId) && !this.isUserIdKnown(userId)) return undefined;
@@ -124,4 +127,28 @@ export class UserProfilesStore {
             return !!room.getMember(userId);
         });
     }
+
+    /**
+     * Simple cache invalidation if a room membership event is received and
+     * at least one profile value differs from the cached one.
+     */
+    private onRoomMembershipEvent = (event: MatrixEvent, member: RoomMember): void => {
+        const profile = this.profiles.get(member.userId);
+
+        if (
+            profile &&
+            (profile.displayname !== member.rawDisplayName || profile.avatar_url !== member.getMxcAvatarUrl())
+        ) {
+            this.profiles.delete(member.userId);
+        }
+
+        const knownProfile = this.knownProfiles.get(member.userId);
+
+        if (
+            knownProfile &&
+            (knownProfile.displayname !== member.rawDisplayName || knownProfile.avatar_url !== member.getMxcAvatarUrl())
+        ) {
+            this.knownProfiles.delete(member.userId);
+        }
+    };
 }

src/utils/LruCache.ts

@@ -124,6 +124,21 @@ export class LruCache<K, V> {
         }
     }
 
+    /**
+     * Deletes an item from the cache.
+     *
+     * @param key - Key of the item to be removed
+     */
+    public delete(key: K): void {
+        const item = this.map.get(key);
+
+        // Unknown item.
+        if (!item) return;
+
+        this.removeItemFromList(item);
+        this.map.delete(key);
+    }
+
     /**
      * Returns an iterator over the cached values.
      */
@@ -143,17 +158,7 @@ export class LruCache<K, V> {
         // No update required.
         if (item === this.head) return item;
 
-        // Remove item from the list…
-        if (item === this.tail && item.prev) {
-            this.tail = item.prev;
-        }
-
-        item.prev.next = item.next;
-
-        if (item.next) {
-            item.next.prev = item.prev;
-        }
-
+        this.removeItemFromList(item);
         // …and put it to the front.
         this.head.prev = item;
         item.prev = null;
@@ -162,4 +167,22 @@ export class LruCache<K, V> {
 
         return item;
     }
+
+    private removeItemFromList(item: CacheItem<K, V>): void {
+        if (item === this.head) {
+            this.head = item.next ?? null;
+        }
+
+        if (item === this.tail) {
+            this.tail = item.prev ?? null;
+        }
+
+        if (item.prev) {
+            item.prev.next = item.next;
+        }
+
+        if (item.next) {
+            item.next.prev = item.prev;
+        }
+    }
 }

test/stores/UserProfilesStore-test.ts

@@ -15,13 +15,13 @@ limitations under the License.
 */
 
 import { mocked, Mocked } from "jest-mock";
-import { IMatrixProfile, MatrixClient, Room } from "matrix-js-sdk/src/matrix";
+import { IMatrixProfile, MatrixClient, MatrixEvent, Room, RoomMemberEvent } from "matrix-js-sdk/src/matrix";
 
 import { UserProfilesStore } from "../../src/stores/UserProfilesStores";
-import { filterConsole, mkRoomMemberJoinEvent, stubClient } from "../test-utils";
+import { filterConsole, mkRoomMember, mkRoomMemberJoinEvent, stubClient } from "../test-utils";
 
 describe("UserProfilesStore", () => {
-    const userUnknownId = "@unknown:example.com";
+    const userIdDoesNotExist = "@unknown:example.com";
     const user1Id = "@user1:example.com";
     const user1Profile: IMatrixProfile = { displayname: "User 1", avatar_url: null };
     const user2Id = "@user2:example.com";
@@ -36,7 +36,7 @@ describe("UserProfilesStore", () => {
         "Error retrieving profile for userId @user3:example.com",
     );
 
-    beforeAll(() => {
+    beforeEach(() => {
         mockClient = mocked(stubClient());
         room = new Room("!room:example.com", mockClient, mockClient.getSafeUserId());
         room.currentState.setStateEvents([
@@ -58,37 +58,67 @@ describe("UserProfilesStore", () => {
         expect(userProfilesStore.getProfile(user1Id)).toBeUndefined();
     });
 
-    it("fetchProfile should return the profile from the API and cache it", async () => {
-        const profile = await userProfilesStore.fetchProfile(user1Id);
-        expect(profile).toBe(user1Profile);
-        expect(userProfilesStore.getProfile(user1Id)).toBe(user1Profile);
-    });
+    describe("fetchProfile", () => {
+        it("should return the profile from the API and cache it", async () => {
+            const profile = await userProfilesStore.fetchProfile(user1Id);
+            expect(profile).toBe(user1Profile);
+            expect(userProfilesStore.getProfile(user1Id)).toBe(user1Profile);
+        });
 
-    it("fetchProfile for an unknown user should return null and cache it", async () => {
-        const profile = await userProfilesStore.fetchProfile(userUnknownId);
-        expect(profile).toBeNull();
-        expect(userProfilesStore.getProfile(userUnknownId)).toBeNull();
+        it("for an user that does not exist should return null and cache it", async () => {
+            const profile = await userProfilesStore.fetchProfile(userIdDoesNotExist);
+            expect(profile).toBeNull();
+            expect(userProfilesStore.getProfile(userIdDoesNotExist)).toBeNull();
+        });
     });
 
     it("getOnlyKnownProfile should return undefined if the profile was not fetched", () => {
         expect(userProfilesStore.getOnlyKnownProfile(user1Id)).toBeUndefined();
     });
 
-    it("fetchOnlyKnownProfile should return undefined if no room shared with the user", async () => {
-        const profile = await userProfilesStore.fetchOnlyKnownProfile(user1Id);
-        expect(profile).toBeUndefined();
-        expect(userProfilesStore.getOnlyKnownProfile(user1Id)).toBeUndefined();
-    });
+    describe("fetchOnlyKnownProfile", () => {
+        it("should return undefined if no room shared with the user", async () => {
+            const profile = await userProfilesStore.fetchOnlyKnownProfile(user1Id);
+            expect(profile).toBeUndefined();
+            expect(userProfilesStore.getOnlyKnownProfile(user1Id)).toBeUndefined();
+        });
 
-    it("fetchOnlyKnownProfile for a known user should return the profile from the API and cache it", async () => {
-        const profile = await userProfilesStore.fetchOnlyKnownProfile(user2Id);
-        expect(profile).toBe(user2Profile);
-        expect(userProfilesStore.getOnlyKnownProfile(user2Id)).toBe(user2Profile);
+        it("for a known user should return the profile from the API and cache it", async () => {
+            const profile = await userProfilesStore.fetchOnlyKnownProfile(user2Id);
+            expect(profile).toBe(user2Profile);
+            expect(userProfilesStore.getOnlyKnownProfile(user2Id)).toBe(user2Profile);
+        });
+
+        it("for a known user not found via API should return null and cache it", async () => {
+            const profile = await userProfilesStore.fetchOnlyKnownProfile(user3Id);
+            expect(profile).toBeNull();
+            expect(userProfilesStore.getOnlyKnownProfile(user3Id)).toBeNull();
+        });
     });
 
-    it("fetchOnlyKnownProfile for a known user not found via API should return null and cache it", async () => {
-        const profile = await userProfilesStore.fetchOnlyKnownProfile(user3Id);
-        expect(profile).toBeNull();
-        expect(userProfilesStore.getOnlyKnownProfile(user3Id)).toBeNull();
+    describe("when there are cached values and membership updates", () => {
+        beforeEach(async () => {
+            await userProfilesStore.fetchProfile(user1Id);
+            await userProfilesStore.fetchOnlyKnownProfile(user2Id);
+        });
+
+        describe("and membership events with the same values appear", () => {
+            beforeEach(() => {
+                const roomMember1 = mkRoomMember(room.roomId, user1Id);
+                roomMember1.rawDisplayName = user1Profile.displayname;
+                roomMember1.getMxcAvatarUrl = () => null;
+                mockClient.emit(RoomMemberEvent.Membership, {} as MatrixEvent, roomMember1);
+
+                const roomMember2 = mkRoomMember(room.roomId, user2Id);
+                roomMember2.rawDisplayName = user2Profile.displayname;
+                roomMember2.getMxcAvatarUrl = () => null;
+                mockClient.emit(RoomMemberEvent.Membership, {} as MatrixEvent, roomMember2);
+            });
+
+            it("should not invalidate the cache", () => {
+                expect(userProfilesStore.getProfile(user1Id)).toBe(user1Profile);
+                expect(userProfilesStore.getOnlyKnownProfile(user2Id)).toBe(user2Profile);
+            });
+        });
     });
 });

test/utils/LruCache-test.ts

@@ -74,6 +74,33 @@ describe("LruCache", () => {
                 cache.set("c", "c value");
             });
 
+            it("deleting an unkonwn item should not raise an error", () => {
+                cache.delete("unknown");
+            });
+
+            it("deleting the first item should work", () => {
+                cache.delete("a");
+                expect(Array.from(cache.values())).toEqual(["b value", "c value"]);
+            });
+
+            it("deleting the item in the middle should work", () => {
+                cache.delete("b");
+                expect(Array.from(cache.values())).toEqual(["a value", "c value"]);
+            });
+
+            it("deleting the last item should work", () => {
+                cache.delete("c");
+                expect(Array.from(cache.values())).toEqual(["a value", "b value"]);
+            });
+
+            it("deleting and adding some items should work", () => {
+                cache.set("d", "d value");
+                cache.get("b");
+                cache.delete("b");
+                cache.set("e", "e value");
+                expect(Array.from(cache.values())).toEqual(["c value", "d value", "e value"]);
+            });
+
             describe("and accesing the first added item and adding another item", () => {
                 beforeEach(() => {
                     cache.get("a");