← Bank
Code Review

Swallowed Exception in Python

Code ReviewJunior~8m
error-handlingdebuggingpython

Prompt

This helper showed up in a PR for our user-profile service. Support has been getting reports that profiles "randomly come up empty" and nobody can reproduce it. This function is on that read path. Review it — what would you flag, and would you approve?

def fetch_user_data(user_id):
    try:
        data = database.query(f"SELECT * FROM users WHERE id = {user_id}")
        return data
    except:
        pass

How this round runs

Rank what you find by severity and connect at least one finding to the "randomly empty profiles" symptom. Tell me what error handling you'd require before this merges, and whether you'd approve.

Model answer

Block. Ranked by severity:

1 (highest — security: SQL injection). user_id is interpolated straight into the query with an f-string. If it ever carries attacker input (and on a profile read path it usually traces back to a request), 1 OR 1=1, 1; DROP TABLE users and friends are live. This is the finding I lead with even though the PR is "about" exception handling — security outranks the rest. Fix is a parameterized query.

2 (correctness — this is the bug behind the symptom). The bare except: swallows every exception and pass discards it, so a transient DB error, timeout, or any failure returns None. The function also has no return after the try, so the success-with-no-row path and the error path both yield None — indistinguishable. That is almost certainly the "randomly empty profiles": intermittent DB hiccups silently become empty profiles with no log, no trace, nothing to reproduce against. The error context is destroyed at the point it would be most useful.

Bare except: also catches KeyboardInterrupt and SystemExit, so it can swallow a Ctrl-C or a shutdown signal — a separate reason never to write it.

Root cause: the function conflates "no error happened" with "I have data." The fix separates them — narrow the catch, log with context, and let failures be visible (re-raise or return a result the caller can distinguish from a legitimate miss):

def fetch_user_data(user_id):
    try:
        return database.query(
            "SELECT * FROM users WHERE id = %s", (user_id,)
        )
    except DatabaseError:
        logger.exception("fetch_user_data failed for user_id=%s", user_id)
        raise

What I'd require before merge: a parameterized query, a specific exception type, a log line that preserves the traceback, and a deliberate decision on the failure contract — re-raise, or return a sentinel the caller checks — but never a silent None. Naming and formatting are fine; I wouldn't hold the PR on those.

Signals — what a strong answer shows
  • Flagged the SQL injection unprompted and ranked it at the top, even though the PR was framed as an error-handling change
  • Tied the swallowed exception to the reported symptom (intermittent empty profiles) — root cause, not a generic 'bare except is bad'
  • Caught the second failure mode: success-no-row and error both return None and are indistinguishable
  • Specified the required handling: narrow catch, preserve traceback, deliberate failure contract
  • Clear block with a concrete merge checklist
Follow-ups — where it goes next
  • Should it re-raise or return a default? → Depends on the caller; the point is the two outcomes must be distinguishable and the error must be visible.
  • Why is bare `except:` worse than `except Exception:`? → It also catches KeyboardInterrupt/SystemExit, swallowing shutdown and Ctrl-C.
  • Where would you fix this if you couldn't change the signature? → Handle at the call site where the retry/fallback decision actually belongs.
Study this: Programming