From 8fbfa83d0d749d163a462b2d25b14bcaf58742c0 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 29 Sep 2022 20:59:20 -0400 Subject: [PATCH 01/12] WIP docs update --- dev/src/reference.ts | 22 +++++++---- types/firestore.d.ts | 92 +++++++++++++++++++++++++++----------------- 2 files changed, 71 insertions(+), 43 deletions(-) diff --git a/dev/src/reference.ts b/dev/src/reference.ts index 1bd33822d..1470515dc 100644 --- a/dev/src/reference.ts +++ b/dev/src/reference.ts @@ -1599,11 +1599,21 @@ export class Query implements firestore.Query { } /** - * Returns an `AggregateQuery` that counts the number of documents in the - * result set. + * Returns a query that counts the documents in the result set of this + * query. + * + * The returned query, when executed, counts the documents in the result set + * of this query without actually downloading the documents. + * + * Using the returned query to count the documents is efficient because only + * the final count, not the documents' data, is downloaded. The returned + * query can even count the documents if the result set would be + * prohibitively large to download entirely (e.g. thousands of documents). * - * @return an `AggregateQuery` that counts the number of documents in the - * result set. + * @return a query that counts the documents in the result set of this + * query. The count can be retrieved from `snapshot.data().count`, where + * `snapshot` is the `AggregateQuerySnapshot` resulting from running the + * returned query. */ count(): AggregateQuery<{count: firestore.AggregateField}> { return this._aggregate({count: AggregateField.count()}); @@ -2860,10 +2870,6 @@ export class AggregateField implements firestore.AggregateField { static count(): AggregateField { return new AggregateField(); } - - isEqual(other: firestore.AggregateField): boolean { - return this === other || other instanceof AggregateField; - } } export class AggregateQuery diff --git a/types/firestore.d.ts b/types/firestore.d.ts index d84dcc054..442e50d52 100644 --- a/types/firestore.d.ts +++ b/types/firestore.d.ts @@ -1707,9 +1707,21 @@ declare namespace FirebaseFirestore { ): () => void; /** - * Returns count `AggregateQuery` based on this `Query`. + * Returns a query that counts the documents in the result set of this + * query. + * + * The returned query, when executed, counts the documents in the result set + * of this query without actually downloading the documents. + * + * Using the returned query to count the documents is efficient because only + * the final count, not the documents' data, is downloaded. The returned + * query can even count the documents if the result set would be + * prohibitively large to download entirely (e.g. thousands of documents). * - * @return AggregateQuery that contains count aggregate. + * @return a query that counts the documents in the result set of this + * query. The count can be retrieved from `snapshot.data().count`, where + * `snapshot` is the `AggregateQuerySnapshot` resulting from running the + * returned query. */ count(): AggregateQuery<{count: AggregateField}>; @@ -2034,94 +2046,104 @@ declare namespace FirebaseFirestore { } /** - * An `AggregateField`that captures input type T. + * Represents an aggregation that can be performed by Firestore. */ export class AggregateField { private constructor(); /** - * Creates and returns an aggregation field that counts the documents in the result set. - * @returns An `AggregateField` object with number input type. + * Returns an aggregation field that counts the documents in the result set + * of a query. */ static count(): AggregateField; - - /** - * Returns true if the aggregate function in this `AggregateField` is equal to the - * provided one. - * - * @param other The `AggregateField` to compare against. - * @return true if this `AggregateField` is equal to the provided one. - */ - isEqual(other: AggregateField): boolean; } /** - * The union of all `AggregateField` types that are returned from the factory - * functions. + * The union of all `AggregateField` types that are supported by Firestore. */ export type AggregateFieldType = ReturnType; /** - * A type whose values are all `AggregateField` objects. - * This is used as an argument to the "getter" functions, and the snapshot will - * map the same names to the corresponding values. + * A type whose property values are all `AggregateField` objects. */ export interface AggregateSpec { [field: string]: AggregateFieldType; } /** - * A type whose keys are taken from an `AggregateSpec` type, and whose values - * are the result of the aggregation performed by the corresponding + * A type whose keys are taken from an `AggregateSpec`, and whose values are + * the result of the aggregation performed by the corresponding * `AggregateField` from the input `AggregateSpec`. */ export type AggregateSpecData = { [P in keyof T]: T[P] extends AggregateField ? U : never; }; + /** + * A query that calculates aggregations over an underlying query. + * */ export class AggregateQuery { private constructor(); + /** The query whose aggregations will be calculated by this object. */ readonly query: Query; + /** + * Executes this query. + * + * @return A {promise that will be resolved with the results of the query. + */ get(): Promise>; /** - * Returns true if the query and aggregates in this `AggregateQuery` is equal to the - * provided one. + * Compares this object with the given object for equality. * - * @param other The `AggregateQuery` to compare against. - * @return true if this `AggregateQuery` is equal to the provided one. + * This object is considered "equal" to the other object if and only if + * `other` performs the same aggregations as this {@link AggregateQuery} and + * the underlying `Query` of `other` compares equal to that of this object. + * + * @param other The object to compare to this object for equality. + * @return `true` if this object is "equal" to the given object, as + * defined above, or `false` otherwise. */ isEqual(other: AggregateQuery): boolean; } /** - * An `AggregateQuerySnapshot` contains the results of running an aggregate query. + * The results of executing an aggregation query. */ export class AggregateQuerySnapshot { private constructor(); + /** The query that was executed to produce this result. */ readonly query: AggregateQuery; + /** The time this snapshot was read. */ readonly readTime: Timestamp; /** - * The results of the requested aggregations. The keys of the returned object - * will be the same as those of the `AggregateSpec` object specified to the - * aggregation method, and the values will be the corresponding aggregation - * result. + * Returns the results of the aggregations performed over the underlying + * query. * - * @returns The aggregation statistics result of running a query. + * The keys of the returned object will be the same as those of the + * `AggregateSpec` object specified to the aggregation method, and the + * values will be the corresponding aggregation result. + * + * @returns The results of the aggregations performed over the underlying + * query. */ data(): AggregateSpecData; /** - * Returns true if the query, read time, and data in this `AggregateQuerySnapshot` - * is equal to the provided one. + * Compares this object with the given object for equality. + * + * Two `AggregateQuerySnapshot` instances are considered "equal" if they + * have the read time, the same data, and underlying queries that compare + * "equal" using `AggregateQuery.isEqual()`. * - * @param other The `AggregateQuerySnapshot` to compare against. - * @return true if this `AggregateQuerySnapshot` is equal to the provided one. + * @param other The object to compare to this object for equality. + * @return `true` if this object is "equal" to the given object, as + * defined above, or `false` otherwise. */ isEqual(other: AggregateQuerySnapshot): boolean; } From fdc78f587d0f67fe27505b8a36a77ace457e4b34 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 29 Sep 2022 21:20:21 -0400 Subject: [PATCH 02/12] minor fixes --- types/firestore.d.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/types/firestore.d.ts b/types/firestore.d.ts index 442e50d52..45d0d7cea 100644 --- a/types/firestore.d.ts +++ b/types/firestore.d.ts @@ -21,7 +21,7 @@ // Declare a global (ambient) namespace // (used when not using import statement, but just script include). declare namespace FirebaseFirestore { - // Alias for `any` but used where a Firestore field value would be provided. + /** Alias for `any` but used where a Firestore field value would be provided. */ export type DocumentFieldValue = any; /** @@ -2081,7 +2081,7 @@ declare namespace FirebaseFirestore { /** * A query that calculates aggregations over an underlying query. - * */ + */ export class AggregateQuery { private constructor(); @@ -2091,7 +2091,7 @@ declare namespace FirebaseFirestore { /** * Executes this query. * - * @return A {promise that will be resolved with the results of the query. + * @return A promise that will be resolved with the results of the query. */ get(): Promise>; From e5ade52894cdb019b68eaa4a40fea3d0427f5fdb Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 29 Sep 2022 21:57:08 -0400 Subject: [PATCH 03/12] Rewrite AggregateQuerySnapshot.isEqual() to accommodate AggregateField.isEqual() being deleted --- dev/src/reference.ts | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/dev/src/reference.ts b/dev/src/reference.ts index 1470515dc..cb027d609 100644 --- a/dev/src/reference.ts +++ b/dev/src/reference.ts @@ -3061,23 +3061,28 @@ export class AggregateQuery if (this === other) { return true; } - if (other instanceof AggregateQuery) { - if (!this._query.isEqual(other._query)) { - return false; - } - - const thisAggregates: [string, AggregateField][] = - Object.entries(this._aggregates); - const otherAggregates = other._aggregates; + if (!(other instanceof AggregateQuery)) { + return false; + } + if (!this._query.isEqual(other._query)) { + return false; + } - return ( - thisAggregates.length === Object.keys(otherAggregates).length && - thisAggregates.every(([alias, field]) => - field.isEqual(otherAggregates[alias]) - ) - ); + // Verify that this object has the same aggregation keys as `other`. + const thisAggregateKeys = Object.keys(this._aggregates).sort(); + const otherAggregateKeys = Object.keys(other._aggregates).sort(); + if (thisAggregateKeys.length != otherAggregateKeys.length) { + return false; } - return false; + if (thisAggregateKeys.some((key, index) => key !== otherAggregateKeys[index])) { + return false; + } + + // TODO: Also compare the `AggregateField` instances from `this._aggregates` + // to their counterparts in `other._aggregates` once `AggregateField` gains + // an `isEqual()` method, which will happen once we support more than one + // type of `AggregateField` (currently, we only support "count"). + return true; } } From a7a860cd0579fe0f44cea57cbd276a70b727e1c2 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 29 Sep 2022 22:33:16 -0400 Subject: [PATCH 04/12] finish the docs update --- dev/src/reference.ts | 194 ++++++++++++++++++++++++++++--------------- types/firestore.d.ts | 10 +-- 2 files changed, 127 insertions(+), 77 deletions(-) diff --git a/dev/src/reference.ts b/dev/src/reference.ts index cb027d609..b85e9ceab 100644 --- a/dev/src/reference.ts +++ b/dev/src/reference.ts @@ -56,6 +56,7 @@ import { import {DocumentWatch, QueryWatch} from './watch'; import {validateDocumentData, WriteBatch, WriteResult} from './write-batch'; import api = protos.google.firestore.v1; +import {Primitive} from "@google-cloud/firestore"; /** * The direction of a `Query.orderBy()` clause is specified as 'desc' or 'asc' @@ -1616,19 +1617,7 @@ export class Query implements firestore.Query { * returned query. */ count(): AggregateQuery<{count: firestore.AggregateField}> { - return this._aggregate({count: AggregateField.count()}); - } - - /** - * Returns an `AggregateQuery` that performs the given aggregations. - * - * @param aggregates the aggregations to perform. - * @return an `AggregateQuery` that performs the given aggregations. - */ - private _aggregate( - aggregates: T - ): AggregateQuery { - return new AggregateQuery(this, aggregates); + return new AggregateQuery(this, {count: {}}); } /** @@ -2866,27 +2855,33 @@ export class CollectionReference export class AggregateField implements firestore.AggregateField { private constructor() {} - - static count(): AggregateField { - return new AggregateField(); - } } +/** + * A query that calculates aggregations over an underlying query. + */ export class AggregateQuery implements firestore.AggregateQuery { - private readonly _query: Query; - private readonly _aggregates: T; - - constructor(query: Query, aggregates: T) { - this._query = query; - this._aggregates = aggregates; + /** + * @private + * @internal + * + * @param _query The query whose aggregations will be calculated by this object. + * @param _aggregates The aggregations that will be performed by this query. + */ + constructor(_query: Query, private readonly _aggregates: T) { + this.query = _query; } - get query(): firestore.Query { - return this._query; - } + /** The query whose aggregations will be calculated by this object. */ + readonly query: Query; + /** + * Executes this query. + * + * @return A promise that will be resolved with the results of the query. + */ get(): Promise> { return this._get(); } @@ -2920,14 +2915,14 @@ export class AggregateQuery /** * Internal streaming method that accepts an optional transaction ID. * - * @param transactionId A transaction ID. * @private * @internal + * @param transactionId A transaction ID. * @returns A stream of document results. */ _stream(transactionId?: Uint8Array): Readable { const tag = requestTag(); - const firestore = this._query.firestore; + const firestore = this.query.firestore; const stream: Transform = new Transform({ objectMode: true, @@ -3003,17 +2998,15 @@ export class AggregateQuery /** * Internal method to decode values within result. - * - * @param proto * @private */ - private decodeResult( + private _decodeResult( proto: api.IAggregationResult ): firestore.AggregateSpecData { const data: any = {}; const fields = proto.aggregateFields; if (fields) { - const serializer = this._query.firestore._serializer!; + const serializer = this.query.firestore._serializer!; for (const prop of Object.keys(fields)) { if (this._aggregates[prop] === undefined) { throw new Error( @@ -3034,8 +3027,8 @@ export class AggregateQuery * @internal * @returns Serialized JSON for the query. */ - toProto(transactionId?: Uint8Array): api.IRunAggregationQueryRequest { - const queryProto = this._query.toProto(); + _toProto(transactionId?: Uint8Array): api.IRunAggregationQueryRequest { + const queryProto = this.query.toProto(); //TODO(tomandersen) inspect _query to build request - this is just hard coded count right now. const runQueryRequest: api.IRunAggregationQueryRequest = { parent: queryProto.parent, @@ -3057,6 +3050,17 @@ export class AggregateQuery return runQueryRequest; } + /** + * Compares this object with the given object for equality. + * + * This object is considered "equal" to the other object if and only if + * `other` performs the same aggregations as this `AggregateQuery` and + * the underlying `Query` of `other` compares equal to that of this object. + * + * @param other The object to compare to this object for equality. + * @return `true` if this object is "equal" to the given object, as + * defined above, or `false` otherwise. + */ isEqual(other: firestore.AggregateQuery): boolean { if (this === other) { return true; @@ -3064,17 +3068,14 @@ export class AggregateQuery if (!(other instanceof AggregateQuery)) { return false; } - if (!this._query.isEqual(other._query)) { + if (!this.query.isEqual(other.query)) { return false; } // Verify that this object has the same aggregation keys as `other`. const thisAggregateKeys = Object.keys(this._aggregates).sort(); const otherAggregateKeys = Object.keys(other._aggregates).sort(); - if (thisAggregateKeys.length != otherAggregateKeys.length) { - return false; - } - if (thisAggregateKeys.some((key, index) => key !== otherAggregateKeys[index])) { + if (!isPrimitiveArrayEqual(thisAggregateKeys, otherAggregateKeys)) { return false; } @@ -3086,52 +3087,84 @@ export class AggregateQuery } } +/** + * The results of executing an aggregation query. + */ export class AggregateQuerySnapshot implements firestore.AggregateQuerySnapshot { + /** + * @private + * @internal + * + * @param _query The query that was executed to produce this result. + * @param _readTime The time this snapshot was read. + * @param _data The results of the aggregations performed over the underlying + * query. + */ constructor( - private readonly _query: AggregateQuery, - private readonly _readTime: Timestamp, + _query: AggregateQuery, + _readTime: Timestamp, private readonly _data: firestore.AggregateSpecData - ) {} - - get query(): firestore.AggregateQuery { - return this._query; + ) { + this.query = _query; + this.readTime = _readTime; } - get readTime(): firestore.Timestamp { - return this._readTime; + /** The query that was executed to produce this result. */ + readonly query: AggregateQuery; + + /** The time this snapshot was read. */ + readonly readTime: firestore.Timestamp; + + /** + * Returns the results of the aggregations performed over the underlying + * query. + * + * The keys of the returned object will be the same as those of the + * `AggregateSpec` object specified to the aggregation method, and the + * values will be the corresponding aggregation result. + * + * @returns The results of the aggregations performed over the underlying + * query. + */ + data(): firestore.AggregateSpecData { + return this._data; } + /** + * Compares this object with the given object for equality. + * + * Two `AggregateQuerySnapshot` instances are considered "equal" if they + * have the read time, the same data, and underlying queries that compare + * "equal" using `AggregateQuery.isEqual()`. + * + * @param other The object to compare to this object for equality. + * @return `true` if this object is "equal" to the given object, as + * defined above, or `false` otherwise. + */ isEqual(other: firestore.AggregateQuerySnapshot): boolean { if (this === other) { return true; } - if (other instanceof AggregateQuerySnapshot) { - if (!this._query.isEqual(other._query)) { - return false; - } - - const thisData = this._data; - const thisDataKeys: string[] = Object.keys(thisData); - - const otherData = other._data; - const otherDataKeys: string[] = Object.keys(otherData); + if (!(other instanceof AggregateQuerySnapshot)) { + return false; + } + if (!this.readTime.isEqual(other.readTime)) { + return false; + } + if (!this.query.isEqual(other.query)) { + return false; + } - return ( - thisDataKeys.length === otherDataKeys.length && - thisDataKeys.every( - alias => - Object.prototype.hasOwnProperty.call(otherData, alias) && - thisData[alias] === otherData[alias] - ) - ); + // Verify that this object has the same aggregation keys as `other`. + const thisAggregateKeys = Object.keys(this._data).sort(); + const otherAggregateKeys = Object.keys(other._data).sort(); + if (!isPrimitiveArrayEqual(thisAggregateKeys, otherAggregateKeys)) { + return false; } - return false; - } - data(): firestore.AggregateSpecData { - return this._data; + return thisAggregateKeys.every(key => this._data[key] === other._data[key]); } } @@ -3266,6 +3299,29 @@ function isArrayEqual boolean}>( return true; } +/** + * Verifies equality for an array of primitives using the `===` operator. + * + * @private + * @internal + * @param left Array of primitives. + * @param right Array of primitives. + * @return True if arrays are equal. + */ +function isPrimitiveArrayEqual(left: T[], right: T[]): boolean { + if (left.length !== right.length) { + return false; + } + + for (let i = 0; i < left.length; ++i) { + if (left[i] !== right[i]) { + return false; + } + } + + return true; +} + /** * Returns the first non-undefined value or `undefined` if no such value exists. * @private diff --git a/types/firestore.d.ts b/types/firestore.d.ts index 45d0d7cea..5d1317b7d 100644 --- a/types/firestore.d.ts +++ b/types/firestore.d.ts @@ -2050,18 +2050,12 @@ declare namespace FirebaseFirestore { */ export class AggregateField { private constructor(); - - /** - * Returns an aggregation field that counts the documents in the result set - * of a query. - */ - static count(): AggregateField; } /** * The union of all `AggregateField` types that are supported by Firestore. */ - export type AggregateFieldType = ReturnType; + export type AggregateFieldType = AggregateField; /** * A type whose property values are all `AggregateField` objects. @@ -2099,7 +2093,7 @@ declare namespace FirebaseFirestore { * Compares this object with the given object for equality. * * This object is considered "equal" to the other object if and only if - * `other` performs the same aggregations as this {@link AggregateQuery} and + * `other` performs the same aggregations as this `AggregateQuery` and * the underlying `Query` of `other` compares equal to that of this object. * * @param other The object to compare to this object for equality. From a233d94ffe203855191fc98d8f1eccab6181c544 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 29 Sep 2022 22:51:43 -0400 Subject: [PATCH 05/12] lint cleanup --- dev/src/reference.ts | 53 ++++++++++++++++++++++++++------------------ types/firestore.d.ts | 1 + 2 files changed, 32 insertions(+), 22 deletions(-) diff --git a/dev/src/reference.ts b/dev/src/reference.ts index b85e9ceab..726bc6f52 100644 --- a/dev/src/reference.ts +++ b/dev/src/reference.ts @@ -56,7 +56,7 @@ import { import {DocumentWatch, QueryWatch} from './watch'; import {validateDocumentData, WriteBatch, WriteResult} from './write-batch'; import api = protos.google.firestore.v1; -import {Primitive} from "@google-cloud/firestore"; +import {Primitive} from '@google-cloud/firestore'; /** * The direction of a `Query.orderBy()` clause is specified as 'desc' or 'asc' @@ -2853,10 +2853,6 @@ export class CollectionReference } } -export class AggregateField implements firestore.AggregateField { - private constructor() {} -} - /** * A query that calculates aggregations over an underlying query. */ @@ -2867,15 +2863,22 @@ export class AggregateQuery * @private * @internal * - * @param _query The query whose aggregations will be calculated by this object. + * @param _query The query whose aggregations will be calculated by this + * object. * @param _aggregates The aggregations that will be performed by this query. */ - constructor(_query: Query, private readonly _aggregates: T) { - this.query = _query; - } + constructor( + // eslint-disable-next-line @typescript-eslint/no-explicit-any + private readonly _query: Query, + private readonly _aggregates: T + ) {} /** The query whose aggregations will be calculated by this object. */ - readonly query: Query; + + /** The query that was executed to produce this result. */ + get query(): Query { + return this._query; + } /** * Executes this query. @@ -3000,9 +3003,10 @@ export class AggregateQuery * Internal method to decode values within result. * @private */ - private _decodeResult( + private decodeResult( proto: api.IAggregationResult ): firestore.AggregateSpecData { + // eslint-disable-next-line @typescript-eslint/no-explicit-any const data: any = {}; const fields = proto.aggregateFields; if (fields) { @@ -3027,9 +3031,10 @@ export class AggregateQuery * @internal * @returns Serialized JSON for the query. */ - _toProto(transactionId?: Uint8Array): api.IRunAggregationQueryRequest { + toProto(transactionId?: Uint8Array): api.IRunAggregationQueryRequest { const queryProto = this.query.toProto(); - //TODO(tomandersen) inspect _query to build request - this is just hard coded count right now. + //TODO(tomandersen) inspect _query to build request - this is just hard + // coded count right now. const runQueryRequest: api.IRunAggregationQueryRequest = { parent: queryProto.parent, structuredAggregationQuery: { @@ -3103,19 +3108,20 @@ export class AggregateQuerySnapshot * query. */ constructor( - _query: AggregateQuery, - _readTime: Timestamp, + private readonly _query: AggregateQuery, + private readonly _readTime: Timestamp, private readonly _data: firestore.AggregateSpecData - ) { - this.query = _query; - this.readTime = _readTime; - } + ) {} /** The query that was executed to produce this result. */ - readonly query: AggregateQuery; + get query(): AggregateQuery { + return this._query; + } /** The time this snapshot was read. */ - readonly readTime: firestore.Timestamp; + get readTime(): Timestamp { + return this._readTime; + } /** * Returns the results of the aggregations performed over the underlying @@ -3308,7 +3314,10 @@ function isArrayEqual boolean}>( * @param right Array of primitives. * @return True if arrays are equal. */ -function isPrimitiveArrayEqual(left: T[], right: T[]): boolean { +function isPrimitiveArrayEqual( + left: T[], + right: T[] +): boolean { if (left.length !== right.length) { return false; } diff --git a/types/firestore.d.ts b/types/firestore.d.ts index 5d1317b7d..02f801186 100644 --- a/types/firestore.d.ts +++ b/types/firestore.d.ts @@ -2048,6 +2048,7 @@ declare namespace FirebaseFirestore { /** * Represents an aggregation that can be performed by Firestore. */ + // eslint-disable-next-line @typescript-eslint/no-unused-vars export class AggregateField { private constructor(); } From bd210d2507f592ba512de34168883b0f1a78446e Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 29 Sep 2022 23:10:38 -0400 Subject: [PATCH 06/12] Remove accidental import from @google-cloud/firestore --- dev/src/reference.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/dev/src/reference.ts b/dev/src/reference.ts index 726bc6f52..1d9090f47 100644 --- a/dev/src/reference.ts +++ b/dev/src/reference.ts @@ -56,7 +56,6 @@ import { import {DocumentWatch, QueryWatch} from './watch'; import {validateDocumentData, WriteBatch, WriteResult} from './write-batch'; import api = protos.google.firestore.v1; -import {Primitive} from '@google-cloud/firestore'; /** * The direction of a `Query.orderBy()` clause is specified as 'desc' or 'asc' @@ -3314,7 +3313,7 @@ function isArrayEqual boolean}>( * @param right Array of primitives. * @return True if arrays are equal. */ -function isPrimitiveArrayEqual( +function isPrimitiveArrayEqual( left: T[], right: T[] ): boolean { From 31fe957459f917f9b66921504882614f50eb8526 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 29 Sep 2022 23:32:24 -0400 Subject: [PATCH 07/12] reference.ts: formatting fix --- dev/src/reference.ts | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/dev/src/reference.ts b/dev/src/reference.ts index 1d9090f47..85ed688b8 100644 --- a/dev/src/reference.ts +++ b/dev/src/reference.ts @@ -2873,8 +2873,6 @@ export class AggregateQuery ) {} /** The query whose aggregations will be calculated by this object. */ - - /** The query that was executed to produce this result. */ get query(): Query { return this._query; } @@ -3313,10 +3311,7 @@ function isArrayEqual boolean}>( * @param right Array of primitives. * @return True if arrays are equal. */ -function isPrimitiveArrayEqual( - left: T[], - right: T[] -): boolean { +function isPrimitiveArrayEqual(left: T[], right: T[]): boolean { if (left.length !== right.length) { return false; } From ca36a444da1ec21e1aadf7fafc10ce3ceebc7441 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 30 Sep 2022 01:21:05 -0400 Subject: [PATCH 08/12] minor cleanup to AggregateQuerySnapshot.isEqual() --- dev/src/reference.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/dev/src/reference.ts b/dev/src/reference.ts index 85ed688b8..3d611b388 100644 --- a/dev/src/reference.ts +++ b/dev/src/reference.ts @@ -3167,7 +3167,12 @@ export class AggregateQuerySnapshot return false; } - return thisAggregateKeys.every(key => this._data[key] === other._data[key]); + // Verify that this object has the same aggregation results as `other`. + if (!thisAggregateKeys.every(key => this._data[key] === other._data[key])) { + return false; + } + + return true; } } From b6bdc1fb568ca07bf04725c0eed48f1b99328f27 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 30 Sep 2022 10:00:06 -0400 Subject: [PATCH 09/12] reference.ts: Fix AggregateQuerySnapshot.isEqual() to ignore readTime. --- dev/src/reference.ts | 10 +++++----- types/firestore.d.ts | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/dev/src/reference.ts b/dev/src/reference.ts index 3d611b388..d16b42aee 100644 --- a/dev/src/reference.ts +++ b/dev/src/reference.ts @@ -3139,8 +3139,8 @@ export class AggregateQuerySnapshot * Compares this object with the given object for equality. * * Two `AggregateQuerySnapshot` instances are considered "equal" if they - * have the read time, the same data, and underlying queries that compare - * "equal" using `AggregateQuery.isEqual()`. + * have the same data and their underlying queries compare "equal" using + * `AggregateQuery.isEqual()`. * * @param other The object to compare to this object for equality. * @return `true` if this object is "equal" to the given object, as @@ -3153,9 +3153,9 @@ export class AggregateQuerySnapshot if (!(other instanceof AggregateQuerySnapshot)) { return false; } - if (!this.readTime.isEqual(other.readTime)) { - return false; - } + // Since the read time is different on every read, we explicitly ignore all + // document metadata in this comparison, just like + // `DocumentSnapshot.isEqual()` does. if (!this.query.isEqual(other.query)) { return false; } diff --git a/types/firestore.d.ts b/types/firestore.d.ts index 02f801186..a7d7e6606 100644 --- a/types/firestore.d.ts +++ b/types/firestore.d.ts @@ -2133,8 +2133,8 @@ declare namespace FirebaseFirestore { * Compares this object with the given object for equality. * * Two `AggregateQuerySnapshot` instances are considered "equal" if they - * have the read time, the same data, and underlying queries that compare - * "equal" using `AggregateQuery.isEqual()`. + * have the same data and their underlying queries compare "equal" using + * `AggregateQuery.isEqual()`. * * @param other The object to compare to this object for equality. * @return `true` if this object is "equal" to the given object, as From 75b20911d1f3fb643866acce7c397e552f7f1174 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 30 Sep 2022 10:02:27 -0400 Subject: [PATCH 10/12] reference.ts: Use deepEqual() in the isEqual() methods of AggregateQuery and AggregateQuerySnapshot. --- dev/src/reference.ts | 54 ++++---------------------------------------- types/firestore.d.ts | 3 ++- 2 files changed, 6 insertions(+), 51 deletions(-) diff --git a/dev/src/reference.ts b/dev/src/reference.ts index d16b42aee..c6d9401ac 100644 --- a/dev/src/reference.ts +++ b/dev/src/reference.ts @@ -3057,7 +3057,8 @@ export class AggregateQuery * * This object is considered "equal" to the other object if and only if * `other` performs the same aggregations as this `AggregateQuery` and - * the underlying `Query` of `other` compares equal to that of this object. + * the underlying Query of `other` compares equal to that of this object + * using `Query.isEqual()`. * * @param other The object to compare to this object for equality. * @return `true` if this object is "equal" to the given object, as @@ -3073,19 +3074,7 @@ export class AggregateQuery if (!this.query.isEqual(other.query)) { return false; } - - // Verify that this object has the same aggregation keys as `other`. - const thisAggregateKeys = Object.keys(this._aggregates).sort(); - const otherAggregateKeys = Object.keys(other._aggregates).sort(); - if (!isPrimitiveArrayEqual(thisAggregateKeys, otherAggregateKeys)) { - return false; - } - - // TODO: Also compare the `AggregateField` instances from `this._aggregates` - // to their counterparts in `other._aggregates` once `AggregateField` gains - // an `isEqual()` method, which will happen once we support more than one - // type of `AggregateField` (currently, we only support "count"). - return true; + return deepEqual(this._aggregates, other._aggregates); } } @@ -3160,19 +3149,7 @@ export class AggregateQuerySnapshot return false; } - // Verify that this object has the same aggregation keys as `other`. - const thisAggregateKeys = Object.keys(this._data).sort(); - const otherAggregateKeys = Object.keys(other._data).sort(); - if (!isPrimitiveArrayEqual(thisAggregateKeys, otherAggregateKeys)) { - return false; - } - - // Verify that this object has the same aggregation results as `other`. - if (!thisAggregateKeys.every(key => this._data[key] === other._data[key])) { - return false; - } - - return true; + return deepEqual(this._data, other._data); } } @@ -3307,29 +3284,6 @@ function isArrayEqual boolean}>( return true; } -/** - * Verifies equality for an array of primitives using the `===` operator. - * - * @private - * @internal - * @param left Array of primitives. - * @param right Array of primitives. - * @return True if arrays are equal. - */ -function isPrimitiveArrayEqual(left: T[], right: T[]): boolean { - if (left.length !== right.length) { - return false; - } - - for (let i = 0; i < left.length; ++i) { - if (left[i] !== right[i]) { - return false; - } - } - - return true; -} - /** * Returns the first non-undefined value or `undefined` if no such value exists. * @private diff --git a/types/firestore.d.ts b/types/firestore.d.ts index a7d7e6606..2c5ceaf7c 100644 --- a/types/firestore.d.ts +++ b/types/firestore.d.ts @@ -2095,7 +2095,8 @@ declare namespace FirebaseFirestore { * * This object is considered "equal" to the other object if and only if * `other` performs the same aggregations as this `AggregateQuery` and - * the underlying `Query` of `other` compares equal to that of this object. + * the underlying Query of `other` compares equal to that of this object + * using `Query.isEqual()`. * * @param other The object to compare to this object for equality. * @return `true` if this object is "equal" to the given object, as From b06a978df56bb6745c211fb8a033aa7e91dad73f Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 30 Sep 2022 10:25:43 -0400 Subject: [PATCH 11/12] reference.ts: add to some return types --- dev/src/reference.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/dev/src/reference.ts b/dev/src/reference.ts index c6d9401ac..e4433c498 100644 --- a/dev/src/reference.ts +++ b/dev/src/reference.ts @@ -2873,7 +2873,7 @@ export class AggregateQuery ) {} /** The query whose aggregations will be calculated by this object. */ - get query(): Query { + get query(): firestore.Query { return this._query; } @@ -3100,12 +3100,12 @@ export class AggregateQuerySnapshot ) {} /** The query that was executed to produce this result. */ - get query(): AggregateQuery { + get query(): firestore.AggregateQuery { return this._query; } /** The time this snapshot was read. */ - get readTime(): Timestamp { + get readTime(): firestore.Timestamp { return this._readTime; } From feb85e7ada09aafbc48b4f165ba430f8ef22e10c Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 30 Sep 2022 10:30:08 -0400 Subject: [PATCH 12/12] fix build errors --- dev/src/reference.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/dev/src/reference.ts b/dev/src/reference.ts index e4433c498..b5cac033a 100644 --- a/dev/src/reference.ts +++ b/dev/src/reference.ts @@ -2922,7 +2922,7 @@ export class AggregateQuery */ _stream(transactionId?: Uint8Array): Readable { const tag = requestTag(); - const firestore = this.query.firestore; + const firestore = this._query.firestore; const stream: Transform = new Transform({ objectMode: true, @@ -3007,7 +3007,7 @@ export class AggregateQuery const data: any = {}; const fields = proto.aggregateFields; if (fields) { - const serializer = this.query.firestore._serializer!; + const serializer = this._query.firestore._serializer!; for (const prop of Object.keys(fields)) { if (this._aggregates[prop] === undefined) { throw new Error( @@ -3029,7 +3029,7 @@ export class AggregateQuery * @returns Serialized JSON for the query. */ toProto(transactionId?: Uint8Array): api.IRunAggregationQueryRequest { - const queryProto = this.query.toProto(); + const queryProto = this._query.toProto(); //TODO(tomandersen) inspect _query to build request - this is just hard // coded count right now. const runQueryRequest: api.IRunAggregationQueryRequest = {