Add CRUD endpoints for movies with pagination and validation #38
Conversation
Implemented CRUD operations for managing movies, including creation, reading, updating, and deletion. Added pagination to the movie listing endpoint and incorporated robust validation with Pydantic schemas. Updated dependencies in `pyproject.toml` to support new features like extra types and country validation.
There was a problem hiding this comment.
Thank you for your hard work on this project! Your implementation is mostly correct and meets the task requirements, but there are a few critical issues that need to be addressed before we can approve the pull request:
-
Budget and Revenue Fields: The
budgetandrevenuefields inMovieCreateResponseSchemaare set togt=0, which prevents them from being zero. Since budgets and revenues can start at zero, especially when a movie is first created, please changegt=0toge=0to allow zero values. -
Committing Within Loops: Committing within loops for genres, actors, and languages can lead to inefficiencies and potential performance issues. It's better to commit once after processing all items to improve performance.
-
Typo in Error Message: There's a typo in the error message: 'is requir' should be 'are required'. This affects the clarity of the error message and needs to be corrected.
Once these issues are resolved, your code will be in great shape! Keep up the excellent work, and feel free to reach out if you have any questions or need further assistance. 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.
| if not movie_data.name or not movie_data.date or not movie_data.overview or not movie_data.status: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_400_BAD_REQUEST, | ||
| detail="Fields 'name', 'date', 'overview' and 'status' is requir to create movie." |
There was a problem hiding this comment.
There is a typo in the error message: 'is requir' should be 'are required'. This is an error related to the task requirements as it affects the clarity of the error message.
| if not genre: | ||
| genre = GenreModel(name=genre_name) | ||
| db.add(genre) | ||
| db.commit() |
There was a problem hiding this comment.
Committing within a loop can lead to inefficiencies and potential performance issues. Consider committing once after all genres have been processed.
| if not actor: | ||
| actor = ActorModel(name=actor_name) | ||
| db.add(actor) | ||
| db.commit() |
There was a problem hiding this comment.
Committing within a loop can lead to inefficiencies and potential performance issues. Consider committing once after all actors have been processed.
| if not language: | ||
| language = LanguageModel(name=language_name) | ||
| db.add(language) | ||
| db.commit() |
There was a problem hiding this comment.
Committing within a loop can lead to inefficiencies and potential performance issues. Consider committing once after all languages have been processed.
| budget: float = Field(gt=0) | ||
| revenue: float = Field(gt=0) |
There was a problem hiding this comment.
The budget and revenue fields in MovieCreateResponseSchema are set to gt=0, which means they cannot be zero. Typically, budgets and revenues can start at zero, especially when a movie is first created. Consider changing gt=0 to ge=0 to allow zero values.
response_model=None
There was a problem hiding this comment.
Great job on implementing the movie routes in FastAPI! 🎬 While your code is mostly correct, there are a few critical issues that need attention:
-
Committing Within Loops: Currently, the code commits changes to the database within loops for genres, actors, and languages. This can lead to inefficiencies and potential performance issues. It's recommended to commit once after processing all items to enhance performance.
-
Typo in Error Message: There's a typo in the error message on line 109: 'is requir' should be corrected to 'are required' to improve clarity.
-
Budget and Revenue Fields: The
budgetandrevenuefields in bothMovieCreateResponseSchemaandMovieUpdateResponseSchemaare set togt=0, which prevents them from being zero. Since budgets and revenues can start at zero, especially when a movie is first created, please changegt=0toge=0to allow zero values.
These adjustments are crucial for the functionality and performance of your code. Addressing these issues will not only improve your current implementation but also help you avoid similar problems in the future. Keep up the good work, and continue learning from these experiences! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| if not genre: | ||
| genre = GenreModel(name=genre_name) | ||
| db.add(genre) | ||
| db.commit() |
There was a problem hiding this comment.
Committing within the loop for genres can lead to inefficiencies. Consider moving the commit outside the loop to improve performance.
| if not actor: | ||
| actor = ActorModel(name=actor_name) | ||
| db.add(actor) | ||
| db.commit() |
There was a problem hiding this comment.
Committing within the loop for actors can lead to inefficiencies. Consider moving the commit outside the loop to improve performance.
| if not language: | ||
| language = LanguageModel(name=language_name) | ||
| db.add(language) | ||
| db.commit() |
There was a problem hiding this comment.
Committing within the loop for languages can lead to inefficiencies. Consider moving the commit outside the loop to improve performance.
| if not movie_data.name or not movie_data.date or not movie_data.overview or not movie_data.status: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_400_BAD_REQUEST, | ||
| detail="Fields 'name', 'date', 'overview' and 'status' is requir to create movie." |
There was a problem hiding this comment.
Typo in error message: 'is requir' should be 'are required'.
| score: float = Field(ge=0, le=100) | ||
| overview: str | ||
| status: MoviesStatusEnumSchema | ||
| budget: float = Field(gt=0) |
There was a problem hiding this comment.
The budget field is set to gt=0, which prevents it from being zero. Change gt=0 to ge=0 to allow zero values.
| overview: str | ||
| status: MoviesStatusEnumSchema | ||
| budget: float = Field(gt=0) | ||
| revenue: float = Field(gt=0) |
There was a problem hiding this comment.
The revenue field is set to gt=0, which prevents it from being zero. Change gt=0 to ge=0 to allow zero values.
| score: Optional[float] = Field(None, ge=0, le=100) | ||
| overview: Optional[str] = None | ||
| status: Optional[MoviesStatusEnumSchema] = None | ||
| budget: Optional[float] = Field(None, gt=0) |
There was a problem hiding this comment.
The budget field in MovieUpdateResponseSchema is set to gt=0, which prevents it from being zero. Change gt=0 to ge=0 to allow zero values.
| overview: Optional[str] = None | ||
| status: Optional[MoviesStatusEnumSchema] = None | ||
| budget: Optional[float] = Field(None, gt=0) | ||
| revenue: Optional[float] = Field(None, gt=0) |
There was a problem hiding this comment.
The revenue field in MovieUpdateResponseSchema is set to gt=0, which prevents it from being zero. Change gt=0 to ge=0 to allow zero values.
Implemented CRUD operations for managing movies, including creation, reading, updating, and deletion. Added pagination to the movie listing endpoint and incorporated robust validation with Pydantic schemas. Updated dependencies in
pyproject.tomlto support new features like extra types and country validation.