-
Notifications
You must be signed in to change notification settings - Fork 409
Add executeQuery and executeMutation APIs to Data Connect #2979
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
base: master
Are you sure you want to change the base?
Conversation
Note:The integration tests have a huge diff because I moved the existing tests under a new parent |
Forgot a few changes! Didn't know that you could close and re-open :P thanks Yuchen! |
wait - need to privatize the execute API, execution should come from operation refs (in a future PR) |
|
||
/** The Firebase Data Connect backend connector URL format. */ | ||
const FIREBASE_DATA_CONNECT_CONNECTORS_URL_FORMAT = | ||
'https://autopush-firebasedataconnect.sandbox.googleapis.com/{version}/projects/{projectId}/locations/{locationId}/services/{serviceId}/connectors/{connectorId}:{endpointId}'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving a comment to block merging on these changes
this.connectorConfig.connector, | ||
); | ||
return this.makeGqlRequest<GraphqlResponse>(url, data) | ||
.then((resp) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get rid of this line? And can we replace this block with a try/catch?
}) | ||
const url = await this.getUrl(API_VERSION, this.connectorConfig.location, this.connectorConfig.serviceId, endpoint); | ||
return this.makeGqlRequest<GraphqlResponse>(url, data) | ||
.then((resp) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can get rid of this
options: GraphqlOptions<Variables> | ||
): Promise<ExecuteGraphqlResponse<GraphqlResponse>> { | ||
if ( | ||
typeof options.operationName === 'undefined' || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just as a note, this'll change when we move things like name
and options
out of the GraphqlOptions
field
}; | ||
let urlFormat: string; | ||
if (useEmulator()) { | ||
(urlParams as any).host = emulatorHost(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this more strongly typed? I'd rather we add an empty host field in urlParams
than cast to any
and lose type-safety
user_upsert(data: { id: "fred_id", address: "32 Elm St.", name: "Fred" }) | ||
} | ||
mutation updateFredrickUserImpersonation @auth(level: USER) { | ||
mutation updateFredrickUserImpersonation @auth(level: USER, insecureReason: "test") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: These changes likely require a re-deploy on the CI project
const authenticatedOptions: GraphqlOptions<unknown> = | ||
{ operationName: 'authenticatedQuery', impersonate: { authClaims: { sub: 'authenticated-UUID' } } }; | ||
|
||
it('should reject when no operationName is provided', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since these are all rejections, should they be nested in a separate reject
describe, like on line 308?
const stub = sandbox | ||
.stub(HttpClient.prototype, 'send') | ||
.resolves(utils.responseFrom(TEST_RESPONSE, 200)); | ||
return apiClient.executeQuery<UsersResponse, unknown>(unauthenticatedOptions).then((resp) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably await this
}); | ||
|
||
it('should use DATA_CONNECT_EMULATOR_HOST if set', () => { | ||
process.env.DATA_CONNECT_EMULATOR_HOST = 'localhost:9399'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we reset this after the test is complete?
}); | ||
|
||
it('should use DATA_CONNECT_EMULATOR_HOST if set', () => { | ||
process.env.DATA_CONNECT_EMULATOR_HOST = 'localhost:9399'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reset after?
…s, bypassing auth policies
…ions, bypassing auth policies
API Changes
executeQuery()
andexecuteMutation()
tosrc/data-connect/data-connect.ts
. These allow users to call deployed operations with impersonated auth credentials.Testing
executeGraphql*
APIsexecuteGraphql*
APIs