library management solution by StrawHato · Pull Request #636 · mate-academy/py-fastapi-library-management-api

StrawHato · 2026-04-05T22:48:27Z

No description provided.

mateacademy-ai-mentor

Review Summary

Great job on the overall structure of your FastAPI application! The code is well-organized into separate modules for models, schemas, and CRUD operations. However, I'm requesting a couple of changes to address critical issues that prevent the application from meeting all requirements.

First, the application is missing the logic to create the database tables on startup. This is a core requirement, and without it, the API will not function. You can fix this in main.py by importing your models and engine, then calling models.Base.metadata.create_all(bind=engine) before the app starts.

Second, the endpoint for creating a book does not validate if the provided author_id exists. This leads to an unhandled 500 error if an invalid ID is used. To fix this, you should first check if the author exists and raise an HTTPException with a 404 status if they are not found.

Once these two issues are resolved, your project will be in great shape!

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor · 2026-04-05T22:50:41Z

+def upgrade() -> None:
+    """Upgrade schema."""
+    # ### commands auto generated by Alembic - please adjust! ###
+    pass


This upgrade function is empty. Based on the migration's name, it seems you intended to add lengths to the string columns in your models. You'll need to use op.alter_column() to apply these changes to the database schema.

mateacademy-ai-mentor · 2026-04-05T22:50:41Z

+def downgrade() -> None:
+    """Downgrade schema."""
+    # ### commands auto generated by Alembic - please adjust! ###
+    pass


The downgrade function is also empty. It should contain the logic to reverse the changes made in the upgrade function. If upgrade adds string lengths, downgrade should remove them.

mateacademy-ai-mentor · 2026-04-05T22:50:41Z

