kaxil commented on code in PR #68305:
URL: https://github.com/apache/airflow/pull/68305#discussion_r3500653207
##########
providers/hashicorp/tests/unit/hashicorp/secrets/test_vault.py:
##########
@@ -82,7 +82,9 @@ def variable_result(self):
}
@mock.patch("airflow.providers.hashicorp._internal_client.vault_client.hvac")
- def test_get_connection(self, mock_hvac):
+ def test_get_conn_value(self, mock_hvac):
+ import json
Review Comment:
`import json` is repeated inside five test bodies (86, 201, 245, 622, 795)
and there's no module-level import. Hoist a single `import json` to the top
with the other stdlib imports and drop the in-method copies.
##########
providers/hashicorp/src/airflow/providers/hashicorp/secrets/vault.py:
##########
@@ -224,32 +224,30 @@ def _get_team_or_global_secret(self, base_path: str |
None, team_name: str | Non
return self._get_secret_with_base(path, key)
- # Make sure connection is imported this way for type checking, otherwise
when importing
- # the backend it will get a circular dependency and fail
- if TYPE_CHECKING:
- from airflow.models.connection import Connection
-
- def get_connection(self, conn_id: str, team_name: str | None = None) ->
Connection | None:
+ def get_conn_value(self, conn_id: str, team_name: str | None = None) ->
str | None:
"""
- Get connection from Vault as secret.
+ Retrieve a connection from Vault as a serialized string.
- Prioritize conn_uri if exists, if not fall back to normal Connection
creation.
+ Returns the ``conn_uri`` value verbatim when present, otherwise
serializes
+ the secret dict to JSON. The base-class ``get_connection``
deserializes the
+ returned string using the Connection class that the framework injected
for the
+ current execution context (ORM Connection on the server, SDK
Connection in
+ workers), which avoids triggering SQLAlchemy mapper initialization in
Review Comment:
This reads as unconditional, but the framework's per-process
Connection-class injection (`_get_connection_class`) only exists on 3.2+. On
the older releases this provider still supports (2.11 / 3.0 / 3.1) the base
`deserialize_connection` hard-imports the ORM `Connection`, so the mapper-init
avoidance doesn't hold there. Same 3.0/3.1 gap as the earlier thread; worth
qualifying the docstring (e.g. "on Airflow 3.2+") so it doesn't promise more
than the supported floor delivers.
--
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]
