Merge branch 'master' into firebase-kotlin-format

Author: mrober

firebase-firestore/CHANGELOG.md

@@ -1,5 +1,6 @@
 # Unreleased
 * [feature] Add support for disjunctions in queries (`OR` queries).
+* [fixed] Fixed stack overflow caused by deeply nested server timestamps (#4702).
 
 # 24.4.4
 * [changed] Relaxed certain query validations performed by the SDK (#4231).

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

@@ -14,6 +14,7 @@
 
 package com.google.firebase.firestore;
 
+import static com.google.common.truth.Truth.assertWithMessage;
 import static com.google.firebase.firestore.testutil.IntegrationTestUtil.isRunningAgainstEmulator;
 import static com.google.firebase.firestore.testutil.IntegrationTestUtil.nullList;
 import static com.google.firebase.firestore.testutil.IntegrationTestUtil.querySnapshotToIds;
@@ -39,6 +40,8 @@
 import com.google.firebase.firestore.testutil.EventAccumulator;
 import com.google.firebase.firestore.testutil.IntegrationTestUtil;
 import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
@@ -1030,6 +1033,73 @@ public void testMultipleUpdatesWhileOffline() {
     assertEquals(asList(map("foo", "zzyzx", "bar", "2")), querySnapshotToValues(snapshot2));
   }
 
+  @Test
+  public void resumingAQueryShouldUseExistenceFilterToDetectDeletes() throws Exception {
+    // Prepare the names and contents of the 100 documents to create.
+    Map<String, Map<String, Object>> testData = new HashMap<>();
+    for (int i = 0; i < 100; i++) {
+      testData.put("doc" + (1000 + i), map("key", 42));
+    }
+
+    // Create 100 documents in a new collection.
+    CollectionReference collection = testCollectionWithDocs(testData);
+
+    // Run a query to populate the local cache with the 100 documents and a resume token.
+    List<DocumentReference> createdDocuments = new ArrayList<>();
+    {
+      QuerySnapshot querySnapshot = waitFor(collection.get());
+      assertWithMessage("querySnapshot1").that(querySnapshot.size()).isEqualTo(100);
+      for (DocumentSnapshot documentSnapshot : querySnapshot.getDocuments()) {
+        createdDocuments.add(documentSnapshot.getReference());
+      }
+    }
+
+    // Delete 50 of the 100 documents. Do this in a transaction, rather than
+    // DocumentReference.delete(), to avoid affecting the local cache.
+    HashSet<String> deletedDocumentIds = new HashSet<>();
+    waitFor(
+        collection
+            .getFirestore()
+            .runTransaction(
+                transaction -> {
+                  for (int i = 0; i < createdDocuments.size(); i += 2) {
+                    DocumentReference documentToDelete = createdDocuments.get(i);
+                    transaction.delete(documentToDelete);
+                    deletedDocumentIds.add(documentToDelete.getId());
+                  }
+                  return null;
+                }));
+
+    // Wait for 10 seconds, during which Watch will stop tracking the query and will send an
+    // existence filter rather than "delete" events when the query is resumed.
+    Thread.sleep(10000);
+
+    // Resume the query and save the resulting snapshot for verification.
+    QuerySnapshot snapshot2 = waitFor(collection.get());
+
+    // Verify that the snapshot from the resumed query contains the expected documents; that is,
+    // that it contains the 50 documents that were _not_ deleted.
+    // TODO(b/270731363): Remove the "if" condition below once the Firestore Emulator is fixed to
+    // send an existence filter. At the time of writing, the Firestore emulator fails to send an
+    // existence filter, resulting in the client including the deleted documents in the snapshot
+    // of the resumed query.
+    if (!(isRunningAgainstEmulator() && snapshot2.size() == 100)) {
+      HashSet<String> actualDocumentIds = new HashSet<>();
+      for (DocumentSnapshot documentSnapshot : snapshot2.getDocuments()) {
+        actualDocumentIds.add(documentSnapshot.getId());
+      }
+      HashSet<String> expectedDocumentIds = new HashSet<>();
+      for (DocumentReference documentRef : createdDocuments) {
+        if (!deletedDocumentIds.contains(documentRef.getId())) {
+          expectedDocumentIds.add(documentRef.getId());
+        }
+      }
+      assertWithMessage("snapshot2.docs")
+          .that(actualDocumentIds)
+          .containsExactlyElementsIn(expectedDocumentIds);
+    }
+  }
+
   @Test
   public void testOrQueries() {
     Map<String, Map<String, Object>> testDocs =

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

@@ -36,6 +36,7 @@
 import com.google.firebase.firestore.ListenerRegistration;
 import com.google.firebase.firestore.MetadataChanges;
 import com.google.firebase.firestore.QuerySnapshot;
+import com.google.firebase.firestore.WriteBatch;
 import com.google.firebase.firestore.auth.User;
 import com.google.firebase.firestore.core.DatabaseInfo;
 import com.google.firebase.firestore.model.DatabaseId;
@@ -347,8 +348,27 @@ public static CollectionReference testCollectionWithDocs(Map<String, Map<String,
 
   public static void writeAllDocs(
       CollectionReference collection, Map<String, Map<String, Object>> docs) {
+    WriteBatch writeBatch = null;
+    int writeBatchSize = 0;
+
     for (Map.Entry<String, Map<String, Object>> doc : docs.entrySet()) {
-      waitFor(collection.document(doc.getKey()).set(doc.getValue()));
+      if (writeBatch == null) {
+        writeBatch = collection.getFirestore().batch();
+      }
+
+      writeBatch.set(collection.document(doc.getKey()), doc.getValue());
+      writeBatchSize++;
+
+      // Write batches are capped at 500 writes. Use 400 just to be safe.
+      if (writeBatchSize == 400) {
+        waitFor(writeBatch.commit());
+        writeBatch = null;
+        writeBatchSize = 0;
+      }
+    }
+
+    if (writeBatch != null) {
+      waitFor(writeBatch.commit());
     }
   }
 

firebase-firestore/src/main/java/com/google/firebase/firestore/model/ServerTimestamps.java

@@ -62,6 +62,16 @@ public static Value valueOf(Timestamp localWriteTime, @Nullable Value previousVa
             .putFields(TYPE_KEY, encodedType)
             .putFields(LOCAL_WRITE_TIME_KEY, encodeWriteTime);
 
+    // We should avoid storing deeply nested server timestamp map values, because we never use the
+    // intermediate "previous values".
+    // For example:
+    // previous: 42L, add: t1, result: t1 -> 42L
+    // previous: t1,  add: t2, result: t2 -> 42L (NOT t2 -> t1 -> 42L)
+    // previous: t2,  add: t3, result: t3 -> 42L (NOT t3 -> t2 -> t1 -> 42L)
+    // previous: t3,  add: t4, result: t4 -> 42L (NOT t4 -> t3 -> t2 -> t1 -> 42L)
+    // `getPreviousValue` recursively traverses server timestamps to find the least recent Value.
+    previousValue =
+        isServerTimestamp(previousValue) ? getPreviousValue(previousValue) : previousValue;
     if (previousValue != null) {
       mapRepresentation.putFields(PREVIOUS_VALUE_KEY, previousValue);
     }

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

@@ -32,12 +32,29 @@
 import static java.util.Collections.emptyList;
 import static java.util.Collections.singletonList;
 
+import com.google.firebase.Timestamp;
 import com.google.firebase.firestore.FieldValue;
 import com.google.firebase.firestore.core.Query;
+import com.google.firebase.firestore.model.DocumentKey;
 import com.google.firebase.firestore.model.FieldIndex;
+import com.google.firebase.firestore.model.FieldPath;
+import com.google.firebase.firestore.model.ObjectValue;
+import com.google.firebase.firestore.model.ServerTimestamps;
+import com.google.firebase.firestore.model.mutation.FieldMask;
+import com.google.firebase.firestore.model.mutation.FieldTransform;
+import com.google.firebase.firestore.model.mutation.PatchMutation;
+import com.google.firebase.firestore.model.mutation.Precondition;
+import com.google.firebase.firestore.model.mutation.ServerTimestampOperation;
+import com.google.firestore.v1.Value;
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.atomic.AtomicReference;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.robolectric.RobolectricTestRunner;
@@ -290,4 +307,63 @@ public void testIndexesServerTimestamps() {
     assertOverlayTypes(keyMap("coll/a", CountingQueryEngine.OverlayType.Set));
     assertQueryReturned("coll/a");
   }
+
+  @Test
+  public void testDeeplyNestedServerTimestamps() {
+    Timestamp timestamp = Timestamp.now();
+    Value initialServerTimestamp = ServerTimestamps.valueOf(timestamp, null);
+    Map<String, Value> fields =
+        new HashMap<String, Value>() {
+          {
+            put("timestamp", ServerTimestamps.valueOf(timestamp, initialServerTimestamp));
+          }
+        };
+    FieldPath path = FieldPath.fromSingleSegment("timestamp");
+    FieldMask mask =
+        FieldMask.fromSet(
+            new HashSet<FieldPath>() {
+              {
+                add(path);
+              }
+            });
+    FieldTransform fieldTransform =
+        new FieldTransform(path, ServerTimestampOperation.getInstance());
+    List<FieldTransform> fieldTransforms =
+        new ArrayList<FieldTransform>() {
+          {
+            add(fieldTransform);
+          }
+        };
+    // The purpose of this test is to ensure that deeply nested server timestamps do not result in
+    // a stack overflow error. Below we use a `Thread` object to create a large number of mutations
+    // because the `Thread` class allows us to specify the maximum stack size.
+    AtomicReference<Throwable> error = new AtomicReference<>();
+    Thread thread =
+        new Thread(
+            Thread.currentThread().getThreadGroup(),
+            () -> {
+              try {
+                for (int i = 0; i < 1000; ++i) {
+                  writeMutation(
+                      new PatchMutation(
+                          DocumentKey.fromPathString("some/object/for/test"),
+                          ObjectValue.fromMap(fields),
+                          mask,
+                          Precondition.NONE,
+                          fieldTransforms));
+                }
+              } catch (Throwable e) {
+                error.set(e);
+              }
+            },
+            /* name */ "test",
+            /* stackSize */ 1024 * 1024);
+    try {
+      thread.start();
+      thread.join();
+    } catch (InterruptedException e) {
+      throw new AssertionError(e);
+    }
+    assertThat(error.get()).isNull();
+  }
 }