Store the coerced base value in the BaseMutation (#1928)

Author: schmidt-sebastian

packages/firestore/CHANGELOG.md

@@ -1,5 +1,8 @@
 
 # Unreleased
+- [fixed] Fixed an internal assertion that was triggered when an update
+   with a `FieldValue.serverTimestamp()` and an update with a
+  `FieldValue.increment()` were pending for the same document.
 - [feature] Added `clearPersistence()`, which clears the persistent storage
   including pending writes and cached documents. This is intended to help
   write reliable tests (#449).

packages/firestore/src/local/local_store.ts

@@ -27,7 +27,7 @@ import {
   maybeDocumentMap,
   MaybeDocumentMap
 } from '../model/collections';
-import { Document, MaybeDocument } from '../model/document';
+import { MaybeDocument } from '../model/document';
 import { DocumentKey } from '../model/document_key';
 import { Mutation, PatchMutation, Precondition } from '../model/mutation';
 import {
@@ -40,7 +40,6 @@ import { assert } from '../util/assert';
 import * as log from '../util/log';
 import * as objUtils from '../util/obj';
 
-import { ObjectValue } from '../model/field_value';
 import { LocalDocumentsView } from './local_documents_view';
 import { LocalViewChanges } from './local_view_changes';
 import { LruGarbageCollector, LruResults } from './lru_garbage_collector';
@@ -272,32 +271,21 @@ export class LocalStore {
             const baseMutations: Mutation[] = [];
 
             for (const mutation of mutations) {
-              const maybeDoc = existingDocs.get(mutation.key);
-              if (!mutation.isIdempotent) {
-                // Theoretically, we should only include non-idempotent fields
-                // in this field mask as this mask is used to populate the base
-                // state for all DocumentTransforms.  By  including all fields,
-                // we incorrectly prevent rebasing of idempotent transforms
-                // (such as `arrayUnion()`) when any non-idempotent transforms
-                // are present.
-                const fieldMask = mutation.fieldMask;
-                if (fieldMask) {
-                  const baseValues =
-                    maybeDoc instanceof Document
-                      ? fieldMask.applyTo(maybeDoc.data)
-                      : ObjectValue.EMPTY;
-                  // NOTE: The base state should only be applied if there's some
-                  // existing document to override, so use a Precondition of
-                  // exists=true
-                  baseMutations.push(
-                    new PatchMutation(
-                      mutation.key,
-                      baseValues,
-                      fieldMask,
-                      Precondition.exists(true)
-                    )
-                  );
-                }
+              const baseValue = mutation.extractBaseValue(
+                existingDocs.get(mutation.key)
+              );
+              if (baseValue != null) {
+                // NOTE: The base state should only be applied if there's some
+                // existing document to override, so use a Precondition of
+                // exists=true
+                baseMutations.push(
+                  new PatchMutation(
+                    mutation.key,
+                    baseValue,
+                    baseValue.fieldMask(),
+                    Precondition.exists(true)
+                  )
+                );
               }
             }
 

packages/firestore/src/model/field_value.ts

@@ -22,10 +22,11 @@ import { Timestamp } from '../api/timestamp';
 import { DatabaseId } from '../core/database_info';
 import { assert, fail } from '../util/assert';
 import { primitiveComparator } from '../util/misc';
-import { SortedMap } from '../util/sorted_map';
-
 import { DocumentKey } from './document_key';
+import { FieldMask } from './mutation';
 import { FieldPath } from './path';
+import { SortedMap } from '../util/sorted_map';
+import { SortedSet } from '../util/sorted_set';
 
 /**
  * Supported data value types:
@@ -605,6 +606,34 @@ export class ObjectValue extends FieldValue {
     return field;
   }
 
+  /**
+   * Returns a FieldMask built from all FieldPaths starting from this ObjectValue,
+   * including paths from nested objects.
+   */
+  fieldMask(): FieldMask {
+    let fields = new SortedSet<FieldPath>(FieldPath.comparator);
+    this.internalValue.forEach((key, value) => {
+      const currentPath = new FieldPath([key]);
+      if (value instanceof ObjectValue) {
+        const nestedMask = value.fieldMask();
+        const nestedFields = nestedMask.fields;
+        if (nestedFields.isEmpty()) {
+          // Preserve the empty map by adding it to the FieldMask.
+          fields = fields.add(currentPath);
+        } else {
+          // For nested and non-empty ObjectValues, add the FieldPath of the
+          // leaf nodes.
+          nestedFields.forEach(nestedPath => {
+            fields = fields.add(currentPath.child(nestedPath));
+          });
+        }
+      } else {
+        fields = fields.add(currentPath);
+      }
+    });
+    return FieldMask.fromSet(fields);
+  }
+
   toString(): string {
     return this.internalValue.toString();
   }

packages/firestore/src/model/mutation.ts

@@ -73,27 +73,6 @@ export class FieldMask {
     return found;
   }
 
-  /**
-   * Applies this field mask to the provided object value and returns an object
-   * that only contains fields that are specified in both the input object and
-   * this field mask.
-   */
-  applyTo(data: ObjectValue): ObjectValue {
-    let filteredObject = ObjectValue.EMPTY;
-    this.fields.forEach(fieldMaskPath => {
-      if (fieldMaskPath.isEmpty()) {
-        return data;
-      } else {
-        const newValue = data.field(fieldMaskPath);
-        if (newValue !== null) {
-          filteredObject = filteredObject.set(fieldMaskPath, newValue);
-        }
-      }
-    });
-
-    return filteredObject;
-  }
-
   isEqual(other: FieldMask): boolean {
     return this.fields.isEqual(other.fields);
   }
@@ -106,11 +85,6 @@ export class FieldTransform {
     readonly transform: TransformOperation
   ) {}
 
-  /** Whether this field transform is idempotent. */
-  get isIdempotent(): boolean {
-    return this.transform.isIdempotent;
-  }
-
   isEqual(other: FieldTransform): boolean {
     return (
       this.field.isEqual(other.field) && this.transform.isEqual(other.transform)
@@ -302,17 +276,25 @@ export abstract class Mutation {
     localWriteTime: Timestamp
   ): MaybeDocument | null;
 
-  abstract isEqual(other: Mutation): boolean;
-
   /**
-   * If applicable, returns the field mask for this mutation. Fields that are
-   * not included in this field mask are not modified when this mutation is
-   * applied. Mutations that replace the entire document return 'null'.
+   * If this mutation is not idempotent, returns the base value to persist with
+   * this mutation. If a base value is returned, the mutation is always applied
+   * to this base value, even if document has already been updated.
+   *
+   * The base value is a sparse object that consists of only the document
+   * fields for which this mutation contains a non-idempotent transformation
+   * (e.g. a numeric increment). The provided alue guarantees consistent
+   * behavior for non-idempotent transforms and allow us to return the same
+   * latency-compensated value even if the backend has already applied the
+   * mutation. The base value is null for idempotent mutations, as they can be
+   * re-played even if the backend has already applied them.
+   *
+   * @return a base value to store along with the mutation, or null for
+   * idempotent mutations.
    */
-  abstract get fieldMask(): FieldMask | null;
+  abstract extractBaseValue(maybeDoc: MaybeDocument | null): ObjectValue | null;
 
-  /** Returns whether all operations in the mutation are idempotent. */
-  abstract get isIdempotent(): boolean;
+  abstract isEqual(other: Mutation): boolean;
 
   protected verifyKeyMatches(maybeDoc: MaybeDocument | null): void {
     if (maybeDoc != null) {
@@ -393,11 +375,7 @@ export class SetMutation extends Mutation {
     });
   }
 
-  get isIdempotent(): true {
-    return true;
-  }
-
-  get fieldMask(): null {
+  extractBaseValue(maybeDoc: MaybeDocument | null): null {
     return null;
   }
 
@@ -479,8 +457,8 @@ export class PatchMutation extends Mutation {
     });
   }
 
-  get isIdempotent(): true {
-    return true;
+  extractBaseValue(maybeDoc: MaybeDocument | null): null {
+    return null;
   }
 
   isEqual(other: Mutation): boolean {
@@ -592,6 +570,7 @@ export class TransformMutation extends Mutation {
     const doc = this.requireDocument(maybeDoc);
     const transformResults = this.localTransformResults(
       localWriteTime,
+      maybeDoc,
       baseDoc
     );
     const newData = this.transformObject(doc.data, transformResults);
@@ -600,22 +579,29 @@ export class TransformMutation extends Mutation {
     });
   }
 
-  get isIdempotent(): boolean {
+  extractBaseValue(maybeDoc: MaybeDocument | null): ObjectValue | null {
+    let baseObject: ObjectValue | null = null;
     for (const fieldTransform of this.fieldTransforms) {
-      if (!fieldTransform.isIdempotent) {
-        return false;
+      const existingValue =
+        maybeDoc instanceof Document
+          ? maybeDoc.field(fieldTransform.field)
+          : undefined;
+      const coercedValue = fieldTransform.transform.computeBaseValue(
+        existingValue || null
+      );
+
+      if (coercedValue != null) {
+        if (baseObject == null) {
+          baseObject = ObjectValue.EMPTY.set(
+            fieldTransform.field,
+            coercedValue
+          );
+        } else {
+          baseObject = baseObject.set(fieldTransform.field, coercedValue);
+        }
       }
     }
-
-    return true;
-  }
-
-  get fieldMask(): FieldMask {
-    let fieldMask = new SortedSet<FieldPath>(FieldPath.comparator);
-    this.fieldTransforms.forEach(
-      transform => (fieldMask = fieldMask.add(transform.field))
-    );
-    return new FieldMask(fieldMask);
+    return baseObject;
   }
 
   isEqual(other: Mutation): boolean {
@@ -690,19 +676,30 @@ export class TransformMutation extends Mutation {
    *
    * @param localWriteTime The local time of the transform mutation (used to
    *     generate ServerTimestampValues).
+   * @param maybeDoc The current state of the document after applying all
+   *     previous mutations.
    * @param baseDoc The document prior to applying this mutation batch.
    * @return The transform results list.
    */
   private localTransformResults(
     localWriteTime: Timestamp,
+    maybeDoc: MaybeDocument | null,
     baseDoc: MaybeDocument | null
   ): FieldValue[] {
     const transformResults = [] as FieldValue[];
     for (const fieldTransform of this.fieldTransforms) {
       const transform = fieldTransform.transform;
 
       let previousValue: FieldValue | null = null;
-      if (baseDoc instanceof Document) {
+      if (maybeDoc instanceof Document) {
+        previousValue = maybeDoc.field(fieldTransform.field);
+      }
+
+      if (previousValue === null && baseDoc instanceof Document) {
+        // If the current document does not contain a value for the mutated
+        // field, use the value that existed before applying this mutation
+        // batch. This solves an edge case where a PatchMutation clears the
+        // values in a nested map before the TransformMutation is applied.
         previousValue = baseDoc.field(fieldTransform.field);
       }
 
@@ -779,19 +776,15 @@ export class DeleteMutation extends Mutation {
     return new NoDocument(this.key, SnapshotVersion.forDeletedDoc());
   }
 
+  extractBaseValue(maybeDoc: MaybeDocument | null): null {
+    return null;
+  }
+
   isEqual(other: Mutation): boolean {
     return (
       other instanceof DeleteMutation &&
       this.key.isEqual(other.key) &&
       this.precondition.isEqual(other.precondition)
     );
   }
-
-  get isIdempotent(): true {
-    return true;
-  }
-
-  get fieldMask(): null {
-    return null;
-  }
 }