Solution#657
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation has the supporting code ready (CRUD functions, schemas, models), but three required book API endpoints are missing from main.py, which is a critical blocker. You need to add: (1) POST /books/ to create a new book for a specific author, (2) GET /books/ with pagination (skip, limit query parameters), and (3) GET /books/?author_id={id} to filter books by author ID. These correspond to CHECKLIST ITEM #8. Additionally, consider changing the database URL in database.py from sqlite:///:memory: to sqlite:///./library.db to persist data across restarts.
✨ 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
| return db.scalar(select(models.Author).where(models.Author.id == author_id)) | ||
|
|
||
|
|
||
| def create_author(db: Session, author: AuthorCreate): | ||
| db_author = models.Author( | ||
| name=author.name, | ||
| bio=author.bio, | ||
| ) | ||
| db.add(db_author) | ||
| db.commit() | ||
| db.refresh(db_author) | ||
|
|
||
| return db_author | ||
|
|
||
|
|
||
| def get_all_books(db: Session, | ||
| author_id: int | None = None, | ||
| skip: int = Query(default=0, ge=0), | ||
| limit: int = Query(default=9, ge=1) | ||
| ): | ||
| queryset = select(models.Book) | ||
|
|
||
| if author_id is not None: | ||
| queryset = queryset.join(models.Author).where(models.Author.id == author_id) | ||
|
|
There was a problem hiding this comment.
Missing /books/ POST endpoint to create a new book for a specific author, which is required by the task checklist item #8.
| return db.scalar(select(models.Author).where(models.Author.id == author_id)) | ||
|
|
||
|
|
||
| def create_author(db: Session, author: AuthorCreate): | ||
| db_author = models.Author( | ||
| name=author.name, | ||
| bio=author.bio, | ||
| ) | ||
| db.add(db_author) | ||
| db.commit() | ||
| db.refresh(db_author) | ||
|
|
||
| return db_author | ||
|
|
||
|
|
||
| def get_all_books(db: Session, | ||
| author_id: int | None = None, | ||
| skip: int = Query(default=0, ge=0), | ||
| limit: int = Query(default=9, ge=1) | ||
| ): | ||
| queryset = select(models.Book) | ||
|
|
||
| if author_id is not None: | ||
| queryset = queryset.join(models.Author).where(models.Author.id == author_id) | ||
|
|
There was a problem hiding this comment.
Missing /books/ GET endpoint to retrieve a list of books with pagination (skip, limit), which is required by the task checklist item #8.
| return db.scalar(select(models.Author).where(models.Author.id == author_id)) | ||
|
|
||
|
|
||
| def create_author(db: Session, author: AuthorCreate): | ||
| db_author = models.Author( | ||
| name=author.name, | ||
| bio=author.bio, | ||
| ) | ||
| db.add(db_author) | ||
| db.commit() | ||
| db.refresh(db_author) | ||
|
|
||
| return db_author | ||
|
|
||
|
|
||
| def get_all_books(db: Session, | ||
| author_id: int | None = None, | ||
| skip: int = Query(default=0, ge=0), | ||
| limit: int = Query(default=9, ge=1) | ||
| ): | ||
| queryset = select(models.Book) | ||
|
|
||
| if author_id is not None: | ||
| queryset = queryset.join(models.Author).where(models.Author.id == author_id) | ||
|
|
There was a problem hiding this comment.
Missing /books/?author_id={id} filtering capability via query parameter for books by author ID, which is required by the task checklist item #8.
| from sqlalchemy import select | ||
| from sqlalchemy.orm import Session | ||
|
|
||
| import models |
There was a problem hiding this comment.
The database uses an in-memory SQLite URL (sqlite:///:memory:). For a library management system where data persistence matters, consider using a file-based database like sqlite:///./library.db so data persists across restarts.
| from sqlalchemy.orm import Session | ||
|
|
||
| from fastapi import FastAPI, Depends, HTTPException | ||
|
|
||
| import crud | ||
| import schemas | ||
| from database import base, engine, SessionLocal | ||
|
|
||
| app = FastAPI() | ||
|
|
||
|
|
||
| base.metadata.create_all(bind=engine) | ||
|
|
||
|
|
||
| def get_db() -> Session: | ||
| db = SessionLocal() | ||
| try: | ||
| yield db | ||
| finally: | ||
| db.close() | ||
|
|
||
|
|
||
| @app.get("/authors/", response_model=list[schemas.AuthorList]) | ||
| def authors_list(db: Session = Depends(get_db)): | ||
| return crud.get_all_authors(db) | ||
|
|
||
|
|
||
| @app.get("/authors/{author_id}/", response_model=schemas.AuthorList) | ||
| def author_detail( | ||
| author_id: int, | ||
| db: Session = Depends(get_db) | ||
| ): | ||
| author = crud.get_author(db=db, author_id=author_id) | ||
|
|
||
| if author is None: | ||
| raise HTTPException(status_code=404, detail="Author with this id is not found") | ||
|
|
||
| return author | ||
|
|
||
|
|
||
| @app.post("/authors/", response_model=schemas.AuthorList) | ||
| def author_create( | ||
| author: schemas.AuthorCreate, | ||
| db: Session = Depends(get_db) | ||
| ): | ||
| return crud.create_author(db=db, author=author) |
There was a problem hiding this comment.
Missing endpoints: The task requires implementing: (1) Create a new book for a specific author, (2) Retrieve a list of books with pagination, and (3) Filter books by author ID. These endpoints are not present in main.py despite CRUD functions (create_book, get_all_books) and schemas (BookCreate, BookList) being properly implemented.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The code is missing the required pagination query parameters for both endpoints. The task explicitly requires 'Retrieve a list of authors with pagination (skip, limit)' and 'Retrieve a list of books with pagination (skip, limit)', but the current endpoints don't expose skip and limit to clients. To fix this, update the GET /authors/ endpoint to add: python skip: int = Query(default=0, ge=0), limit: int = Query(default=9, ge=1) And pass them to crud.get_all_authors(skip=skip, limit=limit). Similarly, update the GET /books/ endpoint with the same parameters and pass them to crud.get_all_books(skip=skip, limit=limit, author_id=author_id). The underlying CRUD functions already support these parameters, so this is just a matter of exposing them in the FastAPI route definitions. Once these changes are made, clients will be able to control pagination by passing query parameters like GET /authors/?skip=0&limit=10. This is a required feature per the task checklist, not a cosmetic issue.
✨ 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
|
|
||
| @app.get("/authors/", response_model=list[schemas.AuthorList]) | ||
| def authors_list(db: Session = Depends(get_db)): |
There was a problem hiding this comment.
This endpoint is missing skip and limit query parameters. The task requirement states 'Retrieve a list of authors with pagination (skip, limit)' - add skip: int = Query(default=0, ge=0) and limit: int = Query(default=9, ge=1) parameters and pass them to crud.get_all_authors().
|
|
||
| @app.get("/books/", response_model=list[schemas.BookList]) | ||
| def get_all_books( | ||
| author_id: int | None = None, | ||
| db: Session = Depends(get_db) | ||
| ): |
There was a problem hiding this comment.
This endpoint is missing skip and limit query parameters. The task requirement states 'Retrieve a list of books with pagination (skip, limit)' - add skip: int = Query(default=0, ge=0) and limit: int = Query(default=9, ge=1) parameters and pass them to crud.get_all_books().
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation fully satisfies all task requirements. All API endpoints are correctly implemented with proper pagination support using skip and limit query parameters for both /authors/ and /books/. The project uses FastAPI with SQLAlchemy ORM and SQLite, includes proper Pydantic schemas with from_attributes=True, and has all required CRUD operations. The code structure is clean and follows best practices. Great work completing this library API!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
3 participants
No description provided.