1#641
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The provided response is not valid JSON and contains markdown formatting, comments, and instructions instead of a proper JSON object. It must be fixed to match the expected schema with 'decision' as either 'APPROVE' or 'REQUEST_CHANGES' and a 'reason' string.
✨ 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.orm import Session | ||
|
|
||
| import schemas | ||
| from db import models |
There was a problem hiding this comment.
The import statement from db import models indicates the models are located in a db/ subdirectory. The requirements specified creating models.py at the project root level. Ensure this structure matches your intended deployment.
| @@ -0,0 +1,11 @@ | |||
| from sqlalchemy import create_engine | |||
There was a problem hiding this comment.
The main.py file is missing the database table creation on startup. The task requires creating database tables using SQLAlchemy models, but there's no Base.metadata.create_all(bind=engine) call anywhere. Add this at the end of main.py to initialize the tables when the app starts.
| @@ -0,0 +1,24 @@ | |||
| from datetime import date | |||
There was a problem hiding this comment.
The task requirements explicitly state that main.py must 'Create the database tables using the SQLAlchemy models'. This critical initialization step is missing. Add from db.database import engine, Base and Base.metadata.create_all(bind=engine) after the app initialization to create the tables when the application starts.
| def get_books(skip: int = 0, limit: int = 10, db: Session = Depends(get_db)): | ||
| return crud.get_books(db, skip=skip, limit=limit) | ||
|
|
||
| @app.get("/books/by-author/{author_id}", response_model=list[schemas.Book]) |
There was a problem hiding this comment.
The skip and limit parameters on line 43 don't have default values (unlike lines 17 and 39 which use skip: int = 0, limit: int = 10). This makes them required parameters, but users would expect optional pagination like other endpoints. Add default values: skip: int = 0, limit: int = 10.
| @app.get("/books/by-author/{author_id}", response_model=list[schemas.Book]) | ||
| def get_books_by_author(author_id: int, skip: int, limit: int, db: Session = Depends(get_db)): |
There was a problem hiding this comment.
The endpoint retrieves books by author but doesn't verify the author exists first. Unlike /authors/{author_id} (line 27-28) which returns 404 for non-existent authors, this endpoint silently returns an empty list. Consider adding an author existence check for consistency and clearer API behavior.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This implementation has two critical blockers that prevent the API from functioning correctly. First, the database tables are never created — you must add Base.metadata.create_all(bind=engine) in main.py before the app starts, as this is explicitly required by the task checklist item #8. Without this, the SQLite database will have no tables and all queries will fail. Second, the get_books_by_author endpoint in main.py assigns the result to a variable but never returns it, causing the endpoint to return null instead of the book list. Add return books at the end of that function. Additionally, the BookBase schema incorrectly includes author_id as a request body field — since books are created through /authors/{author_id}/books, the author_id should come from the path parameter, not the request body. Remove author_id from BookBase and keep it only in the Book response schema for serialization. These issues must be fixed before the API can work properly.
✨ 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
| # 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 |
There was a problem hiding this comment.
The author_id field should not be in BookBase and BookCreate schemas. Since books are created through /authors/{author_id}/books, the author_id is determined by the path parameter, not the request body. Remove author_id from these schemas.
| # target_metadata = mymodel.Base.metadata | ||
| target_metadata = [Base.metadata] | ||
|
|
||
| # other values from the config, defined by the needs of env.py, | ||
| # can be acquired: |
There was a problem hiding this comment.
The Book response schema doesn't need author_id since it already includes the full author relationship. Including both is redundant. Consider removing author_id from this response schema.
| from logging.config import fileConfig | ||
|
|
||
| from sqlalchemy import engine_from_config | ||
| from sqlalchemy import pool | ||
|
|
||
| from alembic import context | ||
|
|
||
| from models import Base | ||
|
|
||
| # this is the Alembic Config object, which provides | ||
| # access to the values within the .ini file in use. | ||
| config = context.config | ||
|
|
||
| # Interpret the config file for Python logging. | ||
| # This line sets up loggers basically. |
There was a problem hiding this comment.
Missing implementation: The task requires creating database tables using SQLAlchemy models in main.py (requirement #8). Add Base.metadata.create_all(bind=engine) to create tables on startup.
| """ | ||
| url = config.get_main_option("sqlalchemy.url") | ||
| context.configure( |
There was a problem hiding this comment.
For consistency with the create endpoint (which checks if author exists), consider verifying the author exists before querying their books. If the author doesn't exist, return 404.
|
|
||
| """ | ||
| from typing import Sequence, Union | ||
|
|
There was a problem hiding this comment.
The database tables are never created. Need to call Base.metadata.create_all(bind=engine) to initialize the database schema on application startup.
| @app.get("/books/by-author/{author_id}", response_model=list[schemas.Book]) | ||
| def get_books_by_author(author_id: int, skip: int, limit: int, db: Session = Depends(get_db)): |
There was a problem hiding this comment.
Missing author validation: Unlike create_book and get_author endpoints which verify the author exists first, this endpoint should also check if the author exists and return 404 if not found. Consider using crud.get_author(db, author_id=author_id) to verify before querying books.
| from datetime import date | ||
|
|
||
| from pydantic import BaseModel | ||
|
|
||
|
|
||
| class AuthorBase(BaseModel): | ||
| name: str | ||
| bio: str | ||
|
|
||
|
|
||
| class AuthorCreate(AuthorBase): | ||
| pass | ||
|
|
||
|
|
||
| class Author(AuthorBase): |
There was a problem hiding this comment.
Missing requirement: The task specifies creating database tables on startup. Add from database import engine, Base and call Base.metadata.create_all(bind=engine) before defining routes.
| class BookBase(BaseModel): | ||
| title: str | ||
| summary: str | ||
| publication_date: date |
There was a problem hiding this comment.
Schema design issue: author_id should be removed from BookBase. Since books are created via /authors/{author_id}/books, the author_id comes from the path, not the request body. This makes BookCreate require an unnecessary author_id field.
| class BookCreate(BookBase): | ||
| pass | ||
|
|
||
|
|
There was a problem hiding this comment.
Redundant field: The Book response schema inherits author_id from BookBase but also has author: Author. Including both is redundant. Consider creating separate schemas: BookCreate without author_id, BookResponse with author relationship only.
| from datetime import date | ||
|
|
||
| from sqlalchemy import ForeignKey, Date, String | ||
| from sqlalchemy.orm import Mapped, mapped_column, relationship | ||
|
|
||
| from database import Base | ||
|
|
There was a problem hiding this comment.
The task requires creating database tables using SQLAlchemy models (requirement #8). Import Base and engine from database.py, then add Base.metadata.create_all(bind=engine) before the app = FastAPI() line to create tables on startup.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation is approved as an educational override. You've done excellent work fixing the two critical blockers from previous reviews - database tables are now properly created at startup with Base.metadata.create_all(bind=engine), and the get_books_by_author endpoint correctly returns results. All core requirements are implemented and functional. One schema design issue remains: BookBase (in schemas.py line 23) includes author_id: int as a request body field, but since books are created via /authors/{author_id}/books, the author_id comes from the path parameter. This means clients would need to provide author_id twice (in URL and body), which is redundant. Remove author_id from BookBase and keep it only in the Book response schema for proper serialization. This is a Medium-priority usability issue rather than a blocking bug. Your core functionality works correctly, and fixing this schema design will make your API cleaner and more intuitive. Keep up the great progress!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| # 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 |
There was a problem hiding this comment.
BookBase includes author_id as a required request body field, but since books are created via /authors/{author_id}/books, the author_id should come from the path parameter, not the request body. Remove author_id from BookBase and keep it only in the Book response schema for serialization.
| ) | ||
| db.add(author) | ||
| db.commit() | ||
| db.refresh(author) |
There was a problem hiding this comment.
The BookBase schema includes author_id: int as a request body field, but since books are created through /authors/{author_id}/books endpoint, the author_id should come from the path parameter, not the request body. Remove author_id from BookBase (keep it only in the response schema) so clients don't need to send author_id in the request body.
| id: Mapped[int] = mapped_column(primary_key=True) | ||
| title: Mapped[str] = mapped_column(String(255)) | ||
| summary: Mapped[str] = mapped_column(String(255)) | ||
| publication_date: Mapped[date] = mapped_column(Date, nullable=False) | ||
| author_id: Mapped[int] = mapped_column(ForeignKey("author.id"), nullable=False) | ||
| author: Mapped[Author] = relationship(back_populates="books") |
There was a problem hiding this comment.
BookBase includes author_id as a request body field, but since books are created through /authors/{author_id}/books, the author_id should come from the path parameter, not the request body. Remove author_id from BookBase and keep it only in the Book response schema for serialization. This will allow clients to create books without duplicating the author_id in both the URL and body.
|
|
||
| class BookBase(BaseModel): | ||
| title: str | ||
| summary: str | ||
| publication_date: date |
There was a problem hiding this comment.
BookBase still includes author_id as a request body field (line 23). Since books are created via /authors/{author_id}/books endpoint, the author_id should come from the path parameter, not the request body. Clients would need to send author_id twice (in path and body), which is redundant. Remove author_id from BookBase and keep it only in the Book response schema for serialization.
| db.close() | ||
|
|
||
|
|
||
| @app.get("/authors/", response_model=list[schemas.Author]) | ||
| def get_authors(skip: int = 0, limit: int = 10, db: Session = Depends(get_db)): | ||
| return crud.get_authors(db, skip=skip, limit=limit) |
There was a problem hiding this comment.
BookBase includes author_id: int as a request body field, but since books are created via /authors/{author_id}/books, the author_id comes from the path parameter. This requires clients to send author_id twice (in path and body), which is redundant. Remove author_id from BookBase and only include it in the Book response schema for serialization.
2 participants
No description provided.