Feedback (next round)

Author: schmidt-sebastian

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

@@ -158,17 +158,7 @@ public DocumentChanges computeDocChanges(
     for (Map.Entry<DocumentKey, Document> entry : docChanges) {
       DocumentKey key = entry.getKey();
       Document oldDoc = oldDocumentSet.getDocument(key);
-      Document newDoc = entry.getValue();
-
-      hardAssert(
-          key.equals(entry.getKey()),
-          "Mismatching key in doc change %s != %s",
-          key,
-          entry.getKey());
-
-      if (!query.matches(entry.getValue())) {
-        newDoc = null;
-      }
+      Document newDoc = query.matches(entry.getValue()) ? entry.getValue() : null;
 
       boolean oldDocHadPendingMutations =
           oldDoc != null && this.mutatedKeys.contains(oldDoc.getKey());

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

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

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

@@ -56,7 +56,7 @@ com.google.firebase.firestore.proto.MaybeDocument encodeMaybeDocument(MutableDoc
     } else if (document.isUnknownDocument()) {
       builder.setUnknownDocument(encodeUnknownDocument(document));
     } else {
-      throw fail("Unknown Document %s", document);
+      throw fail("Cannot encode invalid document %s", document);
     }
 
     builder.setHasCommittedMutations(document.hasCommittedMutations());

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

@@ -96,16 +96,20 @@ public ImmutableSortedMap<DocumentKey, MutableDocument> getAllDocumentsMatchingQ
       }
 
       MutableDocument doc = entry.getValue().first;
-      if (doc.isFoundDocument()) {
+      if (!doc.isFoundDocument()) {
+        continue;
+      }
+
         SnapshotVersion readTime = entry.getValue().second;
         if (readTime.compareTo(sinceReadTime) <= 0) {
           continue;
         }
 
-        if (query.matches(doc)) {
-          result = result.insert(doc.getKey(), doc.clone());
+        if (!query.matches(doc)) {
+          continue;
         }
-      }
+
+      result = result.insert(doc.getKey(), doc.clone())
     }
 
     return result;

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

@@ -35,7 +35,7 @@ interface RemoteDocumentCache {
    * <p>The cache key is extracted from {@code maybeDocument.key}. If there is already a cache entry
    * for the key, it will be replaced.
    *
-   * @param document A Document or NoDocument to put in the cache.
+   * @param document A document to put in the cache.
    * @param readTime The time at which the document was read or committed.
    */
   void add(MutableDocument document, SnapshotVersion readTime);
@@ -47,16 +47,16 @@ interface RemoteDocumentCache {
    * Looks up an entry in the cache.
    *
    * @param documentKey The key of the entry to look up.
-   * @return The cached Document or NoDocument entry, or an invalid document if nothing is cached.
+   * @return The cached document entry, or an invalid document if nothing is cached.
    */
   MutableDocument get(DocumentKey documentKey);
 
   /**
    * Looks up a set of entries in the cache.
    *
    * @param documentKeys The keys of the entries to look up.
-   * @return The cached Document or NoDocument entries indexed by key. If an entry is not cached, an
-   *     invalid document is returned.
+   * @return The cached document entries indexed by key. If an entry is not cached, the
+   * corresponding key will be mapped to an invalid document
    */
   Map<DocumentKey, MutableDocument> getAll(Iterable<DocumentKey> documentKeys);
 
@@ -66,7 +66,7 @@ interface RemoteDocumentCache {
    * <p>Implementations may return extra documents if convenient. The results should be re-filtered
    * by the consumer before presenting them to the user.
    *
-   * <p>Cached NoDocument entries have no bearing on query results.
+   * <p>Cached entries for non-existing documents have no bearing on query results.
    *
    * @param query The query to match documents against.
    * @param sinceReadTime If not set to SnapshotVersion.MIN, return only documents that have been

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

@@ -84,7 +84,7 @@ public void applyToRemoteDocument(
       DocumentKey documentKey, MutableDocument document, MutationBatchResult batchResult) {
     hardAssert(
         document.getKey().equals(documentKey),
-        "applyToRemoteDocument: key %s doesn't match maybeDoc key %s",
+        "applyToRemoteDocument: key %s doesn't match document key %s",
         documentKey,
         document.getKey());
 
@@ -133,8 +133,7 @@ public ImmutableSortedMap<DocumentKey, Document> applyToLocalDocumentSet(
     // O(n).
     for (DocumentKey key : getKeys()) {
       // TODO(mutabledocuments): This method should take a map of MutableDocuments and we should
-      // remove
-      // this cast.
+      // remove this cast.
       MutableDocument document = (MutableDocument) documentMap.get(key);
       applyToLocalView(document);
       if (!document.isValidDocument()) {

firebase-firestore/src/testUtil/java/com/google/firebase/firestore/testutil/TestUtil.java

@@ -183,15 +183,19 @@ public static SnapshotVersion version(long versionMicros) {
   }
 
   public static MutableDocument doc(String key, long version, Map<String, Object> data) {
-    return new MutableDocument(key(key)).setFoundDocument(version(version), wrapObject(data));
+    return doc(key(key), version, wrapObject(data));
   }
 
   public static MutableDocument doc(DocumentKey key, long version, Map<String, Object> data) {
-    return new MutableDocument(key).setFoundDocument(version(version), wrapObject(data));
+    return doc(key, version, wrapObject(data));
   }
 
   public static MutableDocument doc(String key, long version, ObjectValue data) {
-    return new MutableDocument(key(key)).setFoundDocument(version(version), data);
+    return doc(key(key), version, data);
+  }
+
+  public static MutableDocument doc(DocumentKey key, long version, ObjectValue data) {
+    return new MutableDocument(key).setFoundDocument(version(version), data);
   }
 
   public static MutableDocument deletedDoc(String key, long version) {