feat(AIR302): check whether removed context key "conf" is access through context.get(...) #15240
Conversation
…ugh context.get(...)
|
This PR also adds a utility function and works as an example to check whether this kind of access is from taskflow. for #15144 |
|
@sunank200 As I'll be out traveling for the following week and will have almost no access to laptop, if there're comments to be resolved, please feel free to push to this branch directly and this PR should help #15144 moving forward as well. |
|
|
I'm not able to finish the python_callable part in time but that part should be checked as well as discussed in #15144 (comment) |
| return; | ||
| }; | ||
|
|
||
| // Ensure the method called is `context` |
There was a problem hiding this comment.
| // Ensure the method called is `context` | |
| // Ensure the method called is on `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"]; |
There was a problem hiding this comment.
How many arguments are you planning to deprecate for this to be a list?
There was a problem hiding this comment.
I think there are multiple as stated in this PR description: #15144 (comment)
Safe and good travels! Enjoy your time without a laptop ;) |
| /// 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.
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 @task.
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.
@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
dhruvmanila
added
rule
|
@sunank200 This PR and the one that you've opened (#15144) seems very familiar. Does your PR supersedes this? If not, can / should they be merged? |
|
@dhruvmanila this PR #15144 supersedes this. We could keep #15144. I can make @Lee-W coauthor on my PR. |
|
Sounds good @sunank200, thanks. I'll close this PR then. |
Summary
context key
confis removed in Airflow3. This PR handle the case that these keys are accessed throughcontext.get(...)e.g.,
Test Plan
A test fixture is included in the PR.