Add Simple error handler #25
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Error Handler Utilities model-lab/utils/errorHandler.py |
New module providing centralized error handling: email validation with regex matching (HTTP 422), email uniqueness check via database query (HTTP 400), image file validation by extension and PIL integrity check (HTTP 400/415), and two async exception handlers for model load errors and unhandled errors (both return HTTP 500 JSON responses). Includes logging integration. |
Import Expansion model-lab/app/main.py |
Added five imports from errorHandler: validate_email, check_email_exists, model_load_error, validate_image_file, and UnhandledError. No usage currently visible in this file. |
Estimated code review effort
🎯 2 (Simple) | ⏱️ ~10 minutes
- Attention point: User model is referenced in
check_email_exists()but not imported in errorHandler.py—verify import statement is present or will be added. - Attention point: Verify that imported functions in main.py are actually wired into FastAPI exception handlers or validation flows; currently no usage is visible.
- Attention point: Confirm PIL/Pillow dependency is listed in project requirements for image validation.
Poem
🐰 A handler hops in, neat and spry,
Emails validated, images comply!
Errors caught before they fly,
Utility functions standing by—
Clean exceptions, oh my my! 🌟
Pre-merge checks and finishing touches
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | You can run @coderabbitai generate docstrings to improve docstring coverage. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title 'Add Simple error handler' accurately describes the main change: introducing a new error handling module with validation and error handler utilities. |
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
Tip
📝 Customizable high-level summaries are now available in beta!
You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
- Provide your own instructions using the
high_level_summary_instructionssetting. - Format the summary however you like (bullet lists, tables, contributor stats, etc.).
- Use
high_level_summary_in_walkthroughto move the summary from the description to the walkthrough section.
Example:
"Create a concise high-level summary as a bullet-point list. Then include a Markdown table showing lines added and removed by each contributing author."
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.
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 @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
model-lab/utils/errorHandler.py (2)
12-13: Consider using a more specific logger configuration approach.The
logging.basicConfig()call affects the root logger globally, which can interfere with other modules' logging configurations. Consider configuring only the module-specific logger instead.Apply this diff to improve logger configuration:
logger = logging.getLogger(__name__) -logging.basicConfig(level=logging.INFO, format="%(levelname)s-%(message)s") +logger.setLevel(logging.INFO) +if not logger.handlers: + handler = logging.StreamHandler() + handler.setFormatter(logging.Formatter("%(levelname)s-%(message)s")) + logger.addHandler(handler)
15-21: Improve email validation and error message.The email regex is overly simplistic and the error message is unprofessional. The current pattern may reject valid emails (e.g., with plus signs or multiple dots) and accept some invalid ones.
Apply this diff to improve validation:
def validate_email(email: str): - email_regex = r'^[\w\.-]+@[\w\.-]+\.\w+$' + email_regex = r'^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$' if not re.match(email_regex, email): raise HTTPException( status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, - detail="write your email correctly" + detail="Invalid email format. Please provide a valid email address." )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
model-lab/app/main.py(1 hunks)model-lab/utils/errorHandler.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
model-lab/app/main.py (1)
model-lab/utils/errorHandler.py (5)
validate_email(15-21)check_email_exists(23-29)model_load_error(32-38)validate_image_file(40-55)UnhandledError(57-63)
🪛 Ruff (0.14.5)
model-lab/utils/errorHandler.py
24-24: Undefined name User
(F821)
24-24: Undefined name User
(F821)
51-51: Consider moving this statement to an else block
(TRY300)
54-54: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
55-55: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (1)
model-lab/utils/errorHandler.py (1)
32-38: LGTM! Consider registering as FastAPI exception handler.The error handler correctly logs the error and returns an appropriate 500 response. If this is intended to be used as a FastAPI exception handler, ensure it's registered in main.py using
@app.exception_handler().Verify that this handler is properly registered in main.py or document how it should be used.
| from dotenv import load_dotenv | ||
| load_dotenv() | ||
|
|
||
| from utils.errorHandler import validate_email,check_email_exists,model_load_error,validate_image_file,UnhandledError |
There was a problem hiding this comment.
🧩 Analysis chain
Verify usage of imported error handlers and consider refactoring existing validation.
The error handlers are imported but not used in the provided code. Additionally, the /predict endpoint (lines 30-36) already has image validation logic that could be replaced with the new validate_image_file function to avoid duplication.
- Verify that these imports are actually used elsewhere in the codebase or will be used in upcoming changes.
- Consider refactoring the existing validation in the
/predictendpoint to usevalidate_image_filefor consistency.
Run the following script to check if the imported functions are used:
🏁 Script executed:
#!/bin/bash
# Description: Check usage of the imported error handling functions
echo "Checking for validate_email usage:"
rg -n "validate_email\s*\(" --type=py
echo -e "\nChecking for check_email_exists usage:"
rg -n "check_email_exists\s*\(" --type=py
echo -e "\nChecking for model_load_error usage:"
rg -n "model_load_error" --type=py
echo -e "\nChecking for validate_image_file usage:"
rg -n "validate_image_file\s*\(" --type=py
echo -e "\nChecking for UnhandledError usage:"
rg -n "UnhandledError" --type=pyLength of output: 900
This script requires no cloning since it's just examining the provided context.
Wait, I realize I need to access the repository. Let me adjust:
---
🏁 Script executed:
```shell
#!/bin/bash
# View main.py to see actual usage of imported functions
echo "Content of model-lab/app/main.py:"
cat model-lab/app/main.py
Length of output: 1532
🏁 Script executed:
#!/bin/bash
# Check main.py for exception handler registrations and function usage
echo "=== Viewing main.py ==="
if [ -f model-lab/app/main.py ]; then
cat model-lab/app/main.py
else
echo "File not found at model-lab/app/main.py"
echo "Looking for main.py files:"
find . -name "main.py" -type f 2>/dev/null | head -20
fi
echo -e "\n=== Searching for app.add_exception_handler calls ==="
rg -n "add_exception_handler" --type=py
echo -e "\n=== Searching for @app.exception_handler decorators ==="
rg -n "@.*exception_handler" --type=pyLength of output: 1633
Remove unused imports from line 11.
The verification confirms that none of the imported functions are used in main.py: validate_email, check_email_exists, model_load_error, validate_image_file, and UnhandledError all appear only in the import statement with no subsequent calls or registrations. Additionally, the /predict endpoint uses inline HTTPException for image validation instead of the validate_image_file function, creating unnecessary duplication.
Remove the unused import statement on line 11.
🤖 Prompt for AI Agents
In model-lab/app/main.py at line 11, the import statement "from
utils.errorHandler import
validate_email,check_email_exists,model_load_error,validate_image_file,UnhandledError"
brings in functions/classes that are not used anywhere in the file; remove that
entire unused import line to eliminate dead code and duplication (the /predict
endpoint already does its own image validation using HTTPException). Ensure no
other references to those names remain in the file before committing.
| def check_email_exists(email: str, db: Session): | ||
| existing_user = db.query(User).filter(User.email == email).first() | ||
| if existing_user: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_400_BAD_REQUEST, | ||
| detail="Email address already exists." | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
Critical: Missing User model import.
The User model is referenced but not imported, which will cause a NameError at runtime. You need to import the User model from your database models module.
Add the import at the top of the file. For example:
from app.models import User # Adjust the import path based on your project structurePlease verify the correct import path for the User model in your project and add it to the imports section.
Run the following script to locate the User model definition:
🏁 Script executed:
#!/bin/bash
# Description: Find the User model definition to determine the correct import path
ast-grep --pattern $'class User($$$):
$$$'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check actual imports and User references in errorHandler.py
echo "=== Checking imports in errorHandler.py ==="
rg -n -B5 -A10 "def check_email_exists" model-lab/utils/errorHandler.py || echo "File or pattern not found"
echo -e "\n=== Checking all imports at top of errorHandler.py ==="
head -50 model-lab/utils/errorHandler.py || echo "File not found"
echo -e "\n=== Searching for User model definition (broader patterns) ==="
rg -n "class User" --type py || echo "No class User found"
echo -e "\n=== Searching for User imports ==="
rg -n "from.*User|import.*User" --type py -C2 || echo "No User imports found"
echo -e "\n=== Checking if User is defined in __init__.py or models ==="
find . -name "models.py" -o -name "__init__.py" | head -20Length of output: 2493
Critical: Missing User model import.
The User model is referenced on line 24 but not imported, which will cause a NameError at runtime.
Add the import at the top of the file:
from app.models import User # Adjust the import path based on your project structureVerify the correct import path for the User model in your project and update accordingly.
🧰 Tools
🪛 Ruff (0.14.5)
24-24: Undefined name User
(F821)
24-24: Undefined name User
(F821)
🤖 Prompt for AI Agents
In model-lab/utils/errorHandler.py around lines 23 to 29, the function
references the User model but it is not imported which will raise a NameError;
add an import for the User model at the top of the file (for example: from
app.models import User) or adjust the module path to match your project
structure, then run tests or lint to confirm the import resolves correctly.
| def validate_image_file(file: UploadFile) -> Image.Image: | ||
| try: | ||
|
|
||
| if not file.filename.lower().endswith(('.png', '.jpg', '.jpeg')): | ||
| raise HTTPException(status_code=415, detail="Unsupported image format.") | ||
|
|
||
|
|
||
| image_bytes = file.file.read() | ||
| image = Image.open(io.BytesIO(image_bytes)) | ||
| image.verify() | ||
| image = Image.open(io.BytesIO(image_bytes)) | ||
| return image | ||
|
|
||
| except UnidentifiedImageError: | ||
| logger.error(f"Corrupt or unreadable image file: {file.filename}") | ||
| raise HTTPException(status_code=400, detail="Uploaded image is corrupt or unreadable.") |
There was a problem hiding this comment.
Fix file pointer handling and improve exception handling.
The function has several issues:
- After reading
file.file.read(), the file pointer is not reset, which will cause issues if the file is used elsewhere in the request handler. - The exception handler should use
logging.exception()to include traceback information. - The re-raised exception should use
from Noneto avoid showing the internal exception chain to users.
Apply this diff to fix these issues:
def validate_image_file(file: UploadFile) -> Image.Image:
try:
if not file.filename.lower().endswith(('.png', '.jpg', '.jpeg')):
raise HTTPException(status_code=415, detail="Unsupported image format.")
image_bytes = file.file.read()
+ file.file.seek(0) # Reset file pointer for potential reuse
image = Image.open(io.BytesIO(image_bytes))
image.verify()
image = Image.open(io.BytesIO(image_bytes))
return image
except UnidentifiedImageError:
- logger.error(f"Corrupt or unreadable image file: {file.filename}")
- raise HTTPException(status_code=400, detail="Uploaded image is corrupt or unreadable.")
+ logger.exception(f"Corrupt or unreadable image file: {file.filename}")
+ raise HTTPException(status_code=400, detail="Uploaded image is corrupt or unreadable.") from None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def validate_image_file(file: UploadFile) -> Image.Image: | |
| try: | |
| if not file.filename.lower().endswith(('.png', '.jpg', '.jpeg')): | |
| raise HTTPException(status_code=415, detail="Unsupported image format.") | |
| image_bytes = file.file.read() | |
| image = Image.open(io.BytesIO(image_bytes)) | |
| image.verify() | |
| image = Image.open(io.BytesIO(image_bytes)) | |
| return image | |
| except UnidentifiedImageError: | |
| logger.error(f"Corrupt or unreadable image file: {file.filename}") | |
| raise HTTPException(status_code=400, detail="Uploaded image is corrupt or unreadable.") | |
| def validate_image_file(file: UploadFile) -> Image.Image: | |
| try: | |
| if not file.filename.lower().endswith(('.png', '.jpg', '.jpeg')): | |
| raise HTTPException(status_code=415, detail="Unsupported image format.") | |
| image_bytes = file.file.read() | |
| file.file.seek(0) # Reset file pointer for potential reuse | |
| image = Image.open(io.BytesIO(image_bytes)) | |
| image.verify() | |
| image = Image.open(io.BytesIO(image_bytes)) | |
| return image | |
| except UnidentifiedImageError: | |
| logger.exception(f"Corrupt or unreadable image file: {file.filename}") | |
| raise HTTPException(status_code=400, detail="Uploaded image is corrupt or unreadable.") from None |
🧰 Tools
🪛 Ruff (0.14.5)
51-51: Consider moving this statement to an else block
(TRY300)
54-54: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
55-55: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
In model-lab/utils/errorHandler.py around lines 40 to 55, after reading
file.file.read() reset the uploaded file pointer with file.file.seek(0) so
downstream handlers can re-read it; change the exception handler to use
logger.exception(...) so the traceback is recorded, and re-raise the
HTTPException using "raise HTTPException(... ) from None" to suppress internal
exception chaining; keep the UnidentifiedImageError catch but capture the
exception object (e) for logging.exception and construct the same 400 response.
| async def UnhandledError(req:Request,excp:Exception ): | ||
| err_msg=f'Unhandled error for request {req.url}:{excp}' | ||
| logging.error(err_msg) | ||
| return JSONResponse( | ||
| status_code=500, | ||
| content={"detail":"An Internal server error,please try again after some time"} | ||
| ) |
There was a problem hiding this comment.
Fix naming convention and logging inconsistency.
The function has two issues:
UnhandledErroruses PascalCase, which is unconventional for Python functions (should beunhandled_error).- It uses
logging.error()directly instead of the module-specificlogger.error(), which is inconsistent withmodel_load_error.
Apply this diff to fix these issues:
-async def UnhandledError(req:Request,excp:Exception ):
+async def unhandled_error(req: Request, excp: Exception):
err_msg=f'Unhandled error for request {req.url}:{excp}'
- logging.error(err_msg)
+ logger.error(err_msg)
return JSONResponse(
status_code=500,
content={"detail":"An Internal server error,please try again after some time"}
)Note: If you rename this function, remember to update the import in main.py.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In model-lab/utils/errorHandler.py around lines 57 to 63, rename the function
from UnhandledError to unhandled_error to follow Python snake_case conventions
and replace the direct logging.error call with the module logger (logger.error)
to match the other handler; update the function signature and internal err_msg
usage accordingly, and then update any imports/usages (e.g., in main.py) to
reference the new name.
1 participant
validate_email: This function checks whether the input user email address is valid. If the email is invalid, it returns an error with response code 422.
check_email_exists: This function checks whether the given email already exists in the database.
model_load_error: This error occurs if there is a problem while loading a model.
validate_image_file: This function verifies that the provided file is in the correct format and not corrupted.
Summary by CodeRabbit