Implement configureFieldIndexes with tests (#6403)

* Implement ConfigureFieldIndexes * Implement ConfigureFieldIndexes * Implement ConfigureFieldIndexes * Implement ConfigureFieldIndexes * Enable Firestore client side indexing * Enable Firestore client side indexing * Fix CountingQueryEngine * Fix CountingQueryEngine * Fix CountingQueryEngine * Add spec test * Remove comment * Update yarn.lock * Add firestore changeset * Whitespace * Dependency fix
firebase · Jul 26, 2022 · f5426a5 · f5426a5
1 parent 34c503c
commit f5426a5
Show file tree

Hide file tree

Showing 23 changed files with 932 additions and 73 deletions.
diff --git a/.changeset/five-yaks-travel.md b/.changeset/five-yaks-travel.md
@@ -0,0 +1,5 @@
+---
+'@firebase/firestore': patch
+---
+
+Add internal implementation of setIndexConfiguration
diff --git a/packages/firestore/package.json b/packages/firestore/package.json
@@ -99,6 +99,7 @@
     "@rollup/plugin-json": "4.1.0",
     "@types/eslint": "7.29.0",
     "@types/json-stable-stringify": "1.0.34",
+    "chai-exclude": "2.1.0",
     "json-stable-stringify": "1.0.1",
     "protobufjs": "6.11.3",
     "rollup": "2.72.1",

diff --git a/packages/firestore/src/api/credentials.ts b/packages/firestore/src/api/credentials.ts
@@ -473,24 +473,25 @@ export class FirebaseAppCheckTokenProvider
     asyncQueue: AsyncQueue,
     changeListener: CredentialChangeListener<string>
   ): void {
-    const onTokenChanged: (tokenResult: AppCheckTokenResult) => Promise<void> =
-      tokenResult => {
-        if (tokenResult.error != null) {
-          logDebug(
-            'FirebaseAppCheckTokenProvider',
-            `Error getting App Check token; using placeholder token instead. Error: ${tokenResult.error.message}`
-          );
-        }
-        const tokenUpdated = tokenResult.token !== this.latestAppCheckToken;
-        this.latestAppCheckToken = tokenResult.token;
+    const onTokenChanged: (
+      tokenResult: AppCheckTokenResult
+    ) => Promise<void> = tokenResult => {
+      if (tokenResult.error != null) {
         logDebug(
           'FirebaseAppCheckTokenProvider',
-          `Received ${tokenUpdated ? 'new' : 'existing'} token.`
+          `Error getting App Check token; using placeholder token instead. Error: ${tokenResult.error.message}`
         );
-        return tokenUpdated
-          ? changeListener(tokenResult.token)
-          : Promise.resolve();
-      };
+      }
+      const tokenUpdated = tokenResult.token !== this.latestAppCheckToken;
+      this.latestAppCheckToken = tokenResult.token;
+      logDebug(
+        'FirebaseAppCheckTokenProvider',
+        `Received ${tokenUpdated ? 'new' : 'existing'} token.`
+      );
+      return tokenUpdated
+        ? changeListener(tokenResult.token)
+        : Promise.resolve();
+    };
 
     this.tokenListener = (tokenResult: AppCheckTokenResult) => {
       asyncQueue.enqueueRetryable(() => onTokenChanged(tokenResult));

diff --git a/packages/firestore/src/api/database.ts b/packages/firestore/src/api/database.ts
@@ -56,6 +56,7 @@ import { AsyncQueue } from '../util/async_queue';
 import { newAsyncQueue } from '../util/async_queue_impl';
 import { Code, FirestoreError } from '../util/error';
 import { cast } from '../util/input_validation';
+import { logWarn } from '../util/log';
 import { Deferred } from '../util/promise';
 
 import { LoadBundleTask } from './bundle';
@@ -332,7 +333,7 @@ function setPersistenceProviders(
         if (!canFallbackFromIndexedDbError(error)) {
           throw error;
         }
-        console.warn(
+        logWarn(
           'Error enabling offline persistence. Falling back to ' +
             'persistence disabled: ' +
             error

diff --git a/packages/firestore/src/api/index_configuration.ts b/packages/firestore/src/api/index_configuration.ts
@@ -15,7 +15,9 @@
  * limitations under the License.
  */
 
+import { getLocalStore } from '../core/firestore_client';
 import { fieldPathFromDotSeparatedString } from '../lite-api/user_data_reader';
+import { localStoreConfigureFieldIndexes } from '../local/local_store_impl';
 import {
   FieldIndex,
   IndexKind,
@@ -24,6 +26,7 @@ import {
 } from '../model/field_index';
 import { Code, FirestoreError } from '../util/error';
 import { cast } from '../util/input_validation';
+import { logWarn } from '../util/log';
 
 import { ensureFirestoreConfigured, Firestore } from './database';
 
@@ -150,17 +153,29 @@ export function setIndexConfiguration(
   jsonOrConfiguration: string | IndexConfiguration
 ): Promise<void> {
   firestore = cast(firestore, Firestore);
-  ensureFirestoreConfigured(firestore);
+  const client = ensureFirestoreConfigured(firestore);
 
+  // PORTING NOTE: We don't return an error if the user has not enabled
+  // persistence since `enableIndexeddbPersistence()` can fail on the Web.
+  if (!client.offlineComponents?.indexBackfillerScheduler) {
+    logWarn('Cannot enable indexes when persistence is disabled');
+    return Promise.resolve();
+  }
+  const parsedIndexes = parseIndexes(jsonOrConfiguration);
+  return getLocalStore(client).then(localStore =>
+    localStoreConfigureFieldIndexes(localStore, parsedIndexes)
+  );
+}
+
+export function parseIndexes(
+  jsonOrConfiguration: string | IndexConfiguration
+): FieldIndex[] {
   const indexConfiguration =
     typeof jsonOrConfiguration === 'string'
       ? (tryParseJson(jsonOrConfiguration) as IndexConfiguration)
       : jsonOrConfiguration;
   const parsedIndexes: FieldIndex[] = [];
 
-  // PORTING NOTE: We don't return an error if the user has not enabled
-  // persistence since `enableIndexeddbPersistence()` can fail on the Web.
-
   if (Array.isArray(indexConfiguration.indexes)) {
     for (const index of indexConfiguration.indexes) {
       const collectionGroup = tryGetString(index, 'collectionGroup');
@@ -194,9 +209,7 @@ export function setIndexConfiguration(
       );
     }
   }
-
-  // TODO(indexing): Configure indexes
-  return Promise.resolve();
+  return parsedIndexes;
 }
 
 function tryParseJson(json: string): Record<string, unknown> {
@@ -205,7 +218,7 @@ function tryParseJson(json: string): Record<string, unknown> {
   } catch (e) {
     throw new FirestoreError(
       Code.INVALID_ARGUMENT,
-      'Failed to parse JSON:' + (e as Error)?.message
+      'Failed to parse JSON: ' + (e as Error)?.message
     );
   }
 }

diff --git a/packages/firestore/src/local/index_backfiller.ts b/packages/firestore/src/local/index_backfiller.ts
@@ -25,7 +25,6 @@ import { debugAssert } from '../util/assert';
 import { AsyncQueue, DelayedOperation, TimerId } from '../util/async_queue';
 import { logDebug } from '../util/log';
 
-import { INDEXING_ENABLED } from './indexeddb_schema';
 import { ignoreIfPrimaryLeaseLoss, LocalStore } from './local_store';
 import { LocalWriteResult } from './local_store_impl';
 import { Persistence, Scheduler } from './persistence';
@@ -60,9 +59,7 @@ export class IndexBackfillerScheduler implements Scheduler {
       this.task === null,
       'Cannot start an already started IndexBackfillerScheduler'
     );
-    if (INDEXING_ENABLED) {
-      this.schedule(INITIAL_BACKFILL_DELAY_MS);
-    }
+    this.schedule(INITIAL_BACKFILL_DELAY_MS);
   }
 
   stop(): void {

diff --git a/packages/firestore/src/local/indexeddb_schema.ts b/packages/firestore/src/local/indexeddb_schema.ts
@@ -28,11 +28,6 @@ import {
 import { EncodedResourcePath } from './encoded_resource_path';
 import { DbTimestampKey } from './indexeddb_sentinels';
 
-// TODO(indexing): Remove this constant
-export const INDEXING_ENABLED = false;
-
-export const INDEXING_SCHEMA_VERSION = 15;
-
 /**
  * Schema Version for the Web client:
  * 1.  Initial version including Mutation Queue, Query Cache, and Remote
@@ -58,7 +53,7 @@ export const INDEXING_SCHEMA_VERSION = 15;
  * 15. Add indexing support.
  */
 
-export const SCHEMA_VERSION = INDEXING_ENABLED ? INDEXING_SCHEMA_VERSION : 14;
+export const SCHEMA_VERSION = 15;
 
 /**
  * Wrapper class to store timestamps (seconds and nanos) in IndexedDb objects.

diff --git a/packages/firestore/src/local/indexeddb_schema_converter.ts b/packages/firestore/src/local/indexeddb_schema_converter.ts
@@ -45,7 +45,7 @@ import {
   DbTarget,
   DbTargetDocument,
   DbTargetGlobal,
-  INDEXING_SCHEMA_VERSION
+  SCHEMA_VERSION
 } from './indexeddb_schema';
 import {
   DbRemoteDocument as DbRemoteDocumentLegacy,
@@ -146,7 +146,7 @@ export class SchemaConverter implements SimpleDbSchemaConverter {
     debugAssert(
       fromVersion < toVersion &&
         fromVersion >= 0 &&
-        toVersion <= INDEXING_SCHEMA_VERSION,
+        toVersion <= SCHEMA_VERSION,
       `Unexpected schema upgrade from v${fromVersion} to v${toVersion}.`
     );
 

diff --git a/packages/firestore/src/local/local_store_impl.ts b/packages/firestore/src/local/local_store_impl.ts
@@ -39,6 +39,8 @@ import {
 import { Document } from '../model/document';
 import { DocumentKey } from '../model/document_key';
 import {
+  FieldIndex,
+  fieldIndexSemanticComparator,
   INITIAL_LARGEST_BATCH_ID,
   newIndexOffsetSuccessorFromReadTime
 } from '../model/field_index';
@@ -57,6 +59,7 @@ import {
 } from '../protos/firestore_bundle_proto';
 import { RemoteEvent, TargetChange } from '../remote/remote_event';
 import { fromVersion, JsonProtoSerializer } from '../remote/serializer';
+import { diffArrays } from '../util/array';
 import { debugAssert, debugCast, hardAssert } from '../util/assert';
 import { ByteString } from '../util/byte_string';
 import { logDebug } from '../util/log';
@@ -1483,3 +1486,37 @@ export async function localStoreSaveNamedQuery(
     }
   );
 }
+
+export async function localStoreConfigureFieldIndexes(
+  localStore: LocalStore,
+  newFieldIndexes: FieldIndex[]
+): Promise<void> {
+  const localStoreImpl = debugCast(localStore, LocalStoreImpl);
+  const indexManager = localStoreImpl.indexManager;
+  const promises: Array<PersistencePromise<void>> = [];
+  return localStoreImpl.persistence.runTransaction(
+    'Configure indexes',
+    'readwrite',
+    transaction =>
+      indexManager
+        .getFieldIndexes(transaction)
+        .next(oldFieldIndexes =>
+          diffArrays(
+            oldFieldIndexes,
+            newFieldIndexes,
+            fieldIndexSemanticComparator,
+            fieldIndex => {
+              promises.push(
+                indexManager.addFieldIndex(transaction, fieldIndex)
+              );
+            },
+            fieldIndex => {
+              promises.push(
+                indexManager.deleteFieldIndex(transaction, fieldIndex)
+              );
+            }
+          )
+        )
+        .next(() => PersistencePromise.waitFor(promises))
+  );
+}
diff --git a/packages/firestore/src/local/query_engine.ts b/packages/firestore/src/local/query_engine.ts
@@ -43,7 +43,6 @@ import { Iterable } from '../util/misc';
 import { SortedSet } from '../util/sorted_set';
 
 import { IndexManager, IndexType } from './index_manager';
-import { INDEXING_ENABLED } from './indexeddb_schema';
 import { LocalDocumentsView } from './local_documents_view';
 import { PersistencePromise } from './persistence_promise';
 import { PersistenceTransaction } from './persistence_transaction';
@@ -134,10 +133,6 @@ export class QueryEngine {
     transaction: PersistenceTransaction,
     query: Query
   ): PersistencePromise<DocumentMap | null> {
-    if (!INDEXING_ENABLED) {
-      return PersistencePromise.resolve<DocumentMap | null>(null);
-    }
-
     if (queryMatchesAllDocuments(query)) {
       // Queries that match all documents don't benefit from using
       // key-based lookups. It is more efficient to scan all documents in a

diff --git a/packages/firestore/src/util/array.ts b/packages/firestore/src/util/array.ts
@@ -55,3 +55,59 @@ export function findIndex<A>(
   }
   return null;
 }
+
+/**
+ * Compares two array for equality using comparator. The method computes the
+ * intersection and invokes `onAdd` for every element that is in `after` but not
+ * `before`. `onRemove` is invoked for every element in `before` but missing
+ * from `after`.
+ *
+ * The method creates a copy of both `before` and `after` and runs in O(n log
+ * n), where n is the size of the two lists.
+ *
+ * @param before - The elements that exist in the original array.
+ * @param after - The elements to diff against the original array.
+ * @param comparator - The comparator for the elements in before and after.
+ * @param onAdd - A function to invoke for every element that is part of `
+ * after` but not `before`.
+ * @param onRemove - A function to invoke for every element that is part of
+ * `before` but not `after`.
+ */
+export function diffArrays<T>(
+  before: T[],
+  after: T[],
+  comparator: (l: T, r: T) => number,
+  onAdd: (entry: T) => void,
+  onRemove: (entry: T) => void
+): void {
+  before = [...before];
+  after = [...after];
+  before.sort(comparator);
+  after.sort(comparator);
+
+  const bLen = before.length;
+  const aLen = after.length;
+  let a = 0;
+  let b = 0;
+  while (a < aLen && b < bLen) {
+    const cmp = comparator(before[b], after[a]);
+    if (cmp < 0) {
+      // The element was removed if the next element in our ordered
+      // walkthrough is only in `before`.
+      onRemove(before[b++]);
+    } else if (cmp > 0) {
+      // The element was added if the next element in our ordered walkthrough
+      // is only in `after`.
+      onAdd(after[a++]);
+    } else {
+      a++;
+      b++;
+    }
+  }
+  while (a < aLen) {
+    onAdd(after[a++]);
+  }
+  while (b < bLen) {
+    onRemove(before[b++]);
+  }
+}
diff --git a/packages/firestore/src/util/async_observer.ts b/packages/firestore/src/util/async_observer.ts
@@ -18,6 +18,7 @@
 import { Observer } from '../core/event_manager';
 
 import { FirestoreError } from './error';
+import { logError } from './log';
 import { EventHandler } from './misc';
 
 /*
@@ -44,7 +45,7 @@ export class AsyncObserver<T> implements Observer<T> {
     if (this.observer.error) {
       this.scheduleEvent(this.observer.error, error);
     } else {
-      console.error('Uncaught Error in snapshot listener:', error);
+      logError('Uncaught Error in snapshot listener:', error);
     }
   }
 

diff --git a/packages/firestore/test/integration/api/index_configuration.test.ts b/packages/firestore/test/integration/api/index_configuration.test.ts
@@ -22,7 +22,7 @@ import { apiDescribe, withTestDb } from '../util/helpers';
 
 apiDescribe('Index Configuration:', (persistence: boolean) => {
   it('supports JSON', () => {
-    return withTestDb(persistence, db => {
+    return withTestDb(persistence, async db => {
       return setIndexConfiguration(
         db,
         '{\n' +
@@ -59,7 +59,7 @@ apiDescribe('Index Configuration:', (persistence: boolean) => {
   });
 
   it('supports schema', () => {
-    return withTestDb(persistence, db => {
+    return withTestDb(persistence, async db => {
       return setIndexConfiguration(db, {
         indexes: [
           {
@@ -79,14 +79,18 @@ apiDescribe('Index Configuration:', (persistence: boolean) => {
 
   it('bad JSON does not crash client', () => {
     return withTestDb(persistence, async db => {
-      expect(() => setIndexConfiguration(db, '{,}')).to.throw(
-        'Failed to parse JSON'
-      );
+      const action = (): Promise<void> => setIndexConfiguration(db, '{,}');
+      if (persistence) {
+        expect(action).to.throw(/Failed to parse JSON/);
+      } else {
+        // Silently do nothing. Parsing is not done and therefore no error is thrown.
+        await action();
+      }
     });
   });
 
   it('bad index does not crash client', () => {
-    return withTestDb(persistence, db => {
+    return withTestDb(persistence, async db => {
       return setIndexConfiguration(
         db,
         '{\n' +