Solution#653
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This submission has two critical issues that must be fixed before approval. First, the /books endpoint is missing the required skip and limit pagination parameters that were explicitly specified in the task requirements for listing books with pagination. You need to add these query parameters to the function signature and apply them when fetching books. Second, there is a function naming conflict in main.py — the endpoint function get_books at line 59 shadows the imported crud.get_books function, which will cause a NameError at runtime when the endpoint tries to call crud.get_books(). Rename the endpoint function (e.g., to read_books). Additionally, change the HTTP status code from 400 to 404 for "author not found" responses at line 40, since 400 indicates malformed requests while 404 correctly represents a missing resource. Once these issues are addressed, the implementation should meet all requirements.
✨ 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("/books", response_model=list[schemas.BookRead]) | ||
| def get_books(db: Session = Depends(get_db)): | ||
| return crud.get_books(db=db) |
There was a problem hiding this comment.
The /books endpoint lacks pagination parameters. Per the requirements, it must support 'skip' and 'limit' parameters like the /authors endpoint does.
| @app.get("/authors", response_model=list[schemas.AuthorRead]) | ||
| def get_authors(skip: int = 0, limit: int = 10, db: Session = Depends(get_db)): | ||
| authors = crud.get_authors(db) | ||
| return authors[skip: skip + limit] |
There was a problem hiding this comment.
The /authors endpoint correctly implements pagination with skip/limit parameters. The /books endpoint should follow the same pattern.
| if author is None: | ||
| raise HTTPException(status_code=400, detail=f"There is not author with id {author_id}") |
There was a problem hiding this comment.
Consider using HTTP 404 (Not Found) instead of 400 (Bad Request) for 'author not found' responses. Status code 400 typically indicates malformed request syntax, while 404 is the standard for resources that don't exist.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Two critical issues must be fixed before approval. First, there are duplicate endpoint function names - both get_books functions at lines 60 and 65 will cause Python to overwrite the first definition, completely losing one of your API endpoints. You need to rename one (e.g., get_books_by_author). Second, line 47 uses HTTP status code 400 for 'author not found', but this should be 404 since it represents a missing resource, not a malformed request. The pagination parameters on the /books endpoint are correctly implemented, and your models, schemas, and CRUD operations look solid - just fix these two issues and the implementation will meet all requirements.
✨ 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("/books", response_model=list[schemas.BookRead]) | ||
| def get_books(skip: int = 0, limit: int = 10, db: Session = Depends(get_db)): | ||
| books = crud.get_books(db=db) | ||
| return books[skip: skip + limit] | ||
|
|
||
| @app.get("/author/{author_id}/books", response_model=list[schemas.BookRead]) | ||
| def get_books(author_id: int, skip: int = 0, limit: int = 10, db: Session = Depends(get_db)): |
There was a problem hiding this comment.
Duplicate function name: Both endpoint functions are named get_books (lines 60 and 65). The second definition overwrites the first, which can cause unexpected behavior. Rename one of them (e.g., get_books_by_author).
| @app.post("/author/{author_id}/create_book", response_model=schemas.BookBase) | ||
| def create_book(author_id: int, data: schemas.BookCreate, db: Session = Depends(get_db)): | ||
| author = crud.get_author_by_id(db=db,author_id=author_id) | ||
| if author is None: |
There was a problem hiding this comment.
HTTP status code should be 404 (Not Found): When an author doesn't exist, returning 400 is incorrect since this is a missing resource error, not a malformed request. Change to status_code=404.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved! You've successfully resolved both critical issues from the previous review: the duplicate get_books function is now correctly named get_books_by_author at line 65, and the author not found endpoint now returns the proper 404 status code. All 20 checklist requirements are met with a well-structured FastAPI application using SQLAlchemy ORM and SQLite. Your five-file project structure (main.py, database.py, models.py, schemas.py, crud.py) demonstrates good organization, and you've correctly implemented all CRUD operations, Pydantic schemas, and API endpoints with pagination support. Great work on building a complete, functional library management API!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.