Checking for UnknownDocument (#1174)

* Checking for UnknownDocument

* Remove unused import

* Adding an assert in Transaction

Author: schmidt-sebastian

packages/firestore/src/api/database.ts

@@ -597,7 +597,11 @@ export class Transaction implements firestore.Transaction {
         } else if (doc instanceof Document) {
           return new DocumentSnapshot(this._firestore, ref._key, doc, false);
         } else {
-          throw fail('MaybeDocument is neither Document nor NoDocument');
+          throw fail(
+            `BatchGetDocumentsRequest returned unexpected document type: ${
+              doc.constructor.name
+            }`
+          );
         }
       });
   }

packages/firestore/src/core/transaction.ts

@@ -17,12 +17,13 @@
 import { ParsedSetData, ParsedUpdateData } from '../api/user_data_converter';
 import { SnapshotVersion } from './snapshot_version';
 import { documentVersionMap } from '../model/collections';
-import { NoDocument } from '../model/document';
+import { NoDocument, Document } from '../model/document';
 import { MaybeDocument } from '../model/document';
 import { DocumentKey } from '../model/document_key';
 import { DeleteMutation, Mutation, Precondition } from '../model/mutation';
 import { Datastore } from '../remote/datastore';
 import { Code, FirestoreError } from '../util/error';
+import { fail } from '../util/assert';
 
 /**
  * Internal transaction object responsible for accumulating the mutations to
@@ -36,7 +37,7 @@ export class Transaction {
 
   constructor(private datastore: Datastore) {}
 
-  private recordVersion(doc: MaybeDocument): void {
+  private recordVersion(doc: NoDocument | Document): void {
     let docVersion = doc.version;
     if (doc instanceof NoDocument) {
       // For deleted docs, we must use baseVersion 0 when we overwrite them.
@@ -68,7 +69,13 @@ export class Transaction {
       );
     }
     return this.datastore.lookup(keys).then(docs => {
-      docs.forEach(doc => this.recordVersion(doc));
+      docs.forEach(doc => {
+        if (doc instanceof NoDocument || doc instanceof Document) {
+          this.recordVersion(doc);
+        } else {
+          fail('Document in a transaction was a ' + doc.constructor.name);
+        }
+      });
       return docs;
     });
   }

packages/firestore/src/local/local_documents_view.ts

@@ -27,7 +27,6 @@ import { Document, MaybeDocument, NoDocument } from '../model/document';
 import { DocumentKey } from '../model/document_key';
 import { MutationBatch } from '../model/mutation_batch';
 import { ResourcePath } from '../model/path';
-import { fail } from '../util/assert';
 
 import { MutationQueue } from './mutation_queue';
 import { PersistenceTransaction } from './persistence';
@@ -168,12 +167,10 @@ export class LocalDocumentsView {
               baseDoc,
               batch.localWriteTime
             );
-            if (!mutatedDoc || mutatedDoc instanceof NoDocument) {
-              results = results.remove(key);
-            } else if (mutatedDoc instanceof Document) {
+            if (mutatedDoc instanceof Document) {
               results = results.insert(key, mutatedDoc);
             } else {
-              fail('Unknown MaybeDocument: ' + mutatedDoc);
+              results = results.remove(key);
             }
           }
         }

packages/firestore/src/model/mutation.ts

@@ -156,11 +156,7 @@ export class Precondition {
         maybeDoc.version.isEqual(this.updateTime)
       );
     } else if (this.exists !== undefined) {
-      if (this.exists) {
-        return maybeDoc instanceof Document;
-      } else {
-        return maybeDoc === null || maybeDoc instanceof NoDocument;
-      }
+      return this.exists === maybeDoc instanceof Document;
     } else {
       assert(this.isNone, 'Precondition should be empty');
       return true;