fix: automated review of PR #24

.env

@@ -1,3 +1,4 @@
 POSTGRES_DB=app
 POSTGRES_USER=user
 POSTGRES_PASSWORD=secret
+DATABASE_URL=postgresql://user:secret@localhost/app

.gitignore

@@ -1,3 +1,4 @@
+.env
 .idea
 .ipynb_checkpoints
 .mypy_cache

Dockerfile

@@ -1,7 +1,10 @@
-FROM python:3.11
-RUN apt-get update && apt-get install -y libpq-dev
+FROM python:3.11.13-slim
+RUN apt-get update && apt-get install -y --no-install-recommends libpq-dev \
+    && rm -rf /var/lib/apt/lists/*
+RUN adduser --disabled-password --gecos "" appuser
 COPY . /app
 WORKDIR /app
-RUN pip install -r requirements.txt
+RUN pip install --no-cache-dir -r requirements.txt
+USER appuser
 EXPOSE 8000
-CMD ["uvicorn", "main:app", "--host", "0.0.0.0", "--port", "8000", "--log-level", "trace"]
+CMD ["uvicorn", "main:app", "--host", "0.0.0.0", "--port", "8000", "--log-level", "warning"]

conftest.py

@@ -1,18 +1,20 @@
 import pytest
-from sqlalchemy import create_engine, inspect, text
+from sqlalchemy import create_engine
 from sqlalchemy.orm import sessionmaker
 from fastapi.testclient import TestClient
+
 from main import create_app
 from dependencies import get_db, database_url
 from models import Base
 
+
 @pytest.fixture(scope="session")
 def test_engine():
     """Create test database engine"""
-    test_engine = create_engine(database_url)
-    Base.metadata.create_all(bind=test_engine)
-    yield test_engine
-    Base.metadata.drop_all(bind=test_engine)
+    engine = create_engine(database_url)
+    Base.metadata.create_all(bind=engine)
+    yield engine
+    Base.metadata.drop_all(bind=engine)
 
 
 @pytest.fixture(scope="function")
@@ -21,8 +23,10 @@ def test_db(test_engine):
     connection = test_engine.connect()
     transaction = connection.begin()
     db = sessionmaker(
-        autocommit=False, autoflush=False, bind=connection,
-        join_transaction_mode="create_savepoint"
+        autocommit=False,
+        autoflush=False,
+        bind=connection,
+        join_transaction_mode="create_savepoint",
     )()
     try:
         yield db
@@ -33,18 +37,14 @@ def test_db(test_engine):
 
 
 @pytest.fixture
-def test_app(test_engine):
-    """Create test FastAPI application with test database"""
+def test_app(test_db):
+    """Create test FastAPI application with test database using transactional rollback"""
 
     def override_get_db():
-        TestingSessionLocal = sessionmaker(
-            autocommit=False, autoflush=False, bind=test_engine
-        )
-        db = TestingSessionLocal()
         try:
-            yield db
+            yield test_db
         finally:
-            db.close()
+            pass  # session lifecycle managed by test_db fixture
 
     app = create_app()
     app.dependency_overrides[get_db] = override_get_db

dependencies.py

@@ -1,11 +1,16 @@
-from typing import Generator
+import logging
 import os
+from typing import Generator
+
 from dotenv import load_dotenv
 from sqlalchemy import create_engine
-from sqlalchemy.orm import Session, sessionmaker
 from sqlalchemy.exc import SQLAlchemyError
+from sqlalchemy.orm import Session, sessionmaker
+
 from models import Base
 
+logger = logging.getLogger(__name__)
+
 load_dotenv()
 database_url = os.getenv("DATABASE_URL")
 if not database_url:
@@ -18,12 +23,12 @@
 SessionLocal = sessionmaker(autocommit=False, autoflush=False, bind=engine)
 
 
-def init_db():
+def init_db() -> None:
     """Initialize the database by creating all tables."""
     try:
         Base.metadata.create_all(bind=engine)
     except SQLAlchemyError as e:
-        print(f"Error initializing the database: {e}")
+        logger.error("Error initializing the database: %s", e)
         raise
 
 

docker-compose.yml

@@ -24,7 +24,7 @@ services:
       POSTGRES_USER: ${POSTGRES_USER}
       POSTGRES_PASSWORD: ${POSTGRES_PASSWORD}
     ports:
-      - "5432:5432"
+      - "127.0.0.1:5432:5432"
     volumes:
       - postgres_data:/var/lib/postgresql/data
       - ./db.sql:/docker-entrypoint-initdb.d/db.sql

main.py

@@ -1,6 +1,6 @@
-from fastapi import FastAPI, Depends
+from fastapi import FastAPI
 from routers import router
-from dependencies import init_db, get_db
+from dependencies import init_db
 
 
 def create_app() -> FastAPI:
@@ -10,8 +10,8 @@ def create_app() -> FastAPI:
     # Initialize database
     init_db()
 
-    # Include routers
-    app.include_router(router, prefix="/api", dependencies=[Depends(get_db)])
+    # Include routers — get_db is declared at the route level, not here
+    app.include_router(router, prefix="/api")
 
     return app
 

models.py

@@ -1,14 +1,14 @@
+import re
+from typing import Optional
 from sqlalchemy.orm import DeclarativeBase, Mapped, mapped_column
 from sqlalchemy import String
-from pydantic import BaseModel, ConfigDict
+from pydantic import BaseModel, ConfigDict, field_validator
 
 
 # SQLAlchemy models
 class Base(DeclarativeBase):
     """Base class for all database models"""
 
-    pass
-
 
 class Book(Base):
     """Book model"""
@@ -18,13 +18,28 @@ class Book(Base):
     id: Mapped[int] = mapped_column(primary_key=True, index=True)
     title: Mapped[str] = mapped_column(String(255), index=True)
     author: Mapped[str] = mapped_column(String(255))
+    isbn: Mapped[str] = mapped_column(String(13))
+
 
 # Pydantic models
 class BookIn(BaseModel):
     """Pydantic model for book input"""
 
     title: str
     author: str
+    isbn: str
+
+    @field_validator("isbn")
+    @classmethod
+    def validate_isbn(cls, v: str) -> str:
+        """Validate ISBN-10 or ISBN-13 format."""
+        cleaned = v.replace("-", "").replace(" ", "")
+        if not re.fullmatch(r"(?:97[89])?\d{9}[\dX]", cleaned):
+            raise ValueError(
+                "isbn must be a valid ISBN-10 (10 digits, last may be X) "
+                "or ISBN-13 (starts with 978 or 979, 13 digits)"
+            )
+        return cleaned
 
 
 class BookOut(BaseModel):
@@ -33,5 +48,6 @@ class BookOut(BaseModel):
     id: int
     title: str
     author: str
+    isbn: str
 
     model_config = ConfigDict(from_attributes=True)

repositories.py

@@ -2,39 +2,48 @@
 import models
 
 
-# Create a new book
-def create_book(db: Session, book: models.BookIn):
-    db_book = models.Book(title=book.title, author=book.author)
+def create_book(db: Session, book: models.BookIn) -> models.Book:
+    """Create a new book record in the database."""
+    db_book = models.Book(title=book.title, author=book.author, isbn=book.isbn)
     db.add(db_book)
     db.commit()
     db.refresh(db_book)
     return db_book
 
 
-# Get all books
-def get_books(db: Session, skip: int = 0, limit: int = 10):
+def get_books(db: Session, skip: int = 0, limit: int = 10) -> list[models.Book]:
+    """Return a paginated list of all books."""
     return db.query(models.Book).offset(skip).limit(limit).all()
 
 
-# Get a book by ID
-def get_book(db: Session, book_id: int):
+def get_book(db: Session, book_id: int) -> models.Book | None:
+    """Return a single book by primary key, or None if not found."""
     return db.query(models.Book).filter(models.Book.id == book_id).first()
 
 
-# Update a book
-def update_book(db: Session, book_id: int, book: models.BookIn):
+def update_book(
+    db: Session, book_id: int, book: models.BookIn
+) -> models.Book | None:
+    """Update an existing book by primary key.
+
+    Returns the updated book, or None if the book does not exist.
+    """
     db_book = db.query(models.Book).filter(models.Book.id == book_id).first()
     if db_book:
         db_book.title = book.title
         db_book.author = book.author
+        db_book.isbn = book.isbn
         db.commit()
         db.refresh(db_book)
         return db_book
     return None
 
 
-# Delete a book
-def delete_book(db: Session, book_id: int):
+def delete_book(db: Session, book_id: int) -> models.Book | None:
+    """Delete a book by primary key.
+
+    Returns the deleted book object, or None if not found.
+    """
     db_book = db.query(models.Book).filter(models.Book.id == book_id).first()
     if db_book:
         db.delete(db_book)

routers.py

@@ -1,10 +1,15 @@
-from fastapi import APIRouter, Depends, HTTPException, status
-from sqlalchemy.orm import Session
+import logging
 from typing import List
+
+from fastapi import APIRouter, Depends, HTTPException, Query, status
+from sqlalchemy.orm import Session
+
 import models
 import repositories
 from dependencies import get_db
 
+logger = logging.getLogger(__name__)
+
 # Create router with prefix
 router = APIRouter()
 
@@ -13,28 +18,41 @@
     "/books/", response_model=models.BookOut, status_code=status.HTTP_201_CREATED
 )
 def create_book(book: models.BookIn, db: Session = Depends(get_db)):
+    """Create a new book."""
     try:
         return repositories.create_book(db=db, book=book)
     except Exception as e:
-        raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(e))
+        logger.exception("Error creating book")
+        raise HTTPException(
+            status_code=status.HTTP_400_BAD_REQUEST,
+            detail="An error occurred processing your request",
+        ) from e
 
 
 @router.get(
     "/books/", response_model=List[models.BookOut], status_code=status.HTTP_200_OK
 )
-def get_books(skip: int = 0, limit: int = 10, db: Session = Depends(get_db)):
+def get_books(
+    skip: int = 0,
+    limit: int = Query(default=10, ge=1, le=100),
+    db: Session = Depends(get_db),
+):
+    """Return a paginated list of books."""
     try:
         return repositories.get_books(db=db, skip=skip, limit=limit)
     except Exception as e:
+        logger.exception("Error retrieving books")
         raise HTTPException(
-            status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail=str(e)
-        )
+            status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
+            detail="An error occurred processing your request",
+        ) from e
 
 
 @router.get(
     "/books/{book_id}", response_model=models.BookOut, status_code=status.HTTP_200_OK
 )
 def get_book(book_id: int, db: Session = Depends(get_db)):
+    """Return a single book by ID."""
     db_book = repositories.get_book(db=db, book_id=book_id)
     if db_book is None:
         raise HTTPException(
@@ -47,6 +65,7 @@ def get_book(book_id: int, db: Session = Depends(get_db)):
     "/books/{book_id}", response_model=models.BookOut, status_code=status.HTTP_200_OK
 )
 def update_book(book_id: int, book: models.BookIn, db: Session = Depends(get_db)):
+    """Update an existing book by ID."""
     db_book = repositories.update_book(db=db, book_id=book_id, book=book)
     if db_book is None:
         raise HTTPException(
@@ -59,6 +78,7 @@ def update_book(book_id: int, book: models.BookIn, db: Session = Depends(get_db)
     "/books/{book_id}", response_model=models.BookOut, status_code=status.HTTP_200_OK
 )
 def delete_book(book_id: int, db: Session = Depends(get_db)):
+    """Delete a book by ID."""
     db_book = repositories.delete_book(db=db, book_id=book_id)
     if db_book is None:
         raise HTTPException(