Remove getAll change and use None for local mutations

Author: wu-hui

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

@@ -26,12 +26,10 @@
 import com.google.firebase.firestore.model.FieldIndex.IndexOffset;
 import com.google.firebase.firestore.model.MutableDocument;
 import com.google.firebase.firestore.model.ResourcePath;
-import com.google.firebase.firestore.model.mutation.DeleteMutation;
 import com.google.firebase.firestore.model.mutation.FieldMask;
 import com.google.firebase.firestore.model.mutation.Mutation;
 import com.google.firebase.firestore.model.mutation.MutationBatch;
 import com.google.firebase.firestore.model.mutation.PatchMutation;
-import com.google.firebase.firestore.model.mutation.SetMutation;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
@@ -259,18 +257,9 @@ private ImmutableSortedMap<DocumentKey, Document> getDocumentsMatchingCollection
   /** Queries the remote documents and overlays by doing a full collection scan. */
   private ImmutableSortedMap<DocumentKey, Document> getDocumentsMatchingCollectionQuery(
       Query query, IndexOffset offset) {
-    Map<DocumentKey, Mutation> overlays = documentOverlayCache.getOverlays(query.getPath(), -1);
-
-    // Remote document elision: avoid reading remote documents if their overlay is Set or Delete.
-    Set<DocumentKey> ignore = new HashSet<>();
-    for (Map.Entry<DocumentKey, Mutation> entry : overlays.entrySet()) {
-      if (entry.getValue() instanceof SetMutation || entry.getValue() instanceof DeleteMutation) {
-        ignore.add(entry.getKey());
-      }
-    }
-
     Map<DocumentKey, MutableDocument> remoteDocuments =
-        remoteDocumentCache.getAll(query.getPath(), offset, ignore);
+        remoteDocumentCache.getAll(query.getPath(), offset);
+    Map<DocumentKey, Mutation> overlays = documentOverlayCache.getOverlays(query.getPath(), -1);
 
     // As documents might match the query because of their overlay we need to include documents
     // for all overlays in the initial document set.

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

@@ -94,7 +94,7 @@ private com.google.firestore.v1.Document encodeDocument(Document document) {
         com.google.firestore.v1.Document.newBuilder();
     builder.setName(rpcSerializer.encodeKey(document.getKey()));
     builder.putAllFields(document.getData().getFieldsMap());
-    Timestamp updateTime = document.getInternalReference().getVersion().getTimestamp();
+    Timestamp updateTime = document.getVersion().getTimestamp();
     builder.setUpdateTime(rpcSerializer.encodeTimestamp(updateTime));
     return builder.build();
   }
@@ -115,8 +115,7 @@ private com.google.firebase.firestore.proto.NoDocument encodeNoDocument(Document
     com.google.firebase.firestore.proto.NoDocument.Builder builder =
         com.google.firebase.firestore.proto.NoDocument.newBuilder();
     builder.setName(rpcSerializer.encodeKey(document.getKey()));
-    builder.setReadTime(
-        rpcSerializer.encodeTimestamp(document.getInternalReference().getVersion().getTimestamp()));
+    builder.setReadTime(rpcSerializer.encodeTimestamp(document.getVersion().getTimestamp()));
     return builder.build();
   }
 
@@ -135,8 +134,7 @@ private com.google.firebase.firestore.proto.UnknownDocument encodeUnknownDocumen
     com.google.firebase.firestore.proto.UnknownDocument.Builder builder =
         com.google.firebase.firestore.proto.UnknownDocument.newBuilder();
     builder.setName(rpcSerializer.encodeKey(document.getKey()));
-    builder.setVersion(
-        rpcSerializer.encodeTimestamp(document.getInternalReference().getVersion().getTimestamp()));
+    builder.setVersion(rpcSerializer.encodeTimestamp(document.getVersion().getTimestamp()));
     return builder.build();
   }
 

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

