Skip to content

Commit e45c046

Browse files
milaGGLmilaGGL
committed
resolve comments
1 parent 370a24f commit e45c046

File tree

11 files changed

+102
-112
lines changed

11 files changed

+102
-112
lines changed

‎packages/firestore/src/platform/browser/text_serializer.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
/**
22
* @license
3-
* Copyright 2022 Google LLC
3+
* Copyright 2023 Google LLC
44
*
55
* Licensed under the Apache License, Version 2.0 (the "License");
66
* you may not use this file except in compliance with the License.

‎packages/firestore/src/platform/browser_lite/text_serializer.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
/**
22
* @license
3-
* Copyright 2022 Google LLC
3+
* Copyright 2023 Google LLC
44
*
55
* Licensed under the Apache License, Version 2.0 (the "License");
66
* you may not use this file except in compliance with the License.

‎packages/firestore/src/platform/node/text_serializer.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
/**
22
* @license
3-
* Copyright 2022 Google LLC
3+
* Copyright 2023 Google LLC
44
*
55
* Licensed under the Apache License, Version 2.0 (the "License");
66
* you may not use this file except in compliance with the License.

‎packages/firestore/src/platform/node_lite/text_serializer.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
/**
22
* @license
3-
* Copyright 2022 Google LLC
3+
* Copyright 2023 Google LLC
44
*
55
* Licensed under the Apache License, Version 2.0 (the "License");
66
* you may not use this file except in compliance with the License.

‎packages/firestore/src/platform/rn/text_serializer.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
/**
22
* @license
3-
* Copyright 2022 Google LLC
3+
* Copyright 2023 Google LLC
44
*
55
* Licensed under the Apache License, Version 2.0 (the "License");
66
* you may not use this file except in compliance with the License.

‎packages/firestore/src/platform/rn_lite/text_serializer.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
/**
22
* @license
3-
* Copyright 2022 Google LLC
3+
* Copyright 2023 Google LLC
44
*
55
* Licensed under the Apache License, Version 2.0 (the "License");
66
* you may not use this file except in compliance with the License.

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

Lines changed: 9 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -63,13 +63,11 @@ export class BloomFilter {
6363
throw new BloomFilterError(`Invalid hash count: ${hashCount}`);
6464
}
6565

66-
if (bitmap.length === 0) {
66+
if (bitmap.length === 0 && padding !== 0) {
6767
// Empty bloom filter should have 0 padding.
68-
if (padding !== 0) {
69-
throw new BloomFilterError(
70-
`Invalid padding when bitmap length is 0: ${padding}`
71-
);
72-
}
68+
throw new BloomFilterError(
69+
`Invalid padding when bitmap length is 0: ${padding}`
70+
);
7371
}
7472

7573
this.bitCount = bitmap.length * 8 - padding;
@@ -113,14 +111,14 @@ export class BloomFilter {
113111
return true;
114112
}
115113

116-
/** Create bloom filter input for testing purposes only. */
114+
/** Create bloom filter for testing purposes only. */
117115
static create(
118-
numOfBits: number,
116+
bitCount: number,
119117
hashCount: number,
120118
contains: string[]
121119
): BloomFilter {
122-
const padding = numOfBits % 8 === 0 ? 0 : 8 - (numOfBits % 8);
123-
const bitmap = new Uint8Array(Math.ceil(numOfBits / 8));
120+
const padding = bitCount % 8 === 0 ? 0 : 8 - (bitCount % 8);
121+
const bitmap = new Uint8Array(Math.ceil(bitCount / 8));
124122
const bloomFilter = new BloomFilter(bitmap, padding, hashCount);
125123
contains.forEach(item => bloomFilter.insert(item));
126124
return bloomFilter;
@@ -147,5 +145,5 @@ export class BloomFilter {
147145
}
148146

149147
export class BloomFilterError extends Error {
150-
readonly name: string = 'BloomFilterError';
148+
readonly name = 'BloomFilterError';
151149
}

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

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -449,6 +449,7 @@ export class WatchChangeAggregator {
449449
hashCount = 0
450450
} = unchangedNames;
451451

452+
// TODO(Mila): Remove this validation, add try catch to normalizeByteString.
452453
if (typeof bitmap === 'string') {
453454
const isValidBitmap = this.isValidBase64String(bitmap);
454455
if (!isValidBitmap) {
@@ -459,7 +460,7 @@ export class WatchChangeAggregator {
459460

460461
const normalizedBitmap = normalizeByteString(bitmap).toUint8Array();
461462

462-
let bloomFilter;
463+
let bloomFilter: BloomFilter;
463464
try {
464465
// BloomFilter throws error if the inputs are invalid
465466
bloomFilter = new BloomFilter(normalizedBitmap, padding, hashCount);
@@ -472,10 +473,6 @@ export class WatchChangeAggregator {
472473
return false;
473474
}
474475

475-
if (bloomFilter.bitCount === 0) {
476-
return false;
477-
}
478-
479476
const removedDocumentCount = this.filterRemovedDocuments(
480477
bloomFilter,
481478
targetId

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

Lines changed: 48 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -259,9 +259,7 @@ describeSpec('Existence Filters:', [], () => {
259259
const docB = doc('collection/b', 1000, { v: 2 });
260260
const bloomFilterProto = generateBloomFilterProto({
261261
contains: [docA],
262-
notContains: [docB],
263-
numOfBits: 2,
264-
hashCount: 1
262+
notContains: [docB]
265263
});
266264
return (
267265
spec()
@@ -294,9 +292,7 @@ describeSpec('Existence Filters:', [], () => {
294292
const docC = doc('collection/c', 1000, { v: 2 });
295293
const bloomFilterProto = generateBloomFilterProto({
296294
contains: [docA, docB],
297-
notContains: [docC],
298-
numOfBits: 3,
299-
hashCount: 1
295+
notContains: [docC]
300296
});
301297
return (
302298
spec()
@@ -309,21 +305,8 @@ describeSpec('Existence Filters:', [], () => {
309305
// BloomFilter correctly identifies docC is deleted, but yields false
310306
// positive results for docB. Re-run query is triggered.
311307
.expectEvents(query1, { fromCache: true })
312-
.expectActiveTargets({ query: query1, resumeToken: '' })
313308
.watchRemoves(query1) // Acks removal of query.
314-
.watchAcksFull(query1, 2000, docA)
315-
.expectLimboDocs(docB.key, docC.key) // DocB, docC are now in limbo.
316-
.ackLimbo(2001, deletedDoc('collection/b', 2001))
317-
.expectEvents(query1, {
318-
removed: [docB],
319-
fromCache: true
320-
})
321-
.expectLimboDocs(docC.key)
322-
.ackLimbo(2002, deletedDoc('collection/c', 2002))
323-
.expectEvents(query1, {
324-
removed: [docC]
325-
})
326-
.expectLimboDocs()
309+
.expectActiveTargets({ query: query1, resumeToken: '' })
327310
);
328311
}
329312
);
@@ -337,10 +320,9 @@ describeSpec('Existence Filters:', [], () => {
337320
const docB = doc('collection/À∑Ò', 1000, { v: 1 });
338321
const bloomFilterProto = generateBloomFilterProto({
339322
contains: [docA],
340-
notContains: [docB],
341-
numOfBits: 2,
342-
hashCount: 1
323+
notContains: [docB]
343324
});
325+
344326
return (
345327
spec()
346328
.userListens(query1)
@@ -365,17 +347,21 @@ describeSpec('Existence Filters:', [], () => {
365347
const docA = doc('collection/a', 1000, { v: 1 });
366348
const docB = doc('collection/b', 1000, { v: 2 });
367349

350+
const bloomFilterProto = generateBloomFilterProto({
351+
contains: [docA],
352+
notContains: [docB]
353+
});
354+
// Omit padding and hashCount. Default value 0 should be used.
355+
delete bloomFilterProto.hashCount;
356+
delete bloomFilterProto.bits!.padding;
357+
368358
return (
369359
spec()
370360
.userListens(query1)
371361
.watchAcksFull(query1, 1000, docA, docB)
372362
.expectEvents(query1, { added: [docA, docB] })
373363
// DocB is deleted in the next sync.
374-
.watchFilters([query1], [docA.key], {
375-
// Bloom filter omitted padding and hashCount. Default value 0 will
376-
// be used.
377-
bits: { bitmap: 'AQ==' }
378-
})
364+
.watchFilters([query1], [docA.key], bloomFilterProto)
379365
.watchSnapshots(2000)
380366
// Re-run query is triggered.
381367
.expectEvents(query1, { fromCache: true })
@@ -392,17 +378,20 @@ describeSpec('Existence Filters:', [], () => {
392378
const docA = doc('collection/a', 1000, { v: 1 });
393379
const docB = doc('collection/b', 1000, { v: 1 });
394380

381+
const bloomFilterProto = generateBloomFilterProto({
382+
contains: [docA],
383+
notContains: [docB]
384+
});
385+
// Set bitmap to invalid base64 string.
386+
bloomFilterProto.bits!.bitmap = 'INVALID_BASE_64';
387+
395388
return (
396389
spec()
397390
.userListens(query1)
398391
.watchAcksFull(query1, 1000, docA, docB)
399392
.expectEvents(query1, { added: [docA, docB] })
400393
// DocB is deleted in the next sync.
401-
.watchFilters([query1], [docA.key], {
402-
// Invalid base64 string in bitmap.
403-
bits: { bitmap: 'INVALID_BASE_64', padding: 1 },
404-
hashCount: 1
405-
})
394+
.watchFilters([query1], [docA.key], bloomFilterProto)
406395
.watchSnapshots(2000)
407396
// Re-run query is triggered.
408397
.expectEvents(query1, { fromCache: true })
@@ -412,23 +401,27 @@ describeSpec('Existence Filters:', [], () => {
412401
);
413402

414403
specTest(
415-
'Full re-query is triggered when bloom filter input is invalid',
404+
'Full re-query is triggered when bloom filter hashCount is invalid',
416405
[],
417406
() => {
418407
const query1 = query('collection');
419408
const docA = doc('collection/a', 1000, { v: 1 });
420409
const docB = doc('collection/b', 1000, { v: 1 });
410+
411+
const bloomFilterProto = generateBloomFilterProto({
412+
contains: [docA],
413+
notContains: [docB]
414+
});
415+
// Set hashCount to negative number.
416+
bloomFilterProto.hashCount = -1;
417+
421418
return (
422419
spec()
423420
.userListens(query1)
424421
.watchAcksFull(query1, 1000, docA, docB)
425422
.expectEvents(query1, { added: [docA, docB] })
426423
// DocB is deleted in the next sync.
427-
.watchFilters([query1], [docA.key], {
428-
// Invalid hashCount in bloom filter.
429-
bits: { bitmap: 'AQ==', padding: 1 },
430-
hashCount: -1
431-
})
424+
.watchFilters([query1], [docA.key], bloomFilterProto)
432425
.watchSnapshots(2000)
433426
// Re-run query is triggered.
434427
.expectEvents(query1, { fromCache: true })
@@ -441,16 +434,22 @@ describeSpec('Existence Filters:', [], () => {
441434
const query1 = query('collection');
442435
const docA = doc('collection/a', 1000, { v: 1 });
443436
const docB = doc('collection/b', 1000, { v: 1 });
437+
438+
//Generate an empty bloom filter.
439+
const bloomFilterProto = generateBloomFilterProto({
440+
contains: [],
441+
notContains: [],
442+
bitCount: 0,
443+
hashCount: 0
444+
});
445+
444446
return (
445447
spec()
446448
.userListens(query1)
447449
.watchAcksFull(query1, 1000, docA, docB)
448450
.expectEvents(query1, { added: [docA, docB] })
449451
// DocB is deleted in the next sync.
450-
.watchFilters([query1], [docA.key], {
451-
bits: { bitmap: '', padding: 0 },
452-
hashCount: 0
453-
})
452+
.watchFilters([query1], [docA.key], bloomFilterProto)
454453
.watchSnapshots(2000)
455454
// Re-run query is triggered.
456455
.expectEvents(query1, { fromCache: true })
@@ -469,13 +468,13 @@ describeSpec('Existence Filters:', [], () => {
469468
const bloomFilterProto1 = generateBloomFilterProto({
470469
contains: [docB],
471470
notContains: [docA, docC],
472-
numOfBits: 5,
473-
hashCount: 1
471+
bitCount: 5,
472+
hashCount: 2
474473
});
475474
const bloomFilterProto2 = generateBloomFilterProto({
476475
contains: [docB],
477476
notContains: [docA, docC],
478-
numOfBits: 4,
477+
bitCount: 4,
479478
hashCount: 1
480479
});
481480
return (
@@ -512,9 +511,7 @@ describeSpec('Existence Filters:', [], () => {
512511

513512
const bloomFilterProto = generateBloomFilterProto({
514513
contains: [docA],
515-
notContains: [docB],
516-
numOfBits: 2,
517-
hashCount: 1
514+
notContains: [docB]
518515
});
519516

520517
return (
@@ -540,9 +537,7 @@ describeSpec('Existence Filters:', [], () => {
540537
const docB = doc('collection/b', 1000, { v: 1 });
541538
const bloomFilterProto = generateBloomFilterProto({
542539
contains: [docA],
543-
notContains: [docB],
544-
numOfBits: 2,
545-
hashCount: 1
540+
notContains: [docB]
546541
});
547542
return spec()
548543
.userListens(query1)
@@ -573,7 +568,7 @@ describeSpec('Existence Filters:', [], () => {
573568
const bloomFilterProto = generateBloomFilterProto({
574569
contains: docs.slice(0, 50),
575570
notContains: docs.slice(50),
576-
numOfBits: 1000,
571+
bitCount: 1000,
577572
hashCount: 16
578573
});
579574
return (

0 commit comments

Comments
 (0)