Merge branch 'master' into mrschmidt-dropacksinschemamigration

Author: schmidt-sebastian

packages/firestore/src/api/database.ts

@@ -594,8 +594,11 @@ export class Transaction implements firestore.Transaction {
         const doc = docs[0];
         if (doc instanceof NoDocument) {
           return new DocumentSnapshot(this._firestore, ref._key, null, false);
+        } else if (doc instanceof Document) {
+          return new DocumentSnapshot(this._firestore, ref._key, doc, false);
+        } else {
+          fail('MaybeDocument is neither Document nor NoDocument');
         }
-        return new DocumentSnapshot(this._firestore, ref._key, doc, false);
       });
   }
 

packages/firestore/src/core/sync_engine.ts

@@ -679,20 +679,16 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
         queryView.targetId
       );
       this.limboDocumentRefs.removeReferencesForId(queryView.targetId);
-      let p = PersistencePromise.resolve();
-      limboKeys.forEach(limboKey => {
-        p = p.next(() => {
-          return this.limboDocumentRefs
-            .containsKey(null, limboKey)
-            .next(isReferenced => {
-              if (!isReferenced) {
-                // We removed the last reference for this key
-                this.removeLimboTarget(limboKey);
-              }
-            });
-        });
-      });
-      await p.toPromise();
+      await PersistencePromise.forEach(limboKeys.toArray(), limboKey => {
+        return this.limboDocumentRefs
+          .containsKey(null, limboKey)
+          .next(isReferenced => {
+            if (!isReferenced) {
+              // We removed the last reference for this key
+              this.removeLimboTarget(limboKey);
+            }
+          });
+      }).toPromise();
     }
   }
 

packages/firestore/src/local/indexeddb_persistence.ts

@@ -376,14 +376,12 @@ export class IndexedDbPersistence implements Persistence {
               client => activeClients.indexOf(client) === -1
             );
           })
-          .next(() => {
+          .next(() =>
             // Delete metadata for clients that are no longer considered active.
-            let p = PersistencePromise.resolve();
-            inactiveClients.forEach(inactiveClient => {
-              p = p.next(() => metadataStore.delete(inactiveClient.clientId));
-            });
-            return p;
-          })
+            PersistencePromise.forEach(inactiveClients, inactiveClient =>
+              metadataStore.delete(inactiveClient.clientId)
+            )
+          )
           .next(() => {
             // Retrieve the minimum change ID from the set of active clients.
 

packages/firestore/src/local/local_store.ts

@@ -301,13 +301,11 @@ export class LocalStore {
           return PersistencePromise.resolve([]);
         }
       })
-      .next(ackedBatches => {
-        let p = PersistencePromise.resolve();
-        ackedBatches.forEach(batch => {
-          p = p.next(() => this.mutationQueue.removeMutationBatch(txn, batch));
-        });
-        return p;
-      });
+      .next(ackedBatches =>
+        PersistencePromise.forEach(ackedBatches, batch =>
+          this.mutationQueue.removeMutationBatch(txn, batch)
+        )
+      );
   }
 
   /* Accept locally generated Mutations and commit them to storage. */
@@ -979,17 +977,15 @@ export class LocalStore {
     batches: MutationBatch[]
   ): PersistencePromise<DocumentKeySet> {
     let affectedDocs = documentKeySet();
-
-    let p = PersistencePromise.resolve();
     for (const batch of batches) {
       for (const mutation of batch.mutations) {
         const key = mutation.key;
         affectedDocs = affectedDocs.add(key);
       }
-      p = p.next(() => this.mutationQueue.removeMutationBatch(txn, batch));
     }
-
-    return p.next(() => affectedDocs);
+    return PersistencePromise.forEach(batches, batch =>
+      this.mutationQueue.removeMutationBatch(txn, batch)
+    ).next(() => affectedDocs);
   }
 
   private applyWriteToRemoteDocuments(

packages/firestore/src/local/persistence_promise.ts

@@ -205,4 +205,15 @@ export class PersistencePromise<T> {
       return results;
     });
   }
+
+  static forEach<T>(
+    elements: T[],
+    callback: (T) => PersistencePromise<void>
+  ): PersistencePromise<void> {
+    let p = PersistencePromise.resolve();
+    for (const element of elements) {
+      p = p.next(() => callback(element));
+    }
+    return p;
+  }
 }

packages/firestore/src/model/document.ts

@@ -26,15 +26,32 @@ export interface DocumentOptions {
   hasLocalMutations: boolean;
 }
 
-export class Document {
+/**
+ * The result of a lookup for a given path may be an existing document or a
+ * marker that this document does not exist at a given version.
+ */
+export abstract class MaybeDocument {
+  constructor(readonly key: DocumentKey, readonly version: SnapshotVersion) {}
+
+  static compareByKey(d1: MaybeDocument, d2: MaybeDocument): number {
+    return DocumentKey.comparator(d1.key, d2.key);
+  }
+}
+
+/**
+ * Represents a document in Firestore with a key, version, data and whether the
+ * data has local mutations applied to it.
+ */
+export class Document extends MaybeDocument {
   readonly hasLocalMutations: boolean;
 
   constructor(
-    readonly key: DocumentKey,
-    readonly version: SnapshotVersion,
+    key: DocumentKey,
+    version: SnapshotVersion,
     readonly data: ObjectValue,
     options: DocumentOptions
   ) {
+    super(key, version);
     this.hasLocalMutations = options.hasLocalMutations;
   }
 
@@ -68,10 +85,6 @@ export class Document {
     );
   }
 
-  static compareByKey(d1: MaybeDocument, d2: MaybeDocument): number {
-    return DocumentKey.comparator(d1.key, d2.key);
-  }
-
   static compareByField(field: FieldPath, d1: Document, d2: Document): number {
     const v1 = d1.field(field);
     const v2 = d2.field(field);
@@ -88,8 +101,10 @@ export class Document {
  * Version is set to 0 if we don't point to any specific time, otherwise it
  * denotes time we know it didn't exist at.
  */
-export class NoDocument {
-  constructor(readonly key: DocumentKey, readonly version: SnapshotVersion) {}
+export class NoDocument extends MaybeDocument {
+  constructor(key: DocumentKey, version: SnapshotVersion) {
+    super(key, version);
+  }
 
   toString(): string {
     return `NoDocument(${this.key}, ${this.version})`;
@@ -102,14 +117,4 @@ export class NoDocument {
       other.key.isEqual(this.key)
     );
   }
-
-  static compareByKey(d1: MaybeDocument, d2: MaybeDocument): number {
-    return DocumentKey.comparator(d1.key, d2.key);
-  }
 }
-
-/**
- * A union type representing either a full document or a deleted document.
- * The NoDocument is used when it doesn't exist on the server.
- */
-export type MaybeDocument = Document | NoDocument;

packages/firestore/src/remote/watch_change.ts

@@ -61,7 +61,7 @@ export class DocumentWatchChange {
      * The new document or NoDocument if it was deleted. Is null if the
      * document went out of view without the server sending a new document.
      */
-    public newDoc: Document | NoDocument | null
+    public newDoc: MaybeDocument | null
   ) {}
 }
 

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

@@ -560,37 +560,53 @@ apiDescribe('Validation:', persistence => {
   );
 
   validationIt(persistence, 'Field paths must not have empty segments', db => {
-    const badFieldPaths = ['', 'foo..baz', '.foo', 'foo.'];
-    const promises = [];
-    for (const fieldPath of badFieldPaths) {
-      const reason =
-        `Invalid field path (${fieldPath}). Paths must not be ` +
-        `empty, begin with '.', end with '.', or contain '..'`;
-      promises.push(expectFieldPathToFail(db, fieldPath, reason));
-    }
-    return Promise.all(promises);
+    const docRef = db.collection('test').doc();
+    return docRef
+      .set({ test: 1 })
+      .then(() => {
+        return docRef.get();
+      })
+      .then(snapshot => {
+        const badFieldPaths = ['', 'foo..baz', '.foo', 'foo.'];
+        const promises = [];
+        for (const fieldPath of badFieldPaths) {
+          const reason =
+            `Invalid field path (${fieldPath}). Paths must not be ` +
+            `empty, begin with '.', end with '.', or contain '..'`;
+          promises.push(expectFieldPathToFail(snapshot, fieldPath, reason));
+        }
+        return Promise.all(promises);
+      });
   });
 
   validationIt(
     persistence,
     'Field paths must not have invalid segments',
     db => {
-      const badFieldPaths = [
-        'foo~bar',
-        'foo*bar',
-        'foo/bar',
-        'foo[1',
-        'foo]1',
-        'foo[1]'
-      ];
-      const promises = [];
-      for (const fieldPath of badFieldPaths) {
-        const reason =
-          `Invalid field path (${fieldPath}). Paths must not ` +
-          `contain '~', '*', '/', '[', or ']'`;
-        promises.push(expectFieldPathToFail(db, fieldPath, reason));
-      }
-      return Promise.all(promises);
+      const docRef = db.collection('test').doc();
+      return docRef
+        .set({ test: 1 })
+        .then(() => {
+          return docRef.get();
+        })
+        .then(snapshot => {
+          const badFieldPaths = [
+            'foo~bar',
+            'foo*bar',
+            'foo/bar',
+            'foo[1',
+            'foo]1',
+            'foo[1]'
+          ];
+          const promises = [];
+          for (const fieldPath of badFieldPaths) {
+            const reason =
+              `Invalid field path (${fieldPath}). Paths must not ` +
+              `contain '~', '*', '/', '[', or ']'`;
+            promises.push(expectFieldPathToFail(snapshot, fieldPath, reason));
+          }
+          return Promise.all(promises);
+        });
     }
   );
 
@@ -953,37 +969,33 @@ function expectWriteToFail(
  * they fail with the specified reason.
  */
 function expectFieldPathToFail(
-  db: firestore.FirebaseFirestore,
+  snapshot: firestore.DocumentSnapshot,
   path: string,
   reason: string
 ): Promise<void> {
   // Get an arbitrary snapshot we can use for testing.
-  const docRef = db.collection('test').doc();
-  return docRef
-    .set({ test: 1 })
-    .then(() => {
-      return docRef.get();
-    })
-    .then(snapshot => {
-      // Snapshot paths.
-      expect(() => snapshot.get(path)).to.throw(
-        'Function DocumentSnapshot.get() called with invalid data. ' + reason
-      );
+  return Promise.resolve().then(() => {
+    // Snapshot paths.
+    expect(() => snapshot.get(path)).to.throw(
+      'Function DocumentSnapshot.get() called with invalid data. ' + reason
+    );
 
-      // Query filter / order fields.
-      const coll = db.collection('test-collection');
-      // <=, etc omitted for brevity since the code path is trivially
-      // shared.
-      expect(() => coll.where(path, '==', 1)).to.throw(
-        'Function Query.where() called with invalid data. ' + reason
-      );
-      expect(() => coll.orderBy(path)).to.throw(
-        'Function Query.orderBy() called with invalid data. ' + reason
-      );
+    const db = snapshot.ref.firestore;
 
-      // Update paths.
-      const data = {} as { [field: string]: number };
-      data[path] = 1;
-      return expectUpdateToFail(db, data, reason);
-    });
+    // Query filter / order fields.
+    const coll = db.collection('test-collection');
+    // <=, etc omitted for brevity since the code path is trivially
+    // shared.
+    expect(() => coll.where(path, '==', 1)).to.throw(
+      'Function Query.where() called with invalid data. ' + reason
+    );
+    expect(() => coll.orderBy(path)).to.throw(
+      'Function Query.orderBy() called with invalid data. ' + reason
+    );
+
+    // Update paths.
+    const data = {} as { [field: string]: number };
+    data[path] = 1;
+    return expectUpdateToFail(db, data, reason);
+  });
 }

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

@@ -290,11 +290,9 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => {
       const sdb = new SimpleDb(db);
       return sdb.runTransaction('readwrite', [DbMutationBatch.store], txn => {
         const store = txn.store(DbMutationBatch.store);
-        let p = PersistencePromise.resolve();
-        for (const testMutation of testMutations) {
-          p = p.next(() => store.put(testMutation));
-        }
-        return p;
+        return PersistencePromise.forEach(testMutations, testMutation =>
+          store.put(testMutation)
+        );
       });
     }).then(() =>
       withDb(4, db => {
@@ -306,14 +304,11 @@ describe('IndexedDbSchema: createOrUpgradeDb', () => {
           const store = txn.store<DbMutationBatchKey, DbMutationBatch>(
             DbMutationBatch.store
           );
-          let p = PersistencePromise.resolve();
-          for (const testMutation of testMutations) {
-            p = p.next(() =>
-              store.get(testMutation.batchId).next(mutationBatch => {
-                expect(mutationBatch).to.deep.equal(testMutation);
-              })
-            );
-          }
+          let p = PersistencePromise.forEach(testMutations, testMutation =>
+            store.get(testMutation.batchId).next(mutationBatch => {
+              expect(mutationBatch).to.deep.equal(testMutation);
+            })
+          );
           p = p.next(() => {
             store
               .add({} as any) // tslint:disable-line:no-any

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

@@ -213,4 +213,13 @@ describe('PersistencePromise', () => {
       })
       .toPromise();
   });
+
+  it('executes forEach in order', async () => {
+    let result = '';
+    await PersistencePromise.forEach(['a', 'b', 'c'], el => {
+      result += el;
+      return PersistencePromise.resolve();
+    }).toPromise;
+    expect(result).to.equal('abc');
+  });
 });