frappe-code-review

品質を重視したコードレビュー

Frappe アプリケーションのレビューチェックリストです。優先度の高い順に、正しさ、セキュリティ、将来の保守担当者を守ります。以下のセクションもこの順に並んでいます。§1 と §2 に最も注意を払ってください。回避策より根本原因の修正を優先し、指摘が重要な理由（何が、誰にとって壊れるのか）を伝えてください。

---

1. 正しさと安定性（最も重要）

最悪のバグは、気付かれないまま状態が破損することです。誤った元帳や在庫がエラーなしで計上されます。状態を変更するコードや、法務・会計・コンプライアンスに関わるコードは、**「失敗が許されない」**コードとして扱ってください。

• 早期に、明確に失敗させる。 内部の不変条件にはアサーションを使います。 assert total_credit == total_debit。（アサーションはコードが保証する不変条件に使うもので、ユーザー向けの検証には使いません。）
• どう壊れるかを想像する。 変更ごとに、どこで壊れる可能性があるか。誰かがどう誤用する可能性があるか。 を考えます。間違いが起きないコードを書いてください。拡張機能やオーバーライドでは、特にあらゆる種類の障害への対処を検討する必要があります。
• 部分的なコミットをしない。 トランザクションの途中にある余計な frappe.db.commit() / db.rollback() はトランザクションを終了させ、部分的な状態を公開します。見つけた場合はすべて指摘してください。検証を回避して送信／保存し、検証に失敗してもドキュメントが SL/GL エントリーなしで登録されるのは、重大なバグです。
• 利便性のために原子性を犠牲にしない。 （たとえば、初期化の問題を解決するために autocommit を追加すると、トランザクションが非原子的になります。）autocommit を使うのはスキーマの作成時だけです。
• UX より不変条件を優先する。 「UX を妥協しても、正しさは保証する」。動作しているコードを、壊れたコードに合わせて劣化させないでください。
• 修正する前に、問題を検証する。 正しい対応が「修正しない」ことである場合もあります。まず根本原因を特定してください。
• フィルターが空または None の場合の破壊的な DB API に注意する。 set_value("Site", None, ...) / フィルターなしの db.delete は、すべての行を更新／削除します。テーブル全体に対して何も言わずに操作するのではなく、エラーにする必要があります。名前／フィルターが None、空、または攻撃者が制御できる可能性のある set_value／delete／get_value はすべて指摘してください。
• 条件式で比較する値の型が一致しているか確認する。 型の不一致（文字列と datetime、文字列と int）の比較は、何も一致しないか、常に真になる可能性があります。境界で明示的にキャストしてください（cint／flt）。
• 戻り値の形状を疑う。 結果をインデックスで参照する前に、None / [None, None] になり得るか確認してください。as_dict=1（辞書のリスト）とスカラーの取り違えにも注意します。
• 長年定着してきた仕様を黙って変更しない。 呼び出し側が長く依存してきた動作は契約です。シグネチャを変更していなくても、その変更は見えにくい破壊的変更です。

2. セキュリティ

脆弱性の発生を防ぐほうが、安全に修正するよりはるかに簡単です。セキュリティ上重要なコード（認証、認可、権限、ユーザー管理）は、特に慎重に監査してください。

インジェクション

• SQL を文字列連結や f-string で絶対に組み立てないでください。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 にする前に10回考えてください」。これは実際の認証・認可を省略するための近道ではありません。Web ページでは、データの読み取りや共有の前に権限を適用してください。独自のクエリを作成するより、get_list／get_all を優先してください。
• 制限の緩和は対象を正確に絞ってください。 レート制限や権限の例外が、意図したプリンシパルだけを対象にしていることを確認してください。たとえば、ゲスト以外のすべてのユーザーを対象にしてはいけません。

パストラバーサル／ファイルシステム

• File DocType API を優先してください。ユーザー入力がパスに含まれる場合、サイトフォルダーの外へ移動できないことを確認してください（/../../）。

暗号化／シークレット

