Address feedback

Author: schmidt-sebastian

packages/firestore/src/local/indexeddb_persistence.ts

@@ -70,7 +70,6 @@ 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';
 
@@ -1240,7 +1239,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

@@ -424,8 +424,7 @@ export class IndexedDbRemoteDocumentCache implements RemoteDocumentCache {
     }
 
     protected applyChanges(
-      transaction: PersistenceTransaction,
-      readTime: SnapshotVersion
+      transaction: PersistenceTransaction
     ): PersistencePromise<void> {
       const promises: Array<PersistencePromise<void>> = [];
 
@@ -440,7 +439,7 @@ export class IndexedDbRemoteDocumentCache implements RemoteDocumentCache {
         if (maybeDocument) {
           const doc = this.documentCache.serializer.toDbRemoteDocument(
             maybeDocument,
-            readTime
+            this.readTime!
           );
           const size = dbDocumentSize(doc);
           sizeDelta += size - previousSize!;

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,7 +623,7 @@ 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';
@@ -655,11 +655,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
   ) {}
 }
 

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

@@ -353,7 +353,7 @@ export class LocalStore {
           .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));
       }
@@ -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';
 
@@ -285,7 +284,7 @@ export class MemoryEagerDelegate implements ReferenceDelegate {
       }
     ).next(() => {
       this._orphanedDocuments = null;
-      return changeBuffer.apply(txn, SnapshotVersion.forDeletedDoc());
+      return changeBuffer.apply(txn);
     });
   }
 
@@ -425,9 +424,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 +472,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/remote_document_change_buffer.ts

@@ -47,6 +47,8 @@ export abstract class RemoteDocumentChangeBuffer {
     MaybeDocument | null
   > = new ObjectMap(key => key.toString());
 
+  protected readTime: SnapshotVersion | undefined;
+
   private changesApplied = false;
 
   protected abstract getFromCache(
@@ -60,8 +62,7 @@ export abstract class RemoteDocumentChangeBuffer {
   ): PersistencePromise<NullableMaybeDocumentMap>;
 
   protected abstract applyChanges(
-    transaction: PersistenceTransaction,
-    readTime: SnapshotVersion
+    transaction: PersistenceTransaction
   ): PersistencePromise<void>;
 
   /**
@@ -70,8 +71,17 @@ export abstract class RemoteDocumentChangeBuffer {
    * You can only modify documents that have already been retrieved via
    * `getEntry()/getEntries()` (enforced via IndexedDbs `apply()`).
    */
-  addEntry(maybeDocument: MaybeDocument): void {
+  addEntry(maybeDocument: MaybeDocument, readTime: SnapshotVersion): void {
     this.assertNotApplied();
+
+    // Assert that every read time matches since we only track a single read
+    // time per document change set.
+    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);
   }
 
@@ -132,13 +142,10 @@ export abstract class RemoteDocumentChangeBuffer {
    * Applies buffered changes to the underlying RemoteDocumentCache, using
    * the provided transaction.
    */
-  apply(
-    transaction: PersistenceTransaction,
-    readTime: SnapshotVersion
-  ): PersistencePromise<void> {
+  apply(transaction: PersistenceTransaction): PersistencePromise<void> {
     this.assertNotApplied();
     this.changesApplied = true;
-    return this.applyChanges(transaction, readTime);
+    return this.applyChanges(transaction);
   }
 
   /** Helper to assert this.changes is not null  */

packages/firestore/src/local/simple_db.ts

@@ -108,9 +108,6 @@ export class SimpleDb {
           event.oldVersion
         );
         const db = (event.target as IDBOpenDBRequest).result;
-        // We are provided a version upgrade transaction from the request, so
-        // we wrap that in a SimpleDbTransaction to allow use of our friendlier
-        // API for schema migration operations.
         schemaConverter
           .createOrUpgrade(
             db,

packages/firestore/test/integration/api/batch_writes.test.ts

@@ -125,6 +125,7 @@ apiDescribe('Database batch writes', (persistence: boolean) => {
   });
 
   it('can delete documents', () => {
+    // TODO(#1865): This test fails with node:persistence against Prod
     return integrationHelpers.withTestDoc(persistence, doc => {
       return doc
         .set({ foo: 'bar' })

packages/firestore/test/integration/api/database.test.ts

@@ -70,6 +70,7 @@ apiDescribe('Database', (persistence: boolean) => {
   });
 
   it('can delete a document', () => {
+    // TODO(#1865): This test fails with node:persistence against Prod
     return withTestDoc(persistence, docRef => {
       return docRef
         .set({ foo: 'bar' })