resolve comments

Author: milaGGL

packages/firestore/src/lite-api/aggregate.ts

@@ -16,25 +16,21 @@
  */
 
 import { invokeRunAggregationQueryRpc } from '../remote/datastore';
+import { hardAssert } from '../util/assert';
 import { cast } from '../util/input_validation';
 import { getDatastore } from './components';
 import { Firestore } from './database';
-import { DocumentData, Query, queryEqual } from './reference';
+import { Query, queryEqual } from './reference';
 import { LiteUserDataWriter } from './reference_impl';
 
 /**
- * A {@code AggregateQuery} computes some aggregation statistics from the result set of a base
- * {@link Query}.
- *
- * <p><b>Subclassing Note</b>: Cloud Firestore classes are not meant to be subclassed except for use
- * in test mocks. Subclassing is not supported in production code and new SDK releases may break
- * code that does so.
+ * A `AggregateQuery` computes some aggregation statistics from the result set of
+ * a base `Query`.
  */
 export class AggregateQuery {
   readonly type = 'AggregateQuery';
   /**
-   * The query on which you called {@link countQuery} in order to get this
-   * `AggregateQuery`.
+   * The query on which you called `countQuery` in order to get this `AggregateQuery`.
    * Query type is set to unknown to avoid error caused by query type converter.
    * might change it back to T after testing if the error do exist or not
    */
@@ -44,15 +40,10 @@ export class AggregateQuery {
   constructor(query: Query<unknown>) {
     this.query = query;
   }
-
 }
 
 /**
- * A {@code AggregateQuerySnapshot} contains results of a {@link AggregateQuery}.
- *
- * <p><b>Subclassing Note</b>: Cloud Firestore classes are not meant to be subclassed except for use
- * in test mocks. Subclassing is not supported in production code and new SDK releases may break
- * code that does so.
+ * A `AggregateQuerySnapshot` contains results of a `AggregateQuery`.
  */
 export class AggregateQuerySnapshot {
   readonly type = 'AggregateQuerySnapshot';
@@ -73,15 +64,12 @@ export class AggregateQuerySnapshot {
 }
 
 /**
- * Creates an {@link AggregateQuery} counting the number of documents matching this query.
+ * Creates an `AggregateQuery` counting the number of documents matching this query.
  *
- * @return An {@link AggregateQuery} object that can be used to count the number of documents in
+ * @return An `AggregateQuery` object that can be used to count the number of documents in
  * the result set of this query.
  */
 export function countQuery(query: Query<unknown>): AggregateQuery {
-  /**
-   * TODO(mila): add the "count" aggregateField to the params after the AggregateQuery is updated.
-   */
   return new AggregateQuery(query);
 }
 
@@ -95,24 +83,18 @@ export function getAggregateFromServerDirect(
   return invokeRunAggregationQueryRpc(datastore, aggregateQuery).then(
     result => {
       const aggregationFields = new Map<string, any>();
-      /**
-       * while getting aggregation fields from server direct, it should have only
-       * one RunAggregationQueryResponse returned.
-       * But we used streaming rpc here, so we will have an array of
-       * (one, or possibly more) RunAggregationQueryResponse. For this specific
-       * function, we get the first RunAggregationQueryResponse only.
-       */
+
       for (const [key, value] of Object.entries(result[0])) {
         aggregationFields.set(key, userDataWriter.convertValue(value));
       }
+      const countField = aggregationFields.get('count_alias');
+
+      hardAssert(
+        countField !== undefined && typeof countField === 'number',
+        'Count aggeragte field is invalid. countField:' + countField
+      );
       return Promise.resolve(
-        new AggregateQuerySnapshot(
-          aggregateQuery,
-          //return the count value , or null if count is undfined
-          aggregationFields.has('count_alias')
-            ? aggregationFields.get('count_alias')
-            : null
-        )
+        new AggregateQuerySnapshot(aggregateQuery, countField!)
       );
     }
   );

packages/firestore/src/remote/datastore.ts

@@ -254,7 +254,7 @@ export async function invokeRunAggregationQueryRpc(
   });
   return (
     response
-      // Omit RunQueryResponses that only contain readTimes.
+      // Omit RunAggregationQueryResponse that only contain readTimes.
       .filter(proto => !!proto.result && !!proto.result.aggregateFields)
       .map(proto => proto.result!.aggregateFields!)
   );

packages/firestore/src/remote/serializer.ts

@@ -77,13 +77,13 @@ import {
   OrderDirection as ProtoOrderDirection,
   Precondition as ProtoPrecondition,
   QueryTarget as ProtoQueryTarget,
+  RunAggregationQueryRequest as ProtoRunAggregationQueryRequest,
   Status as ProtoStatus,
   Target as ProtoTarget,
   TargetChangeTargetChangeType as ProtoTargetChangeTargetChangeType,
   Timestamp as ProtoTimestamp,
   Write as ProtoWrite,
-  WriteResult as ProtoWriteResult,
-  RunAggregationQueryRequest as ProtoRunAggregationQueryRequest
+  WriteResult as ProtoWriteResult
 } from '../protos/firestore_proto_api';
 import { debugAssert, fail, hardAssert } from '../util/assert';
 import { ByteString } from '../util/byte_string';
@@ -857,62 +857,20 @@ export function toRunAggregationQueryRequest(
   serializer: JsonProtoSerializer,
   target: Target
 ): ProtoRunAggregationQueryRequest {
-  // Dissect the path into parent, collectionId, and optional key filter.
-  const result: ProtoRunAggregationQueryRequest = {
+  const queryTarget = toQueryTarget(serializer, target);
+
+  return {
     structuredAggregationQuery: {
       aggregations: [
-        //TODO: dynamically add the aggregate fields in future development
         {
-          count: {
-          },
-          alias:"count_alias"
-        },
+          count: {},
+          alias: 'count_alias'
+        }
       ],
-      structuredQuery: {}
-    }
+      structuredQuery: queryTarget.structuredQuery
+    },
+    parent: queryTarget.parent
   };
-  const path = target.path;
-  if (target.collectionGroup !== null) {
-    debugAssert(
-      path.length % 2 === 0,
-      'Collection Group queries should be within a document path or root.'
-    );
-    result.parent = toQueryPath(serializer, path);
-    result.structuredAggregationQuery!.structuredQuery!.from = [
-      {
-        collectionId: target.collectionGroup,
-        allDescendants: true
-      }
-    ];
-  } else {
-    debugAssert(
-      path.length % 2 !== 0,
-      'Document queries with filters are not supported.'
-    );
-    result.parent = toQueryPath(serializer, path.popLast());
-    result.structuredAggregationQuery!.structuredQuery!.from = [
-      { collectionId: path.lastSegment() }
-    ];
-  }
-
-  const where = toFilter(target.filters);
-  if (where) {
-    result.structuredAggregationQuery!.structuredQuery!.where = where;
-  }
-  /** QUESTION: in count query, do we need to add OrderBy? Number of
-   *  documents shouldn't be impacted by how the documents are ordered.
-   */
-  const orderBy = toOrder(target.orderBy);
-  if (orderBy) {
-    result.structuredAggregationQuery!.structuredQuery!.orderBy = orderBy;
-  }
-  
-  const limit = toInt32Proto(serializer, target.limit);
-  if (limit !== null) {
-    result.structuredAggregationQuery!.structuredQuery!.limit = limit;
-  }
-
-  return result;
 }
 
 export function convertQueryTargetToQuery(target: ProtoQueryTarget): Query {

packages/firestore/test/lite/integration.test.ts

@@ -2069,118 +2069,118 @@ describe('countQuery()', () => {
     { key: 'c', value: 'Cooper' }
   ];
 
-  it('empty collection count equals to 0', () => {
+  it.only('AggregateQuery and AggregateQuerySnapshot inherits the original query', () => {
     return withTestCollection(async coll => {
-      const countQuery_ = countQuery(query(coll));
+      const query_ = query(coll);
+
+      const countQuery_ = countQuery(query_);
       expect(countQuery_.type).to.equal('AggregateQuery');
+      expect(countQuery_.query).to.equal(query_);
+
+      const snapshot = await getAggregateFromServerDirect(countQuery_);
+      expect(snapshot.query.query).to.equal(query_);
+    });
+  });
+
+  it.only('empty collection count equals to 0', () => {
+    return withTestCollection(async coll => {
+      const countQuery_ = countQuery(query(coll));
 
       const snapshot = await getAggregateFromServerDirect(countQuery_);
       expect(snapshot.getCount()).to.equal(0);
     });
   });
 
-  it('test collection count equals to 6', () => {
+  it.only('test collection count equals to 6', () => {
     return withTestCollectionAndInitialData(testDocs, async collection => {
       const countQuery_ = countQuery(query(collection));
       const snapshot = await getAggregateFromServerDirect(countQuery_);
       expect(snapshot.getCount()).to.equal(6);
     });
   });
 
-  it('test collection count with filter', () => {
+  it.only('test collection count with filter', () => {
     return withTestCollectionAndInitialData(testDocs, async collection => {
       const query_ = query(collection, where('key', '==', 'a'));
-
       const countQuery_ = countQuery(query_);
       const snapshot = await getAggregateFromServerDirect(countQuery_);
       expect(snapshot.getCount()).to.equal(2);
     });
   });
 
-  it('test collection count with filter effected by small limit', () => {
-    // limit that is less that the actual count would work like count up_to.
+  it.only('test collection count with filter and a small limit size', () => {
     return withTestCollectionAndInitialData(testDocs, async collection => {
       const query_ = query(collection, where('key', '==', 'a'), limit(1));
-
       const countQuery_ = countQuery(query_);
       const snapshot = await getAggregateFromServerDirect(countQuery_);
       expect(snapshot.getCount()).to.equal(1);
     });
   });
 
-  it('test collection count with filter not effected by large limit', () => {
-    //limit that is larger than actual count wouldn't impact the return value
+  it.only('test collection count with filter and a large limit size', () => {
     return withTestCollectionAndInitialData(testDocs, async collection => {
       const query_ = query(collection, where('key', '==', 'a'), limit(3));
-
       const countQuery_ = countQuery(query_);
       const snapshot = await getAggregateFromServerDirect(countQuery_);
       expect(snapshot.getCount()).to.equal(2);
     });
   });
 
-  it('test collection count with converter on query', () => {
-    //testing out the converter impact on the AggregateQuery type
+  it.only('test collection count with converter on query', () => {
     return withTestCollectionAndInitialData(testDocs, async collection => {
       const query_ = query(collection, where('key', '==', 'a')).withConverter(
         converter
       );
-
       const countQuery_ = countQuery(query_);
       const snapshot = await getAggregateFromServerDirect(countQuery_);
       expect(snapshot.getCount()).to.equal(2);
     });
   });
 
-  it('test collection count with converter on collection', () => {
-    //testing out the converter impact on the AggregateQuery type
+  it.only('aggregateQueryEqual returns true on same queries', () => {
     return withTestCollectionAndInitialData(testDocs, async collection => {
-      const ref = collection.withConverter(converter);
-      const query_ = query(ref, where('key', '==', 'a'));
-
-      const countQuery_ = countQuery(query_);
-      const snapshot = await getAggregateFromServerDirect(countQuery_);
-      expect(snapshot.getCount()).to.equal(2);
+      const query1 = query(collection, where('key', '==', 'a'));
+      const query2 = query(collection, where('key', '==', 'a'));
+      const countQuery1 = countQuery(query1);
+      const countQuery2 = countQuery(query2);
+      expect(aggregateQueryEqual(countQuery1, countQuery2)).to.be.true;
     });
   });
 
-  it('aggregateQueryEqual returns true on same queries', () => {
+  it.only('aggregateQueryEqual returns false on different queries', () => {
     return withTestCollectionAndInitialData(testDocs, async collection => {
-      const query_1 = query(collection, where('key', '==', 'a'));
-      const query_2 = query(collection, where('key', '==', 'a'));
-
-      const countQuery_1 = countQuery(query_1);
-      const countQuery_2 = countQuery(query_2);
-      expect(aggregateQueryEqual(countQuery_1, countQuery_2)).to.be.true;
+      const query1 = query(collection, where('key', '==', 'a'));
+      const query2 = query(collection, where('key', '!=', 'a'));
+      const countQuery1 = countQuery(query1);
+      const countQuery2 = countQuery(query2);
+      expect(aggregateQueryEqual(countQuery1, countQuery2)).to.be.false;
     });
   });
-  
-  it('aggregateQueryEqual returns false on different queries', () => {
-    return withTestCollectionAndInitialData(testDocs, async collection => {
-      const query_1 = query(collection, where('key', '==', 'a'));
-      const query_2 = query(collection, where('key', '!=', 'a'));
 
-      const countQuery_1 = countQuery(query_1);
-      const countQuery_2 = countQuery(query_2);
-      expect(aggregateQueryEqual(countQuery_1, countQuery_2)).to.be.false;
+  it.only('aggregateQuerySnapshotEqual returns true on same queries', () => {
+    return withTestCollectionAndInitialData(testDocs, async collection => {
+      const query1 = query(collection, where('key', '==', 'a'));
+      const query2 = query(collection, where('key', '==', 'a'));
+      const countQuery1A = countQuery(query1);
+      const countQuery1B = countQuery(query1);
+      const countQuery2 = countQuery(query2);
+      const snapshot1A = await getAggregateFromServerDirect(countQuery1A);
+      const snapshot1B = await getAggregateFromServerDirect(countQuery1B);
+      const snapshot2 = await getAggregateFromServerDirect(countQuery2);
+      expect(aggregateQuerySnapshotEqual(snapshot1A, snapshot1B)).to.be.true;
+      expect(aggregateQuerySnapshotEqual(snapshot1A, snapshot2)).to.be.true;
     });
   });
 
-  it('aggregateQuerySnapshotEqual returns true on same queries', () => {
+  it.only('aggregateQuerySnapshotEqual returns false on different queries', () => {
     return withTestCollectionAndInitialData(testDocs, async collection => {
-      const query_original = query(collection, where('key', '==', 'a'));
-      const query_copy = query(collection, where('key', '==', 'a'));
-
-      const countQuery_original_1 = countQuery(query_original);
-      const countQuery_original_2 = countQuery(query_original);
-      const countQuery_copy = countQuery(query_copy);
-
-      const snapshot_original_1 = await getAggregateFromServerDirect(countQuery_original_1);
-      const snapshot_original_2 = await getAggregateFromServerDirect(countQuery_original_2);
-      const snapshot_copy = await getAggregateFromServerDirect(countQuery_copy);
-
-      expect(aggregateQuerySnapshotEqual(snapshot_original_1, snapshot_original_2)).to.be.true;
-      expect(aggregateQuerySnapshotEqual(snapshot_original_1, snapshot_copy)).to.be.true;
+      const query1 = query(collection, where('key', '==', 'a'));
+      const query2 = query(collection, where('key', '==', 'b'));
+      const countQuery1 = countQuery(query1);
+      const countQuery2 = countQuery(query2);
+      const snapshot1 = await getAggregateFromServerDirect(countQuery1);
+      const snapshot2 = await getAggregateFromServerDirect(countQuery2);
+      expect(aggregateQuerySnapshotEqual(snapshot1, snapshot2)).to.be.false;
     });
   });
 });