• 暗号化処理を独自に実装しないでください。既存の実装を再利用してください。ゲスト／Webhook リクエストの真正性（HMAC）を検証してください。
• 署名付き／ワンタイム URL：本当に秘密の署名値を使用し、時間と初回使用の両方を基準に有効期限を設定してください。フォームデータと結合せず、URL だけを使って検証してください（Frappe は URL とフォームデータを結合するため、有効な署名を使ったリプレイ攻撃が可能になります）。
• シークレットはパスワードフィールドに保存してください。平文で保存したり、ログやエラーメッセージに漏らしたりしないでください。

XSS とその他の OWASP 対策

• ユーザー入力を DOM に挿入しないでください。一見些細なものでも XSS を重大な問題として扱ってください。HTML／JS インジェクションは、通常、アカウントの乗っ取りにつながります。
• 特殊文字をサニタイズして破棄することで XSS を修正しないでください。値を DOM に挿入する直前にエスケープすることを優先してください。

3. パフォーマンスは正しさの一部です

パフォーマンスの問題を修正する最適なタイミングはレビュー時です。遅いコードはマージされると、何年も検出されないまま残ります。パフォーマンスは機能の一部です（ドハーティの閾値。人間は約100msで認識します）。

• 予算： 一般的な読み取りは100ms未満、読み取りは1秒未満、ほとんどの書き込みは5秒未満とし、決して約10秒を超えないようにします。頻繁に呼び出される読み取り専用リクエストのP99は約1秒にします。遅い同期リクエストはワーカーをブロックします（ヘッドオブラインブロッキング）。
• 計算量のルール： 頻繁に呼び出されるエンドポイントの処理は、O(1)またはO(log N)にする必要があります。定数倍が大きいのは許容できますが、O(N)は決して許容できません。行数のカウントはO(N)であり、O(1)ではありません。大規模なテーブルやフィルター済みテーブルに対するCOUNT(*)は高コストです。すべてをスキャンするのではなく、スキャン範囲に上限を設けます（例：過去3か月、「1000+」の番兵値）。
• インデックスはコードです。 インデックスのない列を使うWHERE／結合／フィルターはすべて指摘します。インデックス（およびカスタムインデックス）は場当たり的に適用するのではなく、コードにコミットする必要があります。そうしないと、マイグレーション時に失われます。コメント／バージョン／割り当てを取得するフォームの読み込みでは、これらすべてのクエリにインデックスが必要です。インデックスのないクエリが1つあるだけで、全体の動作が遅くなります。
• ループ内でDBを呼び出さない。「ループ内でDBを呼び出す検証を書かないでください。」N+1パターンを指摘します。UOM、docstatus、ステータスなどの安定した値は再クエリせず、キャッシュします。バックグラウンドジョブでは許容されますが、リクエストでは決して行いません。
• 遅いコードを修正できない場合は、削除 → 削減 → 再利用の順で対処します。 そのコードを削除する、呼び出し回数を減らす、またはメモ化します。適切なキャッシュのスコープ（@redis_cache、@request_cache、@site_cache。最後のものは使いすぎるとメモリーを膨張させます）を選びます。frappe.localやfrappe.flagsで独自のキャッシュを作ってはいけません。「新たなキャッシュ無効化のバグを自分で作ることになるだけです。」ごく低コストの処理はキャッシュしません。
• メモリー： 共有モジュールレベルのファイル／__init__.py／クラスレベルの状態に不要なデータを詰め込まないでください。永久にメモリーに常駐します。使われていないモジュールレベルのインポートは削除します（使用する関数内に移動します）。メモリーリークに注意してください。
• 条件式の順序を入れ替え、DB呼び出しを最後にします。 ブール式では、コストの低いインメモリのチェックを先に配置し、短絡評価によってクエリを完全にスキップできるようにします。
• PythonではなくSQLで集計します。 すべての行を取得してメモリー上で集計するのではなく、クエリ内でSUM()／COUNT()を使います。フィルターはサブクエリ内に適用し、結合の前に処理されるようにします。
• 1つの値を取得するためにドキュメント全体を取得しないでください。 1列だけ取得する場合は、get_doc().save()ではなくget_value／get_single_value／set_valueを使います。ドキュメントを取得してから削除するget_doc().delete()ではなく、frappe.delete_docを使います。
• 負荷の高い処理経路でMyISAMテーブルを使わないでください。 MyISAMテーブルを読み取ると暗黙のテーブルレベルロックが発生します。リクエスト経路では決して触れないでください。
• 時間のかかる処理はバックグラウンドキューに移します。 時間のかかる処理には、ワーカーをブロックする同期リクエストではなく、enqueue(..., queue="long")を使います。

