优化存储结构 #2
优化存储结构 #2
Conversation
…removing DB-based config
…save behavior; replace LoadFromDatabase calls
…del/DAO/migration
…model/DAO; update services to use ConfigManager
… copies and add migration docs
…nup; add dto placeholder
…ers; persist config via YAML-first flow
…tions into internal/static
…c admin static registration; enforce protected handlers in routes/admin.go
… via protected handlers
…dd HEAD handlers)
…h to avoid blocking login page
There was a problem hiding this comment.
Pull Request Overview
This PR implements a comprehensive storage structure optimization for the FileCodeBox system, transitioning from a database-driven configuration approach to a YAML-first configuration management system. The changes improve maintainability, performance, and user experience.
Key Changes:
- Migration from database key-value configuration to YAML-first configuration management
- Enhanced folder upload functionality with ZIP compression support
- Improved authentication system supporting both session-based JWT and admin credentials
- Optimized frontend UI with better file/folder selection and upload progress tracking
Reviewed Changes
Copilot reviewed 119 out of 125 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| main.go | Restructured application lifecycle with delayed database initialization and dynamic route registration |
| internal/config/* | Complete overhaul of configuration system from database-driven to YAML-first approach |
| themes/2025/setup.html | Enhanced setup UI with username/email fields, password confirmation, and local storage configuration |
| themes/2025/js/upload.js | Added comprehensive folder upload support with JSZip compression |
| themes/2025/js/dashboard.js | Improved error handling, user authentication flow, and folder upload capabilities |
| internal/handlers/* | Streamlined request validation using utility functions and improved authentication |
| internal/services/admin/* | Simplified configuration management with direct struct updates instead of DTO mapping |
| internal/repository/* | Removed KeyValue DAO and simplified user update methods |
| internal/models/* | Cleaned up DTO models and consolidated type aliases |
| placeholder="系统管理令牌，用于管理后台登录" required> | ||
| <div class="form-group hidden"> | ||
| <label for="adminToken">管理员令牌（已移除）</label> | ||
| <input type="text" id="adminToken" name="password" class="form-control" |
There was a problem hiding this comment.
The hidden admin token field with name='password' could cause confusion. Consider either removing this field entirely or using a more descriptive name like name='deprecated_admin_token' to avoid potential conflicts with the actual password field.
| <input type="text" id="adminToken" name="password" class="form-control" | |
| <input type="text" id="adminToken" name="deprecated_admin_token" class="form-control" |
| try { | ||
| // 使用JSZip创建压缩包 | ||
| if (typeof JSZip === 'undefined') { | ||
| throw new Error('文件夹上传功能需要加载JSZip库，请刷新页面重试'); |
There was a problem hiding this comment.
The error message suggests refreshing the page as a solution, but this may not resolve the JSZip loading issue. Consider a more specific message like '文件夹上传功能需要JSZip库，请检查网络连接或联系管理员' or implement a fallback loading mechanism.
| throw new Error('文件夹上传功能需要加载JSZip库，请刷新页面重试'); | |
| throw new Error('文件夹上传功能需要JSZip库，请检查网络连接或联系管理员'); |
| // 如果 opacity 为字符串 '0' 或 '0.0'，不应把整个页面设为完全透明。 | ||
| // 只在解析为数字且大于0时应用不透明度设置。 | ||
| const op = parseFloat(window.AppConfig.opacity); | ||
| if (!isNaN(op) && op > 0) { |
There was a problem hiding this comment.
The opacity validation only checks op > 0, but valid CSS opacity values range from 0 to 1. An opacity of 0 is valid and should be applied. Change the condition to op >= 0 && op <= 1 to properly handle the full range of valid opacity values.
| if (!isNaN(op) && op > 0) { | |
| if (!isNaN(op) && op >= 0 && op <= 1) { |
| // 准备更新字段 | ||
| updateFields := &models.UserUpdateFields{} | ||
| func (s *Service) UpdateUser(user models.User) error { | ||
|
|
There was a problem hiding this comment.
The method signature changed from accepting individual parameters to a single User struct, but there's no validation that the User.ID field is set. Consider adding validation: if user.ID == 0 { return errors.New('user ID is required') } to prevent updating all users accidentally.
| if user.ID == 0 { | |
| return errors.New("user ID is required") | |
| } |
| func GetUsagePercent(path string) (float64, error) { | ||
| var stat syscall.Statfs_t | ||
| if err := syscall.Statfs(path, &stat); err != nil { | ||
| return 0, err | ||
| } |
There was a problem hiding this comment.
The function uses syscall.Statfs which is Unix-specific and will fail on Windows. Consider adding build tags (//go:build !windows) or implementing a Windows-compatible version using appropriate system calls to ensure cross-platform compatibility.
| <button class="btn-sm btn-danger" onclick="Dashboard.deleteFile('${file.code}')" title="删除文件"> | ||
| 🗑️ 删除 | ||
| </button> |
There was a problem hiding this comment.
The delete button now uses file.code instead of file.id. Ensure that the backend DeleteFile method has been updated to accept and handle file codes instead of IDs, as this represents a breaking change in the API contract.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codex Review: Here are some suggestions.
Reply with @codex fix comments to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
| func (rl *RateLimiter) GetLimiter(key string, r rate.Limit, b int) *rate.Limiter { | ||
| // Note: for simplicity we avoid mutex here; callers should ensure safe usage | ||
| limiter, exists := rl.limiters[key] | ||
| if !exists { | ||
| limiter = rate.NewLimiter(r, b) | ||
| rl.limiters[key] = limiter | ||
| } | ||
| return limiter | ||
| } | ||
|
|
||
| func (rl *RateLimiter) Cleanup() { | ||
| ticker := time.NewTicker(time.Hour) | ||
| go func() { | ||
| for range ticker.C { | ||
| for key, limiter := range rl.limiters { | ||
| if limiter.Allow() { | ||
| delete(rl.limiters, key) | ||
| } |
There was a problem hiding this comment.
[P0] Protect shared limiter map from concurrent access
The new rate limiter stores per‑IP limiters in a plain map and both GetLimiter and the cleanup goroutine mutate that map without any synchronization. Because RateLimit is installed as a Gin middleware, multiple requests will call GetLimiter concurrently and the cleanup goroutine also deletes entries while handlers may read/write. Go maps are not safe for concurrent use, so under concurrent traffic the process can panic with fatal error: concurrent map read and map write, effectively taking down the server.
Useful? React with 👍 / 👎.
| // mergeSimpleFields 合并简单字段 | ||
| func (cm *ConfigManager) mergeSimpleFields(fileCfg *ConfigManager) { | ||
| if fileCfg.NotifyTitle != "" { | ||
| cm.NotifyTitle = fileCfg.NotifyTitle | ||
| } | ||
|
|
||
| // 检查是否已经有配置数据 | ||
| var count int64 | ||
| if err := cm.db.Model(&models.KeyValue{}).Count(&count).Error; err != nil { | ||
| return fmt.Errorf("检查配置数据失败: %w", err) | ||
| } | ||
|
|
||
| // 如果已有数据，不进行初始化 | ||
| if count > 0 { | ||
| return nil | ||
| if fileCfg.NotifyContent != "" { | ||
| cm.NotifyContent = fileCfg.NotifyContent | ||
| } | ||
|
|
||
| // 使用公共方法获取配置映射 | ||
| defaultConfigs := cm.buildConfigMap() | ||
|
|
||
| // 批量插入默认配置 | ||
| var keyValues []models.KeyValue | ||
| for key, value := range defaultConfigs { | ||
| keyValues = append(keyValues, models.KeyValue{ | ||
| Key: key, | ||
| Value: value, | ||
| }) | ||
| } | ||
|
|
||
| if err := cm.db.CreateInBatches(keyValues, 50).Error; err != nil { | ||
| return fmt.Errorf("插入默认配置失败: %w", err) | ||
| if fileCfg.SysStart != "" { | ||
| cm.SysStart = fileCfg.SysStart | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // SetDB 设置数据库连接 | ||
| func (cm *ConfigManager) SetDB(db *gorm.DB) { | ||
| cm.db = db | ||
| } | ||
|
|
||
| // Save 保存配置 | ||
| func (cm *ConfigManager) Save() error { | ||
| // 验证配置 | ||
| if err := cm.Validate(); err != nil { | ||
| // LoadFromYAML 从 YAML 文件加载配置 | ||
| func (cm *ConfigManager) LoadFromYAML(path string) error { | ||
| b, err := os.ReadFile(path) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // 只保存到数据库 | ||
| if cm.db != nil { | ||
| return cm.saveToDatabase() | ||
| } | ||
|
|
||
| return errors.New("数据库连接未设置，无法保存配置") | ||
| } | ||
|
|
||
| // saveToDatabase 保存配置到数据库 | ||
| func (cm *ConfigManager) saveToDatabase() error { | ||
| if cm.db == nil { | ||
| return errors.New("数据库连接未设置") | ||
| var fileCfg ConfigManager | ||
| if err := yaml.Unmarshal(b, &fileCfg); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // 使用公共方法获取配置映射 | ||
| configMap := cm.buildConfigMap() | ||
|
|
||
| for key, value := range configMap { | ||
| kv := models.KeyValue{ | ||
| Key: key, | ||
| Value: value, | ||
| } | ||
|
|
||
| // 使用 UPSERT 操作 | ||
| if err := cm.db.Where("key = ?", key).Assign(models.KeyValue{Value: value}).FirstOrCreate(&kv).Error; err != nil { | ||
| return fmt.Errorf("保存配置项 %s 失败: %w", key, err) | ||
| } | ||
| } | ||
| // 按模块合并配置 | ||
| cm.mergeConfigModules(&fileCfg) | ||
| cm.mergeUserConfig(fileCfg.User) | ||
| cm.mergeSimpleFields(&fileCfg) |
There was a problem hiding this comment.
[P1] YAML upload/error rate settings never applied
LoadFromYAML only copies nested modules and the three simple fields handled by mergeSimpleFields, but it never transfers the top‑level UploadMinute, UploadCount, ErrorMinute, ErrorCount, or ExpireStyle values read from the YAML file into the live manager. As a result these settings stay at their zero defaults (or whatever was compiled in) regardless of the configuration file, which means administrators cannot change rate limits via config.yaml and the limiter middleware runs with unintended defaults (currently unlimited).
Useful? React with 👍 / 👎.
No description provided.