kaxil commented on code in PR #68305:
URL: https://github.com/apache/airflow/pull/68305#discussion_r3416874037
##########
providers/hashicorp/src/airflow/providers/hashicorp/secrets/vault.py:
##########
@@ -237,19 +237,35 @@ def get_connection(self, conn_id: str, team_name: str |
None = None) -> Connecti
:return: A Connection object constructed from Vault data
"""
- # The Connection needs to be locally imported because otherwise we get
into cyclic import
- # problems when instantiating the backend during configuration
- from airflow.models.connection import Connection
+ # Use the compat SDK Connection (Pydantic in Airflow 3, SQLAlchemy in
Airflow 2) to avoid
+ # triggering SQLAlchemy mapper initialization for unrelated models
(e.g. DagModel) in
+ # task-execution subprocesses such as PythonVirtualenvOperator.
+ from airflow.providers.common.compat.sdk import Connection
Review Comment:
This swaps one hard-coded Connection class for another. The old code pinned
the ORM `Connection` (that's the worker bug), and this pins
`compat.sdk.Connection` instead, but the framework already hands each backend
the right class for the process it runs in: the ORM `Connection` on the server
(`airflow-core/src/airflow/configuration.py:764,772`) and the SDK `Connection`
in the worker (`task-sdk/src/airflow/sdk/configuration.py:293,301`), both
through `_set_connection_class()`. You can read it back here with
`self._get_connection_class()`.
Pinning the SDK class fixes the `PythonVirtualenvOperator` path but
regresses the server side. `airflow connections get <vault_conn>` goes through
`Connection.get_connection_from_secrets`, which returns whatever the backend
hands back, and the CLI then runs that through
`ConnectionDisplayMapper.full_details`, which reads `conn.id` and
`conn.is_encrypted` (`connection_command.py:79,88`). The attrs SDK Connection
has neither field, so that's an `AttributeError` on a command that worked
before this change.
If you build from `conn_class = self._get_connection_class()`, the worker
gets the SDK class (mapper init avoided, original bug fixed) and the server
keeps the ORM class (no regression). That's also what the shared base already
does in `_deserialize_connection_value`
(`airflow-core/src/airflow/_shared/secrets_backend/base.py:128`), with the same
`hasattr(conn_class, "from_uri")` check this code reinvents. akeyless
(`akeyless.py:177`) hard-codes the ORM class the same way and has the mirror of
the worker bug, so the injected class would be the consistent fix for both.
Minor: the comment just above calls the Airflow 3 SDK Connection "Pydantic",
it's attrs-based.
--
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]
