Initial FDC resolver support in functions SDK. #1784
Conversation
Summary of ChangesHello @rosalyntan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces foundational support for Firebase Data Connect (FDC) resolvers within the Firebase Functions SDK. It provides a new entry point, Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces initial support for Firebase Data Connect (FDC) resolvers via a new onGraphRequest function. This allows developers to create GraphQL endpoints. My review focuses on the implementation of onGraphRequest and its associated options. I've identified a critical issue with how optional dependencies are handled, which will cause runtime errors. I've also found some bugs related to handling server options, such as ignoring the customizable path and schema file path. Additionally, there are some areas for improvement in the options interface and a hardcoded value that needs clarification.
src/v2/providers/dataconnect.ts
Outdated
| if ( | ||
| (await import("@apollo/server").then((mod) => !!mod)) && | ||
| (await import("@as-integrations/express4").then((mod) => !!mod)) | ||
| ) { | ||
| const { ApolloServer } = await import("@apollo/server"); | ||
| const { expressMiddleware } = await import("@as-integrations/express4"); | ||
| const serverPromise = (async () => { | ||
| const app = express(); | ||
| const server = new ApolloServer({ | ||
| typeDefs: opts.schema, | ||
| resolvers: { Query: opts.resolvers.query, Mutation: opts.resolvers.mutation }, | ||
| }); | ||
|
|
||
| await server.start(); | ||
|
|
||
| app.use("/graphql", cors(), json(), expressMiddleware(server)); | ||
| return app; | ||
| })(); | ||
| // Exports a function running a GraphQL server at the URL | ||
| // https://functionName-<PROJECT_NUMBER>.<REGION>.run.app/graphql | ||
| return onRequest(async (req, res) => { | ||
| const app = await serverPromise; | ||
| app(req, res); | ||
| }); | ||
| } else { | ||
| // Handle case where optional-package is not available | ||
| console.warn("Optional feature requires 'optional-package'."); | ||
| } |
There was a problem hiding this comment.
The current implementation for checking optional dependencies is flawed and will cause a runtime error.
- The function
onGraphRequestis declared to return aPromise<HttpsFunction>, but theelseblock (lines 412-415) does not return a value. This is a type violation that will lead to issues. - The condition
(await import("...").then((mod) => !!mod))will not work as expected. If a package is not installed,import()will throw an error, and the.then()callback will not be executed. Theelseblock is effectively unreachable in the case of a missing dependency.
A more robust approach is to use a try...catch block to handle the dynamic imports. If the imports fail, you should throw an informative error to the developer.
For example:
try {
const { ApolloServer } = await import("@apollo/server");
const { expressMiddleware } = await import("@as-integrations/express4");
// ... server setup
return onRequest(...);
} catch (e) {
throw new Error(
"Optional dependencies '@apollo/server' and '@as-integrations/express4' are required for 'onGraphRequest'. Please install them to use this feature."
);
}This ensures that the function either successfully returns an HttpsFunction or clearly communicates the missing dependency issue to the developer.
|
|
||
| await server.start(); | ||
|
|
||
| app.use("/graphql", cors(), json(), expressMiddleware(server)); |
There was a problem hiding this comment.
The GraphQL server path is hardcoded to "/graphql". The GraphqlServerOptions interface defines a path property for this purpose, but it is not being used. This prevents developers from customizing the endpoint path as the options suggest.
app.use(opts.path || "/graphql", cors(), json(), expressMiddleware(server));| export interface GraphqlServerOptions { | ||
| /** | ||
| * A valid SDL string that represents the GraphQL server's schema. | ||
| * Either `schema` or `schemaFilePath` is required. | ||
| */ | ||
| schema: string; | ||
| /** | ||
| * A file path to a valid GraphQL schema. | ||
| * Either `schema` or `schemaFilePath` is required. | ||
| */ | ||
| schemaFilePath: string; | ||
| /** | ||
| * The path where the GraphQL server will be served on the Cloud Run function. | ||
| * If no path is provided, the server will be served at `/graphql`. | ||
| */ | ||
| path: string; | ||
| /** A map of functions that populate data for individual GraphQL schema fields. */ | ||
| resolvers: GraphqlResolvers; | ||
| } |
There was a problem hiding this comment.
The GraphqlServerOptions interface has a few issues:
schemaandschemaFilePathare both marked as required (schema: string), but the documentation states that only one is required. They should be optional (schema?: string).- The implementation of
onGraphRequestonly usesopts.schemaand does not handleschemaFilePath. If a developer providesschemaFilePath, it will be ignored, and the server will likely fail ifschemais not also provided. Logic to read the file content fromschemaFilePathis missing. pathis marked as required, but the documentation says it defaults to"/graphql", which implies it should be optional.
I recommend updating the interface and implementation to reflect the documented behavior.
export interface GraphqlServerOptions {
/**
* A valid SDL string that represents the GraphQL server's schema.
* Either `schema` or `schemaFilePath` is required.
*/
schema?: string;
/**
* A file path to a valid GraphQL schema.
* Either `schema` or `schemaFilePath` is required.
*/
schemaFilePath?: string;
/**
* The path where the GraphQL server will be served on the Cloud Run function.
* If no path is provided, the server will be served at `/graphql`.
*/
path?: string;
/** A map of functions that populate data for individual GraphQL schema fields. */
resolvers: GraphqlResolvers;
}| */ | ||
| export async function onGraphRequest(opts: GraphqlServerOptions): Promise<HttpsFunction> { | ||
| setGlobalOptions({ | ||
| invoker: "<FDC-P4SA>@developer.gserviceaccount.com", |
There was a problem hiding this comment.
The invoker service account is hardcoded to "<FDC-P4SA>@developer.gserviceaccount.com". The angle brackets suggest this might be a placeholder value that was intended to be replaced. If this is a fixed, required value for Data Connect functions, it would be better to define it as a constant with a descriptive name to improve clarity and maintainability. If it's a placeholder, it should be resolved to a correct value or made configurable. Hardcoding placeholder-like values can lead to deployment or security issues.
1 participant
Description
Code sample