1
0
mirror of https://github.com/bitwarden/browser synced 2025-12-17 08:43:33 +00:00

Ps/pm 2910/state framework improvements (#6860)

* Allow for update logic in state update callbacks

* Prefer reading updates to sending in stream

* Inform state providers when they must deserialize

* Update DefaultGlobalState to act more like DefaultUserState

* Fully Implement AbstractStorageService

* Add KeyDefinitionOptions

* Address PR feedback

* More Descriptive Error

---------

Co-authored-by: Justin Baur <19896123+justindbaur@users.noreply.github.com>
This commit is contained in:
Matt Gibson
2023-11-16 14:15:34 -05:00
committed by GitHub
parent bb46907951
commit 29aabeb4f5
25 changed files with 761 additions and 171 deletions

View File

@@ -3,9 +3,10 @@
* @jest-environment ../shared/test.environment.ts
*/
import { firstValueFrom, of } from "rxjs";
import { Jsonify } from "type-fest";
import { trackEmissions } from "../../../../spec";
import { trackEmissions, awaitAsync } from "../../../../spec";
import { FakeStorageService } from "../../../../spec/fake-storage.service";
import { KeyDefinition, globalKeyBuilder } from "../key-definition";
import { StateDefinition } from "../state-definition";
@@ -28,16 +29,15 @@ class TestState {
const testStateDefinition = new StateDefinition("fake", "disk");
const testKeyDefinition = new KeyDefinition<TestState>(
testStateDefinition,
"fake",
TestState.fromJSON
);
const testKeyDefinition = new KeyDefinition<TestState>(testStateDefinition, "fake", {
deserializer: TestState.fromJSON,
});
const globalKey = globalKeyBuilder(testKeyDefinition);
describe("DefaultGlobalState", () => {
let diskStorageService: FakeStorageService;
let globalState: DefaultGlobalState<TestState>;
const newData = { date: new Date() };
beforeEach(() => {
diskStorageService = new FakeStorageService();
@@ -48,51 +48,154 @@ describe("DefaultGlobalState", () => {
jest.resetAllMocks();
});
it("should emit when storage updates", async () => {
const emissions = trackEmissions(globalState.state$);
const newData = { date: new Date() };
await diskStorageService.save(globalKey, newData);
describe("state$", () => {
it("should emit when storage updates", async () => {
const emissions = trackEmissions(globalState.state$);
await diskStorageService.save(globalKey, newData);
await awaitAsync();
expect(emissions).toEqual([
null, // Initial value
newData,
// JSON.parse(JSON.stringify(newData)), // This is due to the way `trackEmissions` clones
]);
});
it("should not emit when update key does not match", async () => {
const emissions = trackEmissions(globalState.state$);
const newData = { date: new Date() };
await diskStorageService.save("wrong_key", newData);
expect(emissions).toEqual(
expect.arrayContaining([
expect(emissions).toEqual([
null, // Initial value
])
);
});
it("should save on update", async () => {
const newData = { date: new Date() };
const result = await globalState.update((state) => {
return newData;
newData,
]);
});
expect(diskStorageService.mock.save).toHaveBeenCalledTimes(1);
expect(result).toEqual(newData);
});
it("should not emit when update key does not match", async () => {
const emissions = trackEmissions(globalState.state$);
await diskStorageService.save("wrong_key", newData);
it("should emit once per update", async () => {
const emissions = trackEmissions(globalState.state$);
const newData = { date: new Date() };
await globalState.update((state) => {
return newData;
expect(emissions).toHaveLength(0);
});
expect(emissions).toEqual([
null, // Initial value
newData,
]);
it("should emit initial storage value on first subscribe", async () => {
const initialStorage: Record<string, TestState> = {};
initialStorage[globalKey] = TestState.fromJSON({
date: "2022-09-21T13:14:17.648Z",
});
diskStorageService.internalUpdateStore(initialStorage);
const state = await firstValueFrom(globalState.state$);
expect(diskStorageService.mock.get).toHaveBeenCalledTimes(1);
expect(diskStorageService.mock.get).toHaveBeenCalledWith("global_fake_fake", undefined);
expect(state).toBeTruthy();
});
});
describe("update", () => {
it("should save on update", async () => {
const result = await globalState.update((state) => {
return newData;
});
expect(diskStorageService.mock.save).toHaveBeenCalledTimes(1);
expect(result).toEqual(newData);
});
it("should emit once per update", async () => {
const emissions = trackEmissions(globalState.state$);
await awaitAsync(); // storage updates are behind a promise
await globalState.update((state) => {
return newData;
});
await awaitAsync();
expect(emissions).toEqual([
null, // Initial value
newData,
]);
});
it("should provided combined dependencies", async () => {
const emissions = trackEmissions(globalState.state$);
await awaitAsync(); // storage updates are behind a promise
const combinedDependencies = { date: new Date() };
await globalState.update(
(state, dependencies) => {
expect(dependencies).toEqual(combinedDependencies);
return newData;
},
{
combineLatestWith: of(combinedDependencies),
}
);
await awaitAsync();
expect(emissions).toEqual([
null, // Initial value
newData,
]);
});
it("should not update if shouldUpdate returns false", async () => {
const emissions = trackEmissions(globalState.state$);
const result = await globalState.update(
(state) => {
return newData;
},
{
shouldUpdate: () => false,
}
);
expect(diskStorageService.mock.save).not.toHaveBeenCalled();
expect(emissions).toEqual([null]); // Initial value
expect(result).toBeUndefined();
});
it("should provide the update callback with the current State", async () => {
const emissions = trackEmissions(globalState.state$);
await awaitAsync(); // storage updates are behind a promise
// Seed with interesting data
const initialData = { date: new Date(2020, 1, 1) };
await globalState.update((state, dependencies) => {
return initialData;
});
await awaitAsync();
await globalState.update((state) => {
expect(state).toEqual(initialData);
return newData;
});
await awaitAsync();
expect(emissions).toEqual([
null, // Initial value
initialData,
newData,
]);
});
it("should give initial state for update call", async () => {
const initialStorage: Record<string, TestState> = {};
const initialState = TestState.fromJSON({
date: "2022-09-21T13:14:17.648Z",
});
initialStorage[globalKey] = initialState;
diskStorageService.internalUpdateStore(initialStorage);
const emissions = trackEmissions(globalState.state$);
await awaitAsync(); // storage updates are behind a promise
const newState = {
...initialState,
date: new Date(initialState.date.getFullYear(), initialState.date.getMonth() + 1),
};
const actual = await globalState.update((existingState) => newState);
await awaitAsync();
expect(actual).toEqual(newState);
expect(emissions).toHaveLength(2);
expect(emissions).toEqual(expect.arrayContaining([initialState, newState]));
});
});
});

View File

@@ -1,15 +1,29 @@
import { BehaviorSubject, Observable, defer, filter, map, shareReplay, tap } from "rxjs";
import { Jsonify } from "type-fest";
import {
BehaviorSubject,
Observable,
defer,
filter,
firstValueFrom,
shareReplay,
switchMap,
tap,
timeout,
} from "rxjs";
import { AbstractStorageService } from "../../abstractions/storage.service";
import { GlobalState } from "../global-state";
import { KeyDefinition, globalKeyBuilder } from "../key-definition";
import { StateUpdateOptions, populateOptionsWithDefault } from "../state-update-options";
import { getStoredValue } from "./util";
const FAKE_DEFAULT = Symbol("fakeDefault");
export class DefaultGlobalState<T> implements GlobalState<T> {
private storageKey: string;
private seededPromise: Promise<void>;
protected stateSubject: BehaviorSubject<T | null> = new BehaviorSubject<T | null>(null);
protected stateSubject: BehaviorSubject<T | typeof FAKE_DEFAULT> = new BehaviorSubject<
T | typeof FAKE_DEFAULT
>(FAKE_DEFAULT);
state$: Observable<T>;
@@ -19,15 +33,17 @@ export class DefaultGlobalState<T> implements GlobalState<T> {
) {
this.storageKey = globalKeyBuilder(this.keyDefinition);
this.seededPromise = this.chosenLocation.get<Jsonify<T>>(this.storageKey).then((data) => {
const serializedData = this.keyDefinition.deserializer(data);
this.stateSubject.next(serializedData);
});
const storageUpdates$ = this.chosenLocation.updates$.pipe(
filter((update) => update.key === this.storageKey),
map((update) => {
return this.keyDefinition.deserializer(update.value as Jsonify<T>);
switchMap(async (update) => {
if (update.updateType === "remove") {
return null;
}
return await getStoredValue(
this.storageKey,
this.chosenLocation,
this.keyDefinition.deserializer
);
}),
shareReplay({ bufferSize: 1, refCount: false })
);
@@ -37,24 +53,53 @@ export class DefaultGlobalState<T> implements GlobalState<T> {
this.stateSubject.next(value);
});
this.getFromState().then((s) => {
this.stateSubject.next(s);
});
return this.stateSubject.pipe(
tap({
complete: () => storageUpdateSubscription.unsubscribe(),
complete: () => {
storageUpdateSubscription.unsubscribe();
},
})
);
});
}).pipe(
shareReplay({ refCount: false, bufferSize: 1 }),
filter<T>((i) => i != FAKE_DEFAULT)
);
}
async update(configureState: (state: T) => T): Promise<T> {
await this.seededPromise;
const currentState = this.stateSubject.getValue();
const newState = configureState(currentState);
async update<TCombine>(
configureState: (state: T, dependency: TCombine) => T,
options: StateUpdateOptions<T, TCombine> = {}
): Promise<T> {
options = populateOptionsWithDefault(options);
const currentState = await this.getGuaranteedState();
const combinedDependencies =
options.combineLatestWith != null
? await firstValueFrom(options.combineLatestWith.pipe(timeout(options.msTimeout)))
: null;
if (!options.shouldUpdate(currentState, combinedDependencies)) {
return;
}
const newState = configureState(currentState, combinedDependencies);
await this.chosenLocation.save(this.storageKey, newState);
return newState;
}
private async getGuaranteedState() {
const currentValue = this.stateSubject.getValue();
return currentValue === FAKE_DEFAULT ? await this.getFromState() : currentValue;
}
async getFromState(): Promise<T> {
const data = await this.chosenLocation.get<Jsonify<T>>(this.storageKey);
return this.keyDefinition.deserializer(data);
return await getStoredValue(
this.storageKey,
this.chosenLocation,
this.keyDefinition.deserializer
);
}
}

View File

@@ -1,8 +1,12 @@
/**
* need to update test environment so trackEmissions works appropriately
* @jest-environment ../shared/test.environment.ts
*/
import { any, mock } from "jest-mock-extended";
import { BehaviorSubject, firstValueFrom, timeout } from "rxjs";
import { BehaviorSubject, firstValueFrom, of, timeout } from "rxjs";
import { Jsonify } from "type-fest";
import { trackEmissions } from "../../../../spec";
import { awaitAsync, trackEmissions } from "../../../../spec";
import { FakeStorageService } from "../../../../spec/fake-storage.service";
import { AccountInfo, AccountService } from "../../../auth/abstractions/account.service";
import { AuthenticationStatus } from "../../../auth/enums/authentication-status";
@@ -29,11 +33,9 @@ class TestState {
const testStateDefinition = new StateDefinition("fake", "disk");
const testKeyDefinition = new KeyDefinition<TestState>(
testStateDefinition,
"fake",
TestState.fromJSON
);
const testKeyDefinition = new KeyDefinition<TestState>(testStateDefinition, "fake", {
deserializer: TestState.fromJSON,
});
describe("DefaultUserState", () => {
const accountService = mock<AccountService>();
@@ -62,7 +64,7 @@ describe("DefaultUserState", () => {
name: `Test User ${id}`,
status: AuthenticationStatus.Unlocked,
});
await new Promise((resolve) => setTimeout(resolve, 1));
await awaitAsync();
};
afterEach(() => {
@@ -70,51 +72,42 @@ describe("DefaultUserState", () => {
});
it("emits updates for each user switch and update", async () => {
diskStorageService.internalUpdateStore({
"user_00000000-0000-1000-a000-000000000001_fake_fake": {
date: "2022-09-21T13:14:17.648Z",
array: ["value1", "value2"],
} as Jsonify<TestState>,
"user_00000000-0000-1000-a000-000000000002_fake_fake": {
date: "2021-09-21T13:14:17.648Z",
array: ["user2_value"],
},
});
const user1 = "user_00000000-0000-1000-a000-000000000001_fake_fake";
const user2 = "user_00000000-0000-1000-a000-000000000002_fake_fake";
const state1 = {
date: new Date(2021, 0),
array: ["value1"],
};
const state2 = {
date: new Date(2022, 0),
array: ["value2"],
};
const initialState: Record<string, TestState> = {};
initialState[user1] = state1;
initialState[user2] = state2;
diskStorageService.internalUpdateStore(initialState);
const emissions = trackEmissions(userState.state$);
// User signs in
changeActiveUser("1");
await new Promise<void>((resolve) => setTimeout(resolve, 1));
await awaitAsync();
// Service does an update
await userState.update((state) => {
state.array.push("value3");
state.date = new Date(2023, 0);
return state;
});
await new Promise<void>((resolve) => setTimeout(resolve, 1));
const updatedState = {
date: new Date(2023, 0),
array: ["value3"],
};
await userState.update(() => updatedState);
await awaitAsync();
// Emulate an account switch
await changeActiveUser("2");
expect(emissions).toHaveLength(3);
// Gotten starter user data
expect(emissions[0]).toBeTruthy();
expect(emissions[0].array).toHaveLength(2);
expect(emissions).toEqual([state1, updatedState, state2]);
// Gotten emission for the update call
expect(emissions[1]).toBeTruthy();
expect(emissions[1].array).toHaveLength(3);
expect(new Date(emissions[1].date).getUTCFullYear()).toBe(2023);
// The second users data
expect(emissions[2]).toBeTruthy();
expect(emissions[2].array).toHaveLength(1);
expect(new Date(emissions[2].date).getUTCFullYear()).toBe(2021);
// Should only be called twice to get state, once for each user
expect(diskStorageService.mock.get).toHaveBeenCalledTimes(2);
// Should be called three time to get state, once for each user and once for the update
expect(diskStorageService.mock.get).toHaveBeenCalledTimes(3);
expect(diskStorageService.mock.get).toHaveBeenNthCalledWith(
1,
"user_00000000-0000-1000-a000-000000000001_fake_fake",
@@ -122,6 +115,11 @@ describe("DefaultUserState", () => {
);
expect(diskStorageService.mock.get).toHaveBeenNthCalledWith(
2,
"user_00000000-0000-1000-a000-000000000001_fake_fake",
any()
);
expect(diskStorageService.mock.get).toHaveBeenNthCalledWith(
3,
"user_00000000-0000-1000-a000-000000000002_fake_fake",
any()
);
@@ -161,9 +159,9 @@ describe("DefaultUserState", () => {
diskStorageService.internalUpdateStore({
"user_00000000-0000-1000-a000-000000000001_fake_fake": {
date: "2020-09-21T13:14:17.648Z",
date: new Date(2020, 0),
array: ["testValue"],
} as Jsonify<TestState>,
} as TestState,
});
const promise = firstValueFrom(userState.state$.pipe(timeout(20)))
@@ -233,4 +231,102 @@ describe("DefaultUserState", () => {
// this value is correct.
expect(emissions).toHaveLength(2);
});
describe("update", () => {
const newData = { date: new Date(), array: ["test"] };
beforeEach(async () => {
changeActiveUser("1");
});
it("should save on update", async () => {
const result = await userState.update((state, dependencies) => {
return newData;
});
expect(diskStorageService.mock.save).toHaveBeenCalledTimes(1);
expect(result).toEqual(newData);
});
it("should emit once per update", async () => {
const emissions = trackEmissions(userState.state$);
await awaitAsync(); // Need to await for the initial value to be emitted
await userState.update((state, dependencies) => {
return newData;
});
await awaitAsync();
expect(emissions).toEqual([
null, // initial value
newData,
]);
});
it("should provide combined dependencies", async () => {
const emissions = trackEmissions(userState.state$);
await awaitAsync(); // Need to await for the initial value to be emitted
const combinedDependencies = { date: new Date() };
await userState.update(
(state, dependencies) => {
expect(dependencies).toEqual(combinedDependencies);
return newData;
},
{
combineLatestWith: of(combinedDependencies),
}
);
await awaitAsync();
expect(emissions).toEqual([
null, // initial value
newData,
]);
});
it("should not update if shouldUpdate returns false", async () => {
const emissions = trackEmissions(userState.state$);
await awaitAsync(); // Need to await for the initial value to be emitted
const result = await userState.update(
(state, dependencies) => {
return newData;
},
{
shouldUpdate: () => false,
}
);
await awaitAsync();
expect(diskStorageService.mock.save).not.toHaveBeenCalled();
expect(result).toBe(undefined);
expect(emissions).toEqual([null]);
});
it("should provide the current state to the update callback", async () => {
const emissions = trackEmissions(userState.state$);
await awaitAsync(); // Need to await for the initial value to be emitted
// Seed with interesting data
const initialData = { date: new Date(2020, 0), array: ["value1", "value2"] };
await userState.update((state, dependencies) => {
return initialData;
});
await userState.update((state, dependencies) => {
expect(state).toEqual(initialData);
return newData;
});
await awaitAsync();
expect(emissions).toEqual([
null, // Initial value
initialData,
newData,
]);
});
});
});

View File

@@ -9,8 +9,8 @@ import {
firstValueFrom,
combineLatestWith,
filter,
timeout,
} from "rxjs";
import { Jsonify } from "type-fest";
import { AccountService } from "../../../auth/abstractions/account.service";
import { UserId } from "../../../types/guid";
@@ -18,9 +18,11 @@ import { EncryptService } from "../../abstractions/encrypt.service";
import { AbstractStorageService } from "../../abstractions/storage.service";
import { DerivedUserState } from "../derived-user-state";
import { KeyDefinition, userKeyBuilder } from "../key-definition";
import { StateUpdateOptions, populateOptionsWithDefault } from "../state-update-options";
import { Converter, UserState } from "../user-state";
import { DefaultDerivedUserState } from "./default-derived-state";
import { getStoredValue } from "./util";
const FAKE_DEFAULT = Symbol("fakeDefault");
@@ -54,9 +56,11 @@ export class DefaultUserState<T> implements UserState<T> {
if (key == null) {
return FAKE_DEFAULT;
}
const jsonData = await this.chosenStorageLocation.get<Jsonify<T>>(key);
const data = keyDefinition.deserializer(jsonData);
return data;
return await getStoredValue(
key,
this.chosenStorageLocation,
this.keyDefinition.deserializer
);
}),
// Share the execution
shareReplay({ refCount: false, bufferSize: 1 })
@@ -65,8 +69,16 @@ export class DefaultUserState<T> implements UserState<T> {
const storageUpdates$ = this.chosenStorageLocation.updates$.pipe(
combineLatestWith(this.formattedKey$),
filter(([update, key]) => key !== null && update.key === key),
map(([update]) => {
return keyDefinition.deserializer(update.value as Jsonify<T>);
switchMap(async ([update, key]) => {
if (update.updateType === "remove") {
return null;
}
const data = await getStoredValue(
key,
this.chosenStorageLocation,
this.keyDefinition.deserializer
);
return data;
})
);
@@ -94,23 +106,53 @@ export class DefaultUserState<T> implements UserState<T> {
.pipe(filter<T>((value) => value != FAKE_DEFAULT));
}
async update(configureState: (state: T) => T): Promise<T> {
async update<TCombine>(
configureState: (state: T, dependency: TCombine) => T,
options: StateUpdateOptions<T, TCombine> = {}
): Promise<T> {
options = populateOptionsWithDefault(options);
const key = await this.createKey();
const currentState = await this.getGuaranteedState(key);
const newState = configureState(currentState);
const combinedDependencies =
options.combineLatestWith != null
? await firstValueFrom(options.combineLatestWith.pipe(timeout(options.msTimeout)))
: null;
if (!options.shouldUpdate(currentState, combinedDependencies)) {
return;
}
const newState = configureState(currentState, combinedDependencies);
await this.saveToStorage(key, newState);
return newState;
}
async updateFor(userId: UserId, configureState: (state: T) => T): Promise<T> {
async updateFor<TCombine>(
userId: UserId,
configureState: (state: T, dependencies: TCombine) => T,
options: StateUpdateOptions<T, TCombine> = {}
): Promise<T> {
if (userId == null) {
throw new Error("Attempting to update user state, but no userId has been supplied.");
}
options = populateOptionsWithDefault(options);
const key = userKeyBuilder(userId, this.keyDefinition);
const currentStore = await this.chosenStorageLocation.get<Jsonify<T>>(key);
const currentState = this.keyDefinition.deserializer(currentStore);
const newState = configureState(currentState);
const currentState = await getStoredValue(
key,
this.chosenStorageLocation,
this.keyDefinition.deserializer
);
const combinedDependencies =
options.combineLatestWith != null
? await firstValueFrom(options.combineLatestWith.pipe(timeout(options.msTimeout)))
: null;
if (!options.shouldUpdate(currentState, combinedDependencies)) {
return;
}
const newState = configureState(currentState, combinedDependencies);
await this.saveToStorage(key, newState);
return newState;
@@ -118,8 +160,7 @@ export class DefaultUserState<T> implements UserState<T> {
async getFromState(): Promise<T> {
const key = await this.createKey();
const data = await this.chosenStorageLocation.get<Jsonify<T>>(key);
return this.keyDefinition.deserializer(data);
return await getStoredValue(key, this.chosenStorageLocation, this.keyDefinition.deserializer);
}
createDerived<TTo>(converter: Converter<T, TTo>): DerivedUserState<TTo> {
@@ -140,10 +181,13 @@ export class DefaultUserState<T> implements UserState<T> {
}
private async seedInitial(key: string): Promise<T> {
const data = await this.chosenStorageLocation.get<Jsonify<T>>(key);
const serializedData = this.keyDefinition.deserializer(data);
this.stateSubject.next(serializedData);
return serializedData;
const value = await getStoredValue(
key,
this.chosenStorageLocation,
this.keyDefinition.deserializer
);
this.stateSubject.next(value);
return value;
}
protected saveToStorage(key: string, data: T): Promise<void> {

View File

@@ -0,0 +1,50 @@
import { FakeStorageService } from "../../../../spec/fake-storage.service";
import { getStoredValue } from "./util";
describe("getStoredValue", () => {
const key = "key";
const deserializedValue = { value: 1 };
const value = JSON.stringify(deserializedValue);
const deserializer = (v: string) => JSON.parse(v);
let storageService: FakeStorageService;
beforeEach(() => {
storageService = new FakeStorageService();
});
describe("when the storage service requires deserialization", () => {
beforeEach(() => {
storageService.internalUpdateValuesRequireDeserialization(true);
});
it("should deserialize", async () => {
storageService.save(key, value);
const result = await getStoredValue(key, storageService, deserializer);
expect(result).toEqual(deserializedValue);
});
});
describe("when the storage service does not require deserialization", () => {
beforeEach(() => {
storageService.internalUpdateValuesRequireDeserialization(false);
});
it("should not deserialize", async () => {
storageService.save(key, value);
const result = await getStoredValue(key, storageService, deserializer);
expect(result).toEqual(value);
});
it("should convert undefined to null", async () => {
storageService.save(key, undefined);
const result = await getStoredValue(key, storageService, deserializer);
expect(result).toEqual(null);
});
});
});

View File

@@ -0,0 +1,18 @@
import { Jsonify } from "type-fest";
import { AbstractStorageService } from "../../abstractions/storage.service";
export async function getStoredValue<T>(
key: string,
storage: AbstractStorageService,
deserializer: (jsonValue: Jsonify<T>) => T
) {
if (storage.valuesRequireDeserialization) {
const jsonValue = await storage.get<Jsonify<T>>(key);
const value = deserializer(jsonValue);
return value;
} else {
const value = await storage.get<T>(key);
return value ?? null;
}
}