Properly handle invalid or unreadable images in FaceDetector #677
Conversation
|
|
Walkthrough
Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
1 similar comment
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/app/models/FaceDetector.py (1)
2-3: Consider decoupling the model from FastAPI.Importing
HTTPExceptiondirectly in a model/service class tightly couples it to FastAPI. If this class is ever reused in a different context (CLI tool, different web framework, testing), the FastAPI dependency becomes problematic.Consider creating a custom exception (e.g.,
InvalidImageError) and raising that instead. Let the FastAPI route handler catch it and convert toHTTPException:# In a custom exceptions module class InvalidImageError(Exception): def __init__(self, image_path: str): self.image_path = image_path super().__init__(f"Invalid or unreadable image file: {image_path}")Then in the route handler:
try: result = face_detector.detect_faces(...) except InvalidImageError as e: raise HTTPException(status_code=400, detail=str(e))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
__pycache__/app.cpython-313.pycis excluded by!**/*.pycutils/__pycache__/cache.cpython-313.pycis excluded by!**/*.pyc
📒 Files selected for processing (1)
backend/app/models/FaceDetector.py(4 hunks)
🔇 Additional comments (5)
backend/app/models/FaceDetector.py (5)
47-47: Comment improvement is accurate.Good clarification - the code creates a Python dictionary, not JSON. The term "JSON format" was technically imprecise.
54-55: Formatting improvement aligns with common Python style.Removing spaces around slice colons is consistent with common Python conventions and improves readability.
75-75: LGTM! Improved docstring format.The concise docstring is clear and follows Python documentation conventions.
76-82: Resource cleanup implementation looks correct.The method properly checks for
Nonebefore closing and nullifies references afterward, which is good practice for preventing use-after-close errors.
32-35: Verify that all callers handle the new exception.Changing from returning
Noneto raisingHTTPExceptionis a breaking change in the method's error-handling contract. Ensure that all callers ofdetect_facesare prepared to handle this exception. Search the codebase for all usages of.detect_faces()and confirm that callers either have try-except blocks to catchHTTPExceptionor are designed to propagate the exception appropriately.
|
|
2 similar comments
|
|
|
|
|
Issue has now been linked to this PR for proper tracking. ✅ |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/tests/test_unit_face_detector.py (2)
34-47: Test correctly validates the invalid image error path.The test properly mocks
cv2.imreadto returnNoneand verifies thatHTTPException(400)is raised with the expected error message, aligning with the PR objectives.Consider adding
detector.close()in a tearDown method or try/finally block to match production cleanup patterns, even though the resources are mocked:@patch('cv2.imread') def test_detect_faces_invalid_image_raises_400(self, mock_imread): """ Ensure detect_faces raises HTTPException(400) when image reading fails. """ # Simulate cv2.imread failing (returning None) mock_imread.return_value = None detector = FaceDetector() - with self.assertRaises(HTTPException) as context: - detector.detect_faces("test_id", "invalid/path/image.jpg") + try: + with self.assertRaises(HTTPException) as context: + detector.detect_faces("test_id", "invalid/path/image.jpg") - self.assertEqual(context.exception.status_code, 400) - self.assertEqual(context.exception.detail, "Invalid or unreadable image file") + self.assertEqual(context.exception.status_code, 400) + self.assertEqual(context.exception.detail, "Invalid or unreadable image file") + finally: + detector.close()
49-65: Test correctly validates the valid image path with zero detections.The test properly mocks a valid image load and YOLO detection results, then verifies the method returns a non-null result with
num_faces=0, which exercises the success path.Consider adding
detector.close()after the assertions to match production cleanup patterns:@patch('cv2.imread') def test_detect_faces_valid_image(self, mock_imread): """ Ensure detect_faces processes valid images correctly without error. """ # Simulate valid image load mock_img = MagicMock() mock_imread.return_value = mock_img detector = FaceDetector() # Mock detection results: boxes, scores, class_ids detector.yolo_detector.return_value = ([], [], []) result = detector.detect_faces("test_id", "valid.jpg") self.assertIsNotNone(result) self.assertEqual(result["num_faces"], 0) + + detector.close()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/models/FaceDetector.py(2 hunks)backend/tests/test_unit_face_detector.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/tests/test_unit_face_detector.py (1)
backend/app/models/FaceDetector.py (2)
FaceDetector(15-82)detect_faces(26-70)
🔇 Additional comments (4)
backend/app/models/FaceDetector.py (2)
3-3: LGTM! HTTPException import is appropriate.The import is correctly placed and necessary for the error handling implemented in the
detect_facesmethod.
28-33: LGTM! Error handling correctly addresses the PR objectives.The implementation properly detects when
cv2.imread()returnsNoneand raises an appropriateHTTPException(400)with a generic error message. The full path is logged on line 29 for debugging while avoiding exposure in the API response, which addresses the security concern from the past review.backend/tests/test_unit_face_detector.py (2)
1-27: LGTM! Test setup is well-structured.The test setup appropriately mocks heavy dependencies before importing
FaceDetector, which is essential for fast, isolated unit tests. The conditional import with try/except provides defensive error handling.
29-31: LGTM! setUp method handles import failures gracefully.The conditional skip is consistent with the import pattern and ensures tests won't fail due to missing dependencies.
1 participant
This PR fixes an issue in the FaceDetector module where cv2.imread() would return None for invalid or unreadable image files.
Previously, the function returned None, which caused silent failures and unexpected behavior in the API and frontend.
What’s Changed
Added a FastAPI HTTPException with 400 Bad Request when image loading fails.
Improved error logging for easier debugging.
Ensures all API consumers receive a clear, structured error response instead of a None return.
Why This Fix Is Important
Prevents backend crashes when corrupted/invalid images are provided.
Ensures consistent API behavior.
Provides meaningful error responses to the frontend.
Enhances reliability of face detection and embedding workflows.
Example Error Response
{
"detail": "Invalid or unreadable image file: "
}
Testing
Tested with valid images → face detection works normally.
Tested with corrupted/non-existent images → API correctly returns 400 error.
No issue linked — general improvement PR.
Since no existing issue tracked this bug, this PR does not close any issue.
Fixes #709
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.