Skip to content

Conversation

@JaD1ng
Copy link
Collaborator

@JaD1ng JaD1ng commented Sep 12, 2025

  • 重构数据库模型,简化部署任务和状态管理结构
  • 移除不再使用的部署批次相关代码
  • 更新数据库连接配置和SQL查询语句
  • 添加CORS中间件支持
  • 调整API路由顺序和移除不再使用的端点
  • 更新健康状态枚举命名和类型定义
  • 添加数据库schema文件定义表结构

变更背景和解决方案

关联issue: #

文档更新(架构文档、API文档、升级文档)

Checklist

  • 确认是否有文档更新
  • 确认是否自测
  • 确认是否考虑兼容升级,不兼容升级需要有对应的解决方案
  • 确认是否考虑添加监控指标
  • API 设计是否符合 API 设计规范, 如果没有 API 变更,请忽略
  • 我已经添加了相关人员到 Reviewers 列表中
  • 我已经设置了 Development 关联的 Issue

@niupilot
Copy link

niupilot bot commented Sep 12, 2025

Code Review for PR #65 - Service Manager Refactoring

Review Checklist:

  • Gather context and analyze changed files
  • Review database model refactoring changes
  • Analyze API and routing modifications
  • Review CORS middleware addition
  • Check code quality and best practices
  • Identify potential bugs or issues
  • Verify documentation consistency
  • Provide comprehensive feedback with specific recommendations

Summary

This PR implements a significant refactoring of the service manager module, focusing on simplifying the database model and removing unused deployment batch functionality. The changes are well-structured and improve the overall architecture.

✅ Positive Changes

Database Model Improvements

  • Simplified schema: Removed the complex deploy_batch model which was likely over-engineered
  • Clean table structure: schema.sql provides a clear, normalized database schema with proper foreign key relationships
  • Better indexing: Added appropriate indexes for query performance (service_states, deploy_tasks)
  • Mock data: Includes realistic test data for development/testing

CORS Middleware Addition

  • Good implementation: internal/middleware/cors.go follows standard CORS patterns
  • Proper integration: Correctly added to the API setup in api.go:24

Code Organization

  • Cleaner API structure: Removed unused endpoints and consolidated routing logic
  • Better separation: Database operations are more focused and less complex

⚠️ Issues and Concerns

1. Data Migration Concerns (High Priority)

File: internal/service_manager/model/schema.sql:3-8

The schema includes DROP TABLE IF EXISTS statements which will destroy all existing data. This is dangerous for production deployments.

Recommendations:

  • Remove DROP statements from the schema file
  • Create separate migration scripts for existing deployments
  • Add proper migration strategy documentation

2. Incomplete Migration in Repository Layer (High Priority)

File: internal/service_manager/database/deploy_repo.go:85-86, 137-138, 191-196

Multiple TODO comments indicate incomplete implementation:

// TODO: 需要根据业务逻辑决定如何处理service过滤
// TODO: 需要根据业务逻辑决定如何处理version更新
// TODO: 需要根据业务逻辑决定如何检查部署冲突

Impact: Core functionality is missing, which could cause runtime failures.

Recommendations:

  • Implement the missing business logic before merging
  • If not implementable now, add proper error handling instead of silent ignoring

3. CORS Security Issue (Medium Priority)

File: internal/middleware/cors.go:15,28

c.Header("Access-Control-Allow-Origin", "*")
// ... later overwritten by:
c.Header("Access-Control-Allow-Origin", origin)

Issues:

  • Sets wildcard origin then overwrites it - confusing logic
  • Access-Control-Allow-Credentials: true with wildcard origin is a security risk
  • No origin validation

Recommendations:

func CORS() fox.HandlerFunc {
    return func(c *fox.Context) {
        origin := c.Request.Header.Get("Origin")
        
        // Validate allowed origins
        allowedOrigins := []string{"https://app.example.com", "http://localhost:3000"}
        if isValidOrigin(origin, allowedOrigins) {
            c.Header("Access-Control-Allow-Origin", origin)
            c.Header("Access-Control-Allow-Credentials", "true")
        }
        
        // ... rest of headers
    }
}

4. Database Connection Configuration (Low Priority)

File: internal/service_manager/database/database.go:19-20

Hard-coded SSL mode in DSN construction. Consider making this configurable through the config struct.

