Conversation
1.main.js: 修复静态资源路径 (../public → ./public) 2.utils/db.js: 添加 MySQL 配置检查，未配置时不会崩溃 3.services/BaseService.js: 添加数据库连接验证
There was a problem hiding this comment.
Pull request overview
This pull request adds Docker deployment support to the MS_OAuth2API_Next project, making MySQL database usage optional instead of required. The key changes enable the application to run in Docker containers with flexible database configuration.
Key Changes:
- Makes MySQL database optional with conditional initialization based on environment variables
- Adds complete Docker deployment setup with docker-compose for app, Redis, and optional MySQL services
- Updates database connection logic to handle scenarios where MySQL is not configured
- Adds comprehensive Docker deployment documentation
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| utils/db.js | Implements conditional MySQL pool creation based on environment variables, only initializing when DB_HOST and DB_DATABASE are provided |
| services/BaseService.js | Adds null check for database pool in the query method and fixes trailing whitespace |
| main.js | Initializes database connection module and fixes static file path from '../public' to './public' |
| docker-compose.yml | Defines multi-service Docker setup with app, Redis, and optional MySQL services with proper networking and health checks |
| Dockerfile | Implements multi-stage build with frontend compilation and backend setup optimized for production |
| README.md | Adds comprehensive Docker deployment documentation including quick start guide, port mappings, and MySQL enablement instructions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const [result] = await pool.query( | ||
| `UPDATE ?? SET ${setClause} WHERE id = ?`, | ||
| [this.tableName, ...values, id] | ||
| ); |
There was a problem hiding this comment.
This method directly uses pool.query() without checking if pool is null. Since the database is now optional, this should use this.query() instead to ensure consistent error handling when the database is not configured.
| } | ||
|
|
||
| const [result] = await pool.query( |
There was a problem hiding this comment.
This method directly uses pool.query() without checking if pool is null. Since the database is now optional, this should use this.query() instead to ensure consistent error handling when the database is not configured.
| const [result] = await pool.query( | |
| const [result] = await this.query( |
| if (process.env.DB_HOST && process.env.DB_DATABASE) { | ||
| pool = mysql.createPool({ | ||
| host: process.env.DB_HOST, | ||
| port: process.env.DB_PORT || 3306, |
There was a problem hiding this comment.
The port value should be converted to an integer. Currently, if DB_PORT is provided as a string environment variable, it will be passed as a string to mysql.createPool(). Use parseInt(process.env.DB_PORT) || 3306 to ensure proper type conversion.
| port: process.env.DB_PORT || 3306, | |
| port: parseInt(process.env.DB_PORT) || 3306, |
| #- DB_PASSWORD=ms_oauth2api_root | ||
| #- DB_DATABASE=ms_oauth2api | ||
| #- DB_WAIT_FOR_CONNECTIONS=true | ||
| #- DB_CONNECTION_LIMIT=10 |
There was a problem hiding this comment.
The DB_QUEUE_LIMIT environment variable is commented out in the Docker configuration but is still referenced in the db.js file with a fallback value. Consider adding this environment variable to the commented MySQL configuration section for completeness and consistency.
| #- DB_CONNECTION_LIMIT=10 | |
| #- DB_CONNECTION_LIMIT=10 | |
| #- DB_QUEUE_LIMIT=50 |
| # Copy frontend source code | ||
| COPY web/MS_OAuth2API_Next_Web/ ./ | ||
|
|
||
| # Build frontend (skip type-check for faster builds) |
There was a problem hiding this comment.
The comment "skip type-check for faster builds" is misleading because npm run build-only is a standard Next.js/Vite command that naturally skips type checking (unlike npm run build). Consider removing or clarifying this comment to avoid confusion.
| # Build frontend (skip type-check for faster builds) | |
| # Build frontend using the build-only script |
| const [result] = await pool.query( | ||
| `INSERT INTO ?? (??) VALUES (${placeholders})`, | ||
| [this.tableName, fields, ...values] | ||
| ); |
There was a problem hiding this comment.
This method directly uses pool.query() without checking if pool is null. Since the database is now optional, this should use this.query() instead to ensure consistent error handling when the database is not configured.
| CMD wget --no-verbose --tries=1 --spider http://localhost:13000/ || exit 1 | ||
|
|
||
| # Start command | ||
| CMD ["node", "main.js"] |
There was a problem hiding this comment.
This container runs node main.js as the root user because no non-root user is configured in this Dockerfile. If an attacker ever gains code execution in the Node process (for example via a framework or dependency vulnerability), they will have root privileges inside the container, which increases the risk of container escape and damage to any mounted volumes. Create and switch to a dedicated unprivileged user via USER before starting the app to limit the blast radius of a compromise.
1 participant
新增Docker部署，可选择MySQL数据库