🐛 Ensure that dependencies can always catch validation errors #12508
Conversation
|
Any feedback would be highly appreciated |
|
A kind reminder 😉 |
|
@tiangolo could you please take a look? |
|
I'd like to have this bug fixed. Is there a way I can help you with this PR? |
|
Hi @anton-ryzhov, Thanks for your contribution! Definitely appreciate the detailed explanation and the various tests you've added to the PR. We're currently working through a backlog of PRs and will put this in the queue to review. We'll get back to you once someone in the team has been able to review this in more detail. Thanks for your patience! 🙏 |
There was a problem hiding this comment.
Simplified test that shows the idea of this PR:
from fastapi import Depends, FastAPI
from fastapi.exceptions import RequestValidationError
from fastapi.testclient import TestClient
app = FastAPI()
errors = []
def dep_with_yield():
try:
yield "_"
except RequestValidationError as e:
print("Caught RequestValidationError")
errors.append(e)
raise
@app.get("/")
def get_(param: bool, _: str = Depends(dep_with_yield)):
pass
client = TestClient(app)
def test_():
errors.clear()
client.get("/?param=invalid_value")
assert len(errors) == 1 # assert 0 == 1
assert isinstance(errors[0], RequestValidationError)For now we can't catch RequestValidationError using try..except block inside the dependency with yield. This PR makes it possible.
I don't think that in current implementation "code block was just accidentally deindented and was placed after async with-block not inside" (<= quote from the initial comment). For me it looks like it was designed this way intentionally.
At the same time I couldn't find any downsides of this new behavior except the fact that it may brake the code of existing projects if they catch exceptions and rewrite them (in some cases it may lead to hiding request validation errors). But it's easily fixable.
UPD: here is the commit that introduced the if not errors: block
Yeah, that's exactly the problem I'm trying to solve here.
That was just my assumption. I think this is not very important now.
This block was originally added here and And this commit has put it out for some reason. |
|
What's next? Are there any chances of this to be merged? |
|
@anton-ryzhov: we have put this on Sebastián's internal board to solicit feedback from him. Thanks for your patience 🙏 |
|
This pull request has a merge conflict that needs to be resolved. |
|
#14099 has fixed the original issue. Do you still want to merge new tests for that case? |
|
Thanks for the effort @anton-ryzhov! 🍰 If the original issue is now solved, then I think we can close this one. Thanks for reporting back that as well! ☕ |
|
I've re-tested it again and noticed an issue with the current implementation. If a handler returns success value, a dependency can't overwrite it with an Exception / HTTPException. This returns 200: from fastapi import Depends, FastAPI, HTTPException
async def dependency():
try:
yield
finally:
raise HTTPException(status_code=400, detail="Dependency raises")
app = FastAPI(dependencies=[Depends(dependency)])
@app.get("/")
async def root():
return Noneand logs that error: If a handler raises |
|
This breaks scenario when a dependency manages DB session and commits transaction after all. Currently an error on such commit will be ignored and false successful result will be returned |
|
Please check the new docs for dependencies with scope: https://fastapi.tiangolo.com/tutorial/dependencies/dependencies-with-yield/#early-exit-and-scope |
Hello everyone,
According to the documentation dependencies should be able to catch and handle (postprocess, ignore, overwrite, …) exceptions.
In fact that works for almost all cases, but not for pydantic validation errors for input parameters. It looks to me that the code block was just accidentally deindented and was placed after
async with-block not inside.First commit adds new tests where 2 of 6 are failing. The second fixes the issue.