Simpler fix

Author: wu-hui

firebase-firestore/CHANGELOG.md

@@ -2,6 +2,9 @@ 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).
 
+# 24.1.0
+- [fixed] Fixed missing document fields issue with offline overlays (#3528)
+
 # 24.0.2
 - [fixed] Fixed an issue of long grpc reconnection period, when App moves to
   foreground after staying in background for a while.

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

@@ -122,19 +122,6 @@ ImmutableSortedMap<DocumentKey, Document> getLocalViewOfDocuments(
     return computeViews(docs, overlays, existenceStateChanged);
   }
 
-  /**
-   * Similar to {@link #getDocuments}, but creates the local view from the given {@code baseDocs}
-   * without retrieving documents from the local store.
-   *
-   * @param docs The documents to apply local mutations to get the local views.
-   */
-  ImmutableSortedMap<DocumentKey, Document> getLocalViewOfDocuments(
-      Map<DocumentKey, MutableDocument> docs) {
-    Map<DocumentKey, Overlay> overlays = new HashMap<>();
-    populateOverlays(overlays, docs.keySet());
-    return computeViews(docs, overlays, new HashSet<>());
-  }
-
   /*Computes the local view for doc */
   private ImmutableSortedMap<DocumentKey, Document> computeViews(
       Map<DocumentKey, MutableDocument> docs,

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

@@ -244,20 +244,9 @@ public LocalDocumentsResult writeLocally(List<Mutation> mutations) {
     return persistence.runTransaction(
         "Locally write mutations",
         () -> {
-          // Figure out which keys do not have a remote version in the cache, this is needed to
-          // create the right overlay mutation: if no remote version presents, we do not need to
-          // create overlays as patch mutations.
-          Map<DocumentKey, MutableDocument> remoteDocs = remoteDocuments.getAll(keys);
-          Set<DocumentKey> docsWithoutRemoteVersion = new HashSet<>();
-          for (Map.Entry<DocumentKey, MutableDocument> entry : remoteDocs.entrySet()) {
-            if (!entry.getValue().isValidDocument()) {
-              docsWithoutRemoteVersion.add(entry.getKey());
-            }
-          }
           // Load and apply all existing mutations. This lets us compute the current base state for
           // all non-idempotent transforms before applying any additional user-provided writes.
-          ImmutableSortedMap<DocumentKey, Document> documents =
-              localDocuments.getLocalViewOfDocuments(remoteDocs);
+          ImmutableSortedMap<DocumentKey, Document> documents = localDocuments.getDocuments(keys);
 
           // For non-idempotent mutations (such as `FieldValue.increment()`), we record the base
           // state in a separate patch mutation. This is later used to guarantee consistent values
@@ -281,8 +270,7 @@ public LocalDocumentsResult writeLocally(List<Mutation> mutations) {
 
           MutationBatch batch =
               mutationQueue.addMutationBatch(localWriteTime, baseMutations, mutations);
-          Map<DocumentKey, Mutation> overlays =
-              batch.applyToLocalDocumentSet(documents, docsWithoutRemoteVersion);
+          Map<DocumentKey, Mutation> overlays = batch.applyToLocalDocumentSet(documents);
           documentOverlayCache.saveOverlays(batch.getBatchId(), overlays);
           return new LocalDocumentsResult(batch.getBatchId(), documents);
         });

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

@@ -23,6 +23,7 @@
 import com.google.firebase.firestore.model.FieldPath;
 import com.google.firebase.firestore.model.MutableDocument;
 import com.google.firebase.firestore.model.ObjectValue;
+import com.google.firebase.firestore.model.SnapshotVersion;
 import com.google.firestore.v1.Value;
 import java.util.ArrayList;
 import java.util.HashMap;
@@ -93,7 +94,7 @@ public static Mutation calculateOverlayMutation(MutableDocument doc, @Nullable F
     }
 
     // mask == null when there are Set or Delete being applied to get to the current document.
-    if (mask == null) {
+    if (mask == null || doc.getVersion().equals(SnapshotVersion.NONE)) {
       if (doc.isNoDocument()) {
         return new DeleteMutation(doc.getKey(), Precondition.NONE);
       } else {

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

@@ -136,8 +136,7 @@ public FieldMask applyToLocalView(MutableDocument document, @Nullable FieldMask
    * applications.
    */
   public Map<DocumentKey, Mutation> applyToLocalDocumentSet(
-      ImmutableSortedMap<DocumentKey, Document> documentMap,
-      Set<DocumentKey> documentsWithoutRemoteVersion) {
+      ImmutableSortedMap<DocumentKey, Document> documentMap) {
     // TODO(mrschmidt): This implementation is O(n^2). If we iterate through the mutations first
     // (as done in `applyToLocalView(MutableDocument d)`), we can reduce the complexity to
     // O(n).
@@ -147,9 +146,6 @@ public Map<DocumentKey, Mutation> applyToLocalDocumentSet(
       // remove this cast.
       MutableDocument document = (MutableDocument) documentMap.get(key);
       FieldMask mutatedFields = applyToLocalView(document);
-      // Set mutationFields to null if the document is only from local mutations, this creates
-      // a Set(or Delete) mutation, instead of trying to create a patch mutation as the overlay.
-      mutatedFields = documentsWithoutRemoteVersion.contains(key) ? null : mutatedFields;
       Mutation overlay = Mutation.calculateOverlayMutation(document, mutatedFields);
       overlays.put(key, overlay);
       if (!document.isValidDocument()) {

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

@@ -759,7 +759,7 @@ public void testHandlesDeleteMutationThenPatchMutationThenAckThenAck() {
 
     acknowledgeMutation(2); // delete mutation
     assertRemoved("foo/bar");
-    assertContains(deletedDoc("foo/bar", 0).setHasLocalMutations());
+    assertContains(deletedDoc("foo/bar", 2).setHasCommittedMutations());
 
     acknowledgeMutation(3); // patch mutation
     assertChanged(unknownDoc("foo/bar", 3));