fix/status-review-sync-20260409 #1
15
.gitignore
vendored
15
.gitignore
vendored
@@ -75,3 +75,18 @@ uploads/avatars/*
|
||||
|
||||
# Backup temp
|
||||
backup_temp/
|
||||
|
||||
# SQLite temp files
|
||||
sub2api
|
||||
sub2api-shm
|
||||
sub2api-wal
|
||||
|
||||
# Codex temp
|
||||
.codex-tmp/
|
||||
|
||||
# Workbuddy memory (local AI memory, not project files)
|
||||
.workbuddy/memory/
|
||||
.workbuddy/expert-history.json
|
||||
|
||||
# Test coverage output
|
||||
frontend/admin/coverage/
|
||||
|
||||
550
docs/code-review/SENIOR_DEV_REVIEW_2026-04-10.md
Normal file
550
docs/code-review/SENIOR_DEV_REVIEW_2026-04-10.md
Normal file
@@ -0,0 +1,550 @@
|
||||
# 资深工程师代码 Review 报告
|
||||
|
||||
**项目**：用户管理系统（UMS）
|
||||
**Review 日期**：2026-04-10 23:45
|
||||
**分支**：`fix/status-review-sync-20260409`
|
||||
**Reviewer**：资深全栈工程师
|
||||
**Review 范围**：后端 Go + 前端 React/TS，全项目维度
|
||||
|
||||
---
|
||||
|
||||
## 执行摘要
|
||||
|
||||
> 本次 Review 基于**真实工具执行结果**（go build / go test / 覆盖率数据 / 代码扫描），不依赖文档自述。
|
||||
|
||||
| 维度 | 评分 | 状态 |
|
||||
|------|------|------|
|
||||
| 构建稳定性 | **9/10** | ✅ 全链路编译通过 |
|
||||
| 测试覆盖率 | **4/10** | 🔴 核心层极低（Service 15.2%，Handler 15.7%）|
|
||||
| 代码质量 | **6.5/10** | 🟠 存在 Stub 谎报、职责混乱等问题 |
|
||||
| 安全实践 | **7/10** | 🟡 基础加固到位，中级加固有缺口 |
|
||||
| 架构设计 | **6/10** | 🟠 分层存在渗漏，Service 依赖具体实现 |
|
||||
| 工程规范 | **6/10** | 🟠 行尾符乱、文档滞后、魔法数字残留 |
|
||||
| **综合评分** | **6.4/10** | ⚠️ **不达上线标准** |
|
||||
|
||||
---
|
||||
|
||||
## 一、构建与基础质量（实测数据）
|
||||
|
||||
### 1.1 编译结果
|
||||
|
||||
```
|
||||
go build ./cmd/server ✅ PASS
|
||||
go vet ./... ✅ PASS（无警告）
|
||||
go test ./... -short ✅ PASS（所有包通过）
|
||||
```
|
||||
|
||||
**结论**：基础工程卫生合格，无编译错误，无 vet 警告。
|
||||
|
||||
### 1.2 测试覆盖率——真实扫描结果
|
||||
|
||||
| 包 | 覆盖率 | 评价 |
|
||||
|----|--------|------|
|
||||
| `internal/api/handler` | **15.7%** | 🔴 严重不足 |
|
||||
| `internal/service` | **15.2%** | 🔴 严重不足 |
|
||||
| `internal/api/middleware` | **21.5%** | 🔴 严重不足 |
|
||||
| `internal/auth` | **28.1%** | 🔴 不足（安全敏感） |
|
||||
| `internal/repository` | **47.1%** | 🟡 中等，需提升 |
|
||||
| `internal/security` | **37.9%** | 🟡 中等 |
|
||||
| `internal/config` | **85.2%** | ✅ 良好 |
|
||||
| `internal/auth/providers` | **80.6%** | ✅ 良好 |
|
||||
| `internal/pkg/proxyurl` | **100%** | ✅ 优秀 |
|
||||
| `internal/pagination` | **0.0%** | 🔴 无测试（游标分页核心模块！） |
|
||||
| `internal/domain` | **2.7%** | 🔴 基本零测试 |
|
||||
|
||||
**核心问题**：Handler 和 Service 是业务逻辑的关键层，覆盖率双双仅 15%，意味着 85% 的业务逻辑完全没有测试保护。这是目前最危险的质量问题。
|
||||
|
||||
---
|
||||
|
||||
## 二、代码问题清单
|
||||
|
||||
### P0 - 文档声称已实现，代码实为 Stub
|
||||
|
||||
**问题位置**：`internal/api/handler/user_handler.go:337-339`
|
||||
|
||||
```go
|
||||
func (h *UserHandler) UploadAvatar(c *gin.Context) {
|
||||
c.JSON(http.StatusOK, gin.H{"message": "avatar upload not implemented"})
|
||||
}
|
||||
```
|
||||
|
||||
**严重性**：🔴 P0
|
||||
**说明**：`docs/code-review/PROJECT_REAL_COMPLETION_REVIEW_2026-04-10.md` 明确写道"Avatar Upload — 已实现且已验证"，甚至列出了测试场景（UploadAvatar_Unauthorized、UploadAvatar_NonAdminCannotUpdateOther）。但 Handler 层函数体仅返回 `"avatar upload not implemented"`，是纯 stub。Service 层也没有 `UploadAvatar` 函数。这是文档声称与真实代码**完全矛盾**的典型案例——也是团队中"Live 不等于闭环"原则被违反的直接证据。
|
||||
|
||||
**修复方向**：
|
||||
1. 实现真实的 multipart 文件接收、校验（大小/类型）、存储逻辑
|
||||
2. 添加 Service 层 `UploadAvatar` 方法
|
||||
3. 对失败路径实现文件清理（cleanup on partial write）
|
||||
4. 补充真实 401/403/413 响应测试
|
||||
|
||||
---
|
||||
|
||||
### P0 - AdminRoleID 硬编码魔法数字
|
||||
|
||||
**问题位置**：`internal/service/user_service.go:284`
|
||||
|
||||
```go
|
||||
const AdminRoleID = 1
|
||||
```
|
||||
|
||||
**严重性**：🔴 P0
|
||||
**说明**：这是典型的魔法常量设计反模式。管理员角色的 ID 完全依赖数据库插入顺序，在以下场景会直接断裂：
|
||||
- 数据库迁移到新环境
|
||||
- 插入顺序变化（数据 seed 逻辑修改）
|
||||
- 多租户场景
|
||||
|
||||
**修复方向**：通过角色 `code` 字段（如 `"admin"`）动态查询角色 ID，不要依赖自增 ID。
|
||||
|
||||
```go
|
||||
// 正确做法：通过 code 查询
|
||||
adminRole, err := s.roleRepo.GetByCode(ctx, "admin")
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### P1 - Service 层依赖具体实现而非接口
|
||||
|
||||
**问题位置**：`internal/service/user_service.go:17-24`
|
||||
|
||||
```go
|
||||
type UserService struct {
|
||||
userRepo *repository.UserRepository // ← 具体类型
|
||||
userRoleRepo *repository.UserRoleRepository // ← 具体类型
|
||||
roleRepo *repository.RoleRepository // ← 具体类型
|
||||
passwordHistoryRepo *repository.PasswordHistoryRepository // ← 具体类型
|
||||
}
|
||||
```
|
||||
|
||||
**严重性**：🟠 P1
|
||||
**说明**：Service 层直接依赖 Repository 具体结构体，而非接口。这违反了依赖倒置原则（DIP），导致：
|
||||
1. 无法对 Service 层进行单元测试（需要真实数据库）
|
||||
2. 无法 Mock 依赖（这是覆盖率仅 15% 的根因之一）
|
||||
3. 切换数据库实现或添加缓存层时，需要修改 Service 代码
|
||||
|
||||
**这是覆盖率低的架构根因**，必须优先解决。
|
||||
|
||||
**修复方向**：
|
||||
```go
|
||||
// 定义接口
|
||||
type UserRepository interface {
|
||||
GetByID(ctx context.Context, id int64) (*domain.User, error)
|
||||
Create(ctx context.Context, user *domain.User) error
|
||||
// ...
|
||||
}
|
||||
|
||||
// Service 依赖接口
|
||||
type UserService struct {
|
||||
userRepo UserRepository
|
||||
// ...
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### P1 - AssignRoles 删旧建新非事务，存在数据竞争风险
|
||||
|
||||
**问题位置**：`internal/service/user_service.go:267-280`
|
||||
|
||||
```go
|
||||
// 删除用户现有角色
|
||||
if err := s.userRoleRepo.DeleteByUserID(ctx, userID); err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
// 创建新的用户角色关联（←非原子操作，删旧成功但建新失败 → 用户无角色）
|
||||
var userRoles []*domain.UserRole
|
||||
for _, roleID := range roleIDs {
|
||||
userRoles = append(userRoles, &domain.UserRole{...})
|
||||
}
|
||||
return s.userRoleRepo.BatchCreate(ctx, userRoles)
|
||||
```
|
||||
|
||||
**严重性**：🟠 P1
|
||||
**说明**：删除旧角色和创建新角色之间没有事务包装。若 BatchCreate 失败，用户角色会被清空（陷入无角色状态）。并发请求场景下窗口期内用户权限会出现短暂真空。
|
||||
|
||||
**修复方向**：用 DB 事务包装整个操作：
|
||||
```go
|
||||
return s.db.Transaction(func(tx *gorm.DB) error {
|
||||
if err := s.userRoleRepo.WithTx(tx).DeleteByUserID(ctx, userID); err != nil {
|
||||
return err
|
||||
}
|
||||
return s.userRoleRepo.WithTx(tx).BatchCreate(ctx, userRoles)
|
||||
})
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### P1 - ListAdmins / GetUserRoles 存在 N+1 查询问题
|
||||
|
||||
**问题位置**：`internal/service/user_service.go:241-247` 和 `299-307`
|
||||
|
||||
```go
|
||||
// N+1 查询反模式
|
||||
for _, roleID := range roleIDs {
|
||||
role, err := s.roleRepo.GetByID(ctx, roleID) // ← 每个角色一次查询
|
||||
// ...
|
||||
}
|
||||
```
|
||||
|
||||
同样的模式在 `ListAdmins` 中：
|
||||
```go
|
||||
for _, adminID := range adminUserIDs {
|
||||
user, err := s.userRepo.GetByID(ctx, adminID) // ← 每个用户一次查询
|
||||
}
|
||||
```
|
||||
|
||||
**严重性**：🟠 P1
|
||||
**说明**：N+1 查询在角色/管理员数量增长时会导致明显性能退化。100 个管理员 = 101 次数据库查询。
|
||||
|
||||
**修复方向**：
|
||||
```go
|
||||
// Repository 提供批量查询方法
|
||||
roles, err := s.roleRepo.GetByIDs(ctx, roleIDs)
|
||||
|
||||
// 同样用于用户列表
|
||||
users, err := s.userRepo.GetByIDs(ctx, adminUserIDs)
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### P1 - 密码修改中哈希计算重复两次
|
||||
|
||||
**问题位置**：`internal/service/user_service.go:81-104`
|
||||
|
||||
```go
|
||||
// 第一次哈希（用于历史记录）
|
||||
newHashedPassword, hashErr := auth.HashPassword(newPassword)
|
||||
|
||||
// ... goroutine 里保存历史 ...
|
||||
|
||||
// 第二次哈希（用于更新用户密码）← 重复计算！
|
||||
newHashedPassword, err := auth.HashPassword(newPassword)
|
||||
user.Password = newHashedPassword
|
||||
```
|
||||
|
||||
**严重性**：🟠 P1
|
||||
**说明**：Argon2id（64MB 内存，5 次迭代）的哈希计算成本很高，对同一密码哈希两次是纯浪费。此外代码有逻辑问题：若历史记录分支进入 goroutine，主流程再哈希一次，两次结果是不同的哈希（因为 Argon2 包含随机盐），但这不是主要问题——主要问题是性能浪费和代码逻辑不清晰。
|
||||
|
||||
**修复方向**：哈希一次，复用结果：
|
||||
```go
|
||||
newHashedPassword, err := auth.HashPassword(newPassword)
|
||||
if err != nil {
|
||||
return errors.New("密码哈希失败")
|
||||
}
|
||||
// 复用 newHashedPassword 给历史记录和用户更新
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### P2 - 响应格式不统一
|
||||
|
||||
**问题位置**：`internal/api/handler/user_handler.go`
|
||||
|
||||
多处响应格式不一致：
|
||||
```go
|
||||
// 有的接口使用 code/message/data 包装
|
||||
c.JSON(http.StatusCreated, gin.H{
|
||||
"code": 0,
|
||||
"message": "success",
|
||||
"data": toUserResponse(user),
|
||||
})
|
||||
|
||||
// 有的接口裸返回
|
||||
c.JSON(http.StatusOK, toUserResponse(user)) // GetUser
|
||||
|
||||
// 有的返回字符串
|
||||
c.JSON(http.StatusOK, gin.H{"message": "user deleted"}) // DeleteUser
|
||||
```
|
||||
|
||||
**严重性**：🟡 P2
|
||||
**说明**：前端需要处理三种不同的响应结构，这是前后端联调噩梦的来源。
|
||||
|
||||
---
|
||||
|
||||
### P2 - 行尾符污染（git 警告已暴露）
|
||||
|
||||
**问题位置**：15 个文件存在 LF/CRLF 混用
|
||||
|
||||
```
|
||||
warning: in the working copy of 'internal/api/handler/user_handler.go',
|
||||
LF will be replaced by CRLF the next time Git touches it
|
||||
```
|
||||
|
||||
**严重性**：🟡 P2
|
||||
**说明**：Windows 开发环境下 git 行尾符不一致会影响 diff 可读性、代码审查效率，以及跨平台 CI/CD。
|
||||
|
||||
**修复方向**：在 `.gitattributes` 中强制统一行尾符：
|
||||
```
|
||||
* text=auto eol=lf
|
||||
*.go text eol=lf
|
||||
*.ts text eol=lf
|
||||
*.tsx text eol=lf
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### P2 - JWT 密钥缺乏启动时强制校验
|
||||
|
||||
**问题位置**：`configs/config.yaml:57`
|
||||
|
||||
```yaml
|
||||
jwt:
|
||||
secret: "" # ⚠️ 生产环境必须通过 JWT_SECRET 环境变量设置
|
||||
```
|
||||
|
||||
**严重性**：🟡 P2
|
||||
**说明**：注释写明了"必须通过环境变量设置"，但代码是否在启动时强制检查（release 模式下 secret 为空则拒绝启动）？若没有，服务会以空密钥运行，所有 JWT 签名均可伪造。
|
||||
|
||||
需要在启动代码中验证：
|
||||
```go
|
||||
if cfg.Server.Mode == "release" && cfg.JWT.Secret == "" {
|
||||
log.Fatal("FATAL: JWT_SECRET must be set in release mode")
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 三、架构评估
|
||||
|
||||
### 3.1 优点（值得肯定）
|
||||
|
||||
| 方面 | 亮点 |
|
||||
|------|------|
|
||||
| **Argon2id** | 密码哈希使用 Argon2id，参数配置合理（64MB/5次/4并行）✅ |
|
||||
| **crypto/rand** | 所有随机数使用 `crypto/rand`，无 `math/rand` ✅ |
|
||||
| **游标分页** | Sprint 18 实现的 Cursor 分页设计扎实，keyset 模式正确 ✅ |
|
||||
| **SQLite WAL** | WAL 模式 + PRAGMA 调优，体现了工程意识 ✅ |
|
||||
| **Token 轮换** | Refresh Token 滚动轮换防无限流实现正确 ✅ |
|
||||
| **非 root 容器** | Dockerfile 使用非 root 用户运行 ✅ |
|
||||
| **健康检查** | Docker HEALTHCHECK 已配置 ✅ |
|
||||
| **CSRF 保护** | CSRF token 机制存在且有效 ✅ |
|
||||
|
||||
### 3.2 架构债务
|
||||
|
||||
```
|
||||
┌─────────────────────────────────────────────────────┐
|
||||
│ Handler 层 │
|
||||
│ ✅ 职责基本清晰，但响应格式不统一 │
|
||||
└─────────────────────────────────────────────────────┘
|
||||
│ 调用（具体类型 ↓）
|
||||
┌─────────────────────────────────────────────────────┐
|
||||
│ Service 层 ⚠️ │
|
||||
│ - 依赖具体 Repository 结构体（违反 DIP） │
|
||||
│ - 存在 N+1 查询 │
|
||||
│ - AdminRoleID 硬编码 │
|
||||
│ - 无事务包装的多步操作 │
|
||||
└─────────────────────────────────────────────────────┘
|
||||
│ 调用（直接依赖 ↓）
|
||||
┌─────────────────────────────────────────────────────┐
|
||||
│ Repository 层 ✅ │
|
||||
│ - GORM 使用规范 │
|
||||
│ - 游标分页实现正确 │
|
||||
│ - LIKE 注入防护已处理 │
|
||||
└─────────────────────────────────────────────────────┘
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 四、安全评估
|
||||
|
||||
| 安全点 | 状态 | 说明 |
|
||||
|--------|------|------|
|
||||
| 密码哈希算法 | ✅ 优秀 | Argon2id 配置合理 |
|
||||
| 随机数生成 | ✅ 优秀 | 全部 crypto/rand |
|
||||
| JWT JTI | ✅ 良好 | timestamp+random 格式 |
|
||||
| Token 轮换 | ✅ 良好 | 滚动轮换防重放 |
|
||||
| access_token 存储 | ✅ 良好 | 内存存储，非 localStorage |
|
||||
| CSRF 保护 | ✅ 良好 | 机制存在且已验证 |
|
||||
| 容器安全 | ✅ 良好 | 非 root 用户 |
|
||||
| JWT 密钥强制校验 | ⚠️ 缺口 | release 模式未见强制启动失败 |
|
||||
| 登录响应时序 | ✅ 已修复 | 常数时间比较 |
|
||||
| `GetUserRoles` 授权 | ✅ 已修复 | self/admin 验证已添加 |
|
||||
| 文件上传安全 | 🔴 Stub | `UploadAvatar` 未实现，无校验逻辑 |
|
||||
| gosec 扫描 | ❓ 未知 | `gosec-report.json` 存在但本次未分析 |
|
||||
|
||||
---
|
||||
|
||||
## 五、工程规范评估
|
||||
|
||||
### 5.1 Git 规范
|
||||
|
||||
- ✅ 提交信息格式规范（`feat:`/`fix:`/`test:`/`docs:` 前缀）
|
||||
- ✅ 功能分支隔离（`fix/status-review-sync-20260409`）
|
||||
- ⚠️ **行尾符污染**：15 个文件存在 LF/CRLF 混用，git 已在每次操作时发出警告，需要通过 `.gitattributes` 根治
|
||||
|
||||
### 5.2 文档一致性
|
||||
|
||||
- 🔴 **严重文档漂移**：`PROJECT_REAL_COMPLETION_REVIEW_2026-04-10.md` 声称 "Avatar Upload — 已实现且已验证"，实际代码为纯 stub（`"avatar upload not implemented"`）。文档与代码存在**直接矛盾**。
|
||||
- ✅ 有历史 Sprint 记录的习惯，审计链路清晰
|
||||
- 🟡 多份 Review 报告（24 个文件）存在重叠和相互矛盾的结论，容易造成认知混乱
|
||||
|
||||
### 5.3 测试规范
|
||||
|
||||
| 测试类型 | 状态 |
|
||||
|--------|------|
|
||||
| 后端单元测试 | ⚠️ 存在但覆盖率极低（15-28%）|
|
||||
| 后端集成测试 | ✅ 有 `internal/integration/` 包 |
|
||||
| 前端单元测试 | ✅ 325 测试通过，无 jsdom 噪声 |
|
||||
| E2E 测试 | ⚠️ 脚本存在但环境变量问题未解决 |
|
||||
| 性能测试 | ✅ 有 `internal/performance/` 包 |
|
||||
|
||||
---
|
||||
|
||||
## 六、前端质量评估
|
||||
|
||||
| 维度 | 状态 | 说明 |
|
||||
|------|------|------|
|
||||
| TypeScript 严格模式 | ✅ | tsconfig 启用 strict |
|
||||
| 构建 | ✅ | Vite 构建通过 |
|
||||
| Lint | ✅ | ESLint 通过，无错误 |
|
||||
| 单元测试 | ✅ | 325 测试，无噪声 |
|
||||
| jsdom 噪声 | ✅ | 已修复（window.alert mock）|
|
||||
| 401 刷新机制 | ✅ | 单次刷新 + 并发锁 |
|
||||
| Token 存储 | ✅ | access_token 内存，refresh_token HttpOnly Cookie |
|
||||
| 设备信任 | ⚠️ | localStorage 持久化，但 device_id 为随机值 |
|
||||
| 响应格式处理 | 🟠 | 需适配不一致的后端响应格式 |
|
||||
|
||||
---
|
||||
|
||||
## 七、改进路线图
|
||||
|
||||
### 第一阶段：P0 修复（必须在下一个 PR 完成）
|
||||
|
||||
**优先级**：不修复不允许声称上线就绪
|
||||
|
||||
| # | 任务 | 预估工时 | 负责人 |
|
||||
|---|------|----------|--------|
|
||||
| 1 | 实现真实的 `UploadAvatar` Handler（文件校验+存储+错误清理） | 3h | 后端 |
|
||||
| 2 | 添加 Service 层 `UploadAvatar` 方法 | 1h | 后端 |
|
||||
| 3 | 将 `AdminRoleID` 从硬编码改为动态查询 role code | 1h | 后端 |
|
||||
| 4 | 更新文档，同步真实状态（删除虚假"已验证"结论） | 0.5h | 全体 |
|
||||
|
||||
### 第二阶段：P1 架构修复（本周完成）
|
||||
|
||||
| # | 任务 | 预估工时 | 团队收益 |
|
||||
|---|------|----------|----------|
|
||||
| 1 | 为 Repository 层提取接口（UserRepository/RoleRepository 等） | 4h | 解锁 Service 单元测试，覆盖率可从 15% → 60%+ |
|
||||
| 2 | 用 DB 事务包装 `AssignRoles` 的删旧建新操作 | 1h | 消除数据竞争窗口 |
|
||||
| 3 | 为 `GetUserRoles` / `ListAdmins` 提供批量查询方法（消除 N+1） | 2h | 性能提升 |
|
||||
| 4 | 统一 Handler 响应格式（全部使用 code/message/data 结构） | 2h | 前端联调质量提升 |
|
||||
| 5 | release 模式下 JWT secret 空值强制启动失败 | 0.5h | 消除安全漏洞 |
|
||||
|
||||
### 第三阶段：P2 工程规范（本月完成）
|
||||
|
||||
| # | 任务 | 预估工时 |
|
||||
|---|------|----------|
|
||||
| 1 | 添加 `.gitattributes` 统一行尾符（LF） | 0.5h |
|
||||
| 2 | 将 `internal/pagination` 包覆盖率从 0% 提升至 80%+ | 2h |
|
||||
| 3 | 将 Handler/Service 覆盖率目标提升至 60%（通过接口+mock 解锁） | 8h |
|
||||
| 4 | 解析 `gosec-report.json`，修复 SEC 级别问题 | 2h |
|
||||
| 5 | 整合多份 Review 文档，归档旧版，保留单一权威状态文档 | 1h |
|
||||
|
||||
---
|
||||
|
||||
## 八、团队技术能力提升建议
|
||||
|
||||
基于本次 Review，针对团队现状提出以下系统性建议：
|
||||
|
||||
### 8.1 必须立即建立的编码规范
|
||||
|
||||
**规范 1：Service 层必须面向接口编程**
|
||||
```go
|
||||
// ❌ 错误做法（当前状态）
|
||||
type UserService struct {
|
||||
userRepo *repository.UserRepository
|
||||
}
|
||||
|
||||
// ✅ 正确做法
|
||||
type UserRepository interface {
|
||||
GetByID(ctx context.Context, id int64) (*domain.User, error)
|
||||
Create(ctx context.Context, user *domain.User) error
|
||||
}
|
||||
|
||||
type UserService struct {
|
||||
userRepo UserRepository
|
||||
}
|
||||
```
|
||||
|
||||
**规范 2：多步数据库操作必须用事务**
|
||||
```go
|
||||
// ❌ 危险做法（当前状态）
|
||||
s.userRoleRepo.DeleteByUserID(ctx, userID) // 失败后下面不执行
|
||||
s.userRoleRepo.BatchCreate(ctx, userRoles) // 成功但上面失败 → 数据不一致
|
||||
|
||||
// ✅ 正确做法
|
||||
db.Transaction(func(tx *gorm.DB) error {
|
||||
if err := roleRepo.WithTx(tx).DeleteByUserID(ctx, userID); err != nil {
|
||||
return err // 自动回滚
|
||||
}
|
||||
return roleRepo.WithTx(tx).BatchCreate(ctx, userRoles)
|
||||
})
|
||||
```
|
||||
|
||||
**规范 3：文档必须与代码同步，禁止超前声称**
|
||||
- 合并门禁：PR 描述中的"已实现"必须附带 grep 证据或测试截图
|
||||
- 函数体内有 `"not implemented"` 字符串的接口，不允许在文档中标注为"已实现"
|
||||
|
||||
### 8.2 测试文化建设
|
||||
|
||||
当前团队测试覆盖率极低（核心层 15%）的根本原因是**架构不支持测试**——Service 依赖具体类型导致无法 Mock。
|
||||
|
||||
建立以下测试规范：
|
||||
|
||||
1. **新功能必须先写测试**（TDD）：不是要求 100% 覆盖，而是核心 happy path + 主要错误路径
|
||||
2. **单元测试必须可以离线运行**：不依赖真实数据库（通过接口+mock 实现）
|
||||
3. **覆盖率下限**：Service 层 ≥ 60%，Handler 层 ≥ 50%（当前目标，通过接口重构后可达）
|
||||
|
||||
### 8.3 代码 Review 要求（从下一个 PR 开始执行）
|
||||
|
||||
PR 描述必须包含：
|
||||
1. **变更原因**（1-2 句）
|
||||
2. **实际执行过的验证命令及输出**（不接受"应该通过"这种表述）
|
||||
3. **影响范围说明**（后端/前端/数据库结构）
|
||||
4. **Checklist**：
|
||||
- [ ] `go build ./...` 通过
|
||||
- [ ] `go vet ./...` 无警告
|
||||
- [ ] `go test ./... -short` 通过
|
||||
- [ ] 新增代码有对应测试
|
||||
- [ ] 文档已同步
|
||||
|
||||
---
|
||||
|
||||
## 九、诚实状态评估
|
||||
|
||||
基于本次实测，以下是可以诚实声称的状态：
|
||||
|
||||
### ✅ 可以诚实声称
|
||||
|
||||
- 后端全量测试通过（-short 模式）
|
||||
- `go build` / `go vet` 零错误
|
||||
- 前端 325 单元测试通过，lint/build 绿灯
|
||||
- Argon2id 密码安全、Token 机制、CSRF 保护已到位
|
||||
- 游标分页设计正确，P99 延迟满足 SLA（<100ms）
|
||||
- 非 root 容器、健康检查、WAL 模式已配置
|
||||
|
||||
### ❌ 不可以声称
|
||||
|
||||
- "Avatar Upload 已实现" — **虚假，Handler 是 stub**
|
||||
- "核心业务逻辑有充分测试保护" — Handler/Service 覆盖率 15%，远不充分
|
||||
- "架构设计符合 DIP 原则" — Service 依赖具体类型，违反 DIP
|
||||
- "E2E 主入口已验证" — 脚本存在环境变量问题，未完成完整验证
|
||||
- "项目达到上线标准" — P0 问题（Stub 谎报）未解决
|
||||
|
||||
---
|
||||
|
||||
## 十、附：资深工程师给团队的话
|
||||
|
||||
这个项目整体基础不差——安全加固方向是对的，游标分页的工程思维体现了对性能的重视，Sprint 制度的执行留下了清晰的审计链。这些都是值得保持的好习惯。
|
||||
|
||||
但有一个模式需要立即纠正：**文档超前于代码**。当"已实现"写进文档但代码是 stub 时，信任就会崩塌。上面的 UploadAvatar 例子说明了这一点——文档甚至列出了测试场景（401/403），但测的是一个永远返回 200 的 stub。这不是 TDD，这是文档驱动的自我欺骗。
|
||||
|
||||
**核心修炼方向**：
|
||||
1. 代码会说话，文档只是辅助——先有代码，再有结论
|
||||
2. 面向接口编程是解锁高覆盖率测试的钥匙，不是"以后再说"的事
|
||||
3. 事务不是可选项，多步数据库操作必须原子
|
||||
|
||||
---
|
||||
|
||||
**Review 完成时间**：2026-04-10 23:50
|
||||
**下次 Review 建议**：完成 P0 修复 + 接口重构后，再次评估覆盖率和架构健康度
|
||||
|
||||
Reference in New Issue
Block a user