Skip to content

Commit 8142123

Browse files
schmidt-sebastianschmidt-sebastian
committed
Review feedback
1 parent b818ec9 commit 8142123

12 files changed

+83
-173
lines changed

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

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -75,8 +75,7 @@ const LOG_TAG = 'IndexedDbPersistence';
7575

7676
/**
7777
* Oldest acceptable age in milliseconds for client metadata before the client
78-
* is considered inactive and its associated data (such as the remote document
79-
* cache changelog) is garbage collected.
78+
* is considered inactive and its associated data is garbage collected.
8079
*/
8180
const MAX_CLIENT_AGE_MS = 30 * 60 * 1000; // 30 minutes
8281

@@ -259,7 +258,7 @@ export class IndexedDbPersistence implements Persistence {
259258
/** The client metadata refresh task. */
260259
private clientMetadataRefresher: CancelablePromise<void> | null = null;
261260

262-
/** The last time we garbage collected the Remote Document Changelog. */
261+
/** The last time we garbage collected the client metadata object store. */
263262
private lastGarbageCollectionTime = Number.NEGATIVE_INFINITY;
264263

265264
/** Whether to allow shared multi-tab access to the persistence layer. */
@@ -1210,9 +1209,7 @@ export class IndexedDbLruDelegate implements ReferenceDelegate, LruDelegate {
12101209
upperBound: ListenSequenceNumber
12111210
): PersistencePromise<number> {
12121211
const documentCache = this.db.getRemoteDocumentCache();
1213-
const changeBuffer = documentCache.newChangeBuffer({
1214-
createSentinelDocumentsToTrackDeletes: false
1215-
});
1212+
const changeBuffer = documentCache.newChangeBuffer();
12161213

12171214
const promises: Array<PersistencePromise<void>> = [];
12181215
let documentCount = 0;

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

Lines changed: 22 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,6 @@ import { RemoteDocumentCache } from './remote_document_cache';
4848
import { RemoteDocumentChangeBuffer } from './remote_document_change_buffer';
4949
import { SimpleDb, SimpleDbStore, SimpleDbTransaction } from './simple_db';
5050
import { ObjectMap } from '../util/obj_map';
51-
import { Timestamp } from '../api/timestamp';
5251

5352
export class IndexedDbRemoteDocumentCache implements RemoteDocumentCache {
5453
/** The read time of the last entry consumed by `getNewDocumentChanges()`. */
@@ -111,7 +110,8 @@ export class IndexedDbRemoteDocumentCache implements RemoteDocumentCache {
111110
}
112111

113112
/**
114-
* Updates the document change log and adds the given delta to the cached current size.
113+
* Updates the current size.
114+
*
115115
* Callers to `addEntry()` and `removeEntry()` *must* call this afterwards to update the
116116
* cache's metadata.
117117
*/
@@ -307,6 +307,8 @@ export class IndexedDbRemoteDocumentCache implements RemoteDocumentCache {
307307
.iterate(
308308
{ index: DbRemoteDocument.readTimeIndex, range },
309309
(_, dbRemoteDoc) => {
310+
// Unlike `getEntry()` and others, `getNewDocumentChanges()` parses
311+
// the documents directly since we want to keep sentinel deletes.
310312
const doc = this.serializer.fromDbRemoteDocument(dbRemoteDoc);
311313
changedDocs = changedDocs.insert(doc.key, doc);
312314
this.lastProcessedReadTime = this.serializer.fromDbTimestampKey(
@@ -318,8 +320,9 @@ export class IndexedDbRemoteDocumentCache implements RemoteDocumentCache {
318320
}
319321

320322
/**
321-
* Sets the last processed read time to the maximum read time from cache,
322-
* allowing calls to getNewDocumentChanges() to return subsequent changes.
323+
* Sets the last processed read time to the maximum read time of the backing
324+
* object store, allowing calls to getNewDocumentChanges() to return subsequent
325+
* changes.
323326
*/
324327
private synchronizeLastProcessedReadTime(
325328
transaction: SimpleDbTransaction
@@ -335,21 +338,21 @@ export class IndexedDbRemoteDocumentCache implements RemoteDocumentCache {
335338
{ index: DbRemoteDocument.readTimeIndex, reverse: true },
336339
(key, value, control) => {
337340
if (value.readTime) {
338-
this.lastProcessedReadTime = SnapshotVersion.fromTimestamp(
339-
new Timestamp(value.readTime![0], value.readTime![1])
341+
this.lastProcessedReadTime = this.serializer.fromDbTimestampKey(
342+
value.readTime
340343
);
341344
}
342345
control.done();
343346
}
344347
);
345348
}
346349

347-
newChangeBuffer(options: {
348-
createSentinelDocumentsToTrackDeletes: boolean;
350+
newChangeBuffer(options?: {
351+
trackRemovals: boolean;
349352
}): RemoteDocumentChangeBuffer {
350353
return new IndexedDbRemoteDocumentCache.RemoteDocumentChangeBuffer(
351354
this,
352-
options.createSentinelDocumentsToTrackDeletes
355+
!!options && options.trackRemovals
353356
);
354357
}
355358

@@ -388,8 +391,8 @@ export class IndexedDbRemoteDocumentCache implements RemoteDocumentCache {
388391
doc instanceof NoDocument &&
389392
doc.version.isEqual(SnapshotVersion.forDeletedDoc())
390393
) {
391-
// The document is a sentinel delete and should only be used in the
392-
// change log.
394+
// The document is a sentinel removal and should only be used in the
395+
// `getNewDocumentChanges()`.
393396
return null;
394397
}
395398

@@ -413,12 +416,12 @@ export class IndexedDbRemoteDocumentCache implements RemoteDocumentCache {
413416

414417
/**
415418
* @param documentCache The IndexedDbRemoteDocumentCache to apply the changes to.
416-
* @param trackDeletes Whether to create sentinel deletes that can be tracked by
419+
* @param trackRemovals Whether to create sentinel deletes that can be tracked by
417420
* `getNewDocumentChanges()`.
418421
*/
419422
constructor(
420423
private readonly documentCache: IndexedDbRemoteDocumentCache,
421-
private readonly trackDeletes: boolean
424+
private readonly trackRemovals: boolean
422425
) {
423426
super();
424427
}
@@ -446,11 +449,14 @@ export class IndexedDbRemoteDocumentCache implements RemoteDocumentCache {
446449
promises.push(this.documentCache.addEntry(transaction, key, doc));
447450
} else {
448451
sizeDelta -= previousSize!;
449-
if (this.trackDeletes) {
450-
// A sentinel delete is symbolized by a NoDocument with a version of 0.
452+
if (this.trackRemovals) {
453+
// In order to track removals, we store a "sentinel delete" in the
454+
// RemoteDocumentCache. This entry is represented by a NoDocument
455+
// with a version of 0 and ignored by `maybeDecodeDocument()` but
456+
// preserved in `getNewDocumentChanges()`.
451457
const deletedDoc = this.documentCache.serializer.toDbRemoteDocument(
452458
new NoDocument(key, SnapshotVersion.forDeletedDoc()),
453-
readTime
459+
this.readTime!
454460
);
455461
promises.push(
456462
this.documentCache.addEntry(transaction, key, deletedDoc)

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

Lines changed: 7 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -138,13 +138,10 @@ export class SchemaConverter implements SimpleDbSchemaConverter {
138138

139139
if (fromVersion < 9 && toVersion >= 9) {
140140
p = p.next(() => {
141-
<<<<<<< HEAD
142141
// Multi-Tab used to manage its own changelog, but this has been moved
143142
// to the DbRemoteDocument object store itself. Since the previous change
144143
// log only contained transient data, we can drop its object store.
145144
dropRemoteDocumentChangesStore(db);
146-
=======
147-
>>>>>>> b4d70ace... Index-Free: Track readTime in the RemoteDocument store (#2125)
148145
createRemoteDocumentReadTimeIndex(txn);
149146
});
150147
}
@@ -613,7 +610,6 @@ export class DbRemoteDocument {
613610
static store = 'remoteDocuments';
614611

615612
/**
616-
<<<<<<< HEAD
617613
* An index that provides access to all entries sorted by read time (which
618614
* corresponds to the last modification time of each row).
619615
*
@@ -624,8 +620,6 @@ export class DbRemoteDocument {
624620
static readTimeIndexPath = 'readTime';
625621

626622
/**
627-
=======
628-
>>>>>>> b4d70ace... Index-Free: Track readTime in the RemoteDocument store (#2125)
629623
* An index that provides access to documents in a collection sorted by read
630624
* time.
631625
*
@@ -636,6 +630,12 @@ export class DbRemoteDocument {
636630

637631
static collectionReadTimeIndexPath = ['parentPath', 'readTime'];
638632

633+
// TODO: We are currently storing full document keys almost three times
634+
// (once as part of the primary key, once - partly - as `parentPath` and once
635+
// inside the encoded documents). During our next migration, we should
636+
// rewrite the primary key as parentPath + document ID which would allow us
637+
// to drop one value.
638+
639639
constructor(
640640
/**
641641
* Set to an instance of DbUnknownDocument if the data for a document is
@@ -1003,19 +1003,6 @@ function createRemoteDocumentReadTimeIndex(txn: IDBTransaction): void {
10031003
);
10041004
}
10051005

1006-
/**
1007-
* Creates indices on the RemoteDocuments store used for both multi-tab
1008-
* and Index-Free queries.
1009-
*/
1010-
function createRemoteDocumentReadTimeIndex(txn: IDBTransaction): void {
1011-
const remoteDocumentStore = txn.objectStore(DbRemoteDocument.store);
1012-
remoteDocumentStore.createIndex(
1013-
DbRemoteDocument.collectionReadTimeIndex,
1014-
DbRemoteDocument.collectionReadTimeIndexPath,
1015-
{ unique: false }
1016-
);
1017-
}
1018-
10191006
/**
10201007
* A record of the metadata state of each client.
10211008
*
@@ -1071,6 +1058,7 @@ export const V1_STORES = [
10711058
export const V3_STORES = V1_STORES;
10721059

10731060
// Visible for testing
1061+
// Note: DbRemoteDocumentChanges is no longer used and dropped with v9.
10741062
export const V4_STORES = [...V3_STORES, DbClientMetadata.store];
10751063

10761064
// V5 does not change the set of stores.

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

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -123,14 +123,11 @@ export class LocalSerializer {
123123
return [timestamp.seconds, timestamp.nanoseconds];
124124
}
125125

126-
<<<<<<< HEAD
127126
fromDbTimestampKey(dbTimestampKey: DbTimestampKey): SnapshotVersion {
128127
const timestamp = new Timestamp(dbTimestampKey[0], dbTimestampKey[1]);
129128
return SnapshotVersion.fromTimestamp(timestamp);
130129
}
131130

132-
=======
133-
>>>>>>> b4d70ace... Index-Free: Track readTime in the RemoteDocument store (#2125)
134131
private toDbTimestamp(snapshotVersion: SnapshotVersion): DbTimestamp {
135132
const timestamp = snapshotVersion.toTimestamp();
136133
return new DbTimestamp(timestamp.seconds, timestamp.nanoseconds);

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

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -346,7 +346,7 @@ export class LocalStore {
346346
txn => {
347347
const affected = batchResult.batch.keys();
348348
const documentBuffer = this.remoteDocuments.newChangeBuffer({
349-
createSentinelDocumentsToTrackDeletes: true
349+
trackRemovals: true // Make sure document removals show up in `getNewDocumentChanges()`
350350
});
351351
return this.mutationQueue
352352
.acknowledgeBatch(txn, batchResult.batch, batchResult.streamToken)
@@ -450,13 +450,9 @@ export class LocalStore {
450450
* queue.
451451
*/
452452
applyRemoteEvent(remoteEvent: RemoteEvent): Promise<MaybeDocumentMap> {
453-
<<<<<<< HEAD
454453
const documentBuffer = this.remoteDocuments.newChangeBuffer({
455-
createSentinelDocumentsToTrackDeletes: true
454+
trackRemovals: true // Make sure document removals show up in `getNewDocumentChanges()`
456455
});
457-
=======
458-
const documentBuffer = this.remoteDocuments.newChangeBuffer();
459-
>>>>>>> b4d70ace... Index-Free: Track readTime in the RemoteDocument store (#2125)
460456
const remoteVersion = remoteEvent.snapshotVersion;
461457
return this.persistence.runTransaction(
462458
'Apply remote event',

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

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -270,9 +270,7 @@ export class MemoryEagerDelegate implements ReferenceDelegate {
270270
): PersistencePromise<void> {
271271
// Remove newly orphaned documents.
272272
const cache = this.persistence.getRemoteDocumentCache();
273-
const changeBuffer = cache.newChangeBuffer({
274-
createSentinelDocumentsToTrackDeletes: false
275-
});
273+
const changeBuffer = cache.newChangeBuffer();
276274
return PersistencePromise.forEach(
277275
this.orphanedDocuments,
278276
(key: DocumentKey) => {
@@ -413,9 +411,7 @@ export class MemoryLruDelegate implements ReferenceDelegate, LruDelegate {
413411
): PersistencePromise<number> {
414412
let count = 0;
415413
const cache = this.persistence.getRemoteDocumentCache();
416-
const changeBuffer = cache.newChangeBuffer({
417-
createSentinelDocumentsToTrackDeletes: false
418-
});
414+
const changeBuffer = cache.newChangeBuffer();
419415
const p = cache.forEachDocumentKey(txn, key => {
420416
return this.isPinned(txn, key, upperBound).next(isPinned => {
421417
if (!isPinned) {

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -178,9 +178,11 @@ export class MemoryRemoteDocumentCache implements RemoteDocumentCache {
178178
return PersistencePromise.resolve(changedDocs);
179179
}
180180

181-
newChangeBuffer(options: {
182-
createSentinelDocumentsToTrackDeletes: boolean;
181+
newChangeBuffer(options?: {
182+
trackRemovals: boolean;
183183
}): RemoteDocumentChangeBuffer {
184+
// `trackRemovals` is ignores since the MemoryRemoteDocumentCache keeps
185+
// a separate changelog and does not need special handling for removals.
184186
return new MemoryRemoteDocumentCache.RemoteDocumentChangeBuffer(this);
185187
}
186188

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

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -81,11 +81,8 @@ export interface RemoteDocumentCache {
8181
/**
8282
* Returns the set of documents that have been updated since the last call.
8383
* If this is the first call, returns the set of changes since client
84-
* initialization.
85-
*
86-
* If the changelog was garbage collected and can no longer be replayed,
87-
* `getNewDocumentChanges` will reject the returned Promise. Further
88-
* invocations will return document changes since the point of rejection.
84+
* initialization. Further invocations will return document changes since
85+
* the point of rejection.
8986
*/
9087
// PORTING NOTE: This is only used for multi-tab synchronization.
9188
getNewDocumentChanges(
@@ -98,12 +95,12 @@ export interface RemoteDocumentCache {
9895
*
9996
* Multi-Tab Note: This should only be called by the primary client.
10097
*
101-
* @param options.createSentinelDocumentsToTrackDeletes Whether to create
102-
* sentinel entries for deleted documents, which allow deletes to be tracked
103-
* by `getNewDocumentChanges()`.
98+
* @param options.trackRemovals Whether to create sentinel entries for
99+
* removed documents, which allows removals to be tracked by
100+
* `getNewDocumentChanges()`.
104101
*/
105-
newChangeBuffer(options: {
106-
createSentinelDocumentsToTrackDeletes?: boolean;
102+
newChangeBuffer(options?: {
103+
trackRemovals: boolean;
107104
}): RemoteDocumentChangeBuffer;
108105

109106
/**

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

Lines changed: 34 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -820,9 +820,9 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => {
820820
});
821821
});
822822

823-
it('RemoteDocumentCache can filter documents by read time', async () => {
824-
const oldDocPaths = ['coll/doc1', 'coll/doc2'];
825-
const newDocPaths = ['coll/doc3', 'coll/doc4'];
823+
it('can get recent document changes in a collection', async () => {
824+
const oldDocPaths = ['coll/doc1', 'coll/doc2', 'ignored/doc1'];
825+
const newDocPaths = ['coll/doc3', 'coll/doc4', 'ignored/doc2'];
826826

827827
await withDb(9, db => {
828828
const sdb = new SimpleDb(db);
@@ -854,6 +854,37 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => {
854854
});
855855
});
856856

857+
it('can get recent document changes', async () => {
858+
const oldDocPaths = ['coll1/old', 'coll2/old'];
859+
const newDocPaths = ['coll1/new', 'coll2/new'];
860+
861+
await withDb(9, db => {
862+
const sdb = new SimpleDb(db);
863+
return sdb.runTransaction('readwrite', V8_STORES, txn => {
864+
return addDocs(txn, oldDocPaths, /* version= */ 1).next(() =>
865+
addDocs(txn, newDocPaths, /* version= */ 2).next(() => {
866+
const remoteDocumentStore = txn.store<
867+
DbRemoteDocumentKey,
868+
DbRemoteDocument
869+
>(DbRemoteDocument.store);
870+
871+
const lastReadTime = TEST_SERIALIZER.toDbTimestampKey(version(1));
872+
const range = IDBKeyRange.lowerBound(lastReadTime, true);
873+
return remoteDocumentStore
874+
.loadAll(DbRemoteDocument.readTimeIndex, range)
875+
.next(docsRead => {
876+
const keys = docsRead.map(dbDoc => dbDoc.document!.name);
877+
expect(keys).to.have.members([
878+
'projects/test-project/databases/(default)/documents/coll1/new',
879+
'projects/test-project/databases/(default)/documents/coll2/new'
880+
]);
881+
});
882+
})
883+
);
884+
});
885+
});
886+
});
887+
857888
it('downgrading throws a custom error', async () => {
858889
// Upgrade to latest version
859890
await withDb(SCHEMA_VERSION, async db => {

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

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -252,9 +252,7 @@ function genericLruGarbageCollectorTests(
252252
txn: PersistenceTransaction,
253253
doc: MaybeDocument
254254
): PersistencePromise<void> {
255-
const changeBuffer = documentCache.newChangeBuffer({
256-
createSentinelDocumentsToTrackDeletes: false
257-
});
255+
const changeBuffer = documentCache.newChangeBuffer();
258256
return changeBuffer.getEntry(txn, doc.key).next(() => {
259257
changeBuffer.addEntry(doc, doc.version);
260258
return changeBuffer.apply(txn);

0 commit comments

Comments
 (0)