quality-code-review

代码质量审查

供 Frappe 应用审查者使用的检查清单。按后果严重程度依次保障正确性、安全性和未来的维护者。以下各节也按此顺序排列——请将大部分注意力放在 §1 和 §2。优先修复根本原因，而非采用权宜之计，并说明问题为何重要（会导致什么故障、影响哪些人）。

---

1. 正确性与稳定性（后果最严重）

最严重的 Bug 是悄无声息的状态数据损坏——过账的分类账／库存数据错误，却没有报错。将有状态代码以及法律、会计和合规相关代码视为**“绝不能失败”**的代码。

• 尽早、大声地失败。 使用断言检查内部不变量： assert total_credit == total_debit.（断言用于代码保证成立的不变量，而非面向用户的验证。）
• 设想代码会如何出错。 对每项更改都要问：可能在哪里出错？别人会如何误用？ 编写不易出错的代码。扩展和覆盖尤其应考虑处理各种失败情况。
• 禁止部分提交。 在事务中途意外调用 frappe.db.commit() / db.rollback() 会结束事务并暴露部分状态——发现任何此类调用都要标记。绕过验证进行提交／保存（验证失败时仍发布了没有 SL/GL 分录的文档）属于严重 Bug。
• 不要为了方便而牺牲原子性（例如，添加 autocommit 来修复引导过程，会让事务失去原子性）。autocommit 只应用于创建架构。
• 维护不变量优先于用户体验。 “可以牺牲用户体验，但必须保证正确性。”不要为了迁就有问题的代码而破坏原本正常工作的代码。
• 修复前先验证问题。 有时正确的做法是“不要修复”。先找出根本原因。
• 注意筛选条件为空或为 None 时的破坏性数据库 API。 set_value("Site", None, ...) / 不带筛选条件的 db.delete 会更新／删除所有行。这些操作必须报错，而不能悄悄对整张表执行。对于任何 set_value/delete/get_value，只要名称／筛选条件可能为 None、为空或由攻击者控制，就应标记。
• 检查条件中的类型是否匹配。 类型不匹配的比较（字符串与 datetime、字符串与 int）会悄悄地始终不匹配或始终为真——应在边界处显式转换（cint/flt）。
• 确认返回值的形状。 对结果进行索引前，先确认它是否可能为 None / [None, None]；注意区分 as_dict=1（字典列表）与标量。
• 不要悄悄改变长期沿用的语义。 调用方长期依赖的行为就是一种契约——即使签名没有改变，修改这种行为也会伪装成非破坏性更改，实则造成破坏性变更。

2. 安全

避免漏洞远比安全地修复漏洞容易。应格外严格地审计安全关键代码（身份验证、授权、权限、用户管理）。

注入

• 绝不通过字符串拼接或 f-string 构造 SQL。应使用 ORM 或查询构造器。如果无法避免原始 SQL，应使用参数替换（frappe.db.sql("... where name = %s", (user,))）——绝不要自行插入参数。更好的做法是，完全避免引入新的原始 SQL：除了注入风险，它还会将代码绑定到单一数据库，而框架旨在保持数据库无关（支持 Postgres）。优先使用 frappe.qb。
• 即使使用 ORM，类型混淆也是一种注入途径。 Frappe 接受复杂类型，因此预期为字符串的参数可能以筛选列表的形式传入：将 {"key": ["!=", ""]} 传给 db.get_value，就能绕过密钥检查。在信任边界验证输入的类型——显式使用 isinstance(key, str)。审计每个 @frappe.whitelist 方法。
• 绝不要自行 eval/exec 任何内容。仅使用 safe_eval/safe_exec，且要严格限制使用量；并且“safe_exec 并非万能”。绝不要接受客户端提供的方法路径并执行。

沙箱与信任边界

• 沙箱执行（RestrictedPython/safe_exec）并不可靠安全——应假定存在逃逸方式。安全开关必须位于正确的信任边界：服务器脚本的启用配置必须在 bench 级别，绝不能在站点级别（租户可能启用它并控制整台服务器）。
• 优先使用允许列表，而非禁止列表——禁止列表可以被绕过。不要默认开放所有内容。

访问控制

• “在设置 allow_guest=True 之前三思”——这不是绕过真正身份验证/授权的捷径。网页必须先应用权限，再读取或共享数据。优先使用 get_list/get_all，而非手写查询。
• 精确限定放宽范围。 验证速率限制/权限例外恰好针对预期主体，而不是例如所有非访客用户。

路径遍历/文件系统

• 优先使用 File doctype API。如果用户输入进入路径，确保它无法遍历（/../../）到站点文件夹之外。

加密/密钥

• 绝不要自行实现加密；应复用现有实现。验证访客/Webhook 请求的真实性（HMAC）。
• 签名/一次性 URL：使用真正保密的签名值；同时通过时间和首次使用来过期；仅使用 URL 验证，不要使用合并后的表单数据（Frappe 会合并 URL 和表单数据 → 攻击者可利用一个有效签名发起重放攻击）。
• 将密钥存储在密码字段中；绝不能以纯文本存储，也绝不能在日志或错误消息中泄露密钥。

