Skip to content

Commit 7ef820f

Browse files
schmidt-sebastianschmidt-sebastian
committed
Address feedback
1 parent 64cfe5f commit 7ef820f

16 files changed

+186
-127
lines changed

‎packages/firestore/src/local/indexeddb_persistence.ts

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,6 @@ import { QueryData } from './query_data';
7070
import { ReferenceSet } from './reference_set';
7171
import { ClientId } from './shared_client_state';
7272
import { SimpleDb, SimpleDbStore, SimpleDbTransaction } from './simple_db';
73-
import { SnapshotVersion } from '../core/snapshot_version';
7473

7574
const LOG_TAG = 'IndexedDbPersistence';
7675

@@ -1264,7 +1263,7 @@ export class IndexedDbLruDelegate implements ReferenceDelegate, LruDelegate {
12641263

12651264
return iteration
12661265
.next(() => PersistencePromise.waitFor(promises))
1267-
.next(() => changeBuffer.apply(txn, SnapshotVersion.forDeletedDoc()))
1266+
.next(() => changeBuffer.apply(txn))
12681267
.next(() => documentCount);
12691268
}
12701269

‎packages/firestore/src/local/indexeddb_remote_document_cache.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -462,8 +462,7 @@ export class IndexedDbRemoteDocumentCache implements RemoteDocumentCache {
462462
}
463463

464464
protected applyChanges(
465-
transaction: PersistenceTransaction,
466-
readTime: SnapshotVersion
465+
transaction: PersistenceTransaction
467466
): PersistencePromise<void> {
468467
const promises: Array<PersistencePromise<void>> = [];
469468

@@ -479,7 +478,7 @@ export class IndexedDbRemoteDocumentCache implements RemoteDocumentCache {
479478
if (maybeDocument) {
480479
const doc = this.documentCache.serializer.toDbRemoteDocument(
481480
maybeDocument,
482-
readTime
481+
this.readTime!
483482
);
484483
const size = dbDocumentSize(doc);
485484
sizeDelta += size - previousSize!;

‎packages/firestore/src/local/indexeddb_schema.ts

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -308,7 +308,7 @@ export class DbTimestamp {
308308
}
309309

310310
/** A timestamp type that can be used in IndexedDb keys. */
311-
export type DbTimestampKey = number[];
311+
export type DbTimestampKey = [/* seconds */ number, /* nanos */ number];
312312

313313
// The key for the singleton object in the DbPrimaryClient is a single string.
314314
export type DbPrimaryClientKey = typeof DbPrimaryClient.key;
@@ -610,7 +610,7 @@ export class DbRemoteDocument {
610610
* An index that provides access to documents in a collection sorted by read
611611
* time.
612612
*
613-
* This index is used to allow Index-Free queries to fetch newly changed
613+
* This index is used to allow the RemoteDocumentCache to fetch newly changed
614614
* documents in a collection.
615615
*/
616616
static collectionReadTimeIndex = 'collectionReadTimeIndex';
@@ -642,11 +642,17 @@ export class DbRemoteDocument {
642642
*/
643643
public hasCommittedMutations: boolean | undefined,
644644

645-
/** When the document was read from the backend. */
645+
/**
646+
* When the document was read from the backend. Undefined for data written
647+
* prior to schema version 9.
648+
*/
646649
public readTime: DbTimestampKey | undefined,
647650

648-
/** The path of the collection this document is part of. */
649-
public parentPath: string | undefined
651+
/**
652+
* The path of the collection this document is part of. Undefined for data
653+
* written prior to schema version 9.
654+
*/
655+
public parentPath: string[] | undefined
650656
) {}
651657
}
652658

‎packages/firestore/src/local/local_serializer.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ export class LocalSerializer {
7676
readTime: SnapshotVersion
7777
): DbRemoteDocument {
7878
const dbReadTime = this.toDbTimestampKey(readTime);
79-
const parentPath = maybeDoc.key.path.popLast().canonicalString();
79+
const parentPath = maybeDoc.key.path.popLast().toArray();
8080
if (maybeDoc instanceof Document) {
8181
const doc = maybeDoc.proto
8282
? maybeDoc.proto
@@ -114,7 +114,7 @@ export class LocalSerializer {
114114
parentPath
115115
);
116116
} else {
117-
return fail('Unexpected MaybeDocumment');
117+
return fail('Unexpected MaybeDocument');
118118
}
119119
}
120120

‎packages/firestore/src/local/local_store.ts

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -351,7 +351,7 @@ export class LocalStore {
351351
.next(() =>
352352
this.applyWriteToRemoteDocuments(txn, batchResult, documentBuffer)
353353
)
354-
.next(() => documentBuffer.apply(txn, batchResult.commitVersion))
354+
.next(() => documentBuffer.apply(txn))
355355
.next(() => this.mutationQueue.performConsistencyCheck(txn))
356356
.next(() => this.localDocuments.getDocuments(txn, affected));
357357
}
@@ -522,7 +522,7 @@ export class LocalStore {
522522
(doc.version.compareTo(existingDoc.version) === 0 &&
523523
existingDoc.hasPendingWrites)
524524
) {
525-
documentBuffer.addEntry(doc);
525+
documentBuffer.addEntry(doc, remoteVersion);
526526
changedDocs = changedDocs.insert(key, doc);
527527
} else if (
528528
doc instanceof NoDocument &&
@@ -582,7 +582,7 @@ export class LocalStore {
582582
}
583583

584584
return PersistencePromise.waitFor(promises)
585-
.next(() => documentBuffer.apply(txn, remoteVersion))
585+
.next(() => documentBuffer.apply(txn))
586586
.next(() => {
587587
return this.localDocuments.getLocalViewOfDocuments(
588588
txn,
@@ -863,7 +863,10 @@ export class LocalStore {
863863
' resulted in null'
864864
);
865865
} else {
866-
documentBuffer.addEntry(doc);
866+
// We use the commitVersion as the readTime rather than the
867+
// document's updateTime since the updateTime is not advanced
868+
// for updates that do not modify the underlying document.
869+
documentBuffer.addEntry(doc, batchResult.commitVersion);
867870
}
868871
}
869872
});

‎packages/firestore/src/local/memory_persistence.ts

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,6 @@ import { PersistencePromise } from './persistence_promise';
4949
import { QueryData } from './query_data';
5050
import { ReferenceSet } from './reference_set';
5151
import { ClientId } from './shared_client_state';
52-
import { SnapshotVersion } from '../core/snapshot_version';
5352

5453
const LOG_TAG = 'MemoryPersistence';
5554

@@ -283,7 +282,7 @@ export class MemoryEagerDelegate implements ReferenceDelegate {
283282
}
284283
).next(() => {
285284
this._orphanedDocuments = null;
286-
return changeBuffer.apply(txn, SnapshotVersion.forDeletedDoc());
285+
return changeBuffer.apply(txn);
287286
});
288287
}
289288

@@ -421,9 +420,7 @@ export class MemoryLruDelegate implements ReferenceDelegate, LruDelegate {
421420
}
422421
});
423422
});
424-
return p
425-
.next(() => changeBuffer.apply(txn, SnapshotVersion.forDeletedDoc()))
426-
.next(() => count);
423+
return p.next(() => changeBuffer.apply(txn)).next(() => count);
427424
}
428425

