Files
water/docs/code-review-standards.md
yuhaiming 70e2356f22 fix(app): 修复 AppController 安全与查询问题
- 加强异常处理、类型安全和图片上传校验
- 优化设备相关查询,避免重复访问数据源
- 补充并记录并发测试与审查修复实施计划
2026-07-17 08:20:44 +08:00

24 KiB
Raw Blame History

Water 项目代码审查标准与流程

版本: 1.0 | 适用项目: water (IoT 智能灌溉设备管理系统) | 技术栈: Spring Boot 3.5 + Java 17 + MyBatis Plus + MQTT + Redis


目录

  1. 代码审查标准
  2. 代码审查流程
  3. 代码审查检查清单
  4. 静态分析工具集成方案
  5. 附录water 项目典型问题案例

1. 代码审查标准

1.1 问题分级体系

所有审查意见必须标注严重等级,审查者不得给出无分级的意见:

等级 标记 含义 处理要求
Blocker 🔴 存在安全漏洞、数据丢失风险、破坏 API 契约、竞态条件 必须修复后才能合并
Suggestion 🟡 缺失输入校验、命名混乱、缺少测试、性能问题、重复代码 应当修复,特殊情况可延期
Nit 💭 风格不一致、命名微调、文档缺失 建议改进,不阻塞合并

1.2 架构规范

🔴 Blocker

  • 禁止跨层调用Controller 不得直接操作 Mapper/数据库Service 不得返回 HttpEntity/ResponseEntity 等 Web 层对象
  • 禁止在 Controller 中写业务逻辑Controller 只做参数接收、校验、调用 Service、组装返回值。逻辑超过 5 行的方法必须下沉到 Service
  • 循环依赖必须根治:禁止用 @Lazy @Autowired 掩盖循环依赖。如果存在循环依赖,说明模块划分有问题,应重构拆分

water 项目现状MqttCommandAckService@Lazy @Autowired MqttClientManager 是典型的循环依赖掩盖。审查中遇到此类代码应标记为 🟡 Suggestion要求补充重构计划

  • 单一职责:单个 Controller/Service 类行数不得超过 500 行。超过的必须拆分

water 项目现状AppController 达 940 行,包含设备、排程、浇水记录、用户、统计、版本等全部业务接口,是典型的上帝类。新代码不得再向此类添加方法

