Add ISBN field to book model#24
Conversation
Automated Code Review — PR #24Reviewed at: 11-05-2026 19:28:35 UTC Security ReviewI now have all the information needed to produce the complete security review. Here is the result: {
"summary": "The PR introduces a single-line change adding an `isbn` field to the SQLAlchemy `Book` model, which is low-risk on its own; however, the broader codebase contains several pre-existing issues—most notably a plaintext `.env` credential file committed to the repository, an unpinned dependency supply chain, and a container running as root—that together represent a moderate overall security risk.",
"findings": {
"critical": [
".env:1-3 — Plaintext credentials committed to the repository: `POSTGRES_PASSWORD=secret` (and associated DB user/name) are stored in `.env`, which is NOT listed in `.gitignore` and is therefore tracked by git and present in the commit history. Any clone of this repo exposes the database password. Fix: add `.env` to `.gitignore` immediately, rotate the credential, and provide a `.env.example` template with placeholder values instead."
],
"warnings": [
"models.py:21 — Schema / data-model mismatch (introduced by this PR): The new `isbn` column is declared `nullable=False` in the SQLAlchemy ORM model but the `BookIn` Pydantic input schema does NOT include an `isbn` field. This means every `create_book` call will attempt to INSERT a row without supplying a value for a NOT NULL column, causing a database integrity error at runtime. Additionally, `repositories.py:7` constructs `models.Book(title=book.title, author=book.author)` and never passes `isbn`, guaranteeing a constraint violation. Fix: add `isbn: str` to `BookIn` (and `BookOut`), and pass `isbn=book.isbn` in `repositories.create_book`.",
"Dockerfile:7 — Container runs as root (semgrep: CWE-250): No `USER` directive is set, so the application process runs as root inside the container. If the process is compromised, the attacker gains root-level container access. Fix: add `RUN useradd -m appuser` and `USER appuser` before the `CMD` instruction.",
"Dockerfile:7 — Verbose log level in production image: `--log-level trace` is baked into the production Docker image CMD. Trace-level logs may expose sensitive request/response data, database queries, and internal stack traces. Fix: change to `--log-level info` (or `warning`) and allow the level to be overridden via an environment variable.",
"routers.py:18 — Internal exception details leaked to API clients: `detail=str(e)` on the `HTTP_400_BAD_REQUEST` handler for `create_book` propagates raw exception messages (which may include SQL error text, table names, or constraint names) to the caller. Fix: log the exception server-side and return a generic message such as `\"Invalid request data\"`.",
"routers.py:28-29 — Internal exception details leaked to API clients: same pattern in the `get_books` 500-handler. Fix: log `e` and return a generic `\"Internal server error\"` message.",
"requirements.txt:1-7 — All dependencies are unpinned (minimum-version specifiers only): This exposes the project to supply-chain attacks where a malicious or broken future release is pulled automatically. Safety/pip-audit also warned about this. Fix: pin all dependencies to exact versions (e.g. via `pip freeze > requirements.txt`) and use a lock file.",
"docker-compose.yml:24 — PostgreSQL port 5432 exposed on all host interfaces: `ports: - \"5432:5432\"` makes the database directly reachable from outside the Docker network. Fix: remove the `ports` mapping for the `db` service (or bind to `127.0.0.1:5432:5432`) so only the `web` service can reach it via the internal Docker network."
],
"informational": [
"models.py:21 — The `isbn` field uses `String(255)` which is significantly larger than needed; a standard ISBN-13 is 13 characters. Consider `String(13)` with a validator to enforce format.",
"models.py:21 — No uniqueness constraint on `isbn`: ISBNs should be globally unique identifiers. Consider adding `unique=True` to the column definition.",
"routers.py:24 — No maximum cap on the `limit` query parameter for `GET /books/`: a client can request an arbitrarily large result set. Consider capping at a reasonable value (e.g. 100) to prevent resource exhaustion.",
"main.py:8 — FastAPI is instantiated without disabling the auto-generated `/docs` and `/redoc` endpoints. These should be disabled in production (`docs_url=None, redoc_url=None`) to avoid exposing the API schema publicly.",
".gitignore — `.env` is not listed in `.gitignore`, which allowed the credentials file to be committed. This should be corrected even after the file is removed from tracking.",
"Dockerfile:1 — Base image `python:3.11` is not pinned to a specific digest or patch version (e.g. `python:3.11.12-slim`). Using a digest pin prevents silent base-image changes. Consider also using a `-slim` variant to reduce the attack surface."
]
},
"tool_output": "bandit: 19 LOW-severity findings, all B101 (assert used in test file test_main.py) — no production code issues found. semgrep: 1 blocking finding — Dockerfile:7 missing USER directive (CWE-250, OWASP A04:2021). trivy: no vulnerabilities or secrets detected in filesystem scan. gitleaks: binary not found, could not run. pip-audit: no known vulnerabilities found. safety: 0 vulnerabilities reported; warned that all 7 packages (fastapi, sqlalchemy, psycopg2-binary, pydantic, pytest, python-dotenv, uvicorn) are unpinned.",
"positive_observations": "Database access is handled exclusively through SQLAlchemy ORM with parameterized queries, eliminating SQL injection risk. The `DATABASE_URL` is sourced from an environment variable with a hard startup failure if absent, preventing accidental plaintext connection strings in code. Session lifecycle management in `dependencies.py` correctly uses a generator with a `finally` block to ensure connections are always closed. Pydantic models enforce request body structure and type validation on all endpoints. HTTP status codes are used correctly throughout (201 for creation, 404 for not-found, etc.). Docker Compose uses a healthcheck on the database service before starting the web container, reducing race-condition startup failures."
}Code Quality ReviewI now have all the information needed. Let me compile the full review: {
"summary": "The PR adds an `isbn` field to the `Book` SQLAlchemy model, but the change is critically incomplete: the field is never surfaced in the Pydantic I/O schemas (`BookIn`/`BookOut`), is never written in the repository `create_book`/`update_book` functions, and carries a redundant `nullable=False` annotation that conflicts with the `Mapped[str]` type hint which already implies non-null.",
"findings": {
"critical": [
"models.py:21 — `isbn` is added to the `Book` ORM model but is absent from both `BookIn` and `BookOut` Pydantic schemas, so the field can never be supplied by callers and is never returned in API responses; every `POST /books/` request will fail at the database level with an NOT NULL constraint violation because `repositories.create_book` does not populate `isbn`. Fix: add `isbn: str` to `BookIn` and `BookOut`, and pass `isbn=book.isbn` in `create_book` and `update_book` in repositories.py.",
"repositories.py:7 — `create_book` constructs `models.Book(title=book.title, author=book.author)` and never sets `isbn`, which will cause an IntegrityError / NOT NULL violation at commit time now that the column is `nullable=False`. Fix: add `isbn=book.isbn` once `BookIn` exposes the field.",
"repositories.py:27-28 — `update_book` similarly only updates `title` and `author`, leaving `isbn` stale after a PUT. Fix: add `db_book.isbn = book.isbn` once `BookIn` exposes the field."
],
"warnings": [
"models.py:21 — `nullable=False` is redundant when the column type is `Mapped[str]` (non-optional); SQLAlchemy 2.x already infers NOT NULL from the non-optional `Mapped` annotation. The redundancy can cause confusion about the source of truth. Fix: remove `nullable=False`.",
"models.py:21 — ISBN-13 has a fixed maximum length of 13 characters; `String(255)` is far too wide and wastes storage. Fix: use `String(13)` and add a validator in `BookIn` (e.g. `@field_validator`) to enforce format.",
"routers.py:19 — `raise HTTPException(...) from e` is missing (bare `raise ... from e` pattern omitted); pylint W0707 flags this. The original exception chain is silently dropped, making debugging harder. Fix: use `raise HTTPException(...) from e`.",
"routers.py:29 — Same `raise-missing-from` issue in the `get_books` exception handler. Fix: use `raise HTTPException(...) from e`.",
"routers.py:17-18 — Catching bare `Exception` in `create_book` and mapping it unconditionally to HTTP 400 is misleading; a database connectivity error is not a bad-request. Fix: catch specific exceptions (e.g. `sqlalchemy.exc.IntegrityError` → 409 Conflict, `sqlalchemy.exc.SQLAlchemyError` → 500).",
"main.py:12 — `dependencies=[Depends(get_db)]` on `app.include_router` opens a second DB session per request in addition to the one opened by each route's own `db: Session = Depends(get_db)` parameter, doubling the connection overhead. Fix: remove the router-level dependency; each route already declares its own `db` dependency.",
"conftest.py:2 — `inspect` and `text` are imported but never used (ruff F401). Fix: remove them.",
"test_main.py:3 — `create_engine` is imported but never used (ruff F401 / pylint W0611). Fix: remove it.",
"conftest.py:12 — The `test_engine` fixture rebinds the name `test_engine` inside its own scope (pylint W0621 `redefined-outer-name`), shadowing the fixture parameter. Fix: rename the inner variable (e.g. `engine`).",
"conftest.py:36/55 — Same shadow issue with `test_engine` and `test_app` in subsequent fixtures. Fix: rename inner variables."
],
"informational": [
"models.py:22 — Missing blank line between the `Book` class and the `# Pydantic models` comment (PEP 8 requires two blank lines between top-level definitions).",
"routers.py:3 — `from typing import List` should be a stdlib import placed before third-party imports (pylint C0411); in Python 3.9+ `list[...]` can be used directly without the import.",
"test_main.py:35 — `#assert len(books) >= 2` is commented-out dead code. Either re-enable the assertion or remove the line.",
"routers.py:9 — The comment `# Create router with prefix` is inaccurate; no prefix is set on the `APIRouter()` itself (the prefix `/api` is applied in `main.py`). Remove or correct the comment.",
"test_main.py:12,23 — Test classes are missing docstrings (pylint C0115); minor style issue.",
"models.py:7-10 — `Base(DeclarativeBase)` has an empty `pass` body (pylint W0107); since it has a docstring the `pass` is unnecessary. Fix: remove the `pass`.",
"conftest.py:40 — `TestingSessionLocal` does not follow snake_case naming convention for a local variable (pylint C0103). Fix: rename to `testing_session_local` or `session_factory`."
]
},
"tool_output": "ruff: 3 errors — conftest.py:2 unused imports `inspect` and `text` (F401); test_main.py:3 unused import `create_engine` (F401). All auto-fixable.\n\npylint (score 1.53/10): routers.py:19,29 W0707 raise-missing-from (exception chain dropped); models.py:10 W0107 unnecessary pass; conftest.py:12,19,36 W0621 redefined-outer-name (`test_engine`); conftest.py:55 W0621 redefined-outer-name (`test_app`); conftest.py:2 W0611 unused imports; test_main.py:3 W0611 unused import; conftest.py:40 C0103 invalid variable name; routers.py:3 C0411 wrong import order; main.py:8 W0621 redefined-outer-name (`app` inside `create_app` shadows module-level `app`); widespread C0114/C0115/C0116 missing docstrings.\n\nmypy: Could not analyse — package name `books-api` is not a valid Python identifier, causing mypy to abort.",
"positive_observations": "The project follows a clean layered architecture (models → repositories → routers → main) with proper separation of concerns. The use of SQLAlchemy 2.x `Mapped`/`mapped_column` annotations and Pydantic v2 `ConfigDict(from_attributes=True)` reflects modern best practices. The `get_db` generator with a `finally` block correctly ensures session cleanup. The test fixtures use savepoint-based rollback for proper test isolation, and the factory-function pattern (`create_app`) makes dependency overriding straightforward in tests."
}Test Coverage ReviewI now have all the information needed to produce the full review. Here is the result: {
"summary": "The PR adds a NOT NULL `isbn` column to the `Book` SQLAlchemy model but fails to update `BookIn` (Pydantic input schema), `repositories.create_book`/`update_book`, or any test fixture — causing 5 of 8 tests to fail immediately with an IntegrityError. There are no tests at all for the HTTP layer (routers.py is only 46% covered), the new field is entirely untested, and several other structural issues exist.",
"findings": {
"critical": [
"models.py:20 — `isbn` added as `NOT NULL` to the ORM model but `BookIn` (Pydantic input schema) was NOT updated to include `isbn`; `repositories.create_book` still only sets `title` and `author`, so every INSERT violates the NOT NULL constraint. Fix: add `isbn: str` to `BookIn`, pass it through `repositories.create_book` and `repositories.update_book`, and update `BookOut` to expose it.",
"test_main.py:26,33,43,51,60 — 5 of 8 tests fail at runtime with `sqlalchemy.exc.IntegrityError: NOT NULL constraint failed: books.isbn`. The entire `TestBookRepository` class is broken because the test fixtures (`TEST_BOOKS`, inline `BookIn(...)` calls) do not supply an `isbn` value. Fix: add `isbn` to every `BookIn` instantiation in tests and test-data constants.",
"test_main.py (entire file) — Zero HTTP-layer tests exist; `routers.py` is only 46% covered (all route bodies are uncovered). The `client` fixture is defined in `conftest.py` but never used in any test. Fix: add integration tests using the `client` fixture covering POST /books/, GET /books/, GET /books/{id}, PUT /books/{id}, DELETE /books/{id}, and their respective 404 error paths."
],
"warnings": [
"repositories.py:7 — `create_book` does not accept or persist the new `isbn` field (`db_book = models.Book(title=book.title, author=book.author)`). Even after `BookIn` is fixed, this line must be updated to pass `isbn=book.isbn`. Same issue applies to `update_book` (line 26) which does not update `isbn` on edit.",
"models.py:20 / models.py:30-35 — `BookOut` does not include `isbn`. Any API response will silently drop the field that was just added. Fix: add `isbn: str` to `BookOut`.",
"test_main.py:35 — `assert len(books) >= 2` is commented out (`#assert len(books) >= 2`) with no explanation, weakening the `test_get_books` assertion. The remaining assertions only check that the two created IDs appear somewhere in the list; pagination side-effects or test-ordering issues could go unnoticed. Fix: uncomment and rely on the savepoint-based rollback isolation already in place, or document why the assertion is intentionally skipped.",
"conftest.py:10-14 — The `test_engine` fixture uses `scope='session'` and points at the real `DATABASE_URL` (a Postgres URL that requires a running server, absent in CI). There is no fallback to an in-memory SQLite URL when `DATABASE_URL` is not a test-safe DSN. Fix: override `DATABASE_URL` to `sqlite:///./test.db` (or `sqlite:///:memory:`) unconditionally inside the fixture so tests are hermetic.",
"conftest.py:31-47 — The `test_app` fixture creates a plain `sessionmaker` without savepoint support, meaning HTTP-layer tests (once written) will not roll back between test runs and can leave data behind. Fix: use the same savepoint pattern as `test_db` for the HTTP override.",
"routers.py:26-29 — `get_books` catches all exceptions and returns 500; there are no tests exercising this error path. Fix: add a test that simulates a DB failure (e.g., mock `repositories.get_books` to raise) and asserts HTTP 500."
],
"informational": [
"test_main.py:12 — `TestMainApp.test_create_app` only asserts `test_app is not None`; this is a pure smoke test with no behavioral value. Consider removing it or replacing it with an assertion about registered routes.",
"test_main.py:7-9 — `TEST_BOOKS` constants use a plain dict; after adding `isbn`, keeping them as dicts means any future schema change will again silently miss fields. Consider using `BookIn` instances or typed dataclasses.",
"test_main.py:1 — Unused import `from sqlalchemy import create_engine, inspect` (both are already imported in `conftest.py`). Remove to reduce noise.",
"test_main.py — No test class or function covers HTTP status codes (201 on create, 404 on missing resource, 422 on invalid input). The naming convention is consistent and docstrings are present, which is good, but the class `TestMainApp` only contains infrastructure smoke tests rather than app-behavior tests.",
"conftest.py:2 — `text` is imported from sqlalchemy but never used in `conftest.py`. Remove unused import."
]
},
"tool_output": "============================= test session starts ==============================\nplatform linux -- Python 3.11.15, pytest-9.0.3, pluggy-1.6.0\nrootdir: /root/books-api\nconfigfile: pyproject.toml\nplugins: anyio-4.9.0, cov-7.1.0\ncollected 8 items\n\ntest_main.py ..FFFFF. [100%]\n\n5 failed, 3 passed in 4.20s\n\nName Stmts Miss Cover Missing\n-----------------------------------------------\n__init__.py 0 0 100%\nconftest.py 37 6 84% 40-47, 57\ndependencies.py 24 8 67% 12, 25-27, 32-36\nmain.py 9 0 100%\nmodels.py 19 0 100%\nrepositories.py 28 11 61% 10-11, 16, 28-32, 40-42\nrouters.py 37 20 46% 16-19, 26-29, 38-43, 50-55, 62-67\ntest_main.py 45 20 56% 27-29, 34-39, 44-47, 52-56, 61-65\n-----------------------------------------------\nTOTAL 199 65 67%",
"positive_observations": "The test infrastructure is well-structured: the savepoint-based rollback pattern in `test_db` ensures proper test isolation at the repository layer, each test function is clearly named and has a docstring, and the `test_nonexistent_operations` test correctly covers all three None-returning repository paths in a single test. The `conftest.py` cleanly separates engine, session, app, and client fixtures using appropriate scopes."
}Generated by review-swarm (job: f70cbe6a4752) |
Reviewed at: 12-05-2026 18:31:56 UTC Security reviewSummaryThis PR adds a single FindingsCritical
Warnings
Informational
Tool Outputbandit: 19 low-severity findings, all semgrep: 1 blocking finding — trivy: No vulnerabilities or secrets detected in filesystem scan (language-specific dependency lock file not present for deep scanning). gitleaks: Tool not available in this environment; scan skipped. pip-audit: 5 known vulnerabilities across 2 packages — safety: Confirmed the same 4 pip/wheel vulnerabilities. Path traversal and arbitrary file overwrite issues in the build toolchain. Positive ObservationsThe application correctly uses SQLAlchemy ORM parameterised queries throughout Generated by review-swarm (job: 60b9d3315b48) using anthropic/claude-sonnet-4-6 |
Reviewed at: 12-05-2026 18:31:56 UTC Code quality reviewSummaryThis PR adds an FindingsCritical
Warnings
Informational
Tool Outputruff: Found 3 fixable F401 (unused import) errors. pylint: Rated 1.53/10 (largely driven by pre-existing issues). Key findings relevant to the PR: mypy: Could not run — the directory name Positive ObservationsThe overall repository structure is clean and follows a reasonable layered architecture (models → repositories → routers → main), with a proper application factory pattern ( Generated by review-swarm (job: 60b9d3315b48) using anthropic/claude-sonnet-4-6 |
Reviewed at: 12-05-2026 18:31:56 UTC Test coverage reviewSummaryThe PR introduces a new non-nullable FindingsCritical
Warnings
Informational
Tool Outputpytest + coverage results Coverage per file
Positive ObservationsThe Generated by review-swarm (job: 60b9d3315b48) using anthropic/claude-sonnet-4-6 |
Automated developer fixesReviewed at: 12-05-2026 18:31:56 UTC All 18 tests pass and all changes are staged. Here is the structured summary: Security Fixes
Code Quality Fixes
Test Additions
Skipped Items
Fix PR: #36 Generated by review-swarm (job: 60b9d3315b48) using anthropic/claude-sonnet-4-6 |
|
Reviewed at: 12-05-2026 18:47:02 UTC Security reviewSummaryThis PR adds an FindingsCritical
Warnings
Informational
Tool Outputbandit: 19 low-severity findings, all semgrep: 1 blocking finding — trivy: No vulnerabilities or secrets detected in the filesystem scan (language-specific lockfile not found; only filesystem secrets scan completed cleanly). gitleaks: Tool not found in the environment ( pip-audit: 5 vulnerabilities in 2 packages — safety: 4 vulnerabilities reported — Positive ObservationsThe application uses SQLAlchemy ORM with parameterised queries throughout Generated by review-swarm (job: 111517a62bf3) using anthropic/claude-sonnet-4-6 |
|
Reviewed at: 12-05-2026 18:47:02 UTC Code quality reviewSummaryThis PR adds a single FindingsCritical
Warnings
Informational
Tool Outputruff: Found 3 fixable F401 errors — unused imports pylint: Rated 1.53/10. Key findings: multiple E0401 import errors (environment not installed, not true code issues); mypy: Could not run — reported Positive ObservationsThe repository pattern is cleanly separated from the router layer, and the router correctly uses FastAPI's dependency injection ( Generated by review-swarm (job: 111517a62bf3) using anthropic/claude-sonnet-4-6 |
|
Reviewed at: 12-05-2026 18:47:02 UTC Test coverage reviewSummaryThe PR adds an FindingsCritical
Warnings
Informational
Tool Outputpytest + coverage results (final 25 lines) Result: 5 failed, 3 passed in 0.14s Failures (all 5): Positive ObservationsThe Generated by review-swarm (job: 111517a62bf3) using anthropic/claude-sonnet-4-6 |
Automated developer fixesReviewed at: 12-05-2026 18:47:02 UTC 20/20 passing. Here is the structured summary: Security Fixes
Code Quality Fixes
Test Additions
Skipped Items
Fix PR: #37 Generated by review-swarm (job: 111517a62bf3) using anthropic/claude-sonnet-4-6 |
1 participant
No description provided.