Merge master into release

Author: google-oss-bot

.github/workflows/canary-deploy.yml

@@ -76,3 +76,15 @@ jobs:
         NPM_TOKEN_APP_CHECK_COMPAT: ${{ secrets.NPM_TOKEN_APP_CHECK_COMPAT }}
         NPM_TOKEN_API_DOCUMENTER: ${{ secrets.NPM_TOKEN_API_DOCUMENTER }}
         CI: true
+    - name: Launch E2E tests workflow
+      # Trigger e2e-test.yml
+      run: |
+        VERSION_SCRIPT="const pkg = require('./packages/firebase/package.json'); console.log(pkg.version);"
+        VERSION_OR_TAG=`node -e "${VERSION_SCRIPT}"`
+        OSS_BOT_GITHUB_TOKEN=${{ secrets.OSS_BOT_GITHUB_TOKEN }}
+        curl -X POST \
+        -H "Content-Type:application/json" \
+        -H "Accept:application/vnd.github.v3+json" \
+        -H "Authorization:Bearer $OSS_BOT_GITHUB_TOKEN" \
+        -d "{\"event_type\":\"canary-tests\", \"client_payload\":{\"versionOrTag\":\"$VERSION_OR_TAG\"}}" \
+        https://api.github.com/repos/firebase/firebase-js-sdk/dispatches

.github/workflows/e2e-test.yml

@@ -3,7 +3,7 @@ name: E2E Smoke Tests
 # Allows REST trigger. Currently triggered by release-cli script during a staging run.
 on:
   repository_dispatch:
-    types: [staging-tests]
+    types: [staging-tests,canary-tests]
 
 jobs:
   test:
@@ -62,6 +62,8 @@ jobs:
       - name: Tests succeeded
         if: success()
         run: node scripts/ci/notify-test-result.js success
+        # we don't want THIS step erroring to trigger the failure notification
+        continue-on-error: true
         env:
           WEBHOOK_URL: ${{ secrets.JSCORE_CHAT_WEBHOOK_URL }}
           RELEASE_TRACKER_URL: ${{ secrets.RELEASE_TRACKER_URL }}

.github/workflows/test-all.yml

@@ -7,7 +7,7 @@ on:
 env:
   # make chromedriver detect installed Chrome version and download the corresponding driver
   DETECT_CHROMEDRIVER_VERSION: true
-  artifactRetentionDays: 2
+  artifactRetentionDays: 14
 
 jobs:
   build:

packages/firestore/src/remote/watch_change.ts

