Update invokeRun*QueryRpc functions to support paths with special characters.

Author: MarkDuckworth

packages/firestore/src/remote/datastore.ts

@@ -52,7 +52,8 @@ import {
   toMutation,
   toName,
   toQueryTarget,
-  toRunAggregationQueryRequest
+  toRunAggregationQueryRequest,
+  toQueryTargetPath
 } from './serializer';
 
 /**
@@ -225,11 +226,13 @@ export async function invokeRunQueryRpc(
   query: Query
 ): Promise<Document[]> {
   const datastoreImpl = debugCast(datastore, DatastoreImpl);
-  const request = toQueryTarget(datastoreImpl.serializer, queryToTarget(query));
+  const target = queryToTarget(query);
+  const request = toQueryTarget(datastoreImpl.serializer, target);
+  const encodedPath = toQueryTargetPath(datastoreImpl.serializer, target, true);
   const response = await datastoreImpl.invokeStreamingRPC<
     ProtoRunQueryRequest,
     ProtoRunQueryResponse
-  >('RunQuery', request.parent!, { structuredQuery: request.structuredQuery });
+  >('RunQuery', encodedPath, { structuredQuery: request.structuredQuery });
   return (
     response
       // Omit RunQueryResponses that only contain readTimes.
@@ -246,20 +249,21 @@ export async function invokeRunAggregationQueryRpc(
   aggregates: Aggregate[]
 ): Promise<ApiClientObjectMap<Value>> {
   const datastoreImpl = debugCast(datastore, DatastoreImpl);
+  const target = queryToTarget(query);
   const { request, aliasMap } = toRunAggregationQueryRequest(
     datastoreImpl.serializer,
-    queryToTarget(query),
+    target,
     aggregates
   );
+  const encodedPath = toQueryTargetPath(datastoreImpl.serializer, target, true);
 
-  const parent = request.parent;
   if (!datastoreImpl.connection.shouldResourcePathBeIncludedInRequest) {
     delete request.parent;
   }
   const response = await datastoreImpl.invokeStreamingRPC<
     ProtoRunAggregationQueryRequest,
     ProtoRunAggregationQueryResponse
-  >('RunAggregationQuery', parent!, request, /*expectedResponseCount=*/ 1);
+  >('RunAggregationQuery', encodedPath, request, /*expectedResponseCount=*/ 1);
 
   // Omit RunAggregationQueryResponse that only contain readTimes.
   const filteredResult = response.filter(proto => !!proto.result);

packages/firestore/src/remote/serializer.ts

