From 3d4869501bf247175f4c7de74f9c89a0a84e13c7 Mon Sep 17 00:00:00 2001 From: vikram-blaxel Date: Mon, 11 May 2026 16:04:12 +0530 Subject: [PATCH 1/2] feat: add ISBN field to model --- models.py | 1 + 1 file changed, 1 insertion(+) diff --git a/models.py b/models.py index bd73c60..dd91182 100644 --- a/models.py +++ b/models.py @@ -18,6 +18,7 @@ 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(255), nullable=False) # Pydantic models class BookIn(BaseModel): From 253d80b3dccad1fed214f7647f16bcb4932ef161 Mon Sep 17 00:00:00 2001 From: Reviewer Swarm Date: Mon, 11 May 2026 20:46:46 +0000 Subject: [PATCH 2/2] fix: automated review fixes for PR #24 --- .env | 1 + .gitignore | 1 + Dockerfile | 11 +++-- conftest.py | 30 ++++++------- dependencies.py | 13 ++++-- docker-compose.yml | 2 +- main.py | 8 ++-- models.py | 23 ++++++++-- repositories.py | 31 +++++++++----- routers.py | 32 +++++++++++--- test_main.py | 103 ++++++++++++++++++++++++++++++++++++++++++--- 11 files changed, 200 insertions(+), 55 deletions(-) diff --git a/.env b/.env index 8ddb176..e75ee9e 100644 --- a/.env +++ b/.env @@ -1,3 +1,4 @@ POSTGRES_DB=app POSTGRES_USER=user POSTGRES_PASSWORD=secret +DATABASE_URL=postgresql://user:secret@localhost/app diff --git a/.gitignore b/.gitignore index aa0926b..7550d14 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,4 @@ +.env .idea .ipynb_checkpoints .mypy_cache diff --git a/Dockerfile b/Dockerfile index b482401..7218549 100644 --- a/Dockerfile +++ b/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"] diff --git a/conftest.py b/conftest.py index b9cb0aa..58eaaaa 100644 --- a/conftest.py +++ b/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 diff --git a/dependencies.py b/dependencies.py index eb51233..6227b3c 100644 --- a/dependencies.py +++ b/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 diff --git a/docker-compose.yml b/docker-compose.yml index d222eda..e4222f3 100644 --- a/docker-compose.yml +++ b/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 diff --git a/main.py b/main.py index 5d31686..130e322 100644 --- a/main.py +++ b/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 diff --git a/models.py b/models.py index dd91182..66ca863 100644 --- a/models.py +++ b/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,7 +18,8 @@ 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(255), nullable=False) + isbn: Mapped[str] = mapped_column(String(13)) + # Pydantic models class BookIn(BaseModel): @@ -26,6 +27,19 @@ class BookIn(BaseModel): 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): @@ -34,5 +48,6 @@ class BookOut(BaseModel): id: int title: str author: str + isbn: str model_config = ConfigDict(from_attributes=True) diff --git a/repositories.py b/repositories.py index dc9ff88..4aa3a52 100644 --- a/repositories.py +++ b/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) diff --git a/routers.py b/routers.py index db667c3..bf19788 100644 --- a/routers.py +++ b/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( diff --git a/test_main.py b/test_main.py index 3fea397..60fde33 100644 --- a/test_main.py +++ b/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": "9780385533348"}, + {"title": "Ready Player One", "author": "Ernest Cline", "isbn": "9780307887443"}, ] + class TestMainApp: def test_create_app(self, test_app): """Test application creation""" @@ -19,6 +21,7 @@ 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): @@ -26,6 +29,7 @@ def test_create_book(self, test_db): 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,13 @@ 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="9780385533348") + ) deleted_book = delete_book(test_db, book.id) assert deleted_book is not None @@ -67,5 +75,88 @@ 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="9780385533348"), + ) + is None + ) assert delete_book(test_db, 999999) is None + + +# HTTP / Router Tests +class TestBookAPI: + def test_create_book(self, client): + """Test POST /api/books/ creates a book and returns 201""" + 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/ with an 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 a list of books""" + client.post("/api/books/", json=TEST_BOOKS[0]) + client.post("/api/books/", json=TEST_BOOKS[1]) + response = client.get("/api/books/") + assert response.status_code == 200 + books = response.json() + assert isinstance(books, list) + assert len(books) >= 2 + + def test_get_books_limit_cap(self, client): + """Test GET /api/books/ rejects limit > 100""" + response = client.get("/api/books/?limit=999") + assert response.status_code == 422 + + def test_get_book(self, client): + """Test GET /api/books/{id} returns the correct book""" + created = client.post("/api/books/", json=TEST_BOOKS[0]).json() + response = client.get(f"/api/books/{created['id']}") + assert response.status_code == 200 + data = response.json() + assert data["id"] == created["id"] + assert data["isbn"] == TEST_BOOKS[0]["isbn"] + + 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_update_book(self, client): + """Test PUT /api/books/{id} updates book fields""" + created = client.post("/api/books/", json=TEST_BOOKS[0]).json() + response = client.put(f"/api/books/{created['id']}", json=TEST_BOOKS[1]) + assert response.status_code == 200 + data = response.json() + assert data["title"] == TEST_BOOKS[1]["title"] + assert data["isbn"] == TEST_BOOKS[1]["isbn"] + + 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=TEST_BOOKS[0]) + assert response.status_code == 404 + + def test_delete_book(self, client): + """Test DELETE /api/books/{id} removes the book""" + created = client.post("/api/books/", json=TEST_BOOKS[0]).json() + response = client.delete(f"/api/books/{created['id']}") + assert response.status_code == 200 + # Confirm it's gone + assert client.get(f"/api/books/{created['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