Require lifetime specification of user-scoped data (#8669)

* Require lifetime specification of user-scoped data

* Decouple tests for different classes

This coupling assumed constant interfaces with classes that isn't a guarantee and requires significant acrobatics to make types work, now that key definitions are not a consistent base.

* Fix types

libs/common/src/platform/state/implementations/default-active-user-state.provider.ts

@@ -2,8 +2,7 @@ import { Observable, distinctUntilChanged, map } from "rxjs";
 
 import { AccountService } from "../../../auth/abstractions/account.service";
 import { UserId } from "../../../types/guid";
-import { KeyDefinition } from "../key-definition";
-import { UserKeyDefinition, isUserKeyDefinition } from "../user-key-definition";
+import { UserKeyDefinition } from "../user-key-definition";
 import { ActiveUserState } from "../user-state";
 import { ActiveUserStateProvider, SingleUserStateProvider } from "../user-state.provider";
 
@@ -23,11 +22,7 @@ export class DefaultActiveUserStateProvider implements ActiveUserStateProvider {
     );
   }
 
-  get<T>(keyDefinition: KeyDefinition<T> | UserKeyDefinition<T>): ActiveUserState<T> {
-    if (!isUserKeyDefinition(keyDefinition)) {
-      keyDefinition = UserKeyDefinition.fromBaseKeyDefinition(keyDefinition);
-    }
-
+  get<T>(keyDefinition: UserKeyDefinition<T>): ActiveUserState<T> {
     // All other providers cache the creation of their corresponding `State` objects, this instance
     // doesn't need to do that since it calls `SingleUserStateProvider` it will go through their caching
     // layer, because of that, the creation of this instance is quite simple and not worth caching.

libs/common/src/platform/state/implementations/default-single-user-state.provider.ts

@@ -1,8 +1,7 @@
 import { UserId } from "../../../types/guid";
 import { StorageServiceProvider } from "../../services/storage-service.provider";
-import { KeyDefinition } from "../key-definition";
 import { StateEventRegistrarService } from "../state-event-registrar.service";
-import { UserKeyDefinition, isUserKeyDefinition } from "../user-key-definition";
+import { UserKeyDefinition } from "../user-key-definition";
 import { SingleUserState } from "../user-state";
 import { SingleUserStateProvider } from "../user-state.provider";
 
@@ -16,13 +15,7 @@ export class DefaultSingleUserStateProvider implements SingleUserStateProvider {
     private readonly stateEventRegistrarService: StateEventRegistrarService,
   ) {}
 
-  get<T>(
-    userId: UserId,
-    keyDefinition: KeyDefinition<T> | UserKeyDefinition<T>,
-  ): SingleUserState<T> {
-    if (!isUserKeyDefinition(keyDefinition)) {
-      keyDefinition = UserKeyDefinition.fromBaseKeyDefinition(keyDefinition);
-    }
+  get<T>(userId: UserId, keyDefinition: UserKeyDefinition<T>): SingleUserState<T> {
     const [location, storageService] = this.storageServiceProvider.get(
       keyDefinition.stateDefinition.defaultStorageLocation,
       keyDefinition.stateDefinition.storageLocationOverrides,

libs/common/src/platform/state/implementations/default-state.provider.spec.ts

@@ -17,6 +17,7 @@ import { UserId } from "../../../types/guid";
 import { DeriveDefinition } from "../derive-definition";
 import { KeyDefinition } from "../key-definition";
 import { StateDefinition } from "../state-definition";
+import { UserKeyDefinition } from "../user-key-definition";
 
 import { DefaultStateProvider } from "./default-state.provider";
 
@@ -52,12 +53,12 @@ describe("DefaultStateProvider", () => {
   describe.each([
     [
       "getUserState$",
-      (keyDefinition: KeyDefinition<string>, userId?: UserId) =>
+      (keyDefinition: UserKeyDefinition<string>, userId?: UserId) =>
         sut.getUserState$(keyDefinition, userId),
     ],
     [
       "getUserStateOrDefault$",
-      (keyDefinition: KeyDefinition<string>, userId?: UserId) =>
+      (keyDefinition: UserKeyDefinition<string>, userId?: UserId) =>
         sut.getUserStateOrDefault$(keyDefinition, { userId: userId }),
     ],
   ])(
@@ -65,7 +66,7 @@ describe("DefaultStateProvider", () => {
     (
       _testName: string,
       methodUnderTest: (
-        keyDefinition: KeyDefinition<string>,
+        keyDefinition: UserKeyDefinition<string>,
         userId?: UserId,
       ) => Observable<string>,
     ) => {
@@ -75,9 +76,14 @@ describe("DefaultStateProvider", () => {
         name: "name",
         status: AuthenticationStatus.LoggedOut,
       };
-      const keyDefinition = new KeyDefinition<string>(new StateDefinition("test", "disk"), "test", {
-        deserializer: (s) => s,
-      });
+      const keyDefinition = new UserKeyDefinition<string>(
+        new StateDefinition("test", "disk"),
+        "test",
+        {
+          deserializer: (s) => s,
+          clearOn: [],
+        },
+      );
 
       it("should follow the specified user if userId is provided", async () => {
         const state = singleUserStateProvider.getFake(userId, keyDefinition);
@@ -125,9 +131,14 @@ describe("DefaultStateProvider", () => {
       name: "name",
       status: AuthenticationStatus.LoggedOut,
     };
-    const keyDefinition = new KeyDefinition<string>(new StateDefinition("test", "disk"), "test", {
-      deserializer: (s) => s,
-    });
+    const keyDefinition = new UserKeyDefinition<string>(
+      new StateDefinition("test", "disk"),
+      "test",
+      {
+        deserializer: (s) => s,
+        clearOn: [],
+      },
+    );
 
     it("should not emit any values until a truthy user id is supplied", async () => {
       accountService.activeAccountSubject.next(null);
@@ -149,9 +160,14 @@ describe("DefaultStateProvider", () => {
   });
 
   describe("getUserStateOrDefault$", () => {
-    const keyDefinition = new KeyDefinition<string>(new StateDefinition("test", "disk"), "test", {
-      deserializer: (s) => s,
-    });
+    const keyDefinition = new UserKeyDefinition<string>(
+      new StateDefinition("test", "disk"),
+      "test",
+      {
+        deserializer: (s) => s,
+        clearOn: [],
+      },
+    );
 
     it("should emit default value if no userId supplied and first active user id emission in falsy", async () => {
       accountService.activeAccountSubject.next(null);
@@ -168,9 +184,14 @@ describe("DefaultStateProvider", () => {
   });
 
   describe("setUserState", () => {
-    const keyDefinition = new KeyDefinition<string>(new StateDefinition("test", "disk"), "test", {
-      deserializer: (s) => s,
-    });
+    const keyDefinition = new UserKeyDefinition<string>(
+      new StateDefinition("test", "disk"),
+      "test",
+      {
+        deserializer: (s) => s,
+        clearOn: [],
+      },
+    );
 
     it("should set the state for the active user if no userId is provided", async () => {
       const value = "value";
@@ -202,8 +223,9 @@ describe("DefaultStateProvider", () => {
   });
 
   it("should bind the activeUserStateProvider", () => {
-    const keyDefinition = new KeyDefinition(new StateDefinition("test", "disk"), "test", {
+    const keyDefinition = new UserKeyDefinition(new StateDefinition("test", "disk"), "test", {
       deserializer: () => null,
+      clearOn: [],
     });
     const existing = activeUserStateProvider.get(keyDefinition);
     const actual = sut.getActive(keyDefinition);
@@ -212,8 +234,9 @@ describe("DefaultStateProvider", () => {
 
   it("should bind the singleUserStateProvider", () => {
     const userId = "user" as UserId;
-    const keyDefinition = new KeyDefinition(new StateDefinition("test", "disk"), "test", {
+    const keyDefinition = new UserKeyDefinition(new StateDefinition("test", "disk"), "test", {
       deserializer: () => null,
+      clearOn: [],
     });
     const existing = singleUserStateProvider.get(userId, keyDefinition);
     const actual = sut.getUser(userId, keyDefinition);

libs/common/src/platform/state/implementations/default-state.provider.ts

@@ -6,7 +6,6 @@ import { DeriveDefinition } from "../derive-definition";
 import { DerivedState } from "../derived-state";
 import { DerivedStateProvider } from "../derived-state.provider";
 import { GlobalStateProvider } from "../global-state.provider";
-import { KeyDefinition } from "../key-definition";
 import { StateProvider } from "../state.provider";
 import { UserKeyDefinition } from "../user-key-definition";
 import { ActiveUserStateProvider, SingleUserStateProvider } from "../user-state.provider";
@@ -22,47 +21,44 @@ export class DefaultStateProvider implements StateProvider {
     this.activeUserId$ = this.activeUserStateProvider.activeUserId$;
   }
 
-  getUserState$<T>(
-    keyDefinition: KeyDefinition<T> | UserKeyDefinition<T>,
-    userId?: UserId,
-  ): Observable<T> {
+  getUserState$<T>(userKeyDefinition: UserKeyDefinition<T>, userId?: UserId): Observable<T> {
     if (userId) {
-      return this.getUser<T>(userId, keyDefinition).state$;
+      return this.getUser<T>(userId, userKeyDefinition).state$;
     } else {
       return this.activeUserId$.pipe(
         filter((userId) => userId != null), // Filter out null-ish user ids since we can't get state for a null user id
         take(1),
-        switchMap((userId) => this.getUser<T>(userId, keyDefinition).state$),
+        switchMap((userId) => this.getUser<T>(userId, userKeyDefinition).state$),
       );
     }
   }
 
   getUserStateOrDefault$<T>(
-    keyDefinition: KeyDefinition<T> | UserKeyDefinition<T>,
+    userKeyDefinition: UserKeyDefinition<T>,
     config: { userId: UserId | undefined; defaultValue?: T },
   ): Observable<T> {
     const { userId, defaultValue = null } = config;
     if (userId) {
-      return this.getUser<T>(userId, keyDefinition).state$;
+      return this.getUser<T>(userId, userKeyDefinition).state$;
     } else {
       return this.activeUserId$.pipe(
         take(1),
         switchMap((userId) =>
-          userId != null ? this.getUser<T>(userId, keyDefinition).state$ : of(defaultValue),
+          userId != null ? this.getUser<T>(userId, userKeyDefinition).state$ : of(defaultValue),
         ),
       );
     }
   }
 
   async setUserState<T>(
-    keyDefinition: KeyDefinition<T> | UserKeyDefinition<T>,
+    userKeyDefinition: UserKeyDefinition<T>,
     value: T,
     userId?: UserId,
   ): Promise<[UserId, T]> {
     if (userId) {
-      return [userId, await this.getUser<T>(userId, keyDefinition).update(() => value)];
+      return [userId, await this.getUser<T>(userId, userKeyDefinition).update(() => value)];
     } else {
-      return await this.getActive<T>(keyDefinition).update(() => value);
+      return await this.getActive<T>(userKeyDefinition).update(() => value);
     }
   }
 

libs/common/src/platform/state/implementations/specific-state.provider.spec.ts

@@ -7,6 +7,7 @@ import { StorageServiceProvider } from "../../services/storage-service.provider"
 import { KeyDefinition } from "../key-definition";
 import { StateDefinition } from "../state-definition";
 import { StateEventRegistrarService } from "../state-event-registrar.service";
+import { UserKeyDefinition } from "../user-key-definition";
 
 import { DefaultActiveUserState } from "./default-active-user-state";
 import { DefaultActiveUserStateProvider } from "./default-active-user-state.provider";
@@ -41,94 +42,132 @@ describe("Specific State Providers", () => {
   const fakeDiskStateDefinition = new StateDefinition("fake", "disk");
   const fakeAlternateDiskStateDefinition = new StateDefinition("fakeAlternate", "disk");
   const fakeMemoryStateDefinition = new StateDefinition("fake", "memory");
-
-  const fakeDiskKeyDefinition = new KeyDefinition<boolean>(fakeDiskStateDefinition, "fake", {
-    deserializer: (b) => b,
-  });
-  const fakeAlternateKeyDefinition = new KeyDefinition<boolean>(
-    fakeAlternateDiskStateDefinition,
-    "fake",
-    {
+  const makeKeyDefinition = (stateDefinition: StateDefinition, key: string) =>
+    new KeyDefinition<boolean>(stateDefinition, key, {
       deserializer: (b) => b,
-    },
-  );
-  const fakeMemoryKeyDefinition = new KeyDefinition<boolean>(fakeMemoryStateDefinition, "fake", {
-    deserializer: (b) => b,
-  });
-  const fakeDiskKeyDefinitionAlternate = new KeyDefinition<boolean>(
-    fakeDiskStateDefinition,
-    "fakeAlternate",
-    {
+    });
+  const makeUserKeyDefinition = (stateDefinition: StateDefinition, key: string) =>
+    new UserKeyDefinition<boolean>(stateDefinition, key, {
       deserializer: (b) => b,
+      clearOn: [],
+    });
+  const keyDefinitions = {
+    disk: {
+      keyDefinition: makeKeyDefinition(fakeDiskStateDefinition, "fake"),
+      userKeyDefinition: makeUserKeyDefinition(fakeDiskStateDefinition, "fake"),
+      altKeyDefinition: makeKeyDefinition(fakeDiskStateDefinition, "fakeAlternate"),
+      altUserKeyDefinition: makeUserKeyDefinition(fakeDiskStateDefinition, "fakeAlternate"),
     },
-  );
+    memory: {
+      keyDefinition: makeKeyDefinition(fakeMemoryStateDefinition, "fake"),
+      userKeyDefinition: makeUserKeyDefinition(fakeMemoryStateDefinition, "fake"),
+    },
+    alternateDisk: {
+      keyDefinition: makeKeyDefinition(fakeAlternateDiskStateDefinition, "fake"),
+      userKeyDefinition: makeUserKeyDefinition(fakeAlternateDiskStateDefinition, "fake"),
+    },
+  };
 
-  const globalAndSingle = [
-    {
-      getMethod: (keyDefinition: KeyDefinition<boolean>) => globalSut.get(keyDefinition),
-      expectedInstance: DefaultGlobalState,
-    },
-    {
-      // 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<boolean>) => singleSut.get(fakeUser1, keyDefinition),
-      expectedInstance: DefaultSingleUserState,
-    },
-  ];
+  describe("active provider", () => {
+    it("returns a DefaultActiveUserState", () => {
+      const state = activeSut.get(keyDefinitions.disk.userKeyDefinition);
 
-  describe.each([
-    {
-      getMethod: (keyDefinition: KeyDefinition<boolean>) => activeSut.get(keyDefinition),
-      expectedInstance: DefaultActiveUserState,
-    },
-    ...globalAndSingle,
-  ])("common behavior %s", ({ getMethod, expectedInstance }) => {
-    it("returns expected instance", () => {
-      const state = getMethod(fakeDiskKeyDefinition);
-
-      expect(state).toBeTruthy();
-      expect(state).toBeInstanceOf(expectedInstance);
+      expect(state).toBeInstanceOf(DefaultActiveUserState);
     });
 
     it("returns different instances when the storage location differs", () => {
-      const stateDisk = getMethod(fakeDiskKeyDefinition);
-      const stateMemory = getMethod(fakeMemoryKeyDefinition);
+      const stateDisk = activeSut.get(keyDefinitions.disk.userKeyDefinition);
+      const stateMemory = activeSut.get(keyDefinitions.memory.userKeyDefinition);
       expect(stateDisk).not.toStrictEqual(stateMemory);
     });
 
     it("returns different instances when the state name differs", () => {
-      const state = getMethod(fakeDiskKeyDefinition);
-      const stateAlt = getMethod(fakeAlternateKeyDefinition);
+      const state = activeSut.get(keyDefinitions.disk.userKeyDefinition);
+      const stateAlt = activeSut.get(keyDefinitions.alternateDisk.userKeyDefinition);
       expect(state).not.toStrictEqual(stateAlt);
     });
 
     it("returns different instances when the key differs", () => {
-      const state = getMethod(fakeDiskKeyDefinition);
-      const stateAlt = getMethod(fakeDiskKeyDefinitionAlternate);
+      const state = activeSut.get(keyDefinitions.disk.userKeyDefinition);
+      const stateAlt = activeSut.get(keyDefinitions.disk.altUserKeyDefinition);
       expect(state).not.toStrictEqual(stateAlt);
     });
   });
 
-  describe.each(globalAndSingle)("Global And Single Behavior", ({ getMethod }) => {
-    it("returns cached instance on repeated request", () => {
-      const stateFirst = getMethod(fakeDiskKeyDefinition);
-      const stateCached = getMethod(fakeDiskKeyDefinition);
-      expect(stateFirst).toStrictEqual(stateCached);
-    });
-  });
+  describe("single provider", () => {
+    it("returns a DefaultSingleUserState", () => {
+      const state = singleSut.get(fakeUser1, keyDefinitions.disk.userKeyDefinition);
+
+      expect(state).toBeInstanceOf(DefaultSingleUserState);
+    });
+
+    it("returns different instances when the storage location differs", () => {
+      const stateDisk = singleSut.get(fakeUser1, keyDefinitions.disk.userKeyDefinition);
+      const stateMemory = singleSut.get(fakeUser1, keyDefinitions.memory.userKeyDefinition);
+      expect(stateDisk).not.toStrictEqual(stateMemory);
+    });
+
+    it("returns different instances when the state name differs", () => {
+      const state = singleSut.get(fakeUser1, keyDefinitions.disk.userKeyDefinition);
+      const stateAlt = singleSut.get(fakeUser1, keyDefinitions.alternateDisk.userKeyDefinition);
+      expect(state).not.toStrictEqual(stateAlt);
+    });
+
+    it("returns different instances when the key differs", () => {
+      const state = singleSut.get(fakeUser1, keyDefinitions.disk.userKeyDefinition);
+      const stateAlt = singleSut.get(fakeUser1, keyDefinitions.disk.altUserKeyDefinition);
+      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);
+      const user1State = singleSut.get(fakeUser1, keyDefinitions.disk.userKeyDefinition);
+      const user2State = singleSut.get(fakeUser2, keyDefinitions.disk.userKeyDefinition);
       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);
+      const userState = singleSut.get(fakeUser1, keyDefinitions.disk.userKeyDefinition);
       expect(userState.userId).toBe(fakeUser1);
     });
+
+    it("returns cached instance on repeated request", () => {
+      const stateFirst = singleSut.get(fakeUser1, keyDefinitions.disk.userKeyDefinition);
+      const stateCached = singleSut.get(fakeUser1, keyDefinitions.disk.userKeyDefinition);
+      expect(stateFirst).toStrictEqual(stateCached);
+    });
+  });
+
+  describe("global provider", () => {
+    it("returns a DefaultGlobalState", () => {
+      const state = globalSut.get(keyDefinitions.disk.keyDefinition);
+
+      expect(state).toBeInstanceOf(DefaultGlobalState);
+    });
+
+    it("returns different instances when the storage location differs", () => {
+      const stateDisk = globalSut.get(keyDefinitions.disk.keyDefinition);
+      const stateMemory = globalSut.get(keyDefinitions.memory.keyDefinition);
+      expect(stateDisk).not.toStrictEqual(stateMemory);
+    });
+
+    it("returns different instances when the state name differs", () => {
+      const state = globalSut.get(keyDefinitions.disk.keyDefinition);
+      const stateAlt = globalSut.get(keyDefinitions.alternateDisk.keyDefinition);
+      expect(state).not.toStrictEqual(stateAlt);
+    });
+
+    it("returns different instances when the key differs", () => {
+      const state = globalSut.get(keyDefinitions.disk.keyDefinition);
+      const stateAlt = globalSut.get(keyDefinitions.disk.altKeyDefinition);
+      expect(state).not.toStrictEqual(stateAlt);
+    });
+
+    it("returns cached instance on repeated request", () => {
+      const stateFirst = globalSut.get(keyDefinitions.disk.keyDefinition);
+      const stateCached = globalSut.get(keyDefinitions.disk.keyDefinition);
+      expect(stateFirst).toStrictEqual(stateCached);
+    });
   });
 });