Quality Code Review

A reviewer's checklist for Frappe applications. Protect correctness, security, and the future maintainer, in that order of consequence. The sections below run in that order — spend most attention on §1 and §2. Prefer a root-cause fix over a workaround, and say why a finding matters (what breaks, for whom).

1. Correctness & stability (highest consequence)

The worst bug is silent stateful corruption — wrong ledgers/stock posted with no error. Treat stateful and legal/accounting/compliance code as "failure is not an option" code.

Fail early and loudly. Use assertions for internal invariants:
```
assert total_credit == total_debit
```
. (Assertions are for invariants the code guarantees — not user-facing validation.)
Picture how it breaks. For every change ask: Where can this break? How will someone misuse this? Write fool-proof code. Extensions and overrides should especially consider handling all sorts of failure modes.
No partial commits. A stray
```
frappe.db.commit()
```
/
```
db.rollback()
```
mid-transaction ends the transaction and exposes partial state — flag every one. Submitting/saving with validation bypassed (docs posted with no SL/GL entry when validation fails) is a critical bug.
Don't sacrifice atomicity for convenience (e.g. adding
```
autocommit
```
to fix bootstrapping makes transactions non-atomic). Autocommit belongs only on schema creation.
Preserve invariants over UX. "Compromise UX, but guarantee correctness." Don't degrade working code to accommodate broken code.
Validate the issue before fixing it. Sometimes the correct fix is "don't fix this". Identify the root causes first.
Watch for destructive DB APIs with empty/
None
filters.
```
set_value("Site", None, ...)
```
/
```
db.delete
```
with no filter updates/deletes every row. These must error, not silently operate on the whole table. Flag any
```
set_value
```
/
```
delete
```
/
```
get_value
```
where the name/filter could be
```
None
```
or empty- or attacker-controlled.
Check the types in a condition actually match. A comparison between mismatched types (string vs
```
datetime
```
, string vs int) silently never matches or is always true — cast explicitly (
```
cint
```
/
```
flt
```
) at the boundary.
Question the return shape. Before indexing a result, ask whether it can be
```
None
```
/
```
[None, None]
```
; watch
```
as_dict=1
```
(list of dicts) vs scalar confusion.
Don't silently change long-standing semantics. Behavior callers have relied on for a long time is a contract — altering it is a breaking change in disguise, even when no signature changed.

2. Security

Avoiding a vulnerability is far easier than fixing one safely. Audit security-critical code (auth, authorization, permissions, user management) especially hard.

Injection

NEVER build SQL by string concatenation/f-strings. Use the ORM or query builder. If raw SQL is unavoidable, use parameter substitution (
```
frappe.db.sql("... where name = %s", (user,))
```
) — never interpolate yourself. Better still, avoid introducing new raw SQL at all: beyond injection risk it ties code to one database, and the framework aims to stay DB-agnostic (Postgres support). Prefer
```
frappe.qb
```
.
Type confusion is an injection vector even with the ORM. Frappe accepts complex types, so a parameter expected to be a string can arrive as a filter list:
```
{"key": ["!=", ""]}
```
passed to
```
db.get_value
```
bypasses a secret-key check. Validate input types at trust boundaries — explicit
```
isinstance(key, str)
```
. Audit every
```
@frappe.whitelist
```
method for this.
Never
```
eval
```
/
```
exec
```
anything yourself.
```
safe_eval
```
/
```
safe_exec
```
only, in limited volume, and "safe_exec is not magic." Never accept a client-supplied method path to execute.

Sandboxing & trust boundaries

Sandboxed execution (RestrictedPython/
```
safe_exec
```
) is not reliably safe — assume escapes exist. Security toggles must live at the right trust boundary: server-script enablement is a bench-level config, never site-level (a tenant could enable it and take over the whole server).
Prefer allowlists over blocklists — blocklists are bypassable. Don't expose everything by default.

Access control

"Think 10 times before
```
allow_guest=True
```
" — it is not a shortcut around real authn/authz. Web pages must apply permissions before reading/sharing data. Prefer
```
get_list
```
/
```
get_all
```
over hand-rolled queries.
Scope relaxations precisely. Verify a rate-limit/permission exception targets exactly the intended principal — not, say, all non-guest users.

Path traversal / filesystem