@@ -29,7 +29,6 @@
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.Map;
-import java.util.Set;
 
 /** In-memory cache of remote documents. */
 final class MemoryRemoteDocumentCache implements RemoteDocumentCache {
@@ -79,9 +78,7 @@ public void removeAll(Collection<DocumentKey> keys) {
   @Override
   public MutableDocument get(DocumentKey key) {
     Document doc = docs.get(key);
-    return doc != null
-        ? doc.getInternalReference().mutableCopy()
-        : MutableDocument.newInvalidDocument(key);
+    return doc != null ? doc.mutableCopy() : MutableDocument.newInvalidDocument(key);
   }
 
   @Override
@@ -101,8 +98,7 @@ public Map<DocumentKey, MutableDocument> getAll(
   }
 
   @Override
-  public Map<DocumentKey, MutableDocument> getAll(
-      ResourcePath collection, IndexOffset offset, Set<DocumentKey> ignoreSet) {
+  public Map<DocumentKey, MutableDocument> getAll(ResourcePath collection, IndexOffset offset) {
     Map<DocumentKey, MutableDocument> result = new HashMap<>();
 
     // Documents are ordered by key, so we can use a prefix scan to narrow down the documents
@@ -112,10 +108,6 @@ public Map<DocumentKey, MutableDocument> getAll(
 
     while (iterator.hasNext()) {
       Map.Entry<DocumentKey, Document> entry = iterator.next();
-      if (ignoreSet.contains(entry.getKey())) {
-        continue;
-      }
-
       Document doc = entry.getValue();
 
       DocumentKey key = entry.getKey();
@@ -134,7 +126,7 @@ public Map<DocumentKey, MutableDocument> getAll(
         continue;
       }
 
-      result.put(doc.getKey(), doc.getInternalReference().mutableCopy());
+      result.put(doc.getKey(), doc.mutableCopy());
     }
 
     return result;

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

@@ -215,11 +215,7 @@ private boolean needsRefill(
       return false;
     }
     return documentAtLimitEdge.hasPendingWrites()
-        || documentAtLimitEdge
-                .getInternalReference()
-                .getVersion()
-                .compareTo(limboFreeSnapshotVersion)
-            > 0;
+        || documentAtLimitEdge.getVersion().compareTo(limboFreeSnapshotVersion) > 0;
   }
 
   private ImmutableSortedMap<DocumentKey, Document> executeFullCollectionScan(Query query) {

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

@@ -21,7 +21,6 @@
 import com.google.firebase.firestore.model.SnapshotVersion;
 import java.util.Collection;
 import java.util.Map;
-import java.util.Set;
 
 /**
  * Represents cached documents received from the remote backend.
@@ -82,11 +81,9 @@ interface RemoteDocumentCache {
    *
    * @param collection The collection to read.
    * @param offset The read time and document key to start scanning at (exclusive).
-   * @param ignoreSet The set of document keys to ignore from the result.
    * @return A newly created map with the set of documents in the collection.
    */
-  Map<DocumentKey, MutableDocument> getAll(
-      ResourcePath collection, IndexOffset offset, Set<DocumentKey> ignoreSet);
+  Map<DocumentKey, MutableDocument> getAll(ResourcePath collection, IndexOffset offset);
 
   /** Returns the latest read time of any document in the cache. */
   SnapshotVersion getLatestReadTime();

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

@@ -37,10 +37,8 @@
 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.Set;
 import java.util.concurrent.Executor;
 
 final class SQLiteRemoteDocumentCache implements RemoteDocumentCache {
@@ -166,18 +164,15 @@ public Map<DocumentKey, MutableDocument> getAll(
     if (collections.isEmpty()) {
       return Collections.emptyMap();
     } else if (BINDS_PER_STATEMENT * collections.size() < SQLitePersistence.MAX_ARGS) {
-      return getAll(collections, offset, limit, Collections.emptySet());
+      return getAll(collections, offset, limit);
     } else {
       // We need to fan out our collection scan since SQLite only supports 999 binds per statement.
       Map<DocumentKey, MutableDocument> results = new HashMap<>();
       int pageSize = SQLitePersistence.MAX_ARGS / BINDS_PER_STATEMENT;
       for (int i = 0; i < collections.size(); i += pageSize) {
         results.putAll(
             getAll(
-                collections.subList(i, Math.min(collections.size(), i + pageSize)),
-                offset,
-                limit,
-                new HashSet<>()));
+                collections.subList(i, Math.min(collections.size(), i + pageSize)), offset, limit));
       }
       return firstNEntries(results, limit, IndexOffset.DOCUMENT_COMPARATOR);
     }
@@ -187,7 +182,7 @@ public Map<DocumentKey, MutableDocument> getAll(
    * Returns the next {@code count} documents from the provided collections, ordered by read time.
    */
   private Map<DocumentKey, MutableDocument> getAll(
-      List<ResourcePath> collections, IndexOffset offset, int count, Set<DocumentKey> ignoreSet) {
+      List<ResourcePath> collections, IndexOffset offset, int count) {
     Timestamp readTime = offset.getReadTime().getTimestamp();
     DocumentKey documentKey = offset.getDocumentKey();
 
@@ -223,18 +218,10 @@ private Map<DocumentKey, MutableDocument> getAll(
     Map<DocumentKey, MutableDocument>[] results =
         (HashMap<DocumentKey, MutableDocument>[]) (new HashMap[] {new HashMap()});
 
-    Set<String> ignoredPath = new HashSet<>();
-    for (DocumentKey key : ignoreSet) {
-      ignoredPath.add(EncodedPath.encode(key.getPath()));
-    }
     db.query(sql.toString())
         .binding(bindVars)
         .forEach(
             row -> {
-              if (ignoredPath.contains(row.getString(3))) {
-                return;
-              }
-
               // Store row values in array entries to provide the correct context inside the
               // executor.
               final byte[] rawDocument = row.getBlob(0);
@@ -264,9 +251,8 @@ private Map<DocumentKey, MutableDocument> getAll(
   }
 
   @Override
-  public Map<DocumentKey, MutableDocument> getAll(
-      ResourcePath collection, IndexOffset offset, Set<DocumentKey> ignoreSet) {
-    return getAll(Collections.singletonList(collection), offset, Integer.MAX_VALUE, ignoreSet);
+  public Map<DocumentKey, MutableDocument> getAll(ResourcePath collection, IndexOffset offset) {
+    return getAll(Collections.singletonList(collection), offset, Integer.MAX_VALUE);
   }
 
   @Override

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

@@ -19,8 +19,8 @@
 import java.util.Comparator;
 
 /**
- * Represents a document in Firestore with a key, data and whether the data has local mutations
- * applied to it.
+ * Represents a document in Firestore with a key, version, data and whether the data has local
+ * mutations applied to it.
  */
 public interface Document {
   /** A document comparator that returns document by key and key only. */
@@ -29,6 +29,18 @@ public interface Document {
   /** The key for this document */
   DocumentKey getKey();
 
+  /**
+   * Returns the version of this document if it exists or a version at which this document was
+   * guaranteed to not exist.
+   */
+  SnapshotVersion getVersion();
+
+  /**
+   * Returns the timestamp at which this document was read from the remote server. Returns
+   * `SnapshotVersion.NONE` for documents created by the user.
+   */
+  SnapshotVersion getReadTime();
+
   /**
    * Returns whether this document is valid (i.e. it is an entry in the RemoteDocumentCache, was
    * created by a mutation or read from the backend).
@@ -62,5 +74,6 @@ public interface Document {
    */
   boolean hasPendingWrites();
 
-  InternalDocument getInternalReference();
+  /** Creates a mutable copy of this document. */
+  MutableDocument mutableCopy();
 }

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

@@ -150,8 +150,7 @@ public static IndexOffset create(SnapshotVersion readTime) {
 
     /** Creates a new offset based on the provided document. */
     public static IndexOffset fromDocument(Document document) {
-      return new AutoValue_FieldIndex_IndexOffset(
-          document.getInternalReference().getReadTime(), document.getKey());
+      return new AutoValue_FieldIndex_IndexOffset(document.getReadTime(), document.getKey());
     }
 
     /**

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

@@ -26,7 +26,7 @@
  * one of these states even after all mutations have been applied, {@link #isValidDocument} returns
  * false and the document should be removed from all views.
  */
-public final class MutableDocument implements InternalDocument {
+public final class MutableDocument implements Document {
 
   private enum DocumentType {
     /**
@@ -159,6 +159,7 @@ public MutableDocument setHasCommittedMutations() {
 
   public MutableDocument setHasLocalMutations() {
     this.documentState = DocumentState.HAS_LOCAL_MUTATIONS;
+    this.version = SnapshotVersion.NONE;
     return this;
   }
 
@@ -197,11 +198,6 @@ public boolean hasPendingWrites() {
     return hasLocalMutations() || hasCommittedMutations();
   }
 
-  @Override
-  public InternalDocument getInternalReference() {
-    return (InternalDocument) this;
-  }
-
   @Override
   public ObjectValue getData() {
     return value;

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

@@ -31,7 +31,6 @@
 import java.util.Collection;
 import java.util.List;
 import java.util.Map;
-import java.util.Set;
 
 /**
  * A test-only QueryEngine that forwards all API calls and exposes the number of documents and
@@ -156,9 +155,8 @@ public Map<DocumentKey, MutableDocument> getAll(
       }
 
       @Override
-      public Map<DocumentKey, MutableDocument> getAll(
-          ResourcePath collection, IndexOffset offset, Set<DocumentKey> ignoreSet) {
-        Map<DocumentKey, MutableDocument> result = subject.getAll(collection, offset, ignoreSet);
+      public Map<DocumentKey, MutableDocument> getAll(ResourcePath collection, IndexOffset offset) {
+        Map<DocumentKey, MutableDocument> result = subject.getAll(collection, offset);
         documentsReadByCollection[0] += result.size();
         return result;
       }