From 57609737f1e35eea83901bb07dc34e7a79fbc83e Mon Sep 17 00:00:00 2001 From: Justin Baur <19896123+justindbaur@users.noreply.github.com> Date: Thu, 18 Jan 2024 13:02:30 -0500 Subject: [PATCH] Fix SingleUserStateProvider (#7593) * Fix SingleUserStateProvider - Fix cache key to be unique per instance per user * Add Specific State Provider Tests * Add Missing await --- .../src/platform/services/crypto.service.ts | 2 +- .../specific-state.provider.spec.ts | 124 ++++++++++++++++++ .../src/platform/state/key-definition.spec.ts | 22 ++++ .../src/platform/state/key-definition.ts | 8 +- 4 files changed, 152 insertions(+), 4 deletions(-) create mode 100644 libs/common/src/platform/state/implementations/specific-state.provider.spec.ts diff --git a/libs/common/src/platform/services/crypto.service.ts b/libs/common/src/platform/services/crypto.service.ts index 8041be6590..602785940e 100644 --- a/libs/common/src/platform/services/crypto.service.ts +++ b/libs/common/src/platform/services/crypto.service.ts @@ -62,7 +62,7 @@ export class CryptoService implements CryptoServiceAbstraction { userId ??= (await firstValueFrom(this.accountService.activeAccount$))?.id; if (key != null) { // Key should never be null anyway - this.stateProvider.getUser(userId, USER_EVER_HAD_USER_KEY).update(() => true); + await this.stateProvider.getUser(userId, USER_EVER_HAD_USER_KEY).update(() => true); } await this.stateService.setUserKey(key, { userId: userId }); await this.storeAdditionalKeys(key, userId); diff --git a/libs/common/src/platform/state/implementations/specific-state.provider.spec.ts b/libs/common/src/platform/state/implementations/specific-state.provider.spec.ts new file mode 100644 index 0000000000..d663751672 --- /dev/null +++ b/libs/common/src/platform/state/implementations/specific-state.provider.spec.ts @@ -0,0 +1,124 @@ +import { mockAccountServiceWith } from "../../../../spec/fake-account-service"; +import { FakeStorageService } from "../../../../spec/fake-storage.service"; +import { UserId } from "../../../types/guid"; +import { KeyDefinition } from "../key-definition"; +import { StateDefinition } from "../state-definition"; + +import { DefaultActiveUserState } from "./default-active-user-state"; +import { DefaultActiveUserStateProvider } from "./default-active-user-state.provider"; +import { DefaultGlobalState } from "./default-global-state"; +import { DefaultGlobalStateProvider } from "./default-global-state.provider"; +import { DefaultSingleUserState } from "./default-single-user-state"; +import { DefaultSingleUserStateProvider } from "./default-single-user-state.provider"; + +describe("Specific State Providers", () => { + let singleSut: DefaultSingleUserStateProvider; + let activeSut: DefaultActiveUserStateProvider; + let globalSut: DefaultGlobalStateProvider; + + const fakeUser1 = "00000000-0000-1000-a000-000000000001" as UserId; + + beforeEach(() => { + singleSut = new DefaultSingleUserStateProvider( + new FakeStorageService() as any, + new FakeStorageService(), + ); + activeSut = new DefaultActiveUserStateProvider( + mockAccountServiceWith(null), + new FakeStorageService() as any, + new FakeStorageService(), + ); + globalSut = new DefaultGlobalStateProvider( + new FakeStorageService() as any, + new FakeStorageService(), + ); + }); + + const fakeDiskStateDefinition = new StateDefinition("fake", "disk"); + const fakeAlternateDiskStateDefinition = new StateDefinition("fakeAlternate", "disk"); + const fakeMemoryStateDefinition = new StateDefinition("fake", "memory"); + + const fakeDiskKeyDefinition = new KeyDefinition(fakeDiskStateDefinition, "fake", { + deserializer: (b) => b, + }); + const fakeAlternateKeyDefinition = new KeyDefinition( + fakeAlternateDiskStateDefinition, + "fake", + { + deserializer: (b) => b, + }, + ); + const fakeMemoryKeyDefinition = new KeyDefinition(fakeMemoryStateDefinition, "fake", { + deserializer: (b) => b, + }); + const fakeDiskKeyDefinitionAlternate = new KeyDefinition( + fakeDiskStateDefinition, + "fakeAlternate", + { + deserializer: (b) => b, + }, + ); + + describe.each([ + { + // Use a static user id so that it has the same signature as the rest and then write special tests + // handling differing user id + getMethod: (keyDefinition: KeyDefinition) => singleSut.get(fakeUser1, keyDefinition), + expectedInstance: DefaultSingleUserState, + }, + { + getMethod: (keyDefinition: KeyDefinition) => activeSut.get(keyDefinition), + expectedInstance: DefaultActiveUserState, + }, + { + getMethod: (keyDefinition: KeyDefinition) => globalSut.get(keyDefinition), + expectedInstance: DefaultGlobalState, + }, + ])("common behavior %s", ({ getMethod, expectedInstance }) => { + it("returns expected instance", () => { + const state = getMethod(fakeDiskKeyDefinition); + + expect(state).toBeTruthy(); + expect(state).toBeInstanceOf(expectedInstance); + }); + + it("returns cached instance on repeated request", () => { + const stateFirst = getMethod(fakeDiskKeyDefinition); + const stateCached = getMethod(fakeDiskKeyDefinition); + expect(stateFirst).toStrictEqual(stateCached); + }); + + it("returns different instances when the storage location differs", () => { + const stateDisk = getMethod(fakeDiskKeyDefinition); + const stateMemory = getMethod(fakeMemoryKeyDefinition); + expect(stateDisk).not.toStrictEqual(stateMemory); + }); + + it("returns different instances when the state name differs", () => { + const state = getMethod(fakeDiskKeyDefinition); + const stateAlt = getMethod(fakeAlternateKeyDefinition); + expect(state).not.toStrictEqual(stateAlt); + }); + + it("returns different instances when the key differs", () => { + const state = getMethod(fakeDiskKeyDefinition); + const stateAlt = getMethod(fakeDiskKeyDefinitionAlternate); + expect(state).not.toStrictEqual(stateAlt); + }); + }); + + describe("DefaultSingleUserStateProvider only behavior", () => { + const fakeUser2 = "00000000-0000-1000-a000-000000000002" as UserId; + + it("returns different instances when the user id differs", () => { + const user1State = singleSut.get(fakeUser1, fakeDiskKeyDefinition); + const user2State = singleSut.get(fakeUser2, fakeDiskKeyDefinition); + expect(user1State).not.toStrictEqual(user2State); + }); + + it("returns an instance with the userId property corresponding to the user id passed in", () => { + const userState = singleSut.get(fakeUser1, fakeDiskKeyDefinition); + expect(userState.userId).toBe(fakeUser1); + }); + }); +}); diff --git a/libs/common/src/platform/state/key-definition.spec.ts b/libs/common/src/platform/state/key-definition.spec.ts index ee926bccd8..ba9a056c52 100644 --- a/libs/common/src/platform/state/key-definition.spec.ts +++ b/libs/common/src/platform/state/key-definition.spec.ts @@ -1,5 +1,7 @@ import { Opaque } from "type-fest"; +import { UserId } from "../../types/guid"; + import { KeyDefinition } from "./key-definition"; import { StateDefinition } from "./state-definition"; @@ -109,4 +111,24 @@ describe("KeyDefinition", () => { expect(deserializedValue[1]).toBeFalsy(); }); }); + + describe("buildCacheKey", () => { + const keyDefinition = new KeyDefinition(fakeStateDefinition, "fake", { + deserializer: (s) => s, + }); + + it("builds unique cache key for each user", () => { + const cacheKeys: string[] = []; + + // single user keys + cacheKeys.push(keyDefinition.buildCacheKey("user", "1" as UserId)); + cacheKeys.push(keyDefinition.buildCacheKey("user", "2" as UserId)); + + expect(new Set(cacheKeys).size).toBe(cacheKeys.length); + }); + + it("throws with bad usage", () => { + expect(() => keyDefinition.buildCacheKey("user", null)).toThrow(); + }); + }); }); diff --git a/libs/common/src/platform/state/key-definition.ts b/libs/common/src/platform/state/key-definition.ts index 4f2481b33f..64bd0acd66 100644 --- a/libs/common/src/platform/state/key-definition.ts +++ b/libs/common/src/platform/state/key-definition.ts @@ -148,11 +148,13 @@ export class KeyDefinition { */ buildCacheKey(scope: "user" | "global", userId?: "active" | UserId): string { if (scope === "user" && userId == null) { - throw new Error("You must provide a userId when building a user scoped cache key."); + throw new Error( + "You must provide a userId or 'active' when building a user scoped cache key.", + ); } return userId === null - ? `${this.stateDefinition.storageLocation}_${scope}_${userId}_${this.stateDefinition.name}_${this.key}` - : `${this.stateDefinition.storageLocation}_${scope}_${this.stateDefinition.name}_${this.key}`; + ? `${this.stateDefinition.storageLocation}_${scope}_${this.stateDefinition.name}_${this.key}` + : `${this.stateDefinition.storageLocation}_${scope}_${userId}_${this.stateDefinition.name}_${this.key}`; } private get errorKeyName() {