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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
1d76d33
Update invokeRun*QueryRpc functions to support paths with special cha…
MarkDuckworth Jun 28, 2023
2a8559e
Create shaggy-garlics-leave.md
MarkDuckworth Jun 28, 2023
77fac22
Fixing typos.
MarkDuckworth Jun 28, 2023
b112a15
Merge branch 'master' of github.com:firebase/firebase-js-sdk into mar…
MarkDuckworth Jun 28, 2023
7128eec
Merge branch 'markduckworth/run-rpc-path-special-chars' of github.com…
MarkDuckworth Jun 28, 2023
48c09d5
Merge remote-tracking branch 'origin/master' into markduckworth/run-r…
dconeybe Jun 29, 2023
a1707ea
Denver's fix
dconeybe Jun 29, 2023
aa7346a
Merge branch 'master' of github.com:firebase/firebase-js-sdk into mar…
MarkDuckworth Dec 13, 2023
cc839f4
PR feedback.
MarkDuckworth Dec 15, 2023
fbe27c3
Moving path encoding code from the Datastore implementation to the Co…
MarkDuckworth Dec 18, 2023
d0f2e87
Unit test bug.
MarkDuckworth Dec 18, 2023
ca6a28c
More fixes.
MarkDuckworth Dec 18, 2023
608ff83
Code cleanup: string[] replaced with ResourcePath.
MarkDuckworth Dec 20, 2023
a6c3460
Fix use of ResourcePath.
MarkDuckworth Dec 20, 2023
1ce5e4c
More build errors and cleanup.
MarkDuckworth Dec 20, 2023
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
9 changes: 9 additions & 0 deletions packages/firestore/src/model/path.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,15 @@ export class ResourcePath extends BasePath<ResourcePath> {
return this.canonicalString();
}

/**
* Returns a string representation of this path
* where each path segment has been encoded with
* `encodeURIComponent`.
*/
toUriEncodedString(): string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, toUriEncodedString() shouldn't be on ResourcePath since it's an implementation detail specific to RestConnection, especially since it's only called once in the entire SDK. Consider moving this logic into RestConnection.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered that, but I felt more strongly that the structure of the path and how to URI encode the path should be the responsibility of the Path class. IMO, the connection shouldn't be responsible for knowing how to break up a path and encode its parts.

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
Expand Down
5 changes: 3 additions & 2 deletions packages/firestore/src/platform/node/grpc_connection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -114,7 +115,7 @@ export class GrpcConnection implements Connection {

invokeRPC<Req, Resp>(
rpcName: string,
path: string[],
path: ResourcePath,
request: Req,
authToken: Token | null,
appCheckToken: Token | null
Expand Down Expand Up @@ -166,7 +167,7 @@ export class GrpcConnection implements Connection {

invokeStreamingRPC<Req, Resp>(
rpcName: string,
path: string[],
path: ResourcePath,
request: Req,
authToken: Token | null,
appCheckToken: Token | null,
Expand Down
5 changes: 3 additions & 2 deletions packages/firestore/src/remote/connection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/

import { Token } from '../api/credentials';
import { ResourcePath } from '../model/path';
import { FirestoreError } from '../util/error';

/**
Expand Down Expand Up @@ -46,7 +47,7 @@ export interface Connection {
*/
invokeRPC<Req, Resp>(
rpcName: string,
path: string[],
path: ResourcePath,
request: Req,
authToken: Token | null,
appCheckToken: Token | null
Expand All @@ -67,7 +68,7 @@ export interface Connection {
*/
invokeStreamingRPC<Req, Resp>(
rpcName: string,
path: string[],
path: ResourcePath,
request: Req,
authToken: Token | null,
appCheckToken: Token | null,
Expand Down
6 changes: 2 additions & 4 deletions packages/firestore/src/remote/datastore.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,10 +106,9 @@ class DatastoreImpl extends Datastore {
this.appCheckCredentials.getToken()
])
.then(([authToken, appCheckToken]) => {
const path = toResourcePath(databaseId, resourcePath).toArray();
return this.connection.invokeRPC<Req, Resp>(
rpcName,
path,
toResourcePath(databaseId, resourcePath),
request,
authToken,
appCheckToken
Expand Down Expand Up @@ -142,10 +141,9 @@ class DatastoreImpl extends Datastore {
this.appCheckCredentials.getToken()
])
.then(([authToken, appCheckToken]) => {
const path = toResourcePath(databaseId, resourcePath).toArray();
return this.connection.invokeStreamingRPC<Req, Resp>(
rpcName,
path,
toResourcePath(databaseId, resourcePath),
request,
authToken,
appCheckToken,
Expand Down
7 changes: 4 additions & 3 deletions packages/firestore/src/remote/rest_connection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -82,13 +83,13 @@ export abstract class RestConnection implements Connection {

invokeRPC<Req, Resp>(
rpcName: string,
path: string[],
path: ResourcePath,
req: Req,
authToken: Token | null,
appCheckToken: Token | null
): Promise<Resp> {
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 = {
Expand Down Expand Up @@ -119,7 +120,7 @@ export abstract class RestConnection implements Connection {

invokeStreamingRPC<Req, Resp>(
rpcName: string,
path: string[],
path: ResourcePath,
request: Req,
authToken: Token | null,
appCheckToken: Token | null,
Expand Down
5 changes: 3 additions & 2 deletions packages/firestore/test/unit/remote/datastore.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -49,7 +50,7 @@ describe('Datastore', () => {

invokeRPC<Req, Resp>(
rpcName: string,
path: string[],
path: ResourcePath,
request: Req,
token: Token | null
): Promise<Resp> {
Expand All @@ -58,7 +59,7 @@ describe('Datastore', () => {

invokeStreamingRPC<Req, Resp>(
rpcName: string,
path: string[],
path: ResourcePath,
request: Req,
token: Token | null
): Promise<Resp[]> {
Expand Down
21 changes: 16 additions & 5 deletions packages/firestore/test/unit/remote/rest_connection.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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
Expand All @@ -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')
Expand All @@ -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('')
Expand All @@ -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
Expand All @@ -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
Expand Down