Prefer the File doctype API. If user input enters a path, ensure it can't traverse (
```
/../../
```
) outside the site folder.

Crypto / secrets

Never roll your own crypto; reuse existing implementations. Verify authenticity of guest/webhook requests (HMAC).
Signed/one-time URLs: use a truly secret signing value; expire by both time and first use; validate using the URL alone, not merged form data (Frappe merges URL + form data → replay attacks with one valid signature).
Store secrets in password fields; never plain text; never leak secrets in logs or error messages.

XSS & the rest of OWASP

Don't inject user input into the DOM. Treat XSS as critical even when it looks trivial — HTML/JS injection usually leads to account hijack.
Don't fix XSS by sanitizing and throwing away special characters. Prefer escaping right before injecting values in DOM.

3. Performance is correctness

The cheapest time to fix performance is at review; slow code merged sits undetected for years. Performance is a feature (Doherty threshold; humans perceive ~100ms).

Budgets: common reads < 100ms; reads < 1s; most writes < 5s; never exceed ~10s. P99 of a frequent read-only request should be ~1s. A slow synchronous request blocks a worker (head-of-line blocking).
Complexity rule: a frequently-called endpoint must do O(1) or O(log N) work — a large constant factor at worst, never O(N). Counting rows is O(N), not O(1);
```
COUNT(*)
```
over a large/filtered table is expensive. Bound unbounded scans (e.g. last 3 months, a "1000+" sentinel) rather than scanning everything.
Indexes are code. Flag any
```
WHERE
```
/join/filter on an unindexed column. Indexes (and custom indexes) must be committed in code, not applied ad-hoc — they get lost on migration otherwise. Form loads that pull comments/versions/assignments need all those queries indexed; one unindexed query makes everything sluggish.
No DB calls in loops. "Don't write validations that call db in LOOPS." Flag N+1 patterns. Cache stable values (UOM, docstatus, status) instead of re-querying. This is acceptable in background jobs, but never in requests.
The Remove → Reduce → Reuse ladder for slow code you can't fix: remove it, invoke it less, or memoize. Pick the right cache scope (
```
@redis_cache
```
,
```
@request_cache
```
,
```
@site_cache
```
— the last balloons memory if overused). DO NOT hand-roll caches in
```
frappe.local
```
or
```
frappe.flags
```
: "you'll just be creating brand-new cache-invalidation bugs." Don't cache trivially cheap work.
Memory: don't stuff junk into shared module-level files /
```
__init__.py
```
/ class-level state — it stays resident forever. Remove unused module-level imports (move into the function that uses them). Watch for leaks.
Reorder conditionals so the DB call is last. In a boolean expression, put cheap in-memory checks first so short-circuiting can skip the query entirely.
Aggregate in SQL, not Python. Use
```
SUM()
```
/
```
COUNT()
```
in the query instead of fetching all rows to reduce them in memory; push filters into the subquery so they apply before the join.
Don't fetch a whole doc for one value. Use
```
get_value
```
/
```
get_single_value
```
/
```
set_value
```
for a single column instead of
```
get_doc().save()
```
; use
```
frappe.delete_doc
```
instead of
```
get_doc().delete()
```
(which fetches the doc only to delete it).
No MyISAM tables in hot paths. Reading a MyISAM table takes an implicit table-level lock — never touch one in a request path.
Move long work to a background queue. Long-running work belongs in
```
enqueue(..., queue="long")
```
, not a synchronous request that blocks a worker.

4. Concurrency

Check-then-act is a race.
```
if not frappe.db.exists(...): insert()
```
— two workers both see "not exists" and both insert. Prefer a DB-level unique constraint; "outsource integrity to the database."
Locking footguns:
```
SELECT ... FOR UPDATE
```
on an unindexed query locks every scanned row (and gaps) — always ensure the filter uses an index, or you lock the whole table. Locking a parent but not its children yields a "mutant" doc.
Global mutable state / class attributes are global in Python — a shared engine/class attribute leaking query state across concurrent requests produces garbage. Make query-building stateless. Don't do "weird shit with
```
frappe.local
```
" —
```
local
```
is for variables, not static state.

5. Readability & maintainability

~50% of dev time is spent reading code; rotten code eventually forces a rewrite.

