getBaseDocument should always get results from remoteDocumentCache.

ehsannas · ehsannas · commit a5b06da14c1d · 2022-11-23T12:26:34.000-08:00
diff --git a/packages/firestore/src/local/local_documents_view.ts b/packages/firestore/src/local/local_documents_view.ts
@@ -91,7 +91,7 @@ export class LocalDocumentsView {
       .getOverlay(transaction, key)
       .next(value => {
         overlay = value;
-        return this.getBaseDocument(transaction, key, overlay);
+        return this.remoteDocumentCache.getEntry(transaction, key);
       })
       .next(document => {
         if (overlay !== null) {
@@ -423,11 +423,11 @@ export class LocalDocumentsView {
               if (originalDocs.get(key)) {
                 return PersistencePromise.resolve();
               }
-              return this.getBaseDocument(transaction, key, overlay).next(
-                doc => {
+              return this.remoteDocumentCache
+                .getEntry(transaction, key)
+                .next(doc => {
                   modifiedDocs = modifiedDocs.insert(key, doc);
-                }
-              );
+                });
             }
           )
             .next(() =>
@@ -549,17 +549,4 @@ export class LocalDocumentsView {
         return results;
       });
   }
-
-  /** Returns a base document that can be used to apply `overlay`. */
-  private getBaseDocument(
-    transaction: PersistenceTransaction,
-    key: DocumentKey,
-    overlay: Overlay | null
-  ): PersistencePromise<MutableDocument> {
-    return this.remoteDocumentCache.getEntry(transaction, key);
-    // TODO(COUNT): ROI of this is pretty low and it could be quite confusing.
-    // return overlay === null || overlay.mutation.type === MutationType.Patch
-    //   ? this.remoteDocumentCache.getEntry(transaction, key)
-    //   : PersistencePromise.resolve(MutableDocument.newInvalidDocument(key));
-  }
 }
diff --git a/packages/firestore/test/unit/local/local_store.test.ts b/packages/firestore/test/unit/local/local_store.test.ts
@@ -398,7 +398,10 @@ class LocalStoreTester {
     return this;
   }
 
-  toReturnChanged(...docs: Document[]): LocalStoreTester {
+  toReturnChangedInternal(
+    docs: Document[],
+    isEqual?: (lhs: Document | null, rhs: Document | null) => boolean
+  ): LocalStoreTester {
     this.promiseChain = this.promiseChain.then(() => {
       debugAssert(
         this.lastChanges !== null,
@@ -407,13 +410,29 @@ class LocalStoreTester {
       expect(this.lastChanges.size).to.equal(docs.length, 'number of changes');
       for (const doc of docs) {
         const returned = this.lastChanges.get(doc.key);
-        expectEqual(doc, returned, `Expected '${returned}' to equal '${doc}'.`);
+        const message = `Expected '${returned}' to equal '${doc}'.`;
+        if (isEqual) {
+          expect(isEqual(doc, returned)).to.equal(true, message);
+        } else {
+          expectEqual(doc, returned, message);
+        }
       }
       this.lastChanges = null;
     });
     return this;
   }
 
+  toReturnChanged(...docs: Document[]): LocalStoreTester {
+    return this.toReturnChangedInternal(docs);
+  }
+
+  toReturnChangedWithDocComparator(
+    isEqual: (lhs: Document | null, rhs: Document | null) => boolean,
+    ...docs: Document[]
+  ): LocalStoreTester {
+    return this.toReturnChangedInternal(docs, isEqual);
+  }
+
   toReturnRemoved(...keyStrings: string[]): LocalStoreTester {
     this.promiseChain = this.promiseChain.then(() => {
       debugAssert(
@@ -433,16 +452,20 @@ class LocalStoreTester {
     return this;
   }
 
-  toContain(doc: Document): LocalStoreTester {
+  toContain(
+    doc: Document,
+    isEqual?: (lhs: Document, rhs: Document) => boolean
+  ): LocalStoreTester {
     this.promiseChain = this.promiseChain.then(() => {
       return localStoreReadDocument(this.localStore, doc.key).then(result => {
-        expectEqual(
-          result,
-          doc,
-          `Expected ${
-            result ? result.toString() : null
-          } to match ${doc.toString()}.`
-        );
+        const message = `Expected ${
+          result ? result.toString() : null
+        } to match ${doc.toString()}.`;
+        if (isEqual) {
+          expect(isEqual(result, doc)).to.equal(true, message);
+        } else {
+          expectEqual(result, doc, message);
+        }
       });
     });
     return this;
@@ -539,22 +562,38 @@ class LocalStoreTester {
   }
 }
 
-describe('LocalStore w/ Memory Persistence', () => {
-  async function initialize(): Promise<LocalStoreComponents> {
-    const queryEngine = new CountingQueryEngine();
-    const persistence = await persistenceHelpers.testMemoryEagerPersistence();
-    const localStore = newLocalStore(
-      persistence,
-      queryEngine,
-      User.UNAUTHENTICATED,
-      JSON_SERIALIZER
-    );
-    return { queryEngine, persistence, localStore };
-  }
+// The `isEqual` method for the Document class does not compare createTime and
+// readTime. For some tests, we'd like to verify that a certain createTime has
+// been calculated for documents. In such cases we can use this comparator.
+function compareDocsWithCreateTime(
+  lhs: Document | null,
+  rhs: Document | null
+): boolean {
+  return (
+    (lhs === null && rhs === null) ||
+    (lhs !== null &&
+      rhs !== null &&
+      lhs.isEqual(rhs) &&
+      lhs.createTime.isEqual(rhs.createTime))
+  );
+}
 
-  addEqualityMatcher();
-  genericLocalStoreTests(initialize, /* gcIsEager= */ true);
-});
+// describe('LocalStore w/ Memory Persistence', () => {
+//   async function initialize(): Promise<LocalStoreComponents> {
+//     const queryEngine = new CountingQueryEngine();
+//     const persistence = await persistenceHelpers.testMemoryEagerPersistence();
+//     const localStore = newLocalStore(
+//       persistence,
+//       queryEngine,
+//       User.UNAUTHENTICATED,
+//       JSON_SERIALIZER
+//     );
+//     return { queryEngine, persistence, localStore };
+//   }
+//
+//   addEqualityMatcher();
+//   genericLocalStoreTests(initialize, /* gcIsEager= */ true);
+// });
 
 describe('LocalStore w/ IndexedDB Persistence', () => {
   if (!IndexedDbPersistence.isAvailable()) {
@@ -1964,28 +2003,58 @@ function genericLocalStoreTests(
         .afterAllocatingQuery(query('col'))
         .toReturnTargetId(2)
         .after(docAddedRemoteEvent(doc('col/doc1', 12, { foo: 'bar' }, 5), [2]))
-        .toReturnChanged(doc('col/doc1', 12, { foo: 'bar' }, 5))
-        .toContain(doc('col/doc1', 12, { foo: 'bar' }, 5))
+        .toReturnChangedWithDocComparator(
+          compareDocsWithCreateTime,
+          doc('col/doc1', 12, { foo: 'bar' }, 5)
+        )
+        .toContain(
+          doc('col/doc1', 12, { foo: 'bar' }, 5),
+          compareDocsWithCreateTime
+        )
         .after(setMutation('col/doc1', { foo: 'newBar' }))
-        .toReturnChanged(
+        .toReturnChangedWithDocComparator(
+          compareDocsWithCreateTime,
           doc('col/doc1', 12, { foo: 'newBar' }, 5).setHasLocalMutations()
         )
         .toContain(
-          doc('col/doc1', 12, { foo: 'newBar' }, 5).setHasLocalMutations()
+          doc('col/doc1', 12, { foo: 'newBar' }, 5).setHasLocalMutations(),
+          compareDocsWithCreateTime
         )
         .afterAcknowledgingMutation({ documentVersion: 13 })
         // We haven't seen the remote event yet
-        .toReturnChanged(
+        .toReturnChangedWithDocComparator(
+          compareDocsWithCreateTime,
           doc('col/doc1', 13, { foo: 'newBar' }, 5).setHasCommittedMutations()
         )
         .toContain(
-          doc('col/doc1', 13, { foo: 'newBar' }, 5).setHasCommittedMutations()
+          doc('col/doc1', 13, { foo: 'newBar' }, 5).setHasCommittedMutations(),
+          compareDocsWithCreateTime
         )
         .finish()
     );
   });
 
-  it('saves updateTime as createTime when creating new doc', () => {
+  it('saves updateTime as createTime when receives ack for creating a new doc', () => {
+    if (gcIsEager) {
+      return;
+    }
+
+    return expectLocalStore()
+      .after(setMutation('col/doc1', { foo: 'newBar' }))
+      .afterAcknowledgingMutation({ documentVersion: 13 })
+      .afterExecutingQuery(query('col'))
+      .toReturnChangedWithDocComparator(
+        compareDocsWithCreateTime,
+        doc('col/doc1', 13, { foo: 'newBar' }, 13).setHasCommittedMutations()
+      )
+      .toContain(
+        doc('col/doc1', 13, { foo: 'newBar' }, 13).setHasCommittedMutations(),
+        compareDocsWithCreateTime
+      )
+      .finish();
+  });
+
+  it('saves updateTime as createTime when recreating a deleted doc', async () => {
     if (gcIsEager) {
       return;
     }
@@ -1998,28 +2067,29 @@ function genericLocalStoreTests(
         .toReturnChanged(deletedDoc('col/doc1', 12))
         .toContain(deletedDoc('col/doc1', 12))
         .after(setMutation('col/doc1', { foo: 'newBar' }))
-        .toReturnChanged(
+        .toReturnChangedWithDocComparator(
+          compareDocsWithCreateTime,
           doc('col/doc1', 12, { foo: 'newBar' }, 12).setHasLocalMutations()
         )
-        // TODO(COUNT): Below has createTime=0 due to an optimization that uses invalid doc as base doc for
-        // set mutations. This is OK because it has no impact on aggregation's heuristic logic. But it feels
-        // "wrong" to have createTime 0 here. We should revisit this.
         .toContain(
-          doc('col/doc1', 12, { foo: 'newBar' }, 0).setHasLocalMutations()
+          doc('col/doc1', 12, { foo: 'newBar' }, 12).setHasLocalMutations(),
+          compareDocsWithCreateTime
         )
         .afterAcknowledgingMutation({ documentVersion: 13 })
         // We haven't seen the remote event yet
-        .toReturnChanged(
+        .toReturnChangedWithDocComparator(
+          compareDocsWithCreateTime,
           doc('col/doc1', 13, { foo: 'newBar' }, 13).setHasCommittedMutations()
         )
         .toContain(
-          doc('col/doc1', 13, { foo: 'newBar' }, 13).setHasCommittedMutations()
+          doc('col/doc1', 13, { foo: 'newBar' }, 13).setHasCommittedMutations(),
+          compareDocsWithCreateTime
         )
         .finish()
     );
   });
 
-  it('saves updateTime as createTime when creating new doc', () => {
+  it('document createTime is preserved through Set -> Ack -> Patch -> Ack', () => {
     if (gcIsEager) {
       return;
     }
@@ -2028,11 +2098,98 @@ function genericLocalStoreTests(
       .after(setMutation('col/doc1', { foo: 'newBar' }))
       .afterAcknowledgingMutation({ documentVersion: 13 })
       .afterExecutingQuery(query('col'))
-      .toReturnChanged(
+      .toReturnChangedWithDocComparator(
+        compareDocsWithCreateTime,
         doc('col/doc1', 13, { foo: 'newBar' }, 13).setHasCommittedMutations()
       )
       .toContain(
-        doc('col/doc1', 13, { foo: 'newBar' }, 13).setHasCommittedMutations()
+        doc('col/doc1', 13, { foo: 'newBar' }, 13).setHasCommittedMutations(),
+        compareDocsWithCreateTime
+      )
+      .afterMutations([patchMutation('col/doc1', { 'likes': 1 })])
+      .toReturnChangedWithDocComparator(
+        compareDocsWithCreateTime,
+        doc(
+          'col/doc1',
+          13,
+          { foo: 'newBar', likes: 1 },
+          13
+        ).setHasLocalMutations()
+      )
+      .toContain(
+        doc(
+          'col/doc1',
+          13,
+          { foo: 'newBar', likes: 1 },
+          13
+        ).setHasLocalMutations(),
+        compareDocsWithCreateTime
+      )
+      .afterAcknowledgingMutation({ documentVersion: 14 })
+      .toReturnChangedWithDocComparator(
+        compareDocsWithCreateTime,
+        doc(
+          'col/doc1',
+          14,
+          { foo: 'newBar', likes: 1 },
+          13
+        ).setHasCommittedMutations()
+      )
+      .toContain(
+        doc(
+          'col/doc1',
+          14,
+          { foo: 'newBar', likes: 1 },
+          13
+        ).setHasCommittedMutations(),
+        compareDocsWithCreateTime
+      )
+      .finish();
+  });
+
+  it('document createTime is preserved through Doc Added -> Patch -> Ack', () => {
+    if (gcIsEager) {
+      return;
+    }
+    return expectLocalStore()
+      .afterAllocatingQuery(query('col'))
+      .toReturnTargetId(2)
+      .after(docAddedRemoteEvent(doc('col/doc1', 12, { foo: 'bar' }, 5), [2]))
+      .toReturnChangedWithDocComparator(
+        compareDocsWithCreateTime,
+        doc('col/doc1', 12, { foo: 'bar' }, 5)
+      )
+      .toContain(
+        doc('col/doc1', 12, { foo: 'bar' }, 5),
+        compareDocsWithCreateTime
+      )
+      .afterMutations([patchMutation('col/doc1', { 'likes': 1 })])
+      .toReturnChangedWithDocComparator(
+        compareDocsWithCreateTime,
+        doc('col/doc1', 13, { foo: 'bar', likes: 1 }, 5).setHasLocalMutations()
+      )
+      .toContain(
+        doc('col/doc1', 13, { foo: 'bar', likes: 1 }, 5).setHasLocalMutations(),
+        compareDocsWithCreateTime
+      )
+      .afterAcknowledgingMutation({ documentVersion: 14 })
+      .toReturnChangedWithDocComparator(
+        compareDocsWithCreateTime,
+        doc(
+          'col/doc1',
+          14,
+          { foo: 'bar', likes: 1 },
+          5
+        ).setHasCommittedMutations()
+      )
+      .toContain(
+        doc(
+          'col/doc1',
+          14,
+          { foo: 'bar', likes: 1 },
+          5
+        ).setHasCommittedMutations(),
+        compareDocsWithCreateTime
       )
       .finish();
   });