XSS 与 OWASP 其他风险

• 不要将用户输入注入 DOM。即使 XSS 看似微不足道，也应视为严重问题——HTML/JS 注入通常会导致账户被劫持。
• 不要通过清理并丢弃特殊字符来修复 XSS。优先在将值注入 DOM 前进行转义。

3. 性能就是正确性

在代码审查阶段修复性能问题的成本最低；合入的慢代码可能多年都未被发现。性能是一项功能（多尔蒂阈值；人类能感知约 100ms 的延迟）。

• **性能预算：**常见读取 < 100ms；读取 < 1s；大多数写入 < 5s；绝不超过约 10s。高频只读请求的 P99 应约为 1s。缓慢的同步请求会阻塞工作进程（队头阻塞）。
• **复杂度规则：**高频调用的端点必须执行 O(1) 或 O(log N) 的工作量——最坏情况下允许较大的常数因子，但绝不能是 O(N)。统计行数是 O(N)，不是 O(1)；对大型或经过筛选的表执行 COUNT(*) 开销很大。为无界扫描设定上限（例如最近 3 个月，或使用“1000+”哨兵值），不要扫描所有数据。
• 索引就是代码。标记任何针对无索引列的 WHERE/联接/筛选。索引（包括自定义索引）必须提交到代码中，不能临时应用——否则迁移时会丢失。加载表单时，如果要拉取评论/版本/分配信息，这些查询都必须有索引；其中一个查询没有索引，就会导致整个操作变慢。
• 循环中不要调用数据库。“不要编写会在循环中调用数据库的校验。”标记 N+1 模式。对稳定值（UOM、docstatus、status）进行缓存，而不是重新查询。这在后台任务中可以接受，但绝不能用于请求。
• **慢代码的“移除 → 减少 → 复用”阶梯：**如果无法修复慢代码，就移除它、减少调用次数，或进行记忆化。选择合适的缓存范围（ @redis_cache、@request_cache、@site_cache——最后一种如果过度使用，会大量占用内存）。不要在 frappe.local 或 frappe.flags 中手动实现缓存：“这样只会引入全新的缓存失效问题。”不要缓存成本微乎其微的工作。
• **内存：**不要把无用内容塞进共享的模块级文件 / __init__.py / 类级状态中——它们会一直驻留在内存中。移除未使用的模块级导入（将其移到使用它的函数中）。留意内存泄漏。
• **调整条件判断顺序，让数据库调用最后执行。**在布尔表达式中，先放开销低的内存检查，以便短路求值时完全跳过查询。
• **在 SQL 中聚合，而不是在 Python 中聚合。**在查询中使用 SUM()/COUNT()，而不是先获取所有行再在内存中汇总；将筛选条件推入子查询，使其在联接之前生效。
• **不要为了获取一个值而读取整个文档。**获取单列时，使用 get_value/get_single_value/set_value，而不是 get_doc().save()；使用 frappe.delete_doc，而不是 get_doc().delete()（后者只为删除文档就先读取了整个文档）。
• **热路径中不要使用 MyISAM 表。**读取 MyISAM 表会隐式锁定整个表——请求路径中绝不能访问这类表。
• **将耗时操作移至后台队列。**耗时操作应放入 enqueue(..., queue="long")，而不是放在会阻塞工作进程的同步请求中。

4. 并发

• 先检查再操作会产生竞态。if not frappe.db.exists(...): insert()——两个工作进程都可能看到“不存在”，然后都插入。优先使用数据库级唯一约束；“将完整性保障交给数据库。”
• **锁相关陷阱：**对无索引查询执行 SELECT ... FOR UPDATE 会锁定所有扫描到的行（以及间隙）——务必确保筛选条件使用索引，否则就会锁住整张表。锁定父项却不锁定其子项，会产生“变异体”文档。
• 全局可变状态 / 类属性在 Python 中都是全局的——共享的引擎/类属性若将查询状态泄漏到并发请求之间，就会产生垃圾结果。构建查询时应保持无状态。不要“在 frappe.local 里搞些奇怪的东西”——local 用于存放变量，不用于存放静态状态。

5. 可读性与可维护性

约 50% 的开发时间都花在阅读代码上；糟糕的代码最终会迫使团队重写。

