Skip to content

Embed metadata directly into the RPC call #979

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 7 commits into from
Jul 10, 2018
Merged

Embed metadata directly into the RPC call #979

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
98f7c0f
Embed metadata directly into the RPC call
ryanpbrewster Jul 3, 2018
c1fe2a1
[AUTOMATED]: Prettier Code Styling
ryanpbrewster Jul 3, 2018
6d7bf23
Use ...args
ryanpbrewster Jul 3, 2018
014696e
[AUTOMATED]: Prettier Code Styling
ryanpbrewster Jul 3, 2018
63957d2
Minimize diff
ryanpbrewster Jul 3, 2018
c6cddf5
Add the OAuth assertion back in
ryanpbrewster Jul 3, 2018
224ff71
Merge branch 'master' into rpb/direct-grpc-metadata
ryanpbrewster Jul 10, 2018
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
67 changes: 24 additions & 43 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 @@ -48,47 +48,6 @@ type UnaryRpc<Req, Resp> = (
callback: (err?: grpc.ServiceError, resp?: Resp) => void
) => grpc.ClientUnaryCall;

function createHeaders(databaseInfo: DatabaseInfo, token: Token | null): {} {
assert(
token === null || token.type === 'OAuth',
'If provided, token must be OAuth'
);

const channelCredentials = databaseInfo.ssl
? grpc.credentials.createSsl()
: grpc.credentials.createInsecure();

const callCredentials = grpc.credentials.createFromMetadataGenerator(
(
context: { service_url: string },
cb: (error: Error | null, metadata?: grpc.Metadata) => void
) => {
const metadata = new grpc.Metadata();
if (token) {
for (const header in token.authHeaders) {
if (token.authHeaders.hasOwnProperty(header)) {
metadata.set(header, token.authHeaders[header]);
}
}
}
metadata.set('x-goog-api-client', X_GOOG_API_CLIENT_VALUE);
// This header is used to improve routing and project isolation by the
// backend.
metadata.set(
'google-cloud-resource-prefix',
`projects/${databaseInfo.databaseId.projectId}/` +
`databases/${databaseInfo.databaseId.database}`
);
cb(null, metadata);
}
);

return grpc.credentials.combineChannelCredentials(
channelCredentials,
callCredentials
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool upside of this change: ssl: false now works with the Node SDK 😄

}

// The type of these stubs is dynamically generated by the GRPC runtime
// from the protocol buffer.
// tslint:disable-next-line:no-any
Expand Down Expand Up @@ -122,7 +81,9 @@ export class GrpcConnection implements Connection {
private ensureActiveStub(token: Token | null): GeneratedGrpcStub {
if (!this.cachedStub || !this.sameToken(this.cachedStub.token, token)) {
log.debug(LOG_TAG, 'Creating Firestore stub.');
const credentials = createHeaders(this.databaseInfo, token);
const credentials = this.databaseInfo.ssl
? grpc.credentials.createSsl()
: grpc.credentials.createInsecure();
this.cachedStub = {
stub: new this.firestore.Firestore(this.databaseInfo.host, credentials),
token
Expand All @@ -142,7 +103,27 @@ export class GrpcConnection implements Connection {
const stub = this.ensureActiveStub(token);
const rpc = stub[rpcMethod];
assert(rpc != null, 'Unknown RPC: ' + rpcName);
return rpc.bind(stub);

const metadata = new grpc.Metadata();
if (token) {
for (const header in token.authHeaders) {
if (token.authHeaders.hasOwnProperty(header)) {
metadata.set(header, token.authHeaders[header]);
}
}
}
metadata.set('x-goog-api-client', X_GOOG_API_CLIENT_VALUE);
// This header is used to improve routing and project isolation by the
// backend.
metadata.set(
'google-cloud-resource-prefix',
`projects/${this.databaseInfo.databaseId.projectId}/` +
`databases/${this.databaseInfo.databaseId.database}`
);
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, I think it's probably worth keeping this metadata creation in a helper. createMetadata(this.databaseInfo, token) or whatever.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

const f = rpc.bind(stub);
return function() {
return f(arguments, metadata);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The linter is complaining about this. Any ideas? I'm struggling with the fact that rpc.bind(stub) is a potentially 0-, 1-, or 2-ary function.

}

invokeRPC<Req, Resp>(
Expand Down