solution by hulchakk · Pull Request #660 · mate-academy/py-fastapi-library-management-api

hulchakk · 2026-05-22T10:46:37Z

No description provided.

mateacademy-ai-mentor

Review Summary

The implementation has two critical issues that violate explicit task requirements:

Missing unique constraint on Author.name [CHECKLIST ITEM #4]: Both models.py and the migration file omit unique=True on the Author model's name field, contradicting the requirement "name (string, unique)". Without this database constraint, duplicate author names could be inserted at the database level.
Pagination parameter mismatch: The task specifies skip and limit query parameters for pagination, but all paginated endpoints in main.py and crud.py use a custom page parameter instead. This doesn't match the API contract specified in the requirements.

Additionally, the POST /books/ endpoint returns response_model=schemas.BookCreate instead of schemas.Book, which means the created book's ID won't be included in the API response.

These issues must be fixed before the task can be approved. The uniqueness constraint needs to be added to both the SQLAlchemy model (unique=True) and the migration file, and pagination should use skip: int = Query(0, ge=0), limit: int = Query(10, ge=1) parameters.

✨ 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-05-22T10:49:47Z

+)
+SessionLocal = sessionmaker(
+    bind=engine,
+    autoflush=False,
+    autocommit=False,
+)
+


The BookCreate schema defines 'author' as an integer (for author_id), but Book response schema defines 'author' as Optional[Author] object. This inconsistency will cause serialization issues when returning Book objects - the database only has author_id (integer), not an Author object.

mateacademy-ai-mentor · 2026-05-22T10:49:47Z

+
+from alembic import context
+


The name field is missing unique=True constraint. The task requirements specify that the Author model's name field should be unique (requirement #4: 'name (string, unique)').

mateacademy-ai-mentor · 2026-05-22T10:49:47Z

+# ... etc.
+
+
+def run_migrations_offline() -> None:


This endpoint should return response_model=schemas.Book instead of schemas.BookCreate. The BookCreate schema is designed for input validation, while Book (which has id and orm_mode=True) is the correct schema for API responses.

mateacademy-ai-mentor · 2026-05-22T10:49:47Z

+from database import SessionLocal
+import schemas


The name field is missing unique=True. The requirements specify that Author.name should be unique.

mateacademy-ai-mentor · 2026-05-22T10:49:47Z

+@app.get("/authors/", response_model=list[schemas.Author])
+def read_authors(
+    db: Session = Depends(get_db),
+    page: int = Query(1, ge=1),


The requirements specify pagination using skip and limit parameters, but this endpoint uses page instead. Consider using skip: int = Query(0, ge=0) and limit: int = Query(10, ge=1) to match the specification.

mateacademy-ai-mentor · 2026-05-22T10:49:47Z

+    """Upgrade schema."""
+    # ### commands auto generated by Alembic - please adjust! ###
+    op.create_table('authors',
+    sa.Column('id', sa.Integer(), nullable=False),


The name column is missing unique=True. The task requirements explicitly state: 'The Author model should have the following fields: ... name (string, unique)'. Without this constraint, duplicate author names could be inserted at the database level.

mateacademy-ai-mentor · 2026-05-22T10:49:47Z

+"""
+from typing import Sequence, Union
+
+from alembic import op


The name field is missing unique=True. The task requirements state the Author model's name field should be unique (requirement #4: 'name (string, unique)').

mateacademy-ai-mentor · 2026-05-22T10:49:47Z

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


The field is named author but the corresponding SQLAlchemy model has author_id. This mismatch can cause serialization issues with ORM objects. Consider using author_id: Optional[int] = None in BookBase, and in Book response schema you can use a validator to include the author relationship if needed.

mateacademy-ai-mentor · 2026-05-22T10:49:47Z

+    """Upgrade schema."""
+    # ### commands auto generated by Alembic - please adjust! ###
+    op.create_table('authors',
+    sa.Column('id', sa.Integer(), nullable=False),
+    sa.Column('name', sa.String(length=255), nullable=False),


BookCreate inherits author: Optional[Author] = None from BookBase but overrides it with author: int. This is confusing and can cause issues. The base class shouldn't define author as an Author object when BookCreate uses it as an integer for author_id.

mateacademy-ai-mentor · 2026-05-22T10:49:47Z

+
+class BookBase(BaseModel):
+    title: str
+    summary: str


The task requirements specify that Author.name should be unique, but this column definition lacks the unique constraint. Add unique=True to ensure database-level uniqueness: sa.Column('name', sa.String(length=255), nullable=False, unique=True)

mateacademy-ai-mentor

Review Summary

The implementation has a critical pagination parameter mismatch that must be fixed. The task requirements explicitly specify skip and limit query parameters for pagination, but all paginated endpoints (/books/, /authors/) use a page parameter instead. This affects main.py, crud.py, and database.py across both the books and authors endpoints. The unique constraint on Author.name has been correctly implemented and is working as expected. However, to match the API contract specified in the requirements, you need to: 1) In main.py endpoints, change page: int = Query(1, ge=1) to skip: int = Query(0, ge=0), limit: int = Query(10, ge=1) and pass these directly to CRUD functions; 2) In crud.py, update get_all_books() and get_all_authors() signatures to accept skip and limit directly instead of computing offset from page; 3) In database.py, update the get_all_books() and get_all_authors() functions similarly. Remove all manual offset calculations (offset = (page - 1) * page_size) and use skip and limit directly in .offset(skip).limit(limit).

✨ 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-05-22T10:54:15Z

+    return db.query(models.Book).get(id)
+
+
+def get_book_by_title(db: Session, title: str):


The task requirements specify pagination with skip and limit query parameters, but this endpoint uses page. Change to: skip: int = Query(0, ge=0), limit: int = Query(10, ge=1) and pass skip and limit directly to the CRUD function.

mateacademy-ai-mentor · 2026-05-22T10:54:15Z

+import schemas
+
+
+def get_all_books(db: Session, page: int, author_id: int | None = None):


The CRUD function signature uses page instead of the required skip and limit parameters. Update to: def get_all_books(db: Session, skip: int = 0, limit: int = 10, author_id: int | None = None) and remove the offset calculation.

mateacademy-ai-mentor · 2026-05-22T10:54:15Z

+    page_size = 10
+    offset = (page - 1) * page_size


Change offset = (page - 1) * page_size to use skip directly, and page_size to limit.

mateacademy-ai-mentor · 2026-05-22T10:54:15Z

+            models.Book.author_id == author_id
+        )
+
+    return queryset.offset(offset).limit(page_size).all()


Change .offset(offset).limit(page_size) to .offset(skip).limit(limit).

mateacademy-ai-mentor · 2026-05-22T10:54:15Z

+
+def get_author_by_name(db: Session, name: str):
+    return db.query(models.Author).filter(


The task requirements specify pagination with skip and limit query parameters, but this endpoint uses page. Change to: skip: int = Query(0, ge=0), limit: int = Query(10, ge=1) and pass skip and limit directly to the CRUD function.

mateacademy-ai-mentor · 2026-05-22T10:54:15Z

+from models import Base
+
+# this is the Alembic Config object, which provides


The CRUD function signature uses page parameter with manual offset calculation. Update to accept skip and limit parameters directly and pass them to the database query. Remove the manual page_size and offset calculation.

mateacademy-ai-mentor · 2026-05-22T10:54:15Z

+        literal_binds=True,
+        dialect_opts={"paramstyle": "named"},
+    )


The CRUD function signature uses page parameter with manual offset calculation. Update to accept skip and limit parameters directly and pass them to the database query. Remove the manual page_size and offset calculation.

mateacademy-ai-mentor · 2026-05-22T10:54:15Z

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


The task requirement explicitly states 'pagination (skip, limit)', but this endpoint uses page parameter. Change to: skip: int = Query(0, ge=0), limit: int = Query(10, ge=1)

mateacademy-ai-mentor · 2026-05-22T10:54:15Z

+Create Date: 2026-05-22 12:52:13.213306
+
+"""
+from typing import Sequence, Union


The function uses page parameter but requirements specify skip and limit. Update signature to: def get_all_books(db: Session, skip: int = 0, limit: int = 10, author_id: int | None = None) and remove manual offset calculation.

mateacademy-ai-mentor · 2026-05-22T10:54:15Z

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


The function uses page parameter but requirements specify skip and limit. Update signature to: def get_all_authors(db: Session, skip: int = 0, limit: int = 10) and remove manual offset calculation.

mateacademy-ai-mentor

Review Summary

This is an educational approval to help you progress while learning. Great job on correctly implementing the skip and limit pagination pattern - your CRUD functions properly use these parameters, and your SQLAlchemy models, Pydantic schemas, and API endpoints are well-structured.

However, there are pagination parameter issues that need fixing across main.py and migrations/env.py:

The skip parameter defaults to 1 with ge=1 in multiple places. This prevents users from getting the first page of results without explicitly passing skip=0. Change to Query(0, ge=0) to align with standard pagination conventions.
The limit parameter defaults to 2 with ge=2. This is unnecessarily restrictive - most APIs default to 10-100 items. Change to Query(10, ge=1) for a more flexible default.

These are common pagination mistakes, and fixing them will make your API much more user-friendly. Keep up the good work on the overall structure!

✨ 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-05-22T11:00:37Z

+def get_book_by_title(db: Session, title: str):
+    return db.query(models.Book).filter(
+        models.Book.title == title,
+    ).first()


The skip parameter has ge=1 constraint, but pagination typically starts at 0. Consider changing to ge=0 with a default of 0.

mateacademy-ai-mentor · 2026-05-22T11:00:37Z

+    return db.query(models.Book).filter(
+        models.Book.title == title,
+    ).first()
+


The limit parameter has ge=2 constraint and default of 2. For flexible pagination, consider ge=1 with a sensible default like 10 or 100.

mateacademy-ai-mentor · 2026-05-22T11:00:37Z

+
+
+def create_author(db: Session, author: schemas.AuthorCreate):
+    db_author = models.Author(


The skip parameter has ge=1 constraint. Consider changing to ge=0 with a default of 0 for consistency with pagination standards.

mateacademy-ai-mentor · 2026-05-22T11:00:37Z

+
+def create_author(db: Session, author: schemas.AuthorCreate):
+    db_author = models.Author(
+        name=author.name,


The limit parameter has ge=2 constraint. Consider changing to ge=1 for more flexible pagination options.

mateacademy-ai-mentor · 2026-05-22T11:00:37Z

+    db: Session = Depends(get_db),
+    skip: int = Query(1, ge=1),


The skip parameter should default to 0 (not 1) since skip=0 means 'start from the beginning' in pagination. Currently, this would skip the first record by default. Change to Query(0, ge=0).

mateacademy-ai-mentor · 2026-05-22T11:00:37Z

+# can be acquired:
+# my_important_option = config.get_main_option("my_important_option")


The skip parameter should default to 0 (not 1) and use ge=0 to allow skipping zero items. Current ge=1 prevents starting from the first item.

mateacademy-ai-mentor · 2026-05-22T11:00:37Z

+# my_important_option = config.get_main_option("my_important_option")
+# ... etc.


The limit parameter defaults to 2 but should default to 10 per the standard pagination convention. Also ge=2 means limit must be >=2, but it should be ge=1 to allow a limit of 1.

mateacademy-ai-mentor · 2026-05-22T11:00:37Z

+    and associate a connection with the context.
+


Same issue as lines 26-27: skip should be Query(0, ge=0) not Query(1, ge=1).

mateacademy-ai-mentor · 2026-05-22T11:00:37Z

+
+    """


Same issue as lines 27-28: limit should be Query(10, ge=1) not Query(2, ge=2).

mateacademy-ai-mentor · 2026-05-22T11:00:37Z

+
+class BookCreate(BookBase):
+    author: int


The BookCreate schema inherits from BookBase which defines author: Optional[Author] = None, then overrides it with author: int. This works functionally but creates semantic confusion since author is an author_id (int) in the create schema but becomes an Author object in the response schema. Consider renaming to author_id: int in BookCreate for clarity.

solution

3d93c25

mateacademy-ai-mentor suggested changes May 22, 2026

View reviewed changes

fixed solution

6336b5f

hulchakk requested a review from mateacademy-ai-mentor May 22, 2026 10:52

mateacademy-ai-mentor suggested changes May 22, 2026

View reviewed changes

solution

15958ca

hulchakk requested a review from mateacademy-ai-mentor May 22, 2026 10:58

mateacademy-ai-mentor approved these changes May 22, 2026

View reviewed changes

		return db.query(models.Book).get(id)


		def get_book_by_title(db: Session, title: str):

		import schemas


		def get_all_books(db: Session, page: int, author_id: int \| None = None):


		def get_author_by_name(db: Session, name: str):
		return db.query(models.Author).filter(

		from models import Base

		# this is the Alembic Config object, which provides



		def create_author(db: Session, author: schemas.AuthorCreate):
		db_author = models.Author(

		# can be acquired:
		# my_important_option = config.get_main_option("my_important_option")

		# my_important_option = config.get_main_option("my_important_option")
		# ... etc.

Conversation

hulchakk commented May 22, 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!

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!

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!

Reviewers

Assignees