fix: automated review of PR #24

Dockerfile

@@ -1,7 +1,9 @@
-FROM python:3.11
-RUN apt-get update && apt-get install -y libpq-dev
+FROM python:3.11-slim
+RUN apt-get update && apt-get install -y --no-install-recommends libpq-dev && rm -rf /var/lib/apt/lists/*
 COPY . /app
 WORKDIR /app
 RUN pip install -r requirements.txt
+RUN useradd -r appuser && chown -R appuser /app
+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"]

dependencies.py

@@ -1,3 +1,4 @@
+import logging
 from typing import Generator
 import os
 from dotenv import load_dotenv
@@ -6,6 +7,8 @@
 from sqlalchemy.exc import SQLAlchemyError
 from models import Base
 
+logger = logging.getLogger(__name__)
+
 load_dotenv()
 database_url = os.getenv("DATABASE_URL")
 if not database_url:
@@ -23,7 +26,7 @@ def init_db():
     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

models.py

@@ -1,14 +1,13 @@
+import re
 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 +17,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(17), nullable=False)
+
 
 # Pydantic models
 class BookIn(BaseModel):
     """Pydantic model for book input"""
 
     title: str
     author: str
+    isbn: str
+
+    @field_validator("isbn")
+    @classmethod
+    def validate_isbn(cls, value: str) -> str:
+        """Validate ISBN-10 or ISBN-13 format (digits and hyphens, correct length)."""
+        # Strip hyphens for length/digit checks
+        digits = value.replace("-", "")
+        if not re.fullmatch(r"[0-9X]+", digits, re.IGNORECASE):
+            raise ValueError("ISBN must contain only digits, hyphens, or a trailing X")
+        if len(digits) not in (10, 13):
+            raise ValueError("ISBN must be 10 or 13 digits (excluding hyphens)")
+        return value
 
 
 class BookOut(BaseModel):
@@ -33,5 +47,6 @@ class BookOut(BaseModel):
     id: int
     title: str
     author: str
+    isbn: str
 
     model_config = ConfigDict(from_attributes=True)

repositories.py

@@ -4,7 +4,7 @@
 
 # Create a new book
 def create_book(db: Session, book: models.BookIn):
-    db_book = models.Book(title=book.title, author=book.author)
+    db_book = models.Book(title=book.title, author=book.author, isbn=book.isbn)
     db.add(db_book)
     db.commit()
     db.refresh(db_book)
@@ -27,6 +27,7 @@ def update_book(db: Session, book_id: int, book: models.BookIn):
     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

routers.py

@@ -1,10 +1,12 @@
-from fastapi import APIRouter, Depends, HTTPException, status
+import logging
+from fastapi import APIRouter, Depends, HTTPException, Query, status
 from sqlalchemy.orm import Session
-from typing import List
 import models
 import repositories
 from dependencies import get_db
 
+logger = logging.getLogger(__name__)
+
 # Create router with prefix
 router = APIRouter()
 
@@ -16,19 +18,29 @@ def create_book(book: models.BookIn, db: Session = Depends(get_db)):
     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: %s", e)
+        raise HTTPException(
+            status_code=status.HTTP_400_BAD_REQUEST,
+            detail="Could not create book",
+        ) from e
 
 
 @router.get(
-    "/books/", response_model=List[models.BookOut], status_code=status.HTTP_200_OK
+    "/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 = Query(default=0, ge=0),
+    limit: int = Query(default=10, ge=1, le=100),
+    db: Session = Depends(get_db),
+):
     try:
         return repositories.get_books(db=db, skip=skip, limit=limit)
     except Exception as e:
+        logger.exception("Error retrieving books: %s", e)
         raise HTTPException(
-            status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail=str(e)
-        )
+            status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
+            detail="An internal error occurred",
+        ) from e
 
 
 @router.get(

test_main.py

@@ -1,14 +1,17 @@
+import pytest
+from sqlalchemy import inspect
+from fastapi import status
 from repositories import create_book, get_books, get_book, update_book, delete_book
 from models import BookIn
-from sqlalchemy import create_engine, inspect
 
 
-# Test data constants
+# Test data constants — include isbn field (valid ISBN-13 values)
 TEST_BOOKS = [
-    {"title": "Carrie", "author": "Stephen King"},
-    {"title": "Ready Player One", "author": "Ernest Cline"},
+    {"title": "Carrie", "author": "Stephen King", "isbn": "9780385533225"},
+    {"title": "Ready Player One", "author": "Ernest Cline", "isbn": "9780307887436"},
 ]
 
+
 class TestMainApp:
     def test_create_app(self, test_app):
         """Test application creation"""
@@ -19,13 +22,15 @@ def test_database_initialization(self, test_engine):
         inspector = inspect(test_engine)
         assert "books" in inspector.get_table_names()
 
+
 # Repository Tests
 class TestBookRepository:
     def test_create_book(self, test_db):
         """Test creating a new book"""
         book = create_book(test_db, BookIn(**TEST_BOOKS[0]))
         assert book.title == TEST_BOOKS[0]["title"]
         assert book.author == TEST_BOOKS[0]["author"]
+        assert book.isbn == TEST_BOOKS[0]["isbn"]
         assert book.id is not None
 
     def test_get_books(self, test_db):
@@ -34,17 +39,33 @@ def test_get_books(self, test_db):
         book2 = create_book(test_db, BookIn(**TEST_BOOKS[1]))
 
         books = get_books(test_db)
-        #assert len(books) >= 2
+        assert len(books) >= 2
         assert any(b.id == book1.id for b in books)
         assert any(b.id == book2.id for b in books)
 
+    def test_get_books_pagination(self, test_db):
+        """Test pagination parameters (skip / limit) in get_books"""
+        create_book(test_db, BookIn(**TEST_BOOKS[0]))
+        create_book(test_db, BookIn(**TEST_BOOKS[1]))
+
+        # limit=1 should return exactly one book
+        books = get_books(test_db, skip=0, limit=1)
+        assert len(books) == 1
+
+        # skip=1, limit=1 should return the second book only
+        all_books = get_books(test_db)
+        books_skipped = get_books(test_db, skip=1, limit=1)
+        assert len(books_skipped) == 1
+        assert books_skipped[0].id != books[0].id
+
     def test_get_book(self, test_db):
         """Test getting a specific book"""
         created_book = create_book(test_db, BookIn(**TEST_BOOKS[0]))
         retrieved_book = get_book(test_db, created_book.id)
         assert retrieved_book is not None
         assert retrieved_book.id == created_book.id
         assert retrieved_book.title == TEST_BOOKS[0]["title"]
+        assert retrieved_book.isbn == TEST_BOOKS[0]["isbn"]
 
     def test_update_book(self, test_db):
         """Test updating a book"""
@@ -54,10 +75,11 @@ def test_update_book(self, test_db):
         assert updated_book is not None
         assert updated_book.title == TEST_BOOKS[1]["title"]
         assert updated_book.author == TEST_BOOKS[1]["author"]
+        assert updated_book.isbn == TEST_BOOKS[1]["isbn"]
 
     def test_delete_book(self, test_db):
         """Test deleting a book"""
-        book = create_book(test_db, BookIn(title="To Delete", author="Author"))
+        book = create_book(test_db, BookIn(title="To Delete", author="Author", isbn="9780307887436"))
         deleted_book = delete_book(test_db, book.id)
 
         assert deleted_book is not None
@@ -67,5 +89,118 @@ def test_delete_book(self, test_db):
     def test_nonexistent_operations(self, test_db):
         """Test operations on nonexistent books"""
         assert get_book(test_db, 999999) is None
-        assert update_book(test_db, 999999, BookIn(title="Test", author="Test")) is None
+        assert update_book(test_db, 999999, BookIn(title="Test", author="Test", isbn="9780307887436")) is None
         assert delete_book(test_db, 999999) is None
+
+
+# ISBN validation tests
+class TestIsbnValidation:
+    def test_valid_isbn13(self):
+        """BookIn accepts a valid ISBN-13"""
+        book = BookIn(title="T", author="A", isbn="9780385533225")
+        assert book.isbn == "9780385533225"
+
+    def test_valid_isbn13_with_hyphens(self):
+        """BookIn accepts a hyphenated ISBN-13"""
+        book = BookIn(title="T", author="A", isbn="978-0-385-53322-5")
+        assert book.isbn == "978-0-385-53322-5"
+
+    def test_valid_isbn10(self):
+        """BookIn accepts a valid ISBN-10"""
+        book = BookIn(title="T", author="A", isbn="0306406152")
+        assert book.isbn == "0306406152"
+
+    def test_invalid_isbn_too_short(self):
+        """BookIn rejects an ISBN that is too short"""
+        with pytest.raises(Exception):
+            BookIn(title="T", author="A", isbn="12345")
+
+    def test_invalid_isbn_too_long(self):
+        """BookIn rejects an ISBN that is too long"""
+        with pytest.raises(Exception):
+            BookIn(title="T", author="A", isbn="12345678901234")
+
+    def test_invalid_isbn_non_numeric(self):
+        """BookIn rejects an ISBN with invalid characters"""
+        with pytest.raises(Exception):
+            BookIn(title="T", author="A", isbn="97803854HELLO")
+
+
+# Router / HTTP integration tests
+class TestBookRoutes:
+    def test_create_book_returns_201(self, client):
+        """POST /api/books/ with valid payload returns 201 and the created book"""
+        payload = {"title": "Dune", "author": "Frank Herbert", "isbn": "9780441013593"}
+        response = client.post("/api/books/", json=payload)
+        assert response.status_code == status.HTTP_201_CREATED
+        data = response.json()
+        assert data["title"] == payload["title"]
+        assert data["author"] == payload["author"]
+        assert data["isbn"] == payload["isbn"]
+        assert "id" in data
+
+    def test_create_book_missing_isbn_returns_422(self, client):
+        """POST /api/books/ without isbn returns 422 Unprocessable Entity"""
+        payload = {"title": "Dune", "author": "Frank Herbert"}
+        response = client.post("/api/books/", json=payload)
+        assert response.status_code == 422
+
+    def test_create_book_invalid_isbn_returns_422(self, client):
+        """POST /api/books/ with an invalid isbn returns 422"""
+        payload = {"title": "Dune", "author": "Frank Herbert", "isbn": "NOTANISBN"}
+        response = client.post("/api/books/", json=payload)
+        assert response.status_code == 422
+
+    def test_get_books_returns_200(self, client):
+        """GET /api/books/ returns 200 and a list"""
+        response = client.get("/api/books/")
+        assert response.status_code == status.HTTP_200_OK
+        assert isinstance(response.json(), list)
+
+    def test_get_books_limit_cap(self, client):
+        """GET /api/books/ with limit > 100 returns 422"""
+        response = client.get("/api/books/?limit=10000000")
+        assert response.status_code == 422
+
+    def test_get_book_not_found_returns_404(self, client):
+        """GET /api/books/{id} for unknown id returns 404"""
+        response = client.get("/api/books/999999")
+        assert response.status_code == status.HTTP_404_NOT_FOUND
+
+    def test_get_book_returns_isbn(self, client):
+        """GET /api/books/{id} response includes isbn field"""
+        payload = {"title": "Foundation", "author": "Isaac Asimov", "isbn": "9780553293357"}
+        created = client.post("/api/books/", json=payload).json()
+        response = client.get(f"/api/books/{created['id']}")
+        assert response.status_code == status.HTTP_200_OK
+        assert response.json()["isbn"] == payload["isbn"]
+
+    def test_update_book_returns_updated_data(self, client):
+        """PUT /api/books/{id} updates and returns the book"""
+        payload = {"title": "Old Title", "author": "Old Author", "isbn": "9780385533225"}
+        created = client.post("/api/books/", json=payload).json()
+        update_payload = {"title": "New Title", "author": "New Author", "isbn": "9780441013593"}
+        response = client.put(f"/api/books/{created['id']}", json=update_payload)
+        assert response.status_code == status.HTTP_200_OK
+        data = response.json()
+        assert data["title"] == "New Title"
+        assert data["isbn"] == "9780441013593"
+
+    def test_update_book_not_found_returns_404(self, client):
+        """PUT /api/books/{id} for unknown id returns 404"""
+        payload = {"title": "T", "author": "A", "isbn": "9780385533225"}
+        response = client.put("/api/books/999999", json=payload)
+        assert response.status_code == status.HTTP_404_NOT_FOUND
+
+    def test_delete_book_returns_deleted_book(self, client):
+        """DELETE /api/books/{id} returns the deleted book"""
+        payload = {"title": "To Delete", "author": "Author", "isbn": "9780441013593"}
+        created = client.post("/api/books/", json=payload).json()
+        response = client.delete(f"/api/books/{created['id']}")
+        assert response.status_code == status.HTTP_200_OK
+        assert response.json()["id"] == created["id"]
+
+    def test_delete_book_not_found_returns_404(self, client):
+        """DELETE /api/books/{id} for unknown id returns 404"""
+        response = client.delete("/api/books/999999")
+        assert response.status_code == status.HTTP_404_NOT_FOUND