Skip to content
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] Implement fast-api-unused-path-parameter (FAST003) #12638

Merged
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
423206c
Basic implementation of fastapi_unused_path_parameter
Matthieu-LAURENT39 Aug 2, 2024
5168d72
Ignore path parameters that aren't valid identifiers
Matthieu-LAURENT39 Aug 2, 2024
b322603
Add some tests related to path convertors
Matthieu-LAURENT39 Aug 2, 2024
0c4ee27
Add tests with multiple path parameters
Matthieu-LAURENT39 Aug 2, 2024
18ca13f
Show the specific path causing the issue
Matthieu-LAURENT39 Aug 2, 2024
35ded68
Add the add_parameter edit function
Matthieu-LAURENT39 Aug 2, 2024
4985888
Add automatic fixing, and improve message when the argument is presen…
Matthieu-LAURENT39 Aug 2, 2024
bc6346e
Mark fast-api-unused-path-parameter as sometimes fixable
Matthieu-LAURENT39 Aug 2, 2024
7d4ac25
Add fast-api-unused-path-parameter to the rule schema
Matthieu-LAURENT39 Aug 2, 2024
ba47aaf
Add snapshot for fast-api-unused-path-parameter
Matthieu-LAURENT39 Aug 2, 2024
be57618
Fix clippy errors
Matthieu-LAURENT39 Aug 2, 2024
5663e86
Fix docs formatting
Matthieu-LAURENT39 Aug 2, 2024
10e7381
Use str instead of String where possible
Matthieu-LAURENT39 Aug 3, 2024
4a13fb4
Use iterators for extract_path_params_from_route, and move away from …
Matthieu-LAURENT39 Aug 3, 2024
4bb9fa8
Add an extra OK test
Matthieu-LAURENT39 Aug 3, 2024
4b95723
Add some extra tests
Matthieu-LAURENT39 Aug 3, 2024
a8e6aff
Use the f-string parser
Matthieu-LAURENT39 Aug 3, 2024
739821e
Remove unused import
Matthieu-LAURENT39 Aug 3, 2024
77e0067
Ignore path parameters with conversions or debug text
Matthieu-LAURENT39 Aug 4, 2024
f17e8df
don't collect path_params into a vec
Matthieu-LAURENT39 Aug 4, 2024
8169cc4
Few smaller nits
MichaReiser Aug 13, 2024
79454db
Fix doc formatting
MichaReiser Aug 13, 2024
9ec7632
Revert to not using the f-string parser
Matthieu-LAURENT39 Aug 13, 2024
288bd10
Minor tweaks
charliermarsh Aug 16, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 113 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/fastapi/FAST003.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
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):
...


# 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}
3 changes: 3 additions & 0 deletions crates/ruff_linter/src/checkers/ast/analyze/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -919,6 +919,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),
Expand Down
41 changes: 40 additions & 1 deletion crates/ruff_linter/src/fix/edits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -283,6 +283,45 @@ 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.last() {
// Case 1: at least one regular parameter, so append after the last one.
Edit::insertion(format!(", {parameter}"), last.range().end())
} else if let Some(last) = parameters.posonlyargs.last() {
// Case 2: no regular parameter, but a positional-only parameter exist, so add parameter after that.
// We take care to add it *after* the `/` separator.
let pos = last.range().end();
let mut tokenizer = SimpleTokenizer::starts_at(pos, source);
let slash = tokenizer
.find(|token| token.kind == SimpleTokenKind::Slash)
.expect("Unable to find ,");
// 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.range().start();
let mut tokenizer = SimpleTokenizer::starts_at(pos, source);
let star = tokenizer
.find(|token| token.kind == SimpleTokenKind::Star)
.expect("Unable to find *");
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
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/fastapi/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
Loading
Loading