From df3b20bf3d0884202e3476efac436dea51cb2b78 Mon Sep 17 00:00:00 2001 From: Michael Weimann Date: Wed, 22 Mar 2023 15:29:19 +0100 Subject: [PATCH] Add simple profile cache invalidation --- src/stores/UserProfilesStores.ts | 35 ++++++++++-- src/utils/LruCache.ts | 45 +++++++++++---- test/stores/UserProfilesStore-test.ts | 82 ++++++++++++++++++--------- test/utils/LruCache-test.ts | 27 +++++++++ 4 files changed, 148 insertions(+), 41 deletions(-) diff --git a/src/stores/UserProfilesStores.ts b/src/stores/UserProfilesStores.ts index 49f07fc5990..cd41bea28e0 100644 --- a/src/stores/UserProfilesStores.ts +++ b/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(cacheSize); private knownProfiles = new LruCache(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 { - 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); + } + }; } diff --git a/src/utils/LruCache.ts b/src/utils/LruCache.ts index 8c80bba3092..39f84e3fa6b 100644 --- a/src/utils/LruCache.ts +++ b/src/utils/LruCache.ts @@ -124,6 +124,21 @@ export class LruCache { } } + /** + * 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 { // 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 { return item; } + + private removeItemFromList(item: CacheItem): 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; + } + } } diff --git a/test/stores/UserProfilesStore-test.ts b/test/stores/UserProfilesStore-test.ts index dbe2b687966..15711957a11 100644 --- a/test/stores/UserProfilesStore-test.ts +++ b/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); + }); + }); }); }); diff --git a/test/utils/LruCache-test.ts b/test/utils/LruCache-test.ts index 2bec8f96e1d..5f0dff8c5ca 100644 --- a/test/utils/LruCache-test.ts +++ b/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");