fix: automated review of PR #24

[vikram-blaxel]

Automated review fixes for PR #24.

18/18 tests pass. Here is the structured summary:

---

Security Fixes

1. .env committed with plaintext credentials — Added .env to .gitignore (and test.db while there). Created .env.example with placeholder values as a safe template for contributors.

2. Container runs as root (CWE-250) — Added RUN useradd -m appuser and USER appuser to Dockerfile before CMD.

3. Verbose trace log level in production image — Changed --log-level trace → --log-level info in Dockerfile CMD.

4. Internal exception details leaked to API clients — routers.py: create_book now returns generic "Invalid request data" / "Internal server error" messages; get_books returns "Internal server error". Raw str(e) is never sent to callers. All exceptions are logged server-side via logging.

5. PostgreSQL port exposed on all interfaces — Changed docker-compose.yml db ports binding from "5432:5432" → "127.0.0.1:5432:5432".

6. raise ... from e missing (exception chain dropped) — All raise HTTPException(...) calls in routers.py now use raise ... from e to preserve the exception chain for debugging.

Code Quality Fixes

1. isbn field missing from BookIn / BookOut and repositories — Added isbn: str to both Pydantic models (BookIn, BookOut). repositories.create_book now passes isbn=book.isbn; repositories.update_book now sets db_book.isbn = book.isbn.

2. Redundant nullable=False on Mapped[str] — Removed from models.py; Mapped[str] already implies NOT NULL in SQLAlchemy 2.x.

3. Unnecessary pass in Base class — Removed the pass statement (class already has a docstring).

4. Duplicate Depends(get_db) on router — Removed dependencies=[Depends(get_db)] from app.include_router(...) in main.py; each route already declares its own db dependency. Removed now-unused Depends import.

5. Broad except Exception → specific exceptions in create_book — Catches IntegrityError (→ 409 Conflict), SQLAlchemyError (→ 500), then falls back to generic 400 for other errors.

6. Limit cap for GET /books/ — get_books now caps limit = min(limit, 100) to prevent resource exhaustion.

7. Wrong import order in routers.py — Moved from typing import List and import logging to the top; stdlib before third-party imports.

8. Unused imports removed — conftest.py: removed inspect and text from sqlalchemy imports (re-added inspect to test_main.py where it's actually used). test_main.py: removed unused create_engine import.

9. Variable shadowing fixed in conftest.py — Renamed inner test_engine variable → engine in the test_engine fixture; renamed TestingSessionLocal → session_factory in test_app.

10. Blank line before # Pydantic models comment — Added to models.py for PEP 8 compliance.

11. Tests use hermetic SQLite — conftest.py now sets DATABASE_URL to a SQLite file URI before importing application modules, making the test suite runnable without an external PostgreSQL server.

Test Additions

Added a new TestBookRoutes class in test_main.py with 10 HTTP-layer integration tests using the existing client fixture:

• test_create_book_returns_201 — POST creates a book, returns 201 + body with all fields including isbn
• test_create_book_invalid_body_returns_422 — missing required fields returns 422
• test_get_books_returns_200 — GET list returns 200 + JSON array
• test_get_book_returns_200 — GET single book by ID returns 200
• test_get_book_not_found_returns_404 — GET unknown ID returns 404
• test_update_book_returns_200 — PUT updates all fields, returns 200
• test_update_book_not_found_returns_404 — PUT unknown ID returns 404
• test_delete_book_returns_200 — DELETE removes book, second GET returns 404
• test_delete_book_not_found_returns_404 — DELETE unknown ID returns 404
• test_get_books_limit_cap — limit=9999 is accepted (capped internally) and returns 200

Also updated all existing repository tests and TEST_BOOKS constants to include isbn (fixing the 5 previously failing tests). Uncommented assert len(books) >= 2 in test_get_books. Added isbn assertions to test_create_book and test_update_book.

Skipped Items

• Pin dependencies to exact versions (requirements.txt) — Not implemented. Pinning requires knowing the currently installed exact versions and could break the existing CI if the pinned versions differ from what the workflow installs. This is a supply-chain hygiene improvement best done as a separate dedicated change with proper lockfile tooling (pip-compile).
• Disable /docs and /redoc in production (main.py) — Not implemented. This is an operational/deployment concern; disabling them would break developer experience in non-production environments and the review flagged it as informational only.
• String(13) for ISBN / unique=True — Not implemented. The review notes say "Do NOT change database schemas." Changing the column length or adding a uniqueness constraint would alter the schema.
• python:3.11 → pinned digest/slim variant in Dockerfile — Not implemented; no new dependencies/base changes were requested and this is informational.
• Removing .env from git history — Cannot be done with a simple git add; requires git filter-repo / BFG which would rewrite history. The file is now in .gitignore and future commits will not track it. Credential rotation is an operational action outside the scope of code fixes.