429426
removeMutationReference(
@@ -471,7 +468,7 @@ export class MemoryLruDelegate implements ReferenceDelegate, LruDelegate {
471468
documentSize(maybeDoc: MaybeDocument): number {
472469
const remoteDocument = this.serializer.toDbRemoteDocument(
473470
maybeDoc,
474-
SnapshotVersion.forDeletedDoc()
471+
maybeDoc.version
475472
);
476473
let value: unknown;
477474
if (remoteDocument.document) {

‎packages/firestore/src/local/remote_document_change_buffer.ts

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,8 @@ export abstract class RemoteDocumentChangeBuffer {
4747
MaybeDocument | null
4848
> = new ObjectMap(key => key.toString());
4949

50+
protected readTime: SnapshotVersion | undefined;
51+
5052
private changesApplied = false;
5153

5254
protected abstract getFromCache(
@@ -60,8 +62,7 @@ export abstract class RemoteDocumentChangeBuffer {
6062
): PersistencePromise<NullableMaybeDocumentMap>;
6163

6264
protected abstract applyChanges(
63-
transaction: PersistenceTransaction,
64-
readTime: SnapshotVersion
65+
transaction: PersistenceTransaction
6566
): PersistencePromise<void>;
6667

6768
/**
@@ -70,8 +71,17 @@ export abstract class RemoteDocumentChangeBuffer {
7071
* You can only modify documents that have already been retrieved via
7172
* `getEntry()/getEntries()` (enforced via IndexedDbs `apply()`).
7273
*/
73-
addEntry(maybeDocument: MaybeDocument): void {
74+
addEntry(maybeDocument: MaybeDocument, readTime: SnapshotVersion): void {
7475
this.assertNotApplied();
76+
77+
// Assert that every read time matches since we only track a single read
78+
// time per document change set.
79+
assert(
80+
this.readTime === undefined || this.readTime.isEqual(readTime),
81+
'All changes in a RemoteDocumentChangeBuffer must have the same read time'
82+
);
83+
84+
this.readTime = readTime;
7585
this.changes.set(maybeDocument.key, maybeDocument);
7686
}
7787

@@ -132,13 +142,10 @@ export abstract class RemoteDocumentChangeBuffer {
132142
* Applies buffered changes to the underlying RemoteDocumentCache, using
133143
* the provided transaction.
134144
*/
135-
apply(
136-
transaction: PersistenceTransaction,
137-
readTime: SnapshotVersion
138-
): PersistencePromise<void> {
145+
apply(transaction: PersistenceTransaction): PersistencePromise<void> {
139146
this.assertNotApplied();
140147
this.changesApplied = true;
141-
return this.applyChanges(transaction, readTime);
148+
return this.applyChanges(transaction);
142149
}
143150

144151
/** Helper to assert this.changes is not null */

‎packages/firestore/src/local/simple_db.ts

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -108,9 +108,6 @@ export class SimpleDb {
108108
event.oldVersion
109109
);
110110
const db = (event.target as IDBOpenDBRequest).result;
111-
// We are provided a version upgrade transaction from the request, so
112-
// we wrap that in a SimpleDbTransaction to allow use of our friendlier
113-
// API for schema migration operations.
114111
schemaConverter
115112
.createOrUpgrade(
116113
db,

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,7 @@ apiDescribe('Database batch writes', (persistence: boolean) => {
125125
});
126126

127127
it('can delete documents', () => {
128+
// TODO(#1865): This test fails with node:persistence against Prod
128129
return integrationHelpers.withTestDoc(persistence, doc => {
129130
return doc
130131
.set({ foo: 'bar' })

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@ apiDescribe('Database', (persistence: boolean) => {
7070
});
7171

7272
it('can delete a document', () => {
73+
// TODO(#1865): This test fails with node:persistence against Prod
7374
return withTestDoc(persistence, docRef => {
7475
return docRef
7576
.set({ foo: 'bar' })

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

Lines changed: 73 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ import {
5252
import { LruParams } from '../../../src/local/lru_garbage_collector';
5353
import { PersistencePromise } from '../../../src/local/persistence_promise';
5454
import { ClientId } from '../../../src/local/shared_client_state';
55-
import { SimpleDb } from '../../../src/local/simple_db';
55+
import { SimpleDb, SimpleDbTransaction } from '../../../src/local/simple_db';
5656
import { PlatformSupport } from '../../../src/platform/platform';
5757
import { firestoreV1ApiClientInterfaces } from '../../../src/protos/firestore_proto_api';
5858
import { JsonProtoSerializer } from '../../../src/remote/serializer';
@@ -175,6 +175,24 @@ function getAllObjectStores(db: IDBDatabase): string[] {
175175
return objectStores;
176176
}
177177

178+
function addDocs(
179+
txn: SimpleDbTransaction,
180+
keys: string[],
181+
version: number
182+
): PersistencePromise<void> {
183+
const remoteDocumentStore = txn.store<DbRemoteDocumentKey, DbRemoteDocument>(
184+
DbRemoteDocument.store
185+
);
186+
return PersistencePromise.forEach(keys, (key: string) => {
187+
const remoteDoc = doc(key, version, { data: 'foo' });
188+
const dbRemoteDoc = TEST_SERIALIZER.toDbRemoteDocument(
189+
remoteDoc,
190+
remoteDoc.version
191+
);
192+
return remoteDocumentStore.put(remoteDoc.key.path.toArray(), dbRemoteDoc);
193+
});
194+
}
195+
178196
describe('IndexedDbSchema: createOrUpgradeDb', () => {
179197
if (!IndexedDbPersistence.isAvailable()) {
180198
console.warn('No IndexedDB. Skipping createOrUpgradeDb() tests.');
@@ -758,59 +776,70 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => {
758776
});
759777
});
760778

761-
// Migrate to v8 and verify that new documents are indexed.
779+
// Migrate to v9 and verify that new documents are indexed.
762780
await withDb(9, db => {
763781
const sdb = new SimpleDb(db);
764782
return sdb.runTransaction('readwrite', V8_STORES, txn => {
765-
const remoteDocumentStore = txn.store<
766-
DbRemoteDocumentKey,
767-
DbRemoteDocument
768-
>(DbRemoteDocument.store);
769-
770783
// Verify the existing remote document entries.
771-
return PersistencePromise.forEach(existingDocPaths, (path: string) => {
772-
const remoteDoc = doc(path, /*version=*/ 1, { data: 1 });
773-
774-
return remoteDocumentStore
775-
.get(remoteDoc.key.path.toArray())
776-
.next(value => {
777-
expect(value).to.be.not.null;
778-
});
779-
})
780-
.next(() => {
781-
// Add new entries, which will be added to the index.
782-
return PersistencePromise.forEach(newDocPaths, (path: string) => {
783-
const remoteDoc = doc(path, /*version=*/ 2, { data: 1 });
784-
const dbRemoteDoc = TEST_SERIALIZER.toDbRemoteDocument(
785-
remoteDoc,
786-
remoteDoc.version
787-
);
788-
return remoteDocumentStore.put(
789-
remoteDoc.key.path.toArray(),
790-
dbRemoteDoc
791-
);
792-
});
784+
return addDocs(txn, existingDocPaths, /* version= */ 1).next(() =>
785+
addDocs(txn, newDocPaths, /* version= */ 2).next(() => {
786+
const remoteDocumentStore = txn.store<
787+
DbRemoteDocumentKey,
788+
DbRemoteDocument
789+
>(DbRemoteDocument.store);
790+
791+
// Verify that we can get recent changes in a collection filtered by
792+
// read time.
793+
const lastReadTime = TEST_SERIALIZER.toDbTimestampKey(version(1));
794+
const range = IDBKeyRange.lowerBound(
795+
[['coll2'], lastReadTime],
796+
true
797+
);
798+
return remoteDocumentStore
799+
.loadAll(DbRemoteDocument.collectionReadTimeIndex, range)
800+
.next(docsRead => {
801+
const keys = docsRead.map(dbDoc => dbDoc.document!.name);
802+
expect(keys).to.have.members([
803+
'projects/test-project/databases/(default)/documents/coll2/doc3',
804+
'projects/test-project/databases/(default)/documents/coll2/doc4'
805+
]);
806+
});
793807
})
794-
.next(() => {
795-
// Verify that we can get recent changes in a collection filtered by read time.
796-
const docsRead: string[] = [];
797-
const lastReadTime = TEST_SERIALIZER.toDbTimestampKey(version(0));
808+
);
809+
});
810+
});
811+
});
812+
813+
it('RemoteDocumentCache can filter documents by read time', async () => {
814+
const oldDocPaths = ['coll/doc1', 'coll/doc2'];
815+
const newDocPaths = ['coll/doc3', 'coll/doc4'];
816+
817+
await withDb(9, db => {
818+
const sdb = new SimpleDb(db);
819+
return sdb.runTransaction('readwrite', V8_STORES, txn => {
820+
return addDocs(txn, oldDocPaths, /* version= */ 1).next(() =>
821+
addDocs(txn, newDocPaths, /* version= */ 2).next(() => {
822+
const remoteDocumentStore = txn.store<
823+
DbRemoteDocumentKey,
824+
DbRemoteDocument
825+
>(DbRemoteDocument.store);
826+
827+
const lastReadTime = TEST_SERIALIZER.toDbTimestampKey(version(1));
798828
const range = IDBKeyRange.lowerBound(
799-
['coll2', ...lastReadTime],
829+
[['coll'], lastReadTime],
800830
true
801831
);
802832
return remoteDocumentStore
803-
.iterate(
804-
{ index: DbRemoteDocument.collectionReadTimeIndex, range },
805-
(_, dbRemoteDoc) => {
806-
const doc = TEST_SERIALIZER.fromDbRemoteDocument(dbRemoteDoc);
807-
docsRead.push(doc.key.path.toString());
808-
}
809-
)
810-
.next(() => {
811-
expect(docsRead).to.have.members(['coll2/doc3', 'coll2/doc4']);
833+
.loadAll(DbRemoteDocument.collectionReadTimeIndex, range)
834+
.next(docsRead => {
835+
const keys = docsRead.map(dbDoc => dbDoc.document!.name);
836+
expect(keys).to.have.members([
837+
'projects/test-project/databases/(default)/documents/coll/doc3',
838+
'projects/test-project/databases/(default)/documents/coll/doc4'
839+
]);
812840
});
813-
});
841+
})
842+
);
814843
});
815844
});
816845
});

0 commit comments

Comments
 (0)