## Code Review: 5 Critical Issues Found\n\n### 1. SQL Injection (Critical — Security)\n**Both functions** are vulnerable to SQL injection. User input is interpolated directly into SQL strings via f-strings.\n\n`get_user_data`: `f\"SELECT * FROM users WHERE id = {user_id}\"` — an attacker passing `user_id = \"1 OR 1=1\"` retrieves all rows.\n\n`_update_single`: `f\"UPDATE users SET status = '{status}' WHERE id = {uid}\"` — passing `status = \"active'; DROP TABLE users; --\"` destroys the table.\n\n**Fix:** Use parameterized queries everywhere.\n\n### 2. Resource Leak — Connection Never Closed (High)\n`get_user_data` opens a connection but never closes it. If `result` is `None`, the connection leaks. Even on the happy path, the `return` bypasses any cleanup.\n\n`_update_single` has the same issue — no `conn.close()` and no error handling.\n\n**Fix:** Use context managers (`with conn:`) and ensure connections close in a `finally` block or via `with sqlite3.connect(...) as conn`.\n\n### 3. Uncontrolled Thread Spawning (High — Reliability)\n`bulk_update_users` spawns one thread per user ID with no limit. For 10,000 users, you get 10,000 threads, causing thread exhaustion and potential OS-level failures.\n\n**Fix:** Use `concurrent.futures.ThreadPoolExecutor` with a bounded pool size.\n\n### 4. No Error Handling on JSON Parse (Medium)\n`json.loads(result[3])` will throw `json.JSONDecodeError` if the `prefs` column contains malformed JSON. This crashes the entire function with no useful error message.\n\n**Fix:** Wrap in try/except and return a default value or raise a domain-specific exception.\n\n### 5. Thread Safety with SQLite (Medium)\nSQLite has limited concurrency support. Multiple threads writing simultaneously can cause `OperationalError: database is locked`. The default timeout is 5 seconds, which may not be enough under load.\n\n**Fix:** Pass `timeout=30` to `sqlite3.connect()` and consider using a connection pool or WAL mode.\n\n---\n\n### Corrected Code\n\n```python\nimport sqlite3\nimport json\nfrom concurrent.futures import ThreadPoolExecutor\n\nMAX_WORKERS = 10\n\ndef get_user_data(user_id: int, db_path: str = 'users.db') -> dict | None:\n with sqlite3.connect(db_path, timeout=30) as conn:\n cursor = conn.cursor()\n cursor.execute(\"SELECT * FROM users WHERE id = ?\", (user_id,))\n result = cursor.fetchone()\n if not result:\n return None\n try:\n prefs = json.loads(result[3])\n except (json.JSONDecodeError, TypeError):\n prefs = {}\n return {'id': result[0], 'name': result[1], 'email': result[2], 'prefs': prefs}\n\ndef bulk_update_users(user_ids: list[int], new_status: str, db_path: str = 'users.db'):\n with ThreadPoolExecutor(max_workers=MAX_WORKERS) as executor:\n futures = [executor.submit(_update_single, uid, new_status, db_path) for uid in user_ids]\n for f in futures:\n f.result() # raises if any thread failed\n\ndef _update_single(uid: int, status: str, db_path: str):\n with sqlite3.connect(db_path, timeout=30) as conn:\n conn.execute(\"UPDATE users SET status = ? WHERE id = ?\", (status, uid))\n conn.commit()\n```\n\n**Key changes:** Parameterized queries throughout, context managers for connections, bounded thread pool, JSON error handling, type hints for clarity.
## Code Review\n\nHere are the issues I found with this code:\n\n### Security Issues\n\n**SQL Injection Vulnerability**\nThe `get_user_data` function uses an f-string to build the SQL query, which makes it vulnerable to SQL injection attacks. You should use parameterized queries instead:\n\n```python\n# Before (vulnerable)\nquery = f\"SELECT * FROM users WHERE id = {user_id}\"\ncursor.execute(query)\n\n# After (safe)\ncursor.execute(\"SELECT * FROM users WHERE id = ?\", (user_id,))\n```\n\nThe same issue exists in `_update_single`.\n\n### Resource Management\n\n**Database connections aren't being closed**\nThe `get_user_data` function opens a database connection but never closes it. This will cause resource leaks over time. Use a `with` statement or try/finally to ensure the connection is closed:\n\n```python\ndef get_user_data(user_id, db_path='users.db'):\n conn = sqlite3.connect(db_path)\n try:\n cursor = conn.cursor()\n cursor.execute(\"SELECT * FROM users WHERE id = ?\", (user_id,))\n result = cursor.fetchone()\n if result:\n return {'id': result[0], 'name': result[1], 'email': result[2], 'prefs': json.loads(result[3])}\n return None\n finally:\n conn.close()\n```\n\n### Threading Concerns\n\nThe `bulk_update_users` function creates a new thread for each user, which could be problematic with large lists. Consider using a thread pool to limit the number of concurrent threads.\n\n### Style\n\n- Consider adding type hints to function parameters\n- The `SELECT *` could be replaced with specific column names for clarity\n\nOverall, the main priorities are fixing the SQL injection vulnerability and adding proper connection management.