From 6d902e3c68e7893566a84cdb57c03a4b675b8a9f Mon Sep 17 00:00:00 2001 From: Mark Duckworth <1124037+MarkDuckworth@users.noreply.github.com> Date: Thu, 20 Apr 2023 13:12:13 -0600 Subject: [PATCH 1/5] New test for long aliases --- .../test/integration/api/aggregation.test.ts | 26 ++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/packages/firestore/test/integration/api/aggregation.test.ts b/packages/firestore/test/integration/api/aggregation.test.ts index 95c5b2c24d1..ca6f3caa5cb 100644 --- a/packages/firestore/test/integration/api/aggregation.test.ts +++ b/packages/firestore/test/integration/api/aggregation.test.ts @@ -216,6 +216,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"; + } + + let 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]: sum("num") + }); + + expect(snapshot.data()[longAlias]).to.equal(2); + expect(snapshot.data()[longerAlias]).to.equal(8); + }); + }); + it('can get duplicate aggregations using getAggregationFromServer', () => { const testDocs = { a: { author: 'authorA', title: 'titleA' }, @@ -428,7 +452,7 @@ apiDescribe.skip( }); await expect(promise).to.eventually.be.rejectedWith( - /INVALID_ARGUMENT.*maximum number of aggregations/ + /maximum number of aggregations/ ); }); }); From 49779e5347126211ee5453c3456b9b8adae9965f Mon Sep 17 00:00:00 2001 From: Mark Duckworth <1124037+MarkDuckworth@users.noreply.github.com> Date: Thu, 20 Apr 2023 14:02:44 -0600 Subject: [PATCH 2/5] Implementing alias re-mapping to handle long alias values. --- packages/firestore/src/api/aggregate.ts | 3 +- packages/firestore/src/core/aggregate.ts | 5 +- packages/firestore/src/lite-api/aggregate.ts | 3 +- .../firestore/src/model/aggregate_alias.ts | 48 ------------------- packages/firestore/src/remote/datastore.ts | 30 ++++++++++-- packages/firestore/src/remote/serializer.ts | 33 +++++++++---- .../test/integration/api/aggregation.test.ts | 8 ++-- 7 files changed, 58 insertions(+), 72 deletions(-) delete mode 100644 packages/firestore/src/model/aggregate_alias.ts diff --git a/packages/firestore/src/api/aggregate.ts b/packages/firestore/src/api/aggregate.ts index ef3c83683f2..0b9dfd353e3 100644 --- a/packages/firestore/src/api/aggregate.ts +++ b/packages/firestore/src/api/aggregate.ts @@ -20,7 +20,6 @@ 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 { 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 ); 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/lite-api/aggregate.ts b/packages/firestore/src/lite-api/aggregate.ts index 3cfb4dac4f8..33e8012245e 100644 --- a/packages/firestore/src/lite-api/aggregate.ts +++ b/packages/firestore/src/lite-api/aggregate.ts @@ -18,7 +18,6 @@ import { deepEqual } from '@firebase/util'; import { AggregateImpl } from '../core/aggregate'; -import { AggregateAlias } from '../model/aggregate_alias'; import { ApiClientObjectMap, Value } from '../protos/firestore_proto_api'; import { invokeRunAggregationQueryRpc } from '../remote/datastore'; import { cast } from '../util/input_validation'; @@ -96,7 +95,7 @@ export function getAggregate( const internalAggregates = mapToArray(aggregateSpec, (aggregate, alias) => { return new AggregateImpl( - new AggregateAlias(alias), + alias, aggregate._aggregateType, aggregate._internalFieldPath ); 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 96ee8a52a65..6795e509b6a 100644 --- a/packages/firestore/src/remote/datastore.ts +++ b/packages/firestore/src/remote/datastore.ts @@ -245,20 +245,25 @@ export async function invokeRunAggregationQueryRpc( aggregates: Aggregate[] ): Promise> { const datastoreImpl = debugCast(datastore, DatastoreImpl); - const request = toRunAggregationQueryRequest( + const wrapper = toRunAggregationQueryRequest( datastoreImpl.serializer, queryToTarget(query), aggregates ); - const parent = request.parent; + const parent = wrapper.request.parent; if (!datastoreImpl.connection.shouldResourcePathBeIncludedInRequest) { - delete request.parent; + delete wrapper.request.parent; } const response = await datastoreImpl.invokeStreamingRPC< ProtoRunAggregationQueryRequest, ProtoRunAggregationQueryResponse - >('RunAggregationQuery', parent!, request, /*expectedResponseCount=*/ 1); + >( + 'RunAggregationQuery', + parent!, + wrapper.request, + /*expectedResponseCount=*/ 1 + ); // Omit RunAggregationQueryResponse that only contain readTimes. const filteredResult = response.filter(proto => !!proto.result); @@ -276,7 +281,22 @@ export async function invokeRunAggregationQueryRpc( 'aggregationQueryResponse.result.aggregateFields' ); - return filteredResult[0].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(wrapper.aliasMap[key]), + `'${key}' not present in aliasMap result` + ); + accumulator[wrapper.aliasMap[key]] = unmappedAggregateFields[key]!; + return accumulator; + }, {}); + + return remappedFields; } export function newPersistentWriteStream( diff --git a/packages/firestore/src/remote/serializer.ts b/packages/firestore/src/remote/serializer.ts index df3610ff59e..f726751bd49 100644 --- a/packages/firestore/src/remote/serializer.ts +++ b/packages/firestore/src/remote/serializer.ts @@ -887,30 +887,44 @@ export function toQueryTarget( return result; } +export interface RunAggregationQueryRequestWrapper { + request: ProtoRunAggregationQueryRequest; + aliasMap: Record; +} + export function toRunAggregationQueryRequest( serializer: JsonProtoSerializer, target: Target, aggregates: Aggregate[] -): ProtoRunAggregationQueryRequest { +): RunAggregationQueryRequestWrapper { 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!) } @@ -919,11 +933,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 ca6f3caa5cb..fee813ccead 100644 --- a/packages/firestore/test/integration/api/aggregation.test.ts +++ b/packages/firestore/test/integration/api/aggregation.test.ts @@ -218,12 +218,12 @@ 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 = ""; + let longAlias = ''; for (let i = 0; i < 150; i++) { - longAlias += "0123456789"; + longAlias += '0123456789'; } - let longerAlias = longAlias + longAlias; + const longerAlias = longAlias + longAlias; const testDocs = { a: { num: 3 }, @@ -232,7 +232,7 @@ apiDescribe('Aggregation queries', (persistence: boolean) => { return withTestCollection(persistence, testDocs, async coll => { const snapshot = await getAggregateFromServer(coll, { [longAlias]: count(), - [longerAlias]: sum("num") + [longerAlias]: sum('num') }); expect(snapshot.data()[longAlias]).to.equal(2); From 88fa4842b2f5c60b43851deba0b39a591f263750 Mon Sep 17 00:00:00 2001 From: Mark Duckworth <1124037+MarkDuckworth@users.noreply.github.com> Date: Fri, 21 Apr 2023 16:01:36 -0600 Subject: [PATCH 3/5] Address PR feedback. --- packages/firestore/src/remote/datastore.ts | 17 ++++++----------- packages/firestore/src/remote/serializer.ts | 10 ++++------ 2 files changed, 10 insertions(+), 17 deletions(-) diff --git a/packages/firestore/src/remote/datastore.ts b/packages/firestore/src/remote/datastore.ts index 6795e509b6a..97ada7aa273 100644 --- a/packages/firestore/src/remote/datastore.ts +++ b/packages/firestore/src/remote/datastore.ts @@ -245,25 +245,20 @@ export async function invokeRunAggregationQueryRpc( aggregates: Aggregate[] ): Promise> { const datastoreImpl = debugCast(datastore, DatastoreImpl); - const wrapper = toRunAggregationQueryRequest( + const { request, aliasMap } = toRunAggregationQueryRequest( datastoreImpl.serializer, queryToTarget(query), aggregates ); - const parent = wrapper.request.parent; + const parent = request.parent; if (!datastoreImpl.connection.shouldResourcePathBeIncludedInRequest) { - delete wrapper.request.parent; + delete request.parent; } const response = await datastoreImpl.invokeStreamingRPC< ProtoRunAggregationQueryRequest, ProtoRunAggregationQueryResponse - >( - 'RunAggregationQuery', - parent!, - wrapper.request, - /*expectedResponseCount=*/ 1 - ); + >('RunAggregationQuery', parent!, request, /*expectedResponseCount=*/ 1); // Omit RunAggregationQueryResponse that only contain readTimes. const filteredResult = response.filter(proto => !!proto.result); @@ -289,10 +284,10 @@ export async function invokeRunAggregationQueryRpc( ApiClientObjectMap >((accumulator, key) => { debugAssert( - !isNullOrUndefined(wrapper.aliasMap[key]), + !isNullOrUndefined(aliasMap[key]), `'${key}' not present in aliasMap result` ); - accumulator[wrapper.aliasMap[key]] = unmappedAggregateFields[key]!; + accumulator[aliasMap[key]] = unmappedAggregateFields[key]!; return accumulator; }, {}); diff --git a/packages/firestore/src/remote/serializer.ts b/packages/firestore/src/remote/serializer.ts index f726751bd49..835d25112a8 100644 --- a/packages/firestore/src/remote/serializer.ts +++ b/packages/firestore/src/remote/serializer.ts @@ -887,16 +887,14 @@ export function toQueryTarget( return result; } -export interface RunAggregationQueryRequestWrapper { - request: ProtoRunAggregationQueryRequest; - aliasMap: Record; -} - export function toRunAggregationQueryRequest( serializer: JsonProtoSerializer, target: Target, aggregates: Aggregate[] -): RunAggregationQueryRequestWrapper { +): { + request: ProtoRunAggregationQueryRequest; + aliasMap: Record; +} { const queryTarget = toQueryTarget(serializer, target); const aliasMap: Record = {}; From cc5da3bc7bad427d32cf788746f9727d6a1033c5 Mon Sep 17 00:00:00 2001 From: Mark Duckworth <1124037+MarkDuckworth@users.noreply.github.com> Date: Mon, 24 Apr 2023 10:45:26 -0600 Subject: [PATCH 4/5] Fix long alias test so that it does not use unsupported aggregation type. --- packages/firestore/test/integration/api/aggregation.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/firestore/test/integration/api/aggregation.test.ts b/packages/firestore/test/integration/api/aggregation.test.ts index 3341154722a..2eb46917fe4 100644 --- a/packages/firestore/test/integration/api/aggregation.test.ts +++ b/packages/firestore/test/integration/api/aggregation.test.ts @@ -234,7 +234,7 @@ apiDescribe('Aggregation queries', (persistence: boolean) => { return withTestCollection(persistence, testDocs, async coll => { const snapshot = await getAggregateFromServer(coll, { [longAlias]: count(), - [longerAlias]: sum('num') + [longerAlias]: count() }); expect(snapshot.data()[longAlias]).to.equal(2); From 03c253bab09bc02865a0897b83b7f9eba62af66b Mon Sep 17 00:00:00 2001 From: Mark Duckworth <1124037+MarkDuckworth@users.noreply.github.com> Date: Tue, 25 Apr 2023 09:16:01 -0600 Subject: [PATCH 5/5] Fix long alias test. --- packages/firestore/test/integration/api/aggregation.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/firestore/test/integration/api/aggregation.test.ts b/packages/firestore/test/integration/api/aggregation.test.ts index 2eb46917fe4..d9d59799ee8 100644 --- a/packages/firestore/test/integration/api/aggregation.test.ts +++ b/packages/firestore/test/integration/api/aggregation.test.ts @@ -238,7 +238,7 @@ apiDescribe('Aggregation queries', (persistence: boolean) => { }); expect(snapshot.data()[longAlias]).to.equal(2); - expect(snapshot.data()[longerAlias]).to.equal(8); + expect(snapshot.data()[longerAlias]).to.equal(2); }); });