Wrap input validation error in RFC9457 response#311
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 45 minutes and 41 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughA new FastAPI exception handler 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
for more information, see https://pre-commit.ci
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
validation_exception_handler,HTTPStatus.UNPROCESSABLE_ENTITYis included directly in the JSONcontentand may not be JSON-serializable as an enum; consider usingHTTPStatus.UNPROCESSABLE_ENTITY.value(or422) for thestatusfield. - The new
validation_exception_handlerlargely duplicates the structure ofproblem_detail_exception_handler; consider extracting a small helper to build RFC 9457 problem details so both handlers stay consistent and easier to evolve.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `validation_exception_handler`, `HTTPStatus.UNPROCESSABLE_ENTITY` is included directly in the JSON `content` and may not be JSON-serializable as an enum; consider using `HTTPStatus.UNPROCESSABLE_ENTITY.value` (or `422`) for the `status` field.
- The new `validation_exception_handler` largely duplicates the structure of `problem_detail_exception_handler`; consider extracting a small helper to build RFC 9457 problem details so both handlers stay consistent and easier to evolve.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #311 +/- ##
==========================================
+ Coverage 93.57% 93.58% +0.01%
==========================================
Files 72 72
Lines 3112 3117 +5
Branches 220 220
==========================================
+ Hits 2912 2917 +5
Misses 143 143
Partials 57 57 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/errors.py`:
- Around line 93-111: The validation_exception_handler currently returns
exc.errors() directly which can contain non-JSON-serializable values; wrap the
errors with FastAPI's jsonable_encoder before including them in the JSONResponse
(i.e., replace "errors": exc.errors() with "errors":
jsonable_encoder(exc.errors())), and add the import for jsonable_encoder from
fastapi.encoders if not already present so the response can't raise during
rendering.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 349770bf-697c-4bd7-9e6d-6d84e0956ac5
📒 Files selected for processing (4)
src/core/errors.pysrc/main.pytests/routers/openml/dataset_tag_test.pytests/routers/openml/task_list_test.py
| def validation_exception_handler( | ||
| request: Request, # noqa: ARG001 | ||
| exc: RequestValidationError, | ||
| ) -> JSONResponse: | ||
| """FastAPI exception handler for RequestValidationError. | ||
|
|
||
| Returns a RFC 9457 compliant response for input validation failures. | ||
| """ | ||
| return JSONResponse( | ||
| status_code=HTTPStatus.UNPROCESSABLE_ENTITY, | ||
| content={ | ||
| "type": "about:blank", | ||
| "title": "Validation Error", | ||
| "status": HTTPStatus.UNPROCESSABLE_ENTITY, | ||
| "detail": "Input validation failed.", | ||
| "errors": exc.errors(), | ||
| }, | ||
| media_type="application/problem+json", | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In FastAPI’s built-in RequestValidationError handler, is exc.errors()wrapped withjsonable_encoderbefore sendingJSONResponse, and can exc.errors()include non-JSON-serializable values inctx?
💡 Result:
Yes, in FastAPI's built-in RequestValidationError handler, exc.errors is wrapped with jsonable_encoder before being sent in the JSONResponse. The handler code explicitly does: content={"detail": jsonable_encoder(exc.errors)}. Yes, exc.errors can include non-JSON-serializable values in the ctx field, particularly the input value from Pydantic v2 validation errors, which may contain complex objects like bytes or other non-serializable types. This can cause jsonable_encoder to fail in some cases, as reported in user discussions and issues.
Citations:
- 1: https://github.com/tiangolo/fastapi/blob/master/fastapi/exception_handlers.py
- 2: https://fastapi.tiangolo.com/tutorial/handling-errors/
- 3: RequestValidationError can't json serialize some inputs fastapi/fastapi#11923
🏁 Script executed:
# First, check if HTTPStatus is JSON-serializable in content dict
python <<EOF
import json
from http import HTTPStatus
# Test if HTTPStatus enum is directly JSON-serializable
try:
json.dumps({"status": HTTPStatus.UNPROCESSABLE_ENTITY})
print("HTTPStatus IS JSON-serializable")
except TypeError as e:
print(f"HTTPStatus is NOT JSON-serializable: {e}")
# Test if int(HTTPStatus) is JSON-serializable
try:
json.dumps({"status": int(HTTPStatus.UNPROCESSABLE_ENTITY)})
print("int(HTTPStatus) IS JSON-serializable")
except TypeError as e:
print(f"int(HTTPStatus) is NOT JSON-serializable: {e}")
EOFRepository: openml/server-api
Length of output: 129
🏁 Script executed:
# Check the actual code in src/core/errors.py
if [ -f "src/core/errors.py" ]; then
echo "=== Checking imports at top of file ==="
head -20 src/core/errors.py
echo ""
echo "=== Checking lines 93-111 ==="
sed -n '93,111p' src/core/errors.py
else
echo "File not found, searching for errors.py"
find . -name "errors.py" -type f 2>/dev/null
fiRepository: openml/server-api
Length of output: 1387
🏁 Script executed:
# Also check what version of FastAPI and what the current import pattern is
rg "from fastapi" src/core/errors.py -A 2 -B 2Repository: openml/server-api
Length of output: 295
Wrap exc.errors() with jsonable_encoder before returning it.
At line 108, exc.errors() is passed directly. Validation errors can include non-JSON-serializable values in ctx, which may raise during response rendering and convert the 422 into a 500. FastAPI's built-in handler wraps this with jsonable_encoder for this reason.
Proposed fix
from fastapi import Request
+from fastapi.encoders import jsonable_encoder
from fastapi.exceptions import RequestValidationError
from fastapi.responses import JSONResponse
@@
return JSONResponse(
status_code=HTTPStatus.UNPROCESSABLE_ENTITY,
content={
"type": "about:blank",
"title": "Validation Error",
"status": HTTPStatus.UNPROCESSABLE_ENTITY,
"detail": "Input validation failed.",
- "errors": exc.errors(),
+ "errors": jsonable_encoder(exc.errors()),
},
media_type="application/problem+json",
)📝 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 validation_exception_handler( | |
| request: Request, # noqa: ARG001 | |
| exc: RequestValidationError, | |
| ) -> JSONResponse: | |
| """FastAPI exception handler for RequestValidationError. | |
| Returns a RFC 9457 compliant response for input validation failures. | |
| """ | |
| return JSONResponse( | |
| status_code=HTTPStatus.UNPROCESSABLE_ENTITY, | |
| content={ | |
| "type": "about:blank", | |
| "title": "Validation Error", | |
| "status": HTTPStatus.UNPROCESSABLE_ENTITY, | |
| "detail": "Input validation failed.", | |
| "errors": exc.errors(), | |
| }, | |
| media_type="application/problem+json", | |
| ) | |
| def validation_exception_handler( | |
| request: Request, # noqa: ARG001 | |
| exc: RequestValidationError, | |
| ) -> JSONResponse: | |
| """FastAPI exception handler for RequestValidationError. | |
| Returns a RFC 9457 compliant response for input validation failures. | |
| """ | |
| return JSONResponse( | |
| status_code=HTTPStatus.UNPROCESSABLE_ENTITY, | |
| content={ | |
| "type": "about:blank", | |
| "title": "Validation Error", | |
| "status": HTTPStatus.UNPROCESSABLE_ENTITY, | |
| "detail": "Input validation failed.", | |
| "errors": jsonable_encoder(exc.errors()), | |
| }, | |
| media_type="application/problem+json", | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/errors.py` around lines 93 - 111, The validation_exception_handler
currently returns exc.errors() directly which can contain non-JSON-serializable
values; wrap the errors with FastAPI's jsonable_encoder before including them in
the JSONResponse (i.e., replace "errors": exc.errors() with "errors":
jsonable_encoder(exc.errors())), and add the import for jsonable_encoder from
fastapi.encoders if not already present so the response can't raise during
rendering.
1 participant
No description provided.