Skip to content

Conversation

@Sharashchandra
Copy link
Contributor

Bug introduced in: #49482
The infinite loop was caused by calling the property self._get_conn_params in the property self.account_identifier. The self.account_identifier was used in the get_oauth_token function, thus resulting in the infinite loop of calls to self._get_conn_params


@Sharashchandra
Copy link
Contributor Author

cc: @eladkal @gopidesupavan @bugraoz93

Copy link
Contributor

@bugraoz93 bugraoz93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Sharashchandra !

@eladkal
Copy link
Contributor

eladkal commented May 8, 2025

Tests are failing

FAILED providers/snowflake/tests/unit/snowflake/hooks/test_snowflake_sql_api.py::TestSnowflakeSqlApiHook::test_proper_parametrization_of_execute_query_api_request[test_hook_params0-select i from user_test order by i;-1-expected_payload0-expected_response0] - AttributeError: 'SnowflakeSqlApiHook' object has no attribute 'account_identifier'

@bugraoz93
Copy link
Contributor

Yes, most probably, the property is now a method and needs to be updated in the test accordingly

@Sharashchandra
Copy link
Contributor Author

Fixed that. Reverting the account_identifier property to remain as it was before because it's used in multiple places in the SnowflakeSqlApiHook
I'm using the account name to construct the url for snowflake oauth.
We don't need the region to be used in the url anymore as that's a legacy valid url
https://docs.snowflake.com/en/user-guide/organizations-connect#label-connecting-via-url

cc: @eladkal @bugraoz93

@eladkal
Copy link
Contributor

eladkal commented May 8, 2025

Tests are still failing
FAILED providers/snowflake/tests/unit/snowflake/hooks/test_snowflake.py::TestPytestSnowflakeHook::test_get_oauth_token - AssertionError: expected call not found.

@bugraoz93
Copy link
Contributor

Yes, because we remove the region now tests are expecting it

@Sharashchandra
Copy link
Contributor Author

Fixed the test cases, eveything is passing

@eladkal eladkal merged commit b90c746 into apache:main May 8, 2025
64 checks passed
eladkal pushed a commit to eladkal/airflow that referenced this pull request May 8, 2025
…oken from snowflake (apache#50344)

* fix(snowflake_oauth): Fix infinite recursive call of _get_conn_params while getting oauth token from snowflake

* fix(snowflake_oauth): Reverting account_identifier as a property, using account to construct oauth_token url

* fix(snowflake_oauth): Adding https:// for the snowflake oauth url

* fix(snowflake_oauth): Fix test case for oauth which was expecting region to be part of the url

* fix(snowflake_oauth): Fix failing text cases for snowflake api hook using region in the url for the oauth call
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers provider:snowflake Issues related to Snowflake provider

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants