Copilot commented on code in PR #61917:
URL: https://github.com/apache/airflow/pull/61917#discussion_r3066844273
##########
airflow-core/src/airflow/settings.py:
##########
@@ -681,6 +702,16 @@ def __getattr__(name: str):
from airflow.exceptions import RemovedInAirflow4Warning
+ if name == "SQL_ALCHEMY_CONN":
+ warnings.warn(
+ "settings.SQL_ALCHEMY_CONN has been replaced by
get_sql_alchemy_conn(). This shim is just for compatibility. "
+ "Please upgrade your provider or integration.",
+ RemovedInAirflow4Warning,
+ stacklevel=2,
+ )
+ return get_sql_alchemy_conn()
+ if name == "SQL_ALCHEMY_CONN_ASYNC":
+ return _AirflowSettings.sql_alchemy_conn_async
Review Comment:
`settings.SQL_ALCHEMY_CONN_ASYNC` is still readable even after
`_AirflowSettings.block_orm_access` is enabled. If `configure_vars()` already
populated `sql_alchemy_conn_async`, task/DAG code could still retrieve the real
async connection string and connect to the metadatabase, undermining
`block_orm_access()`. Consider guarding this path the same as
`SQL_ALCHEMY_CONN` (raise when blocked and/or return a blocked URL + emit a
deprecation warning).
```suggestion
warnings.warn(
"settings.SQL_ALCHEMY_CONN_ASYNC has been replaced by
get_sql_alchemy_conn_async(). "
"This shim is just for compatibility. Please upgrade your
provider or integration.",
RemovedInAirflow4Warning,
stacklevel=2,
)
return get_sql_alchemy_conn_async()
```
##########
airflow-core/src/airflow/settings.py:
##########
@@ -498,7 +519,7 @@ def prepare_engine_args(disable_connection_pool=False,
pool_class=None):
default_args = {}
for dialect, default in DEFAULT_ENGINE_ARGS.items():
- if SQL_ALCHEMY_CONN.startswith(dialect):
+ if _AirflowSettings.sql_alchemy_conn.startswith(dialect):
default_args = default.copy()
break
Review Comment:
`prepare_engine_args()` relies on
`_AirflowSettings.sql_alchemy_conn.startswith(...)` without ensuring the
connection string is configured or checking the `block_orm_access` flag. Using
`get_sql_alchemy_conn()` (or a shared helper) here would give consistent error
behavior and avoid accidental DB access when blocked.
##########
airflow-core/tests/unit/utils/test_db.py:
##########
@@ -485,18 +489,21 @@ def test_mysql_variants_detected(self, mocker):
def test_none_sql_alchemy_conn(self, mocker):
"""Test behavior when SQL_ALCHEMY_CONN is None."""
- mocker.patch.object(settings, "SQL_ALCHEMY_CONN", None)
+ mocker.patch.object(settings._AirflowSettings, "sql_alchemy_conn",
None)
mock_dispose = mocker.patch.object(settings, "dispose_orm")
mock_configure = mocker.patch.object(settings, "configure_orm")
- with AutocommitEngineForMySQL():
- # Should be a no-op when connection string is None
- mock_dispose.assert_not_called()
- mock_configure.assert_not_called()
+ with pytest.raises(RuntimeError, match="connection string not
configured"):
+ with AutocommitEngineForMySQL():
+ pass
+
+ # Should be a no-op when connection string is None
Review Comment:
This test now expects a `RuntimeError` when the connection string is unset,
but the trailing comment still says it “should be a no-op when connection
string is None”. Consider rewording the comment to reflect the new behavior
(exception with no side effects), or adjust expectations if the intent is truly
a no-op.
```suggestion
# Failure should not trigger any ORM disposal or reconfiguration
side effects.
```
##########
task-sdk/src/airflow/sdk/execution_time/supervisor.py:
##########
@@ -319,8 +319,7 @@ def __getattr__(name: str):
settings.__getattr__ = __getattr__
- settings.SQL_ALCHEMY_CONN = conn
- settings.SQL_ALCHEMY_CONN_ASYNC = conn
+ settings.block_orm_access()
Review Comment:
`settings.block_orm_access()` currently only affects
`get_sql_alchemy_conn()`/`SQL_ALCHEMY_CONN`. Since `SQL_ALCHEMY_CONN_ASYNC` is
still returned by `airflow.settings.__getattr__` without checking the block
flag, task/DAG code could still obtain an async metadb URL if it was set before
blocking. Consider also blocking the async URL (e.g., update core to guard
`SQL_ALCHEMY_CONN_ASYNC` when blocked, and/or ensure the async value is
overwritten to the blocked URL here).
##########
airflow-core/src/airflow/settings.py:
##########
@@ -439,14 +460,14 @@ def configure_orm(disable_connection_pool=False,
pool_class=None):
engine_args = prepare_engine_args(disable_connection_pool, pool_class)
connect_args = _get_connect_args("sync")
- if SQL_ALCHEMY_CONN.startswith("sqlite"):
+ if _AirflowSettings.sql_alchemy_conn.startswith("sqlite"):
# FastAPI runs sync endpoints in a separate thread. SQLite does not
allow
# to use objects created in another threads by default. Allowing that
in test
# to so the `test` thread and the tested endpoints can use common
objects.
connect_args["check_same_thread"] = False
engine = create_metadata_engine(
- SQL_ALCHEMY_CONN,
+ _AirflowSettings.sql_alchemy_conn,
Review Comment:
`configure_orm()` uses `_AirflowSettings.sql_alchemy_conn` directly (e.g.
`.startswith(...)`) and never consults `get_sql_alchemy_conn()` /
`_AirflowSettings.block_orm_access`. This means ORM configuration can still
proceed even when ORM access is meant to be blocked, and it can also fail with
a confusing `'NoneType' object has no attribute 'startswith'` if
`configure_vars()` hasn’t been called yet. Suggestion: fetch `conn_str =
get_sql_alchemy_conn()` once at the start of `configure_orm()` and use that
throughout.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
