Updated samples for using Cloud SQL with App Engine. #4202
Conversation
product-auto-label
bot
added
samples
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Summary of ChangesHello @chandrabistaiah, 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 significantly refactors the Cloud SQL samples for App Engine, providing clearer distinctions and dedicated examples for both flexible and standard environments. It introduces new standard environment samples for MySQL and PostgreSQL, including basic and connection-pooling implementations, and updates existing flexible environment samples. The changes also involve upgrading the Node.js runtime to version 10 in standard environment configurations and updating CI build settings to accommodate the new sample structure. 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 refactors the Cloud SQL samples for App Engine, splitting them by environment (standard/flexible) and database type (MySQL/PostgreSQL), and adding examples for connection pooling. The changes are well-structured. However, I've found some critical issues in the new sample code, such as database connection leaks and improper error handling that could crash the server. I've also suggested updates for outdated dependencies and fixed some typos in the documentation. Please review my comments for details.
| app.get(`/`, (req, res, next) => { | ||
| var connection = mysql.createConnection(dbConfig); | ||
| connection.query( | ||
| `SELECT NOW() as now`, | ||
| function (error, results, fields) { | ||
| if (error) throw error; | ||
| var now = results[0].now; | ||
| res | ||
| .status(200) | ||
| .send(`Current time: ${now}`) | ||
| .end(); | ||
| } | ||
| ); | ||
| }); |
There was a problem hiding this comment.
This request handler has two critical issues:
- Connection Leak: A new database connection is created for each request, but it's never closed. This will lead to connection leaks and can exhaust database resources.
- Unhandled Errors:
throw error;will crash the Node.js process if the database query fails. In an Express application, you should pass errors to thenext()function to be handled by an error-handling middleware.
I've suggested a change that fixes both of these issues.
| app.get(`/`, (req, res, next) => { | |
| var connection = mysql.createConnection(dbConfig); | |
| connection.query( | |
| `SELECT NOW() as now`, | |
| function (error, results, fields) { | |
| if (error) throw error; | |
| var now = results[0].now; | |
| res | |
| .status(200) | |
| .send(`Current time: ${now}`) | |
| .end(); | |
| } | |
| ); | |
| }); | |
| app.get(`/`, (req, res, next) => { | |
| const connection = mysql.createConnection(dbConfig); | |
| connection.query( | |
| `SELECT NOW() as now`, | |
| (error, results) => { | |
| connection.end(); | |
| if (error) { | |
| return next(error); | |
| } | |
| const now = results[0].now; | |
| res.status(200).send(`Current time: ${now}`); | |
| } | |
| ); | |
| }); |
| app.get(`/`, (req, res, next) => { | ||
| var client = new pg.Client(dbConfig); | ||
| client.connect(); | ||
| client | ||
| .query(`SELECT NOW() as now`) | ||
| .then(result => { | ||
| var now = result.rows[0].now; | ||
| res | ||
| .status(200) | ||
| .send(`Current time: ${now}`) | ||
| .end(); | ||
| }) | ||
| .catch(err => { | ||
| next(err); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
This request handler has two critical issues:
- Connection Leak: A new database client is created for each request, but the connection is never closed with
client.end(). This will lead to connection leaks and can exhaust database resources. - Unhandled Promise:
client.connect()returns a promise that is not handled. Whilepgmight queue the query, it's better practice to ensure the connection is established before querying.
I've suggested a change that uses promise chaining to correctly handle the connection lifecycle and errors.
app.get(`/`, (req, res, next) => {
const client = new pg.Client(dbConfig);
client.connect()
.then(() => client.query(`SELECT NOW() as now`))
.then(result => {
const now = result.rows[0].now;
res.status(200).send(`Current time: ${now}`);
})
.catch(err => next(err))
.then(() => client.end());
});| pool.query( | ||
| `SELECT NOW() as now`, | ||
| function (error, results, fields) { | ||
| if (error) throw error; | ||
| var now = results[0].now; | ||
| res | ||
| .status(200) | ||
| .send(`Current time: ${now}`) | ||
| .end(); | ||
| } | ||
| ); |
There was a problem hiding this comment.
Unhandled Errors: throw error; will crash the Node.js process if the database query fails. In an Express application, you should pass errors to the next() function so they can be handled properly.
I've suggested a change that fixes this.
pool.query(
`SELECT NOW() as now`,
(error, results) => {
if (error) {
return next(error);
}
const now = results[0].now;
res.status(200).send(`Current time: ${now}`);
}
);| }, | ||
| "devDependencies": { | ||
| "@google-cloud/nodejs-repo-tools": "2.3.0", | ||
| "ava": "0.25.0", |
There was a problem hiding this comment.
The version of ava is quite old (0.25.0 was released in 2017). Consider upgrading to a more recent version (e.g., ^5.0.0) to take advantage of performance improvements, new features, and bug fixes.
| "ava": "0.25.0", | |
| "ava": "^5.0.0", |
| }, | ||
| "devDependencies": { | ||
| "@google-cloud/nodejs-repo-tools": "2.3.0", | ||
| "ava": "0.25.0", |
There was a problem hiding this comment.
The version of ava is quite old (0.25.0 was released in 2017). Consider upgrading to a more recent version (e.g., ^5.0.0) to take advantage of performance improvements, new features, and bug fixes.
| "ava": "0.25.0", | |
| "ava": "^5.0.0", |
| @@ -0,0 +1,11 @@ | |||
| # Cloud SQL for PostgreSQL Node.js sample on App Engine standard environment | |||
|
|
|||
| This sample application shows how to use [Google Cloud SQL[[sql] for [PostgreSQL][postgres] | |||
There was a problem hiding this comment.
There's a typo here. It should be [Google Cloud SQL][sql] instead of [Google Cloud SQL[[sql].
| This sample application shows how to use [Google Cloud SQL[[sql] for [PostgreSQL][postgres] | |
| This sample application shows how to use [Google Cloud SQL][sql] for [PostgreSQL][postgres] |
| }, | ||
| "devDependencies": { | ||
| "@google-cloud/nodejs-repo-tools": "2.2.1", | ||
| "ava": "^0.25.0", |
There was a problem hiding this comment.
The version of ava is quite old (^0.25.0 was released in 2017). Consider upgrading to a more recent version (e.g., ^5.0.0) to take advantage of performance improvements, new features, and bug fixes.
| "ava": "^0.25.0", | |
| "ava": "^5.0.0", |
| @@ -0,0 +1,11 @@ | |||
| # Cloud SQL for PostgreSQL Node.js sample on App Engine standard environment | |||
|
|
|||
| This sample application shows how to use [Google Cloud SQL[[sql] for [PostgreSQL][postgres] | |||
There was a problem hiding this comment.
There's a typo here. It should be [Google Cloud SQL][sql] instead of [Google Cloud SQL[[sql].
| This sample application shows how to use [Google Cloud SQL[[sql] for [PostgreSQL][postgres] | |
| This sample application shows how to use [Google Cloud SQL][sql] for [PostgreSQL][postgres] |
| }, | ||
| "devDependencies": { | ||
| "@google-cloud/nodejs-repo-tools": "2.2.1", | ||
| "ava": "0.25.0", |
There was a problem hiding this comment.
The version of ava is quite old (0.25.0 was released in 2017). Consider upgrading to a more recent version (e.g., ^5.0.0) to take advantage of performance improvements, new features, and bug fixes.
| "ava": "0.25.0", | |
| "ava": "^5.0.0", |
2 participants
Description
Fixes #
Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.
Checklist
npm test(see Testing)npm run lint(see Style)GoogleCloudPlatform/nodejs-docs-samples. Not a fork.