Successfully tested bundling a QuerySnapshot

Also includes:

 -  format
 -  lint fixes.
 - Comments for bundler methods
 - Addition of DocumentSnapshotBundleData & QuerySnapshotBundleData types.

Author: DellaBitta

packages/firestore/src/api/snapshot.ts

@@ -36,7 +36,11 @@ import { AbstractUserDataWriter } from '../lite-api/user_data_writer';
 import { Document } from '../model/document';
 import { DocumentKey } from '../model/document_key';
 import { debugAssert, fail } from '../util/assert';
-import { BundleBuilder, DocumentBundleData } from '../util/bundle_builder_impl';
+import {
+  BundleBuilder,
+  DocumentSnapshotBundleData,
+  QuerySnapshotBundleData
+} from '../util/bundle_builder_impl';
 import { Code, FirestoreError } from '../util/error';
 import { AutoId } from '../util/misc';
 
@@ -500,34 +504,26 @@ export class DocumentSnapshot<
   }
 
   toJSON(): object {
+    const document = this._document;
     if (
-      !this._document ||
-      !this._document.isValidDocument() ||
-      !this._document.isFoundDocument()
+      !document ||
+      !document.isValidDocument() ||
+      !document.isFoundDocument()
     ) {
       return { bundle: '' };
     }
-
     const builder: BundleBuilder = new BundleBuilder(
       this._firestore,
       AutoId.newId()
     );
     const documentData = this._userDataWriter.convertObjectMap(
-      this._document.data.value.mapValue.fields,
+      document.data.value.mapValue.fields,
       'previous'
     );
-
     builder.addBundleDocument(
-      DocumentToDocumentBundleData(
-        this._firestore,
-        this.ref.path,
-        documentData,
-        this._document
-      )
+      ToDocumentSnapshotBundleData(this.ref.path, documentData, document)
     );
-    return {
-      bundle: builder.build()
-    };
+    return { bundle: builder.build() };
   }
 }
 
@@ -690,7 +686,10 @@ export class QuerySnapshot<
       this._firestore,
       AutoId.newId()
     );
-    const docBundleDataArray: DocumentBundleData[] = [];
+    const databaseId = this._firestore._databaseId.database;
+    const projectId = this._firestore._databaseId.projectId;
+    const parent = `projects/${projectId}/databases/${databaseId}/documents`;
+    const docBundleDataArray: DocumentSnapshotBundleData[] = [];
     const docArray = this.docs;
     docArray.forEach(doc => {
       if (doc._document === null) {
@@ -701,16 +700,15 @@ export class QuerySnapshot<
         'previous'
       );
       docBundleDataArray.push(
-        DocumentToDocumentBundleData(
-          this._firestore,
-          doc.ref.path,
-          documentData,
-          doc._document
-        )
+        ToDocumentSnapshotBundleData(doc.ref.path, documentData, doc._document)
       );
     });
-
-    builder.addBundleQuery(this.query._query, docBundleDataArray);
+    const bundleData: QuerySnapshotBundleData = {
+      query: this.query._query,
+      parent,
+      docBundleDataArray
+    };
+    builder.addBundleQuery(bundleData);
     return { bundle: builder.build() };
   }
 }
@@ -853,12 +851,12 @@ export function snapshotEqual<AppModelType, DbModelType extends DocumentData>(
   return false;
 }
 
-function DocumentToDocumentBundleData(
-  firestore: Firestore,
+// Formats Document data for bundling a DocumentSnapshot.
+function ToDocumentSnapshotBundleData(
   path: string,
   documentData: DocumentData,
   document: Document
-): DocumentBundleData {
+): DocumentSnapshotBundleData {
   return {
     documentData,
     documentKey: document.mutableCopy().key,

packages/firestore/src/util/bundle_builder_impl.ts

@@ -43,6 +43,7 @@ import {
   Document as ProtoDocument,
   Document
 } from '../protos/firestore_proto_api';
+import { AutoId } from '../util/misc';
 
 import { debugAssert } from './assert';
 
@@ -82,7 +83,9 @@ export class BundleBuilder {
     );
   }
 
-  toBundleDocument(docBundleData: DocumentBundleData): ProtoDocument {
+  private toBundleDocument(
+    docBundleData: DocumentSnapshotBundleData
+  ): ProtoDocument {
     // TODO handle documents that have mutations
     debugAssert(
       !docBundleData.documentData.hasLocalMutations,
@@ -105,8 +108,16 @@ export class BundleBuilder {
     };
   }
 
+  /**
+   * Adds data from a DocumentSnapshot to the bundle.
+   * @internal
+   * @param docBundleData A DocumentSnapshotBundleData containing information from the
+   * DocumentSnapshot. Note we cannot accept a DocumentSnapshot directly due to a circular
+   * dependency error.
+   * @param queryName The name of the QuerySnapshot if this document is part of a Query.
+   */
   addBundleDocument(
-    docBundleData: DocumentBundleData,
+    docBundleData: DocumentSnapshotBundleData,
     queryName?: string
   ): void {
     const originalDocument = this.documents.get(docBundleData.documentPath);
@@ -130,7 +141,6 @@ export class BundleBuilder {
         }
       });
     }
-
     // Update `queries` to include both original and `queryName`.
     if (queryName) {
       const newDocument = this.documents.get(docBundleData.documentPath)!;
@@ -144,28 +154,33 @@ export class BundleBuilder {
     }
   }
 
-  addBundleQuery(query: Query, docBundleDataArray: DocumentBundleData[]): void {
-    const queryTarget = toQueryTarget(this.serializer, queryToTarget(query));
-    const name = queryTarget.parent.canonicalString();
-
+  /**
+   * Adds data from a QuerySnapshot to the bundle.
+   * @internal
+   * @param docBundleData A QuerySnapshotBundleData containing information from the
+   * QuerySnapshot. Note we cannot accept a QuerySnapshot directly due to a circular
+   * dependency error.
+   */
+  addBundleQuery(queryBundleData: QuerySnapshotBundleData): void {
+    const name = AutoId.newId();
     if (this.namedQueries.has(name)) {
       throw new Error(`Query name conflict: ${name} has already been added.`);
     }
-
     let latestReadTime = new Timestamp(0, 0);
-    for (const docBundleData of docBundleDataArray) {
+    for (const docBundleData of queryBundleData.docBundleDataArray) {
       this.addBundleDocument(docBundleData, name);
       if (docBundleData.readTime && docBundleData.readTime > latestReadTime) {
         latestReadTime = docBundleData.readTime;
       }
     }
-
+    const queryTarget = toQueryTarget(
+      this.serializer,
+      queryToTarget(queryBundleData.query)
+    );
     const bundledQuery = {
-      parent: queryTarget.parent.canonicalString(),
-      structuredQuery: queryTarget.queryTarget.structuredQuery,
-      limitType: null
+      parent: queryBundleData.parent,
+      structuredQuery: queryTarget.queryTarget.structuredQuery
     };
-
     this.namedQueries.set(name, {
       name,
       bundledQuery,
@@ -226,19 +241,26 @@ export class BundleBuilder {
 
 /**
  * Interface for an object that contains data required to bundle a DocumentSnapshot.
- * Accessing the methods of DocumentSnapshot directly to retreivew this data in this
- * implementation would create a circular dependency.
- *
  * @internal
  */
-export interface DocumentBundleData {
-  readonly documentData: DocumentData;
-  readonly documentKey: DocumentKey;
-  readonly documentPath: string;
-  readonly documentExists: boolean;
-  readonly createdTime: Timestamp;
-  readonly readTime?: Timestamp;
-  readonly versionTime: Timestamp;
+export interface DocumentSnapshotBundleData {
+  documentData: DocumentData;
+  documentKey: DocumentKey;
+  documentPath: string;
+  documentExists: boolean;
+  createdTime: Timestamp;
+  readTime?: Timestamp;
+  versionTime: Timestamp;
+}
+
+/**
+ * Interface for an object that contains data required to bundle a QuerySnapshot.
+ * @internal
+ */
+export interface QuerySnapshotBundleData {
+  query: Query;
+  parent: string;
+  docBundleDataArray: DocumentSnapshotBundleData[];
 }
 
 /**
@@ -252,47 +274,3 @@ class BundledDocument {
     readonly document?: Document
   ) {}
 }
-
-/**
- * Validates that 'value' is a string.
- *
- * @private
- * @internal
- * @param arg The argument name or argument index (for varargs methods).
- * @param value The input to validate.
- * @param options Options that specify whether the string can be omitted.
- */
-export function validateString(arg: string | number, value: unknown): void {
-  if (typeof value !== 'string') {
-    throw new Error(invalidArgumentMessage(arg, 'string'));
-  }
-}
-
-/**
- * Generates an error message to use with invalid arguments.
- *
- * @private
- * @internal
- * @param arg The argument name or argument index (for varargs methods).
- * @param expectedType The expected input type.
- */
-export function invalidArgumentMessage(
-  arg: string | number,
-  expectedType: string
-): string {
-  return `${formatArgumentName(arg)} is not a valid ${expectedType}.`;
-}
-
-/**
- * Creates a descriptive name for the provided argument name or index.
- *
- * @private
- * @internal
- * @param arg The argument name or argument index (for varargs methods).
- * @return Either the argument name or its index description.
- */
-function formatArgumentName(arg: string | number): string {
-  return typeof arg === 'string'
-    ? `Value for argument "${arg}"`
-    : `Element at index ${arg}`;
-}