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

Author: schmidt-sebastian

firebase-firestore/CHANGELOG.md

@@ -1,4 +1,7 @@
 # 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 (#491).
 - [changed] Improved performance for queries with filters that only return a
   small subset of the documents in a collection.
 - [changed] Instead of failing silently, Firestore now crashes the client app

firebase-firestore/src/androidTest/java/com/google/firebase/firestore/NumericTransformsTest.java

@@ -19,6 +19,7 @@
 import static com.google.firebase.firestore.testutil.TestUtil.map;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
 
 import androidx.test.ext.junit.runners.AndroidJUnit4;
 import com.google.android.gms.tasks.Tasks;
@@ -158,4 +159,55 @@ public void multipleDoubleIncrements() throws ExecutionException, InterruptedExc
     snap = accumulator.awaitRemoteEvent();
     assertEquals(0.111D, snap.getDouble("sum"), DOUBLE_EPSILON);
   }
+
+  @Test
+  public void incrementTwiceInABatch() {
+    writeInitialData(map("sum", "overwrite"));
+    waitFor(
+        docRef
+            .getFirestore()
+            .batch()
+            .update(docRef, "sum", FieldValue.increment(1))
+            .update(docRef, "sum", FieldValue.increment(1))
+            .commit());
+    expectLocalAndRemoteValue(2L);
+  }
+
+  @Test
+  public void incrementDeleteIncrementInABatch() {
+    writeInitialData(map("sum", "overwrite"));
+    waitFor(
+        docRef
+            .getFirestore()
+            .batch()
+            .update(docRef, "sum", FieldValue.increment(1))
+            .update(docRef, "sum", FieldValue.delete())
+            .update(docRef, "sum", FieldValue.increment(3))
+            .commit());
+    expectLocalAndRemoteValue(3L);
+  }
+
+  @Test
+  public void serverTimestampAndIncrement() throws ExecutionException, InterruptedException {
+    // This test stacks two pending transforms (a ServerTimestamp and an Increment transform) and
+    // reproduces the setup that was reported in
+    // https://github.com/firebase/firebase-android-sdk/issues/491
+    // In our original code, a NumericIncrementTransformOperation could cause us to decode the
+    // ServerTimestamp as part of a PatchMutation, which triggered an assertion failure.
+    Tasks.await(docRef.getFirestore().disableNetwork());
+
+    docRef.set(map("val", FieldValue.serverTimestamp()));
+    docRef.set(map("val", FieldValue.increment(1)));
+
+    DocumentSnapshot snap = accumulator.awaitLocalEvent();
+    assertNotNull(snap.getTimestamp("val", DocumentSnapshot.ServerTimestampBehavior.ESTIMATE));
+
+    snap = accumulator.awaitLocalEvent();
+    assertEquals(1, (long) snap.getLong("val"));
+
+    Tasks.await(docRef.getFirestore().enableNetwork());
+
+    snap = accumulator.awaitRemoteEvent();
+    assertEquals(1, (long) snap.getLong("val"));
+  }
 }

firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ServerTimestampTest.java

