fix(firestore): Fix PreferRest caching (#2040)

Fixed a caching issue when a non-empty FirestoreSettings object is passed to initializeFirestore(...)

Author: lahirumaramba

src/firestore/firestore-internal.ts

@@ -53,35 +53,43 @@ export class FirestoreService {
     this.appInternal = app;
   }
 
-  getDatabase(databaseId: string, settings?: FirestoreSettings): Firestore {
-    settings ??= {};
+  initializeDatabase(databaseId: string, settings: FirestoreSettings): Firestore {
+    const existingInstance = this.databases.get(databaseId);
+    if (existingInstance) {
+      const initialSettings = this.firestoreSettings.get(databaseId) ?? {};
+      if (this.checkIfSameSettings(settings, initialSettings)) {
+        return existingInstance;
+      }
+      throw new FirebaseFirestoreError({
+        code: 'failed-precondition',
+        message: 'initializeFirestore() has already been called with ' +
+          'different options. To avoid this error, call initializeFirestore() with the ' +
+          'same options as when it was originally called, or call getFirestore() to return the' +
+          ' already initialized instance.'
+      });
+    }
+    const newInstance = initFirestore(this.app, databaseId, settings);
+    this.databases.set(databaseId, newInstance);
+    this.firestoreSettings.set(databaseId, settings);
+    return newInstance;
+  }
+
+  getDatabase(databaseId: string): Firestore {
     let database = this.databases.get(databaseId);
     if (database === undefined) {
-      database = initFirestore(this.app, databaseId, settings);
+      database = initFirestore(this.app, databaseId, {});
       this.databases.set(databaseId, database);
-      this.firestoreSettings.set(databaseId, settings);
-    } else {
-      if (!this.checkIfSameSettings(databaseId, settings)) {
-        throw new FirebaseFirestoreError({
-          code: 'failed-precondition',
-          message: 'initializeFirestore() has already been called with ' +
-            'different options. To avoid this error, call initializeFirestore() with the ' +
-            'same options as when it was originally called, or call getFirestore() to return the' +
-            ' already initialized instance.'
-        });    
-      }
+      this.firestoreSettings.set(databaseId, {});
     }
     return database;
   }
 
-  private checkIfSameSettings(databaseId: string, firestoreSettings: FirestoreSettings): boolean {
+  private checkIfSameSettings(settingsA: FirestoreSettings, settingsB: FirestoreSettings): boolean {
+    const a = settingsA ?? {};
+    const b = settingsB ?? {};
     // If we start passing more settings to Firestore constructor,
     // replace this with deep equality check.
-    const existingSettings = this.firestoreSettings.get(databaseId);
-    if (!existingSettings) {
-      return true;
-    }
-    return (existingSettings.preferRest === firestoreSettings.preferRest);
+    return (a.preferRest === b.preferRest);
   }
 
   /**

src/firestore/index.ts

@@ -184,5 +184,5 @@ export function initializeFirestore(
   const firestoreService = firebaseApp.getOrInitService(
     'firestore', (app) => new FirestoreService(app));
 
-  return firestoreService.getDatabase(databaseId, settings);
+  return firestoreService.initializeDatabase(databaseId, settings);
 }

test/unit/firestore/index.spec.ts

@@ -33,15 +33,17 @@ chai.use(chaiAsPromised);
 const expect = chai.expect;
 
 describe('Firestore', () => {
-  let mockApp: App;
+  let mockAppOne: App;
+  let mockAppTwo: App;
   let mockCredentialApp: App;
 
   const noProjectIdError = 'Failed to initialize Google Cloud Firestore client with the '
   + 'available credentials. Must initialize the SDK with a certificate credential or '
   + 'application default credentials to use Cloud Firestore API.';
 
   beforeEach(() => {
-    mockApp = mocks.app();
+    mockAppOne = mocks.app();
+    mockAppTwo = mocks.app();
     mockCredentialApp = mocks.mockCredentialApp();
   });
 
@@ -61,26 +63,26 @@ describe('Firestore', () => {
 
     it('should not throw given a valid app', () => {
       expect(() => {
-        return getFirestore(mockApp);
+        return getFirestore(mockAppOne);
       }).not.to.throw();
     });
 
     it('should return the same instance for a given app instance', () => {
-      const db1: Firestore = getFirestore(mockApp);
-      const db2: Firestore = getFirestore(mockApp, DEFAULT_DATABASE_ID);
+      const db1: Firestore = getFirestore(mockAppOne);
+      const db2: Firestore = getFirestore(mockAppOne, DEFAULT_DATABASE_ID);
       expect(db1).to.equal(db2);
     });
 
     it('should return the same instance for a given app instance and databaseId', () => {
-      const db1: Firestore = getFirestore(mockApp, 'db');
-      const db2: Firestore = getFirestore(mockApp, 'db');
+      const db1: Firestore = getFirestore(mockAppOne, 'db');
+      const db2: Firestore = getFirestore(mockAppOne, 'db');
       expect(db1).to.equal(db2);
     });
 
     it('should return the different instance for given same app instance, but different databaseId', () => {
-      const db0: Firestore = getFirestore(mockApp, DEFAULT_DATABASE_ID);
-      const db1: Firestore = getFirestore(mockApp, 'db1');
-      const db2: Firestore = getFirestore(mockApp, 'db2');
+      const db0: Firestore = getFirestore(mockAppOne, DEFAULT_DATABASE_ID);
+      const db1: Firestore = getFirestore(mockAppOne, 'db1');
+      const db2: Firestore = getFirestore(mockAppOne, 'db2');
       expect(db0).to.not.equal(db1);
       expect(db0).to.not.equal(db2);
       expect(db1).to.not.equal(db2);
@@ -97,41 +99,65 @@ describe('Firestore', () => {
 
     it('should not throw given a valid app', () => {
       expect(() => {
-        return initializeFirestore(mockApp);
+        return initializeFirestore(mockAppOne);
       }).not.to.throw();
     });
 
     it('should return the same instance for a given app instance', () => {
-      const db1: Firestore = initializeFirestore(mockApp);
-      const db2: Firestore = initializeFirestore(mockApp, {}, DEFAULT_DATABASE_ID);
+      const db1: Firestore = initializeFirestore(mockAppOne);
+      const db2: Firestore = initializeFirestore(mockAppOne, {}, DEFAULT_DATABASE_ID);
+
+      const db3: Firestore = initializeFirestore(mockAppTwo, { preferRest: true });
+      const db4: Firestore = initializeFirestore(mockAppTwo, { preferRest: true }, DEFAULT_DATABASE_ID);
+
       expect(db1).to.equal(db2);
+      expect(db3).to.equal(db4);
     });
 
     it('should return the same instance for a given app instance and databaseId', () => {
-      const db1: Firestore = initializeFirestore(mockApp, {}, 'db');
-      const db2: Firestore = initializeFirestore(mockApp, {}, 'db');
+      const db1: Firestore = initializeFirestore(mockAppOne, {}, 'db');
+      const db2: Firestore = initializeFirestore(mockAppOne, {}, 'db');
+
+      const db3: Firestore = initializeFirestore(mockAppTwo, { preferRest: true }, 'db');
+      const db4: Firestore = initializeFirestore(mockAppTwo, { preferRest: true }, 'db');
+
       expect(db1).to.equal(db2);
+      expect(db3).to.equal(db4);
     });
 
-    it('should return the different instance for given same app instance, but different databaseId', () => {
-      const db0: Firestore = initializeFirestore(mockApp, {}, DEFAULT_DATABASE_ID);
-      const db1: Firestore = initializeFirestore(mockApp, {}, 'db1');
-      const db2: Firestore = initializeFirestore(mockApp, {}, 'db2');
+    it('should return a different instance for given same app instance, but different databaseId', () => {
+      const db0: Firestore = initializeFirestore(mockAppOne, {}, DEFAULT_DATABASE_ID);
+      const db1: Firestore = initializeFirestore(mockAppOne, {}, 'db1');
+      const db2: Firestore = initializeFirestore(mockAppOne, {}, 'db2');
+
+      const db3: Firestore = initializeFirestore(mockAppTwo, { preferRest: true }, DEFAULT_DATABASE_ID);
+      const db4: Firestore = initializeFirestore(mockAppTwo, { preferRest: true }, 'db1');
+      const db5: Firestore = initializeFirestore(mockAppTwo, { preferRest: true }, 'db2');
+
       expect(db0).to.not.equal(db1);
       expect(db0).to.not.equal(db2);
       expect(db1).to.not.equal(db2);
+
+      expect(db3).to.not.equal(db4);
+      expect(db3).to.not.equal(db5);
+      expect(db4).to.not.equal(db5);
     });
 
     it('getFirestore should return the same instance as initializeFirestore returned earlier', () => {
-      const db1: Firestore = initializeFirestore(mockApp, {}, 'db');
-      const db2: Firestore = getFirestore(mockApp, 'db');
+      const db1: Firestore = initializeFirestore(mockAppOne, {}, 'db');
+      const db2: Firestore = getFirestore(mockAppOne, 'db');
+      
+      const db3: Firestore = initializeFirestore(mockAppTwo, { preferRest: true });
+      const db4: Firestore = getFirestore(mockAppTwo);
+
       expect(db1).to.equal(db2);
+      expect(db3).to.equal(db4);
     });
 
     it('initializeFirestore should not allow create an instance with different settings', () => {
-      initializeFirestore(mockApp, {}, 'db');
+      initializeFirestore(mockAppTwo, {}, 'db');
       expect(() => {
-        return initializeFirestore(mockApp, { preferRest: true }, 'db');
+        return initializeFirestore(mockAppTwo, { preferRest: true }, 'db');
       }).to.throw(/has already been called with different options/);
     });
   });