resolve comments

Author: milaGGL

packages/firestore/src/remote/bloom_filter.ts

@@ -42,11 +42,11 @@ function get64BitUints(Bytes: Uint8Array): [Integer, Integer] {
 }
 
 export class BloomFilter {
-  readonly size: number;
-  private readonly sizeInInteger: Integer;
+  readonly bitCount: number;
+  private readonly bitCountInInteger: Integer;
 
   constructor(
-    private readonly _bitmap: Uint8Array,
+    readonly bitmap: Uint8Array,
     readonly padding: number,
     readonly hashCount: number
   ) {
@@ -58,12 +58,12 @@ export class BloomFilter {
       throw new BloomFilterError(`Invalid hash count: ${hashCount}`);
     }
 
-    if (_bitmap.length > 0 && this.hashCount === 0) {
+    if (bitmap.length > 0 && this.hashCount === 0) {
       // Only empty bloom filter can have 0 hash count.
       throw new BloomFilterError(`Invalid hash count: ${hashCount}`);
     }
 
-    if (_bitmap.length === 0) {
+    if (bitmap.length === 0) {
       // Empty bloom filter should have 0 padding.
       if (padding !== 0) {
         throw new BloomFilterError(
@@ -72,13 +72,9 @@ export class BloomFilter {
       }
     }
 
-    this.size = _bitmap.length * 8 - padding;
-    // Set the size in Integer to avoid repeated calculation in mightContain().
-    this.sizeInInteger = Integer.fromNumber(this.size);
-  }
-
-  getBitMap(): Uint8Array {
-    return new Uint8Array(this._bitmap);
+    this.bitCount = bitmap.length * 8 - padding;
+    // Set the bit count in Integer to avoid repetition in mightContain().
+    this.bitCountInInteger = Integer.fromNumber(this.bitCount);
   }
 
   // Calculate the ith hash value based on the hashed 64bit integers,
@@ -90,25 +86,25 @@ export class BloomFilter {
     if (hashValue.compare(MAX_64_BIT_UNSIGNED_INTEGER) === 1) {
       hashValue = new Integer([hashValue.getBits(0), hashValue.getBits(1)], 0);
     }
-    return hashValue.modulo(this.sizeInInteger).toNumber();
+    return hashValue.modulo(this.bitCountInInteger).toNumber();
   }
 
   // Return whether the bit on the given index in the bitmap is set to 1.
   private isBitSet(index: number): boolean {
     // To retrieve bit n, calculate: (bitmap[n / 8] & (0x01 << (n % 8))).
-    const byte = this._bitmap[Math.floor(index / 8)];
+    const byte = this.bitmap[Math.floor(index / 8)];
     const offset = index % 8;
     return (byte & (0x01 << offset)) !== 0;
   }
 
   mightContain(value: string): boolean {
-    // Empty bitmap and empty value should always return false on membership
-    // check.
-    if (this.size === 0 || value === '') {
+    // Empty bitmap should always return false on membership check.
+    if (this.bitCount === 0) {
       return false;
     }
-
+    console.log("value to Hash:",value)
     const md5HashedValue = getMd5HashValue(value);
+    console.log("Hashed value: ", md5HashedValue)
     const [hash1, hash2] = get64BitUints(md5HashedValue);
     for (let i = 0; i < this.hashCount; i++) {
       const index = this.getBitIndex(hash1, hash2, i);
@@ -133,7 +129,11 @@ export class BloomFilter {
   }
 
   private insert(value: string): void {
-    if (this.size === 0 || value === '') {
+    if (value === '') {
+      throw new Error('Cannot insert empty string to a bloom filter.');
+    }
+
+    if (this.bitCount === 0) {
       return;
     }
 
@@ -148,7 +148,7 @@ export class BloomFilter {
   private setBit(index: number): void {
     const indexOfByte = Math.floor(index / 8);
     const offset = index % 8;
-    this._bitmap[indexOfByte] |= 0x01 << offset;
+    this.bitmap[indexOfByte] |= 0x01 << offset;
   }
 }
 

packages/firestore/src/remote/watch_change.ts

@@ -411,8 +411,8 @@ export class WatchChangeAggregator {
         }
       } else {
         const currentSize = this.getCurrentDocumentCountForTarget(targetId);
-        // Existence filter mismatch. Mark the documents as being in limbo, and raise a
-        // snapshot with `isFromCache:true`.
+        // Existence filter mismatch. Mark the documents as being in limbo, and
+        // raise a snapshot with `isFromCache:true`.
         if (currentSize !== expectedCount) {
           // Apply bloom filter to identify and mark removed documents.
           const bloomFilterApplied = this.applyBloomFilter(
@@ -421,8 +421,8 @@ export class WatchChangeAggregator {
             currentSize
           );
           if (!bloomFilterApplied) {
-            // If bloom filter application fails, we reset the mapping and trigger
-            // re-run of the query.
+            // If bloom filter application fails, we reset the mapping and
+            // trigger re-run of the query.
             this.resetTarget(targetId);
             this.pendingTargetResets = this.pendingTargetResets.add(targetId);
           }
@@ -449,22 +449,30 @@ export class WatchChangeAggregator {
       hashCount = 0
     } = unchangedNames;
 
+    if (typeof bitmap === 'string') {
+      const isValidBitmap = this.isValidBase64String(bitmap);
+      if (!isValidBitmap) {
+        logWarn('Invalid base64 string. Applying bloom filter failed.');
+        return false;
+      }
+    }
+
+    const normalizedBitmap = normalizeByteString(bitmap).toUint8Array();
+
     let bloomFilter;
     try {
-      // normalizeByteString throws error if the bitmap includes invalid 64base characters
-      const normalizedBitmap = normalizeByteString(bitmap).toUint8Array();
       // BloomFilter throws error if the inputs are invalid
       bloomFilter = new BloomFilter(normalizedBitmap, padding, hashCount);
     } catch (err) {
       if (err instanceof BloomFilterError) {
-        logWarn('BloomFilter', err);
+        logWarn('BloomFilter error: ', err);
       } else {
         logWarn('Applying bloom filter failed: ', err);
       }
       return false;
     }
 
-    if (bloomFilter.size === 0) {
+    if (bloomFilter.bitCount === 0) {
       return false;
     }
 
@@ -476,9 +484,16 @@ export class WatchChangeAggregator {
     return currentCount - removedDocumentCount === expectedCount;
   }
 
+  private isValidBase64String(value: string): boolean {
+    const regExp = new RegExp(
+      '^(?:[A-Za-z0-9+/]{4})*(?:[A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?$'
+    );
+    return regExp.test(value);
+  }
+
   /**
-   * Filter out removed documents based on bloom filter membership result and return number
-   * of documents removed.
+   * Filter out removed documents based on bloom filter membership result and
+   * return number of documents removed.
    */
   private filterRemovedDocuments(
     bloomFilter: BloomFilter,

packages/firestore/test/unit/remote/bloom_filter.test.ts

@@ -24,7 +24,7 @@ import * as TEST_DATA from './bloom_filter_golden_test_data';
 describe('BloomFilter', () => {
   it('can instantiate an empty bloom filter', () => {
     const bloomFilter = new BloomFilter(new Uint8Array(0), 0, 0);
-    expect(bloomFilter.size).to.equal(0);
+    expect(bloomFilter.bitCount).to.equal(0);
   });
 
   it('should throw error if empty bloom filter inputs are invalid', () => {
@@ -46,14 +46,14 @@ describe('BloomFilter', () => {
     const bloomFilter6 = new BloomFilter(new Uint8Array(1), 6, 1);
     const bloomFilter7 = new BloomFilter(new Uint8Array(1), 7, 1);
 
-    expect(bloomFilter0.size).to.equal(8);
-    expect(bloomFilter1.size).to.equal(7);
-    expect(bloomFilter2.size).to.equal(6);
-    expect(bloomFilter3.size).to.equal(5);
-    expect(bloomFilter4.size).to.equal(4);
-    expect(bloomFilter5.size).to.equal(3);
-    expect(bloomFilter6.size).to.equal(2);
-    expect(bloomFilter7.size).to.equal(1);
+    expect(bloomFilter0.bitCount).to.equal(8);
+    expect(bloomFilter1.bitCount).to.equal(7);
+    expect(bloomFilter2.bitCount).to.equal(6);
+    expect(bloomFilter3.bitCount).to.equal(5);
+    expect(bloomFilter4.bitCount).to.equal(4);
+    expect(bloomFilter5.bitCount).to.equal(3);
+    expect(bloomFilter6.bitCount).to.equal(2);
+    expect(bloomFilter7.bitCount).to.equal(1);
   });
 
   it('should throw error if padding is invalid', () => {
@@ -90,7 +90,7 @@ describe('BloomFilter', () => {
     expect(bloomFilter.mightContain('def')).to.be.false;
   });
 
-  it('mightContain should always return false for empty string', () => {
+  it.only('mightContain should always return false for empty string', () => {
     const emptyBloomFilter = new BloomFilter(new Uint8Array(0), 0, 0);
     const nonEmptyBloomFilter = new BloomFilter(
       new Uint8Array([255, 255, 255]),