review-fix: automated review of PR #24

.env.example

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

.gitignore

@@ -9,3 +9,8 @@
 
 # macOS
 .DS_Store
+
+# Environment / secrets — never commit real credentials
+.env
+*.pem
+*.key

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 pip install --no-cache-dir -r requirements.txt
+RUN useradd -r -u 1001 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"]

conftest.py

@@ -1,5 +1,5 @@
 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

models.py

@@ -1,6 +1,7 @@
+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
@@ -18,13 +19,29 @@ 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(20), unique=True)
+
 
 # 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 (digits and hyphens, optional X suffix for ISBN-10)."""
+        # Strip hyphens and spaces for validation
+        stripped = v.replace("-", "").replace(" ", "")
+        if re.fullmatch(r"\d{9}[\dX]", stripped) or re.fullmatch(r"\d{13}", stripped):
+            return v
+        raise ValueError(
+            "Invalid ISBN format. Must be a valid ISBN-10 (e.g. 0-306-40615-2) "
+            "or ISBN-13 (e.g. 978-3-16-148410-0)."
+        )
 
 
 class BookOut(BaseModel):
@@ -33,5 +50,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,13 @@
-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()
 
@@ -16,19 +19,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")
+        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
 )
-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")
         raise HTTPException(
-            status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail=str(e)
-        )
+            status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
+            detail="Could not retrieve books.",
+        ) from e
 
 
 @router.get(

test_main.py

@@ -1,14 +1,16 @@
 from repositories import create_book, get_books, get_book, update_book, delete_book
 from models import BookIn
-from sqlalchemy import create_engine, inspect
+from sqlalchemy import inspect
 
 
-# Test data constants
+# Test data constants — each test that inserts uses its own unique ISBNs
+# to work safely within the shared SQLite test session.
 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": "9780307887443"},
 ]
 
+
 class TestMainApp:
     def test_create_app(self, test_app):
         """Test application creation"""
@@ -19,45 +21,47 @@ 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"]
+        book = create_book(test_db, BookIn(title="Carrie", author="Stephen King", isbn="9780006280002"))
+        assert book.title == "Carrie"
+        assert book.author == "Stephen King"
+        assert book.isbn == "9780006280002"
         assert book.id is not None
 
     def test_get_books(self, test_db):
         """Test getting all books"""
-        book1 = create_book(test_db, BookIn(**TEST_BOOKS[0]))
-        book2 = create_book(test_db, BookIn(**TEST_BOOKS[1]))
+        book1 = create_book(test_db, BookIn(title="Carrie", author="Stephen King", isbn="9780006280019"))
+        book2 = create_book(test_db, BookIn(title="Ready Player One", author="Ernest Cline", isbn="9780006280026"))
 
         books = get_books(test_db)
-        #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_book(self, test_db):
         """Test getting a specific book"""
-        created_book = create_book(test_db, BookIn(**TEST_BOOKS[0]))
+        created_book = create_book(test_db, BookIn(title="Carrie", author="Stephen King", isbn="9780006280033"))
         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.title == "Carrie"
+        assert retrieved_book.isbn == "9780006280033"
 
     def test_update_book(self, test_db):
         """Test updating a book"""
-        book = create_book(test_db, BookIn(**TEST_BOOKS[0]))
-        updated_data = BookIn(**TEST_BOOKS[1])
-        updated_book = update_book(test_db, book.id, updated_data)
+        book = create_book(test_db, BookIn(title="Carrie", author="Stephen King", isbn="9780006280040"))
+        updated_book = update_book(test_db, book.id, BookIn(title="Ready Player One", author="Ernest Cline", isbn="9780006280057"))
         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.title == "Ready Player One"
+        assert updated_book.author == "Ernest Cline"
+        assert updated_book.isbn == "9780006280057"
 
     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="0306406152"))
         deleted_book = delete_book(test_db, book.id)
 
         assert deleted_book is not None
@@ -67,5 +71,95 @@ 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="9780385533225")) is None
         assert delete_book(test_db, 999999) is None
+
+
+# HTTP-layer (router) tests
+class TestBooksAPI:
+    def test_create_book(self, client):
+        """Test POST /api/books/ returns 201 with isbn in response"""
+        payload = {"title": "Dune", "author": "Frank Herbert", "isbn": "9780441013593"}
+        response = client.post("/api/books/", json=payload)
+        assert response.status_code == 201
+        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_invalid_isbn(self, client):
+        """Test POST /api/books/ with invalid ISBN returns 422"""
+        payload = {"title": "Bad Book", "author": "Nobody", "isbn": "not-an-isbn"}
+        response = client.post("/api/books/", json=payload)
+        assert response.status_code == 422
+
+    def test_get_books(self, client):
+        """Test GET /api/books/ returns 200"""
+        response = client.get("/api/books/")
+        assert response.status_code == 200
+        assert isinstance(response.json(), list)
+
+    def test_get_books_pagination_bounds(self, client):
+        """Test GET /api/books/ rejects out-of-bounds limit"""
+        response = client.get("/api/books/?limit=0")
+        assert response.status_code == 422
+        response = client.get("/api/books/?limit=101")
+        assert response.status_code == 422
+
+    def test_get_book_not_found(self, client):
+        """Test GET /api/books/{id} returns 404 for missing book"""
+        response = client.get("/api/books/999999")
+        assert response.status_code == 404
+
+    def test_get_book_found(self, client):
+        """Test GET /api/books/{id} returns 200 for existing book"""
+        created = client.post(
+            "/api/books/",
+            json={"title": "Foundation", "author": "Isaac Asimov", "isbn": "9780553293357"},
+        )
+        assert created.status_code == 201
+        book_id = created.json()["id"]
+        response = client.get(f"/api/books/{book_id}")
+        assert response.status_code == 200
+        assert response.json()["isbn"] == "9780553293357"
+
+    def test_update_book(self, client):
+        """Test PUT /api/books/{id} returns 200"""
+        created = client.post(
+            "/api/books/",
+            json={"title": "Old Title", "author": "Old Author", "isbn": "9780743273565"},
+        )
+        assert created.status_code == 201
+        book_id = created.json()["id"]
+        update_payload = {"title": "New Title", "author": "New Author", "isbn": "9780062316097"}
+        response = client.put(f"/api/books/{book_id}", json=update_payload)
+        assert response.status_code == 200
+        assert response.json()["title"] == "New Title"
+        assert response.json()["isbn"] == "9780062316097"
+
+    def test_update_book_not_found(self, client):
+        """Test PUT /api/books/{id} returns 404 for missing book"""
+        response = client.put(
+            "/api/books/999999",
+            json={"title": "X", "author": "Y", "isbn": "9780385533225"},
+        )
+        assert response.status_code == 404
+
+    def test_delete_book(self, client):
+        """Test DELETE /api/books/{id} returns 200"""
+        created = client.post(
+            "/api/books/",
+            json={"title": "To Delete", "author": "Author", "isbn": "9780525559474"},
+        )
+        assert created.status_code == 201
+        book_id = created.json()["id"]
+        response = client.delete(f"/api/books/{book_id}")
+        assert response.status_code == 200
+        # Confirm deletion
+        assert client.get(f"/api/books/{book_id}").status_code == 404
+
+    def test_delete_book_not_found(self, client):
+        """Test DELETE /api/books/{id} returns 404 for missing book"""
+        response = client.delete("/api/books/999999")
+        assert response.status_code == 404