Skip to content

Commit e2f56be

Browse files
milaGGLmilaGGL
committed
sort docChanges
1 parent e5e4f92 commit e2f56be

File tree

4 files changed

+34
-27
lines changed

4 files changed

+34
-27
lines changed

‎packages/firestore/src/remote/watch_change.ts

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -448,18 +448,20 @@ export class WatchChangeAggregator {
448448
bits: { bitmap = '', padding = 0 },
449449
hashCount = 0
450450
} = unchangedNames;
451-
const normalizedBitmap = normalizeByteString(bitmap).toUint8Array();
452451

453452
let bloomFilter;
454453
try {
454+
// normalizeByteString throws error if the bitmap includes invalid 64base characters
455+
const normalizedBitmap = normalizeByteString(bitmap).toUint8Array();
456+
// BloomFilter throws error if the inputs are invalid
455457
bloomFilter = new BloomFilter(normalizedBitmap, padding, hashCount);
456458
} catch (err) {
457459
if (err instanceof BloomFilterError) {
458460
logWarn('BloomFilter', err);
459-
return false;
460461
} else {
461-
throw err;
462+
logWarn('Applying bloom filter failed: ', err);
462463
}
464+
return false;
463465
}
464466

465467
if (bloomFilter.size === 0) {

‎packages/firestore/test/unit/specs/existence_filter_spec.test.ts

Lines changed: 20 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -274,7 +274,7 @@ describeSpec('Existence Filters:', [], () => {
274274
// BloomFilter identify docB is deleted, skip full query and put docB
275275
// into limbo directly.
276276
.expectEvents(query1, { fromCache: true })
277-
.expectLimboDocs(docB.key) // docB is now in limbo
277+
.expectLimboDocs(docB.key) // docB is now in limbo.
278278
.ackLimbo(2000, deletedDoc('collection/b', 2000))
279279
.expectLimboDocs() // docB is no longer in limbo
280280
.expectEvents(query1, {
@@ -310,9 +310,9 @@ describeSpec('Existence Filters:', [], () => {
310310
// positive results for docB. Re-run query is triggered.
311311
.expectEvents(query1, { fromCache: true })
312312
.expectActiveTargets({ query: query1, resumeToken: '' })
313-
.watchRemoves(query1) // Acks removal of query
313+
.watchRemoves(query1) // Acks removal of query.
314314
.watchAcksFull(query1, 2000, docA)
315-
.expectLimboDocs(docB.key, docC.key) // docB and docC are now in limbo
315+
.expectLimboDocs(docB.key, docC.key) // docB and docC are now in limbo.
316316
.ackLimbo(2001, deletedDoc('collection/b', 2001))
317317
.expectEvents(query1, {
318318
removed: [docB],
@@ -352,7 +352,7 @@ describeSpec('Existence Filters:', [], () => {
352352
// BloomFilter identify docB is deleted, skip full query and put docB
353353
// into limbo directly.
354354
.expectEvents(query1, { fromCache: true })
355-
.expectLimboDocs(docB.key) // docB is now in limbo
355+
.expectLimboDocs(docB.key) // docB is now in limbo.
356356
);
357357
}
358358
);
@@ -372,7 +372,7 @@ describeSpec('Existence Filters:', [], () => {
372372
.expectEvents(query1, { added: [docA, docB] })
373373
// DocB is deleted in the next sync.
374374
.watchFilters([query1], [docA.key], {
375-
// Invalid 64base string in bitmap
375+
// Invalid 64base string in bitmap.
376376
bits: { bitmap: '*#!', padding: 1 },
377377
hashCount: 1
378378
})
@@ -398,7 +398,7 @@ describeSpec('Existence Filters:', [], () => {
398398
.expectEvents(query1, { added: [docA, docB] })
399399
// DocB is deleted in the next sync.
400400
.watchFilters([query1], [docA.key], {
401-
// Invalid hashCount in bloom filter
401+
// Invalid hashCount in bloom filter.
402402
bits: { bitmap: 'AQ==', padding: 1 },
403403
hashCount: -1
404404
})
@@ -431,7 +431,7 @@ describeSpec('Existence Filters:', [], () => {
431431
);
432432
});
433433

434-
specTest('Same docs can have different bloom filter', [], () => {
434+
specTest('Same docs can have different bloom filters', [], () => {
435435
const query1 = query('collection', filter('v', '<=', 2));
436436
const query2 = query('collection', filter('v', '>=', 2));
437437

@@ -461,18 +461,19 @@ describeSpec('Existence Filters:', [], () => {
461461
.watchAcksFull(query2, 1001, docB, docC)
462462
.expectEvents(query2, { added: [docC] })
463463

464-
// DocA is deleted in the next sync.
464+
// DocA is deleted in the next sync for query1.
465465
.watchFilters([query1], [docB.key], bloomFilterProto1)
466466
.watchSnapshots(2000)
467467
// BloomFilter identify docA is deleted, skip full query.
468468
.expectEvents(query1, { fromCache: true })
469-
.expectLimboDocs(docA.key) // docA is now in limbo
469+
.expectLimboDocs(docA.key) // docA is now in limbo.
470470

471+
// DocC is deleted in the next sync for query2.
471472
.watchFilters([query2], [docB.key], bloomFilterProto2)
472473
.watchSnapshots(3000)
473-
// BloomFilter identify docA is deleted, skip full query.
474+
// BloomFilter identify docC is deleted, skip full query.
474475
.expectEvents(query2, { fromCache: true })
475-
.expectLimboDocs(docA.key, docC.key) // docA is now in limbo
476+
.expectLimboDocs(docA.key, docC.key) // docC is now in limbo.
476477
);
477478
});
478479

@@ -498,7 +499,6 @@ describeSpec('Existence Filters:', [], () => {
498499
// send a new global snapshot. We should not see an event until we
499500
// receive the snapshot.
500501
.watchFilters([query1], [docA.key], bloomFilterProto)
501-
// BloomFilter identifies docB is removed, moves it to limbo.
502502
.watchSends({ affects: [query1] }, docC)
503503
.watchSnapshots(2000)
504504
.expectEvents(query1, { added: [docC], fromCache: true })
@@ -524,12 +524,12 @@ describeSpec('Existence Filters:', [], () => {
524524
.watchFilters([query1], [docA.key], bloomFilterProto)
525525
.watchSnapshots(2000)
526526
.expectEvents(query1, { fromCache: true })
527-
.expectLimboDocs(docB.key) // docB is now in limbo
527+
.expectLimboDocs(docB.key) // docB is now in limbo.
528528
.watchRemoves(
529529
newQueryForPath(docB.key.path),
530530
new RpcError(Code.PERMISSION_DENIED, 'no')
531531
)
532-
.expectLimboDocs() // docB is no longer in limbo
532+
.expectLimboDocs() // docB is no longer in limbo.
533533
.expectEvents(query1, {
534534
removed: [docB]
535535
});
@@ -538,14 +538,14 @@ describeSpec('Existence Filters:', [], () => {
538538
specTest('Bloom filter with large size works as expected', [], () => {
539539
const query1 = query('collection');
540540
const docs = [];
541-
for (let i = 0; i < 10; i++) {
541+
for (let i = 0; i < 100; i++) {
542542
docs.push(doc(`collection/doc${i}`, 1000, { v: 1 }));
543543
}
544544
const docKeys = docs.map(item => item.key);
545545

546546
const bloomFilterProto = generateBloomFilterProto({
547-
contains: docKeys.slice(0, 5).map(item => item.toString()),
548-
notContains: docKeys.slice(5).map(item => item.toString()),
547+
contains: docKeys.slice(0, 50).map(item => item.toString()),
548+
notContains: docKeys.slice(50).map(item => item.toString()),
549549
numOfBits: 1000,
550550
hashCount: 16
551551
});
@@ -554,12 +554,12 @@ describeSpec('Existence Filters:', [], () => {
554554
.userListens(query1)
555555
.watchAcksFull(query1, 1000, ...docs)
556556
.expectEvents(query1, { added: docs })
557-
// Doc0 to doc5 are deleted in the next sync.
558-
.watchFilters([query1], docKeys.slice(0, 5), bloomFilterProto)
557+
// Doc0 to doc50 are deleted in the next sync.
558+
.watchFilters([query1], docKeys.slice(0, 50), bloomFilterProto)
559559
.watchSnapshots(2000)
560560
// BloomFilter correctly identifies docs that deleted, skip full query.
561561
.expectEvents(query1, { fromCache: true })
562-
.expectLimboDocs(...docKeys.slice(5))
562+
.expectLimboDocs(...docKeys.slice(50))
563563
);
564564
});
565565
});

‎packages/firestore/test/unit/specs/limbo_spec.test.ts

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -938,8 +938,7 @@ describeSpec('Limbo Documents:', [], () => {
938938
numOfBits: 3,
939939
hashCount: 1
940940
});
941-
// Verify that limbo resolution throttling works correctly with bloom filter
942-
// application on existence filter mismatches.
941+
// Verify that limbo resolution throttling works as expected with bloom filter.
943942
return (
944943
spec()
945944
.withMaxConcurrentLimboResolutions(2)

‎packages/firestore/test/unit/specs/spec_test_runner.ts

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1148,7 +1148,7 @@ abstract class TestRunner {
11481148
actual.error!
11491149
);
11501150
} else {
1151-
const expectedChanges: DocumentViewChange[] = [];
1151+
let expectedChanges: DocumentViewChange[] = [];
11521152
if (expected.removed) {
11531153
expected.removed.forEach(change => {
11541154
expectedChanges.push(this.parseChange(ChangeType.Removed, change));
@@ -1171,7 +1171,13 @@ abstract class TestRunner {
11711171
});
11721172
}
11731173

1174-
expect(actual.view!.docChanges).to.deep.equal(expectedChanges);
1174+
expectedChanges = expectedChanges.sort((a, b) =>
1175+
primitiveComparator(a.doc, b.doc)
1176+
);
1177+
const actualChanges = actual.view!.docChanges.sort((a, b) =>
1178+
primitiveComparator(a.doc, b.doc)
1179+
);
1180+
expect(actualChanges).to.deep.equal(expectedChanges);
11751181

11761182
expect(actual.view!.hasPendingWrites).to.equal(
11771183
expected.hasPendingWrites,

0 commit comments

Comments
 (0)