@@ -283,12 +283,21 @@ export function fromVersion(version: ProtoTimestamp): SnapshotVersion {
 
 export function toResourceName(
   databaseId: DatabaseId,
-  path: ResourcePath
+  path: ResourcePath,
+  uriEncoded: boolean = false
 ): string {
-  return fullyQualifiedPrefixPath(databaseId)
+  const resourcePath = fullyQualifiedPrefixPath(databaseId)
     .child('documents')
-    .child(path)
-    .canonicalString();
+    .child(path);
+
+  if (uriEncoded) {
+    return resourcePath
+      .toArray()
+      .map(segment => encodeURIComponent(segment))
+      .join('/');
+  } else {
+    return resourcePath.toArray().join('/');
+  }
 }
 
 function fromResourceName(name: string): ResourcePath {
@@ -337,9 +346,10 @@ export function fromName(
 
 function toQueryPath(
   serializer: JsonProtoSerializer,
-  path: ResourcePath
+  path: ResourcePath,
+  urlEncoded: boolean = false
 ): string {
-  return toResourceName(serializer.databaseId, path);
+  return toResourceName(serializer.databaseId, path, urlEncoded);
 }
 
 function fromQueryPath(name: string): ResourcePath {
@@ -841,24 +851,16 @@ export function toQueryTarget(
   // Dissect the path into parent, collectionId, and optional key filter.
   const result: ProtoQueryTarget = { structuredQuery: {} };
   const path = target.path;
+  result.parent = toQueryTargetPath(serializer, target, false);
+
   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.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.structuredQuery!.from = [{ collectionId: path.lastSegment() }];
   }
 
@@ -887,6 +889,36 @@ export function toQueryTarget(
   return result;
 }
 
+function getQueryParentResourcePath(
+  serializer: JsonProtoSerializer,
+  target: Target
+): ResourcePath {
+  const path = target.path;
+
+  if (target.collectionGroup !== null) {
+    debugAssert(
+      path.length % 2 === 0,
+      'Collection Group queries should be within a document path or root.'
+    );
+    return path;
+  } else {
+    debugAssert(
+      path.length % 2 !== 0,
+      'Document queries with filters are not supported.'
+    );
+    return path.popLast();
+  }
+}
+
+export function toQueryTargetPath(
+  serializer: JsonProtoSerializer,
+  target: Target,
+  urlEncoded: boolean
+): string {
+  const parentResourcePath = getQueryParentResourcePath(serializer, target);
+  return toQueryPath(serializer, parentResourcePath, urlEncoded);
+}
+
 export function toRunAggregationQueryRequest(
   serializer: JsonProtoSerializer,
   target: Target,

packages/firestore/test/integration/api/aggregation.test.ts

@@ -32,7 +32,9 @@ import {
   writeBatch,
   count,
   sum,
-  average
+  average,
+  addDoc,
+  setLogLevel
 } from '../util/firebase_export';
 import {
   apiDescribe,
@@ -54,6 +56,37 @@ apiDescribe('Count queries', (persistence: boolean) => {
     });
   });
 
+  it('can run count query getCountFromServer with + in document name', () => {
+    setLogLevel('debug');
+    const testDocs = {
+      'a+1': { author: 'authorA' },
+      'b1': { author: 'authorB' },
+      'c1': { author: 'authorC' }
+    };
+    return withTestCollection(persistence, testDocs, async coll => {
+      const subColl1 = collection(coll, 'a+1/sub');
+      await addDoc(subColl1, { foo: 'bar' });
+      await addDoc(subColl1, { foo: 'baz' });
+
+      const subColl2 = collection(coll, 'b1/su+b');
+      await addDoc(subColl2, { foo: 'bar' });
+      await addDoc(subColl2, { foo: 'baz' });
+
+      const subColl3 = collection(coll, 'c1/sub');
+      await addDoc(subColl3, { foo: 'bar' });
+      await addDoc(subColl3, { foo: 'baz' });
+
+      const snapshot1 = await getCountFromServer(subColl1);
+      expect(snapshot1.data().count).to.equal(2);
+
+      const snapshot2 = await getCountFromServer(subColl2);
+      expect(snapshot2.data().count).to.equal(2);
+
+      const snapshot3 = await getCountFromServer(subColl3);
+      expect(snapshot3.data().count).to.equal(2);
+    });
+  });
+
   it("count query doesn't use converter", () => {
     const testDocs = {
       a: { author: 'authorA', title: 'titleA' },

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

@@ -1118,6 +1118,27 @@ describe('Query', () => {
       );
     });
   });
+
+  it('supports query over collection path with special characters', () => {
+    return withTestCollection(async collRef => {
+      const docWithSpecials = doc(collRef, 'so!@#$%^&*()_+special');
+      await setDoc(docWithSpecials, {});
+
+      const collectionWithSpecials = collection(
+        docWithSpecials,
+        'so!@#$%^&*()_+special'
+      );
+      await addDoc(collectionWithSpecials, { foo: 1 });
+      await addDoc(collectionWithSpecials, { foo: 2 });
+
+      const result = await getDocs(
+        query(collectionWithSpecials, orderBy('foo', 'asc'))
+      );
+
+      expect(result.size).to.equal(2);
+      verifyResults(result, { foo: 1 }, { foo: 2 });
+    });
+  });
 });
 
 describe('equality', () => {
@@ -2131,6 +2152,35 @@ describe('Count queries', () => {
     });
   });
 
+  it('can run count query getCountFromServer with + in document name', () => {
+    return withTestCollection(async coll => {
+      await setDoc(doc(coll, 'a+1'), {});
+      await setDoc(doc(coll, 'b1'), {});
+      await setDoc(doc(coll, 'c1'), {});
+
+      const subColl1 = collection(coll, 'a+1/sub');
+      await addDoc(subColl1, { foo: 'bar' });
+      await addDoc(subColl1, { foo: 'baz' });
+
+      const subColl2 = collection(coll, 'b1/su+b');
+      await addDoc(subColl2, { foo: 'bar' });
+      await addDoc(subColl2, { foo: 'baz' });
+
+      const subColl3 = collection(coll, 'c1/sub');
+      await addDoc(subColl3, { foo: 'bar' });
+      await addDoc(subColl3, { foo: 'baz' });
+
+      const snapshot1 = await getCount(subColl1);
+      expect(snapshot1.data().count).to.equal(2);
+
+      const snapshot2 = await getCount(subColl2);
+      expect(snapshot2.data().count).to.equal(2);
+
+      const snapshot3 = await getCount(subColl3);
+      expect(snapshot3.data().count).to.equal(2);
+    });
+  });
+
   it('run count query on empty collection', () => {
     return withTestCollection(async coll => {
       const snapshot = await getCount(coll);