Make IdbManager only handle values of type InstallationEntry (#2245)

Author: mmermerkaya

packages/installations/src/functions/delete-installation.ts

@@ -19,21 +19,13 @@ import { FirebaseApp } from '@firebase/app-types';
 import { deleteInstallation as deleteInstallationRequest } from '../api/delete-installation';
 import { extractAppConfig } from '../helpers/extract-app-config';
 import { remove, update } from '../helpers/idb-manager';
-import {
-  InProgressInstallationEntry,
-  InstallationEntry,
-  RegisteredInstallationEntry,
-  RequestStatus
-} from '../interfaces/installation-entry';
+import { RequestStatus } from '../interfaces/installation-entry';
 import { ERROR_FACTORY, ErrorCode } from '../util/errors';
 
 export async function deleteInstallation(app: FirebaseApp): Promise<void> {
   const appConfig = extractAppConfig(app);
 
-  const entry = await update(appConfig, (oldEntry?: InstallationEntry):
-    | InProgressInstallationEntry
-    | RegisteredInstallationEntry
-    | undefined => {
+  const entry = await update(appConfig, oldEntry => {
     if (oldEntry && oldEntry.registrationStatus === RequestStatus.NOT_STARTED) {
       // Delete the unregistered entry without sending a deleteInstallation request.
       return undefined;

packages/installations/src/helpers/get-installation-entry.test.ts

@@ -107,8 +107,8 @@ describe('getInstallationEntry', () => {
     clock.next(); // Finish registration request.
     await expect(registrationPromise).to.be.fulfilled;
 
-    const newDbEntry = (await get(appConfig)) as RegisteredInstallationEntry;
-    expect(newDbEntry.registrationStatus).to.equal(RequestStatus.COMPLETED);
+    const newDbEntry = await get(appConfig);
+    expect(newDbEntry!.registrationStatus).to.equal(RequestStatus.COMPLETED);
   });
 
   it('saves the InstallationEntry in the database when registration fails', async () => {
@@ -137,8 +137,8 @@ describe('getInstallationEntry', () => {
     clock.next(); // Finish registration request.
     await expect(registrationPromise).to.be.rejected;
 
-    const newDbEntry = (await get(appConfig)) as UnregisteredInstallationEntry;
-    expect(newDbEntry.registrationStatus).to.equal(RequestStatus.NOT_STARTED);
+    const newDbEntry = await get(appConfig);
+    expect(newDbEntry!.registrationStatus).to.equal(RequestStatus.NOT_STARTED);
   });
 
   it('removes the InstallationEntry from the database when registration fails with 409', async () => {

packages/installations/src/helpers/get-installation-entry.ts

@@ -43,18 +43,15 @@ export async function getInstallationEntry(
 ): Promise<InstallationEntryWithRegistrationPromise> {
   let registrationPromise: Promise<RegisteredInstallationEntry> | undefined;
 
-  const installationEntry = await update(
-    appConfig,
-    (oldEntry?: InstallationEntry): InstallationEntry => {
-      const installationEntry = updateOrCreateInstallationEntry(oldEntry);
-      const entryWithPromise = triggerRegistrationIfNecessary(
-        appConfig,
-        installationEntry
-      );
-      registrationPromise = entryWithPromise.registrationPromise;
-      return entryWithPromise.installationEntry;
-    }
-  );
+  const installationEntry = await update(appConfig, oldEntry => {
+    const installationEntry = updateOrCreateInstallationEntry(oldEntry);
+    const entryWithPromise = triggerRegistrationIfNecessary(
+      appConfig,
+      installationEntry
+    );
+    registrationPromise = entryWithPromise.registrationPromise;
+    return entryWithPromise.installationEntry;
+  });
 
   if (installationEntry.fid === INVALID_FID) {
     // FID generation failed. Waiting for the FID from the server.
@@ -67,6 +64,10 @@ export async function getInstallationEntry(
   };
 }
 
+/**
+ * Creates a new Installation Entry if one does not exist.
+ * Also clears timed out pending requests.
+ */
 function updateOrCreateInstallationEntry(
   oldEntry: InstallationEntry | undefined
 ): InstallationEntry {
@@ -75,19 +76,12 @@ function updateOrCreateInstallationEntry(
     registrationStatus: RequestStatus.NOT_STARTED
   };
 
-  if (hasInstallationRequestTimedOut(entry)) {
-    return {
-      fid: entry.fid,
-      registrationStatus: RequestStatus.NOT_STARTED
-    };
-  }
-
-  return entry;
+  return clearTimedOutRequest(entry);
 }
 
 /**
- * If the Firebase Installation is not registered yet, this will trigger the registration
- * and return an InProgressInstallationEntry.
+ * If the Firebase Installation is not registered yet, this will trigger the
+ * registration and return an InProgressInstallationEntry.
  */
 function triggerRegistrationIfNecessary(
   appConfig: AppConfig,
@@ -189,23 +183,23 @@ async function waitUntilFidRegistration(
 function updateInstallationRequest(
   appConfig: AppConfig
 ): Promise<InstallationEntry> {
-  return update(
-    appConfig,
-    (oldEntry?: InstallationEntry): InstallationEntry => {
-      if (!oldEntry) {
-        throw ERROR_FACTORY.create(ErrorCode.INSTALLATION_NOT_FOUND);
-      }
-
-      if (hasInstallationRequestTimedOut(oldEntry)) {
-        return {
-          fid: oldEntry.fid,
-          registrationStatus: RequestStatus.NOT_STARTED
-        };
-      }
-
-      return oldEntry;
+  return update(appConfig, oldEntry => {
+    if (!oldEntry) {
+      throw ERROR_FACTORY.create(ErrorCode.INSTALLATION_NOT_FOUND);
     }
-  );
+    return clearTimedOutRequest(oldEntry);
+  });
+}
+
+function clearTimedOutRequest(entry: InstallationEntry): InstallationEntry {
+  if (hasInstallationRequestTimedOut(entry)) {
+    return {
+      fid: entry.fid,
+      registrationStatus: RequestStatus.NOT_STARTED
+    };
+  }
+
+  return entry;
 }
 
 function hasInstallationRequestTimedOut(

packages/installations/src/helpers/idb-manager.test.ts

@@ -20,6 +20,19 @@ import { AppConfig } from '../interfaces/app-config';
 import { getFakeAppConfig } from '../testing/get-fake-app';
 import '../testing/setup';
 import { clear, get, remove, set, update } from './idb-manager';
+import {
+  InstallationEntry,
+  RequestStatus
+} from '../interfaces/installation-entry';
+
+const VALUE_A: InstallationEntry = {
+  fid: 'VALUE_A',
+  registrationStatus: RequestStatus.NOT_STARTED
+};
+const VALUE_B: InstallationEntry = {
+  fid: 'VALUE_B',
+  registrationStatus: RequestStatus.NOT_STARTED
+};
 
 describe('idb manager', () => {
   let appConfig1: AppConfig;
@@ -32,9 +45,9 @@ describe('idb manager', () => {
 
   describe('get / set', () => {
     it('sets a value and then gets the same value back', async () => {
-      await set(appConfig1, 'value');
+      await set(appConfig1, VALUE_A);
       const value = await get(appConfig1);
-      expect(value).to.equal('value');
+      expect(value).to.deep.equal(VALUE_A);
     });
 
     it('gets undefined for a key that does not exist', async () => {
@@ -43,22 +56,22 @@ describe('idb manager', () => {
     });
 
     it('sets and gets multiple values with different keys', async () => {
-      await set(appConfig1, 'value');
-      await set(appConfig2, 'value2');
-      expect(await get(appConfig1)).to.equal('value');
-      expect(await get(appConfig2)).to.equal('value2');
+      await set(appConfig1, VALUE_A);
+      await set(appConfig2, VALUE_B);
+      expect(await get(appConfig1)).to.deep.equal(VALUE_A);
+      expect(await get(appConfig2)).to.deep.equal(VALUE_B);
     });
 
     it('overwrites a value', async () => {
-      await set(appConfig1, 'value');
-      await set(appConfig1, 'newValue');
-      expect(await get(appConfig1)).to.equal('newValue');
+      await set(appConfig1, VALUE_A);
+      await set(appConfig1, VALUE_B);
+      expect(await get(appConfig1)).to.deep.equal(VALUE_B);
     });
   });
 
   describe('remove', () => {
     it('deletes a key', async () => {
-      await set(appConfig1, 'value');
+      await set(appConfig1, VALUE_A);
       await remove(appConfig1);
       expect(await get(appConfig1)).to.be.undefined;
     });
@@ -71,8 +84,8 @@ describe('idb manager', () => {
 
   describe('clear', () => {
     it('deletes all keys', async () => {
-      await set(appConfig1, 'value');
-      await set(appConfig2, 'value2');
+      await set(appConfig1, VALUE_A);
+      await set(appConfig2, VALUE_B);
       await clear();
       expect(await get(appConfig1)).to.be.undefined;
       expect(await get(appConfig2)).to.be.undefined;
@@ -83,53 +96,27 @@ describe('idb manager', () => {
     it('gets and sets a value atomically, returns the new value', async () => {
       let isGetCalled = false;
 
-      await set(appConfig1, 'value');
+      await set(appConfig1, VALUE_A);
 
-      const resultPromise = update<string, string>(appConfig1, oldValue => {
+      const resultPromise = update(appConfig1, oldValue => {
         // get is already called for the same key, but it will only complete
         // after update transaction finishes, at which point it will return the
         // new value.
         expect(isGetCalled).to.be.true;
 
-        expect(oldValue).to.equal('value');
-        return 'newValue';
+        expect(oldValue).to.deep.equal(VALUE_A);
+        return VALUE_B;
       });
 
       // Called immediately after update, but before update completed.
       const getPromise = get(appConfig1);
       isGetCalled = true;
 
       // Update returns the new value
-      expect(await resultPromise).to.equal('newValue');
+      expect(await resultPromise).to.deep.equal(VALUE_B);
 
       // If update weren't atomic, this would return the old value.
-      expect(await getPromise).to.equal('newValue');
-    });
-
-    it('can change the type of the value', async () => {
-      let isGetCalled = false;
-
-      await set(appConfig1, 'value');
-
-      const resultPromise = update<string, number>(appConfig1, oldValue => {
-        // get is already called for the same key, but it will only complete
-        // after update transaction finishes, at which point it will return the
-        // new value.
-        expect(isGetCalled).to.be.true;
-
-        expect(oldValue).to.equal('value');
-        return 123;
-      });
-
-      // Called immediately after update, but before update completed.
-      const getPromise = get(appConfig1);
-      isGetCalled = true;
-
-      // Update returns the new value
-      expect(await resultPromise).to.equal(123);
-
-      // If update weren't atomic, this would return the old value.
-      expect(await getPromise).to.equal(123);
+      expect(await getPromise).to.deep.equal(VALUE_B);
     });
   });
 });

packages/installations/src/helpers/idb-manager.ts

@@ -17,6 +17,7 @@
 
 import { DB, openDb } from 'idb';
 import { AppConfig } from '../interfaces/app-config';
+import { InstallationEntry } from '../interfaces/installation-entry';
 
 const DATABASE_NAME = 'firebase-installations-database';
 const DATABASE_VERSION = 1;
@@ -41,7 +42,9 @@ function getDbPromise(): Promise<DB> {
 }
 
 /** Gets record(s) from the objectStore that match the given key. */
-export async function get(appConfig: AppConfig): Promise<unknown> {
+export async function get(
+  appConfig: AppConfig
+): Promise<InstallationEntry | undefined> {
   const key = getKey(appConfig);
   const db = await getDbPromise();
   return db
@@ -51,7 +54,7 @@ export async function get(appConfig: AppConfig): Promise<unknown> {
 }
 
 /** Assigns or overwrites the record for the given key with the given value. */
-export async function set<ValueType>(
+export async function set<ValueType extends InstallationEntry>(
   appConfig: AppConfig,
   value: ValueType
 ): Promise<ValueType> {
@@ -78,21 +81,17 @@ export async function remove(appConfig: AppConfig): Promise<void> {
  * deleted instead.
  * @return Updated value
  */
-export async function update<OldType, NewType>(
+export async function update<ValueType extends InstallationEntry | undefined>(
   appConfig: AppConfig,
-  updateFn: (previousValue: OldType | undefined) => NewType
-): Promise<NewType> {
+  updateFn: (previousValue: InstallationEntry | undefined) => ValueType
+): Promise<ValueType> {
   const key = getKey(appConfig);
   const db = await getDbPromise();
   const tx = db.transaction(OBJECT_STORE_NAME, 'readwrite');
   const store = tx.objectStore(OBJECT_STORE_NAME);
-  const oldValue = await store.get(key);
+  const oldValue: InstallationEntry | undefined = await store.get(key);
   const newValue = updateFn(oldValue);
 
-  if (newValue === oldValue) {
-    return newValue;
-  }
-
   if (newValue === undefined) {
     await store.delete(key);
   } else {