moomindani commented on code in PR #71840:
URL: https://github.com/apache/airflow/pull/71840#discussion_r3878233484
##########
providers/databricks/src/airflow/providers/databricks/hooks/databricks_base.py:
##########
@@ -196,6 +196,32 @@ def databricks_conn(self) -> Connection:
def get_conn(self) -> Connection:
return self.databricks_conn
+
+ async def adatabricks_conn(self):
+ if not hasattr(self, "_adatabricks_conn"):
+ self._adatabricks_conn = await
self.aget_connection(self.databricks_conn_id)
Review Comment:
`aget_connection` is only available from Airflow 3.1.0, so this line raises
`AttributeError` on every version below that — including 3.0.x, the one window
where #71525 actually reproduces (verified matrix in
https://github.com/apache/airflow/issues/71525#issuecomment-5302280351), and
2.11, which this provider still declares.
`apache-airflow-providers-common-compat` is already a dependency and ships
the compatible form:
```python
from airflow.providers.common.compat.connection import get_async_connection
async def adatabricks_conn(self) -> Connection:
if not hasattr(self, "_adatabricks_conn"):
self._adatabricks_conn = await
get_async_connection(self.databricks_conn_id, hook=self)
return self._adatabricks_conn
```
It uses `aget_connection` where it exists and
`sync_to_async(hook.get_connection)` otherwise, which is the mechanism that
works on 3.0.6.
Two smaller points on the same method: the return type is unannotated while
every other member of this class is typed, and the `hasattr` guard is not
concurrency-safe — two coroutines awaiting this at once both fetch. Harmless
today, but a plain `asyncio.Lock` or resolving the connection once in
`__aenter__` avoids it.
##########
providers/databricks/src/airflow/providers/databricks/hooks/databricks_base.py:
##########
@@ -257,6 +283,11 @@ def _parse_host(host: str) -> str:
# In this case, host = xx.cloud.databricks.com
return host
+ async def _a_get_connection_attr(self, attr_name: str):
+ if not (attr := getattr(await self.adatabricks_conn(), attr_name)):
+ raise AirflowException(f"{attr_name} shouldn't be empty")
Review Comment:
Two problems here.
`raise AirflowException(...)` is a new direct usage, which the
`check-no-new-airflow-exceptions` hook rejects for `providers/` — the project
is actively reducing these. The same applies to `_a_get_required_client_id` at
`:941`.
More importantly this diverges from the sync twin it mirrors:
`_get_connection_attr` raises `ValueError("`{attr}` must be present in
Connection")`. So the same misconfiguration now produces a different exception
type *and* a different message depending on whether the caller is sync or
async, and anything catching `ValueError` around the sync path silently misses
the async one. Raising the same exception type with the same message keeps the
two paths interchangeable, which is the whole point of the `_a_*` mirroring in
this file.
The divergence is not hypothetical: the sibling method at `:941` has the
same problem and it already breaks a test.
`test_a_get_federated_token_missing_client_id` fails on this head because the
new async message wraps the field in backticks while that test's `match=`
expects the sync wording (locally: 1 failed, 127 passed).
##########
providers/databricks/tests/unit/databricks/hooks/test_databricks_base.py:
##########
@@ -635,10 +637,7 @@ def test_get_token_not_configured_raises(self, mock_conn):
hook._get_token(raise_error=True)
@pytest.mark.asyncio
- @mock.patch(
-
"airflow.providers.databricks.hooks.databricks_base.BaseDatabricksHook.databricks_conn",
- new_callable=mock.PropertyMock,
- )
+
@mock.patch("airflow.providers.databricks.hooks.databricks_base.BaseDatabricksHook.adatabricks_conn")
Review Comment:
Patching `adatabricks_conn` at class level means the new method's own body
never runs, and that is why the `aget_connection` availability problem is
invisible to this suite: the test file has zero references to
`aget_connection`, and every async test either patches this method or presets
`hook._adatabricks_conn`.
A test that lets the real method run would have caught it — delete
`aget_connection` from the hook's MRO (simulating any Airflow below 3.1.0) and
drive one async path, asserting the connection is still resolved. With the
compat helper in place that passes; against this revision it fails with
`AttributeError`.
--
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]
