Adding FieldValue.numericAdd()

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

@@ -30,6 +30,7 @@
 import org.junit.Ignore;
 import org.junit.Test;
 
+// TODO(b/119035894): Enable these tests once available in Production
 @Ignore("Not yet available in production")
 public class NumericTransformsTest {
   private static final double DOUBLE_EPSILON = 0.000001;
@@ -88,7 +89,13 @@ public void createDocumentWithIncrement() {
   }
 
   @Test
-  public void integerIncrementExistingInteger() {
+  public void mergeOnNonExistingDocumentWithIncrement() {
+    waitFor(docRef.set(map("sum", FieldValue.numericAdd(1337)), SetOptions.merge()));
+    expectLocalAndRemoteValue(1337L);
+  }
+
+  @Test
+  public void integerIncrementWithExistingInteger() {
     writeInitialData(map("sum", 1337L));
     waitFor(docRef.update("sum", FieldValue.numericAdd(1)));
     expectLocalAndRemoteValue(1338L);
@@ -102,28 +109,28 @@ public void doubleIncrementWithExistingDouble() {
   }
 
   @Test
-  public void integerIncrementExistingDouble() {
+  public void integerIncrementWithExistingDouble() {
     writeInitialData(map("sum", 13.37D));
     waitFor(docRef.update("sum", FieldValue.numericAdd(1)));
     expectLocalAndRemoteValue(14.37D);
   }
 
   @Test
-  public void doubleIncrementExistingInteger() {
+  public void doubleIncrementWithExistingInteger() {
     writeInitialData(map("sum", 1337L));
     waitFor(docRef.update("sum", FieldValue.numericAdd(0.1)));
     expectLocalAndRemoteValue(1337.1D);
   }
 
   @Test
-  public void integerIncrementExistingString() {
+  public void integerIncrementWithExistingString() {
     writeInitialData(map("sum", "overwrite"));
     waitFor(docRef.update("sum", FieldValue.numericAdd(1337)));
     expectLocalAndRemoteValue(1337L);
   }
 
   @Test
-  public void doubleIncrementExistingString() {
+  public void doubleIncrementWithExistingString() {
     writeInitialData(map("sum", "overwrite"));
     waitFor(docRef.update("sum", FieldValue.numericAdd(13.37)));
     expectLocalAndRemoteValue(13.37D);

firebase-firestore/src/androidTest/java/com/google/firebase/firestore/testutil/IntegrationTestUtil.java

@@ -61,6 +61,7 @@ public class IntegrationTestUtil {
 
   // Whether the integration tests should run against a local Firestore emulator instead of the
   // Production environment. Note that the Android Emulator treats "10.0.2.2" as its host machine.
+  // TODO(mrschmidt): Support multiple envrionments (Emulator, QA, Nightly, Production)
   private static final boolean CONNECT_TO_EMULATOR = false;
 
   private static final String EMULATOR_HOST = "10.0.2.2";

firebase-firestore/src/main/java/com/google/firebase/firestore/FieldValue.java

@@ -158,8 +158,8 @@ public static FieldValue arrayRemove(@NonNull Object... elements) {
    * the given value to the field's current value.
    *
    * <p>If the current field value is an integer, possible integer overflows are resolved to
-   * Long.MAX_VALUE or Long.MIN_VALUE respectively. If the current field value is a double, both
-   * values will be interpreted as doubles and the arithmetic will follow IEEE 754 semantics.
+   * Long.MAX_VALUE or Long.MIN_VALUE. If the current field value is a double, both values will be
+   * interpreted as doubles and the arithmetic will follow IEEE 754 semantics.
    *
    * <p>If field is not an integer or double, or if the field does not yet exist, the transformation
    * will set the field to the given value.

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

@@ -211,12 +211,22 @@ public LocalWriteResult writeLocally(List<Mutation> mutations) {
           List<Mutation> baseMutations = new ArrayList<>();
           for (Mutation mutation : mutations) {
             MaybeDocument maybeDocument = existingDocuments.get(mutation.getKey());
-            if (maybeDocument instanceof Document && !mutation.isIdempotent()) {
+            if (!mutation.isIdempotent()) {
               FieldMask fieldMask = mutation.getFieldMask();
               if (fieldMask != null) {
-                ObjectValue baseValues = fieldMask.applyTo(((Document) maybeDocument).getData());
-                baseMutations.add(
-                    new PatchMutation(mutation.getKey(), baseValues, fieldMask, Precondition.NONE));
+                if (maybeDocument instanceof Document) {
+                  ObjectValue baseValues = fieldMask.applyTo(((Document) maybeDocument).getData());
+                  baseMutations.add(
+                      new PatchMutation(
+                          mutation.getKey(), baseValues, fieldMask, Precondition.NONE));
+                } else {
+                  baseMutations.add(
+                      new PatchMutation(
+                          mutation.getKey(),
+                          ObjectValue.emptyObject(),
+                          fieldMask,
+                          Precondition.NONE));
+                }
               }
             }
           }

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

@@ -119,8 +119,8 @@ public MaybeDocument applyToLocalView(DocumentKey documentKey, @Nullable MaybeDo
           maybeDoc.getKey());
     }
 
-    // First, apply the base state. This allows us to apply certain non-idempotent transform against
-    // a consistent set of values.
+    // First, apply the base state. This allows us to apply non-idempotent transform against a
+    // consistent set of values.
     for (int i = 0; i < baseMutations.size(); i++) {
       Mutation mutation = baseMutations.get(i);
       if (mutation.getKey().equals(documentKey)) {
@@ -143,14 +143,18 @@ public MaybeDocument applyToLocalView(DocumentKey documentKey, @Nullable MaybeDo
   /** Computes the local view for all provided documents given the mutations in this batch. */
   public ImmutableSortedMap<DocumentKey, MaybeDocument> applyToLocalView(
       ImmutableSortedMap<DocumentKey, MaybeDocument> maybeDocumentMap) {
-    ImmutableSortedMap<DocumentKey, MaybeDocument> changedDocuments = maybeDocumentMap;
+    // TODO(mrschmidt): This implementation is O(n^2). If we iterate through the mutations first
+    // (as done in `applyToLocalView(DocumentKey k, MaybeDoc d)`), we can reduce the complexity to
+    // O(n).
+
+    ImmutableSortedMap<DocumentKey, MaybeDocument> mutatedDocuments = maybeDocumentMap;
     for (DocumentKey key : getKeys()) {
-      MaybeDocument maybeDocument = applyToLocalView(key, changedDocuments.get(key));
-      if (maybeDocument != null) {
-        changedDocuments = changedDocuments.insert(maybeDocument.getKey(), maybeDocument);
+      MaybeDocument mutatedDocument = applyToLocalView(key, mutatedDocuments.get(key));
+      if (mutatedDocument != null) {
+        mutatedDocuments = mutatedDocuments.insert(mutatedDocument.getKey(), mutatedDocument);
       }
     }
-    return changedDocuments;
+    return mutatedDocuments;
   }
 
   @Override
@@ -225,7 +229,8 @@ public boolean isTombstone() {
 
   /** Converts this batch to a tombstone. */
   public MutationBatch toTombstone() {
-    return new MutationBatch(batchId, localWriteTime, baseMutations, Collections.emptyList());
+    return new MutationBatch(
+        batchId, localWriteTime, Collections.emptyList(), Collections.emptyList());
   }
 
   /** @return The user-provided mutations in this mutation batch. */

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

@@ -52,17 +52,18 @@ public FieldValue applyToLocalView(FieldValue previousValue, Timestamp localWrit
   }
 
   /**
-   * Implementation of Java 8's `safeAdd()` that resolves positive and negative numeric overflows to
-   * Long.MAX_VALUE or Long.MIN_VALUE respectively (instead of throwing an ArithmeticException).
+   * 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).
    */
   private long safeAdd(long x, long y) {
     long r = x + y;
 
+    // See "Hacker's Delight" 2-12: Overflow if both arguments have the opposite sign of the result
     if (((x ^ r) & (y ^ r)) >= 0) {
       return r;
     }
 
-    if (r > 0L) {
+    if (r >= 0L) {
       return Long.MIN_VALUE;
     } else {
       return Long.MAX_VALUE;

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

@@ -123,6 +123,10 @@ public MaybeDocument applyToLocalView(
 
   @Override
   public FieldMask getFieldMask() {
+    // Note: 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.
     List<FieldPath> fieldMask = new ArrayList<>();
     for (FieldTransform transform : fieldTransforms) {
       fieldMask.add(transform.getFieldPath());

firebase-firestore/src/proto/google/firebase/firestore/proto/mutation.proto

@@ -56,9 +56,15 @@ message WriteBatch {
   // The local time at which the write batch was initiated.
   google.protobuf.Timestamp local_write_time = 3;
 
-  // A list of writes that are used to populate a base state when this mutation
-  // is applied locally. These writes are used to locally overwrite values that
-  // are persisted in the remote document cache. These writes are never sent to
-  // the backend.
+  // A list of "writes" that represent a partial base state from when this
+  // write batch was initially created. During local application of the write
+  // batch, these base_writes are applied prior to the real writes in order to
+  // override certain document fields from the remote document cache. This is
+  // necessary in the case of non-idempotent writes (e.g. numericAdd
+  // transforms) to make sure that the local view of the modified documents
+  // doesn't flicker if the remote document cache receives the result of the
+  // non-idempotent write before the write is removed from the queue.
+  //
+  // These writes are never sent to the backend.
   repeated google.firestore.v1beta1.Write base_writes = 4;
 }

firebase-firestore/src/test/java/com/google/firebase/firestore/local/LocalStoreTestCase.java

@@ -968,6 +968,9 @@ public void testHandlesSetMutationThenTransformMutationThenTransformMutation() {
   @Test
   public void testHandlesSetMutationThenAckThenTransformMutationThenAckThenTransformMutation() {
     if (garbageCollectorIsEager()) {
+      // Since this test doesn't open a Query, Eager GC removes the documents from the cache as soon
+      // as the mutation is applied. This creates a lot of special casing in this unit test but does
+      // not expand its test coverage.
       return;
     }
 
@@ -1088,4 +1091,22 @@ public void testHandlesSetMutationThenTransformMutationThenRemoteEventThenTransf
     assertChanged(doc("foo/bar", 4, map("sum", 1339), Document.DocumentState.COMMITTED_MUTATIONS));
     assertContains(doc("foo/bar", 4, map("sum", 1339), Document.DocumentState.COMMITTED_MUTATIONS));
   }
+
+  @Test
+  public void testHandlesMergeMutationThenRemoteEvent() {
+    Query query = Query.atPath(ResourcePath.fromString("foo"));
+    allocateQuery(query);
+    assertTargetId(2);
+
+    writeMutations(
+        asList(
+            patchMutation("foo/bar", map(), Collections.emptyList()),
+            transformMutation("foo/bar", map("sum", FieldValue.numericAdd(1)))));
+    assertChanged(doc("foo/bar", 0, map("sum", 1), Document.DocumentState.LOCAL_MUTATIONS));
+    assertContains(doc("foo/bar", 0, map("sum", 1), Document.DocumentState.LOCAL_MUTATIONS));
+
+    applyRemoteEvent(addedRemoteEvent(doc("foo/bar", 1, map("sum", 1337)), asList(2), emptyList()));
+    assertChanged(doc("foo/bar", 1, map("sum", 1), Document.DocumentState.LOCAL_MUTATIONS));
+    assertContains(doc("foo/bar", 1, map("sum", 1), Document.DocumentState.LOCAL_MUTATIONS));
+  }
 }

firebase-firestore/src/test/java/com/google/firebase/firestore/model/MutationTest.java

@@ -171,7 +171,23 @@ public void testAppliesLocalServerTimestampTransformsToDocuments() {
   @Test
   public void testAppliesNumericAddTransformToDocument() {
     Map<String, Object> baseDoc =
-        map("longPlusLong", 1, "longPlusDouble", 2, "doublePlusLong", 3.3, "doublePlusDouble", 4.0);
+        map(
+            "longPlusLong",
+            1,
+            "longPlusDouble",
+            2,
+            "doublePlusLong",
+            3.3,
+            "doublePlusDouble",
+            4.0,
+            "longPlusNan",
+            5,
+            "doublePlusNan",
+            6.6,
+            "longPlusInfinity",
+            7,
+            "doublePlusInfinity",
+            8.8);
     Map<String, Object> transform =
         map(
             "longPlusLong",
@@ -181,7 +197,15 @@ public void testAppliesNumericAddTransformToDocument() {
             "doublePlusLong",
             FieldValue.numericAdd(3),
             "doublePlusDouble",
-            FieldValue.numericAdd(4.4));
+            FieldValue.numericAdd(4.4),
+            "longPlusNan",
+            FieldValue.numericAdd(Double.NaN),
+            "doublePlusNan",
+            FieldValue.numericAdd(Double.NaN),
+            "longPlusInfinity",
+            FieldValue.numericAdd(Double.POSITIVE_INFINITY),
+            "doublePlusInfinity",
+            FieldValue.numericAdd(Double.POSITIVE_INFINITY));
     Map<String, Object> expected =
         map(
             "longPlusLong",
@@ -191,7 +215,15 @@ public void testAppliesNumericAddTransformToDocument() {
             "doublePlusLong",
             6.3D,
             "doublePlusDouble",
-            8.4D);
+            8.4D,
+            "longPlusNan",
+            Double.NaN,
+            "doublePlusNan",
+            Double.NaN,
+            "longPlusInfinity",
+            Double.POSITIVE_INFINITY,
+            "doublePlusInfinity",
+            Double.POSITIVE_INFINITY);
 
     verifyTransform(baseDoc, transform, expected);
   }
@@ -224,10 +256,47 @@ public void testAppliesNumericAddTransformsConsecutively() {
 
   @Test
   public void testAppliesNumericAddWithoutOverflow() {
-    Map<String, Object> baseDoc = map("max", Long.MAX_VALUE - 1, "min", Long.MIN_VALUE + 1);
+    Map<String, Object> baseDoc =
+        map(
+            "a",
+            Long.MAX_VALUE - 1,
+            "b",
+            Long.MAX_VALUE - 1,
+            "c",
+            Long.MAX_VALUE,
+            "d",
+            Long.MAX_VALUE);
+    Map<String, Object> transform =
+        map(
+            "a", FieldValue.numericAdd(1),
+            "b", FieldValue.numericAdd(Long.MAX_VALUE),
+            "c", FieldValue.numericAdd(1),
+            "d", FieldValue.numericAdd(Long.MAX_VALUE));
+    Map<String, Object> expected =
+        map("a", Long.MAX_VALUE, "b", Long.MAX_VALUE, "c", Long.MAX_VALUE, "d", Long.MAX_VALUE);
+    verifyTransform(baseDoc, transform, expected);
+  }
+
+  @Test
+  public void testAppliesNumericAddWithoutUnderflow() {
+    Map<String, Object> baseDoc =
+        map(
+            "a",
+            Long.MIN_VALUE + 1,
+            "b",
+            Long.MIN_VALUE + 1,
+            "c",
+            Long.MIN_VALUE,
+            "d",
+            Long.MIN_VALUE);
     Map<String, Object> transform =
-        map("max", FieldValue.numericAdd(2), "min", FieldValue.numericAdd(-2));
-    Map<String, Object> expected = map("max", Long.MAX_VALUE, "min", Long.MIN_VALUE);
+        map(
+            "a", FieldValue.numericAdd(-1),
+            "b", FieldValue.numericAdd(Long.MIN_VALUE),
+            "c", FieldValue.numericAdd(-1),
+            "d", FieldValue.numericAdd(Long.MIN_VALUE));
+    Map<String, Object> expected =
+        map("a", Long.MIN_VALUE, "b", Long.MIN_VALUE, "c", Long.MIN_VALUE, "d", Long.MIN_VALUE);
     verifyTransform(baseDoc, transform, expected);
   }