Skip to content

Commit 4879484

Browse files
schmidt-sebastianschmidt-sebastian
committed
Review feedback
1 parent 4800897 commit 4879484

File tree

7 files changed

+72
-73
lines changed

7 files changed

+72
-73
lines changed

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

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1209,9 +1209,7 @@ export class IndexedDbLruDelegate implements ReferenceDelegate, LruDelegate {
12091209
upperBound: ListenSequenceNumber
12101210
): PersistencePromise<number> {
12111211
const documentCache = this.db.getRemoteDocumentCache();
1212-
const changeBuffer = documentCache.newChangeBuffer({
1213-
trackRemovals: false
1214-
});
1212+
const changeBuffer = documentCache.newChangeBuffer();
12151213

12161214
const promises: Array<PersistencePromise<void>> = [];
12171215
let documentCount = 0;

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

Lines changed: 14 additions & 12 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()`. */
@@ -308,8 +307,8 @@ export class IndexedDbRemoteDocumentCache implements RemoteDocumentCache {
308307
.iterate(
309308
{ index: DbRemoteDocument.readTimeIndex, range },
310309
(_, dbRemoteDoc) => {
311-
// Unlike `getEntry()` and others, `getNewDocumentChanges()` uses the
312-
// serializer directly since we want to keep sentinel deletes.
310+
// Unlike `getEntry()` and others, `getNewDocumentChanges()` parses
311+
// the documents directly since we want to keep sentinel deletes.
313312
const doc = this.serializer.fromDbRemoteDocument(dbRemoteDoc);
314313
changedDocs = changedDocs.insert(doc.key, doc);
315314
this.lastProcessedReadTime = this.serializer.fromDbTimestampKey(
@@ -321,8 +320,9 @@ export class IndexedDbRemoteDocumentCache implements RemoteDocumentCache {
321320
}
322321

323322
/**
324-
* Sets the last processed read time to the maximum read time from cache,
325-
* 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.
326326
*/
327327
private synchronizeLastProcessedReadTime(
328328
transaction: SimpleDbTransaction
@@ -338,19 +338,21 @@ export class IndexedDbRemoteDocumentCache implements RemoteDocumentCache {
338338
{ index: DbRemoteDocument.readTimeIndex, reverse: true },
339339
(key, value, control) => {
340340
if (value.readTime) {
341-
this.lastProcessedReadTime = this.serializer.fromDbTimestampKey(value.readTime)
341+
this.lastProcessedReadTime = this.serializer.fromDbTimestampKey(
342+
value.readTime
343+
);
342344
}
343345
control.done();
344346
}
345347
);
346348
}
347349

348-
newChangeBuffer(options: {
350+
newChangeBuffer(options?: {
349351
trackRemovals: boolean;
350352
}): RemoteDocumentChangeBuffer {
351353
return new IndexedDbRemoteDocumentCache.RemoteDocumentChangeBuffer(
352354
this,
353-
options.trackRemovals
355+
!!options && options.trackRemovals
354356
);
355357
}
356358

@@ -448,10 +450,10 @@ export class IndexedDbRemoteDocumentCache implements RemoteDocumentCache {
448450
} else {
449451
sizeDelta -= previousSize!;
450452
if (this.trackRemovals) {
451-
// In order to track removals, we store "sentinel delete" entries in
452-
// the Remote Document Cache. These entries are represented by a
453-
// NoDocument with a version of 0 and are ignored by
454-
// `maybeDecodeDocument()` but preserved in `getNewDocumentChanges()`.
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()`.
455457
const deletedDoc = this.documentCache.serializer.toDbRemoteDocument(
456458
new NoDocument(key, SnapshotVersion.forDeletedDoc()),
457459
this.readTime!

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

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -630,11 +630,11 @@ export class DbRemoteDocument {
630630

631631
static collectionReadTimeIndexPath = ['parentPath', 'readTime'];
632632

633-
// TODO: We are currently storing document keys three times (once as part
634-
// of the primary key, once as `parentPath` (mostly) and once inside the
635-
// encoded documents). During our next migration, we should rewrite the
636-
// primary key as parentPath + document ID which would allow us to drop
637-
// one value.
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.
638638

639639
constructor(
640640
/**
@@ -1017,6 +1017,9 @@ export class DbClientMetadata {
10171017
static keyPath = 'clientId';
10181018

10191019
constructor(
1020+
// Note: Previous schema versions included a field
1021+
// "lastProcessedDocumentChangeId". Don't use anymore.
1022+
10201023
/** The auto-generated client id assigned at client startup. */
10211024
public clientId: string,
10221025
/** The last time this state was updated. */
@@ -1025,9 +1028,6 @@ export class DbClientMetadata {
10251028
public networkEnabled: boolean,
10261029
/** Whether this client is running in a foreground tab. */
10271030
public inForeground: boolean
1028-
1029-
// Note: Previous schema versions included a field
1030-
// "lastProcessedDocumentChangeId". Don't use anymore.
10311031
) {}
10321032
}
10331033

@@ -1058,7 +1058,7 @@ export const V1_STORES = [
10581058
export const V3_STORES = V1_STORES;
10591059

10601060
// Visible for testing
1061-
// Note: DbRemoteDocumentChanges is no longer used and dropped with version 9
1061+
// Note: DbRemoteDocumentChanges is no longer used and dropped with v9.
10621062
export const V4_STORES = [...V3_STORES, DbClientMetadata.store];
10631063

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

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -346,8 +346,8 @@ export class LocalStore {
346346
txn => {
347347
const affected = batchResult.batch.keys();
348348
const documentBuffer = this.remoteDocuments.newChangeBuffer(
349-
{trackRemovals: true} // Make sure document removals show up in `getNewDocumentChanges()`
350-
);
349+
{ trackRemovals: true } // Make sure document removals show up in `getNewDocumentChanges()`
350+
);
351351
return this.mutationQueue
352352
.acknowledgeBatch(txn, batchResult.batch, batchResult.streamToken)
353353
.next(() =>
@@ -451,7 +451,7 @@ export class LocalStore {
451451
*/
452452
applyRemoteEvent(remoteEvent: RemoteEvent): Promise<MaybeDocumentMap> {
453453
const documentBuffer = this.remoteDocuments.newChangeBuffer({
454-
trackRemovals: true // Make sure document removals show up in `getNewDocumentChanges()`
454+
trackRemovals: true // Make sure document removals show up in `getNewDocumentChanges()`
455455
});
456456
const remoteVersion = remoteEvent.snapshotVersion;
457457
return this.persistence.runTransaction(

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -181,7 +181,7 @@ export class MemoryRemoteDocumentCache implements RemoteDocumentCache {
181181
newChangeBuffer(options?: {
182182
trackRemovals: boolean;
183183
}): RemoteDocumentChangeBuffer {
184-
// `trackRemovals` is ignores since the memory remote document cache keeps
184+
// `trackRemovals` is ignores since the MemoryRemoteDocumentCache keeps
185185
// a separate changelog and does not need special handling for removals.
186186
return new MemoryRemoteDocumentCache.RemoteDocumentChangeBuffer(this);
187187
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@ export interface RemoteDocumentCache {
9696
* Multi-Tab Note: This should only be called by the primary client.
9797
*
9898
* @param options.trackRemovals Whether to create sentinel entries for
99-
* removed documents, which allow removals to be tracked by
99+
* removed documents, which allows removals to be tracked by
100100
* `getNewDocumentChanges()`.
101101
*/
102102
newChangeBuffer(options?: {

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

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

823-
it('can get recent document changes', 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);
829829
return sdb.runTransaction('readwrite', V8_STORES, txn => {
830830
return addDocs(txn, oldDocPaths, /* version= */ 1).next(() =>
831-
addDocs(txn, newDocPaths, /* version= */ 2).next(() => {
832-
const remoteDocumentStore = txn.store<
833-
DbRemoteDocumentKey,
834-
DbRemoteDocument
835-
>(DbRemoteDocument.store);
836-
837-
const lastReadTime = TEST_SERIALIZER.toDbTimestampKey(version(1));
838-
const range = IDBKeyRange.lowerBound(
839-
[['coll'], lastReadTime],
840-
true
841-
);
842-
return remoteDocumentStore
843-
.loadAll(DbRemoteDocument.collectionReadTimeIndex, range)
844-
.next(docsRead => {
845-
const keys = docsRead.map(dbDoc => dbDoc.document!.name);
846-
expect(keys).to.have.members([
847-
'projects/test-project/databases/(default)/documents/coll/doc3',
848-
'projects/test-project/databases/(default)/documents/coll/doc4'
849-
]);
850-
});
851-
})
831+
addDocs(txn, newDocPaths, /* version= */ 2).next(() => {
832+
const remoteDocumentStore = txn.store<
833+
DbRemoteDocumentKey,
834+
DbRemoteDocument
835+
>(DbRemoteDocument.store);
836+
837+
const lastReadTime = TEST_SERIALIZER.toDbTimestampKey(version(1));
838+
const range = IDBKeyRange.lowerBound(
839+
[['coll'], lastReadTime],
840+
true
841+
);
842+
return remoteDocumentStore
843+
.loadAll(DbRemoteDocument.collectionReadTimeIndex, range)
844+
.next(docsRead => {
845+
const keys = docsRead.map(dbDoc => dbDoc.document!.name);
846+
expect(keys).to.have.members([
847+
'projects/test-project/databases/(default)/documents/coll/doc3',
848+
'projects/test-project/databases/(default)/documents/coll/doc4'
849+
]);
850+
});
851+
})
852852
);
853853
});
854854
});
855855
});
856856

857-
it('can get recent document changes in a collection', async () => {
857+
it('can get recent document changes', async () => {
858858
const oldDocPaths = ['coll1/old', 'coll2/old'];
859859
const newDocPaths = ['coll1/new', 'coll2/new'];
860860

861861
await withDb(9, db => {
862862
const sdb = new SimpleDb(db);
863863
return sdb.runTransaction('readwrite', V8_STORES, txn => {
864864
return addDocs(txn, oldDocPaths, /* version= */ 1).next(() =>
865-
addDocs(txn, newDocPaths, /* version= */ 2).next(() => {
866-
const remoteDocumentStore = txn.store<DbRemoteDocumentKey,
867-
DbRemoteDocument>(DbRemoteDocument.store);
868-
869-
const lastReadTime = TEST_SERIALIZER.toDbTimestampKey(version(1));
870-
const range = IDBKeyRange.lowerBound(
871-
lastReadTime,
872-
true
873-
);
874-
return remoteDocumentStore
875-
.loadAll(DbRemoteDocument.readTimeIndex, range)
876-
.next(docsRead => {
877-
const keys = docsRead.map(dbDoc => dbDoc.document!.name);
878-
expect(keys).to.have.members([
879-
'projects/test-project/databases/(default)/documents/coll1/new',
880-
'projects/test-project/databases/(default)/documents/coll2/new'
881-
]);
882-
});
883-
})
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+
})
884883
);
885884
});
886885
});

0 commit comments

Comments
 (0)