From 1d76d33dd9f5ff9afc0d0ce1ba2f261eed3e7e8d Mon Sep 17 00:00:00 2001 From: Mark Duckworth <1124037+MarkDuckworth@users.noreply.github.com> Date: Wed, 28 Jun 2023 14:09:36 -0600 Subject: [PATCH 01/11] Update invokeRun*QueryRpc functions to support paths with special characters. --- packages/firestore/src/remote/datastore.ts | 16 +++-- packages/firestore/src/remote/serializer.ts | 64 ++++++++++++++----- .../test/integration/api/aggregation.test.ts | 35 +++++++++- .../firestore/test/lite/integration.test.ts | 50 +++++++++++++++ 4 files changed, 142 insertions(+), 23 deletions(-) diff --git a/packages/firestore/src/remote/datastore.ts b/packages/firestore/src/remote/datastore.ts index de26e2435b6..cbd38f68769 100644 --- a/packages/firestore/src/remote/datastore.ts +++ b/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 { 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> { 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); diff --git a/packages/firestore/src/remote/serializer.ts b/packages/firestore/src/remote/serializer.ts index 2a6910b10e6..aee104a77e1 100644 --- a/packages/firestore/src/remote/serializer.ts +++ b/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,12 +851,9 @@ 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, @@ -854,11 +861,6 @@ export function toQueryTarget( } ]; } 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, diff --git a/packages/firestore/test/integration/api/aggregation.test.ts b/packages/firestore/test/integration/api/aggregation.test.ts index d9d59799ee8..41dbfe56f6c 100644 --- a/packages/firestore/test/integration/api/aggregation.test.ts +++ b/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' }, diff --git a/packages/firestore/test/lite/integration.test.ts b/packages/firestore/test/lite/integration.test.ts index 0c8515f038c..8c21043e92b 100644 --- a/packages/firestore/test/lite/integration.test.ts +++ b/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); From 2a8559ec47edac97a428d12df0bfb35a8753c197 Mon Sep 17 00:00:00 2001 From: Mark Duckworth <1124037+MarkDuckworth@users.noreply.github.com> Date: Wed, 28 Jun 2023 14:18:55 -0600 Subject: [PATCH 02/11] Create shaggy-garlics-leave.md --- .changeset/shaggy-garlics-leave.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/shaggy-garlics-leave.md diff --git a/.changeset/shaggy-garlics-leave.md b/.changeset/shaggy-garlics-leave.md new file mode 100644 index 00000000000..b48961d7e6a --- /dev/null +++ b/.changeset/shaggy-garlics-leave.md @@ -0,0 +1,5 @@ +--- +"@firebase/firestore": patch +--- + +Support special characters in query paths sent to `getCountFromServer(...)`, `getCount(...)` (lite API), and `getDocs(...)` (lite API). From 77fac22b3c297863a40aa6911cd3e8f21faaa89b Mon Sep 17 00:00:00 2001 From: Mark Duckworth <1124037+MarkDuckworth@users.noreply.github.com> Date: Wed, 28 Jun 2023 14:58:36 -0600 Subject: [PATCH 03/11] Fixing typos. --- packages/firestore/src/remote/serializer.ts | 4 ++-- packages/firestore/test/lite/integration.test.ts | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/firestore/src/remote/serializer.ts b/packages/firestore/src/remote/serializer.ts index aee104a77e1..b6b548ea2b1 100644 --- a/packages/firestore/src/remote/serializer.ts +++ b/packages/firestore/src/remote/serializer.ts @@ -284,13 +284,13 @@ export function fromVersion(version: ProtoTimestamp): SnapshotVersion { export function toResourceName( databaseId: DatabaseId, path: ResourcePath, - uriEncoded: boolean = false + urlEncoded: boolean = false ): string { const resourcePath = fullyQualifiedPrefixPath(databaseId) .child('documents') .child(path); - if (uriEncoded) { + if (urlEncoded) { return resourcePath .toArray() .map(segment => encodeURIComponent(segment)) diff --git a/packages/firestore/test/lite/integration.test.ts b/packages/firestore/test/lite/integration.test.ts index 8c21043e92b..edcb00bd228 100644 --- a/packages/firestore/test/lite/integration.test.ts +++ b/packages/firestore/test/lite/integration.test.ts @@ -2152,7 +2152,7 @@ describe('Count queries', () => { }); }); - it('can run count query getCountFromServer with + in document name', () => { + it('can run count query getCount with + in document name', () => { return withTestCollection(async coll => { await setDoc(doc(coll, 'a+1'), {}); await setDoc(doc(coll, 'b1'), {}); From a1707eaefad577d3637670eb1181d71253cb5529 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 29 Jun 2023 16:15:33 -0400 Subject: [PATCH 04/11] Denver's fix --- .../firestore/src/local/local_serializer.ts | 2 +- packages/firestore/src/remote/datastore.ts | 62 ++++++++--- packages/firestore/src/remote/serializer.ts | 101 +++++++----------- .../firestore/test/lite/integration.test.ts | 2 +- .../test/unit/local/bundle_cache.test.ts | 17 ++- .../test/unit/remote/serializer.helper.ts | 40 +++++-- .../firestore/test/unit/util/bundle_data.ts | 5 +- packages/firestore/test/util/helpers.ts | 5 +- 8 files changed, 136 insertions(+), 98 deletions(-) diff --git a/packages/firestore/src/local/local_serializer.ts b/packages/firestore/src/local/local_serializer.ts index 3923e6777a3..e31f57ed9f2 100644 --- a/packages/firestore/src/local/local_serializer.ts +++ b/packages/firestore/src/local/local_serializer.ts @@ -285,7 +285,7 @@ export function toDbTarget( queryProto = toQueryTarget( localSerializer.remoteSerializer, targetData.target - ); + ).queryTarget; } // We can't store the resumeToken as a ByteString in IndexedDb, so we diff --git a/packages/firestore/src/remote/datastore.ts b/packages/firestore/src/remote/datastore.ts index cbd38f68769..ea3f80d6292 100644 --- a/packages/firestore/src/remote/datastore.ts +++ b/packages/firestore/src/remote/datastore.ts @@ -52,9 +52,11 @@ import { toMutation, toName, toQueryTarget, - toRunAggregationQueryRequest, - toQueryTargetPath + toResourcePath, + toRunAggregationQueryRequest } from './serializer'; +import { DatabaseId } from '../core/database_info'; +import { ResourcePath } from '../model/path'; /** * Datastore and its related methods are a wrapper around the external Google @@ -95,7 +97,8 @@ class DatastoreImpl extends Datastore { /** Invokes the provided RPC with auth and AppCheck tokens. */ invokeRPC( rpcName: string, - path: string, + databaseId: DatabaseId, + resourcePath: ResourcePath, request: Req ): Promise { this.verifyInitialized(); @@ -104,6 +107,10 @@ class DatastoreImpl extends Datastore { this.appCheckCredentials.getToken() ]) .then(([authToken, appCheckToken]) => { + const path = toResourcePath(databaseId, resourcePath) + .toArray() + .map(encodeURIComponent) + .join('/'); return this.connection.invokeRPC( rpcName, path, @@ -128,7 +135,8 @@ class DatastoreImpl extends Datastore { /** Invokes the provided RPC with streamed results with auth and AppCheck tokens. */ invokeStreamingRPC( rpcName: string, - path: string, + databaseId: DatabaseId, + resourcePath: ResourcePath, request: Req, expectedResponseCount?: number ): Promise { @@ -138,6 +146,10 @@ class DatastoreImpl extends Datastore { this.appCheckCredentials.getToken() ]) .then(([authToken, appCheckToken]) => { + const path = toResourcePath(databaseId, resourcePath) + .toArray() + .map(encodeURIComponent) + .join('/'); return this.connection.invokeStreamingRPC( rpcName, path, @@ -186,11 +198,15 @@ export async function invokeCommitRpc( mutations: Mutation[] ): Promise { const datastoreImpl = debugCast(datastore, DatastoreImpl); - const path = getEncodedDatabaseId(datastoreImpl.serializer) + '/documents'; const request = { writes: mutations.map(m => toMutation(datastoreImpl.serializer, m)) }; - await datastoreImpl.invokeRPC('Commit', path, request); + await datastoreImpl.invokeRPC( + 'Commit', + datastoreImpl.serializer.databaseId, + ResourcePath.emptyPath(), + request + ); } export async function invokeBatchGetDocumentsRpc( @@ -198,14 +214,19 @@ export async function invokeBatchGetDocumentsRpc( keys: DocumentKey[] ): Promise { const datastoreImpl = debugCast(datastore, DatastoreImpl); - const path = getEncodedDatabaseId(datastoreImpl.serializer) + '/documents'; const request = { documents: keys.map(k => toName(datastoreImpl.serializer, k)) }; const response = await datastoreImpl.invokeStreamingRPC< ProtoBatchGetDocumentsRequest, ProtoBatchGetDocumentsResponse - >('BatchGetDocuments', path, request, keys.length); + >( + 'BatchGetDocuments', + datastoreImpl.serializer.databaseId, + ResourcePath.emptyPath(), + request, + keys.length + ); const docs = new Map(); response.forEach(proto => { @@ -226,13 +247,16 @@ export async function invokeRunQueryRpc( query: Query ): Promise { const datastoreImpl = debugCast(datastore, DatastoreImpl); - const target = queryToTarget(query); - const request = toQueryTarget(datastoreImpl.serializer, target); - const encodedPath = toQueryTargetPath(datastoreImpl.serializer, target, true); + const { queryTarget: request, parent } = toQueryTarget( + datastoreImpl.serializer, + queryToTarget(query) + ); const response = await datastoreImpl.invokeStreamingRPC< ProtoRunQueryRequest, ProtoRunQueryResponse - >('RunQuery', encodedPath, { structuredQuery: request.structuredQuery }); + >('RunQuery', datastoreImpl.serializer.databaseId, parent, { + structuredQuery: request.structuredQuery + }); return ( response // Omit RunQueryResponses that only contain readTimes. @@ -249,13 +273,11 @@ export async function invokeRunAggregationQueryRpc( aggregates: Aggregate[] ): Promise> { const datastoreImpl = debugCast(datastore, DatastoreImpl); - const target = queryToTarget(query); - const { request, aliasMap } = toRunAggregationQueryRequest( + const { request, aliasMap, parent } = toRunAggregationQueryRequest( datastoreImpl.serializer, - target, + queryToTarget(query), aggregates ); - const encodedPath = toQueryTargetPath(datastoreImpl.serializer, target, true); if (!datastoreImpl.connection.shouldResourcePathBeIncludedInRequest) { delete request.parent; @@ -263,7 +285,13 @@ export async function invokeRunAggregationQueryRpc( const response = await datastoreImpl.invokeStreamingRPC< ProtoRunAggregationQueryRequest, ProtoRunAggregationQueryResponse - >('RunAggregationQuery', encodedPath, request, /*expectedResponseCount=*/ 1); + >( + 'RunAggregationQuery', + datastoreImpl.serializer.databaseId, + parent, + request, + /*expectedResponseCount=*/ 1 + ); // Omit RunAggregationQueryResponse that only contain readTimes. const filteredResult = response.filter(proto => !!proto.result); diff --git a/packages/firestore/src/remote/serializer.ts b/packages/firestore/src/remote/serializer.ts index b6b548ea2b1..175c3110f60 100644 --- a/packages/firestore/src/remote/serializer.ts +++ b/packages/firestore/src/remote/serializer.ts @@ -283,21 +283,17 @@ export function fromVersion(version: ProtoTimestamp): SnapshotVersion { export function toResourceName( databaseId: DatabaseId, - path: ResourcePath, - urlEncoded: boolean = false + path: ResourcePath ): string { - const resourcePath = fullyQualifiedPrefixPath(databaseId) - .child('documents') - .child(path); - - if (urlEncoded) { - return resourcePath - .toArray() - .map(segment => encodeURIComponent(segment)) - .join('/'); - } else { - return resourcePath.toArray().join('/'); - } + return toResourcePath(databaseId, path).canonicalString(); +} + +export function toResourcePath( + databaseId: DatabaseId, + path?: ResourcePath +): ResourcePath { + const resourcePath = fullyQualifiedPrefixPath(databaseId).child('documents'); + return path === undefined ? resourcePath : resourcePath.child(path); } function fromResourceName(name: string): ResourcePath { @@ -346,10 +342,9 @@ export function fromName( function toQueryPath( serializer: JsonProtoSerializer, - path: ResourcePath, - urlEncoded: boolean = false + path: ResourcePath ): string { - return toResourceName(serializer.databaseId, path, urlEncoded); + return toResourceName(serializer.databaseId, path); } function fromQueryPath(name: string): ResourcePath { @@ -847,76 +842,56 @@ export function fromDocumentsTarget( export function toQueryTarget( serializer: JsonProtoSerializer, target: Target -): ProtoQueryTarget { +): { queryTarget: ProtoQueryTarget; parent: ResourcePath } { // Dissect the path into parent, collectionId, and optional key filter. - const result: ProtoQueryTarget = { structuredQuery: {} }; + const queryTarget: ProtoQueryTarget = { structuredQuery: {} }; const path = target.path; - result.parent = toQueryTargetPath(serializer, target, false); - + let parent: ResourcePath; if (target.collectionGroup !== null) { - result.structuredQuery!.from = [ + debugAssert( + path.length % 2 === 0, + 'Collection Group queries should be within a document path or root.' + ); + parent = path; + queryTarget.structuredQuery!.from = [ { collectionId: target.collectionGroup, allDescendants: true } ]; } else { - result.structuredQuery!.from = [{ collectionId: path.lastSegment() }]; + debugAssert( + path.length % 2 !== 0, + 'Document queries with filters are not supported.' + ); + parent = path.popLast(); + queryTarget.structuredQuery!.from = [{ collectionId: path.lastSegment() }]; } + queryTarget.parent = toQueryPath(serializer, parent); const where = toFilters(target.filters); if (where) { - result.structuredQuery!.where = where; + queryTarget.structuredQuery!.where = where; } const orderBy = toOrder(target.orderBy); if (orderBy) { - result.structuredQuery!.orderBy = orderBy; + queryTarget.structuredQuery!.orderBy = orderBy; } const limit = toInt32Proto(serializer, target.limit); if (limit !== null) { - result.structuredQuery!.limit = limit; + queryTarget.structuredQuery!.limit = limit; } if (target.startAt) { - result.structuredQuery!.startAt = toStartAtCursor(target.startAt); + queryTarget.structuredQuery!.startAt = toStartAtCursor(target.startAt); } if (target.endAt) { - result.structuredQuery!.endAt = toEndAtCursor(target.endAt); + queryTarget.structuredQuery!.endAt = toEndAtCursor(target.endAt); } - 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); + return { queryTarget, parent }; } export function toRunAggregationQueryRequest( @@ -926,8 +901,9 @@ export function toRunAggregationQueryRequest( ): { request: ProtoRunAggregationQueryRequest; aliasMap: Record; + parent: ResourcePath; } { - const queryTarget = toQueryTarget(serializer, target); + const { queryTarget, parent } = toQueryTarget(serializer, target); const aliasMap: Record = {}; const aggregations: ProtoAggregation[] = []; @@ -970,7 +946,8 @@ export function toRunAggregationQueryRequest( }, parent: queryTarget.parent }, - aliasMap + aliasMap, + parent }; } @@ -1073,7 +1050,7 @@ export function toTarget( if (targetIsDocumentTarget(target)) { result = { documents: toDocumentsTarget(serializer, target) }; } else { - result = { query: toQueryTarget(serializer, target) }; + result = { query: toQueryTarget(serializer, target).queryTarget }; } result.targetId = targetData.targetId; diff --git a/packages/firestore/test/lite/integration.test.ts b/packages/firestore/test/lite/integration.test.ts index edcb00bd228..8c21043e92b 100644 --- a/packages/firestore/test/lite/integration.test.ts +++ b/packages/firestore/test/lite/integration.test.ts @@ -2152,7 +2152,7 @@ describe('Count queries', () => { }); }); - it('can run count query getCount with + in document name', () => { + 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'), {}); diff --git a/packages/firestore/test/unit/local/bundle_cache.test.ts b/packages/firestore/test/unit/local/bundle_cache.test.ts index 674e1144493..44a06114cb4 100644 --- a/packages/firestore/test/unit/local/bundle_cache.test.ts +++ b/packages/firestore/test/unit/local/bundle_cache.test.ts @@ -140,7 +140,10 @@ function genericBundleCacheTests(cacheFn: () => TestBundleCache): void { filter('sort', '>=', 2), orderBy('sort') ); - const queryTarget = toQueryTarget(JSON_SERIALIZER, queryToTarget(query1)); + const queryTarget = toQueryTarget( + JSON_SERIALIZER, + queryToTarget(query1) + ).queryTarget; await cache.setNamedQuery({ name: 'query-1', @@ -157,7 +160,10 @@ function genericBundleCacheTests(cacheFn: () => TestBundleCache): void { it('returns saved collection group queries', async () => { const query = newQueryForCollectionGroup('collection'); - const queryTarget = toQueryTarget(JSON_SERIALIZER, queryToTarget(query)); + const queryTarget = toQueryTarget( + JSON_SERIALIZER, + queryToTarget(query) + ).queryTarget; await cache.setNamedQuery({ name: 'query-1', @@ -179,7 +185,10 @@ function genericBundleCacheTests(cacheFn: () => TestBundleCache): void { 3, LimitType.First ); - const queryTarget = toQueryTarget(JSON_SERIALIZER, queryToTarget(query1)); + const queryTarget = toQueryTarget( + JSON_SERIALIZER, + queryToTarget(query1) + ).queryTarget; await cache.setNamedQuery({ name: 'query-1', @@ -209,7 +218,7 @@ function genericBundleCacheTests(cacheFn: () => TestBundleCache): void { const queryTarget = toQueryTarget( JSON_SERIALIZER, queryToTarget(limitQuery) - ); + ).queryTarget; await cache.setNamedQuery({ name: 'query-1', diff --git a/packages/firestore/test/unit/remote/serializer.helper.ts b/packages/firestore/test/unit/remote/serializer.helper.ts index 79140e73999..afe85796965 100644 --- a/packages/firestore/test/unit/remote/serializer.helper.ts +++ b/packages/firestore/test/unit/remote/serializer.helper.ts @@ -1311,7 +1311,9 @@ export function serializerTest( }, targetId: 1 }); - expect(fromQueryTarget(toQueryTarget(s, q))).to.deep.equal(q); + expect(fromQueryTarget(toQueryTarget(s, q).queryTarget)).to.deep.equal( + q + ); }); it('converts nested ancestor queries', () => { @@ -1333,7 +1335,9 @@ export function serializerTest( targetId: 1 }; expect(result).to.deep.equal(expected); - expect(fromQueryTarget(toQueryTarget(s, q))).to.deep.equal(q); + expect(fromQueryTarget(toQueryTarget(s, q).queryTarget)).to.deep.equal( + q + ); }); it('converts single filters at first-level collections', () => { @@ -1366,7 +1370,9 @@ export function serializerTest( targetId: 1 }; expect(result).to.deep.equal(expected); - expect(fromQueryTarget(toQueryTarget(s, q))).to.deep.equal(q); + expect(fromQueryTarget(toQueryTarget(s, q).queryTarget)).to.deep.equal( + q + ); }); it('converts multiple filters at first-level collections', () => { @@ -1441,7 +1447,9 @@ export function serializerTest( targetId: 1 }; expect(result).to.deep.equal(expected); - expect(fromQueryTarget(toQueryTarget(s, q))).to.deep.equal(q); + expect(fromQueryTarget(toQueryTarget(s, q).queryTarget)).to.deep.equal( + q + ); }); it('converts single filters on deeper collections', () => { @@ -1476,7 +1484,9 @@ export function serializerTest( targetId: 1 }; expect(result).to.deep.equal(expected); - expect(fromQueryTarget(toQueryTarget(s, q))).to.deep.equal(q); + expect(fromQueryTarget(toQueryTarget(s, q).queryTarget)).to.deep.equal( + q + ); }); it('converts multi-layer composite filters with OR at the first layer', () => { @@ -1562,7 +1572,9 @@ export function serializerTest( targetId: 1 }; expect(result).to.deep.equal(expected); - expect(fromQueryTarget(toQueryTarget(s, q))).to.deep.equal(q); + expect(fromQueryTarget(toQueryTarget(s, q).queryTarget)).to.deep.equal( + q + ); }); it('converts multi-layer composite filters with AND at the first layer', () => { @@ -1648,7 +1660,9 @@ export function serializerTest( targetId: 1 }; expect(result).to.deep.equal(expected); - expect(fromQueryTarget(toQueryTarget(s, q))).to.deep.equal(q); + expect(fromQueryTarget(toQueryTarget(s, q).queryTarget)).to.deep.equal( + q + ); }); it('converts order bys', () => { @@ -1674,7 +1688,9 @@ export function serializerTest( targetId: 1 }; expect(result).to.deep.equal(expected); - expect(fromQueryTarget(toQueryTarget(s, q))).to.deep.equal(q); + expect(fromQueryTarget(toQueryTarget(s, q).queryTarget)).to.deep.equal( + q + ); }); it('converts limits', () => { @@ -1699,7 +1715,9 @@ export function serializerTest( targetId: 1 }; expect(result).to.deep.equal(expected); - expect(fromQueryTarget(toQueryTarget(s, q))).to.deep.equal(q); + expect(fromQueryTarget(toQueryTarget(s, q).queryTarget)).to.deep.equal( + q + ); }); it('converts startAt/endAt', () => { @@ -1747,7 +1765,9 @@ export function serializerTest( targetId: 1 }; expect(result).to.deep.equal(expected); - expect(fromQueryTarget(toQueryTarget(s, q))).to.deep.equal(q); + expect(fromQueryTarget(toQueryTarget(s, q).queryTarget)).to.deep.equal( + q + ); }); it('converts resume tokens', () => { diff --git a/packages/firestore/test/unit/util/bundle_data.ts b/packages/firestore/test/unit/util/bundle_data.ts index 92cfb8c30cd..3901e802e6f 100644 --- a/packages/firestore/test/unit/util/bundle_data.ts +++ b/packages/firestore/test/unit/util/bundle_data.ts @@ -94,7 +94,10 @@ export class TestBundleBuilder { query = queryWithLimit(query, query.limit!, LimitType.First); bundledLimitType = 'LAST'; } - const queryTarget = toQueryTarget(this.serializer, queryToTarget(query)); + const queryTarget = toQueryTarget( + this.serializer, + queryToTarget(query) + ).queryTarget; this.elements.push({ namedQuery: { name, diff --git a/packages/firestore/test/util/helpers.ts b/packages/firestore/test/util/helpers.ts index 6cc59a5f779..0a6bef6ad48 100644 --- a/packages/firestore/test/util/helpers.ts +++ b/packages/firestore/test/util/helpers.ts @@ -576,10 +576,11 @@ export function namedQuery( name, readTime: toTimestamp(JSON_SERIALIZER, readTime.toTimestamp()), bundledQuery: { - parent: toQueryTarget(JSON_SERIALIZER, queryToTarget(query)).parent, + parent: toQueryTarget(JSON_SERIALIZER, queryToTarget(query)).queryTarget + .parent, limitType, structuredQuery: toQueryTarget(JSON_SERIALIZER, queryToTarget(query)) - .structuredQuery + .queryTarget.structuredQuery } }, matchingDocuments From cc839f48b29e94dea2b4f0384673fd0d955189e5 Mon Sep 17 00:00:00 2001 From: Mark Duckworth <1124037+MarkDuckworth@users.noreply.github.com> Date: Fri, 15 Dec 2023 15:30:35 -0700 Subject: [PATCH 05/11] PR feedback. --- packages/firestore/src/remote/datastore.ts | 9 ++-- .../test/integration/api/aggregation.test.ts | 50 +++++++------------ .../firestore/test/lite/integration.test.ts | 47 +++++++---------- 3 files changed, 39 insertions(+), 67 deletions(-) diff --git a/packages/firestore/src/remote/datastore.ts b/packages/firestore/src/remote/datastore.ts index 7d82073a5e0..47e5062cb46 100644 --- a/packages/firestore/src/remote/datastore.ts +++ b/packages/firestore/src/remote/datastore.ts @@ -18,10 +18,12 @@ import { CredentialsProvider } from '../api/credentials'; import { User } from '../auth/user'; import { Aggregate } from '../core/aggregate'; +import { DatabaseId } from '../core/database_info'; import { queryToAggregateTarget, Query, queryToTarget } from '../core/query'; import { Document } from '../model/document'; import { DocumentKey } from '../model/document_key'; import { Mutation } from '../model/mutation'; +import { ResourcePath } from '../model/path'; import { ApiClientObjectMap, BatchGetDocumentsRequest as ProtoBatchGetDocumentsRequest, @@ -47,7 +49,6 @@ import { import { fromDocument, fromBatchGetDocumentsResponse, - getEncodedDatabaseId, JsonProtoSerializer, toMutation, toName, @@ -55,8 +56,6 @@ import { toResourcePath, toRunAggregationQueryRequest } from './serializer'; -import { DatabaseId } from '../core/database_info'; -import { ResourcePath } from '../model/path'; /** * Datastore and its related methods are a wrapper around the external Google @@ -248,7 +247,7 @@ export async function invokeRunQueryRpc( query: Query ): Promise { const datastoreImpl = debugCast(datastore, DatastoreImpl); - const { queryTarget: request, parent } = toQueryTarget( + const { queryTarget, parent } = toQueryTarget( datastoreImpl.serializer, queryToTarget(query) ); @@ -256,7 +255,7 @@ export async function invokeRunQueryRpc( ProtoRunQueryRequest, ProtoRunQueryResponse >('RunQuery', datastoreImpl.serializer.databaseId, parent, { - structuredQuery: request.structuredQuery + structuredQuery: queryTarget.structuredQuery }); return ( response diff --git a/packages/firestore/test/integration/api/aggregation.test.ts b/packages/firestore/test/integration/api/aggregation.test.ts index 16435783e96..345f2bc2e91 100644 --- a/packages/firestore/test/integration/api/aggregation.test.ts +++ b/packages/firestore/test/integration/api/aggregation.test.ts @@ -34,8 +34,7 @@ import { count, sum, average, - addDoc, - setLogLevel + addDoc } from '../util/firebase_export'; import { apiDescribe, @@ -57,36 +56,23 @@ apiDescribe('Count queries', persistence => { }); }); - 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); - }); - }); + ['so!@#$%^&*()_+special/sub', 'b1/so!@#$%^&*()_+special'].forEach( + documentPath => { + it( + 'can run count query getCountFromServer with special chars in the document path: ' + + documentPath, + () => { + return withTestCollection(persistence, {}, async coll => { + const subColl1 = collection(coll, documentPath); + await addDoc(subColl1, { foo: 'bar' }); + await addDoc(subColl1, { foo: 'baz' }); + const snapshot1 = await getCountFromServer(subColl1); + expect(snapshot1.data().count).to.equal(2); + }); + } + ); + } + ); it("count query doesn't use converter", () => { const testDocs = { diff --git a/packages/firestore/test/lite/integration.test.ts b/packages/firestore/test/lite/integration.test.ts index a0d142923b2..f6d9184a676 100644 --- a/packages/firestore/test/lite/integration.test.ts +++ b/packages/firestore/test/lite/integration.test.ts @@ -1129,7 +1129,6 @@ 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, @@ -1142,7 +1141,6 @@ describe('Query', () => { query(collectionWithSpecials, orderBy('foo', 'asc')) ); - expect(result.size).to.equal(2); verifyResults(result, { foo: 1 }, { foo: 2 }); }); }); @@ -2159,34 +2157,23 @@ 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); - }); - }); + ['so!@#$%^&*()_+special/sub', 'b1/so!@#$%^&*()_+special'].forEach( + documentPath => { + it( + 'can run count query getCount with special chars in the document path: ' + + documentPath, + () => { + return withTestCollection(async coll => { + const subColl1 = collection(coll, documentPath); + await addDoc(subColl1, { foo: 'bar' }); + await addDoc(subColl1, { foo: 'baz' }); + const snapshot1 = await getCount(subColl1); + expect(snapshot1.data().count).to.equal(2); + }); + } + ); + } + ); it('run count query on empty collection', () => { return withTestCollection(async coll => { From fbe27c34f28ffbb11746d3e9d6618f35f205b2bf Mon Sep 17 00:00:00 2001 From: Mark Duckworth <1124037+MarkDuckworth@users.noreply.github.com> Date: Mon, 18 Dec 2023 11:22:55 -0700 Subject: [PATCH 06/11] Moving path encoding code from the Datastore implementation to the Connection implementation, because the encoding is Connection specific. --- .../firestore/src/platform/node/grpc_connection.ts | 4 ++-- packages/firestore/src/remote/connection.ts | 10 ++++++---- packages/firestore/src/remote/datastore.ts | 10 ++-------- packages/firestore/src/remote/rest_connection.ts | 6 +++--- 4 files changed, 13 insertions(+), 17 deletions(-) diff --git a/packages/firestore/src/platform/node/grpc_connection.ts b/packages/firestore/src/platform/node/grpc_connection.ts index 387a612b381..ba44f35923d 100644 --- a/packages/firestore/src/platform/node/grpc_connection.ts +++ b/packages/firestore/src/platform/node/grpc_connection.ts @@ -114,7 +114,7 @@ export class GrpcConnection implements Connection { invokeRPC( rpcName: string, - path: string, + path: string[], request: Req, authToken: Token | null, appCheckToken: Token | null @@ -166,7 +166,7 @@ export class GrpcConnection implements Connection { invokeStreamingRPC( rpcName: string, - path: string, + path: string[], request: Req, authToken: Token | null, appCheckToken: Token | null, diff --git a/packages/firestore/src/remote/connection.ts b/packages/firestore/src/remote/connection.ts index 34f458f1a1d..51fc8754d13 100644 --- a/packages/firestore/src/remote/connection.ts +++ b/packages/firestore/src/remote/connection.ts @@ -38,14 +38,15 @@ export interface Connection { * representing the JSON to send. * * @param rpcName - the name of the RPC to invoke - * @param path - the path to invoke this RPC on + * @param path - the path to invoke this RPC on. An array of path segments + * that will be encoded and joined with path separators when required. * @param request - the Raw JSON object encoding of the request message * @param token - the Token to use for the RPC. * @returns a Promise containing the JSON object encoding of the response */ invokeRPC( rpcName: string, - path: string, + path: string[], request: Req, authToken: Token | null, appCheckToken: Token | null @@ -57,7 +58,8 @@ export interface Connection { * completion and then returned as an array. * * @param rpcName - the name of the RPC to invoke - * @param path - the path to invoke this RPC on + * @param path - the path to invoke this RPC on. An array of path segments + * that will be encoded and joined with path separators when required. * @param request - the Raw JSON object encoding of the request message * @param token - the Token to use for the RPC. * @returns a Promise containing an array with the JSON object encodings of the @@ -65,7 +67,7 @@ export interface Connection { */ invokeStreamingRPC( rpcName: string, - path: string, + path: string[], request: Req, authToken: Token | null, appCheckToken: Token | null, diff --git a/packages/firestore/src/remote/datastore.ts b/packages/firestore/src/remote/datastore.ts index 47e5062cb46..b8de0192f09 100644 --- a/packages/firestore/src/remote/datastore.ts +++ b/packages/firestore/src/remote/datastore.ts @@ -106,10 +106,7 @@ class DatastoreImpl extends Datastore { this.appCheckCredentials.getToken() ]) .then(([authToken, appCheckToken]) => { - const path = toResourcePath(databaseId, resourcePath) - .toArray() - .map(encodeURIComponent) - .join('/'); + const path = toResourcePath(databaseId, resourcePath).toArray(); return this.connection.invokeRPC( rpcName, path, @@ -145,10 +142,7 @@ class DatastoreImpl extends Datastore { this.appCheckCredentials.getToken() ]) .then(([authToken, appCheckToken]) => { - const path = toResourcePath(databaseId, resourcePath) - .toArray() - .map(encodeURIComponent) - .join('/'); + const path = toResourcePath(databaseId, resourcePath).toArray(); return this.connection.invokeStreamingRPC( rpcName, path, diff --git a/packages/firestore/src/remote/rest_connection.ts b/packages/firestore/src/remote/rest_connection.ts index d76dd88d2fb..76b2846a923 100644 --- a/packages/firestore/src/remote/rest_connection.ts +++ b/packages/firestore/src/remote/rest_connection.ts @@ -82,13 +82,13 @@ export abstract class RestConnection implements Connection { invokeRPC( rpcName: string, - path: string, + path: string[], req: Req, authToken: Token | null, appCheckToken: Token | null ): Promise { const streamId = generateUniqueDebugId(); - const url = this.makeUrl(rpcName, path); + const url = this.makeUrl(rpcName, path.map(encodeURIComponent).join('/')); logDebug(LOG_TAG, `Sending RPC '${rpcName}' ${streamId}:`, url, req); const headers: StringMap = { @@ -119,7 +119,7 @@ export abstract class RestConnection implements Connection { invokeStreamingRPC( rpcName: string, - path: string, + path: string[], request: Req, authToken: Token | null, appCheckToken: Token | null, From d0f2e8719525ef31bdd10cc16c6950cecf92b941 Mon Sep 17 00:00:00 2001 From: Mark Duckworth <1124037+MarkDuckworth@users.noreply.github.com> Date: Mon, 18 Dec 2023 12:34:11 -0700 Subject: [PATCH 07/11] Unit test bug. --- .../firestore/test/unit/remote/rest_connection.test.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/firestore/test/unit/remote/rest_connection.test.ts b/packages/firestore/test/unit/remote/rest_connection.test.ts index 46c801ac010..703bc6915c5 100644 --- a/packages/firestore/test/unit/remote/rest_connection.test.ts +++ b/packages/firestore/test/unit/remote/rest_connection.test.ts @@ -73,7 +73,7 @@ describe('RestConnection', () => { it('url uses from path', async () => { await connection.invokeRPC( 'Commit', - 'projects/testproject/databases/(default)/documents', + 'projects/testproject/databases/(default)/documents'.split('/'), {}, null, null @@ -86,7 +86,7 @@ describe('RestConnection', () => { it('merges headers', async () => { await connection.invokeRPC( 'RunQuery', - 'projects/testproject/databases/(default)/documents/foo', + 'projects/testproject/databases/(default)/documents/foo'.split('/'), {}, new OAuthToken('owner', User.UNAUTHENTICATED), new AppCheckToken('some-app-check-token') @@ -105,7 +105,7 @@ describe('RestConnection', () => { it('empty app check token is not added to headers', async () => { await connection.invokeRPC( 'RunQuery', - 'projects/testproject/databases/(default)/documents/foo', + 'projects/testproject/databases/(default)/documents/foo'.split('/'), {}, null, new AppCheckToken('') @@ -124,7 +124,7 @@ describe('RestConnection', () => { connection.nextResponse = Promise.resolve({ response: true }); const response = await connection.invokeRPC( 'RunQuery', - 'projects/testproject/databases/(default)/documents/coll', + 'projects/testproject/databases/(default)/documents/coll'.split('/'), {}, null, null @@ -138,7 +138,7 @@ describe('RestConnection', () => { return expect( connection.invokeRPC( 'RunQuery', - 'projects/testproject/databases/(default)/documents/coll', + 'projects/testproject/databases/(default)/documents/coll'.split('/'), {}, null, null From ca6a28c23ef40c188cb2c68d4e02b767bf64246d Mon Sep 17 00:00:00 2001 From: Mark Duckworth <1124037+MarkDuckworth@users.noreply.github.com> Date: Mon, 18 Dec 2023 14:29:32 -0700 Subject: [PATCH 08/11] More fixes. --- packages/firestore/test/unit/remote/datastore.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/firestore/test/unit/remote/datastore.test.ts b/packages/firestore/test/unit/remote/datastore.test.ts index a64f0f8405f..c2d279282c7 100644 --- a/packages/firestore/test/unit/remote/datastore.test.ts +++ b/packages/firestore/test/unit/remote/datastore.test.ts @@ -49,7 +49,7 @@ describe('Datastore', () => { invokeRPC( rpcName: string, - path: string, + path: string[], request: Req, token: Token | null ): Promise { @@ -58,7 +58,7 @@ describe('Datastore', () => { invokeStreamingRPC( rpcName: string, - path: string, + path: string[], request: Req, token: Token | null ): Promise { From 608ff83089e41a51569766cd760c0628eb2a1f77 Mon Sep 17 00:00:00 2001 From: Mark Duckworth <1124037+MarkDuckworth@users.noreply.github.com> Date: Tue, 19 Dec 2023 17:53:37 -0700 Subject: [PATCH 09/11] Code cleanup: string[] replaced with ResourcePath. --- packages/firestore/src/model/path.ts | 9 +++++++++ packages/firestore/src/platform/node/grpc_connection.ts | 5 +++-- packages/firestore/src/remote/connection.ts | 5 +++-- packages/firestore/src/remote/rest_connection.ts | 3 ++- packages/firestore/test/unit/remote/datastore.test.ts | 5 +++-- 5 files changed, 20 insertions(+), 7 deletions(-) diff --git a/packages/firestore/src/model/path.ts b/packages/firestore/src/model/path.ts index ecdb9ed086c..4a4095ccfb1 100644 --- a/packages/firestore/src/model/path.ts +++ b/packages/firestore/src/model/path.ts @@ -215,6 +215,15 @@ export class ResourcePath extends BasePath { return this.canonicalString(); } + /** + * Returns a string representation of this path + * where each path segment has been encoded with + * `encodeURIComponent`. + */ + toUriEncodedString(): string { + return this.toArray().map(encodeURIComponent).join('/'); + } + /** * Creates a resource path from the given slash-delimited string. If multiple * arguments are provided, all components are combined. Leading and trailing diff --git a/packages/firestore/src/platform/node/grpc_connection.ts b/packages/firestore/src/platform/node/grpc_connection.ts index ba44f35923d..ab9e44792ca 100644 --- a/packages/firestore/src/platform/node/grpc_connection.ts +++ b/packages/firestore/src/platform/node/grpc_connection.ts @@ -22,6 +22,7 @@ import * as grpc from '@grpc/grpc-js'; import { Token } from '../../api/credentials'; import { DatabaseInfo } from '../../core/database_info'; import { SDK_VERSION } from '../../core/version'; +import { ResourcePath } from '../../model/path'; import { Connection, Stream } from '../../remote/connection'; import { mapCodeFromRpcCode } from '../../remote/rpc_error'; import { StreamBridge } from '../../remote/stream_bridge'; @@ -114,7 +115,7 @@ export class GrpcConnection implements Connection { invokeRPC( rpcName: string, - path: string[], + path: ResourcePath, request: Req, authToken: Token | null, appCheckToken: Token | null @@ -166,7 +167,7 @@ export class GrpcConnection implements Connection { invokeStreamingRPC( rpcName: string, - path: string[], + path: ResourcePath, request: Req, authToken: Token | null, appCheckToken: Token | null, diff --git a/packages/firestore/src/remote/connection.ts b/packages/firestore/src/remote/connection.ts index 51fc8754d13..b727bc63c17 100644 --- a/packages/firestore/src/remote/connection.ts +++ b/packages/firestore/src/remote/connection.ts @@ -16,6 +16,7 @@ */ import { Token } from '../api/credentials'; +import { ResourcePath } from '../model/path'; import { FirestoreError } from '../util/error'; /** @@ -46,7 +47,7 @@ export interface Connection { */ invokeRPC( rpcName: string, - path: string[], + path: ResourcePath, request: Req, authToken: Token | null, appCheckToken: Token | null @@ -67,7 +68,7 @@ export interface Connection { */ invokeStreamingRPC( rpcName: string, - path: string[], + path: ResourcePath, request: Req, authToken: Token | null, appCheckToken: Token | null, diff --git a/packages/firestore/src/remote/rest_connection.ts b/packages/firestore/src/remote/rest_connection.ts index 76b2846a923..0fd73aae638 100644 --- a/packages/firestore/src/remote/rest_connection.ts +++ b/packages/firestore/src/remote/rest_connection.ts @@ -22,6 +22,7 @@ import { DatabaseInfo, DEFAULT_DATABASE_NAME } from '../core/database_info'; +import { ResourcePath } from '../model/path'; import { debugAssert } from '../util/assert'; import { generateUniqueDebugId } from '../util/debug_uid'; import { FirestoreError } from '../util/error'; @@ -82,7 +83,7 @@ export abstract class RestConnection implements Connection { invokeRPC( rpcName: string, - path: string[], + path: ResourcePath, req: Req, authToken: Token | null, appCheckToken: Token | null diff --git a/packages/firestore/test/unit/remote/datastore.test.ts b/packages/firestore/test/unit/remote/datastore.test.ts index c2d279282c7..3f2a2da146a 100644 --- a/packages/firestore/test/unit/remote/datastore.test.ts +++ b/packages/firestore/test/unit/remote/datastore.test.ts @@ -24,6 +24,7 @@ import { Token } from '../../../src/api/credentials'; import { DatabaseId } from '../../../src/core/database_info'; +import { ResourcePath } from '../../../src/model/path'; import { Connection, Stream } from '../../../src/remote/connection'; import { Datastore, @@ -49,7 +50,7 @@ describe('Datastore', () => { invokeRPC( rpcName: string, - path: string[], + path: ResourcePath, request: Req, token: Token | null ): Promise { @@ -58,7 +59,7 @@ describe('Datastore', () => { invokeStreamingRPC( rpcName: string, - path: string[], + path: ResourcePath, request: Req, token: Token | null ): Promise { From a6c34608a772bb90763425035ca1de4b85f58b0d Mon Sep 17 00:00:00 2001 From: Mark Duckworth <1124037+MarkDuckworth@users.noreply.github.com> Date: Tue, 19 Dec 2023 17:55:57 -0700 Subject: [PATCH 10/11] Fix use of ResourcePath. --- packages/firestore/src/remote/datastore.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/packages/firestore/src/remote/datastore.ts b/packages/firestore/src/remote/datastore.ts index b8de0192f09..ac47f0cb931 100644 --- a/packages/firestore/src/remote/datastore.ts +++ b/packages/firestore/src/remote/datastore.ts @@ -106,10 +106,9 @@ class DatastoreImpl extends Datastore { this.appCheckCredentials.getToken() ]) .then(([authToken, appCheckToken]) => { - const path = toResourcePath(databaseId, resourcePath).toArray(); return this.connection.invokeRPC( rpcName, - path, + toResourcePath(databaseId, resourcePath), request, authToken, appCheckToken @@ -142,10 +141,9 @@ class DatastoreImpl extends Datastore { this.appCheckCredentials.getToken() ]) .then(([authToken, appCheckToken]) => { - const path = toResourcePath(databaseId, resourcePath).toArray(); return this.connection.invokeStreamingRPC( rpcName, - path, + toResourcePath(databaseId, resourcePath), request, authToken, appCheckToken, From 1ce5e4ca06e557d6e65f3d02a767c7d286fb621c Mon Sep 17 00:00:00 2001 From: Mark Duckworth <1124037+MarkDuckworth@users.noreply.github.com> Date: Tue, 19 Dec 2023 18:11:03 -0700 Subject: [PATCH 11/11] More build errors and cleanup. --- .../firestore/src/remote/rest_connection.ts | 4 ++-- .../test/unit/remote/rest_connection.test.ts | 21 ++++++++++++++----- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/packages/firestore/src/remote/rest_connection.ts b/packages/firestore/src/remote/rest_connection.ts index 0fd73aae638..470cb332ce2 100644 --- a/packages/firestore/src/remote/rest_connection.ts +++ b/packages/firestore/src/remote/rest_connection.ts @@ -89,7 +89,7 @@ export abstract class RestConnection implements Connection { appCheckToken: Token | null ): Promise { const streamId = generateUniqueDebugId(); - const url = this.makeUrl(rpcName, path.map(encodeURIComponent).join('/')); + const url = this.makeUrl(rpcName, path.toUriEncodedString()); logDebug(LOG_TAG, `Sending RPC '${rpcName}' ${streamId}:`, url, req); const headers: StringMap = { @@ -120,7 +120,7 @@ export abstract class RestConnection implements Connection { invokeStreamingRPC( rpcName: string, - path: string[], + path: ResourcePath, request: Req, authToken: Token | null, appCheckToken: Token | null, diff --git a/packages/firestore/test/unit/remote/rest_connection.test.ts b/packages/firestore/test/unit/remote/rest_connection.test.ts index 703bc6915c5..d45a75ce67b 100644 --- a/packages/firestore/test/unit/remote/rest_connection.test.ts +++ b/packages/firestore/test/unit/remote/rest_connection.test.ts @@ -21,6 +21,7 @@ import { AppCheckToken, OAuthToken, Token } from '../../../src/api/credentials'; import { User } from '../../../src/auth/user'; import { DatabaseId, DatabaseInfo } from '../../../src/core/database_info'; import { SDK_VERSION } from '../../../src/core/version'; +import { ResourcePath } from '../../../src/model/path'; import { Stream } from '../../../src/remote/connection'; import { RestConnection } from '../../../src/remote/rest_connection'; import { Code, FirestoreError } from '../../../src/util/error'; @@ -73,7 +74,9 @@ describe('RestConnection', () => { it('url uses from path', async () => { await connection.invokeRPC( 'Commit', - 'projects/testproject/databases/(default)/documents'.split('/'), + new ResourcePath( + 'projects/testproject/databases/(default)/documents'.split('/') + ), {}, null, null @@ -86,7 +89,9 @@ describe('RestConnection', () => { it('merges headers', async () => { await connection.invokeRPC( 'RunQuery', - 'projects/testproject/databases/(default)/documents/foo'.split('/'), + new ResourcePath( + 'projects/testproject/databases/(default)/documents/foo'.split('/') + ), {}, new OAuthToken('owner', User.UNAUTHENTICATED), new AppCheckToken('some-app-check-token') @@ -105,7 +110,9 @@ describe('RestConnection', () => { it('empty app check token is not added to headers', async () => { await connection.invokeRPC( 'RunQuery', - 'projects/testproject/databases/(default)/documents/foo'.split('/'), + new ResourcePath( + 'projects/testproject/databases/(default)/documents/foo'.split('/') + ), {}, null, new AppCheckToken('') @@ -124,7 +131,9 @@ describe('RestConnection', () => { connection.nextResponse = Promise.resolve({ response: true }); const response = await connection.invokeRPC( 'RunQuery', - 'projects/testproject/databases/(default)/documents/coll'.split('/'), + new ResourcePath( + 'projects/testproject/databases/(default)/documents/coll'.split('/') + ), {}, null, null @@ -138,7 +147,9 @@ describe('RestConnection', () => { return expect( connection.invokeRPC( 'RunQuery', - 'projects/testproject/databases/(default)/documents/coll'.split('/'), + new ResourcePath( + 'projects/testproject/databases/(default)/documents/coll'.split('/') + ), {}, null, null