Release Overlay (#3112)

Author: wu-hui

firebase-firestore/CHANGELOG.md

@@ -2,7 +2,8 @@ Android changes are not released automatically. Ensure that changes are released
 by opting into a release at 
 [go/firebase-android-release](http:go/firebase-android-release) (Googlers only).
 
-# Unreleased
+# 24.1.0
+- [changed] Performance optimization for offline usage.
 - [changed] Improved performance for queries with collections that contain
   subcollections.
 

firebase-firestore/firebase-firestore.gradle

@@ -112,13 +112,6 @@ android.libraryVariants.all { variant ->
     } else {
         variant.buildConfigField("boolean", "ENABLE_INDEXING", "false")
     }
-
-    // TODO(Overlay): Delete below once overlay is shipped.
-    if (localProps['firestoreEnableOverlay']) {
-        variant.buildConfigField("boolean", "ENABLE_OVERLAY", "true")
-    } else {
-        variant.buildConfigField("boolean", "ENABLE_OVERLAY", "false")
-    }
 }
 
 configurations.all {

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

@@ -255,8 +255,6 @@ public static FirebaseFirestore testFirestore(
     // This unfortunately is a global setting that affects existing Firestore clients.
     Logger.setLogLevel(logLevel);
 
-    // TODO(Overlay): Remove below once this is ready to ship.
-    Persistence.OVERLAY_SUPPORT_ENABLED = true;
     Persistence.INDEXING_SUPPORT_ENABLED = true;
 
     Context context = ApplicationProvider.getApplicationContext();

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

@@ -203,7 +203,8 @@ private void recalculateAndSaveOverlays(Map<DocumentKey, MutableDocument> docs)
     // along the way.
     for (MutationBatch batch : batches) {
       for (DocumentKey key : batch.getKeys()) {
-        FieldMask mask = batch.applyToLocalView(docs.get(key), masks.get(key));
+        FieldMask mask = masks.containsKey(key) ? masks.get(key) : FieldMask.EMPTY;
+        mask = batch.applyToLocalView(docs.get(key), mask);
         masks.put(key, mask);
         int batchId = batch.getBatchId();
         if (!documentsByBatchId.containsKey(batchId)) {

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

@@ -52,7 +52,7 @@ public abstract class Persistence {
 
   /** Temporary setting for enabling document overlays. */
   // TODO(Overlay): Remove this.
-  public static boolean OVERLAY_SUPPORT_ENABLED = BuildConfig.ENABLE_OVERLAY;
+  public static boolean OVERLAY_SUPPORT_ENABLED = true;
   /** Constant string to indicate a data migration is required to support overlays. */
   public static String DATA_MIGRATION_BUILD_OVERLAYS = "BUILD_OVERLAYS";
 

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

@@ -49,12 +49,10 @@ class SQLiteSchema {
    * The version of the schema. Increase this by one for each migration added to runMigrations
    * below.
    */
-  static final int VERSION = 13;
-
-  static final int OVERLAY_SUPPORT_VERSION = VERSION + 1;
+  static final int VERSION = 14;
 
   // TODO(indexing): Remove this constant and increment VERSION to enable indexing support
-  static final int INDEXING_SUPPORT_VERSION = OVERLAY_SUPPORT_VERSION + 1;
+  static final int INDEXING_SUPPORT_VERSION = VERSION + 1;
 
   /**
    * The batch size for data migrations.
@@ -75,14 +73,11 @@ class SQLiteSchema {
   }
 
   void runSchemaUpgrades() {
-    runSchemaUpgrades(0, VERSION);
+    runSchemaUpgrades(0);
   }
 
   void runSchemaUpgrades(int fromVersion) {
     int toVersion = VERSION;
-    if (Persistence.OVERLAY_SUPPORT_ENABLED) {
-      toVersion = OVERLAY_SUPPORT_VERSION;
-    }
     if (Persistence.INDEXING_SUPPORT_ENABLED) {
       toVersion = INDEXING_SUPPORT_VERSION;
     }
@@ -178,8 +173,16 @@ void runSchemaUpgrades(int fromVersion, int toVersion) {
       ensurePathLength();
     }
 
+    if (fromVersion < 14 && toVersion >= 14) {
+      Preconditions.checkState(
+          Persistence.OVERLAY_SUPPORT_ENABLED || Persistence.INDEXING_SUPPORT_ENABLED);
+      createOverlays();
+      createDataMigrationTable();
+      addPendingDataMigration(Persistence.DATA_MIGRATION_BUILD_OVERLAYS);
+    }
+
     /*
-     * Adding a new migration? READ THIS FIRST!
+     * Adding a new schema upgrade? READ THIS FIRST!
      *
      * Be aware that the SDK version may be downgraded then re-upgraded. This means that running
      * your new migration must not prevent older versions of the SDK from functioning. Additionally,
@@ -190,13 +193,6 @@ void runSchemaUpgrades(int fromVersion, int toVersion) {
      *    maintained invariants from later versions, so migrations that update values cannot assume
      *    that existing values have been properly maintained. Calculate them again, if applicable.
      */
-    if (fromVersion < OVERLAY_SUPPORT_VERSION && toVersion >= OVERLAY_SUPPORT_VERSION) {
-      Preconditions.checkState(
-          Persistence.OVERLAY_SUPPORT_ENABLED || Persistence.INDEXING_SUPPORT_ENABLED);
-      createOverlays();
-      createDataMigrationTable();
-      addPendingDataMigration(Persistence.DATA_MIGRATION_BUILD_OVERLAYS);
-    }
 
     if (fromVersion < INDEXING_SUPPORT_VERSION && toVersion >= INDEXING_SUPPORT_VERSION) {
       Preconditions.checkState(Persistence.INDEXING_SUPPORT_ENABLED);
@@ -708,7 +704,9 @@ private void createDataMigrationTable() {
   }
 
   private void addPendingDataMigration(String migration) {
-    db.execSQL("INSERT INTO data_migrations (migration_name) VALUES (?)", new String[] {migration});
+    db.execSQL(
+        "INSERT OR IGNORE INTO data_migrations (migration_name) VALUES (?)",
+        new String[] {migration});
   }
 
   private boolean tableExists(String table) {

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

@@ -15,6 +15,7 @@
 package com.google.firebase.firestore.model.mutation;
 
 import com.google.firebase.firestore.model.FieldPath;
+import java.util.HashSet;
 import java.util.Set;
 
 /**
@@ -26,6 +27,8 @@
  * object foo. If foo is not an object, foo is replaced with an object containing foo.
  */
 public final class FieldMask {
+  public static FieldMask EMPTY = fromSet(new HashSet<>());
+
   public static FieldMask fromSet(Set<FieldPath> mask) {
     return new FieldMask(mask);
   }

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

@@ -30,7 +30,6 @@
 import java.util.Map;
 import org.junit.After;
 import org.junit.Before;
-import org.junit.BeforeClass;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.TestName;
@@ -52,17 +51,6 @@ public abstract class DocumentOverlayCacheTestCase {
   private DocumentOverlayCache overlays;
   private static boolean overlayEnabled = false;
 
-  @BeforeClass
-  public static void beforeClass() {
-    overlayEnabled = Persistence.OVERLAY_SUPPORT_ENABLED;
-    Persistence.OVERLAY_SUPPORT_ENABLED = true;
-  }
-
-  @BeforeClass
-  public static void afterClass() {
-    Persistence.OVERLAY_SUPPORT_ENABLED = overlayEnabled;
-  }
-
   @Before
   public void setUp() {
     persistence = getPersistence();

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

@@ -38,7 +38,6 @@
 import static com.google.firebase.firestore.testutil.TestUtil.viewChanges;
 import static java.util.Arrays.asList;
 import static java.util.Collections.emptyList;
-import static java.util.Collections.singletonList;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
@@ -75,16 +74,17 @@
 import com.google.firebase.firestore.testutil.TestUtil;
 import com.google.firebase.firestore.util.AsyncQueue;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map.Entry;
 import java.util.Set;
+import java.util.stream.Collectors;
 import javax.annotation.Nullable;
 import org.junit.After;
 import org.junit.Assert;
 import org.junit.Before;
-import org.junit.Ignore;
 import org.junit.Test;
 
 /**
@@ -155,23 +155,27 @@ private void udpateViews(int targetId, boolean fromCache) {
     notifyLocalViewChanges(viewChanges(targetId, fromCache, asList(), asList()));
   }
 
-  private void acknowledgeMutation(long documentVersion, @Nullable Object transformResult) {
+  private void acknowledgeMutationWithTransformResults(
+      long documentVersion, Object... transformResult) {
     MutationBatch batch = batches.remove(0);
     SnapshotVersion version = version(documentVersion);
-    MutationResult mutationResult =
-        new MutationResult(
-            version,
-            transformResult != null
-                ? Collections.singletonList(TestUtil.wrap(transformResult))
-                : Collections.emptyList());
+    List<MutationResult> mutationResults =
+        Collections.singletonList(new MutationResult(version, emptyList()));
+
+    if (transformResult.length != 0) {
+      mutationResults =
+          Arrays.stream(transformResult)
+              .map(r -> new MutationResult(version, Collections.singletonList(TestUtil.wrap(r))))
+              .collect(Collectors.toList());
+    }
+
     MutationBatchResult result =
-        MutationBatchResult.create(
-            batch, version, singletonList(mutationResult), WriteStream.EMPTY_STREAM_TOKEN);
+        MutationBatchResult.create(batch, version, mutationResults, WriteStream.EMPTY_STREAM_TOKEN);
     lastChanges = localStore.acknowledgeBatch(result);
   }
 
   private void acknowledgeMutation(long documentVersion) {
-    acknowledgeMutation(documentVersion, null);
+    acknowledgeMutationWithTransformResults(documentVersion);
   }
 
   private void rejectMutation() {
@@ -974,7 +978,10 @@ public void testReadsAllDocumentsForInitialCollectionQueries() {
     localStore.executeQuery(query, /* usePreviousResults= */ true);
 
     assertRemoteDocumentsRead(/* byKey= */ 0, /* byQuery= */ 2);
-    if (!Persistence.OVERLAY_SUPPORT_ENABLED) {
+    if (Persistence.OVERLAY_SUPPORT_ENABLED) {
+      // No mutations are read because only overlay is needed.
+      assertMutationsRead(/* byKey= */ 0, /* byQuery= */ 0);
+    } else {
       assertMutationsRead(/* byKey= */ 0, /* byQuery= */ 1);
     }
   }
@@ -1070,7 +1077,7 @@ public void testHandlesSetMutationThenAckThenTransformThenAckThenTransform() {
     assertContains(doc("foo/bar", 1, map("sum", 1)).setHasLocalMutations());
     assertChanged(doc("foo/bar", 1, map("sum", 1)).setHasLocalMutations());
 
-    acknowledgeMutation(2, 1);
+    acknowledgeMutationWithTransformResults(2, 1);
     assertChanged(doc("foo/bar", 2, map("sum", 1)).setHasCommittedMutations());
     assertContains(doc("foo/bar", 2, map("sum", 1)).setHasCommittedMutations());
 
@@ -1284,19 +1291,17 @@ public void testHandlesSetMutationThenTransformThenRemoteEventThenTransform() {
     assertChanged(doc("foo/bar", 2, map("sum", 3)).setHasLocalMutations());
     assertContains(doc("foo/bar", 2, map("sum", 3)).setHasLocalMutations());
 
-    acknowledgeMutation(3, 1);
+    acknowledgeMutationWithTransformResults(3, 1);
     assertChanged(doc("foo/bar", 3, map("sum", 3)).setHasLocalMutations());
     assertContains(doc("foo/bar", 3, map("sum", 3)).setHasLocalMutations());
 
-    acknowledgeMutation(4, 1339);
+    acknowledgeMutationWithTransformResults(4, 1339);
     assertChanged(doc("foo/bar", 4, map("sum", 1339)).setHasCommittedMutations());
     assertContains(doc("foo/bar", 4, map("sum", 1339)).setHasCommittedMutations());
   }
 
   @Test
-  @Ignore("Test fails in CI")
-  // TODO(Overlay): Fix me :)
-  public void testHoldsBackOnlyNonIdempotentTransforms() {
+  public void testHoldsBackTransforms() {
     Query query = Query.atPath(ResourcePath.fromString("foo"));
     allocateQuery(query);
     assertTargetId(2);
@@ -1333,8 +1338,13 @@ public void testHoldsBackOnlyNonIdempotentTransforms() {
             asList(2),
             emptyList()));
     assertChanged(
-        doc("foo/bar", 2, map("sum", 1, "array_union", asList("bar", "foo")))
-            .setHasLocalMutations());
+        doc("foo/bar", 2, map("sum", 1, "array_union", asList("foo"))).setHasLocalMutations());
+
+    acknowledgeMutationWithTransformResults(3, 1338, asList("bar", "foo"));
+    assertChanged(
+        doc("foo/bar", 3, map("sum", 1338, "array_union", asList("bar", "foo")))
+            .withReadTime(new SnapshotVersion(new Timestamp(0, 3000)))
+            .setHasCommittedMutations());
   }
 
   @Test

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

@@ -14,8 +14,6 @@
 
 package com.google.firebase.firestore.local;
 
-import org.junit.AfterClass;
-import org.junit.BeforeClass;
 import org.junit.runner.RunWith;
 import org.robolectric.RobolectricTestRunner;
 import org.robolectric.annotation.Config;
@@ -25,17 +23,6 @@
 public class MemoryLocalStoreTest extends LocalStoreTestCase {
   private static boolean enabled = false;
 
-  @BeforeClass
-  public static void beforeClass() {
-    enabled = Persistence.OVERLAY_SUPPORT_ENABLED;
-    Persistence.OVERLAY_SUPPORT_ENABLED = false;
-  }
-
-  @AfterClass
-  public static void afterClass() {
-    Persistence.OVERLAY_SUPPORT_ENABLED = enabled;
-  }
-
   @Override
   Persistence getPersistence() {
     return PersistenceTestHelpers.createEagerGCMemoryPersistence();