From 39d0238738ec88517881fafb785f88fb921dea1c Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Wed, 9 Feb 2022 17:00:13 -0600 Subject: [PATCH 1/9] Integrate Document Overlay with the SDK. --- .../src/local/local_documents_view.ts | 338 +++++++++--- .../firestore/src/local/local_store_impl.ts | 116 ++++- packages/firestore/src/local/query_engine.ts | 8 +- packages/firestore/src/model/collections.ts | 8 + packages/firestore/src/model/document.ts | 5 +- packages/firestore/src/model/field_mask.ts | 4 + packages/firestore/src/model/mutation.ts | 133 ++++- .../firestore/src/model/mutation_batch.ts | 51 +- .../test/unit/local/counting_query_engine.ts | 1 + .../test/unit/local/local_store.test.ts | 130 +++-- .../test/unit/local/query_engine.test.ts | 93 +++- .../test/unit/model/mutation.test.ts | 483 +++++++++++++++++- packages/firestore/test/util/helpers.ts | 86 +++- 13 files changed, 1231 insertions(+), 225 deletions(-) diff --git a/packages/firestore/src/local/local_documents_view.ts b/packages/firestore/src/local/local_documents_view.ts index cf60ca6dbf1..3014efbd296 100644 --- a/packages/firestore/src/local/local_documents_view.ts +++ b/packages/firestore/src/local/local_documents_view.ts @@ -22,20 +22,32 @@ import { Query, queryMatches } from '../core/query'; -import { SnapshotVersion } from '../core/snapshot_version'; +import { Timestamp } from '../lite-api/timestamp'; import { DocumentKeySet, + OverlayMap, DocumentMap, - documentMap, - MutableDocumentMap + MutableDocumentMap, + newDocumentKeyMap, + newMutationMap, + newOverlayMap } from '../model/collections'; import { Document, MutableDocument } from '../model/document'; import { DocumentKey } from '../model/document_key'; -import { mutationApplyToLocalView } from '../model/mutation'; -import { MutationBatch } from '../model/mutation_batch'; +import { IndexOffset } from '../model/field_index'; +import { FieldMask } from '../model/field_mask'; +import { + calculateOverlayMutation, + mutationApplyToLocalView, + PatchMutation +} from '../model/mutation'; +import { Overlay } from '../model/overlay'; import { ResourcePath } from '../model/path'; import { debugAssert } from '../util/assert'; +import { SortedMap } from '../util/sorted_map'; +import { SortedSet } from '../util/sorted_set'; +import { DocumentOverlayCache } from './document_overlay_cache'; import { IndexManager } from './index_manager'; import { MutationQueue } from './mutation_queue'; import { PersistencePromise } from './persistence_promise'; @@ -52,6 +64,7 @@ export class LocalDocumentsView { constructor( readonly remoteDocumentCache: RemoteDocumentCache, readonly mutationQueue: MutationQueue, + readonly documentOverlayCache: DocumentOverlayCache, readonly indexManager: IndexManager ) {} @@ -65,36 +78,24 @@ export class LocalDocumentsView { transaction: PersistenceTransaction, key: DocumentKey ): PersistencePromise { - return this.mutationQueue - .getAllMutationBatchesAffectingDocumentKey(transaction, key) - .next(batches => this.getDocumentInternal(transaction, key, batches)); - } - - /** Internal version of `getDocument` that allows reusing batches. */ - private getDocumentInternal( - transaction: PersistenceTransaction, - key: DocumentKey, - inBatches: MutationBatch[] - ): PersistencePromise { - return this.remoteDocumentCache.getEntry(transaction, key).next(doc => { - for (const batch of inBatches) { - batch.applyToLocalView(doc); - } - return doc as Document; - }); - } - - // Returns the view of the given `docs` as they would appear after applying - // all mutations in the given `batches`. - private applyLocalMutationsToDocuments( - docs: MutableDocumentMap, - batches: MutationBatch[] - ): void { - docs.forEach((key, localView) => { - for (const batch of batches) { - batch.applyToLocalView(localView); - } - }); + let overlay: Overlay | null = null; + return this.documentOverlayCache + .getOverlay(transaction, key) + .next(value => { + overlay = value; + return this.getBaseDocument(transaction, key, overlay); + }) + .next(document => { + if (overlay !== null) { + mutationApplyToLocalView( + overlay.mutation, + document, + null, + Timestamp.now() + ); + } + return document as Document; + }); } /** @@ -110,23 +111,178 @@ export class LocalDocumentsView { return this.remoteDocumentCache .getEntries(transaction, keys) .next(docs => - this.applyLocalViewToDocuments(transaction, docs).next( - () => docs as DocumentMap - ) + this.getLocalViewOfDocuments( + transaction, + docs, + new SortedSet(DocumentKey.comparator) + ).next(() => docs as DocumentMap) ); } /** - * Applies the local view the given `baseDocs` without retrieving documents - * from the local store. + * Similar to `getDocuments`, but creates the local view from the given + * `baseDocs` without retrieving documents from the local store. + * + * @param transaction - The transaction this operation is scoped to. + * @param docs - The documents to apply local mutations to get the local views. + * @param existenceStateChanged - The set of document keys whose existence state + * is changed. This is useful to determine if some documents overlay needs + * to be recalculated. + */ + getLocalViewOfDocuments( + transaction: PersistenceTransaction, + docs: MutableDocumentMap, + existenceStateChanged: DocumentKeySet + ): PersistencePromise { + return this.computeViews( + transaction, + docs, + newOverlayMap(), + existenceStateChanged + ); + } + + /** + * Computes the local view for documents, applying overlays from both + * `memoizedOverlays` and the overlay cache. */ - applyLocalViewToDocuments( + computeViews( + transaction: PersistenceTransaction, + docs: MutableDocumentMap, + memoizedOverlays: OverlayMap, + existenceStateChanged: DocumentKeySet + ): PersistencePromise { + let results = new SortedMap(DocumentKey.comparator); + let recalculateDocuments = new SortedMap( + DocumentKey.comparator + ); + const promises: Array> = []; + docs.forEach((_, doc) => { + const overlayPromise = memoizedOverlays.has(doc.key) + ? PersistencePromise.resolve(memoizedOverlays.get(doc.key)!) + : this.documentOverlayCache.getOverlay(transaction, doc.key); + + promises.push( + overlayPromise.next((overlay: Overlay | null) => { + // Recalculate an overlay if the document's existence state is changed + // due to a remote event *and* the overlay is a PatchMutation. This is + // because document existence state can change if some patch mutation's + // preconditions are met. + // NOTE: we recalculate when `overlay` is null as well, because there + // might be a patch mutation whose precondition does not match before + // the change (hence overlay==null), but would now match. + if ( + existenceStateChanged.has(doc.key) && + (overlay === null || overlay.mutation instanceof PatchMutation) + ) { + recalculateDocuments = recalculateDocuments.insert(doc.key, doc); + } else if (overlay !== null) { + mutationApplyToLocalView( + overlay.mutation, + doc, + null, + Timestamp.now() + ); + } + }) + ); + }); + + return PersistencePromise.waitFor(promises) + .next(() => + this.recalculateAndSaveOverlays(transaction, recalculateDocuments) + ) + .next(() => { + docs.forEach((key, value) => { + results = results.insert(key, value); + }); + return results; + }); + } + + private recalculateAndSaveOverlays( transaction: PersistenceTransaction, - baseDocs: MutableDocumentMap + docs: MutableDocumentMap ): PersistencePromise { + const masks = newDocumentKeyMap(); + // A reverse lookup map from batch id to the documents within that batch. + let documentsByBatchId = new SortedMap( + (key1: number, key2: number) => key1 - key2 + ); + let processed = new SortedSet(DocumentKey.comparator); return this.mutationQueue - .getAllMutationBatchesAffectingDocumentKeys(transaction, baseDocs) - .next(batches => this.applyLocalMutationsToDocuments(baseDocs, batches)); + .getAllMutationBatchesAffectingDocumentKeys(transaction, docs) + .next(batches => { + batches.forEach(batch => { + batch.keys().forEach(key => { + let mask: FieldMask | null = masks.has(key) + ? masks.get(key)! + : FieldMask.empty(); + mask = batch.applyToLocalViewWithFieldMask(docs.get(key)!, mask); + masks.set(key, mask); + if (documentsByBatchId.get(batch.batchId) === null) { + documentsByBatchId = documentsByBatchId.insert( + batch.batchId, + new SortedSet(DocumentKey.comparator) + ); + } + const newSet = documentsByBatchId.get(batch.batchId)!.add(key); + documentsByBatchId = documentsByBatchId.insert( + batch.batchId, + newSet + ); + }); + }); + }) + .next(() => { + const promises: Array> = []; + // Iterate in descending order of batch IDs, and skip documents that are + // already saved. + const iter = documentsByBatchId.getReverseIterator(); + while (iter.hasNext()) { + const entry = iter.getNext(); + const batchId = entry.key; + const keys = entry.value; + const overlays = newMutationMap(); + keys.forEach(key => { + if (!processed.has(key)) { + // TODO: Should we change `overlays` type to Map + // and update `saveOverlays` to accept (and skip) null values? + const overlayMutation = calculateOverlayMutation( + docs.get(key)!, + masks.get(key)! + ); + if (overlayMutation !== null) { + overlays.set(key, overlayMutation); + } + processed = processed.add(key); + } + }); + promises.push( + this.documentOverlayCache.saveOverlays( + transaction, + batchId, + overlays + ) + ); + } + return PersistencePromise.waitFor(promises); + }); + } + + /** + * Recalculates overlays by reading the documents from remote document cache + * first, and saves them after they are calculated. + */ + recalculateAndSaveOverlaysForDocumentKeys( + transaction: PersistenceTransaction, + documentKeys: DocumentKeySet + ): PersistencePromise { + return this.remoteDocumentCache + .getEntries(transaction, documentKeys) + .next(docs => { + return this.recalculateAndSaveOverlays(transaction, docs); + }); } /** @@ -140,7 +296,7 @@ export class LocalDocumentsView { getDocumentsMatchingQuery( transaction: PersistenceTransaction, query: Query, - sinceReadTime: SnapshotVersion + offset: IndexOffset ): PersistencePromise { if (isDocumentQuery(query)) { return this.getDocumentsMatchingDocumentQuery(transaction, query.path); @@ -148,13 +304,13 @@ export class LocalDocumentsView { return this.getDocumentsMatchingCollectionGroupQuery( transaction, query, - sinceReadTime + offset ); } else { return this.getDocumentsMatchingCollectionQuery( transaction, query, - sinceReadTime + offset ); } } @@ -166,7 +322,9 @@ export class LocalDocumentsView { // Just do a simple document lookup. return this.getDocument(transaction, new DocumentKey(docPath)).next( document => { - let result = documentMap(); + let result = new SortedMap( + DocumentKey.comparator + ); if (document.isFoundDocument()) { result = result.insert(document.key, document); } @@ -178,14 +336,14 @@ export class LocalDocumentsView { private getDocumentsMatchingCollectionGroupQuery( transaction: PersistenceTransaction, query: Query, - sinceReadTime: SnapshotVersion + offset: IndexOffset ): PersistencePromise { debugAssert( query.path.isEmpty(), 'Currently we only support collection group queries at the root.' ); const collectionId = query.collectionGroup!; - let results = documentMap(); + let results = new SortedMap(DocumentKey.comparator); return this.indexManager .getCollectionParents(transaction, collectionId) .next(parents => { @@ -199,7 +357,7 @@ export class LocalDocumentsView { return this.getDocumentsMatchingCollectionQuery( transaction, collectionQuery, - sinceReadTime + offset ).next(r => { r.forEach((key, doc) => { results = results.insert(key, doc); @@ -212,46 +370,64 @@ export class LocalDocumentsView { private getDocumentsMatchingCollectionQuery( transaction: PersistenceTransaction, query: Query, - sinceReadTime: SnapshotVersion + offset: IndexOffset ): PersistencePromise { // Query the remote documents and overlay mutations. - let results: MutableDocumentMap; + let remoteDocuments: MutableDocumentMap; return this.remoteDocumentCache - .getAll(transaction, query.path, sinceReadTime) + .getAll(transaction, query.path, offset.readTime) .next(queryResults => { - results = queryResults; - return this.mutationQueue.getAllMutationBatchesAffectingQuery( + remoteDocuments = queryResults; + return this.documentOverlayCache.getOverlaysForCollection( transaction, - query + query.path, + offset.largestBatchId ); }) - .next(mutationBatches => { - for (const batch of mutationBatches) { - for (const mutation of batch.mutations) { - const key = mutation.key; - let document = results.get(key); - if (document == null) { - // Create invalid document to apply mutations on top of - document = MutableDocument.newInvalidDocument(key); - results = results.insert(key, document); - } - mutationApplyToLocalView(mutation, document, batch.localWriteTime); - if (!document.isFoundDocument()) { - results = results.remove(key); - } - } - } - }) - .next(() => { - // Finally, filter out any documents that don't actually match - // the query. - results.forEach((key, doc) => { - if (!queryMatches(query, doc)) { - results = results.remove(key); + .next(overlays => { + // As documents might match the query because of their overlay we need to + // include documents for all overlays in the initial document set. + overlays.forEach((_, overlay) => { + const key = overlay.getKey(); + if (remoteDocuments.get(key) === null) { + remoteDocuments = remoteDocuments.insert( + key, + MutableDocument.newInvalidDocument(key) + ); } }); - return results as DocumentMap; + // Apply the overlays and match against the query. + let results = new SortedMap( + DocumentKey.comparator + ); + remoteDocuments.forEach((key, document) => { + const overlay = overlays.get(key); + if (overlay !== undefined) { + mutationApplyToLocalView( + overlay.mutation, + document, + null, + Timestamp.now() + ); + } + // Finally, insert the documents that still match the query + if (queryMatches(query, document)) { + results = results.insert(key, document); + } + }); + return results; }); } + + /** Returns a base document that can be used to apply `overlay`. */ + private getBaseDocument( + transaction: PersistenceTransaction, + key: DocumentKey, + overlay: Overlay | null + ): PersistencePromise { + return overlay === null || overlay.mutation instanceof PatchMutation + ? this.remoteDocumentCache.getEntry(transaction, key) + : PersistencePromise.resolve(MutableDocument.newInvalidDocument(key)); + } } diff --git a/packages/firestore/src/local/local_store_impl.ts b/packages/firestore/src/local/local_store_impl.ts index fb18c423dc7..4b1eb5e49b9 100644 --- a/packages/firestore/src/local/local_store_impl.ts +++ b/packages/firestore/src/local/local_store_impl.ts @@ -55,6 +55,7 @@ import { SortedMap } from '../util/sorted_map'; import { BATCHID_UNKNOWN } from '../util/types'; import { BundleCache } from './bundle_cache'; +import { DocumentOverlayCache } from './document_overlay_cache'; import { IndexManager } from './index_manager'; import { IndexedDbMutationQueue } from './indexeddb_mutation_queue'; import { IndexedDbPersistence } from './indexeddb_persistence'; @@ -125,6 +126,12 @@ class LocalStoreImpl implements LocalStore { */ mutationQueue!: MutationQueue; + /** + * The overlays that can be used to short circuit applying all mutations from + * mutation queue. + */ + documentOverlayCache!: DocumentOverlayCache; + /** The set of all cached remote documents. */ remoteDocuments: RemoteDocumentCache; @@ -186,6 +193,7 @@ class LocalStoreImpl implements LocalStore { initializeUserComponents(user: User): void { // TODO(indexing): Add spec tests that test these components change after a // user change + this.documentOverlayCache = this.persistence.getDocumentOverlayCache(user); this.indexManager = this.persistence.getIndexManager(user); this.mutationQueue = this.persistence.getMutationQueue( user, @@ -194,6 +202,7 @@ class LocalStoreImpl implements LocalStore { this.localDocuments = new LocalDocumentsView( this.remoteDocuments, this.mutationQueue, + this.documentOverlayCache, this.indexManager ); this.remoteDocuments.setIndexManager(this.indexManager); @@ -209,6 +218,13 @@ class LocalStoreImpl implements LocalStore { } } +class DocumentChangeResult { + constructor( + readonly changedDocuments: MutableDocumentMap, + readonly existenceChangedKeys: DocumentKeySet + ) {} +} + export function newLocalStore( /** Manages our in-memory or durable persistence. */ persistence: Persistence, @@ -296,6 +312,7 @@ export function localStoreWriteLocally( const keys = mutations.reduce((keys, m) => keys.add(m.key), documentKeySet()); let existingDocs: DocumentMap; + let mutationBatch: MutationBatch; return localStoreImpl.persistence .runTransaction('Locally write mutations', 'readwrite', txn => { @@ -340,11 +357,19 @@ export function localStoreWriteLocally( baseMutations, mutations ); + }) + .next(batch => { + mutationBatch = batch; + const overlays = batch.applyToLocalDocumentSet(existingDocs); + return localStoreImpl.documentOverlayCache.saveOverlays( + txn, + batch.batchId, + overlays + ); }); }) - .then(batch => { - batch.applyToLocalDocumentSet(existingDocs); - return { batchId: batch.batchId, changes: existingDocs }; + .then(() => { + return { batchId: mutationBatch.batchId, changes: existingDocs }; }); } @@ -383,11 +408,38 @@ export function localStoreAcknowledgeBatch( ) .next(() => documentBuffer.apply(txn)) .next(() => localStoreImpl.mutationQueue.performConsistencyCheck(txn)) + .next(() => + localStoreImpl.documentOverlayCache.removeOverlaysForBatchId( + txn, + affected, + batchResult.batch.batchId + ) + ) + .next(() => + localStoreImpl.localDocuments.recalculateAndSaveOverlaysForDocumentKeys( + txn, + getKeysWithTransformResults(batchResult) + ) + ) .next(() => localStoreImpl.localDocuments.getDocuments(txn, affected)); } ); } +function getKeysWithTransformResults( + batchResult: MutationBatchResult +): DocumentKeySet { + let result = documentKeySet(); + + for (let i = 0; i < batchResult.mutationResults.length; ++i) { + const mutationResult = batchResult.mutationResults[i]; + if (mutationResult.transformResults.length > 0) { + result = result.add(batchResult.batch.mutations[i].key); + } + } + return result; +} + /** * Removes mutations from the MutationQueue for the specified batch; * LocalDocuments will be recalculated. @@ -412,6 +464,19 @@ export function localStoreRejectBatch( return localStoreImpl.mutationQueue.removeMutationBatch(txn, batch); }) .next(() => localStoreImpl.mutationQueue.performConsistencyCheck(txn)) + .next(() => + localStoreImpl.documentOverlayCache.removeOverlaysForBatchId( + txn, + affectedKeys, + batchId + ) + ) + .next(() => + localStoreImpl.localDocuments.recalculateAndSaveOverlaysForDocumentKeys( + txn, + affectedKeys + ) + ) .next(() => localStoreImpl.localDocuments.getDocuments(txn, affectedKeys) ); @@ -530,6 +595,7 @@ export function localStoreApplyRemoteEventToLocalCache( }); let changedDocs = mutableDocumentMap(); + let existenceChangedKeys = documentKeySet(); remoteEvent.documentUpdates.forEach(key => { if (remoteEvent.resolvedLimboDocuments.has(key)) { promises.push( @@ -541,15 +607,16 @@ export function localStoreApplyRemoteEventToLocalCache( } }); - // Each loop iteration only affects its "own" doc, so it's safe to get all the remote - // documents in advance in a single call. + // Each loop iteration only affects its "own" doc, so it's safe to get all + // the remote documents in advance in a single call. promises.push( populateDocumentChangeBuffer( txn, documentBuffer, remoteEvent.documentUpdates ).next(result => { - changedDocs = result; + changedDocs = result.changedDocuments; + existenceChangedKeys = result.existenceChangedKeys; }) ); @@ -580,9 +647,10 @@ export function localStoreApplyRemoteEventToLocalCache( return PersistencePromise.waitFor(promises) .next(() => documentBuffer.apply(txn)) .next(() => - localStoreImpl.localDocuments.applyLocalViewToDocuments( + localStoreImpl.localDocuments.getLocalViewOfDocuments( txn, - changedDocs + changedDocs, + existenceChangedKeys ) ) .next(() => changedDocs); @@ -595,7 +663,11 @@ export function localStoreApplyRemoteEventToLocalCache( /** * Populates document change buffer with documents from backend or a bundle. - * Returns the document changes resulting from applying those documents. + * Returns the document changes resulting from applying those documents, and + * also a set of documents whose existence state are changed as a result. + * + * Note: this function will use `documentVersions` if it is defined; + * when it is not defined, resorts to `globalVersion`. * * @param txn - Transaction to use to read existing documents from storage. * @param documentBuffer - Document buffer to collect the resulted changes to be @@ -605,22 +677,25 @@ export function localStoreApplyRemoteEventToLocalCache( * documents have the same read time. * @param documentVersions - A DocumentKey-to-SnapshotVersion map if documents * have their own read time. - * - * Note: this function will use `documentVersions` if it is defined; - * when it is not defined, resorts to `globalVersion`. */ function populateDocumentChangeBuffer( txn: PersistenceTransaction, documentBuffer: RemoteDocumentChangeBuffer, documents: MutableDocumentMap -): PersistencePromise { +): PersistencePromise { let updatedKeys = documentKeySet(); + let conditionChanged = documentKeySet(); documents.forEach(k => (updatedKeys = updatedKeys.add(k))); return documentBuffer.getEntries(txn, updatedKeys).next(existingDocs => { let changedDocs = mutableDocumentMap(); documents.forEach((key, doc) => { const existingDoc = existingDocs.get(key)!; + // Check if see if there is a existence state change for this document. + if (doc.isFoundDocument() !== existingDoc.isFoundDocument()) { + conditionChanged = conditionChanged.add(key); + } + // Note: The order of the steps below is important, since we want // to ensure that rejected limbo resolutions (which fabricate // NoDocuments with SnapshotVersion.min()) never add documents to @@ -655,7 +730,7 @@ function populateDocumentChangeBuffer( ); } }); - return changedDocs; + return new DocumentChangeResult(changedDocs, conditionChanged); }); } @@ -1225,11 +1300,11 @@ export async function localStoreApplyBundledDocuments( 'readwrite', txn => { return populateDocumentChangeBuffer(txn, documentBuffer, documentMap) - .next(changedDocs => { + .next(documentChangeResult => { documentBuffer.apply(txn); - return changedDocs; + return documentChangeResult; }) - .next(changedDocs => { + .next(documentChangeResult => { return localStoreImpl.targetCache .removeMatchingKeysForTargetId(txn, umbrellaTargetData.targetId) .next(() => @@ -1240,12 +1315,13 @@ export async function localStoreApplyBundledDocuments( ) ) .next(() => - localStoreImpl.localDocuments.applyLocalViewToDocuments( + localStoreImpl.localDocuments.getLocalViewOfDocuments( txn, - changedDocs + documentChangeResult.changedDocuments, + documentChangeResult.existenceChangedKeys ) ) - .next(() => changedDocs); + .next(() => documentChangeResult.changedDocuments); }); } ); diff --git a/packages/firestore/src/local/query_engine.ts b/packages/firestore/src/local/query_engine.ts index 3cf3ed956e1..66f4d59de1a 100644 --- a/packages/firestore/src/local/query_engine.ts +++ b/packages/firestore/src/local/query_engine.ts @@ -28,6 +28,10 @@ import { import { SnapshotVersion } from '../core/snapshot_version'; import { DocumentKeySet, DocumentMap } from '../model/collections'; import { Document } from '../model/document'; +import { + IndexOffset, + newIndexOffsetSuccessorFromReadTime +} from '../model/field_index'; import { debugAssert } from '../util/assert'; import { getLogLevel, LogLevel, logDebug } from '../util/log'; import { SortedSet } from '../util/sorted_set'; @@ -117,7 +121,7 @@ export class QueryEngine { return this.localDocumentsView!.getDocumentsMatchingQuery( transaction, query, - lastLimboFreeSnapshotVersion + newIndexOffsetSuccessorFromReadTime(lastLimboFreeSnapshotVersion, -1) ).next(updatedResults => { // We merge `previousResults` into `updateResults`, since // `updateResults` is already a DocumentMap. If a document is @@ -207,7 +211,7 @@ export class QueryEngine { return this.localDocumentsView!.getDocumentsMatchingQuery( transaction, query, - SnapshotVersion.min() + IndexOffset.min() ); } } diff --git a/packages/firestore/src/model/collections.ts b/packages/firestore/src/model/collections.ts index de760c6e00b..9440c7f78a7 100644 --- a/packages/firestore/src/model/collections.ts +++ b/packages/firestore/src/model/collections.ts @@ -66,6 +66,14 @@ export function newMutationMap(): MutationMap { ); } +export type DocumentKeyMap = ObjectMap; +export function newDocumentKeyMap(): DocumentKeyMap { + return new ObjectMap( + key => key.toString(), + (l, r) => l.isEqual(r) + ); +} + export type DocumentVersionMap = SortedMap; const EMPTY_DOCUMENT_VERSION_MAP = new SortedMap( DocumentKey.comparator diff --git a/packages/firestore/src/model/document.ts b/packages/firestore/src/model/document.ts index 5ab81c4e6d5..279f5031371 100644 --- a/packages/firestore/src/model/document.ts +++ b/packages/firestore/src/model/document.ts @@ -285,11 +285,8 @@ export class MutableDocument implements Document { } setHasLocalMutations(): MutableDocument { - debugAssert( - this.isFoundDocument(), - 'Only found documents can have local mutations' - ); this.documentState = DocumentState.HAS_LOCAL_MUTATIONS; + this.version = SnapshotVersion.min(); return this; } diff --git a/packages/firestore/src/model/field_mask.ts b/packages/firestore/src/model/field_mask.ts index 4c1d146b36e..e30d8f6a65b 100644 --- a/packages/firestore/src/model/field_mask.ts +++ b/packages/firestore/src/model/field_mask.ts @@ -42,6 +42,10 @@ export class FieldMask { ); } + static empty(): FieldMask { + return new FieldMask([]); + } + /** * Verifies that `fieldPath` is included by at least one field in this field * mask. diff --git a/packages/firestore/src/model/mutation.ts b/packages/firestore/src/model/mutation.ts index 80d30dd39a7..9a24adb3502 100644 --- a/packages/firestore/src/model/mutation.ts +++ b/packages/firestore/src/model/mutation.ts @@ -20,6 +20,7 @@ import { Timestamp } from '../lite-api/timestamp'; import { Value as ProtoValue } from '../protos/firestore_proto_api'; import { debugAssert, hardAssert } from '../util/assert'; import { arrayEquals } from '../util/misc'; +import { SortedSet } from '../util/sorted_set'; import { Document, MutableDocument } from './document'; import { DocumentKey } from './document_key'; @@ -214,6 +215,59 @@ export abstract class Mutation { abstract readonly fieldTransforms: FieldTransform[]; } +/** + * A utility method to calculate a `Mutation` representing the overlay from the + * final state of the document, and a `FieldMask` representing the fields that + * are mutated by the local mutations. + */ +export function calculateOverlayMutation( + doc: MutableDocument, + mask: FieldMask | null +): Mutation | null { + if (!doc.hasLocalMutations || (mask !== null && mask!.fields.length === 0)) { + return null; + } + + // mask is null when there are Set or Delete being applied to get to the + // current document. + if (mask === null) { + if (doc.isNoDocument()) { + return new DeleteMutation(doc.key, Precondition.none()); + } else { + return new SetMutation(doc.key, doc.data, Precondition.none()); + } + } else { + const docValue = doc.data; + const patchValue = ObjectValue.empty(); + let maskSet = new SortedSet(FieldPath.comparator); + mask.fields.forEach(path => { + if (!maskSet.has(path)) { + const value = docValue.field(path); + // If we are deleting a nested field, we take the immediate parent as + // the mask used to construct the resulting mutation. + // Justification: Nested fields can create parent fields implicitly. If + // only a leaf entry is deleted in later mutations, the parent field + // should still remain, but we may have lost this information. + // Consider mutation (foo.bar 1), then mutation (foo.bar delete()). + // This leaves the final result (foo, {}). Despite the fact that `doc` + // has the correct result, `foo` is not in `mask`, and the resulting + // mutation would miss `foo`. + if (value === null && path.length > 1) { + path = path.popLast(); + } + patchValue.set(path, docValue.field(path)!); + maskSet = maskSet.add(path); + } + }); + return new PatchMutation( + doc.key, + patchValue, + new FieldMask(maskSet.toArray()), + Precondition.none() + ); + } +} + /** * Applies this mutation to the given document for the purposes of computing a * new remote document. If the input document doesn't match the expected state @@ -254,26 +308,39 @@ export function mutationApplyToRemoteDocument( * @param document - The document to mutate. The input document can be an * invalid document if the client has no knowledge of the pre-mutation state * of the document. + * @param previousMask - The fields that have been updated before applying this mutation. * @param localWriteTime - A timestamp indicating the local write time of the * batch this mutation is a part of. + * @returns A `FieldMask` representing the fields that are changed by applying this mutation. */ export function mutationApplyToLocalView( mutation: Mutation, document: MutableDocument, + previousMask: FieldMask | null, localWriteTime: Timestamp -): void { +): FieldMask | null { mutationVerifyKeyMatches(mutation, document); if (mutation instanceof SetMutation) { - setMutationApplyToLocalView(mutation, document, localWriteTime); + return setMutationApplyToLocalView( + mutation, + document, + previousMask, + localWriteTime + ); } else if (mutation instanceof PatchMutation) { - patchMutationApplyToLocalView(mutation, document, localWriteTime); + return patchMutationApplyToLocalView( + mutation, + document, + previousMask, + localWriteTime + ); } else { debugAssert( mutation instanceof DeleteMutation, 'Unexpected mutation type: ' + mutation ); - deleteMutationApplyToLocalView(mutation, document); + return deleteMutationApplyToLocalView(mutation, document, previousMask); } } @@ -306,7 +373,7 @@ export function mutationExtractBaseValue( ); if (coercedValue != null) { - if (baseObject == null) { + if (baseObject === null) { baseObject = ObjectValue.empty(); } baseObject.set(fieldTransform.field, coercedValue); @@ -358,16 +425,6 @@ function mutationVerifyKeyMatches( ); } -/** - * Returns the version from the given document for use as the result of a - * mutation. Mutations are defined to return the version of the base document - * only if it is an existing document. Deleted and unknown documents have a - * post-mutation version of SnapshotVersion.min(). - */ -function getPostMutationVersion(document: MutableDocument): SnapshotVersion { - return document.isFoundDocument() ? document.version : SnapshotVersion.min(); -} - /** * A mutation that creates or replaces the document at the given key with the * object value contents. @@ -408,12 +465,13 @@ function setMutationApplyToRemoteDocument( function setMutationApplyToLocalView( mutation: SetMutation, document: MutableDocument, + previousMask: FieldMask | null, localWriteTime: Timestamp -): void { +): FieldMask | null { if (!preconditionIsValidForDocument(mutation.precondition, document)) { // The mutation failed to apply (e.g. a document ID created with add() // caused a name collision). - return; + return previousMask; } const newData = mutation.value.clone(); @@ -424,8 +482,10 @@ function setMutationApplyToLocalView( ); newData.setAll(transformResults); document - .convertToFoundDocument(getPostMutationVersion(document), newData) + .convertToFoundDocument(document.version, newData) .setHasLocalMutations(); + // SetMutation overwrites all fields. + return null; } /** @@ -485,10 +545,11 @@ function patchMutationApplyToRemoteDocument( function patchMutationApplyToLocalView( mutation: PatchMutation, document: MutableDocument, + previousMask: FieldMask | null, localWriteTime: Timestamp -): void { +): FieldMask | null { if (!preconditionIsValidForDocument(mutation.precondition, document)) { - return; + return previousMask; } const transformResults = localTransformResults( @@ -500,8 +561,24 @@ function patchMutationApplyToLocalView( newData.setAll(getPatch(mutation)); newData.setAll(transformResults); document - .convertToFoundDocument(getPostMutationVersion(document), newData) + .convertToFoundDocument(document.version, newData) .setHasLocalMutations(); + + if (previousMask === null) { + return null; + } + + let mergedMaskSet = new SortedSet(FieldPath.comparator); + previousMask.fields.forEach( + fieldPath => (mergedMaskSet = mergedMaskSet.add(fieldPath)) + ); + mutation.fieldMask.fields.forEach( + fieldPath => (mergedMaskSet = mergedMaskSet.add(fieldPath)) + ); + mutation.fieldTransforms + .map(transform => transform.field) + .forEach(fieldPath => (mergedMaskSet = mergedMaskSet.add(fieldPath))); + return new FieldMask(mergedMaskSet.toArray()); } /** @@ -565,8 +642,7 @@ function serverTransformResults( * @param fieldTransforms - The field transforms to apply the result to. * @param localWriteTime - The local time of the mutation (used to * generate ServerTimestampValues). - * @param mutableDocument - The current state of the document after applying all - * previous mutations. + * @param mutableDocument - The document to apply transforms on. * @returns The transform results list. */ function localTransformResults( @@ -621,17 +697,18 @@ function deleteMutationApplyToRemoteDocument( function deleteMutationApplyToLocalView( mutation: DeleteMutation, - document: MutableDocument -): void { + document: MutableDocument, + previousMask: FieldMask | null +): FieldMask | null { debugAssert( document.key.isEqual(mutation.key), 'Can only apply mutation to document with same key' ); if (preconditionIsValidForDocument(mutation.precondition, document)) { - // We don't call `setHasLocalMutations()` since we want to be backwards - // compatible with the existing SDK behavior. - document.convertToNoDocument(SnapshotVersion.min()); + document.convertToNoDocument(document.version).setHasLocalMutations(); + return null; } + return previousMask; } /** diff --git a/packages/firestore/src/model/mutation_batch.ts b/packages/firestore/src/model/mutation_batch.ts index 82dd01bfd9a..6ba1fdc09ac 100644 --- a/packages/firestore/src/model/mutation_batch.ts +++ b/packages/firestore/src/model/mutation_batch.ts @@ -24,12 +24,16 @@ import { arrayEquals } from '../util/misc'; import { documentKeySet, DocumentKeySet, + MutationMap, DocumentMap, DocumentVersionMap, - documentVersionMap + documentVersionMap, + newMutationMap } from './collections'; import { MutableDocument } from './document'; +import { FieldMask } from './field_mask'; import { + calculateOverlayMutation, Mutation, mutationApplyToLocalView, mutationApplyToRemoteDocument, @@ -89,48 +93,81 @@ export class MutationBatch { } } } + /** + * Computes the local view of a document given all the mutations in this + * batch. + * + * @param document - The document to apply mutations to. + * @returns A `FieldMask` representing all the fields that are mutated. + */ + applyToLocalView(document: MutableDocument): FieldMask | null { + return this.applyToLocalViewWithFieldMask(document, FieldMask.empty()); + } /** * Computes the local view of a document given all the mutations in this * batch. * * @param document - The document to apply mutations to. + * @param mutatedFields - Fields that have been updated before applying this mutation batch. + * @returns A `FieldMask` representing all the fields that are mutated. */ - applyToLocalView(document: MutableDocument): void { + applyToLocalViewWithFieldMask( + document: MutableDocument, + mutatedFields: FieldMask | null + ): FieldMask | null { // First, apply the base state. This allows us to apply non-idempotent // transform against a consistent set of values. for (const mutation of this.baseMutations) { if (mutation.key.isEqual(document.key)) { - mutationApplyToLocalView(mutation, document, this.localWriteTime); + mutatedFields = mutationApplyToLocalView( + mutation, + document, + mutatedFields, + this.localWriteTime + ); } } // Second, apply all user-provided mutations. for (const mutation of this.mutations) { if (mutation.key.isEqual(document.key)) { - mutationApplyToLocalView(mutation, document, this.localWriteTime); + mutatedFields = mutationApplyToLocalView( + mutation, + document, + mutatedFields, + this.localWriteTime + ); } } + return mutatedFields; } /** * Computes the local view for all provided documents given the mutations in - * this batch. + * this batch. Returns a `DocumentKey` to `Mutation` map which can be used to + * replace all the mutation applications. */ - applyToLocalDocumentSet(documentMap: DocumentMap): void { + applyToLocalDocumentSet(documentMap: DocumentMap): MutationMap { // TODO(mrschmidt): This implementation is O(n^2). If we apply the mutations // directly (as done in `applyToLocalView()`), we can reduce the complexity // to O(n). + const overlays = newMutationMap(); this.mutations.forEach(m => { const document = documentMap.get(m.key)!; // TODO(mutabledocuments): This method should take a MutableDocumentMap // and we should remove this cast. const mutableDocument = document as MutableDocument; - this.applyToLocalView(mutableDocument); + const mutatedFields = this.applyToLocalView(mutableDocument); + const overlay = calculateOverlayMutation(mutableDocument, mutatedFields); + if (overlay !== null) { + overlays.set(m.key, overlay); + } if (!document.isValidDocument()) { mutableDocument.convertToNoDocument(SnapshotVersion.min()); } }); + return overlays; } keys(): DocumentKeySet { diff --git a/packages/firestore/test/unit/local/counting_query_engine.ts b/packages/firestore/test/unit/local/counting_query_engine.ts index 08e7b662da5..2516b0081e6 100644 --- a/packages/firestore/test/unit/local/counting_query_engine.ts +++ b/packages/firestore/test/unit/local/counting_query_engine.ts @@ -83,6 +83,7 @@ export class CountingQueryEngine extends QueryEngine { const view = new LocalDocumentsView( this.wrapRemoteDocumentCache(localDocuments.remoteDocumentCache), this.wrapMutationQueue(localDocuments.mutationQueue), + localDocuments.documentOverlayCache, localDocuments.indexManager ); diff --git a/packages/firestore/test/unit/local/local_store.test.ts b/packages/firestore/test/unit/local/local_store.test.ts index da8228d7b9d..83912f7692a 100644 --- a/packages/firestore/test/unit/local/local_store.test.ts +++ b/packages/firestore/test/unit/local/local_store.test.ts @@ -241,24 +241,19 @@ class LocalStoreTester { afterAcknowledgingMutation(options: { documentVersion: TestSnapshotVersion; - transformResult?: api.Value; + transformResults?: api.Value[]; }): LocalStoreTester { this.prepareNextStep(); this.promiseChain = this.promiseChain .then(() => { const batch = this.batches.shift()!; - expect(batch.mutations.length).to.equal( - 1, - 'Acknowledging more than one mutation not supported.' - ); const ver = version(options.documentVersion); - const mutationResults = [ - new MutationResult( - ver, - options.transformResult ? [options.transformResult] : [] - ) - ]; + const mutationResults = options.transformResults + ? options.transformResults.map( + value => new MutationResult(ver, [value]) + ) + : [new MutationResult(ver, [])]; const write = MutationBatchResult.from(batch, ver, mutationResults); return localStoreAcknowledgeBatch(this.localStore, write); @@ -801,7 +796,7 @@ function genericLocalStoreTests( return expectLocalStore() .after(deleteMutation('foo/bar')) .toReturnRemoved('foo/bar') - .toContain(deletedDoc('foo/bar', 0)) + .toContain(deletedDoc('foo/bar', 0).setHasLocalMutations()) .afterAcknowledgingMutation({ documentVersion: 1 }) .toReturnRemoved('foo/bar') .toNotContainIfEager(deletedDoc('foo/bar', 1).setHasCommittedMutations()) @@ -819,7 +814,7 @@ function genericLocalStoreTests( .toContain(doc('foo/bar', 1, { it: 'base' })) .after(deleteMutation('foo/bar')) .toReturnRemoved('foo/bar') - .toContain(deletedDoc('foo/bar', 0)) + .toContain(deletedDoc('foo/bar', 0).setHasLocalMutations()) // remove the mutation so only the mutation is pinning the doc .afterReleasingTarget(2) .afterAcknowledgingMutation({ documentVersion: 2 }) @@ -839,10 +834,10 @@ function genericLocalStoreTests( .toReturnTargetId(2) .after(deleteMutation('foo/bar')) .toReturnRemoved('foo/bar') - .toContain(deletedDoc('foo/bar', 0)) + .toContain(deletedDoc('foo/bar', 0).setHasLocalMutations()) .after(docUpdateRemoteEvent(doc('foo/bar', 1, { it: 'base' }), [2])) .toReturnRemoved('foo/bar') - .toContain(deletedDoc('foo/bar', 0)) + .toContain(deletedDoc('foo/bar', 0).setHasLocalMutations()) // Don't need to keep doc pinned anymore .afterReleasingTarget(2) .afterAcknowledgingMutation({ documentVersion: 2 }) @@ -948,10 +943,10 @@ function genericLocalStoreTests( return expectLocalStore() .after(deleteMutation('foo/bar')) .toReturnRemoved('foo/bar') - .toContain(deletedDoc('foo/bar', 0)) + .toContain(deletedDoc('foo/bar', 0).setHasLocalMutations()) .after(patchMutation('foo/bar', { foo: 'bar' })) .toReturnRemoved('foo/bar') - .toContain(deletedDoc('foo/bar', 0)) + .toContain(deletedDoc('foo/bar', 0).setHasLocalMutations()) .afterAcknowledgingMutation({ documentVersion: 2 }) // delete mutation .toReturnRemoved('foo/bar') .toContain(deletedDoc('foo/bar', 2).setHasCommittedMutations()) @@ -1007,15 +1002,15 @@ function genericLocalStoreTests( .after(deleteMutation('foo/baz')) .toContain(doc('foo/bar', 1, { foo: 'bar' }).setHasLocalMutations()) .toContain(doc('foo/bah', 0, { foo: 'bah' }).setHasLocalMutations()) - .toContain(deletedDoc('foo/baz', 0)) + .toContain(deletedDoc('foo/baz', 0).setHasLocalMutations()) .afterAcknowledgingMutation({ documentVersion: 3 }) .toNotContain('foo/bar') .toContain(doc('foo/bah', 0, { foo: 'bah' }).setHasLocalMutations()) - .toContain(deletedDoc('foo/baz', 0)) + .toContain(deletedDoc('foo/baz', 0).setHasLocalMutations()) .afterAcknowledgingMutation({ documentVersion: 4 }) .toNotContain('foo/bar') .toNotContain('foo/bah') - .toContain(deletedDoc('foo/baz', 0)) + .toContain(deletedDoc('foo/baz', 0).setHasLocalMutations()) .afterAcknowledgingMutation({ documentVersion: 5 }) .toNotContain('foo/bar') .toNotContain('foo/bah') @@ -1041,15 +1036,15 @@ function genericLocalStoreTests( .after(deleteMutation('foo/baz')) .toContain(doc('foo/bar', 1, { foo: 'bar' }).setHasLocalMutations()) .toContain(doc('foo/bah', 0, { foo: 'bah' }).setHasLocalMutations()) - .toContain(deletedDoc('foo/baz', 0)) + .toContain(deletedDoc('foo/baz', 0).setHasLocalMutations()) .afterRejectingMutation() // patch mutation .toNotContain('foo/bar') .toContain(doc('foo/bah', 0, { foo: 'bah' }).setHasLocalMutations()) - .toContain(deletedDoc('foo/baz', 0)) + .toContain(deletedDoc('foo/baz', 0).setHasLocalMutations()) .afterRejectingMutation() // set mutation .toNotContain('foo/bar') .toNotContain('foo/bah') - .toContain(deletedDoc('foo/baz', 0)) + .toContain(deletedDoc('foo/baz', 0).setHasLocalMutations()) .afterRejectingMutation() // delete mutation .toNotContain('foo/bar') .toNotContain('foo/bah') @@ -1181,36 +1176,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] + 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() - ) - .toHaveRead({ documentsByCollection: 2, mutationsByCollection: 1 }) - .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() + ) + // TODO(overlays): implement overlaysReadByKey and overlaysReadByCollection + // No mutations are read because only overlay is needed. + .toHaveRead({ documentsByCollection: 2, mutationsByCollection: 0 }) + .finish() + ); }); // eslint-disable-next-line no-restricted-properties @@ -1340,7 +1339,7 @@ function genericLocalStoreTests( .toContain(doc('foo/bar', 1, { sum: 1 }).setHasLocalMutations()) .afterAcknowledgingMutation({ documentVersion: 2, - transformResult: { integerValue: 1 } + transformResults: [{ integerValue: 1 }] }) .toReturnChanged( doc('foo/bar', 2, { sum: 1 }).setHasCommittedMutations() @@ -1385,13 +1384,13 @@ function genericLocalStoreTests( .toContain(doc('foo/bar', 2, { sum: 3 }).setHasLocalMutations()) .afterAcknowledgingMutation({ documentVersion: 3, - transformResult: { integerValue: 1 } + transformResults: [{ integerValue: 1 }] }) .toReturnChanged(doc('foo/bar', 3, { sum: 3 }).setHasLocalMutations()) .toContain(doc('foo/bar', 3, { sum: 3 }).setHasLocalMutations()) .afterAcknowledgingMutation({ documentVersion: 4, - transformResult: { integerValue: 1339 } + transformResults: [{ integerValue: 1339 }] }) .toReturnChanged( doc('foo/bar', 4, { sum: 1339 }).setHasCommittedMutations() @@ -1452,9 +1451,28 @@ function genericLocalStoreTests( .toReturnChanged( doc('foo/bar', 2, { sum: 1, - arrayUnion: ['bar', 'foo'] + arrayUnion: ['foo'] }).setHasLocalMutations() ) + .afterAcknowledgingMutation({ + documentVersion: 3, + transformResults: [ + { integerValue: 1338 }, + { + arrayValue: { + values: [{ stringValue: 'bar' }, { stringValue: 'foo' }] + } + } + ] + }) + .toReturnChanged( + doc('foo/bar', 3, { + sum: 1338, + arrayUnion: ['bar', 'foo'] + }) + .setReadTime(SnapshotVersion.fromTimestamp(new Timestamp(0, 3000))) + .setHasCommittedMutations() + ) .finish() ); }); diff --git a/packages/firestore/test/unit/local/query_engine.test.ts b/packages/firestore/test/unit/local/query_engine.test.ts index 430ce67bd57..5ce5b938d55 100644 --- a/packages/firestore/test/unit/local/query_engine.test.ts +++ b/packages/firestore/test/unit/local/query_engine.test.ts @@ -21,20 +21,37 @@ import { User } from '../../../src/auth/user'; import { LimitType, Query, queryWithLimit } from '../../../src/core/query'; import { SnapshotVersion } from '../../../src/core/snapshot_version'; import { View } from '../../../src/core/view'; +import { Timestamp } from '../../../src/lite-api/timestamp'; +import { DocumentOverlayCache } from '../../../src/local/document_overlay_cache'; import { LocalDocumentsView } from '../../../src/local/local_documents_view'; import { MemoryIndexManager } from '../../../src/local/memory_index_manager'; +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 { QueryEngine } from '../../../src/local/query_engine'; import { RemoteDocumentCache } from '../../../src/local/remote_document_cache'; import { TargetCache } from '../../../src/local/target_cache'; -import { documentKeySet, DocumentMap } from '../../../src/model/collections'; +import { + documentKeySet, + DocumentMap, + newMutationMap +} from '../../../src/model/collections'; import { Document, MutableDocument } from '../../../src/model/document'; import { DocumentKey } from '../../../src/model/document_key'; import { DocumentSet } from '../../../src/model/document_set'; +import { IndexOffset } from '../../../src/model/field_index'; +import { Mutation } from '../../../src/model/mutation'; import { debugAssert } from '../../../src/util/assert'; -import { doc, filter, key, orderBy, query, version } from '../../util/helpers'; +import { + deleteMutation, + doc, + filter, + key, + orderBy, + query, + version +} from '../../util/helpers'; import { testMemoryEagerPersistence } from './persistence_test_helpers'; @@ -67,23 +84,26 @@ class TestLocalDocumentsView extends LocalDocumentsView { getDocumentsMatchingQuery( transaction: PersistenceTransaction, query: Query, - sinceReadTime: SnapshotVersion + offset: IndexOffset ): PersistencePromise { - const skipsDocumentsBeforeSnapshot = - !SnapshotVersion.min().isEqual(sinceReadTime); + const skipsDocumentsBeforeSnapshot = !SnapshotVersion.min().isEqual( + offset.readTime + ); expect(skipsDocumentsBeforeSnapshot).to.eq( !this.expectFullCollectionScan, 'Observed query execution mode did not match expectation' ); - return super.getDocumentsMatchingQuery(transaction, query, sinceReadTime); + return super.getDocumentsMatchingQuery(transaction, query, offset); } } describe('QueryEngine', () => { let persistence!: Persistence; let remoteDocumentCache!: RemoteDocumentCache; + let mutationQueue!: MutationQueue; + let documentOverlayCache!: DocumentOverlayCache; let targetCache!: TargetCache; let queryEngine!: QueryEngine; let localDocuments!: TestLocalDocumentsView; @@ -111,6 +131,23 @@ describe('QueryEngine', () => { }); } + /** Adds a mutation to the mutation queue. */ + function addMutation(mutation: Mutation): Promise { + return persistence.runTransaction('addMutation', 'readwrite', txn => { + return mutationQueue + .addMutationBatch(txn, Timestamp.now(), [], [mutation]) + .next(batch => { + const overlayMap = newMutationMap(); + overlayMap.set(mutation.key, mutation); + return documentOverlayCache.saveOverlays( + txn, + batch.batchId, + overlayMap + ); + }); + }); + } + async function expectOptimizedCollectionQuery( op: () => Promise ): Promise { @@ -174,10 +211,17 @@ describe('QueryEngine', () => { const indexManager = persistence.getIndexManager(User.UNAUTHENTICATED); remoteDocumentCache = persistence.getRemoteDocumentCache(); remoteDocumentCache.setIndexManager(indexManager); - + mutationQueue = persistence.getMutationQueue( + User.UNAUTHENTICATED, + indexManager + ); + documentOverlayCache = persistence.getDocumentOverlayCache( + User.UNAUTHENTICATED + ); localDocuments = new TestLocalDocumentsView( remoteDocumentCache, - persistence.getMutationQueue(User.UNAUTHENTICATED, indexManager), + mutationQueue, + documentOverlayCache, new MemoryIndexManager() ); queryEngine.setLocalDocumentsView(localDocuments); @@ -399,6 +443,39 @@ describe('QueryEngine', () => { doc('coll/b', 1, { order: 3 }) ]); }); + + it('does not include documents deleted by mutation', async () => { + const query1 = query('coll'); + await addDocument(MATCHING_DOC_A, MATCHING_DOC_B); + await persistQueryMapping(MATCHING_DOC_A.key, MATCHING_DOC_B.key); + + // Add an unacknowledged mutation + await addMutation(deleteMutation('coll/b')); + const docs = await expectFullCollectionQuery(() => + persistence.runTransaction('runQuery', 'readonly', txn => { + return targetCache + .getMatchingKeysForTargetId(txn, TEST_TARGET_ID) + .next(remoteKeys => { + return queryEngine + .getDocumentsMatchingQuery( + txn, + query1, + LAST_LIMBO_FREE_SNAPSHOT, + remoteKeys + ) + .next(documentMap => { + let result = new DocumentSet(); + documentMap.forEach((_, v) => { + result = result.add(v); + }); + return result; + }); + }); + }) + ); + + verifyResult(docs, [MATCHING_DOC_A]); + }); }); function verifyResult(actualDocs: DocumentSet, expectedDocs: Document[]): void { diff --git a/packages/firestore/test/unit/model/mutation.test.ts b/packages/firestore/test/unit/model/mutation.test.ts index e5765805e6f..1945b852204 100644 --- a/packages/firestore/test/unit/model/mutation.test.ts +++ b/packages/firestore/test/unit/model/mutation.test.ts @@ -26,13 +26,15 @@ import { deleteField } from '../../../src'; import { MutableDocument } from '../../../src/model/document'; +import { FieldMask } from '../../../src/model/field_mask'; import { mutationApplyToLocalView, mutationApplyToRemoteDocument, mutationExtractBaseValue, Mutation, MutationResult, - Precondition + Precondition, + calculateOverlayMutation } from '../../../src/model/mutation'; import { serverTimestamp as serverTimestampInternal } from '../../../src/model/server_timestamps'; import { @@ -42,12 +44,15 @@ import { import { Dict } from '../../../src/util/obj'; import { addEqualityMatcher } from '../../util/equality_matcher'; import { + computeCombinations, + computePermutations, deletedDoc, deleteMutation, doc, field, invalidDoc, key, + mergeMutation, mutationResult, patchMutation, setMutation, @@ -62,12 +67,88 @@ describe('Mutation', () => { const timestamp = Timestamp.now(); + /** + * For each document in `docs`, calculate the overlay mutations of each + * possible permutation, check whether this holds: + * document + overlay_mutation = document + mutation_list + * Returns how many cases it has run. + */ + function runPermutationTests( + docs: MutableDocument[], + mutations: Mutation[] + ): number { + let testCases = 0; + const permutations = computePermutations(mutations); + docs.forEach(doc => { + permutations.forEach(permutation => { + verifyOverlayRoundTrips(doc, ...permutation); + testCases++; + }); + }); + return testCases; + } + + function getDescription( + document: MutableDocument, + mutations: Mutation[], + overlay: Mutation | null + ): string { + let result = 'Overlay Mutation failed with:\n'; + result += 'document:\n'; + result += document.toString(); + result += '\n\n'; + + result += 'mutations:\n'; + mutations.forEach(mutation => { + result += mutation.toString() + '\n'; + }); + result += '\n'; + + result += 'overlay:\n'; + result += overlay === null ? 'null' : overlay.toString(); + result += '\n\n'; + return result; + } + + function verifyOverlayRoundTrips( + doc: MutableDocument, + ...mutations: Mutation[] + ): void { + const docForMutations = doc.mutableCopy(); + const docForOverlay = doc.mutableCopy(); + + let mask: FieldMask | null = null; + for (const mutation of mutations) { + mask = mutationApplyToLocalView( + mutation, + docForMutations, + mask, + timestamp + ); + } + + const overlay = calculateOverlayMutation(docForMutations, mask); + if (overlay !== null) { + mutationApplyToLocalView( + overlay, + docForOverlay, + /* previousMask */ null, + timestamp + ); + } + + expect(docForOverlay).to.deep.equal( + docForMutations, + getDescription(doc, mutations, overlay) + ); + } + it('can apply sets to documents', () => { const docData = { foo: 'foo-value', baz: 'baz-value' }; const document = doc('collection/key', 0, docData); const set = setMutation('collection/key', { bar: 'bar-value' }); - mutationApplyToLocalView(set, document, timestamp); + mutationApplyToLocalView(set, document, /* previousMask */ null, timestamp); expect(document).to.deep.equal( doc('collection/key', 0, { bar: 'bar-value' }).setHasLocalMutations() ); @@ -81,7 +162,12 @@ describe('Mutation', () => { 'foo.bar': 'new-bar-value' }); - mutationApplyToLocalView(patch, document, timestamp); + mutationApplyToLocalView( + patch, + document, + /* previousMask */ null, + timestamp + ); expect(document).to.deep.equal( doc('collection/key', 0, { foo: { bar: 'new-bar-value' }, @@ -100,7 +186,12 @@ describe('Mutation', () => { Precondition.none() ); - mutationApplyToLocalView(patch, document, timestamp); + mutationApplyToLocalView( + patch, + document, + /* previousMask */ null, + timestamp + ); expect(document).to.deep.equal( doc('collection/key', 0, { foo: { bar: 'new-bar-value' } @@ -118,7 +209,12 @@ describe('Mutation', () => { Precondition.none() ); - mutationApplyToLocalView(patch, document, timestamp); + mutationApplyToLocalView( + patch, + document, + /* previousMask */ null, + timestamp + ); expect(document).to.deep.equal( doc('collection/key', 0, { foo: { bar: 'new-bar-value' } @@ -134,7 +230,12 @@ describe('Mutation', () => { 'foo.bar': deleteField() }); - mutationApplyToLocalView(patch, document, timestamp); + mutationApplyToLocalView( + patch, + document, + /* previousMask */ null, + timestamp + ); expect(document).to.deep.equal( doc('collection/key', 0, { foo: { baz: 'baz-value' } @@ -151,7 +252,12 @@ describe('Mutation', () => { 'foo.bar': 'new-bar-value' }); - mutationApplyToLocalView(patch, document, timestamp); + mutationApplyToLocalView( + patch, + document, + /* previousMask */ null, + timestamp + ); expect(document).to.deep.equal( doc('collection/key', 0, { foo: { bar: 'new-bar-value' }, @@ -163,7 +269,12 @@ describe('Mutation', () => { it('patching a NoDocument yields a NoDocument', () => { const document = deletedDoc('collection/key', 0); const patch = patchMutation('collection/key', { foo: 'bar' }); - mutationApplyToLocalView(patch, document, timestamp); + mutationApplyToLocalView( + patch, + document, + /* previousMask */ null, + timestamp + ); expect(document).to.deep.equal(deletedDoc('collection/key', 0)); }); @@ -175,7 +286,12 @@ describe('Mutation', () => { 'foo.bar': serverTimestamp() }); - mutationApplyToLocalView(transform, document, timestamp); + mutationApplyToLocalView( + transform, + document, + /* previousMask */ null, + timestamp + ); // Server timestamps aren't parsed, so we manually insert it. const data = wrapObject({ @@ -193,13 +309,13 @@ describe('Mutation', () => { // test once we have integration tests. it('can create arrayUnion() transform.', () => { const transform = patchMutation('collection/key', { - foo: arrayUnion('tag'), + a: arrayUnion('tag'), 'bar.baz': arrayUnion(true, { nested: { a: [1, 2] } }) }); expect(transform.fieldTransforms).to.have.lengthOf(2); const first = transform.fieldTransforms[0]; - expect(first.field).to.deep.equal(field('foo')); + expect(first.field).to.deep.equal(field('a')); expect(first.transform).to.deep.equal( new ArrayUnionTransformOperation([wrap('tag')]) ); @@ -340,7 +456,12 @@ describe('Mutation', () => { for (const transformData of transforms) { const transform = patchMutation('collection/key', transformData); - mutationApplyToLocalView(transform, document, timestamp); + mutationApplyToLocalView( + transform, + document, + /* previousMask */ null, + timestamp + ); } const expectedDoc = doc( @@ -479,8 +600,15 @@ describe('Mutation', () => { const document = doc('collection/key', 0, { foo: 'bar' }); const mutation = deleteMutation('collection/key'); - mutationApplyToLocalView(mutation, document, Timestamp.now()); - expect(document).to.deep.equal(deletedDoc('collection/key', 0)); + mutationApplyToLocalView( + mutation, + document, + /* previousMask */ null, + Timestamp.now() + ); + expect(document).to.deep.equal( + deletedDoc('collection/key', 0).setHasLocalMutations() + ); }); it('can apply sets with mutation results', () => { @@ -627,10 +755,333 @@ describe('Mutation', () => { const inc = { sum: increment(1) }; const transform = setMutation('collection/key', inc); - mutationApplyToLocalView(transform, document, Timestamp.now()); - mutationApplyToLocalView(transform, document, Timestamp.now()); + mutationApplyToLocalView( + transform, + document, + /* previousMask */ null, + Timestamp.now() + ); + mutationApplyToLocalView( + transform, + document, + /* previousMask */ null, + Timestamp.now() + ); expect(document.isFoundDocument()).to.be.true; expect(document.data.field(field('sum'))).to.deep.equal(wrap(2)); }); + + // Mutation Overlay tests + + it('overlay with no mutation', () => { + const doc1 = doc('collection/key', 1, { + 'foo': 'foo-value', + 'baz': 'baz-value' + }); + verifyOverlayRoundTrips(doc1); + }); + + it('overlay with mutations fail by preconditions', () => { + verifyOverlayRoundTrips( + deletedDoc('collection/key', 1), + patchMutation('collection/key', { 'foo': 'bar' }), + patchMutation('collection/key', { 'a': 1 }) + ); + }); + + it('overlay with patch on invalid document', () => { + verifyOverlayRoundTrips( + MutableDocument.newInvalidDocument(key('collection/key')), + patchMutation('collection/key', { 'a': 1 }) + ); + }); + + it('overlay with one set mutation', () => { + const doc1 = doc('collection/key', 1, { + 'foo': 'foo-value', + 'baz': 'baz-value' + }); + verifyOverlayRoundTrips( + doc1, + setMutation('collection/key', { 'bar': 'bar-value' }) + ); + }); + + it('overlay with one patch mutation', () => { + const doc1 = doc('collection/key', 1, { + 'foo': { 'bar': 'bar-value' }, + 'baz': 'baz-value' + }); + verifyOverlayRoundTrips( + doc1, + patchMutation('collection/key', { 'foo.bar': 'new-bar-value' }) + ); + }); + + it('overlay with patch then merge', () => { + const upsert = mergeMutation( + 'collection/key', + { 'foo.bar': 'new-bar-value' }, + [field('foo.bar')] + ); + + verifyOverlayRoundTrips(deletedDoc('collection/key', 1), upsert); + }); + + it('overlay with delete then patch', () => { + const doc1 = doc('collection/key', 1, { 'foo': 1 }); + const deleteMutation1 = deleteMutation('collection/key'); + const patchMutation1 = patchMutation('collection/key', { + 'foo.bar': 'new-bar-value' + }); + + verifyOverlayRoundTrips(doc1, deleteMutation1, patchMutation1); + }); + + it('overlay with delete then merge', () => { + const doc1 = doc('collection/key', 1, { 'foo': 1 }); + const deleteMutation1 = deleteMutation('collection/key'); + const mergeMutation1 = mergeMutation( + 'collection/key', + { 'foo.bar': 'new-bar-value' }, + [field('foo.bar')] + ); + + verifyOverlayRoundTrips(doc1, deleteMutation1, mergeMutation1); + }); + + it('overlay with patch then patch to delete field', () => { + const doc1 = doc('collection/key', 1, { 'foo': 1 }); + const patch = patchMutation('collection/key', { + 'foo': 'foo-patched-value', + 'bar.baz': increment(1) + }); + const patchToDeleteField = patchMutation('collection/key', { + 'foo': 'foo-patched-value', + 'bar.baz': deleteField() + }); + + verifyOverlayRoundTrips(doc1, patch, patchToDeleteField); + }); + + it('overlay with patch then merge with array union', () => { + const doc1 = doc('collection/key', 1, { 'foo': 1 }); + const patch = patchMutation('collection/key', { + 'foo': 'foo-patched-value', + 'bar.baz': increment(1) + }); + const merge = mergeMutation( + 'collection/key', + { 'arrays': arrayUnion(1, 2, 3) }, + [field('arrays')] + ); + + verifyOverlayRoundTrips(doc1, patch, merge); + }); + + it('overlay with array union then remove', () => { + const doc1 = doc('collection/key', 1, { 'foo': 1 }); + const union = mergeMutation( + 'collection/key', + { 'arrays': arrayUnion(1, 2, 3) }, + [] + ); + const remove = mergeMutation( + 'collection/key', + { 'foo': 'xxx', 'arrays': arrayRemove(2) }, + [field('foo')] + ); + + verifyOverlayRoundTrips(doc1, union, remove); + }); + + it('overlay with set then increment', () => { + const doc1 = doc('collection/key', 1, { 'foo': 1 }); + const set = setMutation('collection/key', { 'foo': 2 }); + const update = patchMutation('collection/key', { 'foo': increment(2) }); + + verifyOverlayRoundTrips(doc1, set, update); + }); + + it('overlay with set then patch on deleted doc', () => { + const doc1 = deletedDoc('collection/key', 1); + const set = setMutation('collection/key', { 'bar': 'bar-value' }); + const patch = patchMutation('collection/key', { + 'foo': 'foo-patched-value', + 'bar.baz': serverTimestamp() + }); + + verifyOverlayRoundTrips(doc1, set, patch); + }); + + it('overlay with field deletion of nested field', () => { + const doc1 = doc('collection/key', 1, { 'foo': 1 }); + const patch1 = patchMutation('collection/key', { + 'foo': 'foo-patched-value', + 'bar.baz': increment(1) + }); + const patch2 = patchMutation('collection/key', { + 'foo': 'foo-patched-value', + 'bar.baz': serverTimestamp() + }); + const patch3 = patchMutation('collection/key', { + 'foo': 'foo-patched-value', + 'bar.baz': deleteField() + }); + + verifyOverlayRoundTrips(doc1, patch1, patch2, patch3); + }); + + it('overlay created from empty set with merge', () => { + const doc1 = deletedDoc('collection/key', 1); + const merge = mergeMutation('collection/key', {}, []); + verifyOverlayRoundTrips(doc1, merge); + + const doc2 = doc('collection/key', 1, { 'foo': 'foo-value' }); + verifyOverlayRoundTrips(doc2, merge); + }); + + // Below tests run on automatically generated mutation list, they are + // deterministic, but hard to debug when they fail. They will print the + // failure case, and the best way to debug is recreate the case manually in a + // separate test. + + it('overlay with mutation with multiple deletes', () => { + const docs = [ + doc('collection/key', 1, { 'foo': 'foo-value', 'bar.baz': 1 }), + deletedDoc('collection/key', 1), + unknownDoc('collection/key', 1) + ]; + + const mutations = [ + setMutation('collection/key', { 'bar': 'bar-value' }), + deleteMutation('collection/key'), + deleteMutation('collection/key'), + patchMutation('collection/key', { + 'foo': 'foo-patched-value', + 'bar.baz': serverTimestamp() + }) + ]; + + const testCases = runPermutationTests(docs, mutations); + + // There are 4! * 3 cases + expect(testCases).to.equal(72); + }); + + it('overlay by combinations and permutations', () => { + const docs: MutableDocument[] = [ + doc('collection/key', 1, { 'foo': 'foo-value', 'bar': 1 }), + deletedDoc('collection/key', 1), + unknownDoc('collection/key', 1) + ]; + + const mutations: Mutation[] = [ + setMutation('collection/key', { 'bar': 'bar-value' }), + setMutation('collection/key', { 'bar.rab': 'bar.rab-value' }), + deleteMutation('collection/key'), + patchMutation('collection/key', { + 'foo': 'foo-patched-value-incr', + 'bar': increment(1) + }), + patchMutation('collection/key', { + 'foo': 'foo-patched-value-delete', + 'bar': deleteField() + }), + patchMutation('collection/key', { + 'foo': 'foo-patched-value-st', + 'bar': serverTimestamp() + }), + mergeMutation('collection/key', { 'arrays': arrayUnion(1, 2, 3) }, [ + field('arrays') + ]) + ]; + + // Take all possible combinations of the subsets of the mutation list, run each combination for + // all possible permutation, for all 3 different type of documents. + let testCases = 0; + computeCombinations(mutations).forEach(combination => { + testCases += runPermutationTests(docs, combination); + }); + + // There are (0! + 7*1! + 21*2! + 35*3! + 35*4! + 21*5! + 7*6! + 7!) * 3 = 41100 cases. + expect(testCases).to.equal(41100); + }); + + it('overlay by combinations and permutations for array transforms', () => { + const docs: MutableDocument[] = [ + doc('collection/key', 1, { 'foo': 'foo-value', 'bar.baz': 1 }), + deletedDoc('collection/key', 1), + unknownDoc('collection/key', 1) + ]; + + const mutations: Mutation[] = [ + setMutation('collection/key', { 'bar': 'bar-value' }), + mergeMutation( + 'collection/key', + { 'foo': 'xxx', 'arrays': arrayRemove(2) }, + [field('foo')] + ), + deleteMutation('collection/key'), + patchMutation('collection/key', { + 'foo': 'foo-patched-value-1', + 'arrays': arrayUnion(4, 5) + }), + patchMutation('collection/key', { + 'foo': 'foo-patched-value-2', + 'arrays': arrayRemove(5, 6) + }), + mergeMutation( + 'collection/key', + { 'foo': 'yyy', 'arrays': arrayUnion(1, 2, 3, 999) }, + [field('foo')] + ) + ]; + + let testCases = 0; + computeCombinations(mutations).forEach(combination => { + testCases += runPermutationTests(docs, combination); + }); + + // There are (0! + 6*1! + 15*2! + 20*3! + 15*4! + 6*5! + 6!) * 3 = 5871 cases. + expect(testCases).to.equal(5871); + }); + + it('overlay by combinations and permutations for increments', () => { + const docs: MutableDocument[] = [ + doc('collection/key', 1, { 'foo': 'foo-value', 'bar': 1 }), + deletedDoc('collection/key', 1), + unknownDoc('collection/key', 1) + ]; + + const mutations: Mutation[] = [ + setMutation('collection/key', { 'bar': 'bar-value' }), + mergeMutation( + 'collection/key', + { 'foo': 'foo-merge', 'bar': increment(2) }, + [field('foo')] + ), + deleteMutation('collection/key'), + patchMutation('collection/key', { + 'foo': 'foo-patched-value-1', + 'bar': increment(-1.4) + }), + patchMutation('collection/key', { + 'foo': 'foo-patched-value-2', + 'bar': increment(3.3) + }), + mergeMutation('collection/key', { 'foo': 'yyy', 'bar': increment(-41) }, [ + field('foo') + ]) + ]; + + let testCases = 0; + computeCombinations(mutations).forEach(combination => { + testCases += runPermutationTests(docs, combination); + }); + + // There are (0! + 6*1! + 15*2! + 20*3! + 15*4! + 6*5! + 6!) * 3 = 5871 cases. + expect(testCases).to.equal(5871); + }); }); diff --git a/packages/firestore/test/util/helpers.ts b/packages/firestore/test/util/helpers.ts index f7970ef872c..50de8e45f57 100644 --- a/packages/firestore/test/util/helpers.ts +++ b/packages/firestore/test/util/helpers.ts @@ -81,7 +81,8 @@ import { MutationResult, PatchMutation, Precondition, - SetMutation + SetMutation, + FieldTransform } from '../../src/model/mutation'; import { JsonObject, ObjectValue } from '../../src/model/object_value'; import { FieldPath, ResourcePath } from '../../src/model/path'; @@ -285,6 +286,23 @@ export function patchMutation( if (precondition === undefined) { precondition = Precondition.exists(true); } + return patchMutationHelper(keyStr, json, precondition, /* updateMask */ null); +} + +export function mergeMutation( + keyStr: string, + json: JsonObject, + updateMask: FieldPath[] +): PatchMutation { + return patchMutationHelper(keyStr, json, Precondition.none(), updateMask); +} + +export function patchMutationHelper( + keyStr: string, + json: JsonObject, + precondition: Precondition, + updateMask: FieldPath[] | null +): PatchMutation { // Replace '' from JSON with FieldValue forEach(json, (k, v) => { if (v === '') { @@ -298,12 +316,31 @@ export function patchMutation( patchKey, json ); + + // `mergeMutation()` provides an update mask for the merged fields, whereas + // `patchMutation()` requires the update mask to be parsed from the values. + const mask = updateMask !== null ? updateMask : parsed.fieldMask.fields; + + // We sort the fieldMaskPaths to make the order deterministic in tests. + // (Otherwise, when we flatten a Set to a proto repeated field, we'll end up + // comparing in iterator order and possibly consider {foo,bar} != {bar,foo}.) + let fieldMaskPaths = new SortedSet(FieldPath.comparator); + mask.forEach(value => (fieldMaskPaths = fieldMaskPaths.add(value))); + + // The order of the transforms doesn't matter, but we sort them so tests can + // assume a particular order. + const fieldTransforms: FieldTransform[] = []; + fieldTransforms.push(...parsed.fieldTransforms); + fieldTransforms.sort((lhs, rhs) => + FieldPath.comparator(lhs.field, rhs.field) + ); + return new PatchMutation( patchKey, parsed.data, - parsed.fieldMask, + new FieldMask(fieldMaskPaths.toArray()), precondition, - parsed.fieldTransforms + fieldTransforms ); } @@ -970,3 +1007,46 @@ export function forEachNumber( } } } + +/** Returns all possible permutations of the given array. */ +export function computePermutations(input: T[]): T[][] { + if (input.length === 0) { + return [[]]; + } + + const result: T[][] = []; + for (let i = 0; i < input.length; ++i) { + const rest = computePermutations( + input.slice(0, i).concat(input.slice(i + 1)) + ); + + if (rest.length === 0) { + result.push([input[i]]); + } else { + for (let j = 0; j < rest.length; ++j) { + result.push([input[i]].concat(rest[j])); + } + } + } + return result; +} + +/** + * Returns all possible combinations of the given array, including an empty + * array. + */ +export function computeCombinations(input: T[]): T[][] { + const computeNonEmptyCombinations = (input: T[]): T[][] => { + if (input.length === 1) { + return [input]; + } else { + const first = input[0]; + const rest = computeNonEmptyCombinations(input.slice(1)); + return rest.concat( + rest.map(e => e.concat(first)), + [[first]] + ); + } + }; + return computeNonEmptyCombinations(input).concat([[]]); +} From 5f887281bd9c23d1ab7737bcf5a31a3d4cf7054d Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Mon, 7 Mar 2022 16:30:47 -0600 Subject: [PATCH 2/9] we should call ObjectValue.delete if value is null. --- packages/firestore/src/model/mutation.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/packages/firestore/src/model/mutation.ts b/packages/firestore/src/model/mutation.ts index 9a24adb3502..979e94709df 100644 --- a/packages/firestore/src/model/mutation.ts +++ b/packages/firestore/src/model/mutation.ts @@ -242,7 +242,7 @@ export function calculateOverlayMutation( let maskSet = new SortedSet(FieldPath.comparator); mask.fields.forEach(path => { if (!maskSet.has(path)) { - const value = docValue.field(path); + let value = docValue.field(path); // If we are deleting a nested field, we take the immediate parent as // the mask used to construct the resulting mutation. // Justification: Nested fields can create parent fields implicitly. If @@ -254,8 +254,13 @@ export function calculateOverlayMutation( // mutation would miss `foo`. if (value === null && path.length > 1) { path = path.popLast(); + value = docValue.field(path); + } + if (value === null) { + patchValue.delete(path); + } else { + patchValue.set(path, value); } - patchValue.set(path, docValue.field(path)!); maskSet = maskSet.add(path); } }); From eeb2243a8ad9d1dccb9c5679f6e628a422197bca Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 10 Mar 2022 11:31:41 -0500 Subject: [PATCH 3/9] Remove unnecessary null check from MemoryDocumentOverlayCache.saveOverlay(), like is done in https://github.com/firebase/firebase-android-sdk/pull/3518 --- packages/firestore/src/local/memory_document_overlay_cache.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/packages/firestore/src/local/memory_document_overlay_cache.ts b/packages/firestore/src/local/memory_document_overlay_cache.ts index adfa45adbbc..4227ecc214d 100644 --- a/packages/firestore/src/local/memory_document_overlay_cache.ts +++ b/packages/firestore/src/local/memory_document_overlay_cache.ts @@ -152,10 +152,6 @@ export class MemoryDocumentOverlayCache implements DocumentOverlayCache { largestBatchId: number, mutation: Mutation ): void { - if (mutation === null) { - return; - } - // Remove the association of the overlay to its batch id. const existing = this.overlays.get(mutation.key); if (existing !== null) { From ec8fe086edb9e3f047039745a2b5ed1c045c536b Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Thu, 10 Mar 2022 15:27:16 -0600 Subject: [PATCH 4/9] Address comments. --- .../src/local/local_documents_view.ts | 42 ++++++++----------- .../firestore/src/local/local_store_impl.ts | 20 ++++----- packages/firestore/src/model/mutation.ts | 24 +++++------ .../firestore/src/model/mutation_batch.ts | 14 +------ packages/firestore/test/util/helpers.ts | 12 ++++-- 5 files changed, 45 insertions(+), 67 deletions(-) diff --git a/packages/firestore/src/local/local_documents_view.ts b/packages/firestore/src/local/local_documents_view.ts index 3014efbd296..4ed8fe90f84 100644 --- a/packages/firestore/src/local/local_documents_view.ts +++ b/packages/firestore/src/local/local_documents_view.ts @@ -30,7 +30,10 @@ import { MutableDocumentMap, newDocumentKeyMap, newMutationMap, - newOverlayMap + newOverlayMap, + documentMap, + mutableDocumentMap, + documentKeySet } from '../model/collections'; import { Document, MutableDocument } from '../model/document'; import { DocumentKey } from '../model/document_key'; @@ -45,7 +48,6 @@ import { Overlay } from '../model/overlay'; import { ResourcePath } from '../model/path'; import { debugAssert } from '../util/assert'; import { SortedMap } from '../util/sorted_map'; -import { SortedSet } from '../util/sorted_set'; import { DocumentOverlayCache } from './document_overlay_cache'; import { IndexManager } from './index_manager'; @@ -111,11 +113,9 @@ export class LocalDocumentsView { return this.remoteDocumentCache .getEntries(transaction, keys) .next(docs => - this.getLocalViewOfDocuments( - transaction, - docs, - new SortedSet(DocumentKey.comparator) - ).next(() => docs as DocumentMap) + this.getLocalViewOfDocuments(transaction, docs, documentKeySet()).next( + () => docs as DocumentMap + ) ); } @@ -152,10 +152,8 @@ export class LocalDocumentsView { memoizedOverlays: OverlayMap, existenceStateChanged: DocumentKeySet ): PersistencePromise { - let results = new SortedMap(DocumentKey.comparator); - let recalculateDocuments = new SortedMap( - DocumentKey.comparator - ); + let results = documentMap(); + let recalculateDocuments = mutableDocumentMap(); const promises: Array> = []; docs.forEach((_, doc) => { const overlayPromise = memoizedOverlays.has(doc.key) @@ -209,21 +207,21 @@ export class LocalDocumentsView { let documentsByBatchId = new SortedMap( (key1: number, key2: number) => key1 - key2 ); - let processed = new SortedSet(DocumentKey.comparator); + let processed = documentKeySet(); return this.mutationQueue .getAllMutationBatchesAffectingDocumentKeys(transaction, docs) .next(batches => { - batches.forEach(batch => { + for (const batch of batches) { batch.keys().forEach(key => { let mask: FieldMask | null = masks.has(key) ? masks.get(key)! : FieldMask.empty(); - mask = batch.applyToLocalViewWithFieldMask(docs.get(key)!, mask); + mask = batch.applyToLocalView(docs.get(key)!, mask); masks.set(key, mask); if (documentsByBatchId.get(batch.batchId) === null) { documentsByBatchId = documentsByBatchId.insert( batch.batchId, - new SortedSet(DocumentKey.comparator) + documentKeySet() ); } const newSet = documentsByBatchId.get(batch.batchId)!.add(key); @@ -232,7 +230,7 @@ export class LocalDocumentsView { newSet ); }); - }); + } }) .next(() => { const promises: Array> = []; @@ -246,8 +244,6 @@ export class LocalDocumentsView { const overlays = newMutationMap(); keys.forEach(key => { if (!processed.has(key)) { - // TODO: Should we change `overlays` type to Map - // and update `saveOverlays` to accept (and skip) null values? const overlayMutation = calculateOverlayMutation( docs.get(key)!, masks.get(key)! @@ -322,9 +318,7 @@ export class LocalDocumentsView { // Just do a simple document lookup. return this.getDocument(transaction, new DocumentKey(docPath)).next( document => { - let result = new SortedMap( - DocumentKey.comparator - ); + let result = documentMap(); if (document.isFoundDocument()) { result = result.insert(document.key, document); } @@ -343,7 +337,7 @@ export class LocalDocumentsView { 'Currently we only support collection group queries at the root.' ); const collectionId = query.collectionGroup!; - let results = new SortedMap(DocumentKey.comparator); + let results = documentMap(); return this.indexManager .getCollectionParents(transaction, collectionId) .next(parents => { @@ -398,9 +392,7 @@ export class LocalDocumentsView { }); // Apply the overlays and match against the query. - let results = new SortedMap( - DocumentKey.comparator - ); + let results = documentMap(); remoteDocuments.forEach((key, document) => { const overlay = overlays.get(key); if (overlay !== undefined) { diff --git a/packages/firestore/src/local/local_store_impl.ts b/packages/firestore/src/local/local_store_impl.ts index 4b1eb5e49b9..76fdb9d22f7 100644 --- a/packages/firestore/src/local/local_store_impl.ts +++ b/packages/firestore/src/local/local_store_impl.ts @@ -218,11 +218,9 @@ class LocalStoreImpl implements LocalStore { } } -class DocumentChangeResult { - constructor( - readonly changedDocuments: MutableDocumentMap, - readonly existenceChangedKeys: DocumentKeySet - ) {} +interface DocumentChangeResult { + changedDocuments: MutableDocumentMap; + existenceChangedKeys: DocumentKeySet; } export function newLocalStore( @@ -666,17 +664,10 @@ export function localStoreApplyRemoteEventToLocalCache( * Returns the document changes resulting from applying those documents, and * also a set of documents whose existence state are changed as a result. * - * Note: this function will use `documentVersions` if it is defined; - * when it is not defined, resorts to `globalVersion`. - * * @param txn - Transaction to use to read existing documents from storage. * @param documentBuffer - Document buffer to collect the resulted changes to be * applied to storage. * @param documents - Documents to be applied. - * @param globalVersion - A `SnapshotVersion` representing the read time if all - * documents have the same read time. - * @param documentVersions - A DocumentKey-to-SnapshotVersion map if documents - * have their own read time. */ function populateDocumentChangeBuffer( txn: PersistenceTransaction, @@ -730,7 +721,10 @@ function populateDocumentChangeBuffer( ); } }); - return new DocumentChangeResult(changedDocs, conditionChanged); + return { + changedDocuments: changedDocs, + existenceChangedKeys: conditionChanged + }; }); } diff --git a/packages/firestore/src/model/mutation.ts b/packages/firestore/src/model/mutation.ts index 979e94709df..67da809e4a6 100644 --- a/packages/firestore/src/model/mutation.ts +++ b/packages/firestore/src/model/mutation.ts @@ -224,12 +224,11 @@ export function calculateOverlayMutation( doc: MutableDocument, mask: FieldMask | null ): Mutation | null { - if (!doc.hasLocalMutations || (mask !== null && mask!.fields.length === 0)) { + if (!doc.hasLocalMutations || (mask && mask!.fields.length === 0)) { return null; } - // mask is null when there are Set or Delete being applied to get to the - // current document. + // mask is null when sets or deletes are applied to the current document. if (mask === null) { if (doc.isNoDocument()) { return new DeleteMutation(doc.key, Precondition.none()); @@ -240,7 +239,7 @@ export function calculateOverlayMutation( const docValue = doc.data; const patchValue = ObjectValue.empty(); let maskSet = new SortedSet(FieldPath.comparator); - mask.fields.forEach(path => { + for (let path of mask.fields) { if (!maskSet.has(path)) { let value = docValue.field(path); // If we are deleting a nested field, we take the immediate parent as @@ -263,7 +262,7 @@ export function calculateOverlayMutation( } maskSet = maskSet.add(path); } - }); + } return new PatchMutation( doc.key, patchValue, @@ -489,8 +488,7 @@ function setMutationApplyToLocalView( document .convertToFoundDocument(document.version, newData) .setHasLocalMutations(); - // SetMutation overwrites all fields. - return null; + return null; // SetMutation overwrites all fields. } /** @@ -574,12 +572,12 @@ function patchMutationApplyToLocalView( } let mergedMaskSet = new SortedSet(FieldPath.comparator); - previousMask.fields.forEach( - fieldPath => (mergedMaskSet = mergedMaskSet.add(fieldPath)) - ); - mutation.fieldMask.fields.forEach( - fieldPath => (mergedMaskSet = mergedMaskSet.add(fieldPath)) - ); + for (const fieldPath of previousMask.fields) { + mergedMaskSet = mergedMaskSet.add(fieldPath); + } + for (const fieldPath of mutation.fieldMask.fields) { + mergedMaskSet = mergedMaskSet.add(fieldPath); + } mutation.fieldTransforms .map(transform => transform.field) .forEach(fieldPath => (mergedMaskSet = mergedMaskSet.add(fieldPath))); diff --git a/packages/firestore/src/model/mutation_batch.ts b/packages/firestore/src/model/mutation_batch.ts index 6ba1fdc09ac..3070ef0baf9 100644 --- a/packages/firestore/src/model/mutation_batch.ts +++ b/packages/firestore/src/model/mutation_batch.ts @@ -93,16 +93,6 @@ export class MutationBatch { } } } - /** - * Computes the local view of a document given all the mutations in this - * batch. - * - * @param document - The document to apply mutations to. - * @returns A `FieldMask` representing all the fields that are mutated. - */ - applyToLocalView(document: MutableDocument): FieldMask | null { - return this.applyToLocalViewWithFieldMask(document, FieldMask.empty()); - } /** * Computes the local view of a document given all the mutations in this @@ -112,9 +102,9 @@ export class MutationBatch { * @param mutatedFields - Fields that have been updated before applying this mutation batch. * @returns A `FieldMask` representing all the fields that are mutated. */ - applyToLocalViewWithFieldMask( + applyToLocalView( document: MutableDocument, - mutatedFields: FieldMask | null + mutatedFields: FieldMask | null = FieldMask.empty() ): FieldMask | null { // First, apply the base state. This allows us to apply non-idempotent // transform against a consistent set of values. diff --git a/packages/firestore/test/util/helpers.ts b/packages/firestore/test/util/helpers.ts index 50de8e45f57..c7f0504f5e6 100644 --- a/packages/firestore/test/util/helpers.ts +++ b/packages/firestore/test/util/helpers.ts @@ -297,7 +297,7 @@ export function mergeMutation( return patchMutationHelper(keyStr, json, Precondition.none(), updateMask); } -export function patchMutationHelper( +function patchMutationHelper( keyStr: string, json: JsonObject, precondition: Precondition, @@ -319,7 +319,7 @@ export function patchMutationHelper( // `mergeMutation()` provides an update mask for the merged fields, whereas // `patchMutation()` requires the update mask to be parsed from the values. - const mask = updateMask !== null ? updateMask : parsed.fieldMask.fields; + const mask = updateMask ? updateMask : parsed.fieldMask.fields; // We sort the fieldMaskPaths to make the order deterministic in tests. // (Otherwise, when we flatten a Set to a proto repeated field, we'll end up @@ -1008,7 +1008,10 @@ export function forEachNumber( } } -/** Returns all possible permutations of the given array. */ +/** + * Returns all possible permutations of the given array. + * For `[a, b]`, this method returns `[[a, b], [b, a]]`. + */ export function computePermutations(input: T[]): T[][] { if (input.length === 0) { return [[]]; @@ -1033,7 +1036,8 @@ export function computePermutations(input: T[]): T[][] { /** * Returns all possible combinations of the given array, including an empty - * array. + * array. For `[a, b, c]` this method returns + * `[[], [a], [a, b], [a, c], [b, c], [a, b, c]`. */ export function computeCombinations(input: T[]): T[][] { const computeNonEmptyCombinations = (input: T[]): T[][] => { From 1e68233044a7c79cc80c59cd95b1ca71cf2a79b1 Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Tue, 15 Mar 2022 15:38:26 -0500 Subject: [PATCH 5/9] Port changes from Android SDK PR#3420. Note that we are not going to do any processing in the background. --- .../src/local/document_overlay_cache.ts | 9 ++ .../local/indexeddb_document_overlay_cache.ts | 18 +++ .../src/local/local_documents_view.ts | 103 ++++++++++-------- .../local/memory_document_overlay_cache.ts | 18 +++ .../unit/local/document_overlay_cache.test.ts | 25 +++++ .../unit/local/test_document_overlay_cache.ts | 6 + 6 files changed, 134 insertions(+), 45 deletions(-) diff --git a/packages/firestore/src/local/document_overlay_cache.ts b/packages/firestore/src/local/document_overlay_cache.ts index b0d011a7e38..d30e125049a 100644 --- a/packages/firestore/src/local/document_overlay_cache.ts +++ b/packages/firestore/src/local/document_overlay_cache.ts @@ -43,6 +43,15 @@ export interface DocumentOverlayCache { key: DocumentKey ): PersistencePromise; + /** + * Gets the saved overlay mutation for the given document keys. Skips keys for + * which there are no overlays. + */ + getOverlays( + transaction: PersistenceTransaction, + keys: DocumentKeySet + ): PersistencePromise; + /** * Saves the given document mutation map to persistence as overlays. * All overlays will have their largest batch id set to `largestBatchId`. diff --git a/packages/firestore/src/local/indexeddb_document_overlay_cache.ts b/packages/firestore/src/local/indexeddb_document_overlay_cache.ts index 9821530c8a8..0bea0f9bf84 100644 --- a/packages/firestore/src/local/indexeddb_document_overlay_cache.ts +++ b/packages/firestore/src/local/indexeddb_document_overlay_cache.ts @@ -81,6 +81,24 @@ export class IndexedDbDocumentOverlayCache implements DocumentOverlayCache { }); } + getOverlays( + transaction: PersistenceTransaction, + keys: DocumentKeySet + ): PersistencePromise { + const result = newOverlayMap(); + const promises: Array> = []; + keys.forEach(key => { + promises.push( + this.getOverlay(transaction, key).next(overlay => { + if (overlay !== null) { + result.set(key, overlay); + } + }) + ); + }); + return PersistencePromise.waitFor(promises).next(() => result); + } + saveOverlays( transaction: PersistenceTransaction, largestBatchId: number, diff --git a/packages/firestore/src/local/local_documents_view.ts b/packages/firestore/src/local/local_documents_view.ts index 4ed8fe90f84..17113f5c7a4 100644 --- a/packages/firestore/src/local/local_documents_view.ts +++ b/packages/firestore/src/local/local_documents_view.ts @@ -134,12 +134,39 @@ export class LocalDocumentsView { docs: MutableDocumentMap, existenceStateChanged: DocumentKeySet ): PersistencePromise { - return this.computeViews( - transaction, - docs, - newOverlayMap(), - existenceStateChanged - ); + const overlays = newOverlayMap(); + return this.populateOverlays(transaction, overlays, docs).next(() => { + return this.computeViews( + transaction, + docs, + overlays, + existenceStateChanged + ); + }); + } + + /** + * Fetches the overlays for {@code docs} and adds them to provided overlay map + * if the map does not already contain an entry for the given document key. + */ + private populateOverlays( + transaction: PersistenceTransaction, + overlays: OverlayMap, + docs: MutableDocumentMap + ): PersistencePromise { + let missingOverlays = documentKeySet(); + docs.forEach(key => { + if (!overlays.has(key)) { + missingOverlays = missingOverlays.add(key); + } + }); + return this.documentOverlayCache + .getOverlays(transaction, missingOverlays) + .next(result => { + result.forEach((key, val) => { + overlays.set(key, val); + }); + }); } /** @@ -149,53 +176,39 @@ export class LocalDocumentsView { computeViews( transaction: PersistenceTransaction, docs: MutableDocumentMap, - memoizedOverlays: OverlayMap, + overlays: OverlayMap, existenceStateChanged: DocumentKeySet ): PersistencePromise { let results = documentMap(); let recalculateDocuments = mutableDocumentMap(); - const promises: Array> = []; docs.forEach((_, doc) => { - const overlayPromise = memoizedOverlays.has(doc.key) - ? PersistencePromise.resolve(memoizedOverlays.get(doc.key)!) - : this.documentOverlayCache.getOverlay(transaction, doc.key); - - promises.push( - overlayPromise.next((overlay: Overlay | null) => { - // Recalculate an overlay if the document's existence state is changed - // due to a remote event *and* the overlay is a PatchMutation. This is - // because document existence state can change if some patch mutation's - // preconditions are met. - // NOTE: we recalculate when `overlay` is null as well, because there - // might be a patch mutation whose precondition does not match before - // the change (hence overlay==null), but would now match. - if ( - existenceStateChanged.has(doc.key) && - (overlay === null || overlay.mutation instanceof PatchMutation) - ) { - recalculateDocuments = recalculateDocuments.insert(doc.key, doc); - } else if (overlay !== null) { - mutationApplyToLocalView( - overlay.mutation, - doc, - null, - Timestamp.now() - ); - } - }) - ); + const overlay = overlays.get(doc.key); + // Recalculate an overlay if the document's existence state is changed due + // to a remote event *and* the overlay is a PatchMutation. This is because + // document existence state can change if some patch mutation's + // preconditions are met. + // NOTE: we recalculate when `overlay` is undefined as well, because there + // might be a patch mutation whose precondition does not match before the + // change (hence overlay is undefined), but would now match. + if ( + existenceStateChanged.has(doc.key) && + (overlay === undefined || overlay.mutation instanceof PatchMutation) + ) { + recalculateDocuments = recalculateDocuments.insert(doc.key, doc); + } else if (overlay !== undefined) { + mutationApplyToLocalView(overlay.mutation, doc, null, Timestamp.now()); + } }); - return PersistencePromise.waitFor(promises) - .next(() => - this.recalculateAndSaveOverlays(transaction, recalculateDocuments) - ) - .next(() => { - docs.forEach((key, value) => { - results = results.insert(key, value); - }); - return results; + return this.recalculateAndSaveOverlays( + transaction, + recalculateDocuments + ).next(() => { + docs.forEach((key, value) => { + results = results.insert(key, value); }); + return results; + }); } private recalculateAndSaveOverlays( diff --git a/packages/firestore/src/local/memory_document_overlay_cache.ts b/packages/firestore/src/local/memory_document_overlay_cache.ts index 4227ecc214d..9e7983b2a28 100644 --- a/packages/firestore/src/local/memory_document_overlay_cache.ts +++ b/packages/firestore/src/local/memory_document_overlay_cache.ts @@ -50,6 +50,24 @@ export class MemoryDocumentOverlayCache implements DocumentOverlayCache { return PersistencePromise.resolve(this.overlays.get(key)); } + getOverlays( + transaction: PersistenceTransaction, + keys: DocumentKeySet + ): PersistencePromise { + const result = newOverlayMap(); + const promises: Array> = []; + keys.forEach(key => { + promises.push( + this.getOverlay(transaction, key).next(overlay => { + if (overlay !== null) { + result.set(key, overlay); + } + }) + ); + }); + return PersistencePromise.waitFor(promises).next(() => result); + } + saveOverlays( transaction: PersistenceTransaction, largestBatchId: number, diff --git a/packages/firestore/test/unit/local/document_overlay_cache.test.ts b/packages/firestore/test/unit/local/document_overlay_cache.test.ts index 614cc5c9960..d332a43456b 100644 --- a/packages/firestore/test/unit/local/document_overlay_cache.test.ts +++ b/packages/firestore/test/unit/local/document_overlay_cache.test.ts @@ -327,4 +327,29 @@ function genericDocumentOverlayCacheTests(): void { ); expect(await overlayCache.getOverlay(key('coll/doc'))).to.equal(null); }); + + it('skips non-existing overlay in batch lookup', async () => { + const result = await overlayCache.getOverlays( + documentKeySet(key('coll/doc1')) + ); + expect(result.isEmpty()).to.equal(true); + }); + + it('supports empty batch in batch lookup', async () => { + const result = await overlayCache.getOverlays(documentKeySet()); + expect(result.isEmpty()).to.equal(true); + }); + + it('can read saved overlays in batches', async () => { + const m1 = setMutation('coll/a', { 'a': 1 }); + const m2 = setMutation('coll/b', { 'b': 2 }); + const m3 = setMutation('coll/c', { 'c': 3 }); + await saveOverlaysForMutations(3, m1, m2, m3); + const overlays = await overlayCache.getOverlays( + documentKeySet(key('coll/a'), key('coll/b'), key('coll/c')) + ); + verifyEqualMutations(overlays.get(key('coll/a'))!.mutation, m1); + verifyEqualMutations(overlays.get(key('coll/b'))!.mutation, m2); + verifyEqualMutations(overlays.get(key('coll/c'))!.mutation, m3); + }); } diff --git a/packages/firestore/test/unit/local/test_document_overlay_cache.ts b/packages/firestore/test/unit/local/test_document_overlay_cache.ts index df982b0dbad..9c5c214651b 100644 --- a/packages/firestore/test/unit/local/test_document_overlay_cache.ts +++ b/packages/firestore/test/unit/local/test_document_overlay_cache.ts @@ -50,6 +50,12 @@ export class TestDocumentOverlayCache { }); } + getOverlays(keys: DocumentKeySet): Promise { + return this.persistence.runTransaction('getOverlays', 'readonly', txn => { + return this.cache.getOverlays(txn, keys); + }); + } + getOverlayMutation(docKey: string): Promise { return this.getOverlay(key(docKey)).then(value => { if (!value) { From fcf8bdb45624eb6208ebebcac597c71941f8bb13 Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Tue, 15 Mar 2022 23:06:55 -0500 Subject: [PATCH 6/9] Port overlay recalculation bug (Android SDK PR #3495). --- .../src/local/local_documents_view.ts | 27 ++++++++++--------- .../test/unit/local/local_store.test.ts | 19 +++++++++++++ 2 files changed, 34 insertions(+), 12 deletions(-) diff --git a/packages/firestore/src/local/local_documents_view.ts b/packages/firestore/src/local/local_documents_view.ts index 17113f5c7a4..08bb3f4d387 100644 --- a/packages/firestore/src/local/local_documents_view.ts +++ b/packages/firestore/src/local/local_documents_view.ts @@ -226,22 +226,25 @@ export class LocalDocumentsView { .next(batches => { for (const batch of batches) { batch.keys().forEach(key => { - let mask: FieldMask | null = masks.has(key) - ? masks.get(key)! - : FieldMask.empty(); - mask = batch.applyToLocalView(docs.get(key)!, mask); - masks.set(key, mask); - if (documentsByBatchId.get(batch.batchId) === null) { + const baseDoc = docs.get(key); + if (baseDoc !== null) { + let mask: FieldMask | null = masks.has(key) + ? masks.get(key)! + : FieldMask.empty(); + mask = batch.applyToLocalView(baseDoc, mask); + masks.set(key, mask); + if (documentsByBatchId.get(batch.batchId) === null) { + documentsByBatchId = documentsByBatchId.insert( + batch.batchId, + documentKeySet() + ); + } + const newSet = documentsByBatchId.get(batch.batchId)!.add(key); documentsByBatchId = documentsByBatchId.insert( batch.batchId, - documentKeySet() + newSet ); } - const newSet = documentsByBatchId.get(batch.batchId)!.add(key); - documentsByBatchId = documentsByBatchId.insert( - batch.batchId, - newSet - ); }); } }) diff --git a/packages/firestore/test/unit/local/local_store.test.ts b/packages/firestore/test/unit/local/local_store.test.ts index 83912f7692a..8465af26ac1 100644 --- a/packages/firestore/test/unit/local/local_store.test.ts +++ b/packages/firestore/test/unit/local/local_store.test.ts @@ -1801,6 +1801,25 @@ function genericLocalStoreTests( ); }); + it('can handle batch Ack when pending batches have other docs', () => { + // Prepare two batches, the first one will get rejected by the backend. + // When the first batch is rejected, overlay is recalculated with only the + // second batch, even though it has more documents than what is being + // rejected. + return expectLocalStore() + .afterMutations([patchMutation('foo/bar', { 'foo': 'bar' })]) + .afterMutations([ + setMutation('foo/bar', { 'foo': 'bar-set' }), + setMutation('foo/another', { 'foo': 'another' }) + ]) + .afterRejectingMutation() + .toContain(doc('foo/bar', 0, { 'foo': 'bar-set' }).setHasLocalMutations()) + .toContain( + doc('foo/another', 0, { 'foo': 'another' }).setHasLocalMutations() + ) + .finish(); + }); + it('uses target mapping to execute queries', () => { if (gcIsEager) { return; From 2fc215e7a6a2cc7784a255c51779e014c8d76da0 Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Fri, 18 Mar 2022 16:09:20 -0500 Subject: [PATCH 7/9] Fix overlay bug when offline (Port Android SDK PR #3537). --- .../src/local/local_documents_view.ts | 2 +- .../firestore/src/local/local_store_impl.ts | 36 +++++++++++++++---- .../firestore/src/model/mutation_batch.ts | 13 +++++-- .../test/unit/local/local_store.test.ts | 23 +++++++++++- 4 files changed, 64 insertions(+), 10 deletions(-) diff --git a/packages/firestore/src/local/local_documents_view.ts b/packages/firestore/src/local/local_documents_view.ts index 08bb3f4d387..8ca6082b526 100644 --- a/packages/firestore/src/local/local_documents_view.ts +++ b/packages/firestore/src/local/local_documents_view.ts @@ -132,7 +132,7 @@ export class LocalDocumentsView { getLocalViewOfDocuments( transaction: PersistenceTransaction, docs: MutableDocumentMap, - existenceStateChanged: DocumentKeySet + existenceStateChanged: DocumentKeySet = documentKeySet() ): PersistencePromise { const overlays = newOverlayMap(); return this.populateOverlays(transaction, overlays, docs).next(() => { diff --git a/packages/firestore/src/local/local_store_impl.ts b/packages/firestore/src/local/local_store_impl.ts index 76fdb9d22f7..3ba5dd43b11 100644 --- a/packages/firestore/src/local/local_store_impl.ts +++ b/packages/firestore/src/local/local_store_impl.ts @@ -314,11 +314,32 @@ export function localStoreWriteLocally( return localStoreImpl.persistence .runTransaction('Locally write mutations', 'readwrite', txn => { - // Load and apply all existing mutations. This lets us compute the - // current base state for all non-idempotent transforms before applying - // any additional user-provided writes. - return localStoreImpl.localDocuments - .getDocuments(txn, keys) + // Figure out which keys do not have a remote version in the cache, this + // is needed to create the right overlay mutation: if no remote version + // presents, we do not need to create overlays as patch mutations. + // TODO(Overlay): Is there a better way to determine this? Document + // version does not work because local mutations set them back to 0. + let remoteDocs = mutableDocumentMap(); + let docsWithoutRemoteVersion = documentKeySet(); + return localStoreImpl.remoteDocuments + .getEntries(txn, keys) + .next(docs => { + remoteDocs = docs; + remoteDocs.forEach((key, doc) => { + if (!doc.isValidDocument()) { + docsWithoutRemoteVersion = docsWithoutRemoteVersion.add(key); + } + }); + }) + .next(() => { + // Load and apply all existing mutations. This lets us compute the + // current base state for all non-idempotent transforms before applying + // any additional user-provided writes. + return localStoreImpl.localDocuments.getLocalViewOfDocuments( + txn, + remoteDocs + ); + }) .next(docs => { existingDocs = docs; @@ -358,7 +379,10 @@ export function localStoreWriteLocally( }) .next(batch => { mutationBatch = batch; - const overlays = batch.applyToLocalDocumentSet(existingDocs); + const overlays = batch.applyToLocalDocumentSet( + existingDocs, + docsWithoutRemoteVersion + ); return localStoreImpl.documentOverlayCache.saveOverlays( txn, batch.batchId, diff --git a/packages/firestore/src/model/mutation_batch.ts b/packages/firestore/src/model/mutation_batch.ts index 3070ef0baf9..5b8c2c18a28 100644 --- a/packages/firestore/src/model/mutation_batch.ts +++ b/packages/firestore/src/model/mutation_batch.ts @@ -138,7 +138,10 @@ export class MutationBatch { * this batch. Returns a `DocumentKey` to `Mutation` map which can be used to * replace all the mutation applications. */ - applyToLocalDocumentSet(documentMap: DocumentMap): MutationMap { + applyToLocalDocumentSet( + documentMap: DocumentMap, + documentsWithoutRemoteVersion: DocumentKeySet + ): MutationMap { // TODO(mrschmidt): This implementation is O(n^2). If we apply the mutations // directly (as done in `applyToLocalView()`), we can reduce the complexity // to O(n). @@ -148,7 +151,13 @@ export class MutationBatch { // TODO(mutabledocuments): This method should take a MutableDocumentMap // and we should remove this cast. const mutableDocument = document as MutableDocument; - const mutatedFields = this.applyToLocalView(mutableDocument); + let mutatedFields = this.applyToLocalView(mutableDocument); + // Set mutatedFields to null if the document is only from local mutations. + // This creates a Set or Delete mutation, instead of trying to create a + // patch mutation as the overlay. + mutatedFields = documentsWithoutRemoteVersion.has(m.key) + ? null + : mutatedFields; const overlay = calculateOverlayMutation(mutableDocument, mutatedFields); if (overlay !== null) { overlays.set(m.key, overlay); diff --git a/packages/firestore/test/unit/local/local_store.test.ts b/packages/firestore/test/unit/local/local_store.test.ts index 8465af26ac1..565b064653d 100644 --- a/packages/firestore/test/unit/local/local_store.test.ts +++ b/packages/firestore/test/unit/local/local_store.test.ts @@ -949,7 +949,7 @@ function genericLocalStoreTests( .toContain(deletedDoc('foo/bar', 0).setHasLocalMutations()) .afterAcknowledgingMutation({ documentVersion: 2 }) // delete mutation .toReturnRemoved('foo/bar') - .toContain(deletedDoc('foo/bar', 2).setHasCommittedMutations()) + .toContain(deletedDoc('foo/bar', 2).setHasLocalMutations()) .afterAcknowledgingMutation({ documentVersion: 3 }) // patch mutation .toReturnChanged(unknownDoc('foo/bar', 3)) .toNotContainIfEager(unknownDoc('foo/bar', 3)) @@ -1656,6 +1656,27 @@ function genericLocalStoreTests( .finish(); }); + it('add then update while offline', () => { + return expectLocalStore() + .afterMutations([ + setMutation('foo/bar', { 'foo': 'foo-value', 'bar': 1 }) + ]) + .toContain( + doc('foo/bar', 0, { + 'foo': 'foo-value', + 'bar': 1 + }).setHasLocalMutations() + ) + .afterMutations([patchMutation('foo/bar', { 'bar': 2 })]) + .toContain( + doc('foo/bar', 0, { + 'foo': 'foo-value', + 'bar': 2 + }).setHasLocalMutations() + ) + .finish(); + }); + it('handles saving and loading named queries', async () => { return expectLocalStore() .after( From 0fc5c8828e525b3c595716ebdd846dbe115f5985 Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Fri, 1 Apr 2022 17:15:30 -0500 Subject: [PATCH 8/9] Address feedback. --- .../src/local/local_documents_view.ts | 37 ++++++++----------- .../firestore/src/local/local_store_impl.ts | 20 +++++----- packages/firestore/src/model/field_mask.ts | 16 ++++++++ packages/firestore/src/model/mutation.ts | 14 ++----- .../test/unit/local/query_engine.test.ts | 22 +---------- .../test/unit/model/mutation.test.ts | 31 +++++++++------- 6 files changed, 62 insertions(+), 78 deletions(-) diff --git a/packages/firestore/src/local/local_documents_view.ts b/packages/firestore/src/local/local_documents_view.ts index 8af8bc41992..0f54c35adc7 100644 --- a/packages/firestore/src/local/local_documents_view.ts +++ b/packages/firestore/src/local/local_documents_view.ts @@ -183,8 +183,8 @@ export class LocalDocumentsView { let recalculateDocuments = mutableDocumentMap(); docs.forEach((_, doc) => { const overlay = overlays.get(doc.key); - // Recalculate an overlay if the document's existence state is changed due - // to a remote event *and* the overlay is a PatchMutation. This is because + // Recalculate an overlay if the document's existence state changed due to + // a remote event *and* the overlay is a PatchMutation. This is because // document existence state can change if some patch mutation's // preconditions are met. // NOTE: we recalculate when `overlay` is undefined as well, because there @@ -227,24 +227,19 @@ export class LocalDocumentsView { for (const batch of batches) { batch.keys().forEach(key => { const baseDoc = docs.get(key); - if (baseDoc !== null) { - let mask: FieldMask | null = masks.has(key) - ? masks.get(key)! - : FieldMask.empty(); - mask = batch.applyToLocalView(baseDoc, mask); - masks.set(key, mask); - if (documentsByBatchId.get(batch.batchId) === null) { - documentsByBatchId = documentsByBatchId.insert( - batch.batchId, - documentKeySet() - ); - } - const newSet = documentsByBatchId.get(batch.batchId)!.add(key); - documentsByBatchId = documentsByBatchId.insert( - batch.batchId, - newSet - ); + if (baseDoc === null) { + return; } + let mask: FieldMask | null = masks.get(key) ?? FieldMask.empty(); + mask = batch.applyToLocalView(baseDoc, mask); + masks.set(key, mask); + const newSet = ( + documentsByBatchId.get(batch.batchId) ?? documentKeySet() + ).add(key); + documentsByBatchId = documentsByBatchId.insert( + batch.batchId, + newSet + ); }); } }) @@ -292,9 +287,7 @@ export class LocalDocumentsView { ): PersistencePromise { return this.remoteDocumentCache .getEntries(transaction, documentKeys) - .next(docs => { - return this.recalculateAndSaveOverlays(transaction, docs); - }); + .next(docs => this.recalculateAndSaveOverlays(transaction, docs)); } /** diff --git a/packages/firestore/src/local/local_store_impl.ts b/packages/firestore/src/local/local_store_impl.ts index de4484f4ae0..36b967838eb 100644 --- a/packages/firestore/src/local/local_store_impl.ts +++ b/packages/firestore/src/local/local_store_impl.ts @@ -323,8 +323,9 @@ export function localStoreWriteLocally( // Figure out which keys do not have a remote version in the cache, this // is needed to create the right overlay mutation: if no remote version // presents, we do not need to create overlays as patch mutations. - // TODO(Overlay): Is there a better way to determine this? Document - // version does not work because local mutations set them back to 0. + // TODO(Overlay): Is there a better way to determine this? Using the + // document version does not work because local mutations set them back + // to 0. let remoteDocs = mutableDocumentMap(); let docsWithoutRemoteVersion = documentKeySet(); return localStoreImpl.remoteDocuments @@ -705,16 +706,16 @@ function populateDocumentChangeBuffer( documents: MutableDocumentMap ): PersistencePromise { let updatedKeys = documentKeySet(); - let conditionChanged = documentKeySet(); + let existenceChangedKeys = documentKeySet(); documents.forEach(k => (updatedKeys = updatedKeys.add(k))); return documentBuffer.getEntries(txn, updatedKeys).next(existingDocs => { - let changedDocs = mutableDocumentMap(); + let changedDocuments = mutableDocumentMap(); documents.forEach((key, doc) => { const existingDoc = existingDocs.get(key)!; // Check if see if there is a existence state change for this document. if (doc.isFoundDocument() !== existingDoc.isFoundDocument()) { - conditionChanged = conditionChanged.add(key); + existenceChangedKeys = existenceChangedKeys.add(key); } // Note: The order of the steps below is important, since we want @@ -726,7 +727,7 @@ function populateDocumentChangeBuffer( // events. We remove these documents from cache since we lost // access. documentBuffer.removeEntry(key, doc.readTime); - changedDocs = changedDocs.insert(key, doc); + changedDocuments = changedDocuments.insert(key, doc); } else if ( !existingDoc.isValidDocument() || doc.version.compareTo(existingDoc.version) > 0 || @@ -738,7 +739,7 @@ function populateDocumentChangeBuffer( 'Cannot add a document when the remote version is zero' ); documentBuffer.addEntry(doc); - changedDocs = changedDocs.insert(key, doc); + changedDocuments = changedDocuments.insert(key, doc); } else { logDebug( LOG_TAG, @@ -751,10 +752,7 @@ function populateDocumentChangeBuffer( ); } }); - return { - changedDocuments: changedDocs, - existenceChangedKeys: conditionChanged - }; + return { changedDocuments, existenceChangedKeys }; }); } diff --git a/packages/firestore/src/model/field_mask.ts b/packages/firestore/src/model/field_mask.ts index e30d8f6a65b..f33c075c633 100644 --- a/packages/firestore/src/model/field_mask.ts +++ b/packages/firestore/src/model/field_mask.ts @@ -17,6 +17,7 @@ import { debugAssert } from '../util/assert'; import { arrayEquals } from '../util/misc'; +import { SortedSet } from '../util/sorted_set'; import { FieldPath } from './path'; @@ -46,6 +47,21 @@ export class FieldMask { return new FieldMask([]); } + /** + * Returns a new FieldMask object that is the result of adding all the given + * fields paths to this field mask. + */ + unionWith(extraFields: FieldPath[]): FieldMask { + let mergedMaskSet = new SortedSet(FieldPath.comparator); + for (const fieldPath of this.fields) { + mergedMaskSet = mergedMaskSet.add(fieldPath); + } + for (const fieldPath of extraFields) { + mergedMaskSet = mergedMaskSet.add(fieldPath); + } + return new FieldMask(mergedMaskSet.toArray()); + } + /** * Verifies that `fieldPath` is included by at least one field in this field * mask. diff --git a/packages/firestore/src/model/mutation.ts b/packages/firestore/src/model/mutation.ts index 67da809e4a6..3c5c20f8c64 100644 --- a/packages/firestore/src/model/mutation.ts +++ b/packages/firestore/src/model/mutation.ts @@ -571,17 +571,9 @@ function patchMutationApplyToLocalView( return null; } - let mergedMaskSet = new SortedSet(FieldPath.comparator); - for (const fieldPath of previousMask.fields) { - mergedMaskSet = mergedMaskSet.add(fieldPath); - } - for (const fieldPath of mutation.fieldMask.fields) { - mergedMaskSet = mergedMaskSet.add(fieldPath); - } - mutation.fieldTransforms - .map(transform => transform.field) - .forEach(fieldPath => (mergedMaskSet = mergedMaskSet.add(fieldPath))); - return new FieldMask(mergedMaskSet.toArray()); + return previousMask + .unionWith(mutation.fieldMask.fields) + .unionWith(mutation.fieldTransforms.map(transform => transform.field)); } /** diff --git a/packages/firestore/test/unit/local/query_engine.test.ts b/packages/firestore/test/unit/local/query_engine.test.ts index a1fdaf1137f..4c2b5f2f03e 100644 --- a/packages/firestore/test/unit/local/query_engine.test.ts +++ b/packages/firestore/test/unit/local/query_engine.test.ts @@ -454,28 +454,8 @@ describe('QueryEngine', () => { // Add an unacknowledged mutation await addMutation(deleteMutation('coll/b')); const docs = await expectFullCollectionQuery(() => - persistence.runTransaction('runQuery', 'readonly', txn => { - return targetCache - .getMatchingKeysForTargetId(txn, TEST_TARGET_ID) - .next(remoteKeys => { - return queryEngine - .getDocumentsMatchingQuery( - txn, - query1, - LAST_LIMBO_FREE_SNAPSHOT, - remoteKeys - ) - .next(documentMap => { - let result = new DocumentSet(); - documentMap.forEach((_, v) => { - result = result.add(v); - }); - return result; - }); - }); - }) + runQuery(query1, LAST_LIMBO_FREE_SNAPSHOT) ); - verifyResult(docs, [MATCHING_DOC_A]); }); }); diff --git a/packages/firestore/test/unit/model/mutation.test.ts b/packages/firestore/test/unit/model/mutation.test.ts index 1945b852204..10a85a031a0 100644 --- a/packages/firestore/test/unit/model/mutation.test.ts +++ b/packages/firestore/test/unit/model/mutation.test.ts @@ -132,7 +132,7 @@ describe('Mutation', () => { mutationApplyToLocalView( overlay, docForOverlay, - /* previousMask */ null, + /* previousMask= */ null, timestamp ); } @@ -148,7 +148,12 @@ describe('Mutation', () => { const document = doc('collection/key', 0, docData); const set = setMutation('collection/key', { bar: 'bar-value' }); - mutationApplyToLocalView(set, document, /* previousMask */ null, timestamp); + mutationApplyToLocalView( + set, + document, + /* previousMask= */ null, + timestamp + ); expect(document).to.deep.equal( doc('collection/key', 0, { bar: 'bar-value' }).setHasLocalMutations() ); @@ -165,7 +170,7 @@ describe('Mutation', () => { mutationApplyToLocalView( patch, document, - /* previousMask */ null, + /* previousMask= */ null, timestamp ); expect(document).to.deep.equal( @@ -189,7 +194,7 @@ describe('Mutation', () => { mutationApplyToLocalView( patch, document, - /* previousMask */ null, + /* previousMask= */ null, timestamp ); expect(document).to.deep.equal( @@ -212,7 +217,7 @@ describe('Mutation', () => { mutationApplyToLocalView( patch, document, - /* previousMask */ null, + /* previousMask= */ null, timestamp ); expect(document).to.deep.equal( @@ -233,7 +238,7 @@ describe('Mutation', () => { mutationApplyToLocalView( patch, document, - /* previousMask */ null, + /* previousMask= */ null, timestamp ); expect(document).to.deep.equal( @@ -255,7 +260,7 @@ describe('Mutation', () => { mutationApplyToLocalView( patch, document, - /* previousMask */ null, + /* previousMask= */ null, timestamp ); expect(document).to.deep.equal( @@ -272,7 +277,7 @@ describe('Mutation', () => { mutationApplyToLocalView( patch, document, - /* previousMask */ null, + /* previousMask= */ null, timestamp ); expect(document).to.deep.equal(deletedDoc('collection/key', 0)); @@ -289,7 +294,7 @@ describe('Mutation', () => { mutationApplyToLocalView( transform, document, - /* previousMask */ null, + /* previousMask= */ null, timestamp ); @@ -459,7 +464,7 @@ describe('Mutation', () => { mutationApplyToLocalView( transform, document, - /* previousMask */ null, + /* previousMask= */ null, timestamp ); } @@ -603,7 +608,7 @@ describe('Mutation', () => { mutationApplyToLocalView( mutation, document, - /* previousMask */ null, + /* previousMask= */ null, Timestamp.now() ); expect(document).to.deep.equal( @@ -758,13 +763,13 @@ describe('Mutation', () => { mutationApplyToLocalView( transform, document, - /* previousMask */ null, + /* previousMask= */ null, Timestamp.now() ); mutationApplyToLocalView( transform, document, - /* previousMask */ null, + /* previousMask= */ null, Timestamp.now() ); From d88f9f183d810489d7eedfbff6f5a5c746c2deb5 Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Tue, 5 Apr 2022 16:01:18 -0500 Subject: [PATCH 9/9] Better null check. --- packages/firestore/src/local/local_documents_view.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/firestore/src/local/local_documents_view.ts b/packages/firestore/src/local/local_documents_view.ts index 0f54c35adc7..8281344de2b 100644 --- a/packages/firestore/src/local/local_documents_view.ts +++ b/packages/firestore/src/local/local_documents_view.ts @@ -230,11 +230,11 @@ export class LocalDocumentsView { if (baseDoc === null) { return; } - let mask: FieldMask | null = masks.get(key) ?? FieldMask.empty(); + let mask: FieldMask | null = masks.get(key) || FieldMask.empty(); mask = batch.applyToLocalView(baseDoc, mask); masks.set(key, mask); const newSet = ( - documentsByBatchId.get(batch.batchId) ?? documentKeySet() + documentsByBatchId.get(batch.batchId) || documentKeySet() ).add(key); documentsByBatchId = documentsByBatchId.insert( batch.batchId,