@@ -433,15 +433,18 @@ export class WatchChangeAggregator {
         // raise a snapshot with `isFromCache:true`.
         if (currentSize !== expectedCount) {
           // Apply bloom filter to identify and mark removed documents.
-          const applyResult = this.applyBloomFilter(watchChange, currentSize);
+          const bloomFilter = this.parseBloomFilter(watchChange);
+          const status = bloomFilter
+            ? this.applyBloomFilter(bloomFilter, watchChange, currentSize)
+            : BloomFilterApplicationStatus.Skipped;
 
-          if (applyResult.status !== BloomFilterApplicationStatus.Success) {
+          if (status !== BloomFilterApplicationStatus.Success) {
             // If bloom filter application fails, we reset the mapping and
             // trigger re-run of the query.
             this.resetTarget(targetId);
 
             const purpose: TargetPurpose =
-              applyResult.status === BloomFilterApplicationStatus.FalsePositive
+              status === BloomFilterApplicationStatus.FalsePositive
                 ? TargetPurpose.ExistenceFilterMismatchBloom
                 : TargetPurpose.ExistenceFilterMismatch;
             this.pendingTargetResets = this.pendingTargetResets.insert(
@@ -451,10 +454,11 @@ export class WatchChangeAggregator {
           }
           TestingHooks.instance?.notifyOnExistenceFilterMismatch(
             createExistenceFilterMismatchInfoForTestingHooks(
-              applyResult.status,
-              applyResult.bloomFilterMightContain ?? null,
               currentSize,
-              watchChange.existenceFilter
+              watchChange.existenceFilter,
+              this.metadataProvider.getDatabaseId(),
+              bloomFilter,
+              status
             )
           );
         }
@@ -463,21 +467,15 @@ export class WatchChangeAggregator {
   }
 
   /**
-   * Apply bloom filter to remove the deleted documents, and return the
-   * application status.
+   * Parse the bloom filter from the "unchanged_names" field of an existence
+   * filter.
    */
-  private applyBloomFilter(
-    watchChange: ExistenceFilterChange,
-    currentCount: number
-  ): {
-    status: BloomFilterApplicationStatus;
-    bloomFilterMightContain?: (documentPath: string) => boolean;
-  } {
-    const { unchangedNames, count: expectedCount } =
-      watchChange.existenceFilter;
-
+  private parseBloomFilter(
+    watchChange: ExistenceFilterChange
+  ): BloomFilter | null {
+    const unchangedNames = watchChange.existenceFilter.unchangedNames;
     if (!unchangedNames || !unchangedNames.bits) {
-      return { status: BloomFilterApplicationStatus.Skipped };
+      return null;
     }
 
     const {
@@ -495,7 +493,7 @@ export class WatchChangeAggregator {
             err.message +
             '); ignoring the bloom filter and falling back to full re-query.'
         );
-        return { status: BloomFilterApplicationStatus.Skipped };
+        return null;
       } else {
         throw err;
       }
@@ -511,46 +509,56 @@ export class WatchChangeAggregator {
       } else {
         logWarn('Applying bloom filter failed: ', err);
       }
-      return { status: BloomFilterApplicationStatus.Skipped };
+      return null;
     }
 
-    const bloomFilterMightContain = (documentPath: string): boolean => {
-      const databaseId = this.metadataProvider.getDatabaseId();
-      return bloomFilter.mightContain(
-        `projects/${databaseId.projectId}/databases/${databaseId.database}` +
-          `/documents/${documentPath}`
-      );
-    };
-
     if (bloomFilter.bitCount === 0) {
-      return { status: BloomFilterApplicationStatus.Skipped };
+      return null;
     }
 
+    return bloomFilter;
+  }
+
+  /**
+   * Apply bloom filter to remove the deleted documents, and return the
+   * application status.
+   */
+  private applyBloomFilter(
+    bloomFilter: BloomFilter,
+    watchChange: ExistenceFilterChange,
+    currentCount: number
+  ): BloomFilterApplicationStatus {
+    const expectedCount = watchChange.existenceFilter.count;
+
     const removedDocumentCount = this.filterRemovedDocuments(
-      watchChange.targetId,
-      bloomFilterMightContain
+      bloomFilter,
+      watchChange.targetId
     );
 
-    const status =
-      expectedCount === currentCount - removedDocumentCount
-        ? BloomFilterApplicationStatus.Success
-        : BloomFilterApplicationStatus.FalsePositive;
-    return { status, bloomFilterMightContain };
+    return expectedCount === currentCount - removedDocumentCount
+      ? BloomFilterApplicationStatus.Success
+      : BloomFilterApplicationStatus.FalsePositive;
   }
 
   /**
    * Filter out removed documents based on bloom filter membership result and
    * return number of documents removed.
    */
   private filterRemovedDocuments(
-    targetId: number,
-    bloomFilterMightContain: (documentPath: string) => boolean
+    bloomFilter: BloomFilter,
+    targetId: number
   ): number {
     const existingKeys = this.metadataProvider.getRemoteKeysForTarget(targetId);
     let removalCount = 0;
 
     existingKeys.forEach(key => {
-      if (!bloomFilterMightContain(key.path.canonicalString())) {
+      const databaseId = this.metadataProvider.getDatabaseId();
+      const documentPath =
+        `projects/${databaseId.projectId}` +
+        `/databases/${databaseId.database}` +
+        `/documents/${key.path.canonicalString()}`;
+
+      if (!bloomFilter.mightContain(documentPath)) {
         this.removeDocumentFromTarget(targetId, key, /*updatedDocument=*/ null);
         removalCount++;
       }
@@ -835,27 +843,29 @@ function snapshotChangesMap(): SortedMap<DocumentKey, ChangeType> {
 }
 
 function createExistenceFilterMismatchInfoForTestingHooks(
-  status: BloomFilterApplicationStatus,
-  bloomFilterMightContain: null | ((documentPath: string) => boolean),
   localCacheCount: number,
-  existenceFilter: ExistenceFilter
+  existenceFilter: ExistenceFilter,
+  databaseId: DatabaseId,
+  bloomFilter: BloomFilter | null,
+  bloomFilterStatus: BloomFilterApplicationStatus
 ): TestingHooksExistenceFilterMismatchInfo {
   const result: TestingHooksExistenceFilterMismatchInfo = {
     localCacheCount,
-    existenceFilterCount: existenceFilter.count
+    existenceFilterCount: existenceFilter.count,
+    databaseId: databaseId.database,
+    projectId: databaseId.projectId
   };
 
   const unchangedNames = existenceFilter.unchangedNames;
   if (unchangedNames) {
     result.bloomFilter = {
-      applied: status === BloomFilterApplicationStatus.Success,
+      applied: bloomFilterStatus === BloomFilterApplicationStatus.Success,
       hashCount: unchangedNames?.hashCount ?? 0,
       bitmapLength: unchangedNames?.bits?.bitmap?.length ?? 0,
-      padding: unchangedNames?.bits?.padding ?? 0
+      padding: unchangedNames?.bits?.padding ?? 0,
+      mightContain: (value: string): boolean =>
+        bloomFilter?.mightContain(value) ?? false
     };
-    if (bloomFilterMightContain) {
-      result.bloomFilter.mightContain = bloomFilterMightContain;
-    }
   }
 
   return result;

packages/firestore/src/util/testing_hooks.ts

@@ -103,6 +103,18 @@ export interface ExistenceFilterMismatchInfo {
    */
   existenceFilterCount: number;
 
+  /**
+   * The projectId used when checking documents for membership in the bloom
+   * filter.
+   */
+  projectId: string;
+
+  /**
+   * The databaseId used when checking documents for membership in the bloom
+   * filter.
+   */
+  databaseId: string;
+
   /**
    * Information about the bloom filter provided by Watch in the ExistenceFilter
    * message's `unchangedNames` field. If this property is omitted or undefined
@@ -126,19 +138,11 @@ export interface ExistenceFilterMismatchInfo {
     padding: number;
 
     /**
-     * Check if the given document path is contained in the bloom filter.
-     *
-     * The "path" of a document can be retrieved from the
-     * `DocumentReference.path` property.
-     *
-     * Note that due to the probabilistic nature of a bloom filter, it is
-     * possible that false positives may occur; that is, this function may
-     * return `true` even though the given string is not in the bloom filter.
-     *
-     * This property is "optional"; if it is undefined then parsing the bloom
-     * filter failed.
+     * Tests the given string for membership in the bloom filter created from
+     * the existence filter; will be undefined if creating the bloom filter
+     * failed.
      */
-    mightContain?(documentPath: string): boolean;
+    mightContain?: (value: string) => boolean;
   };
 }
 

packages/firestore/test/integration/api/query.test.ts

@@ -62,6 +62,7 @@ import {
   toChangesArray,
   toDataArray,
   toIds,
+  PERSISTENCE_MODE_UNSPECIFIED,
   withEmptyTestCollection,
   withRetry,
   withTestCollection,
@@ -2106,16 +2107,18 @@ apiDescribe('Queries', persistence => {
               snapshot => snapshot.ref
             );
 
-            // Delete 50 of the 100 documents. Use a WriteBatch, rather than
-            // deleteDoc(), to avoid affecting the local cache.
+            // Delete 50 of the 100 documents. Use a different Firestore
+            // instance to avoid affecting the local cache.
             const deletedDocumentIds = new Set<string>();
-            const writeBatchForDocumentDeletes = writeBatch(db);
-            for (let i = 0; i < createdDocuments.length; i += 2) {
-              const documentToDelete = createdDocuments[i];
-              writeBatchForDocumentDeletes.delete(documentToDelete);
-              deletedDocumentIds.add(documentToDelete.id);
-            }
-            await writeBatchForDocumentDeletes.commit();
+            await withTestDb(PERSISTENCE_MODE_UNSPECIFIED, async db2 => {
+              const batch = writeBatch(db2);
+              for (let i = 0; i < createdDocuments.length; i += 2) {
+                const documentToDelete = doc(db2, createdDocuments[i].path);
+                batch.delete(documentToDelete);
+                deletedDocumentIds.add(documentToDelete.id);
+              }
+              await batch.commit();
+            });
 
             // Wait for 10 seconds, during which Watch will stop tracking the
             // query and will send an existence filter rather than "delete"
@@ -2258,11 +2261,12 @@ apiDescribe('Queries', persistence => {
         );
 
         // Delete one of the documents so that the next call to getDocs() will
-        // experience an existence filter mismatch. Use a WriteBatch, rather
-        // than deleteDoc(), to avoid affecting the local cache.
-        const writeBatchForDocumentDeletes = writeBatch(db);
-        writeBatchForDocumentDeletes.delete(doc(coll, 'DocumentToDelete'));
-        await writeBatchForDocumentDeletes.commit();
+        // experience an existence filter mismatch. Use a different Firestore
+        // instance to avoid affecting the local cache.
+        const documentToDelete = doc(coll, 'DocumentToDelete');
+        await withTestDb(PERSISTENCE_MODE_UNSPECIFIED, async db2 => {
+          await deleteDoc(doc(db2, documentToDelete.path));
+        });
 
         // Wait for 10 seconds, during which Watch will stop tracking the query
         // and will send an existence filter rather than "delete" events when
@@ -2278,7 +2282,7 @@ apiDescribe('Queries', persistence => {
           documentSnapshot => documentSnapshot.id
         );
         const testDocIdsMinusDeletedDocId = testDocIds.filter(
-          documentId => documentId !== 'DocumentToDelete'
+          documentId => documentId !== documentToDelete.id
         );
         expect(snapshot2DocumentIds, 'snapshot2DocumentIds').to.have.members(
           testDocIdsMinusDeletedDocId
@@ -2309,20 +2313,17 @@ apiDescribe('Queries', persistence => {
         // is if there is a false positive when testing for 'DocumentToDelete'
         // in the bloom filter. So verify that the bloom filter application is
         // successful, unless there was a false positive.
-        const isFalsePositive = bloomFilter.mightContain!(
-          `${coll.path}/DocumentToDelete`
-        );
+        const isFalsePositive = bloomFilter.mightContain(documentToDelete);
         expect(bloomFilter.applied, 'bloomFilter.applied').to.equal(
           !isFalsePositive
         );
 
         // Verify that the bloom filter contains the document paths with complex
         // Unicode characters.
-        for (const testDocId of testDocIdsMinusDeletedDocId) {
-          const testDocPath = `${coll.path}/${testDocId}`;
+        for (const testDoc of snapshot2.docs.map(snapshot => snapshot.ref)) {
           expect(
-            bloomFilter.mightContain!(testDocPath),
-            `bloomFilter.mightContain('${testDocPath}')`
+            bloomFilter.mightContain(testDoc),
+            `bloomFilter.mightContain('${testDoc.path}')`
           ).to.be.true;
         }
       });