Solution#637
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This is a great start, and the overall structure of your project is solid. However, I'm requesting a few changes because there are some critical issues that prevent the application from running and meeting the core requirements.
Here are the main blockers to address:
- Database Tables Not Created: In
main.py, the application is missing the logic to create the database tables. You need to importengineandBaseand callBase.metadata.create_all(bind=engine)before the app starts to prevent runtime errors. - Incorrect Query Execution: In
crud.py, theget_all_booksandget_authors_listfunctions mix legacy and modern SQLAlchemy syntax. Callingdb.scalars()on adb.query()object will cause an error. Please change this toquery.all()to correctly fetch the results. - Broken Filtering Logic: The
get_all_booksfunction incrud.pyhas inverted logic for filtering byauthor_id. The filter should be applied whenauthor_idis notNone.
Once these issues are resolved, the application should function as expected. Keep up the good work!
✨ 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 db.database import SessionLocal | ||
| from db.models import Book | ||
|
|
||
| app = FastAPI() |
There was a problem hiding this comment.
The application is missing the logic to create the database tables, which is a key requirement. You'll need to import engine from your database setup and Base from your models, and then call Base.metadata.create_all(bind=engine) to ensure the tables exist before the API starts.
|
|
||
|
|
||
| @app.get("/books/", response_model=list[schemas.BookListSchema]) | ||
| def read_cheese_types( |
There was a problem hiding this comment.
The function name read_cheese_types does not reflect what the function does (reading books). A more descriptive name, such as read_books, would significantly improve the code's readability and maintainability.
| ) | ||
|
|
||
|
|
||
| @app.post("/books/", response_model=schemas.BookSchemaBase) |
There was a problem hiding this comment.
The response_model is set to schemas.BookSchemaBase, which does not include the book's id. When a resource is created, the API response should typically include the full resource, including its unique identifier. Consider using a schema that includes the id field.
| return crud.get_authors_list(db=db, skip=skip, limit=limit) | ||
|
|
||
|
|
||
| @app.post("/authors/", response_model=schemas.AuthorSchemaBase) |
There was a problem hiding this comment.
Similar to the book creation endpoint, the response_model here (schemas.AuthorSchemaBase) does not include the new author's id. It's a best practice to return the id of a newly created resource. Using a schema that includes the id would be more conventional for a REST API.
| if author_id is None: | ||
| query = query.where(models.Book.author_id == author_id) |
There was a problem hiding this comment.
The logic for filtering by author_id is inverted. This if condition only applies the filter when author_id is None, but the requirement is to filter when an author_id is provided. You should check if author_id is not None.
| query = query.where(models.Book.author_id == author_id) | ||
| query = query.limit(limit).offset(skip) | ||
|
|
||
| return list(db.scalars(query)) |
There was a problem hiding this comment.
There's a mix of SQLAlchemy query styles here. db.query() returns a Query object (legacy 1.x style), but db.scalars() expects a select() construct (modern 2.0 style). To execute the Query object and get a list of results, you should use .all() instead, like so: query.all().
| def create_author( | ||
| db: Session, | ||
| author: schemas.AuthorCreateSchema | ||
| ) -> models.Book: |
There was a problem hiding this comment.
The function's return type hint is models.Book, but the implementation creates and returns an models.Author object on line 72. The type hint should be updated to models.Author to match the actual return value.
| ) -> list[models.Author]: | ||
| query = db.query(models.Author).offset(skip).limit(limit) | ||
|
|
||
| return list(db.scalars(query)) |
There was a problem hiding this comment.
Similar to the get_all_books function, this line mixes SQLAlchemy API styles. The query object was created with db.query(), so you should call query.all() to retrieve the list of authors instead of using db.scalars().
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this iteration! I am approving your submission as you've successfully addressed all the critical issues and met all the functional requirements. The database setup, models, and CRUD logic are all implemented correctly.
I have a few minor suggestions to enhance code clarity and consistency for you to consider in future projects:
- In
main.py, the functionread_cheese_typescould be renamed toread_booksto better reflect its purpose. - Also in
main.py, consider changing theresponse_modelfor the book creation endpoint toschemas.BookListSchemato include the new book'sidin the response, which is a common REST API practice. - In
crud.py, adding the: Sessiontype hint to thedbparameter inget_authors_listwould make it consistent with other functions.
These are just suggestions for improvement, not blockers. Great job getting the application fully functional!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| return db_author | ||
|
|
||
|
|
||
| def get_authors_list(db, skip: int, limit: int) -> list[models.Author]: |
There was a problem hiding this comment.
It's a good practice to maintain consistency with type hints. The db parameter is missing its type hint here, unlike the other functions in this file. Please add : Session to it.
|
|
||
|
|
||
| @app.get("/books/", response_model=list[schemas.BookListSchema]) | ||
| def read_cheese_types( |
There was a problem hiding this comment.
This function name, read_cheese_types, seems to be a leftover from a template or example. It's a bit confusing for an endpoint that fetches books. Consider renaming it to something more descriptive, like read_books, to match the endpoint's functionality.
| ) | ||
|
|
||
|
|
||
| @app.post("/books/", response_model=schemas.BookSchemaBase) |
There was a problem hiding this comment.
The response_model is set to schemas.BookSchemaBase, which doesn't include the id of the book. It's a common practice in REST APIs to return the full resource, including its unique identifier, after it's been created.
Consider changing this to schemas.BookListSchema to ensure the id of the newly created book is included in the response.
2 participants
No description provided.