5. Error Handling Inconsistencies (Low Priority)

File: internal/service_manager/database/deploy_repo.go:31

Silent JSON marshaling error ignored:

instancesJSON, _ := json.Marshal(instances)

Should handle potential marshaling errors properly.

🔍 Code Quality Observations

Good Practices Followed:

  • Proper context usage throughout the codebase
  • Consistent error handling patterns in API layer
  • Good separation of concerns between API, service, and repository layers
  • Proper use of Go conventions and naming

Minor Improvements:

  1. Type Safety: Consider using strongly-typed IDs instead of strings
  2. Validation: Add input validation for JSON fields (instances array in deploy_tasks)
  3. Constants: Magic strings like table names could be constants

📋 Documentation Consistency

Missing Updates:

  • No README updates reflect the schema changes
  • API documentation needs updates for removed endpoints
  • Migration guide missing for existing deployments

🎯 Recommendations Priority

Before Merge (Critical):

  1. Fix TODO items in deploy_repo.go or add proper error handling
  2. Remove DROP statements from schema.sql
  3. Fix CORS security issues

Post-Merge (Important):

  1. Add proper data migration documentation
  2. Update API documentation
  3. Add input validation for JSON fields

Overall Assessment: This refactoring improves the codebase architecture significantly, but several critical issues need addressing before merge. The simplified database model is a good direction, but migration concerns must be handled carefully.

👉 Want me to re-run this review? Just type /review in the comments! For more usage examples, visit CodeAgent GitHub Repository.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Summary of Changes

Hello @JaD1ng, 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!

本次拉取请求对服务管理模块进行了全面的重构,核心在于优化数据库模型和API设计。通过简化部署任务和状态管理的数据结构,并引入PostgreSQL作为数据库驱动,提升了系统的可维护性和扩展性。同时,API层面的调整和CORS支持也使得服务更加健壮和易于集成。

Highlights

  • 数据库模型重构: 简化了部署任务和状态管理结构,移除了不再使用的部署批次相关代码,并引入了新的 deploy_tasks 表,该表不再直接关联服务和服务版本,而是专注于部署任务本身的生命周期和实例管理。
  • API和路由调整: 调整了API路由顺序,移除了不再使用的 /v1/services/:service 端点,并添加了CORS中间件支持,增强了API的灵活性和安全性。
  • 数据库连接和查询更新: 更新了数据库连接配置,引入了PostgreSQL驱动 github.com/lib/pq,并修改了所有数据库查询语句以适应PostgreSQL的参数占位符(从 ?$)。
  • 健康状态枚举和类型定义变更: 将模型中的 HealthStatus 类型重命名为 Level,并更新了相关常量和结构体字段的类型定义。
  • 新增数据库Schema文件: 添加了 schema.sql 文件,明确定义了所有数据库表的结构,包括 servicesservice_versionsservice_instancesservice_states 和新的 deploy_tasks 表,并包含了初始化mock数据。
Using Gemini Code Assist

The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

本次 PR 对服务管理模块进行了大规模重构,包括数据库模型、API 和相关逻辑。整体上,代码结构更清晰,并切换到了 PostgreSQL 数据库。我发现了一些需要注意的问题,主要集中在 CORS 中间件的实现、数据库操作的安全性和并发问题,以及部分重构后遗留的逻辑缺陷。特别是 CORS 中间件的 bug 和被禁用的部署冲突检查,可能会导致严重的功能或安全问题,需要优先修复。

- 重构数据库模型,简化部署任务和状态管理结构
- 移除不再使用的部署批次相关代码
- 更新数据库连接配置和SQL查询语句
- 添加CORS中间件支持
- 调整API路由顺序和移除不再使用的端点
- 更新健康状态枚举命名和类型定义
- 添加数据库schema文件定义表结构
将分散的内部README文件合并为统一文档,并添加详细架构设计和API说明
将ExceptionStatus重命名为Status以简化命名
新增HealthLevel枚举用于API响应,包含Normal状态
修改service_state和API模型以使用新的枚举类型
更新info_service中的状态映射逻辑
@LiusCraft LiusCraft merged commit edecafc into qiniu:develop Sep 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(ServiceManager): 完成服务管理模块API定义、数据表定义及基本处理逻辑

2 participants