diff --git a/.changeset/olive-goats-greet.md b/.changeset/olive-goats-greet.md new file mode 100644 index 00000000000..c7697e0bb17 --- /dev/null +++ b/.changeset/olive-goats-greet.md @@ -0,0 +1,5 @@ +--- +"@firebase/firestore": patch +--- + +Simplified the internal handling of aggregation results. diff --git a/packages/firestore/src/api/aggregate.ts b/packages/firestore/src/api/aggregate.ts index 3ced0c8b292..0b9dfd353e3 100644 --- a/packages/firestore/src/api/aggregate.ts +++ b/packages/firestore/src/api/aggregate.ts @@ -20,8 +20,7 @@ import { AggregateImpl } from '../core/aggregate'; import { firestoreClientRunAggregateQuery } from '../core/firestore_client'; import { count } from '../lite-api/aggregate'; import { AggregateQuerySnapshot } from '../lite-api/aggregate_types'; -import { AggregateAlias } from '../model/aggregate_alias'; -import { ObjectValue } from '../model/object_value'; +import { ApiClientObjectMap, Value } from '../protos/firestore_proto_api'; import { cast } from '../util/input_validation'; import { mapToArray } from '../util/obj'; @@ -110,7 +109,7 @@ export function getAggregateFromServer( const internalAggregates = mapToArray(aggregateSpec, (aggregate, alias) => { return new AggregateImpl( - new AggregateAlias(alias), + alias, aggregate._aggregateType, aggregate._internalFieldPath ); @@ -136,7 +135,7 @@ export function getAggregateFromServer( function convertToAggregateQuerySnapshot( firestore: Firestore, query: Query, - aggregateResult: ObjectValue + aggregateResult: ApiClientObjectMap ): AggregateQuerySnapshot { const userDataWriter = new ExpUserDataWriter(firestore); const querySnapshot = new AggregateQuerySnapshot( diff --git a/packages/firestore/src/core/aggregate.ts b/packages/firestore/src/core/aggregate.ts index 54e8c826547..42cdd0524ff 100644 --- a/packages/firestore/src/core/aggregate.ts +++ b/packages/firestore/src/core/aggregate.ts @@ -15,7 +15,6 @@ * limitations under the License. */ -import { AggregateAlias } from '../model/aggregate_alias'; import { FieldPath } from '../model/path'; /** @@ -29,7 +28,7 @@ export type AggregateType = 'count' | 'avg' | 'sum'; */ export interface Aggregate { readonly fieldPath?: FieldPath; - readonly alias: AggregateAlias; + readonly alias: string; readonly aggregateType: AggregateType; } @@ -38,7 +37,7 @@ export interface Aggregate { */ export class AggregateImpl implements Aggregate { constructor( - readonly alias: AggregateAlias, + readonly alias: string, readonly aggregateType: AggregateType, readonly fieldPath?: FieldPath ) {} diff --git a/packages/firestore/src/core/firestore_client.ts b/packages/firestore/src/core/firestore_client.ts index 280d1a2ac37..df6127f978e 100644 --- a/packages/firestore/src/core/firestore_client.ts +++ b/packages/firestore/src/core/firestore_client.ts @@ -36,10 +36,10 @@ import { Document } from '../model/document'; import { DocumentKey } from '../model/document_key'; import { FieldIndex } from '../model/field_index'; import { Mutation } from '../model/mutation'; -import { ObjectValue } from '../model/object_value'; import { toByteStreamReader } from '../platform/byte_stream_reader'; import { newSerializer } from '../platform/serializer'; import { newTextEncoder } from '../platform/text_serializer'; +import { ApiClientObjectMap, Value } from '../protos/firestore_proto_api'; import { Datastore, invokeRunAggregationQueryRpc } from '../remote/datastore'; import { RemoteStore, @@ -526,8 +526,8 @@ export function firestoreClientRunAggregateQuery( client: FirestoreClient, query: Query, aggregates: Aggregate[] -): Promise { - const deferred = new Deferred(); +): Promise> { + const deferred = new Deferred>(); client.asyncQueue.enqueueAndForget(async () => { // TODO (sum/avg) should we update this to use the event manager? diff --git a/packages/firestore/src/lite-api/aggregate.ts b/packages/firestore/src/lite-api/aggregate.ts index 0895d8f89c5..33e8012245e 100644 --- a/packages/firestore/src/lite-api/aggregate.ts +++ b/packages/firestore/src/lite-api/aggregate.ts @@ -18,8 +18,7 @@ import { deepEqual } from '@firebase/util'; import { AggregateImpl } from '../core/aggregate'; -import { AggregateAlias } from '../model/aggregate_alias'; -import { ObjectValue } from '../model/object_value'; +import { ApiClientObjectMap, Value } from '../protos/firestore_proto_api'; import { invokeRunAggregationQueryRpc } from '../remote/datastore'; import { cast } from '../util/input_validation'; import { mapToArray } from '../util/obj'; @@ -96,7 +95,7 @@ export function getAggregate( const internalAggregates = mapToArray(aggregateSpec, (aggregate, alias) => { return new AggregateImpl( - new AggregateAlias(alias), + alias, aggregate._aggregateType, aggregate._internalFieldPath ); @@ -115,7 +114,7 @@ export function getAggregate( function convertToAggregateQuerySnapshot( firestore: Firestore, query: Query, - aggregateResult: ObjectValue + aggregateResult: ApiClientObjectMap ): AggregateQuerySnapshot { const userDataWriter = new LiteUserDataWriter(firestore); const querySnapshot = new AggregateQuerySnapshot( diff --git a/packages/firestore/src/lite-api/aggregate_types.ts b/packages/firestore/src/lite-api/aggregate_types.ts index ab9619b58b4..9223b783138 100644 --- a/packages/firestore/src/lite-api/aggregate_types.ts +++ b/packages/firestore/src/lite-api/aggregate_types.ts @@ -16,8 +16,8 @@ */ import { AggregateType } from '../core/aggregate'; -import { ObjectValue } from '../model/object_value'; import { FieldPath as InternalFieldPath } from '../model/path'; +import { ApiClientObjectMap, Value } from '../protos/firestore_proto_api'; import { Query } from './reference'; import { AbstractUserDataWriter } from './user_data_writer'; @@ -85,7 +85,7 @@ export class AggregateQuerySnapshot { constructor( query: Query, private readonly _userDataWriter: AbstractUserDataWriter, - private readonly _data: ObjectValue + private readonly _data: ApiClientObjectMap ) { this.query = query; } @@ -102,8 +102,8 @@ export class AggregateQuerySnapshot { * query. */ data(): AggregateSpecData { - return this._userDataWriter.convertValue( - this._data.value + return this._userDataWriter.convertObjectMap( + this._data ) as AggregateSpecData; } } diff --git a/packages/firestore/src/lite-api/user_data_writer.ts b/packages/firestore/src/lite-api/user_data_writer.ts index 7f2e2b86301..5536b5c8b42 100644 --- a/packages/firestore/src/lite-api/user_data_writer.ts +++ b/packages/firestore/src/lite-api/user_data_writer.ts @@ -32,10 +32,12 @@ import { import { TypeOrder } from '../model/type_order'; import { typeOrder } from '../model/values'; import { + ApiClientObjectMap, ArrayValue as ProtoArrayValue, LatLng as ProtoLatLng, MapValue as ProtoMapValue, Timestamp as ProtoTimestamp, + Value, Value as ProtoValue } from '../protos/firestore_proto_api'; import { isValidResourceName } from '../remote/serializer'; @@ -91,9 +93,19 @@ export abstract class AbstractUserDataWriter { private convertObject( mapValue: ProtoMapValue, serverTimestampBehavior: ServerTimestampBehavior + ): DocumentData { + return this.convertObjectMap(mapValue.fields, serverTimestampBehavior); + } + + /** + * @internal + */ + convertObjectMap( + fields: ApiClientObjectMap | undefined, + serverTimestampBehavior: ServerTimestampBehavior = 'none' ): DocumentData { const result: DocumentData = {}; - forEach(mapValue.fields, (key, value) => { + forEach(fields, (key, value) => { result[key] = this.convertValue(value, serverTimestampBehavior); }); return result; diff --git a/packages/firestore/src/model/aggregate_alias.ts b/packages/firestore/src/model/aggregate_alias.ts deleted file mode 100644 index 2ac63fa9eb3..00000000000 --- a/packages/firestore/src/model/aggregate_alias.ts +++ /dev/null @@ -1,48 +0,0 @@ -/** - * @license - * Copyright 2022 Google LLC - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -const aliasRegExp = /^[_a-zA-Z][_a-zA-Z0-9]*(?:\.[_a-zA-Z][_a-zA-Z0-9]*)*$/; - -/** - * An alias for aggregation results. - * @internal - */ -export class AggregateAlias { - /** - * @internal - * @param alias Un-escaped alias representation - */ - constructor(private alias: string) {} - - /** - * Returns true if the string could be used as an alias. - */ - private static isValidAlias(value: string): boolean { - return aliasRegExp.test(value); - } - - /** - * Return an escaped and quoted string representation of the alias. - */ - canonicalString(): string { - let alias = this.alias.replace(/\\/g, '\\\\').replace(/`/g, '\\`'); - if (!AggregateAlias.isValidAlias(alias)) { - alias = '`' + alias + '`'; - } - return alias; - } -} diff --git a/packages/firestore/src/remote/datastore.ts b/packages/firestore/src/remote/datastore.ts index 7214d82c48e..de26e2435b6 100644 --- a/packages/firestore/src/remote/datastore.ts +++ b/packages/firestore/src/remote/datastore.ts @@ -22,18 +22,20 @@ import { Query, queryToTarget } from '../core/query'; import { Document } from '../model/document'; import { DocumentKey } from '../model/document_key'; import { Mutation } from '../model/mutation'; -import { ObjectValue } from '../model/object_value'; import { + ApiClientObjectMap, BatchGetDocumentsRequest as ProtoBatchGetDocumentsRequest, BatchGetDocumentsResponse as ProtoBatchGetDocumentsResponse, RunAggregationQueryRequest as ProtoRunAggregationQueryRequest, RunAggregationQueryResponse as ProtoRunAggregationQueryResponse, RunQueryRequest as ProtoRunQueryRequest, - RunQueryResponse as ProtoRunQueryResponse + RunQueryResponse as ProtoRunQueryResponse, + Value } from '../protos/firestore_proto_api'; import { debugAssert, debugCast, hardAssert } from '../util/assert'; import { AsyncQueue } from '../util/async_queue'; import { Code, FirestoreError } from '../util/error'; +import { isNullOrUndefined } from '../util/types'; import { Connection } from './connection'; import { @@ -50,8 +52,7 @@ import { toMutation, toName, toQueryTarget, - toRunAggregationQueryRequest, - fromAggregationResult + toRunAggregationQueryRequest } from './serializer'; /** @@ -243,9 +244,9 @@ export async function invokeRunAggregationQueryRpc( datastore: Datastore, query: Query, aggregates: Aggregate[] -): Promise { +): Promise> { const datastoreImpl = debugCast(datastore, DatastoreImpl); - const request = toRunAggregationQueryRequest( + const { request, aliasMap } = toRunAggregationQueryRequest( datastoreImpl.serializer, queryToTarget(query), aggregates @@ -267,8 +268,31 @@ export async function invokeRunAggregationQueryRpc( filteredResult.length === 1, 'Aggregation fields are missing from result.' ); + debugAssert( + !isNullOrUndefined(filteredResult[0].result), + 'aggregationQueryResponse.result' + ); + debugAssert( + !isNullOrUndefined(filteredResult[0].result.aggregateFields), + 'aggregationQueryResponse.result.aggregateFields' + ); + + // Remap the short-form aliases that were sent to the server + // to the client-side aliases. Users will access the results + // using the client-side alias. + const unmappedAggregateFields = filteredResult[0].result?.aggregateFields; + const remappedFields = Object.keys(unmappedAggregateFields).reduce< + ApiClientObjectMap + >((accumulator, key) => { + debugAssert( + !isNullOrUndefined(aliasMap[key]), + `'${key}' not present in aliasMap result` + ); + accumulator[aliasMap[key]] = unmappedAggregateFields[key]!; + return accumulator; + }, {}); - return fromAggregationResult(filteredResult[0]); + return remappedFields; } export function newPersistentWriteStream( diff --git a/packages/firestore/src/remote/serializer.ts b/packages/firestore/src/remote/serializer.ts index 1945ddbff4e..2a6910b10e6 100644 --- a/packages/firestore/src/remote/serializer.ts +++ b/packages/firestore/src/remote/serializer.ts @@ -81,7 +81,6 @@ import { Precondition as ProtoPrecondition, QueryTarget as ProtoQueryTarget, RunAggregationQueryRequest as ProtoRunAggregationQueryRequest, - RunAggregationQueryResponse as ProtoRunAggregationQueryResponse, Aggregation as ProtoAggregation, Status as ProtoStatus, Target as ProtoTarget, @@ -438,22 +437,6 @@ export function fromDocument( return hasCommittedMutations ? result.setHasCommittedMutations() : result; } -export function fromAggregationResult( - aggregationQueryResponse: ProtoRunAggregationQueryResponse -): ObjectValue { - assertPresent( - aggregationQueryResponse.result, - 'aggregationQueryResponse.result' - ); - assertPresent( - aggregationQueryResponse.result.aggregateFields, - 'aggregationQueryResponse.result.aggregateFields' - ); - return new ObjectValue({ - mapValue: { fields: aggregationQueryResponse.result?.aggregateFields } - }); -} - function fromFound( serializer: JsonProtoSerializer, doc: ProtoBatchGetDocumentsResponse @@ -908,26 +891,38 @@ export function toRunAggregationQueryRequest( serializer: JsonProtoSerializer, target: Target, aggregates: Aggregate[] -): ProtoRunAggregationQueryRequest { +): { + request: ProtoRunAggregationQueryRequest; + aliasMap: Record; +} { const queryTarget = toQueryTarget(serializer, target); + const aliasMap: Record = {}; const aggregations: ProtoAggregation[] = []; + let aggregationNum = 0; + aggregates.forEach(aggregate => { + // Map all client-side aliases to a unique short-form + // alias. This avoids issues with client-side aliases that + // exceed the 1500-byte string size limit. + const serverAlias = `aggregate_${aggregationNum++}`; + aliasMap[serverAlias] = aggregate.alias; + if (aggregate.aggregateType === 'count') { aggregations.push({ - alias: aggregate.alias.canonicalString(), + alias: serverAlias, count: {} }); } else if (aggregate.aggregateType === 'avg') { aggregations.push({ - alias: aggregate.alias.canonicalString(), + alias: serverAlias, avg: { field: toFieldPathReference(aggregate.fieldPath!) } }); } else if (aggregate.aggregateType === 'sum') { aggregations.push({ - alias: aggregate.alias.canonicalString(), + alias: serverAlias, sum: { field: toFieldPathReference(aggregate.fieldPath!) } @@ -936,11 +931,14 @@ export function toRunAggregationQueryRequest( }); return { - structuredAggregationQuery: { - aggregations, - structuredQuery: queryTarget.structuredQuery + request: { + structuredAggregationQuery: { + aggregations, + structuredQuery: queryTarget.structuredQuery + }, + parent: queryTarget.parent }, - parent: queryTarget.parent + aliasMap }; } diff --git a/packages/firestore/test/integration/api/aggregation.test.ts b/packages/firestore/test/integration/api/aggregation.test.ts index 98e2d017875..d9d59799ee8 100644 --- a/packages/firestore/test/integration/api/aggregation.test.ts +++ b/packages/firestore/test/integration/api/aggregation.test.ts @@ -218,6 +218,30 @@ apiDescribe('Aggregation queries', (persistence: boolean) => { }); }); + it('allows aliases with length greater than 1500 bytes', () => { + // Alias string length is bytes of UTF-8 encoded alias + 1; + let longAlias = ''; + for (let i = 0; i < 150; i++) { + longAlias += '0123456789'; + } + + const longerAlias = longAlias + longAlias; + + const testDocs = { + a: { num: 3 }, + b: { num: 5 } + }; + return withTestCollection(persistence, testDocs, async coll => { + const snapshot = await getAggregateFromServer(coll, { + [longAlias]: count(), + [longerAlias]: count() + }); + + expect(snapshot.data()[longAlias]).to.equal(2); + expect(snapshot.data()[longerAlias]).to.equal(2); + }); + }); + it('can get duplicate aggregations using getAggregationFromServer', () => { const testDocs = { a: { author: 'authorA', title: 'titleA' }, @@ -432,7 +456,7 @@ apiDescribe.skip( }); await expect(promise).to.eventually.be.rejectedWith( - /INVALID_ARGUMENT.*maximum number of aggregations/ + /maximum number of aggregations/ ); }); }); @@ -770,29 +794,32 @@ apiDescribe.skip( }); it('performs sum that overflows max int using getAggregationFromServer', () => { + // A large value that will be represented as a Long on the server, but + // doubling (2x) this value must overflow Long and force the result to be + // represented as a Double type on the server. + const maxLong = Math.pow(2, 63) - 1; + const testDocs = { a: { author: 'authorA', title: 'titleA', pages: 100, year: 1980, - rating: Number.MAX_SAFE_INTEGER + rating: maxLong }, b: { author: 'authorB', title: 'titleB', pages: 50, year: 2020, - rating: Number.MAX_SAFE_INTEGER + rating: maxLong } }; return withTestCollection(persistence, testDocs, async coll => { const snapshot = await getAggregateFromServer(coll, { totalRating: sum('rating') }); - expect(snapshot.data().totalRating).to.equal( - Number.MAX_SAFE_INTEGER + Number.MAX_SAFE_INTEGER - ); + expect(snapshot.data().totalRating).to.equal(maxLong + maxLong); }); }); @@ -1300,7 +1327,10 @@ apiDescribe.skip( const snapshot = await getAggregateFromServer(coll, { averageRating: average('rating') }); - expect(snapshot.data().averageRating).to.equal(9.2); + expect(snapshot.data().averageRating).to.be.approximately( + 9.2, + 0.0000001 + ); }); }); @@ -1372,7 +1402,7 @@ apiDescribe.skip( }); }); - it('performs average that could overflow IEEE754 during accumulation using getAggregationFromServer', () => { + it('performs average that overflows IEEE754 during accumulation using getAggregationFromServer', () => { const testDocs = { a: { author: 'authorA', @@ -1393,7 +1423,9 @@ apiDescribe.skip( const snapshot = await getAggregateFromServer(coll, { averageRating: average('rating') }); - expect(snapshot.data().averageRating).to.equal(Number.MAX_VALUE); + expect(snapshot.data().averageRating).to.equal( + Number.POSITIVE_INFINITY + ); }); });