review-fix: automated review of PR #24

Dockerfile

@@ -3,5 +3,7 @@ RUN apt-get update && apt-get install -y libpq-dev
 COPY . /app
 WORKDIR /app
 RUN pip install -r requirements.txt
+RUN useradd -m appuser
+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,11 +1,12 @@
 import pytest
-from sqlalchemy import create_engine, inspect, text
+from sqlalchemy import create_engine, inspect
 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"""
@@ -37,10 +38,10 @@ def test_app(test_engine):
     """Create test FastAPI application with test database"""
 
     def override_get_db():
-        TestingSessionLocal = sessionmaker(
+        testing_session_local = sessionmaker(
             autocommit=False, autoflush=False, bind=test_engine
         )
-        db = TestingSessionLocal()
+        db = testing_session_local()
         try:
             yield db
         finally:

db.sql

@@ -2,6 +2,7 @@ DROP TABLE IF EXISTS books;
 
 CREATE TABLE books (
     id SERIAL PRIMARY KEY,
-    title VARCHAR(50) UNIQUE NOT NULL,
-    author VARCHAR(100) UNIQUE NOT NULL
+    title VARCHAR(255) NOT NULL,
+    author VARCHAR(255) NOT NULL,
+    isbn VARCHAR(20) UNIQUE NOT NULL
 );

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

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:
@@ -11,7 +11,7 @@ def create_app() -> FastAPI:
     init_db()
 
     # Include routers
-    app.include_router(router, prefix="/api", dependencies=[Depends(get_db)])
+    app.include_router(router, prefix="/api")
 
     return app
 

models.py

@@ -1,14 +1,16 @@
+import logging
 from sqlalchemy.orm import DeclarativeBase, Mapped, mapped_column
 from sqlalchemy import String
-from pydantic import BaseModel, ConfigDict
+from pydantic import BaseModel, ConfigDict, Field
+
+
+logger = logging.getLogger(__name__)
 
 
 # SQLAlchemy models
 class Base(DeclarativeBase):
     """Base class for all database models"""
 
-    pass
-
 
 class Book(Base):
     """Book model"""
@@ -18,13 +20,16 @@ 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), nullable=False, unique=True)
+
 
 # Pydantic models
 class BookIn(BaseModel):
     """Pydantic model for book input"""
 
     title: str
     author: str
+    isbn: str = Field(..., pattern=r"^(?:97[89]-?)?\d{1,5}-?\d+-?\d+-?[\dX]$")
 
 
 class BookOut(BaseModel):
@@ -33,5 +38,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
+import logging
+from fastapi import APIRouter, Depends, HTTPException, Query, status
+from sqlalchemy.exc import SQLAlchemyError
 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()
 
@@ -15,20 +18,30 @@
 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))
+    except SQLAlchemyError as e:
+        logger.exception("Error creating book: %s", e)
+        raise HTTPException(
+            status_code=status.HTTP_400_BAD_REQUEST,
+            detail="An error occurred creating the 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 = 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:
+    except SQLAlchemyError 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 error occurred retrieving books",
+        ) from e
 
 
 @router.get(

test_main.py

@@ -1,14 +1,24 @@
+from unittest.mock import patch
+from sqlalchemy import inspect
+from sqlalchemy.exc import SQLAlchemyError
 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_BOOKS = [
-    {"title": "Carrie", "author": "Stephen King"},
-    {"title": "Ready Player One", "author": "Ernest Cline"},
+    {"title": "Carrie", "author": "Stephen King", "isbn": "978-0-385-12167-5"},
+    {"title": "Ready Player One", "author": "Ernest Cline", "isbn": "978-0-307-88743-6"},
 ]
 
+# Unique books for the HTTP integration CRUD cycle (avoids ISBN uniqueness conflicts
+# with TEST_BOOKS used in other router tests that commit to the shared test DB).
+CRUD_BOOKS = [
+    {"title": "The Shining", "author": "Stephen King", "isbn": "978-0-385-12168-2"},
+    {"title": "Armada", "author": "Ernest Cline", "isbn": "978-0-307-88744-3"},
+]
+
+
 class TestMainApp:
     def test_create_app(self, test_app):
         """Test application creation"""
@@ -19,13 +29,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,7 +46,7 @@ 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)
 
@@ -54,10 +66,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="978-3-16-148410-0"))
         deleted_book = delete_book(test_db, book.id)
 
         assert deleted_book is not None
@@ -67,5 +80,90 @@ 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="978-3-16-148410-0")) is None
         assert delete_book(test_db, 999999) is None
+
+
+# HTTP Router Tests
+class TestBookRoutes:
+    def test_create_book_success(self, client):
+        """Test POST /api/books/ returns 201 with the created book"""
+        payload = TEST_BOOKS[0]
+        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/ returns 422 for an invalid ISBN"""
+        payload = {"title": "Bad ISBN Book", "author": "Someone", "isbn": "not-an-isbn"}
+        response = client.post("/api/books/", json=payload)
+        assert response.status_code == 422
+
+    def test_create_book_db_error(self, client):
+        """Test POST /api/books/ returns 400 when a database error occurs"""
+        with patch("repositories.create_book", side_effect=SQLAlchemyError("db error")):
+            response = client.post("/api/books/", json=TEST_BOOKS[0])
+        assert response.status_code == 400
+        assert "An error occurred creating the book" in response.json()["detail"]
+
+    def test_get_books_success(self, client):
+        """Test GET /api/books/ returns 200 with a list"""
+        response = client.get("/api/books/")
+        assert response.status_code == 200
+        assert isinstance(response.json(), list)
+
+    def test_get_books_limit_enforced(self, client):
+        """Test GET /api/books/ rejects limit > 100"""
+        response = client.get("/api/books/?limit=9999")
+        assert response.status_code == 422
+
+    def test_get_books_db_error(self, client):
+        """Test GET /api/books/ returns 500 when a database error occurs"""
+        with patch("repositories.get_books", side_effect=SQLAlchemyError("db error")):
+            response = client.get("/api/books/")
+        assert response.status_code == 500
+        assert "An error occurred retrieving books" in response.json()["detail"]
+
+    def test_get_book_not_found(self, client):
+        """Test GET /api/books/{id} returns 404 for unknown id"""
+        response = client.get("/api/books/999999")
+        assert response.status_code == 404
+
+    def test_update_book_not_found(self, client):
+        """Test PUT /api/books/{id} returns 404 for unknown id"""
+        response = client.put("/api/books/999999", json=TEST_BOOKS[0])
+        assert response.status_code == 404
+
+    def test_delete_book_not_found(self, client):
+        """Test DELETE /api/books/{id} returns 404 for unknown id"""
+        response = client.delete("/api/books/999999")
+        assert response.status_code == 404
+
+    def test_create_read_update_delete(self, client):
+        """Full CRUD cycle through the HTTP layer"""
+        # Create using CRUD_BOOKS to avoid ISBN uniqueness conflict with other tests
+        create_resp = client.post("/api/books/", json=CRUD_BOOKS[0])
+        assert create_resp.status_code == 201
+        book_id = create_resp.json()["id"]
+
+        # Read single
+        get_resp = client.get(f"/api/books/{book_id}")
+        assert get_resp.status_code == 200
+        assert get_resp.json()["isbn"] == CRUD_BOOKS[0]["isbn"]
+
+        # Update
+        update_resp = client.put(f"/api/books/{book_id}", json=CRUD_BOOKS[1])
+        assert update_resp.status_code == 200
+        assert update_resp.json()["isbn"] == CRUD_BOOKS[1]["isbn"]
+
+        # Delete
+        delete_resp = client.delete(f"/api/books/{book_id}")
+        assert delete_resp.status_code == 200
+
+        # Confirm gone
+        confirm_resp = client.get(f"/api/books/{book_id}")
+        assert confirm_resp.status_code == 404