fix: automated fixes for PR #24

[vikram-blaxel]

Automated fixes for PR #24.

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

---

Security Fixes

1. ISBN field propagation (Critical – Security/Code Quality/Test reports): Added isbn: str to BookIn Pydantic model with a @field_validator enforcing ISBN-10/ISBN-13 format (^(?:\d{9}[\dX]|\d{13})$); added isbn: str to BookOut. This means invalid ISBNs are rejected at the API boundary with HTTP 422 rather than causing a database IntegrityError.

2. Exception detail leakage (Critical – Security): Replaced detail=str(e) in all except blocks in routers.py with detail="An error occurred processing your request" and added logger.error(...) for server-side logging. Used raise ... from e to preserve exception chaining for debugging. This prevents internal SQLAlchemy query text and schema details from leaking to clients.

3. Unbounded limit query parameter (Warning – Security): Changed limit: int = 10 to limit: int = Query(default=10, ge=1, le=100) and skip: int = 0 to skip: int = Query(default=0, ge=0) in GET /books/. Requests with limit > 100 now return HTTP 422, preventing denial-of-service via huge result sets.

4. .env not in .gitignore (Warning – Security): Added .env to .gitignore to prevent accidental credential commits.

5. Dockerfile: container runs as root (Warning – Security): Added RUN adduser --disabled-password --gecos "" appuser and USER appuser before CMD.

6. Dockerfile: --log-level trace in production (Warning – Security): Changed --log-level trace to --log-level warning to prevent sensitive request/response data from leaking into logs.

7. Docker Compose: PostgreSQL port exposed to host (Warning – Security): Removed the ports: - "5432:5432" mapping from the db service; the database is now only reachable inside the Docker network.

8. ISBN column size (Informational – Security/Code Quality): Changed String(255) to String(13) for the isbn column — the tightest correct size for ISBN-13 (13 chars), enforcing length at the database layer.

Code Quality Fixes

1. repositories.create_book missing isbn (Critical – Code Quality): Added isbn=book.isbn to the Book(...) constructor call in create_book.

2. repositories.update_book missing isbn (Critical – Code Quality): Added db_book.isbn = book.isbn in the update_book function.

3. Redundant nullable=False removed (Warning – Code Quality): Removed the nullable=False kwarg from the isbn mapped_column; Mapped[str] already implies NOT NULL in SQLAlchemy 2.x.

4. raise ... from e (Warning – Code Quality): Both except blocks in routers.py now use raise HTTPException(...) from e.

5. from typing import List replaced (Informational – Code Quality): Replaced List[models.BookOut] with the built-in list[models.BookOut] and removed the typing import from routers.py.

6. Unused imports removed (Informational – Code Quality): Removed text from conftest.py; removed create_engine and inspect-unused-in-old-sense from test_main.py (kept inspect which is used by test_database_initialization).

7. models.py blank line (Informational – Code Quality): Added a blank line before the # Pydantic models section separator to comply with PEP 8.

8. models.py unnecessary pass in Base: Left as-is (pre-existing, not part of the PR diff, and removing it is style-only with no risk reduction).

Test Additions

1. test_main.py – isbn added to all fixtures: Updated TEST_BOOKS constants with valid ISBN-13 values; updated all BookIn(...) calls in repository tests and inline usages.

2. test_main.py – TestBookRepository split and extended:

   • test_get_nonexistent_book, test_update_nonexistent_book, test_delete_nonexistent_book — replaced the single combined test_nonexistent_operations with three separate, named tests.
   • test_get_books_pagination — new test verifying skip/limit pagination at the repository layer.
   • Restored assert len(books) >= 2 in test_get_books.
   • Added isbn field assertions in test_create_book, test_get_book, test_update_book.

3. test_main.py – TestAPIRoutes (new class, 12 tests): Full HTTP-level coverage via the client fixture for all 5 endpoints: create (happy path, missing isbn → 422, invalid isbn → 422), list (happy path, pagination limit, limit > max → 422), get (happy path, not found → 404), update (happy path, not found → 404), delete (happy path, not found → 404).

4. conftest.py – test_app fixture improved: Changed to use a single shared connection with savepoint-based rollback (same pattern as test_db), fixing data-leakage between requests within the same test and ensuring data written through the HTTP client is visible within the same test.

5. test_main.py::TestMainApp::test_create_app strengthened: Added assertion that test_app.routes is non-empty.

Skipped Items

1. Authentication/Authorization (Critical – Security): Adding OAuth2/JWT or API-key authentication was not implemented. This requires introducing new application logic (token issuance, user management or key store) and non-trivial infrastructure changes that go well beyond the scope of propagating the isbn field fix. This should be addressed as a dedicated follow-up PR.

2. Pinning dependency versions (Informational – Security): All requirements.txt entries still use >= lower bounds. Generating a locked requirements file requires running pip-compile or poetry lock and validating against the full test suite; this is a separate operational task and no new dependencies were introduced.

3. models.py unnecessary pass in Base (Code Quality pylint W0107): Pre-existing style issue; removing it has zero functional impact and was not part of the PR diff.