Use immutable Documents in MemoryRemoteDocumentCache (#3225)

Author: schmidt-sebastian

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

@@ -21,6 +21,7 @@
 import com.google.firebase.firestore.bundle.BundledQuery;
 import com.google.firebase.firestore.core.Query.LimitType;
 import com.google.firebase.firestore.core.Target;
+import com.google.firebase.firestore.model.Document;
 import com.google.firebase.firestore.model.DocumentKey;
 import com.google.firebase.firestore.model.FieldIndex;
 import com.google.firebase.firestore.model.FieldPath;
@@ -48,7 +49,7 @@ public LocalSerializer(RemoteSerializer rpcSerializer) {
   }
 
   /** Encodes a MaybeDocument model to the equivalent protocol buffer for local storage. */
-  com.google.firebase.firestore.proto.MaybeDocument encodeMaybeDocument(MutableDocument document) {
+  com.google.firebase.firestore.proto.MaybeDocument encodeMaybeDocument(Document document) {
     com.google.firebase.firestore.proto.MaybeDocument.Builder builder =
         com.google.firebase.firestore.proto.MaybeDocument.newBuilder();
 
@@ -88,7 +89,7 @@ MutableDocument decodeMaybeDocument(com.google.firebase.firestore.proto.MaybeDoc
    * Encodes a Document for local storage. This differs from the v1 RPC serializer for Documents in
    * that it preserves the updateTime, which is considered an output only value by the server.
    */
-  private com.google.firestore.v1.Document encodeDocument(MutableDocument document) {
+  private com.google.firestore.v1.Document encodeDocument(Document document) {
     com.google.firestore.v1.Document.Builder builder =
         com.google.firestore.v1.Document.newBuilder();
     builder.setName(rpcSerializer.encodeKey(document.getKey()));
@@ -110,8 +111,7 @@ private MutableDocument decodeDocument(
   }
 
   /** Encodes a NoDocument value to the equivalent proto. */
-  private com.google.firebase.firestore.proto.NoDocument encodeNoDocument(
-      MutableDocument document) {
+  private com.google.firebase.firestore.proto.NoDocument encodeNoDocument(Document document) {
     com.google.firebase.firestore.proto.NoDocument.Builder builder =
         com.google.firebase.firestore.proto.NoDocument.newBuilder();
     builder.setName(rpcSerializer.encodeKey(document.getKey()));
@@ -130,7 +130,7 @@ private MutableDocument decodeNoDocument(
 
   /** Encodes a UnknownDocument value to the equivalent proto. */
   private com.google.firebase.firestore.proto.UnknownDocument encodeUnknownDocument(
-      MutableDocument document) {
+      Document document) {
     com.google.firebase.firestore.proto.UnknownDocument.Builder builder =
         com.google.firebase.firestore.proto.UnknownDocument.newBuilder();
     builder.setName(rpcSerializer.encodeKey(document.getKey()));

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

@@ -34,14 +34,14 @@
 final class MemoryRemoteDocumentCache implements RemoteDocumentCache {
 
   /** Underlying cache of documents and their read times. */
-  private ImmutableSortedMap<DocumentKey, MutableDocument> docs;
+  private ImmutableSortedMap<DocumentKey, Document> docs;
   /** Manages the collection group index. */
   private IndexManager indexManager;
   /** The latest read time of any document in the cache. */
   private SnapshotVersion latestReadTime;
 
   MemoryRemoteDocumentCache() {
-    docs = ImmutableSortedMap.Builder.emptyMap(DocumentKey.comparator());
+    docs = emptyDocumentMap();
     latestReadTime = SnapshotVersion.NONE;
   }
 
@@ -56,7 +56,7 @@ public void add(MutableDocument document, SnapshotVersion readTime) {
     hardAssert(
         !readTime.equals(SnapshotVersion.NONE),
         "Cannot add document to the RemoteDocumentCache with a read time of zero");
-    docs = docs.insert(document.getKey(), document.clone().withReadTime(readTime));
+    docs = docs.insert(document.getKey(), document.mutableCopy().withReadTime(readTime));
     latestReadTime = readTime.compareTo(latestReadTime) > 0 ? readTime : latestReadTime;
 
     indexManager.addToCollectionParentIndex(document.getKey().getCollectionPath());
@@ -77,8 +77,8 @@ public void removeAll(Collection<DocumentKey> keys) {
 
   @Override
   public MutableDocument get(DocumentKey key) {
-    MutableDocument doc = docs.get(key);
-    return doc != null ? doc.clone() : MutableDocument.newInvalidDocument(key);
+    Document doc = docs.get(key);
+    return doc != null ? doc.mutableCopy() : MutableDocument.newInvalidDocument(key);
   }
 
   @Override
@@ -104,11 +104,11 @@ public Map<DocumentKey, MutableDocument> getAll(ResourcePath collection, IndexOf
     // Documents are ordered by key, so we can use a prefix scan to narrow down the documents
     // we need to match the query against.
     DocumentKey prefix = DocumentKey.fromPath(collection.append(""));
-    Iterator<Map.Entry<DocumentKey, MutableDocument>> iterator = docs.iteratorFrom(prefix);
+    Iterator<Map.Entry<DocumentKey, Document>> iterator = docs.iteratorFrom(prefix);
 
     while (iterator.hasNext()) {
-      Map.Entry<DocumentKey, MutableDocument> entry = iterator.next();
-      MutableDocument doc = entry.getValue();
+      Map.Entry<DocumentKey, Document> entry = iterator.next();
+      Document doc = entry.getValue();
 
       DocumentKey key = entry.getKey();
       if (!collection.isPrefixOf(key.getPath())) {
@@ -126,7 +126,7 @@ public Map<DocumentKey, MutableDocument> getAll(ResourcePath collection, IndexOf
         continue;
       }
 
-      result.put(doc.getKey(), doc.clone());
+      result.put(doc.getKey(), doc.mutableCopy());
     }
 
     return result;
@@ -137,13 +137,13 @@ public SnapshotVersion getLatestReadTime() {
     return latestReadTime;
   }
 
-  Iterable<MutableDocument> getDocuments() {
+  Iterable<Document> getDocuments() {
     return new DocumentIterable();
   }
 
   long getByteSize(LocalSerializer serializer) {
     long count = 0;
-    for (MutableDocument doc : new DocumentIterable()) {
+    for (Document doc : new DocumentIterable()) {
       count += serializer.encodeMaybeDocument(doc).getSerializedSize();
     }
     return count;
@@ -152,20 +152,20 @@ long getByteSize(LocalSerializer serializer) {
   /**
    * A proxy that exposes an iterator over the current set of documents in the RemoteDocumentCache.
    */
-  private class DocumentIterable implements Iterable<MutableDocument> {
+  private class DocumentIterable implements Iterable<Document> {
     @NonNull
     @Override
-    public Iterator<MutableDocument> iterator() {
-      Iterator<Map.Entry<DocumentKey, MutableDocument>> iterator =
+    public Iterator<Document> iterator() {
+      Iterator<Map.Entry<DocumentKey, Document>> iterator =
           MemoryRemoteDocumentCache.this.docs.iterator();
-      return new Iterator<MutableDocument>() {
+      return new Iterator<Document>() {
         @Override
         public boolean hasNext() {
           return iterator.hasNext();
         }
 
         @Override
-        public MutableDocument next() {
+        public Document next() {
           return iterator.next().getValue();
         }
       };

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

@@ -73,4 +73,7 @@ public interface Document {
    * Whether this document has a local mutation applied that has not yet been acknowledged by Watch.
    */
   boolean hasPendingWrites();
+
+  /** Creates a mutable copy of this document. */
+  MutableDocument mutableCopy();
 }

firebase-firestore/src/main/java/com/google/firebase/firestore/model/MutableDocument.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 Document, Cloneable {
+public final class MutableDocument implements Document {
 
   private enum DocumentType {
     /**
@@ -229,7 +229,7 @@ public boolean isUnknownDocument() {
 
   @Override
   @NonNull
-  public MutableDocument clone() {
+  public MutableDocument mutableCopy() {
     return new MutableDocument(key, documentType, version, readTime, value.clone(), documentState);
   }
 

firebase-firestore/src/test/java/com/google/firebase/firestore/model/mutation/MutationTest.java

@@ -564,7 +564,7 @@ private void assertVersionTransitions(
       MutableDocument base,
       MutationResult mutationResult,
       MutableDocument expected) {
-    MutableDocument clone = base.clone();
+    MutableDocument clone = base.mutableCopy();
     mutation.applyToRemoteDocument(clone, mutationResult);
     assertEquals(expected, clone);
   }
@@ -1033,8 +1033,8 @@ private String getDescription(
   }
 
   private void verifyOverlayRoundTrips(MutableDocument doc, Mutation... mutations) {
-    MutableDocument docForMutations = doc.clone();
-    MutableDocument docForOverlay = doc.clone();
+    MutableDocument docForMutations = doc.mutableCopy();
+    MutableDocument docForOverlay = doc.mutableCopy();
     Timestamp now = Timestamp.now();
 
     FieldMask mask = null;