mirror of
https://github.com/bitwarden/browser.git
synced 2024-09-18 02:41:15 +02:00
[PM-8285] Resolve app id race (#9501)
* Do not update appId if it is not null * Prefer linear transformations to side-effect-based changes This leaves us open to repeat emits due to updates, but distinct until changed stops those. Tracker improvements are due to passed in observables with replay causing immediate emits when `expectingEmission`s. This converts to a cold observable that only emits when the tracked observable does _after_ subscribing. * Prefer while * PR review
This commit is contained in:
parent
90ca4345b3
commit
3154d21925
@ -1,10 +1,11 @@
|
|||||||
import { Observable, Subscription, firstValueFrom, throwError, timeout } from "rxjs";
|
import { Observable, Subject, Subscription, firstValueFrom, throwError, timeout } from "rxjs";
|
||||||
|
|
||||||
/** Test class to enable async awaiting of observable emissions */
|
/** Test class to enable async awaiting of observable emissions */
|
||||||
export class ObservableTracker<T> {
|
export class ObservableTracker<T> {
|
||||||
private subscription: Subscription;
|
private subscription: Subscription;
|
||||||
|
private emissionReceived = new Subject<T>();
|
||||||
emissions: T[] = [];
|
emissions: T[] = [];
|
||||||
constructor(private observable: Observable<T>) {
|
constructor(observable: Observable<T>) {
|
||||||
this.emissions = this.trackEmissions(observable);
|
this.emissions = this.trackEmissions(observable);
|
||||||
}
|
}
|
||||||
|
|
||||||
@ -21,7 +22,7 @@ export class ObservableTracker<T> {
|
|||||||
*/
|
*/
|
||||||
async expectEmission(msTimeout = 50): Promise<T> {
|
async expectEmission(msTimeout = 50): Promise<T> {
|
||||||
return await firstValueFrom(
|
return await firstValueFrom(
|
||||||
this.observable.pipe(
|
this.emissionReceived.pipe(
|
||||||
timeout({
|
timeout({
|
||||||
first: msTimeout,
|
first: msTimeout,
|
||||||
with: () => throwError(() => new Error("Timeout exceeded waiting for another emission.")),
|
with: () => throwError(() => new Error("Timeout exceeded waiting for another emission.")),
|
||||||
@ -34,40 +35,38 @@ export class ObservableTracker<T> {
|
|||||||
* @param count The number of emissions to wait for
|
* @param count The number of emissions to wait for
|
||||||
*/
|
*/
|
||||||
async pauseUntilReceived(count: number, msTimeout = 50): Promise<T[]> {
|
async pauseUntilReceived(count: number, msTimeout = 50): Promise<T[]> {
|
||||||
for (let i = 0; i < count - this.emissions.length; i++) {
|
while (this.emissions.length < count) {
|
||||||
await this.expectEmission(msTimeout);
|
await this.expectEmission(msTimeout);
|
||||||
}
|
}
|
||||||
return this.emissions;
|
return this.emissions;
|
||||||
}
|
}
|
||||||
|
|
||||||
private trackEmissions<T>(observable: Observable<T>): T[] {
|
private trackEmissions(observable: Observable<T>): T[] {
|
||||||
const emissions: T[] = [];
|
const emissions: T[] = [];
|
||||||
this.subscription = observable.subscribe((value) => {
|
this.subscription = observable.subscribe((value) => {
|
||||||
switch (value) {
|
if (value == null) {
|
||||||
case undefined:
|
this.emissionReceived.next(null);
|
||||||
case null:
|
|
||||||
emissions.push(value);
|
|
||||||
return;
|
return;
|
||||||
default:
|
|
||||||
// process by type
|
|
||||||
break;
|
|
||||||
}
|
}
|
||||||
|
|
||||||
switch (typeof value) {
|
switch (typeof value) {
|
||||||
case "string":
|
case "string":
|
||||||
case "number":
|
case "number":
|
||||||
case "boolean":
|
case "boolean":
|
||||||
emissions.push(value);
|
this.emissionReceived.next(value);
|
||||||
break;
|
break;
|
||||||
case "symbol":
|
case "symbol":
|
||||||
// Cheating types to make symbols work at all
|
// Cheating types to make symbols work at all
|
||||||
emissions.push(value.toString() as T);
|
this.emissionReceived.next(value as T);
|
||||||
break;
|
break;
|
||||||
default: {
|
default: {
|
||||||
emissions.push(clone(value));
|
this.emissionReceived.next(clone(value));
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
this.emissionReceived.subscribe((value) => {
|
||||||
|
emissions.push(value);
|
||||||
|
});
|
||||||
return emissions;
|
return emissions;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
@ -1,20 +1,23 @@
|
|||||||
import { FakeGlobalStateProvider } from "../../../spec";
|
import { FakeGlobalState, FakeGlobalStateProvider, ObservableTracker } from "../../../spec";
|
||||||
import { Utils } from "../misc/utils";
|
import { Utils } from "../misc/utils";
|
||||||
|
|
||||||
import { ANONYMOUS_APP_ID_KEY, APP_ID_KEY, AppIdService } from "./app-id.service";
|
import { ANONYMOUS_APP_ID_KEY, APP_ID_KEY, AppIdService } from "./app-id.service";
|
||||||
|
|
||||||
describe("AppIdService", () => {
|
describe("AppIdService", () => {
|
||||||
const globalStateProvider = new FakeGlobalStateProvider();
|
let globalStateProvider: FakeGlobalStateProvider;
|
||||||
const appIdState = globalStateProvider.getFake(APP_ID_KEY);
|
let appIdState: FakeGlobalState<string>;
|
||||||
const anonymousAppIdState = globalStateProvider.getFake(ANONYMOUS_APP_ID_KEY);
|
let anonymousAppIdState: FakeGlobalState<string>;
|
||||||
let sut: AppIdService;
|
let sut: AppIdService;
|
||||||
|
|
||||||
beforeEach(() => {
|
beforeEach(() => {
|
||||||
|
globalStateProvider = new FakeGlobalStateProvider();
|
||||||
|
appIdState = globalStateProvider.getFake(APP_ID_KEY);
|
||||||
|
anonymousAppIdState = globalStateProvider.getFake(ANONYMOUS_APP_ID_KEY);
|
||||||
sut = new AppIdService(globalStateProvider);
|
sut = new AppIdService(globalStateProvider);
|
||||||
});
|
});
|
||||||
|
|
||||||
afterEach(() => {
|
afterEach(() => {
|
||||||
jest.restoreAllMocks();
|
jest.resetAllMocks();
|
||||||
});
|
});
|
||||||
|
|
||||||
describe("getAppId", () => {
|
describe("getAppId", () => {
|
||||||
@ -26,19 +29,18 @@ describe("AppIdService", () => {
|
|||||||
expect(appId).toBe("existingAppId");
|
expect(appId).toBe("existingAppId");
|
||||||
});
|
});
|
||||||
|
|
||||||
it.each([null, undefined])(
|
it("creates a new appId only once", async () => {
|
||||||
"uses the util function to create a new id when it AppId does not exist",
|
appIdState.stateSubject.next(null);
|
||||||
async (value) => {
|
|
||||||
appIdState.stateSubject.next(value);
|
|
||||||
const spy = jest.spyOn(Utils, "newGuid");
|
|
||||||
|
|
||||||
await sut.getAppId();
|
const appIds: string[] = [];
|
||||||
|
const promises = [async () => appIds.push(await sut.getAppId())];
|
||||||
|
promises.push(async () => appIds.push(await sut.getAppId()));
|
||||||
|
await Promise.all(promises);
|
||||||
|
|
||||||
expect(spy).toHaveBeenCalledTimes(1);
|
expect(appIds[0]).toBe(appIds[1]);
|
||||||
},
|
});
|
||||||
);
|
|
||||||
|
|
||||||
it.each([null, undefined])("returns a new appId when it does not exist", async (value) => {
|
it.each([null, undefined])("returns a new appId when %s", async (value) => {
|
||||||
appIdState.stateSubject.next(value);
|
appIdState.stateSubject.next(value);
|
||||||
|
|
||||||
const appId = await sut.getAppId();
|
const appId = await sut.getAppId();
|
||||||
@ -46,16 +48,23 @@ describe("AppIdService", () => {
|
|||||||
expect(appId).toMatch(Utils.guidRegex);
|
expect(appId).toMatch(Utils.guidRegex);
|
||||||
});
|
});
|
||||||
|
|
||||||
it.each([null, undefined])(
|
it.each([null, undefined])("stores the new guid when %s", async (value) => {
|
||||||
"stores the new guid when it an existing one is not found",
|
|
||||||
async (value) => {
|
|
||||||
appIdState.stateSubject.next(value);
|
appIdState.stateSubject.next(value);
|
||||||
|
|
||||||
const appId = await sut.getAppId();
|
const appId = await sut.getAppId();
|
||||||
|
|
||||||
expect(appIdState.nextMock).toHaveBeenCalledWith(appId);
|
expect(appIdState.nextMock).toHaveBeenCalledWith(appId);
|
||||||
},
|
});
|
||||||
);
|
|
||||||
|
it("emits only once when creating a new appId", async () => {
|
||||||
|
appIdState.stateSubject.next(null);
|
||||||
|
|
||||||
|
const tracker = new ObservableTracker(sut.appId$);
|
||||||
|
const appId = await sut.getAppId();
|
||||||
|
|
||||||
|
expect(tracker.emissions).toEqual([appId]);
|
||||||
|
await expect(tracker.pauseUntilReceived(2, 50)).rejects.toThrow("Timeout exceeded");
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
describe("getAnonymousAppId", () => {
|
describe("getAnonymousAppId", () => {
|
||||||
@ -67,17 +76,16 @@ describe("AppIdService", () => {
|
|||||||
expect(appId).toBe("existingAppId");
|
expect(appId).toBe("existingAppId");
|
||||||
});
|
});
|
||||||
|
|
||||||
it.each([null, undefined])(
|
it("creates a new anonymousAppId only once", async () => {
|
||||||
"uses the util function to create a new id when it AppId does not exist",
|
anonymousAppIdState.stateSubject.next(null);
|
||||||
async (value) => {
|
|
||||||
anonymousAppIdState.stateSubject.next(value);
|
|
||||||
const spy = jest.spyOn(Utils, "newGuid");
|
|
||||||
|
|
||||||
await sut.getAnonymousAppId();
|
const appIds: string[] = [];
|
||||||
|
const promises = [async () => appIds.push(await sut.getAnonymousAppId())];
|
||||||
|
promises.push(async () => appIds.push(await sut.getAnonymousAppId()));
|
||||||
|
await Promise.all(promises);
|
||||||
|
|
||||||
expect(spy).toHaveBeenCalledTimes(1);
|
expect(appIds[0]).toBe(appIds[1]);
|
||||||
},
|
});
|
||||||
);
|
|
||||||
|
|
||||||
it.each([null, undefined])("returns a new appId when it does not exist", async (value) => {
|
it.each([null, undefined])("returns a new appId when it does not exist", async (value) => {
|
||||||
anonymousAppIdState.stateSubject.next(value);
|
anonymousAppIdState.stateSubject.next(value);
|
||||||
@ -97,5 +105,15 @@ describe("AppIdService", () => {
|
|||||||
expect(anonymousAppIdState.nextMock).toHaveBeenCalledWith(appId);
|
expect(anonymousAppIdState.nextMock).toHaveBeenCalledWith(appId);
|
||||||
},
|
},
|
||||||
);
|
);
|
||||||
|
|
||||||
|
it("emits only once when creating a new anonymousAppId", async () => {
|
||||||
|
anonymousAppIdState.stateSubject.next(null);
|
||||||
|
|
||||||
|
const tracker = new ObservableTracker(sut.anonymousAppId$);
|
||||||
|
const appId = await sut.getAnonymousAppId();
|
||||||
|
|
||||||
|
expect(tracker.emissions).toEqual([appId]);
|
||||||
|
await expect(tracker.pauseUntilReceived(2, 50)).rejects.toThrow("Timeout exceeded");
|
||||||
|
});
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
@ -1,4 +1,4 @@
|
|||||||
import { Observable, filter, firstValueFrom, tap } from "rxjs";
|
import { Observable, concatMap, distinctUntilChanged, firstValueFrom, share } from "rxjs";
|
||||||
|
|
||||||
import { AppIdService as AppIdServiceAbstraction } from "../abstractions/app-id.service";
|
import { AppIdService as AppIdServiceAbstraction } from "../abstractions/app-id.service";
|
||||||
import { Utils } from "../misc/utils";
|
import { Utils } from "../misc/utils";
|
||||||
@ -19,20 +19,28 @@ export class AppIdService implements AppIdServiceAbstraction {
|
|||||||
const appIdState = globalStateProvider.get(APP_ID_KEY);
|
const appIdState = globalStateProvider.get(APP_ID_KEY);
|
||||||
const anonymousAppIdState = globalStateProvider.get(ANONYMOUS_APP_ID_KEY);
|
const anonymousAppIdState = globalStateProvider.get(ANONYMOUS_APP_ID_KEY);
|
||||||
this.appId$ = appIdState.state$.pipe(
|
this.appId$ = appIdState.state$.pipe(
|
||||||
tap(async (appId) => {
|
concatMap(async (appId) => {
|
||||||
if (!appId) {
|
if (!appId) {
|
||||||
await appIdState.update(() => Utils.newGuid());
|
return await appIdState.update(() => Utils.newGuid(), {
|
||||||
|
shouldUpdate: (v) => v == null,
|
||||||
|
});
|
||||||
}
|
}
|
||||||
|
return appId;
|
||||||
}),
|
}),
|
||||||
filter((appId) => !!appId),
|
distinctUntilChanged(),
|
||||||
|
share(),
|
||||||
);
|
);
|
||||||
this.anonymousAppId$ = anonymousAppIdState.state$.pipe(
|
this.anonymousAppId$ = anonymousAppIdState.state$.pipe(
|
||||||
tap(async (appId) => {
|
concatMap(async (appId) => {
|
||||||
if (!appId) {
|
if (!appId) {
|
||||||
await anonymousAppIdState.update(() => Utils.newGuid());
|
return await anonymousAppIdState.update(() => Utils.newGuid(), {
|
||||||
|
shouldUpdate: (v) => v == null,
|
||||||
|
});
|
||||||
}
|
}
|
||||||
|
return appId;
|
||||||
}),
|
}),
|
||||||
filter((appId) => !!appId),
|
distinctUntilChanged(),
|
||||||
|
share(),
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
Loading…
Reference in New Issue
Block a user