Addressing PR feedback.

Author: MarkDuckworth

common/api-review/firestore-lite.api.md

@@ -43,7 +43,7 @@ export interface AggregateSpec {
 
 // @public
 export type AggregateSpecData<T extends AggregateSpec> = {
-    [P in keyof T as TrimBackticks<P>]: T[P] extends AggregateField<infer U> ? U : never;
+    [P in keyof T]: T[P] extends AggregateField<infer U> ? U : never;
 };
 
 // @public
@@ -401,9 +401,6 @@ export interface TransactionOptions {
     readonly maxAttempts?: number;
 }
 
-// @public
-export type TrimBackticks<T> = T extends `\`${infer Body}\`` ? Body : T;
-
 // @public
 export type UnionToIntersection<U> = (U extends unknown ? (k: U) => void : never) extends (k: infer I) => void ? I : never;
 

common/api-review/firestore.api.md

@@ -43,7 +43,7 @@ export interface AggregateSpec {
 
 // @public
 export type AggregateSpecData<T extends AggregateSpec> = {
-    [P in keyof T as TrimBackticks<P>]: T[P] extends AggregateField<infer U> ? U : never;
+    [P in keyof T]: T[P] extends AggregateField<infer U> ? U : never;
 };
 
 // @public
@@ -575,9 +575,6 @@ export interface TransactionOptions {
     readonly maxAttempts?: number;
 }
 
-// @public
-export type TrimBackticks<T> = T extends `\`${infer Body}\`` ? Body : T;
-
 // @public
 export type UnionToIntersection<U> = (U extends unknown ? (k: U) => void : never) extends (k: infer I) => void ? I : never;
 

packages/firestore/lite/index.ts

@@ -43,8 +43,7 @@ export {
   AggregateSpec,
   AggregateSpecData,
   AggregateQuerySnapshot,
-  AggregateType,
-  TrimBackticks
+  AggregateType
 } from '../src/lite-api/aggregate_types';
 
 export { FirestoreSettings as Settings } from '../src/lite-api/settings';

packages/firestore/src/api.ts

@@ -30,7 +30,6 @@ export {
   AggregateFieldType,
   AggregateSpec,
   AggregateSpecData,
-  TrimBackticks,
   AggregateQuerySnapshot,
   AggregateType
 } from './lite-api/aggregate_types';

packages/firestore/src/api/aggregate.ts

@@ -20,6 +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 { cast } from '../util/input_validation';
 import { mapToArray } from '../util/obj';
@@ -109,7 +110,7 @@ export function getAggregateFromServer<T extends AggregateSpec>(
 
   const internalAggregates = mapToArray(aggregateSpec, (aggregate, alias) => {
     return new AggregateImpl(
-      alias,
+      new AggregateAlias(alias),
       aggregate._aggregateType,
       aggregate._internalFieldPath
     );

packages/firestore/src/core/aggregate.ts

@@ -15,6 +15,7 @@
  * limitations under the License.
  */
 
+import { AggregateAlias } from '../model/aggregate_alias';
 import { FieldPath } from '../model/path';
 
 /**
@@ -28,7 +29,7 @@ export type AggregateType = 'count' | 'avg' | 'sum';
  */
 export interface Aggregate {
   readonly fieldPath?: FieldPath;
-  readonly alias: string;
+  readonly alias: AggregateAlias;
   readonly aggregateType: AggregateType;
 }
 
@@ -37,7 +38,7 @@ export interface Aggregate {
  */
 export class AggregateImpl implements Aggregate {
   constructor(
-    readonly alias: string,
+    readonly alias: AggregateAlias,
     readonly aggregateType: AggregateType,
     readonly fieldPath?: FieldPath
   ) {}

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

@@ -18,6 +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 { invokeRunAggregationQueryRpc } from '../remote/datastore';
 import { cast } from '../util/input_validation';
@@ -95,7 +96,7 @@ export function getAggregate<T extends AggregateSpec>(
 
   const internalAggregates = mapToArray(aggregateSpec, (aggregate, alias) => {
     return new AggregateImpl(
-      alias,
+      new AggregateAlias(alias),
       aggregate._aggregateType,
       aggregate._internalFieldPath
     );

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

@@ -65,18 +65,9 @@ export interface AggregateSpec {
  * from the input `AggregateSpec`.
  */
 export type AggregateSpecData<T extends AggregateSpec> = {
-  [P in keyof T as TrimBackticks<P>]: T[P] extends AggregateField<infer U>
-    ? U
-    : never;
+  [P in keyof T]: T[P] extends AggregateField<infer U> ? U : never;
 };
 
-/**
- * Removes enclosing backticks.
- * type Foo = '`Foo`'
- * type TrimmedFoo = TrimBackticks<Foo>; // 'Foo'
- */
-export type TrimBackticks<T> = T extends `\`${infer Body}\`` ? Body : T;
-
 /**
  * The results of executing an aggregation query.
  */

packages/firestore/src/model/aggregate_alias.ts

@@ -0,0 +1,48 @@
+/**
+ * @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;
+  }
+}

packages/firestore/src/remote/serializer.ts

@@ -915,19 +915,19 @@ export function toRunAggregationQueryRequest(
   aggregates.forEach(aggregate => {
     if (aggregate.aggregateType === 'count') {
       aggregations.push({
-        alias: aggregate.alias,
+        alias: aggregate.alias.canonicalString(),
         count: {}
       });
     } else if (aggregate.aggregateType === 'avg') {
       aggregations.push({
-        alias: aggregate.alias,
+        alias: aggregate.alias.canonicalString(),
         avg: {
           field: toFieldPathReference(aggregate.fieldPath!)
         }
       });
     } else if (aggregate.aggregateType === 'sum') {
       aggregations.push({
-        alias: aggregate.alias,
+        alias: aggregate.alias.canonicalString(),
         sum: {
           field: toFieldPathReference(aggregate.fieldPath!)
         }