solution#647
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The API implementation has critical issues: missing database table creation in main.py, route conflict in /books/{author_id} that prevents pagination, and author filtering not implemented in get_all_books. These violate core requirements and must be fixed.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| from sqlalchemy import create_engine | ||
| from sqlalchemy.ext.declarative import declarative_base | ||
| from sqlalchemy.orm import sessionmaker | ||
|
|
||
| SQLALCHEMY_DATABASE_URL = "sqlite:///.library.db" | ||
| engine = create_engine( | ||
| SQLALCHEMY_DATABASE_URL, | ||
| connect_args={"check_same_thread": False} | ||
| ) | ||
| SessionLocal = sessionmaker(autocommit=False, autoflush=False, bind=engine) | ||
| Base = declarative_base() |
There was a problem hiding this comment.
Missing database table creation - The FastAPI application doesn't create tables on startup. While Alembic migrations exist, the task requires creating tables using SQLAlchemy models in main.py. Add: from models import Base and Base.metadata.create_all(bind=engine) inside an lifespan event or at module level.
|
|
||
| def get_all_books(db: Session, | ||
| skip: int = 0, | ||
| limit: int = 10, | ||
| author_id: int | None = None): | ||
| query = select(models.DBBook).offset(skip).limit(limit) | ||
|
|
There was a problem hiding this comment.
The author_id parameter is accepted but never used in the query. The function should filter books by author when author_id is provided, e.g., by adding a conditional .where(models.DBBook.author_id == author_id) when the parameter is not None.
| from sqlalchemy import String, ForeignKey, Date | ||
| from sqlalchemy.orm import Mapped, mapped_column, relationship | ||
| from datetime import date | ||
|
|
||
| from database import Base | ||
|
|
||
|
|
||
| class DBAuthor(Base): | ||
| __tablename__ = "authors" | ||
|
|
||
| id: Mapped[int] = mapped_column(primary_key=True, index=True) |
There was a problem hiding this comment.
Missing database initialization. The task requires 'Create database tables using the SQLAlchemy models' in main.py. Add table creation, typically in a lifespan context manager: from contextlib import asynccontextmanager, then @asynccontextmanager over the app with Base.metadata.create_all(bind=engine) inside, or import Base and call it at module level.
| from fastapi import FastAPI, Depends, HTTPException | ||
| from sqlalchemy.orm import Session | ||
|
|
||
| import crud | ||
| import schemas | ||
| from database import SessionLocal | ||
|
|
||
| app = FastAPI() |
There was a problem hiding this comment.
Missing database table creation. Add: from database import engine, Base and Base.metadata.create_all(bind=engine) to create tables on application startup.
|
|
||
| @app.get("/books/{author_id}", response_model=list[schemas.Book]) | ||
| def read_book_by_author_id(author_id: int, db: Session = Depends(get_db)): | ||
| db_author = crud.get_books_by_author(db=db, author_id=author_id) |
There was a problem hiding this comment.
This route path /books/{author_id} conflicts with /books/ pagination endpoint. FastAPI matches path parameters greedily, so this will catch all requests to /books/ before the pagination endpoint can process them. Use query parameters instead: @app.get("/books/", response_model=list[schemas.Book]) with author_id: int | None = None as a query parameter, and pass it to the CRUD function.
| def read_book_by_author_id(author_id: int, db: Session = Depends(get_db)): | ||
| db_author = crud.get_books_by_author(db=db, author_id=author_id) | ||
|
|
||
| if db_author is None: | ||
| raise HTTPException( | ||
| status_code=404, |
There was a problem hiding this comment.
The variable name db_author is misleading since it holds books, not an author. Also, the None check is incorrect - get_books_by_author returns a list which is truthy even when empty. If keeping this separate endpoint, consider renaming to db_books and removing the None check (empty list is valid).
|
|
||
| class BookBase(BaseModel): | ||
| title: str | ||
| summary: str |
There was a problem hiding this comment.
Missing database table creation - The task requires creating tables using SQLAlchemy models in main.py. Add Base.metadata.create_all(bind=engine) to initialize the database tables on application startup.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation successfully meets all task requirements. The project structure includes all five required files (main.py, database.py, models.py, schemas.py, crud.py), and all API endpoints function correctly: creating authors with uniqueness validation, retrieving paginated author lists, fetching individual authors, creating books with author_id association, and retrieving paginated books with optional author_id filtering. The database tables are properly created at startup, SQLite is configured correctly, SQLAlchemy models define the Author and Book entities with appropriate relationships, and Pydantic schemas handle validation as expected. All checklist items pass with no failures reported.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| config = context.config | ||
|
|
||
| # Interpret the config file for Python logging. | ||
| # This line sets up loggers basically. | ||
| if config.config_file_name is not None: | ||
| fileConfig(config.config_file_name) | ||
|
|
||
| # add your model's MetaData object here | ||
| # for 'autogenerate' support | ||
| # from myapp import mymodel | ||
| # target_metadata = mymodel.Base.metadata |
There was a problem hiding this comment.
While crud.py is provided, the create_author function signature doesn't match the task requirement - it needs to accept an author_id parameter and create a new author in the database
2 participants
No description provided.