review-fix: automated review of PR #24

[vikram-blaxel]

Automated review fixes for PR #24.

Developer actions:

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

---

Security Fixes

1. Dockerfile — Container ran as root (Critical/Warning, semgrep blocking): Added RUN useradd --no-create-home --shell /bin/false appuser and USER appuser before CMD so uvicorn runs as a non-privileged user.

2. Dockerfile — --log-level trace leaked full request/response bodies including auth headers (Warning): Changed to --log-level info.

3. models.py — No unique=True on isbn column (Critical): ISBNs are globally unique identifiers; without uniqueness enforcement duplicate ISBNs could corrupt the data model. Added unique=True, index=True to the isbn column.

4. .env — DATABASE_URL missing (caused startup crash); added it alongside the existing credentials. Note: .env containing POSTGRES_PASSWORD=secret is tracked by git — documented in security review as needing .gitignore treatment and secrets rotation for production (not changed as it's outside the PR scope).

---

Code Quality Fixes

1. models.py — isbn missing from BookIn Pydantic schema (Critical): The field was added to the ORM model but not to the input schema, so clients could never send it and every write failed with a NOT NULL DB constraint violation. Added isbn: str to BookIn.

2. models.py — isbn missing from BookOut Pydantic schema (Critical): The field was never returned in API responses. Added isbn: str to BookOut.

3. models.py — nullable=False redundant in SQLAlchemy 2.x (Warning): Mapped[str] already implies NOT NULL; the explicit kwarg is ignored. Removed nullable=False and let Mapped[str] carry the constraint implicitly (also changed to String(20) — appropriate for ISBN-13 max 17 chars with hyphens).

4. models.py — String(255) too large for ISBN (Warning): Changed to String(20).

5. models.py — Missing blank line before # Pydantic models (Informational/PEP 8): Added blank line between Book class and the comment.

6. repositories.py — create_book didn't pass isbn to ORM constructor (Critical): Changed to models.Book(title=book.title, author=book.author, isbn=book.isbn).

7. repositories.py — update_book didn't update isbn (Critical): Added db_book.isbn = book.isbn.

8. db.sql — Schema not updated with isbn column (Critical): Added isbn VARCHAR(20) UNIQUE NOT NULL. Also fixed pre-existing issues: removed erroneous UNIQUE on title and author, corrected column widths to VARCHAR(255) to match the ORM.

9. conftest.py — Unused imports inspect and text from sqlalchemy (Warning, ruff F401): Removed both unused imports.

---

Test Additions

1. test_main.py — TEST_BOOKS constants updated with isbn key in both entries (was causing ValidationError with every test after BookIn fix).

2. test_main.py — All existing BookIn(...) calls updated to include isbn (test_delete_book, test_nonexistent_operations).

3. test_main.py — isbn assertions added to test_create_book, test_get_book, test_update_book.

4. test_main.py — Unused create_engine import removed (ruff F401).

5. test_main.py — TestMainApp new tests: test_books_table_has_isbn_column and test_isbn_column_is_not_nullable verify the schema at the DDL level.

6. test_main.py — TestBookRepository new ISBN-specific tests: test_isbn_is_stored_and_returned, test_isbn_is_returned_on_get, test_isbn_is_updated_on_update, test_isbn_is_required, test_isbn_uniqueness_enforced.

7. test_main.py — New TestBooksAPI class (8 HTTP-level tests via TestClient): covers POST/GET/LIST/PUT/DELETE endpoints including isbn field in responses, missing-isbn 422, and 404 paths for nonexistent books.

Result: 23 tests pass (was 0 runnable before — the suite couldn't even collect without DATABASE_URL, and would have had 5/8 failures due to NOT NULL violations).

---

Skipped Items

1. requirements.txt — Unpinned dependencies (Warning): Not pinned to exact versions because introducing a requirements-lock.txt would be a new dependency management artifact that goes beyond the scope of a code fix PR, and pinning the existing file could break CI workflows that rely on the >= bounds. Documented in the security review.

2. routers.py — raise HTTPException(...) from e missing chaining (Informational): Pre-existing issue not introduced by this PR; left unchanged to avoid modifying unrelated code.

3. routers.py — from typing import List import ordering (Informational): Pre-existing PEP 8 issue not introduced by this PR.

4. dependencies.py — print() instead of logging (Informational): Pre-existing issue; fixing it is a refactor outside PR scope.

5. Dockerfile — FROM python:3.11 untagged base image (Informational): Pinning to a digest is a good practice but is a pre-existing infra policy decision beyond this PR's scope.

6. No auth/authz layer (Informational): Pre-existing architectural concern, not in scope for this PR.