diff --git a/.changeset/stupid-swans-fix.md b/.changeset/stupid-swans-fix.md new file mode 100644 index 00000000000..312d8af3119 --- /dev/null +++ b/.changeset/stupid-swans-fix.md @@ -0,0 +1,5 @@ +--- +"@firebase/firestore": patch +--- + +Fix an issue that stops some performance optimization being applied. diff --git a/packages/firestore/src/local/local_documents_view.ts b/packages/firestore/src/local/local_documents_view.ts index e5d50c18e25..2572646655f 100644 --- a/packages/firestore/src/local/local_documents_view.ts +++ b/packages/firestore/src/local/local_documents_view.ts @@ -242,6 +242,10 @@ export class LocalDocumentsView { overlay.mutation.getFieldMask(), Timestamp.now() ); + } else { + // no overlay exists + // Using EMPTY to indicate there is no overlay for the document. + mutatedFields.set(doc.key, FieldMask.empty()); } }); diff --git a/packages/firestore/src/local/overlayed_document.ts b/packages/firestore/src/local/overlayed_document.ts index e50e6a9132d..bc7ab3b5f3a 100644 --- a/packages/firestore/src/local/overlayed_document.ts +++ b/packages/firestore/src/local/overlayed_document.ts @@ -27,8 +27,10 @@ export class OverlayedDocument { readonly overlayedDocument: Document, /** - * The fields that are locally mutated by patch mutations. If the overlayed - * document is from set or delete mutations, this returns null. + * The fields that are locally mutated by patch mutations. + * + * If the overlayed document is from set or delete mutations, this is `null`. + * If there is no overlay (mutation) for the document, this is an empty `FieldMask`. */ readonly mutatedFields: FieldMask | null ) {} diff --git a/packages/firestore/src/model/mutation_batch.ts b/packages/firestore/src/model/mutation_batch.ts index 2e7f367639e..56d5f4d2cd3 100644 --- a/packages/firestore/src/model/mutation_batch.ts +++ b/packages/firestore/src/model/mutation_batch.ts @@ -166,6 +166,7 @@ export class MutationBatch { if (overlay !== null) { overlays.set(m.key, overlay); } + if (!mutableDocument.isValidDocument()) { mutableDocument.convertToNoDocument(SnapshotVersion.min()); } diff --git a/packages/firestore/test/unit/local/counting_query_engine.ts b/packages/firestore/test/unit/local/counting_query_engine.ts index a6aaf7493aa..a8a89a590b6 100644 --- a/packages/firestore/test/unit/local/counting_query_engine.ts +++ b/packages/firestore/test/unit/local/counting_query_engine.ts @@ -20,19 +20,12 @@ import { SnapshotVersion } from '../../../src/core/snapshot_version'; import { DocumentOverlayCache } from '../../../src/local/document_overlay_cache'; import { IndexManager } from '../../../src/local/index_manager'; import { LocalDocumentsView } from '../../../src/local/local_documents_view'; -import { MutationQueue } from '../../../src/local/mutation_queue'; import { PersistencePromise } from '../../../src/local/persistence_promise'; import { PersistenceTransaction } from '../../../src/local/persistence_transaction'; import { QueryEngine } from '../../../src/local/query_engine'; import { RemoteDocumentCache } from '../../../src/local/remote_document_cache'; -import { - DocumentKeySet, - DocumentMap, - OverlayMap -} from '../../../src/model/collections'; -import { DocumentKey } from '../../../src/model/document_key'; -import { Overlay } from '../../../src/model/overlay'; -import { ResourcePath } from '../../../src/model/path'; +import { DocumentKeySet, DocumentMap } from '../../../src/model/collections'; +import { MutationType } from '../../../src/model/mutation'; /** * A test-only query engine that forwards all API calls and exposes the number @@ -40,23 +33,23 @@ import { ResourcePath } from '../../../src/model/path'; */ export class CountingQueryEngine extends QueryEngine { /** - * The number of mutations returned by the MutationQueue's - * `getAllMutationBatchesAffectingQuery()` API (since the last call to + * The number of overlays returned by the DocumentOverlayCache's + * `getOverlaysByCollection(Group)` API (since the last call to * `resetCounts()`) */ - mutationsReadByCollection = 0; + overlaysReadByCollection = 0; /** - * The number of mutations returned by the MutationQueue's - * `getAllMutationBatchesAffectingDocumentKey()` and - * `getAllMutationBatchesAffectingDocumentKeys()` APIs (since the last call - * to `resetCounts()`) + * The number of overlays returned by the DocumentOverlayCache's + * `getOverlay(s)` APIs (since the last call to `resetCounts()`) */ - mutationsReadByKey = 0; + overlaysReadByKey = 0; + + overlayTypes: { [k: string]: MutationType } = {}; /** * The number of documents returned by the RemoteDocumentCache's - * `getAll()` API (since the last call to `resetCounts()`) + * `getAll()` API (since the last call to `resetCounts()`). */ documentsReadByCollection = 0; @@ -66,25 +59,12 @@ export class CountingQueryEngine extends QueryEngine { */ documentsReadByKey = 0; - /** - * The number of documents returned by the OverlayCache's `getOverlays()` - * API (since the last call to `resetCounts()`) - */ - overlaysReadByCollection = 0; - - /** - * The number of documents returned by the OverlayCache's `getOverlay()` - * APIs (since the last call to `resetCounts()`) - */ - overlaysReadByKey = 0; - resetCounts(): void { - this.mutationsReadByCollection = 0; - this.mutationsReadByKey = 0; - this.documentsReadByCollection = 0; - this.documentsReadByKey = 0; this.overlaysReadByCollection = 0; this.overlaysReadByKey = 0; + this.overlayTypes = {}; + this.documentsReadByCollection = 0; + this.documentsReadByKey = 0; } getDocumentsMatchingQuery( @@ -107,8 +87,8 @@ export class CountingQueryEngine extends QueryEngine { ): void { const view = new LocalDocumentsView( this.wrapRemoteDocumentCache(localDocuments.remoteDocumentCache), - this.wrapMutationQueue(localDocuments.mutationQueue), - this.wrapDocumentOverlayCache(localDocuments.documentOverlayCache), + localDocuments.mutationQueue, + this.wrapOverlayCache(localDocuments.documentOverlayCache), localDocuments.indexManager ); return super.initialize(view, indexManager); @@ -168,88 +148,48 @@ export class CountingQueryEngine extends QueryEngine { }; } - private wrapMutationQueue(subject: MutationQueue): MutationQueue { + private wrapOverlayCache( + subject: DocumentOverlayCache + ): DocumentOverlayCache { return { - addMutationBatch: subject.addMutationBatch, - checkEmpty: subject.checkEmpty, - getAllMutationBatches: transaction => { - return subject.getAllMutationBatches(transaction).next(result => { - this.mutationsReadByKey += result.length; + getOverlay: (transaction, key) => { + return subject.getOverlay(transaction, key).next(result => { + this.overlaysReadByKey += 1; + if (!!result) { + this.overlayTypes[key.toString()] = result.mutation.type; + } return result; }); }, - getAllMutationBatchesAffectingDocumentKey: (transaction, documentKey) => { - return subject - .getAllMutationBatchesAffectingDocumentKey(transaction, documentKey) - .next(result => { - this.mutationsReadByKey += result.length; - return result; - }); - }, - getAllMutationBatchesAffectingDocumentKeys: ( - transaction, - documentKeys - ) => { - return subject - .getAllMutationBatchesAffectingDocumentKeys(transaction, documentKeys) - .next(result => { - this.mutationsReadByKey += result.length; - return result; - }); - }, - getAllMutationBatchesAffectingQuery: (transaction, query) => { - return subject - .getAllMutationBatchesAffectingQuery(transaction, query) - .next(result => { - this.mutationsReadByCollection += result.length; - return result; + + getOverlays: (transaction, keys) => { + return subject.getOverlays(transaction, keys).next(result => { + this.overlaysReadByKey += keys.length; + result.forEach((key, overlay) => { + this.overlayTypes[key.toString()] = overlay.mutation.type; }); + return result; + }); }, - getHighestUnacknowledgedBatchId: subject.getHighestUnacknowledgedBatchId, - getNextMutationBatchAfterBatchId: - subject.getNextMutationBatchAfterBatchId, - lookupMutationBatch: subject.lookupMutationBatch, - performConsistencyCheck: subject.performConsistencyCheck, - removeMutationBatch: subject.removeMutationBatch - }; - } - private wrapDocumentOverlayCache( - subject: DocumentOverlayCache - ): DocumentOverlayCache { - return { - getOverlay: ( - transaction: PersistenceTransaction, - key: DocumentKey - ): PersistencePromise => { - this.overlaysReadByKey++; - return subject.getOverlay(transaction, key); - }, - getOverlays: ( - transaction: PersistenceTransaction, - keys: DocumentKey[] - ): PersistencePromise => { - this.overlaysReadByKey += keys.length; - return subject.getOverlays(transaction, keys); - }, - getOverlaysForCollection: ( - transaction: PersistenceTransaction, - collection: ResourcePath, - sinceBatchId: number - ): PersistencePromise => { + getOverlaysForCollection: (transaction, collection, sinceBatchId) => { return subject .getOverlaysForCollection(transaction, collection, sinceBatchId) .next(result => { this.overlaysReadByCollection += result.size(); + result.forEach((key, overlay) => { + this.overlayTypes[key.toString()] = overlay.mutation.type; + }); return result; }); }, + getOverlaysForCollectionGroup: ( transaction: PersistenceTransaction, collectionGroup: string, sinceBatchId: number, count: number - ): PersistencePromise => { + ) => { return subject .getOverlaysForCollectionGroup( transaction, @@ -259,11 +199,24 @@ export class CountingQueryEngine extends QueryEngine { ) .next(result => { this.overlaysReadByCollection += result.size(); + result.forEach((key, overlay) => { + this.overlayTypes[key.toString()] = overlay.mutation.type; + }); return result; }); }, - removeOverlaysForBatchId: subject.removeOverlaysForBatchId, - saveOverlays: subject.saveOverlays + + saveOverlays: (transaction, largestBatchId, overlays) => { + return subject.saveOverlays(transaction, largestBatchId, overlays); + }, + + removeOverlaysForBatchId: (transaction, documentKeys, batchId) => { + return subject.removeOverlaysForBatchId( + transaction, + documentKeys, + batchId + ); + } }; } } diff --git a/packages/firestore/test/unit/local/local_store.test.ts b/packages/firestore/test/unit/local/local_store.test.ts index a7b4e519fd7..c25d337dc13 100644 --- a/packages/firestore/test/unit/local/local_store.test.ts +++ b/packages/firestore/test/unit/local/local_store.test.ts @@ -64,6 +64,7 @@ import { Document } from '../../../src/model/document'; import { Mutation, MutationResult, + MutationType, Precondition } from '../../../src/model/mutation'; import { @@ -333,22 +334,29 @@ class LocalStoreTester { * document key lookups. */ toHaveRead(expectedCount: { - mutationsByCollection?: number; - mutationsByKey?: number; + overlaysByCollection?: number; + overlaysByKey?: number; documentsByCollection?: number; documentsByKey?: number; + overlayTypes?: { [k: string]: MutationType }; }): LocalStoreTester { this.promiseChain = this.promiseChain.then(() => { - if (expectedCount.mutationsByCollection !== undefined) { - expect(this.queryEngine.mutationsReadByCollection).to.be.eq( - expectedCount.mutationsByCollection, - 'Mutations read (by collection)' + if (expectedCount.overlaysByCollection !== undefined) { + expect(this.queryEngine.overlaysReadByCollection).to.be.eq( + expectedCount.overlaysByCollection, + 'Overlays read (by collection)' ); } - if (expectedCount.mutationsByKey !== undefined) { - expect(this.queryEngine.mutationsReadByKey).to.be.eq( - expectedCount.mutationsByKey, - 'Mutations read (by key)' + if (expectedCount.overlaysByKey !== undefined) { + expect(this.queryEngine.overlaysReadByKey).to.be.eq( + expectedCount.overlaysByKey, + 'Overlays read (by key)' + ); + } + if (expectedCount.overlayTypes !== undefined) { + expect(this.queryEngine.overlayTypes).to.deep.equal( + expectedCount.overlayTypes, + 'Overlay types read' ); } if (expectedCount.documentsByCollection !== undefined) { @@ -1213,40 +1221,40 @@ function genericLocalStoreTests( const firstQuery = query('foo'); const secondQuery = query('foo', filter('matches', '==', true)); - return ( - expectLocalStore() - .afterAllocatingQuery(firstQuery) - .toReturnTargetId(2) - .after( - docAddedRemoteEvent( - [ - doc('foo/bar', 10, { matches: true }), - doc('foo/baz', 20, { matches: true }) - ], - [2] - ) - ) - .toReturnChanged( - doc('foo/bar', 10, { matches: true }), - doc('foo/baz', 20, { matches: true }) - ) - .after(setMutation('foo/bonk', { matches: true })) - .toReturnChanged( - doc('foo/bonk', 0, { matches: true }).setHasLocalMutations() - ) - .afterAllocatingQuery(secondQuery) - .toReturnTargetId(4) - .afterExecutingQuery(secondQuery) - .toReturnChanged( - doc('foo/bar', 10, { matches: true }), - doc('foo/baz', 20, { matches: true }), - doc('foo/bonk', 0, { matches: true }).setHasLocalMutations() + return expectLocalStore() + .afterAllocatingQuery(firstQuery) + .toReturnTargetId(2) + .after( + docAddedRemoteEvent( + [ + doc('foo/bar', 10, { matches: true }), + doc('foo/baz', 20, { matches: true }) + ], + [2] ) - // TODO(overlays): implement overlaysReadByKey and overlaysReadByCollection - // No mutations are read because only overlay is needed. - .toHaveRead({ documentsByCollection: 2, mutationsByCollection: 0 }) - .finish() - ); + ) + .toReturnChanged( + doc('foo/bar', 10, { matches: true }), + doc('foo/baz', 20, { matches: true }) + ) + .after(setMutation('foo/bonk', { matches: true })) + .toReturnChanged( + doc('foo/bonk', 0, { matches: true }).setHasLocalMutations() + ) + .afterAllocatingQuery(secondQuery) + .toReturnTargetId(4) + .afterExecutingQuery(secondQuery) + .toReturnChanged( + doc('foo/bar', 10, { matches: true }), + doc('foo/baz', 20, { matches: true }), + doc('foo/bonk', 0, { matches: true }).setHasLocalMutations() + ) + .toHaveRead({ + documentsByCollection: 2, + overlaysByCollection: 1, + overlayTypes: { [key('foo/bonk').toString()]: MutationType.Set } + }) + .finish(); }); // eslint-disable-next-line no-restricted-properties @@ -1997,6 +2005,22 @@ function genericLocalStoreTests( .finish(); }); + it('update on remote doc leads to update overlay', () => { + expect(new Map([['a', 1]])).to.deep.equal(new Map([['a', 0]])); + return expectLocalStore() + .afterAllocatingQuery(query('foo')) + .afterRemoteEvent(docUpdateRemoteEvent(doc('foo/baz', 10, { a: 1 }), [2])) + .afterRemoteEvent(docUpdateRemoteEvent(doc('foo/bar', 20, {}), [2])) + .afterMutations([patchMutation('foo/baz', { b: 2 })]) + .afterExecutingQuery(query('foo')) + .toHaveRead({ + documentsByCollection: 2, + overlaysByCollection: 1, + overlayTypes: { [key('foo/baz').toString()]: MutationType.Patch } + }) + .finish(); + }); + it('handles document creation time', () => { return ( expectLocalStore() diff --git a/packages/firestore/test/unit/local/local_store_indexeddb.test.ts b/packages/firestore/test/unit/local/local_store_indexeddb.test.ts index e84128a52cc..f10fa13c1d7 100644 --- a/packages/firestore/test/unit/local/local_store_indexeddb.test.ts +++ b/packages/firestore/test/unit/local/local_store_indexeddb.test.ts @@ -46,7 +46,7 @@ import { IndexKind, IndexOffset } from '../../../src/model/field_index'; -import { Mutation } from '../../../src/model/mutation'; +import { Mutation, MutationType } from '../../../src/model/mutation'; import { MutationBatch } from '../../../src/model/mutation_batch'; import { RemoteEvent } from '../../../src/remote/remote_event'; import { @@ -156,7 +156,11 @@ class AsyncLocalStoreTester { ); } - assertOverlaysRead(byKey: number, byCollection: number): void { + assertOverlaysRead( + byKey: number, + byCollection: number, + overlayTypes: { [k: string]: MutationType } + ): void { expect(this.queryEngine.overlaysReadByCollection).to.equal( byCollection, 'Overlays read (by collection)' @@ -165,6 +169,10 @@ class AsyncLocalStoreTester { byKey, 'Overlays read (by key)' ); + expect(this.queryEngine.overlayTypes).to.deep.equal( + overlayTypes, + 'Overlay types read' + ); } assertQueryReturned(...keys: string[]): void { @@ -354,7 +362,10 @@ describe('LocalStore w/ IndexedDB Persistence (Non generic)', () => { const queryMatches = query('coll', filter('matches', '==', true)); await test.executeQuery(queryMatches); - test.assertOverlaysRead(1, 1); + test.assertOverlaysRead(1, 1, { + [key('coll/a').toString()]: MutationType.Set, + [key('coll/b').toString()]: MutationType.Set + }); test.assertQueryReturned('coll/a', 'coll/b'); }); @@ -390,7 +401,9 @@ describe('LocalStore w/ IndexedDB Persistence (Non generic)', () => { // The query engine first reads the documents by key and then re-runs the query without limit. await test.executeQuery(queryCount); test.assertRemoteDocumentsRead(5, 0); - test.assertOverlaysRead(5, 1); + test.assertOverlaysRead(5, 1, { + [key('coll/b').toString()]: MutationType.Delete + }); test.assertQueryReturned('coll/a', 'coll/c'); }); @@ -423,7 +436,7 @@ describe('LocalStore w/ IndexedDB Persistence (Non generic)', () => { await test.executeQuery(queryCount); test.assertRemoteDocumentsRead(2, 0); - test.assertOverlaysRead(2, 0); + test.assertOverlaysRead(2, 0, {}); test.assertQueryReturned('coll/a', 'coll/c'); }); @@ -441,7 +454,9 @@ describe('LocalStore w/ IndexedDB Persistence (Non generic)', () => { const queryTime = query('coll', orderBy('time', 'asc')); await test.executeQuery(queryTime); - test.assertOverlaysRead(1, 0); + test.assertOverlaysRead(1, 0, { + [key('coll/a').toString()]: MutationType.Set + }); test.assertQueryReturned('coll/a'); }); });