Skip to content

Commit 0fc5c88

Browse files
ehsannasehsannas
committed
Address feedback.
1 parent b94e9cb commit 0fc5c88

File tree

6 files changed

+62
-78
lines changed

6 files changed

+62
-78
lines changed

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

Lines changed: 15 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -183,8 +183,8 @@ export class LocalDocumentsView {
183183
let recalculateDocuments = mutableDocumentMap();
184184
docs.forEach((_, doc) => {
185185
const overlay = overlays.get(doc.key);
186-
// Recalculate an overlay if the document's existence state is changed due
187-
// to a remote event *and* the overlay is a PatchMutation. This is because
186+
// Recalculate an overlay if the document's existence state changed due to
187+
// a remote event *and* the overlay is a PatchMutation. This is because
188188
// document existence state can change if some patch mutation's
189189
// preconditions are met.
190190
// NOTE: we recalculate when `overlay` is undefined as well, because there
@@ -227,24 +227,19 @@ export class LocalDocumentsView {
227227
for (const batch of batches) {
228228
batch.keys().forEach(key => {
229229
const baseDoc = docs.get(key);
230-
if (baseDoc !== null) {
231-
let mask: FieldMask | null = masks.has(key)
232-
? masks.get(key)!
233-
: FieldMask.empty();
234-
mask = batch.applyToLocalView(baseDoc, mask);
235-
masks.set(key, mask);
236-
if (documentsByBatchId.get(batch.batchId) === null) {
237-
documentsByBatchId = documentsByBatchId.insert(
238-
batch.batchId,
239-
documentKeySet()
240-
);
241-
}
242-
const newSet = documentsByBatchId.get(batch.batchId)!.add(key);
243-
documentsByBatchId = documentsByBatchId.insert(
244-
batch.batchId,
245-
newSet
246-
);
230+
if (baseDoc === null) {
231+
return;
247232
}
233+
let mask: FieldMask | null = masks.get(key) ?? FieldMask.empty();
234+
mask = batch.applyToLocalView(baseDoc, mask);
235+
masks.set(key, mask);
236+
const newSet = (
237+
documentsByBatchId.get(batch.batchId) ?? documentKeySet()
238+
).add(key);
239+
documentsByBatchId = documentsByBatchId.insert(
240+
batch.batchId,
241+
newSet
242+
);
248243
});
249244
}
250245
})
@@ -292,9 +287,7 @@ export class LocalDocumentsView {
292287
): PersistencePromise<void> {
293288
return this.remoteDocumentCache
294289
.getEntries(transaction, documentKeys)
295-
.next(docs => {
296-
return this.recalculateAndSaveOverlays(transaction, docs);
297-
});
290+
.next(docs => this.recalculateAndSaveOverlays(transaction, docs));
298291
}
299292

300293
/**

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

Lines changed: 9 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -323,8 +323,9 @@ export function localStoreWriteLocally(
323323
// Figure out which keys do not have a remote version in the cache, this
324324
// is needed to create the right overlay mutation: if no remote version
325325
// presents, we do not need to create overlays as patch mutations.
326-
// TODO(Overlay): Is there a better way to determine this? Document
327-
// version does not work because local mutations set them back to 0.
326+
// TODO(Overlay): Is there a better way to determine this? Using the
327+
// document version does not work because local mutations set them back
328+
// to 0.
328329
let remoteDocs = mutableDocumentMap();
329330
let docsWithoutRemoteVersion = documentKeySet();
330331
return localStoreImpl.remoteDocuments
@@ -705,16 +706,16 @@ function populateDocumentChangeBuffer(
705706
documents: MutableDocumentMap
706707
): PersistencePromise<DocumentChangeResult> {
707708
let updatedKeys = documentKeySet();
708-
let conditionChanged = documentKeySet();
709+
let existenceChangedKeys = documentKeySet();
709710
documents.forEach(k => (updatedKeys = updatedKeys.add(k)));
710711
return documentBuffer.getEntries(txn, updatedKeys).next(existingDocs => {
711-
let changedDocs = mutableDocumentMap();
712+
let changedDocuments = mutableDocumentMap();
712713
documents.forEach((key, doc) => {
713714
const existingDoc = existingDocs.get(key)!;
714715

715716
// Check if see if there is a existence state change for this document.
716717
if (doc.isFoundDocument() !== existingDoc.isFoundDocument()) {
717-
conditionChanged = conditionChanged.add(key);
718+
existenceChangedKeys = existenceChangedKeys.add(key);
718719
}
719720

720721
// Note: The order of the steps below is important, since we want
@@ -726,7 +727,7 @@ function populateDocumentChangeBuffer(
726727
// events. We remove these documents from cache since we lost
727728
// access.
728729
documentBuffer.removeEntry(key, doc.readTime);
729-
changedDocs = changedDocs.insert(key, doc);
730+
changedDocuments = changedDocuments.insert(key, doc);
730731
} else if (
731732
!existingDoc.isValidDocument() ||
732733
doc.version.compareTo(existingDoc.version) > 0 ||
@@ -738,7 +739,7 @@ function populateDocumentChangeBuffer(
738739
'Cannot add a document when the remote version is zero'
739740
);
740741
documentBuffer.addEntry(doc);
741-
changedDocs = changedDocs.insert(key, doc);
742+
changedDocuments = changedDocuments.insert(key, doc);
742743
} else {
743744
logDebug(
744745
LOG_TAG,
@@ -751,10 +752,7 @@ function populateDocumentChangeBuffer(
751752
);
752753
}
753754
});
754-
return {
755-
changedDocuments: changedDocs,
756-
existenceChangedKeys: conditionChanged
757-
};
755+
return { changedDocuments, existenceChangedKeys };
758756
});
759757
}
760758

‎packages/firestore/src/model/field_mask.ts

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717

1818
import { debugAssert } from '../util/assert';
1919
import { arrayEquals } from '../util/misc';
20+
import { SortedSet } from '../util/sorted_set';
2021

2122
import { FieldPath } from './path';
2223

@@ -46,6 +47,21 @@ export class FieldMask {
4647
return new FieldMask([]);
4748
}
4849

50+
/**
51+
* Returns a new FieldMask object that is the result of adding all the given
52+
* fields paths to this field mask.
53+
*/
54+
unionWith(extraFields: FieldPath[]): FieldMask {
55+
let mergedMaskSet = new SortedSet<FieldPath>(FieldPath.comparator);
56+
for (const fieldPath of this.fields) {
57+
mergedMaskSet = mergedMaskSet.add(fieldPath);
58+
}
59+
for (const fieldPath of extraFields) {
60+
mergedMaskSet = mergedMaskSet.add(fieldPath);
61+
}
62+
return new FieldMask(mergedMaskSet.toArray());
63+
}
64+
4965
/**
5066
* Verifies that `fieldPath` is included by at least one field in this field
5167
* mask.

‎packages/firestore/src/model/mutation.ts

Lines changed: 3 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -571,17 +571,9 @@ function patchMutationApplyToLocalView(
571571
return null;
572572
}
573573

574-
let mergedMaskSet = new SortedSet<FieldPath>(FieldPath.comparator);
575-
for (const fieldPath of previousMask.fields) {
576-
mergedMaskSet = mergedMaskSet.add(fieldPath);
577-
}
578-
for (const fieldPath of mutation.fieldMask.fields) {
579-
mergedMaskSet = mergedMaskSet.add(fieldPath);
580-
}
581-
mutation.fieldTransforms
582-
.map(transform => transform.field)
583-
.forEach(fieldPath => (mergedMaskSet = mergedMaskSet.add(fieldPath)));
584-
return new FieldMask(mergedMaskSet.toArray());
574+
return previousMask
575+
.unionWith(mutation.fieldMask.fields)
576+
.unionWith(mutation.fieldTransforms.map(transform => transform.field));
585577
}
586578

587579
/**

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

Lines changed: 1 addition & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -454,28 +454,8 @@ describe('QueryEngine', () => {
454454
// Add an unacknowledged mutation
455455
await addMutation(deleteMutation('coll/b'));
456456
const docs = await expectFullCollectionQuery(() =>
457-
persistence.runTransaction('runQuery', 'readonly', txn => {
458-
return targetCache
459-
.getMatchingKeysForTargetId(txn, TEST_TARGET_ID)
460-
.next(remoteKeys => {
461-
return queryEngine
462-
.getDocumentsMatchingQuery(
463-
txn,
464-
query1,
465-
LAST_LIMBO_FREE_SNAPSHOT,
466-
remoteKeys
467-
)
468-
.next(documentMap => {
469-
let result = new DocumentSet();
470-
documentMap.forEach((_, v) => {
471-
result = result.add(v);
472-
});
473-
return result;
474-
});
475-
});
476-
})
457+
runQuery(query1, LAST_LIMBO_FREE_SNAPSHOT)
477458
);
478-
479459
verifyResult(docs, [MATCHING_DOC_A]);
480460
});
481461
});

‎packages/firestore/test/unit/model/mutation.test.ts

Lines changed: 18 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,7 @@ describe('Mutation', () => {
132132
mutationApplyToLocalView(
133133
overlay,
134134
docForOverlay,
135-
/* previousMask */ null,
135+
/* previousMask= */ null,
136136
timestamp
137137
);
138138
}
@@ -148,7 +148,12 @@ describe('Mutation', () => {
148148
const document = doc('collection/key', 0, docData);
149149

150150
const set = setMutation('collection/key', { bar: 'bar-value' });
151-
mutationApplyToLocalView(set, document, /* previousMask */ null, timestamp);
151+
mutationApplyToLocalView(
152+
set,
153+
document,
154+
/* previousMask= */ null,
155+
timestamp
156+
);
152157
expect(document).to.deep.equal(
153158
doc('collection/key', 0, { bar: 'bar-value' }).setHasLocalMutations()
154159
);
@@ -165,7 +170,7 @@ describe('Mutation', () => {
165170
mutationApplyToLocalView(
166171
patch,
167172
document,
168-
/* previousMask */ null,
173+
/* previousMask= */ null,
169174
timestamp
170175
);
171176
expect(document).to.deep.equal(
@@ -189,7 +194,7 @@ describe('Mutation', () => {
189194
mutationApplyToLocalView(
190195
patch,
191196
document,
192-
/* previousMask */ null,
197+
/* previousMask= */ null,
193198
timestamp
194199
);
195200
expect(document).to.deep.equal(
@@ -212,7 +217,7 @@ describe('Mutation', () => {
212217
mutationApplyToLocalView(
213218
patch,
214219
document,
215-
/* previousMask */ null,
220+
/* previousMask= */ null,
216221
timestamp
217222
);
218223
expect(document).to.deep.equal(
@@ -233,7 +238,7 @@ describe('Mutation', () => {
233238
mutationApplyToLocalView(
234239
patch,
235240
document,
236-
/* previousMask */ null,
241+
/* previousMask= */ null,
237242
timestamp
238243
);
239244
expect(document).to.deep.equal(
@@ -255,7 +260,7 @@ describe('Mutation', () => {
255260
mutationApplyToLocalView(
256261
patch,
257262
document,
258-
/* previousMask */ null,
263+
/* previousMask= */ null,
259264
timestamp
260265
);
261266
expect(document).to.deep.equal(
@@ -272,7 +277,7 @@ describe('Mutation', () => {
272277
mutationApplyToLocalView(
273278
patch,
274279
document,
275-
/* previousMask */ null,
280+
/* previousMask= */ null,
276281
timestamp
277282
);
278283
expect(document).to.deep.equal(deletedDoc('collection/key', 0));
@@ -289,7 +294,7 @@ describe('Mutation', () => {
289294
mutationApplyToLocalView(
290295
transform,
291296
document,
292-
/* previousMask */ null,
297+
/* previousMask= */ null,
293298
timestamp
294299
);
295300

@@ -459,7 +464,7 @@ describe('Mutation', () => {
459464
mutationApplyToLocalView(
460465
transform,
461466
document,
462-
/* previousMask */ null,
467+
/* previousMask= */ null,
463468
timestamp
464469
);
465470
}
@@ -603,7 +608,7 @@ describe('Mutation', () => {
603608
mutationApplyToLocalView(
604609
mutation,
605610
document,
606-
/* previousMask */ null,
611+
/* previousMask= */ null,
607612
Timestamp.now()
608613
);
609614
expect(document).to.deep.equal(
@@ -758,13 +763,13 @@ describe('Mutation', () => {
758763
mutationApplyToLocalView(
759764
transform,
760765
document,
761-
/* previousMask */ null,
766+
/* previousMask= */ null,
762767
Timestamp.now()
763768
);
764769
mutationApplyToLocalView(
765770
transform,
766771
document,
767-
/* previousMask */ null,
772+
/* previousMask= */ null,
768773
Timestamp.now()
769774
);
770775

0 commit comments

Comments
 (0)