fix: automated review of PR #24

.gitignore

@@ -9,3 +9,10 @@
 
 # macOS
 .DS_Store
+
+# Secrets — never commit credentials
+.env
+
+# SQLite test artefacts
+*.db
+file::memory:

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,46 +1,68 @@
 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"""
-    test_engine = create_engine(database_url)
-    Base.metadata.create_all(bind=test_engine)
-    yield test_engine
-    Base.metadata.drop_all(bind=test_engine)
+    """Create test database engine (session-scoped, used only for schema checks)"""
+    engine = create_engine(database_url)
+    Base.metadata.create_all(bind=engine)
+    yield engine
+    Base.metadata.drop_all(bind=engine)
 
 
 @pytest.fixture(scope="function")
 def test_db(test_engine):
-    """Create test database session that rolls back after each test"""
-    connection = test_engine.connect()
-    transaction = connection.begin()
-    db = sessionmaker(
-        autocommit=False, autoflush=False, bind=connection,
-        join_transaction_mode="create_savepoint"
-    )()
-    try:
-        yield db
-    finally:
-        db.close()
-        transaction.rollback()
-        connection.close()
+    """Create an isolated test database session for each test.
+
+    Uses a fresh in-memory SQLite database per test function so that each
+    test starts with a clean schema regardless of database backend.
+    The session-scoped ``test_engine`` is still used for schema-inspection
+    tests (TestMainApp); for repository tests a dedicated per-test engine
+    is created so the unique constraints and NOT NULL constraints start
+    from a clean state every time.
+    """
+    # Determine the dialect.  For SQLite we create a fresh per-test
+    # in-memory DB; for other dialects we keep the savepoint strategy.
+    if test_engine.dialect.name == "sqlite":
+        per_test_engine = create_engine(
+            "sqlite:///:memory:",
+            connect_args={"check_same_thread": False},
+        )
+        Base.metadata.create_all(bind=per_test_engine)
+        db = sessionmaker(autocommit=False, autoflush=False, bind=per_test_engine)()
+        try:
+            yield db
+        finally:
+            db.close()
+            per_test_engine.dispose()
+    else:
+        # PostgreSQL / other ACID databases: use the savepoint rollback strategy
+        connection = test_engine.connect()
+        transaction = connection.begin()
+        db = sessionmaker(
+            autocommit=False, autoflush=False, bind=connection,
+            join_transaction_mode="create_savepoint"
+        )()
+        try:
+            yield db
+        finally:
+            db.close()
+            transaction.rollback()
+            connection.close()
 
 
 @pytest.fixture
 def test_app(test_engine):
     """Create test FastAPI application with test database"""
 
     def override_get_db():
-        TestingSessionLocal = sessionmaker(
-            autocommit=False, autoflush=False, bind=test_engine
-        )
-        db = TestingSessionLocal()
+        db = sessionmaker(autocommit=False, autoflush=False, bind=test_engine)()
         try:
             yield db
         finally:

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:
@@ -23,7 +28,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:
@@ -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 (each route handler declares its own get_db dependency)
+    app.include_router(router, prefix="/api")
 
     return app
 

models.py

@@ -1,3 +1,4 @@
+from typing import Optional
 from sqlalchemy.orm import DeclarativeBase, Mapped, mapped_column
 from sqlalchemy import String
 from pydantic import BaseModel, ConfigDict
@@ -7,8 +8,6 @@
 class Base(DeclarativeBase):
     """Base class for all database models"""
 
-    pass
-
 
 class Book(Base):
     """Book model"""
@@ -18,13 +17,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(13), unique=True, nullable=False)
+
 
 # Pydantic models
 class BookIn(BaseModel):
     """Pydantic model for book input"""
 
     title: str
     author: str
+    isbn: str
 
 
 class BookOut(BaseModel):
@@ -33,5 +35,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,11 +1,17 @@
+import logging
+from typing import List
+
 from fastapi import APIRouter, Depends, HTTPException, status
+from sqlalchemy.exc import IntegrityError
 from sqlalchemy.orm import Session
-from typing import List
+
 import models
 import repositories
 from dependencies import get_db
 
-# Create router with prefix
+logger = logging.getLogger(__name__)
+
+# Router (prefix "/api" is applied in main.py)
 router = APIRouter()
 
 
@@ -15,8 +21,18 @@
 def create_book(book: models.BookIn, db: Session = Depends(get_db)):
     try:
         return repositories.create_book(db=db, book=book)
+    except IntegrityError as e:
+        logger.error("IntegrityError creating book: %s", e)
+        raise HTTPException(
+            status_code=status.HTTP_409_CONFLICT,
+            detail="A book with that ISBN already exists.",
+        ) from e
     except Exception as e:
-        raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(e))
+        logger.error("Unexpected error creating book: %s", e)
+        raise HTTPException(
+            status_code=status.HTTP_400_BAD_REQUEST,
+            detail="Failed to create book.",
+        ) from e
 
 
 @router.get(
@@ -26,9 +42,11 @@ def get_books(skip: int = 0, limit: int = 10, db: Session = Depends(get_db)):
     try:
         return repositories.get_books(db=db, skip=skip, limit=limit)
     except Exception as e:
+        logger.error("Unexpected 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="Failed to retrieve books.",
+        ) from e
 
 
 @router.get(

test_main.py

@@ -1,14 +1,16 @@
+from sqlalchemy import inspect
+
 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": "9780385533225"},
+    {"title": "Ready Player One", "author": "Ernest Cline", "isbn": "9780307887436"},
 ]
 
+
 class TestMainApp:
     def test_create_app(self, test_app):
         """Test application creation"""
@@ -19,13 +21,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 +38,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)
 
@@ -45,6 +49,7 @@ def test_get_book(self, test_db):
         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 +59,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="9780000000001"))
         deleted_book = delete_book(test_db, book.id)
 
         assert deleted_book is not None
@@ -67,5 +73,5 @@ 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="9780000000002")) is None
         assert delete_book(test_db, 999999) is None