Keep functions pure when they can be pure — easy to read and test.
Don't pass mutable objects around to be filled in ("assembly" code) — return new values. Passing a mutable to be mutated forces a reader to open two files to understand one thing.
A function that mutates its input must be named appropriately.
Prefer the boring construct. While that functional map-reduce one-liner looks beautiful, please just write a 4-line for-loop. Favor debuggable code over clever code.
Consistency over personal style. A codebase shouldn't be a hodge-podge of 10 styles. Match the surrounding formatting/naming/import conventions; flag a change that breaks them.
Good taste: restructure so the edge case becomes the common case, removing special-case branches. Ask: can this be simpler? Less code? Is it over-indented if-else soup?
Prefer extending shared components over copy-paste divergence. 3–4 forked implementations of one thing → slow long-term velocity. Avoid tight coupling across modules; integrate through clear, documented public APIs.
Document public modules/classes/functions with docstrings; prefer type annotations over describing types in prose ("type hints are 10x better"); type checkers find non-obvious bugs.
Docstrings should only mention important things. Keep them short and to the point. Don't explain what's trivially understood from function name. Focus on "why".
Split unrelated changes into separate commits/PRs — keeps review focused and
```
git blame
```
/reverts clean.

6. API design & backward compatibility

Principle of least astonishment: an API's name + signature should convey ~90% of intent; users shouldn't be surprised by behavior.
Reject loose/overloaded parameters that accept many disjoint types (string/dict/list/None). Prefer separate single-purpose functions. Beware implicit fallbacks; use explicit variants. "APIs whose correct use depends on tribal knowledge are a liability."
Build for extension, not override. Provide hooks; never monkey-patch core at runtime ("inexcusably horrible" — breaks future fixes) and never copy a whole core file to change a few lines (fixes won't propagate).
Backward compatibility is an obligation for mature/public APIs. Follow semver; minor versions = zero breaking changes. Breaking changes include: removing public functions/fields, reordering args, new mandatory args, changed business logic, moved/renamed files (broken imports), bumped shared deps. Renaming without keeping the old name as an alias is an unnecessary break. Every breaking change ships a deprecation warning + docs.
Watch for schema breaking-change footguns: adding mandatory fields to existing sites, making long-lived fields unique (needs a data patch), changing field types without patches, removing fields.
Schema changes that silently skip existing sites need a data patch. Single doctypes don't sync new-field defaults to existing sites, and a field-type change (e.g. text→int) doesn't convert existing values — both need an explicit patch, tested against a populated site.
New parameters go last as keyword args with safe defaults (
```
None
```
, not
```
""
```
) so existing positional callers don't break. When renaming, keep the old name as a shim:
```
def old_name(...): return new_name(...)
```
.
Patch hygiene. Data patches must be idempotent (safe to re-run), correctly ordered (run after the field/doctype they read exists), and live in the right app (a framework change is patched in the framework, not the downstream app).
A modified existing test is a red flag. If making a change pass required editing an existing test's assertions, you've likely broken a real workflow — justify it explicitly rather than bending the test.

7. Testing

Each PR needs decent test coverage — patch coverage on the diff, not just project coverage. (Frappe target: 85% covered lines in the diff.) Tests should capture the most-used business scenarios.
Regression test every fix. A bug fix without a test that would have caught it invites the regression back. For extreme-consequence (stateful/compliance) code, go beyond examples — property-based testing (Hypothesis).
Flag missing migration/data-patch coverage. Schema changes and data patches are the highest-risk, least-tested area; a change that alters fields or migrates data needs a patch tested against a realistic, populated site (empty tables always "migrate" successfully even when the change is invalid).
Tests must be deterministic and independent. No
```
random
```
(flaky); no reliance on state left by other tests (order-dependence); use
```
freeze_time
```
for time-dependent logic.

8. Error messages, logging & observability

Error message quality is a legitimate review item. Titles must be specific and Google-able (never "Message"/"Error"). Reference field names as fields. State what changed: the row, the field, and before→after values (
```
1 → 2
```
). The user must know "qty changed from what to what?"
Surface failures to the affected party — "a broken email setup is the user's problem only if they know it's broken."
Log things. Preserve tracebacks/exception context (orders of magnitude easier debugging). Log destructive/admin actions with attributable identity (who, when, from where), persisted outside ephemeral containers.

quality-code-review

NPX Install

Tags

SKILL.md Content