From 9e9641231d460716fb47ffcd6e7f0de1f84e254c Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 10 Aug 2023 01:09:40 -0400 Subject: [PATCH 01/28] enable/disablePersistentCacheIndexAutoCreation() implemented, but hidden --- packages/firestore/src/api.ts | 6 + .../src/api/persistent_cache_index_manager.ts | 143 ++++++ .../firestore/src/core/firestore_client.ts | 32 +- packages/firestore/src/local/index_manager.ts | 19 + .../src/local/indexeddb_index_manager.ts | 20 + .../local/indexeddb_remote_document_cache.ts | 5 +- .../src/local/local_documents_view.ts | 24 +- .../firestore/src/local/local_store_impl.ts | 42 +- .../src/local/memory_index_manager.ts | 8 + packages/firestore/src/local/query_context.ts | 38 ++ packages/firestore/src/local/query_engine.ts | 118 ++++- .../src/local/remote_document_cache.ts | 6 +- .../src/model/target_index_matcher.ts | 68 ++- packages/firestore/src/util/async_queue.ts | 4 + packages/firestore/src/util/testing_hooks.ts | 94 ++++ .../persistent_cache_index_manager.test.ts | 365 +++++++++++++++ .../test/integration/util/helpers.ts | 34 ++ .../integration/util/testing_hooks_util.ts | 62 +++ .../test/unit/local/counting_query_engine.ts | 6 +- .../test/unit/local/index_manager.test.ts | 98 +++- .../test/unit/local/local_store.test.ts | 433 ++++++++++++++++-- .../test/unit/local/query_engine.test.ts | 171 ++++++- .../test/unit/local/test_index_manager.ts | 8 + .../unit/model/target_index_matcher.test.ts | 252 ++++++++++ 24 files changed, 1976 insertions(+), 80 deletions(-) create mode 100644 packages/firestore/src/api/persistent_cache_index_manager.ts create mode 100644 packages/firestore/src/local/query_context.ts create mode 100644 packages/firestore/test/integration/api/persistent_cache_index_manager.test.ts diff --git a/packages/firestore/src/api.ts b/packages/firestore/src/api.ts index aabc632d2f6..7a39da825f2 100644 --- a/packages/firestore/src/api.ts +++ b/packages/firestore/src/api.ts @@ -223,4 +223,10 @@ export type { } from './api/credentials'; export { EmptyAuthCredentialsProvider as _EmptyAuthCredentialsProvider } from './api/credentials'; export { EmptyAppCheckTokenProvider as _EmptyAppCheckTokenProvider } from './api/credentials'; +export { + PersistentCacheIndexManager as _PersistentCacheIndexManager, + getPersistentCacheIndexManager as _getPersistentCacheIndexManager, + enablePersistentCacheIndexAutoCreation as _enablePersistentCacheIndexAutoCreation, + disablePersistentCacheIndexAutoCreation as _disablePersistentCacheIndexAutoCreation +} from './api/persistent_cache_index_manager'; export { TestingHooks as _TestingHooks } from './util/testing_hooks'; diff --git a/packages/firestore/src/api/persistent_cache_index_manager.ts b/packages/firestore/src/api/persistent_cache_index_manager.ts new file mode 100644 index 00000000000..2c2b69a8ce2 --- /dev/null +++ b/packages/firestore/src/api/persistent_cache_index_manager.ts @@ -0,0 +1,143 @@ +/** + * @license + * Copyright 2023 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { + firestoreClientSetPersistentCacheIndexAutoCreationEnabled, + FirestoreClient +} from '../core/firestore_client'; +import { cast } from '../util/input_validation'; +import { logDebug, logWarn } from '../util/log'; +import { TestingHooks } from '../util/testing_hooks'; + +import { ensureFirestoreConfigured, Firestore } from './database'; + +/** + * A `PersistentCacheIndexManager` which you can config persistent cache indexes + * used for local query execution. + * + * To use, call `getPersistentCacheIndexManager()` to get an instance. + * + * TODO(CSI) Remove @internal to make the API publicly available. + * @internal + */ +export class PersistentCacheIndexManager { + readonly type: 'PersistentCacheIndexManager' = 'PersistentCacheIndexManager'; + + /** @hideconstructor */ + constructor(readonly _client: FirestoreClient) {} +} + +/** + * Returns the PersistentCache Index Manager used by the given `Firestore` + * object. + * + * @return The `PersistentCacheIndexManager` instance, or `null` if local + * persistent storage is not in use. + * + * TODO(CSI) Remove @internal to make the API publicly available. + * @internal + */ +export function getPersistentCacheIndexManager( + firestore: Firestore +): PersistentCacheIndexManager | null { + firestore = cast(firestore, Firestore); + + const cachedInstance = persistentCacheIndexManagerByFirestore.get(firestore); + if (cachedInstance) { + return cachedInstance; + } + + const client = ensureFirestoreConfigured(firestore); + if (client._uninitializedComponentsProvider?._offlineKind !== 'persistent') { + return null; + } + + const instance = new PersistentCacheIndexManager(client); + persistentCacheIndexManagerByFirestore.set(firestore, instance); + return instance; +} + +/** + * Enables SDK to create persistent cache indexes automatically for local query + * execution when SDK believes cache indexes can help improves performance. + * + * This feature is disabled by default. + * + * TODO(CSI) Remove @internal to make the API publicly available. + * @internal + */ +export function enablePersistentCacheIndexAutoCreation( + indexManager: PersistentCacheIndexManager +): void { + setPersistentCacheIndexAutoCreationEnabled(indexManager, true); +} + +/** + * Stops creating persistent cache indexes automatically for local query + * execution. The indexes which have been created by calling + * `enablePersistentCacheIndexAutoCreation()` still take effect. + * + * TODO(CSI) Remove @internal to make the API publicly available. + * @internal + */ +export function disablePersistentCacheIndexAutoCreation( + indexManager: PersistentCacheIndexManager +): void { + setPersistentCacheIndexAutoCreationEnabled(indexManager, false); +} + +function setPersistentCacheIndexAutoCreationEnabled( + indexManager: PersistentCacheIndexManager, + enabled: boolean +): void { + indexManager._client.verifyNotTerminated(); + + const promise = firestoreClientSetPersistentCacheIndexAutoCreationEnabled( + indexManager._client, + enabled + ); + + promise + .then(_ => + logDebug( + `setting persistent cache index auto creation ` + + `enabled=${enabled} succeeded` + ) + ) + .catch(error => + logWarn( + `setting persistent cache index auto creation ` + + `enabled=${enabled} failed`, + error + ) + ); + + TestingHooks.instance?.notifyPersistentCacheIndexAutoCreationToggled(promise); +} + +/** + * Maps `Firestore` instances to their corresponding + * `PersistentCacheIndexManager` instances. + * + * Use a `WeakMap` so that the mapping will be automatically dropped when the + * `Firestore` instance is garbage collected. This emulates a private member + * as described in https://goo.gle/454yvug. + */ +const persistentCacheIndexManagerByFirestore = new WeakMap< + Firestore, + PersistentCacheIndexManager +>(); diff --git a/packages/firestore/src/core/firestore_client.ts b/packages/firestore/src/core/firestore_client.ts index df6127f978e..c27d50a8998 100644 --- a/packages/firestore/src/core/firestore_client.ts +++ b/packages/firestore/src/core/firestore_client.ts @@ -23,13 +23,16 @@ import { CredentialsProvider } from '../api/credentials'; import { User } from '../auth/user'; +import { IndexType } from '../local/index_manager'; import { LocalStore } from '../local/local_store'; import { localStoreConfigureFieldIndexes, localStoreExecuteQuery, localStoreGetNamedQuery, localStoreHandleUserChange, - localStoreReadDocument + localStoreReadDocument, + localStoreSetIndexAutoCreationEnabled, + TestingHooks as LocalStoreTestingHooks } from '../local/local_store_impl'; import { Persistence } from '../local/persistence'; import { Document } from '../model/document'; @@ -828,3 +831,30 @@ export function firestoreClientSetIndexConfiguration( ); }); } + +export function firestoreClientSetPersistentCacheIndexAutoCreationEnabled( + client: FirestoreClient, + enabled: boolean +): Promise { + return client.asyncQueue.enqueue(async () => { + return localStoreSetIndexAutoCreationEnabled( + await getLocalStore(client), + enabled + ); + }); +} + +/** + * Test-only hooks into the SDK for use exclusively by integration tests. + */ +export class TestingHooks { + constructor(private client: FirestoreClient) {} + + getQueryIndexType(query: Query): Promise { + return this.client.asyncQueue.enqueue(async () => { + const localStore = await getLocalStore(this.client); + const localStoreTestingHooks = new LocalStoreTestingHooks(localStore); + return localStoreTestingHooks.getQueryIndexType(query); + }); + } +} diff --git a/packages/firestore/src/local/index_manager.ts b/packages/firestore/src/local/index_manager.ts index 39d958349d4..78bc47a9471 100644 --- a/packages/firestore/src/local/index_manager.ts +++ b/packages/firestore/src/local/index_manager.ts @@ -41,6 +41,19 @@ export const enum IndexType { FULL } +export function displayNameForIndexType(indexType: IndexType): string { + switch (indexType) { + case IndexType.NONE: + return 'NONE'; + case IndexType.PARTIAL: + return 'PARTIAL'; + case IndexType.FULL: + return 'FULL'; + default: + return `[unknown IndexType: ${indexType}]`; + } +} + /** * Represents a set of indexes that are used to execute queries efficiently. * @@ -92,6 +105,12 @@ export interface IndexManager { index: FieldIndex ): PersistencePromise; + /** Creates a full matched field index which serves the given target. */ + createTargetIndexes( + transaction: PersistenceTransaction, + target: Target + ): PersistencePromise; + /** * Returns a list of field indexes that correspond to the specified collection * group. diff --git a/packages/firestore/src/local/indexeddb_index_manager.ts b/packages/firestore/src/local/indexeddb_index_manager.ts index a776c95c1da..f7e5991a6be 100644 --- a/packages/firestore/src/local/indexeddb_index_manager.ts +++ b/packages/firestore/src/local/indexeddb_index_manager.ts @@ -252,6 +252,26 @@ export class IndexedDbIndexManager implements IndexManager { ); } + createTargetIndexes( + transaction: PersistenceTransaction, + target: Target + ): PersistencePromise { + return PersistencePromise.forEach( + this.getSubTargets(target), + (subTarget: Target) => { + return this.getIndexType(transaction, subTarget).next(type => { + if (type === IndexType.NONE || type === IndexType.PARTIAL) { + const targetIndexMatcher = new TargetIndexMatcher(subTarget); + return this.addFieldIndex( + transaction, + targetIndexMatcher.buildTargetIndex() + ); + } + }); + } + ); + } + getDocumentsMatchingTarget( transaction: PersistenceTransaction, target: Target diff --git a/packages/firestore/src/local/indexeddb_remote_document_cache.ts b/packages/firestore/src/local/indexeddb_remote_document_cache.ts index c3af0655cc4..b3d4658d53d 100644 --- a/packages/firestore/src/local/indexeddb_remote_document_cache.ts +++ b/packages/firestore/src/local/indexeddb_remote_document_cache.ts @@ -55,6 +55,7 @@ import { } from './local_serializer'; import { PersistencePromise } from './persistence_promise'; import { PersistenceTransaction } from './persistence_transaction'; +import { QueryContext } from './query_context'; import { RemoteDocumentCache } from './remote_document_cache'; import { RemoteDocumentChangeBuffer } from './remote_document_change_buffer'; import { SimpleDbStore } from './simple_db'; @@ -279,7 +280,8 @@ class IndexedDbRemoteDocumentCacheImpl implements IndexedDbRemoteDocumentCache { transaction: PersistenceTransaction, query: Query, offset: IndexOffset, - mutatedDocs: OverlayMap + mutatedDocs: OverlayMap, + context?: QueryContext ): PersistencePromise { const collection = query.path; const startKey = [ @@ -300,6 +302,7 @@ class IndexedDbRemoteDocumentCacheImpl implements IndexedDbRemoteDocumentCache { return remoteDocumentsStore(transaction) .loadAll(IDBKeyRange.bound(startKey, endKey, true)) .next(dbRemoteDocs => { + context?.incrementDocumentReadCount(dbRemoteDocs.length); let results = mutableDocumentMap(); for (const dbRemoteDoc of dbRemoteDocs) { const document = this.maybeDecodeDocument( diff --git a/packages/firestore/src/local/local_documents_view.ts b/packages/firestore/src/local/local_documents_view.ts index 78802e443bf..fa64ed76eb2 100644 --- a/packages/firestore/src/local/local_documents_view.ts +++ b/packages/firestore/src/local/local_documents_view.ts @@ -60,6 +60,7 @@ import { MutationQueue } from './mutation_queue'; import { OverlayedDocument } from './overlayed_document'; import { PersistencePromise } from './persistence_promise'; import { PersistenceTransaction } from './persistence_transaction'; +import { QueryContext } from './query_context'; import { RemoteDocumentCache } from './remote_document_cache'; /** @@ -355,11 +356,14 @@ export class LocalDocumentsView { * @param transaction - The persistence transaction. * @param query - The query to match documents against. * @param offset - Read time and key to start scanning by (exclusive). + * @param context - A optional tracker to keep a record of important details + * during database local query execution. */ getDocumentsMatchingQuery( transaction: PersistenceTransaction, query: Query, - offset: IndexOffset + offset: IndexOffset, + context?: QueryContext ): PersistencePromise { if (isDocumentQuery(query)) { return this.getDocumentsMatchingDocumentQuery(transaction, query.path); @@ -367,13 +371,15 @@ export class LocalDocumentsView { return this.getDocumentsMatchingCollectionGroupQuery( transaction, query, - offset + offset, + context ); } else { return this.getDocumentsMatchingCollectionQuery( transaction, query, - offset + offset, + context ); } } @@ -472,7 +478,8 @@ export class LocalDocumentsView { private getDocumentsMatchingCollectionGroupQuery( transaction: PersistenceTransaction, query: Query, - offset: IndexOffset + offset: IndexOffset, + context?: QueryContext ): PersistencePromise { debugAssert( query.path.isEmpty(), @@ -493,7 +500,8 @@ export class LocalDocumentsView { return this.getDocumentsMatchingCollectionQuery( transaction, collectionQuery, - offset + offset, + context ).next(r => { r.forEach((key, doc) => { results = results.insert(key, doc); @@ -506,7 +514,8 @@ export class LocalDocumentsView { private getDocumentsMatchingCollectionQuery( transaction: PersistenceTransaction, query: Query, - offset: IndexOffset + offset: IndexOffset, + context?: QueryContext ): PersistencePromise { // Query the remote documents and overlay mutations. let overlays: OverlayMap; @@ -518,7 +527,8 @@ export class LocalDocumentsView { transaction, query, offset, - overlays + overlays, + context ); }) .next(remoteDocuments => { diff --git a/packages/firestore/src/local/local_store_impl.ts b/packages/firestore/src/local/local_store_impl.ts index e3bb8dd3fdb..1a4ed62de9a 100644 --- a/packages/firestore/src/local/local_store_impl.ts +++ b/packages/firestore/src/local/local_store_impl.ts @@ -70,7 +70,7 @@ import { BATCHID_UNKNOWN } from '../util/types'; import { BundleCache } from './bundle_cache'; import { DocumentOverlayCache } from './document_overlay_cache'; -import { IndexManager } from './index_manager'; +import { IndexManager, IndexType } from './index_manager'; import { IndexedDbMutationQueue } from './indexeddb_mutation_queue'; import { IndexedDbPersistence } from './indexeddb_persistence'; import { IndexedDbTargetCache } from './indexeddb_target_cache'; @@ -1085,7 +1085,7 @@ export function localStoreExecuteQuery( return localStoreImpl.persistence.runTransaction( 'Execute query', - 'readonly', + 'readwrite', // Use readwrite instead of readonly so indexes can be created txn => { return localStoreGetTargetData(localStoreImpl, txn, queryToTarget(query)) .next(targetData => { @@ -1526,3 +1526,41 @@ export async function localStoreConfigureFieldIndexes( .next(() => PersistencePromise.waitFor(promises)) ); } + +export function localStoreSetIndexAutoCreationEnabled( + localStore: LocalStore, + enabled: boolean +): void { + const localStoreImpl = debugCast(localStore, LocalStoreImpl); + localStoreImpl.queryEngine.indexAutoCreationEnabled = enabled; +} + +/** + * Test-only hooks into the SDK for use exclusively by integration tests. + */ +export class TestingHooks { + private readonly localStoreImpl: LocalStoreImpl; + + constructor(localStore: LocalStore) { + this.localStoreImpl = debugCast(localStore, LocalStoreImpl); + } + + setIndexAutoCreationMinCollectionSize(newValue: number): void { + this.localStoreImpl.queryEngine.indexAutoCreationMinCollectionSize = + newValue; + } + + setRelativeIndexReadCostPerDocument(newValue: number): void { + this.localStoreImpl.queryEngine.relativeIndexReadCostPerDocument = newValue; + } + + getQueryIndexType(query: Query): Promise { + const target = queryToTarget(query); + const indexManager = this.localStoreImpl.indexManager; + return this.localStoreImpl.persistence.runTransaction( + 'local_store_impl TestingHooks getQueryIndexType', + 'readonly', + txn => indexManager.getIndexType(txn, target) + ); + } +} diff --git a/packages/firestore/src/local/memory_index_manager.ts b/packages/firestore/src/local/memory_index_manager.ts index 025bc566ab1..5a363118767 100644 --- a/packages/firestore/src/local/memory_index_manager.ts +++ b/packages/firestore/src/local/memory_index_manager.ts @@ -66,6 +66,14 @@ export class MemoryIndexManager implements IndexManager { return PersistencePromise.resolve(); } + createTargetIndexes( + transaction: PersistenceTransaction, + target: Target + ): PersistencePromise { + // Field indices are not supported with memory persistence. + return PersistencePromise.resolve(); + } + getDocumentsMatchingTarget( transaction: PersistenceTransaction, target: Target diff --git a/packages/firestore/src/local/query_context.ts b/packages/firestore/src/local/query_context.ts new file mode 100644 index 00000000000..98efb5ab376 --- /dev/null +++ b/packages/firestore/src/local/query_context.ts @@ -0,0 +1,38 @@ +/** + * @license + * Copyright 2023 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * A tracker to keep a record of important details during database local query + * execution. + */ +export class QueryContext { + /** + * Counts the number of documents passed through during local query execution. + */ + private _documentReadCount = 0; + + get documentReadCount(): number { + return this._documentReadCount; + } + + incrementDocumentReadCount(amount?: number): void { + if (amount === undefined) { + amount = 1; + } + this._documentReadCount += amount; + } +} diff --git a/packages/firestore/src/local/query_engine.ts b/packages/firestore/src/local/query_engine.ts index 7728e9d7aa7..da1567b8f40 100644 --- a/packages/firestore/src/local/query_engine.ts +++ b/packages/firestore/src/local/query_engine.ts @@ -46,6 +46,18 @@ import { IndexManager, IndexType } from './index_manager'; import { LocalDocumentsView } from './local_documents_view'; import { PersistencePromise } from './persistence_promise'; import { PersistenceTransaction } from './persistence_transaction'; +import { QueryContext } from './query_context'; + +const DEFAULT_INDEX_AUTO_CREATION_MIN_COLLECTION_SIZE = 100; + +/** + * This cost represents the evaluation result of + * (([index, docKey] + [docKey, docContent]) per document in the result set) + * / ([docKey, docContent] per documents in full collection scan) coming from + * experiment [enter PR experiment URL here]. + * TODO: Enter PR experiment URL above. + */ +const DEFAULT_RELATIVE_INDEX_READ_COST_PER_DOCUMENT = 2; /** * The Firestore query engine. @@ -90,6 +102,18 @@ export class QueryEngine { private indexManager!: IndexManager; private initialized = false; + indexAutoCreationEnabled = false; + + /** + * SDK only decides whether it should create index when collection size is + * larger than this. + */ + indexAutoCreationMinCollectionSize = + DEFAULT_INDEX_AUTO_CREATION_MIN_COLLECTION_SIZE; + + relativeIndexReadCostPerDocument = + DEFAULT_RELATIVE_INDEX_READ_COST_PER_DOCUMENT; + /** Sets the document view to query against. */ initialize( localDocuments: LocalDocumentsView, @@ -109,6 +133,7 @@ export class QueryEngine { ): PersistencePromise { debugAssert(this.initialized, 'initialize() not called'); + const queryContext = new QueryContext(); return this.performQueryUsingIndex(transaction, query) .next(result => result @@ -121,8 +146,83 @@ export class QueryEngine { ) ) .next(result => - result ? result : this.executeFullCollectionScan(transaction, query) + result + ? result + : this.executeFullCollectionScan(transaction, query, queryContext) + ) + .next(result => + result && this.indexAutoCreationEnabled + ? this.createCacheIndexes( + transaction, + query, + queryContext, + result.size + ).next(_ => result) + : result + ); + } + + createCacheIndexes( + transaction: PersistenceTransaction, + query: Query, + context: QueryContext, + resultSize: number + ): PersistencePromise { + if (context.documentReadCount < this.indexAutoCreationMinCollectionSize) { + if (getLogLevel() <= LogLevel.DEBUG) { + logDebug( + 'QueryEngine', + 'SDK will not create cache indexes for query:', + stringifyQuery(query), + 'since it only creates cache indexes for collection contains', + 'more than or equal to', + this.indexAutoCreationMinCollectionSize, + 'documents' + ); + } + return PersistencePromise.resolve(); + } + + if (getLogLevel() <= LogLevel.DEBUG) { + logDebug( + 'QueryEngine', + 'Query:', + stringifyQuery(query), + 'scans', + context.documentReadCount, + 'local documents and returns', + resultSize, + 'documents as results.' + ); + } + + if ( + context.documentReadCount > + this.relativeIndexReadCostPerDocument * resultSize + ) { + if (getLogLevel() <= LogLevel.DEBUG) { + logDebug( + 'QueryEngine', + 'The SDK decides to create cache indexes for query:', + stringifyQuery(query), + 'as using cache indexes may help improve performance.' + ); + } + return this.indexManager.createTargetIndexes( + transaction, + queryToTarget(query) ); + } else { + if (getLogLevel() <= LogLevel.DEBUG) { + logDebug( + 'QueryEngine', + 'The SDK decides not to create cache indexes for query:', + stringifyQuery(query), + 'as using cache indexes may not help improve performance.' + ); + } + return PersistencePromise.resolve(); + } } /** @@ -221,18 +321,18 @@ export class QueryEngine { query: Query, remoteKeys: DocumentKeySet, lastLimboFreeSnapshotVersion: SnapshotVersion - ): PersistencePromise { + ): PersistencePromise { 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 // collection, rather than to perform individual lookups. - return this.executeFullCollectionScan(transaction, query); + return PersistencePromise.resolve(null); } // Queries that have never seen a snapshot without limbo free documents // should also be run as a full collection scan. if (lastLimboFreeSnapshotVersion.isEqual(SnapshotVersion.min())) { - return this.executeFullCollectionScan(transaction, query); + return PersistencePromise.resolve(null); } return this.localDocumentsView!.getDocuments(transaction, remoteKeys).next( @@ -247,7 +347,7 @@ export class QueryEngine { lastLimboFreeSnapshotVersion ) ) { - return this.executeFullCollectionScan(transaction, query); + return PersistencePromise.resolve(null); } if (getLogLevel() <= LogLevel.DEBUG) { @@ -269,7 +369,7 @@ export class QueryEngine { lastLimboFreeSnapshotVersion, INITIAL_LARGEST_BATCH_ID ) - ); + ).next(results => results); } ); } @@ -343,7 +443,8 @@ export class QueryEngine { private executeFullCollectionScan( transaction: PersistenceTransaction, - query: Query + query: Query, + context: QueryContext ): PersistencePromise { if (getLogLevel() <= LogLevel.DEBUG) { logDebug( @@ -356,7 +457,8 @@ export class QueryEngine { return this.localDocumentsView!.getDocumentsMatchingQuery( transaction, query, - IndexOffset.min() + IndexOffset.min(), + context ); } diff --git a/packages/firestore/src/local/remote_document_cache.ts b/packages/firestore/src/local/remote_document_cache.ts index a66f1b4a253..15fcecdc836 100644 --- a/packages/firestore/src/local/remote_document_cache.ts +++ b/packages/firestore/src/local/remote_document_cache.ts @@ -28,6 +28,7 @@ import { IndexOffset } from '../model/field_index'; import { IndexManager } from './index_manager'; import { PersistencePromise } from './persistence_promise'; import { PersistenceTransaction } from './persistence_transaction'; +import { QueryContext } from './query_context'; import { RemoteDocumentChangeBuffer } from './remote_document_change_buffer'; /** @@ -70,13 +71,16 @@ export interface RemoteDocumentCache { * * @param query - The query to match documents against. * @param offset - The offset to start the scan at (exclusive). + * @param context - A optional tracker to keep a record of important details + * during database local query execution. * @returns The set of matching documents. */ getDocumentsMatchingQuery( transaction: PersistenceTransaction, query: Query, offset: IndexOffset, - mutatedDocs: OverlayMap + mutatedDocs: OverlayMap, + context?: QueryContext ): PersistencePromise; /** diff --git a/packages/firestore/src/model/target_index_matcher.ts b/packages/firestore/src/model/target_index_matcher.ts index d3bd95d473c..431503cf01b 100644 --- a/packages/firestore/src/model/target_index_matcher.ts +++ b/packages/firestore/src/model/target_index_matcher.ts @@ -19,14 +19,17 @@ import { FieldFilter, Operator } from '../core/filter'; import { Direction, OrderBy } from '../core/order_by'; import { Target } from '../core/target'; import { debugAssert, hardAssert } from '../util/assert'; +import { SortedSet } from '../util/sorted_set'; import { FieldIndex, fieldIndexGetArraySegment, fieldIndexGetDirectionalSegments, IndexKind, - IndexSegment + IndexSegment, + IndexState } from './field_index'; +import { FieldPath } from './path'; /** * A light query planner for Firestore. @@ -179,6 +182,69 @@ export class TargetIndexMatcher { return true; } + /** Returns a full matched field index for this target. */ + buildTargetIndex(): FieldIndex { + // We want to make sure only one segment created for one field. For example, + // in case like a == 3 and a > 2, index, a ASCENDING, will only be created + // once. + let uniqueFields = new SortedSet(FieldPath.comparator); + const segments: IndexSegment[] = []; + + for (const filter of this.equalityFilters) { + if (filter.field.isKeyField()) { + continue; + } + const isArrayOperator = + filter.op === Operator.ARRAY_CONTAINS || + filter.op === Operator.ARRAY_CONTAINS_ANY; + if (isArrayOperator) { + segments.push(new IndexSegment(filter.field, IndexKind.CONTAINS)); + } else { + if (uniqueFields.has(filter.field)) { + continue; + } + uniqueFields = uniqueFields.add(filter.field); + segments.push(new IndexSegment(filter.field, IndexKind.ASCENDING)); + } + } + + // Note: We do not explicitly check `this.inequalityFilter` but rather rely + // on the target defining an appropriate "order by" to ensure that the + // required index segment is added. The query engine would reject a query + // with an inequality filter that lacks the required order-by clause. + for (const orderBy of this.orderBys) { + // Stop adding more segments if we see a order-by on key. Typically this + // is the default implicit order-by which is covered in the index_entry + // table as a separate column. If it is not the default order-by, the + // generated index will be missing some segments optimized for order-bys, + // which is probably fine. + if (orderBy.field.isKeyField()) { + continue; + } + + if (uniqueFields.has(orderBy.field)) { + continue; + } + uniqueFields = uniqueFields.add(orderBy.field); + + segments.push( + new IndexSegment( + orderBy.field, + orderBy.dir === Direction.ASCENDING + ? IndexKind.ASCENDING + : IndexKind.DESCENDING + ) + ); + } + + return new FieldIndex( + FieldIndex.UNKNOWN_ID, + this.collectionId, + segments, + IndexState.empty() + ); + } + private hasMatchingEqualityFilter(segment: IndexSegment): boolean { for (const filter of this.equalityFilters) { if (this.matchesFilter(filter, segment)) { diff --git a/packages/firestore/src/util/async_queue.ts b/packages/firestore/src/util/async_queue.ts index bffd4433a39..c430320a4c4 100644 --- a/packages/firestore/src/util/async_queue.ts +++ b/packages/firestore/src/util/async_queue.ts @@ -115,6 +115,10 @@ export class DelayedOperation implements PromiseLike { this.deferred.promise.catch(err => {}); } + get promise(): Promise { + return this.deferred.promise; + } + /** * Creates and returns a DelayedOperation that has been scheduled to be * executed on the provided asyncQueue after the provided delayMs. diff --git a/packages/firestore/src/util/testing_hooks.ts b/packages/firestore/src/util/testing_hooks.ts index abce89ac1c0..2e48e7b7c00 100644 --- a/packages/firestore/src/util/testing_hooks.ts +++ b/packages/firestore/src/util/testing_hooks.ts @@ -15,6 +15,13 @@ * limitations under the License. */ +import { ensureFirestoreConfigured, Firestore } from '../api/database'; +import { TestingHooks as FirestoreClientTestingHooks } from '../core/firestore_client'; +import { Query } from '../lite-api/reference'; +import { IndexType } from '../local/index_manager'; + +import { cast } from './input_validation'; + /** * Manages "testing hooks", hooks into the internals of the SDK to verify * internal state and events during integration tests. Do not use this class @@ -37,6 +44,11 @@ export class TestingHooks { ExistenceFilterMismatchCallback >(); + private readonly onTogglePersistentCacheIndexAutoCreationCallbacks = new Map< + Symbol, + TogglePersistentCacheIndexAutoCreationCallback + >(); + private constructor() {} /** @@ -87,6 +99,75 @@ export class TestingHooks { notifyOnExistenceFilterMismatch(info: ExistenceFilterMismatchInfo): void { this.onExistenceFilterMismatchCallbacks.forEach(callback => callback(info)); } + + /** + * Registers a callback to be notified when + * `enablePersistentCacheIndexAutoCreation()` or + * `disablePersistentCacheIndexAutoCreation()` is invoked. + * + * The relative order in which callbacks are notified is unspecified; do not + * rely on any particular ordering. If a given callback is registered multiple + * times then it will be notified multiple times, once per registration. + * + * @param callback the callback to invoke when + * `enablePersistentCacheIndexAutoCreation()` or + * `disablePersistentCacheIndexAutoCreation()` is invoked. + * + * @return a function that, when called, unregisters the given callback; only + * the first invocation of the returned function does anything; all subsequent + * invocations do nothing. + */ + onTogglePersistentCacheIndexAutoCreation( + callback: TogglePersistentCacheIndexAutoCreationCallback + ): () => void { + const key = Symbol(); + this.onTogglePersistentCacheIndexAutoCreationCallbacks.set(key, callback); + return () => + this.onTogglePersistentCacheIndexAutoCreationCallbacks.delete(key); + } + + /** + * Invokes all currently-registered + * `onTogglePersistentCacheIndexAutoCreation` callbacks. + * @param promise The argument to specify to the callback. + */ + notifyPersistentCacheIndexAutoCreationToggled(promise: Promise): void { + this.onTogglePersistentCacheIndexAutoCreationCallbacks.forEach(callback => + callback(promise) + ); + } + + /** + * Determines the type of client-side index that will be used when executing the + * given query against the local cache. + * + * @param query The query whose client-side index type to get; it is typed as + * `unknown` so that it is usable in the minified, bundled code, but it should + * be a `Query` object. + */ + async getQueryIndexType( + query: unknown + ): Promise<'full' | 'partial' | 'none'> { + const query_ = cast(query as Query, Query); + const firestore = cast(query_.firestore, Firestore); + const client = ensureFirestoreConfigured(firestore); + const firestoreClientTestingHooks = new FirestoreClientTestingHooks(client); + + const indexType = await firestoreClientTestingHooks.getQueryIndexType( + query_._query + ); + + switch (indexType) { + case IndexType.NONE: + return 'none'; + case IndexType.PARTIAL: + return 'partial'; + case IndexType.FULL: + return 'full'; + default: + throw new Error(`unrecognized IndexType: ${indexType}`); + } + } } /** @@ -154,5 +235,18 @@ export type ExistenceFilterMismatchCallback = ( info: ExistenceFilterMismatchInfo ) => void; +/** + * The signature of callbacks registered with + * `TestingUtils.onTogglePersistentCacheIndexAutoCreation()`. + * + * The `promise` argument will be fulfilled when the asynchronous work started + * by the call to `enablePersistentCacheIndexAutoCreation()` or + * `disablePersistentCacheIndexAutoCreation()` completes successfully, or will + * be rejected if it fails. + */ +export type TogglePersistentCacheIndexAutoCreationCallback = ( + promise: Promise +) => void; + /** The global singleton instance of `TestingHooks`. */ let gTestingHooksSingletonInstance: TestingHooks | null = null; diff --git a/packages/firestore/test/integration/api/persistent_cache_index_manager.test.ts b/packages/firestore/test/integration/api/persistent_cache_index_manager.test.ts new file mode 100644 index 00000000000..32e4666cb99 --- /dev/null +++ b/packages/firestore/test/integration/api/persistent_cache_index_manager.test.ts @@ -0,0 +1,365 @@ +/** + * @license + * Copyright 2023 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { expect } from 'chai'; + +import { + getDocs, + getDocsFromCache, + query, + terminate, + where, + _disablePersistentCacheIndexAutoCreation as disablePersistentCacheIndexAutoCreation, + _enablePersistentCacheIndexAutoCreation as enablePersistentCacheIndexAutoCreation, + _getPersistentCacheIndexManager as getPersistentCacheIndexManager, + _PersistentCacheIndexManager as PersistentCacheIndexManager +} from '../util/firebase_export'; +import { + apiDescribe, + partitionedTestDocs, + withTestCollection, + withTestDb +} from '../util/helpers'; +import { + getQueryIndexType, + verifyPersistentCacheIndexAutoCreationToggleSucceedsDuring +} from '../util/testing_hooks_util'; + +apiDescribe('PersistentCacheIndexManager', persistence => { + describe('getPersistentCacheIndexManager()', () => { + it('should return non-null if, and only if, IndexedDB persistence is enabled', () => + withTestDb(persistence, async db => { + const indexManager = getPersistentCacheIndexManager(db); + if (persistence.storage === 'indexeddb') { + expect(indexManager).to.be.instanceof(PersistentCacheIndexManager); + } else { + expect(indexManager).to.be.null; + } + })); + + it('should always return the same object', () => + withTestDb(persistence, async db => { + const indexManager1 = getPersistentCacheIndexManager(db); + const indexManager2 = getPersistentCacheIndexManager(db); + expect(indexManager1).to.equal(indexManager2); + })); + + it('should fail if invoked after terminate()', () => + withTestDb(persistence, async db => { + terminate(db).catch(e => expect.fail(`terminate() failed: ${e}`)); + expect(() => getPersistentCacheIndexManager(db)).to.throw( + 'The client has already been terminated.' + ); + })); + }); + + // Skip the rest of the tests since they require `PersistentCacheIndexManager` + // support, which is only available with indexeddb persistence. + if (persistence.storage !== 'indexeddb') { + return; + } + + describe('enablePersistentCacheIndexAutoCreation()', () => { + it('should return successfully', () => + withTestDb(persistence, async db => { + const indexManager = getPersistentCacheIndexManager(db)!; + enablePersistentCacheIndexAutoCreation(indexManager); + })); + + it('should successfully enable indexing when not yet enabled', () => + withTestDb(persistence, db => { + const indexManager = getPersistentCacheIndexManager(db)!; + return verifyPersistentCacheIndexAutoCreationToggleSucceedsDuring(() => + enablePersistentCacheIndexAutoCreation(indexManager) + ); + })); + + it('should successfully enable indexing when already enabled', () => + withTestDb(persistence, db => { + const indexManager = getPersistentCacheIndexManager(db)!; + return verifyPersistentCacheIndexAutoCreationToggleSucceedsDuring(() => + enablePersistentCacheIndexAutoCreation(indexManager) + ); + })); + + it('should successfully enable indexing after being disabled', () => + withTestDb(persistence, db => { + const indexManager = getPersistentCacheIndexManager(db)!; + enablePersistentCacheIndexAutoCreation(indexManager); + disablePersistentCacheIndexAutoCreation(indexManager); + return verifyPersistentCacheIndexAutoCreationToggleSucceedsDuring(() => + enablePersistentCacheIndexAutoCreation(indexManager) + ); + })); + + it('should fail if invoked after terminate()', () => + withTestDb(persistence, async db => { + const indexManager = getPersistentCacheIndexManager(db)!; + terminate(db).catch(e => expect.fail(`terminate() failed: ${e}`)); + expect(() => + enablePersistentCacheIndexAutoCreation(indexManager) + ).to.throw('The client has already been terminated.'); + })); + }); + + describe('disablePersistentCacheIndexAutoCreation(()', () => { + it('should return successfully', () => + withTestDb(persistence, async db => { + const indexManager = getPersistentCacheIndexManager(db)!; + disablePersistentCacheIndexAutoCreation(indexManager); + })); + + it('should successfully disable indexing when not yet enabled', () => + withTestDb(persistence, db => { + const indexManager = getPersistentCacheIndexManager(db)!; + return verifyPersistentCacheIndexAutoCreationToggleSucceedsDuring(() => + disablePersistentCacheIndexAutoCreation(indexManager) + ); + })); + + it('should successfully disable indexing when enabled', () => + withTestDb(persistence, db => { + const indexManager = getPersistentCacheIndexManager(db)!; + enablePersistentCacheIndexAutoCreation(indexManager); + return verifyPersistentCacheIndexAutoCreationToggleSucceedsDuring(() => + disablePersistentCacheIndexAutoCreation(indexManager) + ); + })); + + it('should successfully enable indexing after being disabled', () => + withTestDb(persistence, db => { + const indexManager = getPersistentCacheIndexManager(db)!; + enablePersistentCacheIndexAutoCreation(indexManager); + disablePersistentCacheIndexAutoCreation(indexManager); + return verifyPersistentCacheIndexAutoCreationToggleSucceedsDuring(() => + disablePersistentCacheIndexAutoCreation(indexManager) + ); + })); + + it('should fail if invoked after terminate()', () => + withTestDb(persistence, async db => { + const indexManager = getPersistentCacheIndexManager(db)!; + terminate(db).catch(e => expect.fail(`terminate() failed: ${e}`)); + expect(() => + disablePersistentCacheIndexAutoCreation(indexManager) + ).to.throw('The client has already been terminated.'); + })); + }); + + describe('Query execution', () => { + it('Indexing is disabled by default', () => + testIndexesGetAutoCreated({ + documentCounts: { matching: 1, notMatching: 100 }, + expectedIndexAutoCreated: false, + indexAutoCreationEnabled: false + })); + + it('Index is not auto-created if collection size is less than 100', () => + testIndexesGetAutoCreated({ + documentCounts: { matching: 1, notMatching: 98 }, + expectedIndexAutoCreated: false + })); + + it('Index is not auto-created if 50% of documents match', () => + testIndexesGetAutoCreated({ + documentCounts: { matching: 50, notMatching: 50 }, + expectedIndexAutoCreated: false + })); + + it('Index is auto-created if 49% of documents match and collection size is 100', () => + testIndexesGetAutoCreated({ + documentCounts: { matching: 49, notMatching: 51 }, + expectedIndexAutoCreated: true + })); + + it('Indexes are only auto-created when enabled', async () => { + const testDocs = partitionedTestDocs({ + FooMatches: { + documentData: { foo: 'match' }, + documentCount: 11 + }, + BarMatches: { + documentData: { bar: 'match' }, + documentCount: 22 + }, + BazMatches: { + documentData: { baz: 'match' }, + documentCount: 33 + }, + NeitherFooNorBarMatch: { + documentData: { foo: 'nomatch', bar: 'nomatch' }, + documentCount: 100 + } + }); + + return withTestCollection(persistence, testDocs, async (coll, db) => { + const indexManager = getPersistentCacheIndexManager(db)!; + expect(indexManager, 'indexManager').is.not.null; + + // Populate the local cache with the entire collection. + await getDocs(coll); + + // Enable automatic index creation and run a query, ensuring that an + // appropriate index is created. + enablePersistentCacheIndexAutoCreation(indexManager); + const query1 = query(coll, where('foo', '==', 'match')); + const snapshot1 = await getDocsFromCache(query1); + expect(snapshot1.size, 'snapshot1.size').to.equal(11); + expect( + await getQueryIndexType(query1), + 'getQueryIndexType(query1)' + ).to.equal('full'); + + // Disable automatic index creation and run a query, ensuring that an + // appropriate index is _not_ created, and the other is maintained. + disablePersistentCacheIndexAutoCreation(indexManager); + const query2 = query(coll, where('bar', '==', 'match')); + const snapshot2 = await getDocsFromCache(query2); + expect(snapshot2.size, 'snapshot2.size').to.equal(22); + expect( + await getQueryIndexType(query2), + 'getQueryIndexType(query2)' + ).to.equal('none'); + expect( + await getQueryIndexType(query1), + 'getQueryIndexType(query1) check #2' + ).to.equal('full'); + + // Re-enable automatic index creation and run a query, ensuring that an + // appropriate index is created, and others are maintained. + enablePersistentCacheIndexAutoCreation(indexManager); + const query3 = query(coll, where('baz', '==', 'match')); + const snapshot3 = await getDocsFromCache(query3); + expect(snapshot3.size, 'snapshot3.size').to.equal(33); + expect( + await getQueryIndexType(query3), + 'getQueryIndexType(query3)' + ).to.equal('full'); + expect( + await getQueryIndexType(query2), + 'getQueryIndexType(query2) check #2' + ).to.equal('none'); + expect( + await getQueryIndexType(query1), + 'getQueryIndexType(query1) check #3' + ).to.equal('full'); + }); + }); + + it('Indexes are NOT upgraded from partial to full', async () => { + const testDocs = partitionedTestDocs({ + FooMatches: { + documentData: { foo: 'match' }, + documentCount: 99 + }, + FooBarMatches: { + documentData: { foo: 'match', bar: 'match' }, + documentCount: 1 + }, + NeitherFooNorBarMatch: { + documentData: { foo: 'nomatch', bar: 'nomatch' }, + documentCount: 110 + } + }); + + return withTestCollection(persistence, testDocs, async (coll, db) => { + const indexManager = getPersistentCacheIndexManager(db)!; + expect(indexManager, 'indexManager').is.not.null; + enablePersistentCacheIndexAutoCreation(indexManager); + + // Populate the local cache with the entire collection. + await getDocs(coll); + + // Run a query to have an index on the 'foo' field created. + { + const fooQuery = query(coll, where('foo', '==', 'match')); + const fooSnapshot = await getDocsFromCache(fooQuery); + expect(fooSnapshot.size, 'fooSnapshot.size').to.equal(100); + expect( + await getQueryIndexType(fooQuery), + 'getQueryIndexType(fooQuery)' + ).to.equal('full'); + } + + // Run a query that filters on both 'foo' and 'bar' fields and ensure + // that the partial index (created by the previous query's execution) + // is NOT upgraded to a full index. Note that in the future we _may_ + // change this behavior since a full index would likely be beneficial to + // the query's execution performance. + { + const fooBarQuery = query( + coll, + where('foo', '==', 'match'), + where('bar', '==', 'match') + ); + expect( + await getQueryIndexType(fooBarQuery), + 'getQueryIndexType(fooBarQuery) before' + ).to.equal('partial'); + const fooBarSnapshot = await getDocsFromCache(fooBarQuery); + expect(fooBarSnapshot.size, 'fooBarSnapshot.size').to.equal(1); + expect( + await getQueryIndexType(fooBarQuery), + 'getQueryIndexType(fooBarQuery) after' + ).to.equal('partial'); + } + }); + }); + + async function testIndexesGetAutoCreated(config: { + documentCounts: { matching: number; notMatching: number }; + expectedIndexAutoCreated: boolean; + indexAutoCreationEnabled?: boolean; + }): Promise { + const testDocs = partitionedTestDocs({ + matching: { + documentData: { foo: 'match' }, + documentCount: config.documentCounts.matching + }, + notMatching: { + documentData: { foo: 'nomatch' }, + documentCount: config.documentCounts.notMatching + } + }); + + return withTestCollection(persistence, testDocs, async (coll, db) => { + // Populate the local cache with the entire collection. + await getDocs(coll); + + // Enable or disable automatic index creation, as requested. + if (config.indexAutoCreationEnabled ?? true) { + const indexManager = getPersistentCacheIndexManager(db)!; + expect(indexManager, 'indexManager').is.not.null; + enablePersistentCacheIndexAutoCreation(indexManager); + } + + // Run a query against the local cache that matches a _subset_ of the + // entire collection. + const query_ = query(coll, where('foo', '==', 'match')); + const snapshot = await getDocsFromCache(query_); + expect(snapshot.size, 'snapshot.size').to.equal( + config.documentCounts.matching + ); + + // Verify that an index was or was not created, as expected. + expect(await getQueryIndexType(query_), 'getQueryIndexType()').to.equal( + config.expectedIndexAutoCreated ? 'full' : 'none' + ); + }); + } + }); +}); diff --git a/packages/firestore/test/integration/util/helpers.ts b/packages/firestore/test/integration/util/helpers.ts index 3b27e5e7c47..9f97ec6bf79 100644 --- a/packages/firestore/test/integration/util/helpers.ts +++ b/packages/firestore/test/integration/util/helpers.ts @@ -483,3 +483,37 @@ export function withTestCollectionSettings( } ); } + +/** + * Creates a `docs` argument suitable for specifying to `withTestCollection()` + * that defines subsets of documents with different document data. + * + * This can be useful for pre-populating a collection with some documents that + * match a query and others that do _not_ match that query. + * + * Each key of the given `partitions` object will be considered a partition + * "name". The returned object will specify `documentCount` documents with the + * `documentData` whose document IDs are prefixed with the partition "name". + */ +export function partitionedTestDocs(partitions: { + [partitionName: string]: { + documentData: DocumentData; + documentCount: number; + }; +}): { [key: string]: DocumentData } { + const testDocs: { [key: string]: DocumentData } = {}; + + for (const partitionName in partitions) { + // Make lint happy (see https://eslint.org/docs/latest/rules/guard-for-in). + if (!Object.prototype.hasOwnProperty.call(partitions, partitionName)) { + continue; + } + const partition = partitions[partitionName]; + for (let i = 0; i < partition.documentCount; i++) { + const documentId = `${partitionName}_${`${i}`.padStart(4, '0')}`; + testDocs[documentId] = partition.documentData; + } + } + + return testDocs; +} diff --git a/packages/firestore/test/integration/util/testing_hooks_util.ts b/packages/firestore/test/integration/util/testing_hooks_util.ts index 3dec7d379d8..edda6f3219c 100644 --- a/packages/firestore/test/integration/util/testing_hooks_util.ts +++ b/packages/firestore/test/integration/util/testing_hooks_util.ts @@ -15,8 +15,12 @@ * limitations under the License. */ +import { expect } from 'chai'; + import { + DocumentData, DocumentReference, + Query, _TestingHooks as TestingHooks } from './firebase_export'; @@ -122,3 +126,61 @@ function createExistenceFilterMismatchInfoFrom( return info; } + +/** + * Verifies than an invocation of `enablePersistentCacheIndexAutoCreation()` or + * `disablePersistentCacheIndexAutoCreation()` made during the execution of the + * given callback succeeds. + * + * @param callback The callback to invoke; this callback must invoke exactly one + * of `enablePersistentCacheIndexAutoCreation()` or + * `disablePersistentCacheIndexAutoCreation()` exactly once; this callback is + * called synchronously by this function, and is called exactly once. + * + * @return a promise that is fulfilled when the asynchronous work started by + * `enablePersistentCacheIndexAutoCreation()` or + * `disablePersistentCacheIndexAutoCreation()` completes successfully, or is + * rejected + */ +export function verifyPersistentCacheIndexAutoCreationToggleSucceedsDuring( + callback: () => void +): Promise { + const promises: Array> = []; + const onTogglePersistentCacheIndexAutoCreationCallback = ( + promise: Promise + ): void => { + promises.push(promise); + }; + + const unregister = + TestingHooks.getOrCreateInstance().onTogglePersistentCacheIndexAutoCreation( + onTogglePersistentCacheIndexAutoCreationCallback + ); + + try { + callback(); + } finally { + unregister(); + } + + expect( + promises, + 'exactly one invocation of enablePersistentCacheIndexAutoCreation() or ' + + 'disablePersistentCacheIndexAutoCreation() should be made by the callback' + ).to.have.length(1); + + return promises[0]; +} + +/** + * Determines the type of client-side index that will be used when executing the + * given query against the local cache. + */ +export function getQueryIndexType< + AppModelType, + DbModelType extends DocumentData +>( + query: Query +): Promise<'full' | 'partial' | 'none'> { + return TestingHooks.getOrCreateInstance().getQueryIndexType(query); +} diff --git a/packages/firestore/test/unit/local/counting_query_engine.ts b/packages/firestore/test/unit/local/counting_query_engine.ts index d407abfd60a..deaef12a829 100644 --- a/packages/firestore/test/unit/local/counting_query_engine.ts +++ b/packages/firestore/test/unit/local/counting_query_engine.ts @@ -105,14 +105,16 @@ export class CountingQueryEngine extends QueryEngine { transaction, query, sinceReadTime, - overlays + overlays, + context ) => { return subject .getDocumentsMatchingQuery( transaction, query, sinceReadTime, - overlays + overlays, + context ) .next(result => { this.documentsReadByCollection += result.size; diff --git a/packages/firestore/test/unit/local/index_manager.test.ts b/packages/firestore/test/unit/local/index_manager.test.ts index f256312399d..1385064e4a0 100644 --- a/packages/firestore/test/unit/local/index_manager.test.ts +++ b/packages/firestore/test/unit/local/index_manager.test.ts @@ -30,7 +30,10 @@ import { queryWithLimit, queryWithStartAt } from '../../../src/core/query'; -import { IndexType } from '../../../src/local/index_manager'; +import { + displayNameForIndexType, + IndexType +} from '../../../src/local/index_manager'; import { IndexedDbPersistence } from '../../../src/local/indexeddb_persistence'; import { Persistence } from '../../../src/local/persistence'; import { documentMap } from '../../../src/model/collections'; @@ -51,6 +54,7 @@ import { filter, key, orderBy, + orFilter, path, query, version, @@ -60,6 +64,26 @@ import { import * as persistenceHelpers from './persistence_test_helpers'; import { TestIndexManager } from './test_index_manager'; +describe('index_manager.ts top-level functions', () => { + describe('displayNameForIndexType()', () => { + it('IndexType.NONE', () => + expect(displayNameForIndexType(IndexType.NONE)).to.equal('NONE')); + + it('IndexType.FULL', () => + expect(displayNameForIndexType(IndexType.FULL)).to.equal('FULL')); + + it('IndexType.PARTIAL', () => + expect(displayNameForIndexType(IndexType.PARTIAL)).to.equal('PARTIAL')); + + it('invalid IndexType', () => + // @ts-expect-error: specifying a string to displayNameForIndexType() + // causes a TypeScript compiler error, but is handled gracefully. + expect(displayNameForIndexType('zzyzx')).to.equal( + '[unknown IndexType: zzyzx]' + )); + }); +}); + describe('MemoryIndexManager', async () => { genericIndexManagerTests(persistenceHelpers.testMemoryEagerPersistence); }); @@ -1660,14 +1684,80 @@ describe('IndexedDbIndexManager', async () => { await validateIsFullIndex(query15); }); + it('createTargetIndexes() creates full indexes for each sub-target', async () => { + const query_ = queryWithAddedFilter( + query('coll'), + orFilter(filter('a', '==', 1), filter('b', '==', 2), filter('c', '==', 3)) + ); + const subQuery1 = queryWithAddedFilter(query('coll'), filter('a', '==', 1)); + const subQuery2 = queryWithAddedFilter(query('coll'), filter('b', '==', 2)); + const subQuery3 = queryWithAddedFilter(query('coll'), filter('c', '==', 3)); + await validateIsNoneIndex(query_); + await validateIsNoneIndex(subQuery1); + await validateIsNoneIndex(subQuery2); + await validateIsNoneIndex(subQuery3); + + await indexManager.createTargetIndexes(queryToTarget(query_)); + + await validateIsFullIndex(query_); + await validateIsFullIndex(subQuery1); + await validateIsFullIndex(subQuery2); + await validateIsFullIndex(subQuery3); + }); + + it('createTargetIndexes() upgrades a partial index to a full index', async () => { + const query_ = queryWithAddedFilter( + queryWithAddedFilter(query('coll'), filter('a', '==', 1)), + filter('b', '==', 2) + ); + const subQuery1 = queryWithAddedFilter(query('coll'), filter('a', '==', 1)); + const subQuery2 = queryWithAddedFilter(query('coll'), filter('b', '==', 2)); + await indexManager.createTargetIndexes(queryToTarget(subQuery1)); + await validateIsPartialIndex(query_); + await validateIsFullIndex(subQuery1); + await validateIsNoneIndex(subQuery2); + + await indexManager.createTargetIndexes(queryToTarget(query_)); + + await validateIsFullIndex(query_); + await validateIsFullIndex(subQuery1); + await validateIsNoneIndex(subQuery2); + }); + + it('createTargetIndexes() does nothing if a full index already exists', async () => { + const query_ = query('coll'); + await indexManager.createTargetIndexes(queryToTarget(query_)); + await validateIsFullIndex(query_); + + await indexManager.createTargetIndexes(queryToTarget(query_)); + + await validateIsFullIndex(query_); + }); + async function validateIsPartialIndex(query: Query): Promise { - const indexType = await indexManager.getIndexType(queryToTarget(query)); - expect(indexType).to.equal(IndexType.PARTIAL); + await validateIndexType(query, IndexType.PARTIAL); } async function validateIsFullIndex(query: Query): Promise { + await validateIndexType(query, IndexType.FULL); + } + + async function validateIsNoneIndex(query: Query): Promise { + await validateIndexType(query, IndexType.NONE); + } + + async function validateIndexType( + query: Query, + expectedIndexType: IndexType + ): Promise { const indexType = await indexManager.getIndexType(queryToTarget(query)); - expect(indexType).to.equal(IndexType.FULL); + expect( + indexType, + 'index type is ' + + displayNameForIndexType(indexType) + + ' but expected ' + + displayNameForIndexType(expectedIndexType) + ).to.equal(expectedIndexType); } async function setUpSingleValueFilter(): Promise { diff --git a/packages/firestore/test/unit/local/local_store.test.ts b/packages/firestore/test/unit/local/local_store.test.ts index 1c388c69776..e5c7f66a759 100644 --- a/packages/firestore/test/unit/local/local_store.test.ts +++ b/packages/firestore/test/unit/local/local_store.test.ts @@ -31,6 +31,7 @@ import { import { SnapshotVersion } from '../../../src/core/snapshot_version'; import { Target } from '../../../src/core/target'; import { BatchId, TargetId } from '../../../src/core/types'; +import { IndexBackfiller } from '../../../src/local/index_backfiller'; import { IndexedDbPersistence } from '../../../src/local/indexeddb_persistence'; import { LocalStore } from '../../../src/local/local_store'; import { @@ -42,6 +43,7 @@ import { localStoreGetHighestUnacknowledgedBatchId, localStoreGetTargetData, localStoreGetNamedQuery, + localStoreSetIndexAutoCreationEnabled, localStoreHasNewerBundle, localStoreWriteLocally, LocalWriteResult, @@ -51,7 +53,8 @@ import { localStoreReleaseTarget, localStoreSaveBundle, localStoreSaveNamedQuery, - newLocalStore + newLocalStore, + TestingHooks as LocalStoreTestingHooks } from '../../../src/local/local_store_impl'; import { LocalViewChanges } from '../../../src/local/local_view_changes'; import { Persistence } from '../../../src/local/persistence'; @@ -134,6 +137,9 @@ class LocalStoreTester { private lastTargetId: TargetId | null = null; private batches: MutationBatch[] = []; private bundleConverter: BundleConverterImpl; + private indexBackfiller: IndexBackfiller; + + private queryExecutionCount = 0; constructor( public localStore: LocalStore, @@ -142,6 +148,7 @@ class LocalStoreTester { readonly gcIsEager: boolean ) { this.bundleConverter = new BundleConverterImpl(JSON_SERIALIZER); + this.indexBackfiller = new IndexBackfiller(localStore, persistence); } private prepareNextStep(): void { @@ -176,6 +183,10 @@ class LocalStoreTester { } } + afterMutation(mutation: Mutation): LocalStoreTester { + return this.afterMutations([mutation]); + } + afterMutations(mutations: Mutation[]): LocalStoreTester { this.prepareNextStep(); @@ -203,6 +214,11 @@ class LocalStoreTester { return this; } + afterRemoteEvents(remoteEvents: RemoteEvent[]): LocalStoreTester { + remoteEvents.forEach(remoteEvent => this.afterRemoteEvent(remoteEvent)); + return this; + } + afterBundleDocuments( documents: BundledDocuments, bundleName?: string @@ -321,12 +337,54 @@ class LocalStoreTester { query, /* usePreviousResults= */ true ).then(({ documents }) => { + this.queryExecutionCount++; this.lastChanges = documents; }) ); return this; } + afterIndexAutoCreationConfigure(config: { + enabled?: boolean; + autoCreationMinCollectionSize?: number; + relativeIndexReadCostPerDocument?: number; + }): LocalStoreTester { + this.prepareNextStep(); + + this.promiseChain = this.promiseChain.then(() => { + if (config.enabled !== undefined) { + localStoreSetIndexAutoCreationEnabled(this.localStore, config.enabled); + } + const testingHooks = new LocalStoreTestingHooks(this.localStore); + if (config.autoCreationMinCollectionSize !== undefined) { + testingHooks.setIndexAutoCreationMinCollectionSize( + config.autoCreationMinCollectionSize + ); + } + if (config.relativeIndexReadCostPerDocument !== undefined) { + testingHooks.setRelativeIndexReadCostPerDocument( + config.relativeIndexReadCostPerDocument + ); + } + }); + + return this; + } + + afterBackfillIndexes(options?: { + maxDocumentsToProcess?: number; + }): LocalStoreTester { + this.prepareNextStep(); + + this.promiseChain = this.promiseChain.then(() => + this.indexBackfiller + .backfill(options?.maxDocumentsToProcess) + .then(() => {}) + ); + + return this; + } + /** * Asserts the expected number of mutations and documents read by * the MutationQueue and the RemoteDocumentCache. @@ -348,36 +406,28 @@ class LocalStoreTester { overlayTypes?: { [k: string]: MutationType }; }): LocalStoreTester { this.promiseChain = this.promiseChain.then(() => { + const actualCount: typeof expectedCount = {}; if (expectedCount.overlaysByCollection !== undefined) { - expect(this.queryEngine.overlaysReadByCollection).to.be.eq( - expectedCount.overlaysByCollection, - 'Overlays read (by collection)' - ); + actualCount.overlaysByCollection = + this.queryEngine.overlaysReadByCollection; } if (expectedCount.overlaysByKey !== undefined) { - expect(this.queryEngine.overlaysReadByKey).to.be.eq( - expectedCount.overlaysByKey, - 'Overlays read (by key)' - ); + actualCount.overlaysByKey = this.queryEngine.overlaysReadByKey; } if (expectedCount.overlayTypes !== undefined) { - expect(this.queryEngine.overlayTypes).to.deep.equal( - expectedCount.overlayTypes, - 'Overlay types read' - ); + actualCount.overlayTypes = this.queryEngine.overlayTypes; } if (expectedCount.documentsByCollection !== undefined) { - expect(this.queryEngine.documentsReadByCollection).to.be.eq( - expectedCount.documentsByCollection, - 'Remote documents read (by collection)' - ); + actualCount.documentsByCollection = + this.queryEngine.documentsReadByCollection; } if (expectedCount.documentsByKey !== undefined) { - expect(this.queryEngine.documentsReadByKey).to.be.eq( - expectedCount.documentsByKey, - 'Remote documents read (by key)' - ); + actualCount.documentsByKey = this.queryEngine.documentsReadByKey; } + expect(actualCount).to.deep.eq( + expectedCount, + `query execution #${this.queryExecutionCount}` + ); }); return this; } @@ -414,7 +464,7 @@ class LocalStoreTester { } toReturnChangedInternal( - docs: Document[], + docsOrKeyStrs: Document[] | string[], isEqual?: (lhs: Document | null, rhs: Document | null) => boolean ): LocalStoreTester { this.promiseChain = this.promiseChain.then(() => { @@ -422,14 +472,21 @@ class LocalStoreTester { this.lastChanges !== null, 'Called toReturnChanged() without prior after()' ); - expect(this.lastChanges.size).to.equal(docs.length, 'number of changes'); - for (const doc of docs) { - const returned = this.lastChanges.get(doc.key); + expect(this.lastChanges.size).to.equal( + docsOrKeyStrs.length, + 'number of changes' + ); + for (const docOrKeyStr of docsOrKeyStrs) { + const docKey = + typeof docOrKeyStr === 'string' ? key(docOrKeyStr) : docOrKeyStr.key; + const returned = this.lastChanges.get(docKey); const message = `Expected '${returned}' to equal '${doc}'.`; - if (isEqual) { - expect(isEqual(doc, returned)).to.equal(true, message); + if (typeof docOrKeyStr === 'string') { + expect(returned?.isValidDocument()).to.equal(true, message); + } else if (isEqual) { + expect(isEqual(docOrKeyStr, returned)).to.equal(true, message); } else { - expectEqual(doc, returned, message); + expectEqual(docOrKeyStr, returned, message); } } this.lastChanges = null; @@ -437,8 +494,10 @@ class LocalStoreTester { return this; } - toReturnChanged(...docs: Document[]): LocalStoreTester { - return this.toReturnChangedInternal(docs); + toReturnChanged(...docs: Document[]): LocalStoreTester; + toReturnChanged(...docKeyStrs: string[]): LocalStoreTester; + toReturnChanged(...docsOrKeyStrs: Document[] | string[]): LocalStoreTester { + return this.toReturnChangedInternal(docsOrKeyStrs); } toReturnChangedWithDocComparator( @@ -631,7 +690,10 @@ describe('LocalStore w/ IndexedDB Persistence', () => { } addEqualityMatcher(); - genericLocalStoreTests(initialize, /* gcIsEager= */ false); + describe('genericLocalStoreTests', () => + genericLocalStoreTests(initialize, /* gcIsEager= */ false)); + describe('indexedDbLocalStoreTests', () => + indexedDbLocalStoreTests(initialize, /* gcIsEager= */ false)); }); function genericLocalStoreTests( @@ -663,6 +725,17 @@ function genericLocalStoreTests( ); } + it('localStoreSetIndexAutoCreationEnabled()', () => { + localStoreSetIndexAutoCreationEnabled(localStore, true); + expect(queryEngine.indexAutoCreationEnabled).to.be.true; + localStoreSetIndexAutoCreationEnabled(localStore, false); + expect(queryEngine.indexAutoCreationEnabled).to.be.false; + localStoreSetIndexAutoCreationEnabled(localStore, true); + expect(queryEngine.indexAutoCreationEnabled).to.be.true; + localStoreSetIndexAutoCreationEnabled(localStore, false); + expect(queryEngine.indexAutoCreationEnabled).to.be.false; + }); + it('handles SetMutation', () => { return expectLocalStore() .after(setMutation('foo/bar', { foo: 'bar' })) @@ -2621,3 +2694,301 @@ function genericLocalStoreTests( } ); } + +function indexedDbLocalStoreTests( + getComponents: () => Promise, + gcIsEager: boolean +): void { + let persistence: Persistence; + let localStore: LocalStore; + let queryEngine: CountingQueryEngine; + + beforeEach(async () => { + const components = await getComponents(); + persistence = components.persistence; + localStore = components.localStore; + queryEngine = components.queryEngine; + }); + + afterEach(async () => { + await persistence.shutdown(); + await persistenceHelpers.clearTestPersistence(); + }); + + function expectLocalStore(): LocalStoreTester { + return new LocalStoreTester( + localStore, + persistence, + queryEngine, + gcIsEager + ); + } + + it('can auto-create indexes', () => { + const query_ = query('coll', filter('matches', '==', true)); + return ( + expectLocalStore() + .afterAllocatingQuery(query_) + .toReturnTargetId(2) + .afterIndexAutoCreationConfigure({ + enabled: true, + autoCreationMinCollectionSize: 0, + relativeIndexReadCostPerDocument: 2 + }) + .afterRemoteEvents([ + docAddedRemoteEvent(doc('coll/a', 10, { matches: true }), [2], []), + docAddedRemoteEvent(doc('coll/b', 10, { matches: false }), [2], []), + docAddedRemoteEvent(doc('coll/c', 10, { matches: false }), [2], []), + docAddedRemoteEvent(doc('coll/d', 10, { matches: false }), [2], []), + docAddedRemoteEvent(doc('coll/e', 10, { matches: true }), [2], []) + ]) + // First time query runs without indexes. + // Based on current heuristic, collection document counts (5) > + // 2 * resultSize (2). + // Full matched index should be created. + .afterExecutingQuery(query_) + .toHaveRead({ documentsByKey: 0, documentsByCollection: 2 }) + .toReturnChanged('coll/a', 'coll/e') + .afterBackfillIndexes() + .afterRemoteEvent( + docAddedRemoteEvent(doc('coll/f', 20, { matches: true }), [2], []) + ) + .afterExecutingQuery(query_) + .toHaveRead({ documentsByKey: 2, documentsByCollection: 1 }) + .toReturnChanged('coll/a', 'coll/e', 'coll/f') + .finish() + ); + }); + + it('does not auto-create indexes for small collections', () => { + const query_ = query('coll', filter('count', '>=', 3)); + return ( + expectLocalStore() + .afterAllocatingQuery(query_) + .toReturnTargetId(2) + .afterIndexAutoCreationConfigure({ + enabled: true, + relativeIndexReadCostPerDocument: 2 + }) + .afterRemoteEvents([ + docAddedRemoteEvent(doc('coll/a', 10, { count: 5 }), [2], []), + docAddedRemoteEvent(doc('coll/b', 10, { count: 1 }), [2], []), + docAddedRemoteEvent(doc('coll/c', 10, { count: 0 }), [2], []), + docAddedRemoteEvent(doc('coll/d', 10, { count: 1 }), [2], []), + docAddedRemoteEvent(doc('coll/e', 10, { count: 3 }), [2], []) + ]) + // SDK will not create indexes since collection size is too small. + .afterExecutingQuery(query_) + .toHaveRead({ documentsByKey: 0, documentsByCollection: 2 }) + .toReturnChanged('coll/a', 'coll/e') + .afterBackfillIndexes() + .afterRemoteEvent( + docAddedRemoteEvent(doc('coll/f', 20, { count: 4 }), [2], []) + ) + .afterExecutingQuery(query_) + .toHaveRead({ documentsByKey: 0, documentsByCollection: 3 }) + .toReturnChanged('coll/a', 'coll/e', 'coll/f') + .finish() + ); + }); + + it('does not auto create indexes when index lookup is expensive', () => { + const query_ = query('coll', filter('array', 'array-contains-any', [0, 7])); + return ( + expectLocalStore() + .afterAllocatingQuery(query_) + .toReturnTargetId(2) + .afterIndexAutoCreationConfigure({ + enabled: true, + autoCreationMinCollectionSize: 0, + relativeIndexReadCostPerDocument: 5 + }) + .afterRemoteEvents([ + docAddedRemoteEvent(doc('coll/a', 10, { array: [2, 7] }), [2], []), + docAddedRemoteEvent(doc('coll/b', 10, { array: [] }), [2], []), + docAddedRemoteEvent(doc('coll/c', 10, { array: [3] }), [2], []), + docAddedRemoteEvent( + doc('coll/d', 10, { array: [2, 10, 20] }), + [2], + [] + ), + docAddedRemoteEvent(doc('coll/e', 10, { array: [2, 0, 8] }), [2], []) + ]) + // SDK will not create indexes since relative read cost is too large. + .afterExecutingQuery(query_) + .toHaveRead({ documentsByKey: 0, documentsByCollection: 2 }) + .toReturnChanged('coll/a', 'coll/e') + .afterBackfillIndexes() + .afterRemoteEvent( + docAddedRemoteEvent(doc('coll/f', 20, { array: [0] }), [2], []) + ) + .afterExecutingQuery(query_) + .toHaveRead({ documentsByKey: 0, documentsByCollection: 3 }) + .toReturnChanged('coll/a', 'coll/e', 'coll/f') + .finish() + ); + }); + + it('index auto creation works when backfiller runs halfway', () => { + const query_ = query('coll', filter('matches', '==', 'foo')); + return ( + expectLocalStore() + .afterAllocatingQuery(query_) + .toReturnTargetId(2) + .afterIndexAutoCreationConfigure({ + enabled: true, + autoCreationMinCollectionSize: 0, + relativeIndexReadCostPerDocument: 2 + }) + .afterRemoteEvents([ + docAddedRemoteEvent(doc('coll/a', 10, { matches: 'foo' }), [2], []), + docAddedRemoteEvent(doc('coll/b', 10, { matches: '' }), [2], []), + docAddedRemoteEvent(doc('coll/c', 10, { matches: 'bar' }), [2], []), + docAddedRemoteEvent(doc('coll/d', 10, { matches: 7 }), [2], []), + docAddedRemoteEvent(doc('coll/e', 10, { matches: 'foo' }), [2], []) + ]) + // First time query runs without indexes. + // Based on current heuristic, collection document counts (5) > + // 2 * resultSize (2). + // Full matched index should be created. + .afterExecutingQuery(query_) + .toHaveRead({ documentsByKey: 0, documentsByCollection: 2 }) + .toReturnChanged('coll/a', 'coll/e') + .afterBackfillIndexes({ maxDocumentsToProcess: 2 }) + .afterRemoteEvent( + docAddedRemoteEvent(doc('coll/f', 20, { matches: 'foo' }), [2], []) + ) + .afterExecutingQuery(query_) + .toHaveRead({ documentsByKey: 1, documentsByCollection: 2 }) + .toReturnChanged('coll/a', 'coll/e', 'coll/f') + .finish() + ); + }); + + it('index created by index auto creation exists after turn off auto creation', () => { + const query_ = query('coll', filter('value', 'not-in', [3])); + return ( + expectLocalStore() + .afterAllocatingQuery(query_) + .toReturnTargetId(2) + .afterIndexAutoCreationConfigure({ + enabled: true, + autoCreationMinCollectionSize: 0, + relativeIndexReadCostPerDocument: 2 + }) + .afterRemoteEvents([ + docAddedRemoteEvent(doc('coll/a', 10, { value: 5 }), [2], []), + docAddedRemoteEvent(doc('coll/b', 10, { value: 3 }), [2], []), + docAddedRemoteEvent(doc('coll/c', 10, { value: 3 }), [2], []), + docAddedRemoteEvent(doc('coll/d', 10, { value: 3 }), [2], []), + docAddedRemoteEvent(doc('coll/e', 10, { value: 2 }), [2], []) + ]) + // First time query runs without indexes. + // Based on current heuristic, collection document counts (5) > + // 2 * resultSize (2). + // Full matched index should be created. + .afterExecutingQuery(query_) + .toHaveRead({ documentsByKey: 0, documentsByCollection: 2 }) + .toReturnChanged('coll/a', 'coll/e') + .afterIndexAutoCreationConfigure({ enabled: false }) + .afterBackfillIndexes() + .afterRemoteEvent( + docAddedRemoteEvent(doc('coll/f', 20, { value: 7 }), [2], []) + ) + .afterExecutingQuery(query_) + .toHaveRead({ documentsByKey: 2, documentsByCollection: 1 }) + .toReturnChanged('coll/a', 'coll/e', 'coll/f') + .finish() + ); + }); + + it('disable index auto creation works', () => { + const query1 = query('coll', filter('value', 'in', [0, 1])); + const query2 = query('foo', filter('value', '!=', Number.NaN)); + return ( + expectLocalStore() + .afterAllocatingQuery(query1) + .toReturnTargetId(2) + .afterIndexAutoCreationConfigure({ + enabled: true, + autoCreationMinCollectionSize: 0, + relativeIndexReadCostPerDocument: 2 + }) + .afterRemoteEvents([ + docAddedRemoteEvent(doc('coll/a', 10, { value: 1 }), [2], []), + docAddedRemoteEvent(doc('coll/b', 10, { value: 8 }), [2], []), + docAddedRemoteEvent(doc('coll/c', 10, { value: 'string' }), [2], []), + docAddedRemoteEvent(doc('coll/d', 10, { value: false }), [2], []), + docAddedRemoteEvent(doc('coll/e', 10, { value: 0 }), [2], []) + ]) + // First time query runs without indexes. + // Based on current heuristic, collection document counts (5) > + // 2 * resultSize (2). + // Full matched index should be created. + .afterExecutingQuery(query1) + .toHaveRead({ documentsByKey: 0, documentsByCollection: 2 }) + .toReturnChanged('coll/a', 'coll/e') + .afterIndexAutoCreationConfigure({ enabled: false }) + .afterBackfillIndexes() + .afterExecutingQuery(query1) + .toHaveRead({ documentsByKey: 2, documentsByCollection: 0 }) + .toReturnChanged('coll/a', 'coll/e') + .afterAllocatingQuery(query2) + .toReturnTargetId(4) + .afterRemoteEvents([ + docAddedRemoteEvent(doc('foo/a', 10, { value: 5 }), [2], []), + docAddedRemoteEvent(doc('foo/b', 10, { value: Number.NaN }), [2], []), + docAddedRemoteEvent(doc('foo/c', 10, { value: Number.NaN }), [2], []), + docAddedRemoteEvent(doc('foo/d', 10, { value: Number.NaN }), [2], []), + docAddedRemoteEvent(doc('foo/e', 10, { value: 'string' }), [2], []) + ]) + .afterExecutingQuery(query2) + .toHaveRead({ documentsByKey: 0, documentsByCollection: 2 }) + .afterBackfillIndexes() + .afterExecutingQuery(query2) + .toHaveRead({ documentsByKey: 0, documentsByCollection: 2 }) + .finish() + ); + }); + + it('index auto creation works with mutation', () => { + const query_ = query( + 'coll', + filter('value', 'array-contains-any', [8, 1, 'string']) + ); + return expectLocalStore() + .afterAllocatingQuery(query_) + .toReturnTargetId(2) + .afterIndexAutoCreationConfigure({ + enabled: true, + autoCreationMinCollectionSize: 0, + relativeIndexReadCostPerDocument: 2 + }) + .afterRemoteEvents([ + docAddedRemoteEvent( + doc('coll/a', 10, { value: [8, 1, 'string'] }), + [2], + [] + ), + docAddedRemoteEvent(doc('coll/b', 10, { value: [] }), [2], []), + docAddedRemoteEvent(doc('coll/c', 10, { value: [3] }), [2], []), + docAddedRemoteEvent(doc('coll/d', 10, { value: [0, 5] }), [2], []), + docAddedRemoteEvent(doc('coll/e', 10, { value: ['string'] }), [2], []) + ]) + .afterExecutingQuery(query_) + .toHaveRead({ documentsByKey: 0, documentsByCollection: 2 }) + .toReturnChanged('coll/a', 'coll/e') + .afterMutation(deleteMutation('coll/e')) + .afterBackfillIndexes() + .afterMutation(setMutation('coll/f', { value: [1] })) + .afterExecutingQuery(query_) + .toHaveRead({ + documentsByKey: 1, + documentsByCollection: 0, + overlaysByKey: 1, + overlaysByCollection: 1 + }) + .toReturnChanged('coll/a', 'coll/f') + .finish(); + }); +} diff --git a/packages/firestore/test/unit/local/query_engine.test.ts b/packages/firestore/test/unit/local/query_engine.test.ts index 7bcf7743b43..bf8ada916ef 100644 --- a/packages/firestore/test/unit/local/query_engine.test.ts +++ b/packages/firestore/test/unit/local/query_engine.test.ts @@ -22,6 +22,7 @@ import { User } from '../../../src/auth/user'; import { LimitType, Query, + queryToTarget, queryWithAddedFilter, queryWithAddedOrderBy, queryWithLimit @@ -29,12 +30,17 @@ import { import { SnapshotVersion } from '../../../src/core/snapshot_version'; import { View } from '../../../src/core/view'; import { DocumentOverlayCache } from '../../../src/local/document_overlay_cache'; +import { + displayNameForIndexType, + IndexType +} from '../../../src/local/index_manager'; import { IndexedDbPersistence } from '../../../src/local/indexeddb_persistence'; import { LocalDocumentsView } from '../../../src/local/local_documents_view'; import { MutationQueue } from '../../../src/local/mutation_queue'; import { Persistence } from '../../../src/local/persistence'; import { PersistencePromise } from '../../../src/local/persistence_promise'; import { PersistenceTransaction } from '../../../src/local/persistence_transaction'; +import { QueryContext } from '../../../src/local/query_context'; import { QueryEngine } from '../../../src/local/query_engine'; import { RemoteDocumentCache } from '../../../src/local/remote_document_cache'; import { TargetCache } from '../../../src/local/target_cache'; @@ -94,7 +100,8 @@ class TestLocalDocumentsView extends LocalDocumentsView { getDocumentsMatchingQuery( transaction: PersistenceTransaction, query: Query, - offset: IndexOffset + offset: IndexOffset, + context?: QueryContext ): PersistencePromise { const skipsDocumentsBeforeSnapshot = indexOffsetComparator(IndexOffset.min(), offset) !== 0; @@ -104,49 +111,45 @@ class TestLocalDocumentsView extends LocalDocumentsView { 'Observed query execution mode did not match expectation' ); - return super.getDocumentsMatchingQuery(transaction, query, offset); + return super.getDocumentsMatchingQuery(transaction, query, offset, context); } } -describe('MemoryQueryEngine', async () => { - /* not durable and without client side indexing */ - genericQueryEngineTest( - false, - persistenceHelpers.testMemoryEagerPersistence, - false - ); -}); +describe('QueryEngine', async () => { + describe('MemoryEagerPersistence', async () => { + /* not durable and without client side indexing */ + genericQueryEngineTest( + persistenceHelpers.testMemoryEagerPersistence, + false + ); + }); -describe('IndexedDbQueryEngine', async () => { if (!IndexedDbPersistence.isAvailable()) { console.warn('No IndexedDB. Skipping IndexedDbQueryEngine tests.'); return; } - let persistencePromise: Promise; - beforeEach(async () => { - persistencePromise = persistenceHelpers.testIndexedDbPersistence(); + describe('IndexedDbPersistence configureCsi=false', async () => { + /* durable but without client side indexing */ + genericQueryEngineTest(persistenceHelpers.testIndexedDbPersistence, false); }); - /* durable but without client side indexing */ - genericQueryEngineTest(true, () => persistencePromise, false); - - /* durable and with client side indexing */ - genericQueryEngineTest(true, () => persistencePromise, true); + describe('IndexedDbQueryEngine configureCsi=true', async () => { + /* durable and with client side indexing */ + genericQueryEngineTest(persistenceHelpers.testIndexedDbPersistence, true); + }); }); /** * Defines the set of tests to run against the memory and IndexedDB-backed * query engine. * - * @param durable Whether the provided persistence is backed by IndexedDB * @param persistencePromise A factory function that returns an initialized * persistence layer. * @param configureCsi Whether tests should configure client side indexing * or use full table scans. */ function genericQueryEngineTest( - durable: boolean, persistencePromise: () => Promise, configureCsi: boolean ): void { @@ -232,7 +235,9 @@ function genericQueryEngineTest( 'expectOptimizedCollectionQuery()/expectFullCollectionQuery()' ); - return persistence.runTransaction('runQuery', 'readonly', txn => { + // NOTE: Use a `readwrite` transaction (instead of `readonly`) so that + // client-side indexes can be written to persistence. + return persistence.runTransaction('runQuery', 'readwrite', txn => { return targetCache .getMatchingKeysForTargetId(txn, TEST_TARGET_ID) .next(remoteKeys => { @@ -832,6 +837,128 @@ function genericQueryEngineTest( verifyResult(results, [doc1, doc2, doc4]); }); + + // A generic test for index auto-creation. + // This function can be called with explicit parameters from it() methods. + const testIndexAutoCreation = async (config: { + indexAutoCreationEnabled: boolean; + indexAutoCreationMinCollectionSize?: number; + relativeIndexReadCostPerDocument?: number; + matchingDocumentCount?: number; + nonmatchingDocumentCount?: number; + expectedPostQueryExecutionIndexType: IndexType; + }): Promise => { + debugAssert(configureCsi, 'Test requires durable persistence'); + + const matchingDocuments: MutableDocument[] = []; + for (let i = 0; i < (config.matchingDocumentCount ?? 3); i++) { + const matchingDocument = doc(`coll/A${i}`, 1, { 'foo': 'match' }); + matchingDocuments.push(matchingDocument); + } + await addDocument(...matchingDocuments); + + const nonmatchingDocuments: MutableDocument[] = []; + for (let i = 0; i < (config.nonmatchingDocumentCount ?? 3); i++) { + const nonmatchingDocument = doc(`coll/X${i}`, 1, { 'foo': 'nomatch' }); + nonmatchingDocuments.push(nonmatchingDocument); + } + await addDocument(...nonmatchingDocuments); + + queryEngine.indexAutoCreationEnabled = config.indexAutoCreationEnabled; + + if (config.indexAutoCreationMinCollectionSize !== undefined) { + queryEngine.indexAutoCreationMinCollectionSize = + config.indexAutoCreationMinCollectionSize; + } + if (config.relativeIndexReadCostPerDocument !== undefined) { + queryEngine.relativeIndexReadCostPerDocument = + config.relativeIndexReadCostPerDocument; + } + + const q = query('coll', filter('foo', '==', 'match')); + const target = queryToTarget(q); + const preQueryExecutionIndexType = await indexManager.getIndexType( + target + ); + expect( + preQueryExecutionIndexType, + 'index type for target _before_ running the query is ' + + displayNameForIndexType(preQueryExecutionIndexType) + + ', but expected ' + + displayNameForIndexType(IndexType.NONE) + ).to.equal(IndexType.NONE); + + const result = await expectFullCollectionQuery(() => + runQuery(q, SnapshotVersion.min()) + ); + verifyResult(result, matchingDocuments); + + const postQueryExecutionIndexType = await indexManager.getIndexType( + target + ); + expect( + postQueryExecutionIndexType, + 'index type for target _after_ running the query is ' + + displayNameForIndexType(postQueryExecutionIndexType) + + ', but expected ' + + displayNameForIndexType(config.expectedPostQueryExecutionIndexType) + ).to.equal(config.expectedPostQueryExecutionIndexType); + }; + + it('creates indexes when indexAutoCreationEnabled=true', () => + testIndexAutoCreation({ + indexAutoCreationEnabled: true, + indexAutoCreationMinCollectionSize: 0, + relativeIndexReadCostPerDocument: 0, + expectedPostQueryExecutionIndexType: IndexType.FULL + })); + + it('does not create indexes when indexAutoCreationEnabled=false', () => + testIndexAutoCreation({ + indexAutoCreationEnabled: false, + indexAutoCreationMinCollectionSize: 0, + relativeIndexReadCostPerDocument: 0, + expectedPostQueryExecutionIndexType: IndexType.NONE + })); + + it( + 'creates indexes when ' + + 'min collection size is met exactly ' + + 'and relative cost is ever-so-slightly better', + () => + testIndexAutoCreation({ + indexAutoCreationEnabled: true, + indexAutoCreationMinCollectionSize: 10, + relativeIndexReadCostPerDocument: 1.9999, + matchingDocumentCount: 5, + nonmatchingDocumentCount: 5, + expectedPostQueryExecutionIndexType: IndexType.FULL + }) + ); + + it( + 'does not create indexes when ' + + 'min collection size is not met by only 1 document', + () => + testIndexAutoCreation({ + indexAutoCreationEnabled: true, + indexAutoCreationMinCollectionSize: 10, + relativeIndexReadCostPerDocument: 0, + matchingDocumentCount: 5, + nonmatchingDocumentCount: 4, + expectedPostQueryExecutionIndexType: IndexType.NONE + }) + ); + + it('does not create indexes when relative cost is equal', () => + testIndexAutoCreation({ + indexAutoCreationEnabled: true, + indexAutoCreationMinCollectionSize: 0, + relativeIndexReadCostPerDocument: 2, + matchingDocumentCount: 5, + nonmatchingDocumentCount: 5, + expectedPostQueryExecutionIndexType: IndexType.NONE + })); } // Tests below this line execute with and without client side indexing diff --git a/packages/firestore/test/unit/local/test_index_manager.ts b/packages/firestore/test/unit/local/test_index_manager.ts index 8b31656cd02..94509073925 100644 --- a/packages/firestore/test/unit/local/test_index_manager.ts +++ b/packages/firestore/test/unit/local/test_index_manager.ts @@ -63,6 +63,14 @@ export class TestIndexManager { ); } + createTargetIndexes(target: Target): Promise { + return this.persistence.runTransaction( + 'createTargetIndexes', + 'readwrite', + txn => this.indexManager.createTargetIndexes(txn, target) + ); + } + getFieldIndexes(collectionGroup?: string): Promise { return this.persistence.runTransaction('getFieldIndexes', 'readonly', txn => collectionGroup diff --git a/packages/firestore/test/unit/model/target_index_matcher.test.ts b/packages/firestore/test/unit/model/target_index_matcher.test.ts index 2d48b12f1a1..8e295e17fef 100644 --- a/packages/firestore/test/unit/model/target_index_matcher.test.ts +++ b/packages/firestore/test/unit/model/target_index_matcher.test.ts @@ -24,6 +24,7 @@ import { Query, newQueryForCollectionGroup } from '../../../src/core/query'; +import { targetGetSegmentCount } from '../../../src/core/target'; import { IndexKind } from '../../../src/model/field_index'; import { TargetIndexMatcher } from '../../../src/model/target_index_matcher'; import { fieldIndex, filter, orderBy, query } from '../../util/helpers'; @@ -51,6 +52,23 @@ describe('Target Bounds', () => { ) ]; + const queriesWithOrderBy = [ + queryWithAddedOrderBy(query('collId'), orderBy('a')), + queryWithAddedOrderBy(query('collId'), orderBy('a', 'asc')), + queryWithAddedOrderBy(query('collId'), orderBy('a', 'desc')), + queryWithAddedOrderBy( + queryWithAddedOrderBy(query('collId'), orderBy('a', 'desc')), + orderBy('__name__') + ), + queryWithAddedOrderBy( + queryWithAddedFilter( + query('collId'), + filter('a', 'array-contains-any', ['a']) + ), + orderBy('b') + ) + ]; + it('can use merge join', () => { let q = queryWithAddedFilter( queryWithAddedFilter(query('collId'), filter('a', '==', 1)), @@ -713,6 +731,240 @@ describe('Target Bounds', () => { validateServesTarget(q, 'a', IndexKind.ASCENDING); }); + describe('buildTargetIndex()', () => { + it('queries with equalities', () => + queriesWithEqualities.forEach( + validateBuildTargetIndexCreateFullMatchIndex + )); + + it('queries with inequalities', () => + queriesWithInequalities.forEach( + validateBuildTargetIndexCreateFullMatchIndex + )); + + it('queries with array contains', () => + queriesWithArrayContains.forEach( + validateBuildTargetIndexCreateFullMatchIndex + )); + + it('queries with order by', () => + queriesWithOrderBy.forEach(validateBuildTargetIndexCreateFullMatchIndex)); + + it('queries with inequalities uses single field index', () => + validateBuildTargetIndexCreateFullMatchIndex( + queryWithAddedFilter( + queryWithAddedFilter(query('collId'), filter('a', '>', 1)), + filter('a', '<', 10) + ) + )); + + it('query of a collection', () => + validateBuildTargetIndexCreateFullMatchIndex(query('collId'))); + + it('query with array contains and order by', () => + validateBuildTargetIndexCreateFullMatchIndex( + queryWithAddedOrderBy( + queryWithAddedFilter( + queryWithAddedFilter( + query('collId'), + filter('a', 'array-contains', 'a') + ), + filter('a', '>', 'b') + ), + orderBy('a', 'asc') + ) + )); + + it('query with equality and descending order', () => + validateBuildTargetIndexCreateFullMatchIndex( + queryWithAddedOrderBy( + queryWithAddedFilter(query('collId'), filter('a', '==', 1)), + orderBy('__name__', 'desc') + ) + )); + + it('query with multiple equalities', () => + validateBuildTargetIndexCreateFullMatchIndex( + queryWithAddedFilter( + queryWithAddedFilter(query('collId'), filter('a1', '==', 'a')), + filter('a2', '==', 'b') + ) + )); + + describe('query with multiple equalities and inequality', () => { + it('inequality last', () => + validateBuildTargetIndexCreateFullMatchIndex( + queryWithAddedFilter( + queryWithAddedFilter( + queryWithAddedFilter( + query('collId'), + filter('equality1', '==', 'a') + ), + filter('equality2', '==', 'b') + ), + filter('inequality', '>=', 'c') + ) + )); + + it('inequality in middle', () => + validateBuildTargetIndexCreateFullMatchIndex( + queryWithAddedFilter( + queryWithAddedFilter( + queryWithAddedFilter( + query('collId'), + filter('equality1', '==', 'a') + ), + filter('inequality', '>=', 'c') + ), + filter('equality2', '==', 'b') + ) + )); + }); + + describe('query with multiple filters', () => { + it('== and > on different fields', () => + validateBuildTargetIndexCreateFullMatchIndex( + queryWithAddedOrderBy( + queryWithAddedFilter( + queryWithAddedFilter(query('collId'), filter('a1', '==', 'a')), + filter('a2', '>', 'b') + ), + orderBy('a2', 'asc') + ) + )); + + it('>=, ==, and <= filters on the same field', () => + validateBuildTargetIndexCreateFullMatchIndex( + queryWithAddedFilter( + queryWithAddedFilter( + queryWithAddedFilter(query('collId'), filter('a', '>=', 1)), + filter('a', '==', 5) + ), + filter('a', '<=', 10) + ) + )); + + it('not-in and >= on the same field', () => + validateBuildTargetIndexCreateFullMatchIndex( + queryWithAddedFilter( + queryWithAddedFilter( + query('collId'), + filter('a', 'not-in', [1, 2, 3]) + ), + filter('a', '>=', 2) + ) + )); + }); + + describe('query with multiple order-bys', () => { + it('order by fff, bar desc, __name__', () => + validateBuildTargetIndexCreateFullMatchIndex( + queryWithAddedOrderBy( + queryWithAddedOrderBy( + queryWithAddedOrderBy(query('collId'), orderBy('fff')), + orderBy('bar', 'desc') + ), + orderBy('__name__') + ) + )); + + it('order by foo, bar, __name__ desc', () => + validateBuildTargetIndexCreateFullMatchIndex( + queryWithAddedOrderBy( + queryWithAddedOrderBy( + queryWithAddedOrderBy(query('collId'), orderBy('foo')), + orderBy('bar') + ), + orderBy('__name__', 'desc') + ) + )); + }); + + it('query with in and not in filters', () => + validateBuildTargetIndexCreateFullMatchIndex( + queryWithAddedFilter( + queryWithAddedFilter( + query('collId'), + filter('a', 'not-in', [1, 2, 3]) + ), + filter('b', 'in', [1, 2, 3]) + ) + )); + + describe('query with equality and different order-by', () => { + it('filter on foo and bar, order by qux', () => + validateBuildTargetIndexCreateFullMatchIndex( + queryWithAddedOrderBy( + queryWithAddedFilter( + queryWithAddedFilter(query('collId'), filter('foo', '==', '')), + filter('bar', '==', '') + ), + orderBy('qux') + ) + )); + + it('filter on aaa, qqq, ccc, order by fff, bbb', () => + validateBuildTargetIndexCreateFullMatchIndex( + queryWithAddedOrderBy( + queryWithAddedOrderBy( + queryWithAddedFilter( + queryWithAddedFilter( + queryWithAddedFilter( + query('collId'), + filter('aaa', '==', '') + ), + filter('qqq', '==', '') + ), + filter('ccc', '==', '') + ), + orderBy('fff', 'desc') + ), + orderBy('bbb') + ) + )); + }); + + it('query with equals and not-in filters', () => + validateBuildTargetIndexCreateFullMatchIndex( + queryWithAddedFilter( + queryWithAddedFilter(query('collId'), filter('a', '==', '1')), + filter('b', 'not-in', [1, 2, 3]) + ) + )); + + describe('query with in and order-by', () => { + it('on different fields', () => + validateBuildTargetIndexCreateFullMatchIndex( + queryWithAddedOrderBy( + queryWithAddedOrderBy( + queryWithAddedFilter( + query('collId'), + filter('a', 'not-in', [1, 2, 3]) + ), + orderBy('a') + ), + orderBy('b') + ) + )); + + it('on the same field', () => + validateBuildTargetIndexCreateFullMatchIndex( + queryWithAddedOrderBy( + queryWithAddedFilter(query('collId'), filter('a', 'in', [1, 2, 3])), + orderBy('a') + ) + )); + }); + + function validateBuildTargetIndexCreateFullMatchIndex(q: Query): void { + const target = queryToTarget(q); + const targetIndexMatcher = new TargetIndexMatcher(target); + const expectedIndex = targetIndexMatcher.buildTargetIndex(); + expect(targetIndexMatcher.servedByIndex(expectedIndex)).is.true; + expect(expectedIndex.fields.length >= targetGetSegmentCount(target)); + } + }); + function validateServesTarget( query: Query, field: string, From 9a63921fac35c97aa5ba9c60a04f88481539c6b9 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 10 Aug 2023 11:18:00 -0400 Subject: [PATCH 02/28] Firestore: testing_hooks.ts: move some logic into testing_hooks_spi.ts --- packages/firestore/src/remote/watch_change.ts | 8 +- packages/firestore/src/util/testing_hooks.ts | 163 ++++++------------ .../firestore/src/util/testing_hooks_spi.ts | 109 ++++++++++++ .../integration/util/testing_hooks_util.ts | 13 +- 4 files changed, 169 insertions(+), 124 deletions(-) create mode 100644 packages/firestore/src/util/testing_hooks_spi.ts diff --git a/packages/firestore/src/remote/watch_change.ts b/packages/firestore/src/remote/watch_change.ts index 758eb0120bb..5259954b957 100644 --- a/packages/firestore/src/remote/watch_change.ts +++ b/packages/firestore/src/remote/watch_change.ts @@ -38,9 +38,9 @@ import { primitiveComparator } from '../util/misc'; import { SortedMap } from '../util/sorted_map'; import { SortedSet } from '../util/sorted_set'; import { - ExistenceFilterMismatchInfo as TestingHooksExistenceFilterMismatchInfo, - TestingHooks -} from '../util/testing_hooks'; + testingHooksSpi, + ExistenceFilterMismatchInfo as TestingHooksExistenceFilterMismatchInfo +} from '../util/testing_hooks_spi'; import { BloomFilter, BloomFilterError } from './bloom_filter'; import { ExistenceFilter } from './existence_filter'; @@ -452,7 +452,7 @@ export class WatchChangeAggregator { purpose ); } - TestingHooks.instance?.notifyOnExistenceFilterMismatch( + testingHooksSpi?.notifyOnExistenceFilterMismatch( createExistenceFilterMismatchInfoForTestingHooks( currentSize, watchChange.existenceFilter, diff --git a/packages/firestore/src/util/testing_hooks.ts b/packages/firestore/src/util/testing_hooks.ts index abce89ac1c0..6ad84284d19 100644 --- a/packages/firestore/src/util/testing_hooks.ts +++ b/packages/firestore/src/util/testing_hooks.ts @@ -15,47 +15,15 @@ * limitations under the License. */ -/** - * Manages "testing hooks", hooks into the internals of the SDK to verify - * internal state and events during integration tests. Do not use this class - * except for testing purposes. - * - * There are two ways to retrieve the global singleton instance of this class: - * 1. The `instance` property, which returns null if the global singleton - * instance has not been created. Use this property if the caller should - * "do nothing" if there are no testing hooks registered, such as when - * delivering an event to notify registered callbacks. - * 2. The `getOrCreateInstance()` method, which creates the global singleton - * instance if it has not been created. Use this method if the instance is - * needed to, for example, register a callback. - * - * @internal - */ -export class TestingHooks { - private readonly onExistenceFilterMismatchCallbacks = new Map< - Symbol, - ExistenceFilterMismatchCallback - >(); +import { + setTestingHooksSpi, + ExistenceFilterMismatchInfo, + TestingHooksSpi +} from './testing_hooks_spi'; - private constructor() {} - - /** - * Returns the singleton instance of this class, or null if it has not been - * initialized. - */ - static get instance(): TestingHooks | null { - return gTestingHooksSingletonInstance; - } - - /** - * Returns the singleton instance of this class, creating it if is has never - * been created before. - */ - static getOrCreateInstance(): TestingHooks { - if (gTestingHooksSingletonInstance === null) { - gTestingHooksSingletonInstance = new TestingHooks(); - } - return gTestingHooksSingletonInstance; +export class TestingHooks { + private constructor() { + throw new Error('instances of this class should not be created'); } /** @@ -72,87 +40,62 @@ export class TestingHooks { * the first invocation of the returned function does anything; all subsequent * invocations do nothing. */ - onExistenceFilterMismatch( + static onExistenceFilterMismatch( callback: ExistenceFilterMismatchCallback - ): () => void { - const key = Symbol(); - this.onExistenceFilterMismatchCallbacks.set(key, callback); - return () => this.onExistenceFilterMismatchCallbacks.delete(key); - } - - /** - * Invokes all currently-registered `onExistenceFilterMismatch` callbacks. - * @param info Information about the existence filter mismatch. - */ - notifyOnExistenceFilterMismatch(info: ExistenceFilterMismatchInfo): void { - this.onExistenceFilterMismatchCallbacks.forEach(callback => callback(info)); + ): Unregister { + return TestingHooksSpiImpl.instance.onExistenceFilterMismatch(callback); } } /** - * Information about an existence filter mismatch, as specified to callbacks - * registered with `TestingUtils.onExistenceFilterMismatch()`. + * The signature of callbacks registered with + * `TestingUtils.onExistenceFilterMismatch()`. */ -export interface ExistenceFilterMismatchInfo { - /** The number of documents that matched the query in the local cache. */ - localCacheCount: number; - - /** - * The number of documents that matched the query on the server, as specified - * in the ExistenceFilter message's `count` field. - */ - existenceFilterCount: number; - - /** - * The projectId used when checking documents for membership in the bloom - * filter. - */ - projectId: string; +export type ExistenceFilterMismatchCallback = ( + info: ExistenceFilterMismatchInfo +) => void; - /** - * The databaseId used when checking documents for membership in the bloom - * filter. - */ - databaseId: string; +/** + * A function that, when invoked, unregisters something that was registered. + * + * Only the *first* invocation of this function has any effect; subsequent + * invocations do nothing. + */ +export type Unregister = () => void; - /** - * Information about the bloom filter provided by Watch in the ExistenceFilter - * message's `unchangedNames` field. If this property is omitted or undefined - * then that means that Watch did _not_ provide a bloom filter. - */ - bloomFilter?: { - /** - * Whether a full requery was averted by using the bloom filter. If false, - * then something happened, such as a false positive, to prevent using the - * bloom filter to avoid a full requery. - */ - applied: boolean; +/** + * The implementation of `TestingHooksSpi`. + */ +class TestingHooksSpiImpl implements TestingHooksSpi { + private readonly existenceFilterMismatchCallbacksById = new Map< + Symbol, + ExistenceFilterMismatchCallback + >(); - /** The number of hash functions used in the bloom filter. */ - hashCount: number; + private constructor() {} - /** The number of bytes in the bloom filter's bitmask. */ - bitmapLength: number; + static get instance(): TestingHooksSpiImpl { + if (!testingHooksSpiImplInstance) { + testingHooksSpiImplInstance = new TestingHooksSpiImpl(); + setTestingHooksSpi(testingHooksSpiImplInstance); + } + return testingHooksSpiImplInstance; + } - /** The number of bits of padding in the last byte of the bloom filter. */ - padding: number; + notifyOnExistenceFilterMismatch(info: ExistenceFilterMismatchInfo): void { + this.existenceFilterMismatchCallbacksById.forEach(callback => + callback(info) + ); + } - /** - * Tests the given string for membership in the bloom filter created from - * the existence filter; will be undefined if creating the bloom filter - * failed. - */ - mightContain?: (value: string) => boolean; - }; + onExistenceFilterMismatch( + callback: ExistenceFilterMismatchCallback + ): Unregister { + const id = Symbol(); + const callbacks = this.existenceFilterMismatchCallbacksById; + callbacks.set(id, callback); + return () => callbacks.delete(id); + } } -/** - * The signature of callbacks registered with - * `TestingUtils.onExistenceFilterMismatch()`. - */ -export type ExistenceFilterMismatchCallback = ( - info: ExistenceFilterMismatchInfo -) => void; - -/** The global singleton instance of `TestingHooks`. */ -let gTestingHooksSingletonInstance: TestingHooks | null = null; +let testingHooksSpiImplInstance: TestingHooksSpiImpl | null = null; diff --git a/packages/firestore/src/util/testing_hooks_spi.ts b/packages/firestore/src/util/testing_hooks_spi.ts new file mode 100644 index 00000000000..73efd279484 --- /dev/null +++ b/packages/firestore/src/util/testing_hooks_spi.ts @@ -0,0 +1,109 @@ +/** + * @license + * Copyright 2023 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * The global, singleton instance of TestingHooksSpi. + * + * This variable will be `null` in all cases _except_ when running from + * integration tests that have registered callbacks to be notified of events + * that happen during the test execution. + */ +export let testingHooksSpi: TestingHooksSpi | null = null; + +/** + * Sets the value of the `testingHooksSpi` object. + * @param instance the instance to set. + */ +export function setTestingHooksSpi(instance: TestingHooksSpi): void { + if (testingHooksSpi) { + throw new Error('a TestingHooksSpi instance is already set'); + } + testingHooksSpi = instance; +} + +/** + * The "service provider interface" for the testing hooks. + * + * The implementation of this object will handle the callbacks made by the SDK + * to be handled by the integration tests. + * + * This "SPI" is separated from the implementation to avoid import cycles and + * to enable production builds to fully tree-shake away the testing hooks logic. + */ +export interface TestingHooksSpi { + /** + * Invokes all callbacks registered with + * `TestingHooks.onExistenceFilterMismatch()` with the given info. + */ + notifyOnExistenceFilterMismatch(info: ExistenceFilterMismatchInfo): void; +} + +/** + * Information about an existence filter mismatch. + */ +export interface ExistenceFilterMismatchInfo { + /** The number of documents that matched the query in the local cache. */ + localCacheCount: number; + + /** + * The number of documents that matched the query on the server, as specified + * in the ExistenceFilter message's `count` field. + */ + existenceFilterCount: number; + + /** + * The projectId used when checking documents for membership in the bloom + * filter. + */ + projectId: string; + + /** + * The databaseId used when checking documents for membership in the bloom + * filter. + */ + databaseId: string; + + /** + * Information about the bloom filter provided by Watch in the ExistenceFilter + * message's `unchangedNames` field. If this property is omitted or undefined + * then that means that Watch did _not_ provide a bloom filter. + */ + bloomFilter?: { + /** + * Whether a full requery was averted by using the bloom filter. If false, + * then something happened, such as a false positive, to prevent using the + * bloom filter to avoid a full requery. + */ + applied: boolean; + + /** The number of hash functions used in the bloom filter. */ + hashCount: number; + + /** The number of bytes in the bloom filter's bitmask. */ + bitmapLength: number; + + /** The number of bits of padding in the last byte of the bloom filter. */ + padding: number; + + /** + * Tests the given string for membership in the bloom filter created from + * the existence filter; will be undefined if creating the bloom filter + * failed. + */ + mightContain?: (value: string) => boolean; + }; +} diff --git a/packages/firestore/test/integration/util/testing_hooks_util.ts b/packages/firestore/test/integration/util/testing_hooks_util.ts index 3dec7d379d8..dc747352b18 100644 --- a/packages/firestore/test/integration/util/testing_hooks_util.ts +++ b/packages/firestore/test/integration/util/testing_hooks_util.ts @@ -38,10 +38,9 @@ export async function captureExistenceFilterMismatches( results.push(createExistenceFilterMismatchInfoFrom(info)); }; - const unregister = - TestingHooks.getOrCreateInstance().onExistenceFilterMismatch( - onExistenceFilterMismatchCallback - ); + const unregister = TestingHooks.onExistenceFilterMismatch( + onExistenceFilterMismatchCallback + ); let callbackResult: T; try { @@ -56,9 +55,6 @@ export async function captureExistenceFilterMismatches( /** * A copy of `ExistenceFilterMismatchInfo` as defined in `testing_hooks.ts`. * - * See the documentation of `TestingHooks.notifyOnExistenceFilterMismatch()` - * for the meaning of these values. - * * TODO: Delete this "interface" definition and instead use the one from * testing_hooks.ts. I tried to do this but couldn't figure out how to get it to * work in a way that survived bundling and minification. @@ -80,9 +76,6 @@ interface ExistenceFilterMismatchInfoInternal { /** * Information about an existence filter mismatch, captured during an invocation * of `captureExistenceFilterMismatches()`. - * - * See the documentation of `TestingHooks.notifyOnExistenceFilterMismatch()` - * for the meaning of these values. */ export interface ExistenceFilterMismatchInfo { localCacheCount: number; From f3489a61e61b1217810761baad5488114cd2f9d7 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 10 Aug 2023 13:08:55 -0400 Subject: [PATCH 03/28] api fixes --- packages/firestore/src/api.ts | 6 +++- packages/firestore/src/util/testing_hooks.ts | 28 ++++++++++--------- .../firestore/src/util/testing_hooks_spi.ts | 1 + .../integration/util/testing_hooks_util.ts | 24 ++-------------- 4 files changed, 23 insertions(+), 36 deletions(-) diff --git a/packages/firestore/src/api.ts b/packages/firestore/src/api.ts index aabc632d2f6..0e871303cb8 100644 --- a/packages/firestore/src/api.ts +++ b/packages/firestore/src/api.ts @@ -223,4 +223,8 @@ export type { } from './api/credentials'; export { EmptyAuthCredentialsProvider as _EmptyAuthCredentialsProvider } from './api/credentials'; export { EmptyAppCheckTokenProvider as _EmptyAppCheckTokenProvider } from './api/credentials'; -export { TestingHooks as _TestingHooks } from './util/testing_hooks'; +export { + ExistenceFilterMismatchCallback as _TestingHooksExistenceFilterMismatchCallback, + TestingHooks as _TestingHooks +} from './util/testing_hooks'; +export { ExistenceFilterMismatchInfo as _TestingHooksExistenceFilterMismatchInfo } from './util/testing_hooks_spi'; diff --git a/packages/firestore/src/util/testing_hooks.ts b/packages/firestore/src/util/testing_hooks.ts index 6ad84284d19..768ee40a2ef 100644 --- a/packages/firestore/src/util/testing_hooks.ts +++ b/packages/firestore/src/util/testing_hooks.ts @@ -15,12 +15,21 @@ * limitations under the License. */ +import { Unsubscribe } from '../api/reference_impl'; + import { setTestingHooksSpi, ExistenceFilterMismatchInfo, TestingHooksSpi } from './testing_hooks_spi'; +/** + * Testing hooks for use by Firestore's integration test suite to reach into the + * SDK internals to validate logic and behavior that is not visible from the + * public API surface. + * + * @internal + */ export class TestingHooks { private constructor() { throw new Error('instances of this class should not be created'); @@ -42,7 +51,7 @@ export class TestingHooks { */ static onExistenceFilterMismatch( callback: ExistenceFilterMismatchCallback - ): Unregister { + ): Unsubscribe { return TestingHooksSpiImpl.instance.onExistenceFilterMismatch(callback); } } @@ -50,18 +59,11 @@ export class TestingHooks { /** * The signature of callbacks registered with * `TestingUtils.onExistenceFilterMismatch()`. + * @internal */ -export type ExistenceFilterMismatchCallback = ( - info: ExistenceFilterMismatchInfo -) => void; - -/** - * A function that, when invoked, unregisters something that was registered. - * - * Only the *first* invocation of this function has any effect; subsequent - * invocations do nothing. - */ -export type Unregister = () => void; +export interface ExistenceFilterMismatchCallback { + (info: ExistenceFilterMismatchInfo): void; +} /** * The implementation of `TestingHooksSpi`. @@ -90,7 +92,7 @@ class TestingHooksSpiImpl implements TestingHooksSpi { onExistenceFilterMismatch( callback: ExistenceFilterMismatchCallback - ): Unregister { + ): Unsubscribe { const id = Symbol(); const callbacks = this.existenceFilterMismatchCallbacksById; callbacks.set(id, callback); diff --git a/packages/firestore/src/util/testing_hooks_spi.ts b/packages/firestore/src/util/testing_hooks_spi.ts index 73efd279484..e6c729e3661 100644 --- a/packages/firestore/src/util/testing_hooks_spi.ts +++ b/packages/firestore/src/util/testing_hooks_spi.ts @@ -54,6 +54,7 @@ export interface TestingHooksSpi { /** * Information about an existence filter mismatch. + * @internal */ export interface ExistenceFilterMismatchInfo { /** The number of documents that matched the query in the local cache. */ diff --git a/packages/firestore/test/integration/util/testing_hooks_util.ts b/packages/firestore/test/integration/util/testing_hooks_util.ts index dc747352b18..eff800f4db6 100644 --- a/packages/firestore/test/integration/util/testing_hooks_util.ts +++ b/packages/firestore/test/integration/util/testing_hooks_util.ts @@ -17,7 +17,8 @@ import { DocumentReference, - _TestingHooks as TestingHooks + _TestingHooks as TestingHooks, + _TestingHooksExistenceFilterMismatchInfo as ExistenceFilterMismatchInfoInternal } from './firebase_export'; /** @@ -52,27 +53,6 @@ export async function captureExistenceFilterMismatches( return [results, callbackResult]; } -/** - * A copy of `ExistenceFilterMismatchInfo` as defined in `testing_hooks.ts`. - * - * TODO: Delete this "interface" definition and instead use the one from - * testing_hooks.ts. I tried to do this but couldn't figure out how to get it to - * work in a way that survived bundling and minification. - */ -interface ExistenceFilterMismatchInfoInternal { - localCacheCount: number; - existenceFilterCount: number; - projectId: string; - databaseId: string; - bloomFilter?: { - applied: boolean; - hashCount: number; - bitmapLength: number; - padding: number; - mightContain?: (value: string) => boolean; - }; -} - /** * Information about an existence filter mismatch, captured during an invocation * of `captureExistenceFilterMismatches()`. From 6caf745761285087dccb5c6d1551be2dd6230421 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 10 Aug 2023 14:43:34 -0400 Subject: [PATCH 04/28] add testing_hooks.ts to externs.json so that _TestingHooksExistenceFilterMismatchCallback doesn't get mangled by rollup in renameInternals() --- packages/firestore/externs.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/firestore/externs.json b/packages/firestore/externs.json index fcd6408548c..a1746227316 100644 --- a/packages/firestore/externs.json +++ b/packages/firestore/externs.json @@ -36,6 +36,7 @@ "packages/firestore/src/util/error.ts", "packages/firestore/src/local/indexeddb_schema.ts", "packages/firestore/src/local/indexeddb_schema_legacy.ts", - "packages/firestore/src/local/shared_client_state_schema.ts" + "packages/firestore/src/local/shared_client_state_schema.ts", + "packages/firestore/src/util/testing_hooks.ts" ] } From 0b4eb3b73cf35fdb893aa5b40b2492488d236f34 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 10 Aug 2023 14:52:51 -0400 Subject: [PATCH 05/28] doc minor fix --- packages/firestore/test/integration/util/testing_hooks_util.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/firestore/test/integration/util/testing_hooks_util.ts b/packages/firestore/test/integration/util/testing_hooks_util.ts index eff800f4db6..efb4907e0ca 100644 --- a/packages/firestore/test/integration/util/testing_hooks_util.ts +++ b/packages/firestore/test/integration/util/testing_hooks_util.ts @@ -56,6 +56,9 @@ export async function captureExistenceFilterMismatches( /** * Information about an existence filter mismatch, captured during an invocation * of `captureExistenceFilterMismatches()`. + * + * See the documentation of `ExistenceFilterMismatchInfo` in + * `testing_hooks_spi.ts` for the meaning of these values. */ export interface ExistenceFilterMismatchInfo { localCacheCount: number; From b1ebeec5b1a67b6cdffec756a73b8ce41a3b3685 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 10 Aug 2023 15:08:14 -0400 Subject: [PATCH 06/28] persistent_cache_index_manager.test.ts: add "Auto" to "Indexing" in a test name --- .../test/integration/api/persistent_cache_index_manager.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/firestore/test/integration/api/persistent_cache_index_manager.test.ts b/packages/firestore/test/integration/api/persistent_cache_index_manager.test.ts index 57bcd2ad155..799b88b3dd2 100644 --- a/packages/firestore/test/integration/api/persistent_cache_index_manager.test.ts +++ b/packages/firestore/test/integration/api/persistent_cache_index_manager.test.ts @@ -161,7 +161,7 @@ apiDescribe('PersistentCacheIndexManager', persistence => { }); describe('Query execution', () => { - it('Indexing is disabled by default', () => + it('Auto-indexing is disabled by default', () => testIndexesGetAutoCreated({ documentCounts: { matching: 1, notMatching: 100 }, expectedIndexAutoCreated: false, From 168d922f8e41ca727ecbf7c0d1b62b6b0953bcc6 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 11 Aug 2023 13:51:29 -0400 Subject: [PATCH 07/28] minor tweaks --- packages/firestore/src/util/testing_hooks.ts | 9 ++++++--- .../test/integration/util/testing_hooks_util.ts | 9 ++------- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/packages/firestore/src/util/testing_hooks.ts b/packages/firestore/src/util/testing_hooks.ts index 768ee40a2ef..36422172a45 100644 --- a/packages/firestore/src/util/testing_hooks.ts +++ b/packages/firestore/src/util/testing_hooks.ts @@ -59,11 +59,14 @@ export class TestingHooks { /** * The signature of callbacks registered with * `TestingUtils.onExistenceFilterMismatch()`. + * + * The return value, if any, is ignored. + * * @internal */ -export interface ExistenceFilterMismatchCallback { - (info: ExistenceFilterMismatchInfo): void; -} +export type ExistenceFilterMismatchCallback = ( + info: ExistenceFilterMismatchInfo +) => unknown; /** * The implementation of `TestingHooksSpi`. diff --git a/packages/firestore/test/integration/util/testing_hooks_util.ts b/packages/firestore/test/integration/util/testing_hooks_util.ts index efb4907e0ca..72604f91a8d 100644 --- a/packages/firestore/test/integration/util/testing_hooks_util.ts +++ b/packages/firestore/test/integration/util/testing_hooks_util.ts @@ -33,14 +33,9 @@ export async function captureExistenceFilterMismatches( callback: () => Promise ): Promise<[ExistenceFilterMismatchInfo[], T]> { const results: ExistenceFilterMismatchInfo[] = []; - const onExistenceFilterMismatchCallback = ( - info: ExistenceFilterMismatchInfoInternal - ): void => { - results.push(createExistenceFilterMismatchInfoFrom(info)); - }; - const unregister = TestingHooks.onExistenceFilterMismatch( - onExistenceFilterMismatchCallback + const unregister = TestingHooks.onExistenceFilterMismatch(info => + results.push(createExistenceFilterMismatchInfoFrom(info)) ); let callbackResult: T; From 9ded3381ee9acae403cb11498a9504406a8be2c8 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 11 Aug 2023 13:54:14 -0400 Subject: [PATCH 08/28] minor tweaks --- packages/firestore/src/util/testing_hooks.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/firestore/src/util/testing_hooks.ts b/packages/firestore/src/util/testing_hooks.ts index 0b10a1fd2d0..72ab6ccad8f 100644 --- a/packages/firestore/src/util/testing_hooks.ts +++ b/packages/firestore/src/util/testing_hooks.ts @@ -139,13 +139,13 @@ export type ExistenceFilterMismatchCallback = ( * `disablePersistentCacheIndexAutoCreation()` completes successfully, or will * be rejected if it fails. * - * The return value of the callback, if any, is ignored. + * The return value, if any, is ignored. * * @internal */ -export interface PersistentCacheIndexAutoCreationToggleCallback { - (promise: Promise): unknown; -} +export type PersistentCacheIndexAutoCreationToggleCallback = ( + promise: Promise +) => unknown; /** * The implementation of `TestingHooksSpi`. From ab8fd36ef3203a423226b43347bec715b48ff123 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 11 Aug 2023 13:57:48 -0400 Subject: [PATCH 09/28] testing_hooks.ts: add registerCallback() --- packages/firestore/src/util/testing_hooks.ts | 33 ++++++++++++-------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/packages/firestore/src/util/testing_hooks.ts b/packages/firestore/src/util/testing_hooks.ts index 72ab6ccad8f..1941f23ed77 100644 --- a/packages/firestore/src/util/testing_hooks.ts +++ b/packages/firestore/src/util/testing_hooks.ts @@ -156,10 +156,8 @@ class TestingHooksSpiImpl implements TestingHooksSpi { ExistenceFilterMismatchCallback >(); - private readonly persistentCacheIndexAutoCreationToggleCallbacks = new Map< - Symbol, - PersistentCacheIndexAutoCreationToggleCallback - >(); + private readonly persistentCacheIndexAutoCreationToggleCallbacksById = + new Map(); private constructor() {} @@ -180,14 +178,14 @@ class TestingHooksSpiImpl implements TestingHooksSpi { onExistenceFilterMismatch( callback: ExistenceFilterMismatchCallback ): Unsubscribe { - const id = Symbol(); - const callbacks = this.existenceFilterMismatchCallbacksById; - callbacks.set(id, callback); - return () => callbacks.delete(id); + return registerCallback( + callback, + this.existenceFilterMismatchCallbacksById + ); } notifyPersistentCacheIndexAutoCreationToggle(promise: Promise): void { - this.persistentCacheIndexAutoCreationToggleCallbacks.forEach(callback => + this.persistentCacheIndexAutoCreationToggleCallbacksById.forEach(callback => callback(promise) ); } @@ -195,11 +193,20 @@ class TestingHooksSpiImpl implements TestingHooksSpi { onPersistentCacheIndexAutoCreationToggle( callback: PersistentCacheIndexAutoCreationToggleCallback ): Unsubscribe { - const id = Symbol(); - const callbacks = this.persistentCacheIndexAutoCreationToggleCallbacks; - callbacks.set(id, callback); - return () => callbacks.delete(id); + return registerCallback( + callback, + this.persistentCacheIndexAutoCreationToggleCallbacksById + ); } } +function registerCallback( + callback: T, + callbacks: Map +): Unsubscribe { + const id = Symbol(); + callbacks.set(id, callback); + return () => callbacks.delete(id); +} + let testingHooksSpiImplInstance: TestingHooksSpiImpl | null = null; From fadb79bbc69a61c054c0519f7a85ca16b3c19b12 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 11 Aug 2023 15:31:40 -0400 Subject: [PATCH 10/28] rework testing hooks --- .../src/api/persistent_cache_index_manager.ts | 25 +++- .../firestore/src/core/firestore_client.ts | 19 ++- .../firestore/src/local/local_store_impl.ts | 24 ++-- packages/firestore/src/util/testing_hooks.ts | 24 ++++ .../persistent_cache_index_manager.test.ts | 117 ++++++++++++------ .../integration/util/testing_hooks_util.ts | 21 ++++ .../test/unit/local/local_store.test.ts | 30 ++--- 7 files changed, 192 insertions(+), 68 deletions(-) diff --git a/packages/firestore/src/api/persistent_cache_index_manager.ts b/packages/firestore/src/api/persistent_cache_index_manager.ts index 83398149de3..af11223d695 100644 --- a/packages/firestore/src/api/persistent_cache_index_manager.ts +++ b/packages/firestore/src/api/persistent_cache_index_manager.ts @@ -17,7 +17,8 @@ import { firestoreClientSetPersistentCacheIndexAutoCreationEnabled, - FirestoreClient + FirestoreClient, + TestingHooks as FirestoreClientTestingHooks } from '../core/firestore_client'; import { cast } from '../util/input_validation'; import { logDebug, logWarn } from '../util/log'; @@ -141,3 +142,25 @@ const persistentCacheIndexManagerByFirestore = new WeakMap< Firestore, PersistentCacheIndexManager >(); + +/** + * Test-only hooks into the SDK for use exclusively by integration tests. + */ +export class TestingHooks { + private constructor() { + throw new Error('creating instances is not supported'); + } + + static setIndexAutoCreationSettings( + indexManager: PersistentCacheIndexManager, + settings: { + indexAutoCreationMinCollectionSize?: number; + relativeIndexReadCostPerDocument?: number; + } + ): Promise { + return FirestoreClientTestingHooks.setPersistentCacheIndexAutoCreationSettings( + indexManager._client, + settings + ); + } +} diff --git a/packages/firestore/src/core/firestore_client.ts b/packages/firestore/src/core/firestore_client.ts index 69792309eeb..b32e10eb6dd 100644 --- a/packages/firestore/src/core/firestore_client.ts +++ b/packages/firestore/src/core/firestore_client.ts @@ -51,7 +51,7 @@ import { remoteStoreHandleCredentialChange } from '../remote/remote_store'; import { JsonProtoSerializer } from '../remote/serializer'; -import { debugAssert } from '../util/assert'; +import { debugAssert, debugCast } from '../util/assert'; import { AsyncObserver } from '../util/async_observer'; import { AsyncQueue, wrapInUserErrorIfRecoverable } from '../util/async_queue'; import { BundleReader } from '../util/bundle_reader'; @@ -861,4 +861,21 @@ export class TestingHooks { return LocalStoreTestingHooks.getQueryIndexType(localStore, query); }); } + + static setPersistentCacheIndexAutoCreationSettings( + client: FirestoreClient, + settings: { + indexAutoCreationMinCollectionSize?: number; + relativeIndexReadCostPerDocument?: number; + } + ): Promise { + const settingsCopy = { ...settings }; + return client.asyncQueue.enqueue(async () => { + const localStore = await getLocalStore(client); + LocalStoreTestingHooks.setIndexAutoCreationSettings( + localStore, + settingsCopy + ); + }); + } } diff --git a/packages/firestore/src/local/local_store_impl.ts b/packages/firestore/src/local/local_store_impl.ts index ce1a2e9ae86..85ffef8ecfb 100644 --- a/packages/firestore/src/local/local_store_impl.ts +++ b/packages/firestore/src/local/local_store_impl.ts @@ -1543,20 +1543,22 @@ export class TestingHooks { throw new Error('creating instances is not supported'); } - static setIndexAutoCreationMinCollectionSize( + static setIndexAutoCreationSettings( localStore: LocalStore, - newValue: number - ): void { - const localStoreImpl = debugCast(localStore, LocalStoreImpl); - localStoreImpl.queryEngine.indexAutoCreationMinCollectionSize = newValue; - } - - static setRelativeIndexReadCostPerDocument( - localStore: LocalStore, - newValue: number + settings: { + indexAutoCreationMinCollectionSize?: number; + relativeIndexReadCostPerDocument?: number; + } ): void { const localStoreImpl = debugCast(localStore, LocalStoreImpl); - localStoreImpl.queryEngine.relativeIndexReadCostPerDocument = newValue; + if (settings.indexAutoCreationMinCollectionSize !== undefined) { + localStoreImpl.queryEngine.indexAutoCreationMinCollectionSize = + settings.indexAutoCreationMinCollectionSize; + } + if (settings.relativeIndexReadCostPerDocument !== undefined) { + localStoreImpl.queryEngine.relativeIndexReadCostPerDocument = + settings.relativeIndexReadCostPerDocument; + } } static getQueryIndexType( diff --git a/packages/firestore/src/util/testing_hooks.ts b/packages/firestore/src/util/testing_hooks.ts index 1941f23ed77..5712137ccc7 100644 --- a/packages/firestore/src/util/testing_hooks.ts +++ b/packages/firestore/src/util/testing_hooks.ts @@ -16,6 +16,10 @@ */ import { ensureFirestoreConfigured, Firestore } from '../api/database'; +import { + PersistentCacheIndexManager, + TestingHooks as PersistentCacheIndexManagerTestingHooks +} from '../api/persistent_cache_index_manager'; import { Unsubscribe } from '../api/reference_impl'; import { TestingHooks as FirestoreClientTestingHooks } from '../core/firestore_client'; import { Query } from '../lite-api/reference'; @@ -116,6 +120,26 @@ export class TestingHooks { throw new Error(`unrecognized IndexType: ${indexType}`); } } + + /** + * Sets the persistent cache index auto-creation settings for the given + * Firestore instance. + * + * @return a Promise that is fulfilled when the settings are successfully + * applied, or rejected if applying the settings fails. + */ + static setPersistentCacheIndexAutoCreationSettings( + indexManager: PersistentCacheIndexManager, + settings: { + indexAutoCreationMinCollectionSize?: number; + relativeIndexReadCostPerDocument?: number; + } + ): Promise { + return PersistentCacheIndexManagerTestingHooks.setIndexAutoCreationSettings( + indexManager, + settings + ); + } } /** diff --git a/packages/firestore/test/integration/api/persistent_cache_index_manager.test.ts b/packages/firestore/test/integration/api/persistent_cache_index_manager.test.ts index 799b88b3dd2..029e25c1b26 100644 --- a/packages/firestore/test/integration/api/persistent_cache_index_manager.test.ts +++ b/packages/firestore/test/integration/api/persistent_cache_index_manager.test.ts @@ -26,7 +26,8 @@ import { _disablePersistentCacheIndexAutoCreation as disablePersistentCacheIndexAutoCreation, _enablePersistentCacheIndexAutoCreation as enablePersistentCacheIndexAutoCreation, _getPersistentCacheIndexManager as getPersistentCacheIndexManager, - _PersistentCacheIndexManager as PersistentCacheIndexManager + _PersistentCacheIndexManager as PersistentCacheIndexManager, + _TestingHooks as TestingHooks } from '../util/firebase_export'; import { apiDescribe, @@ -36,6 +37,7 @@ import { } from '../util/helpers'; import { getQueryIndexType, + setPersistentCacheIndexAutoCreationSettings, verifyPersistentCacheIndexAutoCreationToggleSucceedsDuring } from '../util/testing_hooks_util'; @@ -163,52 +165,80 @@ apiDescribe('PersistentCacheIndexManager', persistence => { describe('Query execution', () => { it('Auto-indexing is disabled by default', () => testIndexesGetAutoCreated({ - documentCounts: { matching: 1, notMatching: 100 }, + documentCounts: { matching: 1, notMatching: 5 }, expectedIndexAutoCreated: false, - indexAutoCreationEnabled: false + indexAutoCreationEnabled: false, + indexAutoCreationMinCollectionSize: 0, + relativeIndexReadCostPerDocument: 2 })); - it('Index is not auto-created if collection size is less than 100', () => - testIndexesGetAutoCreated({ - documentCounts: { matching: 1, notMatching: 98 }, - expectedIndexAutoCreated: false - })); - - it('Index is not auto-created if 50% of documents match', () => - testIndexesGetAutoCreated({ - documentCounts: { matching: 50, notMatching: 50 }, - expectedIndexAutoCreated: false - })); - - it('Index is auto-created if 49% of documents match and collection size is 100', () => - testIndexesGetAutoCreated({ - documentCounts: { matching: 49, notMatching: 51 }, - expectedIndexAutoCreated: true - })); + it( + 'Default indexAutoCreationMinCollectionSize=100: ' + + 'index should *not* be auto-created if lookup scanned 99 documents', + () => + testIndexesGetAutoCreated({ + documentCounts: { matching: 1, notMatching: 98 }, + expectedIndexAutoCreated: false + }) + ); + + it( + 'Default indexAutoCreationMinCollectionSize=100' + + ' index *should* be auto-created if lookup scanned 100 documents', + () => + testIndexesGetAutoCreated({ + documentCounts: { matching: 1, notMatching: 99 }, + expectedIndexAutoCreated: true + }) + ); + + it( + 'Default relativeIndexReadCostPerDocument=2: ' + + 'index should *not* be auto-created if the relative index read cost is matched exactly', + () => + testIndexesGetAutoCreated({ + documentCounts: { matching: 50, notMatching: 50 }, + expectedIndexAutoCreated: false + }) + ); + + it( + 'Default relativeIndexReadCostPerDocument=2: ' + + 'index *should* be auto-created if the relative index read cost is exceeded slightly', + () => + testIndexesGetAutoCreated({ + documentCounts: { matching: 49, notMatching: 51 }, + expectedIndexAutoCreated: true + }) + ); it('Indexes are only auto-created when enabled', async () => { const testDocs = partitionedTestDocs({ FooMatches: { documentData: { foo: 'match' }, - documentCount: 11 + documentCount: 1 }, BarMatches: { documentData: { bar: 'match' }, - documentCount: 22 + documentCount: 2 }, BazMatches: { documentData: { baz: 'match' }, - documentCount: 33 + documentCount: 3 }, - NeitherFooNorBarMatch: { - documentData: { foo: 'nomatch', bar: 'nomatch' }, - documentCount: 100 + NeitherFooNorBarNorMazMatch: { + documentData: { foo: 'nomatch', bar: 'nomatch', baz: 'nomatch' }, + documentCount: 10 } }); return withTestCollection(persistence, testDocs, async (coll, db) => { const indexManager = getPersistentCacheIndexManager(db)!; expect(indexManager, 'indexManager').is.not.null; + await setPersistentCacheIndexAutoCreationSettings(indexManager, { + indexAutoCreationMinCollectionSize: 0, + relativeIndexReadCostPerDocument: 2 + }); // Populate the local cache with the entire collection. await getDocs(coll); @@ -218,7 +248,7 @@ apiDescribe('PersistentCacheIndexManager', persistence => { enablePersistentCacheIndexAutoCreation(indexManager); const query1 = query(coll, where('foo', '==', 'match')); const snapshot1 = await getDocsFromCache(query1); - expect(snapshot1.size, 'snapshot1.size').to.equal(11); + expect(snapshot1.size, 'snapshot1.size').to.equal(1); expect( await getQueryIndexType(query1), 'getQueryIndexType(query1)' @@ -229,7 +259,7 @@ apiDescribe('PersistentCacheIndexManager', persistence => { disablePersistentCacheIndexAutoCreation(indexManager); const query2 = query(coll, where('bar', '==', 'match')); const snapshot2 = await getDocsFromCache(query2); - expect(snapshot2.size, 'snapshot2.size').to.equal(22); + expect(snapshot2.size, 'snapshot2.size').to.equal(2); expect( await getQueryIndexType(query2), 'getQueryIndexType(query2)' @@ -244,7 +274,7 @@ apiDescribe('PersistentCacheIndexManager', persistence => { enablePersistentCacheIndexAutoCreation(indexManager); const query3 = query(coll, where('baz', '==', 'match')); const snapshot3 = await getDocsFromCache(query3); - expect(snapshot3.size, 'snapshot3.size').to.equal(33); + expect(snapshot3.size, 'snapshot3.size').to.equal(3); expect( await getQueryIndexType(query3), 'getQueryIndexType(query3)' @@ -264,15 +294,15 @@ apiDescribe('PersistentCacheIndexManager', persistence => { const testDocs = partitionedTestDocs({ FooMatches: { documentData: { foo: 'match' }, - documentCount: 99 + documentCount: 5 }, - FooBarMatches: { + FooAndBarBothMatch: { documentData: { foo: 'match', bar: 'match' }, documentCount: 1 }, NeitherFooNorBarMatch: { documentData: { foo: 'nomatch', bar: 'nomatch' }, - documentCount: 110 + documentCount: 15 } }); @@ -280,6 +310,10 @@ apiDescribe('PersistentCacheIndexManager', persistence => { const indexManager = getPersistentCacheIndexManager(db)!; expect(indexManager, 'indexManager').is.not.null; enablePersistentCacheIndexAutoCreation(indexManager); + await setPersistentCacheIndexAutoCreationSettings(indexManager, { + indexAutoCreationMinCollectionSize: 0, + relativeIndexReadCostPerDocument: 2 + }); // Populate the local cache with the entire collection. await getDocs(coll); @@ -288,7 +322,7 @@ apiDescribe('PersistentCacheIndexManager', persistence => { { const fooQuery = query(coll, where('foo', '==', 'match')); const fooSnapshot = await getDocsFromCache(fooQuery); - expect(fooSnapshot.size, 'fooSnapshot.size').to.equal(100); + expect(fooSnapshot.size, 'fooSnapshot.size').to.equal(6); expect( await getQueryIndexType(fooQuery), 'getQueryIndexType(fooQuery)' @@ -324,6 +358,8 @@ apiDescribe('PersistentCacheIndexManager', persistence => { documentCounts: { matching: number; notMatching: number }; expectedIndexAutoCreated: boolean; indexAutoCreationEnabled?: boolean; + indexAutoCreationMinCollectionSize?: number; + relativeIndexReadCostPerDocument?: number; }): Promise { const testDocs = partitionedTestDocs({ matching: { @@ -340,12 +376,21 @@ apiDescribe('PersistentCacheIndexManager', persistence => { // Populate the local cache with the entire collection. await getDocs(coll); - // Enable or disable automatic index creation, as requested. + // Configure automatic index creation, as requested. + const indexManager = getPersistentCacheIndexManager(db)!; + expect(indexManager, 'indexManager').is.not.null; if (config.indexAutoCreationEnabled ?? true) { - const indexManager = getPersistentCacheIndexManager(db)!; - expect(indexManager, 'indexManager').is.not.null; enablePersistentCacheIndexAutoCreation(indexManager); } + if ( + config.indexAutoCreationMinCollectionSize !== undefined || + config.relativeIndexReadCostPerDocument !== undefined + ) { + await TestingHooks.setPersistentCacheIndexAutoCreationSettings( + indexManager, + config + ); + } // Run a query against the local cache that matches a _subset_ of the // entire collection. diff --git a/packages/firestore/test/integration/util/testing_hooks_util.ts b/packages/firestore/test/integration/util/testing_hooks_util.ts index e1814370986..264a9556ea3 100644 --- a/packages/firestore/test/integration/util/testing_hooks_util.ts +++ b/packages/firestore/test/integration/util/testing_hooks_util.ts @@ -21,6 +21,7 @@ import { DocumentData, DocumentReference, Query, + _PersistentCacheIndexManager as PersistentCacheIndexManager, _TestingHooks as TestingHooks, _TestingHooksExistenceFilterMismatchInfo as ExistenceFilterMismatchInfoInternal } from './firebase_export'; @@ -149,3 +150,23 @@ export function getQueryIndexType< ): Promise<'full' | 'partial' | 'none'> { return TestingHooks.getQueryIndexType(query); } + +/** + * Sets the persistent cache index auto-creation settings for the given + * Firestore instance. + * + * @return a Promise that is fulfilled when the settings are successfully + * applied, or rejected if applying the settings fails. + */ +export function setPersistentCacheIndexAutoCreationSettings( + indexManager: PersistentCacheIndexManager, + settings: { + indexAutoCreationMinCollectionSize?: number; + relativeIndexReadCostPerDocument?: number; + } +): Promise { + return TestingHooks.setPersistentCacheIndexAutoCreationSettings( + indexManager, + settings + ); +} diff --git a/packages/firestore/test/unit/local/local_store.test.ts b/packages/firestore/test/unit/local/local_store.test.ts index f2991b4ce1e..526dd33e35a 100644 --- a/packages/firestore/test/unit/local/local_store.test.ts +++ b/packages/firestore/test/unit/local/local_store.test.ts @@ -346,7 +346,7 @@ class LocalStoreTester { afterIndexAutoCreationConfigure(config: { enabled?: boolean; - autoCreationMinCollectionSize?: number; + indexAutoCreationMinCollectionSize?: number; relativeIndexReadCostPerDocument?: number; }): LocalStoreTester { this.prepareNextStep(); @@ -355,18 +355,10 @@ class LocalStoreTester { if (config.enabled !== undefined) { localStoreSetIndexAutoCreationEnabled(this.localStore, config.enabled); } - if (config.autoCreationMinCollectionSize !== undefined) { - LocalStoreTestingHooks.setIndexAutoCreationMinCollectionSize( - this.localStore, - config.autoCreationMinCollectionSize - ); - } - if (config.relativeIndexReadCostPerDocument !== undefined) { - LocalStoreTestingHooks.setRelativeIndexReadCostPerDocument( - this.localStore, - config.relativeIndexReadCostPerDocument - ); - } + LocalStoreTestingHooks.setIndexAutoCreationSettings( + this.localStore, + config + ); }); return this; @@ -2733,7 +2725,7 @@ function indexedDbLocalStoreTests( .toReturnTargetId(2) .afterIndexAutoCreationConfigure({ enabled: true, - autoCreationMinCollectionSize: 0, + indexAutoCreationMinCollectionSize: 0, relativeIndexReadCostPerDocument: 2 }) .afterRemoteEvents([ @@ -2801,7 +2793,7 @@ function indexedDbLocalStoreTests( .toReturnTargetId(2) .afterIndexAutoCreationConfigure({ enabled: true, - autoCreationMinCollectionSize: 0, + indexAutoCreationMinCollectionSize: 0, relativeIndexReadCostPerDocument: 5 }) .afterRemoteEvents([ @@ -2838,7 +2830,7 @@ function indexedDbLocalStoreTests( .toReturnTargetId(2) .afterIndexAutoCreationConfigure({ enabled: true, - autoCreationMinCollectionSize: 0, + indexAutoCreationMinCollectionSize: 0, relativeIndexReadCostPerDocument: 2 }) .afterRemoteEvents([ @@ -2874,7 +2866,7 @@ function indexedDbLocalStoreTests( .toReturnTargetId(2) .afterIndexAutoCreationConfigure({ enabled: true, - autoCreationMinCollectionSize: 0, + indexAutoCreationMinCollectionSize: 0, relativeIndexReadCostPerDocument: 2 }) .afterRemoteEvents([ @@ -2912,7 +2904,7 @@ function indexedDbLocalStoreTests( .toReturnTargetId(2) .afterIndexAutoCreationConfigure({ enabled: true, - autoCreationMinCollectionSize: 0, + indexAutoCreationMinCollectionSize: 0, relativeIndexReadCostPerDocument: 2 }) .afterRemoteEvents([ @@ -2962,7 +2954,7 @@ function indexedDbLocalStoreTests( .toReturnTargetId(2) .afterIndexAutoCreationConfigure({ enabled: true, - autoCreationMinCollectionSize: 0, + indexAutoCreationMinCollectionSize: 0, relativeIndexReadCostPerDocument: 2 }) .afterRemoteEvents([ From c681d3d81665671bff66a63ac7c5932c6ca15dbf Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Sat, 12 Aug 2023 10:25:41 -0400 Subject: [PATCH 11/28] run_tests_in_ci.js rewrite --- scripts/run_tests_in_ci.js | 202 +++++++++++++++++++++++++------------ 1 file changed, 140 insertions(+), 62 deletions(-) diff --git a/scripts/run_tests_in_ci.js b/scripts/run_tests_in_ci.js index a5ba6cd3289..8d5f8df6fc4 100644 --- a/scripts/run_tests_in_ci.js +++ b/scripts/run_tests_in_ci.js @@ -1,6 +1,6 @@ /** * @license - * Copyright 2020 Google LLC + * Copyright 2023 Google LLC * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -17,77 +17,155 @@ const yargs = require('yargs'); const path = require('path'); -const { spawn } = require('child-process-promise'); -const { writeFileSync } = require('fs'); +const child_process = require('node:child_process'); +const fs = require('node:fs'); const LOGDIR = process.env.CI ? process.env.HOME : '/tmp'; -// Maps the packages where we should not run `test:all` and instead isolate the cross-browser tests. -// TODO(dwyfrequency): Update object with `storage` and `firestore` packages. -const crossBrowserPackages = { - 'packages/auth': 'test:browser:unit', - 'packages/auth-compat': 'test:browser:unit', - 'packages/firestore': 'test:browser:unit', - 'packages/firestore-compat': 'test:browser' -}; - -function writeLogs(status, name, logText) { - const safeName = name.replace(/@/g, 'at_').replace(/\//g, '_'); - writeFileSync(path.join(LOGDIR, `${safeName}-ci-log.txt`), logText, { - encoding: 'utf8' - }); - writeFileSync( - path.join(LOGDIR, `${safeName}-ci-summary.txt`), - `${status}: ${name}`, - { encoding: 'utf8' } + +async function main() { + const { scriptName, workingDir } = parseArgs(); + const { name } = require(`${workingDir}/package.json`); + logPrefix = name; + + const logFilePath = path.join(LOGDIR, `${makeSafePath(name)}-ci-log.txt`); + const testProcessExitCode = await runTestProcess( + workingDir, + scriptName, + logFilePath + ); + + const summaryFilePath = path.join( + LOGDIR, + `${makeSafePath(name)}-ci-summary.txt` ); + writeSummaryFile(summaryFilePath, name, testProcessExitCode === 0); + + await printFile(logFilePath); + + process.exit(testProcessExitCode); } -const argv = yargs.options({ - d: { - type: 'string', - desc: 'current working directory', - default: '.' - }, - s: { - type: 'string', - desc: 'the npm script to run', - default: 'test' +async function runTestProcess(workingDir, scriptName, logFilePath) { + log(`Saving test process output to file: ${logFilePath}`); + const logFileHandle = fs.openSync(logFilePath, 'w'); + try { + const args = ['yarn', '--cwd', workingDir, scriptName]; + log(`Starting test process: ${args.join(' ')}`); + const proc = child_process.spawn(args[0], args.splice(1), { + stdio: ['inherit', logFileHandle, logFileHandle] + }); + proc.once('spawn', () => log(`Started test process with PID: ${proc.pid}`)); + const exitCode = await new Promise((resolve, reject) => { + proc.once('close', resolve); + proc.once('error', reject); + }); + log(`Test process completed with exit code: ${exitCode}`); + return exitCode; + } finally { + fs.close(logFileHandle); } -}).argv; +} -(async () => { - const myPath = argv.d; - let scriptName = argv.s; - const dir = path.resolve(myPath); - const { name } = require(`${dir}/package.json`); +function writeSummaryFile(summaryFilePath, name, testProcessSuccessful) { + const statusString = testProcessSuccessful ? 'Success' : 'Failure'; + const line = `${statusString}: ${name}`; + log(`Writing summary to file ${summaryFilePath}: ${line}`); + fs.writeFileSync(summaryFilePath, line, { encoding: 'utf8' }); +} - let stdout = ''; - let stderr = ''; - try { - if (process.env?.BROWSERS) { - for (const package in crossBrowserPackages) { - if (dir.endsWith(package)) { - scriptName = crossBrowserPackages[package]; - } +async function printFile(path) { + log('========================================================'); + log(`==== BEGIN ${path}`); + log('========================================================'); + const readStream = fs.createReadStream(path); + readStream.pipe(process.stdout); + await new Promise((resolve, reject) => { + readStream.once('end', resolve); + readStream.once('error', reject); + }); + log('========================================================'); + log(`==== END ${path}`); + log('========================================================'); +} + +let logPrefix = ''; + +function log() { + console.log('run_tests_in_ci.js', logPrefix, elapsedTimeStr(), ...arguments); +} + +function makeSafePath(s) { + return s.replace(/@/g, 'at_').replace(/\//g, '_'); +} + +function parseArgs() { + const argv = yargs.options({ + d: { + type: 'string', + desc: 'current working directory', + default: '.' + }, + s: { + type: 'string', + desc: 'the npm script to run', + default: 'test' + } + }).argv; + + return { + scriptName: resolveScriptNameArg(argv.s), + workingDir: path.resolve(argv.d) + }; +} + +function resolveScriptNameArg(scriptName) { + // Maps the packages where we should not run `test:all` and instead isolate the cross-browser tests. + // TODO(dwyfrequency): Update object with `storage` and `firestore` packages. + const crossBrowserPackages = { + 'packages/auth': 'test:browser:unit', + 'packages/auth-compat': 'test:browser:unit', + 'packages/firestore': 'test:browser:unit', + 'packages/firestore-compat': 'test:browser' + }; + + if (process.env?.BROWSERS) { + for (const package in crossBrowserPackages) { + if (dir.endsWith(package)) { + scriptName = crossBrowserPackages[package]; } } - const testProcess = spawn('yarn', ['--cwd', dir, scriptName]); + } - testProcess.childProcess.stdout.on('data', data => { - stdout += data.toString(); - }); - testProcess.childProcess.stderr.on('data', data => { - stderr += data.toString(); - }); + return scriptName; +} + +let startTime = null; - await testProcess; - console.log('Success: ' + name); - writeLogs('Success', name, stdout + '\n' + stderr); - } catch (e) { - console.error('Failure: ' + name); - console.log(stdout); - console.error(stderr); - writeLogs('Failure', name, stdout + '\n' + stderr); - process.exit(1); +function getElapsedMilliseconds() { + const currentTimeMilliseconds = getCurrentMonotonicTimeMilliseconds(); + if (startTime === null) { + startTime = currentTimeMilliseconds; + return 0; } -})(); + return currentTimeMilliseconds - startTime; +} + +function elapsedTimeStr() { + const milliseconds = getElapsedMilliseconds(); + const minutes = Math.floor(milliseconds / (1000 * 60)); + const seconds = (milliseconds - minutes * 1000 * 60) / 1000; + return ( + (minutes < 10 ? '0' : '') + + minutes + + ':' + + (seconds < 10 ? '0' : '') + + seconds.toFixed(3) + ); +} + +function getCurrentMonotonicTimeMilliseconds() { + const currentTime = process.hrtime(); + return currentTime[0] * 1000 + currentTime[1] / 1_000_000; +} + +main(); From c4822ce56b5f04023751afe0797fb59503d712f1 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Mon, 14 Aug 2023 11:35:37 -0400 Subject: [PATCH 12/28] run_tests_in_ci.js: set `process.exitCode=1` instead of calling `process.exit(1)` to fix truncated output --- scripts/run_tests_in_ci.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/scripts/run_tests_in_ci.js b/scripts/run_tests_in_ci.js index a5ba6cd3289..03c4377c45a 100644 --- a/scripts/run_tests_in_ci.js +++ b/scripts/run_tests_in_ci.js @@ -88,6 +88,10 @@ const argv = yargs.options({ console.log(stdout); console.error(stderr); writeLogs('Failure', name, stdout + '\n' + stderr); - process.exit(1); + + // NOTE: Set `process.exitCode` rather than calling `process.exit()` because + // the latter will exit forcefully even if stdout/stderr have not been fully + // flushed, leading to truncated output. + process.exitCode = 1; } })(); From 526b5613d1b25940cb4a421054f9917110153155 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Mon, 14 Aug 2023 11:55:52 -0400 Subject: [PATCH 13/28] run_tests_in_ci.js: revert local modifications --- scripts/run_tests_in_ci.js | 202 ++++++++++++------------------------- 1 file changed, 62 insertions(+), 140 deletions(-) diff --git a/scripts/run_tests_in_ci.js b/scripts/run_tests_in_ci.js index 8d5f8df6fc4..a5ba6cd3289 100644 --- a/scripts/run_tests_in_ci.js +++ b/scripts/run_tests_in_ci.js @@ -1,6 +1,6 @@ /** * @license - * Copyright 2023 Google LLC + * Copyright 2020 Google LLC * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -17,155 +17,77 @@ const yargs = require('yargs'); const path = require('path'); -const child_process = require('node:child_process'); -const fs = require('node:fs'); +const { spawn } = require('child-process-promise'); +const { writeFileSync } = require('fs'); const LOGDIR = process.env.CI ? process.env.HOME : '/tmp'; - -async function main() { - const { scriptName, workingDir } = parseArgs(); - const { name } = require(`${workingDir}/package.json`); - logPrefix = name; - - const logFilePath = path.join(LOGDIR, `${makeSafePath(name)}-ci-log.txt`); - const testProcessExitCode = await runTestProcess( - workingDir, - scriptName, - logFilePath - ); - - const summaryFilePath = path.join( - LOGDIR, - `${makeSafePath(name)}-ci-summary.txt` +// Maps the packages where we should not run `test:all` and instead isolate the cross-browser tests. +// TODO(dwyfrequency): Update object with `storage` and `firestore` packages. +const crossBrowserPackages = { + 'packages/auth': 'test:browser:unit', + 'packages/auth-compat': 'test:browser:unit', + 'packages/firestore': 'test:browser:unit', + 'packages/firestore-compat': 'test:browser' +}; + +function writeLogs(status, name, logText) { + const safeName = name.replace(/@/g, 'at_').replace(/\//g, '_'); + writeFileSync(path.join(LOGDIR, `${safeName}-ci-log.txt`), logText, { + encoding: 'utf8' + }); + writeFileSync( + path.join(LOGDIR, `${safeName}-ci-summary.txt`), + `${status}: ${name}`, + { encoding: 'utf8' } ); - writeSummaryFile(summaryFilePath, name, testProcessExitCode === 0); - - await printFile(logFilePath); - - process.exit(testProcessExitCode); } -async function runTestProcess(workingDir, scriptName, logFilePath) { - log(`Saving test process output to file: ${logFilePath}`); - const logFileHandle = fs.openSync(logFilePath, 'w'); - try { - const args = ['yarn', '--cwd', workingDir, scriptName]; - log(`Starting test process: ${args.join(' ')}`); - const proc = child_process.spawn(args[0], args.splice(1), { - stdio: ['inherit', logFileHandle, logFileHandle] - }); - proc.once('spawn', () => log(`Started test process with PID: ${proc.pid}`)); - const exitCode = await new Promise((resolve, reject) => { - proc.once('close', resolve); - proc.once('error', reject); - }); - log(`Test process completed with exit code: ${exitCode}`); - return exitCode; - } finally { - fs.close(logFileHandle); +const argv = yargs.options({ + d: { + type: 'string', + desc: 'current working directory', + default: '.' + }, + s: { + type: 'string', + desc: 'the npm script to run', + default: 'test' } -} - -function writeSummaryFile(summaryFilePath, name, testProcessSuccessful) { - const statusString = testProcessSuccessful ? 'Success' : 'Failure'; - const line = `${statusString}: ${name}`; - log(`Writing summary to file ${summaryFilePath}: ${line}`); - fs.writeFileSync(summaryFilePath, line, { encoding: 'utf8' }); -} - -async function printFile(path) { - log('========================================================'); - log(`==== BEGIN ${path}`); - log('========================================================'); - const readStream = fs.createReadStream(path); - readStream.pipe(process.stdout); - await new Promise((resolve, reject) => { - readStream.once('end', resolve); - readStream.once('error', reject); - }); - log('========================================================'); - log(`==== END ${path}`); - log('========================================================'); -} - -let logPrefix = ''; - -function log() { - console.log('run_tests_in_ci.js', logPrefix, elapsedTimeStr(), ...arguments); -} +}).argv; -function makeSafePath(s) { - return s.replace(/@/g, 'at_').replace(/\//g, '_'); -} - -function parseArgs() { - const argv = yargs.options({ - d: { - type: 'string', - desc: 'current working directory', - default: '.' - }, - s: { - type: 'string', - desc: 'the npm script to run', - default: 'test' - } - }).argv; - - return { - scriptName: resolveScriptNameArg(argv.s), - workingDir: path.resolve(argv.d) - }; -} +(async () => { + const myPath = argv.d; + let scriptName = argv.s; + const dir = path.resolve(myPath); + const { name } = require(`${dir}/package.json`); -function resolveScriptNameArg(scriptName) { - // Maps the packages where we should not run `test:all` and instead isolate the cross-browser tests. - // TODO(dwyfrequency): Update object with `storage` and `firestore` packages. - const crossBrowserPackages = { - 'packages/auth': 'test:browser:unit', - 'packages/auth-compat': 'test:browser:unit', - 'packages/firestore': 'test:browser:unit', - 'packages/firestore-compat': 'test:browser' - }; - - if (process.env?.BROWSERS) { - for (const package in crossBrowserPackages) { - if (dir.endsWith(package)) { - scriptName = crossBrowserPackages[package]; + let stdout = ''; + let stderr = ''; + try { + if (process.env?.BROWSERS) { + for (const package in crossBrowserPackages) { + if (dir.endsWith(package)) { + scriptName = crossBrowserPackages[package]; + } } } - } - - return scriptName; -} + const testProcess = spawn('yarn', ['--cwd', dir, scriptName]); -let startTime = null; + testProcess.childProcess.stdout.on('data', data => { + stdout += data.toString(); + }); + testProcess.childProcess.stderr.on('data', data => { + stderr += data.toString(); + }); -function getElapsedMilliseconds() { - const currentTimeMilliseconds = getCurrentMonotonicTimeMilliseconds(); - if (startTime === null) { - startTime = currentTimeMilliseconds; - return 0; + await testProcess; + console.log('Success: ' + name); + writeLogs('Success', name, stdout + '\n' + stderr); + } catch (e) { + console.error('Failure: ' + name); + console.log(stdout); + console.error(stderr); + writeLogs('Failure', name, stdout + '\n' + stderr); + process.exit(1); } - return currentTimeMilliseconds - startTime; -} - -function elapsedTimeStr() { - const milliseconds = getElapsedMilliseconds(); - const minutes = Math.floor(milliseconds / (1000 * 60)); - const seconds = (milliseconds - minutes * 1000 * 60) / 1000; - return ( - (minutes < 10 ? '0' : '') + - minutes + - ':' + - (seconds < 10 ? '0' : '') + - seconds.toFixed(3) - ); -} - -function getCurrentMonotonicTimeMilliseconds() { - const currentTime = process.hrtime(); - return currentTime[0] * 1000 + currentTime[1] / 1_000_000; -} - -main(); +})(); From 24dc0fa595263068acf59b59cf86e3567d50c9f9 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Mon, 14 Aug 2023 21:27:55 -0400 Subject: [PATCH 14/28] undo changes to run_tests_in_ci.js because they aren't part of this PR --- scripts/run_tests_in_ci.js | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/scripts/run_tests_in_ci.js b/scripts/run_tests_in_ci.js index 03c4377c45a..a5ba6cd3289 100644 --- a/scripts/run_tests_in_ci.js +++ b/scripts/run_tests_in_ci.js @@ -88,10 +88,6 @@ const argv = yargs.options({ console.log(stdout); console.error(stderr); writeLogs('Failure', name, stdout + '\n' + stderr); - - // NOTE: Set `process.exitCode` rather than calling `process.exit()` because - // the latter will exit forcefully even if stdout/stderr have not been fully - // flushed, leading to truncated output. - process.exitCode = 1; + process.exit(1); } })(); From e550a5c52855ff1acbe022003fe53c218edc9e6d Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 15 Aug 2023 08:35:41 -0400 Subject: [PATCH 15/28] empty commit to trigger github actions From 92a6de0a97a147897657fa18939b35c77824c45c Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 15 Aug 2023 11:19:56 -0400 Subject: [PATCH 16/28] firestore_client.ts: remove unused import of `debugCast` --- packages/firestore/src/core/firestore_client.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/firestore/src/core/firestore_client.ts b/packages/firestore/src/core/firestore_client.ts index b32e10eb6dd..eb9432a75fd 100644 --- a/packages/firestore/src/core/firestore_client.ts +++ b/packages/firestore/src/core/firestore_client.ts @@ -51,7 +51,7 @@ import { remoteStoreHandleCredentialChange } from '../remote/remote_store'; import { JsonProtoSerializer } from '../remote/serializer'; -import { debugAssert, debugCast } from '../util/assert'; +import { debugAssert } from '../util/assert'; import { AsyncObserver } from '../util/async_observer'; import { AsyncQueue, wrapInUserErrorIfRecoverable } from '../util/async_queue'; import { BundleReader } from '../util/bundle_reader'; From 4a3f01ce9620a459a462fa9636e98967552e6346 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 16 Aug 2023 15:23:20 -0400 Subject: [PATCH 17/28] enabled -> isEnabled --- .../src/api/persistent_cache_index_manager.ts | 8 +++--- .../firestore/src/core/firestore_client.ts | 4 +-- .../firestore/src/local/local_store_impl.ts | 4 +-- .../test/unit/local/local_store.test.ts | 27 ++++++++++--------- 4 files changed, 23 insertions(+), 20 deletions(-) diff --git a/packages/firestore/src/api/persistent_cache_index_manager.ts b/packages/firestore/src/api/persistent_cache_index_manager.ts index af11223d695..4235a75fa2e 100644 --- a/packages/firestore/src/api/persistent_cache_index_manager.ts +++ b/packages/firestore/src/api/persistent_cache_index_manager.ts @@ -103,26 +103,26 @@ export function disablePersistentCacheIndexAutoCreation( function setPersistentCacheIndexAutoCreationEnabled( indexManager: PersistentCacheIndexManager, - enabled: boolean + isEnabled: boolean ): void { indexManager._client.verifyNotTerminated(); const promise = firestoreClientSetPersistentCacheIndexAutoCreationEnabled( indexManager._client, - enabled + isEnabled ); promise .then(_ => logDebug( `setting persistent cache index auto creation ` + - `enabled=${enabled} succeeded` + `isEnabled=${isEnabled} succeeded` ) ) .catch(error => logWarn( `setting persistent cache index auto creation ` + - `enabled=${enabled} failed`, + `isEnabled=${isEnabled} failed`, error ) ); diff --git a/packages/firestore/src/core/firestore_client.ts b/packages/firestore/src/core/firestore_client.ts index eb9432a75fd..c1e3728b2f3 100644 --- a/packages/firestore/src/core/firestore_client.ts +++ b/packages/firestore/src/core/firestore_client.ts @@ -834,12 +834,12 @@ export function firestoreClientSetIndexConfiguration( export function firestoreClientSetPersistentCacheIndexAutoCreationEnabled( client: FirestoreClient, - enabled: boolean + isEnabled: boolean ): Promise { return client.asyncQueue.enqueue(async () => { return localStoreSetIndexAutoCreationEnabled( await getLocalStore(client), - enabled + isEnabled ); }); } diff --git a/packages/firestore/src/local/local_store_impl.ts b/packages/firestore/src/local/local_store_impl.ts index 85ffef8ecfb..ae1e39b3a7b 100644 --- a/packages/firestore/src/local/local_store_impl.ts +++ b/packages/firestore/src/local/local_store_impl.ts @@ -1529,10 +1529,10 @@ export async function localStoreConfigureFieldIndexes( export function localStoreSetIndexAutoCreationEnabled( localStore: LocalStore, - enabled: boolean + isEnabled: boolean ): void { const localStoreImpl = debugCast(localStore, LocalStoreImpl); - localStoreImpl.queryEngine.indexAutoCreationEnabled = enabled; + localStoreImpl.queryEngine.indexAutoCreationEnabled = isEnabled; } /** diff --git a/packages/firestore/test/unit/local/local_store.test.ts b/packages/firestore/test/unit/local/local_store.test.ts index 526dd33e35a..66009fbe89e 100644 --- a/packages/firestore/test/unit/local/local_store.test.ts +++ b/packages/firestore/test/unit/local/local_store.test.ts @@ -345,15 +345,18 @@ class LocalStoreTester { } afterIndexAutoCreationConfigure(config: { - enabled?: boolean; + isEnabled?: boolean; indexAutoCreationMinCollectionSize?: number; relativeIndexReadCostPerDocument?: number; }): LocalStoreTester { this.prepareNextStep(); this.promiseChain = this.promiseChain.then(() => { - if (config.enabled !== undefined) { - localStoreSetIndexAutoCreationEnabled(this.localStore, config.enabled); + if (config.isEnabled !== undefined) { + localStoreSetIndexAutoCreationEnabled( + this.localStore, + config.isEnabled + ); } LocalStoreTestingHooks.setIndexAutoCreationSettings( this.localStore, @@ -2724,7 +2727,7 @@ function indexedDbLocalStoreTests( .afterAllocatingQuery(query_) .toReturnTargetId(2) .afterIndexAutoCreationConfigure({ - enabled: true, + isEnabled: true, indexAutoCreationMinCollectionSize: 0, relativeIndexReadCostPerDocument: 2 }) @@ -2760,7 +2763,7 @@ function indexedDbLocalStoreTests( .afterAllocatingQuery(query_) .toReturnTargetId(2) .afterIndexAutoCreationConfigure({ - enabled: true, + isEnabled: true, relativeIndexReadCostPerDocument: 2 }) .afterRemoteEvents([ @@ -2792,7 +2795,7 @@ function indexedDbLocalStoreTests( .afterAllocatingQuery(query_) .toReturnTargetId(2) .afterIndexAutoCreationConfigure({ - enabled: true, + isEnabled: true, indexAutoCreationMinCollectionSize: 0, relativeIndexReadCostPerDocument: 5 }) @@ -2829,7 +2832,7 @@ function indexedDbLocalStoreTests( .afterAllocatingQuery(query_) .toReturnTargetId(2) .afterIndexAutoCreationConfigure({ - enabled: true, + isEnabled: true, indexAutoCreationMinCollectionSize: 0, relativeIndexReadCostPerDocument: 2 }) @@ -2865,7 +2868,7 @@ function indexedDbLocalStoreTests( .afterAllocatingQuery(query_) .toReturnTargetId(2) .afterIndexAutoCreationConfigure({ - enabled: true, + isEnabled: true, indexAutoCreationMinCollectionSize: 0, relativeIndexReadCostPerDocument: 2 }) @@ -2883,7 +2886,7 @@ function indexedDbLocalStoreTests( .afterExecutingQuery(query_) .toHaveRead({ documentsByKey: 0, documentsByCollection: 2 }) .toReturnChanged('coll/a', 'coll/e') - .afterIndexAutoCreationConfigure({ enabled: false }) + .afterIndexAutoCreationConfigure({ isEnabled: false }) .afterBackfillIndexes() .afterRemoteEvent( docAddedRemoteEvent(doc('coll/f', 20, { value: 7 }), [2], []) @@ -2903,7 +2906,7 @@ function indexedDbLocalStoreTests( .afterAllocatingQuery(query1) .toReturnTargetId(2) .afterIndexAutoCreationConfigure({ - enabled: true, + isEnabled: true, indexAutoCreationMinCollectionSize: 0, relativeIndexReadCostPerDocument: 2 }) @@ -2921,7 +2924,7 @@ function indexedDbLocalStoreTests( .afterExecutingQuery(query1) .toHaveRead({ documentsByKey: 0, documentsByCollection: 2 }) .toReturnChanged('coll/a', 'coll/e') - .afterIndexAutoCreationConfigure({ enabled: false }) + .afterIndexAutoCreationConfigure({ isEnabled: false }) .afterBackfillIndexes() .afterExecutingQuery(query1) .toHaveRead({ documentsByKey: 2, documentsByCollection: 0 }) @@ -2953,7 +2956,7 @@ function indexedDbLocalStoreTests( .afterAllocatingQuery(query_) .toReturnTargetId(2) .afterIndexAutoCreationConfigure({ - enabled: true, + isEnabled: true, indexAutoCreationMinCollectionSize: 0, relativeIndexReadCostPerDocument: 2 }) From e74f00c6563e539b5234a2911d65ae03c001da7c Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 16 Aug 2023 15:29:32 -0400 Subject: [PATCH 18/28] query_context.ts: make `amount` parameter of the incrementDocumentReadCount() function required (rather than optional) --- packages/firestore/src/local/query_context.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/firestore/src/local/query_context.ts b/packages/firestore/src/local/query_context.ts index 98efb5ab376..1d9891720f9 100644 --- a/packages/firestore/src/local/query_context.ts +++ b/packages/firestore/src/local/query_context.ts @@ -29,10 +29,7 @@ export class QueryContext { return this._documentReadCount; } - incrementDocumentReadCount(amount?: number): void { - if (amount === undefined) { - amount = 1; - } + incrementDocumentReadCount(amount: number): void { this._documentReadCount += amount; } } From a5ed9203c8250315f84bc23f3a1bf7573eddcf2c Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 16 Aug 2023 15:29:58 -0400 Subject: [PATCH 19/28] target_index_matcher.ts: make suggested change to inline comment about unique indexes --- packages/firestore/src/model/target_index_matcher.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/firestore/src/model/target_index_matcher.ts b/packages/firestore/src/model/target_index_matcher.ts index 431503cf01b..e274ed7ba05 100644 --- a/packages/firestore/src/model/target_index_matcher.ts +++ b/packages/firestore/src/model/target_index_matcher.ts @@ -185,7 +185,7 @@ export class TargetIndexMatcher { /** Returns a full matched field index for this target. */ buildTargetIndex(): FieldIndex { // We want to make sure only one segment created for one field. For example, - // in case like a == 3 and a > 2, index, a ASCENDING, will only be created + // in case like a == 3 and a > 2, Index {a ASCENDING} will only be created // once. let uniqueFields = new SortedSet(FieldPath.comparator); const segments: IndexSegment[] = []; From 51e017d1348ff04451a8f27e9d4d8097b29739ee Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 22 Aug 2023 13:29:20 -0400 Subject: [PATCH 20/28] remove testing hooks (still need to update the tests and other code that _used_ the testing hooks --- packages/firestore/src/util/testing_hooks.ts | 138 +----------------- .../firestore/src/util/testing_hooks_spi.ts | 7 - .../integration/util/testing_hooks_util.ts | 77 ---------- 3 files changed, 4 insertions(+), 218 deletions(-) diff --git a/packages/firestore/src/util/testing_hooks.ts b/packages/firestore/src/util/testing_hooks.ts index 5712137ccc7..36422172a45 100644 --- a/packages/firestore/src/util/testing_hooks.ts +++ b/packages/firestore/src/util/testing_hooks.ts @@ -15,17 +15,8 @@ * limitations under the License. */ -import { ensureFirestoreConfigured, Firestore } from '../api/database'; -import { - PersistentCacheIndexManager, - TestingHooks as PersistentCacheIndexManagerTestingHooks -} from '../api/persistent_cache_index_manager'; import { Unsubscribe } from '../api/reference_impl'; -import { TestingHooks as FirestoreClientTestingHooks } from '../core/firestore_client'; -import { Query } from '../lite-api/reference'; -import { IndexType } from '../local/index_manager'; -import { cast } from './input_validation'; import { setTestingHooksSpi, ExistenceFilterMismatchInfo, @@ -63,83 +54,6 @@ export class TestingHooks { ): Unsubscribe { return TestingHooksSpiImpl.instance.onExistenceFilterMismatch(callback); } - - /** - * Registers a callback to be notified when - * `enablePersistentCacheIndexAutoCreation()` or - * `disablePersistentCacheIndexAutoCreation()` is invoked. - * - * The relative order in which callbacks are notified is unspecified; do not - * rely on any particular ordering. If a given callback is registered multiple - * times then it will be notified multiple times, once per registration. - * - * @param callback the callback to invoke when - * `enablePersistentCacheIndexAutoCreation()` or - * `disablePersistentCacheIndexAutoCreation()` is invoked. - * - * @return a function that, when called, unregisters the given callback; only - * the first invocation of the returned function does anything; all subsequent - * invocations do nothing. - */ - static onPersistentCacheIndexAutoCreationToggle( - callback: PersistentCacheIndexAutoCreationToggleCallback - ): Unsubscribe { - return TestingHooksSpiImpl.instance.onPersistentCacheIndexAutoCreationToggle( - callback - ); - } - - /** - * Determines the type of client-side index that will be used when executing the - * given query against the local cache. - * - * @param query The query whose client-side index type to get; it is typed as - * `unknown` so that it is usable in the minified, bundled code, but it should - * be a `Query` object. - */ - static async getQueryIndexType( - query: unknown - ): Promise<'full' | 'partial' | 'none'> { - const query_ = cast(query as Query, Query); - const firestore = cast(query_.firestore, Firestore); - const client = ensureFirestoreConfigured(firestore); - - const indexType = await FirestoreClientTestingHooks.getQueryIndexType( - client, - query_._query - ); - - switch (indexType) { - case IndexType.NONE: - return 'none'; - case IndexType.PARTIAL: - return 'partial'; - case IndexType.FULL: - return 'full'; - default: - throw new Error(`unrecognized IndexType: ${indexType}`); - } - } - - /** - * Sets the persistent cache index auto-creation settings for the given - * Firestore instance. - * - * @return a Promise that is fulfilled when the settings are successfully - * applied, or rejected if applying the settings fails. - */ - static setPersistentCacheIndexAutoCreationSettings( - indexManager: PersistentCacheIndexManager, - settings: { - indexAutoCreationMinCollectionSize?: number; - relativeIndexReadCostPerDocument?: number; - } - ): Promise { - return PersistentCacheIndexManagerTestingHooks.setIndexAutoCreationSettings( - indexManager, - settings - ); - } } /** @@ -154,23 +68,6 @@ export type ExistenceFilterMismatchCallback = ( info: ExistenceFilterMismatchInfo ) => unknown; -/** - * The signature of callbacks registered with - * `TestingHooks.onPersistentCacheIndexAutoCreationToggle()`. - * - * The `promise` argument will be fulfilled when the asynchronous work started - * by the call to `enablePersistentCacheIndexAutoCreation()` or - * `disablePersistentCacheIndexAutoCreation()` completes successfully, or will - * be rejected if it fails. - * - * The return value, if any, is ignored. - * - * @internal - */ -export type PersistentCacheIndexAutoCreationToggleCallback = ( - promise: Promise -) => unknown; - /** * The implementation of `TestingHooksSpi`. */ @@ -180,9 +77,6 @@ class TestingHooksSpiImpl implements TestingHooksSpi { ExistenceFilterMismatchCallback >(); - private readonly persistentCacheIndexAutoCreationToggleCallbacksById = - new Map(); - private constructor() {} static get instance(): TestingHooksSpiImpl { @@ -202,35 +96,11 @@ class TestingHooksSpiImpl implements TestingHooksSpi { onExistenceFilterMismatch( callback: ExistenceFilterMismatchCallback ): Unsubscribe { - return registerCallback( - callback, - this.existenceFilterMismatchCallbacksById - ); - } - - notifyPersistentCacheIndexAutoCreationToggle(promise: Promise): void { - this.persistentCacheIndexAutoCreationToggleCallbacksById.forEach(callback => - callback(promise) - ); + const id = Symbol(); + const callbacks = this.existenceFilterMismatchCallbacksById; + callbacks.set(id, callback); + return () => callbacks.delete(id); } - - onPersistentCacheIndexAutoCreationToggle( - callback: PersistentCacheIndexAutoCreationToggleCallback - ): Unsubscribe { - return registerCallback( - callback, - this.persistentCacheIndexAutoCreationToggleCallbacksById - ); - } -} - -function registerCallback( - callback: T, - callbacks: Map -): Unsubscribe { - const id = Symbol(); - callbacks.set(id, callback); - return () => callbacks.delete(id); } let testingHooksSpiImplInstance: TestingHooksSpiImpl | null = null; diff --git a/packages/firestore/src/util/testing_hooks_spi.ts b/packages/firestore/src/util/testing_hooks_spi.ts index c9117273ca4..e6c729e3661 100644 --- a/packages/firestore/src/util/testing_hooks_spi.ts +++ b/packages/firestore/src/util/testing_hooks_spi.ts @@ -50,13 +50,6 @@ export interface TestingHooksSpi { * `TestingHooks.onExistenceFilterMismatch()` with the given info. */ notifyOnExistenceFilterMismatch(info: ExistenceFilterMismatchInfo): void; - - /** - * Invokes all callbacks registered with - * `TestingHooks.onPersistentCacheIndexAutoCreationToggle()` with the given - * promise. - */ - notifyPersistentCacheIndexAutoCreationToggle(promise: Promise): void; } /** diff --git a/packages/firestore/test/integration/util/testing_hooks_util.ts b/packages/firestore/test/integration/util/testing_hooks_util.ts index 264a9556ea3..72604f91a8d 100644 --- a/packages/firestore/test/integration/util/testing_hooks_util.ts +++ b/packages/firestore/test/integration/util/testing_hooks_util.ts @@ -15,13 +15,8 @@ * limitations under the License. */ -import { expect } from 'chai'; - import { - DocumentData, DocumentReference, - Query, - _PersistentCacheIndexManager as PersistentCacheIndexManager, _TestingHooks as TestingHooks, _TestingHooksExistenceFilterMismatchInfo as ExistenceFilterMismatchInfoInternal } from './firebase_export'; @@ -98,75 +93,3 @@ function createExistenceFilterMismatchInfoFrom( return info; } - -/** - * Verifies than an invocation of `enablePersistentCacheIndexAutoCreation()` or - * `disablePersistentCacheIndexAutoCreation()` made during the execution of the - * given callback succeeds. - * - * @param callback The callback to invoke; this callback must invoke exactly one - * of `enablePersistentCacheIndexAutoCreation()` or - * `disablePersistentCacheIndexAutoCreation()` exactly once; this callback is - * called synchronously by this function, and is called exactly once. - * - * @return a promise that is fulfilled when the asynchronous work started by - * `enablePersistentCacheIndexAutoCreation()` or - * `disablePersistentCacheIndexAutoCreation()` completes successfully, or is - * rejected - */ -export function verifyPersistentCacheIndexAutoCreationToggleSucceedsDuring( - callback: () => void -): Promise { - const promises: Array> = []; - - const unregister = TestingHooks.onPersistentCacheIndexAutoCreationToggle( - promise => promises.push(promise) - ); - - try { - callback(); - } finally { - unregister(); - } - - expect( - promises, - 'exactly one invocation of enablePersistentCacheIndexAutoCreation() or ' + - 'disablePersistentCacheIndexAutoCreation() should be made by the callback' - ).to.have.length(1); - - return promises[0]; -} - -/** - * Determines the type of client-side index that will be used when executing the - * given query against the local cache. - */ -export function getQueryIndexType< - AppModelType, - DbModelType extends DocumentData ->( - query: Query -): Promise<'full' | 'partial' | 'none'> { - return TestingHooks.getQueryIndexType(query); -} - -/** - * Sets the persistent cache index auto-creation settings for the given - * Firestore instance. - * - * @return a Promise that is fulfilled when the settings are successfully - * applied, or rejected if applying the settings fails. - */ -export function setPersistentCacheIndexAutoCreationSettings( - indexManager: PersistentCacheIndexManager, - settings: { - indexAutoCreationMinCollectionSize?: number; - relativeIndexReadCostPerDocument?: number; - } -): Promise { - return TestingHooks.setPersistentCacheIndexAutoCreationSettings( - indexManager, - settings - ); -} From 2a14ce62899ab43d621e520e7f34b84975c99ed5 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 22 Aug 2023 15:01:07 -0400 Subject: [PATCH 21/28] remove testing hooks from tests, except for persistent_cache_index_manager.test.ts, which will be done in the next commit --- .../src/api/persistent_cache_index_manager.ts | 26 ------------- .../firestore/src/core/firestore_client.ts | 39 +------------------ .../firestore/src/local/local_store_impl.ts | 15 +------ 3 files changed, 2 insertions(+), 78 deletions(-) diff --git a/packages/firestore/src/api/persistent_cache_index_manager.ts b/packages/firestore/src/api/persistent_cache_index_manager.ts index 4235a75fa2e..c8e7a15c597 100644 --- a/packages/firestore/src/api/persistent_cache_index_manager.ts +++ b/packages/firestore/src/api/persistent_cache_index_manager.ts @@ -18,11 +18,9 @@ import { firestoreClientSetPersistentCacheIndexAutoCreationEnabled, FirestoreClient, - TestingHooks as FirestoreClientTestingHooks } from '../core/firestore_client'; import { cast } from '../util/input_validation'; import { logDebug, logWarn } from '../util/log'; -import { testingHooksSpi } from '../util/testing_hooks_spi'; import { ensureFirestoreConfigured, Firestore } from './database'; @@ -126,8 +124,6 @@ function setPersistentCacheIndexAutoCreationEnabled( error ) ); - - testingHooksSpi?.notifyPersistentCacheIndexAutoCreationToggle(promise); } /** @@ -142,25 +138,3 @@ const persistentCacheIndexManagerByFirestore = new WeakMap< Firestore, PersistentCacheIndexManager >(); - -/** - * Test-only hooks into the SDK for use exclusively by integration tests. - */ -export class TestingHooks { - private constructor() { - throw new Error('creating instances is not supported'); - } - - static setIndexAutoCreationSettings( - indexManager: PersistentCacheIndexManager, - settings: { - indexAutoCreationMinCollectionSize?: number; - relativeIndexReadCostPerDocument?: number; - } - ): Promise { - return FirestoreClientTestingHooks.setPersistentCacheIndexAutoCreationSettings( - indexManager._client, - settings - ); - } -} diff --git a/packages/firestore/src/core/firestore_client.ts b/packages/firestore/src/core/firestore_client.ts index c1e3728b2f3..72810c63293 100644 --- a/packages/firestore/src/core/firestore_client.ts +++ b/packages/firestore/src/core/firestore_client.ts @@ -31,8 +31,7 @@ import { localStoreGetNamedQuery, localStoreHandleUserChange, localStoreReadDocument, - localStoreSetIndexAutoCreationEnabled, - TestingHooks as LocalStoreTestingHooks + localStoreSetIndexAutoCreationEnabled } from '../local/local_store_impl'; import { Persistence } from '../local/persistence'; import { Document } from '../model/document'; @@ -843,39 +842,3 @@ export function firestoreClientSetPersistentCacheIndexAutoCreationEnabled( ); }); } - -/** - * Test-only hooks into the SDK for use exclusively by integration tests. - */ -export class TestingHooks { - private constructor() { - throw new Error('creating instances is not supported'); - } - - static getQueryIndexType( - client: FirestoreClient, - query: Query - ): Promise { - return client.asyncQueue.enqueue(async () => { - const localStore = await getLocalStore(client); - return LocalStoreTestingHooks.getQueryIndexType(localStore, query); - }); - } - - static setPersistentCacheIndexAutoCreationSettings( - client: FirestoreClient, - settings: { - indexAutoCreationMinCollectionSize?: number; - relativeIndexReadCostPerDocument?: number; - } - ): Promise { - const settingsCopy = { ...settings }; - return client.asyncQueue.enqueue(async () => { - const localStore = await getLocalStore(client); - LocalStoreTestingHooks.setIndexAutoCreationSettings( - localStore, - settingsCopy - ); - }); - } -} diff --git a/packages/firestore/src/local/local_store_impl.ts b/packages/firestore/src/local/local_store_impl.ts index ae1e39b3a7b..179e5faa9f8 100644 --- a/packages/firestore/src/local/local_store_impl.ts +++ b/packages/firestore/src/local/local_store_impl.ts @@ -1536,7 +1536,7 @@ export function localStoreSetIndexAutoCreationEnabled( } /** - * Test-only hooks into the SDK for use exclusively by integration tests. + * Test-only hooks into the SDK for use exclusively by tests. */ export class TestingHooks { private constructor() { @@ -1560,17 +1560,4 @@ export class TestingHooks { settings.relativeIndexReadCostPerDocument; } } - - static getQueryIndexType( - localStore: LocalStore, - query: Query - ): Promise { - const localStoreImpl = debugCast(localStore, LocalStoreImpl); - const target = queryToTarget(query); - return localStoreImpl.persistence.runTransaction( - 'local_store_impl TestingHooks getQueryIndexType', - 'readonly', - txn => localStoreImpl.indexManager.getIndexType(txn, target) - ); - } } From 35c670f3fd87815a70073cb8436fbf333bbc3b4a Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 22 Aug 2023 17:38:07 -0400 Subject: [PATCH 22/28] Remove leading underscores from exports from persistent_cache_index_manager.test.ts --- packages/firestore/src/api.ts | 13 +++++++------ .../api/persistent_cache_index_manager.test.ts | 8 ++++---- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/packages/firestore/src/api.ts b/packages/firestore/src/api.ts index 747b4d5e2c9..9f9ac38749a 100644 --- a/packages/firestore/src/api.ts +++ b/packages/firestore/src/api.ts @@ -202,6 +202,13 @@ export { setIndexConfiguration } from './api/index_configuration'; +export { + PersistentCacheIndexManager, + getPersistentCacheIndexManager, + enablePersistentCacheIndexAutoCreation, + disablePersistentCacheIndexAutoCreation +} from './api/persistent_cache_index_manager'; + /** * Internal exports */ @@ -223,12 +230,6 @@ export type { } from './api/credentials'; export { EmptyAuthCredentialsProvider as _EmptyAuthCredentialsProvider } from './api/credentials'; export { EmptyAppCheckTokenProvider as _EmptyAppCheckTokenProvider } from './api/credentials'; -export { - PersistentCacheIndexManager as _PersistentCacheIndexManager, - getPersistentCacheIndexManager as _getPersistentCacheIndexManager, - enablePersistentCacheIndexAutoCreation as _enablePersistentCacheIndexAutoCreation, - disablePersistentCacheIndexAutoCreation as _disablePersistentCacheIndexAutoCreation -} from './api/persistent_cache_index_manager'; export { ExistenceFilterMismatchCallback as _TestingHooksExistenceFilterMismatchCallback, TestingHooks as _TestingHooks diff --git a/packages/firestore/test/integration/api/persistent_cache_index_manager.test.ts b/packages/firestore/test/integration/api/persistent_cache_index_manager.test.ts index 029e25c1b26..37d5ab4257f 100644 --- a/packages/firestore/test/integration/api/persistent_cache_index_manager.test.ts +++ b/packages/firestore/test/integration/api/persistent_cache_index_manager.test.ts @@ -18,15 +18,15 @@ import { expect } from 'chai'; import { + disablePersistentCacheIndexAutoCreation, + enablePersistentCacheIndexAutoCreation, getDocs, getDocsFromCache, + getPersistentCacheIndexManager, + PersistentCacheIndexManager, query, terminate, where, - _disablePersistentCacheIndexAutoCreation as disablePersistentCacheIndexAutoCreation, - _enablePersistentCacheIndexAutoCreation as enablePersistentCacheIndexAutoCreation, - _getPersistentCacheIndexManager as getPersistentCacheIndexManager, - _PersistentCacheIndexManager as PersistentCacheIndexManager, _TestingHooks as TestingHooks } from '../util/firebase_export'; import { From 5e0144a5b1fdbfcefa96dc8f106c0098bf60261f Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 23 Aug 2023 14:13:28 -0400 Subject: [PATCH 23/28] improve getDocumentsMatchingQuery() to not consider creating an index if a index was already used or if an index-free query was used --- packages/firestore/src/local/query_engine.ts | 67 ++-- .../persistent_cache_index_manager.test.ts | 336 ++---------------- 2 files changed, 80 insertions(+), 323 deletions(-) diff --git a/packages/firestore/src/local/query_engine.ts b/packages/firestore/src/local/query_engine.ts index da1567b8f40..e999aaed141 100644 --- a/packages/firestore/src/local/query_engine.ts +++ b/packages/firestore/src/local/query_engine.ts @@ -133,33 +133,48 @@ export class QueryEngine { ): PersistencePromise { debugAssert(this.initialized, 'initialize() not called'); - const queryContext = new QueryContext(); + // Stores the result from executing the query; using this object is more + // convenient than passing the result between steps of the persistence + // transaction and improves readability comparatively. + const queryResult: { result: DocumentMap | null } = { result: null }; + return this.performQueryUsingIndex(transaction, query) - .next(result => - result - ? result - : this.performQueryUsingRemoteKeys( - transaction, - query, - remoteKeys, - lastLimboFreeSnapshotVersion - ) - ) - .next(result => - result - ? result - : this.executeFullCollectionScan(transaction, query, queryContext) - ) - .next(result => - result && this.indexAutoCreationEnabled - ? this.createCacheIndexes( - transaction, - query, - queryContext, - result.size - ).next(_ => result) - : result - ); + .next(result => { + queryResult.result = result; + }) + .next(() => { + if (queryResult.result) { + return; + } + return this.performQueryUsingRemoteKeys( + transaction, + query, + remoteKeys, + lastLimboFreeSnapshotVersion + ).next(result => { + queryResult.result = result; + }); + }) + .next(() => { + if (queryResult.result) { + return; + } + const context = new QueryContext(); + return this.executeFullCollectionScan(transaction, query, context).next( + result => { + queryResult.result = result; + if (this.indexAutoCreationEnabled) { + return this.createCacheIndexes( + transaction, + query, + context, + result.size + ); + } + } + ); + }) + .next(() => queryResult.result!); } createCacheIndexes( diff --git a/packages/firestore/test/integration/api/persistent_cache_index_manager.test.ts b/packages/firestore/test/integration/api/persistent_cache_index_manager.test.ts index 37d5ab4257f..5d09c948a64 100644 --- a/packages/firestore/test/integration/api/persistent_cache_index_manager.test.ts +++ b/packages/firestore/test/integration/api/persistent_cache_index_manager.test.ts @@ -19,15 +19,16 @@ import { expect } from 'chai'; import { disablePersistentCacheIndexAutoCreation, + doc, enablePersistentCacheIndexAutoCreation, + getDoc, getDocs, getDocsFromCache, getPersistentCacheIndexManager, PersistentCacheIndexManager, query, terminate, - where, - _TestingHooks as TestingHooks + where } from '../util/firebase_export'; import { apiDescribe, @@ -35,11 +36,6 @@ import { withTestCollection, withTestDb } from '../util/helpers'; -import { - getQueryIndexType, - setPersistentCacheIndexAutoCreationSettings, - verifyPersistentCacheIndexAutoCreationToggleSucceedsDuring -} from '../util/testing_hooks_util'; apiDescribe('PersistentCacheIndexManager', persistence => { describe('getPersistentCacheIndexManager()', () => { @@ -75,40 +71,40 @@ apiDescribe('PersistentCacheIndexManager', persistence => { return; } - describe('enablePersistentCacheIndexAutoCreation()', () => { - it('should return successfully', () => + describe('enable/disable persistent index auto creation', () => { + it('enable on new instance should succeed', () => withTestDb(persistence, async db => { const indexManager = getPersistentCacheIndexManager(db)!; enablePersistentCacheIndexAutoCreation(indexManager); })); - it('should successfully enable indexing when not yet enabled', () => - withTestDb(persistence, db => { + it('disable on new instance should succeed', () => + withTestDb(persistence, async db => { const indexManager = getPersistentCacheIndexManager(db)!; - return verifyPersistentCacheIndexAutoCreationToggleSucceedsDuring(() => - enablePersistentCacheIndexAutoCreation(indexManager) - ); + disablePersistentCacheIndexAutoCreation(indexManager); })); - it('should successfully enable indexing when already enabled', () => - withTestDb(persistence, db => { + it('enable when already enabled should succeed', async () => + withTestDb(persistence, async db => { + const documentRef = doc(db, 'a/b'); const indexManager = getPersistentCacheIndexManager(db)!; - return verifyPersistentCacheIndexAutoCreationToggleSucceedsDuring(() => - enablePersistentCacheIndexAutoCreation(indexManager) - ); + enablePersistentCacheIndexAutoCreation(indexManager); + await getDoc(documentRef); // flush the async queue + enablePersistentCacheIndexAutoCreation(indexManager); + enablePersistentCacheIndexAutoCreation(indexManager); })); - it('should successfully enable indexing after being disabled', () => - withTestDb(persistence, db => { + it('disable when already disabled should succeed', async () => + withTestDb(persistence, async db => { + const documentRef = doc(db, 'a/b'); const indexManager = getPersistentCacheIndexManager(db)!; - enablePersistentCacheIndexAutoCreation(indexManager); disablePersistentCacheIndexAutoCreation(indexManager); - return verifyPersistentCacheIndexAutoCreationToggleSucceedsDuring(() => - enablePersistentCacheIndexAutoCreation(indexManager) - ); + await getDoc(documentRef); // flush the async queue + disablePersistentCacheIndexAutoCreation(indexManager); + disablePersistentCacheIndexAutoCreation(indexManager); })); - it('should fail if invoked after terminate()', () => + it('enabling after terminate() should throw', () => withTestDb(persistence, async db => { const indexManager = getPersistentCacheIndexManager(db)!; terminate(db).catch(e => expect.fail(`terminate() failed: ${e}`)); @@ -116,43 +112,8 @@ apiDescribe('PersistentCacheIndexManager', persistence => { enablePersistentCacheIndexAutoCreation(indexManager) ).to.throw('The client has already been terminated.'); })); - }); - - describe('disablePersistentCacheIndexAutoCreation(()', () => { - it('should return successfully', () => - withTestDb(persistence, async db => { - const indexManager = getPersistentCacheIndexManager(db)!; - disablePersistentCacheIndexAutoCreation(indexManager); - })); - - it('should successfully disable indexing when not yet enabled', () => - withTestDb(persistence, db => { - const indexManager = getPersistentCacheIndexManager(db)!; - return verifyPersistentCacheIndexAutoCreationToggleSucceedsDuring(() => - disablePersistentCacheIndexAutoCreation(indexManager) - ); - })); - - it('should successfully disable indexing when enabled', () => - withTestDb(persistence, db => { - const indexManager = getPersistentCacheIndexManager(db)!; - enablePersistentCacheIndexAutoCreation(indexManager); - return verifyPersistentCacheIndexAutoCreationToggleSucceedsDuring(() => - disablePersistentCacheIndexAutoCreation(indexManager) - ); - })); - it('should successfully enable indexing after being disabled', () => - withTestDb(persistence, db => { - const indexManager = getPersistentCacheIndexManager(db)!; - enablePersistentCacheIndexAutoCreation(indexManager); - disablePersistentCacheIndexAutoCreation(indexManager); - return verifyPersistentCacheIndexAutoCreationToggleSucceedsDuring(() => - disablePersistentCacheIndexAutoCreation(indexManager) - ); - })); - - it('should fail if invoked after terminate()', () => + it('disabling after terminate() should throw', () => withTestDb(persistence, async db => { const indexManager = getPersistentCacheIndexManager(db)!; terminate(db).catch(e => expect.fail(`terminate() failed: ${e}`)); @@ -160,251 +121,32 @@ apiDescribe('PersistentCacheIndexManager', persistence => { disablePersistentCacheIndexAutoCreation(indexManager) ).to.throw('The client has already been terminated.'); })); - }); - - describe('Query execution', () => { - it('Auto-indexing is disabled by default', () => - testIndexesGetAutoCreated({ - documentCounts: { matching: 1, notMatching: 5 }, - expectedIndexAutoCreated: false, - indexAutoCreationEnabled: false, - indexAutoCreationMinCollectionSize: 0, - relativeIndexReadCostPerDocument: 2 - })); - - it( - 'Default indexAutoCreationMinCollectionSize=100: ' + - 'index should *not* be auto-created if lookup scanned 99 documents', - () => - testIndexesGetAutoCreated({ - documentCounts: { matching: 1, notMatching: 98 }, - expectedIndexAutoCreated: false - }) - ); - - it( - 'Default indexAutoCreationMinCollectionSize=100' + - ' index *should* be auto-created if lookup scanned 100 documents', - () => - testIndexesGetAutoCreated({ - documentCounts: { matching: 1, notMatching: 99 }, - expectedIndexAutoCreated: true - }) - ); - - it( - 'Default relativeIndexReadCostPerDocument=2: ' + - 'index should *not* be auto-created if the relative index read cost is matched exactly', - () => - testIndexesGetAutoCreated({ - documentCounts: { matching: 50, notMatching: 50 }, - expectedIndexAutoCreated: false - }) - ); - - it( - 'Default relativeIndexReadCostPerDocument=2: ' + - 'index *should* be auto-created if the relative index read cost is exceeded slightly', - () => - testIndexesGetAutoCreated({ - documentCounts: { matching: 49, notMatching: 51 }, - expectedIndexAutoCreated: true - }) - ); - - it('Indexes are only auto-created when enabled', async () => { - const testDocs = partitionedTestDocs({ - FooMatches: { - documentData: { foo: 'match' }, - documentCount: 1 - }, - BarMatches: { - documentData: { bar: 'match' }, - documentCount: 2 - }, - BazMatches: { - documentData: { baz: 'match' }, - documentCount: 3 - }, - NeitherFooNorBarNorMazMatch: { - documentData: { foo: 'nomatch', bar: 'nomatch', baz: 'nomatch' }, - documentCount: 10 - } - }); - - return withTestCollection(persistence, testDocs, async (coll, db) => { - const indexManager = getPersistentCacheIndexManager(db)!; - expect(indexManager, 'indexManager').is.not.null; - await setPersistentCacheIndexAutoCreationSettings(indexManager, { - indexAutoCreationMinCollectionSize: 0, - relativeIndexReadCostPerDocument: 2 - }); - - // Populate the local cache with the entire collection. - await getDocs(coll); - - // Enable automatic index creation and run a query, ensuring that an - // appropriate index is created. - enablePersistentCacheIndexAutoCreation(indexManager); - const query1 = query(coll, where('foo', '==', 'match')); - const snapshot1 = await getDocsFromCache(query1); - expect(snapshot1.size, 'snapshot1.size').to.equal(1); - expect( - await getQueryIndexType(query1), - 'getQueryIndexType(query1)' - ).to.equal('full'); - - // Disable automatic index creation and run a query, ensuring that an - // appropriate index is _not_ created, and the other is maintained. - disablePersistentCacheIndexAutoCreation(indexManager); - const query2 = query(coll, where('bar', '==', 'match')); - const snapshot2 = await getDocsFromCache(query2); - expect(snapshot2.size, 'snapshot2.size').to.equal(2); - expect( - await getQueryIndexType(query2), - 'getQueryIndexType(query2)' - ).to.equal('none'); - expect( - await getQueryIndexType(query1), - 'getQueryIndexType(query1) check #2' - ).to.equal('full'); - - // Re-enable automatic index creation and run a query, ensuring that an - // appropriate index is created, and others are maintained. - enablePersistentCacheIndexAutoCreation(indexManager); - const query3 = query(coll, where('baz', '==', 'match')); - const snapshot3 = await getDocsFromCache(query3); - expect(snapshot3.size, 'snapshot3.size').to.equal(3); - expect( - await getQueryIndexType(query3), - 'getQueryIndexType(query3)' - ).to.equal('full'); - expect( - await getQueryIndexType(query2), - 'getQueryIndexType(query2) check #2' - ).to.equal('none'); - expect( - await getQueryIndexType(query1), - 'getQueryIndexType(query1) check #3' - ).to.equal('full'); - }); - }); - it('A full index is not auto-created if there is a partial index match', async () => { + it('query returns correct results when index is auto-created', () => { const testDocs = partitionedTestDocs({ - FooMatches: { - documentData: { foo: 'match' }, - documentCount: 5 - }, - FooAndBarBothMatch: { - documentData: { foo: 'match', bar: 'match' }, - documentCount: 1 - }, - NeitherFooNorBarMatch: { - documentData: { foo: 'nomatch', bar: 'nomatch' }, - documentCount: 15 - } + matching: { documentData: { match: true }, documentCount: 1 }, + nonmatching: { documentData: { match: false }, documentCount: 100 } }); - return withTestCollection(persistence, testDocs, async (coll, db) => { const indexManager = getPersistentCacheIndexManager(db)!; - expect(indexManager, 'indexManager').is.not.null; enablePersistentCacheIndexAutoCreation(indexManager); - await setPersistentCacheIndexAutoCreationSettings(indexManager, { - indexAutoCreationMinCollectionSize: 0, - relativeIndexReadCostPerDocument: 2 - }); - // Populate the local cache with the entire collection. + // Populate the local cache with the entire collection's contents. await getDocs(coll); - // Run a query to have an index on the 'foo' field created. - { - const fooQuery = query(coll, where('foo', '==', 'match')); - const fooSnapshot = await getDocsFromCache(fooQuery); - expect(fooSnapshot.size, 'fooSnapshot.size').to.equal(6); - expect( - await getQueryIndexType(fooQuery), - 'getQueryIndexType(fooQuery)' - ).to.equal('full'); - } - - // Run a query that filters on both 'foo' and 'bar' fields and ensure - // that the partial index (created by the previous query's execution) - // is NOT upgraded to a full index. Note that in the future we _may_ - // change this behavior since a full index would likely be beneficial to - // the query's execution performance. - { - const fooBarQuery = query( - coll, - where('foo', '==', 'match'), - where('bar', '==', 'match') - ); - expect( - await getQueryIndexType(fooBarQuery), - 'getQueryIndexType(fooBarQuery) before' - ).to.equal('partial'); - const fooBarSnapshot = await getDocsFromCache(fooBarQuery); - expect(fooBarSnapshot.size, 'fooBarSnapshot.size').to.equal(1); - expect( - await getQueryIndexType(fooBarQuery), - 'getQueryIndexType(fooBarQuery) after' - ).to.equal('partial'); - } + // Run a query that matches only one of the documents in the collection; + // this should cause an index to be auto-created. + const query_ = query(coll, where('match', '==', true)); + const snapshot1 = await getDocsFromCache(query_); + expect(snapshot1.size).to.equal(1); + + // Run the query that matches only one of the documents again, which + // should _still_ return the one and only document that matches. Since + // the public API surface does not reveal whether an index was used, + // there isn't anything else that can be verified. + const snapshot2 = await getDocsFromCache(query_); + expect(snapshot2.size).to.equal(1); }); }); - - async function testIndexesGetAutoCreated(config: { - documentCounts: { matching: number; notMatching: number }; - expectedIndexAutoCreated: boolean; - indexAutoCreationEnabled?: boolean; - indexAutoCreationMinCollectionSize?: number; - relativeIndexReadCostPerDocument?: number; - }): Promise { - const testDocs = partitionedTestDocs({ - matching: { - documentData: { foo: 'match' }, - documentCount: config.documentCounts.matching - }, - notMatching: { - documentData: { foo: 'nomatch' }, - documentCount: config.documentCounts.notMatching - } - }); - - return withTestCollection(persistence, testDocs, async (coll, db) => { - // Populate the local cache with the entire collection. - await getDocs(coll); - - // Configure automatic index creation, as requested. - const indexManager = getPersistentCacheIndexManager(db)!; - expect(indexManager, 'indexManager').is.not.null; - if (config.indexAutoCreationEnabled ?? true) { - enablePersistentCacheIndexAutoCreation(indexManager); - } - if ( - config.indexAutoCreationMinCollectionSize !== undefined || - config.relativeIndexReadCostPerDocument !== undefined - ) { - await TestingHooks.setPersistentCacheIndexAutoCreationSettings( - indexManager, - config - ); - } - - // Run a query against the local cache that matches a _subset_ of the - // entire collection. - const query_ = query(coll, where('foo', '==', 'match')); - const snapshot = await getDocsFromCache(query_); - expect(snapshot.size, 'snapshot.size').to.equal( - config.documentCounts.matching - ); - - // Verify that an index was or was not created, as expected. - expect(await getQueryIndexType(query_), 'getQueryIndexType()').to.equal( - config.expectedIndexAutoCreated ? 'full' : 'none' - ); - }); - } }); }); From dac99916110f0ca027c1ebda13c9c0000dd75e80 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 23 Aug 2023 14:14:14 -0400 Subject: [PATCH 24/28] persistent_cache_index_manager.ts: prettier fix --- packages/firestore/src/api/persistent_cache_index_manager.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/firestore/src/api/persistent_cache_index_manager.ts b/packages/firestore/src/api/persistent_cache_index_manager.ts index c8e7a15c597..96751fee074 100644 --- a/packages/firestore/src/api/persistent_cache_index_manager.ts +++ b/packages/firestore/src/api/persistent_cache_index_manager.ts @@ -17,7 +17,7 @@ import { firestoreClientSetPersistentCacheIndexAutoCreationEnabled, - FirestoreClient, + FirestoreClient } from '../core/firestore_client'; import { cast } from '../util/input_validation'; import { logDebug, logWarn } from '../util/log'; From 4835585769f231d870b6d9a35ac5ba913f3d90fe Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 23 Aug 2023 14:15:37 -0400 Subject: [PATCH 25/28] yarn lint:fix --- packages/firestore/src/core/firestore_client.ts | 1 - packages/firestore/src/local/local_store_impl.ts | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/firestore/src/core/firestore_client.ts b/packages/firestore/src/core/firestore_client.ts index 72810c63293..6b7950825f6 100644 --- a/packages/firestore/src/core/firestore_client.ts +++ b/packages/firestore/src/core/firestore_client.ts @@ -23,7 +23,6 @@ import { CredentialsProvider } from '../api/credentials'; import { User } from '../auth/user'; -import { IndexType } from '../local/index_manager'; import { LocalStore } from '../local/local_store'; import { localStoreConfigureFieldIndexes, diff --git a/packages/firestore/src/local/local_store_impl.ts b/packages/firestore/src/local/local_store_impl.ts index 179e5faa9f8..11459a1716c 100644 --- a/packages/firestore/src/local/local_store_impl.ts +++ b/packages/firestore/src/local/local_store_impl.ts @@ -70,7 +70,7 @@ import { BATCHID_UNKNOWN } from '../util/types'; import { BundleCache } from './bundle_cache'; import { DocumentOverlayCache } from './document_overlay_cache'; -import { IndexManager, IndexType } from './index_manager'; +import { IndexManager } from './index_manager'; import { IndexedDbMutationQueue } from './indexeddb_mutation_queue'; import { IndexedDbPersistence } from './indexeddb_persistence'; import { IndexedDbTargetCache } from './indexeddb_target_cache'; From d84308fd346b57560ab70c1de3af1eeedf189ff4 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 23 Aug 2023 15:36:36 -0400 Subject: [PATCH 26/28] yarn changeset --- .changeset/cool-games-care.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changeset/cool-games-care.md diff --git a/.changeset/cool-games-care.md b/.changeset/cool-games-care.md new file mode 100644 index 00000000000..ca5eb47ae4c --- /dev/null +++ b/.changeset/cool-games-care.md @@ -0,0 +1,6 @@ +--- +'@firebase/firestore': patch +'firebase': patch +--- + +Implemented internal logic to auto-create client-side indexes From 62d28bab0ea74ab48e12be8c46e0e20f74bb9b97 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 24 Aug 2023 11:49:32 -0400 Subject: [PATCH 27/28] query_engine.ts: remove superfluous log message, as requested in code review --- packages/firestore/src/local/query_engine.ts | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/packages/firestore/src/local/query_engine.ts b/packages/firestore/src/local/query_engine.ts index e999aaed141..f70b8c2e2ac 100644 --- a/packages/firestore/src/local/query_engine.ts +++ b/packages/firestore/src/local/query_engine.ts @@ -227,17 +227,9 @@ export class QueryEngine { transaction, queryToTarget(query) ); - } else { - if (getLogLevel() <= LogLevel.DEBUG) { - logDebug( - 'QueryEngine', - 'The SDK decides not to create cache indexes for query:', - stringifyQuery(query), - 'as using cache indexes may not help improve performance.' - ); - } - return PersistencePromise.resolve(); } + + return PersistencePromise.resolve(); } /** From bedfda010ae8572380ccf91f13e5e41cee121b54 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 24 Aug 2023 11:53:56 -0400 Subject: [PATCH 28/28] yarn prettier --- packages/firestore/src/local/query_engine.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/firestore/src/local/query_engine.ts b/packages/firestore/src/local/query_engine.ts index f70b8c2e2ac..a0adb7ed95a 100644 --- a/packages/firestore/src/local/query_engine.ts +++ b/packages/firestore/src/local/query_engine.ts @@ -228,7 +228,7 @@ export class QueryEngine { queryToTarget(query) ); } - + return PersistencePromise.resolve(); }