Idempotency: Address TODOs, add Changelog (#2270)

Author: schmidt-sebastian

packages/firestore/CHANGELOG.md

@@ -1,8 +1,14 @@
 # Unreleased
+- [changed] Fixed a crash on iOS 13 that occurred when persistence was enabled
+  in a background tab (#2232).
+
+# 1.6.0
 - [fixed] Fixed a regression that caused queries with nested field filters to
   crash the client if the field was not present in the local copy of the
   document. 
-
+- [feature] Added a `Firestore.onSnapshotsInSync()` method that notifies you
+  when all your snapshot listeners are in sync with each other.
+  
 # 1.5.0
 - [feature] Added a `Firestore.waitForPendingWrites()` method that
   allows users to wait until all pending writes are acknowledged by the
@@ -15,8 +21,6 @@
   small subset of the documents in a collection.
 - [fixed] Fixed a race condition between authenticating and initializing
   Firestore that could result in initial writes to the database being dropped.
-- [feature] Added a `Firestore.onSnapshotsInSync()` method that notifies you
-  when all your snapshot listeners are in sync with each other.
 
 # 1.4.10
 - [changed] Transactions now perform exponential backoff before retrying.
@@ -35,7 +39,6 @@
   match the query (https://github.com/firebase/firebase-android-sdk/issues/155).
 
 # 1.4.4
->>>>>>> master
 - [fixed] Fixed an internal assertion that was triggered when an update
    with a `FieldValue.serverTimestamp()` and an update with a
   `FieldValue.increment()` were pending for the same document.

packages/firestore/src/local/simple_db.ts

@@ -274,17 +274,6 @@ export class SimpleDb {
       );
       try {
         const transactionFnResult = transactionFn(transaction)
-          // TODO(schmidt-sebastian): Remove this code/comment or find a way to
-          // make this a test-only setting.
-          // Horrible hack to verify that idempotent functions can be run more
-          // than once.
-          .next(result => {
-            if (idempotent && attemptNumber === 1) {
-              class DOMException {}
-              throw new DOMException();
-            }
-            return result;
-          })
           .catch(error => {
             // Abort the transaction if there was an error.
             transaction.abort(error);

packages/firestore/test/unit/local/local_store.test.ts

@@ -1076,10 +1076,7 @@ function genericLocalStoreTests(
     ]);
   });
 
-  // TODO(schmidt-sebastian): This test makes idempotency testing harder.
-  // Comment back in when done with the idempotent migration.
-  // eslint-disable-next-line no-restricted-properties
-  it.skip('reads all documents for initial collection queries', () => {
+  it('reads all documents for initial collection queries', () => {
     const firstQuery = Query.atPath(path('foo'));
     const secondQuery = Query.atPath(path('foo')).addFilter(
       filter('matches', '==', true)
@@ -1483,55 +1480,55 @@ function genericLocalStoreTests(
     );
   });
 
-  // TODO(schmidt-sebastian): This test makes idempotency testing harder.
-  // Comment back in when done with the idempotent migration.
-  // (queryEngine instanceof IndexFreeQueryEngine && !gcIsEager ? it : it.skip)(
   // eslint-disable-next-line no-restricted-properties
-  it.skip('uses target mapping to execute queries', () => {
-    // This test verifies that once a target mapping has been written, only
-    // documents that match the query are read from the RemoteDocumentCache.
+  (queryEngine instanceof IndexFreeQueryEngine && !gcIsEager ? it : it.skip)(
+    'uses target mapping to execute queries',
+    () => {
+      // This test verifies that once a target mapping has been written, only
+      // documents that match the query are read from the RemoteDocumentCache.
 
-    const query = Query.atPath(path('foo')).addFilter(
-      filter('matches', '==', true)
-    );
-    return (
-      expectLocalStore()
-        .afterAllocatingQuery(query)
-        .toReturnTargetId(2)
-        .after(setMutation('foo/a', { matches: true }))
-        .after(setMutation('foo/b', { matches: true }))
-        .after(setMutation('foo/ignored', { matches: false }))
-        .afterAcknowledgingMutation({ documentVersion: 10 })
-        .afterAcknowledgingMutation({ documentVersion: 10 })
-        .afterAcknowledgingMutation({ documentVersion: 10 })
-        .afterExecutingQuery(query)
-        // Execute the query, but note that we read all existing documents
-        // from the RemoteDocumentCache since we do not yet have target
-        // mapping.
-        .toHaveRead({ documentsByQuery: 2 })
-        .after(
-          docAddedRemoteEvent(
-            [
-              doc('foo/a', 10, { matches: true }),
-              doc('foo/b', 10, { matches: true })
-            ],
-            [2],
-            []
+      const query = Query.atPath(path('foo')).addFilter(
+        filter('matches', '==', true)
+      );
+      return (
+        expectLocalStore()
+          .afterAllocatingQuery(query)
+          .toReturnTargetId(2)
+          .after(setMutation('foo/a', { matches: true }))
+          .after(setMutation('foo/b', { matches: true }))
+          .after(setMutation('foo/ignored', { matches: false }))
+          .afterAcknowledgingMutation({ documentVersion: 10 })
+          .afterAcknowledgingMutation({ documentVersion: 10 })
+          .afterAcknowledgingMutation({ documentVersion: 10 })
+          .afterExecutingQuery(query)
+          // Execute the query, but note that we read all existing documents
+          // from the RemoteDocumentCache since we do not yet have target
+          // mapping.
+          .toHaveRead({ documentsByQuery: 2 })
+          .after(
+            docAddedRemoteEvent(
+              [
+                doc('foo/a', 10, { matches: true }),
+                doc('foo/b', 10, { matches: true })
+              ],
+              [2],
+              []
+            )
           )
-        )
-        .after(
-          noChangeEvent(/* targetId= */ 2, /* snapshotVersion= */ 10, 'foo')
-        )
-        .after(localViewChanges(2, /* fromCache= */ false, {}))
-        .afterExecutingQuery(query)
-        .toHaveRead({ documentsByKey: 2, documentsByQuery: 0 })
-        .toReturnChanged(
-          doc('foo/a', 10, { matches: true }),
-          doc('foo/b', 10, { matches: true })
-        )
-        .finish()
-    );
-  });
+          .after(
+            noChangeEvent(/* targetId= */ 2, /* snapshotVersion= */ 10, 'foo')
+          )
+          .after(localViewChanges(2, /* fromCache= */ false, {}))
+          .afterExecutingQuery(query)
+          .toHaveRead({ documentsByKey: 2, documentsByQuery: 0 })
+          .toReturnChanged(
+            doc('foo/a', 10, { matches: true }),
+            doc('foo/b', 10, { matches: true })
+          )
+          .finish()
+      );
+    }
+  );
 
   it('last limbo free snapshot is advanced during view processing', async () => {
     // This test verifies that the `lastLimboFreeSnapshot` version for QueryData