From 4c051f8d7f3d862991b933626c4c5193f83af7bd Mon Sep 17 00:00:00 2001 From: Matt Gibson Date: Thu, 8 Feb 2024 14:54:15 -0500 Subject: [PATCH] ActiveUserState Update should return the userId of the impacted user. (#7869) This allows us to ensure that linked updates all go to the same user without risking active account changes in the middle of an operation. --- libs/common/spec/fake-state-provider.ts | 10 +++++-- libs/common/spec/fake-state.ts | 6 ++-- .../default-active-user-state.spec.ts | 15 ++++++---- .../default-active-user-state.ts | 17 ++++++----- .../implementations/default-state.provider.ts | 10 +++++-- .../src/platform/state/state.provider.ts | 6 +++- libs/common/src/platform/state/user-state.ts | 30 ++++++++++++++----- 7 files changed, 63 insertions(+), 31 deletions(-) diff --git a/libs/common/spec/fake-state-provider.ts b/libs/common/spec/fake-state-provider.ts index 34cf34d974..406cc1719b 100644 --- a/libs/common/spec/fake-state-provider.ts +++ b/libs/common/spec/fake-state-provider.ts @@ -147,11 +147,15 @@ export class FakeStateProvider implements StateProvider { return this.getActive(keyDefinition).state$; } - async setUserState(keyDefinition: KeyDefinition, value: T, userId?: UserId): Promise { + async setUserState( + keyDefinition: KeyDefinition, + value: T, + userId?: UserId, + ): Promise<[UserId, T]> { if (userId) { - await this.getUser(userId, keyDefinition).update(() => value); + return [userId, await this.getUser(userId, keyDefinition).update(() => value)]; } else { - await this.getActive(keyDefinition).update(() => value); + return await this.getActive(keyDefinition).update(() => value); } } diff --git a/libs/common/spec/fake-state.ts b/libs/common/spec/fake-state.ts index 11b18e76a2..bd033424e0 100644 --- a/libs/common/spec/fake-state.ts +++ b/libs/common/spec/fake-state.ts @@ -170,7 +170,7 @@ export class FakeActiveUserState implements ActiveUserState { async update( configureState: (state: T, dependency: TCombine) => T, options?: StateUpdateOptions, - ): Promise { + ): Promise<[UserId, T]> { options = populateOptionsWithDefault(options); const current = await firstValueFrom(this.state$.pipe(timeout(options.msTimeout))); const combinedDependencies = @@ -178,12 +178,12 @@ export class FakeActiveUserState implements ActiveUserState { ? await firstValueFrom(options.combineLatestWith.pipe(timeout(options.msTimeout))) : null; if (!options.shouldUpdate(current, combinedDependencies)) { - return current; + return [this.userId, current]; } const newState = configureState(current, combinedDependencies); this.stateSubject.next([this.userId, newState]); this.nextMock([this.userId, newState]); - return newState; + return [this.userId, newState]; } updateMock = this.update as jest.MockedFunction; diff --git a/libs/common/src/platform/state/implementations/default-active-user-state.spec.ts b/libs/common/src/platform/state/implementations/default-active-user-state.spec.ts index e0e3542269..399bd0cda8 100644 --- a/libs/common/src/platform/state/implementations/default-active-user-state.spec.ts +++ b/libs/common/src/platform/state/implementations/default-active-user-state.spec.ts @@ -266,12 +266,13 @@ describe("DefaultActiveUserState", () => { }); it("should save on update", async () => { - const result = await userState.update((state, dependencies) => { + const [setUserId, result] = await userState.update((state, dependencies) => { return newData; }); expect(diskStorageService.mock.save).toHaveBeenCalledTimes(1); expect(result).toEqual(newData); + expect(setUserId).toEqual("00000000-0000-1000-a000-000000000001"); }); it("should emit once per update", async () => { @@ -316,7 +317,7 @@ describe("DefaultActiveUserState", () => { const emissions = trackEmissions(userState.state$); await awaitAsync(); // Need to await for the initial value to be emitted - const result = await userState.update( + const [userIdResult, result] = await userState.update( (state, dependencies) => { return newData; }, @@ -328,6 +329,7 @@ describe("DefaultActiveUserState", () => { await awaitAsync(); expect(diskStorageService.mock.save).not.toHaveBeenCalled(); + expect(userIdResult).toEqual("00000000-0000-1000-a000-000000000001"); expect(result).toBeNull(); expect(emissions).toEqual([null]); }); @@ -422,12 +424,13 @@ describe("DefaultActiveUserState", () => { await originalSave(key, obj); }); - const val = await userState.update(() => { + const [userIdResult, val] = await userState.update(() => { return newData; }); await awaitAsync(10); + expect(userIdResult).toEqual(userId); expect(val).toEqual(newData); expect(emissions).toEqual([initialData, newData]); expect(emissions2).toEqual([initialData, newData]); @@ -447,7 +450,7 @@ describe("DefaultActiveUserState", () => { expect(emissions).toEqual([initialData]); let emissions2: TestState[]; - const val = await userState.update( + const [userIdResult, val] = await userState.update( (state) => { return newData; }, @@ -461,6 +464,7 @@ describe("DefaultActiveUserState", () => { await awaitAsync(); + expect(userIdResult).toEqual(userId); expect(val).toEqual(initialData); expect(emissions).toEqual([initialData]); @@ -497,10 +501,11 @@ describe("DefaultActiveUserState", () => { test("updates with FAKE_DEFAULT initial value should resolve correctly", async () => { expect(diskStorageService["updatesSubject"]["observers"]).toHaveLength(0); - const val = await userState.update((state) => { + const [userIdResult, val] = await userState.update((state) => { return newData; }); + expect(userIdResult).toEqual(userId); expect(val).toEqual(newData); const call = diskStorageService.mock.save.mock.calls[0]; expect(call[0]).toEqual(`user_${userId}_fake_fake`); diff --git a/libs/common/src/platform/state/implementations/default-active-user-state.ts b/libs/common/src/platform/state/implementations/default-active-user-state.ts index de84f058d3..e3cb0537d5 100644 --- a/libs/common/src/platform/state/implementations/default-active-user-state.ts +++ b/libs/common/src/platform/state/implementations/default-active-user-state.ts @@ -31,7 +31,7 @@ const FAKE = Symbol("fake"); export class DefaultActiveUserState implements ActiveUserState { [activeMarker]: true; - private updatePromise: Promise | null = null; + private updatePromise: Promise<[UserId, T]> | null = null; private activeUserId$: Observable; @@ -120,15 +120,15 @@ export class DefaultActiveUserState implements ActiveUserState { async update( configureState: (state: T, dependency: TCombine) => T, options: StateUpdateOptions = {}, - ): Promise { + ): Promise<[UserId, T]> { options = populateOptionsWithDefault(options); try { if (this.updatePromise != null) { await this.updatePromise; } this.updatePromise = this.internalUpdate(configureState, options); - const newState = await this.updatePromise; - return newState; + const [userId, newState] = await this.updatePromise; + return [userId, newState]; } finally { this.updatePromise = null; } @@ -137,20 +137,20 @@ export class DefaultActiveUserState implements ActiveUserState { private async internalUpdate( configureState: (state: T, dependency: TCombine) => T, options: StateUpdateOptions, - ) { - const [key, currentState] = await this.getStateForUpdate(); + ): Promise<[UserId, T]> { + const [userId, key, currentState] = await this.getStateForUpdate(); const combinedDependencies = options.combineLatestWith != null ? await firstValueFrom(options.combineLatestWith.pipe(timeout(options.msTimeout))) : null; if (!options.shouldUpdate(currentState, combinedDependencies)) { - return currentState; + return [userId, currentState]; } const newState = configureState(currentState, combinedDependencies); await this.saveToStorage(key, newState); - return newState; + return [userId, newState]; } /** For use in update methods, does not wait for update to complete before yielding state. @@ -170,6 +170,7 @@ export class DefaultActiveUserState implements ActiveUserState { } const fullKey = userKeyBuilder(userId, this.keyDefinition); return [ + userId, fullKey, await getStoredValue(fullKey, this.chosenStorageLocation, this.keyDefinition.deserializer), ] as const; diff --git a/libs/common/src/platform/state/implementations/default-state.provider.ts b/libs/common/src/platform/state/implementations/default-state.provider.ts index 4add19e2ba..4278326a9a 100644 --- a/libs/common/src/platform/state/implementations/default-state.provider.ts +++ b/libs/common/src/platform/state/implementations/default-state.provider.ts @@ -32,11 +32,15 @@ export class DefaultStateProvider implements StateProvider { } } - async setUserState(keyDefinition: KeyDefinition, value: T, userId?: UserId): Promise { + async setUserState( + keyDefinition: KeyDefinition, + value: T, + userId?: UserId, + ): Promise<[UserId, T]> { if (userId) { - await this.getUser(userId, keyDefinition).update(() => value); + return [userId, await this.getUser(userId, keyDefinition).update(() => value)]; } else { - await this.getActive(keyDefinition).update(() => value); + return await this.getActive(keyDefinition).update(() => value); } } diff --git a/libs/common/src/platform/state/state.provider.ts b/libs/common/src/platform/state/state.provider.ts index 51e41dd9e9..3f8b623f01 100644 --- a/libs/common/src/platform/state/state.provider.ts +++ b/libs/common/src/platform/state/state.provider.ts @@ -36,7 +36,11 @@ export abstract class StateProvider { * @param value - The value to set the state to. * @param userId - The userId for which you want to set the state for. If not provided, the state for the currently active user will be set. */ - setUserState: (keyDefinition: KeyDefinition, value: T, userId?: UserId) => Promise; + setUserState: ( + keyDefinition: KeyDefinition, + value: T, + userId?: UserId, + ) => Promise<[UserId, T]>; /** @see{@link ActiveUserStateProvider.get} */ getActive: (keyDefinition: KeyDefinition) => ActiveUserState; /** @see{@link SingleUserStateProvider.get} */ diff --git a/libs/common/src/platform/state/user-state.ts b/libs/common/src/platform/state/user-state.ts index 595d1fcf02..caef1792a7 100644 --- a/libs/common/src/platform/state/user-state.ts +++ b/libs/common/src/platform/state/user-state.ts @@ -19,6 +19,28 @@ export interface UserState { * Emits a stream of data alongside the user id the data corresponds to. */ readonly combinedState$: Observable>; +} + +export const activeMarker: unique symbol = Symbol("active"); +export interface ActiveUserState extends UserState { + readonly [activeMarker]: true; + /** + * Updates backing stores for the active user. + * @param configureState function that takes the current state and returns the new state + * @param options Defaults to @see {module:state-update-options#DEFAULT_OPTIONS} + * @param options.shouldUpdate A callback for determining if you want to update state. Defaults to () => true + * @param options.combineLatestWith An observable that you want to combine with the current state for callbacks. Defaults to null + * @param options.msTimeout A timeout for how long you are willing to wait for a `combineLatestWith` option to complete. Defaults to 1000ms. Only applies if `combineLatestWith` is set. + + * @returns The new state + */ + readonly update: ( + configureState: (state: T, dependencies: TCombine) => T, + options?: StateUpdateOptions, + ) => Promise<[UserId, T]>; +} +export interface SingleUserState extends UserState { + readonly userId: UserId; /** * Updates backing stores for the active user. @@ -35,11 +57,3 @@ export interface UserState { options?: StateUpdateOptions, ) => Promise; } - -export const activeMarker: unique symbol = Symbol("active"); -export interface ActiveUserState extends UserState { - readonly [activeMarker]: true; -} -export interface SingleUserState extends UserState { - readonly userId: UserId; -}