-
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
feat(AIR302): check whether removed context key "conf" is access through context.get(...) #15240
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
import pendulum | ||
|
||
from airflow.decorators import dag, task | ||
from airflow.providers.standard.operators.python import PythonOperator | ||
|
||
|
||
def access_invalid_key_in_context(**context): | ||
print("access invalid key", context["conf"]) | ||
|
||
|
||
@task | ||
def access_invalid_key_task_out_of_dag(**context): | ||
print("access invalid key", context.get("conf")) | ||
|
||
|
||
@dag( | ||
schedule=None, | ||
start_date=pendulum.datetime(2021, 1, 1, tz="UTC"), | ||
catchup=False, | ||
tags=[""], | ||
) | ||
def invalid_dag(): | ||
@task() | ||
def access_invalid_key_task(**context): | ||
print("access invalid key", context.get("conf")) | ||
|
||
task1 = PythonOperator( | ||
task_id="task1", | ||
python_callable=access_invalid_key_in_context, | ||
) | ||
access_invalid_key_task() >> task1 | ||
access_invalid_key_task_out_of_dag() | ||
|
||
|
||
invalid_dag() |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,17 +1,17 @@ | ||||||
use crate::checkers::ast::Checker; | ||||||
use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation}; | ||||||
use ruff_macros::{derive_message_formats, ViolationMetadata}; | ||||||
use ruff_python_ast::helpers::map_callable; | ||||||
use ruff_python_ast::{ | ||||||
name::QualifiedName, Arguments, Expr, ExprAttribute, ExprCall, ExprContext, ExprName, | ||||||
StmtClassDef, | ||||||
name::QualifiedName, Arguments, Expr, ExprAttribute, ExprCall, ExprContext, ExprName, Stmt, | ||||||
StmtClassDef, StmtFunctionDef, | ||||||
}; | ||||||
use ruff_python_semantic::analyze::typing; | ||||||
use ruff_python_semantic::Modules; | ||||||
use ruff_python_semantic::ScopeKind; | ||||||
use ruff_text_size::Ranged; | ||||||
use ruff_text_size::TextRange; | ||||||
|
||||||
use crate::checkers::ast::Checker; | ||||||
|
||||||
/// ## What it does | ||||||
/// Checks for uses of deprecated Airflow functions and values. | ||||||
/// | ||||||
|
@@ -87,6 +87,7 @@ pub(crate) fn removed_in_3(checker: &mut Checker, expr: &Expr) { | |||||
check_call_arguments(checker, &qualname, arguments); | ||||||
}; | ||||||
check_method(checker, call_expr); | ||||||
check_context_get(checker, call_expr); | ||||||
} | ||||||
Expr::Attribute(attribute_expr @ ExprAttribute { attr, .. }) => { | ||||||
check_name(checker, expr, attr.range()); | ||||||
|
@@ -203,6 +204,54 @@ fn check_call_arguments(checker: &mut Checker, qualname: &QualifiedName, argumen | |||||
}; | ||||||
} | ||||||
|
||||||
/// Check whether a removed context key is access through context.get("key"). | ||||||
/// | ||||||
/// ```python | ||||||
/// from airflow.decorators import task | ||||||
/// | ||||||
/// | ||||||
/// @task | ||||||
/// def access_invalid_key_task_out_of_dag(**context): | ||||||
/// print("access invalid key", context.get("conf")) | ||||||
/// ``` | ||||||
fn check_context_get(checker: &mut Checker, call_expr: &ExprCall) { | ||||||
const REMOVED_CONTEXT_KEYS: [&str; 1] = ["conf"]; | ||||||
|
||||||
if !is_taskflow(checker) { | ||||||
return; | ||||||
} | ||||||
|
||||||
let Expr::Attribute(ExprAttribute { value, attr, .. }) = &*call_expr.func else { | ||||||
return; | ||||||
}; | ||||||
|
||||||
// Ensure the method called is `context` | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
if !value | ||||||
.as_name_expr() | ||||||
.is_some_and(|name| matches!(name.id.as_str(), "context")) | ||||||
{ | ||||||
return; | ||||||
} | ||||||
|
||||||
// Ensure the method called is `get` | ||||||
if attr.as_str() != "get" { | ||||||
return; | ||||||
} | ||||||
|
||||||
for removed_key in REMOVED_CONTEXT_KEYS { | ||||||
if let Some(argument) = call_expr.arguments.find_argument_value(removed_key, 0) { | ||||||
checker.diagnostics.push(Diagnostic::new( | ||||||
Airflow3Removal { | ||||||
deprecated: removed_key.to_string(), | ||||||
replacement: Replacement::None, | ||||||
}, | ||||||
argument.range(), | ||||||
)); | ||||||
return; | ||||||
} | ||||||
} | ||||||
} | ||||||
|
||||||
/// Check whether a removed Airflow class attribute (include property) is called. | ||||||
/// | ||||||
/// For example: | ||||||
|
@@ -849,3 +898,35 @@ fn is_airflow_builtin_or_provider(segments: &[&str], module: &str, symbol_suffix | |||||
_ => false, | ||||||
} | ||||||
} | ||||||
|
||||||
/// Check whether the function is decorated by @task | ||||||
/// | ||||||
/// | ||||||
/// Examples for the above patterns: | ||||||
/// ```python | ||||||
/// from airflow.decorators import task | ||||||
/// | ||||||
/// | ||||||
/// @task | ||||||
/// def access_invalid_key_task_out_of_dag(**context): | ||||||
/// print("access invalid key", context.get("conf")) | ||||||
/// ``` | ||||||
fn is_taskflow(checker: &mut Checker) -> bool { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can possible make this generic as I suspect it's going to be used for other decorators as well. Ignore if it's not going to be used for any other decorators other than fn is_decorated_with(checker: &mut Checker, decorator: &str) -> bool {
let mut parents = checker.semantic().current_statements();
if let Some(Stmt::FunctionDef(StmtFunctionDef { decorator_list, .. })) =
parents.find(|stmt| stmt.is_function_def_stmt())
{
for decorator in decorator_list {
if checker
.semantic()
.resolve_qualified_name(map_callable(&decorator.expression))
.is_some_and(|qualified_name| {
matches!(qualified_name.segments(), ["airflow", "decorators", decorator])
})
{
return true;
}
}
}
false
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @task is probably the only case I can think of for now 🤔 I can do the refactoring once we have another case in the future |
||||||
let mut parents = checker.semantic().current_statements(); | ||||||
if let Some(Stmt::FunctionDef(StmtFunctionDef { decorator_list, .. })) = | ||||||
parents.find(|stmt| stmt.is_function_def_stmt()) | ||||||
{ | ||||||
for decorator in decorator_list { | ||||||
if checker | ||||||
.semantic() | ||||||
.resolve_qualified_name(map_callable(&decorator.expression)) | ||||||
.is_some_and(|qualified_name| { | ||||||
matches!(qualified_name.segments(), ["airflow", "decorators", "task"]) | ||||||
}) | ||||||
{ | ||||||
return true; | ||||||
} | ||||||
} | ||||||
} | ||||||
false | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
--- | ||
source: crates/ruff_linter/src/rules/airflow/mod.rs | ||
snapshot_kind: text | ||
--- | ||
AIR302_context.py:13:45: AIR302 `conf` is removed in Airflow 3.0 | ||
| | ||
11 | @task | ||
12 | def access_invalid_key_task_out_of_dag(**context): | ||
13 | print("access invalid key", context.get("conf")) | ||
| ^^^^^^ AIR302 | ||
| | ||
|
||
AIR302_context.py:25:49: AIR302 `conf` is removed in Airflow 3.0 | ||
| | ||
23 | @task() | ||
24 | def access_invalid_key_task(**context): | ||
25 | print("access invalid key", context.get("conf")) | ||
| ^^^^^^ AIR302 | ||
26 | | ||
27 | task1 = PythonOperator( | ||
| |
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.
How many arguments are you planning to deprecate for this to be a list?
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.
I think there are multiple as stated in this PR description: #15144 (comment)