Address comments.

Author: ehsannas

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<DocumentMap> {
-    let results = new SortedMap<DocumentKey, Document>(DocumentKey.comparator);
-    let recalculateDocuments = new SortedMap<DocumentKey, MutableDocument>(
-      DocumentKey.comparator
-    );
+    let results = documentMap();
+    let recalculateDocuments = mutableDocumentMap();
     const promises: Array<PersistencePromise<void>> = [];
     docs.forEach((_, doc) => {
       const overlayPromise = memoizedOverlays.has(doc.key)
@@ -209,21 +207,21 @@ export class LocalDocumentsView {
     let documentsByBatchId = new SortedMap<number, DocumentKeySet>(
       (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<PersistencePromise<void>> = [];
@@ -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<DK, Mutation|null>
-              //  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, Document>(
-          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, Document>(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, Document>(
-          DocumentKey.comparator
-        );
+        let results = documentMap();
         remoteDocuments.forEach((key, document) => {
           const overlay = overlays.get(key);
           if (overlay !== undefined) {

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
+    };
   });
 }
 

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>(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>(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)));

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.

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<unknown>,
   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<V>(
   }
 }
 
-/** 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<T>(input: T[]): T[][] {
   if (input.length === 0) {
     return [[]];
@@ -1033,7 +1036,8 @@ export function computePermutations<T>(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<T>(input: T[]): T[][] {
   const computeNonEmptyCombinations = (input: T[]): T[][] => {