Change the order of checks in applyRemoteEvent() (#2176)

Author: schmidt-sebastian

packages/firestore/src/local/local_store.ts

@@ -516,23 +516,36 @@ export class LocalStore {
           documentBuffer.getEntries(txn, updatedKeys).next(existingDocs => {
             remoteEvent.documentUpdates.forEach((key, doc) => {
               const existingDoc = existingDocs.get(key);
+
+              // Note: The order of the steps below is important, since we want
+              // to ensure that rejected limbo resolutions (which fabricate
+              // NoDocuments with SnapshotVersion.MIN) never add documents to
+              // cache.
               if (
+                doc instanceof NoDocument &&
+                doc.version.isEqual(SnapshotVersion.MIN)
+              ) {
+                // NoDocuments with SnapshotVersion.MIN are used in manufactured
+                // events. We remove these documents from cache since we lost
+                // access.
+                documentBuffer.removeEntry(key);
+                changedDocs = changedDocs.insert(key, doc);
+              } else if (
                 existingDoc == null ||
                 doc.version.compareTo(existingDoc.version) > 0 ||
                 (doc.version.compareTo(existingDoc.version) === 0 &&
                   existingDoc.hasPendingWrites)
               ) {
+                // TODO(index-free): Make this an assert when we enable
+                // Index-Free queries
+                if (SnapshotVersion.MIN.isEqual(remoteEvent.snapshotVersion)) {
+                  log.error(
+                    LOG_TAG,
+                    'Cannot add a document when the remote version is zero'
+                  );
+                }
                 documentBuffer.addEntry(doc);
                 changedDocs = changedDocs.insert(key, doc);
-              } else if (
-                doc instanceof NoDocument &&
-                doc.version.isEqual(SnapshotVersion.MIN)
-              ) {
-                // NoDocuments with SnapshotVersion.MIN are used in manufactured events (e.g. in the
-                // case of a limbo document resolution failing). We remove these documents from cache
-                // since we lost access.
-                documentBuffer.removeEntry(key);
-                changedDocs = changedDocs.insert(key, doc);
               } else {
                 log.debug(
                   LOG_TAG,

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

@@ -239,7 +239,13 @@ class LocalStoreTester {
   toContain(doc: MaybeDocument): LocalStoreTester {
     this.promiseChain = this.promiseChain.then(() => {
       return this.localStore.readDocument(doc.key).then(result => {
-        expectEqual(result, doc);
+        expectEqual(
+          result,
+          doc,
+          `Expected ${
+            result ? result.toString() : null
+          } to match ${doc.toString()}.`
+        );
       });
     });
     return this;
@@ -759,15 +765,15 @@ function genericLocalStoreTests(
       expectLocalStore()
         .afterAllocatingQuery(query)
         .toReturnTargetId(2)
-        .after(docAddedRemoteEvent(doc('foo/bar', 0, { foo: 'old' }), [2]))
+        .after(docAddedRemoteEvent(doc('foo/bar', 1, { foo: 'old' }), [2]))
         .after(patchMutation('foo/bar', { foo: 'bar' }))
         // Release the query so that our target count goes back to 0 and we are considered
         // up-to-date.
         .afterReleasingQuery(query)
         .after(setMutation('foo/bah', { foo: 'bah' }))
         .after(deleteMutation('foo/baz'))
         .toContain(
-          doc('foo/bar', 0, { foo: 'bar' }, { hasLocalMutations: true })
+          doc('foo/bar', 1, { foo: 'bar' }, { hasLocalMutations: true })
         )
         .toContain(
           doc('foo/bah', 0, { foo: 'bah' }, { hasLocalMutations: true })
@@ -800,15 +806,15 @@ function genericLocalStoreTests(
       expectLocalStore()
         .afterAllocatingQuery(query)
         .toReturnTargetId(2)
-        .after(docAddedRemoteEvent(doc('foo/bar', 0, { foo: 'old' }), [2]))
+        .after(docAddedRemoteEvent(doc('foo/bar', 1, { foo: 'old' }), [2]))
         .after(patchMutation('foo/bar', { foo: 'bar' }))
         // Release the query so that our target count goes back to 0 and we are considered
         // up-to-date.
         .afterReleasingQuery(query)
         .after(setMutation('foo/bah', { foo: 'bah' }))
         .after(deleteMutation('foo/baz'))
         .toContain(
-          doc('foo/bar', 0, { foo: 'bar' }, { hasLocalMutations: true })
+          doc('foo/bar', 1, { foo: 'bar' }, { hasLocalMutations: true })
         )
         .toContain(
           doc('foo/bah', 0, { foo: 'bah' }, { hasLocalMutations: true })