+    # ### commands auto generated by Alembic - please adjust! ###
+    op.create_table('author',
+    sa.Column('id', sa.Integer(), nullable=False),
+    sa.Column('name', sa.String(), nullable=True),


The name column is missing the string length (255) defined in the Author model. Also, consider making this column non-nullable (nullable=False), as an author's name is a key identifier and should generally not be optional.

mateacademy-ai-mentor · 2026-04-05T22:50:41Z

+    op.create_table('author',
+    sa.Column('id', sa.Integer(), nullable=False),
+    sa.Column('name', sa.String(), nullable=True),
+    sa.Column('bio', sa.String(), nullable=False),


This column is missing the string length (512) that is specified in the Author model in models.py.

mateacademy-ai-mentor · 2026-04-05T22:50:41Z

+    op.create_index(op.f('ix_author_id'), 'author', ['id'], unique=False)
+    op.create_table('book',
+    sa.Column('id', sa.Integer(), nullable=False),
+    sa.Column('title', sa.String(), nullable=True),


The title column is missing the string length (255) from the Book model. It's also recommended to make the title of a book non-nullable (nullable=False) for better data integrity.

mateacademy-ai-mentor · 2026-04-05T22:50:41Z

+def create_book(book: schemas.BookCreate, db: Session = Depends(get_db)):
+    return crud.create_book(db=db, book=book)


This endpoint doesn't check if the author_id provided in the book payload corresponds to an existing author. If an invalid author_id is passed, the database will raise a foreign key constraint error, resulting in an unhandled 500 error. It would be better to first verify the author exists and return a 404 error if not found.

mateacademy-ai-mentor · 2026-04-05T22:50:41Z

+    __tablename__ = "author"
+
+    id = Column(Integer, primary_key=True, index=True)
+    name = Column(String(255), unique=True)


To ensure every author has a name, it's good practice to make this column non-nullable. You can do this by adding nullable=False, similar to how the bio field is defined.

mateacademy-ai-mentor · 2026-04-05T22:50:41Z

+    __tablename__ = "book"
+
+    id = Column(Integer, primary_key=True, index=True)
+    title = Column(String(255))


A book's title is typically a required piece of information. Consider making this column non-nullable by adding nullable=False to prevent creating book records without a title.

mateacademy-ai-mentor · 2026-04-05T22:50:41Z

+    title = Column(String(255))
+    summary = Column(String(512))
+    publication_date = Column(Date)
+    author_id = Column(Integer, ForeignKey("author.id"))


This foreign key is currently nullable, which means a book can exist without being associated with an author. To enforce the relationship and prevent 'orphan' books, you should make this column non-nullable by adding nullable=False.

mateacademy-ai-mentor · 2026-04-05T22:50:41Z

+    return db.query(models.Author).filter(models.Author.id == author_id).first()
+
+
+def get_books_list(db: Session, author_id: int, skip: int = 0, limit: int = 10):


The type hint for author_id is int, but this parameter can be None when the /books/ endpoint is called without an author_id query parameter (as defined in main.py). To accurately reflect this, you should update the type hint to int | None.

mateacademy-ai-mentor

Review Summary

Excellent work on this submission! You've successfully addressed all the previous feedback, and the application now meets all the core requirements. The database tables are created correctly on startup, and author validation is properly implemented.

I am approving your solution. I did find a couple of minor areas for future improvement, though they are not blockers:

In crud.py, the type hint for author_id in the get_books_list function could be int | None to perfectly match its usage in main.py.
The migration file alembic/versions/fe5305001d25_add_author_and_book_models.py doesn't specify lengths for the String columns, which is inconsistent with your models. While not an issue here since you use create_all, it's good practice for migrations to be precise.

Great job getting everything working correctly!

✨ Please rate this review in the Platform's notification center¹. I will be grateful for your feedback! ✨

Rate AI review example ↩

mateacademy-ai-mentor · 2026-04-05T23:33:53Z

+    sa.Column('name', sa.String(), nullable=True),
+    sa.Column('bio', sa.String(), nullable=False),
+    sa.PrimaryKeyConstraint('id'),
+    sa.UniqueConstraint('name')
+    )
+    op.create_index(op.f('ix_author_id'), 'author', ['id'], unique=False)
+    op.create_table('book',
+    sa.Column('id', sa.Integer(), nullable=False),
+    sa.Column('title', sa.String(), nullable=True),
+    sa.Column('summary', sa.String(), nullable=True),


This migration doesn't specify the lengths for the String columns (name, bio, title, summary). This is inconsistent with your models.py file, which defines specific lengths (e.g., String(255)).

While SQLite might not enforce this, it's important for migrations to accurately reflect the models for consistency and portability to other database systems. Please update the sa.String() calls here to include the lengths defined in your models.

mateacademy-ai-mentor · 2026-04-05T23:33:53Z

+    return db.query(models.Author).filter(models.Author.id == author_id).first()
+
+
+def get_books_list(db: Session, author_id: int, skip: int = 0, limit: int = 10):


The type hint for author_id is int, but the function logic correctly handles the case where it might be None (on line 34). The corresponding endpoint in main.py also defines this parameter as optional (int | None).

To ensure consistency and accurate type hinting, please update the signature to def get_books_list(db: Session, author_id: int | None, skip: int = 0, limit: int = 10):.

Maksym Pakula added 9 commits April 5, 2026 21:32

Add requirements.txt

bb1b9d6

Set up database connection

a6fc780

Add alembic for migrations

6a635db

Add Author & Book models with migration

d3cee6a

Add Author & Book schemas

5a336bd

Add missed strings length

f829c03

Remove unneccesary Books line

18aa687

Add all crud functions

f28bd5a

Add endpoints for all cruds

4d2bc95

mateacademy-ai-mentor suggested changes Apr 5, 2026

View reviewed changes

Add author check before creating book and db start on API start

578230a

StrawHato requested a review from mateacademy-ai-mentor April 5, 2026 23:32

mateacademy-ai-mentor approved these changes Apr 5, 2026

View reviewed changes

		def create_book(book: schemas.BookCreate, db: Session = Depends(get_db)):
		return crud.create_book(db=db, book=book)

		return db.query(models.Author).filter(models.Author.id == author_id).first()


		def get_books_list(db: Session, author_id: int, skip: int = 0, limit: int = 10):

Conversation

StrawHato commented Apr 5, 2026

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants