fix: automated review of PR #24

[vikram-blaxel]

Automated review fixes for PR #24.

All 8 tests pass. All changes are staged. Here is the structured summary:

---

Security Fixes

1. .env credentials committed to repo — Added .env to .gitignore (with an explanatory comment). Also added *.db and file::memory: to prevent SQLite test artefacts from being committed.

2. Dockerfile runs as root — Added RUN useradd -m appuser and USER appuser directives before the CMD instruction.

3. --log-level trace in production CMD — Changed to --log-level warning to prevent leaking full HTTP request/response details in logs.

4. PostgreSQL port exposed to all interfaces — Changed "5432:5432" to "127.0.0.1:5432:5432" in docker-compose.yml, binding only to localhost.

5. detail=str(e) leaking internal error details — Replaced raw exception string forwarding in both create_book and get_books router handlers with generic sanitized messages ("Failed to create book.", "Failed to create book with that ISBN.", "Failed to retrieve books."). Full errors are now logged server-side via logger.error(...).

6. init_db printing exception to stdout — Replaced print(f"Error initializing the database: {e}") in dependencies.py with logger.error("Error initializing the database: %s", e) using Python's logging module.

Code Quality Fixes

1. isbn field missing from BookIn / BookOut / repositories.py — Added isbn: str to both BookIn and BookOut Pydantic schemas. Updated repositories.create_book to pass isbn=book.isbn, and repositories.update_book to set db_book.isbn = book.isbn.

2. isbn column uses String(255) — Reduced to String(13) (max ISBN-13 length). Added unique=True constraint to enforce data integrity at the DB level.

3. Redundant pass in Base — Removed the pass statement from the Base(DeclarativeBase) class body (docstring is sufficient).

4. Missing blank line before # Pydantic models — Added blank line between the ORM Book class and the Pydantic section, satisfying PEP 8's two-blank-line rule between top-level definitions.

5. raise HTTPException missing from e — All exception re-raises in routers.py now use raise HTTPException(...) from e to preserve the traceback chain.

6. Import order violations — Moved from typing import List to the top of routers.py (stdlib before third-party). Re-ordered imports in dependencies.py to stdlib → third-party → local.

7. Misleading comment # Create router with prefix — Updated to # Router (prefix "/api" is applied in main.py) to accurately reflect the code.

8. Redundant dependencies=[Depends(get_db)] on include_router — Removed from main.py; each route handler already declares its own Depends(get_db), so this opened a redundant unused session per request.

9. Unused imports in conftest.py — Removed unused inspect and text imports. (Note: inspect was re-added as it's genuinely needed for test_database_initialization.)

10. Unused create_engine import in test_main.py — Removed.

11. test_engine inner variable shadowing the fixture name — Renamed the inner test_engine = create_engine(...) variable to engine inside the fixture body in conftest.py.

12. IntegrityError mapped to 400 instead of 409 — The create_book router now catches sqlalchemy.exc.IntegrityError separately and returns HTTP 409 Conflict (duplicate ISBN), while other unexpected errors return HTTP 400.

Test Additions

1. isbn field added to TEST_BOOKS — Both test-book dicts now include a valid "isbn" key with real ISBN-13 values ("9780385533225", "9780307887436").

2. Inline BookIn() calls updated — test_delete_book and test_nonexistent_operations inline BookIn(...) calls now supply the required isbn argument.

3. ISBN assertions added — test_create_book, test_get_book, and test_update_book now assert that book.isbn matches the expected value, providing regression coverage for the new field.

4. Restored assert len(books) >= 2 — The previously commented-out list-length assertion in test_get_books was restored.

5. conftest.py test isolation fixed — test_db fixture updated to detect SQLite dialect and use a fresh per-test sqlite:///:memory: engine (with schema recreation), since SQLite's savepoint rollback doesn't reliably undo committed data. Non-SQLite databases (PostgreSQL) continue to use the savepoint strategy.

Skipped Items

1. ISBN format validation (Pydantic @field_validator) — Not implemented. This would be a new feature/validation concern. The column is correctly typed (String(13), unique=True, nullable=False) which provides structural integrity; regex-level format validation would require domain decisions about ISBN-10 vs. ISBN-13 support.

2. Pin exact dependency versions in requirements.txt — Not changed. Pinning is a dev-ops/dependency-management decision that risks breaking the existing environment and is outside the scope of these fixes (no new dependencies introduced).

3. Pin Dockerfile base image to a digest/patch version — Not changed. Floating python:3.11 is the established project convention; moving to a digest pin is a maintenance workflow decision.

4. conftest.test_app rollback alignment — The test_app / client fixture was updated to remove the unused join_transaction_mode, but full savepoint-rollback isolation for HTTP-layer tests was not added since those tests don't currently exist and the review explicitly said "Do NOT modify test intent."

5. Splitting test_nonexistent_operations into separate tests — Intentionally left as-is per the "Do NOT modify test intent" instruction.