-
Notifications
You must be signed in to change notification settings - Fork 1
Complete mongo db integration for get /books/{id}) #8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Complete mongo db integration for get /books/{id}) #8
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR completes the MongoDB integration for the GET /books/{id} endpoint, transitioning from an in-memory implementation to a database-backed solution. The endpoint now queries MongoDB directly using ObjectId validation and proper error handling.
- Replace in-memory book lookup with MongoDB collection.find_one() queries
- Add ObjectId validation for book IDs with appropriate 400 error responses
- Implement comprehensive test suite with both unit and integration tests covering all error scenarios
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| app/routes.py | Replaces in-memory book lookup with MongoDB queries, adds ObjectId validation and proper error handling |
| tests/test_app.py | Adds comprehensive test coverage with unit tests using mocks and integration tests with real database operations |
| tests/conftest.py | Introduces db_setup fixture for clean database state management in tests |
| openapi.yml | Updates API documentation to reflect new error responses (400, 404, 500) with proper schemas |
| mock_collection.find_one.assert_called_once() | ||
| mock_collection.find_one.assert_called_once_with( | ||
| {"_id": fake_book_id, "state": {"$ne": "deleted"}} | ||
| ) # pylint: disable=line-too-long |
Copilot
AI
Jul 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The pylint disable comment for line-too-long is unnecessary. The line is well within reasonable limits and the comment adds clutter.
| ) # pylint: disable=line-too-long | |
| ) |
| # Check that the JSON error message is exactly what the code returns | ||
| expected_error = {"error": "Invalid book ID format"} | ||
| assert response.get_json() == expected_error | ||
|
|
Copilot
AI
Jul 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] This function is missing a blank line before the function definition, which is inconsistent with the formatting of other test functions in the file.
| # Query db for a non-deleted book | ||
| query = {"_id": obj_id, "state": {"$ne": "deleted"}} | ||
| # look it up in MongoDB | ||
| book = collection.find_one(query) |
Copilot
AI
Jul 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding exception handling around the MongoDB query. Database operations can raise exceptions (ConnectionFailure, ServerSelectionTimeoutError) that should be caught and converted to appropriate HTTP error responses.
| # also equivalent to Key version | ||
| # book = raw_books.find_one(_id=obj_id, state={"$ne": "deleted"}) |
Copilot
AI
Jul 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The comment 'also equivalent to Key version' is unclear and doesn't add value. Consider removing it or making it more descriptive.
| # also equivalent to Key version | |
| # book = raw_books.find_one(_id=obj_id, state={"$ne": "deleted"}) | |
| # Alternative query to fetch a non-deleted book by its ID | |
| # book = collection.find_one({"_id": obj_id, "state": {"$ne": "deleted"}}) |
| # look it up in MongoDB | ||
| book = collection.find_one(query) | ||
| # also equivalent to Key version | ||
| # book = raw_books.find_one(_id=obj_id, state={"$ne": "deleted"}) |
Copilot
AI
Jul 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This commented-out code should be removed as it serves no purpose and adds confusion. The variable name 'raw_books' doesn't match the actual collection variable name.
| # book = raw_books.find_one(_id=obj_id, state={"$ne": "deleted"}) |
Description
Trello ticket: https://trello.com/c/O9jraVyM
This change finalizes the transition of the GET /books/{id} endpoint. The endpoint now retrieves book details directly from the MongoDB database using a find_one() query, replacing the previous in-memory or mock implementation. It includes an updated, comprehensive test suite to achieve 100% test coverage for the new logic.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Automated tests, manual testing, cURL, CI/CD pipeline
Checklist: