Post-Merge cleanup of MutableDocuments

firebase-firestore/ktx/src/test/java/com/google/firebase/firestore/testutil/TestUtil.java

@@ -76,7 +76,7 @@ public static MutableDocument doc(String key, long version, ObjectValue data) {
   }
 
   public static MutableDocument doc(DocumentKey key, long version, ObjectValue data) {
-    return new MutableDocument(key).convertToFoundDocument(version(version), data);
+    return MutableDocument.newFoundDocument(key, version(version), data);
   }
 
   public static DocumentSet docSet(Comparator<Document> comparator, MutableDocument... documents) {

firebase-firestore/src/main/java/com/google/firebase/firestore/bundle/BundleLoader.java

@@ -73,8 +73,8 @@ public BundleLoader(BundleCallback bundleCallback, BundleMetadata bundleMetadata
         documents =
             documents.insert(
                 bundledDocumentMetadata.getKey(),
-                new MutableDocument(bundledDocumentMetadata.getKey())
-                    .convertToNoDocument(bundledDocumentMetadata.getReadTime()));
+                MutableDocument.newNoDocument(
+                    bundledDocumentMetadata.getKey(), bundledDocumentMetadata.getReadTime()));
         currentDocument = null;
       }
     } else if (bundleElement instanceof BundleDocument) {

firebase-firestore/src/main/java/com/google/firebase/firestore/bundle/BundleSerializer.java

@@ -111,9 +111,8 @@ BundleDocument decodeDocument(JSONObject document) throws JSONException {
     decodeMapValue(value, document.getJSONObject("fields"));
 
     return new BundleDocument(
-        new MutableDocument(key)
-            .convertToFoundDocument(
-                updateTime, ObjectValue.fromMap(value.getMapValue().getFieldsMap())));
+        MutableDocument.newFoundDocument(
+            key, updateTime, ObjectValue.fromMap(value.getMapValue().getFieldsMap())));
   }
 
   private ResourcePath decodeName(String name) {

firebase-firestore/src/main/java/com/google/firebase/firestore/core/SyncEngine.java

@@ -407,8 +407,7 @@ public void handleRejectedListen(int targetId, Status error) {
       // It's a limbo doc. Create a synthetic event saying it was deleted. This is kind of a hack.
       // Ideally, we would have a method in the local store to purge a document. However, it would
       // be tricky to keep all of the local store's invariants with another method.
-      MutableDocument result =
-          new MutableDocument(limboKey).convertToNoDocument(SnapshotVersion.NONE);
+      MutableDocument result = MutableDocument.newNoDocument(limboKey, SnapshotVersion.NONE);
       Map<DocumentKey, MutableDocument> documentUpdates =
           Collections.singletonMap(limboKey, result);
       Set<DocumentKey> limboDocuments = Collections.singleton(limboKey);

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

@@ -206,7 +206,7 @@ private ImmutableSortedMap<DocumentKey, Document> getDocumentsMatchingCollection
         MutableDocument document = remoteDocuments.get(key);
         if (document == null) {
           // Create invalid document to apply mutations on top of
-          document = new MutableDocument(key);
+          document = MutableDocument.newInvalidDocument(key);
           remoteDocuments = remoteDocuments.insert(key, document);
         }
         mutation.applyToLocalView(document, batch.getLocalWriteTime());

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

@@ -101,8 +101,8 @@ private MutableDocument decodeDocument(
     DocumentKey key = rpcSerializer.decodeKey(document.getName());
     SnapshotVersion version = rpcSerializer.decodeVersion(document.getUpdateTime());
     MutableDocument result =
-        new MutableDocument(key)
-            .convertToFoundDocument(version, ObjectValue.fromMap(document.getFieldsMap()));
+        MutableDocument.newFoundDocument(
+            key, version, ObjectValue.fromMap(document.getFieldsMap()));
     return hasCommittedMutations ? result.setHasCommittedMutations() : result;
   }
 
@@ -121,7 +121,7 @@ private MutableDocument decodeNoDocument(
       com.google.firebase.firestore.proto.NoDocument proto, boolean hasCommittedMutations) {
     DocumentKey key = rpcSerializer.decodeKey(proto.getName());
     SnapshotVersion version = rpcSerializer.decodeVersion(proto.getReadTime());
-    MutableDocument result = new MutableDocument(key).convertToNoDocument(version);
+    MutableDocument result = MutableDocument.newNoDocument(key, version);
     return hasCommittedMutations ? result.setHasCommittedMutations() : result;
   }
 
@@ -140,7 +140,7 @@ private MutableDocument decodeUnknownDocument(
       com.google.firebase.firestore.proto.UnknownDocument proto) {
     DocumentKey key = rpcSerializer.decodeKey(proto.getName());
     SnapshotVersion version = rpcSerializer.decodeVersion(proto.getVersion());
-    return new MutableDocument(key).convertToUnknownDocument(version);
+    return MutableDocument.newUnknownDocument(key, version);
   }
 
   /** Encodes a MutationBatch model for local storage in the mutation queue. */

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

@@ -776,7 +776,7 @@ private void applyWriteToRemoteDocuments(MutationBatchResult batchResult) {
       hardAssert(ackVersion != null, "docVersions should contain every doc in the write.");
 
       if (doc.getVersion().compareTo(ackVersion) < 0) {
-        batch.applyToRemoteDocument(docKey, doc, batchResult);
+        batch.applyToRemoteDocument(doc, batchResult);
         if (doc.isValidDocument()) {
           remoteDocuments.add(doc, batchResult.getCommitVersion());
         }

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

@@ -60,7 +60,7 @@ public void remove(DocumentKey key) {
   @Override
   public MutableDocument get(DocumentKey key) {
     Pair<MutableDocument, SnapshotVersion> entry = docs.get(key);
-    return entry != null ? entry.first.clone() : new MutableDocument(key);
+    return entry != null ? entry.first.clone() : MutableDocument.newInvalidDocument(key);
   }
 
   @Override

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

@@ -82,7 +82,7 @@ public MutableDocument get(DocumentKey documentKey) {
         db.query("SELECT contents FROM remote_documents WHERE path = ?")
             .binding(path)
             .firstValue(row -> decodeMaybeDocument(row.getBlob(0)));
-    return document != null ? document : new MutableDocument(documentKey);
+    return document != null ? document : MutableDocument.newInvalidDocument(documentKey);
   }
 
   @Override
@@ -96,7 +96,7 @@ public Map<DocumentKey, MutableDocument> getAll(Iterable<DocumentKey> documentKe
     for (DocumentKey key : documentKeys) {
       // Make sure each key has a corresponding entry, which is null in case the document is not
       // found.
-      results.put(key, new MutableDocument(key));
+      results.put(key, MutableDocument.newInvalidDocument(key));
     }
 
     SQLitePersistence.LongQuery longQuery =

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

@@ -1,4 +1,4 @@
-// Copyright 2021 Google LLC
+// Copyright 2018 Google LLC
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.

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

@@ -17,6 +17,15 @@
 import androidx.annotation.NonNull;
 import com.google.firestore.v1.Value;
 
+/**
+ * Represents a document in Firestore with a key, version, data and whether it has local mutations
+ * applied to it.
+ *
+ * <p>Documents can transition between states via {@link #convertToFoundDocument}, {@link
+ * #convertToNoDocument} and {@link #convertToUnknownDocument}. If a document does not transition to
+ * 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 {
 
   private enum DocumentType {
@@ -56,8 +65,8 @@ private enum DocumentState {
   private ObjectValue value;
   private DocumentState documentState;
 
-  public MutableDocument(DocumentKey key) {
-    this(key, DocumentType.INVALID, SnapshotVersion.NONE, new ObjectValue(), DocumentState.SYNCED);
+  private MutableDocument(DocumentKey key) {
+    this.key = key;
   }
 
   private MutableDocument(
@@ -73,6 +82,39 @@ private MutableDocument(
     this.value = value;
   }
 
+  /**
+   * Creates a document with no known version or data, but which can serve as base document for
+   * mutations.
+   */
+  public static MutableDocument newInvalidDocument(DocumentKey documentKey) {
+    return new MutableDocument(
+        documentKey,
+        DocumentType.INVALID,
+        SnapshotVersion.NONE,
+        new ObjectValue(),
+        DocumentState.SYNCED);
+  }
+
+  /** Creates a new document that is known to exist with the given data at the given version. */
+  public static MutableDocument newFoundDocument(
+      DocumentKey documentKey, SnapshotVersion version, ObjectValue value) {
+    return new MutableDocument(documentKey).convertToFoundDocument(version, value);
+  }
+
+  /** Creates a new document that is known to not exisr at the given version. */
+  public static MutableDocument newNoDocument(DocumentKey documentKey, SnapshotVersion version) {
+    return new MutableDocument(documentKey).convertToNoDocument(version);
+  }
+
+  /**
+   * Creates a new document that is known to exist at the given version but whose data is not known
+   * (e.g. a document that was updated without a known base document).
+   */
+  public static MutableDocument newUnknownDocument(
+      DocumentKey documentKey, SnapshotVersion version) {
+    return new MutableDocument(documentKey).convertToUnknownDocument(version);
+  }
+
   /**
    * Changes the document type to indicate that it exists and that its version and data are known.
    */
@@ -94,8 +136,8 @@ public MutableDocument convertToNoDocument(SnapshotVersion version) {
   }
 
   /**
-   * Changes the document type to indicate that it exists at a given version but that is data is not
-   * known (e.g. a document that was updated without a known base document).
+   * Changes the document type to indicate that it exists at a given version but that its data is
+   * not known (e.g. a document that was updated without a known base document).
    */
   public MutableDocument convertToUnknownDocument(SnapshotVersion version) {
     this.version = version;
@@ -120,29 +162,21 @@ public DocumentKey getKey() {
     return key;
   }
 
-  /**
-   * Returns the version of this document if it exists or a version at which this document was
-   * guaranteed to not exist.
-   */
+  @Override
   public SnapshotVersion getVersion() {
     return version;
   }
 
-  /** Returns whether local mutations were applied via the mutation queue. */
   @Override
   public boolean hasLocalMutations() {
     return documentState.equals(DocumentState.HAS_LOCAL_MUTATIONS);
   }
 
-  /** Returns whether mutations were applied based on a write acknowledgment. */
   @Override
   public boolean hasCommittedMutations() {
     return documentState.equals(DocumentState.HAS_COMMITTED_MUTATIONS);
   }
 
-  /**
-   * Whether this document has a local mutation applied that has not yet been acknowledged by Watch.
-   */
   @Override
   public boolean hasPendingWrites() {
     return hasLocalMutations() || hasCommittedMutations();

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

@@ -76,18 +76,10 @@ public MutationBatch(
    * Applies all the mutations in this MutationBatch to the specified document to create a new
    * remote document.
    *
-   * @param documentKey The key of the document to apply mutations to.
    * @param document The document to apply mutations to.
    * @param batchResult The result of applying the MutationBatch to the backend.
    */
-  public void applyToRemoteDocument(
-      DocumentKey documentKey, MutableDocument document, MutationBatchResult batchResult) {
-    hardAssert(
-        document.getKey().equals(documentKey),
-        "applyToRemoteDocument: key %s doesn't match document key %s",
-        documentKey,
-        document.getKey());
-
+  public void applyToRemoteDocument(MutableDocument document, MutationBatchResult batchResult) {
     int size = mutations.size();
     List<MutationResult> mutationResults = batchResult.getMutationResults();
     hardAssert(
@@ -98,7 +90,7 @@ public void applyToRemoteDocument(
 
     for (int i = 0; i < size; i++) {
       Mutation mutation = mutations.get(i);
-      if (mutation.getKey().equals(documentKey)) {
+      if (mutation.getKey().equals(document.getKey())) {
         MutationResult mutationResult = mutationResults.get(i);
         mutation.applyToRemoteDocument(document, mutationResult);
       }
@@ -129,7 +121,7 @@ public void applyToLocalView(MutableDocument document) {
   public ImmutableSortedMap<DocumentKey, Document> applyToLocalDocumentSet(
       ImmutableSortedMap<DocumentKey, Document> documentMap) {
     // TODO(mrschmidt): This implementation is O(n^2). If we iterate through the mutations first
-    // (as done in `applyToLocalView(DocumentKey k, MaybeDoc d)`), we can reduce the complexity to
+    // (as done in `applyToLocalView(MutableDocument d)`), we can reduce the complexity to
     // O(n).
     for (DocumentKey key : getKeys()) {
       // TODO(mutabledocuments): This method should take a map of MutableDocuments and we should

firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteSerializer.java

@@ -246,7 +246,7 @@ private MutableDocument decodeFoundDocument(BatchGetDocumentsResponse response)
     SnapshotVersion version = decodeVersion(response.getFound().getUpdateTime());
     hardAssert(
         !version.equals(SnapshotVersion.NONE), "Got a document response with no snapshot version");
-    return new MutableDocument(key).convertToFoundDocument(version, value);
+    return MutableDocument.newFoundDocument(key, version, value);
   }
 
   private MutableDocument decodeMissingDocument(BatchGetDocumentsResponse response) {
@@ -258,7 +258,7 @@ private MutableDocument decodeMissingDocument(BatchGetDocumentsResponse response
     hardAssert(
         !version.equals(SnapshotVersion.NONE),
         "Got a no document response with no snapshot version");
-    return new MutableDocument(key).convertToNoDocument(version);
+    return MutableDocument.newNoDocument(key, version);
   }
 
   // Mutations
@@ -874,7 +874,7 @@ public WatchChange decodeWatchChange(ListenResponse protoChange) {
         hardAssert(
             !version.equals(SnapshotVersion.NONE), "Got a document change without an update time");
         ObjectValue data = ObjectValue.fromMap(docChange.getDocument().getFieldsMap());
-        MutableDocument document = new MutableDocument(key).convertToFoundDocument(version, data);
+        MutableDocument document = MutableDocument.newFoundDocument(key, version, data);
         watchChange = new WatchChange.DocumentChange(added, removed, document.getKey(), document);
         break;
       case DOCUMENT_DELETE:
@@ -883,7 +883,7 @@ public WatchChange decodeWatchChange(ListenResponse protoChange) {
         key = decodeKey(docDelete.getDocument());
         // Note that version might be unset in which case we use SnapshotVersion.NONE
         version = decodeVersion(docDelete.getReadTime());
-        MutableDocument doc = new MutableDocument(key).convertToNoDocument(version);
+        MutableDocument doc = MutableDocument.newNoDocument(key, version);
         watchChange =
             new WatchChange.DocumentChange(Collections.emptyList(), removed, doc.getKey(), doc);
         break;

firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WatchChangeAggregator.java

@@ -189,8 +189,7 @@ public void handleExistenceFilter(ExistenceFilterWatchChange watchChange) {
           // deleted document there might be another query that will raise this document as part of
           // a snapshot  until it is resolved, essentially exposing inconsistency between queries.
           DocumentKey key = DocumentKey.fromPath(target.getPath());
-          MutableDocument result =
-              new MutableDocument(key).convertToNoDocument(SnapshotVersion.NONE);
+          MutableDocument result = MutableDocument.newNoDocument(key, SnapshotVersion.NONE);
           removeDocumentFromTarget(targetId, key, result);
         } else {
           hardAssert(
@@ -228,7 +227,7 @@ public RemoteEvent createRemoteEvent(SnapshotVersion snapshotVersion) {
           // limboDocumentRefs.
           DocumentKey key = DocumentKey.fromPath(targetData.getTarget().getPath());
           if (pendingDocumentUpdates.get(key) == null && !targetContainsDocument(targetId, key)) {
-            MutableDocument result = new MutableDocument(key).convertToNoDocument(snapshotVersion);
+            MutableDocument result = MutableDocument.newNoDocument(key, snapshotVersion);
             removeDocumentFromTarget(targetId, key, result);
           }
         }

firebase-firestore/src/test/java/com/google/firebase/firestore/bundle/BundleSerializerTest.java

@@ -698,13 +698,13 @@ private void assertDecodesValue(String json, Value proto) throws JSONException {
     BundleDocument actualDocument = serializer.decodeDocument(new JSONObject(documentJson));
     BundleDocument expectedDocument =
         new BundleDocument(
-            new MutableDocument(DocumentKey.fromName(TEST_DOCUMENT))
-                .convertToFoundDocument(
-                    new SnapshotVersion(new com.google.firebase.Timestamp(1577836802, 2)),
-                    new ObjectValue(
-                        Value.newBuilder()
-                            .setMapValue(MapValue.newBuilder().putFields("foo", proto))
-                            .build())));
+            MutableDocument.newFoundDocument(
+                DocumentKey.fromName(TEST_DOCUMENT),
+                new SnapshotVersion(new com.google.firebase.Timestamp(1577836802, 2)),
+                new ObjectValue(
+                    Value.newBuilder()
+                        .setMapValue(MapValue.newBuilder().putFields("foo", proto))
+                        .build())));
 
     assertEquals(expectedDocument, actualDocument);
   }

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

@@ -576,7 +576,7 @@ public void testRemoveTargetsThenGC() {
         () -> {
           SnapshotVersion newVersion = version(3);
           MutableDocument doc =
-              new MutableDocument(middleDocToUpdate).convertToFoundDocument(newVersion, testValue);
+              MutableDocument.newFoundDocument(middleDocToUpdate, newVersion, testValue);
           documentCache.add(doc, newVersion);
           updateTargetInTransaction(middleTarget);
         });

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

@@ -107,8 +107,7 @@ public void testReadSeveralDocumentsIncludingMissingDocument() {
 
     List<String> keys = new ArrayList<>(Arrays.asList(paths));
     keys.add("foo/nonexistent");
-    written.put(
-        key("foo/nonexistent"), new MutableDocument(key("foo/nonexistent"))); // Add invalid doc
+    written.put(key("foo/nonexistent"), MutableDocument.newInvalidDocument(key("foo/nonexistent")));
     Map<DocumentKey, MutableDocument> read = getAll(keys);
     assertEquals(written, read);
   }

firebase-firestore/src/test/java/com/google/firebase/firestore/model/DocumentTest.java

@@ -36,8 +36,7 @@ public class DocumentTest {
   @Test
   public void testInstantiation() {
     MutableDocument document =
-        new MutableDocument(key("messages/first"))
-            .convertToFoundDocument(version(1), wrapObject("a", 1));
+        MutableDocument.newFoundDocument(key("messages/first"), version(1), wrapObject("a", 1));
 
     assertEquals(key("messages/first"), document.getKey());
     assertEquals(version(1), document.getVersion());

firebase-firestore/src/test/java/com/google/firebase/firestore/model/ObjectValueBuilderTest.java → firebase-firestore/src/test/java/com/google/firebase/firestore/model/MutableObjectValueTest.java

@@ -29,15 +29,15 @@
 
 @RunWith(RobolectricTestRunner.class)
 @Config(manifest = Config.NONE)
-public class ObjectValueBuilderTest {
+public class MutableObjectValueTest {
   private String fooString = "foo";
   private Value fooValue = wrap(fooString);
   private String barString = "bar";
   private Value barValue = wrap(barString);
   private Value emptyObject = Value.newBuilder().setMapValue(MapValue.getDefaultInstance()).build();
 
   @Test
-  public void supportsEmptyobjectValues() {
+  public void supportsEmptyObjectValues() {
     ObjectValue objectValue = new ObjectValue();
     assertEquals(new ObjectValue(), objectValue);
   }