👨‍🏫 Code Review Teacher

Code Review: Customer subscriptions endpoint — billing-adjacent, read-heavy, and currently a latency bomb

Verdict

Request changes. The logic is correct and I can see what you were going for — but this endpoint has an N+1 query, a broken error contract, and it touches money without a single test or type guarantee in sight. At 200 req/sec on a billing page, this will be your most-hit endpoint, and right now it's a pager waiting to buzz. The good news: every problem here is a well-known pattern with a clean fix, and once you internalize them you won't write this shape of code again.

Findings (ranked by career impact)

1. N+1 query in the hot loop — Blocker

The code:

for s in subs:
    if s.status == "active":
        plan = db.query(Plan).filter(Plan.id == s.plan_id).first()

What I'm pattern-matching to: The instant I see a DB call inside a for loop over DB results, my stomach drops. This is the single most common performance bug in ORM code and it's the one that makes senior engineers sigh audibly in review. One request → 1 + 1 + N queries. A customer with 12 subs is 14 round trips. At 200 rps, that's ~2800 queries/sec to Postgres for one endpoint.

Anti-pattern name: N+1 query / SELECT-in-loop.

Failure mode in production: Endpoint works fine in staging (small data). Ships. Works fine for most customers. Then your biggest customer — the one with 40 subscriptions — loads their dashboard and takes 4 seconds. Their CSM Slacks you. You add a cache. The cache gets stale. Now billing shows wrong numbers. This is how billing incidents start.

The fix: Join, or use selectinload:

subs = (db.query(Subscription)
          .options(selectinload(Subscription.plan))
          .filter(Subscription.customer_id == customer_id,
                  Subscription.status == "active")
          .all())

Also push the status == "active" filter into SQL — don't fetch rows just to throw them away.

Heuristic to carry forever: If your ORM call is inside a loop, you've already written a bug. Pull it out or eager-load.

2. Money as a float — Blocker (this is the finance one)

The code:

"price": plan.price
...
total += plan.price

What I'm pattern-matching to: You're in a billing-adjacent endpoint and I can't tell from the code whether plan.price is a Decimal, float, or cents-as-int. If it's a float, you are one summation away from returning 29.990000000000002 to a customer. On a billing page.

Anti-pattern name: Floating-point money.

Failure mode in production: Customer sees $89.97000000001. Screenshots it. Tweets it. Support ticket. Or worse: you sum prices for invoicing and undercharge by a penny per customer per month, and finance notices in Q3.

The fix: Use Decimal end-to-end, or store cents as int. Type the response model with Pydantic so the contract is explicit:

class SubscriptionOut(BaseModel):
    id: str
    plan_name: str
    price_cents: int

Heuristic to carry forever: Money is never a float. Ever. Not even once.

3. Error contract is a lie — Major

The code:

if not customer:
    return {"error": "not found"}

What I'm pattern-matching to: Returning a 200 with an error key. The client sees HTTP 200 OK and has to introspect the body to know it failed. Every consumer of this API will eventually forget to check, and you'll get silent failures downstream.

Anti-pattern name: Envelope-based errors over HTTP / lying status codes.

Failure mode in production: Frontend shows 'Customer: undefined' because it happily parsed the error object as a customer. Or a retry layer doesn't retry because 200 means success.

The fix: raise HTTPException(status_code=404, detail="customer not found"). Let the framework do its job.

Heuristic to carry forever: HTTP status codes exist so clients don't have to read your mind. Use them.

4. No response model, no types on the boundary — Major

What I'm pattern-matching to: FastAPI's entire value proposition is typed boundaries, and this endpoint throws that away by returning a raw dict. Future-you adds a field, forgets to update the frontend, and nothing complains until a user does.

Failure mode in production: Silent schema drift. The API docs (which FastAPI generates from types) say nothing useful. New engineers guess at the shape.

The fix: Define a Pydantic response_model on the route. Now the contract is enforced and documented.

Heuristic to carry forever: The boundary of your service is a contract. Make the computer check it.

5. `customer_id: str` with no validation — Minor

If IDs are UUIDs, type them as UUID and FastAPI rejects garbage at the door. Saves a DB round trip on every malformed request and gives you a clean 422.

6. Naming: `subs`, `s`, `active` — Nit

In a 15-line function it's fine. I'd still write subscriptions and active_subscriptions because the cost of typing is zero and the cost of ambiguity compounds. Not worth blocking a PR on.

What You Did Well

• You separated the 'fetch' from the 'transform' mentally, even if the DB calls leaked into the loop. The shape of the function is right.
• You returned a computed total on the server instead of making the client sum prices. That's the correct call for money — one source of truth.
• You used dependency injection for the DB session. A lot of 2-year engineers wire sessions globally and regret it.

The One Habit to Build

Treat every endpoint as a contract with a typed request and a typed response, and assume the database is the slowest thing in the room. Those two instincts — 'where are my types?' and 'how many queries is this?' — are 80% of what separates a mid-level backend engineer from a senior one. If you run every PR you write through those two questions before hitting submit, you'll level up faster than any book will take you.

Suggested Practice

Rewrite this endpoint with: (1) a Pydantic request & response model, (2) a single query using selectinload, (3) Decimal or integer cents for money, (4) proper HTTPException for errors, and (5) a pytest test that asserts exactly one SQL query is emitted (use sqlalchemy.event or pytest-postgresql's query counter). The last one is the killer exercise — once you've written a test that fails when an N+1 sneaks in, you start seeing them everywhere.