Review feedback

Author: schmidt-sebastian

packages/firestore/src/local/indexeddb_persistence.ts

@@ -70,14 +70,12 @@ 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';
 
 /**
  * Oldest acceptable age in milliseconds for client metadata before the client
- * is considered inactive and its associated data (such as the remote document
- * cache changelog) is garbage collected.
+ * is considered inactive and its associated data is garbage collected.
  */
 const MAX_CLIENT_AGE_MS = 30 * 60 * 1000; // 30 minutes
 
@@ -260,7 +258,7 @@ export class IndexedDbPersistence implements Persistence {
   /** The client metadata refresh task. */
   private clientMetadataRefresher: CancelablePromise<void> | null = null;
 
-  /** The last time we garbage collected the Remote Document Changelog. */
+  /** The last time we garbage collected the client metadata object store. */
   private lastGarbageCollectionTime = Number.NEGATIVE_INFINITY;
 
   /** Whether to allow shared multi-tab access to the persistence layer. */
@@ -1212,7 +1210,7 @@ export class IndexedDbLruDelegate implements ReferenceDelegate, LruDelegate {
   ): PersistencePromise<number> {
     const documentCache = this.db.getRemoteDocumentCache();
     const changeBuffer = documentCache.newChangeBuffer({
-      createSentinelDocumentsToTrackDeletes: false
+      trackRemovals: false
     });
 
     const promises: Array<PersistencePromise<void>> = [];
@@ -1240,7 +1238,7 @@ export class IndexedDbLruDelegate implements ReferenceDelegate, LruDelegate {
 
     return iteration
       .next(() => PersistencePromise.waitFor(promises))
-      .next(() => changeBuffer.apply(txn, SnapshotVersion.forDeletedDoc()))
+      .next(() => changeBuffer.apply(txn))
       .next(() => documentCount);
   }
 

packages/firestore/src/local/indexeddb_remote_document_cache.ts

@@ -111,7 +111,8 @@ export class IndexedDbRemoteDocumentCache implements RemoteDocumentCache {
   }
 
   /**
-   * Updates the document change log and adds the given delta to the cached current size.
+   * Updates the current size.
+   *
    * Callers to `addEntry()` and `removeEntry()` *must* call this afterwards to update the
    * cache's metadata.
    */
@@ -307,6 +308,8 @@ export class IndexedDbRemoteDocumentCache implements RemoteDocumentCache {
       .iterate(
         { index: DbRemoteDocument.readTimeIndex, range },
         (_, dbRemoteDoc) => {
+          // Unlike `getEntry()` and others, `getNewDocumentChanges()` uses the
+          // serializer directly since we want to keep sentinel deletes.
           const doc = this.serializer.fromDbRemoteDocument(dbRemoteDoc);
           changedDocs = changedDocs.insert(doc.key, doc);
           this.lastProcessedReadTime = this.serializer.fromDbTimestampKey(
@@ -335,21 +338,19 @@ export class IndexedDbRemoteDocumentCache implements RemoteDocumentCache {
       { index: DbRemoteDocument.readTimeIndex, reverse: true },
       (key, value, control) => {
         if (value.readTime) {
-          this.lastProcessedReadTime = SnapshotVersion.fromTimestamp(
-            new Timestamp(value.readTime![0], value.readTime![1])
-          );
+          this.lastProcessedReadTime = this.serializer.fromDbTimestampKey(value.readTime)
         }
         control.done();
       }
     );
   }
 
   newChangeBuffer(options: {
-    createSentinelDocumentsToTrackDeletes: boolean;
+    trackRemovals: boolean;
   }): RemoteDocumentChangeBuffer {
     return new IndexedDbRemoteDocumentCache.RemoteDocumentChangeBuffer(
       this,
-      options.createSentinelDocumentsToTrackDeletes
+      options.trackRemovals
     );
   }
 
@@ -388,8 +389,8 @@ export class IndexedDbRemoteDocumentCache implements RemoteDocumentCache {
         doc instanceof NoDocument &&
         doc.version.isEqual(SnapshotVersion.forDeletedDoc())
       ) {
-        // The document is a sentinel delete and should only be used in the
-        // change log.
+        // The document is a sentinel removal and should only be used in the
+        // `getNewDocumentChanges()`.
         return null;
       }
 
@@ -413,19 +414,18 @@ export class IndexedDbRemoteDocumentCache implements RemoteDocumentCache {
 
     /**
      * @param documentCache The IndexedDbRemoteDocumentCache to apply the changes to.
-     * @param trackDeletes Whether to create sentinel deletes that can be tracked by
+     * @param trackRemovals Whether to create sentinel deletes that can be tracked by
      * `getNewDocumentChanges()`.
      */
     constructor(
       private readonly documentCache: IndexedDbRemoteDocumentCache,
-      private readonly trackDeletes: boolean
+      private readonly trackRemovals: boolean
     ) {
       super();
     }
 
     protected applyChanges(
-      transaction: PersistenceTransaction,
-      readTime: SnapshotVersion
+      transaction: PersistenceTransaction
     ): PersistencePromise<void> {
       const promises: Array<PersistencePromise<void>> = [];
 
@@ -440,18 +440,21 @@ export class IndexedDbRemoteDocumentCache implements RemoteDocumentCache {
         if (maybeDocument) {
           const doc = this.documentCache.serializer.toDbRemoteDocument(
             maybeDocument,
-            readTime
+            this.readTime!
           );
           const size = dbDocumentSize(doc);
           sizeDelta += size - previousSize!;
           promises.push(this.documentCache.addEntry(transaction, key, doc));
         } else {
           sizeDelta -= previousSize!;
-          if (this.trackDeletes) {
-            // A sentinel delete is symbolized by a NoDocument with a version of 0.
+          if (this.trackRemovals) {
+            // In order to track removals, we store "sentinel delete" entries in
+            // the Remote Document Cache. These entries are represented by a
+            // NoDocument with a version of 0 and are ignored by
+            // `maybeDecodeDocument()` but preserved in `getNewDocumentChanges()`.
             const deletedDoc = this.documentCache.serializer.toDbRemoteDocument(
               new NoDocument(key, SnapshotVersion.forDeletedDoc()),
-              readTime
+              this.readTime!
             );
             promises.push(
               this.documentCache.addEntry(transaction, key, deletedDoc)

packages/firestore/src/local/indexeddb_schema.ts

@@ -311,7 +311,7 @@ export class DbTimestamp {
 }
 
 /** A timestamp type that can be used in IndexedDb keys. */
-export type DbTimestampKey = number[];
+export type DbTimestampKey = [/* seconds */ number, /* nanos */ number];
 
 // The key for the singleton object in the DbPrimaryClient is a single string.
 export type DbPrimaryClientKey = typeof DbPrimaryClient.key;
@@ -623,13 +623,19 @@ export class DbRemoteDocument {
    * An index that provides access to documents in a collection sorted by read
    * time.
    *
-   * This index is used to allow Index-Free queries to fetch newly changed
+   * This index is used to allow the RemoteDocumentCache to fetch newly changed
    * documents in a collection.
    */
   static collectionReadTimeIndex = 'collectionReadTimeIndex';
 
   static collectionReadTimeIndexPath = ['parentPath', 'readTime'];
 
+  // TODO: We are currently storing document keys three times (once as part
+  // of the primary key, once as `parentPath` (mostly) and once inside the
+  // encoded documents). During our next migration, we should rewrite the
+  // primary key as parentPath + document ID which would allow us to drop
+  // one value.
+
   constructor(
     /**
      * Set to an instance of DbUnknownDocument if the data for a document is
@@ -655,11 +661,17 @@ export class DbRemoteDocument {
      */
     public hasCommittedMutations: boolean | undefined,
 
-    /** When the document was read from the backend. */
+    /**
+     * When the document was read from the backend. Undefined for data written
+     * prior to schema version 9.
+     */
     public readTime: DbTimestampKey | undefined,
 
-    /** The path of the collection this document is part of. */
-    public parentPath: string | undefined
+    /**
+     * The path of the collection this document is part of. Undefined for data
+     * written prior to schema version 9.
+     */
+    public parentPath: string[] | undefined
   ) {}
 }
 
@@ -1005,9 +1017,6 @@ export class DbClientMetadata {
   static keyPath = 'clientId';
 
   constructor(
-    // Note: Previous schema versions included a field
-    // "lastProcessedDocumentChangeId". Don't use anymore.
-
     /** The auto-generated client id assigned at client startup. */
     public clientId: string,
     /** The last time this state was updated. */
@@ -1016,6 +1025,9 @@ export class DbClientMetadata {
     public networkEnabled: boolean,
     /** Whether this client is running in a foreground tab. */
     public inForeground: boolean
+
+    // Note: Previous schema versions included a field
+    // "lastProcessedDocumentChangeId". Don't use anymore.
   ) {}
 }
 
@@ -1046,6 +1058,7 @@ export const V1_STORES = [
 export const V3_STORES = V1_STORES;
 
 // Visible for testing
+// Note: DbRemoteDocumentChanges is no longer used and dropped with version 9
 export const V4_STORES = [...V3_STORES, DbClientMetadata.store];
 
 // V5 does not change the set of stores.

packages/firestore/src/local/local_serializer.ts

@@ -76,7 +76,7 @@ export class LocalSerializer {
     readTime: SnapshotVersion
   ): DbRemoteDocument {
     const dbReadTime = this.toDbTimestampKey(readTime);
-    const parentPath = maybeDoc.key.path.popLast().canonicalString();
+    const parentPath = maybeDoc.key.path.popLast().toArray();
     if (maybeDoc instanceof Document) {
       const doc = maybeDoc.proto
         ? maybeDoc.proto
@@ -114,7 +114,7 @@ export class LocalSerializer {
         parentPath
       );
     } else {
-      return fail('Unexpected MaybeDocumment');
+      return fail('Unexpected MaybeDocument');
     }
   }
 

packages/firestore/src/local/local_store.ts

@@ -345,15 +345,15 @@ export class LocalStore {
       'readwrite-primary',
       txn => {
         const affected = batchResult.batch.keys();
-        const documentBuffer = this.remoteDocuments.newChangeBuffer({
-          createSentinelDocumentsToTrackDeletes: true
-        });
+        const documentBuffer = this.remoteDocuments.newChangeBuffer(
+            {trackRemovals: true} // Make sure document removals show up in `getNewDocumentChanges()`
+            );
         return this.mutationQueue
           .acknowledgeBatch(txn, batchResult.batch, batchResult.streamToken)
           .next(() =>
             this.applyWriteToRemoteDocuments(txn, batchResult, documentBuffer)
           )
-          .next(() => documentBuffer.apply(txn, batchResult.commitVersion))
+          .next(() => documentBuffer.apply(txn))
           .next(() => this.mutationQueue.performConsistencyCheck(txn))
           .next(() => this.localDocuments.getDocuments(txn, affected));
       }
@@ -451,7 +451,7 @@ export class LocalStore {
    */
   applyRemoteEvent(remoteEvent: RemoteEvent): Promise<MaybeDocumentMap> {
     const documentBuffer = this.remoteDocuments.newChangeBuffer({
-      createSentinelDocumentsToTrackDeletes: true
+        trackRemovals: true // Make sure document removals show up in `getNewDocumentChanges()`
     });
     const remoteVersion = remoteEvent.snapshotVersion;
     return this.persistence.runTransaction(
@@ -526,7 +526,7 @@ export class LocalStore {
                 (doc.version.compareTo(existingDoc.version) === 0 &&
                   existingDoc.hasPendingWrites)
               ) {
-                documentBuffer.addEntry(doc);
+                documentBuffer.addEntry(doc, remoteVersion);
                 changedDocs = changedDocs.insert(key, doc);
               } else if (
                 doc instanceof NoDocument &&
@@ -586,7 +586,7 @@ export class LocalStore {
         }
 
         return PersistencePromise.waitFor(promises)
-          .next(() => documentBuffer.apply(txn, remoteVersion))
+          .next(() => documentBuffer.apply(txn))
           .next(() => {
             return this.localDocuments.getLocalViewOfDocuments(
               txn,
@@ -867,7 +867,10 @@ export class LocalStore {
                   ' resulted in null'
               );
             } else {
-              documentBuffer.addEntry(doc);
+              // We use the commitVersion as the readTime rather than the
+              // document's updateTime since the updateTime is not advanced
+              // for updates that do not modify the underlying document.
+              documentBuffer.addEntry(doc, batchResult.commitVersion);
             }
           }
         });

packages/firestore/src/local/memory_persistence.ts

@@ -49,7 +49,6 @@ 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';
 
@@ -271,9 +270,7 @@ export class MemoryEagerDelegate implements ReferenceDelegate {
   ): PersistencePromise<void> {
     // Remove newly orphaned documents.
     const cache = this.persistence.getRemoteDocumentCache();
-    const changeBuffer = cache.newChangeBuffer({
-      createSentinelDocumentsToTrackDeletes: false
-    });
+    const changeBuffer = cache.newChangeBuffer();
     return PersistencePromise.forEach(
       this.orphanedDocuments,
       (key: DocumentKey) => {
@@ -285,7 +282,7 @@ export class MemoryEagerDelegate implements ReferenceDelegate {
       }
     ).next(() => {
       this._orphanedDocuments = null;
-      return changeBuffer.apply(txn, SnapshotVersion.forDeletedDoc());
+      return changeBuffer.apply(txn);
     });
   }
 
@@ -414,9 +411,7 @@ export class MemoryLruDelegate implements ReferenceDelegate, LruDelegate {
   ): PersistencePromise<number> {
     let count = 0;
     const cache = this.persistence.getRemoteDocumentCache();
-    const changeBuffer = cache.newChangeBuffer({
-      createSentinelDocumentsToTrackDeletes: false
-    });
+    const changeBuffer = cache.newChangeBuffer();
     const p = cache.forEachDocumentKey(txn, key => {
       return this.isPinned(txn, key, upperBound).next(isPinned => {
         if (!isPinned) {
@@ -425,9 +420,7 @@ export class MemoryLruDelegate implements ReferenceDelegate, LruDelegate {
         }
       });
     });
-    return p
-      .next(() => changeBuffer.apply(txn, SnapshotVersion.forDeletedDoc()))
-      .next(() => count);
+    return p.next(() => changeBuffer.apply(txn)).next(() => count);
   }
 
   removeMutationReference(
@@ -475,7 +468,7 @@ export class MemoryLruDelegate implements ReferenceDelegate, LruDelegate {
   documentSize(maybeDoc: MaybeDocument): number {
     const remoteDocument = this.serializer.toDbRemoteDocument(
       maybeDoc,
-      SnapshotVersion.forDeletedDoc()
+      maybeDoc.version
     );
     let value: unknown;
     if (remoteDocument.document) {

packages/firestore/src/local/memory_remote_document_cache.ts

@@ -178,9 +178,11 @@ export class MemoryRemoteDocumentCache implements RemoteDocumentCache {
     return PersistencePromise.resolve(changedDocs);
   }
 
-  newChangeBuffer(options: {
-    createSentinelDocumentsToTrackDeletes: boolean;
+  newChangeBuffer(options?: {
+    trackRemovals: boolean;
   }): RemoteDocumentChangeBuffer {
+    // `trackRemovals` is ignores since the memory remote document cache keeps
+    // a separate changelog and does not need special handling for removals.
     return new MemoryRemoteDocumentCache.RemoteDocumentChangeBuffer(this);
   }