Library Management API#638
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are a couple of functional gaps that can cause the API to fail at runtime, even though the overall structure and use of FastAPI + SQLAlchemy is solid.
Critical issues that must be fixed:
- In
main.py(near the top, around the imports andapp = FastAPI()definition), the app never creates the database tables from your SQLAlchemy models. Per the requirements, you should importBaseandenginefromdb.databaseand callBase.metadata.create_all(bind=engine)at startup so theauthorsandbookstables exist before any requests are handled. - The
create_bookendpoint inmain.pycurrently usesresponse_model=BookCreate, which is the input schema and does not includeidor the nestedauthor. The typical expectation (and tests) is that a create endpoint returns the fullBookschema, so change this toresponse_model=Bookand ensure you actually return aBook-shaped object (includingidand, if intended, the related author). - Your book-related endpoints return ORM
Bookinstances that include anauthorrelationship, but the SQLAlchemy session is closed by the dependency before FastAPI serializes the response. Because the queries incrud.get_books(and afterdb.refresh(db_book)in create-book) do not eagerly loadDBBook.author, this can lead toDetachedInstanceError. To fix this, adjust the queries incrud.pyto eagerly load the relationship, for example usingselect(DBBook).options(selectinload(DBBook.author))..., or explicitly access/populatedb_book.authorwhile the session is still open before returning.
Minor improvements (optional but recommended):
- Consider adding 404 handling for "get by id" endpoints so that requesting a missing author or book returns a clear not-found error instead of
null.
You have done a good job setting up the database module, models, schemas, and basic CRUD wiring; once you address the table creation, response model, and relationship loading issues, your API will be much more robust and aligned with the task 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
| from sqlalchemy.orm import Session | ||
|
|
||
| import crud | ||
| from db.database import SessionLocal |
There was a problem hiding this comment.
You import only SessionLocal from db.database but the app needs to create tables (see requirement). Import Base and the engine (e.g. from db.database import SessionLocal, engine, Base) so you can call Base.metadata.create_all(bind=engine) at startup.
| from db.database import SessionLocal | ||
| from schemas import AuthorCreate, Book, BookCreate, Author | ||
|
|
||
| app = FastAPI(title="Library Management API") |
There was a problem hiding this comment.
The code never creates the database tables using the SQLAlchemy models. Per the task you should call Base.metadata.create_all(bind=engine) (for example after imports / at app startup) so the authors and books tables exist when the API runs.
| return crud.get_author_by_id(db=db, author_id=author_id) | ||
|
|
||
|
|
||
| @app.post("/book", response_model=BookCreate) |
There was a problem hiding this comment.
The create-book endpoint uses response_model=BookCreate, which is the input schema (no id or nested author). It should return the full Book schema (including id and author) or adjust the returned shape to match BookCreate intentionally.
| db: Session = Depends(get_db), | ||
| author_id: int = None | ||
| ): | ||
| return crud.get_books(db=db, skip=skip, limit=limit, author_id=author_id) |
There was a problem hiding this comment.
This return invokes crud.get_books and will return ORM instances with a nested author in the Book response model. Because the DB session is closed after the dependency returns, accessing lazy relationships during response serialization can raise a DetachedInstanceError. Eager-load the author relationship in the query or serialize while the session is open.
| @app.get("/books", response_model=list[Book]) | ||
| def get_books( | ||
| skip: int = 0, | ||
| limit: int = 10, |
There was a problem hiding this comment.
The books query builds select(DBBook).offset(skip).limit(limit) but does not load the author relationship. To safely serialize nested author after the session closes, add relationship loading, e.g. select(DBBook).options(selectinload(DBBook.author))... or joinedload as appropriate.
| @app.get("/author/{author_id}", response_model=Author) | ||
| def get_authors_by_id(author_id: int, db: Session = Depends(get_db)): | ||
| return crud.get_author_by_id(db=db, author_id=author_id) | ||
|
|
There was a problem hiding this comment.
After creating the book you call db.refresh(db_book), which refreshes scalar columns but does not guarantee the author relationship is loaded. If the endpoint must return the nested author, either load the relationship before returning (e.g., db.refresh(db_book); _ = db_book.author) or re-query the book with an eager load while the session is still open.
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 and it clearly meets the core requirements: the FastAPI app, SQLite database configuration, SQLAlchemy models, CRUD utilities, and author/book endpoints are all implemented and wired together correctly. The database layer (db/database.py) and your Pydantic schemas (schemas.py) are set up properly for ORM integration and cover all required fields, so the API should behave as expected for the main use cases. One improvement to consider is validating that an author_id exists before creating a Book (in the function where you construct and commit DBBook), and returning a 400 or 404 instead of relying on a database integrity error during commit(). Similarly, for your GET-by-id endpoints (e.g., fetching a single author or book), it would be more user-friendly to return an explicit 404 when the record is not found rather than None or an empty payload. As a further enhancement, you might catch IntegrityError when creating authors to handle duplicate names gracefully with a clear 400 response. Overall, the implementation is solid and complete, and these suggestions would mainly improve robustness and client-facing error messages.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
|
|
||
| id: Mapped[int] = mapped_column(primary_key=True, index=True) | ||
| title: Mapped[str] = mapped_column() | ||
| summary: Mapped[str] = mapped_column() |
There was a problem hiding this comment.
When creating a book you currently construct and commit the DBBook directly. Consider verifying that the referenced author exists first (e.g., query for the author and return a clear 400/404 if missing) so the endpoint fails with a helpful error instead of raising a DB integrity error during commit.
3 participants
No description provided.