[fastapi] Implement fast-api-unused-path-parameter (FAST003) (#12638)

This adds the `fast-api-unused-path-parameter` lint rule, as described
in #12632.

I'm still pretty new to rust, so the code can probably be improved, feel
free to tell me if there's any changes i should make.

Also, i needed to add the `add_parameter` edit function, not sure if it
was in the scope of the PR or if i should've made another one.
This commit is contained in:
Matthieu LAURENT 2024-08-16 03:46:35 +02:00 committed by GitHub
parent 80efb865e9
commit f121f8b31b
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
9 changed files with 751 additions and 1 deletions

View file

@ -0,0 +1,134 @@
from fastapi import FastAPI
app = FastAPI()
# Errors
@app.get("/things/{thing_id}")
async def read_thing(query: str):
return {"query": query}
@app.get("/books/isbn-{isbn}")
async def read_thing():
...
@app.get("/things/{thing_id:path}")
async def read_thing(query: str):
return {"query": query}
@app.get("/things/{thing_id : path}")
async def read_thing(query: str):
return {"query": query}
@app.get("/books/{author}/{title}")
async def read_thing(author: str):
return {"author": author}
@app.get("/books/{author_name}/{title}")
async def read_thing():
...
@app.get("/books/{author}/{title}")
async def read_thing(author: str, title: str, /):
return {"author": author, "title": title}
@app.get("/books/{author}/{title}/{page}")
async def read_thing(
author: str,
query: str,
): ...
@app.get("/books/{author}/{title}")
async def read_thing():
...
@app.get("/books/{author}/{title}")
async def read_thing(*, author: str):
...
@app.get("/books/{author}/{title}")
async def read_thing(hello, /, *, author: str):
...
@app.get("/things/{thing_id}")
async def read_thing(
query: str,
):
return {"query": query}
@app.get("/things/{thing_id}")
async def read_thing(
query: str = "default",
):
return {"query": query}
@app.get("/things/{thing_id}")
async def read_thing(
*, query: str = "default",
):
return {"query": query}
# OK
@app.get("/things/{thing_id}")
async def read_thing(thing_id: int, query: str):
return {"thing_id": thing_id, "query": query}
@app.get("/books/isbn-{isbn}")
async def read_thing(isbn: str):
return {"isbn": isbn}
@app.get("/things/{thing_id:path}")
async def read_thing(thing_id: str, query: str):
return {"thing_id": thing_id, "query": query}
@app.get("/things/{thing_id : path}")
async def read_thing(thing_id: str, query: str):
return {"thing_id": thing_id, "query": query}
@app.get("/books/{author}/{title}")
async def read_thing(author: str, title: str):
return {"author": author, "title": title}
@app.get("/books/{author}/{title}")
async def read_thing(*, author: str, title: str):
return {"author": author, "title": title}
@app.get("/books/{author}/{title:path}")
async def read_thing(*, author: str, title: str):
return {"author": author, "title": title}
# Ignored
@app.get("/things/{thing-id}")
async def read_thing(query: str):
return {"query": query}
@app.get("/things/{thing_id!r}")
async def read_thing(query: str):
return {"query": query}
@app.get("/things/{thing_id=}")
async def read_thing(query: str):
return {"query": query}

View file

@ -94,6 +94,9 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
if checker.enabled(Rule::FastApiNonAnnotatedDependency) {
fastapi::rules::fastapi_non_annotated_dependency(checker, function_def);
}
if checker.enabled(Rule::FastApiUnusedPathParameter) {
fastapi::rules::fastapi_unused_path_parameter(checker, function_def);
}
if checker.enabled(Rule::AmbiguousFunctionName) {
if let Some(diagnostic) = pycodestyle::rules::ambiguous_function_name(name) {
checker.diagnostics.push(diagnostic);

View file

@ -920,6 +920,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
// fastapi
(FastApi, "001") => (RuleGroup::Preview, rules::fastapi::rules::FastApiRedundantResponseModel),
(FastApi, "002") => (RuleGroup::Preview, rules::fastapi::rules::FastApiNonAnnotatedDependency),
(FastApi, "003") => (RuleGroup::Preview, rules::fastapi::rules::FastApiUnusedPathParameter),
// pydoclint
(Pydoclint, "201") => (RuleGroup::Preview, rules::pydoclint::rules::DocstringMissingReturns),

View file

@ -4,7 +4,7 @@ use anyhow::{Context, Result};
use ruff_diagnostics::Edit;
use ruff_python_ast::parenthesize::parenthesized_range;
use ruff_python_ast::{self as ast, Arguments, ExceptHandler, Expr, ExprList, Stmt};
use ruff_python_ast::{self as ast, Arguments, ExceptHandler, Expr, ExprList, Parameters, Stmt};
use ruff_python_ast::{AnyNodeRef, ArgOrKeyword};
use ruff_python_codegen::Stylist;
use ruff_python_index::Indexer;
@ -282,6 +282,59 @@ pub(crate) fn add_argument(
}
}
/// Generic function to add a (regular) parameter to a function definition.
pub(crate) fn add_parameter(parameter: &str, parameters: &Parameters, source: &str) -> Edit {
if let Some(last) = parameters
.args
.iter()
.filter(|arg| arg.default.is_none())
.last()
{
// Case 1: at least one regular parameter, so append after the last one.
Edit::insertion(format!(", {parameter}"), last.end())
} else if parameters.args.first().is_some() {
// Case 2: no regular parameters, but at least one keyword parameter, so add before the
// first.
let pos = parameters.start();
let mut tokenizer = SimpleTokenizer::starts_at(pos, source);
let name = tokenizer
.find(|token| token.kind == SimpleTokenKind::Name)
.expect("Unable to find name token");
Edit::insertion(format!("{parameter}, "), name.start())
} else if let Some(last) = parameters.posonlyargs.last() {
// Case 2: no regular parameter, but a positional-only parameter exists, so add after that.
// We take care to add it *after* the `/` separator.
let pos = last.end();
let mut tokenizer = SimpleTokenizer::starts_at(pos, source);
let slash = tokenizer
.find(|token| token.kind == SimpleTokenKind::Slash)
.expect("Unable to find `/` token");
// Try to find a comma after the slash.
let comma = tokenizer.find(|token| token.kind == SimpleTokenKind::Comma);
if let Some(comma) = comma {
Edit::insertion(format!(" {parameter},"), comma.start() + TextSize::from(1))
} else {
Edit::insertion(format!(", {parameter}"), slash.start())
}
} else if parameters.kwonlyargs.first().is_some() {
// Case 3: no regular parameter, but a keyword-only parameter exist, so add parameter before that.
// We need to backtrack to before the `*` separator.
// We know there is no non-keyword-only params, so we can safely assume that the `*` separator is the first
let pos = parameters.start();
let mut tokenizer = SimpleTokenizer::starts_at(pos, source);
let star = tokenizer
.find(|token| token.kind == SimpleTokenKind::Star)
.expect("Unable to find `*` token");
Edit::insertion(format!("{parameter}, "), star.start())
} else {
// Case 4: no parameters at all, so add parameter after the opening parenthesis.
Edit::insertion(
parameter.to_string(),
parameters.start() + TextSize::from(1),
)
}
}
/// Safely adjust the indentation of the indented block at [`TextRange`].
///
/// The [`TextRange`] is assumed to represent an entire indented block, including the leading

View file

@ -15,6 +15,7 @@ mod tests {
#[test_case(Rule::FastApiRedundantResponseModel, Path::new("FAST001.py"))]
#[test_case(Rule::FastApiNonAnnotatedDependency, Path::new("FAST002.py"))]
#[test_case(Rule::FastApiUnusedPathParameter, Path::new("FAST003.py"))]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.as_ref(), path.to_string_lossy());
let diagnostics = test_path(

View file

@ -0,0 +1,232 @@
use std::iter::Peekable;
use std::ops::Range;
use std::str::CharIndices;
use ruff_diagnostics::Fix;
use ruff_diagnostics::{Diagnostic, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast as ast;
use ruff_python_semantic::Modules;
use ruff_python_stdlib::identifiers::is_identifier;
use ruff_text_size::{Ranged, TextSize};
use crate::checkers::ast::Checker;
use crate::fix::edits::add_parameter;
use crate::rules::fastapi::rules::is_fastapi_route_decorator;
/// ## What it does
/// Identifies FastAPI routes that declare path parameters in the route path
/// that are not included in the function signature.
///
/// ## Why is this bad?
/// Path parameters are used to extract values from the URL path.
///
/// If a path parameter is declared in the route path but not in the function
/// signature, it will not be accessible in the function body, which is likely
/// a mistake.
///
/// If a path parameter is declared in the route path, but as a positional-only
/// argument in the function signature, it will also not be accessible in the
/// function body, as FastAPI will not inject the parameter.
///
/// ## Known problems
/// If the path parameter is _not_ a valid Python identifier (e.g., `user-id`, as
/// opposed to `user_id`), FastAPI will normalize it. However, this rule simply
/// ignores such path parameters, as FastAPI's normalization behavior is undocumented.
///
/// ## Example
///
/// ```python
/// from fastapi import FastAPI
///
/// app = FastAPI()
///
///
/// @app.get("/things/{thing_id}")
/// async def read_thing(query: str): ...
/// ```
///
/// Use instead:
///
/// ```python
/// from fastapi import FastAPI
///
/// app = FastAPI()
///
///
/// @app.get("/things/{thing_id}")
/// async def read_thing(thing_id: int, query: str): ...
/// ```
///
/// ## Fix safety
/// This rule's fix is marked as unsafe, as modifying a function signature can
/// change the behavior of the code.
#[violation]
pub struct FastApiUnusedPathParameter {
arg_name: String,
function_name: String,
is_positional: bool,
}
impl Violation for FastApiUnusedPathParameter {
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;
#[derive_message_formats]
fn message(&self) -> String {
let Self {
arg_name,
function_name,
is_positional,
} = self;
#[allow(clippy::if_not_else)]
if !is_positional {
format!("Parameter `{arg_name}` appears in route path, but not in `{function_name}` signature")
} else {
format!(
"Parameter `{arg_name}` appears in route path, but only as a positional-only argument in `{function_name}` signature"
)
}
}
fn fix_title(&self) -> Option<String> {
let Self {
arg_name,
is_positional,
..
} = self;
if *is_positional {
None
} else {
Some(format!("Add `{arg_name}` to function signature"))
}
}
}
/// FAST003
pub(crate) fn fastapi_unused_path_parameter(
checker: &mut Checker,
function_def: &ast::StmtFunctionDef,
) {
if !checker.semantic().seen_module(Modules::FASTAPI) {
return;
}
// Get the route path from the decorator.
let route_decorator = function_def
.decorator_list
.iter()
.find_map(|decorator| is_fastapi_route_decorator(decorator, checker.semantic()));
let Some(route_decorator) = route_decorator else {
return;
};
let Some(path_arg) = route_decorator.arguments.args.first() else {
return;
};
let diagnostic_range = path_arg.range();
// We can't really handle anything other than string literals.
let path = match path_arg.as_string_literal_expr() {
Some(path_arg) => &path_arg.value,
None => return,
};
// Extract the path parameters from the route path.
let path_params = PathParamIterator::new(path.to_str());
// Extract the arguments from the function signature
let named_args: Vec<_> = function_def
.parameters
.args
.iter()
.chain(function_def.parameters.kwonlyargs.iter())
.map(|arg| arg.parameter.name.as_str())
.collect();
// Check if any of the path parameters are not in the function signature.
let mut diagnostics = vec![];
for (path_param, range) in path_params {
// Ignore invalid identifiers (e.g., `user-id`, as opposed to `user_id`)
if !is_identifier(path_param) {
continue;
}
// If the path parameter is already in the function signature, we don't need to do anything.
if named_args.contains(&path_param) {
continue;
}
// Determine whether the path parameter is used as a positional-only argument. In this case,
// the path parameter injection won't work, but we also can't fix it (yet), since we'd need
// to make the parameter non-positional-only.
let is_positional = function_def
.parameters
.posonlyargs
.iter()
.any(|arg| arg.parameter.name.as_str() == path_param);
let mut diagnostic = Diagnostic::new(
FastApiUnusedPathParameter {
arg_name: path_param.to_string(),
function_name: function_def.name.to_string(),
is_positional,
},
#[allow(clippy::cast_possible_truncation)]
diagnostic_range
.add_start(TextSize::from(range.start as u32 + 1))
.sub_end(TextSize::from((path.len() - range.end + 1) as u32)),
);
if !is_positional {
diagnostic.set_fix(Fix::unsafe_edit(add_parameter(
path_param,
&function_def.parameters,
checker.locator().contents(),
)));
}
diagnostics.push(diagnostic);
}
checker.diagnostics.extend(diagnostics);
}
/// An iterator to extract parameters from FastAPI route paths.
///
/// The iterator yields tuples of the parameter name and the range of the parameter in the input,
/// inclusive of curly braces.
#[derive(Debug)]
struct PathParamIterator<'a> {
input: &'a str,
chars: Peekable<CharIndices<'a>>,
}
impl<'a> PathParamIterator<'a> {
fn new(input: &'a str) -> Self {
PathParamIterator {
input,
chars: input.char_indices().peekable(),
}
}
}
impl<'a> Iterator for PathParamIterator<'a> {
type Item = (&'a str, Range<usize>);
fn next(&mut self) -> Option<Self::Item> {
while let Some((start, c)) = self.chars.next() {
if c == '{' {
if let Some((end, _)) = self.chars.by_ref().find(|&(_, ch)| ch == '}') {
let param_content = &self.input[start + 1..end];
// We ignore text after a colon, since those are path convertors
// See also: https://fastapi.tiangolo.com/tutorial/path-params/?h=path#path-convertor
let param_name_end = param_content.find(':').unwrap_or(param_content.len());
let param_name = &param_content[..param_name_end].trim();
#[allow(clippy::range_plus_one)]
return Some((param_name, start..end + 1));
}
}
}
None
}
}

View file

@ -1,8 +1,10 @@
pub(crate) use fastapi_non_annotated_dependency::*;
pub(crate) use fastapi_redundant_response_model::*;
pub(crate) use fastapi_unused_path_parameter::*;
mod fastapi_non_annotated_dependency;
mod fastapi_redundant_response_model;
mod fastapi_unused_path_parameter;
use ruff_python_ast::{Decorator, ExprCall, StmtFunctionDef};
use ruff_python_semantic::analyze::typing::resolve_assignment;

View file

@ -0,0 +1,323 @@
---
source: crates/ruff_linter/src/rules/fastapi/mod.rs
---
FAST003.py:7:19: FAST003 [*] Parameter `thing_id` appears in route path, but not in `read_thing` signature
|
6 | # Errors
7 | @app.get("/things/{thing_id}")
| ^^^^^^^^^^ FAST003
8 | async def read_thing(query: str):
9 | return {"query": query}
|
= help: Add `thing_id` to function signature
Unsafe fix
5 5 |
6 6 | # Errors
7 7 | @app.get("/things/{thing_id}")
8 |-async def read_thing(query: str):
8 |+async def read_thing(query: str, thing_id):
9 9 | return {"query": query}
10 10 |
11 11 |
FAST003.py:12:23: FAST003 [*] Parameter `isbn` appears in route path, but not in `read_thing` signature
|
12 | @app.get("/books/isbn-{isbn}")
| ^^^^^^ FAST003
13 | async def read_thing():
14 | ...
|
= help: Add `isbn` to function signature
Unsafe fix
10 10 |
11 11 |
12 12 | @app.get("/books/isbn-{isbn}")
13 |-async def read_thing():
13 |+async def read_thing(isbn):
14 14 | ...
15 15 |
16 16 |
FAST003.py:17:19: FAST003 [*] Parameter `thing_id` appears in route path, but not in `read_thing` signature
|
17 | @app.get("/things/{thing_id:path}")
| ^^^^^^^^^^^^^^^ FAST003
18 | async def read_thing(query: str):
19 | return {"query": query}
|
= help: Add `thing_id` to function signature
Unsafe fix
15 15 |
16 16 |
17 17 | @app.get("/things/{thing_id:path}")
18 |-async def read_thing(query: str):
18 |+async def read_thing(query: str, thing_id):
19 19 | return {"query": query}
20 20 |
21 21 |
FAST003.py:22:19: FAST003 [*] Parameter `thing_id` appears in route path, but not in `read_thing` signature
|
22 | @app.get("/things/{thing_id : path}")
| ^^^^^^^^^^^^^^^^^ FAST003
23 | async def read_thing(query: str):
24 | return {"query": query}
|
= help: Add `thing_id` to function signature
Unsafe fix
20 20 |
21 21 |
22 22 | @app.get("/things/{thing_id : path}")
23 |-async def read_thing(query: str):
23 |+async def read_thing(query: str, thing_id):
24 24 | return {"query": query}
25 25 |
26 26 |
FAST003.py:27:27: FAST003 [*] Parameter `title` appears in route path, but not in `read_thing` signature
|
27 | @app.get("/books/{author}/{title}")
| ^^^^^^^ FAST003
28 | async def read_thing(author: str):
29 | return {"author": author}
|
= help: Add `title` to function signature
Unsafe fix
25 25 |
26 26 |
27 27 | @app.get("/books/{author}/{title}")
28 |-async def read_thing(author: str):
28 |+async def read_thing(author: str, title):
29 29 | return {"author": author}
30 30 |
31 31 |
FAST003.py:32:18: FAST003 [*] Parameter `author_name` appears in route path, but not in `read_thing` signature
|
32 | @app.get("/books/{author_name}/{title}")
| ^^^^^^^^^^^^^ FAST003
33 | async def read_thing():
34 | ...
|
= help: Add `author_name` to function signature
Unsafe fix
30 30 |
31 31 |
32 32 | @app.get("/books/{author_name}/{title}")
33 |-async def read_thing():
33 |+async def read_thing(author_name):
34 34 | ...
35 35 |
36 36 |
FAST003.py:32:32: FAST003 [*] Parameter `title` appears in route path, but not in `read_thing` signature
|
32 | @app.get("/books/{author_name}/{title}")
| ^^^^^^^ FAST003
33 | async def read_thing():
34 | ...
|
= help: Add `title` to function signature
Unsafe fix
30 30 |
31 31 |
32 32 | @app.get("/books/{author_name}/{title}")
33 |-async def read_thing():
33 |+async def read_thing(title):
34 34 | ...
35 35 |
36 36 |
FAST003.py:37:18: FAST003 Parameter `author` appears in route path, but only as a positional-only argument in `read_thing` signature
|
37 | @app.get("/books/{author}/{title}")
| ^^^^^^^^ FAST003
38 | async def read_thing(author: str, title: str, /):
39 | return {"author": author, "title": title}
|
FAST003.py:37:27: FAST003 Parameter `title` appears in route path, but only as a positional-only argument in `read_thing` signature
|
37 | @app.get("/books/{author}/{title}")
| ^^^^^^^ FAST003
38 | async def read_thing(author: str, title: str, /):
39 | return {"author": author, "title": title}
|
FAST003.py:42:27: FAST003 [*] Parameter `title` appears in route path, but not in `read_thing` signature
|
42 | @app.get("/books/{author}/{title}/{page}")
| ^^^^^^^ FAST003
43 | async def read_thing(
44 | author: str,
|
= help: Add `title` to function signature
Unsafe fix
42 42 | @app.get("/books/{author}/{title}/{page}")
43 43 | async def read_thing(
44 44 | author: str,
45 |- query: str,
45 |+ query: str, title,
46 46 | ): ...
47 47 |
48 48 |
FAST003.py:42:35: FAST003 [*] Parameter `page` appears in route path, but not in `read_thing` signature
|
42 | @app.get("/books/{author}/{title}/{page}")
| ^^^^^^ FAST003
43 | async def read_thing(
44 | author: str,
|
= help: Add `page` to function signature
Unsafe fix
42 42 | @app.get("/books/{author}/{title}/{page}")
43 43 | async def read_thing(
44 44 | author: str,
45 |- query: str,
45 |+ query: str, page,
46 46 | ): ...
47 47 |
48 48 |
FAST003.py:49:18: FAST003 [*] Parameter `author` appears in route path, but not in `read_thing` signature
|
49 | @app.get("/books/{author}/{title}")
| ^^^^^^^^ FAST003
50 | async def read_thing():
51 | ...
|
= help: Add `author` to function signature
Unsafe fix
47 47 |
48 48 |
49 49 | @app.get("/books/{author}/{title}")
50 |-async def read_thing():
50 |+async def read_thing(author):
51 51 | ...
52 52 |
53 53 |
FAST003.py:49:27: FAST003 [*] Parameter `title` appears in route path, but not in `read_thing` signature
|
49 | @app.get("/books/{author}/{title}")
| ^^^^^^^ FAST003
50 | async def read_thing():
51 | ...
|
= help: Add `title` to function signature
Unsafe fix
47 47 |
48 48 |
49 49 | @app.get("/books/{author}/{title}")
50 |-async def read_thing():
50 |+async def read_thing(title):
51 51 | ...
52 52 |
53 53 |
FAST003.py:54:27: FAST003 [*] Parameter `title` appears in route path, but not in `read_thing` signature
|
54 | @app.get("/books/{author}/{title}")
| ^^^^^^^ FAST003
55 | async def read_thing(*, author: str):
56 | ...
|
= help: Add `title` to function signature
Unsafe fix
52 52 |
53 53 |
54 54 | @app.get("/books/{author}/{title}")
55 |-async def read_thing(*, author: str):
55 |+async def read_thing(title, *, author: str):
56 56 | ...
57 57 |
58 58 |
FAST003.py:59:27: FAST003 [*] Parameter `title` appears in route path, but not in `read_thing` signature
|
59 | @app.get("/books/{author}/{title}")
| ^^^^^^^ FAST003
60 | async def read_thing(hello, /, *, author: str):
61 | ...
|
= help: Add `title` to function signature
Unsafe fix
57 57 |
58 58 |
59 59 | @app.get("/books/{author}/{title}")
60 |-async def read_thing(hello, /, *, author: str):
60 |+async def read_thing(hello, /, title, *, author: str):
61 61 | ...
62 62 |
63 63 |
FAST003.py:64:19: FAST003 [*] Parameter `thing_id` appears in route path, but not in `read_thing` signature
|
64 | @app.get("/things/{thing_id}")
| ^^^^^^^^^^ FAST003
65 | async def read_thing(
66 | query: str,
|
= help: Add `thing_id` to function signature
Unsafe fix
63 63 |
64 64 | @app.get("/things/{thing_id}")
65 65 | async def read_thing(
66 |- query: str,
66 |+ query: str, thing_id,
67 67 | ):
68 68 | return {"query": query}
69 69 |
FAST003.py:71:19: FAST003 [*] Parameter `thing_id` appears in route path, but not in `read_thing` signature
|
71 | @app.get("/things/{thing_id}")
| ^^^^^^^^^^ FAST003
72 | async def read_thing(
73 | query: str = "default",
|
= help: Add `thing_id` to function signature
Unsafe fix
70 70 |
71 71 | @app.get("/things/{thing_id}")
72 72 | async def read_thing(
73 |- query: str = "default",
73 |+ thing_id, query: str = "default",
74 74 | ):
75 75 | return {"query": query}
76 76 |
FAST003.py:78:19: FAST003 [*] Parameter `thing_id` appears in route path, but not in `read_thing` signature
|
78 | @app.get("/things/{thing_id}")
| ^^^^^^^^^^ FAST003
79 | async def read_thing(
80 | *, query: str = "default",
|
= help: Add `thing_id` to function signature
Unsafe fix
77 77 |
78 78 | @app.get("/things/{thing_id}")
79 79 | async def read_thing(
80 |- *, query: str = "default",
80 |+ thing_id, *, query: str = "default",
81 81 | ):
82 82 | return {"query": query}
83 83 |

1
ruff.schema.json generated
View file

@ -3121,6 +3121,7 @@
"FAST00",
"FAST001",
"FAST002",
"FAST003",
"FBT",
"FBT0",
"FBT00",