Skip to content

Develop#643

Open
B0hdanR wants to merge 4 commits into
mate-academy:mainfrom
B0hdanR:develop
Open

Develop#643
B0hdanR wants to merge 4 commits into
mate-academy:mainfrom
B0hdanR:develop

Conversation

@B0hdanR
Copy link
Copy Markdown

@B0hdanR B0hdanR commented Apr 29, 2026

No description provided.

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Your implementation covers most requirements well - the project structure, FastAPI endpoints, SQLAlchemy models, relationships, schemas, and CRUD operations are all properly implemented. However, Checklist Item #18 is not met: the database tables are not being created automatically on application startup. You need to import Base and engine from database.py (and import models to register the models with Base's metadata), then call Base.metadata.create_all(bind=engine) at module level in main.py - this ensures tables exist when the API starts. There's also a secondary issue in crud.py's get_books_list function where pagination (offset/limit) is applied before the author_id filter, which could return incorrect results when filtering by author with pagination - move the .offset(skip).limit(limit) calls to after the .where() clause in the query chain. These are functional blockers that prevent the application from working correctly on first run.


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread alembic/env.py
Comment thread alembic/versions/5947a86f7d9b_initial_migration.py
Comment thread alembic/versions/5947a86f7d9b_initial_migration.py
Comment thread database.py
Comment thread main.py Outdated
Comment thread models.py
Comment thread schemas.py
Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Your implementation has a critical functional bug in crud.py's get_books_list function. When author_id is None, the pagination (offset(skip).limit(limit)) is only applied inside the if author_id: conditional block, so it never executes. This means the /books/ endpoint returns ALL books in the database instead of paginated results when no author filter is provided—violating the core requirement for paginated book retrieval.

Critical Issue to Fix:
Move the .offset(skip).limit(limit) calls outside the if author_id: block so pagination always applies, regardless of whether filtering by author. The author filter should be conditional, but pagination should be unconditional.

Here's the fix pattern:

query = select(Book)
if author_id is not None:
    query = query.where(Book.author_id == author_id)
query = query.offset(skip).limit(limit)  # Always apply pagination

This same bug exists in alembic/env.py and alembic/versions/5947a86f7d9b_initial_migration.py according to the file comments—ensure pagination is applied after filtering in all affected locations.

Minor note: Consider validating that author_id values are positive integers when provided, though this isn't blocking.

Once this pagination bug is fixed, your implementation will meet all requirements. The project structure, models, schemas, CRUD operations, and API endpoints are otherwise well-implemented.


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread alembic/env.py
Comment thread crud.py
Comment thread alembic/versions/5947a86f7d9b_initial_migration.py
Comment thread main.py
Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Your implementation is approved. Excellent work fixing the critical pagination bug in crud.py — the .offset(skip).limit(limit) calls are now correctly positioned outside the conditional block, ensuring pagination works regardless of whether author filtering is applied. All requirements are satisfied: proper FastAPI/SQLAlchemy/SQLite stack, correct Author and Book models with relationships, Pydantic schemas for validation, CRUD operations, and all required API endpoints with pagination support. Your project structure and database setup are clean and well-organized. Great job!


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

Footnotes

  1. Rate AI review example

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants