From 430e10c221c7b22538f70109add6720d2a09f58b Mon Sep 17 00:00:00 2001 From: Matt Gibson Date: Wed, 16 Nov 2022 18:54:54 -0500 Subject: [PATCH] Allow Manifest V2 usage of session sync Expands use of SessionSyncer to all Subject types. Correctly handles replay buffer for each type to ignore the flood of data upon subscription to each Subject type. --- .../browser-session.decorator.spec.ts | 12 ++-- .../browser-session.decorator.ts | 8 ++- .../session-sync.decorator.ts | 6 +- .../session-syncer.spec.ts | 62 +++++++++++++------ .../session-sync-observable/session-syncer.ts | 42 +++++++++---- .../sync-item-metadata.ts | 13 ++++ .../synced-item-metadata.spec.ts | 12 ++++ .../services/abstractions/state.service.ts | 1 + libs/angular/test-utils.ts | 3 + 9 files changed, 116 insertions(+), 43 deletions(-) create mode 100644 libs/angular/test-utils.ts diff --git a/apps/browser/src/decorators/session-sync-observable/browser-session.decorator.spec.ts b/apps/browser/src/decorators/session-sync-observable/browser-session.decorator.spec.ts index cc8a5618760..92c5dfb0170 100644 --- a/apps/browser/src/decorators/session-sync-observable/browser-session.decorator.spec.ts +++ b/apps/browser/src/decorators/session-sync-observable/browser-session.decorator.spec.ts @@ -1,6 +1,6 @@ import { BehaviorSubject } from "rxjs"; -import { StateService } from "../../services/state.service"; +import { BrowserStateService } from "../../services/browser-state.service"; import { browserSession } from "./browser-session.decorator"; import { SessionStorable } from "./session-storable"; @@ -22,25 +22,25 @@ describe("browserSession decorator", () => { }); it("should create if StateService is a constructor argument", () => { - const stateService = Object.create(StateService.prototype, {}); + const stateService = Object.create(BrowserStateService.prototype, {}); @browserSession class TestClass { - constructor(private stateService: StateService) {} + constructor(private stateService: BrowserStateService) {} } expect(new TestClass(stateService)).toBeDefined(); }); describe("interaction with @sessionSync decorator", () => { - let stateService: StateService; + let stateService: BrowserStateService; @browserSession class TestClass { @sessionSync({ initializer: (s: string) => s }) private behaviorSubject = new BehaviorSubject(""); - constructor(private stateService: StateService) {} + constructor(private stateService: BrowserStateService) {} fromJSON(json: any) { this.behaviorSubject.next(json); @@ -48,7 +48,7 @@ describe("browserSession decorator", () => { } beforeEach(() => { - stateService = Object.create(StateService.prototype, {}) as StateService; + stateService = Object.create(BrowserStateService.prototype, {}) as BrowserStateService; }); it("should create a session syncer", () => { diff --git a/apps/browser/src/decorators/session-sync-observable/browser-session.decorator.ts b/apps/browser/src/decorators/session-sync-observable/browser-session.decorator.ts index 73cdf767357..351343910f1 100644 --- a/apps/browser/src/decorators/session-sync-observable/browser-session.decorator.ts +++ b/apps/browser/src/decorators/session-sync-observable/browser-session.decorator.ts @@ -1,6 +1,6 @@ import { Constructor } from "type-fest"; -import { StateService } from "../../services/state.service"; +import { BrowserStateService } from "../../services/browser-state.service"; import { SessionStorable } from "./session-storable"; import { SessionSyncer } from "./session-syncer"; @@ -22,7 +22,9 @@ export function browserSession>(constructor: TCto super(...args); // Require state service to be injected - const stateService = args.find((arg) => arg instanceof StateService); + const stateService: BrowserStateService = [this as any] + .concat(args) + .find((arg) => typeof arg.setInSessionMemory === "function"); if (!stateService) { throw new Error( `Cannot decorate ${constructor.name} with browserSession, Browser's StateService must be injected` @@ -38,7 +40,7 @@ export function browserSession>(constructor: TCto ); } - buildSyncer(metadata: SyncedItemMetadata, stateService: StateService) { + buildSyncer(metadata: SyncedItemMetadata, stateService: BrowserStateService) { const syncer = new SessionSyncer((this as any)[metadata.propertyKey], stateService, metadata); syncer.init(); return syncer; diff --git a/apps/browser/src/decorators/session-sync-observable/session-sync.decorator.ts b/apps/browser/src/decorators/session-sync-observable/session-sync.decorator.ts index df0764528f7..243fbcc02c7 100644 --- a/apps/browser/src/decorators/session-sync-observable/session-sync.decorator.ts +++ b/apps/browser/src/decorators/session-sync-observable/session-sync.decorator.ts @@ -2,10 +2,11 @@ import { Jsonify } from "type-fest"; import { SessionStorable } from "./session-storable"; -class BuildOptions { +class BuildOptions> { ctor?: new () => T; - initializer?: (keyValuePair: Jsonify) => T; + initializer?: (keyValuePair: TJson) => T; initializeAsArray? = false; + initializeAsRecord? = false; } /** @@ -47,6 +48,7 @@ export function sessionSync(buildOptions: BuildOptions) { ctor: buildOptions.ctor, initializer: buildOptions.initializer, initializeAsArray: buildOptions.initializeAsArray, + initializeAsRecord: buildOptions.initializeAsRecord, }); }; } diff --git a/apps/browser/src/decorators/session-sync-observable/session-syncer.spec.ts b/apps/browser/src/decorators/session-sync-observable/session-syncer.spec.ts index 5286cece1bb..6d8e5c2c495 100644 --- a/apps/browser/src/decorators/session-sync-observable/session-syncer.spec.ts +++ b/apps/browser/src/decorators/session-sync-observable/session-syncer.spec.ts @@ -1,5 +1,6 @@ +import { awaitAsync as flushAsyncObservables } from "@bitwarden/angular/../test-utils"; import { mock, MockProxy } from "jest-mock-extended"; -import { BehaviorSubject } from "rxjs"; +import { BehaviorSubject, ReplaySubject } from "rxjs"; import { BrowserApi } from "../../browser/browserApi"; import { StateService } from "../../services/abstractions/state.service"; @@ -34,10 +35,10 @@ describe("session syncer", () => { }); describe("constructor", () => { - it("should throw if behaviorSubject is not an instance of BehaviorSubject", () => { + it("should throw if subject is not an instance of Subject", () => { expect(() => { new SessionSyncer({} as any, stateService, null); - }).toThrowError("behaviorSubject must be an instance of BehaviorSubject"); + }).toThrowError("subject must inherit from Subject"); }); it("should create if either ctor or initializer is provided", () => { @@ -59,28 +60,50 @@ describe("session syncer", () => { }); }); - describe("manifest v2 init", () => { - let observeSpy: jest.SpyInstance; - let listenForUpdatesSpy: jest.SpyInstance; - - beforeEach(() => { - observeSpy = jest.spyOn(behaviorSubject, "subscribe").mockReturnThis(); - listenForUpdatesSpy = jest.spyOn(BrowserApi, "messageListener").mockReturnValue(); - jest.spyOn(chrome.runtime, "getManifest").mockReturnValue({ - name: "bitwarden-test", - version: "0.0.0", - manifest_version: 2, - }); + describe("init", () => { + it("should ignore all updates currently in a ReplaySubject's buffer", () => { + const replaySubject = new ReplaySubject(Infinity); + replaySubject.next("1"); + replaySubject.next("2"); + replaySubject.next("3"); + sut = new SessionSyncer(replaySubject, stateService, metaData); + // block observing the subject + jest.spyOn(sut as any, "observe").mockImplementation(); sut.init(); + + expect(sut["ignoreNUpdates"]).toBe(3); }); - it("should not start observing", () => { - expect(observeSpy).not.toHaveBeenCalled(); + it("should ignore BehaviorSubject's initial value", () => { + const behaviorSubject = new BehaviorSubject("initial"); + sut = new SessionSyncer(behaviorSubject, stateService, metaData); + // block observing the subject + jest.spyOn(sut as any, "observe").mockImplementation(); + + sut.init(); + + expect(sut["ignoreNUpdates"]).toBe(1); }); - it("should not start listening", () => { - expect(listenForUpdatesSpy).not.toHaveBeenCalled(); + it("should grab an initial value from storage if it exists", () => { + stateService.hasInSessionMemory.mockResolvedValue(true); + //Block a call to update + const updateSpy = jest.spyOn(sut as any, "update").mockImplementation(); + + sut.init(); + + expect(updateSpy).toHaveBeenCalledWith(); + }); + + it("should not grab an initial value from storage if it does not exist", () => { + stateService.hasInSessionMemory.mockResolvedValue(false); + //Block a call to update + const updateSpy = jest.spyOn(sut as any, "update").mockImplementation(); + + sut.init(); + + expect(updateSpy).toHaveBeenCalledWith(); }); }); @@ -146,6 +169,7 @@ describe("session syncer", () => { stateService.getFromSessionMemory.mockResolvedValue("test"); await sut.updateFromMessage({ command: `${sessionKey}_update`, id: "different_id" }); + await flushAsyncObservables(); expect(stateService.getFromSessionMemory).toHaveBeenCalledTimes(1); expect(stateService.getFromSessionMemory).toHaveBeenCalledWith(sessionKey, builder); diff --git a/apps/browser/src/decorators/session-sync-observable/session-syncer.ts b/apps/browser/src/decorators/session-sync-observable/session-syncer.ts index 2acfed2954f..c379f0047b0 100644 --- a/apps/browser/src/decorators/session-sync-observable/session-syncer.ts +++ b/apps/browser/src/decorators/session-sync-observable/session-syncer.ts @@ -1,4 +1,4 @@ -import { BehaviorSubject, concatMap, Subscription } from "rxjs"; +import { BehaviorSubject, concatMap, ReplaySubject, Subject, Subscription } from "rxjs"; import { Utils } from "@bitwarden/common/misc/utils"; @@ -11,16 +11,16 @@ export class SessionSyncer { subscription: Subscription; id = Utils.newGuid(); - // everyone gets the same initial values - private ignoreNextUpdate = true; + // ignore initial values + private ignoreNUpdates = 0; constructor( - private behaviorSubject: BehaviorSubject, + private subject: Subject, private stateService: StateService, private metaData: SyncedItemMetadata ) { - if (!(behaviorSubject instanceof BehaviorSubject)) { - throw new Error("behaviorSubject must be an instance of BehaviorSubject"); + if (!(subject instanceof Subject)) { + throw new Error("subject must inherit from Subject"); } if (metaData.ctor == null && metaData.initializer == null) { @@ -29,11 +29,23 @@ export class SessionSyncer { } init() { - if (BrowserApi.manifestVersion !== 3) { - return; + switch (this.subject.constructor) { + case ReplaySubject: + // ignore all updates currently in the buffer + this.ignoreNUpdates = (this.subject as any)._buffer.length; + break; + case BehaviorSubject: + this.ignoreNUpdates = 1; + break; + default: + break; } this.observe(); + if (this.stateService.hasInSessionMemory(this.metaData.sessionKey)) { + this.update(); + } + this.listenForUpdates(); } @@ -41,11 +53,11 @@ export class SessionSyncer { // This may be a memory leak. // There is no good time to unsubscribe from this observable. Hopefully Manifest V3 clears memory from temporary // contexts. If so, this is handled by destruction of the context. - this.subscription = this.behaviorSubject + this.subscription = this.subject .pipe( concatMap(async (next) => { - if (this.ignoreNextUpdate) { - this.ignoreNextUpdate = false; + if (this.ignoreNUpdates > 0) { + this.ignoreNUpdates -= 1; return; } await this.updateSession(next); @@ -66,10 +78,14 @@ export class SessionSyncer { if (message.command != this.updateMessageCommand || message.id === this.id) { return; } + this.update(); + } + + async update() { const builder = SyncedItemMetadata.builder(this.metaData); const value = await this.stateService.getFromSessionMemory(this.metaData.sessionKey, builder); - this.ignoreNextUpdate = true; - this.behaviorSubject.next(value); + this.ignoreNUpdates = 1; + this.subject.next(value); } private async updateSession(value: any) { diff --git a/apps/browser/src/decorators/session-sync-observable/sync-item-metadata.ts b/apps/browser/src/decorators/session-sync-observable/sync-item-metadata.ts index 2b3f4715d46..db6b9c59c91 100644 --- a/apps/browser/src/decorators/session-sync-observable/sync-item-metadata.ts +++ b/apps/browser/src/decorators/session-sync-observable/sync-item-metadata.ts @@ -4,14 +4,27 @@ export class SyncedItemMetadata { ctor?: new () => any; initializer?: (keyValuePair: any) => any; initializeAsArray?: boolean; + initializeAsRecord?: boolean; static builder(metadata: SyncedItemMetadata): (o: any) => any { + if (metadata.initializeAsArray && metadata.initializeAsRecord) { + throw new Error("initializeAsArray and initializeAsRecord cannot both be true"); + } + const itemBuilder = metadata.initializer != null ? metadata.initializer : (o: any) => Object.assign(new metadata.ctor(), o); if (metadata.initializeAsArray) { return (keyValuePair: any) => keyValuePair.map((o: any) => itemBuilder(o)); + } else if (metadata.initializeAsRecord) { + return (keyValuePair: any) => { + const record: Record = {}; + for (const key in keyValuePair) { + record[key] = itemBuilder(keyValuePair[key]); + } + return record; + }; } else { return (keyValuePair: any) => itemBuilder(keyValuePair); } diff --git a/apps/browser/src/decorators/session-sync-observable/synced-item-metadata.spec.ts b/apps/browser/src/decorators/session-sync-observable/synced-item-metadata.spec.ts index 5cd869a5b67..58efe7307f5 100644 --- a/apps/browser/src/decorators/session-sync-observable/synced-item-metadata.spec.ts +++ b/apps/browser/src/decorators/session-sync-observable/synced-item-metadata.spec.ts @@ -36,4 +36,16 @@ describe("builder", () => { expect(builder([{}])).toBeInstanceOf(Array); expect(builder([{}])[0]).toBe("used initializer"); }); + + it("should honor initialize as record", () => { + const metadata = { + propertyKey, + sessionKey: key, + initializer: initializer, + initializeAsRecord: true, + }; + const builder = SyncedItemMetadata.builder(metadata); + expect(builder({ key: "" })).toBeInstanceOf(Object); + expect(builder({ key: "" })).toStrictEqual({ key: "used initializer" }); + }); }); diff --git a/apps/browser/src/services/abstractions/state.service.ts b/apps/browser/src/services/abstractions/state.service.ts index 294bc814e61..348732a4063 100644 --- a/apps/browser/src/services/abstractions/state.service.ts +++ b/apps/browser/src/services/abstractions/state.service.ts @@ -9,6 +9,7 @@ import { BrowserGroupingsComponentState } from "../../models/browserGroupingsCom import { BrowserSendComponentState } from "../../models/browserSendComponentState"; export abstract class StateService extends BaseStateServiceAbstraction { + abstract hasInSessionMemory(key: string): Promise; abstract getFromSessionMemory(key: string, deserializer?: (obj: Jsonify) => T): Promise; abstract setInSessionMemory(key: string, value: any): Promise; getBrowserGroupingComponentState: ( diff --git a/libs/angular/test-utils.ts b/libs/angular/test-utils.ts new file mode 100644 index 00000000000..a2422e698fd --- /dev/null +++ b/libs/angular/test-utils.ts @@ -0,0 +1,3 @@ +export async function awaitAsync(ms = 0) { + await new Promise((resolve) => setTimeout(resolve, ms)); +}