You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "dabla (via GitHub)" <gi...@apache.org> on 2025/09/02 12:26:42 UTC
[PR] Avoid using extra_dejson method on connection in KiotaRequestAdapterHook to avoid AsyncToSync RuntimeError [airflow]
dabla opened a new pull request, #55179:
URL: https://github.com/apache/airflow/pull/55179
<!--
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.
-->
<!--
Thank you for contributing! 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 committers for the review!
In case of an existing issue, reference it using one of the following:
closes: [#54350](https://github.com/apache/airflow/issues/54350)
related: [#54350](https://github.com/apache/airflow/issues/54350)
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
This PR fixes the "You cannot use AsyncToSync in the same thread as an async event loop - just await the async function directly." RuntimeError. The error originates from the[ Connection.extra_dejson](https://github.com/apache/airflow/blob/main/task-sdk/src/airflow/sdk/definitions/connection.py#L217) property which now masks the secret.
So to temporarely avoid the issue, I just use:
`json.loads(connection.extra)`
instead of:
`connection.extra_dejson`
Beside that, this PR will also check if the BaseHook has the new async aget_connection method, if it does it will use it as is, if it doesn't (which will be the case when azure provider is used with older airflow versions), then it will use the sync_to_async fallbaclk mechanism.
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [airflow-core/newsfragments](https://github.com/apache/airflow/tree/main/airflow-core/newsfragments).
--
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] Avoid using extra_dejson method on connection in KiotaRequestAdapterHook to avoid AsyncToSync RuntimeError [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #55179:
URL: https://github.com/apache/airflow/pull/55179#issuecomment-3292768708
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] Avoid using extra_dejson method on connection in KiotaRequestAdapterHook to avoid AsyncToSync RuntimeError [airflow]
Posted by "dabla (via GitHub)" <gi...@apache.org>.
dabla commented on code in PR #55179:
URL: https://github.com/apache/airflow/pull/55179#discussion_r2333499121
##########
providers/microsoft/azure/src/airflow/providers/microsoft/azure/hooks/msgraph.py:
##########
@@ -250,7 +250,9 @@ def to_msal_proxies(self, authority: str | None, proxies: dict) -> dict | None:
def _build_request_adapter(self, connection) -> tuple[str, RequestAdapter]:
client_id = connection.login
client_secret = connection.password
- config = connection.extra_dejson if connection.extra else {}
+ # TODO: do not use connection.extra_dejson until it's fixed in Airflow otherwise expect:
Review Comment:
There is an [issue](https://github.com/apache/airflow/issues/54350) open to it, which was fixed but then failed again due to the issue with the connection.extra_dejson, should a new issue be opened for that specific case?
--
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] Avoid using extra_dejson method on connection in KiotaRequestAdapterHook to avoid AsyncToSync RuntimeError [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on code in PR #55179:
URL: https://github.com/apache/airflow/pull/55179#discussion_r2347505741
##########
providers/microsoft/azure/src/airflow/providers/microsoft/azure/hooks/msgraph.py:
##########
@@ -250,7 +250,9 @@ def to_msal_proxies(self, authority: str | None, proxies: dict) -> dict | None:
def _build_request_adapter(self, connection) -> tuple[str, RequestAdapter]:
client_id = connection.login
client_secret = connection.password
- config = connection.extra_dejson if connection.extra else {}
+ # TODO: do not use connection.extra_dejson until it's fixed in Airflow otherwise expect:
Review Comment:
```suggestion
# TODO (#54350): do not use connection.extra_dejson until it's fixed in Airflow otherwise expect:
```
##########
providers/microsoft/azure/tests/unit/microsoft/azure/operators/test_powerbi.py:
##########
@@ -29,21 +29,17 @@
)
Review Comment:
Some tests in this one are failing. Can you fix it, please?
https://github.com/apache/airflow/actions/runs/17713756636/job/50336128458?pr=55179
--
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] Avoid using extra_dejson method on connection in KiotaRequestAdapterHook to avoid AsyncToSync RuntimeError [airflow]
Posted by "dabla (via GitHub)" <gi...@apache.org>.
dabla commented on PR #55179:
URL: https://github.com/apache/airflow/pull/55179#issuecomment-3290851903
All remarks have been fixed, issue number has been added in TODO, all tests have been cleaned and fixed even for PowerBI.
--
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] Avoid using extra_dejson method on connection in KiotaRequestAdapterHook to avoid AsyncToSync RuntimeError [airflow]
Posted by "violetnspct (via GitHub)" <gi...@apache.org>.
violetnspct commented on PR #55179:
URL: https://github.com/apache/airflow/pull/55179#issuecomment-3246742881
Should you be adding a unit test to cover the following scenario?
- Test behavior when connection.extra contains Unicode characters
--
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] Avoid using extra_dejson method on connection in KiotaRequestAdapterHook to avoid AsyncToSync RuntimeError [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #55179:
URL: https://github.com/apache/airflow/pull/55179#discussion_r2328649979
##########
providers/microsoft/azure/src/airflow/providers/microsoft/azure/hooks/msgraph.py:
##########
@@ -250,7 +250,9 @@ def to_msal_proxies(self, authority: str | None, proxies: dict) -> dict | None:
def _build_request_adapter(self, connection) -> tuple[str, RequestAdapter]:
client_id = connection.login
client_secret = connection.password
- config = connection.extra_dejson if connection.extra else {}
+ # TODO: do not use connection.extra_dejson until it's fixed in Airflow otherwise expect:
Review Comment:
Yeah. Link to opened issue or PR about it would be great.
--
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] Avoid using extra_dejson method on connection in KiotaRequestAdapterHook to avoid AsyncToSync RuntimeError [airflow]
Posted by "dabla (via GitHub)" <gi...@apache.org>.
dabla commented on PR #55179:
URL: https://github.com/apache/airflow/pull/55179#issuecomment-3271570128
> One nit.
What need's to be done to have this one merged?
--
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] Avoid using extra_dejson method on connection in KiotaRequestAdapterHook to avoid AsyncToSync RuntimeError [airflow]
Posted by "dabla (via GitHub)" <gi...@apache.org>.
dabla commented on code in PR #55179:
URL: https://github.com/apache/airflow/pull/55179#discussion_r2344060719
##########
providers/microsoft/azure/src/airflow/providers/microsoft/azure/hooks/msgraph.py:
##########
@@ -250,7 +250,9 @@ def to_msal_proxies(self, authority: str | None, proxies: dict) -> dict | None:
def _build_request_adapter(self, connection) -> tuple[str, RequestAdapter]:
client_id = connection.login
client_secret = connection.password
- config = connection.extra_dejson if connection.extra else {}
+ # TODO: do not use connection.extra_dejson until it's fixed in Airflow otherwise expect:
Review Comment:
Well the issue I linked to refered to the SyncToAsync issue we had with MSGraphAsyncOperator, normally the issue was fixed in Airflow 3.0.4 and 3.0.5, but once we started testing this previous fix we saw that the problem rose again. This time it came from a different place, namely the Connection.extra_dejson property. It seems there a change was made in 3.0.6 which calls the mask_secret method, which is the cause of the issue, as the method expects to be run in an async context, but isn't the case here so it throws the exception.
--
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] Avoid using extra_dejson method on connection in KiotaRequestAdapterHook to avoid AsyncToSync RuntimeError [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #55179:
URL: https://github.com/apache/airflow/pull/55179#discussion_r2347088639
##########
providers/microsoft/azure/tests/unit/microsoft/azure/hooks/test_msgraph.py:
##########
@@ -62,18 +63,8 @@
AzureIdentityAccessTokenProvider,
)
-try:
- import importlib.util
- if not importlib.util.find_spec("airflow.sdk.bases.hook"):
- raise ImportError
-
- BASEHOOK_PATCH_PATH = "airflow.sdk.bases.hook.BaseHook"
-except ImportError:
- BASEHOOK_PATCH_PATH = "airflow.hooks.base.BaseHook"
-
-
-class TestKiotaRequestAdapterHook:
+class TestKiotaRequestAdapterHook(Base):
Review Comment:
Can you explain this one?
Why Airflow unit test needs to inherit from upstream library base class? I don't think we ever done that before
--
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] Avoid using extra_dejson method on connection in KiotaRequestAdapterHook to avoid AsyncToSync RuntimeError [airflow]
Posted by "dabla (via GitHub)" <gi...@apache.org>.
dabla commented on code in PR #55179:
URL: https://github.com/apache/airflow/pull/55179#discussion_r2347144540
##########
providers/microsoft/azure/tests/unit/microsoft/azure/hooks/test_msgraph.py:
##########
@@ -62,18 +63,8 @@
AzureIdentityAccessTokenProvider,
)
-try:
- import importlib.util
- if not importlib.util.find_spec("airflow.sdk.bases.hook"):
- raise ImportError
-
- BASEHOOK_PATCH_PATH = "airflow.sdk.bases.hook.BaseHook"
-except ImportError:
- BASEHOOK_PATCH_PATH = "airflow.hooks.base.BaseHook"
-
-
-class TestKiotaRequestAdapterHook:
+class TestKiotaRequestAdapterHook(Base):
Review Comment:
This is indeed wrong, will fix 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] Avoid using extra_dejson method on connection in KiotaRequestAdapterHook to avoid AsyncToSync RuntimeError [airflow]
Posted by "dabla (via GitHub)" <gi...@apache.org>.
dabla commented on code in PR #55179:
URL: https://github.com/apache/airflow/pull/55179#discussion_r2347259029
##########
providers/microsoft/azure/tests/unit/microsoft/azure/hooks/test_msgraph.py:
##########
@@ -62,18 +63,8 @@
AzureIdentityAccessTokenProvider,
)
-try:
- import importlib.util
- if not importlib.util.find_spec("airflow.sdk.bases.hook"):
- raise ImportError
-
- BASEHOOK_PATCH_PATH = "airflow.sdk.bases.hook.BaseHook"
-except ImportError:
- BASEHOOK_PATCH_PATH = "airflow.hooks.base.BaseHook"
-
-
-class TestKiotaRequestAdapterHook:
+class TestKiotaRequestAdapterHook(Base):
Review Comment:
It was already the case for the other msgraph related tests but I cleaned it anyway.
--
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] Avoid using extra_dejson method on connection in KiotaRequestAdapterHook to avoid AsyncToSync RuntimeError [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on code in PR #55179:
URL: https://github.com/apache/airflow/pull/55179#discussion_r2319398113
##########
providers/microsoft/azure/src/airflow/providers/microsoft/azure/hooks/msgraph.py:
##########
@@ -250,7 +250,9 @@ def to_msal_proxies(self, authority: str | None, proxies: dict) -> dict | None:
def _build_request_adapter(self, connection) -> tuple[str, RequestAdapter]:
client_id = connection.login
client_secret = connection.password
- config = connection.extra_dejson if connection.extra else {}
+ # TODO: do not use connection.extra_dejson until it's fixed in Airflow otherwise expect:
Review Comment:
When are we planning to make the fix? Do we have any issues to follow u on?
--
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] Avoid using extra_dejson method on connection in KiotaRequestAdapterHook to avoid AsyncToSync RuntimeError [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #55179:
URL: https://github.com/apache/airflow/pull/55179#discussion_r2339710952
##########
providers/microsoft/azure/src/airflow/providers/microsoft/azure/hooks/msgraph.py:
##########
@@ -250,7 +250,9 @@ def to_msal_proxies(self, authority: str | None, proxies: dict) -> dict | None:
def _build_request_adapter(self, connection) -> tuple[str, RequestAdapter]:
client_id = connection.login
client_secret = connection.password
- config = connection.extra_dejson if connection.extra else {}
+ # TODO: do not use connection.extra_dejson until it's fixed in Airflow otherwise expect:
Review Comment:
I think the goal is not to have todos.
The TODO is not so clear to me. Please explain what specifically is missing. The issue you linked to is about msgraph while you are refering to a problem in airflow core.
--
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] Avoid using extra_dejson method on connection in KiotaRequestAdapterHook to avoid AsyncToSync RuntimeError [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #55179:
URL: https://github.com/apache/airflow/pull/55179
--
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