Review

Author: schmidt-sebastian

packages/firestore/src/core/event_manager.ts

@@ -22,6 +22,10 @@ import { Query } from './query';
 import { SyncEngine, SyncEngineListener } from './sync_engine';
 import { OnlineState } from './types';
 import { ChangeType, DocumentViewChange, ViewSnapshot } from './view_snapshot';
+import { logError } from '../util/log';
+import { Code, FirestoreError } from '../util/error';
+
+const LOG_TAG = 'EventManager';
 
 /**
  * Holds the listeners and the last received ViewSnapshot for a query being
@@ -69,10 +73,21 @@ export class EventManager implements SyncEngineListener {
     }
 
     if (firstListen) {
-      queryInfo.viewSnap = await this.syncEngine.listen(query);
+      try {
+        queryInfo.viewSnap = await this.syncEngine.listen(query);
+      } catch (e) {
+        const msg = `Initialization of query '${query}' failed: ${e}`;
+        logError(LOG_TAG, msg);
+        if (e.name === 'IndexedDbTransactionError') {
+          listener.onError(new FirestoreError(Code.UNAVAILABLE, msg));
+        } else {
+          throw e;
+        }
+        return;
+      }
     }
 
-    this.queries.set(query, queryInfo!);
+    this.queries.set(query, queryInfo);
     queryInfo.listeners.push(listener);
 
     // Run global snapshot listeners if a consistent snapshot has been emitted.

packages/firestore/src/core/firestore_client.ts

@@ -27,7 +27,7 @@ import { Datastore } from '../remote/datastore';
 import { RemoteStore } from '../remote/remote_store';
 import { AsyncQueue } from '../util/async_queue';
 import { Code, FirestoreError } from '../util/error';
-import { logDebug, logError } from '../util/log';
+import { logDebug } from '../util/log';
 import { Deferred } from '../util/promise';
 import {
   EventManager,
@@ -398,23 +398,7 @@ export class FirestoreClient {
   ): QueryListener {
     this.verifyNotTerminated();
     const listener = new QueryListener(query, observer, options);
-    this.asyncQueue.enqueueAndForget(async () => {
-      try {
-        await this.eventMgr.listen(listener);
-      } catch (e) {
-        logError(LOG_TAG, `Failed to register query: ${e}`);
-        if (e.name === 'IndexedDbTransactionError') {
-          listener.onError(
-            new FirestoreError(
-              Code.UNAVAILABLE,
-              `Failed to register query: ${e}`
-            )
-          );
-        } else {
-          throw e;
-        }
-      }
-    });
+    this.asyncQueue.enqueueAndForget(() => this.eventMgr.listen(listener));
     return listener;
   }
 

packages/firestore/test/unit/specs/recovery_spec.test.ts

@@ -174,7 +174,7 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
       .expectEvents(query1, {})
       .failDatabase()
       .userListens(query2)
-      .expectEvents(query2, { errorCode: Code.UNKNOWN })
+      .expectEvents(query2, { errorCode: Code.UNAVAILABLE })
       .recoverDatabase()
       .userListens(query3)
       .watchAcksFull(query3, 1)
@@ -190,7 +190,7 @@ describeSpec('Persistence Recovery', ['no-ios', 'no-android'], () => {
       .expectEvents(query1, {})
       .failDatabase()
       .userListens(query2)
-      .expectEvents(query2, { errorCode: Code.UNKNOWN })
+      .expectEvents(query2, { errorCode: Code.UNAVAILABLE })
       .recoverDatabase()
       .userListens(query2)
       .watchAcksFull(query2, 1)

packages/firestore/test/unit/specs/spec_test_runner.ts

@@ -70,7 +70,7 @@ import {
 } from '../../../src/remote/watch_change';
 import { debugAssert, fail } from '../../../src/util/assert';
 import { AsyncQueue, TimerId } from '../../../src/util/async_queue';
-import { Code, FirestoreError } from '../../../src/util/error';
+import { FirestoreError } from '../../../src/util/error';
 import { primitiveComparator } from '../../../src/util/misc';
 import { forEach, objectSize } from '../../../src/util/obj';
 import { ObjectMap } from '../../../src/util/obj_map';
@@ -80,7 +80,7 @@ import {
   deletedDoc,
   deleteMutation,
   doc,
-  expectFirestoreError,
+  validateFirestoreError,
   filter,
   key,
   orderBy,
@@ -358,7 +358,7 @@ class EventAggregator implements Observer<ViewSnapshot> {
   }
 
   error(error: Error): void {
-    expectFirestoreError(error);
+    expect(error.name).to.equal('FirebaseError');
     this.pushEvent({ query: this.query, error: error as FirestoreError });
   }
 }
@@ -597,9 +597,16 @@ abstract class TestRunner {
   }
 
   private async doListen(listenSpec: SpecUserListen): Promise<void> {
+    let targetFailed = false;
+
     const querySpec = listenSpec[1];
     const query = parseQuery(querySpec);
-    const aggregator = new EventAggregator(query, this.pushEvent.bind(this));
+    const aggregator = new EventAggregator(query, e => {
+      if (e.error) {
+        targetFailed = true;
+      }
+      this.pushEvent(e);
+    });
     // TODO(dimond): Allow customizing listen options in spec tests
     const options = {
       includeMetadataChanges: true,
@@ -608,20 +615,11 @@ abstract class TestRunner {
     const queryListener = new QueryListener(query, aggregator, options);
     this.queryListeners.set(query, queryListener);
 
-    const targetAdded = await this.queue.enqueue(async () => {
-      try {
-        await this.eventManager.listen(queryListener);
-        return true;
-      } catch (e) {
-        expect(this.persistence.injectFailures).to.be.true;
-        queryListener.onError(
-          new FirestoreError(Code.UNAVAILABLE, e.message)
-        );
-        return false;
-      }
-    });
+    await this.queue.enqueue(() => this.eventManager.listen(queryListener));
 
-    if (targetAdded) {
+    if (targetFailed) {
+      expect(this.persistence.injectFailures).to.be.true;
+    } else {
       // Skip the backoff that may have been triggered by a previous call to
       // `watchStreamCloses()`.
       if (
@@ -1206,7 +1204,10 @@ abstract class TestRunner {
     const expectedQuery = parseQuery(expected.query);
     expect(actual.query).to.deep.equal(expectedQuery);
     if (expected.errorCode) {
-      expectFirestoreError(actual.error!);
+      validateFirestoreError(
+        mapCodeFromRpcCode(expected.errorCode),
+        actual.error!
+      );
     } else {
       const expectedChanges: DocumentViewChange[] = [];
       if (expected.removed) {

packages/firestore/test/util/helpers.ts

@@ -92,6 +92,7 @@ import { ByteString } from '../../src/util/byte_string';
 import { PlatformSupport } from '../../src/platform/platform';
 import { JsonProtoSerializer } from '../../src/remote/serializer';
 import { Timestamp } from '../../src/api/timestamp';
+import { Code, FirestoreError } from '../../src/util/error';
 
 /* eslint-disable no-restricted-globals */
 
@@ -819,8 +820,12 @@ export function expectEqualitySets<T>(
   }
 }
 
-export function expectFirestoreError(err: Error): void {
-  expect(err.name).to.equal('FirebaseError');
+export function validateFirestoreError(
+  expectedCode: Code,
+  actualError: Error
+): void {
+  expect(actualError.name).to.equal('FirebaseError');
+  expect((actualError as FirestoreError).code).to.equal(expectedCode);
 }
 
 export function forEachNumber<V>(