fix: automated review of PR #24

[vikram-blaxel]

Automated review fixes for PR #24.

All 11 files are staged. Here is the structured summary:

---

Security Fixes

1. ISBN field wired end-to-end (models.py, repositories.py) — Critical: isbn was nullable=False in the ORM but never supplied, causing IntegrityError on every write. Added isbn: str to BookIn and BookOut, passed isbn=book.isbn in create_book, and added db_book.isbn = book.isbn in update_book.

2. ISBN format validation (models.py) — Added a Pydantic @field_validator that enforces valid ISBN-10 or ISBN-13 format via regex ((?:97[89])?\d{9}[\dX]), rejecting malformed values before they reach the database.

3. ISBN column length tightened (models.py) — Changed String(255) → String(13) to match the maximum ISBN-13 length, providing a DB-layer data-quality guard.

4. Removed redundant nullable=False (models.py) — Mapped[str] already implies NOT NULL in SQLAlchemy 2.x; the explicit kwarg was contradictory and confusing.

5. .env added to .gitignore (.gitignore) — Prevents future accidental secret commits. Also added DATABASE_URL to the existing .env for completeness.

6. Non-root container user (Dockerfile) — Added RUN adduser --disabled-password appuser and USER appuser before CMD to eliminate root-in-container risk.

7. Trace-level logging removed (Dockerfile) — Changed --log-level trace → --log-level warning to prevent HTTP bodies/headers appearing in production logs.

8. Base image pinned (Dockerfile) — Changed python:3.11 → python:3.11.13-slim for a reproducible, auditable, smaller base image.

9. DB port restricted to localhost (docker-compose.yml) — Changed "5432:5432" → "127.0.0.1:5432:5432" to prevent unauthenticated external connection attempts.

10. Exception messages no longer leaked to clients (routers.py) — detail=str(e) replaced with detail="An error occurred processing your request" on all error responses; original exception is logged server-side via logger.exception(...).

11. GET /books/ limit capped (routers.py) — limit parameter now uses Query(default=10, ge=1, le=100) to prevent resource-exhaustion DoS.

12. Structured logging replaces print() (dependencies.py) — print(f"Error...") replaced with logger.error(...) for proper production observability.

---

Code Quality Fixes

1. Exception chaining preserved (routers.py) — All raise HTTPException(...) from e now use the from e form (W0707 fix).

2. Import ordering fixed (routers.py) — from typing import List moved above third-party imports per PEP 8 / isort.

3. Extra DB session per request removed (main.py) — Removed dependencies=[Depends(get_db)] from include_router; get_db is already declared at the route level.

4. Unused imports removed (conftest.py) — Dropped inspect and text (ruff F401 × 2).

5. Fixture name shadowing fixed (conftest.py) — Renamed inner variable in test_engine fixture from test_engine to engine (W0621 fix).

6. test_app fixture now uses transactional rollback (conftest.py) — override_get_db now yields the same test_db session so HTTP-layer tests participate in the savepoint-based rollback and leave no residual data.

7. Unused create_engine import removed (test_main.py) — Dropped from test_main.py; inspect moved there directly (F401 fix).

8. Dead comment removed (test_main.py) — Removed #assert len(books) >= 2; replaced with the correct active assertion assert len(books) == 2.

9. pass removed from Base class (models.py) — W0107 unnecessary pass removed; docstring alone is sufficient.

10. Blank line added before # Pydantic models (models.py) — PEP 8 two-blank-line separation between top-level definitions.

11. Docstrings added to all repository functions (repositories.py) — Resolves pylint C0116 × 5.

12. Docstrings added to all route handlers (routers.py) — Resolves pylint C0116 × 5.

13. logging used in dependencies.py (dependencies.py) — Imports and proper logger structured throughout.

---

Test Additions

1. isbn added to TEST_BOOKS fixture data — Both entries now include valid ISBN-13 values (9780385533348, 9780307887443).

2. All repository tests updated — BookIn(...) calls now include isbn; assertions added for book.isbn in test_create_book, test_get_book, and test_update_book.

3. New TestBookAPI class with 10 HTTP-layer tests — Covers all 5 CRUD endpoints and their error paths:

   • test_create_book — POST 201
   • test_create_book_invalid_isbn — POST 422 for bad ISBN
   • test_get_books — GET list 200
   • test_get_books_limit_cap — GET 422 for limit=999
   • test_get_book — GET single 200
   • test_get_book_not_found — GET 404
   • test_update_book — PUT 200
   • test_update_book_not_found — PUT 404
   • test_delete_book — DELETE 200 + confirms gone with 404
   • test_delete_book_not_found — DELETE 404

Total: 18 tests, all passing (up from 3 passing / 5 failing).

---

Skipped Items

1. Rotating the committed POSTGRES_PASSWORD=secret — This is a development-only credential in .env (now gitignored). Rotation of production credentials is an ops/infra concern outside the scope of code changes.

2. Pinning requirements.txt to exact versions — Not done to avoid introducing new dependency constraints without a proper pip-compile workflow; the review noted this as informational only.

3. test_nonexistent_operations split into three tests — The review called this informational. The single test still correctly validates all three behaviours; splitting would not change test intent, which we are instructed not to modify.

4. Pointing test_engine at a dedicated test database URL — The tests run against the real PostgreSQL instance (same as before the PR). Introducing a DATABASE_URL_TEST env var would require CI/infrastructure changes beyond the codebase; left as a future improvement.