Review feedback

Author: schmidt-sebastian

packages/firestore/src/local/indexeddb_persistence.ts

@@ -70,6 +70,7 @@ import { QueryData } from './query_data';
 import { ReferenceSet } from './reference_set';
 import { ClientId } from './shared_client_state';
 import { SimpleDb, SimpleDbStore, SimpleDbTransaction } from './simple_db';
+import { SnapshotVersion } from '../core/snapshot_version';
 
 const LOG_TAG = 'IndexedDbPersistence';
 
@@ -1224,7 +1225,10 @@ export class IndexedDbLruDelegate implements ReferenceDelegate, LruDelegate {
               // Our size accounting requires us to read all documents before
               // removing them.
               return changeBuffer.getEntry(txn, docKey).next(() => {
-                changeBuffer.removeEntry(docKey);
+                changeBuffer.removeEntry(
+                  docKey,
+                  SnapshotVersion.forDeletedDoc()
+                );
                 return documentTargetStore(txn).delete(sentinelKey(docKey));
               });
             }

packages/firestore/src/local/indexeddb_remote_document_cache.ts

@@ -442,7 +442,7 @@ export class IndexedDbRemoteDocumentCache implements RemoteDocumentCache {
         if (maybeDocument) {
           const doc = this.documentCache.serializer.toDbRemoteDocument(
             maybeDocument,
-            this.readTime!
+            this.readTime
           );
           const size = dbDocumentSize(doc);
           sizeDelta += size - previousSize!;
@@ -456,7 +456,7 @@ export class IndexedDbRemoteDocumentCache implements RemoteDocumentCache {
             // preserved in `getNewDocumentChanges()`.
             const deletedDoc = this.documentCache.serializer.toDbRemoteDocument(
               new NoDocument(key, SnapshotVersion.forDeletedDoc()),
-              this.readTime!
+              this.readTime
             );
             promises.push(
               this.documentCache.addEntry(transaction, key, deletedDoc)

packages/firestore/src/local/local_store.ts

@@ -535,7 +535,7 @@ export class LocalStore {
                 // NoDocuments with SnapshotVersion.MIN are used in manufactured events (e.g. in the
                 // case of a limbo document resolution failing). We remove these documents from cache
                 // since we lost access.
-                documentBuffer.removeEntry(key);
+                documentBuffer.removeEntry(key, remoteVersion);
                 changedDocs = changedDocs.insert(key, doc);
               } else {
                 log.debug(

packages/firestore/src/local/memory_persistence.ts

@@ -49,6 +49,7 @@ import { PersistencePromise } from './persistence_promise';
 import { QueryData } from './query_data';
 import { ReferenceSet } from './reference_set';
 import { ClientId } from './shared_client_state';
+import { SnapshotVersion } from '../core/snapshot_version';
 
 const LOG_TAG = 'MemoryPersistence';
 
@@ -276,7 +277,7 @@ export class MemoryEagerDelegate implements ReferenceDelegate {
       (key: DocumentKey) => {
         return this.isReferenced(txn, key).next(isReferenced => {
           if (!isReferenced) {
-            changeBuffer.removeEntry(key);
+            changeBuffer.removeEntry(key, SnapshotVersion.forDeletedDoc());
           }
         });
       }
@@ -416,7 +417,7 @@ export class MemoryLruDelegate implements ReferenceDelegate, LruDelegate {
       return this.isPinned(txn, key, upperBound).next(isPinned => {
         if (!isPinned) {
           count++;
-          changeBuffer.removeEntry(key);
+          changeBuffer.removeEntry(key, SnapshotVersion.forDeletedDoc());
         }
       });
     });

packages/firestore/src/local/remote_document_change_buffer.ts

@@ -48,7 +48,7 @@ export abstract class RemoteDocumentChangeBuffer {
   > = new ObjectMap(key => key.toString());
 
   // The read time to use for all added documents in this change buffer.
-  protected readTime: SnapshotVersion | undefined;
+  private _readTime: SnapshotVersion | undefined;
 
   private changesApplied = false;
 
@@ -66,6 +66,24 @@ export abstract class RemoteDocumentChangeBuffer {
     transaction: PersistenceTransaction
   ): PersistencePromise<void>;
 
+  protected set readTime(value: SnapshotVersion) {
+    // Right now (for simplicity) we just track a single readTime for all the
+    // added entries since we expect them to all be the same, but we could
+    // rework to store per-entry readTimes if necessary.
+    assert(
+      this._readTime === undefined || this._readTime.isEqual(value),
+      'All changes in a RemoteDocumentChangeBuffer must have the same read time'
+    );
+    this._readTime = value;
+  }
+
+  protected get readTime(): SnapshotVersion {
+    assert(
+      this._readTime !== undefined,
+      'Read time is not set. Did you call addEntry/removeEntry?'
+    );
+    return this._readTime!;
+  }
   /**
    * Buffers a `RemoteDocumentCache.addEntry()` call.
    *
@@ -74,15 +92,6 @@ export abstract class RemoteDocumentChangeBuffer {
    */
   addEntry(maybeDocument: MaybeDocument, readTime: SnapshotVersion): void {
     this.assertNotApplied();
-
-    // Right now (for simplicity) we just track a single readTime for all the
-    // added entries since we expect them to all be the same, but we could
-    // rework to store per-entry readTimes if necessary.
-    assert(
-      this.readTime === undefined || this.readTime.isEqual(readTime),
-      'All changes in a RemoteDocumentChangeBuffer must have the same read time'
-    );
-
     this.readTime = readTime;
     this.changes.set(maybeDocument.key, maybeDocument);
   }
@@ -93,8 +102,9 @@ export abstract class RemoteDocumentChangeBuffer {
    * You can only remove documents that have already been retrieved via
    * `getEntry()/getEntries()` (enforced via IndexedDbs `apply()`).
    */
-  removeEntry(key: DocumentKey): void {
+  removeEntry(key: DocumentKey, readTime: SnapshotVersion): void {
     this.assertNotApplied();
+    this.readTime = readTime;
     this.changes.set(key, null);
   }
 

packages/firestore/test/unit/local/indexeddb_persistence.test.ts

@@ -821,8 +821,8 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => {
   });
 
   it('can get recent document changes in a collection', async () => {
-    const oldDocPaths = ['coll/doc1', 'coll/doc2', 'ignored/doc1'];
-    const newDocPaths = ['coll/doc3', 'coll/doc4', 'ignored/doc2'];
+    const oldDocPaths = ['coll/doc1', 'coll/doc2', 'abc/doc1'];
+    const newDocPaths = ['coll/doc3', 'coll/doc4', 'abc/doc2'];
 
     await withDb(9, db => {
       const sdb = new SimpleDb(db);

packages/firestore/test/unit/local/remote_document_cache.test.ts

@@ -133,12 +133,12 @@ function eagerRemoteDocumentCacheTests(
     const totalSize = await cache.getSize();
     expect(totalSize).to.equal(0);
 
-    await cache.removeEntry(doc2.key);
+    await cache.removeEntry(doc2.key, version(3));
 
     const currentSize = await cache.getSize();
     expect(currentSize).to.equal(0);
 
-    await cache.removeEntry(doc1.key);
+    await cache.removeEntry(doc1.key, version(4));
 
     const finalSize = await cache.getSize();
     expect(finalSize).to.equal(0);
@@ -171,12 +171,12 @@ function lruRemoteDocumentCacheTests(
     const totalSize = await cache.getSize();
     expect(totalSize).to.be.greaterThan(doc1Size);
 
-    await cache.removeEntry(doc2.key);
+    await cache.removeEntry(doc2.key, version(3));
 
     const currentSize = await cache.getSize();
     expect(currentSize).to.equal(doc1Size);
 
-    await cache.removeEntry(doc1.key);
+    await cache.removeEntry(doc1.key, version(4));
 
     const finalSize = await cache.getSize();
     expect(finalSize).to.equal(0);
@@ -282,7 +282,7 @@ function genericRemoteDocumentCacheTests(
     return cache
       .addEntry(doc(DOC_PATH, VERSION, DOC_DATA))
       .then(() => {
-        return cache.removeEntry(key(DOC_PATH));
+        return cache.removeEntry(key(DOC_PATH), version(VERSION));
       })
       .then(() => {
         return cache.getEntry(key(DOC_PATH));
@@ -294,7 +294,7 @@ function genericRemoteDocumentCacheTests(
 
   it('can remove nonexistent document', () => {
     // no-op, but make sure it doesn't fail.
-    return cache.removeEntry(key(DOC_PATH));
+    return cache.removeEntry(key(DOC_PATH), version(1));
   });
 
   it('can get documents matching query', async () => {
@@ -341,9 +341,9 @@ function genericRemoteDocumentCacheTests(
       changedDocs
     );
 
-    await cache.addEntry(doc('c/1', 3, DOC_DATA));
+    await cache.addEntry(doc('c/1', 4, DOC_DATA));
     changedDocs = await cache.getNewDocumentChanges();
-    assertMatches([doc('c/1', 3, DOC_DATA)], changedDocs);
+    assertMatches([doc('c/1', 4, DOC_DATA)], changedDocs);
   });
 
   it('can get empty changes', async () => {
@@ -360,7 +360,7 @@ function genericRemoteDocumentCacheTests(
       ],
       version(3)
     );
-    await cache.removeEntry(key('a/2'));
+    await cache.removeEntry(key('a/2'), version(4));
 
     const changedDocs = await cache.getNewDocumentChanges();
     assertMatches(

packages/firestore/test/unit/local/remote_document_change_buffer.test.ts

@@ -73,7 +73,7 @@ describe('RemoteDocumentChangeBuffer', () => {
     const newADoc = doc('coll/a', 43, { new: 'data' });
     buffer.addEntry(newADoc, newADoc.version);
     expect(await buffer.getEntry(key('coll/a'))).to.not.be.null;
-    buffer.removeEntry(newADoc.key);
+    buffer.removeEntry(newADoc.key, newADoc.version);
     expect(await buffer.getEntry(key('coll/a'))).to.be.null;
   });
 
@@ -94,7 +94,7 @@ describe('RemoteDocumentChangeBuffer', () => {
 
   it('can apply changes with removal', async () => {
     expectEqual(await buffer.getEntry(key('coll/a')), INITIAL_DOC);
-    buffer.removeEntry(INITIAL_DOC.key);
+    buffer.removeEntry(INITIAL_DOC.key, INITIAL_DOC.version);
     // Reading directly against the cache should still yield the old result.
     expectEqual(await cache.getEntry(key('coll/a')), INITIAL_DOC);
 

packages/firestore/test/unit/local/test_remote_document_cache.ts

@@ -76,14 +76,17 @@ export class TestRemoteDocumentCache {
     return this.addEntries([maybeDocument], maybeDocument.version);
   }
 
-  removeEntry(documentKey: DocumentKey): Promise<void> {
+  removeEntry(
+    documentKey: DocumentKey,
+    readTime: SnapshotVersion
+  ): Promise<void> {
     return this.persistence.runTransaction(
       'removeEntry',
       'readwrite-primary',
       txn => {
-        const changeBuffer = this.newChangeBuffer();
+        const changeBuffer = this.newChangeBuffer({ trackRemovals: true });
         return changeBuffer.getEntry(txn, documentKey).next(() => {
-          changeBuffer.removeEntry(documentKey);
+          changeBuffer.removeEntry(documentKey, readTime);
           return changeBuffer.apply(txn);
         });
       }
@@ -128,7 +131,9 @@ export class TestRemoteDocumentCache {
     );
   }
 
-  newChangeBuffer(): RemoteDocumentChangeBuffer {
-    return this.cache.newChangeBuffer();
+  newChangeBuffer(options?: {
+    trackRemovals: boolean;
+  }): RemoteDocumentChangeBuffer {
+    return this.cache.newChangeBuffer(options);
   }
 }

packages/firestore/test/unit/local/test_remote_document_change_buffer.ts

@@ -35,8 +35,8 @@ export class TestRemoteDocumentChangeBuffer {
     this.buffer.addEntry(maybeDocument, readTime);
   }
 
-  removeEntry(key: DocumentKey): void {
-    this.buffer.removeEntry(key);
+  removeEntry(key: DocumentKey, readTime: SnapshotVersion): void {
+    this.buffer.removeEntry(key, readTime);
   }
 
   getEntry(documentKey: DocumentKey): Promise<MaybeDocument | null> {