Skip to content

Update invokeRun*QueryRpc functions to support paths with special characters #7402

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 15 commits into from
Dec 20, 2023
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/shaggy-garlics-leave.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@firebase/firestore": patch
---

Support special characters in query paths sent to `getCountFromServer(...)`, `getCount(...)` (lite API), and `getDocs(...)` (lite API).
2 changes: 1 addition & 1 deletion packages/firestore/src/local/local_serializer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
54 changes: 43 additions & 11 deletions packages/firestore/src/remote/datastore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,11 @@ import {
toMutation,
toName,
toQueryTarget,
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
Expand Down Expand Up @@ -94,7 +97,8 @@ class DatastoreImpl extends Datastore {
/** Invokes the provided RPC with auth and AppCheck tokens. */
invokeRPC<Req, Resp>(
rpcName: string,
path: string,
databaseId: DatabaseId,
resourcePath: ResourcePath,
request: Req
): Promise<Resp> {
this.verifyInitialized();
Expand All @@ -103,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<Req, Resp>(
rpcName,
path,
Expand All @@ -127,7 +135,8 @@ class DatastoreImpl extends Datastore {
/** Invokes the provided RPC with streamed results with auth and AppCheck tokens. */
invokeStreamingRPC<Req, Resp>(
rpcName: string,
path: string,
databaseId: DatabaseId,
resourcePath: ResourcePath,
request: Req,
expectedResponseCount?: number
): Promise<Resp[]> {
Expand All @@ -137,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<Req, Resp>(
rpcName,
path,
Expand Down Expand Up @@ -186,26 +199,35 @@ export async function invokeCommitRpc(
mutations: Mutation[]
): Promise<void> {
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(
datastore: Datastore,
keys: DocumentKey[]
): Promise<Document[]> {
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<string, Document>();
response.forEach(proto => {
Expand All @@ -226,11 +248,16 @@ export async function invokeRunQueryRpc(
query: Query
): Promise<Document[]> {
const datastoreImpl = debugCast(datastore, DatastoreImpl);
const request = toQueryTarget(datastoreImpl.serializer, queryToTarget(query));
const { queryTarget: request, parent } = toQueryTarget(
datastoreImpl.serializer,
queryToTarget(query)
);
const response = await datastoreImpl.invokeStreamingRPC<
ProtoRunQueryRequest,
ProtoRunQueryResponse
>('RunQuery', request.parent!, { structuredQuery: request.structuredQuery });
>('RunQuery', datastoreImpl.serializer.databaseId, parent, {
structuredQuery: request.structuredQuery
});
return (
response
// Omit RunQueryResponses that only contain readTimes.
Expand All @@ -247,20 +274,25 @@ export async function invokeRunAggregationQueryRpc(
aggregates: Aggregate[]
): Promise<ApiClientObjectMap<Value>> {
const datastoreImpl = debugCast(datastore, DatastoreImpl);
const { request, aliasMap } = toRunAggregationQueryRequest(
const { request, aliasMap, parent } = toRunAggregationQueryRequest(
datastoreImpl.serializer,
queryToAggregateTarget(query),
aggregates
);

const parent = request.parent;
if (!datastoreImpl.connection.shouldResourcePathBeIncludedInRequest) {
delete request.parent;
}
const response = await datastoreImpl.invokeStreamingRPC<
ProtoRunAggregationQueryRequest,
ProtoRunAggregationQueryResponse
>('RunAggregationQuery', parent!, request, /*expectedResponseCount=*/ 1);
>(
'RunAggregationQuery',
datastoreImpl.serializer.databaseId,
parent,
request,
/*expectedResponseCount=*/ 1
);

// Omit RunAggregationQueryResponse that only contain readTimes.
const filteredResult = response.filter(proto => !!proto.result);
Expand Down
47 changes: 28 additions & 19 deletions packages/firestore/src/remote/serializer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -285,10 +285,15 @@ export function toResourceName(
databaseId: DatabaseId,
path: ResourcePath
): string {
return fullyQualifiedPrefixPath(databaseId)
.child('documents')
.child(path)
.canonicalString();
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 {
Expand Down Expand Up @@ -837,17 +842,18 @@ 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;
let parent: ResourcePath;
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 = [
parent = path;
queryTarget.structuredQuery!.from = [
{
collectionId: target.collectionGroup,
allDescendants: true
Expand All @@ -858,33 +864,34 @@ export function toQueryTarget(
path.length % 2 !== 0,
'Document queries with filters are not supported.'
);
result.parent = toQueryPath(serializer, path.popLast());
result.structuredQuery!.from = [{ collectionId: path.lastSegment() }];
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;
return { queryTarget, parent };
}

export function toRunAggregationQueryRequest(
Expand All @@ -894,8 +901,9 @@ export function toRunAggregationQueryRequest(
): {
request: ProtoRunAggregationQueryRequest;
aliasMap: Record<string, string>;
parent: ResourcePath;
} {
const queryTarget = toQueryTarget(serializer, target);
const { queryTarget, parent } = toQueryTarget(serializer, target);
const aliasMap: Record<string, string> = {};

const aggregations: ProtoAggregation[] = [];
Expand Down Expand Up @@ -938,7 +946,8 @@ export function toRunAggregationQueryRequest(
},
parent: queryTarget.parent
},
aliasMap
aliasMap,
parent
};
}

Expand Down Expand Up @@ -1041,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;
Expand Down
35 changes: 34 additions & 1 deletion packages/firestore/test/integration/api/aggregation.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,9 @@ import {
writeBatch,
count,
sum,
average
average,
addDoc,
setLogLevel
} from '../util/firebase_export';
import {
apiDescribe,
Expand All @@ -55,6 +57,37 @@ 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);
});
});

it("count query doesn't use converter", () => {
const testDocs = {
a: { author: 'authorA', title: 'titleA' },
Expand Down
50 changes: 50 additions & 0 deletions packages/firestore/test/lite/integration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1125,6 +1125,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', () => {
Expand Down Expand Up @@ -2138,6 +2159,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);
Expand Down
Loading