@@ -181,6 +181,12 @@ public void testServerTimestampsCanReturnPreviousValue() {
     DocumentSnapshot previousSnapshot = accumulator.awaitRemoteEvent();
     verifyTimestampsAreResolved(previousSnapshot);
 
+    // The following update includes an update of the nested map "deep", which updates it to contain
+    // a single ServerTimestamp. As such, the update is split into two mutations: One that sets
+    // "deep" to an empty map and overwrites the previous ServerTimestamp value and a second
+    // transform that writes the new ServerTimestamp. This step in the test verifies that we can
+    // still access the old ServerTimestamp value (from `previousSnapshot`) even though it was
+    // removed in an intermediate step.
     waitFor(docRef.update(updateData));
     verifyTimestampsUsePreviousValue(accumulator.awaitLocalEvent(), previousSnapshot);
     verifyTimestampsAreResolved(accumulator.awaitRemoteEvent());

firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalStore.java

@@ -28,7 +28,6 @@
 import com.google.firebase.firestore.model.DocumentKey;
 import com.google.firebase.firestore.model.MaybeDocument;
 import com.google.firebase.firestore.model.SnapshotVersion;
-import com.google.firebase.firestore.model.mutation.FieldMask;
 import com.google.firebase.firestore.model.mutation.Mutation;
 import com.google.firebase.firestore.model.mutation.MutationBatch;
 import com.google.firebase.firestore.model.mutation.MutationBatchResult;
@@ -212,24 +211,17 @@ public LocalWriteResult writeLocally(List<Mutation> mutations) {
           // transform.
           List<Mutation> baseMutations = new ArrayList<>();
           for (Mutation mutation : mutations) {
-            MaybeDocument maybeDocument = existingDocuments.get(mutation.getKey());
-            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.
-              FieldMask fieldMask = mutation.getFieldMask();
-              if (fieldMask != null) {
-                ObjectValue baseValues =
-                    (maybeDocument instanceof Document)
-                        ? fieldMask.applyTo(((Document) maybeDocument).getData())
-                        : ObjectValue.emptyObject();
-                // NOTE: The base state should only be applied if there's some existing
-                // document to override, so use a Precondition of exists=true
-                baseMutations.add(
-                    new PatchMutation(
-                        mutation.getKey(), baseValues, fieldMask, Precondition.exists(true)));
-              }
+            ObjectValue baseValue =
+                mutation.extractBaseValue(existingDocuments.get(mutation.getKey()));
+            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.add(
+                  new PatchMutation(
+                      mutation.getKey(),
+                      baseValue,
+                      baseValue.getFieldMask(),
+                      Precondition.exists(true)));
             }
           }
 

firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/ArrayTransformOperation.java

@@ -39,17 +39,24 @@ public List<FieldValue> getElements() {
   }
 
   @Override
-  public FieldValue applyToLocalView(FieldValue previousValue, Timestamp localWriteTime) {
+  public FieldValue applyToLocalView(@Nullable FieldValue previousValue, Timestamp localWriteTime) {
     return apply(previousValue);
   }
 
   @Override
-  public FieldValue applyToRemoteDocument(FieldValue previousValue, FieldValue transformResult) {
+  public FieldValue applyToRemoteDocument(
+      @Nullable FieldValue previousValue, FieldValue transformResult) {
     // The server just sends null as the transform result for array operations, so we have to
     // calculate a result the same as we do for local applications.
     return apply(previousValue);
   }
 
+  @Override
+  @Nullable
+  public FieldValue computeBaseValue(@Nullable FieldValue currentValue) {
+    return null; // Array transforms are idempotent and don't require a base value.
+  }
+
   @Override
   @SuppressWarnings("EqualsGetClass") // subtype-sensitive equality is intended.
   public boolean equals(Object o) {
@@ -73,7 +80,7 @@ public int hashCode() {
   }
 
   /** Applies this ArrayTransformOperation against the specified previousValue. */
-  protected abstract ArrayValue apply(FieldValue previousValue);
+  protected abstract ArrayValue apply(@Nullable FieldValue previousValue);
 
   /**
    * Inspects the provided value, returning an ArrayList copy of the internal array if it's an
@@ -88,19 +95,14 @@ static ArrayList<FieldValue> coercedFieldValuesArray(@Nullable FieldValue value)
     }
   }
 
-  @Override
-  public boolean isIdempotent() {
-    return true;
-  }
-
   /** An array union transform operation. */
   public static class Union extends ArrayTransformOperation {
     public Union(List<FieldValue> elements) {
       super(elements);
     }
 
     @Override
-    protected ArrayValue apply(FieldValue previousValue) {
+    protected ArrayValue apply(@Nullable FieldValue previousValue) {
       ArrayList<FieldValue> result = coercedFieldValuesArray(previousValue);
       for (FieldValue element : getElements()) {
         if (!result.contains(element)) {
@@ -118,7 +120,7 @@ public Remove(List<FieldValue> elements) {
     }
 
     @Override
-    protected ArrayValue apply(FieldValue previousValue) {
+    protected ArrayValue apply(@Nullable FieldValue previousValue) {
       ArrayList<FieldValue> result = coercedFieldValuesArray(previousValue);
       for (FieldValue element : getElements()) {
         result.removeAll(Collections.singleton(element));

firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/DeleteMutation.java

@@ -21,6 +21,7 @@
 import com.google.firebase.firestore.model.MaybeDocument;
 import com.google.firebase.firestore.model.NoDocument;
 import com.google.firebase.firestore.model.SnapshotVersion;
+import com.google.firebase.firestore.model.value.ObjectValue;
 import javax.annotation.Nullable;
 
 /** Represents a Delete operation */
@@ -85,12 +86,7 @@ public MaybeDocument applyToLocalView(
 
   @Nullable
   @Override
-  public FieldMask getFieldMask() {
+  public ObjectValue extractBaseValue(@Nullable MaybeDocument maybeDoc) {
     return null;
   }
-
-  @Override
-  public boolean isIdempotent() {
-    return true;
-  }
 }

firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/FieldMask.java

@@ -15,8 +15,6 @@
 package com.google.firebase.firestore.model.mutation;
 
 import com.google.firebase.firestore.model.FieldPath;
-import com.google.firebase.firestore.model.value.FieldValue;
-import com.google.firebase.firestore.model.value.ObjectValue;
 import java.util.Set;
 
 /**
@@ -71,25 +69,6 @@ public boolean covers(FieldPath fieldPath) {
     return false;
   }
 
-  /**
-   * 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.
-   */
-  public ObjectValue applyTo(ObjectValue data) {
-    ObjectValue filteredObject = ObjectValue.emptyObject();
-    for (FieldPath path : mask) {
-      if (path.isEmpty()) {
-        return data;
-      } else {
-        FieldValue newValue = data.get(path);
-        if (newValue != null) {
-          filteredObject = filteredObject.set(path, newValue);
-        }
-      }
-    }
-    return filteredObject;
-  }
-
   @Override
   public int hashCode() {
     return mask.hashCode();

firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/FieldTransform.java

@@ -58,8 +58,4 @@ public int hashCode() {
     result = 31 * result + operation.hashCode();
     return result;
   }
-
-  public boolean isIdempotent() {
-    return operation.isIdempotent();
-  }
 }

firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/Mutation.java

@@ -21,6 +21,7 @@
 import com.google.firebase.firestore.model.DocumentKey;
 import com.google.firebase.firestore.model.MaybeDocument;
 import com.google.firebase.firestore.model.SnapshotVersion;
+import com.google.firebase.firestore.model.value.ObjectValue;
 import javax.annotation.Nullable;
 
 /**
@@ -113,6 +114,23 @@ public abstract MaybeDocument applyToRemoteDocument(
   public abstract MaybeDocument applyToLocalView(
       @Nullable MaybeDocument maybeDoc, @Nullable MaybeDocument baseDoc, Timestamp localWriteTime);
 
+  /**
+   * If applicable, returns the base value to persist with this mutation. If a base value is
+   * provided, the mutation is always applied to this base value, even if document has already been
+   * updated.
+   *
+   * <p>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
+   * value 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.
+   */
+  @Nullable
+  public abstract ObjectValue extractBaseValue(@Nullable MaybeDocument maybeDoc);
+
   /** Helper for derived classes to implement .equals(). */
   boolean hasSameKeyAndPrecondition(Mutation other) {
     return key.equals(other.key) && precondition.equals(other.precondition);
@@ -148,15 +166,4 @@ static SnapshotVersion getPostMutationVersion(@Nullable MaybeDocument maybeDoc)
       return SnapshotVersion.NONE;
     }
   }
-
-  /**
-   * 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 all document
-   * values return 'null'.
-   */
-  @Nullable
-  public abstract FieldMask getFieldMask();
-
-  /** Returns whether all operations in the mutation are idempotent. */
-  public abstract boolean isIdempotent();
 }

firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/NumericIncrementTransformOperation.java

@@ -15,7 +15,9 @@
 package com.google.firebase.firestore.model.mutation;
 
 import static com.google.firebase.firestore.util.Assert.fail;
+import static com.google.firebase.firestore.util.Assert.hardAssert;
 
+import androidx.annotation.Nullable;
 import com.google.firebase.Timestamp;
 import com.google.firebase.firestore.model.value.DoubleValue;
 import com.google.firebase.firestore.model.value.FieldValue;
@@ -35,23 +37,47 @@ public NumericIncrementTransformOperation(NumberValue operand) {
   }
 
   @Override
-  public FieldValue applyToLocalView(FieldValue previousValue, Timestamp localWriteTime) {
+  public FieldValue applyToLocalView(@Nullable FieldValue previousValue, Timestamp localWriteTime) {
+    NumberValue baseValue = computeBaseValue(previousValue);
+
     // Return an integer value only if the previous value and the operand is an integer.
-    if (previousValue instanceof IntegerValue && operand instanceof IntegerValue) {
-      long sum = safeIncrement(((IntegerValue) previousValue).getInternalValue(), operandAsLong());
+    if (baseValue instanceof IntegerValue && operand instanceof IntegerValue) {
+      long sum = safeIncrement(((IntegerValue) baseValue).getInternalValue(), operandAsLong());
       return IntegerValue.valueOf(sum);
-    } else if (previousValue instanceof IntegerValue) {
-      double sum = ((IntegerValue) previousValue).getInternalValue() + operandAsDouble();
+    } else if (baseValue instanceof IntegerValue) {
+      double sum = ((IntegerValue) baseValue).getInternalValue() + operandAsDouble();
       return DoubleValue.valueOf(sum);
-    } else if (previousValue instanceof DoubleValue) {
-      double sum = ((DoubleValue) previousValue).getInternalValue() + operandAsDouble();
+    } else {
+      hardAssert(
+          baseValue instanceof DoubleValue,
+          "Expected NumberValue to be of type DoubleValue, but was ",
+          previousValue.getClass().getCanonicalName());
+      double sum = ((DoubleValue) baseValue).getInternalValue() + operandAsDouble();
       return DoubleValue.valueOf(sum);
     }
+  }
+
+  @Override
+  public FieldValue applyToRemoteDocument(
+      @Nullable FieldValue previousValue, FieldValue transformResult) {
+    return transformResult;
+  }
 
-    // If the existing value is not a number, use the value of the transform as the new base value.
+  public FieldValue getOperand() {
     return operand;
   }
 
+  /**
+   * Inspects the provided value, returning the provided value if it is already a NumberValue,
+   * otherwise returning a coerced IntegerValue of 0.
+   */
+  @Override
+  public NumberValue computeBaseValue(@Nullable FieldValue previousValue) {
+    return previousValue instanceof NumberValue
+        ? (NumberValue) previousValue
+        : IntegerValue.valueOf(0L);
+  }
+
   /**
    * Implementation of Java 8's `addExact()` that resolves positive and negative numeric overflows
    * to Long.MAX_VALUE or Long.MIN_VALUE respectively (instead of throwing an ArithmeticException).
@@ -94,18 +120,4 @@ private long operandAsLong() {
               + operand.getClass().getCanonicalName());
     }
   }
-
-  @Override
-  public FieldValue applyToRemoteDocument(FieldValue previousValue, FieldValue transformResult) {
-    return transformResult;
-  }
-
-  public FieldValue getOperand() {
-    return operand;
-  }
-
-  @Override
-  public boolean isIdempotent() {
-    return false;
-  }
 }