You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "dkranchii (via GitHub)" <gi...@apache.org> on 2026/07/28 19:24:20 UTC
[PR] Enable ruff B023 (function-uses-loop-variable) and fix violations [airflow]
dkranchii opened a new pull request, #70640:
URL: https://github.com/apache/airflow/pull/70640
B023 catches the late-binding closure-over-loop-variable footgun where a function defined inside a loop captures the loop variable by reference, so every function in the resulting list sees the same (final) value — a classic silent-bug source in Python. See the [Python FAQ entry](https://docs.python.org/3/faq/programming.html#why-do-lambdas-defined-in-a-loop-with-different-values-all-return-the-same-result).
The one user-visible fix is in `providers/standard/.../triggers/file.py`, where the `FileTrigger`'s `os.walk` lambda was dispatched to a worker thread via `anyio.to_thread.run_sync` while the outer glob iteration could advance, potentially walking the wrong path.
The rest are pre-existing latent-bug or false-positive sites fixed by binding the loop-derived variable as a default argument on the inner function or lambda:
- `airflow-core/.../0101_3_2_0_ui_improvements_for_deadlines.py` (migration; 7 sites in one nested function)
- `dev/breeze/.../ui_commands.py` (two i18n helpers)
- `devel-common/src/sphinx_exts/providers_extensions.py` (two class-extras lambdas)
- `providers/apache/hive/tests/unit/.../test_s3_to_hive.py`
- `providers/exasol/tests/unit/exasol/hooks/test_sql.py`
- `providers/google/tests/system/.../example_cloud_sql_query.py`
- `providers/smtp/src/airflow/providers/smtp/hooks/smtp.py` (OAuth2 auth callback)
- `shared/secrets_masker/.../secrets_masker.py` (subclass compat shim)
Follows the pattern of #66977 (B015), #66978 (PLE1205), #66979 (B008), and #66960 (trigger init/serialize static check).
---
##### Was generative AI tooling used to co-author this PR?
- [X] Yes — Cursor
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enable ruff B023 (function-uses-loop-variable) and fix violations [airflow]
Posted by "dkranchii (via GitHub)" <gi...@apache.org>.
dkranchii commented on code in PR #70640:
URL: https://github.com/apache/airflow/pull/70640#discussion_r3696452471
##########
airflow-core/src/airflow/migrations/versions/0101_3_2_0_ui_improvements_for_deadlines.py:
##########
@@ -594,7 +594,13 @@ def _migrate_deadline_alerts() -> None:
dags_with_deadlines.add(dag_id)
deadline_alerts = dag_deadline if isinstance(dag_deadline, list) else [dag_deadline]
- def _migrate_dag_deadlines(dag_conn: Connection) -> Iterable[str]:
+ def _migrate_dag_deadlines(
Review Comment:
@potiuk Fair point on shipped-migration hygiene. I hadn't thought about the drift between a user's already-applied 3.2.0 source and `main` for the same revision ID. Reverted the function signature to exactly what shipped.
One small deviation from your suggestion: ruff flags B023 at the seven reads of `dag_id` / `serialized_dag_id` / `deadline_alerts` / `created_at` *inside* the closure body, not on the `def` line. So a single `# noqa: B023` on the def doesn't suppress anything, and seven per-line noqas would have added exactly the kind of body-level noise we're trying to avoid in a shipped migration. Went with a file-level `# ruff: noqa: B023` at the top of the module plus a short comment explaining the invariant (closure is invoked and fully drained via `list(_migrate_dag_deadlines(dag_conn))` inside the same iteration). Same intent - historical body byte-identical to 3.2.0, one centralized "why safe" explanation. Happy to switch to per-line noqas if you'd rather have it that way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enable ruff B023 (function-uses-loop-variable) and fix violations [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #70640:
URL: https://github.com/apache/airflow/pull/70640
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enable ruff B023 (function-uses-loop-variable) and fix violations [airflow]
Posted by "dkranchii (via GitHub)" <gi...@apache.org>.
dkranchii commented on code in PR #70640:
URL: https://github.com/apache/airflow/pull/70640#discussion_r3696447987
##########
shared/secrets_masker/src/airflow_shared/secrets_masker/secrets_masker.py:
##########
@@ -223,8 +223,8 @@ def __init_subclass__(cls, **kwargs):
f = cls._redact
@functools.wraps(f)
- def _redact(*args, replacement: str = "***", **kwargs):
- return f(*args, **kwargs)
+ def _redact(*args, replacement: str = "***", _f=f, **kwargs):
Review Comment:
@potiuk Good catch on the signature leak. The bit that pushed it over the edge for me was your `**kwargs` collision point - `_redact` recurses through arbitrary user data (dicts, lists, tuples, sets), and any of those paths that route kwargs through would let a caller sneak `_f` in and substitute a different function *inside* the redaction walk. Not a place I want a sharp edge.
Switched to the factory form you suggested. Kept a short comment at the site so the next person doesn't try to "simplify" it back to a default-arg binding.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enable ruff B023 (function-uses-loop-variable) and fix violations [airflow]
Posted by "dkranchii (via GitHub)" <gi...@apache.org>.
dkranchii commented on code in PR #70640:
URL: https://github.com/apache/airflow/pull/70640#discussion_r3696452471
##########
airflow-core/src/airflow/migrations/versions/0101_3_2_0_ui_improvements_for_deadlines.py:
##########
@@ -594,7 +594,13 @@ def _migrate_deadline_alerts() -> None:
dags_with_deadlines.add(dag_id)
deadline_alerts = dag_deadline if isinstance(dag_deadline, list) else [dag_deadline]
- def _migrate_dag_deadlines(dag_conn: Connection) -> Iterable[str]:
+ def _migrate_dag_deadlines(
Review Comment:
@potiuk Fair point on shipped-migration hygiene. I hadn't thought about the drift between a user's already-applied 3.2.0 source and 'main' for the same revision ID. Reverted the function signature to exactly what shipped.
One small deviation from your suggestion: ruff flags B023 at the seven reads of `dag_id` / `serialized_dag_id` / `deadline_alerts` / `created_at` *inside* the closure body, not on the `def` line. So a single `# noqa: B023` on the def doesn't suppress anything, and seven per-line noqas would have added exactly the kind of body-level noise we're trying to avoid in a shipped migration. Went with a file-level `# ruff: noqa: B023` at the top of the module plus a short comment explaining the invariant (closure is invoked and fully drained via `list(_migrate_dag_deadlines(dag_conn))` inside the same iteration). Same intent - historical body byte-identical to 3.2.0, one centralized "why safe" explanation. Happy to switch to per-line noqas if you'd rather have it that way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enable ruff B023 (function-uses-loop-variable) and fix violations [airflow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #70640:
URL: https://github.com/apache/airflow/pull/70640#issuecomment-5465494812
### Backport successfully created: v3-3-test
Note: As of [Merging PRs targeted for Airflow 3.X](https://github.com/apache/airflow/blob/main/dev/README_AIRFLOW3_DEV.md#merging-prs-targeted-for-airflow-3x)
the committer who merges the PR is responsible for backporting the PRs that are bug fixes (generally speaking) to the maintenance branches.
In matter of doubt please ask in [#release-management](https://apache-airflow.slack.com/archives/C03G9H97MM2) Slack channel.
<table>
<tr>
<th>Status</th>
<th>Branch</th>
<th>Result</th>
</tr>
<tr>
<td>✅</td>
<td>v3-3-test</td>
<td><a href="https://github.com/apache/airflow/pull/72273"><img src="https://img.shields.io/badge/PR-72273-blue" alt="PR Link"></a></td>
</tr>
</table>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enable ruff B023 (function-uses-loop-variable) and fix violations [airflow]
Posted by "dkranchii (via GitHub)" <gi...@apache.org>.
dkranchii commented on code in PR #70640:
URL: https://github.com/apache/airflow/pull/70640#discussion_r3696447987
##########
shared/secrets_masker/src/airflow_shared/secrets_masker/secrets_masker.py:
##########
@@ -223,8 +223,8 @@ def __init_subclass__(cls, **kwargs):
f = cls._redact
@functools.wraps(f)
- def _redact(*args, replacement: str = "***", **kwargs):
- return f(*args, **kwargs)
+ def _redact(*args, replacement: str = "***", _f=f, **kwargs):
Review Comment:
@potiuk Good catch on the signature leak. The bit that pushed it over the edge for me was your `**kwargs` collision point — `_redact` recurses through arbitrary user data (dicts, lists, tuples, sets), and any of those paths that route kwargs through would let a caller sneak `_f` in and substitute a different function *inside* the redaction walk. Not a place I want a sharp edge.
Switched to the factory form you suggested. Kept a short comment at the site so the next person doesn't try to "simplify" it back to a default-arg binding.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Enable ruff B023 (function-uses-loop-variable) and fix violations [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #70640:
URL: https://github.com/apache/airflow/pull/70640#discussion_r3682235286
##########
shared/secrets_masker/src/airflow_shared/secrets_masker/secrets_masker.py:
##########
@@ -223,8 +223,8 @@ def __init_subclass__(cls, **kwargs):
f = cls._redact
@functools.wraps(f)
- def _redact(*args, replacement: str = "***", **kwargs):
- return f(*args, **kwargs)
+ def _redact(*args, replacement: str = "***", _f=f, **kwargs):
Review Comment:
Binding via a default argument leaks `_f` into the function's **keyword signature**, and this one is wrapped in `functools.wraps` — so introspection now advertises a parameter that isn't part of the contract, and a caller who happens to pass `_f` in `**kwargs` would silently substitute a different function. For a secrets-masking code path that's a sharper edge than it looks.
A closure factory avoids exposing anything:
```python
def _make_redact(f):
@functools.wraps(f)
def _redact(*args, replacement: str = "***", **kwargs):
return f(*args, **kwargs)
return _redact
```
Same effect, nothing added to the signature.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
airflow-core/src/airflow/migrations/versions/0101_3_2_0_ui_improvements_for_deadlines.py:
##########
@@ -594,7 +594,13 @@ def _migrate_deadline_alerts() -> None:
dags_with_deadlines.add(dag_id)
deadline_alerts = dag_deadline if isinstance(dag_deadline, list) else [dag_deadline]
- def _migrate_dag_deadlines(dag_conn: Connection) -> Iterable[str]:
+ def _migrate_dag_deadlines(
Review Comment:
Please revert this file and use `# noqa: B023` instead.
This is a **released migration** (0101, shipped in 3.2.0) and the change is semantically a no-op — the closure is called at line 688 inside the same iteration where it's defined here, so no value ever leaked across iterations.
Rewriting a shipped migration to satisfy a linter means a user who already ran it has different source than a fresh install, for zero behavioural gain. A `# noqa: B023` with a one-line comment ("closure is invoked within the same iteration") leaves the historical file intact and documents *why* it's safe, which is more useful to the next reader than five extra default arguments.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org