Quality Code Review

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.