Conversation
Summary of ChangesHello, 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 establishes the foundational structure for a new Flask application. It integrates environment variable management using python-dotenv, sets up a basic Flask app with a health check, and crucially, introduces a dedicated module for managing MongoDB connections. This setup provides a robust starting point for developing features that require database interaction and configurable application settings. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
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
|
There was a problem hiding this comment.
Pull request overview
Adds initial MongoDB connectivity helper for the Flask app, but also includes app scaffolding and environment/dependency files that appear to overlap with the changes described in dependent PR #93.
Changes:
- Add
database.pywith a module-levelMongoClientandget_db()/close_db()helpers. - Add Python dependencies for Flask, dotenv loading, and PyMongo.
- Add Flask app skeleton and
.env.examplecontainingMONGO_URIand Flask settings.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| requirements.txt | Adds Flask, dotenv, and PyMongo dependencies needed to run the app and connect to MongoDB. |
| database.py | Introduces a MongoDB connection helper with a cached global client. |
| app.py | Adds a minimal Flask app with /health and SECRET_KEY setup. |
| .env.example | Provides example environment configuration including MONGO_URI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
app.py
Outdated
| app = Flask(__name__) | ||
|
|
||
| secret_key = os.environ.get("SECRET_KEY") | ||
| if not secret_key: | ||
| if os.environ.get("FLASK_DEBUG", "0").lower() in ("1", "true", "yes"): |
There was a problem hiding this comment.
This PR defines close_db() but doesn’t register any Flask teardown/shutdown hook to call it, so the MongoDB client may stay open across dev server reloads/tests. Consider wiring it up via app.teardown_appcontext (or an app factory) so it reliably runs on shutdown.
![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 a MongoDB connection helper and a basic Flask application. My review focuses on improving the robustness and maintainability of the new code. I've identified a race condition in the database connection logic and a potential resource leak due to the connection not being closed correctly. I've also suggested refactoring the Flask application to use the standard application factory pattern, which addresses several issues including configuration management, resource handling, and testability. These changes will make the application more stable and align it with common Flask best practices.
app.py
Outdated
| load_dotenv() | ||
|
|
||
| app = Flask(__name__) | ||
|
|
||
| secret_key = os.environ.get("SECRET_KEY") | ||
| if not secret_key: | ||
| if os.environ.get("FLASK_DEBUG", "0").lower() in ("1", "true", "yes"): | ||
| secret_key = secrets.token_urlsafe(32) | ||
| else: | ||
| raise RuntimeError("SECRET_KEY environment variable must be set in production.") | ||
| app.config["SECRET_KEY"] = secret_key | ||
|
|
||
|
|
||
| @app.route("/health") | ||
| def health(): | ||
| return jsonify(status="ok") | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| debug = os.environ.get("FLASK_DEBUG", "0").lower() in ("1", "true", "yes") | ||
| port = int(os.environ.get("FLASK_PORT", 5000)) | ||
| app.run(debug=debug, port=port) |
There was a problem hiding this comment.
The current application setup can be improved by using the application factory pattern, which is a standard practice in Flask. This makes the application more modular, testable, and easier to manage configuration for.
Additionally, the MongoClient connection should be closed when the application shuts down. This can be achieved by registering the close_db function with the atexit module.
This refactoring also resolves code duplication for the debug flag check and adds robust error handling for the FLASK_PORT configuration.
Here is a suggested implementation for app.py:
import atexit
import os
import secrets
from dotenv import load_dotenv
from flask import Flask, jsonify
from database import close_db
def create_app():
"""Create and configure an instance of the Flask application."""
load_dotenv()
app = Flask(__name__)
is_debug = os.environ.get("FLASK_DEBUG", "0").lower() in ("1", "true", "yes")
app.config.from_mapping(
SECRET_KEY=os.environ.get("SECRET_KEY"),
DEBUG=is_debug,
)
if not app.config["SECRET_KEY"]:
if app.config["DEBUG"]:
app.config["SECRET_KEY"] = secrets.token_urlsafe(32)
else:
raise RuntimeError("SECRET_KEY environment variable must be set in production.")
@app.route("/health")
def health():
return jsonify(status="ok")
atexit.register(close_db)
return app
if __name__ == "__main__":
app = create_app()
port_str = os.environ.get("FLASK_PORT", "5000")
try:
port = int(port_str)
except ValueError:
raise RuntimeError(f"Invalid value for FLASK_PORT: '{port_str}'. Must be an integer.")
app.run(port=port)
database.py
Outdated
| _client = None | ||
|
|
||
|
|
||
| def get_db(): | ||
| global _client | ||
| if _client is None: | ||
| uri = os.environ.get("MONGO_URI", "mongodb://localhost:27017/bhv") | ||
| _client = MongoClient(uri) | ||
| return _client.get_default_database() | ||
|
|
||
|
|
||
| def close_db(): | ||
| global _client | ||
| if _client is not None: | ||
| _client.close() | ||
| _client = None |
There was a problem hiding this comment.
The current implementation for managing the MongoDB client is unnecessarily complex and has a race condition in a multi-threaded environment. A simpler, safer, and more idiomatic approach is to initialize the MongoClient at the module level. It's designed to be a long-lived singleton. The close_db function should then be simplified. The client should be closed when the application exits, which can be handled in app.py.
| _client = None | |
| def get_db(): | |
| global _client | |
| if _client is None: | |
| uri = os.environ.get("MONGO_URI", "mongodb://localhost:27017/bhv") | |
| _client = MongoClient(uri) | |
| return _client.get_default_database() | |
| def close_db(): | |
| global _client | |
| if _client is not None: | |
| _client.close() | |
| _client = None | |
| uri = os.environ.get("MONGO_URI", "mongodb://localhost:27017/bhv") | |
| _client = MongoClient(uri) | |
| def get_db(): | |
| """Returns the default database from the MongoDB client.""" | |
| return _client.get_default_database() | |
| def close_db(): | |
| """Closes the MongoDB client connection.""" | |
| _client.close() |
kunal-595
force-pushed
the
feat/mongo-connection
branch
from
ca21f8c to
83d7f85
Compare
kunal-595
force-pushed
the
feat/mongo-connection
branch
from
83d7f85 to
595f05b
Compare
|
@mdxabu ,plz review this pr |
|
Try to add this in the same pr #93 |
Depends on: #93
Changes in this PR only added database.py with MongoDB connection helper