You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "Dev-iL (via GitHub)" <gi...@apache.org> on 2026/04/09 18:43:36 UTC
[PR] Add static analysis tests for migration anti-patterns [airflow]
Dev-iL opened a new pull request, #64972:
URL: https://github.com/apache/airflow/pull/64972
## Summary
- Add AST-based static analysis tests that detect 3 migration anti-patterns (MIG001/MIG002/MIG003) across all migration files
- Add unit tests for `disable_sqlite_fkeys`, `mysql_drop_foreignkey_if_exists`, and `ignore_sqlite_value_error` with real database backends
- Fix confirmed SQLite PRAGMA bugs in migrations 0100 and 0106
- Add `# noqa: MIG0XX` suppression comments to 20 old migrations with acknowledged violations
related: #64876
## Motivation
PR #64876 fixed a bug in migration 0097 where `UPDATE` statements before the `disable_sqlite_fkeys` context manager triggered SQLAlchemy autobegin, making `PRAGMA foreign_keys=off` a no-op on SQLite. The same pattern existed in migrations 0100 and 0106 but had no automated detection.
This PR adds two layers of protection:
1. **Static analysis tests** (`test_migration_patterns.py`) — AST-based parametrized tests that scan every migration file for known anti-patterns:
- **MIG001**: DML (`UPDATE`/`INSERT`/`DELETE`) via `op.execute()` before `disable_sqlite_fkeys` — triggers autobegin, making PRAGMA a no-op
- **MIG002**: DDL (`op.add_column`, `op.batch_alter_table`, etc.) before `disable_sqlite_fkeys` — same autobegin issue
- **MIG003**: DML via `op.execute()` without `context.is_offline_mode()` guard — produces data-dependent SQL in offline mode
2. **Utils unit tests** (`test_migration_utils.py`) — tests for the 3 most-used migration utility functions using real database connections via `pytest.mark.db_test`.
## Changes
### New files
- `airflow-core/tests/unit/migrations/test_migration_patterns.py` — 336 parametrized tests (3 rules x 112 migration files), with `# noqa: MIG0XX` suppression support and ruff-style rule documentation in the module docstring
- `airflow-core/tests/unit/migrations/test_migration_utils.py` — 8 tests for `disable_sqlite_fkeys` (PRAGMA on/off on SQLite, no-op on others), `mysql_drop_foreignkey_if_exists` (MySQL-only, skipped elsewhere), and `ignore_sqlite_value_error` (ValueError suppression on SQLite)
### Bug fixes
- **Migration 0100** (`add_timetable_type`): moved `batch_alter_table` (add_column) and `op.execute(UPDATE)` inside the `disable_sqlite_fkeys` block — same pattern as the fix in #64876
- **Migration 0106** (`add_partition_key`): moved `op.add_column("dag_run", ...)` inside the `disable_sqlite_fkeys` block
### Suppressions
- Added `# noqa: MIG003 -- <reason>` to 62 DML lines across 22 migration files where the DML is intentional and the migration is designed to run online only
- Added `external = ["MIG"]` to `[tool.ruff.lint]` in `pyproject.toml` to prevent `RUF100` from stripping the custom noqa comments
<!-- SPDX-License-Identifier: Apache-2.0
https://www.apache.org/licenses/LICENSE-2.0 -->
<!--
Thank you for contributing!
Please provide above a brief description of the changes made in this pull request.
Write a good git commit message following this guide: http://chris.beams.io/posts/git-commit/
Please make sure that your code changes are covered with tests.
And in case of new features or big changes remember to adjust the documentation.
Feel free to ping (in general) for the review if you do not see reaction for a few days
(72 Hours is the minimum reaction time you can expect from volunteers) - we sometimes miss notifications.
In case of an existing issue, reference it using one of the following:
* closes: #ISSUE
* related: #ISSUE
-->
---
##### Was generative AI tooling used to co-author this PR?
<!--
If generative AI tooling has been used in the process of authoring this PR, please
change below checkbox to `[X]` followed by the name of the tool, uncomment the "Generated-by".
-->
- [x] Yes (please specify the tool below)
Generated-by: Claude Opus 4.6 following [the guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
---
* Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
* For fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
* When adding dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
* For significant user-facing changes create newsfragment: `{pr_number}.significant.rst`, in [airflow-core/newsfragments](https://github.com/apache/airflow/tree/main/airflow-core/newsfragments). You can add this file in a follow-up commit after the PR is created so you know the PR number.
--
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] Add static analysis tests for DB migration anti-patterns [airflow]
Posted by "Dev-iL (via GitHub)" <gi...@apache.org>.
Dev-iL commented on code in PR #64972:
URL: https://github.com/apache/airflow/pull/64972#discussion_r3154902303
##########
airflow-core/tests/unit/migrations/test_migration_patterns.py:
##########
@@ -0,0 +1,71 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Pytest harness for migration anti-pattern checks (MIG001/MIG002/MIG003)."""
+
+from __future__ import annotations
+
+import importlib.util
+from pathlib import Path
+
+import pytest
+
+# ---------------------------------------------------------------------------
+# Import check functions from the prek hook script (single source of truth).
+# ---------------------------------------------------------------------------
+_PREK_SCRIPT = Path(__file__).resolve().parents[4] / "scripts" / "ci" / "prek" / "check_migration_patterns.py"
+_spec = importlib.util.spec_from_file_location("check_migration_patterns", _PREK_SCRIPT)
+if _spec is None or _spec.loader is None:
+ raise ImportError(f"Could not load prek script: {_PREK_SCRIPT}")
+_mod = importlib.util.module_from_spec(_spec)
+_spec.loader.exec_module(_mod) # type: ignore[union-attr] # typeshed: Loader lacks exec_module
+check_mig001 = _mod.check_mig001
+check_mig002 = _mod.check_mig002
+check_mig003 = _mod.check_mig003
+
+MIGRATIONS_DIR = Path(__file__).resolve().parents[3] / "src" / "airflow" / "migrations" / "versions"
+
+
+def _migration_files() -> list[Path]:
Review Comment:
It's used at `@pytest.mark.parametrize` collection time so can't be a fixture. Did drop the leading underscore.
--
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] Add static analysis tests for DB migration anti-patterns [airflow]
Posted by "Dev-iL (via GitHub)" <gi...@apache.org>.
Dev-iL commented on code in PR #64972:
URL: https://github.com/apache/airflow/pull/64972#discussion_r3154925007
##########
scripts/tests/ci/prek/test_check_migration_patterns.py:
##########
@@ -0,0 +1,333 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Unit tests for ``scripts/ci/prek/check_migration_patterns.py``."""
+
+from __future__ import annotations
+
+import sys
+from pathlib import Path
+
+from ci.prek.check_migration_patterns import (
+ _get_noqa_codes,
+ _line_has_noqa,
+ check_mig001,
+ check_mig002,
+ check_mig003,
+ main,
+)
+
+# ---------------------------------------------------------------------------
+# Helpers
+# ---------------------------------------------------------------------------
+
+
+def write_migration(tmp_path: Path, source: str) -> Path:
+ """Write *source* to a temp file that looks like a migration file."""
+ p = tmp_path / "0099_test_migration.py"
+ p.write_text(source)
+ return p
+
+
+# ---------------------------------------------------------------------------
+# _get_noqa_codes
+# ---------------------------------------------------------------------------
+
+
+class TestGetNoqaCodes:
+ def test_single_code(self):
+ assert _get_noqa_codes("op.execute(...) # noqa: MIG001") == {"MIG001"}
+
+ def test_multiple_codes(self):
+ assert _get_noqa_codes("op.execute(...) # noqa: MIG001, MIG003") == {"MIG001", "MIG003"}
+
+ def test_code_with_reason_suffix(self):
+ # Reason after '--' should NOT be included in the code set.
+ codes = _get_noqa_codes("op.execute(...) # noqa: MIG003 -- safe in offline mode")
+ assert "MIG003" in codes
+ assert "--" not in "".join(codes)
+ assert "safe" not in "".join(codes)
+
+ def test_no_noqa_comment(self):
+ assert _get_noqa_codes("op.execute('UPDATE foo SET x=1')") == set()
+
+ def test_empty_string(self):
+ assert _get_noqa_codes("") == set()
+
+
+# ---------------------------------------------------------------------------
+# _line_has_noqa
+# ---------------------------------------------------------------------------
+
+
+class TestLineHasNoqa:
+ def test_line_has_matching_code(self):
+ lines = ["op.execute('UPDATE ...') # noqa: MIG001"]
+ assert _line_has_noqa(lines, 1, "MIG001") is True
+
+ def test_line_has_different_code(self):
+ lines = ["op.execute('UPDATE ...') # noqa: MIG002"]
+ assert _line_has_noqa(lines, 1, "MIG001") is False
+
+ def test_out_of_bounds_zero(self):
+ assert _line_has_noqa(["some line"], 0, "MIG001") is False
+
+ def test_out_of_bounds_beyond_end(self):
+ assert _line_has_noqa(["some line"], 2, "MIG001") is False
+
+
+# ---------------------------------------------------------------------------
+# check_mig001
+# ---------------------------------------------------------------------------
+
+
+class TestCheckMig001:
Review Comment:
reorganized/grouped/parameterized
--
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] Add static analysis tests for DB migration anti-patterns [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #64972:
URL: https://github.com/apache/airflow/pull/64972#discussion_r3062656807
##########
scripts/ci/prek/check_migration_patterns.py:
##########
@@ -0,0 +1,384 @@
+#!/usr/bin/env python
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""
+Static analysis checks for Alembic migration anti-patterns.
+
+These checks use AST analysis to detect common migration mistakes that can cause
+silent failures, particularly on SQLite where ``PRAGMA foreign_keys`` has specific
+requirements about transaction state.
+
+TODO: These checks are candidates for future Ruff rules (AIR category).
+
+Rules
+=====
+
+MIG001 -- DML before ``disable_sqlite_fkeys``
+----------------------------------------------
+
+**What it does:**
+Detects ``op.execute()`` calls containing DML keywords (UPDATE, INSERT, DELETE) that
+appear before a ``with disable_sqlite_fkeys(op):`` block in the same function.
+
+**Why is this bad:**
+On SQLite, any DML statement triggers *autobegin*, starting a transaction. Once a
+transaction is active, ``PRAGMA foreign_keys=off`` (issued by ``disable_sqlite_fkeys``)
+becomes a no-op. This means foreign key checks remain enabled, and
+``batch_alter_table`` (which recreates tables) may fail with foreign key constraint
+violations.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL") # triggers autobegin
+ with disable_sqlite_fkeys(op): # PRAGMA is now a no-op!
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ with disable_sqlite_fkeys(op):
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+
+MIG002 -- DDL before ``disable_sqlite_fkeys``
+----------------------------------------------
+
+**What it does:**
+Detects any Alembic ``op.*`` call (DDL operations like ``op.add_column``,
+``op.drop_column``, ``op.create_table``, ``op.drop_table``, ``op.create_index``,
+``op.batch_alter_table``, etc.) that appears before a
+``with disable_sqlite_fkeys(op):`` block in the same function. Excludes DML calls
+covered by MIG001.
+
+**Why is this bad:**
+DDL operations also trigger *autobegin* on SQLite, making the subsequent
+``PRAGMA foreign_keys=off`` a no-op, exactly like DML.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.add_column("dag_run", sa.Column("created_at", ...)) # triggers autobegin
+ with disable_sqlite_fkeys(op): # PRAGMA is now a no-op!
+ with op.batch_alter_table("backfill") as batch_op:
+ batch_op.alter_column("col", nullable=True)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ with disable_sqlite_fkeys(op):
+ op.add_column("dag_run", sa.Column("created_at", ...))
+ with op.batch_alter_table("backfill") as batch_op:
+ batch_op.alter_column("col", nullable=True)
+
+
+MIG003 -- DML without offline-mode guard
Review Comment:
It seems we have lots and lots of exception for this one 🤔 is that expected?
##########
scripts/ci/prek/check_migration_patterns.py:
##########
@@ -0,0 +1,384 @@
+#!/usr/bin/env python
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""
+Static analysis checks for Alembic migration anti-patterns.
+
+These checks use AST analysis to detect common migration mistakes that can cause
+silent failures, particularly on SQLite where ``PRAGMA foreign_keys`` has specific
+requirements about transaction state.
+
+TODO: These checks are candidates for future Ruff rules (AIR category).
+
+Rules
+=====
+
+MIG001 -- DML before ``disable_sqlite_fkeys``
+----------------------------------------------
+
+**What it does:**
+Detects ``op.execute()`` calls containing DML keywords (UPDATE, INSERT, DELETE) that
+appear before a ``with disable_sqlite_fkeys(op):`` block in the same function.
+
+**Why is this bad:**
+On SQLite, any DML statement triggers *autobegin*, starting a transaction. Once a
+transaction is active, ``PRAGMA foreign_keys=off`` (issued by ``disable_sqlite_fkeys``)
+becomes a no-op. This means foreign key checks remain enabled, and
+``batch_alter_table`` (which recreates tables) may fail with foreign key constraint
+violations.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL") # triggers autobegin
+ with disable_sqlite_fkeys(op): # PRAGMA is now a no-op!
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ with disable_sqlite_fkeys(op):
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+
+MIG002 -- DDL before ``disable_sqlite_fkeys``
+----------------------------------------------
+
+**What it does:**
+Detects any Alembic ``op.*`` call (DDL operations like ``op.add_column``,
+``op.drop_column``, ``op.create_table``, ``op.drop_table``, ``op.create_index``,
+``op.batch_alter_table``, etc.) that appears before a
+``with disable_sqlite_fkeys(op):`` block in the same function. Excludes DML calls
+covered by MIG001.
+
+**Why is this bad:**
+DDL operations also trigger *autobegin* on SQLite, making the subsequent
+``PRAGMA foreign_keys=off`` a no-op, exactly like DML.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.add_column("dag_run", sa.Column("created_at", ...)) # triggers autobegin
+ with disable_sqlite_fkeys(op): # PRAGMA is now a no-op!
+ with op.batch_alter_table("backfill") as batch_op:
+ batch_op.alter_column("col", nullable=True)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ with disable_sqlite_fkeys(op):
+ op.add_column("dag_run", sa.Column("created_at", ...))
+ with op.batch_alter_table("backfill") as batch_op:
+ batch_op.alter_column("col", nullable=True)
+
+
+MIG003 -- DML without offline-mode guard
+------------------------------------------
+
+**What it does:**
+Detects ``op.execute()`` calls containing DML keywords (UPDATE, INSERT, DELETE) in
+``upgrade()`` or ``downgrade()`` functions that do not contain a
+``context.is_offline_mode()`` check anywhere in the same function.
+
+**Why is this bad:**
+Alembic's offline mode generates SQL scripts instead of executing against a live
+database. DML statements like ``UPDATE ... WHERE col IS NULL`` depend on actual data
+and produce no useful output in offline mode. Without an ``is_offline_mode()`` guard,
+the generated script includes DML that may fail or silently do nothing when applied
+later.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ if not context.is_offline_mode():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+
+Known Limitation -- Future MIG004 Candidate
+---------------------------------------------
+
+Migrations that perform DML and use ``batch_alter_table`` but never call
+``disable_sqlite_fkeys`` at all are NOT detected by MIG001/MIG002. These migrations
+may silently break SQLite foreign key handling. Detecting this pattern requires
+understanding whether foreign key relationships exist on the affected tables, which
+is beyond AST-level analysis. This is tracked as a future MIG004 candidate.
+
+
+Suppression
+===========
+
+Add ``# noqa: MIG0XX`` to a line to suppress the corresponding check for that line.
+Multiple codes can be comma-separated: ``# noqa: MIG001, MIG003``.
+
+Include a brief reason after ``--``::
+
+ # noqa: MIG003 -- simple UPDATE safe in offline mode
+"""
+
+from __future__ import annotations
+
+import ast
+import re
+import sys
+from pathlib import Path
+
+DML_KEYWORDS = ("UPDATE ", "INSERT ", "DELETE ")
+
+
+def _get_noqa_codes(line: str) -> set[str]:
+ """Extract noqa codes from a source line."""
+ match = re.search(r"#\s*noqa:\s*([\w,\s]+)", line)
+ if match:
+ return {code.strip() for code in match.group(1).split(",")}
+ return set()
+
+
+def _is_dml_string(node: ast.expr) -> bool:
+ """Check if an AST expression is a string literal starting with a DML keyword."""
+ if isinstance(node, ast.Constant) and isinstance(node.value, str):
+ stripped = node.value.strip().upper()
+ return any(stripped.startswith(kw) for kw in DML_KEYWORDS)
+ # f-strings: check the first string fragment
+ if isinstance(node, ast.JoinedStr):
+ for value in node.values:
+ if isinstance(value, ast.Constant) and isinstance(value.value, str):
+ stripped = value.value.strip().upper()
+ if any(stripped.startswith(kw) for kw in DML_KEYWORDS):
+ return True
+ break # only the leading fragment matters
+ return False
+
+
+def _is_op_execute_with_dml(node: ast.Call) -> bool:
+ """Check if a Call node is ``op.execute(<DML string>)``."""
+ if not (
+ isinstance(node.func, ast.Attribute)
+ and isinstance(node.func.value, ast.Name)
+ and node.func.value.id == "op"
+ and node.func.attr == "execute"
+ ):
+ return False
+ if not node.args:
+ return False
+ arg = node.args[0]
+ # Direct string: op.execute("UPDATE ...")
+ if _is_dml_string(arg):
+ return True
+ # Wrapped: op.execute(text("UPDATE ...")) or op.execute(sa.text("UPDATE ..."))
+ if isinstance(arg, ast.Call) and arg.args:
+ return _is_dml_string(arg.args[0])
+ return False
+
+
+def _is_op_call(node: ast.Call) -> bool:
+ """Check if a Call node is ``op.<something>(...)``."""
+ return (
+ isinstance(node.func, ast.Attribute)
+ and isinstance(node.func.value, ast.Name)
+ and node.func.value.id == "op"
+ )
+
+
+def _find_disable_sqlite_fkeys_line(func_node: ast.FunctionDef) -> int | None:
+ """Find the line number of the first ``with disable_sqlite_fkeys(op):`` in a function."""
+ for node in ast.walk(func_node):
+ if not isinstance(node, ast.With):
+ continue
+ for item in node.items:
+ ctx = item.context_expr
+ if isinstance(ctx, ast.Call):
+ func = ctx.func
+ if (isinstance(func, ast.Name) and func.id == "disable_sqlite_fkeys") or (
+ isinstance(func, ast.Attribute) and func.attr == "disable_sqlite_fkeys"
+ ):
+ return node.lineno
+ return None
+
+
+def _has_offline_mode_check(func_node: ast.FunctionDef) -> bool:
+ """Check if function contains ``context.is_offline_mode()`` call."""
+ for node in ast.walk(func_node):
+ if isinstance(node, ast.Call):
+ func = node.func
+ if isinstance(func, ast.Attribute) and func.attr == "is_offline_mode":
+ return True
+ return False
+
+
+def _get_upgrade_downgrade_functions(tree: ast.Module) -> list[ast.FunctionDef]:
+ """Get ``upgrade()`` and ``downgrade()`` function definitions from module."""
+ return [
+ node
+ for node in ast.iter_child_nodes(tree)
+ if isinstance(node, ast.FunctionDef) and node.name in ("upgrade", "downgrade")
+ ]
+
+
+def _line_has_noqa(lines: list[str], lineno: int, code: str) -> bool:
+ """Check if a source line has a noqa suppression for the given code."""
+ if lineno < 1 or lineno > len(lines):
+ return False
+ return code in _get_noqa_codes(lines[lineno - 1])
+
+
+# ---------------------------------------------------------------------------
+# Check functions
+# ---------------------------------------------------------------------------
+
+
+def check_mig001(filepath: Path) -> list[str]:
+ """MIG001: DML before ``disable_sqlite_fkeys``."""
+ source = filepath.read_text()
+ lines = source.splitlines()
+ tree = ast.parse(source, filename=str(filepath))
+ errors: list[str] = []
+
+ for func in _get_upgrade_downgrade_functions(tree):
+ guard_line = _find_disable_sqlite_fkeys_line(func)
+ if guard_line is None:
+ continue # MIG001 only fires when disable_sqlite_fkeys IS present
+
+ for node in ast.walk(func):
+ if not isinstance(node, ast.Call):
+ continue
+ if node.lineno >= guard_line:
+ continue
+ if _is_op_execute_with_dml(node) and not _line_has_noqa(lines, node.lineno, "MIG001"):
+ errors.append(
+ f"MIG001 {func.name}(): op.execute() with DML at line {node.lineno} "
+ f"before disable_sqlite_fkeys at line {guard_line}"
+ )
+
+ return errors
+
+
+def check_mig002(filepath: Path) -> list[str]:
+ """MIG002: DDL before ``disable_sqlite_fkeys``."""
+ source = filepath.read_text()
+ lines = source.splitlines()
+ tree = ast.parse(source, filename=str(filepath))
+ errors: list[str] = []
+
+ for func in _get_upgrade_downgrade_functions(tree):
+ guard_line = _find_disable_sqlite_fkeys_line(func)
+ if guard_line is None:
+ continue # MIG002 only fires when disable_sqlite_fkeys IS present
+
+ for node in ast.walk(func):
+ if not isinstance(node, ast.Call):
+ continue
+ if node.lineno >= guard_line:
+ continue
+ # Skip DML calls — covered by MIG001
+ if _is_op_execute_with_dml(node):
+ continue
+ if _is_op_call(node) and not _line_has_noqa(lines, node.lineno, "MIG002"):
+ attr = node.func.attr if isinstance(node.func, ast.Attribute) else "?"
+ errors.append(
+ f"MIG002 {func.name}(): op.{attr}() at line {node.lineno} "
+ f"before disable_sqlite_fkeys at line {guard_line}"
+ )
+
+ return errors
+
+
+def check_mig003(filepath: Path) -> list[str]:
+ """MIG003: DML without offline-mode guard."""
+ source = filepath.read_text()
+ lines = source.splitlines()
+ tree = ast.parse(source, filename=str(filepath))
+ errors: list[str] = []
+
+ for func in _get_upgrade_downgrade_functions(tree):
+ if _has_offline_mode_check(func):
+ continue # function has the guard
+
+ for node in ast.walk(func):
+ if not isinstance(node, ast.Call):
+ continue
+ if _is_op_execute_with_dml(node) and not _line_has_noqa(lines, node.lineno, "MIG003"):
+ errors.append(
+ f"MIG003 {func.name}(): op.execute() with DML at line {node.lineno} "
+ f"without context.is_offline_mode() guard"
+ )
+
+ return errors
+
+
+# ---------------------------------------------------------------------------
+# CLI entry point
+# ---------------------------------------------------------------------------
+
+
+def main() -> int:
+ """Check migration files for anti-patterns. Exit 0 if clean, 1 if violations found."""
+ checks = [check_mig001, check_mig002, check_mig003]
+ has_errors = False
+
+ for filepath_str in sys.argv[1:]:
+ filepath = Path(filepath_str)
+ try:
+ source = filepath.read_text()
+ except OSError as exc:
+ print(f"{filepath}: {exc}", file=sys.stderr)
Review Comment:
nit: rich give us better TUI
##########
scripts/ci/prek/check_migration_patterns.py:
##########
@@ -0,0 +1,384 @@
+#!/usr/bin/env python
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""
+Static analysis checks for Alembic migration anti-patterns.
+
+These checks use AST analysis to detect common migration mistakes that can cause
+silent failures, particularly on SQLite where ``PRAGMA foreign_keys`` has specific
+requirements about transaction state.
+
+TODO: These checks are candidates for future Ruff rules (AIR category).
Review Comment:
Probably not? AIR rules are more for Airflow users instead of Airflow itself
--
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] Add static analysis tests for DB migration anti-patterns [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #64972:
URL: https://github.com/apache/airflow/pull/64972#discussion_r3067511610
##########
airflow-core/tests/unit/migrations/test_migration_utils.py:
##########
@@ -0,0 +1,183 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Unit tests for ``airflow.migrations.utils``."""
+
+from __future__ import annotations
+
+import pytest
+from alembic.migration import MigrationContext
+from alembic.operations import Operations
+from sqlalchemy import text
+
+from airflow import settings
+from airflow.migrations.utils import (
+ disable_sqlite_fkeys,
+ ignore_sqlite_value_error,
+ mysql_drop_foreignkey_if_exists,
+)
+
+pytestmark = pytest.mark.db_test
+
+
+def _dialect() -> str:
+ assert settings.engine is not None # guaranteed by db_test marker
+ return settings.engine.dialect.name
+
+
+class TestDisableSqliteFkeys:
+ """Tests for :func:`disable_sqlite_fkeys`."""
+
+ def test_yields_op(self):
+ """The context manager yields the *op* object on every backend."""
+ with settings.engine.connect() as conn:
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+ with disable_sqlite_fkeys(op) as yielded:
+ assert yielded is op
+
+ def test_sqlite_turns_off_fkeys(self):
+ """On SQLite, ``PRAGMA foreign_keys`` is OFF inside the context."""
+ if _dialect() != "sqlite":
+ pytest.skip("SQLite-specific test")
Review Comment:
sounds good :)
--
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] Add static analysis tests for DB migration anti-patterns [airflow]
Posted by "Dev-iL (via GitHub)" <gi...@apache.org>.
Dev-iL commented on PR #64972:
URL: https://github.com/apache/airflow/pull/64972#issuecomment-4286394416
@Lee-W I wonder if the change in #65545 is covered by this PR
--
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] Add static analysis tests for DB migration anti-patterns [airflow]
Posted by "Dev-iL (via GitHub)" <gi...@apache.org>.
Dev-iL commented on code in PR #64972:
URL: https://github.com/apache/airflow/pull/64972#discussion_r3069066644
##########
scripts/ci/prek/check_migration_patterns.py:
##########
@@ -0,0 +1,383 @@
+#!/usr/bin/env python
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# /// script
+# requires-python = ">=3.10,<3.11"
+# dependencies = [
+# "rich>=13.6.0",
+# ]
+# ///
+"""
+Static analysis checks for Alembic migration anti-patterns.
+
+These checks use AST analysis to detect common migration mistakes that can cause
+silent failures, particularly on SQLite where ``PRAGMA foreign_keys`` has specific
+requirements about transaction state.
+
+Rules
+=====
+
+MIG001 -- DML before ``disable_sqlite_fkeys``
+----------------------------------------------
+
+**What it does:**
+Detects ``op.execute()`` calls containing DML keywords (UPDATE, INSERT, DELETE) that
+appear before a ``with disable_sqlite_fkeys(op):`` block in the same function.
+
+**Why is this bad:**
+On SQLite, any DML statement triggers *autobegin*, starting a transaction. Once a
+transaction is active, ``PRAGMA foreign_keys=off`` (issued by ``disable_sqlite_fkeys``)
+becomes a no-op. This means foreign key checks remain enabled, and
+``batch_alter_table`` (which recreates tables) may fail with foreign key constraint
+violations.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL") # triggers autobegin
+ with disable_sqlite_fkeys(op): # PRAGMA is now a no-op!
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ with disable_sqlite_fkeys(op):
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+
+MIG002 -- DDL before ``disable_sqlite_fkeys``
+----------------------------------------------
+
+**What it does:**
+Detects any Alembic ``op.*`` call (DDL operations like ``op.add_column``,
+``op.drop_column``, ``op.create_table``, ``op.drop_table``, ``op.create_index``,
+``op.batch_alter_table``, etc.) that appears before a
+``with disable_sqlite_fkeys(op):`` block in the same function. Excludes DML calls
+covered by MIG001.
+
+**Why is this bad:**
+DDL operations also trigger *autobegin* on SQLite, making the subsequent
+``PRAGMA foreign_keys=off`` a no-op, exactly like DML.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.add_column("dag_run", sa.Column("created_at", ...)) # triggers autobegin
+ with disable_sqlite_fkeys(op): # PRAGMA is now a no-op!
+ with op.batch_alter_table("backfill") as batch_op:
+ batch_op.alter_column("col", nullable=True)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ with disable_sqlite_fkeys(op):
+ op.add_column("dag_run", sa.Column("created_at", ...))
+ with op.batch_alter_table("backfill") as batch_op:
+ batch_op.alter_column("col", nullable=True)
+
+
+MIG003 -- DML without offline-mode guard
+------------------------------------------
+
+**What it does:**
+Detects ``op.execute()`` calls containing DML keywords (UPDATE, INSERT, DELETE) in
+``upgrade()`` or ``downgrade()`` functions that do not contain a
+``context.is_offline_mode()`` check anywhere in the same function.
+
+**Why is this bad:**
+Alembic's offline mode generates SQL scripts instead of executing against a live
+database. DML statements like ``UPDATE ... WHERE col IS NULL`` depend on actual data
+and produce no useful output in offline mode. Without an ``is_offline_mode()`` guard,
+the generated script includes DML that may fail or silently do nothing when applied
+later.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ if not context.is_offline_mode():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+
+Known Limitation -- Future MIG004 Candidate
+---------------------------------------------
+
+Migrations that perform DML and use ``batch_alter_table`` but never call
+``disable_sqlite_fkeys`` at all are NOT detected by MIG001/MIG002. These migrations
+may silently break SQLite foreign key handling. Detecting this pattern requires
+understanding whether foreign key relationships exist on the affected tables, which
+is beyond AST-level analysis. This is tracked as a future MIG004 candidate.
+
+
+Suppression
+===========
+
+Add ``# noqa: MIG0XX`` to a line to suppress the corresponding check for that line.
+Multiple codes can be comma-separated: ``# noqa: MIG001, MIG003``.
+
+Include a brief reason after ``--``::
+
+ # noqa: MIG003 -- simple UPDATE safe in offline mode
+"""
+
+from __future__ import annotations
+
+import ast
+import re
+import sys
+from pathlib import Path
+
+from rich.console import Console
+
+console = Console(color_system="standard", width=200)
+
+DML_KEYWORDS = ("UPDATE ", "INSERT ", "DELETE ")
+
+
+def _get_noqa_codes(line: str) -> set[str]:
+ """Extract noqa codes from a source line."""
+ match = re.search(r"#\s*noqa:\s*([\w,\s]+)", line)
+ if match:
+ return {code.strip() for code in match.group(1).split(",")}
+ return set()
+
+
+def _is_dml_string(node: ast.expr) -> bool:
+ """Check if an AST expression is a string literal starting with a DML keyword."""
+ if isinstance(node, ast.Constant) and isinstance(node.value, str):
+ stripped = node.value.strip().upper()
+ return any(stripped.startswith(kw) for kw in DML_KEYWORDS)
+ # f-strings: check the first string fragment
+ if isinstance(node, ast.JoinedStr):
+ for value in node.values:
+ if isinstance(value, ast.Constant) and isinstance(value.value, str):
+ stripped = value.value.strip().upper()
+ if any(stripped.startswith(kw) for kw in DML_KEYWORDS):
+ return True
+ break # only the leading fragment matters
+ return False
+
+
+def _is_op_execute_with_dml(node: ast.Call) -> bool:
+ """Check if a Call node is ``op.execute(<DML string>)``."""
+ if not (
+ isinstance(node.func, ast.Attribute)
+ and isinstance(node.func.value, ast.Name)
+ and node.func.value.id == "op"
+ and node.func.attr == "execute"
+ ):
+ return False
+ if not node.args:
+ return False
+ arg = node.args[0]
+ # Direct string: op.execute("UPDATE ...")
+ if _is_dml_string(arg):
+ return True
+ # Wrapped: op.execute(text("UPDATE ...")) or op.execute(sa.text("UPDATE ..."))
+ if isinstance(arg, ast.Call) and arg.args:
+ return _is_dml_string(arg.args[0])
+ return False
+
+
+def _is_op_call(node: ast.Call) -> bool:
+ """Check if a Call node is ``op.<something>(...)``."""
+ return (
+ isinstance(node.func, ast.Attribute)
+ and isinstance(node.func.value, ast.Name)
+ and node.func.value.id == "op"
+ )
Review Comment:
**Dismiss.** Investigated — zero migration files in the current codebase use non-DDL `op.*` calls (like `op.get_bind()`, `op.get_context()`) before `disable_sqlite_fkeys`. Verified with: `grep -r 'op\.get_bind\|op\.get_context' airflow-core/src/airflow/migrations/versions/` → no hits in any function that also calls `disable_sqlite_fkeys`. If a future migration introduces such a pattern, the false positive would be a safe-side alert rather than a missed violation.
##########
scripts/ci/prek/check_migration_patterns.py:
##########
@@ -0,0 +1,383 @@
+#!/usr/bin/env python
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# /// script
+# requires-python = ">=3.10,<3.11"
+# dependencies = [
+# "rich>=13.6.0",
+# ]
+# ///
+"""
+Static analysis checks for Alembic migration anti-patterns.
+
+These checks use AST analysis to detect common migration mistakes that can cause
+silent failures, particularly on SQLite where ``PRAGMA foreign_keys`` has specific
+requirements about transaction state.
+
+Rules
+=====
+
+MIG001 -- DML before ``disable_sqlite_fkeys``
+----------------------------------------------
+
+**What it does:**
+Detects ``op.execute()`` calls containing DML keywords (UPDATE, INSERT, DELETE) that
+appear before a ``with disable_sqlite_fkeys(op):`` block in the same function.
+
+**Why is this bad:**
+On SQLite, any DML statement triggers *autobegin*, starting a transaction. Once a
+transaction is active, ``PRAGMA foreign_keys=off`` (issued by ``disable_sqlite_fkeys``)
+becomes a no-op. This means foreign key checks remain enabled, and
+``batch_alter_table`` (which recreates tables) may fail with foreign key constraint
+violations.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL") # triggers autobegin
+ with disable_sqlite_fkeys(op): # PRAGMA is now a no-op!
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ with disable_sqlite_fkeys(op):
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+
+MIG002 -- DDL before ``disable_sqlite_fkeys``
+----------------------------------------------
+
+**What it does:**
+Detects any Alembic ``op.*`` call (DDL operations like ``op.add_column``,
+``op.drop_column``, ``op.create_table``, ``op.drop_table``, ``op.create_index``,
+``op.batch_alter_table``, etc.) that appears before a
+``with disable_sqlite_fkeys(op):`` block in the same function. Excludes DML calls
+covered by MIG001.
+
+**Why is this bad:**
+DDL operations also trigger *autobegin* on SQLite, making the subsequent
+``PRAGMA foreign_keys=off`` a no-op, exactly like DML.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.add_column("dag_run", sa.Column("created_at", ...)) # triggers autobegin
+ with disable_sqlite_fkeys(op): # PRAGMA is now a no-op!
+ with op.batch_alter_table("backfill") as batch_op:
+ batch_op.alter_column("col", nullable=True)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ with disable_sqlite_fkeys(op):
+ op.add_column("dag_run", sa.Column("created_at", ...))
+ with op.batch_alter_table("backfill") as batch_op:
+ batch_op.alter_column("col", nullable=True)
+
+
+MIG003 -- DML without offline-mode guard
+------------------------------------------
+
+**What it does:**
+Detects ``op.execute()`` calls containing DML keywords (UPDATE, INSERT, DELETE) in
+``upgrade()`` or ``downgrade()`` functions that do not contain a
+``context.is_offline_mode()`` check anywhere in the same function.
+
+**Why is this bad:**
+Alembic's offline mode generates SQL scripts instead of executing against a live
+database. DML statements like ``UPDATE ... WHERE col IS NULL`` depend on actual data
+and produce no useful output in offline mode. Without an ``is_offline_mode()`` guard,
+the generated script includes DML that may fail or silently do nothing when applied
+later.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ if not context.is_offline_mode():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+
+Known Limitation -- Future MIG004 Candidate
+---------------------------------------------
+
+Migrations that perform DML and use ``batch_alter_table`` but never call
+``disable_sqlite_fkeys`` at all are NOT detected by MIG001/MIG002. These migrations
+may silently break SQLite foreign key handling. Detecting this pattern requires
+understanding whether foreign key relationships exist on the affected tables, which
+is beyond AST-level analysis. This is tracked as a future MIG004 candidate.
+
+
+Suppression
+===========
+
+Add ``# noqa: MIG0XX`` to a line to suppress the corresponding check for that line.
+Multiple codes can be comma-separated: ``# noqa: MIG001, MIG003``.
+
+Include a brief reason after ``--``::
+
+ # noqa: MIG003 -- simple UPDATE safe in offline mode
+"""
+
+from __future__ import annotations
+
+import ast
+import re
+import sys
+from pathlib import Path
+
+from rich.console import Console
+
+console = Console(color_system="standard", width=200)
+
+DML_KEYWORDS = ("UPDATE ", "INSERT ", "DELETE ")
+
+
+def _get_noqa_codes(line: str) -> set[str]:
+ """Extract noqa codes from a source line."""
+ match = re.search(r"#\s*noqa:\s*([\w,\s]+)", line)
+ if match:
+ return {code.strip() for code in match.group(1).split(",")}
+ return set()
+
+
+def _is_dml_string(node: ast.expr) -> bool:
+ """Check if an AST expression is a string literal starting with a DML keyword."""
+ if isinstance(node, ast.Constant) and isinstance(node.value, str):
+ stripped = node.value.strip().upper()
+ return any(stripped.startswith(kw) for kw in DML_KEYWORDS)
+ # f-strings: check the first string fragment
+ if isinstance(node, ast.JoinedStr):
+ for value in node.values:
+ if isinstance(value, ast.Constant) and isinstance(value.value, str):
+ stripped = value.value.strip().upper()
+ if any(stripped.startswith(kw) for kw in DML_KEYWORDS):
+ return True
+ break # only the leading fragment matters
+ return False
+
+
+def _is_op_execute_with_dml(node: ast.Call) -> bool:
+ """Check if a Call node is ``op.execute(<DML string>)``."""
+ if not (
+ isinstance(node.func, ast.Attribute)
+ and isinstance(node.func.value, ast.Name)
+ and node.func.value.id == "op"
+ and node.func.attr == "execute"
+ ):
+ return False
+ if not node.args:
+ return False
+ arg = node.args[0]
+ # Direct string: op.execute("UPDATE ...")
+ if _is_dml_string(arg):
+ return True
+ # Wrapped: op.execute(text("UPDATE ...")) or op.execute(sa.text("UPDATE ..."))
+ if isinstance(arg, ast.Call) and arg.args:
+ return _is_dml_string(arg.args[0])
+ return False
+
+
+def _is_op_call(node: ast.Call) -> bool:
+ """Check if a Call node is ``op.<something>(...)``."""
+ return (
+ isinstance(node.func, ast.Attribute)
+ and isinstance(node.func.value, ast.Name)
+ and node.func.value.id == "op"
+ )
+
+
+def _find_disable_sqlite_fkeys_line(func_node: ast.FunctionDef) -> int | None:
+ """Find the line number of the first ``with disable_sqlite_fkeys(op):`` in a function."""
+ # Collect all matches then take min() — ast.walk BFS order does not guarantee
+ # line-number order when guards appear at different nesting depths.
+ found: list[int] = []
+ for node in ast.walk(func_node):
+ if not isinstance(node, ast.With):
+ continue
+ for item in node.items:
+ ctx = item.context_expr
+ if isinstance(ctx, ast.Call):
+ func = ctx.func
+ if (isinstance(func, ast.Name) and func.id == "disable_sqlite_fkeys") or (
+ isinstance(func, ast.Attribute) and func.attr == "disable_sqlite_fkeys"
+ ):
+ found.append(node.lineno)
+ return min(found) if found else None
Review Comment:
**Dismiss.** Valid edge case in theory, but all existing Airflow migrations use `with disable_sqlite_fkeys(op):` as a standalone `with` statement — none combine it with `op.batch_alter_table(...)` in a multi-context-manager `with` on the same line. Verified with: `grep -n 'with.*disable_sqlite_fkeys.*batch_alter_table' airflow-core/src/airflow/migrations/versions/*.py` → zero matches. The lineno comparison is sufficient for actual codebase patterns.
##########
scripts/ci/prek/check_migration_patterns.py:
##########
@@ -0,0 +1,383 @@
+#!/usr/bin/env python
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# /// script
+# requires-python = ">=3.10,<3.11"
+# dependencies = [
+# "rich>=13.6.0",
+# ]
+# ///
+"""
+Static analysis checks for Alembic migration anti-patterns.
+
+These checks use AST analysis to detect common migration mistakes that can cause
+silent failures, particularly on SQLite where ``PRAGMA foreign_keys`` has specific
+requirements about transaction state.
+
+Rules
+=====
+
+MIG001 -- DML before ``disable_sqlite_fkeys``
+----------------------------------------------
+
+**What it does:**
+Detects ``op.execute()`` calls containing DML keywords (UPDATE, INSERT, DELETE) that
+appear before a ``with disable_sqlite_fkeys(op):`` block in the same function.
+
+**Why is this bad:**
+On SQLite, any DML statement triggers *autobegin*, starting a transaction. Once a
+transaction is active, ``PRAGMA foreign_keys=off`` (issued by ``disable_sqlite_fkeys``)
+becomes a no-op. This means foreign key checks remain enabled, and
+``batch_alter_table`` (which recreates tables) may fail with foreign key constraint
+violations.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL") # triggers autobegin
+ with disable_sqlite_fkeys(op): # PRAGMA is now a no-op!
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ with disable_sqlite_fkeys(op):
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+
+MIG002 -- DDL before ``disable_sqlite_fkeys``
+----------------------------------------------
+
+**What it does:**
+Detects any Alembic ``op.*`` call (DDL operations like ``op.add_column``,
+``op.drop_column``, ``op.create_table``, ``op.drop_table``, ``op.create_index``,
+``op.batch_alter_table``, etc.) that appears before a
+``with disable_sqlite_fkeys(op):`` block in the same function. Excludes DML calls
+covered by MIG001.
+
+**Why is this bad:**
+DDL operations also trigger *autobegin* on SQLite, making the subsequent
+``PRAGMA foreign_keys=off`` a no-op, exactly like DML.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.add_column("dag_run", sa.Column("created_at", ...)) # triggers autobegin
+ with disable_sqlite_fkeys(op): # PRAGMA is now a no-op!
+ with op.batch_alter_table("backfill") as batch_op:
+ batch_op.alter_column("col", nullable=True)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ with disable_sqlite_fkeys(op):
+ op.add_column("dag_run", sa.Column("created_at", ...))
+ with op.batch_alter_table("backfill") as batch_op:
+ batch_op.alter_column("col", nullable=True)
+
+
+MIG003 -- DML without offline-mode guard
+------------------------------------------
+
+**What it does:**
+Detects ``op.execute()`` calls containing DML keywords (UPDATE, INSERT, DELETE) in
+``upgrade()`` or ``downgrade()`` functions that do not contain a
+``context.is_offline_mode()`` check anywhere in the same function.
+
+**Why is this bad:**
+Alembic's offline mode generates SQL scripts instead of executing against a live
+database. DML statements like ``UPDATE ... WHERE col IS NULL`` depend on actual data
+and produce no useful output in offline mode. Without an ``is_offline_mode()`` guard,
+the generated script includes DML that may fail or silently do nothing when applied
+later.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ if not context.is_offline_mode():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+
+Known Limitation -- Future MIG004 Candidate
+---------------------------------------------
+
+Migrations that perform DML and use ``batch_alter_table`` but never call
+``disable_sqlite_fkeys`` at all are NOT detected by MIG001/MIG002. These migrations
+may silently break SQLite foreign key handling. Detecting this pattern requires
+understanding whether foreign key relationships exist on the affected tables, which
+is beyond AST-level analysis. This is tracked as a future MIG004 candidate.
+
+
+Suppression
+===========
+
+Add ``# noqa: MIG0XX`` to a line to suppress the corresponding check for that line.
+Multiple codes can be comma-separated: ``# noqa: MIG001, MIG003``.
+
+Include a brief reason after ``--``::
+
+ # noqa: MIG003 -- simple UPDATE safe in offline mode
+"""
+
+from __future__ import annotations
+
+import ast
+import re
+import sys
+from pathlib import Path
+
+from rich.console import Console
+
+console = Console(color_system="standard", width=200)
+
+DML_KEYWORDS = ("UPDATE ", "INSERT ", "DELETE ")
+
+
+def _get_noqa_codes(line: str) -> set[str]:
+ """Extract noqa codes from a source line."""
+ match = re.search(r"#\s*noqa:\s*([\w,\s]+)", line)
+ if match:
+ return {code.strip() for code in match.group(1).split(",")}
+ return set()
+
+
+def _is_dml_string(node: ast.expr) -> bool:
+ """Check if an AST expression is a string literal starting with a DML keyword."""
+ if isinstance(node, ast.Constant) and isinstance(node.value, str):
+ stripped = node.value.strip().upper()
+ return any(stripped.startswith(kw) for kw in DML_KEYWORDS)
+ # f-strings: check the first string fragment
+ if isinstance(node, ast.JoinedStr):
+ for value in node.values:
+ if isinstance(value, ast.Constant) and isinstance(value.value, str):
+ stripped = value.value.strip().upper()
+ if any(stripped.startswith(kw) for kw in DML_KEYWORDS):
+ return True
+ break # only the leading fragment matters
+ return False
+
+
+def _is_op_execute_with_dml(node: ast.Call) -> bool:
+ """Check if a Call node is ``op.execute(<DML string>)``."""
+ if not (
+ isinstance(node.func, ast.Attribute)
+ and isinstance(node.func.value, ast.Name)
+ and node.func.value.id == "op"
+ and node.func.attr == "execute"
+ ):
+ return False
+ if not node.args:
+ return False
+ arg = node.args[0]
+ # Direct string: op.execute("UPDATE ...")
+ if _is_dml_string(arg):
+ return True
+ # Wrapped: op.execute(text("UPDATE ...")) or op.execute(sa.text("UPDATE ..."))
+ if isinstance(arg, ast.Call) and arg.args:
+ return _is_dml_string(arg.args[0])
+ return False
+
+
+def _is_op_call(node: ast.Call) -> bool:
+ """Check if a Call node is ``op.<something>(...)``."""
+ return (
+ isinstance(node.func, ast.Attribute)
+ and isinstance(node.func.value, ast.Name)
+ and node.func.value.id == "op"
+ )
+
+
+def _find_disable_sqlite_fkeys_line(func_node: ast.FunctionDef) -> int | None:
+ """Find the line number of the first ``with disable_sqlite_fkeys(op):`` in a function."""
+ # Collect all matches then take min() — ast.walk BFS order does not guarantee
+ # line-number order when guards appear at different nesting depths.
+ found: list[int] = []
+ for node in ast.walk(func_node):
+ if not isinstance(node, ast.With):
+ continue
+ for item in node.items:
+ ctx = item.context_expr
+ if isinstance(ctx, ast.Call):
+ func = ctx.func
+ if (isinstance(func, ast.Name) and func.id == "disable_sqlite_fkeys") or (
+ isinstance(func, ast.Attribute) and func.attr == "disable_sqlite_fkeys"
+ ):
+ found.append(node.lineno)
+ return min(found) if found else None
+
+
+def _has_offline_mode_check(func_node: ast.FunctionDef) -> bool:
+ """Check if function contains ``context.is_offline_mode()`` call."""
+ for node in ast.walk(func_node):
+ if isinstance(node, ast.Call):
+ func = node.func
+ if isinstance(func, ast.Attribute) and func.attr == "is_offline_mode":
+ return True
+ return False
Review Comment:
**Dismiss.** Any `.is_offline_mode()` call in a migration file is effectively `context.is_offline_mode()` — no other object in Alembic or Airflow migrations exposes that method name. Tightening to check the receiver name would add complexity without practical benefit, and could miss valid aliases (e.g., `ctx = context; ctx.is_offline_mode()`). Verified with: `grep -rn 'is_offline_mode' airflow-core/src/airflow/migrations/versions/*.py` → all hits are `context.is_offline_mode()`.
##########
scripts/ci/prek/check_migration_patterns.py:
##########
@@ -0,0 +1,383 @@
+#!/usr/bin/env python
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# /// script
+# requires-python = ">=3.10,<3.11"
+# dependencies = [
+# "rich>=13.6.0",
+# ]
+# ///
+"""
+Static analysis checks for Alembic migration anti-patterns.
+
+These checks use AST analysis to detect common migration mistakes that can cause
+silent failures, particularly on SQLite where ``PRAGMA foreign_keys`` has specific
+requirements about transaction state.
+
+Rules
+=====
+
+MIG001 -- DML before ``disable_sqlite_fkeys``
+----------------------------------------------
+
+**What it does:**
+Detects ``op.execute()`` calls containing DML keywords (UPDATE, INSERT, DELETE) that
+appear before a ``with disable_sqlite_fkeys(op):`` block in the same function.
+
+**Why is this bad:**
+On SQLite, any DML statement triggers *autobegin*, starting a transaction. Once a
+transaction is active, ``PRAGMA foreign_keys=off`` (issued by ``disable_sqlite_fkeys``)
+becomes a no-op. This means foreign key checks remain enabled, and
+``batch_alter_table`` (which recreates tables) may fail with foreign key constraint
+violations.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL") # triggers autobegin
+ with disable_sqlite_fkeys(op): # PRAGMA is now a no-op!
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ with disable_sqlite_fkeys(op):
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+
+MIG002 -- DDL before ``disable_sqlite_fkeys``
+----------------------------------------------
+
+**What it does:**
+Detects any Alembic ``op.*`` call (DDL operations like ``op.add_column``,
+``op.drop_column``, ``op.create_table``, ``op.drop_table``, ``op.create_index``,
+``op.batch_alter_table``, etc.) that appears before a
+``with disable_sqlite_fkeys(op):`` block in the same function. Excludes DML calls
+covered by MIG001.
+
+**Why is this bad:**
+DDL operations also trigger *autobegin* on SQLite, making the subsequent
+``PRAGMA foreign_keys=off`` a no-op, exactly like DML.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.add_column("dag_run", sa.Column("created_at", ...)) # triggers autobegin
+ with disable_sqlite_fkeys(op): # PRAGMA is now a no-op!
+ with op.batch_alter_table("backfill") as batch_op:
+ batch_op.alter_column("col", nullable=True)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ with disable_sqlite_fkeys(op):
+ op.add_column("dag_run", sa.Column("created_at", ...))
+ with op.batch_alter_table("backfill") as batch_op:
+ batch_op.alter_column("col", nullable=True)
+
+
+MIG003 -- DML without offline-mode guard
+------------------------------------------
+
+**What it does:**
+Detects ``op.execute()`` calls containing DML keywords (UPDATE, INSERT, DELETE) in
+``upgrade()`` or ``downgrade()`` functions that do not contain a
+``context.is_offline_mode()`` check anywhere in the same function.
+
+**Why is this bad:**
+Alembic's offline mode generates SQL scripts instead of executing against a live
+database. DML statements like ``UPDATE ... WHERE col IS NULL`` depend on actual data
+and produce no useful output in offline mode. Without an ``is_offline_mode()`` guard,
+the generated script includes DML that may fail or silently do nothing when applied
+later.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ if not context.is_offline_mode():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+
+Known Limitation -- Future MIG004 Candidate
+---------------------------------------------
+
+Migrations that perform DML and use ``batch_alter_table`` but never call
+``disable_sqlite_fkeys`` at all are NOT detected by MIG001/MIG002. These migrations
+may silently break SQLite foreign key handling. Detecting this pattern requires
+understanding whether foreign key relationships exist on the affected tables, which
+is beyond AST-level analysis. This is tracked as a future MIG004 candidate.
+
+
+Suppression
+===========
+
+Add ``# noqa: MIG0XX`` to a line to suppress the corresponding check for that line.
+Multiple codes can be comma-separated: ``# noqa: MIG001, MIG003``.
+
+Include a brief reason after ``--``::
+
+ # noqa: MIG003 -- simple UPDATE safe in offline mode
+"""
+
+from __future__ import annotations
+
+import ast
+import re
+import sys
+from pathlib import Path
+
+from rich.console import Console
+
+console = Console(color_system="standard", width=200)
+
+DML_KEYWORDS = ("UPDATE ", "INSERT ", "DELETE ")
+
+
+def _get_noqa_codes(line: str) -> set[str]:
+ """Extract noqa codes from a source line."""
+ match = re.search(r"#\s*noqa:\s*([\w,\s]+)", line)
+ if match:
+ return {code.strip() for code in match.group(1).split(",")}
+ return set()
+
+
+def _is_dml_string(node: ast.expr) -> bool:
+ """Check if an AST expression is a string literal starting with a DML keyword."""
+ if isinstance(node, ast.Constant) and isinstance(node.value, str):
+ stripped = node.value.strip().upper()
+ return any(stripped.startswith(kw) for kw in DML_KEYWORDS)
Review Comment:
**Accept.** Good catch. Fixed in 877a4a27 — `DML_KEYWORDS` now uses bare keywords (`"UPDATE"`, `"INSERT"`, `"DELETE"`) and `_is_dml_string` uses `split(None, 1)` to check the first word, handling any whitespace separator (space, newline, tab). Verified that `DELETE\nFROM`, `UPDATE\n SET`, `INSERT\nINTO` are all now detected, and existing space-separated DML detection is preserved. All 34 existing tests pass.
##########
scripts/ci/prek/check_migration_patterns.py:
##########
@@ -0,0 +1,383 @@
+#!/usr/bin/env python
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# /// script
+# requires-python = ">=3.10,<3.11"
+# dependencies = [
+# "rich>=13.6.0",
+# ]
Review Comment:
**Dismiss.** The PEP 723 `requires-python` is metadata for `uv run --script` mode — it's not used in the actual prek/pre-commit invocation path. The script is called by prek hooks which manage their own Python environment (`language: python` in `.pre-commit-config.yaml` uses whatever Python is available). The constraint is informational only and has no effect on which Python versions can run this script in practice.
##########
.pre-commit-config.yaml:
##########
@@ -1034,6 +1034,14 @@ repos:
pass_filenames: false
files: ^.*\.py$
require_serial: true
+ - id: check-migration-patterns
+ name: Check migration files for anti-patterns (MIG001/MIG002/MIG003)
+ entry: ./scripts/ci/prek/check_migration_patterns.py
+ language: python
+ pass_filenames: true
+ files: ^airflow-core/src/airflow/migrations/versions/.*\.py$
+ additional_dependencies: [rich>=13.6.0]
+ require_serial: true
Review Comment:
**Dismiss.** This follows from the premise in the previous comment about PEP 723 constraints. Since prek hooks manage their own Python environment independently of PEP 723 metadata, no `language_version` pin is needed in `.pre-commit-config.yaml`. The script runs correctly on all supported Python versions (3.10+) — verified by CI passing on Python 3.10, 3.11, 3.12, 3.13, and 3.14.
##########
scripts/ci/prek/check_new_airflow_exception_usage.py:
##########
@@ -59,7 +59,7 @@
from rich.console import Console
from rich.panel import Panel
-console = Console()
+console = Console(color_system="standard", width=200)
Review Comment:
**Partial.** This `Console(color_system="standard", width=200)` change was requested by human reviewer Lee-W ([comment](https://github.com/apache/airflow/pull/64972#discussion_r3062503519)) for the other prek script in this PR. Applied consistently to both scripts in the same commit to keep output formatting uniform across prek checks. Minor scope inclusion but intentional.
--
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] Add static analysis tests for DB migration anti-patterns [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on PR #64972:
URL: https://github.com/apache/airflow/pull/64972#issuecomment-4365349972
nice
--
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] Add static analysis tests for DB migration anti-patterns [airflow]
Posted by "Dev-iL (via GitHub)" <gi...@apache.org>.
Dev-iL commented on code in PR #64972:
URL: https://github.com/apache/airflow/pull/64972#discussion_r3064204266
##########
airflow-core/tests/unit/migrations/test_migration_utils.py:
##########
@@ -0,0 +1,183 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Unit tests for ``airflow.migrations.utils``."""
+
+from __future__ import annotations
+
+import pytest
+from alembic.migration import MigrationContext
+from alembic.operations import Operations
+from sqlalchemy import text
+
+from airflow import settings
+from airflow.migrations.utils import (
+ disable_sqlite_fkeys,
+ ignore_sqlite_value_error,
+ mysql_drop_foreignkey_if_exists,
+)
+
+pytestmark = pytest.mark.db_test
+
+
+def _dialect() -> str:
+ assert settings.engine is not None # guaranteed by db_test marker
+ return settings.engine.dialect.name
+
+
+class TestDisableSqliteFkeys:
+ """Tests for :func:`disable_sqlite_fkeys`."""
+
+ def test_yields_op(self):
+ """The context manager yields the *op* object on every backend."""
+ with settings.engine.connect() as conn:
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+ with disable_sqlite_fkeys(op) as yielded:
+ assert yielded is op
+
+ def test_sqlite_turns_off_fkeys(self):
+ """On SQLite, ``PRAGMA foreign_keys`` is OFF inside the context."""
+ if _dialect() != "sqlite":
+ pytest.skip("SQLite-specific test")
+
+ with settings.engine.connect() as conn:
+ # Ensure foreign_keys is ON before entering the context manager.
+ conn.execute(text("PRAGMA foreign_keys=ON"))
+ conn.commit()
+
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+
+ with disable_sqlite_fkeys(op):
+ result = conn.execute(text("PRAGMA foreign_keys")).scalar()
+ assert result == 0, "foreign_keys should be OFF inside disable_sqlite_fkeys"
+
+ result = conn.execute(text("PRAGMA foreign_keys")).scalar()
+ assert result == 1, "foreign_keys should be restored to ON after disable_sqlite_fkeys"
+
+ def test_non_sqlite_is_noop(self):
+ """On non-SQLite backends, the context manager simply yields."""
+ if _dialect() == "sqlite":
+ pytest.skip("Non-SQLite backends only")
+
+ with settings.engine.connect() as conn:
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+ with disable_sqlite_fkeys(op) as yielded:
+ assert yielded is op
+
+
+class TestMysqlDropForeignkeyIfExists:
+ """Tests for :func:`mysql_drop_foreignkey_if_exists`."""
+
+ def test_drops_existing_fk(self):
+ """On MySQL, an existing foreign key is dropped."""
+ if _dialect() != "mysql":
Review Comment:
Done
##########
airflow-core/tests/unit/migrations/test_migration_utils.py:
##########
@@ -0,0 +1,183 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Unit tests for ``airflow.migrations.utils``."""
+
+from __future__ import annotations
+
+import pytest
+from alembic.migration import MigrationContext
+from alembic.operations import Operations
+from sqlalchemy import text
+
+from airflow import settings
+from airflow.migrations.utils import (
+ disable_sqlite_fkeys,
+ ignore_sqlite_value_error,
+ mysql_drop_foreignkey_if_exists,
+)
+
+pytestmark = pytest.mark.db_test
+
+
+def _dialect() -> str:
+ assert settings.engine is not None # guaranteed by db_test marker
+ return settings.engine.dialect.name
+
+
+class TestDisableSqliteFkeys:
+ """Tests for :func:`disable_sqlite_fkeys`."""
+
+ def test_yields_op(self):
+ """The context manager yields the *op* object on every backend."""
+ with settings.engine.connect() as conn:
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+ with disable_sqlite_fkeys(op) as yielded:
+ assert yielded is op
+
+ def test_sqlite_turns_off_fkeys(self):
+ """On SQLite, ``PRAGMA foreign_keys`` is OFF inside the context."""
+ if _dialect() != "sqlite":
+ pytest.skip("SQLite-specific test")
+
+ with settings.engine.connect() as conn:
+ # Ensure foreign_keys is ON before entering the context manager.
+ conn.execute(text("PRAGMA foreign_keys=ON"))
+ conn.commit()
+
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+
+ with disable_sqlite_fkeys(op):
+ result = conn.execute(text("PRAGMA foreign_keys")).scalar()
+ assert result == 0, "foreign_keys should be OFF inside disable_sqlite_fkeys"
+
+ result = conn.execute(text("PRAGMA foreign_keys")).scalar()
+ assert result == 1, "foreign_keys should be restored to ON after disable_sqlite_fkeys"
+
+ def test_non_sqlite_is_noop(self):
+ """On non-SQLite backends, the context manager simply yields."""
+ if _dialect() == "sqlite":
+ pytest.skip("Non-SQLite backends only")
+
+ with settings.engine.connect() as conn:
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+ with disable_sqlite_fkeys(op) as yielded:
+ assert yielded is op
+
+
+class TestMysqlDropForeignkeyIfExists:
+ """Tests for :func:`mysql_drop_foreignkey_if_exists`."""
+
+ def test_drops_existing_fk(self):
+ """On MySQL, an existing foreign key is dropped."""
+ if _dialect() != "mysql":
+ pytest.skip("MySQL-specific stored procedure test")
+
+ with settings.engine.connect() as conn:
+ conn.execute(text("CREATE TABLE IF NOT EXISTS _test_mig_parent ( id INT PRIMARY KEY)"))
+ conn.execute(
+ text(
+ "CREATE TABLE IF NOT EXISTS _test_mig_child ("
+ " id INT PRIMARY KEY,"
+ " parent_id INT,"
+ " CONSTRAINT _test_mig_fk FOREIGN KEY (parent_id)"
+ " REFERENCES _test_mig_parent(id)"
+ ")"
Review Comment:
Done
--
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] Add static analysis tests for DB migration anti-patterns [airflow]
Posted by "Dev-iL (via GitHub)" <gi...@apache.org>.
Dev-iL commented on PR #64972:
URL: https://github.com/apache/airflow/pull/64972#issuecomment-4239571042
## CI Timeout Investigation: `All-core:LowestDeps:14:3.12:Core...Serialization`
### Summary
The CI job `All-core:LowestDeps:14:3.12:Core...Serialization` was cancelled after 1h5m (exceeding `TOTAL_TEST_TIMEOUT: 3600s`). This is a **pre-existing flake**, not caused by this PR's changes.
### Evidence
**1. Only Python 3.12 affected**
The same test group (`LowestDeps:Core...Serialization`) passed on Python 3.10, 3.11, 3.13, and 3.14. Only the 3.12 run timed out.
**2. "Other" group stuck at 63%**
The "Other" test group reported "1 failed, 1093 passed, 12 skipped" at the 12:47 mark and made no further progress until cancellation at 13:40. This PR adds ~8 tests — the hang is deep in pre-existing tests (1093+).
**3. PR code is byte-identical between force-pushes**
Diffing the PR-specific files between commits 931fbf06 and 553c33ff (pre- and post-rebase) shows zero differences — only the rebased base changed.
**4. Backend markers verified correct**
- `BACKEND=sqlite` env var is set in LowestDeps CI (confirmed from logs)
- `--backend` is NOT passed as a pytest CLI arg (`Extra pytest args: ()`)
- `@pytest.mark.backend("sqlite")` → runs (BACKEND matches) ✓
- `@pytest.mark.backend("postgres", "mysql")` → skipped (BACKEND doesn't match) ✓
- No hang path through the backend marker logic
**5. Upstream changes between force-pushes**
The rebase picked up: `feat: restore os provider on core library (#64764)`, `Remove unused/dead code in migration utils (#64947)`, `Added plugins command to airflowctl (#64935)`, `Breeze: Show local reproduction commands (#63901)`. None touch test infrastructure in a way that would cause a Python 3.12-specific hang.
### Conclusion
Most likely cause: a pre-existing flaky test in the "Other" group that hangs specifically on Python 3.12 (possibly a deadlock or resource contention issue). The PR's changes (AST-based migration checks + migration util tests) are not involved — they ran in a different test group and the PR files are byte-identical across the rebase.
--
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] Add static analysis tests for DB migration anti-patterns [airflow]
Posted by "Dev-iL (via GitHub)" <gi...@apache.org>.
Dev-iL commented on code in PR #64972:
URL: https://github.com/apache/airflow/pull/64972#discussion_r3062674213
##########
scripts/ci/prek/check_migration_patterns.py:
##########
@@ -0,0 +1,384 @@
+#!/usr/bin/env python
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""
+Static analysis checks for Alembic migration anti-patterns.
+
+These checks use AST analysis to detect common migration mistakes that can cause
+silent failures, particularly on SQLite where ``PRAGMA foreign_keys`` has specific
+requirements about transaction state.
+
+TODO: These checks are candidates for future Ruff rules (AIR category).
+
+Rules
+=====
+
+MIG001 -- DML before ``disable_sqlite_fkeys``
+----------------------------------------------
+
+**What it does:**
+Detects ``op.execute()`` calls containing DML keywords (UPDATE, INSERT, DELETE) that
+appear before a ``with disable_sqlite_fkeys(op):`` block in the same function.
+
+**Why is this bad:**
+On SQLite, any DML statement triggers *autobegin*, starting a transaction. Once a
+transaction is active, ``PRAGMA foreign_keys=off`` (issued by ``disable_sqlite_fkeys``)
+becomes a no-op. This means foreign key checks remain enabled, and
+``batch_alter_table`` (which recreates tables) may fail with foreign key constraint
+violations.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL") # triggers autobegin
+ with disable_sqlite_fkeys(op): # PRAGMA is now a no-op!
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ with disable_sqlite_fkeys(op):
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+
+MIG002 -- DDL before ``disable_sqlite_fkeys``
+----------------------------------------------
+
+**What it does:**
+Detects any Alembic ``op.*`` call (DDL operations like ``op.add_column``,
+``op.drop_column``, ``op.create_table``, ``op.drop_table``, ``op.create_index``,
+``op.batch_alter_table``, etc.) that appears before a
+``with disable_sqlite_fkeys(op):`` block in the same function. Excludes DML calls
+covered by MIG001.
+
+**Why is this bad:**
+DDL operations also trigger *autobegin* on SQLite, making the subsequent
+``PRAGMA foreign_keys=off`` a no-op, exactly like DML.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.add_column("dag_run", sa.Column("created_at", ...)) # triggers autobegin
+ with disable_sqlite_fkeys(op): # PRAGMA is now a no-op!
+ with op.batch_alter_table("backfill") as batch_op:
+ batch_op.alter_column("col", nullable=True)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ with disable_sqlite_fkeys(op):
+ op.add_column("dag_run", sa.Column("created_at", ...))
+ with op.batch_alter_table("backfill") as batch_op:
+ batch_op.alter_column("col", nullable=True)
+
+
+MIG003 -- DML without offline-mode guard
Review Comment:
As mentioned on Slack - I think it's justified to err on the side of false-positive in the case of migration issue detection. If you disagree, I can try to tighten this check or disable it. The question is - does it catch real issues we previously missed?
--
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] Add static analysis tests for DB migration anti-patterns [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #64972:
URL: https://github.com/apache/airflow/pull/64972#discussion_r3062739338
##########
airflow-core/tests/unit/migrations/test_migration_utils.py:
##########
@@ -0,0 +1,183 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Unit tests for ``airflow.migrations.utils``."""
+
+from __future__ import annotations
+
+import pytest
+from alembic.migration import MigrationContext
+from alembic.operations import Operations
+from sqlalchemy import text
+
+from airflow import settings
+from airflow.migrations.utils import (
+ disable_sqlite_fkeys,
+ ignore_sqlite_value_error,
+ mysql_drop_foreignkey_if_exists,
+)
+
+pytestmark = pytest.mark.db_test
+
+
+def _dialect() -> str:
+ assert settings.engine is not None # guaranteed by db_test marker
+ return settings.engine.dialect.name
+
+
+class TestDisableSqliteFkeys:
+ """Tests for :func:`disable_sqlite_fkeys`."""
+
+ def test_yields_op(self):
+ """The context manager yields the *op* object on every backend."""
+ with settings.engine.connect() as conn:
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+ with disable_sqlite_fkeys(op) as yielded:
+ assert yielded is op
+
+ def test_sqlite_turns_off_fkeys(self):
+ """On SQLite, ``PRAGMA foreign_keys`` is OFF inside the context."""
+ if _dialect() != "sqlite":
+ pytest.skip("SQLite-specific test")
Review Comment:
we can use `pytest.mark.skipif`
##########
airflow-core/tests/unit/migrations/test_migration_utils.py:
##########
@@ -0,0 +1,183 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Unit tests for ``airflow.migrations.utils``."""
+
+from __future__ import annotations
+
+import pytest
+from alembic.migration import MigrationContext
+from alembic.operations import Operations
+from sqlalchemy import text
+
+from airflow import settings
+from airflow.migrations.utils import (
+ disable_sqlite_fkeys,
+ ignore_sqlite_value_error,
+ mysql_drop_foreignkey_if_exists,
+)
+
+pytestmark = pytest.mark.db_test
+
+
+def _dialect() -> str:
+ assert settings.engine is not None # guaranteed by db_test marker
Review Comment:
```suggestion
if TYPE_CHECKING:
assert settings.engine is not None # guaranteed by db_test marker
```
I think we usually do it for `assert` like this
##########
airflow-core/tests/unit/migrations/test_migration_utils.py:
##########
@@ -0,0 +1,183 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Unit tests for ``airflow.migrations.utils``."""
+
+from __future__ import annotations
+
+import pytest
+from alembic.migration import MigrationContext
+from alembic.operations import Operations
+from sqlalchemy import text
+
+from airflow import settings
+from airflow.migrations.utils import (
+ disable_sqlite_fkeys,
+ ignore_sqlite_value_error,
+ mysql_drop_foreignkey_if_exists,
+)
+
+pytestmark = pytest.mark.db_test
+
+
+def _dialect() -> str:
+ assert settings.engine is not None # guaranteed by db_test marker
+ return settings.engine.dialect.name
+
+
+class TestDisableSqliteFkeys:
+ """Tests for :func:`disable_sqlite_fkeys`."""
+
+ def test_yields_op(self):
+ """The context manager yields the *op* object on every backend."""
+ with settings.engine.connect() as conn:
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+ with disable_sqlite_fkeys(op) as yielded:
+ assert yielded is op
+
+ def test_sqlite_turns_off_fkeys(self):
+ """On SQLite, ``PRAGMA foreign_keys`` is OFF inside the context."""
+ if _dialect() != "sqlite":
+ pytest.skip("SQLite-specific test")
+
+ with settings.engine.connect() as conn:
+ # Ensure foreign_keys is ON before entering the context manager.
+ conn.execute(text("PRAGMA foreign_keys=ON"))
+ conn.commit()
+
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+
+ with disable_sqlite_fkeys(op):
+ result = conn.execute(text("PRAGMA foreign_keys")).scalar()
+ assert result == 0, "foreign_keys should be OFF inside disable_sqlite_fkeys"
+
+ result = conn.execute(text("PRAGMA foreign_keys")).scalar()
+ assert result == 1, "foreign_keys should be restored to ON after disable_sqlite_fkeys"
+
+ def test_non_sqlite_is_noop(self):
+ """On non-SQLite backends, the context manager simply yields."""
+ if _dialect() == "sqlite":
+ pytest.skip("Non-SQLite backends only")
+
+ with settings.engine.connect() as conn:
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+ with disable_sqlite_fkeys(op) as yielded:
+ assert yielded is op
+
+
+class TestMysqlDropForeignkeyIfExists:
+ """Tests for :func:`mysql_drop_foreignkey_if_exists`."""
+
+ def test_drops_existing_fk(self):
+ """On MySQL, an existing foreign key is dropped."""
+ if _dialect() != "mysql":
Review Comment:
same
##########
airflow-core/tests/unit/migrations/test_migration_utils.py:
##########
@@ -0,0 +1,183 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Unit tests for ``airflow.migrations.utils``."""
+
+from __future__ import annotations
+
+import pytest
+from alembic.migration import MigrationContext
+from alembic.operations import Operations
+from sqlalchemy import text
+
+from airflow import settings
+from airflow.migrations.utils import (
+ disable_sqlite_fkeys,
+ ignore_sqlite_value_error,
+ mysql_drop_foreignkey_if_exists,
+)
+
+pytestmark = pytest.mark.db_test
+
+
+def _dialect() -> str:
+ assert settings.engine is not None # guaranteed by db_test marker
+ return settings.engine.dialect.name
+
+
+class TestDisableSqliteFkeys:
+ """Tests for :func:`disable_sqlite_fkeys`."""
+
+ def test_yields_op(self):
+ """The context manager yields the *op* object on every backend."""
+ with settings.engine.connect() as conn:
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+ with disable_sqlite_fkeys(op) as yielded:
+ assert yielded is op
+
+ def test_sqlite_turns_off_fkeys(self):
+ """On SQLite, ``PRAGMA foreign_keys`` is OFF inside the context."""
+ if _dialect() != "sqlite":
+ pytest.skip("SQLite-specific test")
+
+ with settings.engine.connect() as conn:
+ # Ensure foreign_keys is ON before entering the context manager.
+ conn.execute(text("PRAGMA foreign_keys=ON"))
+ conn.commit()
+
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+
+ with disable_sqlite_fkeys(op):
+ result = conn.execute(text("PRAGMA foreign_keys")).scalar()
+ assert result == 0, "foreign_keys should be OFF inside disable_sqlite_fkeys"
+
+ result = conn.execute(text("PRAGMA foreign_keys")).scalar()
+ assert result == 1, "foreign_keys should be restored to ON after disable_sqlite_fkeys"
+
+ def test_non_sqlite_is_noop(self):
+ """On non-SQLite backends, the context manager simply yields."""
+ if _dialect() == "sqlite":
+ pytest.skip("Non-SQLite backends only")
+
+ with settings.engine.connect() as conn:
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+ with disable_sqlite_fkeys(op) as yielded:
+ assert yielded is op
+
+
+class TestMysqlDropForeignkeyIfExists:
+ """Tests for :func:`mysql_drop_foreignkey_if_exists`."""
+
+ def test_drops_existing_fk(self):
+ """On MySQL, an existing foreign key is dropped."""
+ if _dialect() != "mysql":
+ pytest.skip("MySQL-specific stored procedure test")
+
+ with settings.engine.connect() as conn:
+ conn.execute(text("CREATE TABLE IF NOT EXISTS _test_mig_parent ( id INT PRIMARY KEY)"))
+ conn.execute(
+ text(
+ "CREATE TABLE IF NOT EXISTS _test_mig_child ("
+ " id INT PRIMARY KEY,"
+ " parent_id INT,"
+ " CONSTRAINT _test_mig_fk FOREIGN KEY (parent_id)"
+ " REFERENCES _test_mig_parent(id)"
+ ")"
Review Comment:
```suggestion
"""
CREATE TABLE IF NOT EXISTS _test_mig_child (
id INT PRIMARY KEY,
parent_id INT,
CONSTRAINT _test_mig_fk FOREIGN KEY (parent_id)
REFERENCES _test_mig_parent(id)
)"""
```
##########
scripts/ci/prek/check_migration_patterns.py:
##########
@@ -0,0 +1,384 @@
+#!/usr/bin/env python
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""
+Static analysis checks for Alembic migration anti-patterns.
+
+These checks use AST analysis to detect common migration mistakes that can cause
+silent failures, particularly on SQLite where ``PRAGMA foreign_keys`` has specific
+requirements about transaction state.
+
+TODO: These checks are candidates for future Ruff rules (AIR category).
+
+Rules
+=====
+
+MIG001 -- DML before ``disable_sqlite_fkeys``
+----------------------------------------------
+
+**What it does:**
+Detects ``op.execute()`` calls containing DML keywords (UPDATE, INSERT, DELETE) that
+appear before a ``with disable_sqlite_fkeys(op):`` block in the same function.
+
+**Why is this bad:**
+On SQLite, any DML statement triggers *autobegin*, starting a transaction. Once a
+transaction is active, ``PRAGMA foreign_keys=off`` (issued by ``disable_sqlite_fkeys``)
+becomes a no-op. This means foreign key checks remain enabled, and
+``batch_alter_table`` (which recreates tables) may fail with foreign key constraint
+violations.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL") # triggers autobegin
+ with disable_sqlite_fkeys(op): # PRAGMA is now a no-op!
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ with disable_sqlite_fkeys(op):
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+
+MIG002 -- DDL before ``disable_sqlite_fkeys``
+----------------------------------------------
+
+**What it does:**
+Detects any Alembic ``op.*`` call (DDL operations like ``op.add_column``,
+``op.drop_column``, ``op.create_table``, ``op.drop_table``, ``op.create_index``,
+``op.batch_alter_table``, etc.) that appears before a
+``with disable_sqlite_fkeys(op):`` block in the same function. Excludes DML calls
+covered by MIG001.
+
+**Why is this bad:**
+DDL operations also trigger *autobegin* on SQLite, making the subsequent
+``PRAGMA foreign_keys=off`` a no-op, exactly like DML.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.add_column("dag_run", sa.Column("created_at", ...)) # triggers autobegin
+ with disable_sqlite_fkeys(op): # PRAGMA is now a no-op!
+ with op.batch_alter_table("backfill") as batch_op:
+ batch_op.alter_column("col", nullable=True)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ with disable_sqlite_fkeys(op):
+ op.add_column("dag_run", sa.Column("created_at", ...))
+ with op.batch_alter_table("backfill") as batch_op:
+ batch_op.alter_column("col", nullable=True)
+
+
+MIG003 -- DML without offline-mode guard
Review Comment:
I have never encountered this one, so I can't really tell 🤔
I don't disagree, just want to make sure.
##########
airflow-core/tests/unit/migrations/test_migration_utils.py:
##########
@@ -0,0 +1,183 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Unit tests for ``airflow.migrations.utils``."""
+
+from __future__ import annotations
+
+import pytest
+from alembic.migration import MigrationContext
+from alembic.operations import Operations
+from sqlalchemy import text
+
+from airflow import settings
+from airflow.migrations.utils import (
+ disable_sqlite_fkeys,
+ ignore_sqlite_value_error,
+ mysql_drop_foreignkey_if_exists,
+)
+
+pytestmark = pytest.mark.db_test
+
+
+def _dialect() -> str:
+ assert settings.engine is not None # guaranteed by db_test marker
+ return settings.engine.dialect.name
+
+
+class TestDisableSqliteFkeys:
+ """Tests for :func:`disable_sqlite_fkeys`."""
+
+ def test_yields_op(self):
+ """The context manager yields the *op* object on every backend."""
+ with settings.engine.connect() as conn:
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+ with disable_sqlite_fkeys(op) as yielded:
+ assert yielded is op
+
+ def test_sqlite_turns_off_fkeys(self):
+ """On SQLite, ``PRAGMA foreign_keys`` is OFF inside the context."""
+ if _dialect() != "sqlite":
+ pytest.skip("SQLite-specific test")
+
+ with settings.engine.connect() as conn:
+ # Ensure foreign_keys is ON before entering the context manager.
+ conn.execute(text("PRAGMA foreign_keys=ON"))
+ conn.commit()
+
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+
+ with disable_sqlite_fkeys(op):
+ result = conn.execute(text("PRAGMA foreign_keys")).scalar()
+ assert result == 0, "foreign_keys should be OFF inside disable_sqlite_fkeys"
+
+ result = conn.execute(text("PRAGMA foreign_keys")).scalar()
+ assert result == 1, "foreign_keys should be restored to ON after disable_sqlite_fkeys"
+
+ def test_non_sqlite_is_noop(self):
+ """On non-SQLite backends, the context manager simply yields."""
+ if _dialect() == "sqlite":
+ pytest.skip("Non-SQLite backends only")
+
Review Comment:
same here. then we probably don't need to make _dialect a fixture. `_get_dialect` might be a better name?
##########
airflow-core/tests/unit/migrations/test_migration_utils.py:
##########
@@ -0,0 +1,183 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Unit tests for ``airflow.migrations.utils``."""
+
+from __future__ import annotations
+
+import pytest
+from alembic.migration import MigrationContext
+from alembic.operations import Operations
+from sqlalchemy import text
+
+from airflow import settings
+from airflow.migrations.utils import (
+ disable_sqlite_fkeys,
+ ignore_sqlite_value_error,
+ mysql_drop_foreignkey_if_exists,
+)
+
+pytestmark = pytest.mark.db_test
+
+
+def _dialect() -> str:
+ assert settings.engine is not None # guaranteed by db_test marker
+ return settings.engine.dialect.name
+
+
+class TestDisableSqliteFkeys:
+ """Tests for :func:`disable_sqlite_fkeys`."""
+
+ def test_yields_op(self):
+ """The context manager yields the *op* object on every backend."""
+ with settings.engine.connect() as conn:
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+ with disable_sqlite_fkeys(op) as yielded:
+ assert yielded is op
+
+ def test_sqlite_turns_off_fkeys(self):
+ """On SQLite, ``PRAGMA foreign_keys`` is OFF inside the context."""
+ if _dialect() != "sqlite":
+ pytest.skip("SQLite-specific test")
+
+ with settings.engine.connect() as conn:
+ # Ensure foreign_keys is ON before entering the context manager.
+ conn.execute(text("PRAGMA foreign_keys=ON"))
+ conn.commit()
+
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+
+ with disable_sqlite_fkeys(op):
+ result = conn.execute(text("PRAGMA foreign_keys")).scalar()
+ assert result == 0, "foreign_keys should be OFF inside disable_sqlite_fkeys"
+
+ result = conn.execute(text("PRAGMA foreign_keys")).scalar()
+ assert result == 1, "foreign_keys should be restored to ON after disable_sqlite_fkeys"
+
+ def test_non_sqlite_is_noop(self):
+ """On non-SQLite backends, the context manager simply yields."""
+ if _dialect() == "sqlite":
+ pytest.skip("Non-SQLite backends only")
+
+ with settings.engine.connect() as conn:
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+ with disable_sqlite_fkeys(op) as yielded:
+ assert yielded is op
+
+
+class TestMysqlDropForeignkeyIfExists:
+ """Tests for :func:`mysql_drop_foreignkey_if_exists`."""
+
+ def test_drops_existing_fk(self):
+ """On MySQL, an existing foreign key is dropped."""
+ if _dialect() != "mysql":
+ pytest.skip("MySQL-specific stored procedure test")
+
+ with settings.engine.connect() as conn:
+ conn.execute(text("CREATE TABLE IF NOT EXISTS _test_mig_parent ( id INT PRIMARY KEY)"))
+ conn.execute(
+ text(
+ "CREATE TABLE IF NOT EXISTS _test_mig_child ("
+ " id INT PRIMARY KEY,"
+ " parent_id INT,"
+ " CONSTRAINT _test_mig_fk FOREIGN KEY (parent_id)"
+ " REFERENCES _test_mig_parent(id)"
+ ")"
+ )
+ )
+ conn.commit()
+
+ try:
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+ mysql_drop_foreignkey_if_exists("_test_mig_fk", "_test_mig_child", op)
+ conn.commit()
+
+ count = conn.execute(
+ text(
+ "SELECT COUNT(*) FROM information_schema.TABLE_CONSTRAINTS "
Review Comment:
same format as the above one would be easier to read
--
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] Add static analysis tests for DB migration anti-patterns [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #64972:
URL: https://github.com/apache/airflow/pull/64972#discussion_r3152661222
##########
airflow-core/tests/unit/migrations/test_migration_patterns.py:
##########
@@ -0,0 +1,71 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Pytest harness for migration anti-pattern checks (MIG001/MIG002/MIG003)."""
+
+from __future__ import annotations
+
+import importlib.util
+from pathlib import Path
+
+import pytest
+
+# ---------------------------------------------------------------------------
+# Import check functions from the prek hook script (single source of truth).
+# ---------------------------------------------------------------------------
+_PREK_SCRIPT = Path(__file__).resolve().parents[4] / "scripts" / "ci" / "prek" / "check_migration_patterns.py"
+_spec = importlib.util.spec_from_file_location("check_migration_patterns", _PREK_SCRIPT)
+if _spec is None or _spec.loader is None:
+ raise ImportError(f"Could not load prek script: {_PREK_SCRIPT}")
+_mod = importlib.util.module_from_spec(_spec)
+_spec.loader.exec_module(_mod) # type: ignore[union-attr] # typeshed: Loader lacks exec_module
+check_mig001 = _mod.check_mig001
+check_mig002 = _mod.check_mig002
+check_mig003 = _mod.check_mig003
+
+MIGRATIONS_DIR = Path(__file__).resolve().parents[3] / "src" / "airflow" / "migrations" / "versions"
+
+
+def _migration_files() -> list[Path]:
Review Comment:
```suggestion
@pytest.fixture(scope=module)
def _migration_files() -> list[Path]:
```
not a good function name, probably make it a fixture?
##########
airflow-core/tests/unit/migrations/test_migration_utils.py:
##########
@@ -0,0 +1,176 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Unit tests for ``airflow.migrations.utils``."""
+
+from __future__ import annotations
+
+import pytest
+from alembic.migration import MigrationContext
+from alembic.operations import Operations
+from sqlalchemy import text
+
+from airflow import settings
+from airflow.migrations.utils import (
+ disable_sqlite_fkeys,
+ ignore_sqlite_value_error,
+ mysql_drop_foreignkey_if_exists,
+)
+
+pytestmark = pytest.mark.db_test
+
+
+class TestDisableSqliteFkeys:
+ """Tests for :func:`disable_sqlite_fkeys`."""
+
+ def test_yields_op(self):
+ """The context manager yields the *op* object on every backend."""
+ with settings.engine.connect() as conn:
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+ with disable_sqlite_fkeys(op) as yielded:
+ assert yielded is op
+
+ @pytest.mark.backend("sqlite")
+ def test_sqlite_turns_off_fkeys(self):
+ """On SQLite, ``PRAGMA foreign_keys`` is OFF inside the context."""
+
+ with settings.engine.connect() as conn:
+ # Ensure foreign_keys is ON before entering the context manager.
+ conn.execute(text("PRAGMA foreign_keys=ON"))
+ conn.commit()
+
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+
+ with disable_sqlite_fkeys(op):
+ result = conn.execute(text("PRAGMA foreign_keys")).scalar()
+ assert result == 0, "foreign_keys should be OFF inside disable_sqlite_fkeys"
+
+ result = conn.execute(text("PRAGMA foreign_keys")).scalar()
+ assert result == 1, "foreign_keys should be restored to ON after disable_sqlite_fkeys"
+
+ @pytest.mark.backend("postgres", "mysql")
+ def test_non_sqlite_is_noop(self):
+ """On non-SQLite backends, the context manager simply yields."""
+
+ with settings.engine.connect() as conn:
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+ with disable_sqlite_fkeys(op) as yielded:
+ assert yielded is op
+
+
+class TestMysqlDropForeignkeyIfExists:
+ """Tests for :func:`mysql_drop_foreignkey_if_exists`."""
+
+ @pytest.mark.backend("mysql")
+ def test_drops_existing_fk(self):
+ """On MySQL, an existing foreign key is dropped."""
+
+ with settings.engine.connect() as conn:
+ conn.execute(text("CREATE TABLE IF NOT EXISTS _test_mig_parent (id INT PRIMARY KEY)"))
+ conn.execute(
+ text(
+ """
+ CREATE TABLE IF NOT EXISTS _test_mig_child (
+ id INT PRIMARY KEY,
+ parent_id INT,
+ CONSTRAINT _test_mig_fk FOREIGN KEY (parent_id)
+ REFERENCES _test_mig_parent(id)
+ )
+ """
+ )
+ )
+ conn.commit()
+
+ try:
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+ mysql_drop_foreignkey_if_exists("_test_mig_fk", "_test_mig_child", op)
+ conn.commit()
+
+ count = conn.execute(
+ text(
+ """
+ SELECT COUNT(*) FROM information_schema.TABLE_CONSTRAINTS
+ WHERE CONSTRAINT_SCHEMA = DATABASE()
+ AND TABLE_NAME = '_test_mig_child'
+ AND CONSTRAINT_NAME = '_test_mig_fk'
+ AND CONSTRAINT_TYPE = 'FOREIGN KEY'
+ """
+ )
+ ).scalar()
+ assert count == 0, "Foreign key should have been dropped"
+ finally:
+ conn.execute(text("DROP TABLE IF EXISTS _test_mig_child"))
+ conn.execute(text("DROP TABLE IF EXISTS _test_mig_parent"))
+ conn.commit()
+
+ @pytest.mark.backend("mysql")
+ def test_noop_for_nonexistent_fk(self):
+ """On MySQL, dropping a non-existent FK does not raise."""
+
+ with settings.engine.connect() as conn:
+ conn.execute(text("CREATE TABLE IF NOT EXISTS _test_mig_nofk (id INT PRIMARY KEY)"))
+ conn.commit()
+
+ try:
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+ mysql_drop_foreignkey_if_exists("nonexistent_fk", "_test_mig_nofk", op)
+ conn.commit()
+ finally:
+ conn.execute(text("DROP TABLE IF EXISTS _test_mig_nofk"))
+ conn.commit()
+
+
+class TestIgnoreSqliteValueError:
+ """Tests for :func:`ignore_sqlite_value_error`."""
+
+ @pytest.mark.backend("sqlite")
+ def test_sqlite_suppresses_value_error(self):
+ """On SQLite, ``ValueError`` is suppressed."""
+
+ with settings.engine.connect() as conn:
+ ctx = MigrationContext.configure(conn)
+ with Operations.context(ctx):
+ cm = ignore_sqlite_value_error()
+ with cm:
+ raise ValueError("should be suppressed on SQLite")
+
+ @pytest.mark.backend("postgres", "mysql")
+ def test_non_sqlite_does_not_suppress(self):
+ """On non-SQLite backends, ``ValueError`` propagates."""
+
+ with settings.engine.connect() as conn:
+ ctx = MigrationContext.configure(conn)
+ with Operations.context(ctx):
+ cm = ignore_sqlite_value_error()
+ with pytest.raises(ValueError, match="should propagate"):
+ with cm:
+ raise ValueError("should propagate")
+
+ def test_does_not_suppress_other_exceptions(self):
+ """Even on SQLite, only ``ValueError`` is suppressed — other exceptions propagate."""
+ with settings.engine.connect() as conn:
+ ctx = MigrationContext.configure(conn)
+ with Operations.context(ctx):
+ cm = ignore_sqlite_value_error()
+ with pytest.raises(TypeError):
+ with cm:
Review Comment:
```suggestion
with pytest.raises(TypeError):
with ignore_sqlite_value_error():
```
##########
scripts/ci/prek/check_migration_patterns.py:
##########
@@ -0,0 +1,383 @@
+#!/usr/bin/env python
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# /// script
+# requires-python = ">=3.10,<3.11"
Review Comment:
3.10 EOL happens this year.
```suggestion
# requires-python = ">=3.10"
```
##########
airflow-core/tests/unit/migrations/test_migration_utils.py:
##########
@@ -0,0 +1,176 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Unit tests for ``airflow.migrations.utils``."""
+
+from __future__ import annotations
+
+import pytest
+from alembic.migration import MigrationContext
+from alembic.operations import Operations
+from sqlalchemy import text
+
+from airflow import settings
+from airflow.migrations.utils import (
+ disable_sqlite_fkeys,
+ ignore_sqlite_value_error,
+ mysql_drop_foreignkey_if_exists,
+)
+
+pytestmark = pytest.mark.db_test
+
+
+class TestDisableSqliteFkeys:
+ """Tests for :func:`disable_sqlite_fkeys`."""
+
+ def test_yields_op(self):
+ """The context manager yields the *op* object on every backend."""
+ with settings.engine.connect() as conn:
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+ with disable_sqlite_fkeys(op) as yielded:
+ assert yielded is op
+
+ @pytest.mark.backend("sqlite")
+ def test_sqlite_turns_off_fkeys(self):
+ """On SQLite, ``PRAGMA foreign_keys`` is OFF inside the context."""
+
+ with settings.engine.connect() as conn:
+ # Ensure foreign_keys is ON before entering the context manager.
+ conn.execute(text("PRAGMA foreign_keys=ON"))
+ conn.commit()
+
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+
+ with disable_sqlite_fkeys(op):
+ result = conn.execute(text("PRAGMA foreign_keys")).scalar()
+ assert result == 0, "foreign_keys should be OFF inside disable_sqlite_fkeys"
+
+ result = conn.execute(text("PRAGMA foreign_keys")).scalar()
+ assert result == 1, "foreign_keys should be restored to ON after disable_sqlite_fkeys"
+
+ @pytest.mark.backend("postgres", "mysql")
+ def test_non_sqlite_is_noop(self):
+ """On non-SQLite backends, the context manager simply yields."""
+
+ with settings.engine.connect() as conn:
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+ with disable_sqlite_fkeys(op) as yielded:
+ assert yielded is op
+
+
+class TestMysqlDropForeignkeyIfExists:
+ """Tests for :func:`mysql_drop_foreignkey_if_exists`."""
+
+ @pytest.mark.backend("mysql")
+ def test_drops_existing_fk(self):
+ """On MySQL, an existing foreign key is dropped."""
+
+ with settings.engine.connect() as conn:
+ conn.execute(text("CREATE TABLE IF NOT EXISTS _test_mig_parent (id INT PRIMARY KEY)"))
+ conn.execute(
+ text(
+ """
+ CREATE TABLE IF NOT EXISTS _test_mig_child (
+ id INT PRIMARY KEY,
+ parent_id INT,
+ CONSTRAINT _test_mig_fk FOREIGN KEY (parent_id)
+ REFERENCES _test_mig_parent(id)
+ )
+ """
+ )
+ )
+ conn.commit()
+
+ try:
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+ mysql_drop_foreignkey_if_exists("_test_mig_fk", "_test_mig_child", op)
+ conn.commit()
+
+ count = conn.execute(
+ text(
+ """
+ SELECT COUNT(*) FROM information_schema.TABLE_CONSTRAINTS
+ WHERE CONSTRAINT_SCHEMA = DATABASE()
+ AND TABLE_NAME = '_test_mig_child'
+ AND CONSTRAINT_NAME = '_test_mig_fk'
+ AND CONSTRAINT_TYPE = 'FOREIGN KEY'
+ """
+ )
+ ).scalar()
+ assert count == 0, "Foreign key should have been dropped"
+ finally:
+ conn.execute(text("DROP TABLE IF EXISTS _test_mig_child"))
+ conn.execute(text("DROP TABLE IF EXISTS _test_mig_parent"))
+ conn.commit()
+
+ @pytest.mark.backend("mysql")
+ def test_noop_for_nonexistent_fk(self):
+ """On MySQL, dropping a non-existent FK does not raise."""
+
+ with settings.engine.connect() as conn:
+ conn.execute(text("CREATE TABLE IF NOT EXISTS _test_mig_nofk (id INT PRIMARY KEY)"))
+ conn.commit()
+
+ try:
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+ mysql_drop_foreignkey_if_exists("nonexistent_fk", "_test_mig_nofk", op)
+ conn.commit()
+ finally:
+ conn.execute(text("DROP TABLE IF EXISTS _test_mig_nofk"))
+ conn.commit()
+
+
+class TestIgnoreSqliteValueError:
+ """Tests for :func:`ignore_sqlite_value_error`."""
+
+ @pytest.mark.backend("sqlite")
+ def test_sqlite_suppresses_value_error(self):
+ """On SQLite, ``ValueError`` is suppressed."""
+
+ with settings.engine.connect() as conn:
+ ctx = MigrationContext.configure(conn)
+ with Operations.context(ctx):
+ cm = ignore_sqlite_value_error()
+ with cm:
+ raise ValueError("should be suppressed on SQLite")
+
+ @pytest.mark.backend("postgres", "mysql")
+ def test_non_sqlite_does_not_suppress(self):
+ """On non-SQLite backends, ``ValueError`` propagates."""
+
+ with settings.engine.connect() as conn:
+ ctx = MigrationContext.configure(conn)
+ with Operations.context(ctx):
+ cm = ignore_sqlite_value_error()
+ with pytest.raises(ValueError, match="should propagate"):
+ with cm:
Review Comment:
```suggestion
with pytest.raises(ValueError, match="should propagate"):
with ignore_sqlite_value_error():
```
##########
scripts/ci/prek/check_migration_patterns.py:
##########
@@ -0,0 +1,383 @@
+#!/usr/bin/env python
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# /// script
+# requires-python = ">=3.10,<3.11"
+# dependencies = [
+# "rich>=13.6.0",
+# ]
+# ///
+"""
+Static analysis checks for Alembic migration anti-patterns.
+
+These checks use AST analysis to detect common migration mistakes that can cause
+silent failures, particularly on SQLite where ``PRAGMA foreign_keys`` has specific
+requirements about transaction state.
+
+Rules
+=====
+
+MIG001 -- DML before ``disable_sqlite_fkeys``
+----------------------------------------------
+
+**What it does:**
+Detects ``op.execute()`` calls containing DML keywords (UPDATE, INSERT, DELETE) that
+appear before a ``with disable_sqlite_fkeys(op):`` block in the same function.
+
+**Why is this bad:**
+On SQLite, any DML statement triggers *autobegin*, starting a transaction. Once a
+transaction is active, ``PRAGMA foreign_keys=off`` (issued by ``disable_sqlite_fkeys``)
+becomes a no-op. This means foreign key checks remain enabled, and
+``batch_alter_table`` (which recreates tables) may fail with foreign key constraint
+violations.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL") # triggers autobegin
+ with disable_sqlite_fkeys(op): # PRAGMA is now a no-op!
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ with disable_sqlite_fkeys(op):
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+
+MIG002 -- DDL before ``disable_sqlite_fkeys``
+----------------------------------------------
+
+**What it does:**
+Detects any Alembic ``op.*`` call (DDL operations like ``op.add_column``,
+``op.drop_column``, ``op.create_table``, ``op.drop_table``, ``op.create_index``,
+``op.batch_alter_table``, etc.) that appears before a
+``with disable_sqlite_fkeys(op):`` block in the same function. Excludes DML calls
+covered by MIG001.
+
+**Why is this bad:**
+DDL operations also trigger *autobegin* on SQLite, making the subsequent
+``PRAGMA foreign_keys=off`` a no-op, exactly like DML.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.add_column("dag_run", sa.Column("created_at", ...)) # triggers autobegin
+ with disable_sqlite_fkeys(op): # PRAGMA is now a no-op!
+ with op.batch_alter_table("backfill") as batch_op:
+ batch_op.alter_column("col", nullable=True)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ with disable_sqlite_fkeys(op):
+ op.add_column("dag_run", sa.Column("created_at", ...))
+ with op.batch_alter_table("backfill") as batch_op:
+ batch_op.alter_column("col", nullable=True)
+
+
+MIG003 -- DML without offline-mode guard
+------------------------------------------
+
+**What it does:**
+Detects ``op.execute()`` calls containing DML keywords (UPDATE, INSERT, DELETE) in
+``upgrade()`` or ``downgrade()`` functions that do not contain a
+``context.is_offline_mode()`` check anywhere in the same function.
+
+**Why is this bad:**
+Alembic's offline mode generates SQL scripts instead of executing against a live
+database. DML statements like ``UPDATE ... WHERE col IS NULL`` depend on actual data
+and produce no useful output in offline mode. Without an ``is_offline_mode()`` guard,
+the generated script includes DML that may fail or silently do nothing when applied
+later.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ if not context.is_offline_mode():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+
+Known Limitation -- Future MIG004 Candidate
+---------------------------------------------
+
+Migrations that perform DML and use ``batch_alter_table`` but never call
+``disable_sqlite_fkeys`` at all are NOT detected by MIG001/MIG002. These migrations
+may silently break SQLite foreign key handling. Detecting this pattern requires
+understanding whether foreign key relationships exist on the affected tables, which
+is beyond AST-level analysis. This is tracked as a future MIG004 candidate.
+
+
+Suppression
+===========
+
+Add ``# noqa: MIG0XX`` to a line to suppress the corresponding check for that line.
+Multiple codes can be comma-separated: ``# noqa: MIG001, MIG003``.
+
+Include a brief reason after ``--``::
+
+ # noqa: MIG003 -- simple UPDATE safe in offline mode
+"""
+
+from __future__ import annotations
+
+import ast
+import re
+import sys
+from pathlib import Path
+
+from rich.console import Console
+
+console = Console(color_system="standard", width=200)
+
+DML_KEYWORDS = ("UPDATE", "INSERT", "DELETE")
+
+
+def _get_noqa_codes(line: str) -> set[str]:
+ """Extract noqa codes from a source line."""
+ match = re.search(r"#\s*noqa:\s*([\w,\s]+)", line)
+ if match:
+ return {code.strip() for code in match.group(1).split(",")}
+ return set()
+
+
+def _is_dml_string(node: ast.expr) -> bool:
+ """Check if an AST expression is a string literal starting with a DML keyword."""
+ if isinstance(node, ast.Constant) and isinstance(node.value, str):
+ parts = node.value.strip().upper().split(None, 1)
+ return len(parts) > 1 and parts[0] in DML_KEYWORDS
+ # f-strings: check the first string fragment
+ if isinstance(node, ast.JoinedStr):
+ for value in node.values:
+ if isinstance(value, ast.Constant) and isinstance(value.value, str):
+ parts = value.value.strip().upper().split(None, 1)
+ if len(parts) > 1 and parts[0] in DML_KEYWORDS:
+ return True
+ break # only the leading fragment matters
+ return False
+
+
+def _is_op_execute_with_dml(node: ast.Call) -> bool:
+ """Check if a Call node is ``op.execute(<DML string>)``."""
+ if not (
+ isinstance(node.func, ast.Attribute)
+ and isinstance(node.func.value, ast.Name)
+ and node.func.value.id == "op"
+ and node.func.attr == "execute"
+ ):
+ return False
+ if not node.args:
+ return False
+ arg = node.args[0]
+ # Direct string: op.execute("UPDATE ...")
+ if _is_dml_string(arg):
+ return True
+ # Wrapped: op.execute(text("UPDATE ...")) or op.execute(sa.text("UPDATE ..."))
+ if isinstance(arg, ast.Call) and arg.args:
+ return _is_dml_string(arg.args[0])
+ return False
+
+
+def _is_op_call(node: ast.Call) -> bool:
+ """Check if a Call node is ``op.<something>(...)``."""
+ return (
+ isinstance(node.func, ast.Attribute)
+ and isinstance(node.func.value, ast.Name)
+ and node.func.value.id == "op"
+ )
+
+
+def _find_disable_sqlite_fkeys_line(func_node: ast.FunctionDef) -> int | None:
+ """Find the line number of the first ``with disable_sqlite_fkeys(op):`` in a function."""
+ # Collect all matches then take min() — ast.walk BFS order does not guarantee
+ # line-number order when guards appear at different nesting depths.
+ found: list[int] = []
+ for node in ast.walk(func_node):
+ if not isinstance(node, ast.With):
+ continue
+ for item in node.items:
+ ctx = item.context_expr
+ if isinstance(ctx, ast.Call):
+ func = ctx.func
+ if (isinstance(func, ast.Name) and func.id == "disable_sqlite_fkeys") or (
+ isinstance(func, ast.Attribute) and func.attr == "disable_sqlite_fkeys"
+ ):
+ found.append(node.lineno)
+ return min(found) if found else None
+
+
+def _has_offline_mode_check(func_node: ast.FunctionDef) -> bool:
+ """Check if function contains ``context.is_offline_mode()`` call."""
+ for node in ast.walk(func_node):
+ if isinstance(node, ast.Call):
+ func = node.func
+ if isinstance(func, ast.Attribute) and func.attr == "is_offline_mode":
+ return True
+ return False
+
+
+def _get_upgrade_downgrade_functions(tree: ast.Module) -> list[ast.FunctionDef]:
+ """Get ``upgrade()`` and ``downgrade()`` function definitions from module."""
+ return [
+ node
+ for node in ast.iter_child_nodes(tree)
+ if isinstance(node, ast.FunctionDef) and node.name in ("upgrade", "downgrade")
+ ]
+
+
+def _line_has_noqa(lines: list[str], lineno: int, code: str) -> bool:
+ """Check if a source line has a noqa suppression for the given code."""
+ if lineno < 1 or lineno > len(lines):
+ return False
+ return code in _get_noqa_codes(lines[lineno - 1])
+
+
+# ---------------------------------------------------------------------------
+# Check functions
+# ---------------------------------------------------------------------------
+
+
+def check_mig001(filepath: Path) -> list[str]:
+ """MIG001: DML before ``disable_sqlite_fkeys``."""
+ source = filepath.read_text()
+ lines = source.splitlines()
+ tree = ast.parse(source, filename=str(filepath))
+ errors: list[str] = []
+
+ for func in _get_upgrade_downgrade_functions(tree):
+ guard_line = _find_disable_sqlite_fkeys_line(func)
+ if guard_line is None:
+ continue # MIG001 only fires when disable_sqlite_fkeys IS present
+
+ for node in ast.walk(func):
+ if not isinstance(node, ast.Call):
+ continue
+ if node.lineno >= guard_line:
+ continue
+ if _is_op_execute_with_dml(node) and not _line_has_noqa(lines, node.lineno, "MIG001"):
+ errors.append(
+ f"MIG001 {func.name}(): op.execute() with DML at line {node.lineno} "
+ f"before disable_sqlite_fkeys at line {guard_line}"
+ )
+
+ return errors
+
+
+def check_mig002(filepath: Path) -> list[str]:
+ """MIG002: DDL before ``disable_sqlite_fkeys``."""
+ source = filepath.read_text()
+ lines = source.splitlines()
+ tree = ast.parse(source, filename=str(filepath))
+ errors: list[str] = []
+
+ for func in _get_upgrade_downgrade_functions(tree):
+ guard_line = _find_disable_sqlite_fkeys_line(func)
+ if guard_line is None:
+ continue # MIG002 only fires when disable_sqlite_fkeys IS present
+
+ for node in ast.walk(func):
+ if not isinstance(node, ast.Call):
+ continue
+ if node.lineno >= guard_line:
+ continue
+ # Skip DML calls — covered by MIG001
+ if _is_op_execute_with_dml(node):
+ continue
+ if _is_op_call(node) and not _line_has_noqa(lines, node.lineno, "MIG002"):
+ attr = node.func.attr if isinstance(node.func, ast.Attribute) else "?"
+ errors.append(
+ f"MIG002 {func.name}(): op.{attr}() at line {node.lineno} "
+ f"before disable_sqlite_fkeys at line {guard_line}"
+ )
+
+ return errors
+
+
+def check_mig003(filepath: Path) -> list[str]:
+ """MIG003: DML without offline-mode guard."""
+ source = filepath.read_text()
+ lines = source.splitlines()
+ tree = ast.parse(source, filename=str(filepath))
+ errors: list[str] = []
+
+ for func in _get_upgrade_downgrade_functions(tree):
+ if _has_offline_mode_check(func):
+ continue # function has the guard
+
+ for node in ast.walk(func):
+ if not isinstance(node, ast.Call):
+ continue
+ if _is_op_execute_with_dml(node) and not _line_has_noqa(lines, node.lineno, "MIG003"):
+ errors.append(
+ f"MIG003 {func.name}(): op.execute() with DML at line {node.lineno} "
+ f"without context.is_offline_mode() guard"
+ )
+
+ return errors
+
+
+# ---------------------------------------------------------------------------
+# CLI entry point
+# ---------------------------------------------------------------------------
+
+
+def main() -> int:
+ """Check migration files for anti-patterns. Exit 0 if clean, 1 if violations found."""
+ checks = [check_mig001, check_mig002, check_mig003]
Review Comment:
```suggestion
check_fns = [check_mig001, check_mig002, check_mig003]
```
nit: easier to read IMO. i was not sure what this `checks` are when readding line 371. but non-block
##########
scripts/tests/ci/prek/test_check_migration_patterns.py:
##########
@@ -0,0 +1,333 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Unit tests for ``scripts/ci/prek/check_migration_patterns.py``."""
+
+from __future__ import annotations
+
+import sys
+from pathlib import Path
+
+from ci.prek.check_migration_patterns import (
+ _get_noqa_codes,
+ _line_has_noqa,
+ check_mig001,
+ check_mig002,
+ check_mig003,
+ main,
+)
+
+# ---------------------------------------------------------------------------
+# Helpers
+# ---------------------------------------------------------------------------
+
+
+def write_migration(tmp_path: Path, source: str) -> Path:
+ """Write *source* to a temp file that looks like a migration file."""
+ p = tmp_path / "0099_test_migration.py"
+ p.write_text(source)
+ return p
+
+
+# ---------------------------------------------------------------------------
+# _get_noqa_codes
+# ---------------------------------------------------------------------------
+
+
+class TestGetNoqaCodes:
+ def test_single_code(self):
+ assert _get_noqa_codes("op.execute(...) # noqa: MIG001") == {"MIG001"}
+
+ def test_multiple_codes(self):
+ assert _get_noqa_codes("op.execute(...) # noqa: MIG001, MIG003") == {"MIG001", "MIG003"}
+
+ def test_code_with_reason_suffix(self):
+ # Reason after '--' should NOT be included in the code set.
+ codes = _get_noqa_codes("op.execute(...) # noqa: MIG003 -- safe in offline mode")
+ assert "MIG003" in codes
+ assert "--" not in "".join(codes)
+ assert "safe" not in "".join(codes)
+
+ def test_no_noqa_comment(self):
+ assert _get_noqa_codes("op.execute('UPDATE foo SET x=1')") == set()
+
+ def test_empty_string(self):
+ assert _get_noqa_codes("") == set()
+
+
+# ---------------------------------------------------------------------------
+# _line_has_noqa
+# ---------------------------------------------------------------------------
+
+
+class TestLineHasNoqa:
+ def test_line_has_matching_code(self):
+ lines = ["op.execute('UPDATE ...') # noqa: MIG001"]
+ assert _line_has_noqa(lines, 1, "MIG001") is True
+
+ def test_line_has_different_code(self):
+ lines = ["op.execute('UPDATE ...') # noqa: MIG002"]
+ assert _line_has_noqa(lines, 1, "MIG001") is False
+
+ def test_out_of_bounds_zero(self):
+ assert _line_has_noqa(["some line"], 0, "MIG001") is False
+
+ def test_out_of_bounds_beyond_end(self):
+ assert _line_has_noqa(["some line"], 2, "MIG001") is False
Review Comment:
looks like something we can parameterize
##########
airflow-core/tests/unit/migrations/test_migration_utils.py:
##########
@@ -0,0 +1,176 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Unit tests for ``airflow.migrations.utils``."""
+
+from __future__ import annotations
+
+import pytest
+from alembic.migration import MigrationContext
+from alembic.operations import Operations
+from sqlalchemy import text
+
+from airflow import settings
+from airflow.migrations.utils import (
+ disable_sqlite_fkeys,
+ ignore_sqlite_value_error,
+ mysql_drop_foreignkey_if_exists,
+)
+
+pytestmark = pytest.mark.db_test
+
+
+class TestDisableSqliteFkeys:
+ """Tests for :func:`disable_sqlite_fkeys`."""
+
+ def test_yields_op(self):
+ """The context manager yields the *op* object on every backend."""
+ with settings.engine.connect() as conn:
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+ with disable_sqlite_fkeys(op) as yielded:
+ assert yielded is op
+
+ @pytest.mark.backend("sqlite")
+ def test_sqlite_turns_off_fkeys(self):
+ """On SQLite, ``PRAGMA foreign_keys`` is OFF inside the context."""
+
+ with settings.engine.connect() as conn:
+ # Ensure foreign_keys is ON before entering the context manager.
+ conn.execute(text("PRAGMA foreign_keys=ON"))
+ conn.commit()
+
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+
+ with disable_sqlite_fkeys(op):
+ result = conn.execute(text("PRAGMA foreign_keys")).scalar()
+ assert result == 0, "foreign_keys should be OFF inside disable_sqlite_fkeys"
+
+ result = conn.execute(text("PRAGMA foreign_keys")).scalar()
+ assert result == 1, "foreign_keys should be restored to ON after disable_sqlite_fkeys"
+
+ @pytest.mark.backend("postgres", "mysql")
+ def test_non_sqlite_is_noop(self):
+ """On non-SQLite backends, the context manager simply yields."""
+
+ with settings.engine.connect() as conn:
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+ with disable_sqlite_fkeys(op) as yielded:
+ assert yielded is op
+
+
+class TestMysqlDropForeignkeyIfExists:
+ """Tests for :func:`mysql_drop_foreignkey_if_exists`."""
+
+ @pytest.mark.backend("mysql")
+ def test_drops_existing_fk(self):
+ """On MySQL, an existing foreign key is dropped."""
+
+ with settings.engine.connect() as conn:
+ conn.execute(text("CREATE TABLE IF NOT EXISTS _test_mig_parent (id INT PRIMARY KEY)"))
+ conn.execute(
+ text(
+ """
+ CREATE TABLE IF NOT EXISTS _test_mig_child (
+ id INT PRIMARY KEY,
+ parent_id INT,
+ CONSTRAINT _test_mig_fk FOREIGN KEY (parent_id)
+ REFERENCES _test_mig_parent(id)
+ )
+ """
+ )
+ )
+ conn.commit()
+
+ try:
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+ mysql_drop_foreignkey_if_exists("_test_mig_fk", "_test_mig_child", op)
+ conn.commit()
+
+ count = conn.execute(
+ text(
+ """
+ SELECT COUNT(*) FROM information_schema.TABLE_CONSTRAINTS
+ WHERE CONSTRAINT_SCHEMA = DATABASE()
+ AND TABLE_NAME = '_test_mig_child'
+ AND CONSTRAINT_NAME = '_test_mig_fk'
+ AND CONSTRAINT_TYPE = 'FOREIGN KEY'
+ """
+ )
+ ).scalar()
+ assert count == 0, "Foreign key should have been dropped"
+ finally:
+ conn.execute(text("DROP TABLE IF EXISTS _test_mig_child"))
+ conn.execute(text("DROP TABLE IF EXISTS _test_mig_parent"))
+ conn.commit()
+
+ @pytest.mark.backend("mysql")
+ def test_noop_for_nonexistent_fk(self):
+ """On MySQL, dropping a non-existent FK does not raise."""
+
+ with settings.engine.connect() as conn:
+ conn.execute(text("CREATE TABLE IF NOT EXISTS _test_mig_nofk (id INT PRIMARY KEY)"))
+ conn.commit()
+
+ try:
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+ mysql_drop_foreignkey_if_exists("nonexistent_fk", "_test_mig_nofk", op)
+ conn.commit()
+ finally:
+ conn.execute(text("DROP TABLE IF EXISTS _test_mig_nofk"))
+ conn.commit()
+
+
+class TestIgnoreSqliteValueError:
+ """Tests for :func:`ignore_sqlite_value_error`."""
+
+ @pytest.mark.backend("sqlite")
+ def test_sqlite_suppresses_value_error(self):
+ """On SQLite, ``ValueError`` is suppressed."""
+
+ with settings.engine.connect() as conn:
+ ctx = MigrationContext.configure(conn)
+ with Operations.context(ctx):
+ cm = ignore_sqlite_value_error()
+ with cm:
Review Comment:
```suggestion
with ignore_sqlite_value_error():
```
##########
scripts/tests/ci/prek/test_check_migration_patterns.py:
##########
@@ -0,0 +1,333 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Unit tests for ``scripts/ci/prek/check_migration_patterns.py``."""
+
+from __future__ import annotations
+
+import sys
+from pathlib import Path
+
+from ci.prek.check_migration_patterns import (
+ _get_noqa_codes,
+ _line_has_noqa,
+ check_mig001,
+ check_mig002,
+ check_mig003,
+ main,
+)
+
+# ---------------------------------------------------------------------------
+# Helpers
+# ---------------------------------------------------------------------------
+
+
+def write_migration(tmp_path: Path, source: str) -> Path:
+ """Write *source* to a temp file that looks like a migration file."""
+ p = tmp_path / "0099_test_migration.py"
+ p.write_text(source)
+ return p
+
+
+# ---------------------------------------------------------------------------
+# _get_noqa_codes
+# ---------------------------------------------------------------------------
+
+
+class TestGetNoqaCodes:
+ def test_single_code(self):
+ assert _get_noqa_codes("op.execute(...) # noqa: MIG001") == {"MIG001"}
+
+ def test_multiple_codes(self):
+ assert _get_noqa_codes("op.execute(...) # noqa: MIG001, MIG003") == {"MIG001", "MIG003"}
+
+ def test_code_with_reason_suffix(self):
+ # Reason after '--' should NOT be included in the code set.
+ codes = _get_noqa_codes("op.execute(...) # noqa: MIG003 -- safe in offline mode")
+ assert "MIG003" in codes
+ assert "--" not in "".join(codes)
+ assert "safe" not in "".join(codes)
+
+ def test_no_noqa_comment(self):
+ assert _get_noqa_codes("op.execute('UPDATE foo SET x=1')") == set()
+
+ def test_empty_string(self):
+ assert _get_noqa_codes("") == set()
+
+
+# ---------------------------------------------------------------------------
+# _line_has_noqa
+# ---------------------------------------------------------------------------
+
+
+class TestLineHasNoqa:
+ def test_line_has_matching_code(self):
+ lines = ["op.execute('UPDATE ...') # noqa: MIG001"]
+ assert _line_has_noqa(lines, 1, "MIG001") is True
+
+ def test_line_has_different_code(self):
+ lines = ["op.execute('UPDATE ...') # noqa: MIG002"]
+ assert _line_has_noqa(lines, 1, "MIG001") is False
+
+ def test_out_of_bounds_zero(self):
+ assert _line_has_noqa(["some line"], 0, "MIG001") is False
+
+ def test_out_of_bounds_beyond_end(self):
+ assert _line_has_noqa(["some line"], 2, "MIG001") is False
+
+
+# ---------------------------------------------------------------------------
+# check_mig001
+# ---------------------------------------------------------------------------
+
+
+class TestCheckMig001:
Review Comment:
we probably can parametrize this as well. not all, but test cases can be grouped
##########
scripts/tests/ci/prek/test_check_migration_patterns.py:
##########
@@ -0,0 +1,333 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Unit tests for ``scripts/ci/prek/check_migration_patterns.py``."""
+
+from __future__ import annotations
+
+import sys
+from pathlib import Path
+
+from ci.prek.check_migration_patterns import (
+ _get_noqa_codes,
+ _line_has_noqa,
+ check_mig001,
+ check_mig002,
+ check_mig003,
+ main,
+)
+
+# ---------------------------------------------------------------------------
+# Helpers
+# ---------------------------------------------------------------------------
+
+
+def write_migration(tmp_path: Path, source: str) -> Path:
+ """Write *source* to a temp file that looks like a migration file."""
+ p = tmp_path / "0099_test_migration.py"
+ p.write_text(source)
+ return p
+
+
+# ---------------------------------------------------------------------------
+# _get_noqa_codes
+# ---------------------------------------------------------------------------
Review Comment:
I don't think we need these
```suggestion
```
we can move them test docstring if we really want it
--
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] Add static analysis tests for DB migration anti-patterns [airflow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #64972:
URL: https://github.com/apache/airflow/pull/64972#issuecomment-4356068835
### Backport failed to create: v3-2-test. View the failure log <a href='https://github.com/apache/airflow/actions/runs/25188634557'> Run details </a>
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-2-test</td>
<td><a href="https://github.com/apache/airflow/commit/41caf5ee755f4c9d317c96b3248c744028341b36"><img src='https://img.shields.io/badge/Commit-41caf5e-red' alt='Commit Link'></a></td>
</tr>
</table>
You can attempt to backport this manually by running:
```bash
cherry_picker 41caf5e v3-2-test
```
This should apply the commit to the v3-2-test branch and leave the commit in conflict state marking
the files that need manual conflict resolution.
After you have resolved the conflicts, you can continue the backport process by running:
```bash
cherry_picker --continue
```
If you don't have cherry-picker installed, see the [installation guide](https://github.com/apache/airflow/blob/main/dev/README_AIRFLOW3_DEV.md#how-to-backport-pr-with-cherry-picker-cli).
--
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] Add static analysis tests for DB migration anti-patterns [airflow]
Posted by "Dev-iL (via GitHub)" <gi...@apache.org>.
Dev-iL commented on code in PR #64972:
URL: https://github.com/apache/airflow/pull/64972#discussion_r3064202893
##########
airflow-core/tests/unit/migrations/test_migration_utils.py:
##########
@@ -0,0 +1,183 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Unit tests for ``airflow.migrations.utils``."""
+
+from __future__ import annotations
+
+import pytest
+from alembic.migration import MigrationContext
+from alembic.operations import Operations
+from sqlalchemy import text
+
+from airflow import settings
+from airflow.migrations.utils import (
+ disable_sqlite_fkeys,
+ ignore_sqlite_value_error,
+ mysql_drop_foreignkey_if_exists,
+)
+
+pytestmark = pytest.mark.db_test
+
+
+def _dialect() -> str:
+ assert settings.engine is not None # guaranteed by db_test marker
+ return settings.engine.dialect.name
+
+
+class TestDisableSqliteFkeys:
+ """Tests for :func:`disable_sqlite_fkeys`."""
+
+ def test_yields_op(self):
+ """The context manager yields the *op* object on every backend."""
+ with settings.engine.connect() as conn:
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+ with disable_sqlite_fkeys(op) as yielded:
+ assert yielded is op
+
+ def test_sqlite_turns_off_fkeys(self):
+ """On SQLite, ``PRAGMA foreign_keys`` is OFF inside the context."""
+ if _dialect() != "sqlite":
+ pytest.skip("SQLite-specific test")
+
+ with settings.engine.connect() as conn:
+ # Ensure foreign_keys is ON before entering the context manager.
+ conn.execute(text("PRAGMA foreign_keys=ON"))
+ conn.commit()
+
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+
+ with disable_sqlite_fkeys(op):
+ result = conn.execute(text("PRAGMA foreign_keys")).scalar()
+ assert result == 0, "foreign_keys should be OFF inside disable_sqlite_fkeys"
+
+ result = conn.execute(text("PRAGMA foreign_keys")).scalar()
+ assert result == 1, "foreign_keys should be restored to ON after disable_sqlite_fkeys"
+
+ def test_non_sqlite_is_noop(self):
+ """On non-SQLite backends, the context manager simply yields."""
+ if _dialect() == "sqlite":
+ pytest.skip("Non-SQLite backends only")
+
Review Comment:
Implicitly done: since `_dialect()` is gone entirely, no renaming is needed.
--
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] Add static analysis tests for DB migration anti-patterns [airflow]
Posted by "Dev-iL (via GitHub)" <gi...@apache.org>.
Dev-iL commented on code in PR #64972:
URL: https://github.com/apache/airflow/pull/64972#discussion_r3064195126
##########
airflow-core/tests/unit/migrations/test_migration_utils.py:
##########
@@ -0,0 +1,183 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Unit tests for ``airflow.migrations.utils``."""
+
+from __future__ import annotations
+
+import pytest
+from alembic.migration import MigrationContext
+from alembic.operations import Operations
+from sqlalchemy import text
+
+from airflow import settings
+from airflow.migrations.utils import (
+ disable_sqlite_fkeys,
+ ignore_sqlite_value_error,
+ mysql_drop_foreignkey_if_exists,
+)
+
+pytestmark = pytest.mark.db_test
+
+
+def _dialect() -> str:
+ assert settings.engine is not None # guaranteed by db_test marker
Review Comment:
Took a different approach: removed `_dialect()` entirely. All backend-specific tests are now gated by `@pytest.mark.backend(...)` markers, so the helper is no longer needed at all.
--
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] Add static analysis tests for DB migration anti-patterns [airflow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #64972:
URL: https://github.com/apache/airflow/pull/64972#discussion_r3066503513
##########
scripts/ci/prek/check_migration_patterns.py:
##########
@@ -0,0 +1,383 @@
+#!/usr/bin/env python
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# /// script
+# requires-python = ">=3.10,<3.11"
+# dependencies = [
+# "rich>=13.6.0",
+# ]
+# ///
+"""
+Static analysis checks for Alembic migration anti-patterns.
+
+These checks use AST analysis to detect common migration mistakes that can cause
+silent failures, particularly on SQLite where ``PRAGMA foreign_keys`` has specific
+requirements about transaction state.
+
+Rules
+=====
+
+MIG001 -- DML before ``disable_sqlite_fkeys``
+----------------------------------------------
+
+**What it does:**
+Detects ``op.execute()`` calls containing DML keywords (UPDATE, INSERT, DELETE) that
+appear before a ``with disable_sqlite_fkeys(op):`` block in the same function.
+
+**Why is this bad:**
+On SQLite, any DML statement triggers *autobegin*, starting a transaction. Once a
+transaction is active, ``PRAGMA foreign_keys=off`` (issued by ``disable_sqlite_fkeys``)
+becomes a no-op. This means foreign key checks remain enabled, and
+``batch_alter_table`` (which recreates tables) may fail with foreign key constraint
+violations.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL") # triggers autobegin
+ with disable_sqlite_fkeys(op): # PRAGMA is now a no-op!
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ with disable_sqlite_fkeys(op):
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+
+MIG002 -- DDL before ``disable_sqlite_fkeys``
+----------------------------------------------
+
+**What it does:**
+Detects any Alembic ``op.*`` call (DDL operations like ``op.add_column``,
+``op.drop_column``, ``op.create_table``, ``op.drop_table``, ``op.create_index``,
+``op.batch_alter_table``, etc.) that appears before a
+``with disable_sqlite_fkeys(op):`` block in the same function. Excludes DML calls
+covered by MIG001.
+
+**Why is this bad:**
+DDL operations also trigger *autobegin* on SQLite, making the subsequent
+``PRAGMA foreign_keys=off`` a no-op, exactly like DML.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.add_column("dag_run", sa.Column("created_at", ...)) # triggers autobegin
+ with disable_sqlite_fkeys(op): # PRAGMA is now a no-op!
+ with op.batch_alter_table("backfill") as batch_op:
+ batch_op.alter_column("col", nullable=True)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ with disable_sqlite_fkeys(op):
+ op.add_column("dag_run", sa.Column("created_at", ...))
+ with op.batch_alter_table("backfill") as batch_op:
+ batch_op.alter_column("col", nullable=True)
+
+
+MIG003 -- DML without offline-mode guard
+------------------------------------------
+
+**What it does:**
+Detects ``op.execute()`` calls containing DML keywords (UPDATE, INSERT, DELETE) in
+``upgrade()`` or ``downgrade()`` functions that do not contain a
+``context.is_offline_mode()`` check anywhere in the same function.
+
+**Why is this bad:**
+Alembic's offline mode generates SQL scripts instead of executing against a live
+database. DML statements like ``UPDATE ... WHERE col IS NULL`` depend on actual data
+and produce no useful output in offline mode. Without an ``is_offline_mode()`` guard,
+the generated script includes DML that may fail or silently do nothing when applied
+later.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ if not context.is_offline_mode():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+
+Known Limitation -- Future MIG004 Candidate
+---------------------------------------------
+
+Migrations that perform DML and use ``batch_alter_table`` but never call
+``disable_sqlite_fkeys`` at all are NOT detected by MIG001/MIG002. These migrations
+may silently break SQLite foreign key handling. Detecting this pattern requires
+understanding whether foreign key relationships exist on the affected tables, which
+is beyond AST-level analysis. This is tracked as a future MIG004 candidate.
+
+
+Suppression
+===========
+
+Add ``# noqa: MIG0XX`` to a line to suppress the corresponding check for that line.
+Multiple codes can be comma-separated: ``# noqa: MIG001, MIG003``.
+
+Include a brief reason after ``--``::
+
+ # noqa: MIG003 -- simple UPDATE safe in offline mode
+"""
+
+from __future__ import annotations
+
+import ast
+import re
+import sys
+from pathlib import Path
+
+from rich.console import Console
+
+console = Console(color_system="standard", width=200)
+
+DML_KEYWORDS = ("UPDATE ", "INSERT ", "DELETE ")
+
+
+def _get_noqa_codes(line: str) -> set[str]:
+ """Extract noqa codes from a source line."""
+ match = re.search(r"#\s*noqa:\s*([\w,\s]+)", line)
+ if match:
+ return {code.strip() for code in match.group(1).split(",")}
+ return set()
+
+
+def _is_dml_string(node: ast.expr) -> bool:
+ """Check if an AST expression is a string literal starting with a DML keyword."""
+ if isinstance(node, ast.Constant) and isinstance(node.value, str):
+ stripped = node.value.strip().upper()
+ return any(stripped.startswith(kw) for kw in DML_KEYWORDS)
Review Comment:
DML detection uses `startswith` with keywords that include a trailing space (e.g. `\"DELETE \"`). This will miss common multi-line SQL styles such as `\"DELETE\\nFROM ...\"` / `\"INSERT\\nINTO ...\"`, leading to false negatives for MIG001/MIG003. Consider switching to a regex like `r'^(UPDATE|INSERT|DELETE)\\b'` against the stripped/uppercased prefix so any whitespace formatting is handled.
##########
scripts/ci/prek/check_migration_patterns.py:
##########
@@ -0,0 +1,383 @@
+#!/usr/bin/env python
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# /// script
+# requires-python = ">=3.10,<3.11"
+# dependencies = [
+# "rich>=13.6.0",
+# ]
+# ///
+"""
+Static analysis checks for Alembic migration anti-patterns.
+
+These checks use AST analysis to detect common migration mistakes that can cause
+silent failures, particularly on SQLite where ``PRAGMA foreign_keys`` has specific
+requirements about transaction state.
+
+Rules
+=====
+
+MIG001 -- DML before ``disable_sqlite_fkeys``
+----------------------------------------------
+
+**What it does:**
+Detects ``op.execute()`` calls containing DML keywords (UPDATE, INSERT, DELETE) that
+appear before a ``with disable_sqlite_fkeys(op):`` block in the same function.
+
+**Why is this bad:**
+On SQLite, any DML statement triggers *autobegin*, starting a transaction. Once a
+transaction is active, ``PRAGMA foreign_keys=off`` (issued by ``disable_sqlite_fkeys``)
+becomes a no-op. This means foreign key checks remain enabled, and
+``batch_alter_table`` (which recreates tables) may fail with foreign key constraint
+violations.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL") # triggers autobegin
+ with disable_sqlite_fkeys(op): # PRAGMA is now a no-op!
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ with disable_sqlite_fkeys(op):
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+
+MIG002 -- DDL before ``disable_sqlite_fkeys``
+----------------------------------------------
+
+**What it does:**
+Detects any Alembic ``op.*`` call (DDL operations like ``op.add_column``,
+``op.drop_column``, ``op.create_table``, ``op.drop_table``, ``op.create_index``,
+``op.batch_alter_table``, etc.) that appears before a
+``with disable_sqlite_fkeys(op):`` block in the same function. Excludes DML calls
+covered by MIG001.
+
+**Why is this bad:**
+DDL operations also trigger *autobegin* on SQLite, making the subsequent
+``PRAGMA foreign_keys=off`` a no-op, exactly like DML.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.add_column("dag_run", sa.Column("created_at", ...)) # triggers autobegin
+ with disable_sqlite_fkeys(op): # PRAGMA is now a no-op!
+ with op.batch_alter_table("backfill") as batch_op:
+ batch_op.alter_column("col", nullable=True)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ with disable_sqlite_fkeys(op):
+ op.add_column("dag_run", sa.Column("created_at", ...))
+ with op.batch_alter_table("backfill") as batch_op:
+ batch_op.alter_column("col", nullable=True)
+
+
+MIG003 -- DML without offline-mode guard
+------------------------------------------
+
+**What it does:**
+Detects ``op.execute()`` calls containing DML keywords (UPDATE, INSERT, DELETE) in
+``upgrade()`` or ``downgrade()`` functions that do not contain a
+``context.is_offline_mode()`` check anywhere in the same function.
+
+**Why is this bad:**
+Alembic's offline mode generates SQL scripts instead of executing against a live
+database. DML statements like ``UPDATE ... WHERE col IS NULL`` depend on actual data
+and produce no useful output in offline mode. Without an ``is_offline_mode()`` guard,
+the generated script includes DML that may fail or silently do nothing when applied
+later.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ if not context.is_offline_mode():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+
+Known Limitation -- Future MIG004 Candidate
+---------------------------------------------
+
+Migrations that perform DML and use ``batch_alter_table`` but never call
+``disable_sqlite_fkeys`` at all are NOT detected by MIG001/MIG002. These migrations
+may silently break SQLite foreign key handling. Detecting this pattern requires
+understanding whether foreign key relationships exist on the affected tables, which
+is beyond AST-level analysis. This is tracked as a future MIG004 candidate.
+
+
+Suppression
+===========
+
+Add ``# noqa: MIG0XX`` to a line to suppress the corresponding check for that line.
+Multiple codes can be comma-separated: ``# noqa: MIG001, MIG003``.
+
+Include a brief reason after ``--``::
+
+ # noqa: MIG003 -- simple UPDATE safe in offline mode
+"""
+
+from __future__ import annotations
+
+import ast
+import re
+import sys
+from pathlib import Path
+
+from rich.console import Console
+
+console = Console(color_system="standard", width=200)
+
+DML_KEYWORDS = ("UPDATE ", "INSERT ", "DELETE ")
+
+
+def _get_noqa_codes(line: str) -> set[str]:
+ """Extract noqa codes from a source line."""
+ match = re.search(r"#\s*noqa:\s*([\w,\s]+)", line)
+ if match:
+ return {code.strip() for code in match.group(1).split(",")}
+ return set()
+
+
+def _is_dml_string(node: ast.expr) -> bool:
+ """Check if an AST expression is a string literal starting with a DML keyword."""
+ if isinstance(node, ast.Constant) and isinstance(node.value, str):
+ stripped = node.value.strip().upper()
+ return any(stripped.startswith(kw) for kw in DML_KEYWORDS)
+ # f-strings: check the first string fragment
+ if isinstance(node, ast.JoinedStr):
+ for value in node.values:
+ if isinstance(value, ast.Constant) and isinstance(value.value, str):
+ stripped = value.value.strip().upper()
+ if any(stripped.startswith(kw) for kw in DML_KEYWORDS):
+ return True
+ break # only the leading fragment matters
+ return False
+
+
+def _is_op_execute_with_dml(node: ast.Call) -> bool:
+ """Check if a Call node is ``op.execute(<DML string>)``."""
+ if not (
+ isinstance(node.func, ast.Attribute)
+ and isinstance(node.func.value, ast.Name)
+ and node.func.value.id == "op"
+ and node.func.attr == "execute"
+ ):
+ return False
+ if not node.args:
+ return False
+ arg = node.args[0]
+ # Direct string: op.execute("UPDATE ...")
+ if _is_dml_string(arg):
+ return True
+ # Wrapped: op.execute(text("UPDATE ...")) or op.execute(sa.text("UPDATE ..."))
+ if isinstance(arg, ast.Call) and arg.args:
+ return _is_dml_string(arg.args[0])
+ return False
+
+
+def _is_op_call(node: ast.Call) -> bool:
+ """Check if a Call node is ``op.<something>(...)``."""
+ return (
+ isinstance(node.func, ast.Attribute)
+ and isinstance(node.func.value, ast.Name)
+ and node.func.value.id == "op"
+ )
Review Comment:
MIG002 currently treats any `op.<anything>(...)` as DDL (via `_is_op_call`), which will also match non-DDL helpers like `op.get_bind()`, `op.get_context()`, etc. This will produce false positives and force noisy suppressions/refactors. Consider restricting MIG002 to a concrete allowlist of DDL-ish Alembic operations (e.g. `add_column`, `drop_column`, `create_table`, `drop_table`, `create_index`, `drop_index`, `batch_alter_table`, `rename_table`, `alter_column`, `create_foreign_key`, ...), and explicitly excluding known non-DDL methods such as `get_bind`.
##########
.pre-commit-config.yaml:
##########
@@ -1034,6 +1034,14 @@ repos:
pass_filenames: false
files: ^.*\.py$
require_serial: true
+ - id: check-migration-patterns
+ name: Check migration files for anti-patterns (MIG001/MIG002/MIG003)
+ entry: ./scripts/ci/prek/check_migration_patterns.py
+ language: python
+ pass_filenames: true
+ files: ^airflow-core/src/airflow/migrations/versions/.*\.py$
+ additional_dependencies: [rich>=13.6.0]
+ require_serial: true
Review Comment:
If `check_migration_patterns.py` truly requires a specific Python minor version (per its PEP 723 header), this prek hook should pin `language_version` accordingly; otherwise developers/CI may execute it under a different interpreter than intended. Either add an explicit `language_version` here to match the requirement, or (preferably) relax the script’s `requires-python` so the hook can run under the repo’s standard tooling Python.
##########
scripts/ci/prek/check_migration_patterns.py:
##########
@@ -0,0 +1,383 @@
+#!/usr/bin/env python
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# /// script
+# requires-python = ">=3.10,<3.11"
+# dependencies = [
+# "rich>=13.6.0",
+# ]
Review Comment:
The PEP 723 `requires-python = \">=3.10,<3.11\"` upper-bound is unusually restrictive for a repo that typically runs tooling on newer Python versions too, and it can prevent running this script via PEP-723-capable runners (e.g. `uv run`) on Python 3.11+. If there isn’t a hard incompatibility, widen/remove the `<3.11` cap (e.g. `>=3.10`) or align it with the project’s supported tooling Python versions.
##########
scripts/ci/prek/check_new_airflow_exception_usage.py:
##########
@@ -59,7 +59,7 @@
from rich.console import Console
from rich.panel import Panel
-console = Console()
+console = Console(color_system="standard", width=200)
Review Comment:
This console configuration change is unrelated to migration-pattern static analysis (MIG001/2/3) and isn’t mentioned in the PR description. Consider either updating the PR description to include the motivation for this change, or moving it into a separate PR to keep scope focused.
##########
scripts/ci/prek/check_migration_patterns.py:
##########
@@ -0,0 +1,383 @@
+#!/usr/bin/env python
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# /// script
+# requires-python = ">=3.10,<3.11"
+# dependencies = [
+# "rich>=13.6.0",
+# ]
+# ///
+"""
+Static analysis checks for Alembic migration anti-patterns.
+
+These checks use AST analysis to detect common migration mistakes that can cause
+silent failures, particularly on SQLite where ``PRAGMA foreign_keys`` has specific
+requirements about transaction state.
+
+Rules
+=====
+
+MIG001 -- DML before ``disable_sqlite_fkeys``
+----------------------------------------------
+
+**What it does:**
+Detects ``op.execute()`` calls containing DML keywords (UPDATE, INSERT, DELETE) that
+appear before a ``with disable_sqlite_fkeys(op):`` block in the same function.
+
+**Why is this bad:**
+On SQLite, any DML statement triggers *autobegin*, starting a transaction. Once a
+transaction is active, ``PRAGMA foreign_keys=off`` (issued by ``disable_sqlite_fkeys``)
+becomes a no-op. This means foreign key checks remain enabled, and
+``batch_alter_table`` (which recreates tables) may fail with foreign key constraint
+violations.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL") # triggers autobegin
+ with disable_sqlite_fkeys(op): # PRAGMA is now a no-op!
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ with disable_sqlite_fkeys(op):
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+
+MIG002 -- DDL before ``disable_sqlite_fkeys``
+----------------------------------------------
+
+**What it does:**
+Detects any Alembic ``op.*`` call (DDL operations like ``op.add_column``,
+``op.drop_column``, ``op.create_table``, ``op.drop_table``, ``op.create_index``,
+``op.batch_alter_table``, etc.) that appears before a
+``with disable_sqlite_fkeys(op):`` block in the same function. Excludes DML calls
+covered by MIG001.
+
+**Why is this bad:**
+DDL operations also trigger *autobegin* on SQLite, making the subsequent
+``PRAGMA foreign_keys=off`` a no-op, exactly like DML.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.add_column("dag_run", sa.Column("created_at", ...)) # triggers autobegin
+ with disable_sqlite_fkeys(op): # PRAGMA is now a no-op!
+ with op.batch_alter_table("backfill") as batch_op:
+ batch_op.alter_column("col", nullable=True)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ with disable_sqlite_fkeys(op):
+ op.add_column("dag_run", sa.Column("created_at", ...))
+ with op.batch_alter_table("backfill") as batch_op:
+ batch_op.alter_column("col", nullable=True)
+
+
+MIG003 -- DML without offline-mode guard
+------------------------------------------
+
+**What it does:**
+Detects ``op.execute()`` calls containing DML keywords (UPDATE, INSERT, DELETE) in
+``upgrade()`` or ``downgrade()`` functions that do not contain a
+``context.is_offline_mode()`` check anywhere in the same function.
+
+**Why is this bad:**
+Alembic's offline mode generates SQL scripts instead of executing against a live
+database. DML statements like ``UPDATE ... WHERE col IS NULL`` depend on actual data
+and produce no useful output in offline mode. Without an ``is_offline_mode()`` guard,
+the generated script includes DML that may fail or silently do nothing when applied
+later.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ if not context.is_offline_mode():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+
+Known Limitation -- Future MIG004 Candidate
+---------------------------------------------
+
+Migrations that perform DML and use ``batch_alter_table`` but never call
+``disable_sqlite_fkeys`` at all are NOT detected by MIG001/MIG002. These migrations
+may silently break SQLite foreign key handling. Detecting this pattern requires
+understanding whether foreign key relationships exist on the affected tables, which
+is beyond AST-level analysis. This is tracked as a future MIG004 candidate.
+
+
+Suppression
+===========
+
+Add ``# noqa: MIG0XX`` to a line to suppress the corresponding check for that line.
+Multiple codes can be comma-separated: ``# noqa: MIG001, MIG003``.
+
+Include a brief reason after ``--``::
+
+ # noqa: MIG003 -- simple UPDATE safe in offline mode
+"""
+
+from __future__ import annotations
+
+import ast
+import re
+import sys
+from pathlib import Path
+
+from rich.console import Console
+
+console = Console(color_system="standard", width=200)
+
+DML_KEYWORDS = ("UPDATE ", "INSERT ", "DELETE ")
+
+
+def _get_noqa_codes(line: str) -> set[str]:
+ """Extract noqa codes from a source line."""
+ match = re.search(r"#\s*noqa:\s*([\w,\s]+)", line)
+ if match:
+ return {code.strip() for code in match.group(1).split(",")}
+ return set()
+
+
+def _is_dml_string(node: ast.expr) -> bool:
+ """Check if an AST expression is a string literal starting with a DML keyword."""
+ if isinstance(node, ast.Constant) and isinstance(node.value, str):
+ stripped = node.value.strip().upper()
+ return any(stripped.startswith(kw) for kw in DML_KEYWORDS)
+ # f-strings: check the first string fragment
+ if isinstance(node, ast.JoinedStr):
+ for value in node.values:
+ if isinstance(value, ast.Constant) and isinstance(value.value, str):
+ stripped = value.value.strip().upper()
+ if any(stripped.startswith(kw) for kw in DML_KEYWORDS):
+ return True
+ break # only the leading fragment matters
+ return False
+
+
+def _is_op_execute_with_dml(node: ast.Call) -> bool:
+ """Check if a Call node is ``op.execute(<DML string>)``."""
+ if not (
+ isinstance(node.func, ast.Attribute)
+ and isinstance(node.func.value, ast.Name)
+ and node.func.value.id == "op"
+ and node.func.attr == "execute"
+ ):
+ return False
+ if not node.args:
+ return False
+ arg = node.args[0]
+ # Direct string: op.execute("UPDATE ...")
+ if _is_dml_string(arg):
+ return True
+ # Wrapped: op.execute(text("UPDATE ...")) or op.execute(sa.text("UPDATE ..."))
+ if isinstance(arg, ast.Call) and arg.args:
+ return _is_dml_string(arg.args[0])
+ return False
+
+
+def _is_op_call(node: ast.Call) -> bool:
+ """Check if a Call node is ``op.<something>(...)``."""
+ return (
+ isinstance(node.func, ast.Attribute)
+ and isinstance(node.func.value, ast.Name)
+ and node.func.value.id == "op"
+ )
+
+
+def _find_disable_sqlite_fkeys_line(func_node: ast.FunctionDef) -> int | None:
+ """Find the line number of the first ``with disable_sqlite_fkeys(op):`` in a function."""
+ # Collect all matches then take min() — ast.walk BFS order does not guarantee
+ # line-number order when guards appear at different nesting depths.
+ found: list[int] = []
+ for node in ast.walk(func_node):
+ if not isinstance(node, ast.With):
+ continue
+ for item in node.items:
+ ctx = item.context_expr
+ if isinstance(ctx, ast.Call):
+ func = ctx.func
+ if (isinstance(func, ast.Name) and func.id == "disable_sqlite_fkeys") or (
+ isinstance(func, ast.Attribute) and func.attr == "disable_sqlite_fkeys"
+ ):
+ found.append(node.lineno)
+ return min(found) if found else None
Review Comment:
MIG001/MIG002 rely on comparing `node.lineno` with `guard_line`, which misses a key SQLite hazard: `with op.batch_alter_table(...), disable_sqlite_fkeys(op):` (i.e., `disable_sqlite_fkeys` not first in the same `with` statement). In that case, the first context expression is evaluated before the PRAGMA is applied, but both calls share the same `lineno`, so the current `>= guard_line` logic won’t flag it. Suggestion: track the guard location more precisely (e.g., detect `ast.With` nodes and ensure `disable_sqlite_fkeys(...)` is the first item), and report MIG002 (and/or a dedicated rule) when it is not.
##########
scripts/ci/prek/check_migration_patterns.py:
##########
@@ -0,0 +1,383 @@
+#!/usr/bin/env python
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# /// script
+# requires-python = ">=3.10,<3.11"
+# dependencies = [
+# "rich>=13.6.0",
+# ]
+# ///
+"""
+Static analysis checks for Alembic migration anti-patterns.
+
+These checks use AST analysis to detect common migration mistakes that can cause
+silent failures, particularly on SQLite where ``PRAGMA foreign_keys`` has specific
+requirements about transaction state.
+
+Rules
+=====
+
+MIG001 -- DML before ``disable_sqlite_fkeys``
+----------------------------------------------
+
+**What it does:**
+Detects ``op.execute()`` calls containing DML keywords (UPDATE, INSERT, DELETE) that
+appear before a ``with disable_sqlite_fkeys(op):`` block in the same function.
+
+**Why is this bad:**
+On SQLite, any DML statement triggers *autobegin*, starting a transaction. Once a
+transaction is active, ``PRAGMA foreign_keys=off`` (issued by ``disable_sqlite_fkeys``)
+becomes a no-op. This means foreign key checks remain enabled, and
+``batch_alter_table`` (which recreates tables) may fail with foreign key constraint
+violations.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL") # triggers autobegin
+ with disable_sqlite_fkeys(op): # PRAGMA is now a no-op!
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ with disable_sqlite_fkeys(op):
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+
+MIG002 -- DDL before ``disable_sqlite_fkeys``
+----------------------------------------------
+
+**What it does:**
+Detects any Alembic ``op.*`` call (DDL operations like ``op.add_column``,
+``op.drop_column``, ``op.create_table``, ``op.drop_table``, ``op.create_index``,
+``op.batch_alter_table``, etc.) that appears before a
+``with disable_sqlite_fkeys(op):`` block in the same function. Excludes DML calls
+covered by MIG001.
+
+**Why is this bad:**
+DDL operations also trigger *autobegin* on SQLite, making the subsequent
+``PRAGMA foreign_keys=off`` a no-op, exactly like DML.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.add_column("dag_run", sa.Column("created_at", ...)) # triggers autobegin
+ with disable_sqlite_fkeys(op): # PRAGMA is now a no-op!
+ with op.batch_alter_table("backfill") as batch_op:
+ batch_op.alter_column("col", nullable=True)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ with disable_sqlite_fkeys(op):
+ op.add_column("dag_run", sa.Column("created_at", ...))
+ with op.batch_alter_table("backfill") as batch_op:
+ batch_op.alter_column("col", nullable=True)
+
+
+MIG003 -- DML without offline-mode guard
+------------------------------------------
+
+**What it does:**
+Detects ``op.execute()`` calls containing DML keywords (UPDATE, INSERT, DELETE) in
+``upgrade()`` or ``downgrade()`` functions that do not contain a
+``context.is_offline_mode()`` check anywhere in the same function.
+
+**Why is this bad:**
+Alembic's offline mode generates SQL scripts instead of executing against a live
+database. DML statements like ``UPDATE ... WHERE col IS NULL`` depend on actual data
+and produce no useful output in offline mode. Without an ``is_offline_mode()`` guard,
+the generated script includes DML that may fail or silently do nothing when applied
+later.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ if not context.is_offline_mode():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+
+Known Limitation -- Future MIG004 Candidate
+---------------------------------------------
+
+Migrations that perform DML and use ``batch_alter_table`` but never call
+``disable_sqlite_fkeys`` at all are NOT detected by MIG001/MIG002. These migrations
+may silently break SQLite foreign key handling. Detecting this pattern requires
+understanding whether foreign key relationships exist on the affected tables, which
+is beyond AST-level analysis. This is tracked as a future MIG004 candidate.
+
+
+Suppression
+===========
+
+Add ``# noqa: MIG0XX`` to a line to suppress the corresponding check for that line.
+Multiple codes can be comma-separated: ``# noqa: MIG001, MIG003``.
+
+Include a brief reason after ``--``::
+
+ # noqa: MIG003 -- simple UPDATE safe in offline mode
+"""
+
+from __future__ import annotations
+
+import ast
+import re
+import sys
+from pathlib import Path
+
+from rich.console import Console
+
+console = Console(color_system="standard", width=200)
+
+DML_KEYWORDS = ("UPDATE ", "INSERT ", "DELETE ")
+
+
+def _get_noqa_codes(line: str) -> set[str]:
+ """Extract noqa codes from a source line."""
+ match = re.search(r"#\s*noqa:\s*([\w,\s]+)", line)
+ if match:
+ return {code.strip() for code in match.group(1).split(",")}
+ return set()
+
+
+def _is_dml_string(node: ast.expr) -> bool:
+ """Check if an AST expression is a string literal starting with a DML keyword."""
+ if isinstance(node, ast.Constant) and isinstance(node.value, str):
+ stripped = node.value.strip().upper()
+ return any(stripped.startswith(kw) for kw in DML_KEYWORDS)
+ # f-strings: check the first string fragment
+ if isinstance(node, ast.JoinedStr):
+ for value in node.values:
+ if isinstance(value, ast.Constant) and isinstance(value.value, str):
+ stripped = value.value.strip().upper()
+ if any(stripped.startswith(kw) for kw in DML_KEYWORDS):
+ return True
+ break # only the leading fragment matters
+ return False
+
+
+def _is_op_execute_with_dml(node: ast.Call) -> bool:
+ """Check if a Call node is ``op.execute(<DML string>)``."""
+ if not (
+ isinstance(node.func, ast.Attribute)
+ and isinstance(node.func.value, ast.Name)
+ and node.func.value.id == "op"
+ and node.func.attr == "execute"
+ ):
+ return False
+ if not node.args:
+ return False
+ arg = node.args[0]
+ # Direct string: op.execute("UPDATE ...")
+ if _is_dml_string(arg):
+ return True
+ # Wrapped: op.execute(text("UPDATE ...")) or op.execute(sa.text("UPDATE ..."))
+ if isinstance(arg, ast.Call) and arg.args:
+ return _is_dml_string(arg.args[0])
+ return False
+
+
+def _is_op_call(node: ast.Call) -> bool:
+ """Check if a Call node is ``op.<something>(...)``."""
+ return (
+ isinstance(node.func, ast.Attribute)
+ and isinstance(node.func.value, ast.Name)
+ and node.func.value.id == "op"
+ )
+
+
+def _find_disable_sqlite_fkeys_line(func_node: ast.FunctionDef) -> int | None:
+ """Find the line number of the first ``with disable_sqlite_fkeys(op):`` in a function."""
+ # Collect all matches then take min() — ast.walk BFS order does not guarantee
+ # line-number order when guards appear at different nesting depths.
+ found: list[int] = []
+ for node in ast.walk(func_node):
+ if not isinstance(node, ast.With):
+ continue
+ for item in node.items:
+ ctx = item.context_expr
+ if isinstance(ctx, ast.Call):
+ func = ctx.func
+ if (isinstance(func, ast.Name) and func.id == "disable_sqlite_fkeys") or (
+ isinstance(func, ast.Attribute) and func.attr == "disable_sqlite_fkeys"
+ ):
+ found.append(node.lineno)
+ return min(found) if found else None
+
+
+def _has_offline_mode_check(func_node: ast.FunctionDef) -> bool:
+ """Check if function contains ``context.is_offline_mode()`` call."""
+ for node in ast.walk(func_node):
+ if isinstance(node, ast.Call):
+ func = node.func
+ if isinstance(func, ast.Attribute) and func.attr == "is_offline_mode":
+ return True
+ return False
Review Comment:
`_has_offline_mode_check` currently returns true for any call to `*.is_offline_mode()`, not specifically `context.is_offline_mode()`. This can cause false negatives for MIG003 if some other object in the migration happens to have an `is_offline_mode()` method. Tighten the check to require `func.value` is `ast.Name` with `id == 'context'` (and optionally also support the known Alembic patterns used in this repo, if any).
--
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] Add static analysis tests for DB migration anti-patterns [airflow]
Posted by "Dev-iL (via GitHub)" <gi...@apache.org>.
Dev-iL commented on code in PR #64972:
URL: https://github.com/apache/airflow/pull/64972#discussion_r3067588058
##########
airflow-core/tests/unit/migrations/test_migration_utils.py:
##########
@@ -0,0 +1,183 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Unit tests for ``airflow.migrations.utils``."""
+
+from __future__ import annotations
+
+import pytest
+from alembic.migration import MigrationContext
+from alembic.operations import Operations
+from sqlalchemy import text
+
+from airflow import settings
+from airflow.migrations.utils import (
+ disable_sqlite_fkeys,
+ ignore_sqlite_value_error,
+ mysql_drop_foreignkey_if_exists,
+)
+
+pytestmark = pytest.mark.db_test
+
+
+def _dialect() -> str:
+ assert settings.engine is not None # guaranteed by db_test marker
+ return settings.engine.dialect.name
+
+
+class TestDisableSqliteFkeys:
+ """Tests for :func:`disable_sqlite_fkeys`."""
+
+ def test_yields_op(self):
+ """The context manager yields the *op* object on every backend."""
+ with settings.engine.connect() as conn:
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+ with disable_sqlite_fkeys(op) as yielded:
+ assert yielded is op
+
+ def test_sqlite_turns_off_fkeys(self):
+ """On SQLite, ``PRAGMA foreign_keys`` is OFF inside the context."""
+ if _dialect() != "sqlite":
+ pytest.skip("SQLite-specific test")
Review Comment:
I suspect these marks cause tests to time out 🤔
--
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] Add static analysis tests for DB migration anti-patterns [airflow]
Posted by "Dev-iL (via GitHub)" <gi...@apache.org>.
Dev-iL commented on code in PR #64972:
URL: https://github.com/apache/airflow/pull/64972#discussion_r3063930239
##########
airflow-core/tests/unit/migrations/test_migration_utils.py:
##########
@@ -0,0 +1,183 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Unit tests for ``airflow.migrations.utils``."""
+
+from __future__ import annotations
+
+import pytest
+from alembic.migration import MigrationContext
+from alembic.operations import Operations
+from sqlalchemy import text
+
+from airflow import settings
+from airflow.migrations.utils import (
+ disable_sqlite_fkeys,
+ ignore_sqlite_value_error,
+ mysql_drop_foreignkey_if_exists,
+)
+
+pytestmark = pytest.mark.db_test
+
+
+def _dialect() -> str:
+ assert settings.engine is not None # guaranteed by db_test marker
+ return settings.engine.dialect.name
+
+
+class TestDisableSqliteFkeys:
+ """Tests for :func:`disable_sqlite_fkeys`."""
+
+ def test_yields_op(self):
+ """The context manager yields the *op* object on every backend."""
+ with settings.engine.connect() as conn:
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+ with disable_sqlite_fkeys(op) as yielded:
+ assert yielded is op
+
+ def test_sqlite_turns_off_fkeys(self):
+ """On SQLite, ``PRAGMA foreign_keys`` is OFF inside the context."""
+ if _dialect() != "sqlite":
+ pytest.skip("SQLite-specific test")
Review Comment:
How about instead - `@pytest.mark.backend(...)`?
--
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] Add static analysis tests for DB migration anti-patterns [airflow]
Posted by "Dev-iL (via GitHub)" <gi...@apache.org>.
Dev-iL commented on code in PR #64972:
URL: https://github.com/apache/airflow/pull/64972#discussion_r3064190872
##########
scripts/ci/prek/check_migration_patterns.py:
##########
@@ -0,0 +1,384 @@
+#!/usr/bin/env python
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""
+Static analysis checks for Alembic migration anti-patterns.
+
+These checks use AST analysis to detect common migration mistakes that can cause
+silent failures, particularly on SQLite where ``PRAGMA foreign_keys`` has specific
+requirements about transaction state.
+
+TODO: These checks are candidates for future Ruff rules (AIR category).
Review Comment:
Right - TODO removed.
--
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] Add static analysis tests for DB migration anti-patterns [airflow]
Posted by "Dev-iL (via GitHub)" <gi...@apache.org>.
Dev-iL commented on code in PR #64972:
URL: https://github.com/apache/airflow/pull/64972#discussion_r3064206159
##########
airflow-core/tests/unit/migrations/test_migration_utils.py:
##########
@@ -0,0 +1,183 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Unit tests for ``airflow.migrations.utils``."""
+
+from __future__ import annotations
+
+import pytest
+from alembic.migration import MigrationContext
+from alembic.operations import Operations
+from sqlalchemy import text
+
+from airflow import settings
+from airflow.migrations.utils import (
+ disable_sqlite_fkeys,
+ ignore_sqlite_value_error,
+ mysql_drop_foreignkey_if_exists,
+)
+
+pytestmark = pytest.mark.db_test
+
+
+def _dialect() -> str:
+ assert settings.engine is not None # guaranteed by db_test marker
+ return settings.engine.dialect.name
+
+
+class TestDisableSqliteFkeys:
+ """Tests for :func:`disable_sqlite_fkeys`."""
+
+ def test_yields_op(self):
+ """The context manager yields the *op* object on every backend."""
+ with settings.engine.connect() as conn:
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+ with disable_sqlite_fkeys(op) as yielded:
+ assert yielded is op
+
+ def test_sqlite_turns_off_fkeys(self):
+ """On SQLite, ``PRAGMA foreign_keys`` is OFF inside the context."""
+ if _dialect() != "sqlite":
+ pytest.skip("SQLite-specific test")
+
+ with settings.engine.connect() as conn:
+ # Ensure foreign_keys is ON before entering the context manager.
+ conn.execute(text("PRAGMA foreign_keys=ON"))
+ conn.commit()
+
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+
+ with disable_sqlite_fkeys(op):
+ result = conn.execute(text("PRAGMA foreign_keys")).scalar()
+ assert result == 0, "foreign_keys should be OFF inside disable_sqlite_fkeys"
+
+ result = conn.execute(text("PRAGMA foreign_keys")).scalar()
+ assert result == 1, "foreign_keys should be restored to ON after disable_sqlite_fkeys"
+
+ def test_non_sqlite_is_noop(self):
+ """On non-SQLite backends, the context manager simply yields."""
+ if _dialect() == "sqlite":
+ pytest.skip("Non-SQLite backends only")
+
+ with settings.engine.connect() as conn:
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+ with disable_sqlite_fkeys(op) as yielded:
+ assert yielded is op
+
+
+class TestMysqlDropForeignkeyIfExists:
+ """Tests for :func:`mysql_drop_foreignkey_if_exists`."""
+
+ def test_drops_existing_fk(self):
+ """On MySQL, an existing foreign key is dropped."""
+ if _dialect() != "mysql":
+ pytest.skip("MySQL-specific stored procedure test")
+
+ with settings.engine.connect() as conn:
+ conn.execute(text("CREATE TABLE IF NOT EXISTS _test_mig_parent ( id INT PRIMARY KEY)"))
+ conn.execute(
+ text(
+ "CREATE TABLE IF NOT EXISTS _test_mig_child ("
+ " id INT PRIMARY KEY,"
+ " parent_id INT,"
+ " CONSTRAINT _test_mig_fk FOREIGN KEY (parent_id)"
+ " REFERENCES _test_mig_parent(id)"
+ ")"
+ )
+ )
+ conn.commit()
+
+ try:
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+ mysql_drop_foreignkey_if_exists("_test_mig_fk", "_test_mig_child", op)
+ conn.commit()
+
+ count = conn.execute(
+ text(
+ "SELECT COUNT(*) FROM information_schema.TABLE_CONSTRAINTS "
Review Comment:
Done
--
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] Add static analysis tests for DB migration anti-patterns [airflow]
Posted by "Dev-iL (via GitHub)" <gi...@apache.org>.
Dev-iL commented on PR #64972:
URL: https://github.com/apache/airflow/pull/64972#issuecomment-4230895455
## CI Timeout Investigation: `All-core:LowestDeps:14:3.12:Core...Serialization`
### Summary
The CI job `All-core:LowestDeps:14:3.12:Core...Serialization` was cancelled after 1h5m (exceeding `TOTAL_TEST_TIMEOUT: 3600s`). This is a **pre-existing flake**, not caused by this PR's changes.
### Evidence
**1. Only Python 3.12 affected**
The same test group (`LowestDeps:Core...Serialization`) passed on Python 3.10, 3.11, 3.13, and 3.14. Only the 3.12 run timed out.
**2. "Other" group stuck at 63%**
The "Other" test group reported "1 failed, 1093 passed, 12 skipped" at the 12:47 mark and made no further progress until cancellation at 13:40. The PR adds ~8 tests — the hang is deep in pre-existing tests (1093+).
**3. PR code is identical between force-pushes**
Diffing the PR-specific files between commits 931fbf06 and 553c33ff (pre- and post-rebase) shows zero differences — only the rebased base changed.
**4. Backend markers verified correct**
- `BACKEND=sqlite` env var is set in LowestDeps CI (confirmed from logs)
- `--backend` is NOT passed as a pytest CLI arg (`Extra pytest args: ()`)
- `@pytest.mark.backend("sqlite")` → runs (BACKEND matches) ✓
- `@pytest.mark.backend("postgres", "mysql")` → skipped (BACKEND doesn't match) ✓
- No hang path through the backend marker logic
**5. Upstream changes between force-pushes**
The rebase picked up: `feat: restore os provider on core library (#64764)`, `Remove unused/dead code in migration utils (#64947)`, `Added plugins command to airflowctl (#64935)`, `Breeze: Show local reproduction commands (#63901)`. None of these touch the test infrastructure in a way that would cause a Python 3.12-specific hang.
### Conclusion
Most likely cause: a pre-existing flaky test in the "Other" group that hangs specifically on Python 3.12 (possibly a deadlock or resource contention issue). The PR's changes (AST-based migration checks + migration util tests) are not involved — they ran in a different test group and the PR files are byte-identical across the rebase.
--
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] Add static analysis tests for DB migration anti-patterns [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on code in PR #64972:
URL: https://github.com/apache/airflow/pull/64972#discussion_r3167452733
##########
scripts/tests/ci/prek/test_check_migration_patterns.py:
##########
@@ -0,0 +1,473 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Unit tests for ``scripts/ci/prek/check_migration_patterns.py``."""
+
+from __future__ import annotations
+
+import ast
+import sys
+from pathlib import Path
+
+import pytest
+from ci.prek.check_migration_patterns import (
+ MigrationFile,
+ _collect_ddl_table_names,
+ _extract_dml_table_name,
+ _get_noqa_codes,
+ _line_has_noqa,
+ check_mig001,
+ check_mig002,
+ check_mig003,
+ main,
+)
+
+
+def write_migration(tmp_path: Path, source: str) -> Path:
+ """Write *source* to a temp file that looks like a migration file."""
+ p = tmp_path / "0099_test_migration.py"
+ p.write_text(source)
+ return p
+
+
+def parse_migration(tmp_path: Path, source: str) -> MigrationFile:
+ """Write *source* to a temp file and return a parsed ``MigrationFile``."""
+ return MigrationFile.from_path(write_migration(tmp_path, source))
+
+
+class TestGetNoqaCodes:
+ def test_single_code(self):
+ assert _get_noqa_codes("op.execute(...) # noqa: MIG001") == {"MIG001"}
+
+ def test_multiple_codes(self):
+ assert _get_noqa_codes("op.execute(...) # noqa: MIG001, MIG003") == {"MIG001", "MIG003"}
+
+ def test_code_with_reason_suffix(self):
+ # Reason after '--' should NOT be included in the code set.
+ codes = _get_noqa_codes("op.execute(...) # noqa: MIG003 -- safe in offline mode")
+ assert "MIG003" in codes
+ assert "--" not in "".join(codes)
+ assert "safe" not in "".join(codes)
+
+ def test_no_noqa_comment(self):
+ assert _get_noqa_codes("op.execute('UPDATE foo SET x=1')") == set()
+
+ def test_empty_string(self):
+ assert _get_noqa_codes("") == set()
Review Comment:
```suggestion
@pytest.mark.parametrize(
"line, expected",
[
pytest.param(
"op.execute(...) # noqa: MIG001",
{"MIG001"},
id="single-code",
),
pytest.param(
"op.execute(...) # noqa: MIG001, MIG003",
{"MIG001", "MIG003"},
id="multiple-codes",
),
pytest.param(
"op.execute('UPDATE foo SET x=1')",
set(),
id="no-noqa-comment",
),
pytest.param(
"",
set(),
id="empty-string",
),
],
)
def test_get_noqa_codes(self, line: str, expected: set[str]) -> None:
assert _get_noqa_codes(line) == expected
def test_code_with_reason_suffix(self) -> None:
# Reason after '--' should NOT be included in the code set.
codes = _get_noqa_codes("op.execute(...) # noqa: MIG003 -- safe in offline mode")
assert "MIG003" in codes
assert "--" not in "".join(codes)
assert "safe" not in "".join(codes)
```
##########
scripts/tests/ci/prek/test_check_migration_patterns.py:
##########
@@ -0,0 +1,473 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Unit tests for ``scripts/ci/prek/check_migration_patterns.py``."""
+
+from __future__ import annotations
+
+import ast
+import sys
+from pathlib import Path
+
+import pytest
+from ci.prek.check_migration_patterns import (
+ MigrationFile,
+ _collect_ddl_table_names,
+ _extract_dml_table_name,
+ _get_noqa_codes,
+ _line_has_noqa,
+ check_mig001,
+ check_mig002,
+ check_mig003,
+ main,
+)
+
+
+def write_migration(tmp_path: Path, source: str) -> Path:
+ """Write *source* to a temp file that looks like a migration file."""
+ p = tmp_path / "0099_test_migration.py"
+ p.write_text(source)
+ return p
+
+
+def parse_migration(tmp_path: Path, source: str) -> MigrationFile:
+ """Write *source* to a temp file and return a parsed ``MigrationFile``."""
+ return MigrationFile.from_path(write_migration(tmp_path, source))
+
+
+class TestGetNoqaCodes:
+ def test_single_code(self):
+ assert _get_noqa_codes("op.execute(...) # noqa: MIG001") == {"MIG001"}
+
+ def test_multiple_codes(self):
+ assert _get_noqa_codes("op.execute(...) # noqa: MIG001, MIG003") == {"MIG001", "MIG003"}
+
+ def test_code_with_reason_suffix(self):
+ # Reason after '--' should NOT be included in the code set.
+ codes = _get_noqa_codes("op.execute(...) # noqa: MIG003 -- safe in offline mode")
+ assert "MIG003" in codes
+ assert "--" not in "".join(codes)
+ assert "safe" not in "".join(codes)
+
+ def test_no_noqa_comment(self):
+ assert _get_noqa_codes("op.execute('UPDATE foo SET x=1')") == set()
+
+ def test_empty_string(self):
+ assert _get_noqa_codes("") == set()
+
+
+class TestLineHasNoqa:
+ def test_line_has_matching_code(self):
+ lines = ["op.execute('UPDATE ...') # noqa: MIG001"]
+ assert _line_has_noqa(lines, 1, "MIG001") is True
+
+ def test_line_has_different_code(self):
+ lines = ["op.execute('UPDATE ...') # noqa: MIG002"]
+ assert _line_has_noqa(lines, 1, "MIG001") is False
+
+ @pytest.mark.parametrize("lineno", [0, 2], ids=["zero", "beyond_end"])
+ def test_out_of_bounds(self, lineno):
+ assert _line_has_noqa(["some line"], lineno, "MIG001") is False
Review Comment:
same idea should be applied elsewhere as well
##########
scripts/tests/ci/prek/test_check_migration_patterns.py:
##########
@@ -0,0 +1,473 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Unit tests for ``scripts/ci/prek/check_migration_patterns.py``."""
+
+from __future__ import annotations
+
+import ast
+import sys
+from pathlib import Path
+
+import pytest
+from ci.prek.check_migration_patterns import (
+ MigrationFile,
+ _collect_ddl_table_names,
+ _extract_dml_table_name,
+ _get_noqa_codes,
+ _line_has_noqa,
+ check_mig001,
+ check_mig002,
+ check_mig003,
+ main,
+)
+
+
+def write_migration(tmp_path: Path, source: str) -> Path:
+ """Write *source* to a temp file that looks like a migration file."""
+ p = tmp_path / "0099_test_migration.py"
+ p.write_text(source)
+ return p
+
+
+def parse_migration(tmp_path: Path, source: str) -> MigrationFile:
+ """Write *source* to a temp file and return a parsed ``MigrationFile``."""
+ return MigrationFile.from_path(write_migration(tmp_path, source))
+
+
+class TestGetNoqaCodes:
+ def test_single_code(self):
+ assert _get_noqa_codes("op.execute(...) # noqa: MIG001") == {"MIG001"}
+
+ def test_multiple_codes(self):
+ assert _get_noqa_codes("op.execute(...) # noqa: MIG001, MIG003") == {"MIG001", "MIG003"}
+
+ def test_code_with_reason_suffix(self):
+ # Reason after '--' should NOT be included in the code set.
+ codes = _get_noqa_codes("op.execute(...) # noqa: MIG003 -- safe in offline mode")
+ assert "MIG003" in codes
+ assert "--" not in "".join(codes)
+ assert "safe" not in "".join(codes)
+
+ def test_no_noqa_comment(self):
+ assert _get_noqa_codes("op.execute('UPDATE foo SET x=1')") == set()
+
+ def test_empty_string(self):
+ assert _get_noqa_codes("") == set()
+
+
+class TestLineHasNoqa:
+ def test_line_has_matching_code(self):
+ lines = ["op.execute('UPDATE ...') # noqa: MIG001"]
+ assert _line_has_noqa(lines, 1, "MIG001") is True
+
+ def test_line_has_different_code(self):
+ lines = ["op.execute('UPDATE ...') # noqa: MIG002"]
+ assert _line_has_noqa(lines, 1, "MIG001") is False
+
+ @pytest.mark.parametrize("lineno", [0, 2], ids=["zero", "beyond_end"])
+ def test_out_of_bounds(self, lineno):
+ assert _line_has_noqa(["some line"], lineno, "MIG001") is False
Review Comment:
```suggestion
@pytest.mark.parametrize(
"lines, lineno, code, expected",
[
pytest.param(
["op.execute('UPDATE ...') # noqa: MIG001"],
1,
"MIG001",
True,
id="matching-code",
),
pytest.param(
["op.execute('UPDATE ...') # noqa: MIG002"],
1,
"MIG001",
False,
id="different-code",
),
pytest.param(["some line"], 0, "MIG001", False, id="out-of-bounds-zero"),
pytest.param(["some line"], 2, "MIG001", False, id="out-of-bounds-beyond-end"),
],
)
def test_line_has_noqa(lines: list[str], lineno: int, code: str, expected: bool) -> None:
assert _line_has_noqa(lines, lineno, code) is expected
```
--
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] Add static analysis tests for DB migration anti-patterns [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #64972:
URL: https://github.com/apache/airflow/pull/64972
--
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] Add static analysis tests for DB migration anti-patterns [airflow]
Posted by "Dev-iL (via GitHub)" <gi...@apache.org>.
Dev-iL commented on code in PR #64972:
URL: https://github.com/apache/airflow/pull/64972#discussion_r3064185295
##########
scripts/ci/prek/check_migration_patterns.py:
##########
@@ -0,0 +1,384 @@
+#!/usr/bin/env python
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""
+Static analysis checks for Alembic migration anti-patterns.
+
+These checks use AST analysis to detect common migration mistakes that can cause
+silent failures, particularly on SQLite where ``PRAGMA foreign_keys`` has specific
+requirements about transaction state.
+
+TODO: These checks are candidates for future Ruff rules (AIR category).
+
+Rules
+=====
+
+MIG001 -- DML before ``disable_sqlite_fkeys``
+----------------------------------------------
+
+**What it does:**
+Detects ``op.execute()`` calls containing DML keywords (UPDATE, INSERT, DELETE) that
+appear before a ``with disable_sqlite_fkeys(op):`` block in the same function.
+
+**Why is this bad:**
+On SQLite, any DML statement triggers *autobegin*, starting a transaction. Once a
+transaction is active, ``PRAGMA foreign_keys=off`` (issued by ``disable_sqlite_fkeys``)
+becomes a no-op. This means foreign key checks remain enabled, and
+``batch_alter_table`` (which recreates tables) may fail with foreign key constraint
+violations.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL") # triggers autobegin
+ with disable_sqlite_fkeys(op): # PRAGMA is now a no-op!
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ with disable_sqlite_fkeys(op):
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+
+MIG002 -- DDL before ``disable_sqlite_fkeys``
+----------------------------------------------
+
+**What it does:**
+Detects any Alembic ``op.*`` call (DDL operations like ``op.add_column``,
+``op.drop_column``, ``op.create_table``, ``op.drop_table``, ``op.create_index``,
+``op.batch_alter_table``, etc.) that appears before a
+``with disable_sqlite_fkeys(op):`` block in the same function. Excludes DML calls
+covered by MIG001.
+
+**Why is this bad:**
+DDL operations also trigger *autobegin* on SQLite, making the subsequent
+``PRAGMA foreign_keys=off`` a no-op, exactly like DML.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.add_column("dag_run", sa.Column("created_at", ...)) # triggers autobegin
+ with disable_sqlite_fkeys(op): # PRAGMA is now a no-op!
+ with op.batch_alter_table("backfill") as batch_op:
+ batch_op.alter_column("col", nullable=True)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ with disable_sqlite_fkeys(op):
+ op.add_column("dag_run", sa.Column("created_at", ...))
+ with op.batch_alter_table("backfill") as batch_op:
+ batch_op.alter_column("col", nullable=True)
+
+
+MIG003 -- DML without offline-mode guard
+------------------------------------------
+
+**What it does:**
+Detects ``op.execute()`` calls containing DML keywords (UPDATE, INSERT, DELETE) in
+``upgrade()`` or ``downgrade()`` functions that do not contain a
+``context.is_offline_mode()`` check anywhere in the same function.
+
+**Why is this bad:**
+Alembic's offline mode generates SQL scripts instead of executing against a live
+database. DML statements like ``UPDATE ... WHERE col IS NULL`` depend on actual data
+and produce no useful output in offline mode. Without an ``is_offline_mode()`` guard,
+the generated script includes DML that may fail or silently do nothing when applied
+later.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ if not context.is_offline_mode():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+
+Known Limitation -- Future MIG004 Candidate
+---------------------------------------------
+
+Migrations that perform DML and use ``batch_alter_table`` but never call
+``disable_sqlite_fkeys`` at all are NOT detected by MIG001/MIG002. These migrations
+may silently break SQLite foreign key handling. Detecting this pattern requires
+understanding whether foreign key relationships exist on the affected tables, which
+is beyond AST-level analysis. This is tracked as a future MIG004 candidate.
+
+
+Suppression
+===========
+
+Add ``# noqa: MIG0XX`` to a line to suppress the corresponding check for that line.
+Multiple codes can be comma-separated: ``# noqa: MIG001, MIG003``.
+
+Include a brief reason after ``--``::
+
+ # noqa: MIG003 -- simple UPDATE safe in offline mode
+"""
+
+from __future__ import annotations
+
+import ast
+import re
+import sys
+from pathlib import Path
+
+DML_KEYWORDS = ("UPDATE ", "INSERT ", "DELETE ")
+
+
+def _get_noqa_codes(line: str) -> set[str]:
+ """Extract noqa codes from a source line."""
+ match = re.search(r"#\s*noqa:\s*([\w,\s]+)", line)
+ if match:
+ return {code.strip() for code in match.group(1).split(",")}
+ return set()
+
+
+def _is_dml_string(node: ast.expr) -> bool:
+ """Check if an AST expression is a string literal starting with a DML keyword."""
+ if isinstance(node, ast.Constant) and isinstance(node.value, str):
+ stripped = node.value.strip().upper()
+ return any(stripped.startswith(kw) for kw in DML_KEYWORDS)
+ # f-strings: check the first string fragment
+ if isinstance(node, ast.JoinedStr):
+ for value in node.values:
+ if isinstance(value, ast.Constant) and isinstance(value.value, str):
+ stripped = value.value.strip().upper()
+ if any(stripped.startswith(kw) for kw in DML_KEYWORDS):
+ return True
+ break # only the leading fragment matters
+ return False
+
+
+def _is_op_execute_with_dml(node: ast.Call) -> bool:
+ """Check if a Call node is ``op.execute(<DML string>)``."""
+ if not (
+ isinstance(node.func, ast.Attribute)
+ and isinstance(node.func.value, ast.Name)
+ and node.func.value.id == "op"
+ and node.func.attr == "execute"
+ ):
+ return False
+ if not node.args:
+ return False
+ arg = node.args[0]
+ # Direct string: op.execute("UPDATE ...")
+ if _is_dml_string(arg):
+ return True
+ # Wrapped: op.execute(text("UPDATE ...")) or op.execute(sa.text("UPDATE ..."))
+ if isinstance(arg, ast.Call) and arg.args:
+ return _is_dml_string(arg.args[0])
+ return False
+
+
+def _is_op_call(node: ast.Call) -> bool:
+ """Check if a Call node is ``op.<something>(...)``."""
+ return (
+ isinstance(node.func, ast.Attribute)
+ and isinstance(node.func.value, ast.Name)
+ and node.func.value.id == "op"
+ )
+
+
+def _find_disable_sqlite_fkeys_line(func_node: ast.FunctionDef) -> int | None:
+ """Find the line number of the first ``with disable_sqlite_fkeys(op):`` in a function."""
+ for node in ast.walk(func_node):
+ if not isinstance(node, ast.With):
+ continue
+ for item in node.items:
+ ctx = item.context_expr
+ if isinstance(ctx, ast.Call):
+ func = ctx.func
+ if (isinstance(func, ast.Name) and func.id == "disable_sqlite_fkeys") or (
+ isinstance(func, ast.Attribute) and func.attr == "disable_sqlite_fkeys"
+ ):
+ return node.lineno
+ return None
+
+
+def _has_offline_mode_check(func_node: ast.FunctionDef) -> bool:
+ """Check if function contains ``context.is_offline_mode()`` call."""
+ for node in ast.walk(func_node):
+ if isinstance(node, ast.Call):
+ func = node.func
+ if isinstance(func, ast.Attribute) and func.attr == "is_offline_mode":
+ return True
+ return False
+
+
+def _get_upgrade_downgrade_functions(tree: ast.Module) -> list[ast.FunctionDef]:
+ """Get ``upgrade()`` and ``downgrade()`` function definitions from module."""
+ return [
+ node
+ for node in ast.iter_child_nodes(tree)
+ if isinstance(node, ast.FunctionDef) and node.name in ("upgrade", "downgrade")
+ ]
+
+
+def _line_has_noqa(lines: list[str], lineno: int, code: str) -> bool:
+ """Check if a source line has a noqa suppression for the given code."""
+ if lineno < 1 or lineno > len(lines):
+ return False
+ return code in _get_noqa_codes(lines[lineno - 1])
+
+
+# ---------------------------------------------------------------------------
+# Check functions
+# ---------------------------------------------------------------------------
+
+
+def check_mig001(filepath: Path) -> list[str]:
+ """MIG001: DML before ``disable_sqlite_fkeys``."""
+ source = filepath.read_text()
+ lines = source.splitlines()
+ tree = ast.parse(source, filename=str(filepath))
+ errors: list[str] = []
+
+ for func in _get_upgrade_downgrade_functions(tree):
+ guard_line = _find_disable_sqlite_fkeys_line(func)
+ if guard_line is None:
+ continue # MIG001 only fires when disable_sqlite_fkeys IS present
+
+ for node in ast.walk(func):
+ if not isinstance(node, ast.Call):
+ continue
+ if node.lineno >= guard_line:
+ continue
+ if _is_op_execute_with_dml(node) and not _line_has_noqa(lines, node.lineno, "MIG001"):
+ errors.append(
+ f"MIG001 {func.name}(): op.execute() with DML at line {node.lineno} "
+ f"before disable_sqlite_fkeys at line {guard_line}"
+ )
+
+ return errors
+
+
+def check_mig002(filepath: Path) -> list[str]:
+ """MIG002: DDL before ``disable_sqlite_fkeys``."""
+ source = filepath.read_text()
+ lines = source.splitlines()
+ tree = ast.parse(source, filename=str(filepath))
+ errors: list[str] = []
+
+ for func in _get_upgrade_downgrade_functions(tree):
+ guard_line = _find_disable_sqlite_fkeys_line(func)
+ if guard_line is None:
+ continue # MIG002 only fires when disable_sqlite_fkeys IS present
+
+ for node in ast.walk(func):
+ if not isinstance(node, ast.Call):
+ continue
+ if node.lineno >= guard_line:
+ continue
+ # Skip DML calls — covered by MIG001
+ if _is_op_execute_with_dml(node):
+ continue
+ if _is_op_call(node) and not _line_has_noqa(lines, node.lineno, "MIG002"):
+ attr = node.func.attr if isinstance(node.func, ast.Attribute) else "?"
+ errors.append(
+ f"MIG002 {func.name}(): op.{attr}() at line {node.lineno} "
+ f"before disable_sqlite_fkeys at line {guard_line}"
+ )
+
+ return errors
+
+
+def check_mig003(filepath: Path) -> list[str]:
+ """MIG003: DML without offline-mode guard."""
+ source = filepath.read_text()
+ lines = source.splitlines()
+ tree = ast.parse(source, filename=str(filepath))
+ errors: list[str] = []
+
+ for func in _get_upgrade_downgrade_functions(tree):
+ if _has_offline_mode_check(func):
+ continue # function has the guard
+
+ for node in ast.walk(func):
+ if not isinstance(node, ast.Call):
+ continue
+ if _is_op_execute_with_dml(node) and not _line_has_noqa(lines, node.lineno, "MIG003"):
+ errors.append(
+ f"MIG003 {func.name}(): op.execute() with DML at line {node.lineno} "
+ f"without context.is_offline_mode() guard"
+ )
+
+ return errors
+
+
+# ---------------------------------------------------------------------------
+# CLI entry point
+# ---------------------------------------------------------------------------
+
+
+def main() -> int:
+ """Check migration files for anti-patterns. Exit 0 if clean, 1 if violations found."""
+ checks = [check_mig001, check_mig002, check_mig003]
+ has_errors = False
+
+ for filepath_str in sys.argv[1:]:
+ filepath = Path(filepath_str)
+ try:
+ source = filepath.read_text()
+ except OSError as exc:
+ print(f"{filepath}: {exc}", file=sys.stderr)
Review Comment:
Done: switched to `Console(color_system="standard", width=200)`
--
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] Add static analysis tests for DB migration anti-patterns [airflow]
Posted by "Dev-iL (via GitHub)" <gi...@apache.org>.
Dev-iL commented on code in PR #64972:
URL: https://github.com/apache/airflow/pull/64972#discussion_r3064199228
##########
airflow-core/tests/unit/migrations/test_migration_utils.py:
##########
@@ -0,0 +1,183 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Unit tests for ``airflow.migrations.utils``."""
+
+from __future__ import annotations
+
+import pytest
+from alembic.migration import MigrationContext
+from alembic.operations import Operations
+from sqlalchemy import text
+
+from airflow import settings
+from airflow.migrations.utils import (
+ disable_sqlite_fkeys,
+ ignore_sqlite_value_error,
+ mysql_drop_foreignkey_if_exists,
+)
+
+pytestmark = pytest.mark.db_test
+
+
+def _dialect() -> str:
+ assert settings.engine is not None # guaranteed by db_test marker
+ return settings.engine.dialect.name
+
+
+class TestDisableSqliteFkeys:
+ """Tests for :func:`disable_sqlite_fkeys`."""
+
+ def test_yields_op(self):
+ """The context manager yields the *op* object on every backend."""
+ with settings.engine.connect() as conn:
+ ctx = MigrationContext.configure(conn)
+ op = Operations(ctx)
+ with disable_sqlite_fkeys(op) as yielded:
+ assert yielded is op
+
+ def test_sqlite_turns_off_fkeys(self):
+ """On SQLite, ``PRAGMA foreign_keys`` is OFF inside the context."""
+ if _dialect() != "sqlite":
+ pytest.skip("SQLite-specific test")
Review Comment:
Done: used `@pytest.mark.backend(...)` on each backend-specific test method (not at class level, to keep `test_yields_op` running on all backends).
--
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] Add static analysis tests for DB migration anti-patterns [airflow]
Posted by "Lee-W (via GitHub)" <gi...@apache.org>.
Lee-W commented on PR #64972:
URL: https://github.com/apache/airflow/pull/64972#issuecomment-4286530389
> @Lee-W I wonder if the change in #65545 is covered by this PR
>
> EDIT: yes, according to copilot:
> <img alt="image" width="740" height="551" src="https://private-user-images.githubusercontent.com/6509619/581216073-eb833cd2-715f-4d94-be68-678f0e034c83.png?jwt=eyJ0eXAiOiJKV1QiLCJhbGciOiJIUzI1NiJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3NzY3NTUxNTYsIm5iZiI6MTc3Njc1NDg1NiwicGF0aCI6Ii82NTA5NjE5LzU4MTIxNjA3My1lYjgzM2NkMi03MTVmLTRkOTQtYmU2OC02NzhmMGUwMzRjODMucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI2MDQyMSUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNjA0MjFUMDcwMDU2WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9NTdiZGIyNGFkYzAzMGU2MGY2ZDI4MzI4ODBlYjRmZWNmZWYyZjZlZDVkZjY0NzgwNGFkNWU5YzQ0NzA3YTU4MyZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QmcmVzcG9uc2UtY29udGVudC10eXBlPWltYWdlJTJGcG5nIn0.EOc23-bbZW_6Cym_P92rEn1byStdPXSMxmAerC5UK24">
Nice! I use our airflow instance + the round-trip (not yet finished) PR to cache it. hope i'll have some time to take a deeper look today or tmr
--
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] Add static analysis tests for DB migration anti-patterns [airflow]
Posted by "ephraimbuddy (via GitHub)" <gi...@apache.org>.
ephraimbuddy commented on code in PR #64972:
URL: https://github.com/apache/airflow/pull/64972#discussion_r3159553999
##########
scripts/ci/prek/check_migration_patterns.py:
##########
@@ -0,0 +1,383 @@
+#!/usr/bin/env python
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# /// script
+# requires-python = ">=3.10"
+# dependencies = [
+# "rich>=13.6.0",
+# ]
+# ///
+"""
+Static analysis checks for Alembic migration anti-patterns.
+
+These checks use AST analysis to detect common migration mistakes that can cause
+silent failures, particularly on SQLite where ``PRAGMA foreign_keys`` has specific
+requirements about transaction state.
+
+Rules
+=====
+
+MIG001 -- DML before ``disable_sqlite_fkeys``
+----------------------------------------------
+
+**What it does:**
+Detects ``op.execute()`` calls containing DML keywords (UPDATE, INSERT, DELETE) that
+appear before a ``with disable_sqlite_fkeys(op):`` block in the same function.
+
+**Why is this bad:**
+On SQLite, any DML statement triggers *autobegin*, starting a transaction. Once a
+transaction is active, ``PRAGMA foreign_keys=off`` (issued by ``disable_sqlite_fkeys``)
+becomes a no-op. This means foreign key checks remain enabled, and
+``batch_alter_table`` (which recreates tables) may fail with foreign key constraint
+violations.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL") # triggers autobegin
+ with disable_sqlite_fkeys(op): # PRAGMA is now a no-op!
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ with disable_sqlite_fkeys(op):
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+
+MIG002 -- DDL before ``disable_sqlite_fkeys``
+----------------------------------------------
+
+**What it does:**
+Detects any Alembic ``op.*`` call (DDL operations like ``op.add_column``,
+``op.drop_column``, ``op.create_table``, ``op.drop_table``, ``op.create_index``,
+``op.batch_alter_table``, etc.) that appears before a
+``with disable_sqlite_fkeys(op):`` block in the same function. Excludes DML calls
+covered by MIG001.
+
+**Why is this bad:**
+DDL operations also trigger *autobegin* on SQLite, making the subsequent
+``PRAGMA foreign_keys=off`` a no-op, exactly like DML.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.add_column("dag_run", sa.Column("created_at", ...)) # triggers autobegin
+ with disable_sqlite_fkeys(op): # PRAGMA is now a no-op!
+ with op.batch_alter_table("backfill") as batch_op:
+ batch_op.alter_column("col", nullable=True)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ with disable_sqlite_fkeys(op):
+ op.add_column("dag_run", sa.Column("created_at", ...))
+ with op.batch_alter_table("backfill") as batch_op:
+ batch_op.alter_column("col", nullable=True)
+
+
+MIG003 -- DML without offline-mode guard
+------------------------------------------
+
+**What it does:**
+Detects ``op.execute()`` calls containing DML keywords (UPDATE, INSERT, DELETE) in
+``upgrade()`` or ``downgrade()`` functions that do not contain a
+``context.is_offline_mode()`` check anywhere in the same function.
+
+**Why is this bad:**
+Alembic's offline mode generates SQL scripts instead of executing against a live
+database. DML statements like ``UPDATE ... WHERE col IS NULL`` depend on actual data
+and produce no useful output in offline mode. Without an ``is_offline_mode()`` guard,
+the generated script includes DML that may fail or silently do nothing when applied
+later.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ if not context.is_offline_mode():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+
+Known Limitation -- Future MIG004 Candidate
+---------------------------------------------
+
+Migrations that perform DML and use ``batch_alter_table`` but never call
+``disable_sqlite_fkeys`` at all are NOT detected by MIG001/MIG002. These migrations
+may silently break SQLite foreign key handling. Detecting this pattern requires
+understanding whether foreign key relationships exist on the affected tables, which
+is beyond AST-level analysis. This is tracked as a future MIG004 candidate.
+
+
+Suppression
+===========
+
+Add ``# noqa: MIG0XX`` to a line to suppress the corresponding check for that line.
+Multiple codes can be comma-separated: ``# noqa: MIG001, MIG003``.
+
+Include a brief reason after ``--``::
+
+ # noqa: MIG003 -- simple UPDATE safe in offline mode
+"""
+
+from __future__ import annotations
+
+import ast
+import re
+import sys
+from pathlib import Path
+
+from rich.console import Console
+
+console = Console(color_system="standard", width=200)
+
+DML_KEYWORDS = ("UPDATE", "INSERT", "DELETE")
+
+
+def _get_noqa_codes(line: str) -> set[str]:
+ """Extract noqa codes from a source line."""
+ match = re.search(r"#\s*noqa:\s*([\w,\s]+)", line)
+ if match:
+ return {code.strip() for code in match.group(1).split(",")}
+ return set()
+
+
+def _is_dml_string(node: ast.expr) -> bool:
+ """Check if an AST expression is a string literal starting with a DML keyword."""
+ if isinstance(node, ast.Constant) and isinstance(node.value, str):
+ parts = node.value.strip().upper().split(None, 1)
+ return len(parts) > 1 and parts[0] in DML_KEYWORDS
+ # f-strings: check the first string fragment
+ if isinstance(node, ast.JoinedStr):
+ for value in node.values:
+ if isinstance(value, ast.Constant) and isinstance(value.value, str):
+ parts = value.value.strip().upper().split(None, 1)
+ if len(parts) > 1 and parts[0] in DML_KEYWORDS:
+ return True
+ break # only the leading fragment matters
+ return False
+
+
+def _is_op_execute_with_dml(node: ast.Call) -> bool:
+ """Check if a Call node is ``op.execute(<DML string>)``."""
+ if not (
+ isinstance(node.func, ast.Attribute)
+ and isinstance(node.func.value, ast.Name)
+ and node.func.value.id == "op"
+ and node.func.attr == "execute"
+ ):
+ return False
+ if not node.args:
+ return False
+ arg = node.args[0]
+ # Direct string: op.execute("UPDATE ...")
+ if _is_dml_string(arg):
+ return True
+ # Wrapped: op.execute(text("UPDATE ...")) or op.execute(sa.text("UPDATE ..."))
+ if isinstance(arg, ast.Call) and arg.args:
+ return _is_dml_string(arg.args[0])
+ return False
+
+
+def _is_op_call(node: ast.Call) -> bool:
+ """Check if a Call node is ``op.<something>(...)``."""
+ return (
+ isinstance(node.func, ast.Attribute)
+ and isinstance(node.func.value, ast.Name)
+ and node.func.value.id == "op"
+ )
+
+
+def _find_disable_sqlite_fkeys_line(func_node: ast.FunctionDef) -> int | None:
+ """Find the line number of the first ``with disable_sqlite_fkeys(op):`` in a function."""
+ # Collect all matches then take min() — ast.walk BFS order does not guarantee
+ # line-number order when guards appear at different nesting depths.
+ found: list[int] = []
+ for node in ast.walk(func_node):
+ if not isinstance(node, ast.With):
+ continue
+ for item in node.items:
+ ctx = item.context_expr
+ if isinstance(ctx, ast.Call):
+ func = ctx.func
+ if (isinstance(func, ast.Name) and func.id == "disable_sqlite_fkeys") or (
+ isinstance(func, ast.Attribute) and func.attr == "disable_sqlite_fkeys"
+ ):
+ found.append(node.lineno)
+ return min(found) if found else None
+
+
+def _has_offline_mode_check(func_node: ast.FunctionDef) -> bool:
+ """Check if function contains ``context.is_offline_mode()`` call."""
+ for node in ast.walk(func_node):
+ if isinstance(node, ast.Call):
+ func = node.func
+ if isinstance(func, ast.Attribute) and func.attr == "is_offline_mode":
+ return True
+ return False
+
+
+def _get_upgrade_downgrade_functions(tree: ast.Module) -> list[ast.FunctionDef]:
+ """Get ``upgrade()`` and ``downgrade()`` function definitions from module."""
+ return [
+ node
+ for node in ast.iter_child_nodes(tree)
+ if isinstance(node, ast.FunctionDef) and node.name in ("upgrade", "downgrade")
+ ]
+
+
+def _line_has_noqa(lines: list[str], lineno: int, code: str) -> bool:
+ """Check if a source line has a noqa suppression for the given code."""
+ if lineno < 1 or lineno > len(lines):
+ return False
+ return code in _get_noqa_codes(lines[lineno - 1])
+
+
+# ---------------------------------------------------------------------------
+# Check functions
+# ---------------------------------------------------------------------------
+
+
+def check_mig001(filepath: Path) -> list[str]:
+ """MIG001: DML before ``disable_sqlite_fkeys``."""
+ source = filepath.read_text()
+ lines = source.splitlines()
+ tree = ast.parse(source, filename=str(filepath))
+ errors: list[str] = []
+
+ for func in _get_upgrade_downgrade_functions(tree):
+ guard_line = _find_disable_sqlite_fkeys_line(func)
+ if guard_line is None:
+ continue # MIG001 only fires when disable_sqlite_fkeys IS present
+
+ for node in ast.walk(func):
+ if not isinstance(node, ast.Call):
+ continue
+ if node.lineno >= guard_line:
+ continue
+ if _is_op_execute_with_dml(node) and not _line_has_noqa(lines, node.lineno, "MIG001"):
+ errors.append(
+ f"MIG001 {func.name}(): op.execute() with DML at line {node.lineno} "
+ f"before disable_sqlite_fkeys at line {guard_line}"
+ )
+
+ return errors
+
+
+def check_mig002(filepath: Path) -> list[str]:
+ """MIG002: DDL before ``disable_sqlite_fkeys``."""
+ source = filepath.read_text()
+ lines = source.splitlines()
+ tree = ast.parse(source, filename=str(filepath))
+ errors: list[str] = []
+
+ for func in _get_upgrade_downgrade_functions(tree):
+ guard_line = _find_disable_sqlite_fkeys_line(func)
+ if guard_line is None:
+ continue # MIG002 only fires when disable_sqlite_fkeys IS present
+
+ for node in ast.walk(func):
+ if not isinstance(node, ast.Call):
+ continue
+ if node.lineno >= guard_line:
+ continue
+ # Skip DML calls — covered by MIG001
+ if _is_op_execute_with_dml(node):
+ continue
+ if _is_op_call(node) and not _line_has_noqa(lines, node.lineno, "MIG002"):
+ attr = node.func.attr if isinstance(node.func, ast.Attribute) else "?"
+ errors.append(
+ f"MIG002 {func.name}(): op.{attr}() at line {node.lineno} "
+ f"before disable_sqlite_fkeys at line {guard_line}"
+ )
+
+ return errors
+
+
+def check_mig003(filepath: Path) -> list[str]:
+ """MIG003: DML without offline-mode guard."""
+ source = filepath.read_text()
+ lines = source.splitlines()
+ tree = ast.parse(source, filename=str(filepath))
+ errors: list[str] = []
+
+ for func in _get_upgrade_downgrade_functions(tree):
+ if _has_offline_mode_check(func):
+ continue # function has the guard
+
+ for node in ast.walk(func):
+ if not isinstance(node, ast.Call):
+ continue
+ if _is_op_execute_with_dml(node) and not _line_has_noqa(lines, node.lineno, "MIG003"):
+ errors.append(
+ f"MIG003 {func.name}(): op.execute() with DML at line {node.lineno} "
+ f"without context.is_offline_mode() guard"
+ )
+
+ return errors
+
+
+# ---------------------------------------------------------------------------
+# CLI entry point
+# ---------------------------------------------------------------------------
+
+
+def main() -> int:
+ """Check migration files for anti-patterns. Exit 0 if clean, 1 if violations found."""
+ check_fns = [check_mig001, check_mig002, check_mig003]
Review Comment:
Each of this rule, re-reads and re-parses the file. How about we take a parsed-tree-and-source structure as input, parse once per file here? This will avoid future drift temptation for the check functions and also make the tests faster
--
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] Add static analysis tests for DB migration anti-patterns [airflow]
Posted by "Dev-iL (via GitHub)" <gi...@apache.org>.
Dev-iL commented on code in PR #64972:
URL: https://github.com/apache/airflow/pull/64972#discussion_r3160164347
##########
scripts/ci/prek/check_migration_patterns.py:
##########
@@ -0,0 +1,383 @@
+#!/usr/bin/env python
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# /// script
+# requires-python = ">=3.10"
+# dependencies = [
+# "rich>=13.6.0",
+# ]
+# ///
+"""
+Static analysis checks for Alembic migration anti-patterns.
+
+These checks use AST analysis to detect common migration mistakes that can cause
+silent failures, particularly on SQLite where ``PRAGMA foreign_keys`` has specific
+requirements about transaction state.
+
+Rules
+=====
+
+MIG001 -- DML before ``disable_sqlite_fkeys``
+----------------------------------------------
+
+**What it does:**
+Detects ``op.execute()`` calls containing DML keywords (UPDATE, INSERT, DELETE) that
+appear before a ``with disable_sqlite_fkeys(op):`` block in the same function.
+
+**Why is this bad:**
+On SQLite, any DML statement triggers *autobegin*, starting a transaction. Once a
+transaction is active, ``PRAGMA foreign_keys=off`` (issued by ``disable_sqlite_fkeys``)
+becomes a no-op. This means foreign key checks remain enabled, and
+``batch_alter_table`` (which recreates tables) may fail with foreign key constraint
+violations.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL") # triggers autobegin
+ with disable_sqlite_fkeys(op): # PRAGMA is now a no-op!
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ with disable_sqlite_fkeys(op):
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+
+MIG002 -- DDL before ``disable_sqlite_fkeys``
+----------------------------------------------
+
+**What it does:**
+Detects any Alembic ``op.*`` call (DDL operations like ``op.add_column``,
+``op.drop_column``, ``op.create_table``, ``op.drop_table``, ``op.create_index``,
+``op.batch_alter_table``, etc.) that appears before a
+``with disable_sqlite_fkeys(op):`` block in the same function. Excludes DML calls
+covered by MIG001.
+
+**Why is this bad:**
+DDL operations also trigger *autobegin* on SQLite, making the subsequent
+``PRAGMA foreign_keys=off`` a no-op, exactly like DML.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.add_column("dag_run", sa.Column("created_at", ...)) # triggers autobegin
+ with disable_sqlite_fkeys(op): # PRAGMA is now a no-op!
+ with op.batch_alter_table("backfill") as batch_op:
+ batch_op.alter_column("col", nullable=True)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ with disable_sqlite_fkeys(op):
+ op.add_column("dag_run", sa.Column("created_at", ...))
+ with op.batch_alter_table("backfill") as batch_op:
+ batch_op.alter_column("col", nullable=True)
+
+
+MIG003 -- DML without offline-mode guard
+------------------------------------------
+
+**What it does:**
+Detects ``op.execute()`` calls containing DML keywords (UPDATE, INSERT, DELETE) in
+``upgrade()`` or ``downgrade()`` functions that do not contain a
+``context.is_offline_mode()`` check anywhere in the same function.
+
+**Why is this bad:**
+Alembic's offline mode generates SQL scripts instead of executing against a live
+database. DML statements like ``UPDATE ... WHERE col IS NULL`` depend on actual data
+and produce no useful output in offline mode. Without an ``is_offline_mode()`` guard,
+the generated script includes DML that may fail or silently do nothing when applied
+later.
+
+**Example (bad):**
+
+.. code-block:: python
+
+ def upgrade():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+**Use instead:**
+
+.. code-block:: python
+
+ def upgrade():
+ if not context.is_offline_mode():
+ op.execute("UPDATE dag SET col = '' WHERE col IS NULL")
+ with op.batch_alter_table("dag") as batch_op:
+ batch_op.alter_column("col", nullable=False)
+
+
+Known Limitation -- Future MIG004 Candidate
+---------------------------------------------
+
+Migrations that perform DML and use ``batch_alter_table`` but never call
+``disable_sqlite_fkeys`` at all are NOT detected by MIG001/MIG002. These migrations
+may silently break SQLite foreign key handling. Detecting this pattern requires
+understanding whether foreign key relationships exist on the affected tables, which
+is beyond AST-level analysis. This is tracked as a future MIG004 candidate.
+
+
+Suppression
+===========
+
+Add ``# noqa: MIG0XX`` to a line to suppress the corresponding check for that line.
+Multiple codes can be comma-separated: ``# noqa: MIG001, MIG003``.
+
+Include a brief reason after ``--``::
+
+ # noqa: MIG003 -- simple UPDATE safe in offline mode
+"""
+
+from __future__ import annotations
+
+import ast
+import re
+import sys
+from pathlib import Path
+
+from rich.console import Console
+
+console = Console(color_system="standard", width=200)
+
+DML_KEYWORDS = ("UPDATE", "INSERT", "DELETE")
+
+
+def _get_noqa_codes(line: str) -> set[str]:
+ """Extract noqa codes from a source line."""
+ match = re.search(r"#\s*noqa:\s*([\w,\s]+)", line)
+ if match:
+ return {code.strip() for code in match.group(1).split(",")}
+ return set()
+
+
+def _is_dml_string(node: ast.expr) -> bool:
+ """Check if an AST expression is a string literal starting with a DML keyword."""
+ if isinstance(node, ast.Constant) and isinstance(node.value, str):
+ parts = node.value.strip().upper().split(None, 1)
+ return len(parts) > 1 and parts[0] in DML_KEYWORDS
+ # f-strings: check the first string fragment
+ if isinstance(node, ast.JoinedStr):
+ for value in node.values:
+ if isinstance(value, ast.Constant) and isinstance(value.value, str):
+ parts = value.value.strip().upper().split(None, 1)
+ if len(parts) > 1 and parts[0] in DML_KEYWORDS:
+ return True
+ break # only the leading fragment matters
+ return False
+
+
+def _is_op_execute_with_dml(node: ast.Call) -> bool:
+ """Check if a Call node is ``op.execute(<DML string>)``."""
+ if not (
+ isinstance(node.func, ast.Attribute)
+ and isinstance(node.func.value, ast.Name)
+ and node.func.value.id == "op"
+ and node.func.attr == "execute"
+ ):
+ return False
+ if not node.args:
+ return False
+ arg = node.args[0]
+ # Direct string: op.execute("UPDATE ...")
+ if _is_dml_string(arg):
+ return True
+ # Wrapped: op.execute(text("UPDATE ...")) or op.execute(sa.text("UPDATE ..."))
+ if isinstance(arg, ast.Call) and arg.args:
+ return _is_dml_string(arg.args[0])
+ return False
+
+
+def _is_op_call(node: ast.Call) -> bool:
+ """Check if a Call node is ``op.<something>(...)``."""
+ return (
+ isinstance(node.func, ast.Attribute)
+ and isinstance(node.func.value, ast.Name)
+ and node.func.value.id == "op"
+ )
+
+
+def _find_disable_sqlite_fkeys_line(func_node: ast.FunctionDef) -> int | None:
+ """Find the line number of the first ``with disable_sqlite_fkeys(op):`` in a function."""
+ # Collect all matches then take min() — ast.walk BFS order does not guarantee
+ # line-number order when guards appear at different nesting depths.
+ found: list[int] = []
+ for node in ast.walk(func_node):
+ if not isinstance(node, ast.With):
+ continue
+ for item in node.items:
+ ctx = item.context_expr
+ if isinstance(ctx, ast.Call):
+ func = ctx.func
+ if (isinstance(func, ast.Name) and func.id == "disable_sqlite_fkeys") or (
+ isinstance(func, ast.Attribute) and func.attr == "disable_sqlite_fkeys"
+ ):
+ found.append(node.lineno)
+ return min(found) if found else None
+
+
+def _has_offline_mode_check(func_node: ast.FunctionDef) -> bool:
+ """Check if function contains ``context.is_offline_mode()`` call."""
+ for node in ast.walk(func_node):
+ if isinstance(node, ast.Call):
+ func = node.func
+ if isinstance(func, ast.Attribute) and func.attr == "is_offline_mode":
+ return True
+ return False
+
+
+def _get_upgrade_downgrade_functions(tree: ast.Module) -> list[ast.FunctionDef]:
+ """Get ``upgrade()`` and ``downgrade()`` function definitions from module."""
+ return [
+ node
+ for node in ast.iter_child_nodes(tree)
+ if isinstance(node, ast.FunctionDef) and node.name in ("upgrade", "downgrade")
+ ]
+
+
+def _line_has_noqa(lines: list[str], lineno: int, code: str) -> bool:
+ """Check if a source line has a noqa suppression for the given code."""
+ if lineno < 1 or lineno > len(lines):
+ return False
+ return code in _get_noqa_codes(lines[lineno - 1])
+
+
+# ---------------------------------------------------------------------------
+# Check functions
+# ---------------------------------------------------------------------------
+
+
+def check_mig001(filepath: Path) -> list[str]:
+ """MIG001: DML before ``disable_sqlite_fkeys``."""
+ source = filepath.read_text()
+ lines = source.splitlines()
+ tree = ast.parse(source, filename=str(filepath))
+ errors: list[str] = []
+
+ for func in _get_upgrade_downgrade_functions(tree):
+ guard_line = _find_disable_sqlite_fkeys_line(func)
+ if guard_line is None:
+ continue # MIG001 only fires when disable_sqlite_fkeys IS present
+
+ for node in ast.walk(func):
+ if not isinstance(node, ast.Call):
+ continue
+ if node.lineno >= guard_line:
+ continue
+ if _is_op_execute_with_dml(node) and not _line_has_noqa(lines, node.lineno, "MIG001"):
+ errors.append(
+ f"MIG001 {func.name}(): op.execute() with DML at line {node.lineno} "
+ f"before disable_sqlite_fkeys at line {guard_line}"
+ )
+
+ return errors
+
+
+def check_mig002(filepath: Path) -> list[str]:
+ """MIG002: DDL before ``disable_sqlite_fkeys``."""
+ source = filepath.read_text()
+ lines = source.splitlines()
+ tree = ast.parse(source, filename=str(filepath))
+ errors: list[str] = []
+
+ for func in _get_upgrade_downgrade_functions(tree):
+ guard_line = _find_disable_sqlite_fkeys_line(func)
+ if guard_line is None:
+ continue # MIG002 only fires when disable_sqlite_fkeys IS present
+
+ for node in ast.walk(func):
+ if not isinstance(node, ast.Call):
+ continue
+ if node.lineno >= guard_line:
+ continue
+ # Skip DML calls — covered by MIG001
+ if _is_op_execute_with_dml(node):
+ continue
+ if _is_op_call(node) and not _line_has_noqa(lines, node.lineno, "MIG002"):
+ attr = node.func.attr if isinstance(node.func, ast.Attribute) else "?"
+ errors.append(
+ f"MIG002 {func.name}(): op.{attr}() at line {node.lineno} "
+ f"before disable_sqlite_fkeys at line {guard_line}"
+ )
+
+ return errors
+
+
+def check_mig003(filepath: Path) -> list[str]:
+ """MIG003: DML without offline-mode guard."""
+ source = filepath.read_text()
+ lines = source.splitlines()
+ tree = ast.parse(source, filename=str(filepath))
+ errors: list[str] = []
+
+ for func in _get_upgrade_downgrade_functions(tree):
+ if _has_offline_mode_check(func):
+ continue # function has the guard
+
+ for node in ast.walk(func):
+ if not isinstance(node, ast.Call):
+ continue
+ if _is_op_execute_with_dml(node) and not _line_has_noqa(lines, node.lineno, "MIG003"):
+ errors.append(
+ f"MIG003 {func.name}(): op.execute() with DML at line {node.lineno} "
+ f"without context.is_offline_mode() guard"
+ )
+
+ return errors
+
+
+# ---------------------------------------------------------------------------
+# CLI entry point
+# ---------------------------------------------------------------------------
+
+
+def main() -> int:
+ """Check migration files for anti-patterns. Exit 0 if clean, 1 if violations found."""
+ check_fns = [check_mig001, check_mig002, check_mig003]
Review Comment:
I agree - will refactor once my quota resets
--
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] Add static analysis tests for DB migration anti-patterns [airflow]
Posted by "Dev-iL (via GitHub)" <gi...@apache.org>.
Dev-iL commented on PR #64972:
URL: https://github.com/apache/airflow/pull/64972#issuecomment-4342781161
> `# noqa: MIG003` suppressions across a lot of file has a practical risk. The risk is that legit future violations hide in these suppressions. Consider whether `MIG003` could distinguish "DML in a function that already runs DDL on the same table" (likely safe in online mode) from "DML touching unrelated state" — that would shrink the suppression list materially. Not a blocker
I agree the suppression surface is not ideal. I'll take a look at whether we can safely reduce it without adding fragile SQL/table inference.
--
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