🟡 Suggestion

  • 接口与实现分离Service 必须定义接口(IXxxService+ 实现(XxxServiceImplController 依赖接口
  • 依赖注入使用构造器注入:使用 @RequiredArgsConstructor + final 字段,禁止字段注入 @Autowired(除 @Lazy 场景外)
  • 配置类使用 @ConfigurationProperties:禁止用 @Value 散落配置读取

1.3 命名规范

🔴 Blocker

  • 禁止 Raw Type:集合必须使用泛型。Map map = new HashMap() 必须写为 Map<String, Object> map = new HashMap<>()

water 项目现状AppController 中存在 Map retMap = new HashMap()List l = new ArrayList<>()List deviceList = new ArrayList() 等大量 Raw Type 用法。新代码零容忍

  • 变量名不得使用单字母:除循环变量 i/j/k 外,变量名必须有意义。List l 必须写为 List<AppDeviceVo> deviceList

🟡 Suggestion

  • 接口与实现的命名一致:注入字段名应与接口名对应。IAppScheduleService 注入为 appScheduleService,不得出现 appScheduleServiceimpl(小写 impl
  • 布尔变量用 is/has/can 前缀flagisSwitchSuccesshasPermission
  • 常量全大写下划线DEVICE_STATUS_LOCK_PREFIX,禁止魔法值

💭 Nit

  • 方法名动词开头getDeviceInfobindDeviceassertDeviceOwned
  • 包名全小写:不得使用大写字母或下划线

1.4 异常处理规范

🔴 Blocker

  • 禁止 catch (Exception e) 吞异常Controller 中不得使用 try-catch(Exception) 包裹所有逻辑。应依赖全局异常处理器(GlobalExceptionHandler)统一处理

water 项目现状AppController 几乎每个方法都被 try { ... } catch (Exception e) { return fail(e); } 包裹。这会:

  1. 吞掉异常栈,排查问题困难
  2. 与框架全局异常处理器功能重复
  3. 代码冗余严重

正确做法:移除 try-catch让异常自然抛出由全局处理器捕获。仅在需要业务降级或资源清理时才 catch

  • catch 块不得为空catch (Exception e) {} 是严重 bug。至少要记录日志
  • 不得 catch 后丢弃异常信息catch (Exception e) { log.error("出错"); } 丢失了堆栈,应使用 log.error("出错", e)

🟡 Suggestion

  • 异常分类处理:业务异常用 ServiceException/UserException,系统异常让其传播
  • 自定义异常携带上下文:抛出异常时包含设备编号、用户 ID 等关键信息

1.5 安全规范

🔴 Blocker

  • SQL 注入防护:所有数据库查询必须使用 MyBatis Plus 的 LambdaQueryWrapper 或参数化查询,禁止字符串拼接 SQL
  • 所有接口必须有权限控制:使用 @SaCheckLogin/@SaCheckPermission/@SaCheckRole 或在方法内校验用户所有权

water 项目正面案例AppControllerassertDeviceOwned(deviceNo) 校验设备归属权,这个模式是对的

  • 敏感数据不得出现在日志中密码、token、手机号完整信息等不得记录到日志
  • TLS 证书校验不得全局关闭TrustAllManager 等信任所有证书的代码必须有明确的安全边界注释,且仅限内网测试环境

water 项目现状MqttClientManager.createTrustAllSslContext() 创建了信任所有证书的 TrustManager。如果用于生产环境是 🔴 Blocker

  • 输入校验所有外部输入HTTP 参数、MQTT 消息体)必须做格式校验和长度限制

1.6 性能规范

🔴 Blocker

  • 禁止 N+1 查询:在循环中查询数据库/Redis 必须改为批量查询

water 项目现状MqttCommandAckService.findPendingCommandsByDeviceNo() 遍历所有 pending commandId 逐个查 Redis当 pending 命令多时性能差。应改用 Redis 的 MGET 或 Hash 结构批量获取

AppController.scheduleDeviceList() 在 for 循环中逐个设备调用 appSchedulingDeviceService.findByDeviceNo(),是典型的 N+1 查询

🟡 Suggestion

  • 批量操作优先deleteDevice 中的 for (deviceNo) { assertDeviceOwned(deviceNo); } 应改为批量查询
  • 避免在热路径创建重对象new SimpleDateFormat() 应替换为 DateTimeFormatter(线程安全且无需重复创建)

water 项目现状AppController.switchDevice()new SimpleDateFormat("yyyy-MM-dd HH:mm:ss").format(new Date()) 每次请求都创建新实例。应改为:

private static final DateTimeFormatter DATE_TIME_FORMATTER =
    DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss");
// 使用
String startTime = LocalDateTime.now().format(DATE_TIME_FORMATTER);
  • Redis Key 设置 TTL:所有写入 Redis 的缓存必须设置过期时间,防止内存泄漏

1.7 代码整洁规范

🟡 Suggestion

  • 删除注释掉的代码:被注释的代码应删除,版本历史由 Git 管理。// @SaCheckRole("appadmin") 这类应清理
  • 魔法值提取为常量或枚举:状态值 "0"/"1" 应定义为枚举

water 项目正面案例MqttCommandAckService 中的 CommandLockResult 枚举是好的实践。命令状态 "0"/"1" 也应类似处理

  • 重复代码提取:相同逻辑出现 3 次以上必须提取为公共方法

water 项目现状DeviceCommandServiceImpl.parseStartTime() 有 95 行,包含三段几乎相同的 try-catch 块解析不同时间格式,应提取为通用方法

💭 Nit

  • 方法行数不超过 50 行:超过的考虑拆分
  • 方法参数不超过 5 个:超过的考虑封装为参数对象
  • import 不得使用通配符import java.util.* 应写明具体类

1.8 并发规范

🔴 Blocker

  • 共享可变状态必须加锁:多线程访问的可变字段必须使用同步机制保护
  • 锁必须设置超时tryLock() 必须带超时参数,禁止无限等待导致死锁

water 项目正面案例MqttCommandAckService.withCommandLock() 使用 Redisson 分布式锁并设置了超时,这是正确的做法

  • SimpleDateFormat 非线程安全:禁止在多线程环境共享 SimpleDateFormat 实例

🟡 Suggestion

  • 优先使用不可变对象:能声明 final 的就声明 final
  • 优先使用并发集合ConcurrentHashMap 代替 HashMap + synchronized

1.9 测试规范

🟡 Suggestion

  • 核心业务逻辑必须有单元测试Service 层的公共方法和 MQTT Handler 必须有测试覆盖
  • 测试命名规范methodName_scenario_expectedResult,如 handleAck_validPayload_ackConfirmed
  • 测试必须独立:不依赖执行顺序,不依赖外部状态

water 项目正面案例MqttCommandAckService 的静态方法 ackMatchesPendingCommandresolveMissingCommandId 被设计为可测试的纯函数,这是好的实践

1.10 日志规范

🔴 Blocker

  • 日志必须包含上下文MQTT 相关日志必须包含设备编号、命令编号等关键信息

water 项目正面案例log.warn("[MQTT] 收到空 ACK 设备编号={}", deviceNo) 包含了设备编号,格式统一

🟡 Suggestion

  • 日志级别正确使用ERROR 用于系统异常、WARN 用于业务异常、INFO 用于关键业务流程、DEBUG 用于调试信息
  • 使用占位符而非字符串拼接log.info("设备{}上线", deviceNo) 而非 log.info("设备" + deviceNo + "上线")

2. 代码审查流程

2.1 角色定义

角色 职责 资质要求
提交者 (Author) 编写代码、自测、提交 PR、响应审查意见 熟悉项目编码规范
审查者 (Reviewer) 审查代码质量、提出改进意见、确认修复 熟悉相关模块业务逻辑
合并者 (Merger) 最终确认、合并代码 技术负责人或模块 Owner

2.2 PR 提交前(自检阶段)

提交者在创建 PR 前必须完成:

  1. 本地编译通过mvn clean compile -DskipTests
  2. 单元测试通过mvn test(不得使用 -DskipTests
  3. 静态分析通过Checkstyle + SpotBugs 无 Error 级别问题
  4. 自检 Checklist:逐项核对 Section 3 检查清单
  5. PR 描述:包含变更说明、测试方式、影响范围

PR 描述模板

## 变更说明
<!-- 简述本次改了什么、为什么改 -->

## 测试方式
<!-- 如何验证本次变更 -->

## 影响范围
<!-- 影响哪些模块/功能 -->

## 关联 Issue
<!-- Closes #xxx -->

2.3 审查阶段

审查时间要求

PR 规模 审查时限 说明
小型 (< 100 行) 4 小时内 单个 bugfix 或小功能
中型 (100-500 行) 1 个工作日内 常规功能开发
大型 (> 500 行) 2 个工作日内 大功能或重构,建议拆分

审查步骤

Step 1: 全局审视
  ├── PR 描述是否清晰?
  ├── 变更范围是否合理?
  └── 是否有对应的测试?

Step 2: 逐文件审查
  ├── 按 [Section 3 检查清单] 逐项核对
  ├── 标注问题等级 (🔴/🟡/💭)
  └── 给出具体的修改建议和原因

Step 3: 运行验证
  ├── 代码能否编译通过?
  ├── 测试是否通过?
  └── 静态分析是否有新问题?

Step 4: 总结
  ├── Approve — 无 Blocker可合并
  ├── Request Changes — 有 Blocker 或多个 Suggestion
  └── Comment — 仅有 Nit 或讨论性问题

2.4 合并标准

PR 必须满足以下全部条件才能合并:

  • 🔴 Blocker:所有 Blocker 已修复
  • Suggestion 有明确处理:已修复或标注为延期(需技术负责人确认)
  • 至少 1 个 Approve:中型以上 PR 需要模块 Owner Approve
  • CI 通过:编译、测试、静态分析全部通过
  • 无未解决的讨论:所有 review comment 已 resolved

2.5 冲突升级机制

冲突类型 升级路径
审查意见分歧 提交者与审查者协商 → 协商不成由模块 Owner 裁决
架构方案分歧 模块 Owner → 技术负责人(你)最终裁决
紧急修复绕过审查 需技术负责人批准,事后 24 小时内补审查

2.6 紧急修复流程

生产环境紧急 bug 修复可走快速通道:

  1. 技术负责人批准走紧急流程
  2. 最少 1 人审查(可简化为只查 Blocker 级别问题)
  3. 合并后 24 小时内补完整审查
  4. 记录在 docs/hotfix-log.md

3. 代码审查检查清单

审查者逐项核对,每项标记 通过 / 未通过 / 不适用

3.1 安全 (Security)

# 检查项 等级
S1 所有数据库查询使用参数化查询,无 SQL 注入风险 🔴
S2 所有接口有权限控制(@SaCheckLogin / 所有权校验) 🔴
S3 敏感数据密码、token不出现在日志中 🔴
S4 外部输入HTTP 参数、MQTT 消息)做了格式校验和长度限制 🔴
S5 TLS 证书校验未在生产环境关闭 🔴
S6 文件上传做了类型和大小限制 🟡

3.2 正确性 (Correctness)

# 检查项 等级
C1 代码逻辑是否实现了预期功能 🔴
C2 边界条件是否处理(空值、空集合、零值、最大值) 🔴
C3 异常路径是否正确处理(不吞异常、不丢失异常栈) 🔴
C4 并发场景下数据一致性是否保证 🔴
C5 Redis Key 都设置了 TTL 🟡
C6 分布式锁设置了超时时间 🟡

3.3 架构 (Architecture)

# 检查项 等级
A1 Controller 不含业务逻辑(仅参数接收+调用 Service 🔴
A2 无跨层调用Controller 不直接调 Mapper 🔴
A3 无循环依赖(未使用 @Lazy 掩盖) 🔴
A4 单个类行数不超过 500 行 🟡
A5 Service 有接口定义 🟡
A6 使用构造器注入(@RequiredArgsConstructor 🟡

3.4 代码质量 (Code Quality)

# 检查项 等级
Q1 集合使用泛型,无 Raw Type 🔴
Q2 无魔法值(字符串/数字硬编码),常量已提取 🟡
Q3 无注释掉的代码 🟡
Q4 重复代码已提取为公共方法 🟡
Q5 变量命名有意义,无单字母变量 🟡
Q6 方法行数不超过 50 行 💭
Q7 import 无通配符 💭

3.5 性能 (Performance)

# 检查项 等级
P1 无 N+1 查询(循环内不查数据库/Redis 🔴
P2 批量操作使用批量接口 🟡
P3 热路径无重对象创建(如 SimpleDateFormat 🟡
P4 大集合操作考虑分页或流式处理 🟡

3.6 异常处理 (Error Handling)

# 检查项 等级
E1 catch (Exception e) 包裹全部逻辑 🔴
E2 catch 块不为空,至少记录日志 🔴
E3 日志使用 log.error("msg", e) 保留异常栈 🔴
E4 业务异常用 ServiceException,不混用 🟡

3.7 测试 (Testing)

# 检查项 等级
T1 核心业务逻辑有单元测试 🟡
T2 测试覆盖正常路径和异常路径 🟡
T3 测试命名规范,能表达意图 💭
T4 测试独立运行,不依赖顺序 🟡

3.8 日志 (Logging)

# 检查项 等级
L1 关键操作有日志记录 🟡
L2 日志包含上下文信息设备号、用户ID等 🟡
L3 日志级别使用正确 💭
L4 使用占位符而非字符串拼接 💭

4. 静态分析工具集成方案

4.1 工具选型

工具 作用 集成方式 优先级
Checkstyle 代码风格检查命名、import、行数 Maven 插件 P0 立即集成
SpotBugs 潜在 bug 检测(空指针、资源泄漏) Maven 插件 P0 立即集成
SonarQube 综合质量平台(重复率、覆盖率、复杂度) 独立服务 + Scanner P1 二期集成

4.2 Maven 集成配置

在根 pom.xml<build><plugins> 中添加以下配置:

<!-- Checkstyle: 代码风格检查 -->
<plugin>
    <groupId>org.apache.maven.plugins</groupId>
    <artifactId>maven-checkstyle-plugin</artifactId>
    <version>3.5.0</version>
    <dependencies>
        <dependency>
            <groupId>com.puppycrawl.tools</groupId>
            <artifactId>checkstyle</artifactId>
            <version>10.18.0</version>
        </dependency>
    </dependencies>
    <configuration>
        <configLocation>checkstyle.xml</configLocation>
        <consoleOutput>true</consoleOutput>
        <failsOnError>true</failsOnError>
        <includeTestSourceDirectory>true</includeTestSourceDirectory>
    </configuration>
    <executions>
        <execution>
            <id>validate</id>
            <phase>validate</phase>
            <goals>
                <goal>check</goal>
            </goals>
        </execution>
    </executions>
</plugin>

<!-- SpotBugs: 静态 bug 分析 -->
<plugin>
    <groupId>com.github.spotbugs</groupId>
    <artifactId>spotbugs-maven-plugin</artifactId>
    <version>4.8.6.4</version>
    <dependencies>
        <dependency>
            <groupId>com.github.spotbugs</groupId>
            <artifactId>spotbugs</artifactId>
            <version>4.8.6</version>
        </dependency>
    </dependencies>
    <configuration>
        <effort>Max</effort>
        <threshold>Low</threshold>
        <failOnError>true</failOnError>
        <excludeFilterFile>spotbugs-exclude.xml</excludeFilterFile>
    </configuration>
    <executions>
        <execution>
            <id>spotbugs-check</id>
            <phase>verify</phase>
            <goals>
                <goal>check</goal>
            </goals>
        </execution>
    </executions>
</plugin>

4.3 实施路线

阶段 周期 目标
Phase 1 第 1 周 集成 Checkstyle配置规则文件CI 中运行
Phase 2 第 2 周 集成 SpotBugs修复现有 High 级别问题
Phase 3 第 3-4 周 搭建 SonarQube 服务,建立质量基线
Phase 4 持续 将静态分析纳入 PR 检查,设置质量门禁

4.4 过渡策略

重要:集成静态分析工具时,现有代码会有大量违规。采取以下策略:

  1. 新代码零容忍PR 中新增/修改的代码必须通过检查
  2. 存量代码分期修复:用 SonarQube 的 "New Code" 模式,只关注新增问题
  3. 基线快照:记录当前问题数量作为基线,要求只减不增

5. 附录water 项目典型问题案例

案例一:上帝控制器 + 异常吞噬

文件: AppController.java 问题等级: 🔴 Blocker (架构) + 🔴 Blocker (异常处理)

// ❌ 当前代码 — 940 行上帝类,每个方法 try-catch 包裹
@PostMapping("/bindDeviceStatus")
public R<Map> bindDeviceStatus(@RequestBody String body) {
    try {
        AppDeviceBo appDevice = JsonUtils.parseObject(body, AppDeviceBo.class);
        appDevice.setUserId(LoginHelper.getUserId());
        Map<String, Object> map = appDeviceService.bindDeviceStatus(appDevice);
        return R.ok(map);
    } catch (Exception e) {
        return fail(e);  // 吞异常栈,与全局处理器重复
    }
}
// ✅ 改进后 — 移到独立 Controller移除 try-catch
@RestController
@RequestMapping("/app/v1/device")
@RequiredArgsConstructor
public class AppDeviceCommandController extends BaseController {

    private final IAppDeviceService appDeviceService;

    @ApiEncrypt
    @PostMapping("/bindDeviceStatus")
    public R<Map<String, Object>> bindDeviceStatus(@RequestBody String body) {
        AppDeviceBo appDevice = JsonUtils.parseObject(body, AppDeviceBo.class);
        appDevice.setUserId(LoginHelper.getUserId());
        return R.ok(appDeviceService.bindDeviceStatus(appDevice));
    }
}

案例二Raw Type + 魔法值

文件: AppController.java 问题等级: 🔴 Blocker (Raw Type) + 🟡 Suggestion (魔法值)

// ❌ 当前代码
Map retMap = new HashMap();                    // Raw Type
List l = new ArrayList<>();                    // 单字母变量 + 推断丢失泛型
String status = map.get("workStatus");         // 魔法字符串
ack.setStatus("1");                            // 魔法值
// ✅ 改进后
Map<String, Object> resultMap = new HashMap<>();
List<AppDeviceVo> availableDevices = new ArrayList<>();
String status = params.get(DeviceCommand.FIELD_WORK_STATUS);
ack.setStatus(CommandStatus.SUCCESS.getCode());  // 枚举

案例三N+1 查询

文件: AppController.javascheduleDeviceList() 方法 问题等级: 🔴 Blocker (性能)

// ❌ 当前代码 — 循环内逐个查询
for (AppDeviceVo appDeviceVo : appDeviceVos) {
    List<AppSchedulingDeviceVo> scheduleDevices =
        appSchedulingDeviceService.findByDeviceNo(appDeviceVo.getDeviceNo()); // N+1!
    if (scheduleDevices.size() == 0) {
        appDeviceVoList.add(appDeviceVo);
    }
}
// ✅ 改进后 — 批量查询
List<String> deviceNos = appDeviceVos.stream()
    .map(AppDeviceVo::getDeviceNo)
    .collect(Collectors.toList());
Set<String> scheduledDeviceNos = appSchedulingDeviceService
    .findScheduledDeviceNos(deviceNos);  // 一次批量查询
List<AppDeviceVo> availableDevices = appDeviceVos.stream()
    .filter(d -> !scheduledDeviceNos.contains(d.getDeviceNo()))
    .collect(Collectors.toList());

案例四:线程安全问题

文件: AppController.javaswitchDevice() 方法 问题等级: 🔴 Blocker (线程安全)

// ❌ 当前代码 — SimpleDateFormat 非线程安全
String startTime = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss").format(new Date());
// ✅ 改进后 — 使用 DateTimeFormatter线程安全
private static final DateTimeFormatter DATE_TIME_FORMATTER =
    DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss");

String startTime = LocalDateTime.now().format(DATE_TIME_FORMATTER);

案例五:正面案例 — MQTT 消息分发策略模式

文件: MqttMessageDispatcher.java + MqttTopicHandler.java 评价: 🟢 优秀设计

// 策略接口
public interface MqttTopicHandler {
    String getTopicPattern();
    void handle(String topic, String payload);
}

// 分发器 — 新增 Topic 处理器零改动
// 这是开闭原则的标准实践,值得团队学习

值得推广的模式

  1. 策略模式解耦消息分发
  2. 有界队列 + 多消费者的高并发架构
  3. 优雅停机设计(DisposableBean + awaitTermination
  4. 分布式锁保护并发操作

6. 推广与落地建议

6.1 分阶段实施

阶段 时间 目标 负责人
宣贯 第 1 周 团队学习本文档,理解审查标准 技术负责人
工具 第 2 周 集成 Checkstyle + SpotBugs修复 P0 问题 全员
试运行 第 3-4 周 所有 PR 执行审查流程,以学习为主 全员
正式执行 第 5 周起 严格执行 Blocker 零容忍 全员

6.2 度量指标

指标 目标 度量方式
PR 审查覆盖率 100% 所有合并的 PR 都经过审查
Blocker 修复率 100% 所有 Blocker 在合并前修复
平均审查响应时间 < 1 工作日 PR 提交到首次审查
静态分析问题数 逐月递减 SonarQube 趋势图
单元测试覆盖率 核心模块 > 60% JaCoCo 报告

6.3 持续改进

  • 每月复盘:审查中发现的共性问题汇总,更新本标准
  • 季度培训:针对高频问题组织技术分享
  • 标准迭代:本文档每季度评审一次,根据团队反馈调整

文档维护:技术负责人 | 最后更新2026-07-16