-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[fastapi
] Handle parameters with Depends
correctly (FAST003
)
#15364
base: main
Are you sure you want to change the base?
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.
Nice. A few comments related to how the code is structured and the used terminology
crates/ruff_linter/src/rules/fastapi/rules/fastapi_unused_path_parameter.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/fastapi/rules/fastapi_unused_path_parameter.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/fastapi/rules/fastapi_unused_path_parameter.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/fastapi/rules/fastapi_unused_path_parameter.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/fastapi/rules/fastapi_unused_path_parameter.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/fastapi/rules/fastapi_unused_path_parameter.rs
Outdated
Show resolved
Hide resolved
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 is great. One small nit and an understand question from my end.
crates/ruff_linter/src/rules/fastapi/rules/fastapi_unused_path_parameter.rs
Show resolved
Hide resolved
@app.get("/things/{thing_id}") | ||
async def single(other: Annotated[str, Depends(not_so_dependable)]): ... | ||
@app.get("/things/{thing_id}") | ||
async def double(other: Annotated[str, Depends(not_so_dependable), Depends(dependable)]): ... |
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.
Just for my understanding. Is this an error because fastapi only respects the first Depends
? Do you have a reference that documents this behavior or did you find it out by try and error?
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.
The documentation doesn't mention what happens when two Depends
are used in a path operation function, only when multiple of them are used in a path operation decorator.
Testing this again, I think I made a mistake. FastAPI seems to respect the last rather than the first. The code in question:
from fastapi import FastAPI, Depends
from typing import Annotated
app = FastAPI()
def foo(a: str) -> str:
print(f'foo: {a}')
return 'lorem'
def bar(b: str) -> int:
print(f'bar: {b}')
return 42
def baz(c: str) -> bytes:
print(f'baz: {c}')
return b''
@app.get("/things/{c}") # `a`/`b`
async def read_thing(other: Annotated[str, Depends(baz), Depends(bar), Depends(foo)]):
return other
$ uv run fastapi run hello.py
...
INFO 1.2.3.4:5 - "GET /things/ipsum HTTP/1.1" 422
{
"detail": [
{
"type": "missing",
"loc": ["query", "a"],
"msg": "Field required",
"input": null
}
]
}
It would be best if we could ask someone with FastAPI expertise to have a look.
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.
Thanks for the detailed investigation.
@charliermarsh do you know someone we could ping on this?
I'd otherwise suggest to bail (ignore the rule) if multiple Depends
are seen
Summary
Resolves #13657.
Test Plan
cargo nextest run
andcargo insta test
.