Merge branch 'master' of github.com:firebase/firebase-android-sdk into ankita_fis

Author: ankitaj224

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

@@ -375,7 +375,7 @@ public void testUpdateTransactionally() {
     ArrayList<Task<Void>> readTasks = new ArrayList<>();
     // A barrier to make sure every transaction reaches the same spot.
     TaskCompletionSource<Void> barrier = new TaskCompletionSource<>();
-    AtomicInteger started = new AtomicInteger(0);
+    AtomicInteger counter = new AtomicInteger(0);
 
     FirebaseFirestore firestore = testFirestore();
     DocumentReference doc = firestore.collection("counters").document();
@@ -388,9 +388,9 @@ public void testUpdateTransactionally() {
       transactionTasks.add(
           firestore.runTransaction(
               transaction -> {
+                counter.incrementAndGet();
                 DocumentSnapshot snapshot = transaction.get(doc);
                 assertNotNull(snapshot);
-                started.incrementAndGet();
                 resolveRead.trySetResult(null);
                 waitFor(barrier.getTask());
                 transaction.update(doc, map("count", snapshot.getDouble("count") + 1.0));
@@ -400,10 +400,12 @@ public void testUpdateTransactionally() {
 
     // Let all of the transactions fetch the old value and stop once.
     waitFor(Tasks.whenAll(readTasks));
-    assertEquals(3, started.intValue());
-    // Let all of the transactions continue and wait for them to finish.
+    // There should be 3 initial transaction runs.
+    assertEquals(3, counter.get());
     barrier.setResult(null);
     waitFor(Tasks.whenAll(transactionTasks));
+    // There should be a maximum of 3 retries: once for the 2nd update, and twice for the 3rd update
+    assertTrue(counter.get() <= 6);
     // Now all transaction should be completed, so check the result.
     DocumentSnapshot snapshot = waitFor(doc.get());
     assertEquals(8, snapshot.getDouble("count").intValue());
@@ -546,15 +548,17 @@ public void testReadingADocTwiceWithDifferentVersions() {
     FirebaseFirestore firestore = testFirestore();
     DocumentReference doc = firestore.collection("counters").document();
     waitFor(doc.set(map("count", 15.0)));
+    AtomicInteger counter = new AtomicInteger(0);
     Exception e =
         waitForException(
             firestore.runTransaction(
                 transaction -> {
+                  counter.incrementAndGet();
                   // Get the doc once.
                   DocumentSnapshot snapshot1 = transaction.get(doc);
-                  assertEquals(15, snapshot1.getDouble("count").intValue());
-                  // Do a write outside of the transaction.
-                  waitFor(doc.set(map("count", 1234.0)));
+                  // Do a write outside of the transaction. Because the transaction will retry, set
+                  // the document to a different value each time.
+                  waitFor(doc.set(map("count", 1234.0 + counter.get())));
                   // Get the doc again in the transaction with the new version.
                   DocumentSnapshot snapshot2 = transaction.get(doc);
                   // The get itself will fail, because we already read an earlier version of this
@@ -563,8 +567,6 @@ public void testReadingADocTwiceWithDifferentVersions() {
                   return null;
                 }));
     assertEquals(Code.ABORTED, ((FirebaseFirestoreException) e).getCode());
-    DocumentSnapshot snapshot = waitFor(doc.get());
-    assertEquals(1234, snapshot.getDouble("count").intValue());
   }
 
   @Test
@@ -622,6 +624,28 @@ public void testCannotHaveAGetWithoutMutations() {
     assertEquals("Every document read in a transaction must also be written.", e.getMessage());
   }
 
+  @Test
+  public void testDoesNotRetryOnPermanentError() {
+    final FirebaseFirestore firestore = testFirestore();
+    AtomicInteger count = new AtomicInteger(0);
+    // Make a transaction that should fail with a permanent error
+    Task<Void> transactionTask =
+        firestore.runTransaction(
+            transaction -> {
+              count.incrementAndGet();
+              // Get and update a document that doesn't exist so that the transaction fails
+              DocumentSnapshot doc =
+                  transaction.get(firestore.collection("nonexistent").document());
+              transaction.update(doc.getReference(), "foo", "bar");
+              return null;
+            });
+
+    // Let all of the transactions fetch the old value and stop once.
+    Exception e = waitForException(transactionTask);
+    assertEquals(Code.INVALID_ARGUMENT, ((FirebaseFirestoreException) e).getCode());
+    assertEquals(1, count.get());
+  }
+
   @Test
   public void testSuccessWithNoTransactionOperations() {
     FirebaseFirestore firestore = testFirestore();

firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java

@@ -24,6 +24,7 @@
 import com.google.common.base.Function;
 import com.google.firebase.database.collection.ImmutableSortedMap;
 import com.google.firebase.database.collection.ImmutableSortedSet;
+import com.google.firebase.firestore.FirebaseFirestoreException;
 import com.google.firebase.firestore.auth.User;
 import com.google.firebase.firestore.local.LocalStore;
 import com.google.firebase.firestore.local.LocalViewChanges;
@@ -38,6 +39,7 @@
 import com.google.firebase.firestore.model.SnapshotVersion;
 import com.google.firebase.firestore.model.mutation.Mutation;
 import com.google.firebase.firestore.model.mutation.MutationBatchResult;
+import com.google.firebase.firestore.remote.Datastore;
 import com.google.firebase.firestore.remote.RemoteEvent;
 import com.google.firebase.firestore.remote.RemoteStore;
 import com.google.firebase.firestore.remote.TargetChange;
@@ -262,6 +264,9 @@ public <TResult> Task<TResult> transaction(
             asyncQueue.getExecutor(),
             userTask -> {
               if (!userTask.isSuccessful()) {
+                if (retries > 0 && isRetryableTransactionError(userTask.getException())) {
+                  return transaction(asyncQueue, updateFunction, retries - 1);
+                }
                 return userTask;
               }
               return transaction
@@ -272,11 +277,11 @@ public <TResult> Task<TResult> transaction(
                         if (commitTask.isSuccessful()) {
                           return Tasks.forResult(userTask.getResult());
                         }
-                        // TODO: Only retry on real transaction failures.
-                        if (retries == 0) {
-                          return Tasks.forException(commitTask.getException());
+                        Exception e = commitTask.getException();
+                        if (retries > 0 && isRetryableTransactionError(e)) {
+                          return transaction(asyncQueue, updateFunction, retries - 1);
                         }
-                        return transaction(asyncQueue, updateFunction, retries - 1);
+                        return Tasks.forException(e);
                       });
             });
   }
@@ -589,4 +594,16 @@ private boolean errorIsInteresting(Status error) {
 
     return false;
   }
+
+  private boolean isRetryableTransactionError(Exception e) {
+    if (e instanceof FirebaseFirestoreException) {
+      // In transactions, the backend will fail outdated reads with FAILED_PRECONDITION and
+      // non-matching document versions with ABORTED. These errors should be retried.
+      FirebaseFirestoreException.Code code = ((FirebaseFirestoreException) e).getCode();
+      return code == FirebaseFirestoreException.Code.ABORTED
+          || code == FirebaseFirestoreException.Code.FAILED_PRECONDITION
+          || !Datastore.isPermanentError(((FirebaseFirestoreException) e).getCode());
+    }
+    return false;
+  }
 }

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

@@ -17,6 +17,7 @@
 import static com.google.firebase.firestore.util.Assert.fail;
 import static com.google.firebase.firestore.util.Assert.hardAssert;
 
+import com.google.firebase.Timestamp;
 import com.google.firebase.database.collection.ImmutableSortedMap;
 import com.google.firebase.firestore.core.Query;
 import com.google.firebase.firestore.model.Document;
@@ -51,13 +52,18 @@ final class SQLiteRemoteDocumentCache implements RemoteDocumentCache {
   @Override
   public void add(MaybeDocument maybeDocument) {
     String path = pathForKey(maybeDocument.getKey());
+    Timestamp timestamp = maybeDocument.getVersion().getTimestamp();
     MessageLite message = serializer.encodeMaybeDocument(maybeDocument);
 
     statsCollector.recordRowsWritten(STATS_TAG, 1);
 
     db.execute(
-        "INSERT OR REPLACE INTO remote_documents (path, contents) VALUES (?, ?)",
+        "INSERT OR REPLACE INTO remote_documents "
+            + "(path, update_time_seconds, update_time_nanos, contents) "
+            + "VALUES (?, ?, ?, ?)",
         path,
+        timestamp.getSeconds(),
+        timestamp.getNanoseconds(),
         message.toByteArray());
 
     db.getIndexManager().addToCollectionParentIndex(maybeDocument.getKey().getPath().popLast());

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

@@ -45,8 +45,16 @@ class SQLiteSchema {
   /**
    * The version of the schema. Increase this by one for each migration added to runMigrations
    * below.
+   *
+   * <p>TODO(index-free): The migration to schema version 9 doesn't backfill `update_time` as this
+   * requires rewriting the RemoteDocumentCache. For index-free queries to efficiently handle
+   * existing documents, we still need to populate update_time for all existing entries, drop the
+   * RemoteDocumentCache or ask users to invoke `clearPersistence()` manually. If we decide to
+   * backfill or drop the contents of the RemoteDocumentCache, we need to perform an additional
+   * schema migration.
    */
-  static final int VERSION = 8;
+  static final int VERSION = 9;
+
   // Remove this constant and increment VERSION to enable indexing support
   static final int INDEXING_SUPPORT_VERSION = VERSION + 1;
 
@@ -127,6 +135,10 @@ void runMigrations(int fromVersion, int toVersion) {
       createV8CollectionParentsIndex();
     }
 
+    if (fromVersion < 9 && toVersion >= 9) {
+      addUpdateTime();
+    }
+
     /*
      * Adding a new migration? READ THIS FIRST!
      *
@@ -351,6 +363,16 @@ private void addSequenceNumber() {
     }
   }
 
+  private void addUpdateTime() {
+    if (!tableContainsColumn("remote_documents", "update_time_seconds")) {
+      hardAssert(
+          !tableContainsColumn("remote_documents", "update_time_nanos"),
+          "Table contained update_time_seconds, but is missing update_time_nanos");
+      db.execSQL("ALTER TABLE remote_documents ADD COLUMN update_time_seconds INTEGER");
+      db.execSQL("ALTER TABLE remote_documents ADD COLUMN update_time_nanos INTEGER");
+    }
+  }
+
   /**
    * Ensures that each entry in the remote document cache has a corresponding sentinel row. Any
    * entries that lack a sentinel row are given one with the sequence number set to the highest

firebase-firestore/src/main/java/com/google/firebase/firestore/remote/Datastore.java

@@ -189,8 +189,18 @@ public Task<List<MaybeDocument>> lookup(List<DocumentKey> keys) {
    * @see #isPermanentWriteError for classifying write errors.
    */
   public static boolean isPermanentError(Status status) {
+    return isPermanentError(FirebaseFirestoreException.Code.fromValue(status.getCode().value()));
+  }
+
+  /**
+   * Determines whether the given error code represents a permanent error when received in response
+   * to a non-write operation.
+   *
+   * @see #isPermanentWriteError for classifying write errors.
+   */
+  public static boolean isPermanentError(FirebaseFirestoreException.Code code) {
     // See go/firestore-client-errors
-    switch (status.getCode()) {
+    switch (code) {
       case OK:
         throw new IllegalArgumentException("Treated status OK as error");
       case CANCELLED:
@@ -217,7 +227,7 @@ public static boolean isPermanentError(Status status) {
       case DATA_LOSS:
         return true;
       default:
-        throw new IllegalArgumentException("Unknown gRPC status code: " + status.getCode());
+        throw new IllegalArgumentException("Unknown gRPC status code: " + code);
     }
   }