Index-free: Drop last limbo free snapshot if re-upgraded (#734)

Author: schmidt-sebastian

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

@@ -14,6 +14,7 @@
 
 package com.google.firebase.firestore.local;
 
+import static com.google.firebase.firestore.util.Assert.fail;
 import static com.google.firebase.firestore.util.Assert.hardAssert;
 
 import android.content.ContentValues;
@@ -26,7 +27,9 @@
 import androidx.annotation.VisibleForTesting;
 import com.google.common.base.Preconditions;
 import com.google.firebase.firestore.model.ResourcePath;
+import com.google.firebase.firestore.proto.Target;
 import com.google.firebase.firestore.util.Consumer;
+import com.google.protobuf.InvalidProtocolBufferException;
 import java.util.ArrayList;
 import java.util.List;
 
@@ -136,7 +139,16 @@ void runMigrations(int fromVersion, int toVersion) {
     }
 
     if (fromVersion < 9 && toVersion >= 9) {
-      addReadTime();
+      if (!hasReadTime()) {
+        addReadTime();
+      } else {
+        // Index-free queries rely on the fact that documents updated after a query's last limbo
+        // free snapshot version are persisted with their read-time. If a customer upgrades to
+        // schema version 9, downgrades and then upgrades again, some queries may have a last limbo
+        // free snapshot version despite the fact that not all updated document have an associated
+        // read time.
+        dropLastLimboFreeSnapshotVersion();
+      }
     }
 
     /*
@@ -363,14 +375,39 @@ private void addSequenceNumber() {
     }
   }
 
+  private boolean hasReadTime() {
+    boolean hasReadTimeSeconds = tableContainsColumn("remote_documents", "read_time_seconds");
+    boolean hasReadTimeNanos = tableContainsColumn("remote_documents", "read_time_nanos");
+
+    hardAssert(
+        hasReadTimeSeconds == hasReadTimeNanos,
+        "Table contained just one of read_time_seconds or read_time_nanos");
+
+    return hasReadTimeSeconds && hasReadTimeNanos;
+  }
+
   private void addReadTime() {
-    if (!tableContainsColumn("remote_documents", "read_time_seconds")) {
-      hardAssert(
-          !tableContainsColumn("remote_documents", "read_time_nanos"),
-          "Table contained read_time_nanos, but is missing read_time_seconds");
-      db.execSQL("ALTER TABLE remote_documents ADD COLUMN read_time_seconds INTEGER");
-      db.execSQL("ALTER TABLE remote_documents ADD COLUMN read_time_nanos INTEGER");
-    }
+    db.execSQL("ALTER TABLE remote_documents ADD COLUMN read_time_seconds INTEGER");
+    db.execSQL("ALTER TABLE remote_documents ADD COLUMN read_time_nanos INTEGER");
+  }
+
+  private void dropLastLimboFreeSnapshotVersion() {
+    new SQLitePersistence.Query(db, "SELECT target_id, target_proto FROM targets")
+        .forEach(
+            cursor -> {
+              int targetId = cursor.getInt(0);
+              byte[] targetProtoBytes = cursor.getBlob(1);
+
+              try {
+                Target targetProto = Target.parseFrom(targetProtoBytes);
+                targetProto = targetProto.toBuilder().clearLastLimboFreeSnapshotVersion().build();
+                db.execSQL(
+                    "UPDATE targets SET target_proto = ? WHERE target_id = ?",
+                    new Object[] {targetProto.toByteArray(), targetId});
+              } catch (InvalidProtocolBufferException e) {
+                throw fail("Failed to decode Query data for target %s", targetId);
+              }
+            });
   }
 
   /**

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

@@ -21,6 +21,7 @@
 import static com.google.firebase.firestore.testutil.TestUtil.path;
 import static com.google.firebase.firestore.testutil.TestUtil.query;
 import static com.google.firebase.firestore.testutil.TestUtil.version;
+import static com.google.firebase.firestore.util.Assert.fail;
 import static java.util.Arrays.asList;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
@@ -36,10 +37,13 @@
 import com.google.firebase.firestore.model.DocumentKey;
 import com.google.firebase.firestore.model.ResourcePath;
 import com.google.firebase.firestore.proto.MaybeDocument;
+import com.google.firebase.firestore.proto.Target;
 import com.google.firebase.firestore.proto.WriteBatch;
 import com.google.firebase.firestore.remote.RemoteSerializer;
 import com.google.firestore.v1.Document;
 import com.google.firestore.v1.Write;
+import com.google.protobuf.InvalidProtocolBufferException;
+import com.google.protobuf.Timestamp;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.HashSet;
@@ -436,6 +440,68 @@ public void existingDocumentsRemainReadableAfterIndexFreeMigration() {
     assertResultsContain(results, "coll/new");
   }
 
+  @Test
+  public void dropsLastLimboFreeSnapshotIfPreviouslyDowngraded() {
+    schema.runMigrations(0, 9);
+
+    db.execSQL(
+        "INSERT INTO targets (target_id, canonical_id, target_proto) VALUES (?,?, ?)",
+        new Object[] {1, "foo", createDummyQueryTargetWithLimboFreeVersion(1).toByteArray()});
+    db.execSQL(
+        "INSERT INTO targets (target_id, canonical_id, target_proto) VALUES (?, ?, ?)",
+        new Object[] {2, "bar", createDummyQueryTargetWithLimboFreeVersion(2).toByteArray()});
+    db.execSQL(
+        "INSERT INTO targets (target_id, canonical_id, target_proto) VALUES (?,?, ?)",
+        new Object[] {3, "baz", createDummyQueryTargetWithLimboFreeVersion(3).toByteArray()});
+
+    schema.runMigrations(0, 8);
+    schema.runMigrations(8, 9);
+
+    int rowCount =
+        new SQLitePersistence.Query(db, "SELECT target_id, target_proto FROM targets")
+            .forEach(
+                cursor -> {
+                  int targetId = cursor.getInt(0);
+                  byte[] targetProtoBytes = cursor.getBlob(1);
+
+                  try {
+                    Target targetProto = Target.parseFrom(targetProtoBytes);
+                    assertEquals(targetId, targetProto.getTargetId());
+                    assertFalse(targetProto.hasLastLimboFreeSnapshotVersion());
+                  } catch (InvalidProtocolBufferException e) {
+                    fail("Failed to decode Query data");
+                  }
+                });
+
+    assertEquals(3, rowCount);
+  }
+
+  @Test
+  public void keepsLastLimboFreeSnapshotIfNotDowngraded() {
+    schema.runMigrations(0, 9);
+
+    db.execSQL(
+        "INSERT INTO targets (target_id, canonical_id, target_proto) VALUES (?,?, ?)",
+        new Object[] {1, "foo", createDummyQueryTargetWithLimboFreeVersion(1).toByteArray()});
+
+    // Make sure that we don't drop the lastLimboFreeSnapshotVersion if we are already on schema
+    // version 9.
+    schema.runMigrations(9, 9);
+
+    new SQLitePersistence.Query(db, "SELECT target_proto FROM targets")
+        .forEach(
+            cursor -> {
+              byte[] targetProtoBytes = cursor.getBlob(0);
+
+              try {
+                Target targetProto = Target.parseFrom(targetProtoBytes);
+                assertTrue(targetProto.hasLastLimboFreeSnapshotVersion());
+              } catch (InvalidProtocolBufferException e) {
+                fail("Failed to decode Query data");
+              }
+            });
+  }
+
   private SQLiteRemoteDocumentCache createRemoteDocumentCache() {
     DatabaseId databaseId = DatabaseId.forProject("foo");
     LocalSerializer serializer = new LocalSerializer(new RemoteSerializer(databaseId));
@@ -460,6 +526,13 @@ private byte[] createDummyDocument(String name) {
         .toByteArray();
   }
 
+  private Target createDummyQueryTargetWithLimboFreeVersion(int targetId) {
+    return Target.newBuilder()
+        .setTargetId(targetId)
+        .setLastLimboFreeSnapshotVersion(Timestamp.newBuilder().setSeconds(42))
+        .build();
+  }
+
   private void assertResultsContain(
       ImmutableSortedMap<DocumentKey, com.google.firebase.firestore.model.Document> actualResults,
       String... docs) {