• 能保持函数纯粹时就尽量保持纯粹——易于阅读和测试。
• 不要传递可变对象，指望别人填充它们（“组装”代码）——应返回新值。传递一个将被修改的可变对象，会迫使读者打开两个文件才能弄明白一件事。
• 修改输入的函数必须使用恰当的名称。
• 优先选择朴素的写法。 虽然函数式的 map-reduce 单行代码看起来很漂亮，但请直接写一个 4 行的 for 循环。可调试性优先于炫技。
• 一致性优先于个人风格。 一个代码库不应混杂 10 种风格。遵循周围的格式、命名和导入约定；发现变更破坏这些约定时应提出。
• 品味： 重构代码，让边缘情况成为常见情况，从而移除特殊分支。想想：能否更简单？代码能否更少？if-else 是否嵌套过深、杂乱无章？
• 优先扩展共享组件，而不是复制粘贴后各自演变。 同一功能分裂成 3–4 个实现，会拖慢长期开发速度。避免模块间紧密耦合；通过清晰且有文档说明的公共 API 集成。
• 使用文档字符串为公共模块、类和函数编写文档；优先使用类型注解，而不是用文字描述类型（“类型提示好 10 倍”）；类型检查器能发现不明显的错误。
• 文档字符串只应提及重要内容。保持简短、切中要点。不要解释从函数名称就能轻易理解的内容。重点说明“为什么”。
• 将无关变更拆分为不同的提交/PR，以便评审聚焦，并保持 git blame 和回滚记录清晰。

6. API 设计与向后兼容性

• 最小意外原则： API 的名称和签名应能传达约 90% 的意图；用户不应对其行为感到意外。
• 拒绝接受多种互不相关类型的宽泛/重载参数（字符串/字典/列表/None）。优先使用各司其职的独立函数。谨防隐式回退；使用明确的变体。“正确使用方式依赖圈内知识的 API 是一种负担。”
• 为扩展而设计，而不是为了覆盖。 提供钩子；绝不要在运行时对核心进行猴子补丁（“糟糕透顶”——会妨碍未来的修复），也绝不要复制整个核心文件只改几行（修复不会同步）。
• 对成熟的/公共 API，向后兼容是一项义务。遵循语义化版本；次要版本 = 零项破坏性变更。破坏性变更包括：移除公共函数/字段、调整参数顺序、新增必填参数、更改业务逻辑、移动/重命名文件（导致导入失效）、升级共享依赖。重命名但不保留旧名称作为别名，是一种不必要的破坏。每项破坏性变更都必须附带弃用警告和文档。
• 留意架构变更中的破坏性隐患： 在现有站点添加必填字段、将长期存在的字段设为唯一（需要数据补丁）、更改字段类型但不提供补丁、移除字段。
• 会静默跳过现有站点的架构变更需要数据补丁。 单例 DocType 不会将新字段的默认值同步到现有站点；更改字段类型（例如 text→int）也不会转换现有值——这两种情况都需要明确的补丁，并在已填充数据的站点上进行测试。
• 新参数应放在末尾，作为带安全默认值的关键字参数（使用 None，而不是 ""），以免破坏现有的位置参数调用。重命名时，应将旧名称保留为转接函数：def old_name(...): return new_name(...)。
• 补丁规范。 数据补丁必须幂等（可安全重复运行）、顺序正确（在所读取的字段/doctype 创建后运行），并位于正确的应用中（框架变更应在框架中添加补丁，而不是在下游应用中）。
• 修改现有测试是个危险信号。 如果要让变更通过测试就必须修改现有测试的断言，那你很可能破坏了真实工作流——应明确说明理由，而不是迁就测试。

7. 测试

• 每个 PR 都需要足够的测试覆盖率——关注差异部分的补丁覆盖率，而不只是项目覆盖率。（Frappe 的目标：差异部分中有 85% 的代码行得到覆盖。）测试应覆盖最常用的业务场景。
• **为每个修复添加回归测试。**如果修复 Bug 时没有添加能捕获该问题的测试，就可能导致回归。对于后果极其严重的代码（有状态／合规性相关），不要止步于示例测试——应使用基于属性的测试（Hypothesis）。
• **指出缺少迁移／数据补丁覆盖的情况。**架构变更和数据补丁是风险最高、测试最少的部分；如果变更涉及字段调整或数据迁移，就需要在数据已填充的真实站点上测试补丁（空表总能“成功迁移”，即使变更无效）。
• **测试必须具有确定性且相互独立。**不要使用 random（会导致测试不稳定）；不要依赖其他测试遗留的状态（避免顺序依赖）；对于依赖时间的逻辑，请使用 freeze_time。

8. 错误消息、日志记录与可观测性

• **错误消息质量是合理的审查项目。**标题必须具体且便于通过 Google 搜索（绝不能用“Message”／“Error”）。将字段名作为字段引用。说明发生了什么变化：哪一行、哪个字段，以及变化前后的值（1 → 2）。用户必须知道“数量从多少变成了多少？”
• 向受影响方说明故障——“只有当用户知道邮件设置已损坏时，邮件设置出问题才是用户的问题。”
• **记录日志。**保留回溯信息／异常上下文（能让调试容易几个数量级）。记录具有明确责任主体的破坏性／管理员操作（谁、何时、从哪里操作），并将日志持久化到临时容器之外。