4. 並行処理

• 確認してから実行する処理は競合状態になります。 if not frappe.db.exists(...): insert()では、2つのワーカーがどちらも「存在しない」と判断し、両方が挿入します。DBレベルの一意制約を優先し、「整合性の維持をデータベースに任せます。」
• ロックに関する落とし穴： インデックスのないクエリに対するSELECT ... FOR UPDATEは、スキャン対象の行（およびギャップ）をすべてロックします。フィルターで必ずインデックスを使うようにしてください。そうしないと、テーブル全体をロックします。親をロックして子をロックしないと、「変異した」ドキュメントが生じます。
• グローバルな可変状態／クラス属性は、Pythonではグローバルです。 共有エンジン／クラス属性からクエリ状態が並行リクエスト間で漏れると、データが壊れます。クエリの構築はステートレスにします。「frappe.localを使って変なこと」をしてはいけません。localは変数用であり、静的状態用ではありません。

5. 可読性と保守性

開発時間の約50％はコードを読むことに費やされます。質の悪いコードは、いずれ書き直しを余儀なくされます。

• 可能な場合は関数を純粋に保つ — 読みやすく、テストしやすくなります。
• 変更可能なオブジェクトを渡して中身を埋める（「組み立て」コード）のは避けてください — 新しい値を返します。変更可能なオブジェクトを渡して変更させると、1つのことを理解するために2つのファイルを開く必要が生じます。
• 入力を変更する関数には、その動作が分かる適切な名前を付けてください。
• 分かりやすい構文を優先してください。 関数型の map-reduce を使った1行コードは美しく見えるかもしれませんが、4行の for ループを書いてください。巧妙なコードより、デバッグしやすいコードを優先します。
• 個人のスタイルより一貫性を優先してください。 コードベースが10種類ものスタイルの寄せ集めになってはいけません。周囲の書式、命名、インポートの規約に合わせ、それらを崩す変更には指摘してください。
• センスよく書く： エッジケースを通常ケースにできるよう構成を見直し、特別扱いの分岐をなくしてください。もっと簡潔にできないか？ コードを減らせないか？ インデントの深い if-else の羅列になっていないか？ と考えます。
• コピー＆ペーストによる分岐より、共通コンポーネントの拡張を優先してください。 1つの機能に対する3～4種類の分岐した実装は、長期的な開発速度を低下させます。モジュール間の密結合を避け、明確に文書化された公開 API を通じて統合してください。
• 公開されているモジュール、クラス、関数には docstring を記述してください。型を文章で説明するより、型注釈を優先してください（「型ヒントは10倍優れています」）。型チェッカーは見つけにくいバグを検出します。
• docstring には重要なことだけを記述してください。簡潔に要点をまとめます。関数名から明らかなことを説明する必要はありません。「なぜ」に焦点を当ててください。
• 関連のない変更は別々のコミット／PR に分けてください — レビューに集中でき、git blame やリバートも明確になります。

6. API 設計と後方互換性

• 最小驚きの原則： API の名前とシグネチャから、意図の約90％が伝わるようにしてください。ユーザーが動作に驚くことがあってはなりません。
• さまざまな型を受け付ける曖昧な／多重用途のパラメーター（string／dict／list／None）は避けてください。用途を1つに絞った関数を分けて用意します。暗黙のフォールバックに注意し、明示的なバリエーションを使ってください。「正しく使うのに暗黙の知識が必要な API は負債です。」
• オーバーライドではなく拡張できるように設計してください。 フックを用意し、実行時にコアをモンキーパッチしないでください（「弁解の余地なくひどい」方法であり、将来の修正を妨げます）。また、数行を変更するためにコアファイル全体をコピーしないでください（修正が反映されなくなります）。
• 成熟した／公開 API では、後方互換性を守る義務があります。 semver に従い、マイナーバージョンでは破壊的変更を一切行いません。破壊的変更には、公開関数／フィールドの削除、引数の順序変更、新しい必須引数の追加、ビジネスロジックの変更、ファイルの移動／改名（インポートの破損）、共有依存関係の更新が含まれます。以前の名前をエイリアスとして残さない改名は、不要な破壊的変更です。すべての破壊的変更には非推奨警告とドキュメントを添えてください。
• スキーマの破壊的変更につながる落とし穴に注意してください： 既存サイトへの必須フィールド追加、長期間使われているフィールドの一意制約化（データパッチが必要）、データパッチなしでのフィールド型変更、フィールド削除。
• 既存サイトで変更が暗黙に反映されないスキーマ変更には、データパッチが必要です。 シングル doctype は新しいフィールドのデフォルト値を既存サイトに同期しません。また、フィールド型を変更しても（例：text→int）、既存の値は変換されません。どちらの場合も、データが登録済みのサイトでテストした明示的なパッチが必要です。
• 新しいパラメーターは、既存の位置引数呼び出しに影響しないよう、安全なデフォルト値（"" ではなく None）を持つキーワード引数として末尾に追加してください。 名前を変更する場合は、旧名を中継用として残します：def old_name(...): return new_name(...)。
• パッチの衛生管理。 データパッチは冪等（再実行しても安全）で、正しい順序（読み取り対象のフィールド／doctype が存在してから実行）で適用され、適切なアプリに配置されている必要があります（フレームワークの変更にはフレームワーク内でパッチを適用し、下流のアプリには適用しません）。
• 既存テストの変更は危険信号です。 変更を通すために既存テストのアサーションを編集する必要があったなら、実際のワークフローを壊している可能性があります。テストを無理に変更するのではなく、その理由を明確に説明してください。

7. テスト

• 新機能には十分なテストカバレッジが必要です：プロジェクト全体のカバレッジだけでなく、差分のパッチカバレッジを確認します。（Frappe の目標：差分の対象行の 85% をカバー。）テストでは、最もよく使われる業務シナリオを網羅します。
• 複雑な修正には回帰テストを追加します。 バグ修正の差分から修正内容が明らかでない場合、または同じバグが再発した場合に限り、テストを求めます。小規模で明白な修正や、CI またはツール関連の変更には求めません。重大な影響が生じるコード（状態管理やコンプライアンス関連）では、例示的なテストにとどまらず、プロパティベーステスト（Hypothesis）も実施します。
• マイグレーション／データパッチのカバレッジ不足を指摘します。 スキーマ変更とデータパッチは、リスクが最も高く、テストが最も少ない領域です。フィールドを変更する、またはデータを移行する変更は、実データが入った現実的なサイトでパッチをテストする必要があります（空のテーブルでは、変更が不正でも必ず「マイグレーション」が成功します）。
• テストは決定的で、互いに独立している必要があります。 random は使用しません（テストが不安定になります）。ほかのテストが残した状態に依存しません（実行順序に依存します）。時間に依存するロジックには freeze_time を使います。

8. エラーメッセージ、ログ、可観測性

• エラーメッセージの品質は、正当なレビュー項目です。 タイトルは具体的で、Google で検索できるものにします（「Message」や「Error」は使用しません）。フィールド名はフィールドとして明示します。何が変わったかを示します。行、フィールド、変更前から変更後の値（1 → 2）を明記します。ユーザーが「数量は何から何に変わったのか」を把握できる必要があります。
• 影響を受ける関係者に障害を知らせます —「メール設定の不具合がユーザーの問題になるのは、不具合をユーザーが把握している場合だけです。」
• ログを記録します。 トレースバック／例外のコンテキストを保持します（デバッグのしやすさが格段に向上します）。破壊的操作や管理者操作は、実行者を特定できる情報（誰が、いつ、どこから）とともにログに記録し、一時的なコンテナの外部に保存します。