[airflow] Third positional parameter not named ti_key should be flagged for BaseOperatorLink.get_link (AIR303)

[sjyangkevin]

Summary

Context:
apache/airflow#41641
apache/airflow#46415

In the old signature, dttm is used by Airflow as a positional argument, so the argument name in the function declaration may not be dttm. In the new signature, however, the argument name must be ti_key (although it may be non-keyword-only; self, operator, ti_key should be considered new signature). So the deprecated signature detection rule should be exactly three positional arguments, the third one not named ti_key.

Old Signature

class MyOperatorLink(BaseOperatorLink):
    def get_link(self, operator, dttm):
        ...

New Signature

class MyOperatorLink(BaseOperatorLink):
    def get_link(self, operator, *, ti_key):
        ...

This PR extends AIR303 to detect deprecated get_link method signatures in BaseOperatorLink subclasses, which is a method definition. BaseOperatorLink is a base class can be extended by the user, and the get_link method is called by Airflow. As the way Airflow internally call this method change, we need to flag the user's implementation if the method definition is not match with the new signature (@Lee-W , please correct me if I understand it wrong, whenever you have time, thanks). The rule detects deprecated signatures where:

1. The class inherits from BaseOperatorLink (from airflow.models or airflow.sdk)
2. The method is named get_link
3. There are exactly 3 positional parameters
4. The 3rd parameter is not named as ti_key

Test Plan

The test cases are added to AIR303.py and the test snapshot is updated.

[sjyangkevin]

The original definition in function_signature_change_in_3.rs becomes redundant as these enum items only encapsulate a message. Therefore, combine these into a single Message item, and move it to helpers.rs. This enum will be dedicated used for AIR303, or function signature change in future version of Airflow.

#[derive(Clone, Debug, Eq, PartialEq)]
pub(crate) enum FunctionSignatureChangeType {
    /// Function signature changed to only accept keyword arguments.
    KeywordOnly { message: &'static str },
    /// Function signature changed to not accept certain positional arguments.
    PositionalArgumentChange { message: &'static str },
}

[sjyangkevin]

This helper function generalizes the function is_execute_method_inherits_from_airflow_operator defined in the removal_in_3.rs. The function is checking if a subclass of airflow's BaseOperator implements a execute function. In AIR303, we need to check if a subclass of airflow's BaseOperatorLink implements a get_link function. Therefore, this is something reusable across multiple rules. I parameterized the method name and the match condition, to reduce redundant definition for similar checks.

[astral-sh-bot]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

[sjyangkevin]

updated the logic to check for valid signature pattern, which is either def get_link(self, operator, *, ti_key) or ``def get_link(self, operator, ti_key)`, and the diagnostic will be raised when the method signature is evaluated to invalid.

The name of the first two positional parameters doesn't matter for the rule. If there are 2 positional parameters (i.e., self, operator), check if the keyword-only parameter is named ti_key. If there are 3 positional parameters, check if the third one is named ti_key.

The message and test cases are updated accordingly.

[Lee-W]

what would happen if we use def get_link(self, operator, ti_key)

[sjyangkevin]

In this case, I think we have 3 positional parameters, and it will not match the first condition which check if the number of positional parameters is 2. It will proceed to the next condition and check if there are 3 positional parameters and iterate through it see if the 3rd one is named ti_key. I’ve added a test case for this but will also do a double check. Thanks!

[Lee-W]

Thanks!