diff --git a/airflow-core/docs/howto/custom-operator.rst b/airflow-core/docs/howto/custom-operator.rst index 858b1154c412e..b5e14128ca069 100644 --- a/airflow-core/docs/howto/custom-operator.rst +++ b/airflow-core/docs/howto/custom-operator.rst @@ -336,6 +336,22 @@ Therefore, the following example is invalid: def __init__(self, foo) -> None: self.foo = foo.lower() # assignment should be only self.foo = foo +4. Checking whether an argument was *passed* is allowed in the constructor — ``execute()`` cannot +tell a supplied field from a missing one, because a field can be ``None`` after rendering. Write it +as ``is None`` / ``is not None``, never as a truthiness test. Anything that inspects the *value* +still belongs in ``execute()``: + +.. code-block:: python + + class HelloOperator(BaseOperator): + template_fields = ("foo", "bar") + + def __init__(self, foo=None, bar=None) -> None: + if foo is None and bar is None: # allowed: asks what was passed + raise ValueError("Either 'foo' or 'bar' must be provided") + self.foo = foo + self.bar = bar + When an operator inherits from a base operator and does not have a constructor defined on its own, the limitations above do not apply. However, the templated fields must be set properly in the parent according to those limitations. diff --git a/contributing-docs/05_pull_requests.rst b/contributing-docs/05_pull_requests.rst index 2b3c692283bbf..7ba8e8d5e0e8f 100644 --- a/contributing-docs/05_pull_requests.rst +++ b/contributing-docs/05_pull_requests.rst @@ -434,6 +434,36 @@ In such cases we can usually do something like this my_field = my_field or [] self.my_field = my_field +For such a mutable default the ``if my_field is None: my_field = []`` spelling is equally fine. + +Checks that only ask **whether an argument was passed** are the other exception, and they belong in +the constructor — it is the only place that can answer the question. With +``render_template_as_native_obj=True`` a field that *was* passed can render to +``None``, so in ``execute`` a supplied argument is indistinguishable from a missing one. Write the +check as ``is None`` / ``is not None``, never as a truthiness test — a supplied argument can itself +be ``""``, ``0`` or an empty list: + +.. code-block:: python + + from airflow.utils.helpers import exactly_one + + + def __init__(self, *, command: str | None = None, powershell: str | None = None, **kwargs): + if not exactly_one(command is not None, powershell is not None): + raise ValueError("Must provide exactly one of 'command' or 'powershell'") + super().__init__(**kwargs) + self.command = command + self.powershell = powershell + +``self.field is None`` works the same way, after the assignment. Anything that looks at the *value* +still has to move to ``execute``, and the operand has to be the field itself — indirection such as +``all(v is None for v in [a, b])`` is still flagged. + +Converting an existing truthiness check is not neutral: a guard that raises when two arguments are +*both* set only gets stricter, but ``if not field: raise`` gets looser — ``""`` and ``0`` start +passing, and that half is a value check. ``not exactly_one(a, b)`` carries both, since it also +requires at least one. + The reason for doing it is that we are working on a cleaning up our code to have `prek hook <../scripts/ci/prek/validate_operators_init.py>`_ that will make sure all the cases where logic (such as validation and complex conversion) diff --git a/providers/.pre-commit-config.yaml b/providers/.pre-commit-config.yaml index ab6fef5a7f00d..27d886f162add 100644 --- a/providers/.pre-commit-config.yaml +++ b/providers/.pre-commit-config.yaml @@ -52,8 +52,8 @@ repos: always_run: true pass_filenames: false - id: validate-operators-init - name: No templated field logic checks in operator __init__ - description: Prevent templated field logic checks in operators' __init__ + name: Validate templated fields in operator __init__ + description: Require plain assignment of templated fields and forbid reading their un-rendered values language: python entry: ../scripts/ci/prek/validate_operators_init.py pass_filenames: true diff --git a/scripts/ci/prek/validate_operators_init.py b/scripts/ci/prek/validate_operators_init.py index b8893700c9037..664d3e4e471e7 100755 --- a/scripts/ci/prek/validate_operators_init.py +++ b/scripts/ci/prek/validate_operators_init.py @@ -25,6 +25,7 @@ import ast import sys +from collections.abc import Iterator from pathlib import Path from typing import Any @@ -179,7 +180,7 @@ def _handle_constructor_statement( if isinstance(ctor_stmt, ast.Assign): if isinstance(ctor_stmt.targets[0], ast.Attribute): for target in ctor_stmt.targets: - if isinstance(target, ast.Attribute) and target.attr in template_fields: + if isinstance(target, ast.Attribute) and _target_name(target) in template_fields: if isinstance(ctor_stmt.value, ast.IfExp) and _is_value_preserving_ternary( ctor_stmt.value, target.attr ): @@ -196,10 +197,10 @@ def _handle_constructor_statement( ) elif isinstance(ctor_stmt.targets[0], ast.Tuple) and isinstance(ctor_stmt.value, ast.Tuple): for target, value in zip(ctor_stmt.targets[0].elts, ctor_stmt.value.elts): - if isinstance(target, ast.Attribute): + if isinstance(target, ast.Attribute) and _target_name(target) in template_fields: _handle_assigned_field(assigned_template_fields, invalid_assignments, target, value) elif isinstance(ctor_stmt, ast.AnnAssign): - if isinstance(ctor_stmt.target, ast.Attribute) and ctor_stmt.target.attr in template_fields: + if isinstance(ctor_stmt.target, ast.Attribute) and _target_name(ctor_stmt.target) in template_fields: _handle_assigned_field( assigned_template_fields, invalid_assignments, ctor_stmt.target, ctor_stmt.value ) @@ -225,11 +226,11 @@ def _handle_assigned_field( def _target_name(target: ast.expr) -> str | None: """ - Resolve an assignment target to the field name it binds. + Resolve an assignment target — or a comparison operand — to the field name it refers to. - :param target: The assignment target node. - :return: The attribute name for ``self.`` targets, the identifier for bare-name - targets, or None for anything else. + :param target: The node to resolve. + :return: The attribute name for ``self.`` nodes, the identifier for bare names, + or None for anything else. """ if isinstance(target, ast.Attribute) and isinstance(target.value, ast.Name) and target.value.id == "self": return target.attr @@ -254,6 +255,22 @@ def _is_super_init_call(node: ast.Call) -> bool: ) +def _is_none_check(node: ast.expr) -> bool: + """ + Check whether an expression is an ``x is None`` / ``x is not None`` comparison. + + :param node: The expression node to check. + :return: True if the node compares a single operand against the ``None`` literal by identity. + """ + return ( + isinstance(node, ast.Compare) + and len(node.ops) == 1 + and isinstance(node.ops[0], (ast.Is, ast.IsNot)) + and isinstance(node.comparators[0], ast.Constant) + and node.comparators[0].value is None + ) + + def _is_value_preserving_ternary(value: ast.IfExp, field: str) -> bool: """ Check whether a ternary keeps the field value intact when it is set. @@ -274,11 +291,7 @@ def _is_value_preserving_ternary(value: ast.IfExp, field: str) -> bool: isinstance(test, ast.Compare) and isinstance(test.left, ast.Name) and test.left.id == field - and len(test.ops) == 1 - and isinstance(test.ops[0], (ast.Is, ast.IsNot)) - and len(test.comparators) == 1 - and isinstance(test.comparators[0], ast.Constant) - and test.comparators[0].value is None + and _is_none_check(test) ) @@ -289,12 +302,13 @@ def _collect_sanctioned_uses(ctor: ast.FunctionDef, template_fields: list[str]) Sanctioned patterns are the ones the project documents as safe in a constructor: ``self.field = field``, ``self.field = field or ``, the equivalent value-preserving ternaries, the local rebind ``field = field or ``, - tuple assignments pairing names one-to-one, and forwarding via - ``super().__init__(field=field)``. + tuple assignments pairing names one-to-one, forwarding via + ``super().__init__(field=field)``, and ``field is None`` / ``field is not None`` + provision checks. :param ctor: The constructor function node. :param template_fields: The template fields of the class. - :return: Set of ``id()``s of Name nodes participating in sanctioned patterns. + :return: Set of ``id()``s of nodes participating in sanctioned patterns. """ sanctioned: set[int] = set() @@ -325,6 +339,9 @@ def mark(value: ast.expr | None, field: str) -> None: for keyword in node.keywords: if keyword.arg is not None and keyword.arg in template_fields: mark(keyword.value, keyword.arg) + elif isinstance(node, ast.Compare) and _is_none_check(node): + # Reads whether the argument was passed, not its value — only __init__ can see that. + sanctioned.add(id(node.left)) return sanctioned @@ -375,14 +392,20 @@ def _check_constructor_field_logic( if node.id in template_fields and node.id in bound_names and id(node) not in sanctioned: findings.setdefault(node.lineno, set()).add(node.id) elif isinstance(node, ast.Attribute) and isinstance(node.ctx, ast.Load): - if isinstance(node.value, ast.Name) and node.value.id == "self" and node.attr in template_fields: + if ( + isinstance(node.value, ast.Name) + and node.value.id == "self" + and node.attr in template_fields + and id(node) not in sanctioned + ): findings.setdefault(node.lineno, set()).add(f"self.{node.attr}") if findings: console.print( f"{class_node.name}'s constructor applies logic to template fields. Template fields " f"are rendered after the constructor runs, so validation or transformation here acts " - f"on the un-rendered Jinja expression and should move to execute():" + f"on the un-rendered Jinja expression and should move to execute() " + f"(see contributing-docs/05_pull_requests.rst):" ) for lineno in sorted(findings): source = source_lines[lineno - 1].strip() if lineno <= len(source_lines) else "" @@ -392,11 +415,31 @@ def _check_constructor_field_logic( return len(findings) +def _iter_nested_statements(node: ast.AST) -> Iterator[ast.stmt]: + """ + Yield the statements nested inside ``node``, excluding ``node`` itself. + + Nested scopes are skipped: ``self. = ...`` in a function or class defined in the + constructor either belongs to another object or runs after rendering. + + :param node: The node to descend into. + :return: Iterator over the nested statements. + """ + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)): + return + for child in ast.iter_child_nodes(node): + if isinstance(child, ast.stmt): + yield child + yield from _iter_nested_statements(child) + + def _check_constructor_template_fields(class_node: ast.ClassDef, template_fields: list[str]) -> int: """ This method checks a class's constructor for missing or invalid assignments of template fields. When there isn't a constructor - it assumes that the template fields are defined in the parent's constructor correctly. + Nested statements can only add invalid assignments: a branch may not run, so it cannot satisfy + the requirement that the field be assigned. TODO: Enhance this function to work with nested inheritance trees through dynamic imports. :param class_node: the AST node representing the class definition @@ -418,6 +461,9 @@ def _check_constructor_template_fields(class_node: ast.ClassDef, template_fields missing_assignments = _handle_constructor_statement( template_fields, ctor_stmt, missing_assignments, invalid_assignments ) + for nested_stmt in _iter_nested_statements(ctor_stmt): + _handle_parent_constructor_kwargs(template_fields, nested_stmt, [], invalid_assignments) + _handle_constructor_statement(template_fields, nested_stmt, [], invalid_assignments) if init_flag and missing_assignments: count += len(missing_assignments) @@ -428,6 +474,7 @@ def _check_constructor_template_fields(class_node: ast.ClassDef, template_fields ) console.print(f"[red]{missing_assignments}[/red]") + invalid_assignments = sorted(set(invalid_assignments)) if invalid_assignments: count += len(invalid_assignments) console.print( diff --git a/scripts/ci/prek/validate_operators_init_exemptions.txt b/scripts/ci/prek/validate_operators_init_exemptions.txt index 608ce3bda51d0..76ba180595a56 100644 --- a/scripts/ci/prek/validate_operators_init_exemptions.txt +++ b/scripts/ci/prek/validate_operators_init_exemptions.txt @@ -3,10 +3,8 @@ # # Format: :: (one class per line) # -# Fixing a class (moving template-field validation/transformation out of __init__ into -# execute()) MUST remove its entry in the same PR — the hook fails on stale entries. +# Fixing a class MUST remove its entry in the same PR — the hook fails on stale entries. # Burn-down tracked at https://github.com/apache/airflow/issues/70296 -providers/amazon/src/airflow/providers/amazon/aws/operators/emr.py::EmrAddStepsOperator providers/amazon/src/airflow/providers/amazon/aws/operators/neptune.py::NeptuneStartDbClusterOperator providers/amazon/src/airflow/providers/amazon/aws/operators/neptune.py::NeptuneStopDbClusterOperator providers/amazon/src/airflow/providers/amazon/aws/operators/s3.py::S3DeleteObjectsOperator @@ -21,7 +19,6 @@ providers/google/src/airflow/providers/google/cloud/operators/cloud_storage_tran providers/google/src/airflow/providers/google/cloud/operators/dataproc.py::DataprocCreateClusterOperator providers/google/src/airflow/providers/google/cloud/operators/dataproc.py::DataprocSubmitJobOperator providers/google/src/airflow/providers/google/cloud/operators/functions.py::CloudFunctionDeployFunctionOperator -providers/google/src/airflow/providers/google/cloud/operators/gcs.py::GCSDeleteObjectsOperator providers/google/src/airflow/providers/google/cloud/operators/gcs.py::GCSFileTransformOperator providers/google/src/airflow/providers/google/cloud/operators/gcs.py::GCSListObjectsOperator providers/google/src/airflow/providers/google/cloud/sensors/bigquery_dts.py::BigQueryDataTransferServiceTransferRunSensor @@ -30,10 +27,7 @@ providers/google/src/airflow/providers/google/cloud/transfers/azure_fileshare_to providers/google/src/airflow/providers/google/cloud/transfers/bigquery_to_mssql.py::BigQueryToMsSqlOperator providers/google/src/airflow/providers/google/cloud/transfers/gcs_to_bigquery.py::GCSToBigQueryOperator providers/google/src/airflow/providers/google/cloud/transfers/gcs_to_gcs.py::GCSToGCSOperator -providers/google/src/airflow/providers/google/cloud/transfers/gcs_to_local.py::GCSToLocalFilesystemOperator providers/google/src/airflow/providers/google/marketing_platform/operators/campaign_manager.py::GoogleCampaignManagerDeleteReportOperator providers/microsoft/azure/src/airflow/providers/microsoft/azure/transfers/gcs_to_wasb.py::GCSToAzureBlobStorageOperator -providers/microsoft/azure/src/airflow/providers/microsoft/azure/transfers/oracle_to_azure_data_lake.py::OracleToAzureDataLakeOperator providers/microsoft/psrp/src/airflow/providers/microsoft/psrp/operators/psrp.py::PsrpOperator -providers/oracle/src/airflow/providers/oracle/transfers/oracle_to_oracle.py::OracleToOracleOperator providers/standard/src/airflow/providers/standard/operators/bash.py::BashOperator diff --git a/scripts/tests/ci/prek/test_validate_operators_init.py b/scripts/tests/ci/prek/test_validate_operators_init.py index cc308796e4a5c..4546a173ca7c7 100644 --- a/scripts/tests/ci/prek/test_validate_operators_init.py +++ b/scripts/tests/ci/prek/test_validate_operators_init.py @@ -66,8 +66,8 @@ class TestConstructorFieldLogic: pytest.param("self._validate(foo)\nself.foo = foo", 1, id="validation-call"), pytest.param( "if foo is not None:\n self._validate(foo)\nself.foo = foo", - 2, - id="nested-validation-call", + 1, + id="validation-call-behind-provision-guard", ), pytest.param("self.foo = foo\nself.bar = foo.upper()", 1, id="derived-assignment"), pytest.param( @@ -80,6 +80,52 @@ class TestConstructorFieldLogic: 2, id="ctor-validation-raise", ), + pytest.param( + "if foo is None:\n raise ValueError('foo is required')\nself.foo = foo", + 0, + id="provision-check-raise", + ), + pytest.param( + "self.foo = foo\nif self.foo is not None:\n self.bar = 1", + 0, + id="provision-check-on-self-attribute", + ), + pytest.param( + "if foo is None:\n raise ValueError('required')\n" + "if foo.startswith('x'):\n raise ValueError('bad')\nself.foo = foo", + 1, + id="provision-check-alongside-value-read", + ), + pytest.param( + "if foo.get('x') is None:\n raise ValueError('bad')\nself.foo = foo", + 1, + id="none-check-on-derived-value", + ), + pytest.param( + "if foo == None:\n raise ValueError('required')\nself.foo = foo", + 1, + id="equality-check-is-not-a-provision-check", + ), + pytest.param( + "if foo is False:\n raise ValueError('bad')\nself.foo = foo", + 1, + id="identity-check-against-a-non-none-constant", + ), + pytest.param( + "if foo is bar:\n raise ValueError('bad')\nself.foo = foo", + 1, + id="identity-check-against-a-non-constant", + ), + pytest.param( + "if foo is None is not bar:\n raise ValueError('bad')\nself.foo = foo", + 1, + id="chained-comparison-is-not-a-provision-check", + ), + pytest.param( + "if not exactly_one(foo is None, bar is None):\n raise ValueError('x')\nself.foo = foo", + 0, + id="provision-check-passed-to-a-helper", + ), ], ) def test_flags_logic_but_not_sanctioned_patterns(self, ctor_body: str, expected: int): @@ -148,15 +194,73 @@ class Op(BaseOperator): class TestValuePreservingTernaryAssignment: - def test_ternary_default_is_a_valid_assignment(self): - code = """ + @pytest.mark.parametrize( + "value, expected", + [ + pytest.param("conf if conf else {}", 0, id="value-preserving"), + # Neither a valid assignment nor a substitute for one: invalid, and still missing. + pytest.param('conf if conf == "x" else {}', 2, id="value-comparison"), + ], + ) + def test_ternary_default_assignment(self, value: str, expected: int): + code = f""" class Op(BaseOperator): template_fields = ("conf",) def __init__(self, conf=None, **kwargs): - self.conf = conf if conf else {} + self.conf = {value} """ - assert _check_constructor_template_fields(_first_class(code), ["conf"]) == 0 + assert _check_constructor_template_fields(_first_class(code), ["conf"]) == expected + + @pytest.mark.parametrize( + "nested", + [ + pytest.param("if baz:\n self.foo = derive(bar)", id="doubly-nested"), + pytest.param("super().__init__(foo=derive(bar), **kwargs)", id="super-kwarg"), + pytest.param("self.foo = derive(bar)\n self.foo = derive(baz)", id="reported-once"), + pytest.param( + "try:\n pass\n except ValueError:\n self.foo = derive(bar)", + id="except-handler", + ), + ], + ) + def test_derived_assignment_nested_in_a_branch_is_invalid(self, nested: str): + code = _operator_code(f"self.foo = foo\nif bar:\n {nested}") + assert _check_constructor_template_fields(_first_class(code), ["foo"]) == 1 + + @pytest.mark.parametrize( + "nested", + [ + pytest.param("hook.foo = derive()", id="other-object-attribute"), + pytest.param("hook.foo: str = derive()", id="other-object-annotated"), + pytest.param("cfg.foo, cfg.bar = 1, 2", id="other-object-tuple-target"), + ], + ) + def test_assignment_to_another_objects_attribute_is_not_the_field(self, nested: str): + code = _operator_code(f"self.foo = foo\nif bar:\n {nested}") + assert _check_constructor_template_fields(_first_class(code), ["foo"]) == 0 + + @pytest.mark.parametrize( + "ctor_body", + [ + pytest.param("if kwargs:\n self.foo = foo", id="branch"), + pytest.param("if foo is None:\n super().__init__(foo=foo, **kwargs)", id="super-call"), + ], + ) + def test_conditional_assignment_does_not_satisfy_the_field(self, ctor_body: str): + assert _check_constructor_template_fields(_first_class(_operator_code(ctor_body)), ["foo"]) == 1 + + @pytest.mark.parametrize( + "scope", + [ + pytest.param("def on_kill():\n self.foo = derive()", id="nested-function"), + pytest.param("async def on_kill():\n self.foo = derive()", id="nested-async-function"), + pytest.param("class Inner:\n self.foo = derive()", id="nested-class"), + ], + ) + def test_assignment_in_a_nested_scope_belongs_to_that_scope(self, scope: str): + code = _operator_code(f"self.foo = foo\n{scope}") + assert _check_constructor_template_fields(_first_class(code), ["foo"]) == 0 class TestExemptions: