Code Review

@RestController
public class OrderController {
    @Autowired private OrderRepository orderRepo;

    @GetMapping("/api/orders")
    public List<OrderDTO> listOrders(@RequestParam Long userId) {
        List<Order> orders = orderRepo.findByUserId(userId);
        List<OrderDTO> out = new ArrayList<>();
        for (Order o : orders) {
            OrderDTO dto = new OrderDTO();
            dto.id = o.getId();
            dto.total = o.getTotal();
            // Order has @OneToMany(fetch = LAZY) List<LineItem> items;
            dto.itemCount = o.getItems().size();
            out.add(dto);
        }
        return out;
    }
}

This endpoint lists a user's orders. What's the most serious issue?

Because items is LAZY, calling getItems().size() inside the loop issues one extra SELECT per order — classic N+1. Field injection is a style smell but not a correctness or performance issue at this scale.

import threading

class Counter:
    def __init__(self):
        self.value = 0

    def increment(self):
        self.value += 1

def run():
    c = Counter()
    threads = [threading.Thread(target=lambda: [c.increment() for _ in range(100000)])
               for _ in range(10)]
    for t in threads: t.start()
    for t in threads: t.join()
    assert c.value == 1000000, c.value

The assertion fails intermittently with a value less than 1000000. Why?

x += 1 compiles to LOAD, ADD, STORE — three bytecodes. A thread switch between LOAD and STORE drops the other thread's increment. The GIL guarantees one bytecode at a time, not that += is atomic. Wrap in threading.Lock or use itertools.count / atomic primitives.

public List<String> loadUserEmails(long userId) throws SQLException {
    Connection conn = dataSource.getConnection();
    PreparedStatement ps = conn.prepareStatement(
        "SELECT email FROM users WHERE org_id = ?");
    ps.setLong(1, userId);
    ResultSet rs = ps.executeQuery();
    List<String> out = new ArrayList<>();
    while (rs.next()) {
        out.add(rs.getString("email"));
    }
    rs.close();
    ps.close();
    conn.close();
    return out;
}

This runs fine in dev but the connection pool exhausts in production. What's wrong?

Manual close() calls don't run if executeQuery or rs.next() throws — the connection leaks back to nothing. Use try-with-resources so JDBC resources are closed even on exception. The parameter naming (d) is a real code smell but not what exhausts the pool.

import express from "express";
import { pool } from "./db";

const app = express();
app.use(express.json());

app.get("/api/users/search", async (req, res) => {
  const q = String(req.query.q ?? "");
  const sql = "SELECT id, email FROM users WHERE email LIKE '%" + q + "%' LIMIT 50";
  const { rows } = await pool.query(sql);
  res.json(rows);
});

What's the most serious issue with this search endpoint?

q comes straight from the query string into the SQL (Structured Query Language) string. A request like ?q=%27 OR 1=1-- dumps every row, and worse payloads can drop tables or exfiltrate data. Use parameterized queries: pool.query('... LIKE $1 ...', ['%' + q + '%']). Index usage (a) is a perf concern, not a vuln.

async function renderComments(postId) {
  const res = await fetch("/api/comments?post=" + postId);
  const comments = await res.json();
  const container = document.getElementById("comments");
  container.innerHTML = comments
    .map(c => "<div class='comment'>" +
              "<b>" + c.author + "</b>: " + c.body +
              "</div>")
    .join("");
}

What's the most serious issue here?

A comment body of <img src=x onerror=fetch('/steal?c='+document.cookie)> executes the moment it's painted. Use textContent for plain text, or build elements with createElement and assign properties — never interpolate user data into an HTML string. URLSearchParams (a) is cleaner code but not a vuln in this shape.

from fastapi import FastAPI
import psycopg2

app = FastAPI()
conn = psycopg2.connect("dbname=app user=app")

@app.get("/users/{uid}")
async def get_user(uid: int):
    cur = conn.cursor()
    cur.execute("SELECT id, email FROM users WHERE id = %s", (uid,))
    row = cur.fetchone()
    cur.close()
    if row is None:
        return {"error": "not found"}
    return {"id": row[0], "email": row[1]}

This endpoint serves fine under low load but tail latency explodes under concurrency. Why?

FastAPI runs async def handlers directly on the event loop. A blocking call there freezes every other in-flight request until the query returns, so latency snowballs under concurrency. Use an async driver (asyncpg, psycopg3 async) or define the handler as def so FastAPI runs it in a threadpool. The shared connection (a) is also wrong but the event-loop block is the load-bearing bug.

public void writeReport(String userPath, byte[] data) throws IOException {
    File f = new File("/var/reports/" + userPath);
    if (f.exists()) {
        throw new IOException("Report already exists: " + userPath);
    }
    try (FileOutputStream out = new FileOutputStream(f)) {
        out.write(data);
    }
}

What is the most serious correctness issue (assume userPath is already validated to be a safe filename)?

exists() and the open() are two separate syscalls. Between them, anything can create the file — including a malicious symlink — and FileOutputStream will happily overwrite it. The atomic fix is Files.newOutputStream(path, StandardOpenOption.CREATE_NEW), which fails atomically if the file exists.

from flask import Flask, request, jsonify

app = Flask(__name__)

def flatten(node):
    out = []
    if isinstance(node, list):
        for child in node:
            out.extend(flatten(child))
    else:
        out.append(node)
    return out

@app.post("/flatten")
def handler():
    body = request.get_json()
    return jsonify(flatten(body))

An attacker can crash this worker with a single small request. What's the issue?

A payload like [[[[...[1]...]]]] with ~1000 levels overflows the default recursion limit and kills the worker with RecursionError. Convert to an explicit stack-based iterative flatten, or cap depth before recursing. (c) is wrong — list.extend is amortized linear.

const sessions = new Map<string, { lastSeen: number }>();

function reapExpired(now: number, ttlMs: number) {
  for (const [id, session] of sessions) {
    if (now - session.lastSeen > ttlMs) {
      sessions.delete(id);
    }
  }
}

function touch(id: string) {
  sessions.set(id, { lastSeen: Date.now() });
}

What's the subtle bug in reapExpired?

JS Map iteration explicitly supports delete-during-iteration — option (a) is wrong. The real hazard is concurrency: in Node, an await or a callback during reaping (or, more concretely, code that hands the Map to multiple async contexts) can race touch() against delete(), losing live sessions. Snapshot keys or guard the section.

import java.math.BigDecimal;

public class PriceCheck {
    public static boolean isFreeShipping(BigDecimal subtotal) {
        BigDecimal threshold = new BigDecimal("50.00");
        return subtotal.equals(threshold) || subtotal.compareTo(threshold) > 0;
    }

    public static void main(String[] args) {
        BigDecimal s = new BigDecimal("50");
        System.out.println(isFreeShipping(s));
    }
}

main prints true — but a similar-looking input prints false. Where's the bug?

BigDecimal.equals considers both value AND scale: new BigDecimal("50").equals(new BigDecimal("50.00")) is false. Use compareTo(other) == 0 for numeric equality. The compareTo > 0 branch in this code happens to mask the bug when subtotal is greater, but the equals call itself is unreliable.

function dedupePreserveOrder(items) {
  const out = [];
  for (const item of items) {
    if (out.indexOf(item) === -1) {
      out.push(item);
    }
  }
  return out;
}

function deduplicateEvents(events) {
  return dedupePreserveOrder(events.map(e => e.id));
}

This works correctly but melts at production scale. What's the issue?

For each of n items, indexOf scans up to n previously-added items: total work is n*(n+1)/2. At n=100k that's ~5 billion comparisons. Use a Set for O(1) membership: const seen = new Set(); for (...) if (!seen.has(item)) { seen.add(item); out.push(item); }

def withdraw(balance: float, amount: float) -> float:
    if balance == amount:
        return 0.0
    if balance < amount:
        raise ValueError("insufficient funds")
    return balance - amount

balance = 0.1 + 0.2
print(withdraw(balance, 0.3))

Why does this raise ValueError instead of returning 0.0?

0.1 and 0.2 have no exact binary representation; their sum is slightly greater than 0.3. == on floats almost never does what you want. Use math.isclose(a, b) for tolerance-based comparison, or switch to decimal.Decimal for money. Annotations (d) don't coerce at runtime.

@RestController
public class AuthController {
    @Value("${admin.token}") private String adminToken;

    @PostMapping("/admin/login")
    public ResponseEntity<String> login(@RequestBody LoginReq req) {
        if (req.token != null && req.token.equals(adminToken)) {
            return ResponseEntity.ok(issueSession());
        }
        return ResponseEntity.status(401).body("nope");
    }

    private String issueSession() { /* ... */ return "session-cookie"; }
}

What's the security flaw here?

String.equals returns at the first byte that differs, so comparing 'a*****' takes measurably less time than 'admin1***'. An attacker who can measure response time can recover the token byte by byte. Use MessageDigest.isEqual(a.getBytes(), b.getBytes()) which is constant-time. Rate limiting (d) is good defense in depth but doesn't fix the side channel.

const userCache = new Map<string, User>();

export async function getUser(id: string): Promise<User> {
  const cached = userCache.get(id);
  if (cached) return cached;
  const user = await db.users.findById(id);
  userCache.set(id, user);
  return user;
}

export async function updateUserEmail(id: string, email: string): Promise<void> {
  await db.users.update(id, { email });
}

Users report their email change doesn't show up until the process restarts. Why?

Classic cache-invalidation bug: writes go to the source of truth, reads come from a stale snapshot. Either userCache.delete(id) after the update, or write-through (userCache.set with the new value). WeakMap (d) keys by object identity, which doesn't help here.

const express = require("express");
const cookieParser = require("cookie-parser");
const app = express();
app.use(cookieParser());
app.use(express.json());

function requireSession(req, res, next) {
  const sid = req.cookies.sid;
  if (!sid || !sessions.has(sid)) return res.status(401).end();
  req.user = sessions.get(sid);
  next();
}

app.post("/account/transfer", requireSession, async (req, res) => {
  const { toAccount, amount } = req.body;
  await bank.transfer(req.user.id, toAccount, amount);
  res.json({ ok: true });
});

A logged-in user visits evil.com and money leaves their account. What's missing?

Cookie auth alone identifies the user but doesn't prove the request was intentional. evil.com can submit a form (or fetch with credentials) to /account/transfer; the browser attaches the session cookie. Mitigations: SameSite=Lax/Strict on the cookie (modern default), a CSRF (Cross-Site Request Forgery) token the server checks, or requiring a custom header that browsers won't add cross-origin without a preflight.